Add legends column to XY trace viewer#404
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds legend color computation and a dedicated legend column to XY chart tree tables. It threads ChangesLegend color and column feature
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
hoathaitran3209
changed the title
![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
🧹 Nitpick comments (2)
local-libs/traceviewer-libs/react-components/src/components/utils/xy-shared.ts (1)
23-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ColumnSpecduplicatesColumnHeader.
ColumnSpec(title, sortable?, resizable?, tooltip?) is nearly identical to the existingColumnHeaderinterface (which additionally hasdataType?). Maintaining two parallel header shapes (with casts likebuilt.columns as anyelsewhere) risks drift between them.Consider reusing/extending
ColumnHeaderinstead of introducing a new type.🤖 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 `@local-libs/traceviewer-libs/react-components/src/components/utils/xy-shared.ts` around lines 23 - 28, ColumnSpec is duplicating the existing ColumnHeader shape, which can drift over time; update the shared types in xy-shared.ts so ColumnSpec reuses or extends ColumnHeader instead of defining a parallel interface. Keep the ColumnSpec name only if needed for compatibility, but make it derive from ColumnHeader (including its optional dataType field) and then adjust any consumers of ColumnSpec to use the unified type rather than casts like built.columns as any.local-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table.tsx (1)
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
legendColumnIndexprop onTablePropsis unused.No external caller passes
legendColumnIndextoTable—FilterTree.renderTable()only forwardslegendColors.Tablealways computes it locally viagetLegendColumnIndex(this.props.headers)and explicitly passes the computed value toTableBody, overriding any spread value. The declared prop is dead code and could confuse maintainers into thinking they need to supply it.♻️ Remove unused prop
interface TableProps { nodes: TreeNode[]; selectedRow?: number; multiSelectedRows?: number[]; collapsedNodes: number[]; isCheckable: boolean; isClosable: boolean; isPinnable?: boolean; pinnedRows?: number[]; sortConfig: SortConfig[]; onRowClick: (id: number) => void; onMultipleRowClick?: (id: number, isShiftClicked?: boolean) => void; onContextMenu: (event: React.MouseEvent<HTMLDivElement>, id: number) => void; getCheckedStatus: (id: number) => number; onToggleCollapse: (id: number) => void; onToggleCheck: (id: number) => void; onClose: (id: number) => void; onPin?: (id: number) => void; onSort: (sortedNodes: TreeNode[]) => void; onSortReset: () => void; onSortConfigChange: (sortConfig: SortConfig[]) => void; showHeader: boolean; headers: ColumnHeader[]; className: string; hideFillers?: boolean; emptyNodes: number[]; hideEmptyNodes: boolean; - legendColumnIndex?: number; legendColors?: Record<number, string>; }🤖 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 `@local-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table.tsx` around lines 36 - 37, `legendColumnIndex` is dead on `TableProps` and should be removed from the `Table` API. Update the `Table` type definition in `table.tsx` to drop the unused prop, and make sure `Table`, `TableBody`, and any related prop spreads continue to rely only on the locally computed value from `getLegendColumnIndex(this.props.headers)`. Also verify `FilterTree.renderTable()` still passes only the supported props and that no other callers reference `legendColumnIndex`.
🤖 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
`@local-libs/traceviewer-libs/react-components/src/components/utils/xy-shared.ts`:
- Around line 53-74: `buildLegendColors` is using a fallback key that can differ
from the chart’s series color key, causing inconsistent legend colors and extra
allocator entries. Update `buildLegendColors` in `xy-shared.ts` to derive the
legend color key from the same source used by `abstract-xy-output-component.tsx`
for `series.seriesName`, or delay color assignment until `seriesNameById` is
available. Keep the logic aligned around the `buildLegendColors` loop and the
`seriesNameById` lookup so the legend swatch always matches the series color.
---
Nitpick comments:
In
`@local-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table.tsx`:
- Around line 36-37: `legendColumnIndex` is dead on `TableProps` and should be
removed from the `Table` API. Update the `Table` type definition in `table.tsx`
to drop the unused prop, and make sure `Table`, `TableBody`, and any related
prop spreads continue to rely only on the locally computed value from
`getLegendColumnIndex(this.props.headers)`. Also verify
`FilterTree.renderTable()` still passes only the supported props and that no
other callers reference `legendColumnIndex`.
In
`@local-libs/traceviewer-libs/react-components/src/components/utils/xy-shared.ts`:
- Around line 23-28: ColumnSpec is duplicating the existing ColumnHeader shape,
which can drift over time; update the shared types in xy-shared.ts so ColumnSpec
reuses or extends ColumnHeader instead of defining a parallel interface. Keep
the ColumnSpec name only if needed for compatibility, but make it derive from
ColumnHeader (including its optional dataType field) and then adjust any
consumers of ColumnSpec to use the unified type rather than casts like
built.columns as any.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: feadc0f5-de6c-4771-b43f-604f045adbf2
📒 Files selected for processing (13)
local-libs/traceviewer-libs/react-components/src/components/abstract-xy-output-component.tsxlocal-libs/traceviewer-libs/react-components/src/components/generic-xy-output-component.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/entry-tree.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table-body.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table-cell.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table-row.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/tree.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/utils.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/xy-shared.tslocal-libs/traceviewer-libs/react-components/src/components/xy-output-component.tsxlocal-libs/traceviewer-libs/react-components/src/components/xy-output-overview-component.tsxlocal-libs/traceviewer-libs/react-components/style/output-components-style.css
hoathaitran3209
force-pushed
the
bug/IDE-92565-add-legends-column
branch
from
1ba74b0 to
dab1731
Compare
hoathaitran3209
force-pushed
the
bug/IDE-92565-add-legends-column
branch
from
dab1731 to
f5bbbca
Compare
|
Hey, I like this patch a lot, it is something I was planning on doing when I had breathing time. Thanks, the only issue is that it does not take the colors given by the tsp. therefore the totals are no longer red. Can you fix that? Hint from claude: Here's the mechanism, from the cherry-picked code: Chart lines get their color in buildXYData (abstract-xy-output-component.tsx): So the ColorAllocator key for a line is the full series.seriesName. The legend gets its color in buildLegendColors (xy-shared.ts): ColorAllocator.get(key) caches by the string key and hands out palette slots in first-seen order: The problem is timing plus key identity:
registerSeriesNames, which run during updateXY() (called after this setState).
Net effect: the legend swatches are derived from a fresh, differently-keyed palette walk rather than from the color the chart actually The correct fix is to make the legend use the same identity the chart uses — either look up the color already assigned to each series |
|
Hi @MatthewKhouzam, about your comment, I updated it. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@local-libs/traceviewer-libs/react-components/src/components/abstract-xy-output-component.tsx`:
- Line 162: Clear the seriesNameById map before registering series names in both
buildScatterData and buildXYData. Ensure each updateXY cycle removes stale
unchecked-series entries while preserving ColorAllocator caching so rechecked
series retain their previous colors.
In
`@local-libs/traceviewer-libs/react-components/src/components/generic-xy-output-component.tsx`:
- Around line 353-361: Clear this.seriesNameById before registerSeriesNames in
updateXY so unchecked series cannot retain stale legend colors; keep this.colors
unchanged so cached allocations are preserved. In the empty-series early-return
setState branch, also reset legendColors to {}.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c755c040-785d-46c1-a80c-18196d4ed78a
📒 Files selected for processing (13)
local-libs/traceviewer-libs/react-components/src/components/abstract-xy-output-component.tsxlocal-libs/traceviewer-libs/react-components/src/components/generic-xy-output-component.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/entry-tree.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table-body.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table-cell.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table-row.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/tree.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/utils.tsxlocal-libs/traceviewer-libs/react-components/src/components/utils/xy-shared.tslocal-libs/traceviewer-libs/react-components/src/components/xy-output-component.tsxlocal-libs/traceviewer-libs/react-components/src/components/xy-output-overview-component.tsxlocal-libs/traceviewer-libs/react-components/style/output-components-style.css
✅ Files skipped from review due to trivial changes (1)
- local-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table-body.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- local-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/entry-tree.tsx
- local-libs/traceviewer-libs/react-components/style/output-components-style.css
- local-libs/traceviewer-libs/react-components/src/components/xy-output-component.tsx
- local-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table-cell.tsx
- local-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table.tsx
- local-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/table-row.tsx
- local-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/utils.tsx
- local-libs/traceviewer-libs/react-components/src/components/utils/filter-tree/tree.tsx
- local-libs/traceviewer-libs/react-components/src/components/xy-output-overview-component.tsx
- local-libs/traceviewer-libs/react-components/src/components/utils/xy-shared.ts
| private _debouncedUpdateXY = debounce(() => this.updateXY(), 500); | ||
|
|
||
| private colors = new ColorAllocator(); | ||
| private seriesNameById = new Map<number, string>(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear seriesNameById before re-registering to avoid stale legend colors for unchecked series.
seriesNameById is never cleared between updateXY calls. When a user unchecks a series, its entry persists in the map, so buildLegendColors still assigns it a legend color even though it's absent from the chart. Meanwhile, series that were never checked have no entry and get no color. This creates an inconsistent legend: some unchecked rows show colors, others don't.
Clear the map before each registerSeriesNames call so only the currently-fetched (checked) series receive legend colors:
🛡️ Proposed fix for `buildScatterData` (line 473) and `buildXYData` (line 518)
private buildScatterData(seriesObj: XYSeries[]) {
+ this.seriesNameById.clear();
registerSeriesNames(this.seriesNameById, seriesObj); private buildXYData(seriesObj: XYSeries[]) {
+ this.seriesNameById.clear();
registerSeriesNames(this.seriesNameById, seriesObj);The ColorAllocator (this.colors) retains cached colors by key, so re-checking a previously unchecked series will still receive the same color it had before.
Also applies to: 473-473, 518-518
🤖 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
`@local-libs/traceviewer-libs/react-components/src/components/abstract-xy-output-component.tsx`
at line 162, Clear the seriesNameById map before registering series names in
both buildScatterData and buildXYData. Ensure each updateXY cycle removes stale
unchecked-series entries while preserving ColorAllocator caching so rechecked
series retain their previous colors.
| registerSeriesNames(this.seriesNameById, series); | ||
| const xy = this.buildXYData(series, this.mode); | ||
| flushSync(() => this.setState({ xyData: xy, outputStatus: model.status ?? ResponseStatus.COMPLETED })); | ||
| flushSync(() => | ||
| this.setState({ | ||
| xyData: xy, | ||
| outputStatus: model.status ?? ResponseStatus.COMPLETED, | ||
| legendColors: buildLegendColors(this.state.xyTree, this.colors, this.seriesNameById) | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear seriesNameById before re-registering to avoid stale legend colors for unchecked series.
seriesNameById is never cleared between updateXY calls. When a user unchecks a series, its entry persists, so buildLegendColors still assigns it a legend color even though it's absent from the chart. Meanwhile, never-checked series get no color. This creates an inconsistent legend.
🛡️ Proposed fix
registerSeriesNames(this.seriesNameById, series);
+// Or clear at the start of updateXY:
+// this.seriesNameById.clear();
+// registerSeriesNames(this.seriesNameById, series);The simplest approach is to clear before registering:
+this.seriesNameById.clear();
registerSeriesNames(this.seriesNameById, series);The ColorAllocator (this.colors) retains cached colors by key, so re-checking a series will still receive the same color.
Additionally, the empty-series early-return branch (lines 335–346) does not reset legendColors, so the legend retains stale colors when all series are unchecked. Add legendColors: {} to that setState call:
🛡️ Proposed fix for empty-series branch
if (!series.length) {
flushSync(() =>
this.setState({
xyData: { labels: [], datasets: [], labelIndices: [] },
- outputStatus: model.status ?? ResponseStatus.COMPLETED
+ outputStatus: model.status ?? ResponseStatus.COMPLETED,
+ legendColors: {}
})
);🤖 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
`@local-libs/traceviewer-libs/react-components/src/components/generic-xy-output-component.tsx`
around lines 353 - 361, Clear this.seriesNameById before registerSeriesNames in
updateXY so unchecked series cannot retain stale legend colors; keep this.colors
unchanged so cached allocations are preserved. In the empty-series early-return
setState branch, also reset legendColors to {}.
|
Almost there! this looks great! You need to enable the legend for the "total" or default fields. |
Done. I also updated the code Minor "Clear seriesNameById before re-registering to avoid stale legend colors for unchecked series." |
MatthewKhouzam
left a comment
MatthewKhouzam
left a comment
There was a problem hiding this comment.
Works here! thanks!
|
Fix the issues in linting please also you want to squash the commits and update the commit message? if you used coding assistants such as claude opus, please mention it in the commit message. |
2 participants
Summary
This pull request adds a legends column to the XY trace viewer to improve readability and make it easier to identify plotted series.
Changes
Testing
Summary by CodeRabbit