CODAP-1504: draw and hit test point shapes in the canvas renderer by kswenson · Pull Request #2696 · concord-consortium/codap

kswenson · 2026-09-09T01:02:46Z

Draws the seven point shapes in the canvas renderer and makes hit testing shape-aware. Third in the LEADS Point Shapes stack, on top of #2693.

Fixes CODAP-1504.

Geometry

data-display/renderer/point-shapes.ts is the single source of shape geometry for every surface that draws one — the canvas renderer today, the PIXI renderer and the legend keys later. Ported from the design prototype's js/shapes.js, whose tuned constants these are.

Circle is the reference and is unchanged from what CODAP has always drawn. Every other shape is normalized to about 90% of the circle's area rather than to equal area: straight edges and points read heavier than a circle of identical ink, so the small negative correction is what makes a triangle look like the same size point as a circle. Bounding boxes therefore differ between shapes, which is intentional.

The triangle is centered on its center of area rather than its bounding box. The prototype centers it on the box, which makes it sit h/6 low — visible as a downward jump when a category is switched to it. In a scatterplot position is the data, so every shape sits on its centroid.

Hit testing

A point's hit area is the drawn ink unioned with the circle of radius r that CODAP has always used, not the ink alone. A plus is narrower than a circle across its notches and a star between its arms, so testing the outline by itself would shrink the target for anyone who chose one — and shape is meant to be a free second encoding channel, not something you pay for in click accuracy. The union keeps every shape at least as easy to hit as a circle while adding the ink that extends past it: a star's tips, a square's corners, a triangle's apex.

This surfaced a latent defect in pointShapeBoundingRadius, which took half the larger side of the drawn box. That holds only while the box is centered on the point, and the triangle's is not: its apex sits at 1.47r while half its width is 1.27r, so a hit area sized from it stopped short of the apex. It now measures from the vertices. Its test had asserted that the radius covered half the extent, which the old implementation returned by construction — a tautology that no asymmetric shape could fail. That test now measures against the vertices too.

Marquee selection is unaffected: it is forwarded to the background element and never goes through the hit tester, and testing a marquee against point positions rather than outlines is what it should do.

Scope

Behind the pointShapes feature flag, which gates the controls: shapes can only be assigned by a
flag-enabled user. Rendering is deliberately not gated, so a document that already carries shapes
draws them for everyone rather than silently rendering as circles for a reader without the flag.
That follows the convention from CODAP-1448 of gating affordances rather than registrations or
rendering.

The canvas renderer is the fallback used when Graphics Acceleration is turned off in Options; the
PIXI/WebGL path is CODAP-1509.

One thing worth a reviewer's eye: getLegendShape is called per case inside the coordinate
refresh, which runs for every point on every reposition. With no legend it early-returns after
two lookups; with one it does work comparable to the getLegendColorForCase already in that
loop. Not measured.

Reviewing commit by commit is likely easier than the combined diff.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Copilot

🟡 Changes recommended

Shape inheritance, graph refresh behavior, hierarchy handling, and feature gating have unresolved correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds canvas-rendered point shapes, shape-aware hit testing, and propagation of shape styles through graphs and maps.

Changes:

Defines and tests geometry for seven point shapes.
Adds shape-aware canvas rendering and hit testing.
Propagates display/category shapes and updates inspector alignment.

File summaries

File	Description
`v3/src/models/data/category-set.ts`	Adds inherited shape fallback.
`v3/src/components/map/components/map-point-layer.tsx`	Refreshes map point shapes.
`v3/src/components/graph/utilities/graph-utils.ts`	Extends selection styling options.
`v3/src/components/graph/plots/scatter-plot/scatter-plot.tsx`	Supplies scatter-point shapes.
`v3/src/components/graph/plots/case-plot/case-plot.tsx`	Supplies case-plot shapes.
`v3/src/components/graph/models/graph-data-configuration-model.test.ts`	Tests inherited category shapes.
`v3/src/components/graph/hooks/use-plot.ts`	Reacts to graph shape changes.
`v3/src/components/graph/hooks/use-dot-plot.ts`	Supplies dot-plot shapes.
`v3/src/components/graph/hooks/use-chart-dots.ts`	Supplies chart-dot shapes.
`v3/src/components/data-display/renderer/point-shapes.ts`	Defines shared shape geometry.
`v3/src/components/data-display/renderer/point-shapes.test.ts`	Tests geometry and containment.
`v3/src/components/data-display/renderer/point-renderer-types.ts`	Adds shape to point styles.
`v3/src/components/data-display/renderer/canvas/canvas-hit-tester.ts`	Adds shape-aware hit testing.
`v3/src/components/data-display/renderer/canvas/canvas-hit-tester.test.ts`	Tests shaped hit regions.
`v3/src/components/data-display/renderer/canvas-point-renderer.ts`	Draws shapes on canvas.
`v3/src/components/data-display/models/data-configuration-model.ts`	Resolves per-case shapes.
`v3/src/components/data-display/inspector/point-shape-setting.tsx`	Updates comment terminology.
`v3/src/components/data-display/inspector/legend-color-controls.tsx`	Displays inherited shapes.
`v3/src/components/data-display/inspector/display-item-format-control.scss`	Aligns combined controls.
`v3/src/components/data-display/data-display-utils.ts`	Applies shapes during restyling.

Review details

Files reviewed: 20/20 changed files
Comments generated: 4
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

codecov · 2026-09-09T15:23:13Z

Codecov Report

❌ Patch coverage is 98.85057% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.89%. Comparing base (b1ec39b) to head (7c1af50).

Files with missing lines	Patch %	Lines
.../src/components/map/components/map-point-layer.tsx	71.42%	2 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##           leads-point-shapes    #2696       +/-   ##
=======================================================
+ Coverage               73.15%   87.89%   +14.74%     
=======================================================
  Files                     820      821        +1     
  Lines                   47342    47486      +144     
  Branches                12007    12135      +128     
=======================================================
+ Hits                    34632    41738     +7106     
+ Misses                  12674     5732     -6942     
+ Partials                   36       16       -20

Flag	Coverage Δ
cypress	`69.56% <41.31%> (+31.63%)`	⬆️
jest	`63.19% <76.43%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cypress · 2026-09-09T15:24:15Z

codap-v3 Run #12242

Run Properties: Passed #12242 • 7c1af50de1: null

Project	`codap-v3`
Branch Review	`CODAP-1504-point-shapes-canvas`
Run status	`Passed #12242`
Run duration	`08m 18s`
Commit	`7c1af50de1: null`
Committer	`null`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`1`
Pending	`82`
Skipped	`0`
Passing	`384`
View all changes introduced in this branch ↗︎

Copilot

🟡 Changes recommended

Newly added scatter-plot points can remain circles when created during performance mode, and several comments contradict the updated implementation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

v3/src/components/data-display/renderer/point-shapes.ts:188

Hit testing does not use this extent; it uses pointShapeBoundingRadius because the triangle and star extents are not centered on the point. The current wording conflicts with the immediately following contract and could lead a future caller to reintroduce the asymmetric-shape miss.

Files reviewed: 28/28 changed files
Comments generated: 4
Review effort level: Balanced

Copilot

🟡 Changes recommended

Explicit circle assignments are discarded by the existing v2 document importer, breaking reload persistence.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

v3/src/components/data-display/renderer/canvas-point-renderer.ts:589

The central renderer behavior added here is not covered by the existing canvas-point-renderer.test.ts: its drawing test only exercises the default circle/arc path, while the context mock does not even define moveTo, lineTo, or closePath. Add a renderer-level test for a non-circle style that verifies the polygon path is traced and filled/stroked; the geometry-unit tests alone cannot catch this integration path being omitted or wired incorrectly.

Files reviewed: 30/30 changed files
Comments generated: 1
Review effort level: Balanced

kswenson · 2026-09-09T17:31:16Z

Reviewing this stack — four PRs, each based on the one below it, merging bottom-up into leads-point-shapes:

PR	Story	What it is
#2691	CODAP-1506 (part 1)	Format palette restructure — structural, no shape behavior
#2692	CODAP-1505	Per-category shape storage — model only
#2693	CODAP-1506 (part 2)	The shape controls — UI
#2696	CODAP-1504	Canvas rendering and hit testing

CODAP-1506 is split across #2691 and #2693, so approving #2691 does not complete that story — it should not leave code review until #2693 is approved too. The other two map one-to-one.

Some things are introduced early and deliberately revised further up the stack. Rather than have you spend time on them twice, each PR notes what is superseded.

Nothing here is superseded — this is the top of the stack, and the spelling, inheritance and persistence revisions noted on #2692 and #2693 all land in this PR.

Worth your attention:

The tuned constants in point-shapes.ts. Shapes are normalized to ~90% of the circle's area rather than to equal area, because straight edges and points read heavier at equal ink. Please don't re-derive them; the relative visual weight of the whole set depends on them.
The hit area is the drawn ink unioned with the old circle of radius r. A plus is narrower than a circle across its notches and a star between its arms, so testing the ink alone would make a point harder to click purely because someone chose a shape.
Shape is threaded through both the paths that style a point and the ones that create it. That split is where four separate bugs lived during review, so it is the part most worth a skeptical read.
Rendering is deliberately not gated on the pointShapes flag, only the controls are — otherwise a document authored with shapes would draw as circles for every reader without the flag.

Commit by commit is likely easier than the combined diff.

Out of scope by design, each with a ticket: PIXI renderer (CODAP-1509), legend keys (CODAP-1507), residual plot (CODAP-1526).

