feat: added existing edit modal to create scene#435
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBackend scenario creation and updates now carry validated scenario details, while the frontend uses a shared modal editor for creating and editing scenarios. ChangesScenario details create and edit flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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)
frontend/src/features/scenarioInfo/components/DetailEditModal.jsx (1)
3-8: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a name required-check before allowing submission.
The submit button always calls
onSaveregardless of field contents. Now that this modal drives scenario creation (scenario={null}), an emptynamecan be submitted, which will likely fail on the backend (Scenario'snameis presumably a required field) with a generic error toast rather than clear inline feedback.💡 Proposed fix: disable submit when name is blank
<button - onClick={() => onSave({ name, description, estimatedTime })} + disabled={!name.trim()} + onClick={() => onSave({ name, description, estimatedTime })} className={`btn btn-ghost text-base-content border border-base-content/20 hover:bg-base-content/10 hover:border-base-content/40 font-dm`} >Also applies to: 88-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/scenarioInfo/components/DetailEditModal.jsx` around lines 3 - 8, The DetailEditModal submit flow currently allows calling onSave even when the scenario name is empty, which can create invalid submissions when scenario is null. Update DetailEditModal’s submit handling and the submit button state so that a blank name is blocked before onSave runs, using the existing name state in DetailEditModal and any related submit handler/prop wiring. Add a required-check tied to name so the button is disabled or submission is prevented until a non-empty name is entered, while keeping the rest of the modal behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/features/authoring/AuthoringToolPage.jsx`:
- Around line 189-202: The Edit Scenario flow in AuthoringToolPage currently
closes the ModalDialog without handling failures from updateScenarioDetails, and
DetailEditModal’s Save action is also triggering native form submit behavior via
method="dialog". Update the onSave path around updateScenarioDetails to await
the async save and surface any rejection with user-visible error handling, and
adjust the DetailEditModal save button behavior so it does not implicitly submit
the dialog form. Keep the modal open on failure and only close it after a
successful save.
---
Nitpick comments:
In `@frontend/src/features/scenarioInfo/components/DetailEditModal.jsx`:
- Around line 3-8: The DetailEditModal submit flow currently allows calling
onSave even when the scenario name is empty, which can create invalid
submissions when scenario is null. Update DetailEditModal’s submit handling and
the submit button state so that a blank name is blocked before onSave runs,
using the existing name state in DetailEditModal and any related submit
handler/prop wiring. Add a required-check tied to name so the button is disabled
or submission is prevented until a non-empty name is entered, while keeping the
rest of the modal behavior unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eecad49a-431a-4ec8-8555-eba2582fea31
📒 Files selected for processing (7)
backend/src/db/daos/scenarioDao.jsbackend/src/routes/api/scenario.jsfrontend/src/components/CreateScenarioCard/CreateScenarioCard.jsxfrontend/src/context/ScenarioContextProvider.jsxfrontend/src/features/authoring/AuthoringToolPage.jsxfrontend/src/features/create/CreateLandingPage.jsxfrontend/src/features/scenarioInfo/components/DetailEditModal.jsx
💤 Files with no reviewable changes (1)
- frontend/src/components/CreateScenarioCard/CreateScenarioCard.jsx
harbassan
left a comment
harbassan
left a comment
There was a problem hiding this comment.
nice, looking good. in regards to coderabbit review, rebase your branch to get the share modal changes from master, which you can base the loading state of the modal and owner check for the topbar header on.
d2ba0e6 to
b8bc4bf
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/features/scenarioInfo/components/DetailEditModal.jsx (2)
22-32: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo client-side guard against an empty scenario name.
Save/Create is only disabled while
isSubmitting; there's no check thatname.trim()is non-empty before callingonSave. This is more consequential now that the modal also drives scenario creation — a blank submit would create/save a nameless scenario shown in listings. Consider disabling the action button (or short-circuitinghandleSave) whenname.trim()is empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/scenarioInfo/components/DetailEditModal.jsx` around lines 22 - 32, The DetailEditModal handleSave flow allows saving with an empty scenario name, so add a client-side guard using name.trim() before calling onSave. Update the action button state and/or short-circuit handleSave so submission stays blocked until a non-empty name is entered, keeping the existing isSubmitting behavior intact.
22-32: 🗄️ Data Integrity & Integration | 🔵 TrivialConsider optimistic-concurrency safeguards for metadata edits.
This save path is now reachable from three surfaces (create, authoring, scenario info), increasing the chance two users/tabs edit the same scenario's metadata concurrently. The PR description itself flags this as a known risk. Worth considering a version/
updatedAtcheck (or a "last edited by X" indicator) on the backend update path to avoid silent last-write-wins overwrites, as a follow-up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/scenarioInfo/components/DetailEditModal.jsx` around lines 22 - 32, The metadata save flow in DetailEditModal’s handleSave currently does a blind onSave update, which can silently overwrite concurrent edits from other users/tabs. Update the backend update path used by onSave to include an optimistic-concurrency guard such as a version or updatedAt check, and make the client pass whatever revision metadata is available from the scenario record. If the check fails, surface a clear conflict state instead of closing the modal, and keep the DetailEditModal flow aligned with onSave/onClose so users can retry after reloading.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/features/scenarioInfo/components/DetailEditModal.jsx`:
- Around line 22-32: The DetailEditModal handleSave flow allows saving with an
empty scenario name, so add a client-side guard using name.trim() before calling
onSave. Update the action button state and/or short-circuit handleSave so
submission stays blocked until a non-empty name is entered, keeping the existing
isSubmitting behavior intact.
- Around line 22-32: The metadata save flow in DetailEditModal’s handleSave
currently does a blind onSave update, which can silently overwrite concurrent
edits from other users/tabs. Update the backend update path used by onSave to
include an optimistic-concurrency guard such as a version or updatedAt check,
and make the client pass whatever revision metadata is available from the
scenario record. If the check fails, surface a clear conflict state instead of
closing the modal, and keep the DetailEditModal flow aligned with onSave/onClose
so users can retry after reloading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 198092ab-d678-4e77-bdf4-2b597289c7dc
📒 Files selected for processing (8)
backend/src/db/daos/scenarioDao.jsbackend/src/routes/api/scenario.jsfrontend/src/components/CreateScenarioCard/CreateScenarioCard.jsxfrontend/src/context/ScenarioContextProvider.jsxfrontend/src/features/authoring/AuthoringToolPage.jsxfrontend/src/features/create/CreateLandingPage.jsxfrontend/src/features/scenarioInfo/ScenarioInfo.jsxfrontend/src/features/scenarioInfo/components/DetailEditModal.jsx
💤 Files with no reviewable changes (1)
- frontend/src/components/CreateScenarioCard/CreateScenarioCard.jsx
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/src/routes/api/scenario.js
- backend/src/db/daos/scenarioDao.js
- frontend/src/context/ScenarioContextProvider.jsx
- frontend/src/features/authoring/AuthoringToolPage.jsx
I did rebase and it should be fixed now |
harbassan
left a comment
harbassan
left a comment
There was a problem hiding this comment.
nice, all seems to work perfectly 😄
leowla
left a comment
leowla
left a comment
There was a problem hiding this comment.
It should not be possible for a user to rename the scenario to nothing
yea that should be fixed now |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/routes/api/scenario.js`:
- Around line 71-72: Update both scenario route handlers in
backend/src/routes/api/scenario.js: at lines 71-72, make the POST name
validation require a string before trimming; at lines 114-115, make the PATCH
validation allow undefined but require any provided name to be a string before
trimming. Preserve the existing 400 Bad Request behavior for invalid or blank
names.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea4f23be-d9db-4f08-874c-bfabfb001adf
📒 Files selected for processing (2)
backend/src/routes/api/scenario.jsfrontend/src/features/scenarioInfo/components/DetailEditModal.jsx
3 participants
Issue
Currently the scenario metadata can be edited from the "edit scenario" button on the scenario info page which is accessible from the play page. This is confusing, given that an author would expect to be able to edit that information from the "authoring" section of the app, which is the create page and editor.
Solution
Use the existing modal to used on scene creation and inside scene editor.
was also thinking maybe I should delete the details editing button from the play page, thoughts ?
Risk
Could break on multi user scenario metadata editing. Not sure about this thou, just a thought.
Checklist
Summary by CodeRabbit