feat: standalone create page for default-config #826
Conversation
|
Review changes with Changed Files
|
WalkthroughThe changes refactor default config management by introducing dedicated route-based pages for creation and editing. New routes are added at Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
- Add breadcrumb navigation (Default Config → [key] → Edit) - Add header with title and cancel button - Wrap form in card container matching create page styling - Add proper cancel navigation back to config detail page
knutties
force-pushed
the
standalone-create-page
branch
from
5284503 to
988af74
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @crates/frontend/src/pages/default_config_create.rs:
- Around line 29-31: Remove the temporary debug logging call that prints the
parsed prefix: delete the logging::log!("Extracted prefix: '{}'", parsed);
statement in the default_config_create.rs code path that returns parsed (so the
function/closure still returns parsed unchanged); ensure no other
development-only logging remains (search for logging::log! in the same function
or module) before merging.
- Around line 154-163: Remove the temporary debug call and simplify the inline
prefix logic: delete the logging::log! line, then replace the nested inline
conditional with clear steps using the existing prefix.get() value (p) — if
p.is_empty() return None, otherwise ensure the string ends with a single '/'
(append one only if !p.ends_with('/')) and return Some(the normalized string).
Keep references to the same variable names (prefix, p) so you update the
expression used as the prefix={...} prop.
In @crates/frontend/src/pages/default_config.rs:
- Line 264: StoredValue currently captures org.get().0 and workspace.get().0
once, so change breadcrumb_url to be a derived/reactive signal that recomputes
when org or workspace change (e.g., replace StoredValue::new(...) with a
memo/derived created from org and workspace that formats the same string) and
update the view to call breadcrumb_url.get() (instead of
breadcrumb_url.get_value()) so the breadcrumb link stays up-to-date; reference
the breadcrumb_url symbol and the org and workspace signals when making the
change.
🧹 Nitpick comments (4)
crates/frontend/src/pages/default_config_create.rs (2)
67-75: URL-encode the prefix query parameter.If
prefixcontains special characters (spaces, ampersands, etc.), they should be URL-encoded to prevent malformed URLs.♻️ Use URL encoding for query parameters
Consider using
urlencoding::encodeor similar:} else { format!( "{}/admin/{}/{}/default-config?prefix={}", base_submit, org_submit.get().0, workspace_submit.get().0, - current_prefix + urlencoding::encode(¤t_prefix) ) };Apply the same fix to the
handle_cancelcallback (lines 95-101).
46-105: Consider extracting shared navigation URL builder.Both
handle_submitandhandle_cancelconstruct the same navigation URL with nearly identical logic. Consider extracting a helper closure or function to reduce duplication.♻️ Extract shared navigation logic
let build_list_url = move |prefix: &str| { let base_url = format!( "{}/admin/{}/{}/default-config", base, org.get().0, workspace.get().0 ); if prefix.is_empty() { base_url } else { format!("{}?prefix={}", base_url, urlencoding::encode(prefix)) } }; let handle_submit = Callback::new(move |_| { enqueue_alert(/* ... */); navigate(&build_list_url(&prefix.get()), Default::default()); }); let handle_cancel = Callback::new(move |_| { navigate(&build_list_url(&prefix.get()), Default::default()); });crates/frontend/src/pages/default_config.rs (2)
309-311: Inconsistent behavior: no success feedback after edit.Unlike
CreateDefaultConfigwhich shows a success alert and navigates back to the list, this handler only refetches data. Consider adding a success alert and optionally navigating back to the detail page for consistency.♻️ Add success feedback
<DefaultConfigForm edit=true config_key=default_config.key.clone() config_value=default_config.value.clone() type_schema=Value::from(&default_config.schema) description=default_config.description.deref().to_string() validation_function_name=default_config.value_validation_function_name.clone() value_compute_function_name=default_config.value_compute_function_name.clone() handle_submit=move |_| { + enqueue_alert( + "Default config updated successfully!".to_string(), + AlertType::Success, + 5000, + ); default_config_resource.refetch(); } />You'll need to import
enqueue_alertandAlertTypeif not already available in scope.
243-250: Minor: Inconsistent indentation.The
create_blocking_resourceblock has inconsistent indentation compared to the rest of the function.🧹 Fix indentation
-let default_config_resource = create_blocking_resource( + let default_config_resource = create_blocking_resource( move || (default_config_key.get(), workspace.get().0, org.get().0), |(default_config_key, workspace, org_id)| async move { get_default_config(&default_config_key, &workspace, &org_id) .await .ok() }, );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/frontend/src/app.rscrates/frontend/src/pages.rscrates/frontend/src/pages/default_config.rscrates/frontend/src/pages/default_config_create.rscrates/frontend/src/pages/default_config_list.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/frontend/src/app.rs (2)
crates/frontend/src/pages/default_config_create.rs (1)
CreateDefaultConfig(13-170)crates/frontend/src/pages/default_config_list.rs (1)
default_config_list(37-258)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (aarch64-darwin)
🔇 Additional comments (5)
crates/frontend/src/pages/default_config_list.rs (1)
228-233: LGTM!The transition from a modal-based
Buttonto aButtonAnchorfor navigation to the standalone create page is clean. The relative href"-/create"correctly resolves to the new route.crates/frontend/src/pages.rs (1)
7-7: LGTM!The new module
default_config_createis correctly declared and follows the existing naming conventions.crates/frontend/src/app.rs (1)
171-185: Route ordering is correct.The routes are properly ordered with the literal path
"default-config/-/create"before the parameterized"default-config/:config_key/edit"and"default-config/:config_key". This ensures the create route matches before the parameter captures-as a config key.crates/frontend/src/pages/default_config.rs (2)
191-197: LGTM!Clean replacement of the Button with an anchor link for navigation to the edit route.
216-224: LGTM!The
Showcomponent cleanly handles the delete confirmation flow, simplifying the previous match-based approach.
| // Debug: log the parsed prefix | ||
| logging::log!("Extracted prefix: '{}'", parsed); | ||
| parsed |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug logging before merging.
Debug logging::log! statements should not be shipped to production. These are useful during development but add noise to the console output.
🧹 Remove debug logging
- // Debug: log the parsed prefix
- logging::log!("Extracted prefix: '{}'", parsed);
parsed📝 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.
| // Debug: log the parsed prefix | |
| logging::log!("Extracted prefix: '{}'", parsed); | |
| parsed | |
| parsed |
🤖 Prompt for AI Agents
In @crates/frontend/src/pages/default_config_create.rs around lines 29 - 31,
Remove the temporary debug logging call that prints the parsed prefix: delete
the logging::log!("Extracted prefix: '{}'", parsed); statement in the
default_config_create.rs code path that returns parsed (so the function/closure
still returns parsed unchanged); ensure no other development-only logging
remains (search for logging::log! in the same function or module) before
merging.
| prefix={ | ||
| let p = prefix.get(); | ||
| // Debug: log what we're passing to the form | ||
| logging::log!("Passing prefix to form: {:?}", (if p.is_empty() { None } else { Some(if p.ends_with('/') { p.clone() } else { format!("{}/", p.clone()) }) })); | ||
| if p.is_empty() { | ||
| None | ||
| } else { | ||
| Some(if p.ends_with('/') { p } else { format!("{}/", p) }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug logging and simplify inline logic.
The debug logging should be removed, and the complex inline expression can be simplified for readability.
🧹 Simplified prefix handling
<DefaultConfigForm
edit=false
- prefix={
- let p = prefix.get();
- // Debug: log what we're passing to the form
- logging::log!("Passing prefix to form: {:?}", (if p.is_empty() { None } else { Some(if p.ends_with('/') { p.clone() } else { format!("{}/", p.clone()) }) }));
- if p.is_empty() {
- None
- } else {
- Some(if p.ends_with('/') { p } else { format!("{}/", p) })
- }
- }
+ prefix={
+ let p = prefix.get();
+ if p.is_empty() {
+ None
+ } else {
+ Some(if p.ends_with('/') { p } else { format!("{}/", p) })
+ }
+ }
handle_submit=handle_submit
/>📝 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.
| prefix={ | |
| let p = prefix.get(); | |
| // Debug: log what we're passing to the form | |
| logging::log!("Passing prefix to form: {:?}", (if p.is_empty() { None } else { Some(if p.ends_with('/') { p.clone() } else { format!("{}/", p.clone()) }) })); | |
| if p.is_empty() { | |
| None | |
| } else { | |
| Some(if p.ends_with('/') { p } else { format!("{}/", p) }) | |
| } | |
| } | |
| prefix={ | |
| let p = prefix.get(); | |
| if p.is_empty() { | |
| None | |
| } else { | |
| Some(if p.ends_with('/') { p } else { format!("{}/", p) }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In @crates/frontend/src/pages/default_config_create.rs around lines 154 - 163,
Remove the temporary debug call and simplify the inline prefix logic: delete the
logging::log! line, then replace the nested inline conditional with clear steps
using the existing prefix.get() value (p) — if p.is_empty() return None,
otherwise ensure the string ends with a single '/' (append one only if
!p.ends_with('/')) and return Some(the normalized string). Keep references to
the same variable names (prefix, p) so you update the expression used as the
prefix={...} prop.
| navigate(&navigate_to, Default::default()); | ||
| }); | ||
|
|
||
| let breadcrumb_url = StoredValue::new(format!("{}/admin/{}/{}/default-config", base, org.get().0, workspace.get().0)); |
There was a problem hiding this comment.
Breadcrumb URL is not reactive to org/workspace changes.
StoredValue captures org.get().0 and workspace.get().0 at component creation time. If these signals change while on this page, the breadcrumb link will be stale.
🔧 Use a derived signal instead
- let breadcrumb_url = StoredValue::new(format!("{}/admin/{}/{}/default-config", base, org.get().0, workspace.get().0));
+ let breadcrumb_url = Signal::derive(move || {
+ format!("{}/admin/{}/{}/default-config", base, org.get().0, workspace.get().0)
+ });Then in the view, use breadcrumb_url.get() instead of breadcrumb_url.get_value().
🤖 Prompt for AI Agents
In @crates/frontend/src/pages/default_config.rs at line 264, StoredValue
currently captures org.get().0 and workspace.get().0 once, so change
breadcrumb_url to be a derived/reactive signal that recomputes when org or
workspace change (e.g., replace StoredValue::new(...) with a memo/derived
created from org and workspace that formats the same string) and update the view
to call breadcrumb_url.get() (instead of breadcrumb_url.get_value()) so the
breadcrumb link stays up-to-date; reference the breadcrumb_url symbol and the
org and workspace signals when making the change.
Replace StoredValue with Signal::derive so breadcrumb_url recomputes when org or workspace signals change, keeping the breadcrumb link up-to-date.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
crates/frontend/src/pages/default_config_create.rs (3)
29-31: Remove debug logging before merging.The
logging::log!statement is useful for development but should be removed for production code to avoid console noise.🧹 Suggested fix
.unwrap_or_default(); - // Debug: log the parsed prefix - logging::log!("Extracted prefix: '{}'", parsed); parsed
46-105: Consider extracting URL construction to reduce duplication.Both
handle_submitandhandle_cancelconstruct the same navigation URL. Consider extracting a helper to reduce duplication and improve maintainability.♻️ Example refactor
+ // Helper to build navigation URL + let build_list_url = move || { + let current_prefix = prefix.get(); + if current_prefix.is_empty() { + format!( + "{}/admin/{}/{}/default-config", + base, + org.get().0, + workspace.get().0 + ) + } else { + format!( + "{}/admin/{}/{}/default-config?prefix={}", + base, + org.get().0, + workspace.get().0, + current_prefix + ) + } + }; + - // Handle form submission - navigate back to list page - let base_submit = base.clone(); - let nav_submit = navigate.clone(); - let org_submit = org; - let workspace_submit = workspace; let handle_submit = Callback::new(move |_| { enqueue_alert( "Default config key created successfully!".to_string(), AlertType::Success, 5000, ); - - // Navigate back to the list page, preserving current prefix if any - let current_prefix = prefix.get(); - let navigate_to = if current_prefix.is_empty() { - format!( - "{}/admin/{}/{}/default-config", - base_submit, - org_submit.get().0, - workspace_submit.get().0 - ) - } else { - format!( - "{}/admin/{}/{}/default-config?prefix={}", - base_submit, - org_submit.get().0, - workspace_submit.get().0, - current_prefix - ) - }; - - nav_submit(&navigate_to, Default::default()); + navigate(&build_list_url(), Default::default()); }); - // Handle cancel - navigate back to list page - let base_cancel = base.clone(); - let nav_cancel = navigate.clone(); - let org_cancel = org; - let workspace_cancel = workspace; let handle_cancel = Callback::new(move |_| { - let current_prefix = prefix.get(); - let navigate_to = if current_prefix.is_empty() { - format!( - "{}/admin/{}/{}/default-config", - base_cancel, - org_cancel.get().0, - workspace_cancel.get().0 - ) - } else { - format!( - "{}/admin/{}/{}/default-config?prefix={}", - base_cancel, - org_cancel.get().0, - workspace_cancel.get().0, - current_prefix - ) - }; - - nav_cancel(&navigate_to, Default::default()); + navigate(&build_list_url(), Default::default()); });
154-163: Remove debug logging and simplify prefix handling.The debug
logging::log!should be removed. Consider extracting the prefix normalization into a derived signal for clarity and to avoid recomputation.🧹 Suggested improvement
+ // Normalize prefix for form: ensure trailing slash if non-empty + let form_prefix = Signal::derive(move || { + let p = prefix.get(); + if p.is_empty() { + None + } else { + Some(if p.ends_with('/') { p } else { format!("{}/", p) }) + } + }); // In the view: <DefaultConfigForm edit=false - prefix={ - let p = prefix.get(); - // Debug: log what we're passing to the form - logging::log!("Passing prefix to form: {:?}", (if p.is_empty() { None } else { Some(if p.ends_with('/') { p.clone() } else { format!("{}/", p.clone()) }) })); - if p.is_empty() { - None - } else { - Some(if p.ends_with('/') { p } else { format!("{}/", p) }) - } - } + prefix=form_prefix.get() handle_submit=handle_submit />crates/frontend/src/pages/default_config.rs (3)
191-197: Consider usingButtonAnchorfor consistency.The list page uses
ButtonAnchorfor navigation to the create page. Consider usingButtonAnchorhere as well for consistency, rather than manually styling an<A>element.♻️ Suggested change
- <A - href="edit" - class="btn join-item px-5 py-2.5 text-white bg-gradient-to-r from-purple-500 via-purple-600 to-purple-700 shadow-lg rounded-lg" - > - <i class="ri-edit-line mr-2"></i> - "Edit" - </A> + <ButtonAnchor + href="edit" + text="Edit" + icon_class="ri-edit-line" + class="join-item" + />Note: You'll need to add
ButtonAnchorto the imports fromcrate::components::button.
243-250: Fix inconsistent indentation.The
create_blocking_resourceblock has inconsistent indentation compared to the surrounding code.🧹 Suggested fix
-let default_config_resource = create_blocking_resource( + let default_config_resource = create_blocking_resource( move || (default_config_key.get(), workspace.get().0, org.get().0), |(default_config_key, workspace, org_id)| async move { get_default_config(&default_config_key, &workspace, &org_id) .await .ok() }, );
309-311: Consider adding a success alert after edit submission.Unlike
CreateDefaultConfig, this handler doesn't show a success alert after saving. Adding an alert would provide user feedback that the save succeeded, even if staying on the edit page.💡 Suggested enhancement
handle_submit=move |_| { + enqueue_alert( + "Default config updated successfully!".to_string(), + AlertType::Success, + 5000, + ); default_config_resource.refetch(); }Note: You'll need to add
enqueue_alertandAlertTypeto the imports.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/frontend/src/app.rscrates/frontend/src/pages.rscrates/frontend/src/pages/default_config.rscrates/frontend/src/pages/default_config_create.rscrates/frontend/src/pages/default_config_list.rs
🧰 Additional context used
🧬 Code graph analysis (3)
crates/frontend/src/pages/default_config_create.rs (5)
crates/frontend/src/components/button.rs (1)
button(13-61)crates/frontend/src/components/default_config_form.rs (1)
default_config_form(59-527)crates/frontend/src/providers/alert_provider.rs (2)
alert_provider(72-82)enqueue_alert(54-69)crates/frontend/src/query_updater.rs (1)
use_signal_from_query(75-89)crates/frontend/src/utils.rs (1)
use_url_base(52-58)
crates/frontend/src/app.rs (1)
crates/frontend/src/pages/default_config_create.rs (1)
CreateDefaultConfig(13-170)
crates/frontend/src/pages/default_config_list.rs (2)
crates/frontend/src/api.rs (1)
fetch_default_config(40-63)crates/frontend/src/components/button.rs (1)
button(13-61)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Testing
🔇 Additional comments (9)
crates/frontend/src/pages.rs (1)
7-7: LGTM!The new module export is correctly placed in alphabetical order and follows the existing pattern.
crates/frontend/src/pages/default_config_list.rs (2)
17-19: LGTM!The import changes align with the refactor from modal-based to navigation-based creation flow.
fetch_default_configis appropriately used by the blocking resource, andButtonAnchorenables the link-based navigation.
228-233: LGTM!The
ButtonAnchorwith relativehref="-/create"correctly navigates to the new standalone create page. The-/prefix elegantly avoids conflicts with potential config keys named "create".crates/frontend/src/pages/default_config_create.rs (1)
107-147: LGTM!The header layout with breadcrumb navigation and cancel button follows the established patterns for standalone pages in the codebase.
crates/frontend/src/pages/default_config.rs (3)
117-121: LGTM!The
Actionenum is appropriately simplified to onlyNoneandDeletesince editing now uses route-based navigation.
216-224: LGTM!The conditional delete confirmation UI using
Showwith pattern matching is clean and appropriate.
264-264: Consider usingSignal::derivefor reactive breadcrumb URL.
StoredValuecapturesorgandworkspacevalues at creation time. If these signals could change during the component lifecycle, the breadcrumb URL would become stale. UsingSignal::derivewould make this reactive.- let breadcrumb_url = StoredValue::new(format!("{}/admin/{}/{}/default-config", base, org.get().0, workspace.get().0)); + let breadcrumb_url = Signal::derive(move || format!("{}/admin/{}/{}/default-config", base.clone(), org.get().0, workspace.get().0));Then use
breadcrumb_url.get()instead ofbreadcrumb_url.get_value()in the view.crates/frontend/src/app.rs (2)
21-27: LGTM!The imports are properly organized, bringing in
DefaultConfig,EditDefaultConfig,CreateDefaultConfig, andDefaultConfigListfrom their respective modules.
170-185: LGTM!The route definitions are correctly ordered with specific paths before parameterized ones. The
-/createpattern elegantly allows config keys named "create" to exist without route collisions.
| use crate::providers::{alert_provider::enqueue_alert, editor_provider::EditorProvider}; | ||
| use crate::schema::{EnumVariants, JsonSchemaType, SchemaType}; | ||
| use crate::types::{OrganisationId, Workspace}; | ||
| use crate::utils::use_url_base; |
There was a problem hiding this comment.
this should be needed, unless we are doing a manual redirection
| <div class="breadcrumbs text-sm mt-2"> | ||
| <ul> | ||
| <li> | ||
| <a href=breadcrumb_url.get() class="link link-hover"> |
There was a problem hiding this comment.
<A> should be used in all the places it handles a lot of route values internally, here only relative values are neded
Problem
New default-config creation has the modal style form. We want to move to a stand-alone page with its route.
Solution
This page adds a separate page under the route.
/admin/:org/:workspace/default-config/-/createThe
/-/is to allow for keys namedcreate.Environment variable changes
NA
Pre-deployment activity
NA
Post-deployment activity
NA
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.