Vps 61 infinite toast fix - #469
Conversation
…nfinite-toast-fix
…C/VPS into vps-61-infinite-toast-fix
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPlayScenarioPage now refreshes scene and state data after navigation conflicts. It prevents concurrent conflict recovery, updates local data after a successful refresh, and routes to the error page when refresh fails. Scene navigation accepts an optional scene override. ChangesPlay scenario handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PlayScenarioPage
participant NavigationAPI
participant SceneCache
participant ErrorPage
PlayScenarioPage->>NavigationAPI: request current navigation state
NavigationAPI-->>PlayScenarioPage: return active scene and state data
PlayScenarioPage->>SceneCache: cache scenes and update local state
NavigationAPI-->>PlayScenarioPage: return refresh failure
PlayScenarioPage->>ErrorPage: route after refresh failure
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
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 |
![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
🤖 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 `@frontend/src/features/playScenario/PlayScenarioPage.jsx`:
- Around line 136-144: Restore the conflict-recovery path in PlayScenarioPage so
the 409 handler does more than show a toast: in the branch around
handlingConflictRef.current, resynchronize the client by making a separate
non-recursive request with currentScene set to null and no component, then
replace the local scene and state from that authoritative response. Keep
buttonPressed and onSceneChange from leaving the UI on the rejected optimistic
scene, and only use the toast as feedback after the local state has been
corrected.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 473c6b78-e2de-42f9-82ce-a8a0e65fbe38
📒 Files selected for processing (1)
frontend/src/features/playScenario/PlayScenarioPage.jsx
harbassan
left a comment
harbassan
left a comment
There was a problem hiding this comment.
yeah u need to actually make the request to get the new state, which is what it was doing before. The way youve done it it just doesn't make a refresh request, so nothing happens.
Basically: on 409, get the actual current state, then toast after switch.
You have: on 409, toast.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@frontend/src/features/playScenario/PlayScenarioPage.jsx`:
- Around line 171-179: Update the navigation and conflict-recovery flow in
PlayScenarioPage so every request, including refreshFromServer calls, receives a
shared monotonically increasing request ID. Before applying a response via
setSceneId, setStateVariables, or setStateVersion, verify its ID is still
current; also begin conflict recovery only when its assigned ID remains current,
preventing stale recovery responses from overriding newer navigation state.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f274a8f0-7bb6-45b9-bd9e-21c3a07a2eb8
📒 Files selected for processing (1)
frontend/src/features/playScenario/PlayScenarioPage.jsx
| const refreshFromServer = async (user, scenarioId, groupId, isMultiplayer) => { | ||
| const token = await user.getIdToken(); | ||
| const config = isMultiplayer | ||
| ? { | ||
| method: "post", | ||
| url: `/api/navigate/group/${groupId}`, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| data: { | ||
| currentScene: null, | ||
| addFlags: [], | ||
| removeFlags: [], | ||
| componentId: null, | ||
| nextScene: null, | ||
| }, | ||
| } | ||
| : { | ||
| method: "post", | ||
| url: `/api/navigate/user/${scenarioId}`, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| data: { | ||
| currentScene: null, | ||
| addFlags: [], | ||
| removeFlags: [], | ||
| componentId: null, | ||
| nextScene: null, | ||
| startScene: null, | ||
| }, | ||
| }; | ||
| const res = await axios.request(config); | ||
| if (res.data.scenes) { | ||
| res.data.scenes.forEach((scene) => sceneCache.set(scene._id, scene)); | ||
| } | ||
| return { | ||
| newSceneId: res.data.active, | ||
| stateVariables: res.data.stateVariables, | ||
| newStateVersion: res.data.stateVersion, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
i may be missing something, whats the point of this function compared to onSceneChange? Looks like it does the same thing?
There was a problem hiding this comment.
onSceneChange sends active move data like componentId and flags to progress the player forward. refreshFromServer is just for 409 error recovery it sends null for the scene so we can fetch the server's true state without accidentally making a new move or applying state changes. But like onScene change could be refactored to handle both if u want.
There was a problem hiding this comment.
Cause the infinite toast was caused by onSceneChange being called with the same parameters recursively.
2 participants
Issue
The infinite toast bug occurs because of a recursive loop in the 409 error handling
Solution
ref-based guard to prevent recursive 409 error handling
Risk
Idk, should be fine
Checklist
Summary by CodeRabbit