-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: Info panel data not reloading when clicking refresh button in data browser #3027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
📝 WalkthroughWalkthroughBrowser.refresh() was changed to return a boolean indicating completion (false on user-cancel), and to trigger fetches without awaiting in non-relational paths. DataBrowser adds a bound async handleRefresh, wired to child components, which awaits onRefresh result, clears prefetchCache entries for affected objects, and triggers per-panel/multi-panel re-fetch and cloud-function calls. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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)
1040-1066: Return value doesn't reflect actual completion status.The method returns
trueimmediately after callingfetchDatawithout awaiting it (line 1063). This means the caller receivestruebefore the data refresh actually completes, which is misleading.Impact on DataBrowser.handleRefresh:
DataBrowser.handleRefresh()(line 378 in DataBrowser.react.js) awaits this method and uses the return value to determine whether to refresh panel data- If
trueis returned before data fetch completes, there could be race conditions where panel refresh logic executes while the main data is still loadingConsider one of these approaches:
Option 1: Await fetchData to reflect actual completion (Recommended)
} else { this.setState({ ...initialState, relation: null, }); - this.fetchData(this.props.params.className, prevFilters); + await this.fetchData(this.props.params.className, prevFilters); } return true;Option 2: Document the return semantics
If the current behavior is intentional (returning before completion), add a JSDoc comment:
+ /** + * Initiates a refresh of the browser data. + * @returns {Promise<boolean>} true if refresh was initiated (not completed), false if cancelled by user + */ async refresh() {Note: The relational path (line 1057) has the same issue—
setRelationtriggers asyncfetchRelationinternally but isn't awaited either.
📜 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/DataBrowser.react.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T06:12:17.707Z
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2957
File: src/dashboard/Data/Browser/BrowserTable.react.js:584-597
Timestamp: 2025-07-31T06:12:17.707Z
Learning: In Parse Dashboard's data browser selection logic (src/dashboard/Data/Browser/BrowserTable.react.js), the `selection['*']` pattern is used to handle global operations that pass `{ '*': true }` to indicate all items are selected, particularly for bulk operations like delete. This is not dead code but serves as compatibility layer for global operations that don't go through normal individual row selection workflows.
Applied to files:
src/dashboard/Data/Browser/Browser.react.js
⏰ 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 22
- GitHub Check: Node 18
- GitHub Check: Docker linux/amd64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/dashboard/Data/Browser/Browser.react.js (1)
1041-1076: Well-structured async refactor with proper error handling.The implementation correctly:
- Invokes the callback before resetting state, allowing the caller to set loading indicators
- Uses try/catch to handle errors gracefully
- Returns a boolean to indicate success/failure for caller coordination
One minor consideration: The empty catch block at line 1073 swallows the error silently. While the caller handles failures via the return value, logging the error here could aid debugging.
- } catch { + } catch (error) { + console.error('Refresh failed:', error); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/dashboard/Data/Browser/Browser.react.js(4 hunks)src/dashboard/Data/Browser/DataBrowser.react.js(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-31T06:12:17.707Z
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2957
File: src/dashboard/Data/Browser/BrowserTable.react.js:584-597
Timestamp: 2025-07-31T06:12:17.707Z
Learning: In Parse Dashboard's data browser selection logic (src/dashboard/Data/Browser/BrowserTable.react.js), the `selection['*']` pattern is used to handle global operations that pass `{ '*': true }` to indicate all items are selected, particularly for bulk operations like delete. This is not dead code but serves as compatibility layer for global operations that don't go through normal individual row selection workflows.
Applied to files:
src/dashboard/Data/Browser/Browser.react.js
📚 Learning: 2025-05-27T12:09:47.644Z
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the `confirmExecuteScriptRows` method in `src/dashboard/Data/Browser/Browser.react.js`), individual `setState` calls to update `processedScripts` counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
Applied to files:
src/dashboard/Data/Browser/DataBrowser.react.js
⏰ 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). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (9)
src/dashboard/Data/Browser/Browser.react.js (2)
414-414: LGTM: Returning the promise enables proper async coordination.This change allows callers to await or chain the cloud function call, which is essential for the new refresh workflow.
1522-1522: LGTM: Returning the promise enables awaiting relation setup.This aligns with the async refresh workflow, allowing the caller to wait for the relation data to be fully fetched.
src/dashboard/Data/Browser/DataBrowser.react.js (7)
146-146: LGTM: Constructor binding for handleRefresh.
377-424: Improved implementation with batched state updates.The refactored approach correctly:
- Batches all cache deletions into a single
setStatecall (addressing the previous concern about multiple re-renders)- Uses the promisified
setStatepattern to ensure cache is cleared before fetches begin- Collects all fetch promises and awaits them with
Promise.allNote:
fetchDataForMultiPanelswallows its errors internally (line 1126-1132), so individual panel fetch failures won't causePromise.allto reject. This is likely intentional to prevent one panel's failure from blocking others. However,handleCallCloudFunctioncan still reject, which will be caught byhandleRefresh's try/catch.
426-439: Clean helper method for setting loading states.The method appropriately sets loading indicators for both the main panel and all currently displayed multi-panel objects.
441-464: Comprehensive error handling addresses previous review feedback.This implementation properly:
- Wraps the entire refresh flow in try/catch
- Passes a loading callback to ensure UI feedback before data reset
- Resets loading states on both explicit failure (
success === false) and caught exceptions- Logs errors and optionally displays user notification
This addresses the concerns raised in the previous review about error handling.
1086-1134: LGTM: Consistent promise returns enable async coordination.All code paths now return promises, which allows
refreshPanels()to properly await all panel data fetches withPromise.all.
1401-1411: LGTM: Promise returns enable proper refresh coordination.Both the cached and non-cached paths now return promises, allowing the refresh workflow to properly await data loading.
1692-1692: LGTM: Correctly wires handleRefresh to toolbar.The refresh button in the toolbar will now trigger the coordinated refresh flow that handles both data and panel refresh.
# [8.1.0-alpha.7](8.1.0-alpha.6...8.1.0-alpha.7) (2025-11-28) ### Bug Fixes * Info panel data not reloading when clicking refresh button in data browser ([#3027](#3027)) ([8f91d15](8f91d15))
|
🎉 This change has been released in version 8.1.0-alpha.7 |
New Pull Request Checklist
Issue Description
Info panel data not reloading when clicking refresh button in data browser.
Approach
Reload info panel data on data browser reload.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.