Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ui-debug): add unsupported files handling #2260

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

hdinia
Copy link
Member

@hdinia hdinia commented Dec 2, 2024

ANT-2362 => handle unsupported files display
ANT-2529 => display output matrices as raw text

@hdinia hdinia requested a review from skamril December 2, 2024 14:53
@hdinia hdinia self-assigned this Dec 2, 2024
@hdinia hdinia force-pushed the feature/add-unsupported-files-handling branch from 1af012f to cc6b26c Compare December 4, 2024 16:25
@hdinia hdinia requested a review from skamril December 4, 2024 16:25
@hdinia hdinia force-pushed the feature/add-unsupported-files-handling branch 2 times, most recently from 5642122 to 13924e6 Compare December 5, 2024 08:22
@hdinia hdinia force-pushed the feature/add-unsupported-files-handling branch 2 times, most recently from 14875d9 to 4240536 Compare December 12, 2024 09:44
@hdinia hdinia requested a review from skamril December 12, 2024 09:47
@hdinia hdinia force-pushed the feature/add-unsupported-files-handling branch 3 times, most recently from a730983 to c86ac91 Compare December 13, 2024 08:51
////////////////////////////////////////////////////////////////

const URL_SCHEMES = {
MATRIX: ["matrix://", "matrixfile://"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, lower case are used for object keys, even for a constant

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this type of constant object where both the object name and its keys represent fixed values or schemes, it's more conventional to have both in UPPER_SNAKE_CASE, it's not a regular enum-like object see: https://google.github.io/styleguide/tsguide.html#naming

Copy link
Member

@skamril skamril Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your link said like me lol https://google.github.io/styleguide/tsguide.html#identifiers-constants

const URL_SCHEMES = {
  matrix: ...,
  ...
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know its confusing, the style guide example uses lowercase because they're dealing with actual unit names rather than symbolic constants.

The difference is that:

In URL_SCHEMES, the keys (MATRIX, JSON, FILE) are symbolic identifiers representing categories/types
In UNIT_SUFFIXES, the keys ('milliseconds', 'seconds') are the actual string names of time units

look at "Rules by identifier type" section:
says: "CONSTANT_CASE global constant values, including enum values. See Constants below."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should create a ticket and spend time to make our set of rules, especially for this kind of conventions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for a more general example:

  1. When keys are symbolic constants (use UPPER_SNAKE_CASE):
const ERROR_CODES = {
    NOT_FOUND: 404,
    SERVER_ERROR: 500,
    UNAUTHORIZED: 401
};

Here, NOT_FOUND is a symbolic name representing the concept of a "not found" error. The key itself is a symbol/identifier.

  1. When keys are actual VALUES/NAMES (use lowercase):
const MIME_TYPES = {
    'image/jpeg': '.jpg',
    'image/png': '.png',
    'text/html': '.html'
};

Here, 'image/jpeg' is the actual MIME type string that would be used in HTTP headers.

Think of it this way:

  • If you're using the key as a reference/symbol (like URL_SCHEMES.MATRIX), use UPPER_SNAKE_CASE
  • If you're using the key as a literal value (like MIME_TYPES['image/jpeg']), use lowercase

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright! I didn't know this subtlety.

// We filter to only allow extensions that can be properly displayed (.txt, .log, .csv, .tsv, .ini)
// Other extensions (like .RDS or .xlsx) are marked as unsupported since they can't be shown in the UI
return treeData.startsWith(URL_SCHEMES.FILE) &&
SUPPORTED_EXTENSIONS.some((ext) => treeData.endsWith(ext))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I had already said to you, use treeData.toLowerCase().endsWith(ext), because extension are case insensitive, .txt and .TXT are both valid, an user can import file with uppercase extension!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mybad, its fixed

* @returns Whether the path is in the output folder
*/
export function isOutputFolder(path: string): boolean {
return path.startsWith("output/");
Copy link
Member

@skamril skamril Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ouput" value has been forgotten:

return path.split("/")[0] === "output";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or rename isInOuputFolder

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's intended I'll rename it to better reflect the purpose

fontSize: theme.typography.body2.fontSize,
flex: 1,
}}
{...getSyntaxProps(parseContent(text, { filePath, fileType }))}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to parse in the usePromiseWithSnackbarError to make it once

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point their no possible error

Copy link
Member

@skamril skamril Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the error it's for the API call, but I said to had directly after the call to memoized the value.

() => getStudyData<string>(studyId, filePath).then(text => parseContent(text, { filePath, fileType })):
...
deps: [studyId, filePath, fileType],

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I will see that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed its good like this, done

@hdinia hdinia force-pushed the feature/add-unsupported-files-handling branch 2 times, most recently from 894f70a to deeaca0 Compare December 13, 2024 13:10
@hdinia hdinia force-pushed the feature/add-unsupported-files-handling branch from deeaca0 to 2c7eeac Compare December 13, 2024 13:20
@hdinia hdinia merged commit 88e3486 into dev Dec 13, 2024
10 checks passed
@hdinia hdinia deleted the feature/add-unsupported-files-handling branch December 13, 2024 13:24
@makdeuneuv makdeuneuv added this to the v3.0.0 milestone Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants