feat: nested file schema processor #1354
Conversation
himanshu-dixit
requested review from
plxity and
utkarsh-dixit
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| return { | ||
| title: key, | ||
| type, | ||
| description: property.description, | ||
| }; |
There was a problem hiding this comment.
When handling anyOf properties, the new code uses property.description instead of schema.description, which could show incorrect descriptions for file upload fields
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return { | |
| title: key, | |
| type, | |
| description: property.description, | |
| }; | |
| return { | |
| title: key, | |
| type, | |
| description: schema.description, | |
| }; |
| for (const [key, value] of Object.entries(result)) { | ||
| if (!key.endsWith(FILE_SUFFIX)) continue; | ||
|
|
||
| const isEmpty = value === "" || !value; |
There was a problem hiding this comment.
Consider extracting this empty value check into a reusable utility function to maintain consistency across the codebase:
const isEmptyValue = (value: unknown): boolean => value === "" || !value;This would make the code more maintainable and allow for easier updates to empty value logic if needed.
js/src/sdk/utils/processor/file.ts
Outdated
|
|
||
| const updateAnyOfProperties = (key: string, property: any) => { | ||
| let newKeyName = key; | ||
| const newAnyOf = property.anyOf.map((schema: any) => { |
There was a problem hiding this comment.
The property.anyOf array should be type-checked before mapping. Consider adding validation:
if (!Array.isArray(property.anyOf)) {
throw new Error(`Invalid schema: ${key} anyOf property must be an array`);
}This would prevent runtime errors if the schema is malformed.
js/src/sdk/utils/processor/file.ts
Outdated
| for (const [key, property] of Object.entries(newProperties)) { | ||
| if (!property.file_uploadable) continue; | ||
|
|
||
| const updateSingleProperty = (key: string, property: any) => { |
There was a problem hiding this comment.
The property parameter is typed as any. Consider creating a proper type definition:
interface FileSchemaProperty {
file_uploadable?: boolean;
type: string;
description?: string;
[key: string]: unknown;
}This would improve type safety and code maintainability.
|
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f751574 in 39 seconds
More details
- Looked at
28lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. js/src/sdk/models/actions.spec.ts:9
- Draft comment:
Fixed typo: variable 'connectedAccouns' renamed to 'connectedAccounts'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it only states that a typo was fixed. It does not provide any actionable feedback or suggestions for improvement.
2. js/src/sdk/models/actions.spec.ts:9
- Draft comment:
Fixed typo in variable declaration: 'connectedAccouns' → 'connectedAccounts'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, pointing out a typo that was fixed. It doesn't provide a suggestion or ask for confirmation about the change. According to the rules, purely informative comments should be removed.
3. js/src/sdk/models/actions.spec.ts:14
- Draft comment:
Corrected variable name typo during initialization: 'connectedAccouns' → 'connectedAccounts'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative as it only states that a typo was corrected. It does not provide any actionable feedback or suggestions for improvement.
4. js/src/sdk/models/actions.spec.ts:39
- Draft comment:
Fixed typo in method call: changed 'connectedAccouns.list' to 'connectedAccounts.list'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it only states that a typo was fixed in a method call. It does not provide any actionable feedback or suggestions for improvement.
5. js/src/sdk/models/actions.spec.ts:55
- Draft comment:
Typo: 'successfull' should be 'successful' to correctly spell the word. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_AYpohDtlXJKTjuHz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Code Review SummaryOverall AssessmentThe changes implement nested file schema processing with good separation of concerns and proper handling of edge cases. The code is generally well-structured but could benefit from some improvements in type safety and error handling. Strengths✅ Clean separation of concerns between single and nested property handling Areas for Improvement
Rating: 7/10Good implementation with room for improvement in type safety and error handling. The changes are safe to merge after addressing the suggested improvements in the review comments. |
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to bcbfd1b in 2 minutes and 46 seconds
More details
- Looked at
73lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. js/src/sdk/utils/processor/file.ts:41
- Draft comment:
Using '!value' may remove valid falsy values. Given that file uploads expect a non-empty string, ensure that this check doesn't inadvertently remove valid inputs (e.g., '0'). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
Since this is specifically for file uploads (checked by FILE_SUFFIX), the only valid values would be file data or file paths as strings. The value '0' or other falsy values would never be valid file upload data. The code is correctly checking for empty strings and null/undefined values. The comment's concern about falsy values is not relevant in this context.
I could be wrong about what constitutes valid file data - there might be edge cases I'm not considering. The code might handle file-like objects that could have different falsy representations.
Looking at the usage in getFileDataAfterUploadingToS3(), the value is explicitly cast to string and used as file data. No other types would make sense here.
The comment should be deleted as it raises concerns about falsy values that aren't relevant in a file upload context where only string data is valid.
2. js/src/sdk/utils/processor/file.ts:117
- Draft comment:
In updateAnyOfProperties, newKeyName is reassigned within the map for each file_uploadable schema. If multiple anyOf items are file_uploadable, the final key may be unexpected. Confirm if this behavior is intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment raises a valid point - if multiple schemas in anyOf are file_uploadable, the final newKeyName will be from the last file_uploadable schema processed. However, looking at convertFileSchemaProperty(), it always generates the same keyName for a given key (key + FILE_SUFFIX), so multiple file_uploadable schemas would generate the same keyName anyway. This makes the concern moot.
I could be wrong about the intended behavior - maybe there's a case where different file_uploadable schemas should generate different suffixes that I'm not seeing.
The code clearly shows that convertFileSchemaProperty() uses a constant FILE_SUFFIX, so there's no way for different file_uploadable schemas to generate different keyNames for the same base key.
The comment should be deleted because the behavior is actually correct - all file_uploadable schemas for a given key should generate the same keyName.
3. js/src/sdk/utils/processor/file.ts:41
- Draft comment:
Using '!value' in the emptiness check may remove valid non-string falsey values. Confirm that only empty strings are expected. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
Since this is file processing code and the value is used as a string for file upload (line 49: value as string), other falsey values like 0 or false would not be valid file data anyway. The check is specifically looking for empty/missing file data before attempting upload. The current check is appropriate for this use case.
I could be wrong about the requirements - maybe there are valid file upload scenarios where falsey values should be preserved.
Given this is clearly file upload processing code and the value must be a string for S3 upload, preserving non-string falsey values would not make sense in this context.
The comment should be removed as it raises a theoretical issue that is not applicable in this specific file upload context where the value must be a string.
4. js/src/sdk/utils/processor/file.ts:127
- Draft comment:
The variable 'newKeyName' is overwritten for each file_uploadable schema in anyOf. Ensure that if multiple schemas qualify, the intended key is applied. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code does reassign newKeyName for each file_uploadable schema, but this actually appears intentional. The keyName returned by convertFileSchemaProperty will always be the same for a given key (it just appends FILE_SUFFIX). So even with multiple file_uploadable schemas, the final newKeyName would be consistent. The map is transforming the schemas while preserving the overall anyOf structure.
I could be wrong about the intention - maybe there's a case where different schemas should produce different key names. The code could be more explicit about this pattern.
The consistent key name transformation (just appending FILE_SUFFIX) means this isn't actually a bug - it's working as designed to handle file uploads within anyOf schemas.
The comment raises a theoretical concern but doesn't identify an actual bug, since the key name transformation is consistent by design.
5. js/src/sdk/utils/processor/file.ts:147
- Draft comment:
Modifying newProperties while iterating over Object.entries returns a snapshot, but this pattern could be confusing. Consider documenting or refactoring for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_2PRjv3z0drSH6LZw
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| return { | ||
| title: key, | ||
| type, | ||
| description: property.description, |
There was a problem hiding this comment.
In updateAnyOfProperties, the conversion returns a 'description' but the returned object uses property.description instead. Consider using the converted description for consistency.
| description: property.description, | |
| description: description, |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 05ad45c in 32 seconds
More details
- Looked at
28lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. js/src/sdk/utils/processor/file.ts:98
- Draft comment:
Improved multi-line formatting for updateSingleProperty. Consider explicitly typing 'property' as FileBasePropertySchema if applicable for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. js/src/sdk/utils/processor/file.ts:120
- Draft comment:
Consistent multi-line formatting in updateAnyOfProperties. Consider explicitly typing 'property' as FileBasePropertySchema if possible. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. js/src/sdk/utils/processor/file.ts:95
- Draft comment:
Refactor: The updateSingleProperty signature is now multi-line for improved readability. This change is stylistic with no impact on functionality. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. js/src/sdk/utils/processor/file.ts:117
- Draft comment:
Refactor: The updateAnyOfProperties signature was reformatted to use a multi-line format. This improves clarity without altering functionality. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_L9eG7sXW4FXFmZoC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Enhances
FILE_SCHEMA_PROCESSORto handleanyOfproperties and fixes a typo inactions.spec.ts.FILE_SCHEMA_PROCESSORinfile.tsnow handlesanyOfproperties, updating nested schemas and required properties.updateAnyOfProperties()to processanyOfschemas.actions.spec.ts, renamingconnectedAccounstoconnectedAccounts.This description was created by
for 05ad45c. It will automatically update as commits are pushed.