packages/app/e2e/session/session-renderer-diagnostics.spec.ts (1)

167-184: ⚡ Quick win

Avoid binding this E2E to ScrollView CSS internals.

timelineThumbBox() reaches into .scroll-view and .scroll-view__thumb, so a harmless class rename in packages/ui will break this spec. Please expose a data-component/data-action hook for the scrollbar thumb and query that instead.

As per coding guidelines, use data-component, data-action, or semantic roles for selectors instead of CSS class names or IDs.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/app/e2e/session/session-renderer-diagnostics.spec.ts` around lines
167 - 184, The test helper timelineThumbBox currently depends on internal CSS
classes (".scroll-view" and ".scroll-view__thumb"); update the UI ScrollView
component to add a stable data attribute (e.g. data-component="scroll-view" on
the scroll container and data-action="scroll-thumb" on the thumb) and then
change timelineThumbBox to query for those data attributes (use the existing
scrollViewportSelector param for finding the viewport/root and replace
".scroll-view__thumb" with the new data-action selector) so the spec relies on
stable data hooks instead of implementation CSS classes.

packages/app/src/pages/session/use-session-timeline-interaction.ts (2)

77-83: 💤 Low value

Consider also detaching the controller on owner cleanup.

onCleanup cancels a pending recovery frame but never invokes scrollController.detach({ ... }). The session-change effect at lines 62-75 properly detaches the previous controller, but on Solid owner disposal (component unmount, route change) the controller silently goes out of scope without emitting the owner_detached diagnostic that the controller is otherwise designed to produce. If the diagnostic timeline is used to verify lifecycle, missing detach events on unmount will look like the previous owner is still active.

♻️ Proposed fix

   onCleanup(cancelRecoveryFrame)
+  onCleanup(() => {
+    const owner = scrollController.state()
+    scrollController.detach({
+      sessionOwner: owner.sessionOwner,
+      viewportOwner: owner.viewportOwner,
+    })
+  })

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/app/src/pages/session/use-session-timeline-interaction.ts` around
lines 77 - 83, onCleanup currently only cancels the pending recoveryFrame but
does not detach the scroll controller, so the controller never emits its
owner_detached diagnostic on component unmount; update the cleanup handler (the
function referenced by onCleanup/cancelRecoveryFrame) to also check for the
scrollController instance and call its detach method (e.g.,
scrollController.detach(...)) with the appropriate owner_detached
reason/metadata used elsewhere so the controller emits the same diagnostic on
owner disposal as it does during session-change handling.

---

38-75: ⚡ Quick win

Use the factory for the initial controller too.

The inline controller construction at lines 39-48 duplicates the body of createScrollController at lines 51-60 verbatim. Any future change (e.g., new option, different diagnostic transform) has to be made in two places and is easy to miss. Reusing the factory for the initial seed eliminates that drift.

♻️ Proposed fix

-  let scrollController = createSessionTimelineScrollController({
-    sessionOwner: input.sessionKey(),
-    viewportOwner: `timeline:${input.sessionKey()}`,
-    routeSessionID: input.routeSessionID(),
-    visibleSessionID: input.sessionID(),
-    timelineSessionID: input.sessionID(),
-    emitDiagnostic: (event) => {
-      void emitRendererDiagnostic(event).catch(() => {})
-    },
-  })
-
-  const createScrollController = () =>
-    createSessionTimelineScrollController({
-      sessionOwner: input.sessionKey(),
-      viewportOwner: `timeline:${input.sessionKey()}`,
-      routeSessionID: input.routeSessionID(),
-      visibleSessionID: input.sessionID(),
-      timelineSessionID: input.sessionID(),
-      emitDiagnostic: (event) => {
-        void emitRendererDiagnostic(event).catch(() => {})
-      },
-    })
+  const createScrollController = () =>
+    createSessionTimelineScrollController({
+      sessionOwner: input.sessionKey(),
+      viewportOwner: `timeline:${input.sessionKey()}`,
+      routeSessionID: input.routeSessionID(),
+      visibleSessionID: input.sessionID(),
+      timelineSessionID: input.sessionID(),
+      emitDiagnostic: (event) => {
+        void emitRendererDiagnostic(event).catch(() => {})
+      },
+    })
+
+  let scrollController = createScrollController()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/app/src/pages/session/use-session-timeline-interaction.ts` around
lines 38 - 75, The initial scrollController is constructed inline duplicating
createScrollController; instead call createScrollController to seed
scrollController so there's a single factory point. Replace the inline
createSessionTimelineScrollController usage that assigns scrollController (the
block creating recoveryFrame and scrollController) with: let scrollController =
createScrollController() (keeping recoveryFrame), ensuring the
createScrollController function (which wraps
createSessionTimelineScrollController and the emitRendererDiagnostic wrapper) is
the sole place constructing the controller referenced by createEffect (on
input.sessionKey()/input.sessionID()).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes