fix: Add Cancel and Add buttons to the add-row modal (#1995)#3383
fix: Add Cancel and Add buttons to the add-row modal (#1995)#3383dblythy wants to merge 1 commit into
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughThis change adds Cancel and Add button behavior to the "Add row with modal" dialog. Browser.react.js closes the edit dialog on successful save and passes new onSaveNewRow/onAbortAddRow callbacks to EditRowDialog, which now conditionally renders Cancel/Add controls based on whether a new object is being created. ChangesNew row modal dialog controls
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant EditRowDialog
participant Browser
User->>EditRowDialog: Click "Add"
EditRowDialog->>Browser: onSaveNewRow()
Browser->>Browser: saveNewRow (update data, close dialog)
User->>EditRowDialog: Click "Cancel"
EditRowDialog->>EditRowDialog: handleCancel()
EditRowDialog->>Browser: onAbortAddRow()
EditRowDialog->>EditRowDialog: onClose()
Related issues: Suggested labels: enhancement, UI/UX Suggested reviewers: dblythy, mtrezza Poem: A rabbit hops to close the row, 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
dblythy
force-pushed
the
fix/1995-add-row-modal-buttons
branch
from
71be7c5 to
7558830
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dashboard/Data/Browser/Browser.react.js (1)
1000-1043: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale
selectionstate after successful "Add" leaves incorrect "selected rows" UI state.
addRowWithModal()setsselection = { undefined: true }when opening the modal. Previously, the dialog stayed open after save, so the (unchanged)isJustSavedObjectcleanup block inrenderContent()(this.setState({ selection: {} }); this.selectRow(selectedId, true);) ran on the next render and fixed the selection.Now that
saveNewRowalso setsshowEditRowDialog: falsein the same state update that nullsnewObject, theelse if (this.state.showEditRowDialog)branch inrenderContent()is skipped once save succeeds, so that cleanup code never runs for the modal-add flow anymore. The stale{ undefined: true }selection persists indefinitely, incorrectly drivinghasSelectedRows(BrowserFooter) andSelectedRowsNavigationPrompt(selectionCount > 0) after every successful "Add".Also (lower confidence): in the relation branch,
showEditRowDialog: falseis applied synchronously right afterobj.save()resolves, before the relation-linkparent.save()completes — the modal closes before the link is actually confirmed.🐛 Proposed fix to reset selection on close
- const state = { data: this.state.data, showEditRowDialog: false }; + const state = { data: this.state.data, showEditRowDialog: false, selection: {} }; const relation = this.state.relation; if (relation) { const parent = relation.parent; const parentRelation = parent.relation(relation.key); parentRelation.add(obj); const targetClassName = relation.targetClassName; parent.save(null, { useMasterKey }).then( () => { this.setState({ newObject: null, data: [obj, ...this.state.data], + selection: { [obj.id]: true }, relationCount: this.state.relationCount + 1, counts: { ...this.state.counts, [targetClassName]: this.state.counts[targetClassName] + 1, }, }); }, error => { ... } ); } else { state.newObject = null; if (this.props.params.className === obj.className) { this.state.data.unshift(obj); + state.selection = { [obj.id]: true }; } this.state.counts[obj.className] += 1; } this.setState(state);🤖 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 `@src/dashboard/Data/Browser/Browser.react.js` around lines 1000 - 1043, The Add modal flow in Browser.react.js now closes before the old selection cleanup in renderContent() can run, so the stale selection from addRowWithModal() is never cleared after a successful save. Update the save success path in saveNewRow()/the obj.save() handler to explicitly reset selection when closing the dialog (and do the same in the relation branch once the parent.save() completes), using the existing state/update logic around showEditRowDialog, selection, and selectRow so BrowserFooter and SelectedRowsNavigationPrompt no longer see a phantom selected row.
🧹 Nitpick comments (1)
src/dashboard/Data/Browser/EditRowDialog.react.js (1)
227-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding test coverage for the new Cancel/Add modal behavior.
This introduces new interactive states (
isNewObject, conditional Cancel/Close text, conditional Add button) with no accompanying test changes in the provided files. Given the stale-selection regression found inBrowser.react.js'ssaveNewRow(see comment there), a test exercising "add row via modal → Add" would have caught it.Also applies to: 515-521
🤖 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 `@src/dashboard/Data/Browser/EditRowDialog.react.js` around lines 227 - 245, Add test coverage for the new modal state handling in EditRowDialog.react.js, especially the isNewObject-driven Cancel/Add behavior and handleCancel flow. Update or add tests around EditRowDialog and the add-row path in Browser.react.js’s saveNewRow to exercise “add row via modal → Add” and confirm the selected object/row state stays in sync. Make sure the test checks the conditional button text and that Cancel calls onAbortAddRow only for new rows while still closing the dialog.
🤖 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.
Outside diff comments:
In `@src/dashboard/Data/Browser/Browser.react.js`:
- Around line 1000-1043: The Add modal flow in Browser.react.js now closes
before the old selection cleanup in renderContent() can run, so the stale
selection from addRowWithModal() is never cleared after a successful save.
Update the save success path in saveNewRow()/the obj.save() handler to
explicitly reset selection when closing the dialog (and do the same in the
relation branch once the parent.save() completes), using the existing
state/update logic around showEditRowDialog, selection, and selectRow so
BrowserFooter and SelectedRowsNavigationPrompt no longer see a phantom selected
row.
---
Nitpick comments:
In `@src/dashboard/Data/Browser/EditRowDialog.react.js`:
- Around line 227-245: Add test coverage for the new modal state handling in
EditRowDialog.react.js, especially the isNewObject-driven Cancel/Add behavior
and handleCancel flow. Update or add tests around EditRowDialog and the add-row
path in Browser.react.js’s saveNewRow to exercise “add row via modal → Add” and
confirm the selected object/row state stays in sync. Make sure the test checks
the conditional button text and that Cancel calls onAbortAddRow only for new
rows while still closing the dialog.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08c6b35f-71a4-4848-8c98-76bcafc1306b
📒 Files selected for processing (2)
src/dashboard/Data/Browser/Browser.react.jssrc/dashboard/Data/Browser/EditRowDialog.react.js
1 participant
Closes #1995
Before
After
Summary by CodeRabbit
New Features
Bug Fixes