pin why a layout diff cannot answer "what closed"
The panel-close signal (§5.1) was going to be a before/after diff of the layout tree — the todo
document says so. It cannot be. `movePanel` inserts through `newPanelFrom`, which mints a fresh
`uid()`, so a dragged panel's id is gone from the new tree while its app is still on screen; and
`swapPanels` exchanges `{appType, config}` between two ids that both stay put, so a swap reads as
two closes and two opens. Everything downstream of a close signal is destructive — a pty killed, a
session released — so a mechanism that fires on a rearrangement is worse than none.
Two tests, no production change. The signal has to be raised where the intent is known, at
`WorkspaceView`'s `handleRemove`/`handleSetApp` call sites.
This commit is contained in:
@@ -497,3 +497,33 @@ describe('pruneEmptyPanels', () => {
|
||||
expect((pruneEmptyPanels(root) as LayoutPanel).config).toEqual({ agentName: 'frontend' });
|
||||
});
|
||||
});
|
||||
|
||||
// A panel id is a *position in the tree*, not an app instance. These two tests are why the panel-close
|
||||
// signal (§5.1) is raised at the mutator call sites rather than by diffing the layout before and after,
|
||||
// which is what the todo originally proposed: on a diff, both of these read as "that panel closed", and
|
||||
// everything downstream of that signal is destructive — a pty killed, a session released.
|
||||
describe('panel ids do not survive a rearrangement', () => {
|
||||
test('a move mints a new id for the panel it moves', () => {
|
||||
const three = group('g', 'horizontal', [panel('p1', 'x'), panel('p2', 'y'), panel('p3', 'z')]);
|
||||
|
||||
const next = movePanel(three, 'p3', 'p1', 'bottom');
|
||||
|
||||
// `p3`'s app is still on screen. Its id is not: `insertPanel` builds a fresh node via `newPanelFrom`.
|
||||
expect(ids(next)).not.toContain('p3');
|
||||
expect(find(next, 'p1')).toBeDefined();
|
||||
const moved = ids(next).filter((id) => id !== 'p1' && id !== 'p2');
|
||||
expect(moved).toHaveLength(1);
|
||||
expect(find(next, moved[0]!)!.appType).toBe('z');
|
||||
});
|
||||
|
||||
test('a swap leaves both ids in place and exchanges what is in them', () => {
|
||||
const two = group('g', 'horizontal', [panel('p1', 'x'), panel('p2', 'y')]);
|
||||
|
||||
const next = swapPanels(two, 'p1', 'p2');
|
||||
|
||||
// Every id survives, and every app survives — but each id now names a different app than before.
|
||||
expect(ids(next)).toEqual(['p1', 'p2']);
|
||||
expect(find(next, 'p1')!.appType).toBe('y');
|
||||
expect(find(next, 'p2')!.appType).toBe('x');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user