feat(studio): all panels move onto the dockable, persisted layout - #4192
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Not stamping this head, for two reasons that are both cheap to clear. The review below is the real work — the conflict diagnosis at the end should make the rebase a two-minute job.
1. The head is DIRTY — the tree that merges cannot be the tree I read
mergeable: false, mergeable_state: dirty, rebaseable: false, behind_by: 1. Approving here stamps a tree that someone has to resolve first, and on this repo the stamp would be void anyway: require_last_push_approval: true means the resolution push re-gates it (observed live on #4181 earlier today — the approval stayed APPROVED while reviewDecision went back to REVIEW_REQUIRED). So "one approval on this head clears the gate" does not hold at cdfa6060; GitHub will not merge a conflicting PR at any approval count.
The conflict is small, mechanical and entirely mine to have caused. behind_by: 1 is fd6049f3 — the merge of #4189, which I approved eleven minutes before this PR opened. #4189 touched three files; the only one this PR also touches is packages/studio/src/hooks/useAppHotkeys.ts, and this PR's change to it is four deleted lines, no additions, all removing leftSidebarRef / LeftSidebarHandle:
| hunk | line removed | conflicts? |
|---|---|---|
| 1 | import type { LeftSidebarHandle } … |
yes — #4189 deleted the adjacent STUDIO_MOTION_PATH import |
| 2 | leftSidebarRef: React.RefObject<…> in UseAppHotkeysParams |
yes — #4189 replaced the adjacent syncHistoryPreviewAfterApply type with UseEditHistoryActionsOptions["syncHistoryPreviewAfterApply"] |
| 3 | leftSidebarRef, in the destructure |
no — context unchanged |
| 4 | leftSidebarRef, in cbRef.current |
no — context unchanged, offset only |
Both conflicts are contextual, not semantic: #4189 moved the undo/redo wiring out of that file, this PR removes a sidebar ref from it, and the two never touch the same line. The resolution is to take #4189's version of the file and delete those four leftSidebarRef lines. All four still exist in it.
2. Eight deletions, zero entries in ALLOWED_DELETIONS
scripts/check-no-main-deletions.mjs runs git diff --name-status -M <base>...HEAD and fails on any D path not named in its 37-entry map. This branch proposes eight, and none is in it:
packages/studio/src/components/PanelTabButton.tsx
packages/studio/src/components/StudioLeftSidebar.tsx
packages/studio/src/components/nle/TimelineResizeDivider.tsx
packages/studio/src/components/sidebar/LeftSidebar.storage.test.ts
packages/studio/src/components/sidebar/LeftSidebar.tsx
packages/studio/src/hooks/useInspectorSplitResize.ts
packages/studio/src/utils/fitPanels.test.ts
packages/studio/src/utils/fitPanels.ts
Git's own -M may pair one or two of these with the new files, but fitPanels.ts, useInspectorSplitResize.ts, TimelineResizeDivider.tsx and PanelTabButton.tsx have no plausible partner among the additions, so at minimum those land as D.
I'm raising it as a review point rather than a red check because the reason strings are the thing I'd otherwise be asking you for in prose. The script's own comment puts it well — naming each path "makes every intentional removal a reviewable line in a diff." Eight one-line reasons is the deletion audit for this PR, and it's the artifact the next person reading git log for "where did the resize code go" will actually find.
Findings in the code
visiblePanelInZone is called with openPanels at one site, and the name says the opposite. The helper's parameter is visiblePanels and its doc says "the last one focused if still showing, else any showing" (dockLayoutStore.ts:100-105). useRightPanelIntent (useRightPanelIntents.ts:59-62) passes state.openPanels, and its own doc says "whether or not the column is currently showing" — deliberately the negation. The call is right for what that hook wants; the helper's name and parameter are what's wrong now that it has two meanings. panelInZoneFrom(zone, lastActive, candidates) would let both call sites read honestly.
sync's two writes disagree about the center zone. In dockLayoutStore.ts:59-68, the groupActivePanels loop guards if (zone !== "center"), but the activePanel write two lines below does not, so lastActive.center can be set by one path and never by the other. Inert today — every call site of visiblePanelInZone passes "right", and the new store test only asserts left and right — but the two paths give different answers the moment a center reader exists. Either guard both or neither.
The test-count drop reads worse than it is. usePanelLayout.test.ts goes 18 named cases → 9, −296/+108. I compared the name sets: the departed cases are almost entirely width, rail and auto-collapse behaviour ("caps a panel relative to the window instead of at a flat 600px", "rails both panels once the window cannot fit them", "forgets that reopen once the window is wide again") — they tested fitPanels/useInspectorSplitResize, which this PR deletes, so they left with their subject rather than leaving a hole. The nine replacements cover the dock-backed surface including the URL-restore path. Worth stating in the body, because the raw line count invites the opposite reading.
Credit where the cleanup is careful:
.fallowrc.jsoncremoves its own now-dead ignore entries forLeftSidebar.tsxrather than leaving them to rot, and fixes the staleStudioRightPanel→StudioRightPanelsreferences. Deleting your own suppressions in the same PR is the part that usually gets skipped.detachnow clearslastActive(:58), so a second dock doesn't inherit the first one's memory — and the new test pins exactly that.- Gating the
setTitleeffect onopen(Dock.tsx:218-222) stops title writes to panels that aren't in the dock. - 63 lines of new hook arrived with 181 lines of test, and all five exported hooks have real call sites in
App.tsx/StudioRightPanels.tsx— no speculative surface. studioUiPreferencesdrops the four dead keys from the reader, not just the type, so staleleftWidth/timelineHeightvalues in someone's storage are ignored rather than half-honoured.
Scope
I audited the dock store and its snapshot producer, usePanelLayout, useRightPanelIntents, the preference reader, the fallow config, the deletion set and the conflict surface. I did not audit EditorShell.tsx, App.tsx's wiring, StudioLeftPanels.tsx, TimelinePane.tsx or the e2e script — at 54 files I scoped to the state machine and the removals rather than claiming the whole diff.
Ping me at the rebased head with the ALLOWED_DELETIONS entries and I'll approve there.
— Rames
cdfa606 to
23836af
Compare
5dccd2f to
579a283
Compare
…onto it EditorShell, App.tsx, the left and right panel columns, and the inspector hooks now render through Dock.Panel instead of the old fixed-width sidebars and split-pane inspector. usePanelLayout becomes a thin facade over the dock store; fitPanels, the resize divider and the split-inspector hook are dead and removed.
…after dock migration Collapse inspectorButtonActive into inspectorPanelActive, remove unread layers/design active fields, un-export unused prop types, flatten a nested ternary, and point fallow ignores at the renamed/deleted files.
Block params dismiss on selection or when Design is hidden, the slideshow panel can be closed, the selection outline follows the inspector intent, caption editing focuses Design, and a URL slideshow tab opens once the panel exists.
…open after the dock migration Block params stay open over an element that was already selected, caption mode wins over block params, the slideshow panel waits for the dock, the Renders tab title survives a reopen, each side column remembers the tab it last showed, tab switches are reported only when the tab changes, and the e2e thumbnail selector follows the dock.
…ement, and panel titles are covered The dismissal compares the selected element by its target key instead of object identity, because a preview reload rebuilds the selection for the same element. A closed and reopened panel keeps its custom title, with a test, and the e2e selector is formatted.
…ed slideshow placement survives loading Block params are now dismissed by the tab setter the way their old tab was, including a re-pick of the same element, instead of by comparing selections. The slideshow panel is only closed after a file that was a slideshow stops being one, so a restored layout is not discarded while the file loads.
The rebase onto main left two identical inspectorButtonActive/inspectorOpen constants after a conflict resolution; only the first was still referenced.
StudioLeftSidebar.tsx, LeftSidebar.tsx and its storage test, TimelineResizeDivider.tsx, useInspectorSplitResize.ts and fitPanels are replaced by the dock; the CI deletion guard needs each named with its reason.
… its dock panel wrapper
Every dock panel wrapper now carries aria-label=<title>, matching the "Timeline"
title for the timeline panel. Timeline.tsx also set aria-label="Timeline" on its
own content div, so `document.querySelector('[aria-label=\"Timeline\"]')` found
the dock wrapper first and the timeline viewport gate CI check timed out waiting
for an attribute the real content div actually carried. Renamed to "Timeline
track view" and updated every consumer: the production diagnostics selector,
its tests, and the e2e viewport-gate script.
…ase regression Rebasing PR2 onto main replayed an older local commit whose diff did not textually overlap main's newer inspectorButtonActive prop, so git applied it with no conflict marker and silently reverted the header to computing the value itself instead of receiving it. StudioHeader.dom.test.tsx already asserts the prop is respected; restoring it makes that test meaningful again. App.tsx now computes it once (panelLayout.rightCollapsed is already the dock's effective collapse state, no separate railed-by-width case) and passes it down.
579a283 to
7ff2a74
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 7ff2a740. Both things I held on at cdfa6060 are cleared, and I re-read everything that entered the patch since.
The two conditions
1. The head is no longer DIRTY. The useAppHotkeys.ts conflict with #4189's merge is resolved.
2. All seven deletions are declared. scripts/check-no-main-deletions.mjs gains +25 lines and every removed path is now a key in ALLOWED_DELETIONS, each with a reason that names its replacement:
| deleted | declared reason checks out |
|---|---|
StudioLeftSidebar.tsx |
→ StudioLeftPanels.tsx, added at +202 |
LeftSidebar.tsx |
→ dock-mounted CompositionsPanel.tsx, added at +15 |
LeftSidebar.storage.test.ts |
tested fixed-sidebar tab persistence; the dock's own persistence gets dockLayoutStore.test.ts at +28 |
fitPanels.ts / fitPanels.test.ts |
computed fixed left/right widths for the old shell |
useInspectorSplitResize.ts |
resized the old split pane |
TimelineResizeDivider.tsx |
superseded by the dock's sash |
It was eight when I looked; PanelTabButton.tsx moved to #3624. Worth noting the surviving behavior was carried over rather than dropped — LeftSidebar.masterComposition.test.tsx and LeftSidebar.lintBadge.test.tsx are renames, not deletions.
What changed since I read it, and why I re-read it
Six files entered the patch. Five are one mechanical rename, and I audited it as a contract change because a miss here fails silently: readTimelinePerformanceDiagnostics returns the zero baseline when its selector finds nothing, which is indistinguishable from "timeline not mounted" — a stale consumer would report healthy zeros forever.
aria-label="Timeline" → "Timeline track view" has exactly four consumers on main, and this PR updates all four: the producer (player/components/Timeline.tsx), the selector (timelinePerformanceDiagnostics.ts), its unit test, and the timeline-virtualization.mjs e2e. Nothing left pointing at the old label.
Same check on the other contract change, since StudioHeader.dom.test.tsx's mock swapped effectiveRightCollapsed → rightCollapsed and a stale reader of that key would go quietly undefined/falsy rather than fail: effectiveLeftCollapsed/effectiveRightCollapsed resolve to seven files on main, and all seven are in this patch removing them. The derived auto-rail-by-width state is gone wholesale with the fixed sidebars, not half-removed.
Approving on code merit; I did not use CI as a basis.
— Rames
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-stamp at a17f9c47 — my approval at 7ff2a740 was re-gated by the push, as require_last_push_approval does on this repo.
The only delta is a17f9c47 itself: a doc comment in useRightPanelIntents.ts wrapped from one line to two, 2+/1-, no code change and no wording change. Every file's patch is otherwise byte-identical to the head I reviewed, so the verification in my previous review stands unchanged.
— Rames
2 participants
What
Moves all ten Studio panels onto the dock (
Dock.Root/Dock.Panel) built in #4165 and #4186: users can drag any panel to any side, close and reopen it, resize by dragging a sash or with the keyboard, and the layout persists across a reload. Measured against main with #4165 and #4186 already merged.Before
Fixed left/right columns, no dragging.
After
Default layout, a panel moved, a panel closed then reopened, layout persisted across reload, a panel drag, and a narrow window at 560px.
Default dock layout:
A panel moved to another zone:
A panel closed:
The same panel reopened:
Layout persisted across a reload:
Window > Reset layout returns to default:
Catalog panel open, a WebGL effect clip on the timeline, playhead at 00:02, rendering correctly on the dock layout:
Inspector open with an element selected:
Inspector closed:
560px window, everything still fits:
Dragging a panel to a new dock position:
https://github.com/user-attachments/assets/f3201fb5-5939-47e8-92c3-51be3a7ae5e4
Accepted behaviour changes (disclosed, not regressions of a spec)
EditorShelltakes apanelsprop instead of separateleft/rightprops.leftCollapsed,leftWidth,rightWidth,timelineHeight) are dropped with no migration; the dock's own persisted layout replaces them.VITE_STUDIO_FLAT_INSPECTOR_ENABLED=falsesplit-inspector path is removed;docs/weekly-updates.mdxstill references it as a historical note..hf-dock img[src*="/thumbnail/"].Verification
setRightPanelTabcall site checked against its consumer, hostile orderings (add-block while collapsed, hotkeys, group selection), and the slideshow open/close ref rule. No blocking findings; two should-fix items folded into the disclosed list above.Audited / Trusting / Not exercised
setRightPanelTabcaller and itsrevealPanelflag, the hotkey handlers, the slideshow effect's transition logic, mutation testing of the above, every rebase conflict hunk diffed against both parents.Not in scope
Keyboard sash resize, tab arrow navigation and the narrow-window minimum fit landed in #4186. The desktop-host export surface (letting an embedding app build its own layout from this package) is a separate follow-up PR.