fix: prevent problem refetch on language switch#30
Open
pavsoss wants to merge 4 commits into
Open
Conversation
|
@pavsoss is attempting to deploy a commit to the rishabhjtripathi2903-3434's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughProblemWorkspace separates problem fetching (runs on problemId) from language-specific code loading (runs on problemId + language), removes the font-size slider and fixes editor fontSize to 14, reforms Run/execute handling, and adds JSDoc to frontend content API, backend controllers, and the rate limiter. ChangesWorkspace and documentation updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/sections/ProblemWorkspace.tsx (1)
30-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear stale
problemstate before/after failed fetches.When a fetch fails for a new
problemId,problemretains the previous value (Line 35 only sets on success), so the UI can show the wrong problem after loading ends.Suggested fix
useEffect(() => { const fetchProblem = async () => { try { setLoading(true); + setProblem(null); const data = await getProblemById(problemId); setProblem(data); } catch { + setProblem(null); toast.error('Failed to load problem'); } finally { setLoading(false); } };🤖 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/src/sections/ProblemWorkspace.tsx` around lines 30 - 40, The fetch logic in useEffect's fetchProblem leaves the previous problem in state on failures; update fetchProblem to clear stale state by calling setProblem(null) before starting the fetch (or in the catch block) so the UI doesn't display the old problem for a new problemId, while still using setLoading(true/false) and keeping the toast.error call on failure (refer to useEffect, fetchProblem, setProblem, setLoading, problemId, and toast.error to locate the change).
🤖 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/src/sections/ProblemWorkspace.tsx`:
- Around line 46-61: The effect that loads saved code (useEffect) depends only
on language but reads localStorage key keyed by problemId, so navigating to a
different problem with the same language keeps stale code; update the dependency
array to include problemId as well so the effect re-runs when problemId changes,
ensuring localStorage.getItem(`code_${problemId}_${language}`) and the
subsequent setCode(...) (including the boilerplate fallback) run for the new
problem.
---
Outside diff comments:
In `@app/src/sections/ProblemWorkspace.tsx`:
- Around line 30-40: The fetch logic in useEffect's fetchProblem leaves the
previous problem in state on failures; update fetchProblem to clear stale state
by calling setProblem(null) before starting the fetch (or in the catch block) so
the UI doesn't display the old problem for a new problemId, while still using
setLoading(true/false) and keeping the toast.error call on failure (refer to
useEffect, fetchProblem, setProblem, setLoading, problemId, and toast.error to
locate the change).
🪄 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 Plus
Run ID: aa2167fe-e5ff-4851-ba4f-74fbc3c5df23
📒 Files selected for processing (1)
app/src/sections/ProblemWorkspace.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/sections/ProblemWorkspace.tsx (1)
220-220:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove conflicting text size class.
Line 220 has both
text-xsandtext-xlclasses on the same element. Only one font-size utility should be present. Based on the intent to show a larger "A" label (compared to the smaller one on line 212), keeptext-xland removetext-xs.🎨 Proposed fix
- <span className="text-white/40 text-xs text-xl">A</span> + <span className="text-white/40 text-xl">A</span>🤖 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/src/sections/ProblemWorkspace.tsx` at line 220, In ProblemWorkspace.tsx locate the span rendering the "A" label (the span with classes "text-white/40 text-xs text-xl") and remove the conflicting font-size utility by deleting "text-xs" so only "text-xl" remains; ensure the span retains its color class ("text-white/40") and any other classes untouched.
🤖 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.
Outside diff comments:
In `@app/src/sections/ProblemWorkspace.tsx`:
- Line 220: In ProblemWorkspace.tsx locate the span rendering the "A" label (the
span with classes "text-white/40 text-xs text-xl") and remove the conflicting
font-size utility by deleting "text-xs" so only "text-xl" remains; ensure the
span retains its color class ("text-white/40") and any other classes untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c3f7db8b-6fb3-4b2c-ac02-81b34b3d90da
📒 Files selected for processing (1)
app/src/sections/ProblemWorkspace.tsx
Owner
|
@pavsoss check for coderabbit suggestion |
Contributor
Author
|
@Rishabhworkspace implemented the coderabbit suggestions |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/sections/ProblemWorkspace.tsx (2)
119-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid claiming submission persistence before it succeeds.
Line 120 shows “Submission saved” before
updateProblemStatuscompletes, but failures are swallowed, so users can get a false success message.Suggested fix
if (res?.success && res?.allPassed) { - toast.success('All test cases passed! (Submission saved)'); + toast.success('All test cases passed!'); try { await updateProblemStatus(problemId, 'SOLVED'); + toast.success('Submission saved'); } catch (err) { // silently fail if not logged in or other issues } }🤖 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/src/sections/ProblemWorkspace.tsx` around lines 119 - 125, The success toast currently claims "Submission saved" before updateProblemStatus(problemId, 'SOLVED') completes and any errors are swallowed; change the flow so the toast is shown only after updateProblemStatus resolves successfully (or alternatively show a more conservative message like "All test cases passed!" immediately and only append "Submission saved" after updateProblemStatus succeeds), and surface failures from updateProblemStatus by catching errors and calling toast.error (or reverting UI state) instead of silently ignoring them; update the code around res, toast.success, updateProblemStatus, and problemId accordingly.
37-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset stale
problemstate on fetch failure.When loading a new
problemIdfails, the previous problem can remain visible becauseproblemis never cleared in the failure path.Suggested fix
useEffect(() => { const fetchProblem = async () => { try { setLoading(true); + setProblem(null); const data = await getProblemById(problemId); setProblem(data); } catch { + setProblem(null); toast.error('Failed to load problem'); } finally { setLoading(false); } };🤖 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/src/sections/ProblemWorkspace.tsx` around lines 37 - 47, The fetchProblem useEffect currently leaves the previous problem visible on failure; update fetchProblem (or the surrounding effect) to clear stale state by calling setProblem(null) when starting a new load (before the try) or in the catch block where toast.error is called, ensuring setLoading(false) remains in finally; reference the useEffect/fetchProblem closure and the setProblem setter to implement this change.
🤖 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.
Outside diff comments:
In `@app/src/sections/ProblemWorkspace.tsx`:
- Around line 119-125: The success toast currently claims "Submission saved"
before updateProblemStatus(problemId, 'SOLVED') completes and any errors are
swallowed; change the flow so the toast is shown only after updateProblemStatus
resolves successfully (or alternatively show a more conservative message like
"All test cases passed!" immediately and only append "Submission saved" after
updateProblemStatus succeeds), and surface failures from updateProblemStatus by
catching errors and calling toast.error (or reverting UI state) instead of
silently ignoring them; update the code around res, toast.success,
updateProblemStatus, and problemId accordingly.
- Around line 37-47: The fetchProblem useEffect currently leaves the previous
problem visible on failure; update fetchProblem (or the surrounding effect) to
clear stale state by calling setProblem(null) when starting a new load (before
the try) or in the catch block where toast.error is called, ensuring
setLoading(false) remains in finally; reference the useEffect/fetchProblem
closure and the setProblem setter to implement this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 107db789-62d5-487d-9175-69478f698c93
📒 Files selected for processing (4)
app/src/api/content.tsapp/src/sections/ProblemWorkspace.tsxbackend/src/controllers/contentController.tsbackend/src/middleware/rateLimiter.ts
✅ Files skipped from review due to trivial changes (3)
- backend/src/middleware/rateLimiter.ts
- app/src/api/content.ts
- backend/src/controllers/contentController.ts
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
2 participants
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.
Closes #7
Summary
Fixes an issue where switching programming languages in the Problem Workspace triggered a full problem re-fetch from the backend, causing unnecessary network requests and loading state flashes.
Changes Made
useEffectinProblemWorkspace.tsxinto two separate effects.problemIdchanges.localStorageinitialization logic into the language-change effect.languagefrom the problem-fetch effect dependency array.localStorage.Expected Behavior
Language Switch
/api/content/problems/:id.Problem Change
problemIdchanges.Verification
localStoragefor each language.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation