Repository navigation
fix: Prevent Config table crash when a parameter value is null - #3424
Conversation
Null config params are typeof object, so accessing __type threw; skip File/GeoPoint parsing when value is null.
|
🚀 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. |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughConfiguration table row construction is extracted into ChangesConfig table data
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/tests/Config.tableData.test.js (1)
40-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the File and GeoPoint conversion branches.
This regression test covers
nulland a string only. A regression in either modified serialized Parse conversion path would still pass. Add File and GeoPoint fixtures and assert they remain normalized before relying on manual validation.🤖 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/Config.tableData.test.js` around lines 40 - 60, Extend the tableData regression test around Config.prototype.tableData to include serialized Parse File and GeoPoint parameter fixtures alongside the existing null and string values. Assert that both values are converted to their expected normalized representations, covering each modified conversion branch before the existing validation.
🤖 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.
Nitpick comments:
In `@src/lib/tests/Config.tableData.test.js`:
- Around line 40-60: Extend the tableData regression test around
Config.prototype.tableData to include serialized Parse File and GeoPoint
parameter fixtures alongside the existing null and string values. Assert that
both values are converted to their expected normalized representations, covering
each modified conversion branch before the existing validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83653b63-4c87-4434-b15e-052e6e9bb163
📒 Files selected for processing (2)
src/dashboard/Data/Config/Config.react.jssrc/lib/tests/Config.tableData.test.js
|
@mtrezza checked locally in our staging env, the fix is working |
mtrezza
left a comment
mtrezza
left a comment
There was a problem hiding this comment.
Note also that the pre-merge checks are unaddressed.
Move null-safe File/GeoPoint normalization into a pure helper so tests no longer need extensive React component mocks, and cover File/GeoPoint paths.
|
@mtrezza Addressed both points:
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/tests/Config.tableData.test.js (1)
15-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover the missing-metadata input used by production.
This test passes
Map(), so it cannot catch the helper’s.get()crash whenConfig.tableData()supplies{}. Add a regression case matching the production fallback after the contract is fixed.🤖 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/Config.tableData.test.js` around lines 15 - 24, Add a regression test alongside the existing null-parameter case that calls buildConfigTableData with the same parameter map and a plain empty object ({}) for missing metadata, matching Config.tableData()’s production fallback. Assert it does not throw and preserves the expected table rows, including null values.
🤖 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.
Inline comments:
In `@src/lib/ConfigTableData.js`:
- Around line 10-13: Fix the metadata-map contract across all affected sites:
update buildConfigTableData to safely accept missing metadata or explicitly
require a map; in src/dashboard/Data/Config/Config.react.js lines 477-483,
replace the plain-object fallback with an empty Immutable map; and in
src/lib/tests/Config.tableData.test.js lines 15-24, add regression coverage for
absent masterKeyOnly metadata.
---
Nitpick comments:
In `@src/lib/tests/Config.tableData.test.js`:
- Around line 15-24: Add a regression test alongside the existing null-parameter
case that calls buildConfigTableData with the same parameter map and a plain
empty object ({}) for missing metadata, matching Config.tableData()’s production
fallback. Assert it does not throw and preserves the expected table rows,
including null values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 853d6c56-19a7-46ac-8bb9-8e1fde43318b
📒 Files selected for processing (3)
src/dashboard/Data/Config/Config.react.jssrc/lib/ConfigTableData.jssrc/lib/tests/Config.tableData.test.js
Use an Immutable Map fallback and defensive .get access so missing metadata no longer crashes table rendering, with a regression test covering the production {} case.
|
✅ Action performedReview finished.
|
# [9.3.0-alpha.2](9.3.0-alpha.1...9.3.0-alpha.2) (2026-07-23) ### Bug Fixes * Prevent Config table crash when a parameter value is null ([#3424](#3424)) ([7150acf](7150acf))
|
🎉 This change has been released in version 9.3.0-alpha.2 |
# [10.0.0](9.2.0...10.0.0) (2026-11-04) ### Bug Fixes * Add Cancel and Add buttons to the add-row modal ([#1995](#1995)) ([#3383](#3383)) ([2d1b739](2d1b739)) * Advertise localhost instead of 0.0.0.0 in the dashboard startup message ([#3410](#3410)) ([33ce7b8](33ce7b8)) * Bump body-parser from 2.2.2 to 2.3.0 ([#3465](#3465)) ([662899b](662899b)) * Bump body-parser from 2.3.0 to 2.4.0 ([#3508](#3508)) ([cb9caf8](cb9caf8)) * Bump brace-expansion from 1.1.13 to 1.1.21 ([#3454](#3454)) ([937fe17](937fe17)) * Bump glob from 10.4.5 to 10.5.0 and minimatch from 9.0.1 to 9.0.9 ([#3467](#3467)) ([f31ada5](f31ada5)) * Bump immutable from 5.1.5 to 5.1.9 ([#3457](#3457)) ([5097c04](5097c04)) * Bump markdown-it from 14.3.0 to 14.3.3 ([#3494](#3494)) ([12d7cc0](12d7cc0)) * Bump parse from 8.5.0 to 8.6.2 ([#3468](#3468)) ([8a4c12b](8a4c12b)) * Bump proxy-addr from 2.0.7 to 2.0.8 ([#3490](#3490)) ([e017c62](e017c62)) * Bump qs from 6.15.3 to 6.16.0 ([#3458](#3458)) ([04f5acb](04f5acb)) * Info panel data not loaded when opened over an existing selection ([#3384](#3384)) ([4879959](4879959)) * Prevent Config table crash when a parameter value is null ([#3424](#3424)) ([7150acf](7150acf)) * Remote access restrictions not enforced when running behind a reverse proxy ([#3436](#3436)) ([60cb379](60cb379)) * Security upgrade Docker image from Node.js 20 to 24 ([#3518](#3518)) ([9dc2c30](9dc2c30)) ### Features * Add support for Node.js 26 ([#3525](#3525)) ([bdc5d62](bdc5d62)) * React 19 + react-router 7 + GraphiQL 5 coordinated upgrade ([#3409](#3409)) ([c3e16c2](c3e16c2)) * Remove Node 20 support and increase minimum required Node 22 version to `22.13.0` ([#3527](#3527)) ([2ff9098](2ff9098)) ### BREAKING CHANGES * Removes support for Node 20 and increases the minimum required Node 22 version to `22.13.0`. ([2ff9098](2ff9098))
|
🎉 This change has been released in version 10.0.0 |
Pull Request
Issue
Cloud Config table crashed when a parameter value was
null.typeof null === 'object', sotableData()accessedvalue.__typeand threw before rendering.Approach
value !== nullbefore reading__type)buildConfigTableDatahelper so unit tests cover the behavior without React component mocksTasks
Summary by CodeRabbit
nullvalues.masterKeyOnlybehavior (including defaulting tofalsewhen metadata is missing).nullinputs and proper normalization of File/GeoPoint values andmasterKeyOnlyflags.