diff --git a/backlog/tasks/task-205 - Address-PR-19-Claude-frontend-review-follow-ups.md b/backlog/tasks/task-205 - Address-PR-19-Claude-frontend-review-follow-ups.md new file mode 100644 index 0000000..77fe71e --- /dev/null +++ b/backlog/tasks/task-205 - Address-PR-19-Claude-frontend-review-follow-ups.md @@ -0,0 +1,62 @@ +--- +id: TASK-205 +title: 'Address PR #19 Claude frontend review follow-ups' +status: Done +assignee: + - codex +created_date: '2026-03-20 02:41' +updated_date: '2026-03-20 02:46' +labels: [] +milestone: m-1 +dependencies: [] +references: + - stats/src/components/vocabulary/VocabularyTab.tsx + - stats/src/hooks/useSessions.ts + - stats/src/hooks/useTrends.ts +priority: medium +--- + +## Description + + +Assess Claude's latest PR #19 review, apply any valid frontend fixes from that review batch, and verify the stats dashboard behavior stays unchanged aside from the targeted performance and error-handling improvements. + + +## Acceptance Criteria + +- [x] #1 VocabularyTab avoids recomputing expensive known-word and summary aggregates on unrelated rerenders while preserving current displayed values. +- [x] #2 useSessions and useSessionDetail normalize rejected values into stable string errors without throwing from the catch handler. +- [x] #3 Targeted tests cover the addressed review items and pass locally. +- [x] #4 Any user-facing docs remain accurate after the changes. + + +## Implementation Plan + + +1. Add focused tests that fail on the current branch for the two valid Claude findings: render-time aggregate recomputation in VocabularyTab and unsafe non-Error rejection handling in useSessions/useSessionDetail. +2. Update VocabularyTab to memoize the expensive summary and known-word aggregate calculations off the existing filteredWords/kanji/knownWords inputs without changing rendered values. +3. Normalize hook error handling to convert unknown rejection values into stable strings, matching the existing useTrends pattern. +4. Run the targeted stats/frontend test lane, verify no docs changes are needed, and record results in task notes. + + +## Implementation Notes + + +Validated Claude's latest PR #19 review comment from 2026-03-20 and narrowed it to two valid frontend follow-ups: memoized VocabularyTab aggregates and non-Error-safe session hook error handling. + +Added focused regression tests in stats/src/lib/vocabulary-tab.test.ts and stats/src/hooks/useSessions.test.ts before patching the implementation. + +Verification: `cd stats && bun test src/lib/vocabulary-tab.test.ts src/hooks/useSessions.test.ts` passed; `bun run format:check:stats` passed. + +Project-native verifier (`.agents/skills/subminer-change-verification/scripts/verify_subminer_change.sh --lane core ...`) passed root `bun run typecheck` and failed at `bun run test:fast` due an unrelated existing failure in `scripts/update-aur-package.test.ts` (`mapfile: command not found`). Artifact: `.tmp/skill-verification/subminer-verify-20260319-194525-vxVD9V`. + +No user-facing docs changes were needed because the fixes only affect render-time memoization and error normalization. + + +## Final Summary + + +Assessed Claude's latest PR #19 review and applied the two valid follow-ups. `stats/src/components/vocabulary/VocabularyTab.tsx` now memoizes `buildVocabularySummary(filteredWords, kanji)` and the known-word count so unrelated rerenders do not rescan the filtered vocabulary list. `stats/src/hooks/useSessions.ts` now exports a small `toErrorMessage` helper and uses it in both `useSessions` and `useSessionDetail`, preventing `.catch()` handlers from throwing when a promise rejects with a non-`Error` value. + +Added targeted regressions in `stats/src/lib/vocabulary-tab.test.ts` and `stats/src/hooks/useSessions.test.ts` to lock in the memoization shape and error normalization behavior. Verification passed for `cd stats && bun test src/lib/vocabulary-tab.test.ts src/hooks/useSessions.test.ts` and `bun run format:check:stats`. The repo-native verification wrapper for the classified `core` lane also passed root `bun run typecheck`, but `bun run test:fast` is currently blocked by an unrelated existing failure in `scripts/update-aur-package.test.ts` (`mapfile: command not found`); artifacts are recorded under `.tmp/skill-verification/subminer-verify-20260319-194525-vxVD9V`. + diff --git a/stats/src/components/vocabulary/VocabularyTab.tsx b/stats/src/components/vocabulary/VocabularyTab.tsx index 1819b60..dc0aa1b 100644 --- a/stats/src/components/vocabulary/VocabularyTab.tsx +++ b/stats/src/components/vocabulary/VocabularyTab.tsx @@ -47,6 +47,19 @@ export function VocabularyTab({ if (excluded.length > 0) result = result.filter((w) => !isExcluded(w)); return result; }, [words, hideNames, excluded, isExcluded]); + const summary = useMemo( + () => buildVocabularySummary(filteredWords, kanji), + [filteredWords, kanji], + ); + const knownWordCount = useMemo(() => { + if (knownWords.size === 0) return 0; + + let count = 0; + for (const w of filteredWords) { + if (knownWords.has(w.headword)) count += 1; + } + return count; + }, [filteredWords, knownWords]); if (loading) { return ( @@ -63,15 +76,6 @@ export function VocabularyTab({ ); } - const summary = buildVocabularySummary(filteredWords, kanji); - - let knownWordCount = 0; - if (knownWords.size > 0) { - for (const w of filteredWords) { - if (knownWords.has(w.headword)) knownWordCount++; - } - } - const handleSelectWord = (entry: VocabularyEntry): void => { onOpenWordDetail?.(entry.wordId); }; diff --git a/stats/src/hooks/useSessions.test.ts b/stats/src/hooks/useSessions.test.ts new file mode 100644 index 0000000..bcfe5d3 --- /dev/null +++ b/stats/src/hooks/useSessions.test.ts @@ -0,0 +1,20 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import test from 'node:test'; +import { fileURLToPath } from 'node:url'; +import { toErrorMessage } from './useSessions'; + +const USE_SESSIONS_PATH = fileURLToPath(new URL('./useSessions.ts', import.meta.url)); + +test('toErrorMessage normalizes Error and non-Error rejections', () => { + assert.equal(toErrorMessage(new Error('network down')), 'network down'); + assert.equal(toErrorMessage('bad gateway'), 'bad gateway'); + assert.equal(toErrorMessage(503), '503'); +}); + +test('useSessions and useSessionDetail route catch handlers through toErrorMessage', () => { + const source = fs.readFileSync(USE_SESSIONS_PATH, 'utf8'); + const matches = source.match(/setError\(toErrorMessage\(err\)\)/g); + + assert.equal(matches?.length, 2); +}); diff --git a/stats/src/hooks/useSessions.ts b/stats/src/hooks/useSessions.ts index 7f68ca2..4e72be0 100644 --- a/stats/src/hooks/useSessions.ts +++ b/stats/src/hooks/useSessions.ts @@ -3,6 +3,10 @@ import { getStatsClient } from './useStatsApi'; import { SESSION_CHART_EVENT_TYPES } from '../lib/session-events'; import type { SessionSummary, SessionTimelinePoint, SessionEvent } from '../types/stats'; +export function toErrorMessage(err: unknown): string { + return err instanceof Error ? err.message : String(err); +} + export function useSessions(limit = 50) { const [sessions, setSessions] = useState([]); const [loading, setLoading] = useState(true); @@ -21,7 +25,7 @@ export function useSessions(limit = 50) { }) .catch((err) => { if (cancelled) return; - setError(err.message); + setError(toErrorMessage(err)); }) .finally(() => { if (cancelled) return; @@ -77,7 +81,7 @@ export function useSessionDetail(sessionId: number | null) { }) .catch((err) => { if (cancelled) return; - setError(err.message); + setError(toErrorMessage(err)); }) .finally(() => { if (cancelled) return; diff --git a/stats/src/lib/vocabulary-tab.test.ts b/stats/src/lib/vocabulary-tab.test.ts index d6852f4..a5dc52c 100644 --- a/stats/src/lib/vocabulary-tab.test.ts +++ b/stats/src/lib/vocabulary-tab.test.ts @@ -1,11 +1,10 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; -import path from 'node:path'; import test from 'node:test'; +import { fileURLToPath } from 'node:url'; -const VOCABULARY_TAB_PATH = path.resolve( - import.meta.dir, - '../components/vocabulary/VocabularyTab.tsx', +const VOCABULARY_TAB_PATH = fileURLToPath( + new URL('../components/vocabulary/VocabularyTab.tsx', import.meta.url), ); test('VocabularyTab declares all hooks before loading and error early returns', () => { @@ -20,3 +19,16 @@ test('VocabularyTab declares all hooks before loading and error early returns', assert.deepEqual(hooksAfterLoadingGuard ?? [], []); }); + +test('VocabularyTab memoizes summary and known-word aggregate calculations', () => { + const source = fs.readFileSync(VOCABULARY_TAB_PATH, 'utf8'); + + assert.match( + source, + /const summary = useMemo\([\s\S]*buildVocabularySummary\(filteredWords, kanji\)[\s\S]*\[filteredWords, kanji\][\s\S]*\);/, + ); + assert.match( + source, + /const knownWordCount = useMemo\(\(\) => \{[\s\S]*for \(const w of filteredWords\) \{[\s\S]*knownWords\.has\(w\.headword\)[\s\S]*\}\s*return count;\s*\}, \[filteredWords, knownWords\]\);/, + ); +});