Night Shift: rename LanguageFilter "persian" to "original"#20
Night Shift: rename LanguageFilter "persian" to "original"#20
Conversation
The LanguageFilter type hardcoded "persian" as the value for showing original/source text, but the app supports multiple source languages (Korean, Arabic, Hebrew, etc.). Renamed to "original" for clarity. Includes backward compatibility: stored "persian" preferences (Convex DB and localStorage) are normalized to "original" on read. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR renames the "persian" language filter option to "original" across the codebase, introduces backward-compatibility normalization for the renamed filter, expands test coverage for the LyricsDisplay component, applies quote style formatting improvements, and significantly enhances the song page with new playback controls, mode selection UI, and improved progress orchestration logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/LyricsDisplay.tsx (1)
83-83: 🧹 Nitpick | 🔵 Trivial
lineRefsis typed asHTMLButtonElement[]but stores<div>elements.The ref array is typed as
(HTMLButtonElement | null)[](line 83), and the cast on line 105 forcesel as HTMLButtonElement | null. However, the actual element is a<div>(line 102). Since onlyscrollIntoViewis called (which exists on allHTMLElements), this isn't a runtime bug, but the typing is misleading.Suggested fix
- const lineRefs = useRef<(HTMLButtonElement | null)[]>([]); + const lineRefs = useRef<(HTMLDivElement | null)[]>([]);- lineRefs.current[index] = el as HTMLButtonElement | null; + lineRefs.current[index] = el;Also applies to: 102-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/LyricsDisplay.tsx` at line 83, The ref array lineRefs is incorrectly typed as (HTMLButtonElement | null)[] while it stores div elements; update the type used in the useRef declaration (lineRefs) to (HTMLElement | null)[] (or a more specific HTMLDivElement | null[]) and adjust any casts (the setter that currently uses el as HTMLButtonElement | null) to use the matching HTMLElement/HTMLDivElement type so calls like scrollIntoView remain correctly typed; locate useRef(...) for lineRefs and the ref assignment where el is cast and change both to the consistent HTMLElement/HTMLDivElement type.src/routes/song.$songId.tsx (1)
382-409: 🧹 Nitpick | 🔵 TrivialPractice logging fires a Convex mutation every second per active user.
logPracticeMutation({ eventType: "audio_time", value: 1 })is called every second for authenticated users. At scale, this could create significant write load on Convex. Consider batching (e.g., accumulate locally and flush every 10-30 seconds) to reduce mutation frequency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/song`.$songId.tsx around lines 382 - 409, The effect currently calls logPracticeMutation({ eventType: "audio_time", value: 1 }) every second for authenticated users which will cause high write volume; change useEffect logic (the effect using lastActivityRef, IDLE_THRESHOLD_MS, shouldCountTime, practiceSecondsRef, isAuthenticated, logPracticeMutation, logPracticeFn) to accumulate seconds in a local counter (e.g., practiceBatchRef) and only call logPracticeMutation with the batched value on a longer flush interval (e.g., 10–30s) or when user becomes idle/unmounts, while keeping logPracticeFn behavior for anonymous users; ensure you flush remaining batched seconds on cleanup/unload and reset counters appropriately so real-time header updates remain reasonably fresh but mutations are throttled.src/hooks/useAnonymousProgress.ts (1)
12-19: 🧹 Nitpick | 🔵 TrivialConsider renaming
WordProgressItem.persianfield tooriginalfor consistency.The
persianfield inWordProgressItemstores the word's text in the source language. Given this PR's goal of supporting multiple source languages (Korean, Arabic, Hebrew), the field namepersianis misleading for non-Persian songs. This would require a data migration for existing localStorage entries, similar to what's been done forlanguageFilter.This is not blocking since it's a data schema concern beyond this PR's scope, but worth tracking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useAnonymousProgress.ts` around lines 12 - 19, The WordProgressItem interface in useAnonymousProgress.ts uses a Persian-specific field name (WordProgressItem.persian); rename this field to a neutral name like original across the type and all usages (serialization/deserialization, reads/writes, and any accessors in useAnonymousProgress) and add a simple migration when loading saved progress from localStorage to map old persian keys to original (similar to the languageFilter migration) so existing data is preserved; update all code that reads/writes WordProgressItem (including any JSON.parse/JSON.stringify paths) to handle both old and new property names until migration is complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/LyricsDisplay.test.tsx`:
- Around line 1-57: Add tests in LyricsDisplay.test.tsx to cover the checkbox
and info-button interactions: render LyricsDisplay (wrapped with
QueryClientProvider if needed), pass vi.fn() spies for onLineCheckboxClick and
onLineInfoClick props, use testing-library fireEvent or userEvent to click the
checkbox for a specific lyric and the info button for that lyric, and assert the
corresponding spy was called with the expected lyric id or payload; locate
handlers by prop names onLineCheckboxClick and onLineInfoClick and target
elements by role/label/text matching the line content used in the mocked data.
In `@src/components/LyricsDisplay.tsx`:
- Line 57: Replace the hardcoded "persian" fallback used when computing RTL with
the new renamed filter value so unknown source languages default to LTR: update
the isOriginalRTL assignment (the variable isOriginalRTL that calls
isRTLLanguage(sourceLanguage || "persian")) to use "original" (or otherwise pass
an empty/undefined value) instead of "persian" so the RTL check aligns with the
rename and treats unknown languages as LTR.
In `@src/routes/song`.$songId.tsx:
- Around line 822-844: The handleToggleWordLearned callback calls
toggleWordLearnedMutation but never handles failures; update
handleToggleWordLearned to add a .catch on the toggleWordLearnedMutation promise
(or use try/catch if converting to async) that logs the error (e.g.,
console.error or processLogger), shows user-facing feedback (toast or set an
error state) and, if you applied any optimistic UI changes, revert them;
reference the toggleWordLearnedMutation call inside handleToggleWordLearned and
also keep logPracticeMutation and toggleWordLearnedFn behavior unchanged for the
success path.
- Around line 506-511: Refactor duplicated seek-guard blocks to use the existing
seekTo helper: replace the inline pattern in handlePlaybackModeChange,
handleLyricLineClick, and handleWordModalClose with calls to
seekTo(sortedLyrics[lineIndexToPlay].startTime) (or the appropriate startTime
for the clicked/closed line) and remove the manual isSeekingRef/current and
setTimeout logic; to fix handlePlaybackModeChange you must move the seekTo
function definition above handlePlaybackModeChange (or hoist it) and then add
seekTo to handlePlaybackModeChange’s dependency array so hooks remain correct;
keep references to playerRef, isSeekingRef, sortedLyrics and lineIndexToPlay as
before.
---
Outside diff comments:
In `@src/components/LyricsDisplay.tsx`:
- Line 83: The ref array lineRefs is incorrectly typed as (HTMLButtonElement |
null)[] while it stores div elements; update the type used in the useRef
declaration (lineRefs) to (HTMLElement | null)[] (or a more specific
HTMLDivElement | null[]) and adjust any casts (the setter that currently uses el
as HTMLButtonElement | null) to use the matching HTMLElement/HTMLDivElement type
so calls like scrollIntoView remain correctly typed; locate useRef(...) for
lineRefs and the ref assignment where el is cast and change both to the
consistent HTMLElement/HTMLDivElement type.
In `@src/hooks/useAnonymousProgress.ts`:
- Around line 12-19: The WordProgressItem interface in useAnonymousProgress.ts
uses a Persian-specific field name (WordProgressItem.persian); rename this field
to a neutral name like original across the type and all usages
(serialization/deserialization, reads/writes, and any accessors in
useAnonymousProgress) and add a simple migration when loading saved progress
from localStorage to map old persian keys to original (similar to the
languageFilter migration) so existing data is preserved; update all code that
reads/writes WordProgressItem (including any JSON.parse/JSON.stringify paths) to
handle both old and new property names until migration is complete.
In `@src/routes/song`.$songId.tsx:
- Around line 382-409: The effect currently calls logPracticeMutation({
eventType: "audio_time", value: 1 }) every second for authenticated users which
will cause high write volume; change useEffect logic (the effect using
lastActivityRef, IDLE_THRESHOLD_MS, shouldCountTime, practiceSecondsRef,
isAuthenticated, logPracticeMutation, logPracticeFn) to accumulate seconds in a
local counter (e.g., practiceBatchRef) and only call logPracticeMutation with
the batched value on a longer flush interval (e.g., 10–30s) or when user becomes
idle/unmounts, while keeping logPracticeFn behavior for anonymous users; ensure
you flush remaining batched seconds on cleanup/unload and reset counters
appropriately so real-time header updates remain reasonably fresh but mutations
are throttled.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/__tests__/songPageDefaults.test.tssrc/components/LyricsDisplay.test.tsxsrc/components/LyricsDisplay.tsxsrc/hooks/useAnonymousProgress.tssrc/routes/song.$songId.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TanStack Start framework with Bun runtime for the application
Tests must pass locally via
bun run testbefore committing code, as Husky pre-commit hooks will block commits with failing tests
Files:
src/routes/song.$songId.tsxsrc/components/LyricsDisplay.test.tsxsrc/__tests__/songPageDefaults.test.tssrc/components/LyricsDisplay.tsxsrc/hooks/useAnonymousProgress.ts
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Components with logic MUST have corresponding test files following the naming convention
ComponentName.test.tsx
Files:
src/routes/song.$songId.tsxsrc/components/LyricsDisplay.test.tsxsrc/components/LyricsDisplay.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for unit testing and Playwright for integration testing
Files:
src/components/LyricsDisplay.test.tsxsrc/__tests__/songPageDefaults.test.ts
**/*.test.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Component tests should be located in the same directory as the component file with naming convention
ComponentName.test.tsx
Files:
src/components/LyricsDisplay.test.tsx
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All new helpers and utilities MUST have corresponding test files
Files:
src/__tests__/songPageDefaults.test.tssrc/hooks/useAnonymousProgress.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for utilities should be located in the same directory as the utility file with naming convention
utilName.test.ts
Files:
src/__tests__/songPageDefaults.test.ts
src/__tests__/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Integration tests should be located in
src/__tests__/directory with naming conventionfeatureName.test.ts
Files:
src/__tests__/songPageDefaults.test.ts
🧠 Learnings (12)
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Applies to app/routes/**/*.tsx : Use TanStack file-based routing convention with route files like index.tsx, login.tsx, and song.$songId.tsx
Applied to files:
src/routes/song.$songId.tsx
📚 Learning: 2026-01-23T18:12:38.519Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:38.519Z
Learning: Applies to app/routes/**/*.tsx : Use TanStack file-based routing for routes defined in app/routes/ directory
Applied to files:
src/routes/song.$songId.tsx
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Applies to convex/**/*.ts : Use Convex schema.ts for database schema definitions and songs.ts for queries and mutations
Applied to files:
src/routes/song.$songId.tsxsrc/__tests__/songPageDefaults.test.ts
📚 Learning: 2026-01-23T18:12:38.519Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:38.519Z
Learning: Applies to convex/*.ts : Use Convex for database queries, mutations, and authentication configuration
Applied to files:
src/routes/song.$songId.tsx
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Applies to **/*.tsx : Components with logic MUST have corresponding test files following the naming convention `ComponentName.test.tsx`
Applied to files:
src/components/LyricsDisplay.test.tsxsrc/__tests__/songPageDefaults.test.ts
📚 Learning: 2026-01-23T18:12:38.519Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:38.519Z
Learning: Applies to **/*.test.{ts,tsx} : Use Vitest for unit testing and Playwright for integration testing
Applied to files:
src/components/LyricsDisplay.test.tsxsrc/__tests__/songPageDefaults.test.ts
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Applies to **/*.ts : All new helpers and utilities MUST have corresponding test files
Applied to files:
src/__tests__/songPageDefaults.test.ts
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Applies to src/__tests__/**/*.test.ts : Integration tests should be located in `src/__tests__/` directory with naming convention `featureName.test.ts`
Applied to files:
src/__tests__/songPageDefaults.test.ts
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Applies to **/*.test.ts : Unit tests for utilities should be located in the same directory as the utility file with naming convention `utilName.test.ts`
Applied to files:
src/__tests__/songPageDefaults.test.ts
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Applies to **/*.test.tsx : Component tests should be located in the same directory as the component file with naming convention `ComponentName.test.tsx`
Applied to files:
src/__tests__/songPageDefaults.test.ts
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Song transliteration learning app should display song lyrics line by line with transliteration and allow users to follow along and learn pronunciation
Applied to files:
src/components/LyricsDisplay.tsx
📚 Learning: 2026-01-23T18:12:49.193Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*{score,progress,leaderboard}*.{ts,tsx,js} : Use Progress Score Formula: (words_learned × multiplier) + (lines_completed × multiplier × 0.5)
Applied to files:
src/hooks/useAnonymousProgress.ts
🧬 Code graph analysis (2)
src/routes/song.$songId.tsx (5)
src/components/LyricsDisplay.tsx (2)
LanguageFilter(21-26)LyricLine(9-19)src/components/LanguageFlag.tsx (1)
isRTLLanguage(85-90)src/components/ui/select.tsx (5)
Select(178-178)SelectTrigger(186-186)SelectValue(187-187)SelectContent(179-179)SelectItem(181-181)src/components/dashboard/LanguageChip.tsx (1)
getLanguageDisplayName(17-36)src/components/dashboard/index.tsx (1)
getLanguageDisplayName(56-56)
src/components/LyricsDisplay.test.tsx (1)
src/components/LyricsDisplay.tsx (1)
LyricsDisplay(46-190)
🔇 Additional comments (6)
src/components/LyricsDisplay.tsx (1)
21-26: LGTM — LanguageFilter type correctly renamed from "persian" to "original".The type update is clean and aligns with the PR objective.
src/hooks/useAnonymousProgress.ts (1)
216-220: LGTM — Backward-compatible normalization of stored "persian" to "original".The nested ternary correctly handles the migration path:
"persian"→"original", other strings pass through, non-strings default to"all".src/routes/song.$songId.tsx (2)
52-63: LGTM —normalizeLanguageFilterprovides clean backward compatibility.Correctly maps legacy
"persian"to"original", validates known filter values, and falls back to"all".
1116-1119: Good UX: dynamic "Original" label usesgetLanguageDisplayName.The language filter dropdown shows the actual source language name (e.g., "Persian Only", "Korean Only") instead of a generic "Original Only", which is a nice touch for multi-language support.
src/components/LyricsDisplay.test.tsx (1)
169-192: Good coverage of the renamed "original" filter.The test correctly verifies that
languageFilter="original"shows only the original text and hides transliteration, Hebrew, and English. This directly validates the core rename from "persian" to "original".src/__tests__/songPageDefaults.test.ts (1)
1-48: These tests are structure assertions without coverage for thenormalizeLanguageFilterfunction; consider adding behavior-based testing.The tests read and match exact substrings from the source file—a brittle pattern by design. The file correctly uses Vitest and follows integration test conventions. However, the
normalizeLanguageFilterfunction (which handles the "persian" → "original" mapping) is not tested anywhere. Since this function is private and not exported, direct unit testing is not feasible. Instead, consider testing the backward-compatibility behavior indirectly through page rendering assertions or adding a dedicated integration test that verifies the language filter logic through user interactions (e.g., setting preferences with deprecated "persian" value and confirming it's normalized to "original").
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { render, screen, fireEvent } from "@testing-library/react"; | ||
| import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; | ||
| import LyricsDisplay from "./LyricsDisplay"; | ||
|
|
||
| // Mock the convexQuery function | ||
| vi.mock('@convex-dev/react-query', () => ({ | ||
| vi.mock("@convex-dev/react-query", () => ({ | ||
| convexQuery: vi.fn(() => ({ | ||
| queryKey: ['lyrics', 'test-song-id'], | ||
| queryKey: ["lyrics", "test-song-id"], | ||
| queryFn: vi.fn(), | ||
| })), | ||
| })) | ||
| })); | ||
|
|
||
| // Mock useSuspenseQuery to return mock lyrics data | ||
| vi.mock('@tanstack/react-query', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('@tanstack/react-query')>() | ||
| vi.mock("@tanstack/react-query", async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import("@tanstack/react-query")>(); | ||
| return { | ||
| ...actual, | ||
| useSuspenseQuery: vi.fn(() => ({ | ||
| data: [ | ||
| { | ||
| _id: 'lyric1', | ||
| songId: 'test-song-id', | ||
| _id: "lyric1", | ||
| songId: "test-song-id", | ||
| lineNumber: 1, | ||
| startTime: 14.81, | ||
| endTime: 17.46, | ||
| original: 'برای توی کوچه رقصیدن', | ||
| transliteration: 'Barāye tūye kūche raqsidan', | ||
| hebrew: 'בָּרָאיֶה טוּיֶה כּוּצֶ׳ה רַקְסִידַן', | ||
| english: 'For dancing in the alley', | ||
| original: "برای توی کوچه رقصیدن", | ||
| transliteration: "Barāye tūye kūche raqsidan", | ||
| hebrew: "בָּרָאיֶה טוּיֶה כּוּצֶ׳ה רַקְסִידַן", | ||
| english: "For dancing in the alley", | ||
| }, | ||
| { | ||
| _id: 'lyric2', | ||
| songId: 'test-song-id', | ||
| _id: "lyric2", | ||
| songId: "test-song-id", | ||
| lineNumber: 2, | ||
| startTime: 17.46, | ||
| endTime: 20.91, | ||
| original: 'برای ترسیدن به وقت بوسیدن', | ||
| transliteration: 'Barāye tarsidan be vaqt-e būsidan', | ||
| hebrew: 'בָּרָאיֶה טַרְסִידַן בֶּה וַקְטֶה בּוּסִידַן', | ||
| english: 'For being afraid at the moment of kissing', | ||
| original: "برای ترسیدن به وقت بوسیدن", | ||
| transliteration: "Barāye tarsidan be vaqt-e būsidan", | ||
| hebrew: "בָּרָאיֶה טַרְסִידַן בֶּה וַקְטֶה בּוּסִידַן", | ||
| english: "For being afraid at the moment of kissing", | ||
| }, | ||
| { | ||
| _id: 'lyric3', | ||
| songId: 'test-song-id', | ||
| _id: "lyric3", | ||
| songId: "test-song-id", | ||
| lineNumber: 3, | ||
| startTime: 20.91, | ||
| endTime: 24.63, | ||
| original: 'برای خواهرم خواهرت خواهرامون', | ||
| transliteration: 'Barāye khāharam khāharet khāharāmūn', | ||
| original: "برای خواهرم خواهرت خواهرامون", | ||
| transliteration: "Barāye khāharam khāharet khāharāmūn", | ||
| // No Hebrew for this line to test optional field | ||
| english: 'For my sister, your sister, our sisters', | ||
| english: "For my sister, your sister, our sisters", | ||
| }, | ||
| ], | ||
| })), | ||
| } | ||
| }) | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests don't cover the new checkbox/learned-state UI or info button.
The LyricsDisplay component now has significant interactive elements (checkbox toggle, info button) that aren't covered by these tests. While those elements may predate this PR, the coding guidelines state that "Components with logic MUST have corresponding test files." Consider adding tests for onLineCheckboxClick and onLineInfoClick callbacks. As per coding guidelines, **/*.tsx: "Components with logic MUST have corresponding test files following the naming convention ComponentName.test.tsx".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/LyricsDisplay.test.tsx` around lines 1 - 57, Add tests in
LyricsDisplay.test.tsx to cover the checkbox and info-button interactions:
render LyricsDisplay (wrapped with QueryClientProvider if needed), pass vi.fn()
spies for onLineCheckboxClick and onLineInfoClick props, use testing-library
fireEvent or userEvent to click the checkbox for a specific lyric and the info
button for that lyric, and assert the corresponding spy was called with the
expected lyric id or payload; locate handlers by prop names onLineCheckboxClick
and onLineInfoClick and target elements by role/label/text matching the line
content used in the mocked data.
| @@ -51,27 +56,27 @@ export default function LyricsDisplay({ | |||
| }: LyricsDisplayProps) { | |||
| const isOriginalRTL = isRTLLanguage(sourceLanguage || "persian"); | |||
There was a problem hiding this comment.
Hardcoded "persian" fallback is inconsistent with the rename.
The PR renames the language filter from "persian" to "original" throughout, but this line still defaults to "persian" when sourceLanguage is undefined. While isRTLLanguage does handle "persian" correctly, this is the one remaining hardcoded "persian" string in the component. If the app is meant to support Korean, Arabic, Hebrew, etc. as source languages, defaulting to "persian" may also produce incorrect RTL detection for non-Persian songs that lack sourceLanguage.
Suggested fix
- const isOriginalRTL = isRTLLanguage(sourceLanguage || "persian");
+ const isOriginalRTL = isRTLLanguage(sourceLanguage || "");This way, songs without a sourceLanguage default to LTR (which is the safer fallback for unknown languages), rather than assuming Persian/RTL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/LyricsDisplay.tsx` at line 57, Replace the hardcoded "persian"
fallback used when computing RTL with the new renamed filter value so unknown
source languages default to LTR: update the isOriginalRTL assignment (the
variable isOriginalRTL that calls isRTLLanguage(sourceLanguage || "persian")) to
use "original" (or otherwise pass an empty/undefined value) instead of "persian"
so the RTL check aligns with the rename and treats unknown languages as LTR.
| isSeekingRef.current = true; | ||
| playerRef.current?.seekTo(sortedLyrics[lineIndexToPlay].startTime); | ||
| setTimeout(() => { | ||
| isSeekingRef.current = false; | ||
| }, 200); | ||
| playerRef.current?.play(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get file size first to understand scope
wc -l src/routes/song.$songId.tsxRepository: EtanHey/songscript
Length of output: 113
🏁 Script executed:
# Read the specific sections mentioned in the review
# Lines 506-511 (snippet in review)
sed -n '500,515p' src/routes/song.$songId.tsx | cat -n
echo "---"
# Lines 615-619 (mentioned pattern)
sed -n '610,625p' src/routes/song.$songId.tsx | cat -n
echo "---"
# Lines 643 (seekTo helper)
sed -n '640,650p' src/routes/song.$songId.tsx | cat -n
echo "---"
# Lines 773-777 (mentioned pattern)
sed -n '768,785p' src/routes/song.$songId.tsx | cat -n
echo "---"
# Lines 517-525 (dependency array)
sed -n '515,530p' src/routes/song.$songId.tsx | cat -nRepository: EtanHey/songscript
Length of output: 414
🏁 Script executed:
# Search for all instances of the manual seek-with-guard pattern
rg -n 'isSeekingRef\.current = true' src/routes/song.\$songId.tsx
echo "---"
rg -n 'isSeekingRef\.current = false' src/routes/song.\$songId.tsxRepository: EtanHey/songscript
Length of output: 397
🏁 Script executed:
fd 'song.*songId.tsx' src/routes/Repository: EtanHey/songscript
Length of output: 89
🏁 Script executed:
# List files in src/routes/ to see the actual filename
ls -la src/routes/ | grep songRepository: EtanHey/songscript
Length of output: 129
🏁 Script executed:
# Read the sections with proper escaping
sed -n '500,520p' 'src/routes/song.$songId.tsx' | cat -n
echo "---LINES 615-620---"
sed -n '610,625p' 'src/routes/song.$songId.tsx' | cat -n
echo "---LINES 640-655---"
sed -n '640,655p' 'src/routes/song.$songId.tsx' | cat -n
echo "---LINES 770-780---"
sed -n '770,780p' 'src/routes/song.$songId.tsx' | cat -nRepository: EtanHey/songscript
Length of output: 2593
🏁 Script executed:
# Find the seekTo function definition
rg -n 'const seekTo' 'src/routes/song.$songId.tsx'Repository: EtanHey/songscript
Length of output: 114
🏁 Script executed:
# Find the handlePlaybackModeChange function and its dependency array
rg -n 'handlePlaybackModeChange' 'src/routes/song.$songId.tsx' -A 30Repository: EtanHey/songscript
Length of output: 4255
🏁 Script executed:
# Get the full dependency array for handlePlaybackModeChange
sed -n '487,530p' 'src/routes/song.$songId.tsx' | cat -nRepository: EtanHey/songscript
Length of output: 1689
🏁 Script executed:
# Check context around the second instance (lines 610-625) to understand which callback it is
sed -n '600,630p' 'src/routes/song.$songId.tsx' | cat -nRepository: EtanHey/songscript
Length of output: 1102
🏁 Script executed:
# Check context around the fourth instance (lines 765-785)
sed -n '760,790p' 'src/routes/song.$songId.tsx' | cat -nRepository: EtanHey/songscript
Length of output: 1324
Duplicated seek-with-guard pattern — refactor to use the seekTo helper.
The seekTo helper (line 643) encapsulates the isSeekingRef guard + setTimeout clear pattern, but this same pattern is manually inlined at:
- Lines 506–511 in
handlePlaybackModeChange - Lines 615–620 in
handleLyricLineClick - Lines 773–778 in
handleWordModalClose
This creates a maintenance risk if the guard timing needs adjusting.
However, note that seekTo is defined after handlePlaybackModeChange (line 487). To refactor handlePlaybackModeChange, you'll need to either move the seekTo definition before it or restructure the code. The other two instances (in handleLyricLineClick and handleWordModalClose) can use seekTo directly since they're defined after it. If refactoring handlePlaybackModeChange, add seekTo to its dependency array once defined.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/song`.$songId.tsx around lines 506 - 511, Refactor duplicated
seek-guard blocks to use the existing seekTo helper: replace the inline pattern
in handlePlaybackModeChange, handleLyricLineClick, and handleWordModalClose with
calls to seekTo(sortedLyrics[lineIndexToPlay].startTime) (or the appropriate
startTime for the clicked/closed line) and remove the manual
isSeekingRef/current and setTimeout logic; to fix handlePlaybackModeChange you
must move the seekTo function definition above handlePlaybackModeChange (or
hoist it) and then add seekTo to handlePlaybackModeChange’s dependency array so
hooks remain correct; keep references to playerRef, isSeekingRef, sortedLyrics
and lineIndexToPlay as before.
| const handleToggleWordLearned = useCallback( | ||
| (wordId: Id<"words">, persian: string) => { | ||
| if (isAuthenticated) { | ||
| // Authenticated: use Convex mutation | ||
| toggleWordLearnedMutation({ wordId, persian }).then( | ||
| (newLearnedState) => { | ||
| if (newLearnedState) { | ||
| logPracticeMutation({ eventType: "word_learned", value: 1 }); | ||
| } | ||
| }, | ||
| ); | ||
| } else { | ||
| // Anonymous: use localStorage via useProgress hook | ||
| toggleWordLearnedFn(persian, wordId); | ||
| } | ||
| }, | ||
| [ | ||
| isAuthenticated, | ||
| toggleWordLearnedMutation, | ||
| logPracticeMutation, | ||
| toggleWordLearnedFn, | ||
| ], | ||
| ); |
There was a problem hiding this comment.
Missing error handling on toggleWordLearnedMutation promise.
The .then() handles the success case, but there's no .catch(). If the mutation fails, the user gets no feedback and the word's learned state may be out of sync.
Suggested fix
toggleWordLearnedMutation({ wordId, persian }).then(
(newLearnedState) => {
if (newLearnedState) {
logPracticeMutation({ eventType: "word_learned", value: 1 });
}
},
- );
+ ).catch((err) => {
+ console.error("Failed to toggle word learned state:", err);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleToggleWordLearned = useCallback( | |
| (wordId: Id<"words">, persian: string) => { | |
| if (isAuthenticated) { | |
| // Authenticated: use Convex mutation | |
| toggleWordLearnedMutation({ wordId, persian }).then( | |
| (newLearnedState) => { | |
| if (newLearnedState) { | |
| logPracticeMutation({ eventType: "word_learned", value: 1 }); | |
| } | |
| }, | |
| ); | |
| } else { | |
| // Anonymous: use localStorage via useProgress hook | |
| toggleWordLearnedFn(persian, wordId); | |
| } | |
| }, | |
| [ | |
| isAuthenticated, | |
| toggleWordLearnedMutation, | |
| logPracticeMutation, | |
| toggleWordLearnedFn, | |
| ], | |
| ); | |
| const handleToggleWordLearned = useCallback( | |
| (wordId: Id<"words">, persian: string) => { | |
| if (isAuthenticated) { | |
| // Authenticated: use Convex mutation | |
| toggleWordLearnedMutation({ wordId, persian }).then( | |
| (newLearnedState) => { | |
| if (newLearnedState) { | |
| logPracticeMutation({ eventType: "word_learned", value: 1 }); | |
| } | |
| }, | |
| ).catch((err) => { | |
| console.error("Failed to toggle word learned state:", err); | |
| }); | |
| } else { | |
| // Anonymous: use localStorage via useProgress hook | |
| toggleWordLearnedFn(persian, wordId); | |
| } | |
| }, | |
| [ | |
| isAuthenticated, | |
| toggleWordLearnedMutation, | |
| logPracticeMutation, | |
| toggleWordLearnedFn, | |
| ], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/song`.$songId.tsx around lines 822 - 844, The
handleToggleWordLearned callback calls toggleWordLearnedMutation but never
handles failures; update handleToggleWordLearned to add a .catch on the
toggleWordLearnedMutation promise (or use try/catch if converting to async) that
logs the error (e.g., console.error or processLogger), shows user-facing
feedback (toast or set an error state) and, if you applied any optimistic UI
changes, revert them; reference the toggleWordLearnedMutation call inside
handleToggleWordLearned and also keep logPracticeMutation and
toggleWordLearnedFn behavior unchanged for the success path.
1 participant
User description
Automated improvement by Golems Night Shift.
rename LanguageFilter "persian" to "original"
PR Type
Enhancement, Bug fix
Description
Rename LanguageFilter "persian" to "original" for multi-language support
Add backward compatibility layer normalizing stored "persian" values to "original"
Apply code formatting and style improvements across multiple files
Update test cases to reflect new "original" filter naming convention
Diagram Walkthrough
File Walkthrough
LyricsDisplay.tsx
Update LanguageFilter type and rename persian to originalsrc/components/LyricsDisplay.tsx
LanguageFiltertype from "persian" to "original"languageFilter === "persian"tolanguageFilter === "original"song.$songId.tsx
Add backward compatibility for language filter normalizationsrc/routes/song.$songId.tsx
normalizeLanguageFilter()function to convert legacy "persian"values to "original"
of "persian"
useAnonymousProgress.ts
Normalize persian to original in progress validationsrc/hooks/useAnonymousProgress.ts
validateProgress()to convert stored"persian" values to "original"
data validation
LyricsDisplay.test.tsx
Update test to use original filter namingsrc/components/LyricsDisplay.test.tsx
persian" to "shows only original text when filter is original"
semicolons
songPageDefaults.test.ts
Apply code formatting improvements to test filesrc/tests/songPageDefaults.test.ts
semicolons
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests