[eb-v2 review] Generate/SDG: load-failure handling and coordination notes - #1650
[eb-v2 review] Generate/SDG: load-failure handling and coordination notes#1650chiang-daniel wants to merge 1 commit into
Conversation
…ver; fix stale eval_steps comment Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThe generate page now handles task-loading failures, ignores stale requests, renders normalized errors, and prevents retry loops. New tests cover loading, task rendering, failures, and recovery. The guidance comment covers both supported evaluation configuration types. ChangesGenerate task loading
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RouteNavigation
participant handle_routing
participant load_task
participant GeneratePage
RouteNavigation->>handle_routing: provide project and task route
handle_routing->>load_task: load task
load_task-->>handle_routing: task or failure
handle_routing->>GeneratePage: update loading or load_error state
GeneratePage-->>RouteNavigation: render task content or error message
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
| // Show the error instead of spinning forever. The key stays marked as | ||
| // handled so we don't hot-loop retrying a failing load; navigating to a | ||
| // different key (or remounting the route) retries naturally. | ||
| load_error = createKilnError(e) |
There was a problem hiding this comment.
The /generate routing awaited load_task with no catch and no error state, so any task-fetch failure left the page on a permanent silent spinner — a regression against main. This catches the failure into load_error and renders an error branch. See the flag below on the retry behavior.
| loaded_task = await load_task(req_project_id, req_task_id) | ||
| } catch (e) { | ||
| if (req_project_id !== project_id || req_task_id !== task_id) return | ||
| // Show the error instead of spinning forever. The key stays marked as |
There was a problem hiding this comment.
Personal review requested: On a load failure the routing key stays marked as handled, so a failing load isn't retried in place — recovery is by navigating to a different key or remounting the route. I chose this because un-marking the key made the reactive routing hot-loop retry the failing load. Confirm you're happy with navigation-based recovery here, versus an explicit Retry button that clears load_error and re-runs the routing for the same key.
| <div class="w-full min-h-[50vh] flex justify-center items-center"> | ||
| <div class="loading loading-spinner loading-lg"></div> | ||
| </div> | ||
| {:else if load_error} |
There was a problem hiding this comment.
The error branch itself, matching the load-failure states used on the other data pages: a titled message with the underlying error text, shown in place of the spinner once load_error is set. This is what the user now sees instead of an indefinite spinner when the task can't be loaded.
| private judge_eval_steps(): string[] { | ||
| // Only legacy llm_as_judge configs carry eval_steps; the typed properties | ||
| // union doesn't declare the key, so read it untyped. | ||
| // Both legacy config types (llm_as_judge and g_eval) carry eval_steps in |
There was a problem hiding this comment.
The comment here claimed only llm_as_judge configs carry eval_steps, but g_eval configs carry it too — so a reader syncing against main could wrongly assume g_eval judges have no steps to rebuild and drop them. Corrected to name both legacy types; the untyped read is unchanged (the typed union still doesn't declare the key).
|
#1630 overlap — sync map and an offer Samantha — heads up on where #1630 touches the same files as the eval-builder fixes on this integration branch, so the merge is boring when it lands. None of these are asks; just a map. When #1630 flows in:
One concrete offer: we removed an orphaned |
|
Synthetic-user event contract the SDG stream consumes Samantha — for the SDG stream, here's the current shape of the synthetic-user batch events so your consumer stays aligned (
If your stream currently derives turn numbers by counting or omits the discarded-attempt spend, those two are the ones to double-check. |
|
Score-key hazard — shared contract Samantha — a shared gotcha worth stating once, since your starter-code fix and our result-badge fix both live on it: deterministic eval scores are keyed by the eval's spec-name json_keys, not a fixed literal like |
![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)
app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/page.test.ts (1)
178-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a stale-rejection routing test.
Start a pending load for the first route. Navigate to a second route and complete its load. Then reject the first load. Assert that the second route still renders the intro and that no error state appears.
This test protects the stale-request guard in
+page.svelteline 87.🤖 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/web_ui/src/routes/`(app)/generate/[project_id]/[task_id]/page.test.ts around lines 178 - 195, Add a test alongside the existing navigation retry case that keeps the first mockLoadTask request pending, navigates to a second routing key, resolves the second request, then rejects the original request. Assert the second route still renders the data-gen intro stub and does not contain “Error Loading Task,” covering the stale-request guard in the page component.
🤖 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/web_ui/src/routes/`(app)/generate/[project_id]/[task_id]/+page.svelte:
- Around line 221-229: Add role="alert" to the error-state container in the
load_error branch so screen readers announce the task-load failure when it
replaces the loading spinner. Keep the existing error message and layout
unchanged.
---
Nitpick comments:
In `@app/web_ui/src/routes/`(app)/generate/[project_id]/[task_id]/page.test.ts:
- Around line 178-195: Add a test alongside the existing navigation retry case
that keeps the first mockLoadTask request pending, navigates to a second routing
key, resolves the second request, then rejects the original request. Assert the
second route still renders the data-gen intro stub and does not contain “Error
Loading Task,” covering the stale-request guard in the page component.
🪄 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: 65116639-0a48-466e-b983-0b253e5fbe3e
📒 Files selected for processing (4)
app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/+page.svelteapp/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/__tests__/data_gen_intro_stub.svelteapp/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/page.test.tsapp/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/synth_data_guidance_datamodel.ts
| {:else if load_error} | ||
| <div | ||
| class="w-full min-h-[50vh] flex flex-col justify-center items-center gap-2" | ||
| > | ||
| <div class="font-medium">Error Loading Task</div> | ||
| <div class="text-error text-sm"> | ||
| {load_error.getMessage()} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Announce the task-load failure.
load_error replaces the loading spinner asynchronously. A screen reader may not announce the new error state. Add role="alert" to the error container.
Proposed fix
- <div
+ <div
+ role="alert"
class="w-full min-h-[50vh] flex flex-col justify-center items-center gap-2"
>📝 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.
| {:else if load_error} | |
| <div | |
| class="w-full min-h-[50vh] flex flex-col justify-center items-center gap-2" | |
| > | |
| <div class="font-medium">Error Loading Task</div> | |
| <div class="text-error text-sm"> | |
| {load_error.getMessage()} | |
| </div> | |
| </div> | |
| {:else if load_error} | |
| <div | |
| role="alert" | |
| class="w-full min-h-[50vh] flex flex-col justify-center items-center gap-2" | |
| > | |
| <div class="font-medium">Error Loading Task</div> | |
| <div class="text-error text-sm"> | |
| {load_error.getMessage()} | |
| </div> | |
| </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 `@app/web_ui/src/routes/`(app)/generate/[project_id]/[task_id]/+page.svelte
around lines 221 - 229, Add role="alert" to the error-state container in the
load_error branch so screen readers announce the task-load failure when it
replaces the loading spinner. Keep the existing error message and layout
unchanged.
📊 Coverage ReportOverall Coverage: 93% Diff: origin/review/eb-v2/base...HEADNo lines with coverage information in this diff.
|
2 participants
Surface: the /generate landing route and its synth-data guidance model. One commit (landed as d8e558a) — surface a task-load failure on /generate instead of spinning forever, and correct a stale comment about which legacy config types carry eval_steps. About 80% of the diff is tests. One Personal review requested comment flags the failed-key retry behavior. This PR also carries three coordination notes as thread comments: a sync map for the overlap with #1630, the synthetic-user event contract the SDG stream consumes, and a shared note on the score-key hazard.
🤖 Generated with Claude Code
CI note: the "Check API Schema Bindings" failure here is an artifact of this review PR's pinned snapshot — it flagged an annotation file that has since been added on
dchiang/eb-v2-merge(78081dd). No action needed from reviewers.