One source of geometry for every surface that draws a shape: the canvas renderer, the PIXI renderer, and the legend keys. Nothing consumes it yet. Ported from the design prototype's js/shapes.js, which states the same contract -- "ONE source of geometry for every place a shape appears" -- and whose tuned constants these are. Do not re-derive them: the relative visual weight of the set depends on them, and equal area is deliberately not the target. Straight edges and points read heavier than a circle of identical ink, so every non-circular shape is normalized to about 90% of the circle's area. The circle stays an arc rather than a polygonal approximation, because approximating it would change how the shape CODAP already draws is rasterized. Callers switch on the returned kind. Alongside the outline, the module reports each shape's area, its drawn extent, and a bounding radius. Extent is not 2r for anything but the circle -- a star is about 35% wider than a circle of the same visual weight -- which is what hit testing needs rather than the radius. The tests check the constants against the reference table in the prototype's ASSET-SPEC.md, and separately check that each shape's declared area and extent agree with the vertices it actually draws, so the closed forms cannot drift away from the geometry. Two shapes are centered differently, and deliberately. The triangle is bounding-box centered so it shares a visual baseline with the square, which puts its center of area below the origin. The star sits on its circumcircle, which five-fold symmetry makes exactly its center of area, so only its bounding box is offset. Whether the triangle should change is raised on the story. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Connect the stored shape to the canvas renderer, so a graph or map using the canvas path draws the shape each legend category was assigned. Reachable through Options -> Graphics Acceleration Off, which is how this can be checked without dev tooling. Adds getLegendShapeForCase, mirroring getLegendColorForCase and deferred from CODAP-1505 as a rendering concern. It resolves categorical and checkbox legends only: numeric, date and color legends have no categories to assign a shape to, so they fall back to the display's own shape and every point in such a plot shares one. That mirrors how color behaves rather than inventing a shape scale. setPointSelection now fills in style.shape the way it already fills in the color, and the four callers -- both graph hooks, the case plot and the map point layer -- pass the display's shape for the no-legend case. Both new fields are optional: a caller that predates shapes keeps drawing circles rather than breaking. The renderer traces the outline once and shares that path between fill and stroke, and sets a round line join, without which the star's 36-degree points and the X's corners grow spikes at small radii under the default miter. Hit testing is unchanged and still circular, so a star's points and a triangle's corners are drawn outside the region that responds to a click. That is the next commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Assigning a shape updated the model but left the plot alone, so nothing appeared until something else forced a redraw. Legend color changes propagate through a reaction on legendColorDomain, which returns the category set's colorMap -- a computed whose identity changes when a color does. Shapes had no equivalent, so add legendShapeDomain over the shape map and give the graph and the map a reaction on it. The color reaction refreshes positions and masks, because a legend color change can also change which points are plotted. A shape change alters only how each point is drawn, so the graph restyles and the map refreshes points without touching the heatmap, which has no notion of shape. Cheaper, and it cannot perturb positions. Only categorical legends carry shapes, so legendShapeDomain is undefined for the rest and their displays never react. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Switching a category to the triangle visibly shifted its points down. An equilateral triangle's centroid sits h/6 below its bounding-box center -- 2.9px at r = 8, about 7.4px at r = 20 -- so centering the box puts the ink low. The prototype centers it on the box deliberately, for a predictable hit area and a shared baseline with the square. That trades a visible positional bias for an alignment nicety, and in a scatterplot position is the data. Confirmed by eye: the shift was obvious when switching between shapes, and is gone now. Every shape now sits on its center of area, which the tests assert as a single invariant rather than the previous split between box-centered and circumcircle-centered shapes. The bounding box is consequently off center for the triangle and the star, so anything deriving a hit area has to use the drawn box rather than assume symmetry about the point's position. Area and extent are unchanged, so the tuned constants and the reference table still hold. The menu and inspector icons are drawn from the exported SVG assets, which stay box-centered. A glyph centered in its slot is doing a different job from a point that has to sit on its data position, so the two are left to differ. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Comments and two test names only; no behavior change. Fixes centered, centering, center, color and behavior, which had crept into the shapes work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Two faults, both from building the legend case and assuming the rest followed. The scatter plot never passed pointShape to setPointSelection, so a plot with no legend attribute fell back to the default and drew circles whatever the user chose. It was missed because the search that found the other call sites was `setPointSelection({`, and this one wraps its argument onto the next line. With a legend the omission is invisible, since the category's shape wins over the fallback. The inspector's no-legend row spread its three children with space-between, because the rule that lets the label absorb the free space was scoped to the category rows. That rule now covers any row carrying both a shape and a color control, so the two rows line up with each other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Changing a shape left existing plots alone while new ones picked it up. There are two refresh paths and only one writes the shape. setPointSelection sets it; setPointCoordinates and the map's refreshPoints set the radius, fill and stroke but not the shape. Style updates merge, so refreshing through the latter after a shape change leaves the old shape in place -- and points created afterwards look correct, which is why new plots appeared to work. The display's own shape was being observed alongside the other point properties, whose reaction repositions. It now sits with the per-category shapes in the reaction that restyles, in both the graph and the map. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Adding a legend to a plot that already had a shape reverted its points to circles, discarding what the user chose. A category with no shape of its own now inherits the display's. Shape had been mirroring color, where a legend taking over is the point: colors must vary by category, and CODAP assigns distinct palette colors automatically. There is no shape palette, so every category defaulted to the same circle and the handover gave nothing back. Absence already means "not chosen" in the stored shapes, so reading it as "inherit" needs no new state. The inspector's dropdown shows the inherited shape, or the control would read Circle over a plot drawing stars. Not specified by the design; the findings cover where shape lives but not this transition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

