Merged
Conversation
feat(practice): implement template-based practice system with simulat…
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a Practice feature: template types and curated templates, two API routes to list and fetch templates, practice directory and per-template practice pages with localStorage persistence and autosave, DesignCanvas prop additions and simulation callback wiring, sidebar/hero navigation updates, and a simulation change causing Cache nodes to forward only a CACHE_MISS_RATIO portion of outflow. Changes
Sequence DiagramsequenceDiagram
participant User
participant Dir as Practice Directory
participant Page as Practice Page
participant API as API Server
participant Templates as Template Store
participant Canvas as DesignCanvas
participant Engine as Simulation Engine
participant Storage as localStorage
User->>Dir: Visit /practice
Dir->>API: GET /api/templates
API->>Templates: read curatedTemplates
Templates-->>API: return list
API-->>Dir: templates JSON
User->>Page: Open /practice/{id}
Page->>API: GET /api/templates/{id}
API->>Templates: find template by id
Templates-->>API: template JSON
API-->>Page: return template
Page->>Storage: load practice_progress_{id} & practice_solved
Storage-->>Page: saved state
Page->>Canvas: init(template, saved state)
User->>Canvas: edit design / run simulation
Canvas->>Engine: runSimulation(targetRps)
Engine-->>Canvas: metrics
Canvas->>Page: onSimulationChange(metrics)
Canvas->>Storage: autosave periodic
Storage-->>Canvas: ack
User->>Page: Submit solution
Page->>Engine: evaluate bottleneck status
alt solved
Page->>Storage: mark solved, save canvas
Page-->>User: success
else not solved
Page-->>User: failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. |
fix: Lint and build issues
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/lib/simulation/engine.ts (1)
114-116: Consider extracting cache miss ratio into a named constant.The
0.1is behavior-critical now; moving it to a constant makes tuning safer and clearer.♻️ Optional refactor
+const CACHE_MISS_RATIO = 0.1; ... - const effectiveOut = node.type === 'Cache' ? outFlow * 0.1 : outFlow; + const effectiveOut = node.type === 'Cache' ? outFlow * CACHE_MISS_RATIO : outFlow;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/simulation/engine.ts` around lines 114 - 116, The magic number 0.1 used when computing effectiveOut for cache nodes should be extracted into a named constant for clarity and easier tuning: introduce a well-named constant (e.g., CACHE_MISS_RATIO) near the top of the module or function, replace the literal in the effectiveOut calculation (the line computing effectiveOut for node.type === 'Cache') with that constant, and keep the remaining logic that computes flowPerEdge = effectiveOut / outgoingEdges.length unchanged; update any relevant comments to reference CACHE_MISS_RATIO.app/practice/[id]/page.tsx (3)
121-144: Consider defensive check for missingbottleneckNodeId.If a template lacks a valid
bottleneckNodeIdor it doesn't exist innodeMetrics,bottleneckNodeStatuswill beundefined, causing the validation logic to fall through to the error case. This may be the intended behavior, but adding an explicit guard would make the intent clearer.💡 Suggested improvement
const handleSubmit = () => { if (!template || !simulationState) { setFeedback({ type: 'error', text: 'Run the simulation first before submitting your fix.' }); return; } + if (!template.bottleneckNodeId) { + setFeedback({ type: 'error', text: 'Template configuration error: missing bottleneck node.' }); + return; + } + const bottleneckNodeStatus = simulationState.nodeMetrics[template.bottleneckNodeId]?.status;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/practice/`[id]/page.tsx around lines 121 - 144, In handleSubmit, add an explicit guard for a missing or invalid bottleneck node before using simulationState.nodeMetrics: check that template.bottleneckNodeId is present and that simulationState.nodeMetrics[template.bottleneckNodeId] exists; if either is missing, call setFeedback with a clear error (e.g., "No bottleneck node defined or metrics unavailable for the target node.") and return early. This uses the same symbols (handleSubmit, template.bottleneckNodeId, simulationState.nodeMetrics, setFeedback) and keeps the existing success/error branches unchanged.
11-41: Consider extracting shared localStorage helpers into a common module.The
isSolvedfunction and thepractice_solvedparsing logic are duplicated inapp/practice/page.tsx(lines 36-41). Centralizing these helpers would ensure consistent serialization/deserialization and simplify maintenance.💡 Suggested approach
Create a shared module like
src/lib/practice/storage.ts:// src/lib/practice/storage.ts export function getSolvedIds(): string[] { try { return JSON.parse(localStorage.getItem('practice_solved') || '[]') as string[]; } catch { return []; } } export function isSolved(templateId: string): boolean { return getSolvedIds().includes(templateId); } export function markSolved(templateId: string): void { try { const solved = getSolvedIds(); if (!solved.includes(templateId)) { solved.push(templateId); localStorage.setItem('practice_solved', JSON.stringify(solved)); } } catch { /* silently fail */ } } // ... other helpers🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/practice/`[id]/page.tsx around lines 11 - 41, Extract the duplicated localStorage logic into a shared module (e.g., src/lib/practice/storage) and replace the inline helpers in this file with imports; specifically move parsing/serialization for the 'practice_solved' key and functions isSolved(templateId), markSolved(templateId) (and optionally getSavedProgress/saveProgress) into that module so both app/practice/[id]/page.tsx and app/practice/page.tsx call the same implementations, update imports to use the new module, and ensure error handling behavior (silent fallbacks) is preserved.
65-89: Add AbortController to prevent state updates on unmounted component.If the component unmounts before the fetch completes (e.g., user navigates away quickly), the state setters will still fire, which can cause React warnings and unexpected behavior.
♻️ Proposed fix
useEffect(() => { + const controller = new AbortController(); async function loadTemplate() { try { - const res = await fetch(`/api/templates/${id}`); + const res = await fetch(`/api/templates/${id}`, { signal: controller.signal }); if (!res.ok) throw new Error('Template not found'); const data = await res.json(); setTemplate(data.template); // Load saved progress from localStorage const saved = getSavedProgress(id); if (saved) { setUserNodes(saved.nodes); setUserConnections(saved.connections); } // Check if already solved setSolved(isSolved(id)); } catch (err) { + if (err instanceof Error && err.name === 'AbortError') return; setError(err instanceof Error ? err.message : 'Failed to load'); } finally { setLoading(false); } } if (id) loadTemplate(); + return () => controller.abort(); }, [id]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/practice/`[id]/page.tsx around lines 65 - 89, The effect's async loadTemplate may update state after unmount; use an AbortController inside useEffect: create controller, pass controller.signal to fetch(`/api/templates/${id}`), and in the cleanup call controller.abort(); inside loadTemplate avoid calling setTemplate, setUserNodes, setUserConnections, setSolved, setError, or setLoading when controller.signal.aborted (or ignore AbortError by checking err.name === 'AbortError' before setError). Update references in this block (loadTemplate, getSavedProgress, isSolved) to respect the abort signal and prevent state updates after unmount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/practice/page.tsx`:
- Around line 22-34: In useEffect's async load function, validate the fetch
response and add an AbortController: call fetch('/api/templates', { signal:
controller.signal }), then check if (!res.ok) throw new Error(`Failed to load
templates: ${res.status} ${res.statusText}`) before parsing JSON and calling
setTemplates; in the cleanup return handler call controller.abort() to cancel
in-flight requests and in the catch block ignore abort errors or log others, and
ensure setLoading(false) still runs in finally.
In `@components/canvas/DesignCanvas.tsx`:
- Around line 148-149: The component DesignCanvas initializes targetRps with
useState(initialTargetRps) but never updates it when the prop initialTargetRps
changes (so useSimulationEngine runs with stale values); add a useEffect in
DesignCanvas that watches initialTargetRps and calls
setTargetRps(initialTargetRps) (guarding for undefined/null if required) so the
local state stays synchronized with prop changes from async template loads.
In `@components/dashboard/Sidebar.tsx`:
- Line 24: The current exact-match line using href and pathname fails to mark
dynamic practice routes active; update the condition so that for the '/practice'
nav item you treat any path that starts with '/practice/' as active. Concretely,
keep the early-return for dashboard and interview using strict equality but
change the practice branch to: if (href === '/practice') return pathname ===
'/practice' || pathname.startsWith('/practice/'); (referencing href and pathname
in Sidebar.tsx).
---
Nitpick comments:
In `@app/practice/`[id]/page.tsx:
- Around line 121-144: In handleSubmit, add an explicit guard for a missing or
invalid bottleneck node before using simulationState.nodeMetrics: check that
template.bottleneckNodeId is present and that
simulationState.nodeMetrics[template.bottleneckNodeId] exists; if either is
missing, call setFeedback with a clear error (e.g., "No bottleneck node defined
or metrics unavailable for the target node.") and return early. This uses the
same symbols (handleSubmit, template.bottleneckNodeId,
simulationState.nodeMetrics, setFeedback) and keeps the existing success/error
branches unchanged.
- Around line 11-41: Extract the duplicated localStorage logic into a shared
module (e.g., src/lib/practice/storage) and replace the inline helpers in this
file with imports; specifically move parsing/serialization for the
'practice_solved' key and functions isSolved(templateId), markSolved(templateId)
(and optionally getSavedProgress/saveProgress) into that module so both
app/practice/[id]/page.tsx and app/practice/page.tsx call the same
implementations, update imports to use the new module, and ensure error handling
behavior (silent fallbacks) is preserved.
- Around line 65-89: The effect's async loadTemplate may update state after
unmount; use an AbortController inside useEffect: create controller, pass
controller.signal to fetch(`/api/templates/${id}`), and in the cleanup call
controller.abort(); inside loadTemplate avoid calling setTemplate, setUserNodes,
setUserConnections, setSolved, setError, or setLoading when
controller.signal.aborted (or ignore AbortError by checking err.name ===
'AbortError' before setError). Update references in this block (loadTemplate,
getSavedProgress, isSolved) to respect the abort signal and prevent state
updates after unmount.
In `@src/lib/simulation/engine.ts`:
- Around line 114-116: The magic number 0.1 used when computing effectiveOut for
cache nodes should be extracted into a named constant for clarity and easier
tuning: introduce a well-named constant (e.g., CACHE_MISS_RATIO) near the top of
the module or function, replace the literal in the effectiveOut calculation (the
line computing effectiveOut for node.type === 'Cache') with that constant, and
keep the remaining logic that computes flowPerEdge = effectiveOut /
outgoingEdges.length unchanged; update any relevant comments to reference
CACHE_MISS_RATIO.
🪄 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: 170ad7fd-458c-49c6-a2d6-7899d37d7653
📒 Files selected for processing (10)
app/api/templates/[id]/route.tsapp/api/templates/route.tsapp/practice/[id]/page.tsxapp/practice/page.tsxcomponents/canvas/DesignCanvas.tsxcomponents/dashboard/Hero.tsxcomponents/dashboard/Sidebar.tsxsrc/lib/simulation/engine.tssrc/lib/templates/curated.tssrc/lib/templates/types.ts
fix: Lint and build issues
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/practice/`[id]/page.tsx:
- Around line 136-144: The reset currently leaves userNodes/userConnections null
so DesignCanvas may not remount; add a remount token state (e.g., canvasVersion
via useState) and increment it inside handleReset along with setUserNodes(null),
setUserConnections(null), setFeedback(null), setSolved(false), clearProgress(id)
and unmarkSolved(id) so DesignCanvas receives a new key (use canvasVersion in
its key) and truly reinitializes whether coming from first attempt or solution
mode; update the component that renders DesignCanvas to include this
canvasVersion in the key so the canvas fully remounts on reset.
- Around line 119-129: The current success branch only checks for
simulationState.globalStatus !== 'critical' and can mark solved even when the
engine reports 'degraded'; change the condition that triggers
setSolved/markSolved/saveProgress to require simulationState.globalStatus ===
'normal' (and keep the existing 'critical' handling), i.e., within the if
handling bottleneckNodeStatus === 'normal' || 'warning' only call
setSolved(id)/markSolved(id)/saveProgress(...) when simulationState.globalStatus
is exactly 'normal' (otherwise set appropriate feedback and do not persist).
In `@src/lib/practice/storage.ts`:
- Around line 9-13: The parsed localStorage payloads need runtime shape
validation before returning: in getSavedProgress (and the similar helper around
lines 26-29) parse the JSON, then check that the result is a non-null object
with Array.isArray(result.nodes) and Array.isArray(result.connections) (and for
the solved-ids helper ensure the parsed value is an array of strings/numbers),
otherwise treat it as invalid and return null (for getSavedProgress) or an empty
array (for solved ids). Use progressKey to retrieve the raw string, validate
types after JSON.parse, and only return the value when shape checks pass.
🪄 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: 62f31d4a-2b7d-4f74-911f-35a8ea270cf3
📒 Files selected for processing (6)
app/practice/[id]/page.tsxapp/practice/page.tsxcomponents/canvas/DesignCanvas.tsxcomponents/dashboard/Sidebar.tsxsrc/lib/practice/storage.tssrc/lib/simulation/engine.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- components/canvas/DesignCanvas.tsx
fix: Lint and build issues
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/practice/[id]/page.tsx (1)
139-148:⚠️ Potential issue | 🟡 MinorReset does not exit solution view or clear simulation state.
If the user clicks Reset while viewing the solution,
showSolutionremainstrueandsimulationStateretains stale data. This could cause confusing UX where reset leaves them in solution view, and stale metrics could affect the next submission attempt.🔧 Proposed fix to fully reset UI state
const handleReset = () => { if (!template) return; setUserNodes(null); setUserConnections(null); setFeedback(null); setSolved(false); + setShowSolution(false); + setSimulationState(null); setCanvasVersion(v => v + 1); clearProgress(id); unmarkSolved(id); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/practice/`[id]/page.tsx around lines 139 - 148, handleReset currently doesn't exit the solution view or clear the running simulation state; update the handleReset function (the one that calls setUserNodes, setUserConnections, setFeedback, setSolved, setCanvasVersion, clearProgress, unmarkSolved) to also call setShowSolution(false) to leave solution view and reset simulationState by calling setSimulationState(null) or its initial value (and any related simulation metrics state if present) so no stale simulation data remains after reset.
🧹 Nitpick comments (1)
app/practice/[id]/page.tsx (1)
181-181: Back navigation uses non-focusable element.The back arrow (
←) is a<span>withonClick, which is not keyboard-accessible. Users navigating with keyboard cannot focus or activate it.♿ Proposed fix for accessibility
- <span className="text-slate-400 cursor-pointer hover:text-white transition" onClick={() => router.push('/practice')}>←</span> + <button + type="button" + className="text-slate-400 cursor-pointer hover:text-white transition bg-transparent border-none p-0" + onClick={() => router.push('/practice')} + aria-label="Back to practice list" + > + ← + </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/practice/`[id]/page.tsx at line 181, The back arrow is a non-focusable <span> with an onClick handler; replace it with a semantic, focusable element (preferably a <button>) or at minimum add keyboard activation (tabIndex="0" and onKeyDown handling) and ARIA attributes so keyboard users can focus and activate it. Update the element used in the component rendering the back arrow (the span with className "text-slate-400 cursor-pointer hover:text-white transition" and onClick={() => router.push('/practice')}) to a <button> (or add role="button", tabIndex, and handle Enter/Space in onKeyDown), ensure it retains the same styling, and include an accessible name (aria-label="Back to practice") for screen readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/practice/`[id]/page.tsx:
- Around line 160-166: The current render block in page.tsx returns "Error:
{error}" which can produce "Error: null" when template is missing but error is
null; update the error message logic in the conditional that checks (error ||
!template) so it uses a safe fallback string (e.g., use error ?? 'Template not
found' or a ternary that shows a specific message when !template) instead of
directly interpolating the nullable error variable; modify the JSX in the error
return (the <p> element) to display the chosen fallback message and include
context like "Error loading template: ..." using the template and error
identifiers to locate the code.
---
Duplicate comments:
In `@app/practice/`[id]/page.tsx:
- Around line 139-148: handleReset currently doesn't exit the solution view or
clear the running simulation state; update the handleReset function (the one
that calls setUserNodes, setUserConnections, setFeedback, setSolved,
setCanvasVersion, clearProgress, unmarkSolved) to also call
setShowSolution(false) to leave solution view and reset simulationState by
calling setSimulationState(null) or its initial value (and any related
simulation metrics state if present) so no stale simulation data remains after
reset.
---
Nitpick comments:
In `@app/practice/`[id]/page.tsx:
- Line 181: The back arrow is a non-focusable <span> with an onClick handler;
replace it with a semantic, focusable element (preferably a <button>) or at
minimum add keyboard activation (tabIndex="0" and onKeyDown handling) and ARIA
attributes so keyboard users can focus and activate it. Update the element used
in the component rendering the back arrow (the span with className
"text-slate-400 cursor-pointer hover:text-white transition" and onClick={() =>
router.push('/practice')}) to a <button> (or add role="button", tabIndex, and
handle Enter/Space in onKeyDown), ensure it retains the same styling, and
include an accessible name (aria-label="Back to practice") for screen readers.
🪄 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: 50fc582e-dbf9-41e8-85d2-b3b88594d8db
📒 Files selected for processing (2)
app/practice/[id]/page.tsxsrc/lib/practice/storage.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/practice/storage.ts
Comment on lines
+160
to
+166
| if (error || !template) { | ||
| return ( | ||
| <div className="flex h-screen items-center justify-center bg-background-dark"> | ||
| <p className="text-red-500">Error: {error}</p> | ||
| </div> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Error message could show "Error: null" in edge case.
If the template fails to load but error state is null (e.g., API returns success with missing template), this renders "Error: null".
🔧 Proposed fix for fallback error message
if (error || !template) {
return (
<div className="flex h-screen items-center justify-center bg-background-dark">
- <p className="text-red-500">Error: {error}</p>
+ <p className="text-red-500">Error: {error ?? 'Failed to load template'}</p>
</div>
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/practice/`[id]/page.tsx around lines 160 - 166, The current render block
in page.tsx returns "Error: {error}" which can produce "Error: null" when
template is missing but error is null; update the error message logic in the
conditional that checks (error || !template) so it uses a safe fallback string
(e.g., use error ?? 'Template not found' or a ternary that shows a specific
message when !template) instead of directly interpolating the nullable error
variable; modify the JSX in the error return (the <p> element) to display the
chosen fallback message and include context like "Error loading template: ..."
using the template and error identifiers to locate the code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Enhancements