Conversation
A legend key selects cases elsewhere rather than being selected itself, but plain click should still replace the selection rather than toggle it, matching V2, point selection, and Power BI/Vega-Lite. The real gap was that the modifier click could only ever add a category, never remove one. Adds hasSelectionModifier and preservesSelection to platform-utils so every click handler asks one of two questions in one place, and converts the three background-deselect guards that already spelled out preservesSelection's expression. The two differ deliberately: acting on a selection is precise, clearing one is conservative. Also removes a stale comment claiming a commented-out setOrExtendSelection call was a reverted toggle. It was functionally identical to the code that replaced it; what broke the Cypress test was routing selection through applyModelChange, which 34b50cc later did anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2703 +/- ##
==========================================
+ Coverage 88.02% 88.03% +0.01%
==========================================
Files 822 822
Lines 47657 47668 +11
Branches 12071 12166 +95
==========================================
+ Hits 41950 41965 +15
+ Misses 5691 5689 -2
+ 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:
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation is consistent with existing selection semantics and includes comprehensive platform-aware tests.
Pull request overview
Centralizes modifier-key handling and enables categorical legend categories to be toggled with platform-appropriate modifier clicks.
Changes:
- Adds shared selection-modifier utilities with unit coverage.
- Toggles categorical legend selections on Shift/Cmd/Ctrl clicks.
- Reuses centralized selection-preservation logic and adds Cypress coverage.
File summaries
| File | Description |
|---|---|
v3/src/utilities/platform-utils.ts |
Adds modifier-key helpers. |
v3/src/utilities/platform-utils.test.ts |
Tests platform-specific behavior. |
v3/src/components/map/hooks/use-map-click-deselect.ts |
Centralizes map deselection checks. |
v3/src/components/graph/plots/scatter-plot/use-residual-marquee.ts |
Centralizes residual-plot deselection checks. |
v3/src/components/data-display/hooks/use-renderer-pointer-down-deselect.ts |
Centralizes renderer deselection checks. |
v3/src/components/data-display/components/legend/categorical-legend.tsx |
Implements modifier-click toggling. |
v3/cypress/support/helpers/graph-legend-helper.ts |
Adds modifier-click test helpers. |
v3/cypress/e2e/graph-legend.spec.ts |
Covers replacement and toggling behavior. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
codap-v3
 |
||||||||||||||||||||||||||||
| Project |
codap-v3
|
| Branch Review |
CODAP-1508-legend-selection-toggle
|
| Run status |
|
| Run duration | 07m 49s |
| Commit |
|
| Committer | null |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
1
|
 |
82
|
 |
0
|
 |
391
|
| View all changes introduced in this branch ↗︎ | |
Two tests named "will select and unselect ..." had their unselect half commented out, under a note guessing that background-click deselection was broken. It is not: useRendererPointerDown deselects only when event.target is the renderer's canvas, and the helper clicked the SVG plot background layered beneath it. The `force` flag was the tell -- it was there to bypass the actionability check that would have reported the rect as covered, which made the helper simulate a click no user can perform. Repointing the helper at the canvas makes both assertions pass, so the tests now test what their names claim. Verified in the browser first: clicking the canvas clears the selection, clicking the rect does nothing. The map specs have four more sites commented out the same way, but the map clears through use-map-click-deselect and Leaflet rather than the renderer canvas, so that is a separate diagnosis. Noted on CODAP-1533. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The behavior is coherent and well tested; the remaining feedback concerns non-blocking documentation accuracy.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
v3/src/utilities/platform-utils.ts:32
- This says every caller uses the helper, but nominal selection handlers such as
data-display-utils.ts:60andmap-point-grid.tsx:57still inspect modifier flags directly. Please either migrate those callers or phrase this as guidance so the API documentation reflects the current code.
v3/src/utilities/platform-utils.ts:42 - The “any modifier at all” wording contradicts the implementation and its tests, which intentionally ignore
altKey. Naming the three preserving modifiers avoids misleading future callers.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Copilot flagged both. "Every caller routes through here" stated an aspiration as fact -- several graph, map, and case table handlers still read modifier flags directly. And "any modifier at all" contradicted both the implementation and platform-utils.test.ts, which deliberately exclude altKey. Comments only; no behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Summary
Clicking a legend key a second time now leaves the selection alone, as it always has; the modifier click is what changed. It toggles a category in and out of the selection instead of only ever adding it.
The story asked for the opposite — plain click should toggle — but that premise didn't survive checking. V2's
selectCasesInCellreplaces on plain click and never deselects, and v3'shandleClickOnCasehas an explicit branch for an already-selected case that deliberately does nothing unless a modifier is down. Legends that toggle on plain click elsewhere in the industry (Plotly, Highcharts, Chart.js) are toggling series visibility, which has no competing replace operation; legends that drive selection of other things (Power BI, Vega-Lite's legend binding) all replace on plain click and toggle on a modifier. The deciding argument is that there are three intents — replace, adjust, clear — wanting three gestures, and toggling on plain click eliminates "replace" rather than adding anything. Full reasoning is in the Jira story.The real defect was that
selectCases(caseIds, dataset)omitted its third argument, soselectdefaulted totrueand a modifier click could only add a category, never remove one. Once several categories were selected there was no way to take one back out — which is the actual gap behind the report's "there is no way to clear a selection from the legend."Modifier handling is now centralized
src/utilities/platform-utils.tsgains two functions, and they are the only two questions a click handler should ask:hasSelectionModifier(event)— shift or the platform command key. For paths that act on a selection.preservesSelection(event)— any of shift/cmd/ctrl. For paths that would clear one.They differ deliberately. Acting on a selection should be precise, so
hasSelectionModifierexcludes ctrl on a Mac, where it is the secondary-click gesture. Clearing one should be conservative, sopreservesSelectiontreats any modifier as reason not to destroy the user's work. The command key is platform-exclusive — cmd on a Mac, ctrl everywhere else — which matters given how much of our audience is on Chromebooks and Windows.Centralizing this immediately caught something that would otherwise have spread:
altcan't be a selection modifier, because option-click already zooms the plot background and rescales axes. The case table's extend check includes it today.Three background-deselect guards were already spelling out the same expression as
preservesSelectionand now call it instead — no behavior change, but it makes the utility a real single source of truth rather than a helper used once.Also
Removes a stale comment claiming a commented-out
setOrExtendSelectioncall was a reverted toggle. It was functionally identical to the code that replaced it, in Jan 2025 as well as today. What broke the Cypress test back then was routing selection throughapplyModelChange, which shifted the undo counts in a spec built on long undo/redo sequences — and34b50cc3clater moved it onto the notification path anyway.Testing
platform-utils.test.tsis new: 12 tests, with isolated module loading per platform sinceisMacis evaluated at import.graph-legend.spec.ts, platform-aware viaCypress.platformso the same spec asserts cmd-toggles/ctrl-doesn't on macOS and ctrl-toggles/cmd-doesn't on CI's Linux.graph-legend.spec.tspass locally, including the pre-existing selection test that the stale comment blamed.hasSelectionModifierpermissive fails 3 unit tests; collapsingpreservesSelectioninto the exclusive form fails the test encoding the asymmetry; reverting the legend handler to add-only fails both Cypress toggle tests.Follow-up
A consistency sweep is filed separately as CODAP-1533: bars, box-plot ranges, map grid cells, and the map pin layer have the same add-only gap, and the case table's extend check omits
ctrlKeywhile includingaltKey. Ctrl-click multi-select in the case table may well be broken for Windows and Chromebook users today — that is read from the code and not yet confirmed in a browser.Fixes CODAP-1508
🤖 Generated with Claude Code