fix: update CP immediately after predicting#4504
fix: update CP immediately after predicting#4504SylvainChevalier wants to merge 2 commits intomainfrom
Conversation
After submitting a prediction, fetch fresh post data (including updated CP) from the API, matching the reaffirm flow pattern. Previously, the predict flow relied solely on revalidatePath which didn't reliably update the client-side CP display. Closes #4437 Co-authored-by: Sylvain <SylvainChevalier@users.noreply.github.com>
SylvainChevalier
temporarily deployed
to
testing_env
GitHub Actions
Inactive
SylvainChevalier
temporarily deployed
to
testing_env
GitHub Actions
Inactive
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 59 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughForecastMaker component now maintains a local Changes
Sequence DiagramsequenceDiagram
participant User
participant ForecastMaker
participant ClientPostsApi
participant ChildComponents
User->>ForecastMaker: Click Predict
ForecastMaker->>ForecastMaker: handlePredictionSubmit triggered
ForecastMaker->>ClientPostsApi: getPost(currentPost.id)
activate ClientPostsApi
ClientPostsApi-->>ForecastMaker: Fresh post data
deactivate ClientPostsApi
ForecastMaker->>ForecastMaker: Update currentPost state
ForecastMaker->>ForecastMaker: Call original onPredictionSubmit
ForecastMaker->>ChildComponents: Pass updated post & handlePredictionSubmit
ChildComponents-->>User: Render with updated CP
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
SylvainChevalier
temporarily deployed
to
Preview
GitHub Actions
Inactive
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
|
Seems I can't test this because the preview env doesn't recompute CP at all? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
front_end/src/components/forecast_maker/index.tsx (1)
35-37: Add minimal error telemetry for CP refresh failures.Line 35-37 silently swallows refresh errors, which makes production debugging hard when CP does not update as expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/components/forecast_maker/index.tsx` around lines 35 - 37, The empty catch in the forecast_maker component swallows CP refresh errors; change the anonymous catch to capture the error (catch (err)) and emit minimal telemetry and/or log the error so failures are visible in production (e.g., call your telemetry helper like sendTelemetry or trackError with an event name such as "CPRefreshFailed" and include err.message/stack), then keep the existing comment/behavior if desired; update the catch in front_end/src/components/forecast_maker/index.tsx accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@front_end/src/components/forecast_maker/index.tsx`:
- Around line 31-35: The async fetch in handlePredictionSubmit uses
currentPost.id then unconditionally calls setCurrentPost with the response,
which lets a stale response overwrite newer state; fix by capturing the
request's intended id (e.g., const requestedId = currentPost.id) before awaiting
ClientPostsApi.getPost(requestedId) and, after the await, verify the component
still intends to update that same id (compare requestedId to the latest
currentPost.id or an up-to-date ref) before calling setCurrentPost;
alternatively cancel/ignore responses with an AbortController or mounted flag so
setCurrentPost only runs for the latest in-flight request.
---
Nitpick comments:
In `@front_end/src/components/forecast_maker/index.tsx`:
- Around line 35-37: The empty catch in the forecast_maker component swallows CP
refresh errors; change the anonymous catch to capture the error (catch (err))
and emit minimal telemetry and/or log the error so failures are visible in
production (e.g., call your telemetry helper like sendTelemetry or trackError
with an event name such as "CPRefreshFailed" and include err.message/stack),
then keep the existing comment/behavior if desired; update the catch in
front_end/src/components/forecast_maker/index.tsx accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f489c6e1-eb20-45be-88a3-6528357d669b
📒 Files selected for processing (1)
front_end/src/components/forecast_maker/index.tsx
| const { | ||
| group_of_questions: groupOfQuestions, | ||
| conditional, | ||
| question, | ||
| } = currentPost; |
There was a problem hiding this comment.
Fresh post only threads into ForecastMaker subtree. Sibling DetailedQuestionCard / DetailedGroupCard in forecaster_question_view still render stale CP from parent postData until revalidatePath lands. If #4437 is about the chart, this won't fix it – lift the refetch to the parent page.
| try { | ||
| const freshPost = await ClientPostsApi.getPost(currentPost.id); | ||
| setCurrentPost(freshPost); |
There was a problem hiding this comment.
Double-fetch in prediction flow: prediction_flow_post.tsx:62 already refetches the post in its own onPredictionSubmit. This wrapper now fires a second GET /posts/:id right before it.
| useEffect(() => { | ||
| setCurrentPost(post); | ||
| }, [post]); |
There was a problem hiding this comment.
Race: local setCurrentPost(fresh) can be overwritten moments later when the parent re-renders with a staler post prop from revalidatePath, via the useEffect sync. Causes a flash back to stale CP.
|
@claude please make requested changes. |
|
Claude finished @SylvainChevalier's task in 7m 49s —— View job Changes pushed ✓Addressed all three review points from @ncarazon and the stale-response concern from @coderabbitai by lifting the post refetch out of Why this is the right shape
Concerns addressed
Files changed
Tasks
Notes
Commit: |
Move the post-prediction refetch out of ForecastMaker and into the parent ForecasterQuestionView so that siblings (DetailedQuestionCard / DetailedGroupCard) also render the fresh CP immediately, not just the forecast maker subtree. Also avoids double-fetching in the prediction flow, where prediction_flow_post already refetches on submit, and protects against stale in-flight responses via a request-id ref. Addresses review feedback from @ncarazon and @coderabbitai on #4504. Co-authored-by: Sylvain <SylvainChevalier@users.noreply.github.com>
2 participants
Closes #4437
Summary
Test plan
Generated with Claude Code
Summary by CodeRabbit