fix(dock): persist floating panels across a reload #103
No reviewers
Labels
No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
lz/agent-nexus!103
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/floating-layout-persistence"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #102. Issue #96's design D8, deliberately off
mainand not on the #98→#101 stack — the spec scoped it to its own PR because it is a pre-existing bug.Shift-drag a panel out of the dock, reload, and it was silently gone.
stripArtifactPanelsrebuilt the layout with onlygrid/panels/activeGroup, dropping every float on the way to localStorage — whiledisableFloatingGroupsstayed unset, so the gesture kept working. Floating was half-implemented in the worst direction: the gesture worked, the result did not survive.Five commits. The last three came out of two adversarial reviews, and three of the four defects they found were in code written here and justified in its own comments — two of them documented as deliberate decisions.
1. Repair floats instead of dropping them (
45d8717)A float loses only the views actually stripped; one that loses all of them is removed; one that keeps some is kept with the survivors. Three details a naive
.data.viewsfilter misses:datafor a single group,gridfor a nested layout of several. Repairing onlydatawould have silently discarded every multi-group float. The nested form reusespruneGridObject.activeGroupcan name a floating group, so surviving ids are collected from kept floats too.2. Prune tab groups (
7281aa4)viewswas not the only field naming panel ids —tabGroups[].panelIdsdoes too, and it rode through the spread unexamined. Latent (Nexus never creates tab groups) but fixed: a dangling-reference vector inside the module that exists to eliminate them is incoherent either way.3. Keep a float-only layout (
c5c13e2) — a real hole I had called a limitationFloating a panel calls
removePanel(item, { removeEmptyGroup: true })and drops the emptied source group (dockviewComponent.js:1332,:2576). So floating your only panel leaves a childless branch root —stripArtifactPanelsreturnednull,saveLayoutremoved the stored layout, and the float was lost. The same bug this PR exists to fix, by another route, on the most obvious way anyone would try the feature.My recorded reasoning ("dockview needs a grid root, not worth risking a
fromJSONthrow") was factually wrong:_doFromJSONvalidates onlyroot.type === 'branch' && Array.isArray(root.data), and an empty branch is whatGridview.clear()produces. One reviewer confirmed it against real dockview 7.0.2 in jsdom, reproducing the actualtoJSON()after floating the only panel; I confirmed the restore end in a browser.nullnow means one thing: nothing survived anywhere.4. Position, plus a guard (
2adc3d3)Floats came back in the corner. dockview hands sizing to
ShellManager, whose only size source is a ResizeObserver deferred throughrequestAnimationFrame— so the component is still 0×0 on the synchronous line callingfromJSON.constrainBounds()then clamps every restored float against a zero-height container:clamp(top, -100, 0)is0. Because the clamp writesstyle.top, a later save persists0,0permanently. One line — lay the dock out before restoring — fixes it; a rAF deferral would be a race, since dockview's own observer callback is already inside one.Measured with a negative control first: a float seeded at
top:137 / left:211rendered at0px/0pxbefore and137px/211pxafter, stable across reloads.Also replaced the narrowed local types with
NonNullable<SerializedFloatingGroup['data']>— the narrowing is what let the tabGroups bug in, since it omitted the one field naming panel ids.5.
maximizedNode, and the guard's blind spot (df7d394)maximizedNodeis a positional reference —{ location: number[] }, an index path into the tree — so no panel-id audit sees it. The float branch spread it through; the main grid dropped it. Pruning shifts the indices it names, andGridview.deserializethen throwsInvalid location(gridview.js:767-769) — the stripped layout fails to restore where the unstripped one would have. Unreachable in 7.0.2 (maximize()early-returns for non-grid groups), but the asymmetry was the defect. Both sites drop it now; an index path can't be corrected after pruning, and maximize state isn't worth re-deriving.And the guard from commit 4 was one level too shallow. It pinned
keyof SerializedDockview— the top-level set — while a second hand-listed set sat ten lines below rebuildinggrid, unguarded, omitting exactlymaximizedNode. Widening tokeyof SerializedDockview['grid']would not have worked: dockview declares that member inline with four fields whiletoJSONassigns it a fullSerializedGridviewwith five, so the declared type understates the runtime and a guard over it is blind by construction. The second assertion guardsSerializedGridviewitself. Both guards verified by removing a key:Tests
The test the spec named for inversion — it pinned the dropping behaviour, and its staying green is why nothing flagged this bug — is replaced by 35 cases: both serialized forms, partial strip,
activeViewre-pointing,activeGroupon a surviving and a dropped float, malformed floats, tab-group pruning in grid and float, float-only layouts,maximizedNodeon both sites, and the genuine nothing-survives case.Plus the invariant rated above every individual case: a branch root in produces a branch root out, across six shapes — dockview rejects a non-branch root outright, so it decides whether the persisted blob is restorable at all.
Every fix was verified to be pinned rather than merely passing, by reverting the implementation and confirming the new cases go red while the invariants stay green (5/21 float repair, 3/27 tab groups, 1/34 maximizedNode).
Gates: 4855 files / 0 errors 0 warnings, 108 files / 1032 tests, lint clean.
Reviewed and found clean
Idempotence (
strip(strip(x))) and non-mutation survive the rewrite;emptyBranchRootholds on both arms; tab-group pruning reaches both float forms;d.layout(...)has no zero-size mount path (the mobile dock pane is hidden bytransform, which preserves dimensions). The panel-id reference set is complete:views,activeView,tabGroups.panelIds, thepanelskeys,activeGroup— withmaximizedNodethe only structural one, now handled.`views` was not the only field on a group's view state that references panel ids — `tabGroups[].panelIds` does as well. It rode through the `{...data}` spread unexamined, so a tab group could keep naming an artifact panel that had just been stripped: a dangling reference persisted by the one function whose job is to persist none. TypeScript could not see it. The local narrowed `PanelViewState` deliberately omitted every optional field of dockview's `GroupPanelViewState`, and its own comment listed `tabGroups` among them — so the field was carried at runtime and invisible at compile time. It is now modelled locally, with the rule stated: model a field here the moment it can name a panel. Latent rather than live — Nexus never creates tab groups, and no saved layout carries the key today. Fixed anyway because a dangling-reference vector inside the module that exists to eliminate them is incoherent whether or not anything currently triggers it. Emptied tab groups are dropped rather than persisted with an empty panelIds array. Verified the three new tests fail against the previous implementation and the fourth (no tab groups present) passes either way.The previous commit shipped a hole I had documented as a deliberate limitation. It was not one, and it sat on the most natural way to try floating at all. Floating a panel calls `removePanel(item, { removeEmptyGroup: true })` and `removeGroup` fires when the source group empties (dockviewComponent.js:1332, :2576). So floating your ONLY panel leaves a childless branch as the grid root. `pruneGridObject` returned null for that, `stripArtifactPanels` returned null, and `saveLayout` then REMOVED the stored layout — wiping the float on the next save. The same silent loss the previous commit set out to fix, reached by a different route, and reachable by the single most obvious gesture. The reasoning I recorded for it ("dockview needs a grid root, not worth risking a fromJSON throw") is factually wrong for 7.0.2. `_doFromJSON` validates only `root.type === 'branch' && Array.isArray(root.data)` (dockviewComponent.js:1937-1939); an empty branch passes, and is exactly what `Gridview.clear()` produces for an empty dock. Verified in a browser: injecting an empty-branch root plus one float restores the float, renders it, and round-trips through three reloads with a forced relayout each time. So `null` now means one thing only — nothing survived ANYWHERE — which is what saveLayout's removeItem branch is actually for. Those two states were conflated. When the grid is empty but floats survive, the grid is emitted as an empty branch, preserving the original root's own fields rather than rebuilding it. The test that asserted the old behaviour is inverted, and joined by the literal post-float state (already-empty branch root) and by the genuine nothing-survives case, so the two are no longer confused. Found by an adversarial review of the previous commit. It was right and I was wrong: I had called this a corner case, and it is the primary path.Three findings from an adversarial review of this branch. **Floats came back in the corner.** dockview hands sizing to ShellManager, whose only size source is a ResizeObserver deferred through requestAnimationFrame — so the component is still 0x0 on the synchronous line where we call fromJSON. That captures 0x0, and the constrainBounds() fromJSON runs at the end clamps every restored float against a zero-height container: clamp(top, -100, 0) is 0, and the same for left. The float lands at the dock origin, and because the clamp writes style.top, a later save persists 0,0 and the position is gone for good. One line fixes it: lay the dock out before restoring. Deferring the restore into a rAF would be a race instead, since dockview's own observer callback is already inside one. Measured, with a negative control first: seeding a float at top 137 / left 211 and reloading rendered it at 0px/0px before the change and at 137px/211px after, stable across reloads with a forced relayout each time. **The whitelist rebuild had no guard, only comments.** `stripArtifactPanels` rebuilds its result from a hand-listed set of SerializedDockview fields, and that is exactly how floats were lost for a release: a field existed, nothing referenced it, no test noticed. The justification for still dropping popoutGroups/edgeGroups was a comment asserting a fact about the UI that nothing enforced. There is now a compile-time assertion over `keyof SerializedDockview`, in the same idiom as the `_exhaustive: never` guards in mounts/validate.ts. Verified it fires by dropping a key from the allowlist: ERROR layout.ts 185:7 Type 'boolean' is not assignable to type '{ UNHANDLED_DOCKVIEW_KEY: "edgeGroups"; }' The next dockview upgrade that adds a serialized field now fails the build and names it, instead of silently discarding operator state. **The narrowed local types are gone.** `GroupPanelViewState` is not re-exported by name, but it is reachable structurally as `NonNullable<SerializedFloatingGroup['data']>` — through the very type this branch already imported. The hand-narrowed copy is what let the tabGroups bug in two commits ago: it omitted the one field that names panel ids, so the spread carried it through invisibly. The hand-rolled SerializedTabGroup mirror goes too; dockview exports it (`export * from 'dockview-core'`). What was a standing promise to mirror the right fields is now structural. Also adds the invariant the reviewer rated above every individual case: whatever else happens, a branch root in produces a branch root out. dockview rejects a non-branch root outright, so it is the single property deciding whether the blob we persist is restorable at all.The guard added in the previous commit was one level too shallow, and the field it missed was live. `maximizedNode` is a POSITIONAL reference — `{ location: number[] }`, an index path into the grid tree — so it is invisible to a panel-id audit. The float branch spread it through with `{ ...float.grid, root }` while the main grid, which rebuilds field by field, dropped it. Pruning shifts or deletes the indices it names, so `Gridview.deserialize` then calls `getNode(location)` and throws 'Invalid location' (gridview.js:767-769): the STRIPPED layout fails to restore where the unstripped one would have. Measured against real dockview 7.0.2 in jsdom, not a fixture. Unreachable in 7.0.2 — `maximize()` early-returns unless the group's location is 'grid', so a float's nested gridview cannot carry the field — but the asymmetry was the bug: two rebuild sites, two different answers, one of them unsafe. Both now drop it. Dropping beats repairing: an index path cannot be corrected after pruning without re-deriving it, and maximize state is not worth that. The deeper problem is why the guard did not catch it. `AssertNoUnhandledKeys` was parameterised over `keyof SerializedDockview`, which pins the TOP-level field set only — while the grid sub-object at the rebuild site is a second hand-listed set, and hand-listed sets are precisely the mechanism that lost floats for a release. Widening to `keyof SerializedDockview['grid']` would not work either: dockview declares that member inline with four fields (dockviewComponent.d.ts:99-105) while `toJSON` assigns it `this.gridview.serialize()` — a full `SerializedGridview` with five (dockviewComponent.js:1832, :1840). The declared type understates what the runtime writes, so a guard over the declared shape is blind by construction. So the second assertion guards `SerializedGridview` itself, the shape actually written. Verified it fires by removing a key: ERROR layout.ts 225:7 Type 'boolean' is not assignable to type '{ UNHANDLED_DOCKVIEW_KEY: "maximizedNode"; }' Found by the same adversarial review, re-run against the previous commit. Its verdict on the rest of that commit: idempotence and purity survive the rewrite, `emptyBranchRoot` holds on both arms, tabGroups pruning reaches both float forms, and the layout-before-restore call has no zero-size mount to trip on.