PR review

Renders Finish-to-Start dependency arrows on the scheduler timeline. The implementation is careful and unusually well-tested; nothing is merge-blocking. The pure-helper extractions (computeElementPositionInCollection, computeOccurrencesFirstIndexLookup, rowGeometry) are arithmetically identical to the code they replace, and the model-anchored/scroll-isolated arrow computation holds up. Highest-severity issues are one cosmetic edge-clipping bug and a couple of untested edge cases. I could not run the test suite in this environment (Node version mismatch), so test findings are from static reading only.

Bugs (4)

1. 🟠 Arrow segments at the timeline's left/right edge are clipped by the SVG's default overflow: hidden

Location: packages/x-scheduler-premium/src/event-timeline-premium/content/timeline-dependency-arrows/EventTimelinePremiumDependencyArrows.tsx:25 (styled SVG sets no overflow), driven by dependencyArrowGeometry.ts:322-323 (exitX = source.x + STUB, entryX = target.x - TARGET_CLEARANCE).

const DependencyArrowsSvg = styled('svg', { name: 'MuiEventTimeline', slot: 'DependencyArrows' })(
  ({ theme }) => ({ position: 'absolute', top: 0, left: 'var(--title-column-width)', /* no overflow */ }),
);

The <svg> root defaults to overflow: hidden, and the viewBox x-range is exactly 0 … eventsWidth. A backward S-route into an event that starts within ~12px of the timeline start makes entryX = target.x - 12 negative, so the entry elbow — and most of the arrowhead (which occupies [target.x-7, target.x]) — is drawn at x < 0 and clipped. The symmetric case: a predecessor ending at the very right edge pushes exitX = source.x + 8 > eventsWidth, clipping the exit stub.

Failure scenario: The first task of a day (starts at hour 0) has a predecessor it depends on; its incoming arrowhead is cut off and the arrow appears to point at nothing.

Fix: Set overflow: 'visible' on DependencyArrowsSvg. This matches the stated design intent ("below the pinned title column, which covers the arrows on horizontal scroll") — the title column's higher z-index still covers any overflow into the x < 0 region.

2. 🟡 ! non-null assertion is a latent crash, safe only by a cross-selector invariant

Location: packages/x-scheduler-internals-premium/src/event-timeline-premium-selectors/eventTimelinePremiumDependencySelectors.ts:52

const title = processedEventLookup.get(dependency.source)!.title;

Verified safe today: activeModelListSelector only keeps dependencies whose source and target both classifyDependencyEvent(...) === 'ok', which requires presence in processedEventLookup. Flagging only because the assertion silently depends on both selectors reading the same lookup; a future refactor feeding this a differently-filtered list would throw on .title.

Fix: processedEventLookup.get(dependency.source)?.title ?? '' (or keep the ! and add a comment pinning the invariant — the comment is already there, so this is optional).

3. 🟡 Transient resources ↔ rowsMeta.positions desync drops an arrow for one frame

Location: dependencyArrowGeometry.ts:206-208

When a resource is added, groupedByResourceList (driving anchors) can update a commit before the virtualizer's rowsMeta.positions grows; for that render rowPositions[anchor.rowIndex] is undefined and the dependency is skipped. Self-heals on the next commit. The == null guard correctly treats row-0 position 0 as valid, so no off-by-one — purely a one-frame flicker.

4. ℹ️ Self-dependency (source === target) produces undefined geometry

computeDependencyArrows has no guard for dependency.source === dependency.target. Both anchors resolve to the same occurrence → forwardX < 0, same y → it falls into the backward S-route and draws a loop around the event. Not reachable through any current API path, but the data model does not reject it. Consider skipping such dependencies (also see Tests #1).

Tests (4)

1. 🟠 Self-dependency behavior is unpinned

Location: dependencyArrowGeometry.test.ts

No test covers { source: 'a', target: 'a' }. The code produces a loop-shaped S-route with no assertion on whether that is intended or should be skipped — a malformed-but-plausible input yields undefined-by-spec output.

2. 🟡 getVisibleFractionRange has no JSDOM unit test

Location: getVisibleFractionRange.ts

The +1 column offset and Math.max(0, …) clamp are only exercised by browser-only (skipIf(isJSDOM)) integration tests; computeDependencyArrows doesn't use the helper. In a JSDOM run the culling arithmetic is entirely uncovered. A 3-line direct unit test would pin it cheaply.

3. 🟡 Weak "arrow follows the event" assertion

Location: dependencyArrows.EventTimelinePremium.test.tsx (should update the arrow when the predecessor event moves)

The test calls store.updateEvent(...) and asserts only updatedD !== initialD. No real drag/resize gesture is exercised (despite the demo enabling both), and "different" would pass even if the arrow recomputed in the wrong direction. Assert a concrete property of the new path.

4. 🟡 Untested route branches

Location: dependencyArrowGeometry.test.ts

Three reachable branches lack coverage: the forward-but-too-close S-route at a different height (0 < forwardX < STUB+CLEARANCE, all existing S-route tests are backward), the single-candidate forward elbow (lateTurnX <= earlyTurnX, i.e. forwardX at the ~20px boundary), and the eventsWidth <= 0 early return. Each is a one-case addition. (Minor: buildRoundedOrthogonalPath < 2 points → '', formatCoordinate fractional rounding, and empty-input computeOccurrencesFirstIndexLookup are also unpinned — ℹ️.)

Simplifications (2)

1. 🟡 EMPTY_TITLES reinvents the shared EMPTY_ARRAY sentinel

Location: eventTimelinePremiumDependencySelectors.ts:30

const EMPTY_TITLES: readonly string[] = [];

The codebase already exports EMPTY_ARRAY from @base-ui/utils/empty for exactly this "stable empty ref from a selector" purpose (used in schedulerOccurrenceSelectors, schedulerResourceSelectors, and the sibling dependency-utils.ts). EMPTY_ARRAY is readonly [] and satisfies readonly string[]. Import it and delete the bespoke sentinel; the same-instance test passes identically.

2. 🟡 Three exported geometry constants have no external importer

Location: dependencyArrowGeometry.ts:20, 24, 39

DEPENDENCY_ARROW_STUB, DEPENDENCY_ARROW_CORNER_RADIUS, and DEPENDENCY_ARROW_DETOUR_CLEARANCE are exported but grep confirms no importer anywhere (not even tests) — only DEPENDENCY_ARROWHEAD_SIZE is consumed by the component, and DEPENDENCY_ARROW_TARGET_CLEARANCE is deliberately un-exported. Drop export from the three internal-only constants to keep the internal slice's surface consistent. (ℹ️: computeDependencyArrows also recomputes Math.min/max(sourceAnchor.rowIndex, targetAnchor.rowIndex) twice — hoist once; and the two computeOccurrences* helpers share an identical 3-line body that could be one internal function with thin field-pickers.)

Docs (1)

1. 🟡 rowGeometry.ts comment attributes the font-size to the wrong root

Location: packages/x-scheduler-premium/src/event-timeline-premium/content/rowGeometry.ts:15

// `em` in the grid track resolves against the EventsCell's font-size, which
// inherits `theme.typography.body2.fontSize` from the Content root.

The EventTimelinePremiumContentRoot (slot Content) sets no fontSize; it is set on the outer EventTimelinePremium root (EventTimelinePremium.tsx:86). That's precisely why the demo and test host must re-declare fontSize: '0.875rem' — Content does not provide it. Should read "from the EventTimelinePremium root". (Inaccuracy was moved verbatim from the old inline helper, but now lives in new code.)

ℹ️ Also: the maxSpan: 1 qualifier in computeOccurrencesFirstIndexLookup's JSDoc is misleading (firstIndex is independent of maxSpan); the demo's // One event pair per arrow shape over-promises (9 events / 6 deps / 4 shapes); and value={store as any} in the demo and test harness is the file's only unexplained cast.

Verdict

Approve after nits — no critical or high-severity issues; the edge-clipping arrow bug (#1) and the self-dependency gap are the only items worth addressing before merge, and both are low-frequency. Test suite could not be executed here, so the test-gap findings rest on static reading.

🤖 Review generated with Claude Code

PR review

The pure-geometry extractions (computeElementPositionInCollection, computeOccurrencesFirstIndexLookup, rowGeometry, getVisibleFractionRange) are behavior-preserving and well covered, and the arrow-routing engine is thoroughly unit-tested. One merge-relevant issue survives: the new accessibility increment leaks its "Depends on X" text into the successor event's accessible name (not just its description), because the event names itself with a self-referential aria-labelledby, so screen readers announce the dependency twice. Everything else is clean; nothing else blocks.

Bugs (1)

1. 🔴 The "Depends on X" description also pollutes the event's accessible name and is announced twice

Location: packages/x-scheduler-premium/src/event-timeline-premium/content/timeline-event/EventTimelinePremiumEvent.tsx:216

const sharedProps = {
  id,
  // ...
  'aria-labelledby': `${ariaLabelledBy} ${id}`, // self-reference → name from own subtree
};
// ...
<TimelineGrid.Event
  {...sharedProps}
  aria-describedby={dependsOnTitles.length > 0 ? `${id}-dependencies` : undefined}
>
  <EventTimelinePremiumEventLinesClamp>{occurrence.title}</EventTimelinePremiumEventLinesClamp>
  {dependsOnTitles.length > 0 && (
    <span id={`${id}-dependencies`} style={visuallyHidden}>
      Depends on {dependsOnTitles.join(', ')}
    </span>
  )}

The event computes its accessible name from aria-labelledby={${ariaLabelledBy} ${id}}, where ${id} points at the event element itself — so the name is derived from the event's own text subtree ("name from content"). The new description <span> is hidden with visuallyHidden (a clip style), not aria-hidden, so it stays in the accessibility tree and is folded into that name. The same span is then referenced again by aria-describedby.

Net result for a successor: the dependency text lands in both the name and the description. A screen reader announces roughly "9 AM, Event B Depends on Event A, Event C" (name) followed by "Depends on Event A, Event C" (description) — the predecessor list is spoken twice, and the event's name is no longer just its title.

Failure scenario: With NVDA/VoiceOver on the dependency demo, focusing "Event B" (which depends on A and C) reads the predecessors twice and mixes them into the event's name, instead of announcing the title as the name and the dependency once as a supplementary description.

Fix: Keep the span out of the self-referential name while preserving the description. Add aria-hidden to the span — an element explicitly targeted by aria-describedby is still used for the description even when aria-hidden, but an aria-hidden descendant is excluded from the name-from-content traversal:

<span id={`${id}-dependencies`} style={visuallyHidden} aria-hidden>
  Depends on {dependsOnTitles.join(', ')}
</span>

(Alternatively, render the description node as a sibling outside the aria-labelledby-referenced element.) Worth confirming with a browser test that reads the computed name/description rather than textContent.

Tests (1)

1. 🟠 The a11y test asserts textContent, so it cannot catch the name pollution / double announcement

Location: packages/x-scheduler-premium/src/event-timeline-premium/tests/dependencyArrows.EventTimelinePremium.test.tsx:2635

expect(document.getElementById(descriptionId)!.textContent).to.equal(
  'Depends on Event A, Event C',
);

The test only checks that the hidden span's textContent is correct and that aria-describedby is wired up. It never checks the successor's accessible name, so the leak in finding #1 (dependency text appearing in the name and being announced twice) passes unnoticed. This is exactly the assertion that would have flagged the bug.

Failure scenario: A future refactor that fixes or re-breaks the name/description split produces no test signal, because coverage stops at the span's text rather than the resolved name/description a screen reader would use.

Fix: In a browser-mode test, assert the event's accessible name is just its title (e.g. screen.getByRole(..., { name }) scoped to the title, or compute the name) and that the description is present exactly once — pinning both sides of the name/description split.

Simplifications (0)

No findings.

Docs (0)

No findings.

Verdict

Request changes — the accessibility increment double-announces dependencies and pollutes the event's accessible name; the fix is a one-line aria-hidden plus a stronger test. The geometry/refactor work is solid and the moved exports resolve cleanly. (Severity reflects that this is the a11y behavior the PR ships; note the feature is not yet publicly reachable, so you may calibrate down if you prefer to land it and fix at the public flip — but the fix is trivial enough to do now.)

🤖 Review generated with Claude Code

I've completed a thorough review of the diff — the geometry core (dependencyArrowGeometry.ts), the overlay component, the pure-helper extractions, the selectors, and the a11y increment — and cross-checked the key invariants against the surrounding code. Here is the report.

PR review

Pure-rendering FS dependency arrows for the premium timeline. The diff is large but disciplined: the three refactors (computeElementPositionInCollection, rowGeometry, getVisibleFractionRange) are behavior-preserving extractions, and the new geometry/overlay/a11y code is backed by unusually complete unit and browser tests. I verified the load-bearing invariants (row-order alignment between groupedByResourceList and the virtualizer rows, the "active dependency always resolves" precondition behind the non-null assertion, the aria-describedby/aria-hidden name-vs-description split, and edge clamping in every route shape). I found nothing merge-blocking and no actionable correctness, test, or docs defects.

Bugs (0)

No findings.

Specifically checked and cleared:

• Row-order alignment. EventTimelinePremiumDependencyArrows anchors on resources[rowIndex] / rowPositions[rowIndex]; the virtualizer's rows are built from the same groupedByResourceList selector in the same order (EventTimelinePremiumContent.tsx:678), so indices line up. The transient resources/rowsMeta desync is guarded in buildArrow (skips when rowPositions[...] == null).
• Non-null assertion in activeSourceTitlesByTargetSelector (processedEventLookup.get(dependency.source)!) is safe: activeModelListSelector already filters to dependencies where both endpoints classifyDependencyEvent === 'ok'.
• aria-describedby after {...sharedProps} does not clobber anything — TimelineGrid.Event sets no aria-describedby, and sharedProps carries none; passing undefined when there are no predecessors is equivalent to omitting it.
• A11y name/description split — the visually-hidden Depends on … span is aria-hidden (excluded from the self-referential name-from-content) yet still resolved by aria-describedby; both browser assertions confirm it.
• computeElementPositionInCollection extraction is byte-for-byte equivalent to the old inline EventList math (fractionEnd = position + duration === clamp(endMinutes)/totalMinutes), and default dayStartMinute/dayEndMinute match the old hook destructure.
• Edge clamping (S-route exit/entry riding over events at x=0 / x=eventsWidth) and division-by-tickCount paths are covered/guarded; tickCount is always positive and the layer early-returns on eventsWidth <= 0.

Tests (0)

No findings. Coverage is strong across both route-shape branches (including S-detour above/below source and the same-height case), obstacle-avoidance tie-breaks, multi-row fan-out keys, culling, virtualization, and a browser anchor-alignment pin against real layout. The author's TODO(multi-resource rendering) correctly notes the one integration gap (per-appearance rendering), which is already covered at the unit level.

Simplifications (0)

No findings. The extractions actively reduce duplication (getVisibleFractionRange now shared between EventList and the overlay; rowGeometry shared with the virtualizer height path).

Docs (0)

No findings. The hardcoded English "Depends on X" string and the visual constants are already flagged with TODOs and in the PR's own "Flagged for review" list; the S-route not participating in obstacle avoidance is likewise author-acknowledged and reasonable for v1.

Verdict

Approve — no correctness, accessibility, test, or docs defects survived verification; the remaining rough edges (S-route obstacle avoidance, multi-resource anchoring, localization) are explicitly tracked as follow-ups, not defects in this diff.

🤖 Review generated with Claude Code