feat(UI): Standalone page for create and edit default config key #853
Conversation
|
Review changes with Changed Files
|
WalkthroughThis PR reorganizes the frontend API surface by consolidating default-config operations into a dedicated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
🤖 Fix all issues with AI agents
In `@crates/frontend/src/components/default_config_form.rs`:
- Around line 195-196: The call to use_navigate() must be moved out of the async
context: capture the navigate function by calling use_navigate() before entering
the spawn_local async block (e.g., let navigate = use_navigate();), then inside
the spawn_local closure invoke navigate(&redirect_url, Default::default());
ensure you remove any nested use_navigate() calls and use the captured navigate
variable within the async task so the Leptos reactive runtime is accessed from
the correct context.
🧹 Nitpick comments (1)
crates/frontend/src/components/dimension_form.rs (1)
468-475: Unreachable code branch inmonaco_margincalculation.The
monaco_marginvariable checks forDimensionTypeOptions::LocalCohort, but this code is inside theRegular | RemoteCohortmatch arm (line 448-449), so the condition will always be false andmonaco_marginwill always be"mt-3".♻️ Suggested simplification
- let monaco_margin = if matches!( - dimension_type_rs.get(), - DimensionTypeOptions::LocalCohort - ) { - "" - } else { - "mt-3" - }; + let monaco_margin = "mt-3";
| let navigate = use_navigate(); | ||
| navigate(&redirect_url, Default::default()); |
There was a problem hiding this comment.
Move use_navigate() outside the async block.
Calling use_navigate() inside spawn_local may cause issues because Leptos hooks typically require access to the reactive runtime context, which may not be available inside async blocks. Consider capturing the navigate function before entering the async context.
🔧 Proposed fix
Capture navigate before spawn_local:
let on_submit = Callback::new(move |_| {
req_inprogress_ws.set(true);
let key_name = config_key_rs.get_untracked();
// ... other local variables ...
let redirect_url = format!(
"/admin/{}/{}/default-config/{}",
org.0, workspace.0, key_name
);
+ let navigate = use_navigate();
let is_edit = edit;
spawn_local(async move {
// ... async logic ...
match result {
Ok(ResponseType::UpdatePrecheck) => (),
Ok(ResponseType::Response) => {
- let navigate = use_navigate();
navigate(&redirect_url, Default::default());
// ... rest of success handling
}
// ...
}
});
});🤖 Prompt for AI Agents
In `@crates/frontend/src/components/default_config_form.rs` around lines 195 -
196, The call to use_navigate() must be moved out of the async context: capture
the navigate function by calling use_navigate() before entering the spawn_local
async block (e.g., let navigate = use_navigate();), then inside the spawn_local
closure invoke navigate(&redirect_url, Default::default()); ensure you remove
any nested use_navigate() calls and use the captured navigate variable within
the async task so the Leptos reactive runtime is accessed from the correct
context.
ayushjain17
added
enhancement
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn fetch( |
There was a problem hiding this comment.
Minor nit: list or getAll or fetchAll is a better name ?
There was a problem hiding this comment.
fair point, was just following the previous convention which we were following
we can rename them
| workspace.get().0, | ||
| ) | ||
| } | ||
| href="../../../function" |
There was a problem hiding this comment.
this is a little flaky ? will it always be three ../../../ ? or should directly point to function page like it was originally ?
There was a problem hiding this comment.
From create page its always 3 sets of ../ only
| }); | ||
|
|
||
| let cancel_url = format!( | ||
| "../../../default-config?{}", |
| <Route ssr=SsrMode::Async path="dimensions/create" view=CreateDimension /> | ||
| <Route | ||
| ssr=SsrMode::Async | ||
| path="dimensions/action/create" |
There was a problem hiding this comment.
Can we use something else other than action for path segment - kind of clashes with a dimension/default-config/function named action ? just -new- might suffice as a special case.
There was a problem hiding this comment.
It wont be colliding though, right ?
As the next argument in the path has to be create which wont be there
I chose action so that we can extend it further if we will have anything in future
ChangeLog
{resource}/action/createpath for create pages for different resources to allow the resources to have an entity calledcreateSummary by CodeRabbit
New Features
Bug Fixes & Improvements
/action/createand/{entity}/editURL patterns.Refactor
✏️ Tip: You can customize this high-level summary in your review settings.