Skip to content

fix(ui): snapshot timeline guard values to stop k(...) is not a function - #1212

Open
EricSanchezok wants to merge 3 commits into
devfrom
synergy/session-fe6751
Open

fix(ui): snapshot timeline guard values to stop k(...) is not a function#1212
EricSanchezok wants to merge 3 commits into
devfrom
synergy/session-fe6751

Conversation

@EricSanchezok

Copy link
Copy Markdown
Contributor

Summary

Eliminates the k(...) is not a function crash class that recurs during session switches, complementing #1211 (session data view layer).

  • TimelineDisplayInner now resolves its item once through parseTimelineDisplayItem into a discriminated kind snapshot. Each <Match> is keyed on the stable kind string, so streaming ticks (which replace item references via reconcile) keep the subtree mounted — preserving the non-keyed streaming behavior restored in 57f8da845 — while a genuine kind change (reasoning promotion/collapse, activity mode switch) remounts correctly.
  • Branch children short-circuit on undefined and never re-evaluate a destroyed accessor: a disposed owner degrades to no-match instead of Solid's untrack(switchFunc)() calling a dead memo and throwing k(...) is not a function.
  • SessionTurn's root render is wrapped in an ErrorBoundary (data-slot="session-turn-error", localized session-turn.render-error), so previously uncovered guards (shellModePart, specialUserMessageRenderer, the item()/boundary() Show chain) degrade to a localized turn-level error card. The existing TimelineDisplay item-level boundary stays as the inner defense layer.
  • Records the decision (2026-08-19-timeline-guard-value-snapshot.md) and adds the guard-value invariant to the develop-frontend skill.

Verification

  • New session-turn-dispose-guard.dom.test.ts (3 cases): streaming identity preserved with content updates, kind flips switch branches without uncaught errors, disposing the owner then ticking surfaces no window error.
  • Existing boundary/activity/compact-reasoning/projection DOM tests stay green (identity assertion included).
  • bun run --cwd packages/ui test 542 pass; bun run --cwd packages/app test 1581 pass; both packages typecheck; bun run quality:quick all 14 gates pass (incl. decision:check, doc:check, localization:check with extracted catalogs).

Relation to #1211

#1211 fixes the store-intermediate-state reading 'find'/'filter' crash class via a null-safe session data view layer. This PR fixes the disposed-owner guard re-evaluation class, which is data-independent and not reachable by the view layer. The two are complementary: together they close the session-switch crash family.

EricSanchezok and others added 3 commits August 19, 2026 21:23
TimelineDisplay rendered item branches through non-keyed Switch/Match
guards whose memo accessors could be re-evaluated after their responsive
owner was disposed during a session switch, throwing k(...) is not a
function from Solid's untrack(switchFunc)() guard. Resolve the item once
into a discriminated kind snapshot (parseTimelineDisplayItem), key each
Match branch on the stable kind string so streaming ticks keep the
subtree mounted while kind changes remount, and short-circuit branch
children on undefined so a disposed owner degrades to no-match instead
of calling a destroyed accessor. Wrap SessionTurn's root render in an
ErrorBoundary so residual renderer failures degrade to a localized
turn-level error card; the existing TimelineDisplay item boundary stays
as the inner defense layer.

Covered by session-turn-dispose-guard.dom.test.ts (streaming identity,
kind switching, disposed-owner tick) plus the existing boundary/activity
DOM tests.

Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com>
# Conflicts:
#	packages/ui/src/components/session-turn.tsx
Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com>
@synergy-agent

synergy-agent Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

I've completed the review — full static analysis of the Solid runtime semantics plus empirical verification. Here's my report.


Review: #1212 — fix(ui): snapshot timeline guard values to stop k(...) is not a function

Verdict: sound, no blocking findings. I verified the mechanism against the actual Solid runtime shipped in this repo (solid-js 1.9.10) and ran the test suites.

Mechanism verification (against solid-js/dist/solid.cjs)

  • Kind-stable keyed Match preserves streaming. In Switch (solid.cjs:1480–1509), each conditionValue memo tracks prevFunc() || mp.when, and with keyed the branch's when is the raw value. Since every branch's when derives from the single parsedKind() memo (session-turn.tsx:796), a streaming tick that replaces the item reference but keeps the kind string does not change parsedKind → the selected branch's condition doesn't change → no remount. The new payload flows through the props getter chain (props.itemparsed() → the branch payload getter → child props), so children like ActivityTrace (activity-trace.tsx:293, tracks props.group.steps in a memo) update in place. Node identity preservation is asserted by the new test case 1 and by the existing boundary test's identity assertion (session-turn-timeline-boundary.test.ts:228–238).
  • Disposed-owner path is eliminated. The old crash came from the non-keyed child accessor untrack(switchFunc)()?.[0] !== index … return conditionValue() (solid.cjs:1505–1507). All seven branches are now keyed, so the child receives conditionValue() directly (solid.cjs:1504); a disposed memo's readSignal returns its cached value instead of throwing, and the branch payload getters short-circuit on undefined before any !-asserted consumption (session-turn.tsx:802–806 etc.). The k(...) is not a function signature cannot be produced by this subtree anymore.
  • Parser coverage is complete. parseTimelineDisplayItem handles all six isActivityTimelineItem kinds (session-turn-activity.tsx:335–346) plus guided-user/non-root-user/plain timeline items; activity-boundary degrades to no-match exactly as the previous timelineItem() memo did (session-turn.tsx:546–551 vs. 756). No behavior change for unknown kinds.

