Skip to content

fix: pane relayout order - #1678

Open
tonypzy wants to merge 7 commits into
fossasia:devfrom
tonypzy:fix/relayout-order
Open

fix: pane relayout order#1678
tonypzy wants to merge 7 commits into
fossasia:devfrom
tonypzy:fix/relayout-order

Conversation

@tonypzy

@tonypzy tonypzy commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Description

  • Updated relayout to sort panes before creating the bin and update pane/layout state together using a functional state update.
  • Removed direct state mutations and redundant relayout calls by passing the latest layout and filter values explicitly.

Motivation and Context

The panes had already been reordered, but the layout engine was still calculating positions using the old order and dimensions. Additionally, React state updates are asynchronous and do not take effect immediately.
Besides, switching saved views called relayout three times, and Repack called it twice. Now, both operations call relayout only once.

How Has This Been Tested?

N/A

Screenshots (if appropriate):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code refactor or cleanup (changes to existing code for improved readability or performance)

Checklist:

  • I adapted the version number under py/visdom/VERSION according to Semantic Versioning
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Summary by Sourcery

Simplify pane relayout so panes are re-sorted and re-binned in a single, consistent state update and callbacks run after React commits state.

Bug Fixes:

  • Ensure pane layout and pane indices are updated together via functional state updates instead of mutating shared state.
  • Prevent redundant and out-of-date relayout executions when switching views, repacking, or changing filters by calling relayout once with the latest parameters.

Enhancements:

  • Refactor relayout and rebin to operate on sorted layouts and explicit layout IDs, improving determinism of pane ordering across saved views and filters.
  • Move callback flushing into a useEffect so queued callbacks run after state updates have been applied, aligning callback timing with React lifecycle.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry @tonypzy, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@sourcery-ai

sourcery-ai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Refactors the pane relayout pipeline so panes are sorted and re-binned using up-to-date React state in a single relayout pass, removes direct state mutation and callback-string indirection, and ensures saved-view and filter changes trigger exactly one consistent relayout.

Sequence diagram for the updated relayout pipeline

sequenceDiagram
  actor User
  participant FilterControls
  participant App
  participant storeData
  participant BinPacker as _bin

  User ->> FilterControls: onFilterChange
  FilterControls ->> App: relayout({ filterString })
  App ->> App: getCurrLayoutList()
  App ->> App: sortLayout(prev.layout)
  App ->> App: rebin(sorted, layoutID)
  App ->> BinPacker: _bin.position(idx, windowSize.current.cols)
  App ->> storeData: setStoreData(prev => { panes, layout })
Loading

File-Level Changes

Change Details Files
Make relayout operate via a single functional state update that jointly recomputes pane order, applies saved-layout dimensions, and updates both panes and layout.
  • Replace relayout signature with an options object accepting layoutID and filterString so callers pass explicit state instead of relying on globals.
  • Move layout sorting, filtering, and bin packing inside a single setStoreData functional update, deriving sorted layout from prev.layout and prev.panes.
  • Apply saved layout dimensions by calling rebin on the already-sorted layout before computing bin positions, ensuring the bin packer sees the final pane order.
  • Update panes immutably when assigning new indices and return updated panes and layout together from setStoreData instead of mixing setStoreData and updateLayout calls.
js/main.js
Make rebin and layout management React-idiomatic by avoiding direct mutations and using passed-in layout IDs.
  • Change rebin to accept an explicit layoutID (defaulting from selection) and use that ID for lookups, removing the internal selection.layoutID read and mutation.
  • Return new paneLayout objects with updated h/height and w/width rather than mutating paneLayout in place.
  • Simplify updateLayout to only call setStoreData without mutating storeData.layout directly.
js/main.js
Replace string-based callback queueing with direct relayout calls and a safer post-render callback mechanism, reducing redundant relayouts.
  • Replace places that pushed 'relayout' into callbacks.current (pane deletion, filter changes, layout changes, repack) with direct relayout calls that pass current layoutID and/or filterString.
  • Wrap callback flushing in a useEffect that copies and clears callbacks.current before invoking callbacks, instead of running them inline during render.
  • Update layout change handler to relayout once when switching to a non-default saved layout, instead of pushing three relayout callbacks and mutating selection.layoutID.
  • Remove the extra relayout invocation from the Repack button handler so repack results in a single relayout pass.
js/main.js

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@tonypzy

tonypzy commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

@Manik-Khajuria-5 Manik-Khajuria-5 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@tonypzy I was going through the code rebin() is currently mutating _bin.current from within the setStoreData updater. Since React may replay state updaters, it'd be safer to keep the updater pure and move the _bin.current assignment outside of it. Could we separate the layout computation from the bin reconstruction?

@tonypzy

tonypzy commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@tonypzy I was going through the code rebin() is currently mutating _bin.current from within the setStoreData updater. Since React may replay state updaters, it'd be safer to keep the updater pure and move the _bin.current assignment outside of it. Could we separate the layout computation from the bin reconstruction?

done

@Manik-Khajuria-5 Manik-Khajuria-5 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@tonypzy Please make the below mentioned changes . Rest of code LGTM !

Comment thread js/main.js Outdated
Comment thread js/main.js Outdated
@tonypzy
tonypzy requested a review from rajnisht7 August 5, 2026 23:25
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.

3 participants