-
Notifications
You must be signed in to change notification settings - Fork 689
Feat(frontend): save splitpanes and tabs #5831
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
Open
Guilhem-lm
wants to merge
9
commits into
main
Choose a base branch
from
feat/frontend/save-flow-builder-layout
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or 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: |
b6bffac
|
Status: | ✅ Deploy successful! |
Preview URL: | https://fb1bceaa.windmill.pages.dev |
Branch Preview URL: | https://feat-frontend-save-flow-buil.windmill.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to b6bffac in 3 minutes and 26 seconds. Click for details.
- Reviewed
896
lines of code in20
files - Skipped
0
files when reviewing. - Skipped posting
10
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/FlowBuilder.svelte:148
- Draft comment:
Good integration of splitpanes and tabs state restoration via context. Ensure that savedSplitPanesLayout and tabsStateFromUrl default to safe values when undefined to avoid runtime issues. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/src/lib/components/splitPanes/SplitPanesLayout.svelte.ts:44
- Draft comment:
In the #scalePanes method, guard against a potential division by zero and ensure pane sizes are defined before scaling. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. frontend/src/lib/components/splitPanes/Pane.svelte:16
- Draft comment:
Consider verifying that the context methods (setActivePane and removeActivePane) exist before calling them, to avoid potential runtime errors if the context is missing. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code does handle the null context case with ??, but destructuring undefined methods from an empty object will result in undefined values. When these methods are called later, it would cause runtime errors. The comment identifies a real potential issue. However, this seems like it would be caught immediately during testing/development. The context might intentionally be optional in some valid use cases. Also, TypeScript should catch this if the types are set up correctly. While there might be valid optional cases, calling undefined methods would crash the app. However, this is a basic TypeScript/testing issue that would be caught immediately. Delete the comment. While technically correct, this is a basic issue that would be caught by TypeScript or immediate testing. It's not significant enough to warrant a PR comment.
4. frontend/src/lib/components/splitPanes/Splitpanes.svelte:20
- Draft comment:
The use of non‐standard macros like $state and $derived should be well documented to help future maintainers understand their behavior. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. frontend/src/lib/components/common/tabs/tabsState.svelte.ts:6
- Draft comment:
Usage of private fields (e.g. #selected) is modern; ensure compatibility with your build system and consider adding JSDoc comments for clarity. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
6. frontend/src/lib/components/flows/propPicker/PropPickerWrapper.svelte:47
- Draft comment:
The new 'moduleId' prop is used to generate a unique ID for the Splitpanes wrapper. Ensure that this prop is always provided to avoid duplicate IDs. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is informative and suggests ensuring that a prop is always provided. It doesn't provide a specific code suggestion or ask for a test to be written. It violates the rule against making purely informative comments.
7. frontend/src/routes/(root)/(logged)/flows/add/+page.svelte:187
- Draft comment:
Restoration of splitpane and tab state improves UX; ensure the decoded state (via decodeState) handles edge cases and integrates correctly with FlowBuilder. - 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.
8. frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte:101
- Draft comment:
The restoration logic for splitpanes and tabs state here mirrors the add page. Consider adding extra error handling if state decoding fails. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code does access these new state properties without checks. However, the decodeState() function likely already handles malformed JSON. The comment is somewhat speculative - we don't know if error handling is actually needed here. The code seems to use optional chaining already which provides some safety. I may be underestimating the importance of defensive programming here. If state decoding fails, it could cause runtime errors. While defensive programming is good, this seems like an edge case. The code uses TypeScript and optional chaining, and decodeState() likely handles basic errors already. The comment is somewhat speculative and not clearly actionable enough. We don't have strong evidence that additional error handling is needed here.
9. frontend/src/lib/components/FlowBuilder.svelte:111
- Draft comment:
Typo: The variable namesetSavedraftCb
appears to have a typo. Perhaps it should besetSaveDraftCb
? - Reason this comment was not posted:
Comment was on unchanged code.
10. frontend/src/lib/components/FlowBuilder.svelte:152
- Draft comment:
Typo: The context function is namedsetTabStateContext
while the variable istabsState
. For consistency withsetSplitPanesLayoutContext
, consider renaming it tosetTabsStateContext
. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment points out a real naming inconsistency, it's a very minor issue that doesn't affect functionality. The function name is imported from another file, so changing it would require changes in other files. The inconsistency is not confusing enough to warrant a comment. The comment is technically correct - there is an inconsistency. And consistent naming can improve code readability. However, this is an extremely minor stylistic issue. The function name is clear enough as is, and changing it would require modifying other files for minimal benefit. We should focus comments on more impactful issues. Delete this comment. While technically correct, it points out a very minor naming inconsistency that doesn't significantly impact code readability or maintainability.
Workflow ID: wflow_nOZt4cPKzHPtvZk4
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Add possibility to save splitpanes && tab layout
Important
Add functionality to save and restore split panes and tabs layout in the frontend, updating multiple components to support this feature.
FlowBuilder.svelte
.SplitPanesLayout
andTabsState
classes to manage layout and tab states.saveSessionDraft()
inFlowBuilder.svelte
to includesplit_panes_layout
andtabs_state
.FlowBuilder.svelte
to initialize and set contexts forSplitPanesLayout
andTabsState
.ModulePreviewResultViewer.svelte
,FlowEditor.svelte
,FlowBranchesAllWrapper.svelte
,FlowBranchesOneWrapper.svelte
,FlowLoop.svelte
,FlowModuleComponent.svelte
,FlowWhileLoop.svelte
,PropPickerWrapper.svelte
,CaptureSection.svelte
,TriggersEditor.svelte
to use newSplitpanes
andPane
components.SplitPanesLayout.svelte.ts
andtabsState.svelte.ts
for managing layout and tab states.Pane.svelte
,Splitpanes.svelte
, andtypes.ts
insplitPanes
for pane management.+page.svelte
files inflows/add
andflows/edit
to handle new layout and tab state parameters.This description was created by
for b6bffac. You can customize this summary. It will automatically update as commits are pushed.