Implement ranking feature with SSR #9
Conversation
- Add ranking route with loader for SSR - Query top 50 scores from last 7 days from D1 - Add code language filter tabs (all, javascript, python, php, ruby, java, dart) - Add ranking translations for ja and en locales - Display ranking with player name, score, accuracy, and date - Add HTML title with i18n support - Link to individual result pages from ranking
|
Caution Review failedThe pull request is closed. WalkthroughAdds a language-aware ranking page: new route and route file, loader that queries top-50 scores from the last 7 days (optional code_language filter), localization keys for ranking in English and Japanese, and updates navigation links to use language-prefixed routes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Route as "/:lang/ranking"
participant Loader
participant DB as Database
participant UI as Ranking UI
User->>Browser: Open /{lang}/ranking
Browser->>Route: request route
Route->>Loader: invoke loader(params, request, context)
Loader->>Loader: validate lang (ja|en)\nparse code filter
Loader->>DB: query top 50 scores (last 7 days, optional code_language)
DB-->>Loader: scores[]
Loader-->>Route: return { scores, codeLanguage, lang }
Route->>UI: render with useLoaderData()
UI-->>User: display localized table or no-data message
rect rgba(120,180,160,0.08)
Note over UI: highlights: top-3 badges, locale-aware date formatting, language tabs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/routes/ranking.tsx (2)
28-65: Consider adding database indexes for query performance.The ranking queries filter by
created_at,code_language, and order byscore DESC. For optimal performance with the top-50 queries, ensure thescorestable has appropriate indexes:
- For the "all languages" query: Index on
(created_at, score DESC)- For specific language queries: Index on
(code_language, created_at, score DESC)Example index creation (run these as migrations):
-- For all languages query CREATE INDEX IF NOT EXISTS idx_scores_created_score ON scores(created_at, score DESC); -- For language-specific queries CREATE INDEX IF NOT EXISTS idx_scores_lang_created_score ON scores(code_language, created_at, score DESC);
197-202: Consider usingLinkcomponents for non-data-loading navigation.Lines 197-202 (player links) and 226-231 (back to home button) use
<a>tags, which trigger full page reloads. Since these navigations don't require server-side data fetching for the ranking page, using React Router's<Link>component would provide smoother client-side navigation.The language filter tabs (lines 128-141) appropriately use
<a>tags since they need to reload ranking data from the server.Example refactor:
+import { Link } from 'react-router'; // Player name link (line 197-202) - <a + <Link + to={`/result/${score.id}`} - href={`/result/${score.id}`} className="text-sky-600 dark:text-sky-400 hover:underline" > {score.player_name || t('ranking.anonymous', lang)} - </a> + </Link> // Back button (line 226-231) - <a - href={`/${lang}`} + <Link + to={`/${lang}`} className="px-6 py-3 rounded-md bg-sky-500 text-white hover:bg-sky-600 active:bg-sky-700 transition font-medium" > {t('nav.home', lang)} - </a> + </Link>Also applies to: 226-231
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/locales/en.json(1 hunks)app/locales/ja.json(1 hunks)app/routes.ts(1 hunks)app/routes/ranking.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/routes/ranking.tsx (2)
app/locales/index.ts (3)
SupportedLanguage(8-8)detectLanguage(32-56)t(21-24)app/components/Header.tsx (1)
Header(9-68)
🔇 Additional comments (5)
app/locales/en.json (1)
23-34: LGTM! Translation keys properly added.The ranking translation keys are well-structured and cover all UI elements in the ranking page. The trailing comma on line 23 correctly maintains JSON syntax.
app/locales/ja.json (1)
23-34: LGTM! Japanese translations properly added.The Japanese translations for ranking keys are well-formatted and align with the English version. JSON syntax is correct.
app/routes/ranking.tsx (3)
18-26: LGTM! Meta function properly handles i18n.The meta function correctly generates localized page titles using the i18n system and handles language detection appropriately.
87-99: LGTM! Good defensive date formatting.The
formatDatefunction properly handles locale-specific formatting with appropriate error handling. The try-catch ensures the app won't crash on malformed date strings.
177-218: LGTM! Ranking table rendering is well-implemented.The table rendering logic properly handles rank display with special styling for the top 3 positions, includes appropriate hover states, and correctly formats accuracy percentages.
| function getCodeLanguageDisplay(code: string): string { | ||
| const map: Record<string, string> = { | ||
| all: 'All', | ||
| javascript: 'JavaScript', | ||
| python: 'Python', | ||
| php: 'PHP', | ||
| ruby: 'Ruby', | ||
| java: 'Java', | ||
| dart: 'Dart', | ||
| }; | ||
| return map[code] || code; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace hardcoded English strings with i18n for consistency.
The getCodeLanguageDisplay function returns hardcoded English strings, which bypasses the app's i18n system and will display English language names even when the UI is in Japanese. The app already has language.* keys in the locale files for this purpose.
Apply this diff to use the i18n system:
-function getCodeLanguageDisplay(code: string): string {
- const map: Record<string, string> = {
- all: 'All',
- javascript: 'JavaScript',
- python: 'Python',
- php: 'PHP',
- ruby: 'Ruby',
- java: 'Java',
- dart: 'Dart',
- };
- return map[code] || code;
-}
+function getCodeLanguageDisplay(code: string, lang: SupportedLanguage): string {
+ return t(`language.${code}`, lang);
+}Then update the call site at line 205:
- {getCodeLanguageDisplay(score.code_language)}
+ {getCodeLanguageDisplay(score.code_language, lang)}Committable suggestion skipped: line range outside the PR's diff.
- Move ranking route from /ranking to /:lang/ranking - Update ranking page to use params.lang instead of query params - Add ranking link to home page (Welcome component) - Update result page ranking link to include locale - Remove unused useLocation and query param handling
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/routes/$lang.ranking.tsx (1)
72-83: Use i18n for code language labels instead of hardcoded English
getCodeLanguageDisplaycurrently returns hardcoded English strings:function getCodeLanguageDisplay(code: string): string { const map: Record<string, string> = { all: 'All', javascript: 'JavaScript', python: 'Python', php: 'PHP', ruby: 'Ruby', java: 'Java', dart: 'Dart', }; return map[code] || code; }And the ranking table uses it at the call site:
<td className="py-3 px-4 text-sm"> {getCodeLanguageDisplay(score.code_language)} </td>Everywhere else (e.g., the filter tabs) already uses
t('language.*', lang), so this bypasses your i18n layer and forces English labels even in Japanese UI.You can unify this with your existing locale keys:
function getCodeLanguageDisplay(code: string, lang: SupportedLanguage): string { return t(`language.${code}`, lang); }And update the table cell:
<td className="py-3 px-4 text-sm"> {getCodeLanguageDisplay(score.code_language, lang)} </td>This keeps language names consistent with the rest of the app.
Also applies to: 175-204
🧹 Nitpick comments (1)
app/routes/$lang.ranking.tsx (1)
19-25: Normalizelanginmetato avoid callingtwith unsupported locales
metacurrently does:const lang = params.lang as SupportedLanguage; const title = t('ranking.title', lang);If someone manually navigates to an unsupported locale like
/fr/ranking,langwill be'fr'at runtime, andtwill receive a value outsideSupportedLanguage, which can lead to runtime errors in the locale lookup.To be defensive and keep behavior aligned with the loader, normalize or validate here as well, for example:
function normalizeLang(raw: string | undefined): SupportedLanguage { return raw === 'ja' ? 'ja' : 'en'; } export function meta({ params }: Route.MetaArgs) { const lang = normalizeLang(params.lang); const title = t('ranking.title', lang); const period = t('ranking.period.week', lang); return [{ title: `${title} (${period}) | Bug Sniper` }]; }You can then reuse the same helper in the loader to keep language handling centralized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/routes.ts(1 hunks)app/routes/$lang.ranking.tsx(1 hunks)app/routes/result.$id.tsx(1 hunks)app/welcome/welcome.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/welcome/welcome.tsx (1)
app/locales/index.ts (1)
t(21-24)
app/routes/$lang.ranking.tsx (3)
app/routes/result.$id.tsx (1)
loader(60-83)app/locales/index.ts (2)
SupportedLanguage(8-8)t(21-24)app/components/Header.tsx (1)
Header(9-68)
🔇 Additional comments (3)
app/routes/result.$id.tsx (1)
240-251: Ranking link now correctly targets language-prefixed routeUsing
href={/${lang}/ranking}aligns with the new:lang/rankingroute and keeps navigation locale-aware from the result page. Looks good.app/welcome/welcome.tsx (1)
41-46: Welcome ranking CTA is wired consistently with routing and i18nThe new ranking Link (
to={/${lang}/ranking}witht('nav.ranking', lang)) matches the:lang/rankingroute and existing translation usage. No issues from a routing or UX perspective.app/routes.ts (1)
3-10: New:lang/rankingroute integrates cleanly with existing route patternsRegistering
route(':lang/ranking', 'routes/$lang.ranking.tsx')keeps the language-prefixed structure consistent with:langand:lang/:codeLanguage/play. The ordering among these specific paths is safe and should match as expected.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.