Empirical verification

  • New session-turn-dispose-guard.dom.test.ts: 3/3 pass (streaming identity, kind flip, dispose + tick with no window error).
  • Related suites: 49 pass across session-turn-timeline-boundary, session-turn-activity-switch, compact-reasoning, compact-reasoning-settlement, session-turn-activity; 61 pass across session-turn-projection, session-turn-projection-memoization, session-turn-timeline.
  • Full UI suite: 542 pass, 0 fail, exit 0 (matches the PR claim).
  • Typecheck: packages/ui (tsgo --noEmit) and packages/app (tsgo -b) both pass.
  • Gates: decision:check, doc:check, localization:check, test-layout:check all pass; decision record conforms to the implemented/bug-fix/yyyy-mm-dd-topic.md path scheme and format. I did not re-run the packages/app Playwright suite (no Chromium in this environment; the only app-level change is the locale catalogs, which localization:check validated). Note: quality:quick reports secrets:check failed here because the gitleaks binary is absent from this environment — a local tooling gap, not a code failure.

Suggestions (non-blocking)

  1. The new root ErrorBoundary fallback has no direct test. The dispose-guard test covers TimelineDisplay's guard snapshot, and the settlement/activity-switch DOM tests render a real SessionTurn, but none forces an error through the new root boundary to assert [data-slot="session-turn-error"] (session-turn.tsx:1365) renders and sibling rows survive. The PR explicitly claims the previously uncovered guards (shellModePart, specialUserMessageRenderer, the item()/boundary() Show chain) now "degrade to a localized turn-level error card" — per the repo's testing standards ("write a failing behavioral test first for new behavior"), a case that throws inside SessionTurn's render and asserts the turn-level card + no uncaught window error would complete the coverage and guard the fallback against regressions.

  2. Nit: the branch guards double-check p.kind !== kind || p.kind !== "…" (session-turn.tsx:804 etc.), where the second comparison is implied by the first. Harmless and deliberately documented in the decision record — just noting it so it doesn't read as a copy-paste slip.

Synergy: review complete — 0 blocking, 1 suggestion, 1 nit.

@yzxoi

yzxoi commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Thanks for the detailed write-up — I read the decision record and the three DOM tests against the solid-js 1.9.10 runtime. Two things need to change before this can land:

1. The turn-level boundary should log + propagate, not degrade to a card

Swallowing the error into an error card makes this crash family invisible: ErrorBoundary/catchError intercepts the throw, so nothing reaches window.onerror or telemetry. The card also always shows the raw minified message — err?.message || _(SESSION_TURN_DESC.renderError) evaluates renderError only for errors without a message, and this class is TypeErrors with messages, so the localized copy is dead on the main path.

The correct behavior is the one the Part boundary (and skill rule 7) already establishes: log with stable IDs only — no message content, tool payloads, or paths — then let the error propagate to the app-level boundary and render FatalErrorPage, which already has copyable error details, an issue link, and a recovery action. A renderer crash that survives SessionTurn should be fatal-visible, not silently replaced by a card.

solid-js 1.9.10 notes: ErrorBoundary has no onError hook, but throwing from the fallback escapes the boundary (the fallback isn't evaluated inside its own catchError) and reaches the parent boundary; equivalently, use a bare catchError whose handler logs and rethrows.

With log+propagate, session-turn.render-error becomes dead and should be removed, and the decision record + skill rule 8 should document log-then-propagate instead of degrade-to-card.

2. The crash still has no verified reproduction

The decision record explains the mechanism but has no captured stack, and "the disposed memo accessor returns undefined" is inferred rather than evidenced. In solid 1.9.10 a normally disposed memo keeps its cached value and cleanNode detaches it from its sources' observers, so a post-dispose tick notifies nothing — the dispose+tick test cannot exercise the untrack(switchFunc)() path, and by my reading of the runtime all three new cases would also pass against current dev code.

Please attach the field stack (minified + sourcemapped), or add a case that actually fails on current code — e.g. an update enqueued and the owner disposed before flush, or a live observer reading a destroyed guard accessor. Without a failing case we can't tell whether this PR fixes the reported crash or rearranges the same mechanism.

The kind-snapshot core (stable kind key + single parser memo) looks right for keeping streaming remount-free while keying the branches — the concerns are the boundary policy and the missing repro evidence.

@yzxoi

yzxoi commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

CI 全绿但 mergeable=CONFLICTING + DIRTY。dev 近期已合入 #1225/#1226/#1230 等改动,分支需要同步到最新 dev 解决冲突后推送,CI 会重新跑。timeline guard 修复本身与近期 CI 红无关,冲突解决后即可合并。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants