Fix/upvote persist to database#996
Open
OmanshiRaj wants to merge 4 commits into
Open
Conversation
|
@OmanshiRaj is attempting to deploy a commit to the durdana3105's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThree components are updated independently: ChangesUI and Behavior Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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: 3
🤖 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 `@src/components/PeerCard.tsx`:
- Line 183: The onClick handler in PeerCard navigates to `/profile/${peer.id}`,
but this route does not exist in the router configuration (App.tsx only defines
`/profile`), and the Profile component currently loads the logged-in user from
session rather than from route parameters. To fix this broken contract, either:
(a) add a new route matching `/profile/:id` in App.tsx and update the Profile
component to extract and use the id route parameter for fetching the specific
peer profile, or (b) change the navigation in PeerCard's onClick handler to
navigate to `/profile` instead until peer-profile viewing is supported. Choose
option (a) if peer profile viewing should be implemented, or option (b) if this
feature is not yet supported.
In `@src/pages/AnonymousDoubts.tsx`:
- Around line 96-105: The relative div container in AnonymousDoubts.tsx is
missing the textarea input element that users need to type their doubt text
into. Add a textarea element inside the relative container (before the character
counter span) that binds to the text state variable. The textarea should have
appropriate styling and be positioned so that the character counter span (which
uses absolute positioning with bottom-2 right-2 classes) overlays it properly.
Ensure the textarea is configured to update the text state on input changes and
includes any necessary validation or styling attributes.
- Around line 71-87: In the upvote function, the database update on line 79
references the stale `doubts` closure variable instead of the value at the time
of the upvote request. Capture the current upvote count for the matching doubt
before calling setDoubts on line 75, store it in a local variable, and then use
that captured value plus one in the database update operation instead of relying
on the stale `doubts.find((d) => d.id === id)!.upvotes + 1` expression. This
eliminates the race condition where rapid clicks send outdated values to the
database. Also add a null check to safely handle the case where the doubt might
not exist, preventing the non-null assertion from throwing during the fetch.
🪄 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: 6f853421-7bc7-4727-8f77-4f1b99e476e5
📒 Files selected for processing (3)
src/components/PeerCard.tsxsrc/components/studyroom/LiveCodeRunner.tsxsrc/pages/AnonymousDoubts.tsx
| <Button | ||
| size="sm" | ||
| variant="outline" | ||
| onClick={() => navigate(`/profile/${peer.id}`)} |
Contributor
There was a problem hiding this comment.
View Profile route target does not match current router contract.
At Line 183, navigation goes to /profile/${peer.id}, but the router snippet defines /profile (src/App.tsx:314-320), and Profile currently loads the logged-in user from session rather than route params (src/pages/Profile.tsx:27-76). This is a broken cross-file contract for peer-profile navigation.
Either (a) add a matching route (e.g., /profile/:id) and param-based fetch logic, or (b) navigate to the existing /profile route until peer-profile support exists.
🤖 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/components/PeerCard.tsx` at line 183, The onClick handler in PeerCard
navigates to `/profile/${peer.id}`, but this route does not exist in the router
configuration (App.tsx only defines `/profile`), and the Profile component
currently loads the logged-in user from session rather than from route
parameters. To fix this broken contract, either: (a) add a new route matching
`/profile/:id` in App.tsx and update the Profile component to extract and use
the id route parameter for fetching the specific peer profile, or (b) change the
navigation in PeerCard's onClick handler to navigate to `/profile` instead until
peer-profile viewing is supported. Choose option (a) if peer profile viewing
should be implemented, or option (b) if this feature is not yet supported.
Comment on lines
+71
to
+87
| const upvote = async (id: string) => { | ||
| if (!user) { toast.error("Please log in to upvote."); return; } | ||
|
|
||
| // Optimistic UI update | ||
| setDoubts((prev) => prev.map((d) => (d.id === id ? { ...d, upvotes: d.upvotes + 1 } : d))); | ||
|
|
||
| const { error } = await (supabase as any) | ||
| .from("doubts") | ||
| .update({ upvotes: doubts.find((d) => d.id === id)!.upvotes + 1 }) | ||
| .eq("id", id); | ||
|
|
||
| if (error) { | ||
| // Rollback on failure | ||
| setDoubts((prev) => prev.map((d) => (d.id === id ? { ...d, upvotes: d.upvotes - 1 } : d))); | ||
| toast.error("Failed to register upvote."); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Race condition: stale closure causes DB update to use outdated upvote count.
After setDoubts is called on line 75, doubts on line 79 still holds the value from the last render due to closure semantics. With rapid clicks, the UI correctly increments via the functional updater, but the DB update repeatedly sends the same stale value—causing data drift.
Additionally, the non-null assertion ! on line 79 will throw if the doubt was deleted between renders.
🛠️ Proposed fix: capture value before updates
const upvote = async (id: string) => {
if (!user) { toast.error("Please log in to upvote."); return; }
+ const doubt = doubts.find((d) => d.id === id);
+ if (!doubt) return;
+ const newUpvotes = doubt.upvotes + 1;
+
// Optimistic UI update
- setDoubts((prev) => prev.map((d) => (d.id === id ? { ...d, upvotes: d.upvotes + 1 } : d)));
+ setDoubts((prev) => prev.map((d) => (d.id === id ? { ...d, upvotes: newUpvotes } : d)));
const { error } = await (supabase as any)
.from("doubts")
- .update({ upvotes: doubts.find((d) => d.id === id)!.upvotes + 1 })
+ .update({ upvotes: newUpvotes })
.eq("id", id);
if (error) {
// Rollback on failure
- setDoubts((prev) => prev.map((d) => (d.id === id ? { ...d, upvotes: d.upvotes - 1 } : d)));
+ setDoubts((prev) => prev.map((d) => (d.id === id ? { ...d, upvotes: doubt.upvotes } : d)));
toast.error("Failed to register upvote.");
}
};📝 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.
Suggested change
| const upvote = async (id: string) => { | |
| if (!user) { toast.error("Please log in to upvote."); return; } | |
| // Optimistic UI update | |
| setDoubts((prev) => prev.map((d) => (d.id === id ? { ...d, upvotes: d.upvotes + 1 } : d))); | |
| const { error } = await (supabase as any) | |
| .from("doubts") | |
| .update({ upvotes: doubts.find((d) => d.id === id)!.upvotes + 1 }) | |
| .eq("id", id); | |
| if (error) { | |
| // Rollback on failure | |
| setDoubts((prev) => prev.map((d) => (d.id === id ? { ...d, upvotes: d.upvotes - 1 } : d))); | |
| toast.error("Failed to register upvote."); | |
| } | |
| }; | |
| const upvote = async (id: string) => { | |
| if (!user) { toast.error("Please log in to upvote."); return; } | |
| const doubt = doubts.find((d) => d.id === id); | |
| if (!doubt) return; | |
| const newUpvotes = doubt.upvotes + 1; | |
| // Optimistic UI update | |
| setDoubts((prev) => prev.map((d) => (d.id === id ? { ...d, upvotes: newUpvotes } : d))); | |
| const { error } = await (supabase as any) | |
| .from("doubts") | |
| .update({ upvotes: newUpvotes }) | |
| .eq("id", id); | |
| if (error) { | |
| // Rollback on failure | |
| setDoubts((prev) => prev.map((d) => (d.id === id ? { ...d, upvotes: doubt.upvotes } : d))); | |
| toast.error("Failed to register upvote."); | |
| } | |
| }; |
🤖 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/pages/AnonymousDoubts.tsx` around lines 71 - 87, In the upvote function,
the database update on line 79 references the stale `doubts` closure variable
instead of the value at the time of the upvote request. Capture the current
upvote count for the matching doubt before calling setDoubts on line 75, store
it in a local variable, and then use that captured value plus one in the
database update operation instead of relying on the stale `doubts.find((d) =>
d.id === id)!.upvotes + 1` expression. This eliminates the race condition where
rapid clicks send outdated values to the database. Also add a null check to
safely handle the case where the doubt might not exist, preventing the non-null
assertion from throwing during the fetch.
Comment on lines
+96
to
+105
| <div className="relative"> | ||
|
|
||
| <span className={`absolute bottom-2 right-2 text-xs select-none ${ | ||
| text.length > MAX_CHARS ? "text-red-500 font-semibold" | ||
| : text.length >= MAX_CHARS * 0.9 ? "text-amber-500" | ||
| : "text-slate-400"}`} | ||
| > | ||
| {text.length}/{MAX_CHARS} | ||
| </span> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Critical: Textarea element is missing—users cannot enter doubt text.
The <div className="relative"> wrapper contains only the character counter span. There's no <textarea> for users to input their doubt content. The component has text state and uses it for validation/submission, but provides no input element to populate it.
The textarea should be inside this relative container with the counter overlaid on it.
🐛 Proposed fix: add the textarea element
<div className="relative">
+ <textarea
+ className="border p-2 w-full rounded resize-none pb-6"
+ placeholder="Type your doubt here..."
+ value={text}
+ onChange={(e) => setText(e.target.value)}
+ rows={4}
+ />
<span className={`absolute bottom-2 right-2 text-xs select-none ${
text.length > MAX_CHARS ? "text-red-500 font-semibold"
: text.length >= MAX_CHARS * 0.9 ? "text-amber-500"
: "text-slate-400"}`}
>
{text.length}/{MAX_CHARS}
</span>
</div>📝 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.
Suggested change
| <div className="relative"> | |
| <span className={`absolute bottom-2 right-2 text-xs select-none ${ | |
| text.length > MAX_CHARS ? "text-red-500 font-semibold" | |
| : text.length >= MAX_CHARS * 0.9 ? "text-amber-500" | |
| : "text-slate-400"}`} | |
| > | |
| {text.length}/{MAX_CHARS} | |
| </span> | |
| </div> | |
| <div className="relative"> | |
| <textarea | |
| className="border p-2 w-full rounded resize-none pb-6" | |
| placeholder="Type your doubt here..." | |
| value={text} | |
| onChange={(e) => setText(e.target.value)} | |
| rows={4} | |
| /> | |
| <span className={`absolute bottom-2 right-2 text-xs select-none ${ | |
| text.length > MAX_CHARS ? "text-red-500 font-semibold" | |
| : text.length >= MAX_CHARS * 0.9 ? "text-amber-500" | |
| : "text-slate-400"}`} | |
| > | |
| {text.length}/{MAX_CHARS} | |
| </span> | |
| </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 `@src/pages/AnonymousDoubts.tsx` around lines 96 - 105, The relative div
container in AnonymousDoubts.tsx is missing the textarea input element that
users need to type their doubt text into. Add a textarea element inside the
relative container (before the character counter span) that binds to the text
state variable. The textarea should have appropriate styling and be positioned
so that the character counter span (which uses absolute positioning with
bottom-2 right-2 classes) overlays it properly. Ensure the textarea is
configured to update the text state on input changes and includes any necessary
validation or styling attributes.
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.
Closes #925
The upvote() function was only mutating local React state — upvote counts were lost on every page refresh. Fixed by:
Making upvote async and adding a Supabase .update() call after the optimistic UI update
Rolling back the local state if the DB write fails, with a toast error
Guarding the function so unauthenticated users see a "Please log in to upvote" toast instead of a silent no-op
Summary by CodeRabbit
Release Notes