fix(grid): v3 review fixes — split-null crash, monitor race, drag-safe gutters
Adversarial review (4 lenses → per-finding verify) of v3 confirmed 6 issues:
- HIGH: splitForLayout({"grid-4":null}) threw a TypeError — null passed the
`!== undefined` guard, then null.cols threw. Since splitForLayout runs on every
grid render, one corrupt localStorage entry would brick the tab UI. Now guards
`!== null && typeof === 'object'`.
- MED: a monitor toggled before the session finished attaching (id still null)
showed the button active while the pane stayed live and sent a resize, and did
not self-correct. onSessionId now reconciles a pending monitor once the id lands.
- MED: renderGutters destroyed + recreated handles on every applyLayout, dropping
the drag listeners if a re-render fired mid-drag. A draggingGutter flag now skips
the rebuild while dragging.
- LOW: grid-presets hardcoded the 1024px breakpoint → now uses GRID_MIN_WIDTH.
Regression tests added (null-split no-throw, attach reconcile, drag survives a
concurrent re-render). typecheck + build:web clean, 1615 tests pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -303,6 +303,8 @@ export function splitForLayout(splits: GridSplits, layout: GridLayout): TrackSpl
|
||||
const stored = splits[layout]
|
||||
const ok =
|
||||
stored !== undefined &&
|
||||
stored !== null && // a null entry ({"grid-4":null}) is valid JSON but not a TrackSplit
|
||||
typeof stored === 'object' &&
|
||||
Array.isArray(stored.cols) &&
|
||||
stored.cols.length === cols &&
|
||||
Array.isArray(stored.rows) &&
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
* the current arrangement and an apply() via hooks, keeping tabs.ts lean.
|
||||
*/
|
||||
|
||||
import { type GridLayout, type TrackSplit, gridAllowed } from './grid-layout.js'
|
||||
import { type GridLayout, type TrackSplit, gridAllowed, GRID_MIN_WIDTH } from './grid-layout.js'
|
||||
|
||||
export interface GridPreset {
|
||||
name: string
|
||||
@@ -186,7 +186,7 @@ export function mountGridPresets(toolbar: HTMLElement, hooks: PresetHooks): Pres
|
||||
|
||||
const mql =
|
||||
typeof window !== 'undefined' && typeof window.matchMedia === 'function'
|
||||
? window.matchMedia('(min-width: 1024px)')
|
||||
? window.matchMedia(`(min-width: ${GRID_MIN_WIDTH}px)`)
|
||||
: null
|
||||
mql?.addEventListener('change', refresh)
|
||||
refresh()
|
||||
|
||||
@@ -128,6 +128,9 @@ export class TabApp {
|
||||
private maximized = false
|
||||
// v3: persisted per-layout column/row track fractions (draggable splitters).
|
||||
private gridSplits: GridSplits = {}
|
||||
// v3: true while a gutter is being dragged, so a concurrent applyLayout does
|
||||
// not destroy-and-recreate the handle (which would drop the drag listeners).
|
||||
private draggingGutter = false
|
||||
// When true, the home chooser is overlaid on top of the open tabs (so you can
|
||||
// jump back to Sessions/Projects without closing anything). Reset whenever a
|
||||
// tab is activated. Irrelevant while no tab is open (home shows regardless).
|
||||
@@ -659,7 +662,14 @@ export class TabApp {
|
||||
sessionId,
|
||||
...(cwd !== undefined ? { cwd } : {}),
|
||||
...(initialInput !== undefined ? { initialInput } : {}),
|
||||
onSessionId: () => this.persist(),
|
||||
onSessionId: () => {
|
||||
this.persist()
|
||||
// v3: a monitor toggled before attach completed (session.id was null, so
|
||||
// applyLayout's monitor branch fell through to the live pane) engages now
|
||||
// that the id is available — reconcile instead of waiting for the next
|
||||
// unrelated structural re-render.
|
||||
if (entry.monitor && this.isVisible(this.tabs.indexOf(entry))) this.updateHomeView()
|
||||
},
|
||||
// onActivity only fires for hidden (inactive) panes (see TerminalSession).
|
||||
onActivity: () => {
|
||||
entry.hasActivity = true
|
||||
@@ -1134,6 +1144,9 @@ export class TabApp {
|
||||
|
||||
/** Replace the gutter handles for the current layout (none in single mode). */
|
||||
private renderGutters(layout: GridLayout): void {
|
||||
// Never rebuild handles mid-drag — the dragged element owns the pointer
|
||||
// listeners and would lose them (existing handles stay valid for this layout).
|
||||
if (this.draggingGutter) return
|
||||
this.paneHost.querySelectorAll('.grid-gutter').forEach((g) => g.remove())
|
||||
if (layout === 'single') return
|
||||
const { cols, rows } = layoutTracks(layout)
|
||||
@@ -1187,6 +1200,7 @@ export class TabApp {
|
||||
const base = splitForLayout(this.gridSplits, layout)
|
||||
const startFractions = (axis === 'col' ? base.cols : base.rows).slice()
|
||||
const total = startFractions.reduce((a, b) => a + b, 0)
|
||||
this.draggingGutter = true
|
||||
try {
|
||||
handle.setPointerCapture(e.pointerId)
|
||||
} catch {
|
||||
@@ -1199,6 +1213,7 @@ export class TabApp {
|
||||
this.setSplit(layout, axis, adjustSplit(startFractions, gutterIndex, delta))
|
||||
}
|
||||
const up = (): void => {
|
||||
this.draggingGutter = false
|
||||
handle.removeEventListener('pointermove', move)
|
||||
handle.removeEventListener('pointerup', up)
|
||||
handle.removeEventListener('pointercancel', up)
|
||||
|
||||
Reference in New Issue
Block a user