The code cited its plan instead of saying why: `E28 review J4`, `(R6 S3b)`, `S1 justified…`,
`deep-review round 5`. That plan lives in the private ops repo, so for every reader of this one the
citation was noise, and a sentence whose subject was the plan step ("round 2 showed…") hid its
lesson behind an id. Each one is gone and the "why" kept: a bare citation is deleted, a sentence
carrying a lesson is reworded around it ("a later attempt showed…").
Measured (`scripts/audit_metrics.py`, product code): 652 lines in 270 files carried an epic code by
the definition the next commit holds (507 / 241 by the old, two-digit-only one) -> 0. Across all
code, tests, scripts and configs: ~1100 epic-code lines plus ~320 sprint/round ids.
Behaviour unchanged, checked mechanically: every changed .py file has the same AST once docstrings
are blanked; css/sh/toml/yml/json files are identical once comments are stripped; the only
non-comment changes in TS/JS are test titles, one description string in a contract table, and five
ESLint messages, whose text is all the gates snapshot diff holds. Not touched: Markdown (docs and
the changelog keep their history), Alembic migrations, TV episode notation (`S2 · E5`), DESIGN.md
ids (`C1`, `B4`, `S3h`), tracker ids (`SEC-16`, `DX-17`).
One stale claim fixed on the way: test_hls_reaper's docstring said the player checkpoints "every
10 s whether or not it is playing", which plex/stream.py calls wrong; it now says what the code does.
E32 S3. The compiler skipped 93 functions; it now skips 29 (try/finally shapes, two suppressions,
VirtualGrid). react-hooks/refs hits 245 -> 31.
- Popover engine: `ref={pop.refs.setReference}` made the compiler treat the whole popover handle as
a ref, so every component holding one was skipped (21 call sites, the engine and its surface
among them). The setter is destructured now; the same for useScrollFade and useReturnFocus.
- Render-phase `ref.current = x` writes moved to `useEffectEvent` where only effects read them,
else to the new `useLatest` (written in a layout effect) / `useStableCallback`.
- FocusSafeButton (under every Button) read the focused element during render; it reads it through
useSyncExternalStore, which React asks on every render instead of a cached memo.
Bugs this surfaced, fixed:
- Combobox: a chip could keep showing its raw id. The label cache was a ref an effect filled after
the render that needed it; it is state merged during render now (test seen RED).
- useUndoable rendered from a ref plus a forced re-render; compiled, that memoizes a stale value.
The history is state now, mirrored in a ref for the stable callbacks.
- ConfigPanel and the saved-views editor re-seeded their drafts in an effect, painting the stale
draft for a frame; both adjust during render against the remembered key.
- Toaster's handover is written after commit instead of during render; a new test pins that the
inherited set is taken once, at the takeover (seen RED on a per-render re-read).
- IssueComposer mutated a ref prop as a one-shot focus token; it is plain state.
- PlexBrowse wrote its info-scroll memory from an effect declared before the ref, past its own
"one writer" rule; the writer comes first now and the effect goes through it.
The compiler ratchet drops to the measured 29 and now also requires FocusSafeButton and the popover
engine to compile.
Ten more suppressions, and React 19 turns out to supply the missing primitive.
useEffectEvent (stable since React 19.2, which S1 brought in) is the canonical answer to the
"bind the listener once, but read the latest values" pattern these hooks were disabling the
checker for. Three of playerModalHooks' four escape hatches become honest dep lists: the
wheel-volume handler, the native-controls handover, and the fullscreen-change sync. The
contract is no longer prose — the compiler enforces it.
The fourth stays, deliberately, and now says why at the site: the YouTube player's lifecycle
effect must run once per video (re-running destroys and rebuilds the iframe), and YT.Player
takes its callbacks as an object literal, which an effect event may not be passed into.
Fixed, not suppressed:
- useDismiss captured onClose when the popover opened, so a caller that re-rendered with a
different handler while it stayed open kept getting the OLD one called. The disabled dep list
asserted the opposite ("read fresh from this run's closure"). Both the handler and the ref
list are read through refs now; three tests, the two that matter mutation-checked.
- usePref wrapped its seed in a useCallback whose dep list had to be excused. useState calls a
lazy initializer exactly once, so the memo was saving nothing; inlined.
- useCardPager clamped an out-of-range page from an effect. Adjusted during render instead —
React discards that pass rather than committing it, so no out-of-range page is ever painted.
- plexPlayerHooks' auto-skip countdown reads its marker from the ref that already existed, so
the dep list names its real inputs. That also made the set-state-in-effect finding go away:
the progress is genuinely timer-driven, and the checker can now see that.
- useUrlMirror and useChipOverflowFit only needed their stable refs listed.
Two test-harness suppressions (a render-phase probe ref, a module-level setState handle) moved
inline with the reason they exist: both exist to observe a hook from outside without changing
what is being measured.
Suppressions 99 -> 89; exhaustive-deps 35 -> 27.
Round 4 was targeted at the surfaces rounds 2 and 3 changed. 29 findings raised,
26 survived deduplication, 12 survived three adversarial readings each.
`set_delivery` renewed the claim but never checked it was still ours, while its
sibling `release_send` always did. `expire_on_commit` is off, so the token it
re-stamped came from the caller's own memory, not the row: a send that outlived
its 600 s lease came back, overwrote the take-over send's committed links with a
full-column write, and put its own dead token back. It now takes the token, reads
the row's claim with a targeted scalar (not a refresh, which would discard the
`issue_url` assigned one line earlier) and raises `SendClaimLost` rather than
writing. An issue created by a send that had already lost its claim is logged with
its number — that record is the only one that will exist.
The tracker's settings are read above the pre-call checkpoint instead of inside
`_client`. Reading them there was a SELECT one line after the commit that was
meant to end the transaction, and held it — idle, snapshot pinned — across the
repository check and the create. The comment and the module docstring said
otherwise; measuring it in pg_stat_activity is what settled it. Two more comments
were saying things the code had stopped doing: the `**state` spread does not carry
the claim (`set_delivery` strips and re-stamps it), and react-query does not
debounce the preview — the editor being unmounted is what paces it.
The composer stopped stranding the focus on `<body>`: a send that creates the
issue and then fails on an attachment unmounts the button that was just pressed,
and only the success path handed the focus on. Export no longer saves into a
refusal it does not need — the zip is the way out of a stuck send, and its
pre-save was the one step that could not survive one. The delivery banner
announces through a role instead of an `aria-live` created with its own message.
`historyState` returns a union, so `chipTone` matching on the same keys is checked
by tsc rather than by hope.
PlexPlayer was the only full-screen overlay in the tree with no focus containment
at all — Tab walked out of an opaque player into a page nobody can see. It now
carries the shared trap. Closing that hole exposed the one underneath it:
`useDismiss` registered as a layer that traps the Tab while installing no trap,
so the player stood its own down for its own menus. Measured in a real browser:
30 Tabs with the settings panel open, 9 escapes into the nav rail, and 0 after.
Escape ordering is unchanged — the panel first, then the player.
Three tests that did not hold their line: the advisory lock's call site (deleting
it left the suite green), the trail assertion whose `all()` was vacuous over an
empty filter, and a `layerStack` assertion the stand-in dialog already forced.
All eleven executable fixes mutation-checked red for the right reason; the two
comment corrections have nothing to pin.
Replace the three mutually-unaware Escape systems — Modal's private modalStack,
the popovers' document listeners with a blanket stopPropagation, and the two
players' modalCount() checks — with one shared lib/layerStack.ts. Every open
dismissable layer registers in open order; a layer acts on Escape only when it
is topmost, and reads anyLayerAbove() to decide whether to defer. Coordination
is now a stack lookup, not event-propagation order, so it no longer depends on
which listener happens to fire first.
- Modal + useDismiss register via the stack; useDismiss drops the blanket
stopPropagation (its documented side effect: it swallowed a player's own
Escape while any popover was open).
- Both players register as a layer and defer via anyLayerAbove() instead of
modalCount(); PlayerModal checks it before its maximise/fullscreen steps.
A player MENU is a layer too, so it closes first and the next Escape reaches
the auto-skip-cancel — fixing the documented PlexPlayer bug.
- AnchoredPopover routes non-hover Escape through the stack (floating-ui's own
escapeKey off there) and registers as a layer, so a player/modal underneath
defers once S4 migrates the in-player menus. FloatingTree still owns
popover-vs-popover ordering; the stack adds the cross-type ordering.
Review found two low-severity items:
- toast: armDismiss now always clears the prior timer (and arms only when duration is
truthy), so a coalesced repeat that upgrades a toast to requiresInteraction no longer
lets the original timer fire and dismiss it. Was a pre-existing gap the C-3.25 change
hadn't closed.
- overlay: documented the known limitation of useDismiss's blanket stopPropagation — a
PlexPlayer menu open during an auto-skip countdown swallows the Escape that would also
cancel the skip. Proper fix needs R8's shared layer-registry (topmost-only dispatch),
not a broad stopPropagation; accepted until then.
Three player/overlay interaction fixes (PlexPlayer changes share a file, hence one
commit):
- U-C (Escape double-close): useDismiss's Escape now stopPropagation()s (its document
listener bubbles before a player's window listener, so one Escape closes only the
popover, not the popover AND the player). Modal exports modalCount(); both players'
Escape defers when a Modal is open above them, so Escape closes the dialog, not the
player under it and then the dialog.
- U-3.1.4: PlexPlayer's Space now defers to a focused BUTTON/A (e.g. "Skip intro")
instead of toggling playback — matching PlayerModal.
- U-3.3.12: the audio/subtitle menus used theme-adaptive glass-menu + forced
text-white (white-on-white in light theme); they now use the player's own explicit
dark panel (border-white/15 + bg-neutral-900/95), readable in both themes.
lib/useDismiss.ts replaces the identical mousedown-outside + Escape effect that
DataTable (filter popover), Channels (tag picker) and AddToPlaylist each hand-rolled.
Positioning stays with each caller (it genuinely varies); AddToPlaylist keeps its
own resize/scroll reposition effect.
peter