Repository navigation
refactor(AppShell): main's ref is the one owner of its overflow observation and keyboard delegation - #9262
Conversation
…vation and keyboard delegation An unmount Effect repeated the release main's callback ref already performs: React detaches the ref on unmount, and the null call stops the overflow observation and releases the keyboard delegation. The Effect is removed, so each resource has one lifecycle owner. A test pins that unmount still releases both. No behavior changes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC |
|---|---|---|
| AppShell · Storybook | 71.5 kB (−19 B) · 12.0 kB CSS (0) | 1,011 (−9) |
Package size: measured; main has no measurement of the merge base to compare yet.
API changes: core +2 · ~1 · −0 (report stale: run pnpm api:report)
3 API changes (advisory)
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | core |
. · Types · interface SearchContext |
interface SearchContext<T extends SearchableItem = SearchableItem> |
import type {SearchContext} from '@astryxdesign/core' |
| + | core |
./Typeahead · Also exported from an earlier entry point · SearchContext |
SearchContext (.) |
import {SearchContext} from '@astryxdesign/core/Typeahead' |
| ~ | core |
. · Types · interface SearchSource |
search: (query: string) => Promise<T[]> | T[] → search: (query: string, context?: SearchContext<T>) => Promise<T[]> | T[] |
import type {SearchSource} from '@astryxdesign/core'; const x: SearchSource = {search: …} |
Not run on this PR: add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. Removing the unmount Effect is right: main's ref already stops the overflow observation and releases the keyboard delegation when React detaches it. The new unmount test only proves the first half: its keydown/focusin assertions are also met by the focus-ring Effect's own cleanup, so it still passes when the delegation is never released. Please record the exact listeners added while main becomes a scroll region and assert each one is removed on unmount (suggested code on the test).
[Automated review]
| expect(documentRemove).toHaveBeenCalledWith( | ||
| 'keydown', | ||
| expect.any(Function), | ||
| true, | ||
| ); | ||
| expect(documentRemove).toHaveBeenCalledWith( | ||
| 'focusin', | ||
| expect.any(Function), | ||
| true, | ||
| ); |
There was a problem hiding this comment.
These two assertions are also satisfied by the focus-ring Effect's cleanup (AppShell.tsx lines 1142–1144 remove keydown and focusin with capture true), so this test still passes if syncMainDelegation() never releases the delegation on detach. Recording the listeners added while main becomes a scroll region and expecting each exact one to be removed catches it:
const documentAdd = vi.spyOn(document, 'addEventListener');
setMainGeometry(main, 1200);
remeasure();
const added = documentAdd.mock.calls.filter(
([type]) => type === 'keydown' || type === 'focusin',
);
const documentRemove = vi.spyOn(document, 'removeEventListener');
unmount();
for (const [type, listener, options] of added) {
expect(documentRemove).toHaveBeenCalledWith(type, listener, options);
}
…he changeset The unmount test recorded only event types, which the focus-ring Effect's own cleanup also removes. It now records the exact listeners the delegation adds while main becomes a scroll region, and the overflow observation's resize listeners, and requires each to be removed with the same handler. The test fails when the delegation is not released.
There was a problem hiding this comment.
Approval recommended. Deleting the unmount Effect leaves main's callback ref as the only release path, which is what the one-owner rule for listeners and observers asks for, and nothing changes for AppShell users. The rewritten unmount test now fails if either the overflow observation or the keyboard delegation is not released when main detaches, so the earlier test gap is closed and nothing more is needed.
[Automated review]
Maintainer impact
Nothing changes for AppShell's users. A maintainer reading AppShell sees one owner for main's overflow observation and keyboard delegation: main's callback ref. Before this change a second owner, an unmount Effect, released the same resources again. The Effect's cleanup always found them already released, so it did nothing, but it made the lifecycle look split between two owners.
Intended invariant
architecture:react-component-runtime(current):nulldetachment and replacement.Main's ref already owns attach, replacement and detach (#8111), including the detach React performs on unmount.
Change
AppShell.tsx: the unmount-only Effect that stoppedmainOverflowRefand releasedmainDelegationRefis removed.setMainNode(null), which React calls on unmount, already stops the overflow observation, andsyncMainDelegation()releases the delegation.AppShell.test.tsx: a new case makes main overflow so the keyboard delegation attaches, then unmounts. It records the exact listeners the delegation adds (keydown,keyup,pointerdown,focusin, capture) and the overflow observation's windowresizelisteners, and requires each one to be removed with the same handler, so the focus-ring Effect's own cleanup of the same event types cannot stand in. It fails when the delegation is not released..changeset/appshell-main-release-on-detach.md: a[chore]patch note; no behavior changes.From a review note by vjeux on the fill-height keyboard work.
Evidence
pnpm vitest run packages/core/src/AppShell/: 2 files, 68 tests pass, including the new unmount case.Scope