Phase 2: Fix 7 critical bugs from production audit#220
Phase 2: Fix 7 critical bugs from production audit#220ComBba wants to merge 2 commits intophase-1/auth-cors-ratelimitfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request enhances error handling and session management across the application. Key updates include a retry mechanism for chunk load errors to prevent infinite loops, improved error handling for meeting results, and pre-fetching existing results in brainstorm and meeting views to handle page refreshes gracefully. Additionally, the data merging logic in the zero-prompt hook was updated to use nullish coalescing. Feedback identifies a potential regression in the merging logic that could overwrite existing values with defaults and suggests refactoring duplicated SSE initialization code in the brainstorm view to improve maintainability.
| score: nextPatch.score ?? current.score, | ||
| reason: nextPatch.reason ?? current.reason, | ||
| reason_code: nextPatch.reason_code ?? current.reason_code, | ||
| score_breakdown: nextPatch.score_breakdown ?? current.score_breakdown, | ||
| domain: nextPatch.domain ?? current.domain, | ||
| papers_found: nextPatch.papers_found ?? current.papers_found, | ||
| competitors_found: nextPatch.competitors_found ?? current.competitors_found, | ||
| saturation: nextPatch.saturation ?? current.saturation, | ||
| novelty_boost: nextPatch.novelty_boost ?? current.novelty_boost, | ||
| analysis_step: nextPatch.analysis_step ?? current.analysis_step, | ||
| build_step: nextPatch.build_step ?? current.build_step, | ||
| build_phase: nextPatch.build_phase ?? current.build_phase, | ||
| build_node: nextPatch.build_node ?? current.build_node, | ||
| repo_url: nextPatch.repo_url ?? current.repo_url, | ||
| live_url: nextPatch.live_url ?? current.live_url, | ||
| thread_id: nextPatch.thread_id ?? current.thread_id, |
There was a problem hiding this comment.
While switching from || to ?? correctly allows a score of 0 to be preserved, it introduces a regression where fields missing from the event data will overwrite existing values with defaults (like 0 or ""). This happens because nextPatch is pre-filled with these defaults (e.g., score: Number(data.score || 0)). To avoid resetting existing data when an event only updates a subset of fields, you should only apply the patch for fields that are explicitly present in the data object.
| score: nextPatch.score ?? current.score, | |
| reason: nextPatch.reason ?? current.reason, | |
| reason_code: nextPatch.reason_code ?? current.reason_code, | |
| score_breakdown: nextPatch.score_breakdown ?? current.score_breakdown, | |
| domain: nextPatch.domain ?? current.domain, | |
| papers_found: nextPatch.papers_found ?? current.papers_found, | |
| competitors_found: nextPatch.competitors_found ?? current.competitors_found, | |
| saturation: nextPatch.saturation ?? current.saturation, | |
| novelty_boost: nextPatch.novelty_boost ?? current.novelty_boost, | |
| analysis_step: nextPatch.analysis_step ?? current.analysis_step, | |
| build_step: nextPatch.build_step ?? current.build_step, | |
| build_phase: nextPatch.build_phase ?? current.build_phase, | |
| build_node: nextPatch.build_node ?? current.build_node, | |
| repo_url: nextPatch.repo_url ?? current.repo_url, | |
| live_url: nextPatch.live_url ?? current.live_url, | |
| thread_id: nextPatch.thread_id ?? current.thread_id, | |
| score: data.score !== undefined ? nextPatch.score : current.score, | |
| reason: data.reason !== undefined ? nextPatch.reason : current.reason, | |
| reason_code: data.reason_code !== undefined ? nextPatch.reason_code : current.reason_code, | |
| score_breakdown: data.score_breakdown !== undefined ? nextPatch.score_breakdown : current.score_breakdown, | |
| domain: data.domain !== undefined ? nextPatch.domain : current.domain, | |
| papers_found: data.papers_found !== undefined ? nextPatch.papers_found : current.papers_found, | |
| competitors_found: data.competitors_found !== undefined ? nextPatch.competitors_found : current.competitors_found, | |
| saturation: data.saturation !== undefined ? nextPatch.saturation : current.saturation, | |
| novelty_boost: data.novelty_boost !== undefined ? nextPatch.novelty_boost : current.novelty_boost, | |
| analysis_step: data.analysis_step !== undefined ? nextPatch.analysis_step : current.analysis_step, | |
| build_step: data.build_step !== undefined ? nextPatch.build_step : current.build_step, | |
| build_phase: data.build_phase !== undefined ? nextPatch.build_phase : current.build_phase, | |
| build_node: data.build_node !== undefined ? nextPatch.build_node : current.build_node, | |
| repo_url: data.repo_url !== undefined ? nextPatch.repo_url : current.repo_url, | |
| live_url: data.live_url !== undefined ? nextPatch.live_url : current.live_url, | |
| thread_id: data.thread_id !== undefined ? nextPatch.thread_id : current.thread_id, |
| let stopFn: (() => void) | undefined; | ||
| let cancelled = false; | ||
|
|
||
| // Check if result already exists (page refresh case) | ||
| getBrainstormResult(sessionId).then((existing) => { | ||
| if (cancelled) return; | ||
| if (existing) { | ||
| setResult(existing); | ||
| setStreamCompleted(true); | ||
| getBrainstormResult(sessionId).then((res) => { | ||
| if (res) setResult(res); | ||
| }); | ||
| }, | ||
| onError: (err) => setError(err.message), | ||
| return; | ||
| } | ||
|
|
||
| stopFn = createSSEClient({ | ||
| url: `${DASHBOARD_API_URL}/brainstorm`, | ||
| body: { | ||
| prompt: "Run brainstorm and stream all events.", | ||
| config: { configurable: { thread_id: sessionId } }, | ||
| }, | ||
| onEvent: handleEvent, | ||
| onComplete: () => { | ||
| setStreamCompleted(true); | ||
| getBrainstormResult(sessionId).then((res) => { | ||
| if (res) setResult(res); | ||
| }); | ||
| }, | ||
| onError: (err) => setError(err.message), | ||
| }); | ||
| }).catch(() => { | ||
| if (cancelled) return; | ||
| stopFn = createSSEClient({ | ||
| url: `${DASHBOARD_API_URL}/brainstorm`, | ||
| body: { | ||
| prompt: "Run brainstorm and stream all events.", | ||
| config: { configurable: { thread_id: sessionId } }, | ||
| }, | ||
| onEvent: handleEvent, | ||
| onComplete: () => { | ||
| setStreamCompleted(true); | ||
| getBrainstormResult(sessionId).then((res) => { | ||
| if (res) setResult(res); | ||
| }); | ||
| }, | ||
| onError: (err) => setError(err.message), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
1. use-zero-prompt.ts: replace || with ?? (nullish coalescing) for all card field merges — score=0 was treated as falsy, preserving stale data 2. result/[id]/page.tsx: add .catch() to getMeetingResult — API failure caused permanent loading skeleton 3. error.tsx: limit ChunkLoadError auto-reload to 2 attempts via sessionStorage counter — prevents infinite reload loop 4. meeting-view.tsx: check for existing result before starting SSE stream — page refresh no longer re-triggers entire council meeting 5. brainstorm-view.tsx: same refresh guard — check existing result first 6. zero-prompt-workspace.tsx: destructure and display error state from useZeroPrompt hook — errors were silently swallowed 7. decision-gate.tsx: wire onRevise prop in meeting-view — "Revise Idea" button now navigates back to home Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… safety Review fixes for Phase 2 PR #220: - pipeline_runtime.py: fix `if not score` → `if score is None` to preserve legitimate score=0 from council evaluation - use-zero-prompt.ts:209: fix remaining `data.score || 0` → `?? 0` - error.tsx: wrap sessionStorage access in try/catch for Safari private browsing mode compatibility Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
b14cbca to
7d40167
Compare
Summary
||→??for 16 card field merges (score=0 was falsy).catch()to prevent permanent loading skeletononReviseprop → navigates to homeChanges
Test plan
??🤖 Generated with Claude Code