Skip to content

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
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Guilhem-lm
Copy link
Contributor

@Guilhem-lm Guilhem-lm commented May 28, 2025

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.

  • Behavior:
    • Add support to save and restore split panes layout and tabs state in FlowBuilder.svelte.
    • Introduce SplitPanesLayout and TabsState classes to manage layout and tab states.
    • Update saveSessionDraft() in FlowBuilder.svelte to include split_panes_layout and tabs_state.
  • Components:
    • Modify FlowBuilder.svelte to initialize and set contexts for SplitPanesLayout and TabsState.
    • Update ModulePreviewResultViewer.svelte, FlowEditor.svelte, FlowBranchesAllWrapper.svelte, FlowBranchesOneWrapper.svelte, FlowLoop.svelte, FlowModuleComponent.svelte, FlowWhileLoop.svelte, PropPickerWrapper.svelte, CaptureSection.svelte, TriggersEditor.svelte to use new Splitpanes and Pane components.
  • Files:
    • Add SplitPanesLayout.svelte.ts and tabsState.svelte.ts for managing layout and tab states.
    • Add Pane.svelte, Splitpanes.svelte, and types.ts in splitPanes for pane management.
  • Misc:
    • Update +page.svelte files in flows/add and flows/edit to handle new layout and tab state parameters.

This description was created by Ellipsis for b6bffac. You can customize this summary. It will automatically update as commits are pushed.

Copy link

cloudflare-workers-and-pages bot commented May 28, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

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

View logs

@Guilhem-lm Guilhem-lm marked this pull request as ready for review June 2, 2025 12:08
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 20 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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 name setSavedraftCb appears to have a typo. Perhaps it should be setSaveDraftCb?
  • 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 named setTabStateContext while the variable is tabsState. For consistency with setSplitPanesLayoutContext, consider renaming it to setTabsStateContext.
  • 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 Ellipsis 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant