fix(frontend): reset AttentionFeed showAll state bugs#366
Conversation
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 6 issue(s) (2 warning).
server/chat.ts
Clean refactor from library-based to HTTP-based boot context with good test coverage and a well-tested AttentionFeed fix. Main concerns are minor: cosmetic type casts, a tokenBudget default change from 8000→0 in the zero-value fallback, and an untested default path.
- 🔵 style (L383): The union type
Record<string, unknown> | BootContextMessageinsend()is immediately erased bydata as Record<string, unknown>.BootContextMessageis already structurally assignable toRecord<string, unknown>, so the union and cast are both unnecessary — the original signature works as-is.[fixable] - 🔵 style (L919): Double cast
msg as unknown as Record<string, unknown>to storebootContext. SinceBootContextMessageis assignable toRecord<string, unknown>, a single cast (msg as Record<string, unknown>) suffices. Theas unknownintermediate makes it look like there's a real type incompatibility when there isn't one.[fixable] - 🟡 regressions (L919): The cached
bootContextnow includestype: 'boot_context'(fromBootContextMessage), whereas previously it was stored withouttype. Replay inws-handler-v2.ts:285-288does{ type: 'boot_context', ...found.session.bootContext }— the spread overwrites the literaltypewith the same value so it works, but the redundancy is accidental. IfBootContextMessage.typeever diverges from the literal, replay would silently send the wrong type.[fixable] - 🟡 regressions (L114):
FALLBACK_BOOT_CONTEXT.tokenBudgetis0, while the old code always fell back to8000(viaDEFAULT_TOKEN_BUDGET). This changes UI behavior when both ContexGin and the Python script fail — the boot context pill will show 0 budget instead of 8000. Minor since it only affects the double-failure case, but worth confirming this is intentional.[fixable] - 🔵 unsafe_assumptions (L130):
localBootContextFallbackuses dynamicimport('child_process')andimport('util')for modules that are already statically imported at the top of the file (execFileSyncfromchild_process). The dynamic import adds unnecessary complexity — sincechild_processis a Node built-in that's already loaded, a static import would be simpler and more predictable.[fixable]
server/__tests__/boot-context.test.ts
Clean refactor from library-based to HTTP-based boot context with good test coverage and a well-tested AttentionFeed fix. Main concerns are minor: cosmetic type casts, a tokenBudget default change from 8000→0 in the zero-value fallback, and an untested default path.
- 🔵 missing_tests (L178): The
tokenBudgetdefault of8000(line 199 of chat.ts:typeof boot.tokenBudget === 'number' ? boot.tokenBudget : 8000) is not tested. The 'handles empty sources array' test omitstokenBudgetfrom the response but doesn't assert the resultingtokenBudgetvalue. Addingexpect(result.tokenBudget).toBe(8000)would cover this default.[fixable]
| /** Send data via transport (isOpen guard is inside the transport). */ | ||
| function send(transport: SessionTransport, data: Record<string, unknown>) { | ||
| if (transport.isOpen()) transport.send(data); | ||
| function send(transport: SessionTransport, data: Record<string, unknown> | BootContextMessage) { |
There was a problem hiding this comment.
🔵 style: The union type Record<string, unknown> | BootContextMessage in send() is immediately erased by data as Record<string, unknown>. BootContextMessage is already structurally assignable to Record<string, unknown>, so the union and cast are both unnecessary — the original signature works as-is. [fixable]
| if (s) s.bootContext = fallbackPayload; | ||
| } | ||
| })(); | ||
| if (s) s.bootContext = msg as unknown as Record<string, unknown>; |
There was a problem hiding this comment.
🔵 style: Double cast msg as unknown as Record<string, unknown> to store bootContext. Since BootContextMessage is assignable to Record<string, unknown>, a single cast (msg as Record<string, unknown>) suffices. The as unknown intermediate makes it look like there's a real type incompatibility when there isn't one. [fixable]
| if (s) s.bootContext = fallbackPayload; | ||
| } | ||
| })(); | ||
| if (s) s.bootContext = msg as unknown as Record<string, unknown>; |
There was a problem hiding this comment.
🟡 regressions: The cached bootContext now includes type: 'boot_context' (from BootContextMessage), whereas previously it was stored without type. Replay in ws-handler-v2.ts:285-288 does { type: 'boot_context', ...found.session.bootContext } — the spread overwrites the literal type with the same value so it works, but the redundancy is accidental. If BootContextMessage.type ever diverges from the literal, replay would silently send the wrong type. [fixable]
| source: 'local-fallback', | ||
| sourceCount: 0, | ||
| tokenCount: 0, | ||
| tokenBudget: 0, |
There was a problem hiding this comment.
🟡 regressions: FALLBACK_BOOT_CONTEXT.tokenBudget is 0, while the old code always fell back to 8000 (via DEFAULT_TOKEN_BUDGET). This changes UI behavior when both ContexGin and the Python script fail — the boot context pill will show 0 budget instead of 8000. Minor since it only affects the double-failure case, but worth confirming this is intentional. [fixable]
| const { execFile } = await import('child_process'); | ||
| const { promisify } = await import('util'); | ||
| const execFileAsync = promisify(execFile); | ||
| const { stdout } = await execFileAsync('python3', [scriptPath, '--json'], { |
There was a problem hiding this comment.
🔵 unsafe_assumptions: localBootContextFallback uses dynamic import('child_process') and import('util') for modules that are already statically imported at the top of the file (execFileSync from child_process). The dynamic import adds unnecessary complexity — since child_process is a Node built-in that's already loaded, a static import would be simpler and more predictable. [fixable]
| expect(result.fullMarkdown).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('handles empty sources array', async () => { |
There was a problem hiding this comment.
🔵 missing_tests: The tokenBudget default of 8000 (line 199 of chat.ts: typeof boot.tokenBudget === 'number' ? boot.tokenBudget : 8000) is not tested. The 'handles empty sources array' test omits tokenBudget from the response but doesn't assert the resulting tokenBudget value. Adding expect(result.tokenBudget).toBe(8000) would cover this default. [fixable]
AttentionFeed showAll state could stick true when items dropped below the visible threshold or when the section was collapsed and reopened. Add useEffect to auto-reset and reset on collapse toggle. Add 13 component-level tests covering toggle, collapse, shrink, and rendering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
force-pushed
the
session/2026-06-02-a8942eaa8d1e
branch
from
97fc9a0 to
6d498be
Compare
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 2 issue(s).
frontend/src/components/__tests__/AttentionFeed.test.tsx
Clean, well-scoped fix with solid test coverage. Two minor boundary cases in tests worth considering but nothing blocking.
- 🔵 missing_tests (L148): The shrink test goes from 8 → 3 but doesn't test the boundary: items shrinking to exactly DEFAULT_VISIBLE_COUNT (5). Since the condition is
<=, a test with 8 → 5 would verify the boundary is correct and guard against an off-by-one if someone changes it to<.[fixable] - 🔵 missing_tests (L148): No test for the case where items shrink but remain above threshold (e.g. 8 → 6 while showAll is true). In that scenario showAll stays true and the user sees all 6 items with a 'Show less' button — worth a test to document that this is intentional behavior, not an oversight.
[fixable]
| }); | ||
|
|
||
| describe('showAll resets when items shrink', () => { | ||
| it('auto-resets showAll when items drop below threshold', () => { |
There was a problem hiding this comment.
🔵 missing_tests: The shrink test goes from 8 → 3 but doesn't test the boundary: items shrinking to exactly DEFAULT_VISIBLE_COUNT (5). Since the condition is <=, a test with 8 → 5 would verify the boundary is correct and guard against an off-by-one if someone changes it to <. [fixable]
| }); | ||
|
|
||
| describe('showAll resets when items shrink', () => { | ||
| it('auto-resets showAll when items drop below threshold', () => { |
There was a problem hiding this comment.
🔵 missing_tests: No test for the case where items shrink but remain above threshold (e.g. 8 → 6 while showAll is true). In that scenario showAll stays true and the user sees all 6 items with a 'Show less' button — worth a test to document that this is intentional behavior, not an oversight. [fixable]
1 participant
Summary
useEffectto auto-resetshowAllwhenitems.lengthdrops to/belowDEFAULT_VISIBLE_COUNT(5)toggleOpennow resetsshowAlltofalseon collapse, so reopening always shows the truncated viewFollow-up to PR #354.
Test plan
vitest run AttentionFeed.test.tsx)vitest run useAttentionFeed.test.ts)🤖 Generated with Claude Code