pointShapeBoundingRadius took half of the larger side of the drawn box, which holds only while the box is centered on the point. The triangle is centered on its centroid, so its box hangs low: the apex sits at 2h/3 = 1.47r while half the width is 1.27r. Anything sizing a hit area from it stopped short of the apex. Measured from the vertices instead, which cannot disagree with what is drawn. Its test asserted the radius covered half the extent, which is what the old implementation returned by construction -- true of any value large enough for a symmetric shape, and no constraint at all on an asymmetric one. It now measures against the vertices, with the triangle called out. isPointInShape answers whether a click is on a point. It tests the ink unioned with the circle of radius r, rather than the ink alone: a plus is narrower than a circle across its notches and a star between its arms, so testing the outline by itself would shrink the target for anyone who chose one of them. Shape is a second encoding channel, and picking one should not cost you a point that is harder to click. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

The hit tester filed every point in the spatial grid by its radius and answered containment with a distance check, so a click on a star's tip or a triangle's apex fell outside the point that was drawn under it. Entries now carry the shape. The grid files a point by the shape's reach, since the drawn ink extends past the radius for everything but a circle, and containment defers to the geometry. A style with no shape is a circle and hits exactly as it did. The grid change is not observable at the default cell size: hitTest already searches a cell either side of the query, and that margin is far wider than any shape's overhang at the radii points are drawn at today. The cell size is a constructor argument and points scale with the size slider, so its test tunes the cell size down to let the grid rather than the margin decide. Marquee selection is unaffected -- it is forwarded to the background element and does not go through the hit tester. Testing a marquee against point positions rather than outlines is also what it should do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

The dot plot captured the point shape at render time and closed over it in refreshPointSelection. The shape reaction runs when the model changes, which is before the re-render that would rebuild that callback, so the restyle drew the appearance the plot had before the change: with no legend the first change appeared to do nothing and every later one showed the change before it. Nothing corrected the stale draw, since the other caller of refreshPointSelection returns early unless the renderer itself changed. Read from the model inside the callback, as the case plot, scatter plot and chart dots already do -- the dot plot was the one that did not. Point color and stroke were captured the same way and are read there now too. They never showed the lag because their reaction goes through a debounced callback that runs after the re-render, which is also why the pattern looked safe to follow. Deferring the reaction to the next frame would have hidden this as well, but only because the re-render happens to land first. With a legend the shape is resolved per case inside setPointSelection, from state read at call time, so only the no-legend case could go stale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Absence in the shapes map used to mean the default, and the setter wrote that meaning by deleting the entry for a circle. Since a category with no shape of its own now inherits the display's, absence means "inherit" instead, and the two readings disagree: with a display shape of star, choosing Circle for one category deleted its entry and the getter immediately returned star again, so circle could not be selected at all. Every choice is stored now, circle included. A category that has never been assigned is still absent, so exports stay empty until a user picks something. Two tests asserted the old meaning and encoded the bug with it: one expected the entry to disappear, the other expected a provisional category set not to be promoted for it. An explicit circle has to survive a reload like any other choice, so it is worth persisting and the promotion is correct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

getLegendShapeForCase said it mirrored getLegendColorForCase and did not: the color path refuses a legend attribute from a collection more childmost than the plotted cases, because such a point stands for several children at once and getStrValue would resolve through an arbitrary one of them, attributing that child's shape to the whole group. The guard was a closure inside the color path. It is now a view both share rather than a second copy that can drift from the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Shape reached points only through the selection-restyle path, but that is not the path that creates them. Adding a case to a plot with shapes assigned ran matchCirclesToData and refreshPointPositions, neither of which carried a shape, so the new point was drawn as a circle until something unrelated restyled it. setPointCoordinates takes a getLegendShape resolver alongside the getLegendColor it already had, and the seven graph plots and the map layer supply one. Shape is meant to be a second encoding channel beside color, so it is carried the same way color is rather than by a separate mechanism. Resolved inside the refresh callback rather than at render scope, so it reads current state whenever the callback runs. setPointCoordinates had no tests. These are the first, covering the resolver being applied per case and callers that supply none leaving the style untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

