feat: Move create, edit, clone and view overrides into a new page#974
feat: Move create, edit, clone and view overrides into a new page#974
Conversation
|
Review changes with Changed Files
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. 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:
WalkthroughThe pull request introduces a comprehensive override management feature with route-based navigation and multi-step UI flows. It adds new routed pages for creating, editing, and viewing context overrides, implements a reusable multi-step form component with step indicator UI, and refactors the existing context override page to use route navigation instead of drawer-based forms. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Page as Override Page
participant Form as ContextOverrideForm
participant Modal as ChangeLogSummary
participant API as Backend API
User->>Page: Navigate to create/edit override
Page->>Form: Render with initial data
Form->>Form: Step 1: Enter context conditions
User->>Form: Fill context & metadata
Form->>Form: Step 2: Enter override values
User->>Form: Fill overrides
User->>Form: Submit
alt Editing with Precheck
Form->>API: try_update_context_payload()
API-->>Form: Precheck response
Form->>Modal: Show ChangeLogSummary
User->>Modal: Confirm changes
Modal->>API: update_context(payload)
API-->>Modal: Success
Modal->>Page: Navigate to detail/redirect
else Creating
Form->>API: create_context(overrides, context, ...)
API-->>Form: Success with new ID
Form->>Page: Navigate to override detail
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
This PR moves override creation/editing/cloning and detail viewing out of the drawer flow into dedicated pages, enabling multi-step editing and shareable URLs for specific overrides.
Changes:
- Adds new override routes/pages for create, edit, and detail views.
- Introduces a multi-step override form UI (step indicator + navigation) and a shared form component.
- Updates the overrides listing page/cards to navigate to the new pages (and adds a “See More” link).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/frontend/src/pages/override_page.rs | New pages for override detail, edit, and create (with clone support). |
| crates/frontend/src/pages/context_override.rs | Listing page updated to navigate to new routes; exports change-log confirmation UI for reuse. |
| crates/frontend/src/pages.rs | Exposes the new override_page module. |
| crates/frontend/src/components/step_indicator.rs | New step indicator + step navigation components for multi-step forms. |
| crates/frontend/src/components/context_override_form.rs | New multi-step create/edit form and a read-only override detail view component. |
| crates/frontend/src/components/context_card.rs | Adds per-card navigation (“See More”) and prevents dropdown click propagation. |
| crates/frontend/src/components/button.rs | Adds an explicit disabled prop to Button. |
| crates/frontend/src/components.rs | Exposes new components (context_override_form, step_indicator). |
| crates/frontend/src/app.rs | Registers new routes for override create/edit/detail pages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class:cursor-pointer=move || { | ||
| let cs = current_step.get(); | ||
| on_step_click.is_some() && step_type <= cs | ||
| } | ||
| class:hover:ring-2=move || { | ||
| let cs = current_step.get(); | ||
| on_step_click.is_some() && step_type <= cs | ||
| } | ||
| class:hover:ring-purple-300=move || { | ||
| let cs = current_step.get(); | ||
| on_step_click.is_some() && step_type <= cs | ||
| } | ||
| class:transition-all=true | ||
| on:click=move |_| { | ||
| let cs = current_step.get_untracked(); | ||
| if let Some(callback) = on_step_click { | ||
| if step_type <= cs { | ||
| callback.call(idx); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
on_step_click is moved into multiple move closures (class:* and on:click) and is also moved out via if let Some(callback) = on_step_click { ... }. Since Option<Callback<..>> isn’t Copy, this won’t compile and (even if it did) would consume the callback on first click. Consider storing/cloning the callback (e.g., wrap on_step_click in StoredValue/Rc, or clone the Callback into each closure and use as_ref()/cloned() inside the click handler).
There was a problem hiding this comment.
Nah bro, Callbacks implement Copy trait so they can do this. Don't worry about it, I personally tested it
| #[derive(Debug, Clone)] | ||
| enum FormMode { | ||
| Edit(String), | ||
| Clone(String), |
There was a problem hiding this comment.
FormMode::Clone (and the corresponding drawer header/match arm) appears to be dead code now: on_context_clone navigates to the new create page and there’s no remaining set_form_mode.set(Some(FormMode::Clone(...))) call. Consider removing the Clone variant and the associated drawer UI/arms (or reintroduce setting form_mode if the drawer-based clone flow is still intended).
| Clone(String), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
crates/frontend/src/components/context_card.rs (1)
89-110: Consider whetherhrefneeds URL-escaping, and note the removed per-card metadata now only exists on the detail page.
hrefis forwarded straight to<A href=...>. Context IDs in this codebase appear to be hash-style strings, so practically safe, but if an ID ever contains?,#,/, or whitespace, the link will misroute. Considerjs_sys::encode_uri_component(or equivalent) at the one call site (crates/frontend/src/pages/context_override.rs:658).The removed "Created / Last Modified" metadata is only visible now via the new
OverridePage. Worth confirming that’s the intended UX tradeoff and thatOverrideDetailViewcovers everything the drawer used to show.Also applies to: 129-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/context_card.rs` around lines 89 - 110, The href string is passed directly into the link component (see the local href variable created via store_value and the <A href=...> usage), so URL-escape the context ID before rendering (e.g., use js_sys::encode_uri_component or equivalent at the call site where href is produced) to prevent misrouting for IDs containing ?, #, /, or whitespace; also confirm the removed per-card "Created / Last Modified" metadata was intentionally migrated to the OverrideDetailView/OverridePage and that OverrideDetailView renders all previously visible drawer metadata.crates/frontend/src/components/button.rs (1)
19-49: LGTM,is_disabled = loading || disabledis applied to both class and the nativedisabledattribute, and the change is backward-compatible (defaultfalse).Optional nit: the
on:clickhandler is still attached even when disabled; nativedisabledsuppresses clicks on a<button>, but if this component is ever re-used with a non-button tag this would regress silently. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/button.rs` around lines 19 - 49, The on:click handler is still attached even when is_disabled is true; update the button rendering logic in the component (where is_disabled, on:click and the <button> are defined) to only set the on:click attribute when is_disabled is false (i.e., conditionally attach the handler instead of always using on:click=move |e| on_click.call(e)) so clicks are ignored both via the native disabled attribute and by not registering the event handler when disabled.crates/frontend/src/pages/override_page.rs (2)
271-298:clone_contextreturnsNonefor the no-clone case and is then re-read unconditionally — fine, but note double blocking.Both
clone_contextandpage_resourcearecreate_blocking_resource. SSR will block on both. If cloning is rare, consider makingclone_contexta non-blockingcreate_resourceso the common (non-clone) Create path isn’t penalized by a wasted blocking roundtrip wheneverclone_idisNone(the async body already short-circuits onid?, but SSR still waits).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/pages/override_page.rs` around lines 271 - 298, The clone_context create_blocking_resource call causes an extra SSR blocking roundtrip even when clone_id is None; change clone_context to use create_resource (non-blocking) instead of create_blocking_resource, keeping the same move || clone_id.get() loader and the async body that short-circuits on id? so no behavior change for the clone path, while avoiding blocking in the common no-clone case; leave page_resource as create_blocking_resource to continue blocking for the heavier page loads.
228-239:redirect_url_cancelandredirect_url_successare identical — consider collapsing.Both build the same
/admin/{org}/{workspace}/overrides/{ctx_id}URL. Either this is intentional (and a single variable named e.g.redirect_urlwould be clearer), or success is meant to land somewhere different (e.g., back to the list). Worth double-checking against the UX intent described in the PR.- let redirect_url_cancel = format!( - "/admin/{}/{}/overrides/{}", - org.get().0, - workspace.get().0, - ctx_id, - ); - let redirect_url_success = format!( - "/admin/{}/{}/overrides/{}", - org.get().0, - workspace.get().0, - ctx_id, - ); + let redirect_url = format!( + "/admin/{}/{}/overrides/{}", + org.get().0, + workspace.get().0, + ctx_id, + );and pass
redirect_url_cancel=redirect_url.clone() redirect_url_success=redirect_url.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/pages/override_page.rs` around lines 228 - 239, The two identical variables redirect_url_cancel and redirect_url_success should be collapsed into a single redirect_url built from org.get().0, workspace.get().0, and ctx_id (the same format!"/admin/{}/{}/overrides/{}"), and update call sites to pass redirect_url_cancel=redirect_url.clone() and redirect_url_success=redirect_url so you keep ownership semantics while removing duplication; locate the current declarations of redirect_url_cancel/redirect_url_success and replace them with one redirect_url, then update where those names are used.crates/frontend/src/components/step_indicator.rs (3)
152-159: Drop theis_last_stepprop until it’s actually used (YAGNI).
is_last_stepis accepted, immediately rebound as_is_last_stepwith a "future extensibility" comment, and never read. The real "last step" decision is inlined elsewhere viacurrent_step.get() == StepType::OverrideStep. This is confusing for callers because the prop looks wired but isn’t. Removing it now is trivial; re-add when the extensibility lands.- #[prop(default = false)] is_last_step: bool, #[prop(default = false)] next_disabled: bool, ... - let _is_last_step = is_last_step; // Used for future extensibility🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/step_indicator.rs` around lines 152 - 159, Remove the unused prop and its binding: delete the is_last_step parameter from the prop list and remove the line that rebinding it to _is_last_step in the step indicator component (the unused symbols are is_last_step and _is_last_step); keep the existing runtime logic that determines last-step via current_step.get() == StepType::OverrideStep and update any tests or callers that pass is_last_step to stop providing it.
7-12:StepTypeordering relies on variant declaration order — leave a note.Because
PartialOrdis derived, the enum literally depends onContextStepbeing declared beforeOverrideStepforstep_type < csto correctly mean “completed”. A future maintainer adding a step in the middle (or re-ordering alphabetically out of habit) will silently break the indicator. A short doc comment will save someone an hour:#[derive(Clone, Copy, PartialEq, PartialOrd, Display)] #[strum(serialize_all = "snake_case")] +/// Variant order is load-bearing: derived `PartialOrd` uses declaration order, +/// and `StepIndicator` treats `step_type < current_step` as "completed". +/// Keep variants listed in flow order. pub enum StepType { ContextStep, OverrideStep, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/step_indicator.rs` around lines 7 - 12, The derived PartialOrd on StepType relies on variant declaration order (ContextStep before OverrideStep) so comparisons like step_type < cs depend on that ordering; add a short doc comment above the StepType enum warning future editors not to reorder or alphabetize variants, and either (recommended) give explicit discriminant values to each variant or implement Ord/PartialOrd manually for explicit, maintainable ordering; reference the StepType enum and the ContextStep/OverrideStep variants in the comment so maintainers know why order matters.
193-222: Branching submit vs next onStepType::OverrideStepduplicates the "last step" concept.The component accepts both an
is_last_stepprop and derives last-step-ness fromcurrent_step == OverrideStep. Right now they agree (becauseOverrideStephappens to be the last step), but the mismatch will bite if/when another step is added. Consider keying this branch offis_last_step(once it is actually threaded) or exposing alast_step_type: StepTypeprop, soStepNavigationstops hard-coding knowledge of the flow’s terminal step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/step_indicator.rs` around lines 193 - 222, The branch that decides whether to render the "submit" vs "next" button relies on current_step == StepType::OverrideStep, duplicating the "last step" concept; change it to use the component's canonical last-step indicator (use is_last_step prop or a provided last_step_type) instead of hard-coding StepType::OverrideStep so the logic becomes: if is_last_step { render submit button that uses on_submit, submit_text, submit_loading } else { render next button that uses on_next, next_text, next_disabled }; update the closure that currently references current_step and StepType::OverrideStep to reference is_last_step (or compare against last_step_type) and keep on_submit, on_next, submit_text, next_text, submit_loading, and next_disabled unchanged.crates/frontend/src/components/context_override_form.rs (3)
100-116: Collapse the no-op match arms inon_previous/on_next.Both callbacks only ever transition in one direction; the same-step arms are dead branches. An
if letormatches!guard is clearer.♻️ Proposed simplification
- let on_previous = Callback::new(move |_| { - current_step.update(|step| { - *step = match step { - StepType::OverrideStep => StepType::ContextStep, - StepType::ContextStep => StepType::ContextStep, - }; - }); - }); - - let on_next = Callback::new(move |_| { - current_step.update(|step| { - *step = match step { - StepType::ContextStep => StepType::OverrideStep, - StepType::OverrideStep => StepType::OverrideStep, - }; - }); - }); + let on_previous = Callback::new(move |_| { + current_step.update(|step| { + if matches!(step, StepType::OverrideStep) { + *step = StepType::ContextStep; + } + }); + }); + + let on_next = Callback::new(move |_| { + current_step.update(|step| { + if matches!(step, StepType::ContextStep) { + *step = StepType::OverrideStep; + } + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/context_override_form.rs` around lines 100 - 116, The on_previous and on_next callbacks contain no-op match arms; simplify them by replacing the match in current_step.update closures with a single-branch conditional that only changes the step when it matches the expected variant. For example, in on_previous update only when *step == StepType::OverrideStep to set StepType::ContextStep, and in on_next update only when *step == StepType::ContextStep to set StepType::OverrideStep (use if let or matches! against current_step within the closure), removing the dead same-step branches.
356-420: Move theEffectout of the view body and de‑duplicate error logging.Two small concerns in
ChangeLogSummary:
Effect::new(...)is being created inside the<Suspense>children block (lines 360–367). The returned()is discarded byview!, which works incidentally but is an anti‑pattern — the effect should be registered in the component body so ownership/disposal is obvious and it isn't re-registered if the view re‑renders.- On resource error the same error is logged twice: once in the effect at line 365 and again in the match arm at line 415. Keep a single log site (the match arm shows the user-visible error UI, so log there and drop it from the effect).
♻️ Proposed refactor
- view! { + Effect::new(move |_| { + if let Some(Ok(_)) = context_resource.get() { + disabled_rws.set(false); + } + }); + + view! { <ChangeLogPopup title description confirm_text on_confirm on_close disabled=disabled_rws inprogress > <Suspense fallback=move || { view! { <Skeleton variant=SkeletonVariant::Block style_class="h-10".to_string() /> } }> - { - Effect::new(move |_| { - let context = context_resource.get(); - if let Some(Ok(_)) = context { - disabled_rws.set(false); - } else if let Some(Err(e)) = context { - logging::error!("Error fetching context: {}", e); - } - }); - } {move || match context_resource.get() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/context_override_form.rs` around lines 356 - 420, Move the Effect::new(...) that reads context_resource and sets disabled_rws out of the Suspense view body and register it in the component body (e.g. at the top of the function) so it isn’t recreated on each render; keep its behavior limited to checking context_resource.get() and calling disabled_rws.set(false) when Some(Ok(_)) and remove the logging from this effect. Keep the single error log in the Suspense match arm that handles Some(Err(e)) (the block that currently does logging::error!("Error fetching context: {}", e) and renders the error UI) so errors are only logged once. Ensure you reference the existing symbols Effect::new, context_resource, and disabled_rws when moving the logic.
431-468: Changechange_reasonparameter fromStringtoChangeReasontype.The caller in
override_page.rsalready holds the value asChangeReason(viacontext.change_reason), converts it toStringunnecessarily, and passes it toOverrideDetailView, which then converts it back usingtry_from().unwrap_or_default(). This round-trip conversion silently discards validation errors: empty strings and overlong inputs triggerunwrap_or_default()without any indication to the user on a read-only detail view.Accept the value as-is:
🔧 Proposed change
pub fn OverrideDetailView( context: Conditions, overrides: Vec<(String, Value)>, description: Description, override_id: String, - change_reason: String, + change_reason: ChangeReason, created_by: String, ... ) -> impl IntoView { ... <crate::components::description::ContentDescription description=description - change_reason=ChangeReason::try_from(change_reason).unwrap_or_default() + change_reason=change_reason created_by=created_byRemove the
.deref().to_string()conversion inoverride_page.rsand passchange_reasondirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/context_override_form.rs` around lines 431 - 468, The function currently accepts change_reason: String and then does ChangeReason::try_from(change_reason).unwrap_or_default(), which causes silent validation loss; change the parameter type to ChangeReason (e.g., change_reason: ChangeReason) in the function signature in context_override_form.rs and update the call site in override_page.rs to pass the ChangeReason directly (remove the .deref().to_string() conversion). Also update where the value is used (the prop passed into crate::components::description::ContentDescription) to use the ChangeReason value directly so you no longer need try_from()/unwrap_or_default() inside this component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/frontend/src/components/context_override_form.rs`:
- Around line 290-302: The ChangeLogSummary confirm button doesn't reflect the
real loading state because it uses its default inprogress Signal; forward the
existing req_inprogress_rs into the component so the popup uses the actual
in‑flight signal. In the view where update_request_rws is matched (the block
that creates <ChangeLogSummary ...>), add a prop like
inprogress=req_inprogress_rs.clone() (or pass the Signal directly) and ensure
ChangeLogSummary's prop name (inprogress) is used rather than its internal
Signal::derive default; this ensures update_context's in‑flight signal prevents
double submissions and provides loading feedback.
In `@crates/frontend/src/pages/override_page.rs`:
- Around line 38-43: The code currently silently falls back to the string "1"
when extracting the "context_id" route param in the Memo created for context_id
(using leptos_router::use_params_map and Memo::new), which masks routing bugs;
change the extraction to not invent a default—either call expect/unwrap with a
clear message or convert the param to an Option and make the surrounding
component return an early error view when the param is missing. Apply the same
change in the EditOverride component (the code block building its context_id at
lines ~174–176): stop using unwrap_or("1".into()) and instead propagate/handle
the missing param explicitly so a misconfigured route surfaces an error.
- Around line 263-309: The redirect URL values are built using reactive reads
workspace.get() and org.get() at component setup which triggers "called outside
reactive context" warnings and can produce stale values; update CreateOverride
to call get_untracked() on the workspace and org signals (or move the formatting
into the view closure) when constructing redirect_url_cancel and
redirect_url_success so the reads are non-reactive and silence warnings while
keeping intent explicit (apply the same get_untracked() change for any similar
URL construction in EditOverride if present).
- Around line 54-86: The confirm_delete closure currently calls use_navigate()
and reads org.get()/workspace.get() inside the spawn_local async block; capture
the navigate function and the untracked org/workspace values synchronously when
building the closure so the async task uses those captured values: call
use_navigate() outside spawn_local and store its result in a local (e.g.,
navigate), call org.get_untracked() and workspace.get_untracked() before
awaiting delete_context and move those captured values into the async block,
then use them to build redirect_url and navigate after the await; ensure
delete_context still receives workspace.get_untracked() and org.get_untracked()
as before.
---
Nitpick comments:
In `@crates/frontend/src/components/button.rs`:
- Around line 19-49: The on:click handler is still attached even when
is_disabled is true; update the button rendering logic in the component (where
is_disabled, on:click and the <button> are defined) to only set the on:click
attribute when is_disabled is false (i.e., conditionally attach the handler
instead of always using on:click=move |e| on_click.call(e)) so clicks are
ignored both via the native disabled attribute and by not registering the event
handler when disabled.
In `@crates/frontend/src/components/context_card.rs`:
- Around line 89-110: The href string is passed directly into the link component
(see the local href variable created via store_value and the <A href=...>
usage), so URL-escape the context ID before rendering (e.g., use
js_sys::encode_uri_component or equivalent at the call site where href is
produced) to prevent misrouting for IDs containing ?, #, /, or whitespace; also
confirm the removed per-card "Created / Last Modified" metadata was
intentionally migrated to the OverrideDetailView/OverridePage and that
OverrideDetailView renders all previously visible drawer metadata.
In `@crates/frontend/src/components/context_override_form.rs`:
- Around line 100-116: The on_previous and on_next callbacks contain no-op match
arms; simplify them by replacing the match in current_step.update closures with
a single-branch conditional that only changes the step when it matches the
expected variant. For example, in on_previous update only when *step ==
StepType::OverrideStep to set StepType::ContextStep, and in on_next update only
when *step == StepType::ContextStep to set StepType::OverrideStep (use if let or
matches! against current_step within the closure), removing the dead same-step
branches.
- Around line 356-420: Move the Effect::new(...) that reads context_resource and
sets disabled_rws out of the Suspense view body and register it in the component
body (e.g. at the top of the function) so it isn’t recreated on each render;
keep its behavior limited to checking context_resource.get() and calling
disabled_rws.set(false) when Some(Ok(_)) and remove the logging from this
effect. Keep the single error log in the Suspense match arm that handles
Some(Err(e)) (the block that currently does logging::error!("Error fetching
context: {}", e) and renders the error UI) so errors are only logged once.
Ensure you reference the existing symbols Effect::new, context_resource, and
disabled_rws when moving the logic.
- Around line 431-468: The function currently accepts change_reason: String and
then does ChangeReason::try_from(change_reason).unwrap_or_default(), which
causes silent validation loss; change the parameter type to ChangeReason (e.g.,
change_reason: ChangeReason) in the function signature in
context_override_form.rs and update the call site in override_page.rs to pass
the ChangeReason directly (remove the .deref().to_string() conversion). Also
update where the value is used (the prop passed into
crate::components::description::ContentDescription) to use the ChangeReason
value directly so you no longer need try_from()/unwrap_or_default() inside this
component.
In `@crates/frontend/src/components/step_indicator.rs`:
- Around line 152-159: Remove the unused prop and its binding: delete the
is_last_step parameter from the prop list and remove the line that rebinding it
to _is_last_step in the step indicator component (the unused symbols are
is_last_step and _is_last_step); keep the existing runtime logic that determines
last-step via current_step.get() == StepType::OverrideStep and update any tests
or callers that pass is_last_step to stop providing it.
- Around line 7-12: The derived PartialOrd on StepType relies on variant
declaration order (ContextStep before OverrideStep) so comparisons like
step_type < cs depend on that ordering; add a short doc comment above the
StepType enum warning future editors not to reorder or alphabetize variants, and
either (recommended) give explicit discriminant values to each variant or
implement Ord/PartialOrd manually for explicit, maintainable ordering; reference
the StepType enum and the ContextStep/OverrideStep variants in the comment so
maintainers know why order matters.
- Around line 193-222: The branch that decides whether to render the "submit" vs
"next" button relies on current_step == StepType::OverrideStep, duplicating the
"last step" concept; change it to use the component's canonical last-step
indicator (use is_last_step prop or a provided last_step_type) instead of
hard-coding StepType::OverrideStep so the logic becomes: if is_last_step {
render submit button that uses on_submit, submit_text, submit_loading } else {
render next button that uses on_next, next_text, next_disabled }; update the
closure that currently references current_step and StepType::OverrideStep to
reference is_last_step (or compare against last_step_type) and keep on_submit,
on_next, submit_text, next_text, submit_loading, and next_disabled unchanged.
In `@crates/frontend/src/pages/override_page.rs`:
- Around line 271-298: The clone_context create_blocking_resource call causes an
extra SSR blocking roundtrip even when clone_id is None; change clone_context to
use create_resource (non-blocking) instead of create_blocking_resource, keeping
the same move || clone_id.get() loader and the async body that short-circuits on
id? so no behavior change for the clone path, while avoiding blocking in the
common no-clone case; leave page_resource as create_blocking_resource to
continue blocking for the heavier page loads.
- Around line 228-239: The two identical variables redirect_url_cancel and
redirect_url_success should be collapsed into a single redirect_url built from
org.get().0, workspace.get().0, and ctx_id (the same
format!"/admin/{}/{}/overrides/{}"), and update call sites to pass
redirect_url_cancel=redirect_url.clone() and redirect_url_success=redirect_url
so you keep ownership semantics while removing duplication; locate the current
declarations of redirect_url_cancel/redirect_url_success and replace them with
one redirect_url, then update where those names are used.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cfe92187-7ac3-410f-ac8f-206794f4f6ca
📒 Files selected for processing (9)
crates/frontend/src/app.rscrates/frontend/src/components.rscrates/frontend/src/components/button.rscrates/frontend/src/components/context_card.rscrates/frontend/src/components/context_override_form.rscrates/frontend/src/components/step_indicator.rscrates/frontend/src/pages.rscrates/frontend/src/pages/context_override.rscrates/frontend/src/pages/override_page.rs
| {move || match update_request_rws.get() { | ||
| None => ().into_view(), | ||
| Some((ctx_id, update_request)) => { | ||
| view! { | ||
| <ChangeLogSummary | ||
| context_id=ctx_id | ||
| change_type=ChangeType::Update(update_request) | ||
| on_confirm=on_submit | ||
| on_close=Callback::new(move |_| update_request_rws.set(None)) | ||
| /> | ||
| } | ||
| } | ||
| }} |
There was a problem hiding this comment.
Forward req_inprogress_rs to ChangeLogSummary so the confirm button reflects loading.
ChangeLogSummary defaults inprogress to Signal::derive(|| false) (line 312), so while the confirm‑triggered update_context future is awaiting, the popup's confirm button remains enabled. There is a small race window between confirm (line 119) and the synchronous update_request_rws.set(None) at line 130 in which the user can double‑click confirm and issue two concurrent update_context calls. Piping the existing in‑flight signal closes that window and gives users feedback.
🔧 Proposed fix
<ChangeLogSummary
context_id=ctx_id
change_type=ChangeType::Update(update_request)
on_confirm=on_submit
on_close=Callback::new(move |_| update_request_rws.set(None))
+ inprogress=req_inprogress_rs.into()
/>📝 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.
| {move || match update_request_rws.get() { | |
| None => ().into_view(), | |
| Some((ctx_id, update_request)) => { | |
| view! { | |
| <ChangeLogSummary | |
| context_id=ctx_id | |
| change_type=ChangeType::Update(update_request) | |
| on_confirm=on_submit | |
| on_close=Callback::new(move |_| update_request_rws.set(None)) | |
| /> | |
| } | |
| } | |
| }} | |
| {move || match update_request_rws.get() { | |
| None => ().into_view(), | |
| Some((ctx_id, update_request)) => { | |
| view! { | |
| <ChangeLogSummary | |
| context_id=ctx_id | |
| change_type=ChangeType::Update(update_request) | |
| on_confirm=on_submit | |
| on_close=Callback::new(move |_| update_request_rws.set(None)) | |
| inprogress=req_inprogress_rs.into() | |
| /> | |
| } | |
| } | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/frontend/src/components/context_override_form.rs` around lines 290 -
302, The ChangeLogSummary confirm button doesn't reflect the real loading state
because it uses its default inprogress Signal; forward the existing
req_inprogress_rs into the component so the popup uses the actual in‑flight
signal. In the view where update_request_rws is matched (the block that creates
<ChangeLogSummary ...>), add a prop like inprogress=req_inprogress_rs.clone()
(or pass the Signal directly) and ensure ChangeLogSummary's prop name
(inprogress) is used rather than its internal Signal::derive default; this
ensures update_context's in‑flight signal prevents double submissions and
provides loading feedback.
| let path_params = leptos_router::use_params_map(); | ||
| let workspace = use_context::<Signal<Workspace>>().unwrap(); | ||
| let org = use_context::<Signal<OrganisationId>>().unwrap(); | ||
| let context_id = Memo::new(move |_| { | ||
| path_params.with(|params| params.get("context_id").cloned().unwrap_or("1".into())) | ||
| }); |
There was a problem hiding this comment.
Silent "1" fallback for a missing context_id hides routing bugs.
params.get("context_id").cloned().unwrap_or("1".into()) means any misconfigured route would silently fetch/edit the context with id "1" instead of surfacing the problem. The route guarantees the param exists, so prefer early-returning an error view (or unwrap_or_default() and letting the fetch fail) over inventing "1".
- let context_id = Memo::new(move |_| {
- path_params.with(|params| params.get("context_id").cloned().unwrap_or("1".into()))
- });
+ let context_id = Memo::new(move |_| {
+ path_params.with(|params| params.get("context_id").cloned().unwrap_or_default())
+ });Same comment applies to EditOverride at lines 174–176.
Also applies to: 170-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/frontend/src/pages/override_page.rs` around lines 38 - 43, The code
currently silently falls back to the string "1" when extracting the "context_id"
route param in the Memo created for context_id (using
leptos_router::use_params_map and Memo::new), which masks routing bugs; change
the extraction to not invent a default—either call expect/unwrap with a clear
message or convert the param to an Option and make the surrounding component
return an early error view when the param is missing. Apply the same change in
the EditOverride component (the code block building its context_id at lines
~174–176): stop using unwrap_or("1".into()) and instead propagate/handle the
missing param explicitly so a misconfigured route surfaces an error.
| let confirm_delete = move |context_id: String| { | ||
| delete_inprogress_rws.set(true); | ||
| spawn_local(async move { | ||
| let result = delete_context( | ||
| context_id.clone(), | ||
| &workspace.get_untracked(), | ||
| &org.get_untracked(), | ||
| ) | ||
| .await; | ||
| delete_inprogress_rws.set(false); | ||
| match result { | ||
| Ok(_) => { | ||
| logging::log!("Context deleted successfully"); | ||
| let navigate = use_navigate(); | ||
| let redirect_url = format!( | ||
| "/admin/{}/{}/overrides", | ||
| org.get().0, | ||
| workspace.get().0, | ||
| ); | ||
| navigate(&redirect_url, Default::default()); | ||
| enqueue_alert( | ||
| String::from("Override deleted successfully"), | ||
| AlertType::Success, | ||
| 5000, | ||
| ); | ||
| } | ||
| Err(e) => { | ||
| logging::error!("Error deleting context: {:?}", e); | ||
| enqueue_alert(e, AlertType::Error, 5000); | ||
| } | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In leptos 0.6, is it safe to call use_navigate() from inside spawn_local, or must it be obtained in the component/callback synchronously?
💡 Result:
No, it is not safe to call use_navigate directly from inside spawn_local. use_navigate is a hook that must be called synchronously during component setup or in synchronous reactive contexts (like effects or callbacks) within the component's reactive scope. spawn_local spawns an asynchronous task that runs outside the synchronous reactive Owner scope of the component, so calling use_navigate inside it will fail because it cannot access the router context provided by the Router component. To navigate from an async task like spawn_local, obtain the navigate function synchronously in the component first (let navigate = use_navigate;), clone it (as it implements Clone), and move the clone into the spawn_local closure. Then call the cloned navigate function inside the async block. This pattern is demonstrated in Leptos examples and aligns with hook usage rules enforced in later versions (e.g., explicit Router scope checks in 0.7+), and context access patterns where values must be retrieved synchronously before async use.
Citations:
- 1: https://docs.rs/leptos_router/latest/leptos_router/hooks/fn.use_navigate.html
- 2: Get error on upgrade to leptos 0.7. You cannot call `use_navigate` outside a <Router>. leptos-rs/leptos#3723
- 3: https://github.com/leptos-rs/leptos/blob/main/examples/router/src/lib.rs
- 4: https://docs.rs/leptos_router/latest/src/leptos_router/components.rs.html
- 5: https://docs.rs/leptos/0.8.17/leptos/prelude/fn.use_context.html
Capture use_navigate outside spawn_local, and use get_untracked() consistently for post-await reads.
Two issues in confirm_delete (lines 54–86):
use_navigate()is invoked inside the async task (line 67). In Leptos,use_navigateis a hook that requires the synchronous reactive scope of the component; calling it insidespawn_local(which runs outside that scope) will fail to access the router context. Instead, capture the navigate function synchronously when building the callback and move it into the async block.- Lines 70–71 read
org.get()/workspace.get()reactively insidespawn_local, while lines 59–60 useget_untracked(). Useget_untracked()consistently here as well—the values cannot change meaningfully mid-delete.
🛠️ Proposed fix
let confirm_delete = move |context_id: String| {
delete_inprogress_rws.set(true);
+ let navigate = use_navigate();
spawn_local(async move {
let result = delete_context(
context_id.clone(),
&workspace.get_untracked(),
&org.get_untracked(),
)
.await;
delete_inprogress_rws.set(false);
match result {
Ok(_) => {
logging::log!("Context deleted successfully");
- let navigate = use_navigate();
let redirect_url = format!(
"/admin/{}/{}/overrides",
- org.get().0,
- workspace.get().0,
+ org.get_untracked().0,
+ workspace.get_untracked().0,
);
navigate(&redirect_url, Default::default());📝 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.
| let confirm_delete = move |context_id: String| { | |
| delete_inprogress_rws.set(true); | |
| spawn_local(async move { | |
| let result = delete_context( | |
| context_id.clone(), | |
| &workspace.get_untracked(), | |
| &org.get_untracked(), | |
| ) | |
| .await; | |
| delete_inprogress_rws.set(false); | |
| match result { | |
| Ok(_) => { | |
| logging::log!("Context deleted successfully"); | |
| let navigate = use_navigate(); | |
| let redirect_url = format!( | |
| "/admin/{}/{}/overrides", | |
| org.get().0, | |
| workspace.get().0, | |
| ); | |
| navigate(&redirect_url, Default::default()); | |
| enqueue_alert( | |
| String::from("Override deleted successfully"), | |
| AlertType::Success, | |
| 5000, | |
| ); | |
| } | |
| Err(e) => { | |
| logging::error!("Error deleting context: {:?}", e); | |
| enqueue_alert(e, AlertType::Error, 5000); | |
| } | |
| } | |
| }); | |
| }; | |
| let confirm_delete = move |context_id: String| { | |
| delete_inprogress_rws.set(true); | |
| let navigate = use_navigate(); | |
| spawn_local(async move { | |
| let result = delete_context( | |
| context_id.clone(), | |
| &workspace.get_untracked(), | |
| &org.get_untracked(), | |
| ) | |
| .await; | |
| delete_inprogress_rws.set(false); | |
| match result { | |
| Ok(_) => { | |
| logging::log!("Context deleted successfully"); | |
| let redirect_url = format!( | |
| "/admin/{}/{}/overrides", | |
| org.get_untracked().0, | |
| workspace.get_untracked().0, | |
| ); | |
| navigate(&redirect_url, Default::default()); | |
| enqueue_alert( | |
| String::from("Override deleted successfully"), | |
| AlertType::Success, | |
| 5000, | |
| ); | |
| } | |
| Err(e) => { | |
| logging::error!("Error deleting context: {:?}", e); | |
| enqueue_alert(e, AlertType::Error, 5000); | |
| } | |
| } | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/frontend/src/pages/override_page.rs` around lines 54 - 86, The
confirm_delete closure currently calls use_navigate() and reads
org.get()/workspace.get() inside the spawn_local async block; capture the
navigate function and the untracked org/workspace values synchronously when
building the closure so the async task uses those captured values: call
use_navigate() outside spawn_local and store its result in a local (e.g.,
navigate), call org.get_untracked() and workspace.get_untracked() before
awaiting delete_context and move those captured values into the async block,
then use them to build redirect_url and navigate after the await; ensure
delete_context still receives workspace.get_untracked() and org.get_untracked()
as before.
| pub fn CreateOverride() -> impl IntoView { | ||
| let workspace = use_context::<Signal<Workspace>>().unwrap(); | ||
| let org = use_context::<Signal<OrganisationId>>().unwrap(); | ||
| let query = leptos_router::use_query_map(); | ||
| let clone_id = Memo::new(move |_| { | ||
| query.with(|q| q.get("clone").cloned()) | ||
| }); | ||
|
|
||
| let clone_context = create_blocking_resource( | ||
| move || clone_id.get(), | ||
| move |id| async move { | ||
| let id = id?; | ||
| get_context(&id, &workspace.get().0, &org.get().0).await.ok() | ||
| }, | ||
| ); | ||
|
|
||
| let page_resource = create_blocking_resource( | ||
| move || (workspace.get().0, org.get().0), | ||
| |(workspace, org_id)| async move { | ||
| let empty_list_filters = PaginationParams::all_entries(); | ||
| let default_config_filters = DefaultConfigFilters::default(); | ||
| let (dimensions_result, default_config_result) = join!( | ||
| dimensions::list(&empty_list_filters, &workspace, &org_id), | ||
| default_configs::list( | ||
| &empty_list_filters, | ||
| &default_config_filters, | ||
| &workspace, | ||
| &org_id | ||
| ), | ||
| ); | ||
| FormPageResource { | ||
| dimensions: dimensions_result.unwrap_or_default().data, | ||
| default_config: default_config_result.unwrap_or_default().data, | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| let redirect_url_cancel = store_value(format!( | ||
| "/admin/{}/{}/overrides", | ||
| org.get().0, | ||
| workspace.get().0 | ||
| )); | ||
| let redirect_url_success = store_value(format!( | ||
| "/admin/{}/{}/overrides", | ||
| org.get().0, | ||
| workspace.get().0 | ||
| )); |
There was a problem hiding this comment.
org.get() / workspace.get() at component setup — use get_untracked() (or move into the view closure) to avoid reactive-read-outside-context warnings and stale URLs.
Lines 300–309 call org.get() / workspace.get() at component-body scope (outside any reactive closure) and stash the formatted URLs in store_value. Two minor issues:
- In leptos 0.6 these reactive reads outside a reactive context typically log
at_whole_app: called outside reactive context(or similar) warnings and won’t re-subscribe correctly. Since this is top-level setup,get_untracked()expresses the intent and silences the warning. - If the user switches org/workspace while the
CreateOverridepage is mounted, these URLs won’t update — probably impossible in practice but worth a sanity check.
- let redirect_url_cancel = store_value(format!(
- "/admin/{}/{}/overrides",
- org.get().0,
- workspace.get().0
- ));
- let redirect_url_success = store_value(format!(
- "/admin/{}/{}/overrides",
- org.get().0,
- workspace.get().0
- ));
+ let redirect_url = store_value(format!(
+ "/admin/{}/{}/overrides",
+ org.get_untracked().0,
+ workspace.get_untracked().0
+ ));Same pattern could be applied to the EditOverride URLs inside the view closure — those are fine reactively, but also effectively never change, so get_untracked() is more precise there too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/frontend/src/pages/override_page.rs` around lines 263 - 309, The
redirect URL values are built using reactive reads workspace.get() and org.get()
at component setup which triggers "called outside reactive context" warnings and
can produce stale values; update CreateOverride to call get_untracked() on the
workspace and org signals (or move the formatting into the view closure) when
constructing redirect_url_cancel and redirect_url_success so the reads are
non-reactive and silence warnings while keeping intent explicit (apply the same
get_untracked() change for any similar URL construction in EditOverride if
present).
Datron
force-pushed
the
superposition-separate-forms
branch
from
1375330 to
05402d8
Compare
Problem
The drawer for overrides form made it inconvenient for users to edit multiple overrides and a flow was not established. For a particular override, it could not be shared by URL so that someone else could view it.
Solution
Move create, edit, clone forms and view overrides to a new pages. The form is now multi-step, allowing more space for editing context and overrides
Screen.Recording.2026-04-20.at.5.39.12.PM.mov
Summary by CodeRabbit
Release Notes