CODAP-1505: store point shape per legend category - #2692
Open
kswenson wants to merge 3 commits into
Open
Conversation
Add the model layer for point shapes: each legend category can carry its own shape, and a display with no legend attribute carries a single one. Nothing reads these yet -- the controls come with CODAP-1506 and the rendering with CODAP-1504. Per-category shape lives on the attribute's CategorySet, next to per-category color, so two displays using the same legend attribute agree by construction. The single shape lives on DisplayItemDescriptionModel next to point color. That pairing mirrors color exactly, which is what the existing per-category color setter already established. The default is stored as absence: assigning circle removes the entry rather than recording it. A document therefore carries only the shapes a user chose, and one that never used the feature is byte-for-byte unaffected. V2 round trip ------------- V2 has no field for point shape, so both halves travel in v3 extension namespaces: per-category shapes on the attribute, the single shape on the graph component. Both survive a v3 -> v2 -> v3 round trip. A v2 re-save drops them, which is lossy but harmless. They deliberately do NOT ride in _categoryMap, which would be the obvious home next to colors. V2's updateCategoryMap treats every key of that map except __order, stroke-color and stroke-transparency as a category: it appends unknown keys to __order and writes the result back out. A shapes key there would appear as a phantom category in the user's legend and be persisted by v2 itself. An unknown key in the attribute's v3 namespace is merely mixed onto the SproutCore model and dropped by its toArchive allowlist, so v2 is unaffected. A test pins this, since the safe-looking change is the harmful one. Imported shapes are not filtered against the categories present in the data, and so differ from colors, which are. Every shape entry is a deliberate user assignment, whereas some paths assign colors automatically by category position -- which is why the color import keeps only a color differing from the one that position would have produced. Shapes have no generated noise to age out, so a category whose cases are deleted and later restored keeps its shape. V2 does the same, for the reason its own comment gives. Provisional promotion --------------------- A category set stays provisional until the user changes something that has to persist, at which point it is promoted into the document. Shapes join moves and colors as such a change: assigning a shape can be the only thing a user does, so the promotion predicate has to count it for the shape to be saved at all. Also remove CategorySet.userActionNames, which named the changes that promote a set but was read by nothing. Its test asserted against the list rather than the behavior; the equivalent coverage now runs against the promotion predicate itself, where moves, colors and shapes are checked together. Document that promotion replaces the category-set instance, so a reference held across a modification goes stale and later writes are silently lost. The per-category setters re-resolve for this reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## CODAP-1506-format-panel-restructure #2692 +/- ##
=======================================================================
- Coverage 87.89% 87.85% -0.05%
=======================================================================
Files 818 819 +1
Lines 47231 47279 +48
Branches 11948 12085 +137
=======================================================================
+ Hits 41513 41536 +23
- Misses 5702 5729 +27
+ Partials 16 14 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
codap-v3
 |
||||||||||||||||||||||||||||
| Project |
codap-v3
|
| Branch Review |
CODAP-1505-per-category-point-shape
|
| Run status |
|
| Run duration | 08m 28s |
| Commit |
|
| Committer | null |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
82
|
 |
0
|
 |
