feat (frontend): improve capture UI #5051
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying windmill with
|
| Latest commit: |
41ea843
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e8bf32e9.windmill.pages.dev |
| Branch Preview URL: | https://glm-captures-v3.windmill.pages.dev |
# Conflicts: # frontend/src/lib/components/ArgInput.svelte
![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! Reviewed everything up to c5e7b0b in 1 minute and 56 seconds
More details
- Looked at
2296lines of code in14files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/lib/components/FirstStepInputs.svelte:30
- Draft comment:
Consider adding explicit handling for the case whereflowStoreor$flowStateStoreis undefined to avoid potential runtime errors. - Reason this comment was not posted:
Confidence changes required:50%
The code inFirstStepInputs.sveltehas a potential issue with theloadSchemafunction. The function is called conditionally based on the presence offlowStoreand$flowStateStore, but it does not handle the case where these are undefined. It would be better to handle this case explicitly to avoid potential runtime errors.
2. frontend/src/lib/components/flows/content/FlowInput.svelte:191
- Draft comment:
Consider adding explicit handling for the case wherepayloadis an empty object in theupdatePreviewSchemaAndArgsfunction to avoid potential runtime errors. - Reason this comment was not posted:
Confidence changes required:50%
The code inFlowInput.sveltehas a potential issue with theupdatePreviewSchemaAndArgsfunction. The function setspayloadDataandselectedSchemato undefined whenpayloadis falsy, but it does not handle the case wherepayloadis an empty object. It would be better to handle this case explicitly to avoid potential runtime errors.
Workflow ID: wflow_QEDw8GaZPhw4CLCl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1c940aa in 31 seconds
More details
- Looked at
33lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/flows/content/FlowInput.svelte:230
- Draft comment:
Consider usingstructuredCloneconsistently or only when necessary. For example,savedPreviewArgs = structuredClone(previewArguments)is used here but not when assigningpreviewArguments = savedPreviewArgs. Ensure consistency and necessity of deep cloning. - Reason this comment was not posted:
Confidence changes required:50%
The use ofstructuredCloneis inconsistent and potentially unnecessary in some places.
Workflow ID: wflow_eTxvL8ofq3odA9UV
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7176d80 in 23 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/ArgInput.svelte:748
- Draft comment:
TheschemaSkippedValuesprop is added but not explained in the PR description. Ensure its purpose and usage are clear and documented. - Reason this comment was not posted:
Confidence changes required:50%
The added propschemaSkippedValuesis used in two places, but its purpose is not clear from the PR description. It seems to be intended to skip certain schema values, but without further context, it's hard to determine if this is the correct implementation.
Workflow ID: wflow_FolVdjinDKamHwfH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f114bef in 1 minute and 6 seconds
More details
- Looked at
33lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_Y7djKKLTzCcUGjjM
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.
| let compatible = false | ||
| if (a.type === b.type) { | ||
| if (a.type === 'object' && a.format === b.format) { | ||
| if (a.oneOf && b.oneOf) { |
There was a problem hiding this comment.
The logic for checking oneOf in isCompatibleObject is redundant. The else block is unnecessary since it sets compatible = true regardless of the condition. Simplify the logic to avoid redundancy.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2acc21b in 1 minute and 9 seconds
More details
- Looked at
115lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/lib/components/EditableSchemaForm.svelte:539
- Draft comment:
UsingsetTimeoutwith a fixed delay can lead to unpredictable behavior. Consider using a more reliable method to ensure the schema update is processed correctly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code uses setTimeout as a workaround, with a comment "No better solution than this, needs future rework". This suggests the developers are aware it's not ideal but haven't found a better solution yet. The comment points out a real issue but doesn't provide any concrete alternative solution. In Svelte, state updates and UI synchronization can be tricky, and sometimes setTimeout is used as a last resort.
The comment could be right that there are better solutions, like using Svelte's tick() function or reactive statements. But without understanding the exact race condition or timing issue being worked around, we can't be certain.
While the comment identifies a real code smell, it doesn't provide actionable guidance. The existing code comment already acknowledges this is a temporary solution needing rework.
Delete the comment. While it points out a legitimate concern, it's not actionable enough and the code already acknowledges the issue with a TODO comment.
2. frontend/src/lib/components/schema/schemaUtils.ts:31
- Draft comment:
The assumption thatoneOfproperties are always compatible may not be valid. Consider revisiting this logic to ensure accurate compatibility checks. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code was actually improved to handle oneOf better than before, adding explicit compatibility checks. While the comment is technically correct that the assumption may not be valid, there's already a TODO acknowledging this limitation. The comment doesn't provide specific guidance on how to improve it.
The comment identifies a real potential issue, and the code does make an assumption about oneOf compatibility. The TODO suggests the team is already aware of this limitation.
While the concern is valid, the comment doesn't add value beyond the existing TODO comment. The code has already been improved to handle oneOf cases better than before.
Delete this comment since it doesn't provide actionable feedback beyond what's already acknowledged in the TODO comment.
3. frontend/src/lib/components/schema/schemaUtils.ts:19
- Draft comment:
isCompatibleObjectdoes not account for additional properties inaorbthat might affect compatibility. Consider checking for these properties. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The function was significantly refactored to be more structured. While it's true that there could be other properties affecting compatibility, the current implementation seems intentionally focused on specific properties (type, format, oneOf) that matter for schema compatibility. The TODO comment on line 31 suggests this is a known limitation being handled incrementally. The comment is speculative without concrete examples of what properties matter.
I could be wrong about which properties matter for schema compatibility. There might be important schema properties I'm not considering.
Without specific examples of which additional properties matter for compatibility, and given the intentional TODO about handling oneOf compatibility, this comment is too vague to be actionable.
Delete the comment as it's speculative and doesn't provide concrete guidance on which additional properties need checking.
4. frontend/src/lib/components/schema/schemaUtils.ts:72
- Draft comment:
UsingJSON.stringifyfor object comparison can be inefficient and may not handle all cases correctly. Consider using a deep comparison utility instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While JSON.stringify can have edge cases with object key ordering and special values like undefined, in this context we're comparing schema objects that should be well-formed JSON. The comparison is just an optimization - if it fails, the code falls back to more detailed compatibility checks. The suggested change would add complexity without clear benefit.
The comment raises a valid technical point about JSON.stringify limitations. A deep comparison utility could handle edge cases better.
The current approach is simple and sufficient given the fallback logic and the well-defined schema object structure. The suggested change would add complexity without meaningful benefit in this specific use case.
Delete the comment. The current implementation is appropriate for this specific use case, and the suggested change would add unnecessary complexity.
Workflow ID: wflow_QYCU0mhlozWZ1TFg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6765692 in 1 minute and 7 seconds
More details
- Looked at
3556lines of code in38files - Skipped
1files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/RunFormAdvancedPopup.svelte:32
- Draft comment:
Consider refactoring the logic for loading worker groups into a reusable function or store action to avoid code duplication. This pattern is repeated in multiple files. - Reason this comment was not posted:
Confidence changes required:50%
The code in multiple files is fetching worker tags using the same logic. This is a clear case of code duplication, which can lead to maintenance issues. It would be better to refactor this into a reusable function or a store action.
Workflow ID: wflow_3D89IexuMzIvLI1W
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f107344 in 24 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/schema/EditableSchemaDrawer.svelte:158
- Draft comment:
Reassigningschemato itself is unnecessary. Consider removingschema = schema;as it doesn't change the state. - Reason this comment was not posted:
Confidence changes required:50%
The code uses a pattern where the schema is reassigned to itself, which is unnecessary and can be misleading. This pattern appears in multiple places.
Workflow ID: wflow_6HomwY6oNvwIy3CF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 80866db in 23 seconds
More details
- Looked at
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/schema/schemaUtils.ts:28
- Draft comment:
The added check for compatibility between 'number' and 'integer' types is a valid improvement. - Reason this comment was not posted:
Confidence changes required:0%
The added code inisCompatibleObjectcorrectly handles the compatibility between 'number' and 'integer' types, which is a valid improvement.
Workflow ID: wflow_vjxYZOHLV3heOoCU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0282cf4 in 45 seconds
More details
- Looked at
48lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/flows/content/FlowInput.svelte:317
- Draft comment:
DuplicateresetArgsfunction. Consider removing one of the definitions to avoid redundancy. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_o6PCP5jRO0MZGwRZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 41ea843 in 29 seconds
More details
- Looked at
49lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/schema/EditableSchemaDrawer.svelte:194
- Draft comment:
Consider ensuring that theschemais consistently updated across all components usingbind:schemato avoid potential synchronization issues. This applies to other instances ofbind:schemain this file and inEditableSchemaWrapper.svelte. - Reason this comment was not posted:
Confidence changes required:50%
The code in both files uses thebind:schemadirective multiple times. This can lead to potential issues if the schema is modified in one place and not updated in others. It would be better to ensure that the schema is consistently updated across all components.
Workflow ID: wflow_PJu5Kf6s1j6qVyxE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Enhances frontend schema management and input handling with schema diff support, improved UI components, and better keyboard event handling.
ArgInput.svelte,EditableSchemaForm.svelte, andSchemaForm.svelteto manage added, removed, and modified schema properties.SideBarTab.sveltefor improved navigation inFlowInput.svelte.CaptureTable.svelteandFlowPreviewContent.svelte.SideBarTab.sveltefor dropdown navigation.EditableSchemaForm.svelteandSchemaForm.svelteto handle schema diffs and dispatch changes.SimpleEditor.svelteto improve placeholder visibility.schemaUtils.tsfor computing and applying schema diffs.AddPropertyV2.svelteandEditableSchemaDrawer.sveltefor better schema property management.FirstStepInputs.svelte,HistoricInputs.svelte, andSavedInputsPicker.svelteto prevent default actions on Escape key.This description was created by
for 41ea843. It will automatically update as commits are pushed.