matchCirclesToData creates points from a default style that carried the display's color but not its shape, so every point was born a circle. Three of the callers create points without refreshing in the same breath, relying on the resulting case-data change to trigger a refresh later, so the shape arrived only on a subsequent pass. The default style now carries the display's shape exactly as it carries the display's color. The per-case shape still comes from the refresh that follows; this is only what the point is drawn as until it does. Found by auditing every path that creates or styles a point, after this class of bug had turned up three times in review. matchCirclesToData had no tests for the style it creates points with; these are the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Extracting the childmost-collection guard into a shared view left both callers reaching it through `this`, which broke legend colors on every graph: the plots hand these views around as detached function references -- a plot assigns dataConfig.getLegendColorForCase to a local and setPointCoordinates calls it bare -- so `this` inside them is undefined and the lookup threw. The guard moves one views block earlier so both callers can reach it through `self`, which is bound however the function is called. The unit tests missed this because they call the views on the instance, where `this` is fine. The Cypress smoke tests caught it. Covered now by a test that calls both views the way the plots do, through a detached reference. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

testing-library/render-result-naming-convention treats makeRenderer() as a testing-library render and demands its result be named view or utils. It is a stub point renderer with no relation to render(), so the helper is renamed rather than the variable, which would have made the test read as something it is not. Also drops an assertion on a parameter that is already `any`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Three comments still described the arrangement from before shape was carried through the coordinate path. The graph and map reactions each claimed the restyle path was the only one that writes a shape. That was the reason they were written, and it stopped being true when setPointCoordinates and the map's refreshPoints began writing one. The restyle path is still right, but because it is cheaper, not because it is the only option -- so the comments now say that instead. setShapeForCategory carried two contradictory comments at once: the new one explaining that every choice is stored, stacked directly under the old one saying a circle is dropped and round-trips as unset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Storing every choice in setShapeForCategory fixed the model but not the v2 path: the importer independently filtered circles out, so the exporter wrote one and the importer threw it away. A document with a display shape of star and a category deliberately set to circle came back with that category showing star -- the exact bug storing the choice was meant to remove, still alive across a reload. The importer keeps any recognized shape now. Its comment already said every entry is a deliberate user assignment, which the filter contradicted; it now says why a circle counts as one. The test asserting the old behavior is rewritten, and a round-trip test covers the whole path, since export and import each decide separately what is worth keeping and agreeing at one end proves nothing about the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

The geometry module was unit-tested and the renderer's use of it was not, so deleting the shape branch from the draw path left every test passing while plots silently drew circles. The context mock did not even define moveTo, lineTo or closePath, so nothing could have exercised the polygon path. Two tests: a non-circle traces and closes a polygon rather than an arc, and the number of lines traced follows the shape's vertex count, which tells the shapes apart rather than only telling polygons from circles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH

Drop another claim that is not true. The note on the drawn extent said hit testing needs it rather than the radius, but hit testing uses the bounding radius; the extent has no caller outside the tests here. It now says what it is for. The graph and the map explained their shape reactions in nearly the same twelve lines. All each needed was its own reason for restyling rather than repositioning -- masks on one, the heatmap on the other. The rest was shared boilerplate, and the claim that both shape sources are observed only restated the reaction's own dependencies. That only a categorical legend carries shapes was stated on two adjacent views, at more length on the second. The header quoted the prototype's slogan back at itself, and the note on centering the triangle ended by restating its first sentence. The placement of the childmost-collection guard keeps its explanation. It is what stops someone moving it back in beside its callers, which is what broke legend colors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The last of mine, in a comment that had already merged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

kswenson added the v3 CODAP v3 label Sep 9, 2026

kswenson requested a balanced review from Copilot September 9, 2026 01:09

