Fix deeplink for chat-created prompts and run configs#1314
Fix deeplink for chat-created prompts and run configs#1314chiang-daniel wants to merge 3 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThe PR updates task prompt and run config loading across multiple routes by introducing SvelteKit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces force-refreshing for task prompts and run configurations across several pages to ensure deep-linked resources are correctly loaded. It also adds explicit loading and error handling to the saved prompt detail view. Review feedback identifies a race condition between the local loading state and the global task store, and notes that moving logic into onMount has broken reactivity for prompt selection and form initialization when URL parameters change.
📊 Coverage ReportOverall Coverage: 92% Diff: origin/main...HEADNo lines with coverage information in this diff.
|
![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)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte (1)
100-113: Minor: task-mismatch message only shows after a successful load.With the new flow, when
$current_task?.id != task_idwe still perform the forced network load before showing the "link is to another task's prompt" warning. Not a correctness bug (load succeeds and data is cached), just a small wasted fetch. Consider short-circuiting theonMountload when the task id doesn't match$current_task.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte around lines 100 - 113, The component performs the forced network load even when $current_task?.id !== task_id, causing an unnecessary fetch; modify the onMount load handler in +page.svelte to short-circuit early by checking $current_task?.id !== task_id before starting the fetch (use the same reactive/current_task value used in the template), set loading to false (and optionally set a specific loading_error or state) and return so the network call is skipped; ensure you reference the existing loading, loading_error, $current_task and task_id variables so the UI still shows the task-mismatch message without performing the fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte:
- Around line 25-45: The bug is that prompt_model is set only in onMount so it
doesn't update when prompt_id changes; keep the force-refresh in onMount
(load_task_prompts) but move the prompt lookup into a reactive block that
re-runs when prompt_id or the store changes. Specifically, after the onMount
block add a reactive statement that assigns prompt_model =
$prompts_by_task_composite_id[get_task_composite_id(project_id,
task_id)]?.prompts.find(p => p.id === prompt_id) ?? null so prompt_model (and
the Clone href) updates whenever prompt_id or the prompts store updates; leave
loading/loading_error logic in onMount.
---
Nitpick comments:
In
`@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte:
- Around line 100-113: The component performs the forced network load even when
$current_task?.id !== task_id, causing an unnecessary fetch; modify the onMount
load handler in +page.svelte to short-circuit early by checking
$current_task?.id !== task_id before starting the fetch (use the same
reactive/current_task value used in the template), set loading to false (and
optionally set a specific loading_error or state) and return so the network call
is skipped; ensure you reference the existing loading, loading_error,
$current_task and task_id variables so the UI still shows the task-mismatch
message without performing 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91df9325-1e87-4de1-84e9-6886300a0ea1
📒 Files selected for processing (3)
app/web_ui/src/routes/(app)/optimize/[project_id]/[task_id]/run_config/[run_config_id]/+page.svelteapp/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/clone/[prompt_id]/+page.svelteapp/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte (2)
40-43: Redundant type cast onprompt_model.
$prompts_by_task_composite_id[...]?.promptsis already typed viaPromptResponse, so.find((p) => p.id === prompt_id)should already returnApiPrompt | undefinedwithout theas ApiPrompt | undefinedcast. If the cast is needed due to a type mismatch between the store's prompt type andApiPrompt, that's worth a brief comment; otherwise consider dropping the cast (and theApiPromptimport) for cleanliness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte around lines 40 - 43, Remove the unnecessary type assertion on prompt_model: the expression $prompts_by_task_composite_id[get_task_composite_id(project_id, task_id)]?.prompts.find((p) => p.id === prompt_id) is already typed as ApiPrompt | undefined via PromptResponse, so delete the "as ApiPrompt | undefined" cast and, if no other uses remain, remove the ApiPrompt import; if the store's prompt type truly differs, add a short comment next to the cast explaining why the conversion is required.
28-38: Minor: consider extracting the duplicated load pattern.The
onMount+loading/loading_error+ force-refresh block here is essentially identical to the one inclone/[prompt_id]/+page.svelte. Not blocking, but if a third call site appears, extracting a small helper (or co-locating the force-refresh inside a shared loader) would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte around lines 28 - 38, Extract the duplicated onMount/loading pattern into a small shared helper (e.g., loadPromptsWithRefresh) that accepts project_id and task_id and encapsulates the try { await load_task_prompts(project_id, task_id, true) } catch (e) { loading_error = createKilnError(e) } finally { loading = false } logic; then replace the inline onMount blocks in this file and clone/[prompt_id]/+page.svelte to call that helper (or expose a shared loader that performs the force-refresh) so state handling (loading, loading_error) and the force-refresh behavior are maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte:
- Around line 40-43: Remove the unnecessary type assertion on prompt_model: the
expression $prompts_by_task_composite_id[get_task_composite_id(project_id,
task_id)]?.prompts.find((p) => p.id === prompt_id) is already typed as ApiPrompt
| undefined via PromptResponse, so delete the "as ApiPrompt | undefined" cast
and, if no other uses remain, remove the ApiPrompt import; if the store's prompt
type truly differs, add a short comment next to the cast explaining why the
conversion is required.
- Around line 28-38: Extract the duplicated onMount/loading pattern into a small
shared helper (e.g., loadPromptsWithRefresh) that accepts project_id and task_id
and encapsulates the try { await load_task_prompts(project_id, task_id, true) }
catch (e) { loading_error = createKilnError(e) } finally { loading = false }
logic; then replace the inline onMount blocks in this file and
clone/[prompt_id]/+page.svelte to call that helper (or expose a shared loader
that performs the force-refresh) so state handling (loading, loading_error) and
the force-refresh behavior are maintained in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6df4f731-ba7f-4cc5-be8b-c9e7f94fc8b2
📒 Files selected for processing (2)
app/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/clone/[prompt_id]/+page.svelteapp/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/web_ui/src/routes/(app)/optimize/[project_id]/[task_id]/run_config/[run_config_id]/+page.ts (1)
14-17:Promise.allwill reject the whole load if either store refresh fails.If either
load_task_run_configsorload_task_promptsthrows (both re-throw on error), the entireloadrejects and SvelteKit renders+error.svelte, even though the other store may have refreshed successfully and the component has its own error surfaces backed by*_errors_by_task_composite_id. ConsiderPromise.allSettled, or wrap each call in a try/catch, to keep the page rendering and rely on per-store error state:♻️ Proposed refactor
- await Promise.all([ - load_task_run_configs(params.project_id, params.task_id, true), - load_task_prompts(params.project_id, params.task_id, true), - ]) + // Errors are captured in each store's *_errors_by_task_composite_id; + // use allSettled so a failure in one doesn't block the page from rendering. + await Promise.allSettled([ + load_task_run_configs(params.project_id, params.task_id, true), + load_task_prompts(params.project_id, params.task_id, true), + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/optimize/[project_id]/[task_id]/run_config/[run_config_id]/+page.ts around lines 14 - 17, The current use of Promise.all([...]) with load_task_run_configs and load_task_prompts will cause the whole load to reject if either call throws; change it so each store refresh can fail independently (use Promise.allSettled([...]) or call each function in its own try/catch) so the load function does not throw and the page can render while each store surface reports its own errors via *_errors_by_task_composite_id; update the block that currently awaits Promise.all to use Promise.allSettled or individual awaits with error handling around load_task_run_configs(params.project_id, params.task_id, true) and load_task_prompts(params.project_id, params.task_id, true).app/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/clone/[prompt_id]/+page.ts (1)
8-14: Consider swallowing load errors so the page can render its own error UI.
load_task_prompts(..., true)re-throws on failure (network error, etc.). Because this runs inside a SvelteKitload, an unhandled rejection will bubble up and cause SvelteKit to render the nearest+error.svelterather than letting the route's component display a localized error/"not found" state via the store'sprompts_errors_by_task_composite_id. Since the store already captures the error internally, you can make the load resilient by catching:♻️ Proposed refactor
export async function load({ params, }: { params: { project_id: string; task_id: string } }) { - await load_task_prompts(params.project_id, params.task_id, true) + try { + await load_task_prompts(params.project_id, params.task_id, true) + } catch { + // Error is already captured in prompts_errors_by_task_composite_id; + // let the page render and surface it in-component. + } }Also, consider using
PageLoadfrom./$typesinstead of the inline param type for better type safety as params evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/clone/[prompt_id]/+page.ts around lines 8 - 14, The load function currently calls load_task_prompts(params.project_id, params.task_id, true) which re-throws on failure and causes SvelteKit to render the global error page; wrap that call in a try/catch inside the exported load to catch and swallow errors (do not re-throw) so the store's prompts_errors_by_task_composite_id can drive the route UI, and change the load signature to use PageLoad from './$types' for stronger typing of params; ensure the catch is minimal (log if needed) but does not propagate the error.app/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.ts (1)
5-14: Duplication across three new+page.tsloaders; consider a small helper.This file, the
clone/[prompt_id]/+page.ts, andoptimize/.../+page.tsrepeat the same "refresh task prompts on navigation" pattern with identical comments and typing. A tiny shared helper (e.g.,refresh_task_prompts_on_nav(params)in$lib/stores/prompts_storeor a small utility module) would keep the three route loaders in sync and make future changes (e.g., error handling strategy, addingPageLoadtyping) a one-liner.Also note the concern raised on the other two files:
load_task_promptsre-throws on failure, which here will trigger+error.svelteinstead of letting the page render the "Prompt not found" UI backed by the store's error map. A try/catch (orallSettled-equivalent) would be consistent with the intent of reading error state from the store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.ts around lines 5 - 14, Extract the duplicated loader logic into a small helper (e.g., export async function refresh_task_prompts_on_nav(params: { project_id: string; task_id: string }) in $lib/stores/prompts_store or a tiny util) and call that from each +page.ts loader instead of repeating the block that calls load_task_prompts; inside that helper call load_task_prompts(params.project_id, params.task_id, true) wrapped in a try/catch so any error is captured and not re-thrown (so the page can render using the store's error map) and return a resolved value or boolean indicating success; also consider exporting the function with the PageLoad typing where used (replace the inline param typing in each loader with the shared helper call).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/web_ui/src/routes/`(app)/optimize/[project_id]/[task_id]/run_config/[run_config_id]/+page.ts:
- Around line 14-17: The current use of Promise.all([...]) with
load_task_run_configs and load_task_prompts will cause the whole load to reject
if either call throws; change it so each store refresh can fail independently
(use Promise.allSettled([...]) or call each function in its own try/catch) so
the load function does not throw and the page can render while each store
surface reports its own errors via *_errors_by_task_composite_id; update the
block that currently awaits Promise.all to use Promise.allSettled or individual
awaits with error handling around load_task_run_configs(params.project_id,
params.task_id, true) and load_task_prompts(params.project_id, params.task_id,
true).
In
`@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/clone/[prompt_id]/+page.ts:
- Around line 8-14: The load function currently calls
load_task_prompts(params.project_id, params.task_id, true) which re-throws on
failure and causes SvelteKit to render the global error page; wrap that call in
a try/catch inside the exported load to catch and swallow errors (do not
re-throw) so the store's prompts_errors_by_task_composite_id can drive the route
UI, and change the load signature to use PageLoad from './$types' for stronger
typing of params; ensure the catch is minimal (log if needed) but does not
propagate the error.
In
`@app/web_ui/src/routes/`(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.ts:
- Around line 5-14: Extract the duplicated loader logic into a small helper
(e.g., export async function refresh_task_prompts_on_nav(params: { project_id:
string; task_id: string }) in $lib/stores/prompts_store or a tiny util) and call
that from each +page.ts loader instead of repeating the block that calls
load_task_prompts; inside that helper call load_task_prompts(params.project_id,
params.task_id, true) wrapped in a try/catch so any error is captured and not
re-thrown (so the page can render using the store's error map) and return a
resolved value or boolean indicating success; also consider exporting the
function with the PageLoad typing where used (replace the inline param typing in
each loader with the shared helper call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f83eaf16-161a-45b3-b5a7-d6869c93947e
📒 Files selected for processing (4)
app/web_ui/src/routes/(app)/optimize/[project_id]/[task_id]/run_config/[run_config_id]/+page.tsapp/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/clone/[prompt_id]/+page.tsapp/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelteapp/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/web_ui/src/routes/(app)/prompts/[project_id]/[task_id]/saved/[prompt_id]/+page.svelte
2 participants
What does this PR do?
When chat created a new prompt or run config, clicking the link to it showed "not found" until you visited the parent list page first. This is caused by the detail page reading from a cached list that didn't know about the new entity.
Fixed by refreshing the list when the detail page opens so new entities always show up.
Tested by creating new entity and asking Chat to link it.
Checklists
Summary by CodeRabbit
Bug Fixes
Improvements