Staging#224
Conversation
- badges, sections - new user profile page will follow soon
- next up the badges and fixing few achievements
… architecture - set of 5 questions generated silently in the backdrop of the ai analysis of the refs - passing it with >= 80% marks the ref as passed and the badge and the achievement is assigned
Feat/achievments
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces a complete achievement, XP, and leveling system: Mongoose models ( ChangesAchievement & XP System
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
✅ Deploy Preview for system-craft-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (25)
Copilot/.frontendskills/Achivements.md-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFix filename typo:
Achivements.md→Achievements.mdThe filename is misspelled (missing 'e' after 'v'). This will propagate to all references, links, and documentation indices. Rename the file now before downstream references are created.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Copilot/.frontendskills/Achivements.md` at line 1, The file name is misspelled and should be corrected from Achivements.md to Achievements.md. Rename the markdown file itself, then update any references, links, or documentation indices that point to the current name so they use the corrected Achievements.md identifier.app/dashboard/reference-architectures/[id]/page.tsx-37-72 (1)
37-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRender
quizErrorinstead of spinning forever.
quizErroris assigned but never used; when quiz generation fails, the panel stays on “Preparing knowledge check...” with no recovery path.Proposed fix outline
<AnalysisPanel @@ isQuizCompleted={isQuizCompleted} + quizError={quizError} />isQuizCompleted + quizError }: { @@ isQuizCompleted: boolean; + quizError: string | null; }) {- ) : ( + ) : quizError ? ( + <div className="flex flex-col items-center gap-3 text-rose-400"> + <span className="material-symbols-outlined text-[18px]">error</span> + <span className="text-[10px] font-mono uppercase tracking-widest">{quizError}</span> + <button + onClick={() => setShowQuiz(false)} + className="px-4 py-2 rounded border border-rose-500/30 bg-rose-500/10 text-[10px] font-mono uppercase tracking-widest" + > + Back to Analysis + </button> + </div> + ) : ( <div className="flex items-center gap-2 text-white/40">Also applies to: 441-467
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/reference-architectures/`[id]/page.tsx around lines 37 - 72, The quiz generation failure state is being set in triggerQuizGeneration but never surfaced, so the UI can stay stuck in the loading message. Update the reference-architectures page component to render quizError in the quiz panel instead of always showing the spinner, and use the existing state/handlers like setQuizError, quizError, and triggerQuizGeneration to provide a visible fallback or retry path when the fetch fails.Source: Linters/SAST tools
app/dashboard/reference-architectures/[id]/page.tsx-42-51 (1)
42-51: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOnly mark the quiz completed after tracking succeeds.
authFetchdoes not throw on non-2xx responses, so a 401/500 from/api/user/trackstill shows “Verified Studied” even though completion was not recorded.Proposed fix
const handleQuizPass = useCallback(async () => { - setIsQuizCompleted(true); try { - await authFetch('/api/user/track', { + const response = await authFetch('/api/user/track', { method: 'POST', + headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ event: 'reference_architecture_completed' }), }); + if (!response.ok) { + throw new Error(`Failed to track completion (${response.status})`); + } + setIsQuizCompleted(true); } catch (err) { console.error('Failed to track completion:', err); } }, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/reference-architectures/`[id]/page.tsx around lines 42 - 51, In handleQuizPass, the completion state is being set before the tracking request succeeds, so a non-2xx authFetch response still marks the quiz as completed. Move the setIsQuizCompleted(true) update to after the await authFetch('/api/user/track', ...) call in the handleQuizPass callback, and only set it when the request returns successfully. Keep the existing error handling in the catch block so failed tracking does not update the UI state.components/dashboard/KnowledgeCheck.tsx-22-23 (1)
22-23: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard empty quizzes and derive the pass threshold.
questions[0]is undefined for an empty array, and a hard-coded threshold of4only works for exactly five questions.Proposed fix
- const currentQuestion = questions[currentQuestionIndex]; - const PASS_THRESHOLD = 4; // 80% of 5 + if (questions.length === 0) { + return ( + <div className="rounded-xl border border-white/[0.05] bg-[`#0c0d16`] p-6 text-center text-white/50"> + No knowledge-check questions are available. + </div> + ); + } + + const currentQuestion = questions[currentQuestionIndex]; + const PASS_THRESHOLD = Math.ceil(questions.length * 0.8);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/dashboard/KnowledgeCheck.tsx` around lines 22 - 23, Guard the quiz state in KnowledgeCheck by handling an empty questions array before reading currentQuestion and by deriving the pass threshold from questions.length instead of hard-coding 4. Update the logic around currentQuestion/currentQuestionIndex so an empty quiz does not access questions[0], and compute the passing score from the total number of questions to keep the component correct for any quiz size.app/api/reference-architectures/quiz/route.ts-101-122 (1)
101-122: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the LLM quiz schema before returning it.
The client assumes each question has
question,options, and a validcorrectIndex; returning raw model JSON can crash the quiz or create impossible scoring states.Proposed fix
const data = await response.json(); const content = data.choices?.[0]?.message?.content; @@ parsedContent = JSON.parse(cleanContent); } catch (e) { console.error('Failed to parse AI response as JSON:', content); return NextResponse.json({ error: 'Failed to parse AI response' }, { status: 500 }); } + + const questions = parsedContent?.questions; + const isValidQuiz = + Array.isArray(questions) && + questions.length === 5 && + questions.every((q) => + typeof q?.question === 'string' && + Array.isArray(q.options) && + q.options.length >= 2 && + q.options.every((option: unknown) => typeof option === 'string') && + Number.isInteger(q.correctIndex) && + q.correctIndex >= 0 && + q.correctIndex < q.options.length + ); + + if (!isValidQuiz) { + llmRequestsTotal.inc({ model: LLM_MODEL, status: 'error' }); + return NextResponse.json({ error: 'AI returned invalid quiz format' }, { status: 502 }); + } llmRequestsTotal.inc({ model: LLM_MODEL, status: 'success' });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/reference-architectures/quiz/route.ts` around lines 101 - 122, The quiz route currently returns parsed LLM JSON from the response handling block without checking that it matches the expected quiz shape. Add schema validation in the route handler after JSON.parse and before NextResponse.json so each item has question, options, and a valid correctIndex, and reject invalid payloads with a 500 response. Use the existing route logic around content parsing, parsedContent, and the final NextResponse.json return path to keep invalid model output from reaching the client.components/achievements/AchievementBadgeCard.tsx-37-44 (1)
37-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly hide secret achievements while they are still locked.
This branch hides every
hiddenachievement forever. Once a secret badge is unlocked, the card still renders as “Secret Achievement”, even though the page’s recent-unlocks panel already exposes the real badge metadata. Gate this placeholder onisLockedtoo.Suggested fix
- if (isSecret) { + if (isSecret && isLocked) { return ( <div className="rounded-xl border border-white/[0.04] bg-[`#0c0d16`]/30 p-4 font-mono flex flex-col items-center justify-center gap-2 h-40"> <span className="material-symbols-outlined text-[28px] text-white/10">lock</span> <p className="text-[8px] text-white/20 uppercase tracking-widest">Secret Achievement</p> </div> ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/achievements/AchievementBadgeCard.tsx` around lines 37 - 44, The secret-achievement placeholder in AchievementBadgeCard should only render while the badge is still locked, not for every hidden badge. Update the isSecret branch to also check isLocked so unlocked secret badges fall through to the normal card rendering and show their real metadata. Use the existing isSecret and isLocked logic in AchievementBadgeCard to keep the placeholder limited to locked secrets only.app/achievements/page.tsx-27-28 (1)
27-28: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSurface hook fetch failures instead of rendering an empty dashboard.
useAchievements()anduseMetrics()already exposeerror, but this page ignores them and falls back to[]/null. When either API fails, users get a legitimate-looking “0 badges unlocked” state instead of an error, which hides backend failures and misrepresents their progress.Suggested fix
- const { achievements, recentlyUnlocked, isLoading: achLoading } = useAchievements(); - const { xp, streaks, skills, isLoading: metricsLoading } = useMetrics(); + const { + achievements, + recentlyUnlocked, + isLoading: achLoading, + error: achievementsError, + refresh: refreshAchievements, + } = useAchievements(); + const { + xp, + streaks, + skills, + isLoading: metricsLoading, + error: metricsError, + refresh: refreshMetrics, + } = useMetrics(); + + const error = achievementsError ?? metricsError; ... + if (error) { + return ( + <div className="flex flex-col w-full bg-[`#060810`] min-h-screen"> + <Header /> + <main className="flex-1 p-6 md:p-8"> + <div className="max-w-[1400px] mx-auto rounded-xl border border-red-500/20 bg-red-500/10 p-4 font-mono text-xs text-red-300"> + Failed to load achievement data. + <button + onClick={() => { + refreshAchievements(); + refreshMetrics(); + }} + className="ml-3 underline" + > + Retry + </button> + </div> + </main> + </div> + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/achievements/page.tsx` around lines 27 - 28, The dashboard page is ignoring fetch errors from useAchievements and useMetrics, which lets failures masquerade as empty data. Update the page component to read the error values returned by those hooks and, in the main render path of app/achievements/page.tsx, show an explicit error state instead of defaulting to empty achievements or null metrics when either request fails. Keep the existing loading/data rendering logic in the same component, but gate it on the hook error fields from useAchievements and useMetrics so backend failures are surfaced clearly.app/dashboard/profile/page.tsx-13-16 (1)
13-16: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace the
anyescape hatch with the real metrics contract.Line 15 is already failing lint, and it is also masking a cross-file type mismatch:
/api/achievementsreturns mixed metrics (activityHeatmap,pinnedAchievements, numeric counters), so this object is not a genericRecord<string, number>. Please extract a sharedProfileMetricstype and use that here instead ofRecord<string, any>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/profile/page.tsx` around lines 13 - 16, The ProfileData metrics field is using an unsafe generic type and should be replaced with the shared real contract. Update the ProfileData interface in the Profile page to use a shared ProfileMetrics type instead of Record<string, any>, and define that type from the mixed metrics returned by /api/achievements (including activityHeatmap, pinnedAchievements, and numeric counters). Make sure the shared type is imported/used consistently anywhere metrics are consumed so the cross-file type mismatch is resolved.Source: Linters/SAST tools
app/dashboard/profile/page.tsx-23-23 (1)
23-23: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't make the first fetch failure permanent.
hasFetched.currentis flipped beforefetchProfile()succeeds. If the initial request fails, this page is stuck in the error state until a full remount because later renders will never retry. Reset the guard on failure, or only mark the user as fetched after a successful response.Also applies to: 51-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/profile/page.tsx` at line 23, The fetch guard in profile/page.tsx is set too early, causing a failed initial `fetchProfile()` call to block all future retries. Update the logic around `hasFetched` and `fetchProfile` so the ref is only marked after a successful response, or reset it in the failure path, and make sure the retry behavior in the effect/state handling that covers the referenced block also allows another attempt after an error.components/dashboard/profile/BadgeShowcase.tsx-39-43 (1)
39-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winShow unlocked secret badges.
The predicate on Line 43 removes every hidden achievement, but this block says only hidden locked achievements should be suppressed. As written, a secret badge never appears here even after unlock.
Suggested fix
return achievements - .filter((a) => !a.hidden) + .filter((a) => !a.hidden || a.highestTier !== null) .sort((a, b) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/dashboard/profile/BadgeShowcase.tsx` around lines 39 - 43, The filtering in BadgeShowcase’s useMemo currently removes every hidden achievement, which also hides secret badges after they are unlocked. Update the visible list logic so only hidden locked achievements are excluded, while hidden but unlocked achievements still pass through. Keep the sorting behavior in this block unchanged and adjust the predicate around achievements.filter accordingly.src/hooks/useAchievementNotifications.ts-21-24 (1)
21-24: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReset notification session state per authenticated user.
Because this hook is mounted globally via
app/layout.tsx:45-48,queue,seenKeysRef, andsessionStartRefsurvive sign-out/sign-in cycles. That can show the previous user's pending toast and suppress the next user's unlock toast when they hit the sameachievementId::tierkey.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useAchievementNotifications.ts` around lines 21 - 24, Reset the notification session state when the authenticated user changes in useAchievementNotifications, since queue, seenKeysRef, and sessionStartRef currently persist across sign-out/sign-in. Update the hook to detect changes from useAuth().user and clear the ToastItem queue, reset the seen keys Set, and reinitialize the session start timestamp for each new user so previous users’ toasts and deduping state do not carry over.src/hooks/useMetrics.ts-54-77 (1)
54-77: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDiscard stale metrics across auth changes.
This has the same problem as
useAchievements:!userexits before resetting any state, and a late response from the previous session can still repopulate XP, skills, heatmap, and pinned achievements after logout/account switching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useMetrics.ts` around lines 54 - 77, The useMetrics fetch callback leaves old metrics in state when user becomes null, and a late response from a previous session can still overwrite the current state. Update the fetch logic in useMetrics so that the !user path clears all metric-related state (XP, streaks, heatmap, skills, pinnedAchievements, levels) before returning, and add a cancellation/ignore guard in fetch and useEffect so responses from prior auth sessions are discarded after logout or account switching.src/hooks/useAchievements.ts-56-75 (1)
56-75: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDiscard stale achievement data across auth changes.
When
userbecomesnullthis returns without clearing state, and an older in-flight request can still callsetAchievements/setRecentlyUnlockedafter a logout or account switch. That leaks the previous user's achievement state into the next session until another fetch wins the race.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useAchievements.ts` around lines 56 - 75, The fetch logic in useAchievements should clear stale achievement state when auth changes and prevent older requests from winning after logout/account switches. Update the fetch callback and the useEffect/user flow so that when user is null you reset achievements, recentlyUnlocked, error, and loading state, and add request cancellation or a guard in useCallback/useEffect to ignore late authFetch responses before calling setAchievements or setRecentlyUnlocked.src/hooks/useTrackEvent.ts-41-46 (1)
41-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle HTTP failures from
/api/user/track.
authFetch(...).catch(...)only covers network exceptions. A 4xx/5xx response still resolves, so missed XP/metric writes are silently treated as success and the event is lost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useTrackEvent.ts` around lines 41 - 46, The tracking call in useTrackEvent only handles rejected promises, so HTTP 4xx/5xx responses from authFetch('/api/user/track') are treated as success. Update the useTrackEvent flow to inspect the returned response and explicitly treat non-OK statuses as failures, logging the event name and status/error details before considering the track write successful.src/lib/achievements/engine.ts-94-97 (1)
94-97: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRe-run evaluation after derived metrics change.
Achievements like
distributed_mindandplanet_scaleare evaluated before their source metrics are recomputed, so the final unlock is missed until a later unrelated event triggers another evaluation.Also applies to: 106-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/achievements/engine.ts` around lines 94 - 97, The achievement flow in engine.ts updates derived metrics after unlocking, but does not re-run evaluation, so achievements like distributed_mind and planet_scale can be missed until a later event. In the unlock/evaluation path around the newlyUnlocked handling, call the achievement evaluation logic again after updateDerivedMetrics(userId) completes, and make sure the same fix is applied to the related code path in the 106-137 range so source-metric-driven achievements are checked against the recomputed values.src/lib/achievements/engine.ts-115-126 (1)
115-126: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCount all scalability achievements for
planet_scale.
planet_scalesays “Complete all scalability achievements,” but this set only checks two IDs. It ignoresauto_scaling,high_throughput, andglobal_infrastructure.Suggested fix
- const scalabilityAchievements = new Set(['horizontal_hero', 'million_user_ready']); + const scalabilityAchievements = new Set( + ACHIEVEMENT_DEFINITIONS + .filter((d) => d.category === 'scalability') + .map((d) => d.id) + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/achievements/engine.ts` around lines 115 - 126, The scalability completion check in the achievement engine is only counting a subset of scalability badges, so `planet_scale` can be marked complete too early. Update the `scalabilityAchievements` set and the `scalabilityCompleted` logic in `src/lib/achievements/engine.ts` to include all scalability achievement IDs, including `auto_scaling`, `high_throughput`, and `global_infrastructure`, so the `scalabilityDone` result only becomes 1 when every required badge is unlocked.app/api/achievements/route.ts-30-45 (1)
30-45: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn a stable metrics DTO instead of the raw Mongo document.
New users get a curated baseline, but existing users get the full lean document. That makes the API shape inconsistent and exposes persistence fields like
_id,userId, and timestamps.Suggested fix
- const baseMetrics = metrics || { - totalXP: 0, - level: 1, - currentStreak: 0, - longestStreak: 0, - activityHeatmap: {}, - pinnedAchievements: [], - }; + const heatmapValue = metrics?.activityHeatmap as + | Map<string, number> + | Record<string, number> + | undefined; + const baseMetrics = { + totalXP: metrics?.totalXP ?? 0, + level: metrics?.level ?? 1, + currentStreak: metrics?.currentStreak ?? 0, + longestStreak: metrics?.longestStreak ?? 0, + activityHeatmap: heatmapValue instanceof Map + ? Object.fromEntries(heatmapValue) + : heatmapValue ?? {}, + pinnedAchievements: metrics?.pinnedAchievements ?? [], + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/achievements/route.ts` around lines 30 - 45, The achievements route currently returns a baseline object for new users but a raw lean Mongo document for existing users, which makes the API response shape inconsistent and exposes persistence fields. Update the response assembly in the achievements route so both branches map through the same stable metrics DTO (for example in the route handler before NextResponse.json), explicitly selecting only the public fields from UserMetrics and using the same shape for the default baseline and the found document.src/lib/achievements/trackMetric.ts-16-39 (1)
16-39: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestrict
MetricPatchto incrementable counters only.keyof IUserMetricsDataincludes engine-managed fields liketotalXP,level, streak state, and other derived unlock fields, so callers can$incthem through this API and corrupt metrics ownership. Narrow the key union to the counters thattrackMetric()is meant to accept.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/achievements/trackMetric.ts` around lines 16 - 39, `MetricPatch` is currently too broad because it uses `keyof IUserMetricsData`, which allows engine-managed and derived fields to be incremented through `trackMetric()`. Narrow the patch type in `trackMetric` to only the intended counter fields (the incrementable metrics) so callers cannot `$inc` `totalXP`, `level`, streak state, or unlock-derived fields; update the type alias and any related references to use that restricted key union.app/api/user/metrics/route.ts-55-55 (1)
55-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNormalize
activityHeatmapbefore serializing.findOne().lean()returns a plain object here, soObject.fromEntries(metrics.activityHeatmap ?? new Map())can throw whenactivityHeatmapisn’t aMap, turning this endpoint into a 500 for users with metrics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/user/metrics/route.ts` at line 55, Normalize metrics.activityHeatmap in the user metrics route before calling Object.fromEntries, because the lean() result is a plain object and not always a Map. Update the serialization logic in the route handler that builds the response payload so it safely handles both Map and object-shaped values (or converts the stored heatmap into a Map first) and avoids throwing when activityHeatmap is missing or already plain data.src/lib/achievements/trackMetric.ts-57-65 (1)
57-65: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake max-metric updates atomic.
setMetricIfHigher()reads the current value and then writes with$set, so concurrent requests can still overwrite a higher value with a lower one. Use a single atomic$maxupdate and only run the achievement evaluation when the stored value changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/achievements/trackMetric.ts` around lines 57 - 65, The setMetricIfHigher() flow is not atomic because it reads UserMetrics and then writes with $set, allowing concurrent requests to overwrite a higher value. Update the logic to use a single atomic $max operation on UserMetrics.findOneAndUpdate so only the greatest value is stored, and gate the achievement evaluation on whether the stored metric actually changed. Keep the fix localized to setMetricIfHigher() and its UserMetrics update path.app/api/user/achievements/route.ts-58-80 (1)
58-80: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate pinned achievement IDs before persisting.
The route currently accepts any array of up to 6 items, including duplicates, non-strings, unknown IDs, or achievements the user has not unlocked. Persist only unique unlocked achievement IDs.
Suggested fix
const body = await request.json(); const { pinnedAchievements } = body; - if (!Array.isArray(pinnedAchievements) || pinnedAchievements.length > 6) { + if ( + !Array.isArray(pinnedAchievements) || + pinnedAchievements.length > 6 || + !pinnedAchievements.every((id) => typeof id === 'string') || + new Set(pinnedAchievements).size !== pinnedAchievements.length + ) { return NextResponse.json( - { error: 'pinnedAchievements must be an array of up to 6 achievement IDs' }, + { error: 'pinnedAchievements must be a unique array of up to 6 achievement IDs' }, { status: 400 } ); } @@ if (!user) { return NextResponse.json({ error: 'User not found' }, { status: 404 }); } + + const unlockedIds = new Set( + await UserAchievement.distinct('achievementId', { + userId: user._id, + achievementId: { $in: pinnedAchievements }, + }) + ); + + if (!pinnedAchievements.every((id) => unlockedIds.has(id))) { + return NextResponse.json( + { error: 'Pinned achievements must be unlocked by the current user' }, + { status: 400 } + ); + } const UserMetrics = (await import('`@/src/lib/achievements/metrics`')).default;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/user/achievements/route.ts` around lines 58 - 80, The pinnedAchievements validation in the achievements route only checks array length, so tighten it before the UserMetrics update: in the request handler, dedupe the values, ensure every entry is a string, verify each ID exists in the achievements catalog and belongs to the authenticated user’s unlocked achievements, and reject invalid input with a 400 response. Keep the fix localized to the route logic around request.json, User.findOne, and UserMetrics.findOneAndUpdate so only unique unlocked achievement IDs are persisted.app/api/user/track/route.ts-89-93 (1)
89-93: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ai_review_completedis a no-op for activity tracking.Line 91 passes
{}intotrackMetric(), butsrc/lib/achievements/trackMetric.ts:22-35returns immediately when the increment payload is empty. That means AI reviews currently award XP only; they do not update streaks, heatmap data, or achievement evaluation despite the inline comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/user/track/route.ts` around lines 89 - 93, The ai_review_completed branch is not actually updating activity tracking because trackMetric() receives an empty payload and exits early. Update the ai_review_completed case in route.ts to pass a real metric/increment object into trackMetric() so trackMetric() can update streaks, heatmap data, and achievement evaluation while awardXP(userId, 'AI_REVIEW') still runs.app/api/user/track/route.ts-59-117 (1)
59-117: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDon’t let the client mint authoritative XP/achievement events.
This switch applies server-side rewards from caller-controlled
event/payloadvalues alone. Any authenticated user can replayreference_architecture_completed,ai_review_completed, or spoof values liketargetRpsto farm XP and unlock badges without completing the underlying action. Move reward writes to the server routes that already prove completion, or require a server-validated resource/state check before mutating metrics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/user/track/route.ts` around lines 59 - 117, The reward handling in the track route is trusting caller-controlled event and payload values, which lets users mint XP and unlocks by replaying events like simulation_executed, reference_architecture_completed, or ai_review_completed. Update the route’s switch logic so trackMetric and awardXP are only called after server-side validation from the authoritative completion flow, or move these mutations into the server routes that already verify the action. Use the existing trackMetric, awardXP, and setMetricIfHigher paths as the location to tighten validation before any metrics are mutated.app/api/interview/[id]/evaluate/route.ts-186-223 (1)
186-223: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t feed absolute snapshot metrics into
trackMetric().
trackMetric()persists this payload with$inc, butconsistentPerformerStreakandhighScoreAcrossDifficultiesare computed here as current absolute values. Incrementing by those snapshots will inflate them on every evaluation, so streak/distinct-difficulty achievements can unlock much earlier than intended.Possible fix
- const metricPatch: Record<string, number> = { + const metricPatch: Record<string, number> = { interviewsCompleted: 1, ...(highAverageUnlocked ? { highAverageUnlocked: 1 } : {}), ...(comebackUnlocked ? { comebackUnlocked: 1 } : {}), - consistentPerformerStreak: streakCount, - highScoreAcrossDifficulties: difficultiesWithHighScore.size, }; @@ await Promise.all([ trackMetric(user._id, metricPatch), awardXP(user._id, 'INTERVIEW_COMPLETED', bonusXP), + setMetricIfHigher(user._id, 'consistentPerformerStreak', streakCount), + setMetricIfHigher(user._id, 'highScoreAcrossDifficulties', difficultiesWithHighScore.size), setMetricIfHigher(user._id, 'maxNodesInSingleDesign', nodeCount), ]).catch((err) =>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/interview/`[id]/evaluate/route.ts around lines 186 - 223, The `metricPatch` built in `evaluate` is mixing snapshot values with increment-based metrics, causing `trackMetric()` to overcount `consistentPerformerStreak` and `highScoreAcrossDifficulties` because it applies `$inc`. Update the `evaluate` flow so `trackMetric()` only receives true delta-style counters, and move the absolute snapshot comparisons into separate logic that first checks the stored user metrics before deciding whether to increment. Use the existing `trackMetric`, `metricPatch`, and `setMetricIfHigher` call sites to keep streak and distinct-difficulty achievements from inflating on repeated evaluations.app/api/interview/[id]/evaluate/route.ts-125-225 (1)
125-225: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSkip achievement/XP writes on re-evaluation.
Earlier in this handler you also reclaim sessions whose prior status is already
evaluated, but this block unconditionally addsinterviewsCompleted, XP, and derived metrics again. Re-running evaluation on the same session can therefore farm progression from a single submission.Possible fix
- if (updated?.evaluation) { + if (updated?.evaluation && session.status !== 'evaluated') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/interview/`[id]/evaluate/route.ts around lines 125 - 225, The achievement/XP tracking block in the evaluate handler is incrementing progress again even when the same session is being re-evaluated. Add a guard in the evaluation flow around the metrics/XP write path so it only runs for a first-time completion, not when `updated.evaluation` is processed after an already-`evaluated` session is reclaimed. Use the existing session state and the `trackMetric`, `awardXP`, and `setMetricIfHigher` calls in this block to skip all progression writes on re-evaluation.
🟡 Minor comments (8)
Copilot/.frontendskills/Achivements.md-146-157 (1)
146-157: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid naming collision: "Architect" as both level title and secret achievement
"Architect" appears as a level title (line 265) and as a secret achievement (line 156). This will confuse users and complicate search/indexing. Rename the secret achievement to something distinct like "Secret Architect" or "Shadow Architect".
Also applies to: 241-273
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Copilot/.frontendskills/Achivements.md` around lines 146 - 157, The secret achievement list in Achivements.md includes “Architect”, which conflicts with the existing level title used elsewhere in the same document. Update the entry in the Secret Achievements section to a distinct name such as “Secret Architect” or “Shadow Architect”, and make sure any related references in the achievements list or indexing text use the same new identifier consistently.app/api/reference-architectures/quiz/route.ts-116-118 (1)
116-118: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse or remove the unused catch binding.
Line 116 currently triggers the lint/typecheck warning for unused
e.Proposed fix
- } catch (e) { - console.error('Failed to parse AI response as JSON:', content); + } catch (parseError) { + console.error('Failed to parse AI response as JSON:', parseError, content); return NextResponse.json({ error: 'Failed to parse AI response' }, { status: 500 }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/reference-architectures/quiz/route.ts` around lines 116 - 118, The catch block in the quiz route has an unused binding `e`, which is triggering the lint/typecheck warning. In the `route.ts` error handler around the JSON parse failure, either remove the catch parameter entirely or use it in the `console.error` message so the `catch` clause is consistent with the rest of the handler and no unused variable remains.Source: Linters/SAST tools
app/api/reference-architectures/quiz/route.ts-27-31 (1)
27-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn 400 for malformed or non-string request bodies.
req.json()parse failures currently fall through to the outer catch and return 500. Also validatetitleandanalysisas non-empty strings before interpolating them into the prompt.Proposed fix
- const { title, analysis } = await req.json(); + let body: unknown; + try { + body = await req.json(); + } catch { + return NextResponse.json({ error: 'Invalid JSON body' }, { status: 400 }); + } + + const { title, analysis } = body as { title?: unknown; analysis?: unknown }; - if (!title || !analysis) { + if (typeof title !== 'string' || !title.trim() || typeof analysis !== 'string' || !analysis.trim()) { return NextResponse.json({ error: 'Missing title or analysis data' }, { status: 400 }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/reference-architectures/quiz/route.ts` around lines 27 - 31, The quiz route currently lets malformed request bodies fail through the outer catch and also accepts non-string values for title and analysis; update the handler in the route’s req.json() parsing path to return a 400 when JSON parsing fails, and add explicit validation that title and analysis are non-empty strings before building the prompt. Keep the fix localized to the request handling logic in the route handler so malformed or invalid input is rejected with a client error instead of a 500.components/achievements/AchievementToast.tsx-117-121 (1)
117-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
onDismissstable for mounted toasts.
AchievementToastrestarts its timer effect wheneveronDismisschanges (Lines 31-43). This inline closure is recreated on every queue render, so adding or dismissing any toast resets the 5-second timer for all visible toasts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/achievements/AchievementToast.tsx` around lines 117 - 121, The inline onDismiss callback in the visible toast render is changing on every queue render, which causes AchievementToast’s timer effect to restart for all mounted toasts. Make onDismiss stable in the parent render path for AchievementToast by memoizing the per-item dismiss handler or extracting a stable handler helper around onDismiss(item.key), so each toast keeps the same prop identity while it remains mounted.src/hooks/useAchievements.ts-86-92 (1)
86-92: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSurface
updatePinnedfailures to callers.
app/api/user/achievements/route.ts:49-82rejects invalid saves (for example more than 6 IDs), but this helper resolves regardless because it never checksres.ok. The caller can't roll back optimistic UI or show an error when the pin update is rejected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useAchievements.ts` around lines 86 - 92, `updatePinned` in `useAchievements` currently hides failed pin saves because it ignores the fetch response. Update this helper to check the result of `authFetch` for the `/api/user/achievements` PATCH call and surface non-OK responses by throwing or otherwise rejecting so callers can handle rollback/error UI. Keep the change localized to `updatePinned` and make sure the caller can detect when the server rejects invalid pinnedAchievements updates.src/lib/achievements/engine.ts-15-15 (1)
15-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the unused
AchievementDefimport.The lint/typecheck check already flags this symbol as unused.
Suggested fix
-import { ACHIEVEMENT_DEFINITIONS, type AchievementDef, type AchievementTier } from './definitions'; +import { ACHIEVEMENT_DEFINITIONS, type AchievementTier } from './definitions';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/achievements/engine.ts` at line 15, Remove the unused AchievementDef import from the engine.ts module’s import list so the lint/typecheck check passes. Keep ACHIEVEMENT_DEFINITIONS and AchievementTier if they are still used, and update the import statement accordingly in the achievement engine module.Source: Linters/SAST tools
src/lib/achievements/definitions.ts-691-699 (1)
691-699: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the “System Whisperer” requirement with its threshold.
Line 694 says 5 difficulties, but Line 699 unlocks at 3. Update either the description or threshold so users see the actual requirement.
Suggested fix if 3 difficulties is intended
- description: 'Score 90+ on 5 different interview difficulties.', + description: 'Score 90+ on 3 different interview difficulties.',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/achievements/definitions.ts` around lines 691 - 699, The System Whisperer achievement metadata is inconsistent: the description in achievements/definitions.ts says “Score 90+ on 5 different interview difficulties,” while the tier threshold on the System Whisperer entry unlocks at 3. Update the System Whisperer definition so the description and the tiers threshold match, using the achievement object’s description and tiers fields to keep the requirement clear and consistent.app/api/interview/[id]/evaluate/route.ts-209-215 (1)
209-215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTighten the HA rule matcher.
/ha|high.avail/iwill match any occurrence ofha, so unrelated passed-rule text likeshardingcan sethighAvailabilityDesigns. This should match the actual HA phrases only.Possible fix
- if (passedRules.some((r: string) => /ha|high.avail/i.test(r))) metricPatch.highAvailabilityDesigns = 1; + if (passedRules.some((r: string) => /\b(?:ha|high[- ]?availability)\b/i.test(r))) { + metricPatch.highAvailabilityDesigns = 1; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/interview/`[id]/evaluate/route.ts around lines 209 - 215, The high-availability matcher in the evaluate route is too broad because the current /ha|high.avail/i pattern can match unrelated text like sharding. Update the rule check in the metric patch logic to match only actual HA phrases, using a stricter expression in the passedRules.some(...) condition for highAvailabilityDesigns so it only fires on explicit high-availability wording.
🧹 Nitpick comments (1)
Copilot/.frontendskills/Achivements.md (1)
165-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify tier progression requirement vs. implementation
The spec states "Every badge should support progression tiers," but
first_architecturein the implementation only has a single bronze tier. Either soften this to "Most badges" or add additional tiers to single-tier achievements likefirst_architecture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Copilot/.frontendskills/Achivements.md` at line 165, The achievements spec and implementation are out of sync because first_architecture currently defines only a single bronze tier while the text says every badge should support progression tiers. Update the Achivements.md entry for first_architecture to either add additional progression tiers consistent with the other badge definitions or relax the requirement wording to match the intended scope, and make sure the tier model used by the badge entries remains consistent across the document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae960ca5-e123-4a9d-9c9c-e5ba21b9df39
📒 Files selected for processing (35)
Copilot/.frontendskills/Achivements.mdapp/achievements/page.tsxapp/api/achievements/route.tsapp/api/designs/route.tsapp/api/interview/[id]/evaluate/route.tsapp/api/reference-architectures/quiz/route.tsapp/api/user/achievements/route.tsapp/api/user/metrics/route.tsapp/api/user/track/route.tsapp/dashboard/profile/page.tsxapp/dashboard/reference-architectures/[id]/page.tsxapp/layout.tsxcomponents/achievements/AchievementBadgeCard.tsxcomponents/achievements/AchievementNotificationProvider.tsxcomponents/achievements/AchievementToast.tsxcomponents/achievements/SkillBars.tsxcomponents/achievements/XPLevelBar.tsxcomponents/dashboard/Header.tsxcomponents/dashboard/KnowledgeCheck.tsxcomponents/dashboard/Sidebar.tsxcomponents/dashboard/profile/ActivityHeatmap.tsxcomponents/dashboard/profile/BadgeShowcase.tsxcomponents/dashboard/profile/LevelBanner.tsxcomponents/dashboard/profile/SkillProgress.tsxsrc/hooks/useAchievementNotifications.tssrc/hooks/useAchievements.tssrc/hooks/useMetrics.tssrc/hooks/useTrackEvent.tssrc/lib/achievements/definitions.tssrc/lib/achievements/engine.tssrc/lib/achievements/levels.tssrc/lib/achievements/metrics.tssrc/lib/achievements/trackMetric.tssrc/lib/achievements/userAchievement.tssrc/lib/achievements/xp.ts
fix(lint): lint issues and the unused imports and the ci pipeline
fix: Coderabbit review issues
fix: recurring lint issues
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Copilot/.frontendskills/Achievements.md (1)
203-277: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the XP examples in
Copilot/.frontendskills/Achievements.mdwithACHIEVEMENT_DEFINITIONS.Architecture Createdis+50 XPinsrc/lib/achievements/definitions.ts, so the+20 XPexample is stale; the tier unlock examples should also match the100/250/500/1000/2500rewards defined there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Copilot/.frontendskills/Achievements.md` around lines 203 - 277, The XP examples in Achievements.md are out of sync with ACHIEVEMENT_DEFINITIONS, so update the sample rewards to match the actual values defined in src/lib/achievements/definitions.ts. Specifically, correct the Architecture Created example to the current XP amount and revise the tier unlock examples so they reflect the real 100/250/500/1000/2500 reward thresholds used by the achievements definitions, keeping the README text aligned with the source of truth.app/dashboard/reference-architectures/[id]/page.tsx (1)
58-80: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReset and fence quiz generation to prevent stale completions.
A previous quiz can stay visible while a new analysis is running because
showQuiz && quizQuestionsrenders before the loading state. Since quiz generation is also fire-and-forget, an older quiz request can later overwrite the newer state; passing that stale quiz still postsreference_architecture_completedto/api/user/track.Proposed fix
const abortRef = useRef<AbortController | null>(null); + const quizRequestIdRef = useRef(0); const triggerQuizGeneration = useCallback(async (analysisText: string) => { if (!arch) return; + const requestId = ++quizRequestIdRef.current; + setQuizQuestions(null); + setShowQuiz(false); + setIsQuizCompleted(false); setQuizError(null); try { const response = await fetch('/api/reference-architectures/quiz', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ title: arch.title, analysis: analysisText }), }); + if (requestId !== quizRequestIdRef.current) return; if (response.ok) { const data = await response.json(); - if (data.questions) { + if (Array.isArray(data.questions) && data.questions.length > 0) { setQuizQuestions(data.questions); + } else { + setQuizError('Failed to generate knowledge check'); } } else { console.error('Failed to generate knowledge check'); setQuizError('Failed to generate knowledge check'); } } catch (err) { + if (requestId !== quizRequestIdRef.current) return; console.error(err); setQuizError('Failed to generate knowledge check'); } }, [arch]); const handleGenerateAnalysis = useCallback(async () => { if (!arch || isAnalysing) return; setPanelView('analysis'); setIsAnalysing(true); setAnalysisError(null); setAnalysisContent(''); + setQuizQuestions(null); + setShowQuiz(false); + setIsQuizCompleted(false); + setQuizError(null);Also applies to: 85-89, 128-128, 418-428, 459-472
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/reference-architectures/`[id]/page.tsx around lines 58 - 80, The quiz generation flow in triggerQuizGeneration, showQuiz/quizQuestions rendering, and the completion-tracking effect needs to be fenced so stale quiz responses cannot overwrite newer analyses. Clear or invalidate the previous quiz state when a new analysis starts, and associate each fetch in triggerQuizGeneration with the current analysis so only the latest response can call setQuizQuestions and trigger the reference_architecture_completed tracking. Also ensure the UI does not render an old quiz while loading a new one by gating the quiz display on the current generation/loading state.app/api/user/track/route.ts (1)
57-97: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDon't treat client-submitted track events as authoritative progress.
Lines 57-97 let any authenticated client mutate achievement metrics directly, and
chaos_constraints_addressed/max_nodes_in_designeven accept caller-supplied counts. A user can script this endpoint to mint badges and XP without actually completing the underlying action. These mutations need to live in the server-side routes that already own those actions, or require verifiable server-side evidence per event before updatingUserMetrics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/user/track/route.ts` around lines 57 - 97, The event handling in trackMetric/setMetricIfHigher is trusting client-submitted progress, which lets authenticated users forge achievements and XP. Move these updates out of the generic track route and into the server-side action routes that actually perform the work, or require verified server-side evidence before updating UserMetrics. Pay special attention to simulation_executed, chaos_constraints_addressed, and max_nodes_in_design so they no longer accept caller-controlled counts as authoritative.src/lib/achievements/engine.ts (1)
75-108: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake unlock creation and XP/level updates atomic.
Lines 75-108 split one logical unlock into separate writes. If a request fails after
UserAchievement.create(), later evaluations will skip that tier and the missing XP/level will never be repaired. Concurrent evaluators can also regresslevelbecause Line 108 writes a snapshot derived before other$inc totalXPoperations finish. This needs a transaction or a single idempotent source of truth for XP/level.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/achievements/engine.ts` around lines 75 - 108, Make the unlock write path in achievement engine atomic: the separate UserAchievement.create, UserMetrics.findOneAndUpdate, and UserMetrics.updateOne steps in engine.ts can leave a tier unlocked without its XP/level update or let concurrent evaluators overwrite a newer level. Refactor the unlock flow so the achievement insert and XP/level changes happen in one transactional unit or are driven by a single idempotent update path, using the existing UserAchievement and UserMetrics logic together so a partial failure cannot leave state inconsistent.
🧹 Nitpick comments (1)
src/lib/achievements/trackMetric.ts (1)
67-73: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip achievement evaluation after no-op
$maxupdates.Line 69 is a no-op when the stored value is already higher, but Line 73 still runs the full achievement scan. That turns unchanged high-watermark writes into extra reads over
UserMetrics,UserAchievement, and the full definition set. Short-circuit when the update did not actually insert or raise the metric.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/achievements/trackMetric.ts` around lines 67 - 73, Skip the achievement scan when trackMetric performs a no-op $max update. In trackMetric, inspect the result of UserMetrics.findOneAndUpdate for whether the metric was actually inserted or increased, and only call evaluateAchievements(userId) when the stored value changed. Use the existing trackMetric and evaluateAchievements flow to short-circuit unchanged high-watermark writes and avoid unnecessary reads over UserMetrics, UserAchievement, and the achievement definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/dashboard/reference-architectures/`[id]/page.tsx:
- Around line 472-481: The quiz error state currently shows a “Back to Analysis”
button that calls setShowQuiz(false), but this branch is already on the analysis
view so the action does nothing. Update the button in the quizError branch to
trigger a real recovery path, such as retrying the quiz fetch/start logic or
resetting the quiz state, using the existing state handlers around quizError and
setShowQuiz. If no retry path exists, remove the button entirely so the UI does
not present a dead action.
---
Outside diff comments:
In `@app/api/user/track/route.ts`:
- Around line 57-97: The event handling in trackMetric/setMetricIfHigher is
trusting client-submitted progress, which lets authenticated users forge
achievements and XP. Move these updates out of the generic track route and into
the server-side action routes that actually perform the work, or require
verified server-side evidence before updating UserMetrics. Pay special attention
to simulation_executed, chaos_constraints_addressed, and max_nodes_in_design so
they no longer accept caller-controlled counts as authoritative.
In `@app/dashboard/reference-architectures/`[id]/page.tsx:
- Around line 58-80: The quiz generation flow in triggerQuizGeneration,
showQuiz/quizQuestions rendering, and the completion-tracking effect needs to be
fenced so stale quiz responses cannot overwrite newer analyses. Clear or
invalidate the previous quiz state when a new analysis starts, and associate
each fetch in triggerQuizGeneration with the current analysis so only the latest
response can call setQuizQuestions and trigger the
reference_architecture_completed tracking. Also ensure the UI does not render an
old quiz while loading a new one by gating the quiz display on the current
generation/loading state.
In `@Copilot/.frontendskills/Achievements.md`:
- Around line 203-277: The XP examples in Achievements.md are out of sync with
ACHIEVEMENT_DEFINITIONS, so update the sample rewards to match the actual values
defined in src/lib/achievements/definitions.ts. Specifically, correct the
Architecture Created example to the current XP amount and revise the tier unlock
examples so they reflect the real 100/250/500/1000/2500 reward thresholds used
by the achievements definitions, keeping the README text aligned with the source
of truth.
In `@src/lib/achievements/engine.ts`:
- Around line 75-108: Make the unlock write path in achievement engine atomic:
the separate UserAchievement.create, UserMetrics.findOneAndUpdate, and
UserMetrics.updateOne steps in engine.ts can leave a tier unlocked without its
XP/level update or let concurrent evaluators overwrite a newer level. Refactor
the unlock flow so the achievement insert and XP/level changes happen in one
transactional unit or are driven by a single idempotent update path, using the
existing UserAchievement and UserMetrics logic together so a partial failure
cannot leave state inconsistent.
---
Nitpick comments:
In `@src/lib/achievements/trackMetric.ts`:
- Around line 67-73: Skip the achievement scan when trackMetric performs a no-op
$max update. In trackMetric, inspect the result of UserMetrics.findOneAndUpdate
for whether the metric was actually inserted or increased, and only call
evaluateAchievements(userId) when the stored value changed. Use the existing
trackMetric and evaluateAchievements flow to short-circuit unchanged
high-watermark writes and avoid unnecessary reads over UserMetrics,
UserAchievement, and the achievement definitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b4ceb30-d2f5-47dc-a008-0d4718eba023
📒 Files selected for processing (25)
Copilot/.errors/coderabbit.mdCopilot/.frontendskills/Achievements.mdapp/achievements/page.tsxapp/api/achievements/route.tsapp/api/interview/[id]/evaluate/route.tsapp/api/reference-architectures/quiz/route.tsapp/api/user/achievements/route.tsapp/api/user/metrics/route.tsapp/api/user/track/route.tsapp/dashboard/profile/page.tsxapp/dashboard/reference-architectures/[id]/page.tsxcoderabbit_summary.txtcoderabbit_titles.txtcomponents/achievements/AchievementBadgeCard.tsxcomponents/achievements/AchievementToast.tsxcomponents/dashboard/KnowledgeCheck.tsxcomponents/dashboard/profile/ActivityHeatmap.tsxcomponents/dashboard/profile/BadgeShowcase.tsxsrc/hooks/useAchievementNotifications.tssrc/hooks/useAchievements.tssrc/hooks/useMetrics.tssrc/hooks/useTrackEvent.tssrc/lib/achievements/definitions.tssrc/lib/achievements/engine.tssrc/lib/achievements/trackMetric.ts
💤 Files with no reviewable changes (1)
- components/dashboard/profile/ActivityHeatmap.tsx
✅ Files skipped from review due to trivial changes (1)
- coderabbit_titles.txt
🚧 Files skipped from review as they are similar to previous changes (15)
- src/hooks/useAchievementNotifications.ts
- components/dashboard/profile/BadgeShowcase.tsx
- app/api/achievements/route.ts
- src/hooks/useAchievements.ts
- src/lib/achievements/definitions.ts
- app/dashboard/profile/page.tsx
- components/dashboard/KnowledgeCheck.tsx
- src/hooks/useTrackEvent.ts
- app/api/user/achievements/route.ts
- src/hooks/useMetrics.ts
- components/achievements/AchievementToast.tsx
- app/api/user/metrics/route.ts
- app/achievements/page.tsx
- app/api/interview/[id]/evaluate/route.ts
- app/api/reference-architectures/quiz/route.ts
| ) : quizError ? ( | ||
| <div className="flex flex-col items-center gap-3 text-rose-400"> | ||
| <span className="material-symbols-outlined text-[18px]">error</span> | ||
| <span className="text-[10px] font-mono uppercase tracking-widest">{quizError}</span> | ||
| <button | ||
| onClick={() => setShowQuiz(false)} | ||
| className="px-4 py-2 rounded border border-rose-500/30 bg-rose-500/10 hover:bg-rose-500/20 text-[10px] font-mono font-bold uppercase tracking-widest transition-colors" | ||
| > | ||
| Back to Analysis | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the quiz error action recoverable.
In this branch the user is already on the analysis view, so setShowQuiz(false) is a no-op. Wire the button to a retry path or remove it.
Proposed fix
<button
- onClick={() => setShowQuiz(false)}
+ onClick={onRetry}
className="px-4 py-2 rounded border border-rose-500/30 bg-rose-500/10 hover:bg-rose-500/20 text-[10px] font-mono font-bold uppercase tracking-widest transition-colors"
>
- Back to Analysis
+ Retry Analysis
</button>📝 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.
| ) : quizError ? ( | |
| <div className="flex flex-col items-center gap-3 text-rose-400"> | |
| <span className="material-symbols-outlined text-[18px]">error</span> | |
| <span className="text-[10px] font-mono uppercase tracking-widest">{quizError}</span> | |
| <button | |
| onClick={() => setShowQuiz(false)} | |
| className="px-4 py-2 rounded border border-rose-500/30 bg-rose-500/10 hover:bg-rose-500/20 text-[10px] font-mono font-bold uppercase tracking-widest transition-colors" | |
| > | |
| Back to Analysis | |
| </button> | |
| ) : quizError ? ( | |
| <div className="flex flex-col items-center gap-3 text-rose-400"> | |
| <span className="material-symbols-outlined text-[18px]">error</span> | |
| <span className="text-[10px] font-mono uppercase tracking-widest">{quizError}</span> | |
| <button | |
| onClick={onRetry} | |
| className="px-4 py-2 rounded border border-rose-500/30 bg-rose-500/10 hover:bg-rose-500/20 text-[10px] font-mono font-bold uppercase tracking-widest transition-colors" | |
| > | |
| Retry Analysis | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/dashboard/reference-architectures/`[id]/page.tsx around lines 472 - 481,
The quiz error state currently shows a “Back to Analysis” button that calls
setShowQuiz(false), but this branch is already on the analysis view so the
action does nothing. Update the button in the quizError branch to trigger a real
recovery path, such as retrying the quiz fetch/start logic or resetting the quiz
state, using the existing state handlers around quizError and setShowQuiz. If no
retry path exists, remove the button entirely so the UI does not present a dead
action.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes