mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-04-11 04:19:26 -07:00
fix(stats): address CodeRabbit review on PR #50
- Guard episode deletion against double-submit with an isDeletingRef + setIsDeleting pair threaded through buildDeleteEpisodeHandler, and disable the MediaHeader delete button while a request is in flight. - Restore MediaHeader title truncation by adding min-w-0 flex-1 to the h2 so long titles shrink instead of pushing the delete button away. - Normalize the headword in FrequencyRankTable before comparing it to the (hiragana-normalized) reading so katakana-only entries like カレー no longer render a redundant 【かれー】. Test strengthened to reject any bracketed reading, not just the literal. - Rewrite confirmBucketDelete copy to include the "and all associated data" warning and handle singular/plural cleanly. - Run Prettier across the stats files CI was complaining about (EpisodeDetail, WatchTimeChart, SessionsTab + test, FrequencyRankTable + test, session-grouping test) to clear the format:check:stats gate.
This commit is contained in:
@@ -184,7 +184,8 @@ export function EpisodeDetail({ videoId, onSessionDeleted }: EpisodeDetailProps)
|
|||||||
</div>
|
</div>
|
||||||
{hiddenCardCount > 0 && (
|
{hiddenCardCount > 0 && (
|
||||||
<div className="px-3 pb-3 -mt-1 text-[10px] text-ctp-overlay2 italic">
|
<div className="px-3 pb-3 -mt-1 text-[10px] text-ctp-overlay2 italic">
|
||||||
{hiddenCardCount} {hiddenCardCount === 1 ? 'card' : 'cards'} hidden (deleted from Anki)
|
{hiddenCardCount} {hiddenCardCount === 1 ? 'card' : 'cards'} hidden (deleted from
|
||||||
|
Anki)
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -1,4 +1,4 @@
|
|||||||
import { useEffect, useState } from 'react';
|
import { useEffect, useRef, useState } from 'react';
|
||||||
import { useMediaDetail } from '../../hooks/useMediaDetail';
|
import { useMediaDetail } from '../../hooks/useMediaDetail';
|
||||||
import { apiClient } from '../../lib/api-client';
|
import { apiClient } from '../../lib/api-client';
|
||||||
import { confirmSessionDelete, confirmEpisodeDelete } from '../../lib/delete-confirm';
|
import { confirmSessionDelete, confirmEpisodeDelete } from '../../lib/delete-confirm';
|
||||||
@@ -14,17 +14,31 @@ interface DeleteEpisodeHandlerOptions {
|
|||||||
confirmFn: (title: string) => boolean;
|
confirmFn: (title: string) => boolean;
|
||||||
onBack: () => void;
|
onBack: () => void;
|
||||||
setDeleteError: (msg: string | null) => void;
|
setDeleteError: (msg: string | null) => void;
|
||||||
|
/**
|
||||||
|
* Ref used to guard against reentrant delete calls synchronously. When set,
|
||||||
|
* a subsequent invocation while the previous request is still pending is
|
||||||
|
* ignored so clicks during the await window can't trigger duplicate deletes.
|
||||||
|
*/
|
||||||
|
isDeletingRef?: { current: boolean };
|
||||||
|
/** Optional React state setter so the UI can reflect the pending state. */
|
||||||
|
setIsDeleting?: (value: boolean) => void;
|
||||||
}
|
}
|
||||||
|
|
||||||
export function buildDeleteEpisodeHandler(opts: DeleteEpisodeHandlerOptions): () => Promise<void> {
|
export function buildDeleteEpisodeHandler(opts: DeleteEpisodeHandlerOptions): () => Promise<void> {
|
||||||
return async () => {
|
return async () => {
|
||||||
|
if (opts.isDeletingRef?.current) return;
|
||||||
if (!opts.confirmFn(opts.title)) return;
|
if (!opts.confirmFn(opts.title)) return;
|
||||||
|
if (opts.isDeletingRef) opts.isDeletingRef.current = true;
|
||||||
|
opts.setIsDeleting?.(true);
|
||||||
opts.setDeleteError(null);
|
opts.setDeleteError(null);
|
||||||
try {
|
try {
|
||||||
await opts.apiClient.deleteVideo(opts.videoId);
|
await opts.apiClient.deleteVideo(opts.videoId);
|
||||||
opts.onBack();
|
opts.onBack();
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
opts.setDeleteError(err instanceof Error ? err.message : 'Failed to delete episode.');
|
opts.setDeleteError(err instanceof Error ? err.message : 'Failed to delete episode.');
|
||||||
|
} finally {
|
||||||
|
if (opts.isDeletingRef) opts.isDeletingRef.current = false;
|
||||||
|
opts.setIsDeleting?.(false);
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
@@ -57,6 +71,8 @@ export function MediaDetailView({
|
|||||||
const [localSessions, setLocalSessions] = useState<SessionSummary[] | null>(null);
|
const [localSessions, setLocalSessions] = useState<SessionSummary[] | null>(null);
|
||||||
const [deleteError, setDeleteError] = useState<string | null>(null);
|
const [deleteError, setDeleteError] = useState<string | null>(null);
|
||||||
const [deletingSessionId, setDeletingSessionId] = useState<number | null>(null);
|
const [deletingSessionId, setDeletingSessionId] = useState<number | null>(null);
|
||||||
|
const [isDeletingEpisode, setIsDeletingEpisode] = useState(false);
|
||||||
|
const isDeletingEpisodeRef = useRef(false);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
setLocalSessions(data?.sessions ?? null);
|
setLocalSessions(data?.sessions ?? null);
|
||||||
@@ -108,6 +124,8 @@ export function MediaDetailView({
|
|||||||
confirmFn: confirmEpisodeDelete,
|
confirmFn: confirmEpisodeDelete,
|
||||||
onBack,
|
onBack,
|
||||||
setDeleteError,
|
setDeleteError,
|
||||||
|
isDeletingRef: isDeletingEpisodeRef,
|
||||||
|
setIsDeleting: setIsDeletingEpisode,
|
||||||
});
|
});
|
||||||
|
|
||||||
return (
|
return (
|
||||||
@@ -130,7 +148,11 @@ export function MediaDetailView({
|
|||||||
</button>
|
</button>
|
||||||
) : null}
|
) : null}
|
||||||
</div>
|
</div>
|
||||||
<MediaHeader detail={detail} onDeleteEpisode={handleDeleteEpisode} />
|
<MediaHeader
|
||||||
|
detail={detail}
|
||||||
|
onDeleteEpisode={handleDeleteEpisode}
|
||||||
|
isDeletingEpisode={isDeletingEpisode}
|
||||||
|
/>
|
||||||
{deleteError ? <div className="text-sm text-ctp-red">{deleteError}</div> : null}
|
{deleteError ? <div className="text-sm text-ctp-red">{deleteError}</div> : null}
|
||||||
<MediaSessionList
|
<MediaSessionList
|
||||||
sessions={sessions}
|
sessions={sessions}
|
||||||
|
|||||||
@@ -13,12 +13,14 @@ interface MediaHeaderProps {
|
|||||||
knownWordCount: number;
|
knownWordCount: number;
|
||||||
} | null;
|
} | null;
|
||||||
onDeleteEpisode?: () => void;
|
onDeleteEpisode?: () => void;
|
||||||
|
isDeletingEpisode?: boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
export function MediaHeader({
|
export function MediaHeader({
|
||||||
detail,
|
detail,
|
||||||
initialKnownWordsSummary = null,
|
initialKnownWordsSummary = null,
|
||||||
onDeleteEpisode,
|
onDeleteEpisode,
|
||||||
|
isDeletingEpisode = false,
|
||||||
}: MediaHeaderProps) {
|
}: MediaHeaderProps) {
|
||||||
const knownTokenRate =
|
const knownTokenRate =
|
||||||
detail.totalLookupCount > 0 ? detail.totalLookupHits / detail.totalLookupCount : null;
|
detail.totalLookupCount > 0 ? detail.totalLookupHits / detail.totalLookupCount : null;
|
||||||
@@ -56,14 +58,17 @@ export function MediaHeader({
|
|||||||
/>
|
/>
|
||||||
<div className="flex-1 min-w-0">
|
<div className="flex-1 min-w-0">
|
||||||
<div className="flex items-start justify-between gap-2">
|
<div className="flex items-start justify-between gap-2">
|
||||||
<h2 className="text-lg font-bold text-ctp-text truncate">{detail.canonicalTitle}</h2>
|
<h2 className="min-w-0 flex-1 text-lg font-bold text-ctp-text truncate">
|
||||||
|
{detail.canonicalTitle}
|
||||||
|
</h2>
|
||||||
{onDeleteEpisode != null ? (
|
{onDeleteEpisode != null ? (
|
||||||
<button
|
<button
|
||||||
type="button"
|
type="button"
|
||||||
onClick={onDeleteEpisode}
|
onClick={onDeleteEpisode}
|
||||||
className="shrink-0 text-xs text-ctp-red hover:opacity-75 transition-opacity"
|
disabled={isDeletingEpisode}
|
||||||
|
className="shrink-0 text-xs text-ctp-red hover:opacity-75 transition-opacity disabled:opacity-50 disabled:cursor-not-allowed"
|
||||||
>
|
>
|
||||||
Delete Episode
|
{isDeletingEpisode ? 'Deleting...' : 'Delete Episode'}
|
||||||
</button>
|
</button>
|
||||||
) : null}
|
) : null}
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -1,13 +1,5 @@
|
|||||||
import { useState } from 'react';
|
import { useState } from 'react';
|
||||||
import {
|
import { BarChart, Bar, CartesianGrid, XAxis, YAxis, Tooltip, ResponsiveContainer } from 'recharts';
|
||||||
BarChart,
|
|
||||||
Bar,
|
|
||||||
CartesianGrid,
|
|
||||||
XAxis,
|
|
||||||
YAxis,
|
|
||||||
Tooltip,
|
|
||||||
ResponsiveContainer,
|
|
||||||
} from 'recharts';
|
|
||||||
import { epochDayToDate } from '../../lib/formatters';
|
import { epochDayToDate } from '../../lib/formatters';
|
||||||
import { CHART_DEFAULTS, CHART_THEME, TOOLTIP_CONTENT_STYLE } from '../../lib/chart-theme';
|
import { CHART_DEFAULTS, CHART_THEME, TOOLTIP_CONTENT_STYLE } from '../../lib/chart-theme';
|
||||||
import type { DailyRollup } from '../../types/stats';
|
import type { DailyRollup } from '../../types/stats';
|
||||||
|
|||||||
@@ -75,10 +75,7 @@ test('buildBucketDeleteHandler is a no-op when confirm returns false', async ()
|
|||||||
let deleteCalled = false;
|
let deleteCalled = false;
|
||||||
let successCalled = false;
|
let successCalled = false;
|
||||||
|
|
||||||
const bucket = makeBucket([
|
const bucket = makeBucket([makeSession({ sessionId: 1 }), makeSession({ sessionId: 2 })]);
|
||||||
makeSession({ sessionId: 1 }),
|
|
||||||
makeSession({ sessionId: 2 }),
|
|
||||||
]);
|
|
||||||
|
|
||||||
const handler = buildBucketDeleteHandler({
|
const handler = buildBucketDeleteHandler({
|
||||||
bucket,
|
bucket,
|
||||||
@@ -104,10 +101,7 @@ test('buildBucketDeleteHandler reports errors via onError without calling onSucc
|
|||||||
let errorMessage: string | null = null;
|
let errorMessage: string | null = null;
|
||||||
let successCalled = false;
|
let successCalled = false;
|
||||||
|
|
||||||
const bucket = makeBucket([
|
const bucket = makeBucket([makeSession({ sessionId: 1 }), makeSession({ sessionId: 2 })]);
|
||||||
makeSession({ sessionId: 1 }),
|
|
||||||
makeSession({ sessionId: 2 }),
|
|
||||||
]);
|
|
||||||
|
|
||||||
const handler = buildBucketDeleteHandler({
|
const handler = buildBucketDeleteHandler({
|
||||||
bucket,
|
bucket,
|
||||||
|
|||||||
@@ -269,9 +269,7 @@ export function SessionsTab({
|
|||||||
isExpanded={expandedId === s.sessionId}
|
isExpanded={expandedId === s.sessionId}
|
||||||
detailsId={detailsId}
|
detailsId={detailsId}
|
||||||
onToggle={() =>
|
onToggle={() =>
|
||||||
setExpandedId(
|
setExpandedId(expandedId === s.sessionId ? null : s.sessionId)
|
||||||
expandedId === s.sessionId ? null : s.sessionId,
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
onDelete={() => void handleDeleteSession(s)}
|
onDelete={() => void handleDeleteSession(s)}
|
||||||
deleteDisabled={deletingSessionId === s.sessionId}
|
deleteDisabled={deletingSessionId === s.sessionId}
|
||||||
|
|||||||
@@ -36,5 +36,8 @@ test('omits reading when reading equals headword', () => {
|
|||||||
<FrequencyRankTable words={[entry]} knownWords={new Set()} />,
|
<FrequencyRankTable words={[entry]} knownWords={new Set()} />,
|
||||||
);
|
);
|
||||||
assert.ok(markup.includes('カレー'), 'should include the headword');
|
assert.ok(markup.includes('カレー'), 'should include the headword');
|
||||||
assert.ok(!markup.includes('【カレー】'), 'should not render reading in brackets when equal to headword');
|
assert.ok(
|
||||||
|
!markup.includes('【'),
|
||||||
|
'should not render any bracketed reading when equal to headword',
|
||||||
|
);
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -131,11 +131,13 @@ export function FrequencyRankTable({ words, knownWords, onSelectWord }: Frequenc
|
|||||||
<span className="text-ctp-text font-medium">{w.headword}</span>
|
<span className="text-ctp-text font-medium">{w.headword}</span>
|
||||||
{(() => {
|
{(() => {
|
||||||
const reading = fullReading(w.headword, w.reading);
|
const reading = fullReading(w.headword, w.reading);
|
||||||
if (!reading || reading === w.headword) return null;
|
// `fullReading` normalizes katakana to hiragana, so we normalize the
|
||||||
|
// headword the same way before comparing — otherwise katakana-only
|
||||||
|
// entries like `カレー` would render `【かれー】`.
|
||||||
|
const normalizedHeadword = fullReading(w.headword, w.headword);
|
||||||
|
if (!reading || reading === normalizedHeadword) return null;
|
||||||
return (
|
return (
|
||||||
<span className="text-ctp-subtext0 text-xs ml-1.5">
|
<span className="text-ctp-subtext0 text-xs ml-1.5">【{reading}】</span>
|
||||||
【{reading}】
|
|
||||||
</span>
|
|
||||||
);
|
);
|
||||||
})()}
|
})()}
|
||||||
</td>
|
</td>
|
||||||
|
|||||||
@@ -65,16 +65,15 @@ test('confirmBucketDelete asks about merging multiple sessions of the same episo
|
|||||||
|
|
||||||
try {
|
try {
|
||||||
assert.equal(confirmBucketDelete('My Episode', 3), true);
|
assert.equal(confirmBucketDelete('My Episode', 3), true);
|
||||||
assert.equal(calls.length, 1);
|
assert.deepEqual(calls, [
|
||||||
assert.match(calls[0]!, /3/);
|
'Delete all 3 sessions of "My Episode" from this day and all associated data?',
|
||||||
assert.match(calls[0]!, /My Episode/);
|
]);
|
||||||
assert.match(calls[0]!, /sessions/);
|
|
||||||
} finally {
|
} finally {
|
||||||
globalThis.confirm = originalConfirm;
|
globalThis.confirm = originalConfirm;
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
test('confirmBucketDelete uses singular for one session', () => {
|
test('confirmBucketDelete uses a clean singular form for one session', () => {
|
||||||
const calls: string[] = [];
|
const calls: string[] = [];
|
||||||
const originalConfirm = globalThis.confirm;
|
const originalConfirm = globalThis.confirm;
|
||||||
globalThis.confirm = ((message?: string) => {
|
globalThis.confirm = ((message?: string) => {
|
||||||
@@ -84,7 +83,9 @@ test('confirmBucketDelete uses singular for one session', () => {
|
|||||||
|
|
||||||
try {
|
try {
|
||||||
assert.equal(confirmBucketDelete('Solo Episode', 1), false);
|
assert.equal(confirmBucketDelete('Solo Episode', 1), false);
|
||||||
assert.match(calls[0]!, /1 session of/);
|
assert.deepEqual(calls, [
|
||||||
|
'Delete this session of "Solo Episode" from this day and all associated data?',
|
||||||
|
]);
|
||||||
} finally {
|
} finally {
|
||||||
globalThis.confirm = originalConfirm;
|
globalThis.confirm = originalConfirm;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -19,7 +19,12 @@ export function confirmEpisodeDelete(title: string): boolean {
|
|||||||
}
|
}
|
||||||
|
|
||||||
export function confirmBucketDelete(title: string, count: number): boolean {
|
export function confirmBucketDelete(title: string, count: number): boolean {
|
||||||
|
if (count === 1) {
|
||||||
|
return globalThis.confirm(
|
||||||
|
`Delete this session of "${title}" from this day and all associated data?`,
|
||||||
|
);
|
||||||
|
}
|
||||||
return globalThis.confirm(
|
return globalThis.confirm(
|
||||||
`Delete all ${count} session${count === 1 ? '' : 's'} of "${title}" from this day?`,
|
`Delete all ${count} sessions of "${title}" from this day and all associated data?`,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -32,8 +32,20 @@ test('empty input returns empty array', () => {
|
|||||||
|
|
||||||
test('two unique videoIds produce 2 singleton buckets', () => {
|
test('two unique videoIds produce 2 singleton buckets', () => {
|
||||||
const sessions = [
|
const sessions = [
|
||||||
makeSession({ sessionId: 1, videoId: 10, startedAtMs: 1000, activeWatchedMs: 100, cardsMined: 2 }),
|
makeSession({
|
||||||
makeSession({ sessionId: 2, videoId: 20, startedAtMs: 2000, activeWatchedMs: 200, cardsMined: 3 }),
|
sessionId: 1,
|
||||||
|
videoId: 10,
|
||||||
|
startedAtMs: 1000,
|
||||||
|
activeWatchedMs: 100,
|
||||||
|
cardsMined: 2,
|
||||||
|
}),
|
||||||
|
makeSession({
|
||||||
|
sessionId: 2,
|
||||||
|
videoId: 20,
|
||||||
|
startedAtMs: 2000,
|
||||||
|
activeWatchedMs: 200,
|
||||||
|
cardsMined: 3,
|
||||||
|
}),
|
||||||
];
|
];
|
||||||
const buckets = groupSessionsByVideo(sessions);
|
const buckets = groupSessionsByVideo(sessions);
|
||||||
assert.equal(buckets.length, 2);
|
assert.equal(buckets.length, 2);
|
||||||
@@ -45,8 +57,20 @@ test('two unique videoIds produce 2 singleton buckets', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
test('two sessions sharing a videoId collapse into 1 bucket with summed totals and most-recent representative', () => {
|
test('two sessions sharing a videoId collapse into 1 bucket with summed totals and most-recent representative', () => {
|
||||||
const older = makeSession({ sessionId: 1, videoId: 42, startedAtMs: 1000, activeWatchedMs: 300, cardsMined: 5 });
|
const older = makeSession({
|
||||||
const newer = makeSession({ sessionId: 2, videoId: 42, startedAtMs: 9000, activeWatchedMs: 500, cardsMined: 7 });
|
sessionId: 1,
|
||||||
|
videoId: 42,
|
||||||
|
startedAtMs: 1000,
|
||||||
|
activeWatchedMs: 300,
|
||||||
|
cardsMined: 5,
|
||||||
|
});
|
||||||
|
const newer = makeSession({
|
||||||
|
sessionId: 2,
|
||||||
|
videoId: 42,
|
||||||
|
startedAtMs: 9000,
|
||||||
|
activeWatchedMs: 500,
|
||||||
|
cardsMined: 7,
|
||||||
|
});
|
||||||
const buckets = groupSessionsByVideo([older, newer]);
|
const buckets = groupSessionsByVideo([older, newer]);
|
||||||
assert.equal(buckets.length, 1);
|
assert.equal(buckets.length, 1);
|
||||||
const [bucket] = buckets;
|
const [bucket] = buckets;
|
||||||
|
|||||||
Reference in New Issue
Block a user