mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-03-20 12:11:28 -07:00
fix(stats): address Claude review follow-ups
This commit is contained in:
@@ -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
|
||||||
|
|
||||||
|
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||||
|
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.
|
||||||
|
<!-- SECTION:DESCRIPTION:END -->
|
||||||
|
|
||||||
|
## Acceptance Criteria
|
||||||
|
<!-- AC:BEGIN -->
|
||||||
|
- [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.
|
||||||
|
<!-- AC:END -->
|
||||||
|
|
||||||
|
## Implementation Plan
|
||||||
|
|
||||||
|
<!-- SECTION:PLAN:BEGIN -->
|
||||||
|
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.
|
||||||
|
<!-- SECTION:PLAN:END -->
|
||||||
|
|
||||||
|
## Implementation Notes
|
||||||
|
|
||||||
|
<!-- SECTION:NOTES:BEGIN -->
|
||||||
|
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.
|
||||||
|
<!-- SECTION:NOTES:END -->
|
||||||
|
|
||||||
|
## Final Summary
|
||||||
|
|
||||||
|
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
|
||||||
|
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`.
|
||||||
|
<!-- SECTION:FINAL_SUMMARY:END -->
|
||||||
@@ -47,6 +47,19 @@ export function VocabularyTab({
|
|||||||
if (excluded.length > 0) result = result.filter((w) => !isExcluded(w));
|
if (excluded.length > 0) result = result.filter((w) => !isExcluded(w));
|
||||||
return result;
|
return result;
|
||||||
}, [words, hideNames, excluded, isExcluded]);
|
}, [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) {
|
if (loading) {
|
||||||
return (
|
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 => {
|
const handleSelectWord = (entry: VocabularyEntry): void => {
|
||||||
onOpenWordDetail?.(entry.wordId);
|
onOpenWordDetail?.(entry.wordId);
|
||||||
};
|
};
|
||||||
|
|||||||
20
stats/src/hooks/useSessions.test.ts
Normal file
20
stats/src/hooks/useSessions.test.ts
Normal file
@@ -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);
|
||||||
|
});
|
||||||
@@ -3,6 +3,10 @@ import { getStatsClient } from './useStatsApi';
|
|||||||
import { SESSION_CHART_EVENT_TYPES } from '../lib/session-events';
|
import { SESSION_CHART_EVENT_TYPES } from '../lib/session-events';
|
||||||
import type { SessionSummary, SessionTimelinePoint, SessionEvent } from '../types/stats';
|
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) {
|
export function useSessions(limit = 50) {
|
||||||
const [sessions, setSessions] = useState<SessionSummary[]>([]);
|
const [sessions, setSessions] = useState<SessionSummary[]>([]);
|
||||||
const [loading, setLoading] = useState(true);
|
const [loading, setLoading] = useState(true);
|
||||||
@@ -21,7 +25,7 @@ export function useSessions(limit = 50) {
|
|||||||
})
|
})
|
||||||
.catch((err) => {
|
.catch((err) => {
|
||||||
if (cancelled) return;
|
if (cancelled) return;
|
||||||
setError(err.message);
|
setError(toErrorMessage(err));
|
||||||
})
|
})
|
||||||
.finally(() => {
|
.finally(() => {
|
||||||
if (cancelled) return;
|
if (cancelled) return;
|
||||||
@@ -77,7 +81,7 @@ export function useSessionDetail(sessionId: number | null) {
|
|||||||
})
|
})
|
||||||
.catch((err) => {
|
.catch((err) => {
|
||||||
if (cancelled) return;
|
if (cancelled) return;
|
||||||
setError(err.message);
|
setError(toErrorMessage(err));
|
||||||
})
|
})
|
||||||
.finally(() => {
|
.finally(() => {
|
||||||
if (cancelled) return;
|
if (cancelled) return;
|
||||||
|
|||||||
@@ -1,11 +1,10 @@
|
|||||||
import assert from 'node:assert/strict';
|
import assert from 'node:assert/strict';
|
||||||
import fs from 'node:fs';
|
import fs from 'node:fs';
|
||||||
import path from 'node:path';
|
|
||||||
import test from 'node:test';
|
import test from 'node:test';
|
||||||
|
import { fileURLToPath } from 'node:url';
|
||||||
|
|
||||||
const VOCABULARY_TAB_PATH = path.resolve(
|
const VOCABULARY_TAB_PATH = fileURLToPath(
|
||||||
import.meta.dir,
|
new URL('../components/vocabulary/VocabularyTab.tsx', import.meta.url),
|
||||||
'../components/vocabulary/VocabularyTab.tsx',
|
|
||||||
);
|
);
|
||||||
|
|
||||||
test('VocabularyTab declares all hooks before loading and error early returns', () => {
|
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 ?? [], []);
|
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\]\);/,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user