384
|
| View all changes introduced in this branch ↗︎ | |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Default shape fields alter untouched V3 snapshots, and reserved category names can lose shapes during V2 round trips.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds model-layer storage and V2 round-trip support for point shapes.
Changes:
- Adds point-shape types, defaults, and validation.
- Stores shapes globally or per legend category.
- Adds V2 extensions, notifications, undo strings, and tests.
File summaries
| File | Description |
|---|---|
v3/src/v2/v2-document-round-trip.test.ts |
Tests category-shape round trips. |
v3/src/v2/codap-v2-types.ts |
Adds graph shape extension. |
v3/src/v2/codap-v2-type-utils.ts |
Exports non-default graph shapes. |
v3/src/v2/codap-v2-data-set-importer.ts |
Imports attribute shape maps. |
v3/src/v2/codap-v2-data-context-types.ts |
Types category-shape extensions. |
v3/src/utilities/translation/lang/en-US.json5 |
Adds undo/redo strings. |
v3/src/utilities/point-shape-utils.ts |
Defines supported shapes. |
v3/src/utilities/point-shape-utils.test.ts |
Tests shape utilities. |
v3/src/models/shared/data-set-metadata.ts |
Promotes shape-modified category sets. |
v3/src/models/shared/data-set-metadata.test.ts |
Tests provisional promotion. |
v3/src/models/data/v2-category-set-importer.ts |
Restores validated category shapes. |
v3/src/models/data/v2-category-set-importer.test.ts |
Tests shape imports. |
v3/src/models/data/category-set.ts |
Stores per-category shapes. |
v3/src/models/data/category-set.test.ts |
Tests category shape behavior. |
v3/src/data-interactive/point-shape-v2-export.test.ts |
Tests attribute V2 export. |
v3/src/data-interactive/data-interactive-type-utils.ts |
Exports attribute shape maps. |
v3/src/components/graph/v2-graph-importer.ts |
Imports graph point shapes. |
v3/src/components/graph/v2-graph-exporter.ts |
Exports graph point shapes. |
v3/src/components/graph/v2-graph-exporter.test.ts |
Tests graph shape round trips. |
v3/src/components/graph/models/graph-data-configuration-model.test.ts |
Tests legend shape accessors. |
v3/src/components/data-display/models/display-item-description-model.ts |
Stores global point shape. |
v3/src/components/data-display/models/display-item-description-model.test.ts |
Tests global shape storage. |
v3/src/components/data-display/models/data-configuration-model.ts |
Adds legend shape accessors. |
v3/src/components/data-display/data-display-notifications.ts |
Adds shape-change notifications. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Store the display's shape as absence at the default. types.optional with a
default materializes the field in every snapshot, so opening and saving a
document that never used shapes would change it. maybe leaves it absent, the
getter already resolves that to circle, and the setter now clears it when the
default is chosen -- the same absence-means-default rule the per-category
shapes already follow.
Build the exported and imported shape records with Object.fromEntries rather
than by assignment. A category value of `__proto__` assigned with `map[key] =`
sets the prototype instead of defining an own property, so that category's shape
vanished in both directions. Category values come from the data and can be any
string. Note colorMap has the same pattern and the same latent problem; left
alone as pre-existing rather than widened into this story.
Add tests for changePointShapeNotification, which the ten adjacent notification
helpers already have and this one did not.
The remaining comment is declined: the shapes map does materialize as `{}` in
every CategorySet snapshot, but so does the colors map beside it, so the
snapshot was never unaffected by an empty map. Omitting it needs a
snapshotProcessor and would make shapes behave differently from its sibling.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH
Category values come from the data and can be any string, including ones that mean something to a plain JavaScript object. Two faults, both pre-existing, found while fixing the same class of bug for shapes. Writing: `map["__proto__"] = color` sets the prototype rather than defining an own property, so that category's color vanished from colorMap and from the v2 export built out of it. Reading: for a category named `constructor` or `toString` with no entry, a bare lookup returns the inherited member. That is truthy and is not a color, so the v2 importer dereferenced `.colorString` to undefined and handed it to colord -- inventing a color for a category the document said nothing about. colorMap is now null-prototyped and built from entries, which closes both. The importer guards its lookup with hasOwnProperty and collects entries rather than assigning into an object. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the LEADS point-shapes feature. This is the model layer only — nothing
reads these values yet. The controls come with CODAP-1506 and the rendering with
CODAP-1504.
Stacked on #2691. Base is
CODAP-1506-format-panel-restructure, so reviewthat one first. Once it merges I'll retarget this to
leads-point-shapesandthe diff will shrink to just this story.
Where shape lives
Per-category shape goes on the attribute's
CategorySet, next to per-categorycolor, so two displays using the same legend attribute agree by construction.
The single no-legend shape goes on
DisplayItemDescriptionModel, next to pointcolor. That mirrors how color already works.
The default is stored as absence — assigning
circleremoves the entry ratherthan recording it. A document carries only the shapes a user chose, and one that
never used the feature is byte-for-byte unaffected.
The v2 decision worth reviewing
V2 has no field for point shape, so both halves ride v3 extension namespaces:
per-category on the attribute, the single shape on the graph component. Both
survive a v3 → v2 → v3 round trip; a v2 re-save drops them, which is lossy but
harmless.
They deliberately do not ride in
_categoryMap, which looks like theobvious home next to colors. I checked V2 on
masterbefore ruling it out —attribute_model.js:486:updateCategoryMaptreats every other key as a category: it appends unknownkeys to
__order, thentoArchivewrites the result back. A shapes key therewould surface as a phantom category in the user's legend and be persisted by
v2 itself. By contrast an unknown key in the attribute's
v3namespace ismerely mixed onto the SproutCore model by
DG.Attribute.createand dropped bytoArchive's allowlist, so v2 is unaffected.There's a test pinning this, because the harmful change is the one that looks
safe.
Imported shapes are not filtered, unlike colors
Colors are filtered to categories present in the data; shapes are not. The
difference is that every shape entry is a deliberate user assignment, whereas
some paths assign colors automatically by category position — which is why the
color import keeps only a color differing from the one that position would have
produced. Shapes have no generated noise to age out, so a category whose cases
are deleted and later restored keeps its shape. V2 does the same, for the reason
its own comment gives.
Provisional promotion
A category set stays provisional until the user changes something that must
persist. The predicate watched moves and colors; shapes join it, since assigning
a shape can be the only thing a user does.
This also removes
CategorySet.userActionNames, which named the promotingchanges but was read by nothing. Its test asserted against the list rather than
the behavior; equivalent coverage now runs against the promotion predicate
itself, where moves, colors and shapes are checked together.
Promotion rebuilds the set from a snapshot and so replaces the instance — a
reference held across a modification goes stale and later writes are silently
lost. That's now documented at both
getCategorySetandpromoteProvisionalCategorySet; the per-category setters re-resolve for thisreason.
Two deviations from the ACs
V3.notDG.The AC specifiesDG.Undo.graph.changePointShape, butDG.*is for strings inherited from v2and shape is new in v3, so these are
V3.Undo/Redo.graph.changePointShape.getLegendShapeForCaseis not here. The per-case resolver mirroringgetLegendColorForCaseis a rendering concern; CODAP-1504 adds it where it'sused.
Testing
29 new tests. The full-document round-trip tests are the ones that earned their
keep — they caught the promotion gap that every unit test missed, because unit
tests read back through the same in-memory object.
Mutation-tested; each of these fails at least one test: writing shapes into
_categoryMap, filtering imported shapes to present categories, acceptingunrecognized shape values, storing the default instead of omitting it, dropping
shapes from the promotion predicate, dropping
movesfrom it, and having thegraph importer ignore
v3.pointShape.npm run build:tsc,npm run lint, and the full suite (3805 tests / 357suites) all pass.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH