perf(web): reuse work log rows during streaming - #8006

💭 I'm examining a performance change to session-logic.ts that caches derived work log entries in a WeakMap, renames activityKind to sourceActivityKind, and switches collapseKey to use a Symbol. This falls outside typical UI/CSS scope, but I want to check for a possible mutation bug — specifically whether the WeakMap cache could return the same entry object that later gets mutated during collapsing, rather than the fresh objects the old code created. I want to trace through whether cached entries stay stable if the underlying activity object is mutated rather than replaced, and whether downstream consumers might mutate the shared entry references now that they're reused across calls instead of freshly created each time. I need to actually read the file to confirm.

1. browse_code — apps/web/src/session-logic.ts

2. browse_code — apps/web/src/session-logic.ts

💭 I'm noticing the returned entries previously had activityKind/collapseKey stripped and sourceActivityKind added via fresh object creation, but now the function returns the derived entries directly from the WeakMap cache instead, which changes the object identity semantics. Fields like toolCallId, isWorkflowCoordinator, and isBackgroundTask were already leaking through before too, so that's not new. The real question I want to dig into is whether caching by activity object identity in the WeakMap is safe — it only works if the activity object itself is treated as immutable, so I need to check if anything mutates it after the fact. If the store uses Immer or Zustand's usual pattern producing new objects, this should be fine, but if anything patches status onto an existing activity object in place, the cache would serve stale data — a genuine regression risk for tool status display. I also want to check whether consumers might mutate the returned entries themselves, since they're shared references now, and whether the derived cache actually delivers a meaningful perf improvement given the underlying pass is still O(n) plus sorting.

3. browse_code — In apps/web/src, how are OrchestrationThreadActivity objects stored and updated in the client state (e.g. session store / query cache)? Are existing activity objects ever mutated in place (e.g. patching payload, summary, tone, or status on an existing activity object) when a tool.updated/tool.completed event arrives, or is a new activity object always created? Show the code that inserts/updates activities and any place that mutates activity fields.

4. git_tools — deriveWorkLogEntries

💭 Since activities are immutable, the WeakMap cache should be safe there. But now I need to check whether shared WorkLogEntry objects get mutated downstream, like in ChatView, since previously each derive call created fresh objects but now they're shared references—I should trace where these entries get pushed or modified, like in collapseDerivedWorkLogEntries. I'm now second-guessing whether removing collapseKey and changing property order breaks any toEqual comparisons in tests, particularly around how symbol properties get compared during equality checks. That specific comparison compares two derived entries from the same underlying data, so the symbols would match on both sides — that's fine. I should check whether any tests compare a derived entry against a plain literal object with toEqual, since that would break; but that's more of a CI concern than a UI consistency issue. Instead I want to focus on whether ChatView's memoization of deriveWorkLogEntries could cause a real regression. I'm now checking whether anything downstream mutates the returned entry objects, since sharing identity across memoized derives could expose subtle bugs if consumers write to them. I'm also noting that making collapseKey a symbol effectively hides it from Object.keys, JSON.stringify, and object spreads — a meaningful but likely intentional change from its previous explicit stripping.

5. git_tools — workLogEntries|WorkLogEntry

6. git_tools — toEqual(

💭 I'm checking whether the WeakMap caching is actually safe here—since toDerivedWorkLogEntry is a pure function of a single activity with no dependency on other activities, caching should hold as long as nothing mutates the returned entry. Now I need to verify that collapseDerivedWorkLogEntries doesn't mutate cached entries. Looking at the collapse function, it consistently builds new objects when merging or updating entries, but there's one spot where it pushes the cached entry object directly into the output rather than cloning it—so if any downstream consumer mutates that entry, the cache could be corrupted across renders. I'm also weighing other risks: cross-thread WeakMap retention seems fine, sorting still costs O(n log n) per render, and the module-level cache being shared with server tests doesn't appear problematic. So the caching change might be sound, but that one uncloned push is worth confirming. Now I'm checking whether the collapse-key symbol leaking into public entry objects causes issues if consumers spread the entry elsewhere—likely harmless since it's just a Symbol tag. I want to verify that collapseKey and sourceActivityKind behave identically to before, and I'm now noticing a real discrepancy: the merge logic that combines previous and next derived entries spreads fields in an order that could let activityKind from the next entry override the previous one differently than before. I'm also weighing whether the flaky, timing-based performance assertion (checking sub-100ms processing for 20k entries) is actually in scope for a UI-consistency review, since it's more of a test-reliability concern than a rendering or styling issue. I'm double-checking whether cached derived entries from the WeakMap could leak across separate calls with different activity sets, but each collapse path builds fresh objects, so the shared cache doesn't cause a correctness problem here. I'm checking whether returning DerivedWorkLogEntry arrays as WorkLogEntry is safe given TypeScript's array covariance, and noting that internal fields like toolCallId were already exposed before through rest spreads. Now I'm reconsidering whether there's a subtler stale-render issue tied to the WeakMap under React strict mode. Re-examining redelivered activities with the same id but a new object, I confirm the old object just gets garbage collected from the WeakMap, which is fine, and the older-page merge dedupes by id correctly. I'm also checking the payload projection tests to see if projected and original activity objects being distinct means caching doesn't interfere, but I'm now considering whether there's a risk of cross-test pollution from the module-level WeakMap. Since objects differ across tests, that persistence isn't a problem, and derivation only depends on the activity object itself, so it's deterministic. Given the PR only touches derivation logic and its tests, it looks like there's no correctness issue here, though I want to confirm the diff really only includes these two files before concluding it's all clear. I should also check whether activityKind is referenced elsewhere in apps/web since the old output kept sourceActivityKind for backward compatibility, and verify nothing downstream mutates a returned WorkLogEntry object in a way that would corrupt the shared cache.

7. git_tools — activityKind

8. browse_code — In apps/web/src, do any consumers of the WorkLogEntry objects returned by deriveWorkLogEntries (e.g. ChatView.tsx, work log row components, timeline builders) mutate those entry objects in place (assigning properties like entry.detail = ..., entry.label = ..., Object.assign(entry, ...), sorting/mutating nested arrays such as entry.changedFiles.push, or storing them in refs and patching)? List any in-place mutation sites of WorkLogEntry or TimelineEntry.entry objects.

💭 Since there's no mutation, caching should be safe here. I'm also checking that computeStableMessagesTimelineRows relies on Equal.equals, which for plain objects without an Equal trait falls back to reference equality — so row memoization benefits from stable entry identity without introducing a regression. The real payoff is that stable identity now lets isRowUnchanged actually detect unchanged rows and reuse them, whereas before every derive created fresh objects and forced re-renders — and since activity data is immutable, there's no risk of identity staying the same while content silently changes. Merged entries from collapsed tool lifecycles still get rebuilt fresh each time, so that path is unaffected. Turning to the timing-based test, I'm weighing whether a wall-clock assertion like checking elapsed performance.now() against a threshold counts as a violation worth flagging — it's a classic source of flaky CI, but it doesn't clearly fall under the UI consistency violations the rules call out, so I need to think about whether the default failure stance still applies here. Checking through the enumerated categories — shared controls, CSS ownership, themes, virtualization, visual preservation, change discipline — none of them clearly cover a flaky performance assertion, so I don't think it counts as a concrete violation under these rules. I'm also double-checking whether sharing cached entry objects across different consumers via the WeakMap introduces any risk, but since components only spread or reference them, this looks safe, and the collapse logic still creates new objects when adding CTA-specific fields. Looking at the reference-stability test around slicing activities, I want to confirm identical entries only recur when the same entry gets pushed again. I'm also checking whether work log entries pass through IPC or structuredClone anywhere, since symbol keys would just get silently dropped rather than error, which seems fine. I want to verify the server test comparing derived arrays with symbol collapse keys still equates correctly, and double-check the before/after comparison around lines 460-461 in case a symbol property difference matters there. There's a real risk: if one activity path produces a collapse key while another derives it differently (say from label/detail instead of toolCallId), the two symbol values could diverge even when visible fields match, breaking a toEqual comparison that previously passed when collapse keys were stripped. This looks like a plausible regression from keeping the collapse key as a symbol in the public output, so I should go look at that specific test.

9. browse_code — apps/server/test/ActivityPayloadProjection.test.ts

10. browse_code — apps/server/test/ActivityPayloadProjection.test.ts

💭 I'm reasoning that since the test checks full derived entries for equality, any difference in collapse key would trace back to differences in the underlying fields already being compared—so the collapse logic wouldn't introduce a new failure mode there, unless the key computation itself normalizes differently than the visible fields do. Given the diff only touches caching/derivation logic with no component or CSS changes, this likely doesn't trigger the UI consistency check at all—the correct verdict is probably "All clear." Still, I want to verify object identity handling: activities may be redelivered as new objects with updated payloads, and since the WeakMap keys on identity, a new object correctly triggers a fresh derivation rather than reusing stale cached data.