Repository navigation
fix(deck): pass custom num rows; detect missing columns fix - #793
Conversation
Signed-off-by: Ihor Dykhta <[email protected]>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdjusts dashboard missing-column detection for ChangesDeck dashboard and GeoArrow fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e693173cc0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Ihor Dykhta <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/deck/src/json/compileColorScale.ts (1)
88-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for the batch-missing-field case.
This fixes a real correctness bug (reading the wrong row across batches), but no test appears to cover the new
undefinedreturn path when a batch is present without the requested field. As per coding guidelines, "Add or update tests when changing behavior or public API, when practical."🤖 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 `@packages/deck/src/json/compileColorScale.ts` around lines 88 - 97, Add a regression test for the new batch-missing-field behavior in compileColorScale, covering the path where a batch is present but the requested field is absent and the function returns undefined instead of reading from the global vector. Update or extend the tests around compileColorScale and its mapper behavior to verify nullColor is applied for this GeoArrow batch-local case.Source: Coding guidelines
packages/deck/src/json/rewriteGeoArrowAccessors.ts (1)
58-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the missing-column fallthrough path.
This changes observable behavior (accessor is now compiled with an undefined-producing binding instead of being dropped from props). As per coding guidelines, "Add or update tests when changing behavior or public API, when practical," and no test appears to cover this branch.
🤖 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 `@packages/deck/src/json/rewriteGeoArrowAccessors.ts` around lines 58 - 64, Add or update tests for the missing-column fallthrough path in rewriteGeoArrowAccessors, covering the branch where vector is absent and compileGeoArrowAccessor is still used with an undefined-producing binding. Verify the accessor remains on props and produces undefined per row instead of being dropped, and reference the rewriteGeoArrowAccessors behavior in the test name or assertions so the new fallback path is explicitly exercised.Source: Coding guidelines
🤖 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 `@packages/deck/src/json/compileColorScale.ts`:
- Around line 88-97: Add a regression test for the new batch-missing-field
behavior in compileColorScale, covering the path where a batch is present but
the requested field is absent and the function returns undefined instead of
reading from the global vector. Update or extend the tests around
compileColorScale and its mapper behavior to verify nullColor is applied for
this GeoArrow batch-local case.
In `@packages/deck/src/json/rewriteGeoArrowAccessors.ts`:
- Around line 58-64: Add or update tests for the missing-column fallthrough path
in rewriteGeoArrowAccessors, covering the branch where vector is absent and
compileGeoArrowAccessor is still used with an undefined-producing binding.
Verify the accessor remains on props and produces undefined per row instead of
being dropped, and reference the rewriteGeoArrowAccessors behavior in the test
name or assertions so the new fallback path is explicitly exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2512c9c-b788-4ebb-8d9c-52e47701bf28
📒 Files selected for processing (2)
packages/deck/src/json/compileColorScale.tspackages/deck/src/json/rewriteGeoArrowAccessors.ts
Signed-off-by: Ihor Dykhta <[email protected]>
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
undefinedinstead of being removed.New Features