fix: Crash when filtering unique on a pointer field#3386
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 PR fixes a crash when applying the unique filter on a Pointer field. BrowserCell now guards pointer value extraction by checking that ChangesUnique filter pointer crash fix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant BrowserRow
participant BrowserCell
BrowserRow->>BrowserRow: check isUnique flag
alt isUnique is true
BrowserRow->>BrowserRow: skip _User requiredCols mapping
else isUnique is false
BrowserRow->>BrowserRow: compute _User requiredCols
end
BrowserRow->>BrowserCell: renderCellContent(value)
BrowserCell->>BrowserCell: check value.get is function
alt value.get exists
BrowserCell->>BrowserCell: extract defaultPointerKey via get
else value.get missing
BrowserCell->>BrowserCell: skip get call, avoid crash
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@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 (2)
src/components/BrowserCell/BrowserCell.react.js (2)
96-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
copyableValuestill reads the raw prop instead of the convertedvalue.Line 96-100 builds a proper
Parse.Object(with.idset fromobjectId) into the localvaluevariable specifically to handle raw Pointer objects, but line 113 assignsthis.copyableValue = this.props.value.id— the original prop, which has no.idfor raw pointers. Now that the crash on line 74 is fixed, this path is reachable and will silently setcopyableValuetoundefinedfor unique-filtered Pointer cells, breaking copy/"Add to config" functionality for exactly the scenario this PR fixes.🐛 Proposed fix
- this.copyableValue = this.props.value.id; + this.copyableValue = value.id;🤖 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/components/BrowserCell/BrowserCell.react.js` around lines 96 - 113, The BrowserCell.react.js pointer handling still sets copyableValue from the raw props instead of the normalized local value, so raw Pointer objects end up with undefined copy data. Update the BrowserCell render path that builds the Parse.Object and passes it into the Pill onClick handler so that copyableValue is assigned from the converted value (the local value with id populated from objectId) rather than this.props.value. This should be fixed in the same BrowserCell logic that handles the __type Pointer branch and the copyableValue assignment.
620-655: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard raw Pointer values before
toPointer()inpickFiltersrc/components/BrowserCell/BrowserCell.react.js:620-628— unique-mode Pointer cells pass raw JSON through here, so comparable filter actions from the context menu can throwTypeError: value.toPointer is not a function. A fallback for plain pointer JSON would keep filtering working.🤖 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/components/BrowserCell/BrowserCell.react.js` around lines 620 - 655, The Pointer branch in pickFilter assumes value always has toPointer(), but unique-mode Pointer cells can pass raw JSON and crash comparable filter actions. Update pickFilter in BrowserCell.react.js to detect plain Pointer objects before calling toPointer(), and fall back to using the raw pointer-shaped value when no method exists. Keep the fix localized to the Pointer case inside pickFilter so context-menu filtering still builds the compareTo value safely.
🧹 Nitpick comments (2)
src/lib/tests/BrowserRow.test.js (2)
109-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
global.localStoragemock isn't restored after the suite.Setting
global.localStorageinbeforeAllwithout anafterAllcleanup leaves the mock in place for any subsequent code executed in the same test file/process, which could mask real localStorage-dependent behavior in other tests.🤖 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/lib/tests/BrowserRow.test.js` around lines 109 - 115, The test setup in BrowserRow.test.js leaves the global.localStorage mock in place after the suite, which can affect later tests in the same process. Update the existing beforeAll setup by adding matching afterAll cleanup that restores the original global.localStorage value, and keep the mock scoped to the BrowserRow test suite so subsequent tests see the real storage behavior.
108-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting rendered content, not just "no throw".
The test only checks that rendering doesn't throw. Strengthening it to also assert the rendered pointer value (e.g.
user1objectId showing up in the tree) would better match the PR's stated goal ("distinct pointer values render") and guard against silent regressions where rendering succeeds but shows wrong/blank content.🤖 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/lib/tests/BrowserRow.test.js` around lines 108 - 131, The Unique filter test only verifies that BrowserRow renders without throwing, so it should also assert the rendered pointer content. Update the BrowserRow.test.js case around BrowserRow/renderer.create to inspect the rendered tree and confirm the unique _User pointer objectId (such as user1) is visible in the output, not just that rendering succeeds. Keep the existing no-throw check if helpful, but add a concrete content assertion so the test covers the intended distinct pointer rendering behavior.
🤖 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/components/BrowserCell/BrowserCell.react.js`:
- Around line 96-113: The BrowserCell.react.js pointer handling still sets
copyableValue from the raw props instead of the normalized local value, so raw
Pointer objects end up with undefined copy data. Update the BrowserCell render
path that builds the Parse.Object and passes it into the Pill onClick handler so
that copyableValue is assigned from the converted value (the local value with id
populated from objectId) rather than this.props.value. This should be fixed in
the same BrowserCell logic that handles the __type Pointer branch and the
copyableValue assignment.
- Around line 620-655: The Pointer branch in pickFilter assumes value always has
toPointer(), but unique-mode Pointer cells can pass raw JSON and crash
comparable filter actions. Update pickFilter in BrowserCell.react.js to detect
plain Pointer objects before calling toPointer(), and fall back to using the raw
pointer-shaped value when no method exists. Keep the fix localized to the
Pointer case inside pickFilter so context-menu filtering still builds the
compareTo value safely.
---
Nitpick comments:
In `@src/lib/tests/BrowserRow.test.js`:
- Around line 109-115: The test setup in BrowserRow.test.js leaves the
global.localStorage mock in place after the suite, which can affect later tests
in the same process. Update the existing beforeAll setup by adding matching
afterAll cleanup that restores the original global.localStorage value, and keep
the mock scoped to the BrowserRow test suite so subsequent tests see the real
storage behavior.
- Around line 108-131: The Unique filter test only verifies that BrowserRow
renders without throwing, so it should also assert the rendered pointer content.
Update the BrowserRow.test.js case around BrowserRow/renderer.create to inspect
the rendered tree and confirm the unique _User pointer objectId (such as user1)
is visible in the output, not just that rendering succeeds. Keep the existing
no-throw check if helpful, but add a concrete content assertion so the test
covers the intended distinct pointer rendering behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6669f191-7d22-499f-af3c-860eee9b9629
📒 Files selected for processing (3)
src/components/BrowserCell/BrowserCell.react.jssrc/components/BrowserRow/BrowserRow.react.jssrc/lib/tests/BrowserRow.test.js
1 participant
Closes #2657
Summary by CodeRabbit