fix: Missing alert when changing data browser table data while rows are selected #2994
Conversation
|
🚀 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. |
📝 WalkthroughWalkthroughAdds confirmation guards when rows are selected: updateFilters in Browser prompts and may abort; Browser passes hasSelectedRows and selectedRowsMessage props to BrowserFooter; BrowserFooter adds similar guards for limit and page changes, early-returning on user cancel. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant B as Browser.react
participant D as Confirm Dialog
note over B: updateFilters
U->>B: Change filter
alt Rows selected
B->>D: Prompt with SELECTED_ROWS_MESSAGE
D-->>B: Cancel
B-->>U: Abort filter change
else No rows selected or confirmed
B-->>U: Apply new filters
end
sequenceDiagram
autonumber
actor U as User
participant F as BrowserFooter.react
participant B as Browser.react
participant D as Confirm Dialog
rect rgba(230,240,255,0.4)
note right of F: handleLimitChange / handlePageChange
U->>F: Change limit or page
alt hasSelectedRows
F->>D: Prompt with selectedRowsMessage
D-->>F: Cancel
F-->>U: Abort change
else Not selected or confirmed
F->>B: Update limit/skip and page input
B-->>F: Props/state updated
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
mtrezza
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/BrowserFooter.react.js (1)
21-30: Potential state synchronization issue with page input.When
validateAndApplyPage(line 50) callshandlePageChange, it setspageInputstate before the confirmation prompt. If the user has selected rows and cancels the confirmation inhandlePageChange, the input field will display the new page number while the data still shows the old page.The flow is:
- User types a page number and presses Enter
validateAndApplyPagesetspageInputstate to the new value (line 60)validateAndApplyPagecallshandlePageChange(line 61)- User cancels the confirmation
handlePageChangereturns early (line 36)- Result:
pageInputshows the new page, butskipremains unchangedApply this diff to fix the synchronization:
validateAndApplyPage = () => { const { limit, count } = this.props; let newPage = parseInt(this.state.pageInput, 10); if (isNaN(newPage) || newPage < 1) { newPage = 1; } else if (newPage > Math.ceil(count / limit)) { newPage = Math.ceil(count / limit); } - this.setState({ pageInput: newPage.toString() }); - this.handlePageChange((newPage - 1) * limit); + const newSkip = (newPage - 1) * limit; + if (newSkip >= 0 && newSkip < count) { + if (this.props.hasSelectedRows && !window.confirm(this.props.selectedRowsMessage)) { + // Reset input to current page on cancel + this.setState({ pageInput: (Math.floor(this.props.skip / limit) + 1).toString() }); + return; + } + this.props.setSkip(newSkip); + this.setState({ pageInput: newPage.toString() }); + } + + const table = document.getElementById('browser-table'); + table.scrollTo({ top: 0 }); };Alternatively, move the
pageInputstate update to after the confirmation succeeds inhandlePageChange.
🧹 Nitpick comments (2)
src/dashboard/Data/Browser/BrowserFooter.react.js (1)
32-44: Consider extracting duplicate scroll logic.The table scroll behavior (lines 42-43) occurs after the conditional skip update. If the bounds check fails (
newSkip < 0ornewSkip >= count), the table still scrolls to top even though no page change occurred.While this is existing behavior, consider moving the scroll logic inside the conditional block to ensure it only executes when the page actually changes:
handlePageChange = newSkip => { if (newSkip >= 0 && newSkip < this.props.count) { // Check if there are selected rows if (this.props.hasSelectedRows && !window.confirm(this.props.selectedRowsMessage)) { return; } this.props.setSkip(newSkip); this.setState({ pageInput: (Math.floor(newSkip / this.props.limit) + 1).toString() }); + + const table = document.getElementById('browser-table'); + table.scrollTo({ top: 0 }); } - - const table = document.getElementById('browser-table'); - table.scrollTo({ top: 0 }); };src/dashboard/Data/Browser/Browser.react.js (1)
1454-1471: Consider adding confirmation when changing sort order.The
updateOrderingmethod clears the selection (line 1459) without prompting the user, which differs from the pattern established inupdateFilters,handleLimitChange, andhandlePageChange. Users might expect a warning since their selection is being discarded.However, this may be intentional since re-sorting is a view operation within the current page rather than navigation. If you decide consistency is preferred, apply this diff:
updateOrdering(ordering) { + if (Object.keys(this.state.selection).length > 0) { + if (!window.confirm(SELECTED_ROWS_MESSAGE)) { + return; + } + } const source = this.state.relation || this.props.params.className; this.setState( { ordering: ordering, selection: {}, errorAggregatedData: {}, isLoadingInfoPanel: false, AggregationPanelData: {}, }, () => this.fetchData(source, this.state.filters) ); ColumnPreferences.getColumnSort( ordering, this.context.applicationId, this.props.params.className ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/dashboard/Data/Browser/Browser.react.js(2 hunks)src/dashboard/Data/Browser/BrowserFooter.react.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Node 18
- GitHub Check: Node 20
- GitHub Check: Docker linux/amd64
🔇 Additional comments (3)
src/dashboard/Data/Browser/Browser.react.js (3)
49-118: LGTM! Well-implemented navigation guard.The
SelectedRowsNavigationPromptcomponent correctly handles multiple navigation scenarios:
- Browser navigation (beforeunload, popstate)
- Link clicks with proper filtering (ignoring modified clicks, external links, etc.)
- Integration with React Router's
useBeforeUnloadThe implementation prevents unintended navigation when rows are selected.
1248-1291: LGTM! Confirmation guard correctly prevents filter loss.The guard in
updateFiltersproperly checks for selected rows and prompts for confirmation before applying new filters, which would clear the selection. The early return on cancel prevents the filter update.
2526-2540: Props correctly passed to BrowserFooter.The
hasSelectedRowsboolean andselectedRowsMessagestring are computed and passed appropriately, enabling the footer to enforce the selected-rows guard.
mtrezza
changed the title
# [7.6.0-alpha.4](7.6.0-alpha.3...7.6.0-alpha.4) (2025-10-05) ### Bug Fixes * Missing alert when changing data browser browser data while rows are selected ([#2994](#2994)) ([6cabaa3](6cabaa3))
|
🎉 This change has been released in version 7.6.0-alpha.4 |
New Pull Request Checklist
Issue Description
When rows are selected while trying to changing the data browser table data, the alert is not displayed in the following cases:
Approach
Show alert in above cases.
Summary by CodeRabbit