fix: decouple the panel divider's unmount-commit from the onCommit prop identity - #16
Merged
Conversation
…ntity The mid-drag-unmount cleanup added in #15 tears down the live gesture (it nulls `drag`), so having `onCommit` in its dep list made correctness depend on the caller passing a referentially stable callback. Workspace happens to pass useState setters, so this is latent — but an inline lambda anywhere would break dragging on the next parent render: a premature commit, every later pointermove ignored, and `panel-resizing` stuck on <body> (endDrag returns early on a null gesture, so `dragging` never clears) leaving the whole app in col-resize/no-select. Read onCommit through a ref and key the effect on [dragging] alone. Regression test asserts a parent re-render mid-drag leaves the gesture intact; it fails on the old code at the premature-commit assertion. Also two comment corrections and a docs catch-up: - The cleanup's "⌃R workspace switch" example was inert: App keys Workspace by workspace id, so a switch unmounts the whole thing — the .workspace element (and the inline var) goes with it, and the commit lands on an unmounting component, so nothing persists and nothing needed to. ⌘⇧\ closing the rail is the real case. - Note that the pointer path freezes its editor-room floor for the gesture while the keyboard path re-tightens per keypress, so the two disagree after shrinking past the cap and coming back. - CLAUDE.md: list the two panel-width keys with the other per-workspace layout keys, and give PanelResizer an entry in the components map. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cargo fmt --check` has been red on main since the iOS work landed: the two cfg-gated `MetadataExt` imports in `is_dataless` are out of alphabetical order, which put the rust job in a failing state and so skipped its `cargo test` and `cargo clippy` steps entirely. Pure `cargo fmt` output; the two imports are mutually exclusive by cfg, so the order carries no meaning. Locally: fmt clean, 25 Rust tests pass, clippy --all-targets -D warnings clean (macOS host — CI builds Linux, where the macOS-only paths are cfg'd out).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Follow-up to #15, from the re-review's one remaining finding plus two comment corrections and a docs catch-up.
The fix
PanelResizer's mid-drag-unmount cleanup (added in #15 to stop a rail-close under a held divider from orphaning the inline width var) tears down the live gesture — it nullsdrag.current. HavingonCommitin the effect's dep list therefore made the component's internal correctness depend on the caller passing a referentially stable callback.Workspacehappens to pass rawuseStatesetters, so nothing is broken today. But an inline lambda anywhere — the obvious thing to write — breaks dragging the moment the parent re-renders for any unrelated reason. Measured on the pre-fix code:The third is the bad one:
endDragreturns early on a null gesture, sosetDragging(false)never runs andpanel-resizingstays on<body>— app-widecursor: col-resizeanduser-select: noneuntil the divider unmounts.Reading
onCommitthrough a ref lets the effect key on[dragging]alone, which is what it actually means.Also
⌃R workspace switchexample was inert.AppkeysWorkspaceby workspace id (App.tsx:111), so a switch unmounts the whole thing: the.workspaceelement and its inline var are destroyed, and the commit lands on an unmounting component sousePanelWidth's persistence effect never runs. Nothing was committed and nothing needed to be.⌘⇧\closing the rail (Workspace stays mounted) is the real case.startWidth, so never worse than where the drag began. Deliberate, but the two paths were presented as sharing "the same floor".PanelResizerhas an entry in the components map.Tests
One regression test: a parent re-render mid-drag with a deliberately unstable
onCommitmust leave the gesture intact. Verified it fails on the pre-fix code at the premature-commit assertion, and passes after.970 passing,
typecheck/format:check/ ESLint clean.Unrelated: the red
rustCI jobFolded in as its own commit.
cargo fmt --checkhas been failing onmainsince the iOS work landed — the two cfg-gatedMetadataExtimports inis_datalessare out of alphabetical order. Because that step comes first, the job'scargo testandcargo clippysteps have been skipped ever since, so the Rust suite hasn't actually run in CI since #14.The commit is pure
cargo fmtoutput (the two imports are mutually exclusive by cfg, so the order carries no meaning). CI is now green on both jobs — first fullrustpass sincef2a4c56, which means clippy and the 25 Rust tests are exercised on Linux again.🤖 Generated with Claude Code