Copilot started reviewing on behalf of kswenson September 9, 2026 01:10 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread v3/src/components/data-display/models/data-configuration-model.ts

Comment thread v3/src/components/data-display/renderer/canvas-point-renderer.ts

Comment thread v3/src/components/graph/hooks/use-plot.ts Outdated

Comment thread v3/src/models/data/category-set.ts

github-actions Bot temporarily deployed to development September 9, 2026 15:23 Inactive

kswenson requested a balanced review from Copilot September 9, 2026 15:51

Copilot started reviewing on behalf of kswenson September 9, 2026 15:52 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread v3/src/components/graph/plots/scatter-plot/scatter-plot.tsx

Comment thread v3/src/components/graph/hooks/use-plot.ts Outdated

Comment thread v3/src/components/map/components/map-point-layer.tsx Outdated

Comment thread v3/src/models/data/category-set.ts Outdated

kswenson requested a balanced review from Copilot September 9, 2026 16:31

Copilot started reviewing on behalf of kswenson September 9, 2026 16:31 View session

github-actions Bot temporarily deployed to development September 9, 2026 16:32 Inactive

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread v3/src/models/data/category-set.ts

github-actions Bot temporarily deployed to development September 9, 2026 16:53 Inactive

kswenson added the run regression label Sep 9, 2026

kswenson marked this pull request as ready for review September 9, 2026 17:10

This was referenced Sep 9, 2026

CODAP-1506: restructure the graph Format palette into labelled sections #2691

Merged

CODAP-1505: store point shape per legend category #2692

Merged

kswenson mentioned this pull request Sep 9, 2026

CODAP-1506: add per-category point shape controls #2693

Merged

kswenson requested a review from tealefristoe September 9, 2026 20:18

kswenson force-pushed the CODAP-1504-point-shapes-canvas branch from 2dc723c to 5a11158 Compare September 10, 2026 00:44

github-actions Bot temporarily deployed to development September 10, 2026 00:48 Inactive

kswenson force-pushed the CODAP-1504-point-shapes-canvas branch from 5a11158 to 57a6bb0 Compare September 10, 2026 12:37

kswenson mentioned this pull request Sep 10, 2026

CODAP-1509: draw and hit test point shapes in the PIXI renderer #2697

Open

github-actions Bot temporarily deployed to development September 10, 2026 12:43 Inactive

This was referenced Sep 10, 2026

CODAP-1507: draw legend keys as the category's point shape #2698

Open

CODAP-1526: draw residual points with the category's shape #2699

Open

github-actions Bot temporarily deployed to development September 10, 2026 15:45 Inactive

Base automatically changed from CODAP-1506-shape-controls to leads-point-shapes September 11, 2026 02:58

kswenson and others added 22 commits September 10, 2026 20:02

CODAP-1506: use US spelling in a vars comment

7c1af50

The last of mine, in a comment that had already merged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

kswenson force-pushed the CODAP-1504-point-shapes-canvas branch from 3f399a2 to 7c1af50 Compare September 11, 2026 03:08

github-actions Bot temporarily deployed to development September 11, 2026 03:12 Inactive

Navigation Menu

Uh oh!

CODAP-1504: draw and hit test point shapes in the canvas renderer - #2696

CODAP-1504: draw and hit test point shapes in the canvas renderer#2696
kswenson wants to merge 22 commits into
leads-point-shapesfrom
CODAP-1504-point-shapes-canvas

kswenson commented Sep 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 9, 2026 •

edited

Loading

Uh oh!

cypress Bot commented Sep 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

kswenson commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

kswenson commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Geometry

Hit testing

Scope

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cypress Bot commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

codap-v3 Run #12242

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

kswenson commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kswenson commented Sep 9, 2026 •

edited

Loading

codecov Bot commented Sep 9, 2026 •

edited

Loading

cypress Bot commented Sep 9, 2026 •

edited

Loading