fix(ui): create map and pass down for legend formatter - #527
Conversation
cmatzenbach
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDAG charts now pass quantity specifications to legends. Continuous node and edge legends use quantity-aware formatting with field-based fallback. The workspace and UI package metadata now require pnpm 11.19.0 or newer. ChangesQuantity-aware DAG legends
pnpm version updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
ui/packages/@quent/components/src/dag/DAGChart.tsx-327-332 (1)
327-332: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse own-property-safe lookups for dynamic statistic and quantity keys.
The
HashMap<String, ...>fields do not reject prototype names. InDAGChart,stat.key in resultdrops validconstructor,toString, or__proto__statistics.data.quantitySpecs[stat.quantity]can also resolve inherited functions asQuantitySpecvalues.DAGLegendcan then pass an inherited value toformatQuantity. UseMapor null-prototype records with own-property checks in both locations.🤖 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 `@ui/packages/`@quent/components/src/dag/DAGChart.tsx around lines 327 - 332, Use own-property-safe lookups for dynamic statistic and quantity keys in DAGChart.tsx lines 327-332 and DAGLegend.tsx lines 138-139. Update the DAGChart result/statistic collection and quantitySpecs access to use Map or null-prototype records with own-property checks, preserving valid keys such as constructor, toString, and __proto__; ensure DAGLegend no longer passes inherited values to formatQuantity.
🧹 Nitpick comments (2)
ui/packages/@quent/components/src/dag/DAGChart.tsx (1)
338-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unconditional debug log.
console.log(statQuantitySpecs)runs when the map is recomputed and exposes server-derived metadata in browser consoles. Remove it or use an existing development-only logger.🤖 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 `@ui/packages/`@quent/components/src/dag/DAGChart.tsx at line 338, Remove the unconditional console.log call for statQuantitySpecs from the map recomputation logic in DAGChart, or replace it with the existing development-only logger if diagnostic output is still required.ui/src/components/DataFlowOverlay.test.tsx (1)
461-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-empty quantity-spec path.
statQuantitySpecs={{}}exercises only the fallback path. It cannot detect an incorrect field-to-spec match or broken quantity formatting. Add a case with a canonicalQuantitySpec, a matching custom-stat field, and an unknown field to verify both the override and inferred fallback.As per path instructions, tests must cover observable behavior, fallback/unknown inputs, and canonical production or generated types for fixtures.
🤖 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 `@ui/src/components/DataFlowOverlay.test.tsx` at line 461, Extend the DAGLegend test around the existing isDark={false} case to cover non-empty statQuantitySpecs using a canonical QuantitySpec fixture, a matching custom-stat field, and an unknown field. Assert observable quantity formatting for the matching override and the inferred fallback for the unknown field, using production or generated types rather than ad hoc fixture shapes.Source: Path instructions
🤖 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 `@ui/packages/`@quent/components/src/dag/DAGLegend.tsx:
- Around line 203-208: Update the DAGLegendProps definition and DAGLegend
component to make statQuantitySpecs optional and default it to an empty object,
while preserving the existing quantity-aware legend behavior when callers
provide specifications. Ensure consumers passing only isDark type-check
successfully.
---
Other comments:
In `@ui/packages/`@quent/components/src/dag/DAGChart.tsx:
- Around line 327-332: Use own-property-safe lookups for dynamic statistic and
quantity keys in DAGChart.tsx lines 327-332 and DAGLegend.tsx lines 138-139.
Update the DAGChart result/statistic collection and quantitySpecs access to use
Map or null-prototype records with own-property checks, preserving valid keys
such as constructor, toString, and __proto__; ensure DAGLegend no longer passes
inherited values to formatQuantity.
---
Nitpick comments:
In `@ui/packages/`@quent/components/src/dag/DAGChart.tsx:
- Line 338: Remove the unconditional console.log call for statQuantitySpecs from
the map recomputation logic in DAGChart, or replace it with the existing
development-only logger if diagnostic output is still required.
In `@ui/src/components/DataFlowOverlay.test.tsx`:
- Line 461: Extend the DAGLegend test around the existing isDark={false} case to
cover non-empty statQuantitySpecs using a canonical QuantitySpec fixture, a
matching custom-stat field, and an unknown field. Assert observable quantity
formatting for the matching override and the inferred fallback for the unknown
field, using production or generated types rather than ad hoc fixture shapes.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: e9385d65-0477-4f2a-b168-cc52d7619f44
📒 Files selected for processing (3)
ui/packages/@quent/components/src/dag/DAGChart.tsxui/packages/@quent/components/src/dag/DAGLegend.tsxui/src/components/DataFlowOverlay.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/pnpm-workspace.yaml (1)
35-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStore the shared version constraint in
catalog:.Because this is a workspace-wide dependency constraint, add the range to the existing
catalog:block and reference it fromoverrides:Proposed change
+catalog: + brace-expansion: '>=5.0.9' + overrides: - brace-expansion: '>=5.0.9' + brace-expansion: 'catalog:'Pnpm supports
catalog:references in overrides. (pnpm.io)As per path instructions, shared dependency versions must live in the top-level
catalog:block and usecatalog:references.🤖 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 `@ui/pnpm-workspace.yaml` around lines 35 - 37, Move the brace-expansion version range into the existing top-level catalog block, then update the overrides entry to reference that catalog key with the supported catalog: syntax. Preserve the same >=5.0.9 constraint and follow the workspace’s existing catalog naming conventions.Source: Path instructions
🤖 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 `@ui/pnpm-workspace.yaml`:
- Around line 35-37: Move the brace-expansion version range into the existing
top-level catalog block, then update the overrides entry to reference that
catalog key with the supported catalog: syntax. Preserve the same >=5.0.9
constraint and follow the workspace’s existing catalog naming conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 4472b74d-68b9-41d7-9658-6d4620f2fce4
⛔ Files ignored due to path filters (1)
ui/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!ui/pnpm-lock.yaml
📒 Files selected for processing (2)
ui/packages/@quent/components/src/dag/DAGLegend.tsxui/pnpm-workspace.yaml
johallar
left a comment
johallar
left a comment
There was a problem hiding this comment.
I think the audit failure was fixed in master, try merging that maybe before using the override
It hasn't been fixed yet, I just tested on latest |
Dang this package has come up multiple times then. In any case I think we should prefer updating pnpm-lock.json within the dependencies semver range, instead of the explicit "overrides" section wherever possible.
|
|
/merge |
2 participants
Quickfix follow-up to PR #471 - creates statQuantitySpecs map and passes down to legend for formatting