feat: Keep cell selected by objectId and field name after data browser refresh (#2631)#3252
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. 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. |
✅ 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. |
evolvomind
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemembers a selected data browser cell by {objectId, fieldName} across refreshes, restores selection when refreshed data contains that object/field, exposes Changes
Sequence DiagramsequenceDiagram
participant User
participant DataBrowser
participant DataLayer
participant BrowserCell
User->>DataBrowser: Click Refresh
Note over DataBrowser: data -> null (className unchanged)
DataBrowser->>DataBrowser: Save selected {objectId, fieldName} as pendingRestore
DataBrowser->>DataLayer: Fetch refreshed data
DataLayer-->>DataBrowser: Return new data
alt pendingRestore exists
DataBrowser->>DataBrowser: Find objectId in new data and fieldName in columns
DataBrowser->>BrowserCell: Mark located cell as selected (objectId+fieldName)
BrowserCell->>BrowserCell: Render with data-current-cell, data-object-id, data-field
DataBrowser->>DataBrowser: Clear pendingRestore
else not found
DataBrowser->>DataBrowser: Clear selection and pendingRestore
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
🧹 Nitpick comments (1)
src/lib/tests/DataBrowser.pendingRestore.test.js (1)
63-118: Avoid duplicating the production decision tree in the mock.
runPendingRestoreLogic()andwouldAutoLoadFirstRowFire()are a second implementation of the production branches, so the suite can still pass whenDataBrowser.componentDidUpdate()is ordered differently. Please extract this logic into a shared helper or exercise the real component directly, and include the “class changed whileprevProps.datawas null and new data arrived” regression case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tests/DataBrowser.pendingRestore.test.js` around lines 63 - 118, The test duplicates production update logic in runPendingRestoreLogic and wouldAutoLoadFirstRowFire; remove those copies and either (a) call the real DataBrowser.componentDidUpdate via mounting/updating the component in tests or (b) extract the shared logic into a single helper used by both the component and tests (e.g., move pending/restore and auto-load logic into a helper function referenced from DataBrowser.componentDidUpdate). Also add the missing regression test case where prevProps.data is null, the className changes, and new data arrives (the "class changed while prevProps.data was null and new data arrived" scenario) to assert pendingRestore is discarded and selection behavior matches the component. Reference runPendingRestoreLogic, wouldAutoLoadFirstRowFire, and DataBrowser.componentDidUpdate when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@changelogs/CHANGELOG_alpha.md`:
- Line 3: The heading level under the "# [Unreleased]" section is incorrect:
change the "### Features" heading to a second-level heading so it doesn't jump
levels (replace "### Features" with "## Features") to satisfy markdownlint MD001
and keep the hierarchy under "# [Unreleased]" consistent.
In `@src/dashboard/Data/Browser/DataBrowser.react.js`:
- Around line 526-549: The pendingRestore branch can run before the class-change
cleanup, causing a restore into the wrong class and leaving selectedObjectId
stale on failed restores; reorder/guard the logic so the className change check
runs first (or include a check that this.props.className === prevProps.className
before performing the pendingRestore branch) and when the restore misses
(newRowIndex === -1 || newColIndex === -1) also clear selection by calling
setSelectedObjectId(null) in addition to resetting current and pendingRestore;
modify handling around pendingRestore, setCurrent, setSelectedObjectId, and
handleCallCloudFunction accordingly.
---
Nitpick comments:
In `@src/lib/tests/DataBrowser.pendingRestore.test.js`:
- Around line 63-118: The test duplicates production update logic in
runPendingRestoreLogic and wouldAutoLoadFirstRowFire; remove those copies and
either (a) call the real DataBrowser.componentDidUpdate via mounting/updating
the component in tests or (b) extract the shared logic into a single helper used
by both the component and tests (e.g., move pending/restore and auto-load logic
into a helper function referenced from DataBrowser.componentDidUpdate). Also add
the missing regression test case where prevProps.data is null, the className
changes, and new data arrives (the "class changed while prevProps.data was null
and new data arrived" scenario) to assert pendingRestore is discarded and
selection behavior matches the component. Reference runPendingRestoreLogic,
wouldAutoLoadFirstRowFire, and DataBrowser.componentDidUpdate when making the
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fa17dc1-a95d-4713-955c-5963ae655cc1
📒 Files selected for processing (3)
changelogs/CHANGELOG_alpha.mdsrc/dashboard/Data/Browser/DataBrowser.react.jssrc/lib/tests/DataBrowser.pendingRestore.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/tests/DataBrowser.pendingRestore.test.js`:
- Around line 12-120: The tests currently instantiate MockDataBrowser and call
runPendingRestoreLogic/wouldAutoLoadFirstRowFire, so they exercise only the mock
instead of the real component and won't catch regressions in
DataBrowser.componentDidUpdate; replace the mock-driven assertions by mounting
the actual DataBrowser and invoking its lifecycle (drive componentDidUpdate via
changing props/state) or refactor the duplicated predicates into shared helper
functions exported from a new module used by both DataBrowser.react.js and
tests; locate the mock class (MockDataBrowser) and replace usages with a mounted
DataBrowser instance or import the new helper and update tests to call the real
predicate functions (or call componentDidUpdate indirectly by updating props to
trigger the same code paths).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 920f931a-24eb-474b-b170-943af8572046
📒 Files selected for processing (3)
changelogs/CHANGELOG_alpha.mdsrc/dashboard/Data/Browser/DataBrowser.react.jssrc/lib/tests/DataBrowser.pendingRestore.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/dashboard/Data/Browser/DataBrowser.react.js
- changelogs/CHANGELOG_alpha.md
| class MockDataBrowser { | ||
| constructor(props = {}, state = {}) { | ||
| this.props = { | ||
| data: null, | ||
| className: 'TestClass', | ||
| app: { applicationId: 'testApp' }, | ||
| ...props, | ||
| }; | ||
|
|
||
| this.state = { | ||
| current: null, | ||
| order: [ | ||
| { name: 'objectId' }, | ||
| { name: 'username' }, | ||
| { name: 'email' }, | ||
| ], | ||
| pendingRestore: null, | ||
| selectedObjectId: undefined, | ||
| autoLoadFirstRow: false, | ||
| isPanelVisible: false, | ||
| lastSelectedCol: 0, | ||
| ...state, | ||
| }; | ||
|
|
||
| this._setCurrentCalls = []; | ||
| this._setSelectedObjectIdCalls = []; | ||
| this._handleCallCloudFunctionCalls = []; | ||
| this._setStateCalls = []; | ||
| } | ||
|
|
||
| setCurrent(current) { | ||
| if (JSON.stringify(this.state.current) !== JSON.stringify(current)) { | ||
| this._setCurrentCalls.push(current); | ||
| this.state.current = current; | ||
| } | ||
| } | ||
|
|
||
| setSelectedObjectId(objectId) { | ||
| this._setSelectedObjectIdCalls.push(objectId); | ||
| this.state.selectedObjectId = objectId; | ||
| } | ||
|
|
||
| handleCallCloudFunction(objectId, className, appId) { | ||
| this._handleCallCloudFunctionCalls.push({ objectId, className, appId }); | ||
| } | ||
|
|
||
| setState(partial) { | ||
| this._setStateCalls.push(partial); | ||
| Object.assign(this.state, partial); | ||
| } | ||
|
|
||
| runPendingRestoreLogic(prevProps) { | ||
| // Block A - save selection when refresh clears data | ||
| if ( | ||
| this.props.data === null && | ||
| prevProps.data !== null && | ||
| this.props.className === prevProps.className | ||
| ) { | ||
| const { current, order } = this.state; | ||
| if (current !== null) { | ||
| const objectId = prevProps.data[current.row]?.id; | ||
| const fieldName = order[current.col]?.name; | ||
| if (objectId && fieldName) { | ||
| this.setState({ pendingRestore: { objectId, fieldName } }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Block C takes priority: if the class changed while a restore was pending, | ||
| // discard it to avoid firing against a different class's data. | ||
| // Block B: otherwise, when a refresh delivers new data, restore the selection. | ||
| // If the document is no longer present, deselect and clear selectedObjectId. | ||
| if (this.props.className !== prevProps.className && this.state.pendingRestore) { | ||
| this.setState({ pendingRestore: null }); | ||
| } else if ( | ||
| this.props.data !== null && | ||
| prevProps.data === null && | ||
| this.state.pendingRestore | ||
| ) { | ||
| const { objectId, fieldName } = this.state.pendingRestore; | ||
| const newRowIndex = this.props.data.findIndex(obj => obj.id === objectId); | ||
| const newColIndex = this.state.order.findIndex(col => col.name === fieldName); | ||
|
|
||
| if (newRowIndex !== -1 && newColIndex !== -1) { | ||
| this.setCurrent({ row: newRowIndex, col: newColIndex }); | ||
| this.setState({ pendingRestore: null }); | ||
| this.setSelectedObjectId(objectId); | ||
| this.handleCallCloudFunction(objectId, this.props.className, this.props.app.applicationId); | ||
| } else { | ||
| this.setState({ current: null, pendingRestore: null }); | ||
| this.setSelectedObjectId(undefined); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| wouldAutoLoadFirstRowFire(prevProps, prevState) { | ||
| return !!( | ||
| this.state.autoLoadFirstRow && | ||
| this.state.isPanelVisible && | ||
| this.props.data && | ||
| this.props.data.length > 0 && | ||
| !this.state.selectedObjectId && | ||
| !this.state.pendingRestore && | ||
| ((!prevProps.data || prevProps.data.length === 0) || | ||
| prevProps.className !== this.props.className || | ||
| prevState.isPanelVisible !== this.state.isPanelVisible) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Test the real DataBrowser path instead of a copied clone.
Lines 63-119 and 107-119 duplicate the pending-restore and auto-load branches from src/dashboard/Data/Browser/DataBrowser.react.js, and every assertion below only exercises that mock. This suite can stay green while the shipped component regresses. Please either drive componentDidUpdate on the real DataBrowser instance or extract these predicates into shared helpers that both the component and the tests import.
Also applies to: 126-371
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tests/DataBrowser.pendingRestore.test.js` around lines 12 - 120, The
tests currently instantiate MockDataBrowser and call
runPendingRestoreLogic/wouldAutoLoadFirstRowFire, so they exercise only the mock
instead of the real component and won't catch regressions in
DataBrowser.componentDidUpdate; replace the mock-driven assertions by mounting
the actual DataBrowser and invoking its lifecycle (drive componentDidUpdate via
changing props/state) or refactor the duplicated predicates into shared helper
functions exported from a new module used by both DataBrowser.react.js and
tests; locate the mock class (MockDataBrowser) and replace usages with a mounted
DataBrowser instance or import the new helper and update tests to call the real
predicate functions (or call componentDidUpdate indirectly by updating props to
trigger the same code paths).
| * the root directory of this source tree. | ||
| */ | ||
|
|
||
| class MockDataBrowser { |
There was a problem hiding this comment.
@coderabbitai is it really sensible to mock this entire module? Would it be better to use puppeteer (already a dev dependency) to test the feature?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how puppeteer is used and if there are existing E2E/integration tests
echo "=== package.json puppeteer ==="
rg -n "puppeteer" package.json
echo "=== Existing puppeteer test files ==="
fd -e js -e ts "puppeteer|e2e|integration" --type f
echo "=== Browser control README ==="
fd "README.md" --type f | xargs grep -l -i "browser.control" 2>/dev/null
echo "=== Test scripts in package.json ==="
python3 -c "
import json
with open('package.json') as f:
pkg = json.load(f)
scripts = pkg.get('scripts', {})
for k, v in scripts.items():
if any(x in v.lower() for x in ['test', 'jest', 'puppeteer', 'e2e']):
print(f'{k}: {v}')
"
echo "=== Existing test files structure ==="
fd --type f -e test.js -e spec.js -e test.ts | head -30
echo "=== browser-control directory ==="
fd --type f "browser-control" | head -20
ls Parse-Dashboard/browser-control/ 2>/dev/null || echo "dir not found at that path"
fd -t d "browser-control" | head -5
Length of output: 1436
🏁 Script executed:
#!/bin/bash
echo "=== E2E test file ==="
cat src/lib/tests/e2e/dashboard.e2e.test.js
echo "=== Browser Control README ==="
cat Parse-Dashboard/browser-control/README.md
echo "=== Mock test file (first 100 lines) ==="
head -100 src/lib/tests/DataBrowser.pendingRestore.test.js
echo "=== Mock test file (full) ==="
cat src/lib/tests/DataBrowser.pendingRestore.test.js
Length of output: 40347
@mtrezza, that's a valid concern. The core problem with the current mock approach is that MockDataBrowser duplicates the logic from DataBrowser.react.js (the runPendingRestoreLogic and wouldAutoLoadFirstRowFire methods are verbatim copies) rather than importing and exercising the real component. This means the tests validate the mock's internal copy of the logic — if the real component diverges or has a bug not present in the copy, all tests still pass. It's testing a shadow, not the source.
For comparison, the existing Puppeteer e2e test at src/lib/tests/e2e/dashboard.e2e.test.js follows a pattern of spinning up a real Express + ParseDashboard server and driving an actual browser, which validates real behavior.
There are a few better alternatives, in order of confidence:
-
Puppeteer E2E test (aligned with existing project pattern): Start Parse Server + Dashboard, seed an object, select a cell, trigger a refresh (e.g. via
page.evaluateto call the reload action), then assert viapage.evaluateor DOM queries that the same cell is still selected. This would catch regressions in the real component. -
React Testing Library on the real component: Mount the actual
DataBrowsercomponent in a jsdom environment, simulate theprops.datatransitionnon-null → null → non-null, and assert on the resulting state/DOM. This is faster than Puppeteer and tests the real component. -
Extract the logic as a pure function: Move
pendingRestorebookkeeping into a standalone utility function (e.g.computePendingRestore(prevProps, state)) in its own file and unit-test that. The mock approach is acceptable only when the tested logic is the actual exported function, not a copy.
Given that Puppeteer and the e2e infrastructure are already in place, option 1 or 2 would give the most meaningful coverage here. Would you like me to draft an E2E test for this feature following the pattern in dashboard.e2e.test.js?
🧠 Learnings used
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 0
File: :0-0
Timestamp: 2026-01-15T17:32:12.701Z
Learning: CodeRabbit can use the Browser Control API feature in Parse Dashboard to spin up a headless dashboard and validate features including UI layout during reviews. The feature is documented in Parse-Dashboard/browser-control/README.md and provides endpoints for session management, navigation, screenshots, and JavaScript evaluation to enable automated UI testing and validation.
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 3145
File: src/dashboard/Data/CustomDashboard/CustomDashboard.react.js:1281-1327
Timestamp: 2026-01-23T18:51:19.954Z
Learning: Avoid using disableMouseDown={true} on MenuItem components. Across the codebase, only a small minority of MenuItem instances require it, and MenuItems function correctly with the standard onClick and onMouseDown handler flow. Apply this guideline broadly to all MenuItem usages in JavaScript/JSX files; reserve disableMouseDown for explicit, well-justified edge cases only.
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 3185
File: package.json:51-51
Timestamp: 2026-02-07T02:18:48.037Z
Learning: In parse-dashboard code, async route handlers should wrap logic in try/catch and respond with appropriate HTTP errors (e.g., 500 with an error message). This guards against unhandled promise rejections. If a route handler returns a rejected promise and Express 5 is configured, explicit rejection forwarding may be used instead. Apply this guideline to all server routes and middleware that perform async operations.
There was a problem hiding this comment.
@coderabbitai guide @evolvomind on how to implement a better approach, preferably option 1.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== E2E test file ==="
cat src/lib/tests/e2e/dashboard.e2e.test.js
echo "=== DataBrowser changes (pendingRestore) ==="
cat src/dashboard/Data/Browser/DataBrowser.react.js | grep -n "pendingRestore\|autoLoadFirstRow\|setCurrent\|setSelectedObjectId" | head -60
Length of output: 5136
@evolvomind, here's a concrete guide to replace the mock-based test with a real Puppeteer E2E test, following the pattern already established in src/lib/tests/e2e/dashboard.e2e.test.js.
Why the current approach falls short
MockDataBrowser.runPendingRestoreLogic is a verbatim copy of the real component logic — the tests validate the copy, not the actual DataBrowser.react.js. Any divergence between the two (a future refactor, a bug fix) leaves all tests green while real behavior breaks.
Step-by-step: Puppeteer E2E test
1. Understand the existing scaffold
The existing dashboard.e2e.test.js already shows the pattern — Express + ParseDashboard(settings) + puppeteer.launch. The only missing piece (noted by its own // TODO) is a running Parse Server.
2. Add Parse Server as a test dependency
const ParseServer = require('parse-server').ParseServer; // already a dev dep
const Parse = require('parse/node');3. Draft test file
Create src/lib/tests/e2e/DataBrowser.pendingRestore.e2e.test.js:
/*
* Copyright (c) 2016-present, Parse, LLC
* All rights reserved.
*
* This source code is licensed under the license found in the LICENSE file in
* the root directory of this source tree.
*/
jest.disableAutomock();
jest.setTimeout(60_000);
const express = require('express');
const ParseServer = require('parse-server').ParseServer;
const ParseDashboard = require('../../../../Parse-Dashboard/app');
const puppeteer = require('puppeteer');
const Parse = require('parse/node');
const APP_ID = 'pendingRestoreAppId';
const MASTER_KEY = 'masterKey';
const SERVER_PORT = 5150;
const DASH_PORT = 5151;
const SERVER_URL = `http://localhost:${SERVER_PORT}/parse`;
const DASH_URL = `http://localhost:${DASH_PORT}/dashboard`;
const CLASS_NAME = 'PendingRestoreTest';
let parseServer, dashboardServer, browser;
async function seedTestObject() {
const obj = new Parse.Object(CLASS_NAME);
obj.set('title', 'Hello World');
obj.set('count', 42);
await obj.save(null, { useMasterKey: true });
return obj.id;
}
beforeAll(async () => {
// 1. Start Parse Server
const api = new ParseServer({
databaseURI: 'mongodb://localhost:27017/pendingRestoreTest',
appId: APP_ID,
masterKey: MASTER_KEY,
serverURL: SERVER_URL,
});
const parseApp = express();
parseApp.use('/parse', api.app);
await new Promise(resolve => {
parseServer = parseApp.listen(SERVER_PORT, resolve);
});
// 2. Initialize Parse JS SDK
Parse.initialize(APP_ID, undefined, MASTER_KEY);
Parse.serverURL = SERVER_URL;
// 3. Start Dashboard
const dashApp = express();
dashApp.use(
'/dashboard',
ParseDashboard({
apps: [
{
serverURL: SERVER_URL,
appId: APP_ID,
masterKey: MASTER_KEY,
appName: 'TestApp',
},
],
trustProxy: 1,
})
);
await new Promise(resolve => {
dashboardServer = dashApp.listen(DASH_PORT, resolve);
});
// 4. Launch browser
browser = await puppeteer.launch({ args: ['--no-sandbox'] });
});
afterAll(async () => {
await browser?.close();
await new Promise(resolve => dashboardServer.close(resolve));
await new Promise(resolve => parseServer.close(resolve));
});
describe('DataBrowser - pendingRestore', () => {
it('keeps the correct cell selected after a data refresh', async () => {
// Seed a known object
const objectId = await seedTestObject();
const page = await browser.newPage();
// Navigate directly to the class in DataBrowser
await page.goto(`${DASH_URL}/apps/TestApp/browser/${CLASS_NAME}`, {
waitUntil: 'networkidle2',
});
// Wait for rows to render
await page.waitForSelector('[data-object-id]');
// Click the cell for the seeded object in the 'title' column
// Adjust selectors to match the actual DataBrowser DOM
const cellSelector = `[data-object-id="${objectId}"] [data-field="title"]`;
await page.waitForSelector(cellSelector);
await page.click(cellSelector);
// Confirm cell appears selected
const isSelected = await page.$eval(cellSelector, el =>
el.classList.contains('selected') || el.closest('tr')?.classList.contains('row-selected')
);
expect(isSelected).toBe(true);
// Trigger a refresh — locate and click the refresh/reload button
const refreshSelector = 'button[data-action="refresh"], button[title="Refresh"]';
await page.waitForSelector(refreshSelector);
await page.click(refreshSelector);
// Wait for data to reload (rows disappear then reappear)
await page.waitForSelector('[data-object-id]');
// The same cell must be selected again after refresh
const isStillSelected = await page.$eval(cellSelector, el =>
el.classList.contains('selected') || el.closest('tr')?.classList.contains('row-selected')
);
expect(isStillSelected).toBe(true);
});
it('clears selection when the object no longer exists after refresh', async () => {
const objectId = await seedTestObject();
const page = await browser.newPage();
await page.goto(`${DASH_URL}/apps/TestApp/browser/${CLASS_NAME}`, {
waitUntil: 'networkidle2',
});
await page.waitForSelector('[data-object-id]');
const cellSelector = `[data-object-id="${objectId}"] [data-field="title"]`;
await page.waitForSelector(cellSelector);
await page.click(cellSelector);
// Delete the object before refresh
const query = new Parse.Query(CLASS_NAME);
const obj = await query.get(objectId, { useMasterKey: true });
await obj.destroy({ useMasterKey: true });
// Refresh
const refreshSelector = 'button[data-action="refresh"], button[title="Refresh"]';
await page.click(refreshSelector);
await page.waitForSelector('[data-object-id]');
// No cell should be selected
const anySelected = await page.$$eval('[data-field]', cells =>
cells.some(
el => el.classList.contains('selected') || el.closest('tr')?.classList.contains('row-selected')
)
);
expect(anySelected).toBe(false);
});
});4. Key things to adapt
| What | Why |
|---|---|
data-object-id, data-field, selected class names |
Inspect the actual DataBrowser DOM (use page.screenshot() or devtools) to get real selectors — adjust them to match what the component renders |
button[data-action="refresh"] |
Find the real refresh button selector — can use page.$x('//button[contains(., "Refresh")]') as a fallback |
| MongoDB URI | Use mongodb-memory-server if a real Mongo instance isn't available in CI |
networkidle2 |
May need waitForFunction or waitForResponse matching the Parse query endpoint if transitions are fast |
5. Run it
npx jest DataBrowser.pendingRestore.e2e.test.js --testTimeout=600006. Remove the mock test
Once the E2E test is in place and green, src/lib/tests/DataBrowser.pendingRestore.test.js can be deleted — it provides no additional safety net that the E2E test doesn't cover with real confidence.
🧠 Learnings used
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 0
File: :0-0
Timestamp: 2026-01-15T17:32:12.701Z
Learning: CodeRabbit can use the Browser Control API feature in Parse Dashboard to spin up a headless dashboard and validate features including UI layout during reviews. The feature is documented in Parse-Dashboard/browser-control/README.md and provides endpoints for session management, navigation, screenshots, and JavaScript evaluation to enable automated UI testing and validation.
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 3145
File: src/dashboard/Data/CustomDashboard/CustomDashboard.react.js:1281-1327
Timestamp: 2026-01-23T18:51:19.954Z
Learning: Avoid using disableMouseDown={true} on MenuItem components. Across the codebase, only a small minority of MenuItem instances require it, and MenuItems function correctly with the standard onClick and onMouseDown handler flow. Apply this guideline broadly to all MenuItem usages in JavaScript/JSX files; reserve disableMouseDown for explicit, well-justified edge cases only.
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 3185
File: package.json:51-51
Timestamp: 2026-02-07T02:18:48.037Z
Learning: In parse-dashboard code, async route handlers should wrap logic in try/catch and respond with appropriate HTTP errors (e.g., 500 with an error message). This guards against unhandled promise rejections. If a route handler returns a rejected promise and Express 5 is configured, explicit rejection forwarding may be used instead. Apply this guideline to all server routes and middleware that perform async operations.
Signed-off-by: evolvomind <96019984+evolvomind@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/tests/e2e/DataBrowser.pendingRestore.e2e.test.js`:
- Around line 184-189: The test is calling getAttribute() on an ElementHandle
(currentCell) which doesn't exist; replace those calls with page.evaluate to
read attributes from the element handle (e.g., use page.evaluate(el =>
el.getAttribute('data-object-id'), currentCell) and similarly for 'data-field'),
then assert the returned strings against objectId and 'title'; update the
references to currentCell and the two getAttribute usages in this test to use
page.evaluate instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac7cefd1-c72e-4ca1-a177-70c72287c9ae
📒 Files selected for processing (2)
src/components/BrowserCell/BrowserCell.react.jssrc/lib/tests/e2e/DataBrowser.pendingRestore.e2e.test.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
changelogs/CHANGELOG_alpha.md (1)
1-6:⚠️ Potential issue | 🟠 MajorRemove this manual changelog entry.
This repository does not accept PR edits to
changelogs/CHANGELOG_*.md; semantic-release generates them automatically, so this hunk should be reverted before merge.Based on learnings: In the parse-community/parse-dashboard repository, do not include manual modifications to changelog files. Changelogs are managed automatically by semantic-release. If a pull request contains changes to changelogs/CHANGELOG_*.md or changelogs/CHANGELOG.md, reject the PR and ask the contributor to revert changelog changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@changelogs/CHANGELOG_alpha.md` around lines 1 - 6, The PR contains a manual changelog hunk (the "# [Unreleased]" section and the entry "Keep cell selected by objectId and field name after data browser refresh") which must be removed because changelogs are auto-generated by semantic-release; revert the changes to the changelogs/CHANGELOG_alpha.md hunk so the file is unchanged in this PR and ensure no manual edits to any changelogs/CHANGELOG_*.md remain before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@changelogs/CHANGELOG_alpha.md`:
- Around line 1-6: The PR contains a manual changelog hunk (the "# [Unreleased]"
section and the entry "Keep cell selected by objectId and field name after data
browser refresh") which must be removed because changelogs are auto-generated by
semantic-release; revert the changes to the changelogs/CHANGELOG_alpha.md hunk
so the file is unchanged in this PR and ensure no manual edits to any
changelogs/CHANGELOG_*.md remain before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4be40de1-bc0f-4ae4-8a2d-465785ffa150
📒 Files selected for processing (1)
changelogs/CHANGELOG_alpha.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/tests/e2e/DataBrowser.pendingRestore.e2e.test.js`:
- Around line 175-179: The test uses the removed page.$x() API (variable
refreshByText) — replace both occurrences with the Playwright-style xpath
selector and proper waits: use
page.waitForSelector('xpath//a[.//span[text()="Refresh"]]') or
page.locator('xpath//a[.//span[text()="Refresh"]]') to get the element, click it
(e.g., await (await page.waitForSelector(...)).click() or await
page.locator(...).click()), then await a navigation/network idle wait
(page.waitForNavigation() or page.waitForLoadState('networkidle')) before
awaiting page.waitForSelector('[data-object-id]', { timeout: 10000 }) to avoid
the race condition; update both spots (the one that created refreshByText and
the second occurrence around ~214) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d122dbe1-0f43-4987-a6d6-84b991fa314c
📒 Files selected for processing (1)
src/lib/tests/e2e/DataBrowser.pendingRestore.e2e.test.js
| const refreshByText = await page.$x("//a[.//span[text()='Refresh']]").then(nodes => nodes[0]); | ||
| expect(refreshByText).toBeTruthy(); | ||
| await refreshByText.click(); | ||
|
|
||
| await page.waitForSelector('[data-object-id]', { timeout: 10000 }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Puppeteer page.$x deprecated replacement locator API
💡 Result:
page.$x() (and page.waitForXPath()) were removed in Puppeteer v22.0.0 (2024-02-05). Use the unified selector/Locator APIs instead. [1]
Replacement: Locator API (recommended)
Use an XPath selector with page.locator():
await page.locator('xpath///h2').click(); // prefix "xpath/" + XPath starting with "//h2"Puppeteer docs show this xpath/... locator form. [2]
Replacement: unified selector API (page.$, page.$$, waitForSelector)
You can also query XPath via the normal selector methods by using the xpath/ prefix:
const el = await page.$('xpath///h2');
const els = await page.$$('xpath///h2');
await page.waitForSelector('xpath///h2');Puppeteer explicitly documents that selectors can be prefixed to choose XPath (along with text/aria, etc.). [3]
Sources
[1] Puppeteer changelog (v22.0.0 breaking change: removed $x and waitForXpath) https://pptr.dev/next/CHANGELOG
[2] Puppeteer guide: Page interactions (example page.locator('xpath///h2')) https://pptr.dev/guides/page-interactions
[3] Puppeteer API: Page.$() method (documents selector prefixes including xpath) https://pptr.dev/api/puppeteer.page._
🏁 Script executed:
# Find package.json and check Puppeteer version
fd -t f package.json | xargs grep -l puppeteer | head -5Repository: parse-community/parse-dashboard
Length of output: 87
🏁 Script executed:
# Check the actual Puppeteer version
find . -name "package.json" -o -name "package-lock.json" -o -name "yarn.lock" | head -3 | xargs grep -A 2 '"puppeteer"'Repository: parse-community/parse-dashboard
Length of output: 514
🏁 Script executed:
# Find all uses of page.$x in the test file and codebase
rg "page\.\$x\(" -A 2 -B 2 src/lib/tests/e2e/Repository: parse-community/parse-dashboard
Length of output: 57
🏁 Script executed:
# Check if the test file exists and view the relevant section
if [ -f "src/lib/tests/e2e/DataBrowser.pendingRestore.e2e.test.js" ]; then
echo "File found. Reading lines 170-185:";
sed -n '170,185p' "src/lib/tests/e2e/DataBrowser.pendingRestore.e2e.test.js";
else
echo "File not found. Listing e2e test files:";
find . -path "*/tests/e2e/*.test.js" -type f | head -10;
fiRepository: parse-community/parse-dashboard
Length of output: 748
🏁 Script executed:
# Search for page.$x usage across all files
rg "page\.\\\$x" --type jsRepository: parse-community/parse-dashboard
Length of output: 386
🏁 Script executed:
# Also check if Puppeteer locator or updated selector syntax is already in use
rg "page\.locator|page\.\\\$\(" src/lib/tests/e2e/ -A 1 | head -20Repository: parse-community/parse-dashboard
Length of output: 1007
Fix critical broken page.$x() calls — API was removed in Puppeteer v22.0.0 and no longer exists in v24.37.2.
Replace with the correct xpath/ selector prefix:
- const refreshByText = await page.$x("//a[.//span[text()='Refresh']]").then(nodes => nodes[0]);
+ const refreshByText = await page.$('xpath///a[.//span[text()="Refresh"]]');
expect(refreshByText).toBeTruthy();
await refreshByText.click();This pattern occurs twice in the file (lines 175 and ~214). Also apply the same fix to the second occurrence.
Additionally, after clicking refresh, consider waiting for page.waitForNavigation() or page.waitForNetworkIdle() before checking for [data-object-id] to avoid race conditions where the selector matches the old DOM.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const refreshByText = await page.$x("//a[.//span[text()='Refresh']]").then(nodes => nodes[0]); | |
| expect(refreshByText).toBeTruthy(); | |
| await refreshByText.click(); | |
| await page.waitForSelector('[data-object-id]', { timeout: 10000 }); | |
| const refreshByText = await page.locator('xpath///a[.//span[text()="Refresh"]]'); | |
| expect(refreshByText).toBeTruthy(); | |
| await refreshByText.click(); | |
| await page.waitForSelector('[data-object-id]', { timeout: 10000 }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tests/e2e/DataBrowser.pendingRestore.e2e.test.js` around lines 175 -
179, The test uses the removed page.$x() API (variable refreshByText) — replace
both occurrences with the Playwright-style xpath selector and proper waits: use
page.waitForSelector('xpath//a[.//span[text()="Refresh"]]') or
page.locator('xpath//a[.//span[text()="Refresh"]]') to get the element, click it
(e.g., await (await page.waitForSelector(...)).click() or await
page.locator(...).click()), then await a navigation/network idle wait
(page.waitForNavigation() or page.waitForLoadState('networkidle')) before
awaiting page.waitForSelector('[data-object-id]', { timeout: 10000 }) to avoid
the race condition; update both spots (the one that created refreshByText and
the second occurrence around ~214) accordingly.
3 participants
feat: Keep cell selected by objectId and field name after data browser refresh (#2631)
Pull Request
Issue
Closes #2631
When clicking "Refresh" in the data browser, the selected cell stayed at the same row/column
coordinates even if the underlying document had moved to a different row. This caused the wrong
cell to appear selected, which could lead to user errors.
Approach
A
pendingRestorestate field ({ objectId, fieldName }) is added toDataBrowser. Threenew blocks in
componentDidUpdatehandle the full refresh lifecycle:databecomesnull(refresh starts), the selected document'sobjectIdand column
fieldNameare captured intopendingRestorebefore anything is cleared.datatransitions fromnullback to the new array (refresh ends),findIndexlocates the document byobjectIdand the column byfieldNamein the freshdata. If both are found,
currentis updated to the new coordinates and the aggregationpanel is re-loaded for that object. If either is not found (document deleted, column
removed), the selection is cleared so no wrong cell is highlighted.
pendingRestoreis discarded to prevent it from firing against a different class's data.
An additional guard (
!this.state.pendingRestore) is added to theautoLoadFirstRowblockto prevent it from overwriting the restored selection with row 0 when the panel is open.
Tasks
Summary by CodeRabbit