Skip to content

feat(scatterplot): show zoomed-in view indicator - #403

Open
FlorinSenoner wants to merge 5 commits into
mainfrom
feat/343-zoom-indicator
Open

feat(scatterplot): show zoomed-in view indicator#403
FlorinSenoner wants to merge 5 commits into
mainfrom
feat/343-zoom-indicator

Conversation

@FlorinSenoner

Copy link
Copy Markdown
Collaborator

Summary

  • Show Zoomed in in the existing point-count chip whenever the scatterplot scale is greater than 1.
  • Clear the marker at identity, reset, and zoomed-out scales while keeping the full D3 transform non-reactive.
  • Add component-level and real-browser regression coverage.

Root cause

The scatterplot intentionally stored the full D3 transform as plain state to avoid a Lit render on every pan/zoom frame. The template therefore had no reactive representation of the zoom boundary, so users received no persistent indication that the view was magnified.

Reproduction

  1. Open the Explore view with the sample dataset.
  2. Wheel over the scatterplot.
  3. Observe the D3 transform scale rise from 1 to approximately 2.297 while the UI still shows only 7831 points.
  4. Double-click the plot to reset the transform to identity.

Verification

  • Focused Vitest regression: 1 passed.
  • Playwright zoom/reset regression: 1 passed.
  • Full workspace test suite: 321 utils + 1386 core + 165 app tests passed; 1 app test skipped.
  • pnpm precommit: passed immediately before push.
  • Manual browser verification confirmed the marker appears after wheel zoom and disappears after double-click reset.

Closes #343

@FlorinSenoner FlorinSenoner linked an issue Aug 1, 2026 that may be closed by this pull request
1 task
@FlorinSenoner
FlorinSenoner marked this pull request as ready for review August 1, 2026 20:40
@tsenoner

tsenoner commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Automated review

Does it solve #343? Yes — the PR derives a reactive _isZoomedIn boolean in the scatterplot's onTransform callback (t.k > 1) and renders a Zoomed in marker in the existing bottom-left point-count chip whenever the view scale exceeds identity. Every reset path (dblclick, data swap, isolate/un-isolate, public resetZoom()) funnels through PlotInteractionController.resetZoom(), whose d3 transition substitutes the exact target transform on its final tick, so the marker reliably clears at identity, and _transform is written only at init and in onTransform, so the boolean cannot go stale. Panning at identity and zooming out below identity intentionally show nothing, which design.md declares a non-goal. What remains is polish, not function.

Found 2 issues:

  1. The chip renders as N points ·Zoomed in — two spaces before the separator and none after — instead of the intended N points · Zoomed in. .plot-indicator is display: flex with no gap (scatter-plot.styles.ts:136-156), so the text run and the <span> become separate anonymous flex items: the trailing space after · sits at the end of a line box and is dropped, while the collapsed newline before the binding lands next to the non-collapsible &nbsp;. Reproduced in Chromium — flex gives 149 points ·\nZoomed in (span at 66.84px), the same markup with display:block gives 149 points · Zoomed in (69.70px). Neither test catches it: the vitest helper normalises via .replace(/\s+/g, ' ') and the Playwright assertion is scoped to the span. The sibling .mode-indicator sets gap: 0.25rem for exactly this case. Simplest fix: emit one text run, or move · into the span and add the gap.

? html`
<div class="plot-indicator">
${this._getVisiblePointCount()} points
${this._isZoomedIn
? html`&nbsp;· <span class="zoom-indicator">Zoomed in</span>`
: ''}
</div>

  1. The manual isZoomedIn !== this._isZoomedIn guard duplicates dedupe that Lit's @state setter already performs, and the new test asserts the guard rather than the spec'd behaviour. _isZoomedIn is @state() with no custom hasChanged, so requestUpdate returns early on an equal write (reactive-element 2.1.2, development/reactive-element.js:708-725); @state sets attribute: false and no reflect/useDefault, so the corner-case disjunct cannot fire. The four lines collapse to this._isZoomedIn = t.k > 1; with identical rendering. The coupling is the real cost: scatter-plot.zoom-indicator.test.ts:79-88 spies on requestUpdate and asserts call counts, so removing the redundant guard turns the test red despite unchanged behaviour. The spec delta says "does not schedule a new Lit update" — asserting on scheduled renders (_renderPlot spy or updated() count) would test the stated requirement and survive the simplification. No other @state write in this file uses such a guard.

resolveSlotsToIds: (slots) => this._slotsToInteractiveIds(slots),
onTransform: (t) => {
this._transform = t;
const isZoomedIn = t.k > 1;
if (isZoomedIn !== this._isZoomedIn) {
this._isZoomedIn = isZoomedIn;
}
this._connectorOverlay.updateZoomScale(t.k);
},
onSelect: (ids, clearVisual) => this._commitSelection(ids, clearVisual),

🤖 Generated with Claude Code

Reviewed at 56256d7 against issue #343.

@FlorinSenoner

Copy link
Copy Markdown
Collaborator Author

Independent triage against current head 56256d7dc83df3289363535a2546a39d89cbdbfd:

  1. Actionable — spacing/layout. Confirmed that .plot-indicator is flex with no gap. In headless Chrome, the exact text/span structure has a 0 px dot-to-label gap (versus 3.047 px in normal inline flow) and preserves two whitespace widths before the dot, so the rendered result does not match the OpenSpec form N points · Zoomed in. Required direction: keep the separator and label in one text/inline item, or use explicit flex items/gap without the extra &nbsp;; cover the full chip presentation rather than only normalized text/the nested span.

  2. Actionable — test contract; the guard itself is behaviorally redundant. Confirmed against the pinned @lit/reactive-element@2.1.2: the reactive setter calls requestUpdate, and an equal boolean makes requestUpdate return before enqueueing an update. The current guard therefore does not change scheduled renders, while the spy assertion observes method invocation rather than the spec requirement that no update be scheduled. Required direction: assert actual update/render scheduling, then simplify to direct assignment (or explicitly specify a stronger no-setter-call invariant if that is truly intended).

No code or PR state was changed as part of this triage.

@FlorinSenoner

Copy link
Copy Markdown
Collaborator Author

Implemented both review items in 09ecf535:

  • The chip now uses explicit point-count and · Zoomed in flex items with a 0.25rem gap; the Playwright regression verifies the full marker text and measured/computed spacing.
  • The transform bridge now assigns the derived boolean directly and the component regression observes actual _renderPlot scheduling, so repeated same-side transforms are covered without coupling to requestUpdate() calls.

Verification: focused Vitest 2/2, zoom-indicator Playwright 1/1, openspec validate show-zoom-indicator --strict, and pnpm precommit all passed.

${this.data
? html` <div class="plot-indicator">${this._getVisiblePointCount()} points</div> `
? html`
<div class="plot-indicator">

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Announce the zoom status change

The new indicator is visual-only: in Chromium, the accessibility snapshot exposes 7831 points and · Zoomed in as generic nodes, while this chip, its shadow-root container, and the host have no role or aria-live. Because wheel/reset changes this text without moving focus, screen-reader users receive no zoom-state notification. Please expose the chip as a polite status (the nearby .connector-status already uses role="status" aria-live="polite"; aria-atomic="true" may also be appropriate) and add an accessibility assertion for zoom and reset.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 732de70b. The persistent point-count chip now mirrors the existing connector-status pattern with role="status" aria-live="polite"; the status role also provides implicit atomic announcements. The component regression requires the polite status semantics, and Playwright now locates the chip by role, verifies aria-live, and checks zoom/reset content. Verified with focused Vitest 2/2, zoom-indicator Playwright 1/1, strict OpenSpec validation, and pnpm precommit.

@tsenoner

tsenoner commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Adversarial review

Reviewed in an isolated worktree by three independent lenses (code quality, adversarial correctness, issue-resolution audit), with every finding then put through a refuter whose default position was that it is a false positive. 6 raised, 5 survived refutation.

Cleanups considered but skipped
  • Fix 1 (packages/core/src/components/scatter-plot/scatter-plot.zoom-indicator.test.ts:21, delete the hoisted getContext stub) — SKIPPED: the fix's core premise is factually false. It asserts 'nothing calls HTMLCanvasElement.prototype.getContext at module-evaluation time', but packages/core/src/components/scatter-plot/webgl/color-utils.ts:7-16 has a module-level IIFE (COLOR_PARSER_CTX) that calls canvas.getContext('2d') at import time, and that module is transitively imported by import './scatter-plot'. The hoisted stub and the beforeEach stub therefore cover DISJOINT windows (import time vs test time) and are not duplicates — the beforeEach spy structurally cannot cover the import-time call. Proof: with line 21 deleted the run emits a jsdom "Not implemented: HTMLCanvasElement's getContext()" warning; unmodified it emits none. Deterministic across 3 runs (warning count 1,1,1 vs 0). Tests pass either way (2 passed) and COLOR_PARSER_CTX is null in both cases, so code-under-test behavior is identical — the only delta is added console noise. Noted in fairness to the fix: all three sibling files it cites as the target pattern (transform-reactivity, legend-reactivity, filter-render) already emit that same warning, so the noise would match repo norm rather than be a novel regression. Skipped anyway because it removes a load-bearing line on a wrong rationale, trading a silent test run for a noisy one to save one line.

Issue resolution — resolves the issue

The scoping is defensible and I verified the two load-bearing triage claims myself rather than
trusting the description.

  1. "Keeping the full D3 transform non-reactive" — correct and well handled. _transform really is a
    deliberate plain field (scatter-plot.ts:150-155, with a dedicated characterization test at scatter-
    plot.transform-reactivity.test.ts), and the PR derives a separate @state() boolean instead of
    promoting it. Lit's requestUpdate early-returns on an equal boolean, so consecutive k>1 frames
    schedule nothing; I confirmed the one crossing costs exactly one _renderPlot() (a single WebGL
    render, scatter-plot.ts:1372-1382) and that _getVisiblePointCount() is memoized, so the F-48
    invariant is genuinely preserved. The redundant manual guard flagged in the earlier review round was
    correctly removed in 09ecf53.
  2. "Clear the marker at identity, reset, and zoomed-out scales" — I verified this rather than
    assuming. onTransform is the sole writer of _transform/_isZoomedIn, and
    PlotInteractionController.applyZoom is its sole caller, reached from the d3 zoom handler and from
    resetZoom()'s transition to exact zoomIdentity. All five reset entry points (dblclick, public
    resetZoom(), data swap at line 758, isolate at 2126, un-isolate at 2216) go through it. The
    boolean therefore cannot go stale. The one theoretical staleness window — disconnectedCallback
    interrupting an in-flight reset transition (F-12) with no controller re-initialize on reconnect — is
    pre-existing and unreachable in the single-plot Explore page.
  3. The declared non-goals (zoom-out, pan-at-identity, numeric zoom %, reset button) are consistent
    with the literal issue text ("When zooming in an indicator should be shown that you are zoomed in").
    I checked that zoom-out is genuinely reachable (zoomExtent: [0.1, 1000]), so the exclusion is a
    real narrowing, not an impossibility — but it is a defensible reading of the issue, and design.md
    states it openly rather than hiding it. My only substantive quarrel is that proposal.md justifies
    the change partly by "visible confirmation that double-click reset is applicable" and then ships a
    pointer-events: none label that never mentions reset — the rationale and the deliverable are
    slightly out of step.
  4. Framing nit: the PR body's "Root cause" section describes F-48 as the root cause. F-48 is a
    constraint the implementation had to navigate, not the reason the feature was absent — this is an
    additive feature, not a regression. Harmless, but the section reads as if a bug was diagnosed.
    "Closes [FEATURE] Show an indicator you are zoomed in #343" is warranted. The issue asks for exactly one thing — an indicator that you are zoomed
    in — and a user wheel-zooming the Explore plot now sees N points · Zoomed in in the bottom-left
    chip, which disappears on reset. The remaining items are polish and follow-ups (zoom-out/pan
    coverage, reset affordance, docs, a11y punctuation), none of which block the literal ask.
Gaps found by the issue audit (7)
  • Zoom-out and pan-at-identity produce no indicator, so the user can still be in a non-default view with the chip reading only "N points". zoomExtent is [0.1, 1000] (packages/core/src/components/scatter-plot/config.ts:9), so the same wheel gesture that zooms in also zooms out to 0.1x, and wheel-zoom around the cursor changes translate too — a user who zooms in then back out to k<=1 is left panned with no marker.
    • Why it matters: The issue's underlying confusion ("is this the whole dataset or a subset of the view?") applies identically to a zoomed-out or panned view. The PR's own proposal.md says the plot "makes a zoomed subset easy to mistake for the complete dataset view" — that failure mode is only half covered.
    • Suggested follow-up: Either generalize the marker to any non-identity view (e.g. t.k !== 1 || t.x !== 0 || t.y !== 0, rendering "Zoomed in" / "Zoomed out" / "Custom view"), or open a follow-up issue explicitly recording zoom-out/pan as out of scope for [FEATURE] Show an indicator you are zoomed in #343.
  • The marker gives no way to get back to the default view. .plot-indicator is pointer-events: none (scatter-plot.styles.ts:155) and the text is just · Zoomed in; the only reset is double-click on the plot background, which appears nowhere in the app UI (the product tour at apps/web/src/tour/product-tour.ts:233 mentions pan/zoom/click but not double-click reset) — it is documented only on the docs site.
    • Why it matters: proposal.md's own "Why" justifies the change partly as giving users "visible confirmation that double-click reset is applicable", but the shipped marker never mentions reset. A user who sees "Zoomed in" and doesn't know the gesture is no better off than before.
    • Suggested follow-up: Extend the label (e.g. · Zoomed in — double-click to reset) or make the chip an interactive reset button (dropping pointer-events: none for that span). If deliberately deferred, note it in design.md's Non-Goals with a follow-up issue.
  • docs/explore/scatterplot.md is not updated. That page has a "Quick Reference" table listing Zoom/Pan/Reset view, and an "Understanding the Display" section that documents other on-plot affordances (duplicate count badges), but gains no mention of the new zoom indicator.
    • Why it matters: The docs site is the user-facing description of the scatterplot UI; a newly added persistent on-plot indicator that is not documented there is exactly the "UI changed but docs still show the old form" gap. It is also the natural place to pair the indicator with the double-click reset gesture the indicator implies.
    • Suggested follow-up: Add a line to docs/explore/scatterplot.md under Navigation / Understanding the Display: the bottom-left chip shows the visible point count and appends · Zoomed in while the view is magnified; double-click to reset.
  • The · separator is inside the announced text (<span class="zoom-indicator">· Zoomed in</span>, scatter-plot.ts:1988) rather than being a decorative element, and the chip is now a live region so the whole string is spoken.
    • Why it matters: Some screen readers verbalize the middle dot at default punctuation verbosity, producing "7831 points middle dot Zoomed in". The a11y requirement added in commit 732de70 was specifically about clean announcement of zoom state.
    • Suggested follow-up: Render the separator as <span aria-hidden="true">·</span> (or move it to a CSS ::before) and keep only "Zoomed in" in the announced span.
  • Adding role="status" aria-live="polite" to the point-count chip makes the point count itself a live region, not just the zoom marker. _getVisiblePointCount() changes on filter apply/reset, legend hide/show, and isolate/un-isolate, so all of those now re-announce "N points". The chip also says "1 points" (pre-existing pluralization bug), and the new test bakes that string in at scatter-plot.zoom-indicator.test.ts:88.
    • Why it matters: This is a behavior change beyond issue [FEATURE] Show an indicator you are zoomed in #343 that can make screen-reader use noisier during ordinary filtering, and it makes an existing grammar bug audible for the first time.
    • Suggested follow-up: Either scope the live region to the zoom marker alone (role="status" on the .zoom-indicator span) or confirm the point-count announcements are wanted; separately fix the pluralization (1 point / N points) since it is now spoken.
  • The new Playwright project (zoom-indicator in apps/web/tests/playwright.config.ts:112-119) will not run on this PR. .github/workflows/e2e.yml gates the whole job on contains(github.event.pull_request.labels.*.name, 'run-e2e').
    • Why it matters: The only test covering the actual user-visible journey (real wheel zoom, real dblclick reset, real chip spacing) is not a merge gate; a later regression in the interaction controller would only be caught by the nightly cron.
    • Suggested follow-up: Add the run-e2e label to this PR before merging (or trigger gh workflow run e2e.yml --ref feat/343-zoom-indicator) and paste the run result, so the browser-level claim in the PR body is verified in CI rather than only locally.
  • The boundary test is a strict t.k > 1 with no epsilon. d3 wheel zoom multiplies the scale (k * 2^delta), so a zoom-in tick followed by an equal zoom-out tick can land on k = 1.0000000002 rather than exactly 1.
    • Why it matters: In that case the chip keeps saying "Zoomed in" on a view that is visually indistinguishable from the fit-all default (only the reset transition is guaranteed to hit exact identity). design.md anticipates float residue only for the reset transition, not for wheel round-trips.
    • Suggested follow-up: Compare with a small tolerance (e.g. t.k > 1 + 1e-6) or derive the state from a non-identity test on the whole transform, and add a component-test case at k = 1 + 1e-9.

Findings needing a decision (4)

These were left for you rather than auto-applied: each changes behavior, needs a product call, or reaches outside this diff.

1. Flipping the new reactive _isZoomedIn falls into _reconcileSelectionOverlays's catch-all branch, so each identity crossing triggers a full WebGL _renderPlot() + SVG overlay rebuild that the marker does not need.

packages/core/src/components/scatter-plot/scatter-plot.ts:1229 · medium · efficiency

_isZoomedIn is an @state, so a boundary crossing enqueues a Lit update whose updated() reaches
_reconcileSelectionOverlays (scatter-plot.ts:869-878). changedKeys is ['_isZoomedIn'], which
is not in selectionKeys, so onlySelectionChanged is false and the method runs
this._renderPlot() + this._updateSelectionOverlays(). _renderPlot -> _renderWebGL('plot') ->
_getPointsForRendering() (quadtree query + gatherPlotData above VIRTUALIZATION_THRESHOLD) + a
full WebGLRenderer.render(pd) upload/draw + mainGroup.selectAll('.protein-point').remove();
_updateSelectionOverlays clears every .selected-overlay and re-runs
_dupOverlay.updateSelectionOverlays(). Concretely: the very first wheel tick of any zoom gesture
crosses k=1, so on a 570K-point dataset the user pays an extra full GPU redraw + overlay reconcile
in the same frame the zoom RAF is already rendering (and again on the final reset frame). The marker
is pure light-DOM text; none of that work affects it. This is the same updated()/_renderPlot()
pass that the F-48 comment at scatter-plot.ts:150-154 exists to avoid — the diff re-introduces it,
just at a lower frequency. The new test at scatter-plot.zoom-indicator.test.ts:95 actually asserts
this wasted render happens.

Suggested fix

In packages/core/src/components/scatter-plot/scatter-plot.ts, replace lines 869-873 of
_reconcileSelectionOverlays:
// Render for other changes
const selectionKeys = ['selectedProteinIds', 'highlightedProteinIds'];
const changedKeys = Array.from(changedProperties.keys()).map(String);
const onlySelectionChanged =
changedKeys.length > 0 && changedKeys.every((k) => selectionKeys.includes(k));
with:
// Render for other changes. Keys listed here affect only the rendered
// template (or are handled by the selection block above); the canvas is
// driven imperatively by the zoom RAF, so _isZoomedIn needs no
// _renderPlot pass (F-48 in spirit).
const noRenderKeys = ['selectedProteinIds', 'highlightedProteinIds', '_isZoomedIn'];
const changedKeys = Array.from(changedProperties.keys()).map(String);
const onlySelectionChanged =
changedKeys.length > 0 && changedKeys.every((k) => noRenderKeys.includes(k));
This MUST be applied together with a rewrite of the second test in scatter-plot.zoom-
indicator.test.ts (see finding 1), because expect(renderPlot).toHaveBeenCalledTimes(1) at lines 95
and 104 will then observe 0 calls. Not autofixable: it changes when a WebGL render happens and
requires coordinated test changes plus a run of the core vitest suite.

2. The boundary-dedup test spies the private _renderPlot as a proxy for "no Lit update was scheduled", coupling it to the unrelated _reconcileSelectionOverlays catch-all instead of asserting the scheduling signal directly.

packages/core/src/components/scatter-plot/scatter-plot.zoom-indicator.test.ts:91 · medium · simplification

The behaviour under test is stated in the spec as "the scatterplot does not schedule a new Lit
update solely because the zoomed-in boolean remained true". _renderPlot is two layers downstream
of that: it only fires because _isZoomedIn is absent from selectionKeys in
_reconcileSelectionOverlays (scatter-plot.ts:870). Cost: the obvious follow-up optimisation
(excluding _isZoomedIn from that catch-all so a marker toggle stops forcing a WebGL redraw — see
the efficiency finding above) makes _renderPlot fire zero times on a crossing, and
expect(renderPlot).toHaveBeenCalledTimes(1) at lines 95 and 104 fails even though the zoom-
indicator behaviour is completely intact. The test also drags in vi.spyOn(plot, '_renderPlot').mockImplementation(...) and a mock-clear dance for a property that Lit already
exposes. Note the repo's existing convention for this family of assertions is documented in the
sibling scatter-plot.transform-reactivity.test.ts and scatter-plot.legend-reactivity.test.ts headers
("the load-bearing signal ... is whether the field write calls requestUpdate") — but that specific
spy does not work here, because Lit's generated @state setter calls requestUpdate on every write
and only bails on the hasChanged check inside it, so an equal-value write is indistinguishable
from a changing one via that spy.

Suggested fix

In packages/core/src/components/scatter-plot/scatter-plot.zoom-indicator.test.ts: add
isUpdatePending: boolean; to the ZoomIndicatorInternals type and remove _renderPlot(): void;
from it. Replace the body of the second test ('schedules rendering only when the zoomed-in boundary
changes') with:
const plot = await makePlot();
const host = plot._interactionHost();
host.onTransform(d3.zoomIdentity.scale(2));
expect(plot.isUpdatePending).toBe(true); // crossed above identity
await plot.updateComplete;
host.onTransform(d3.zoomIdentity.scale(3));
expect(plot.isUpdatePending).toBe(false); // still zoomed in -> no update enqueued
host.onTransform(d3.zoomIdentity.translate(30, 20));
expect(plot.isUpdatePending).toBe(true); // back to identity
await plot.updateComplete;
expect(plot.shadowRoot?.querySelector('.zoom-indicator')).toBeNull();
host.onTransform(d3.zoomIdentity.scale(0.5));
expect(plot.isUpdatePending).toBe(false);
and drop the now-unused vi.spyOn(plot, '_renderPlot'). Not autofixable: it rewrites the assertions
of a test and needs the core vitest suite run to confirm the scheduling timing.

3. The zoomed-in boundary uses an exact t.k > 1, so floating-point drift in d3's multiplicative wheel accumulation leaves "· Zoomed in" permanently displayed on a view that is pixel-identical to the identity fit.

packages/core/src/components/scatter-plot/scatter-plot.ts:1229 · medium · correctness

d3-zoom's wheeled computes k = t.k * Math.pow(2, delta) cumulatively, so a symmetric wheel
round-trip does not return to exactly 1. Concrete repro on the Explore page: hover the plot, scroll
3 mouse-wheel notches in, then 3 notches out at the same pointer position (each Chrome notch is
deltaY=±100, deltaMode 0 → d3 delta = ±0.2). Verified in node: 1 * 2^0.2 * 2^0.2 * 2^0.2 * 2^-0.2 * 2^-0.2 * 2^-0.2 === 1.0000000000000002. That is > 1, so _isZoomedIn stays true and the chip
permanently reads "573230 points · Zoomed in" even though the plot is back at the fitted identity
view and visually indistinguishable from load state — with no way to clear it except the double-
click reset the marker is supposed to be advertising. This is not a rare corner: sweeping n=1..40
notches × 20 notch sizes, 293/800 symmetric round-trips end at k>1 and only 56/800 land exactly on 1
(451 land just below 1, which is the benign direction). It directly violates the PR's own spec text
(openspec/changes/show-zoom-indicator/specs/scatterplot-zoom-indicator/spec.md: "The scatterplot
SHALL NOT show that marker at identity scale"). design.md's Risks section anticipates float drift
only for the reset transition (where d3 lands on zoomIdentity exactly), not for the wheel path
where drift actually accumulates. Neither the jsdom test (which uses
scale(2)/scale(3)/translate/scale(0.5)) nor the Playwright test exercises a near-identity k, so the
gap is uncovered.

Suggested fix

In packages/core/src/components/scatter-plot/scatter-plot.ts, add near the other module-level
scatter-plot constants:
/** Wheel zoom accumulates k multiplicatively, so a symmetric round trip can land a few ULPs above

  1. */
    const ZOOM_IDENTITY_EPSILON = 1e-6;
    and change line 1229 from this._isZoomedIn = t.k > 1; to this._isZoomedIn = t.k > 1 + ZOOM_IDENTITY_EPSILON;.
    Add a boundary case to packages/core/src/components/scatter-plot/scatter-plot.zoom-
    indicator.test.ts:
    host.onTransform(d3.zoomIdentity.scale(1.0000000000000002));
    await plot.updateComplete;
    expect(plot.shadowRoot?.querySelector('.zoom-indicator')).toBeNull();
    Not autofixable: it changes the visible threshold behaviour (a product decision on where 'zoomed in'
    starts) and needs a new test.

4. The chip now uses three mechanisms (two unstyled marker spans, a gap rule added to the shared .plot-indicator class, and a literal ·) to render what is one string.

packages/core/src/components/scatter-plot/scatter-plot.ts:1987 · low · simplification

.point-count and .zoom-indicator have no CSS of their own — they exist only as query hooks for
the two new tests. Because they are separate flex items, the literal whitespace between them in the
Lit template collapses away, which is why gap: 0.25rem had to be added at scatter-
plot.styles.ts:143. That rule lands on the shared .plot-indicator class, whose other consumer is
the unrelated "Recalculating bins for ..." chip at scatter-plot.ts:1994 that neither needs nor asked
for it — a shared style edited for one caller. Cost: two extra DOM nodes per render, a shared-class
CSS change with a second (unintended) consumer, and ~25 lines of test that assert class names, child
order, and computed column-gap instead of the rendered string.

Suggested fix

In packages/core/src/components/scatter-plot/scatter-plot.ts, replace lines 1985-1990 with:


${${this._getVisiblePointCount()} points${this._isZoomedIn ? ' · Zoomed in' : ''}}

Remove the gap: 0.25rem; line added at packages/core/src/components/scatter-plot/scatter-
plot.styles.ts:143. Then update packages/core/src/components/scatter-plot/scatter-plot.zoom-
indicator.test.ts to assert chip?.textContent?.trim() equals '1 points · Zoomed in' (and, in the
second test, that it equals '1 points' after the reset) instead of querying .point-count/.zoom- indicator and asserting child class order; and in apps/web/tests/zoom-indicator.spec.ts replace the
pointCount/zoomMarker locators and the whole chipSpacing evaluate block (lines 31-40) with
await expect(pointCountChip).toHaveText(/^\d+ points · Zoomed in$/) while zoomed and /^\d+ points$/ after reset. Not autofixable: it is a rendered-markup/visual change spanning three files
and requires re-running both the vitest and Playwright projects.

1 further finding(s) were raised and refuted during verification.

@FlorinSenoner

Copy link
Copy Markdown
Collaborator Author

Addressed the actionable findings from #403 (comment) in fb660dd6:

  1. Marker boundary redraw — fixed. _isZoomedIn is now classified as template-only in _reconcileSelectionOverlays, so its Lit update no longer enters the WebGL/selection-overlay redraw path. A regression drives the real host transform bridge and verifies neither _renderPlot() nor _updateSelectionOverlays() runs for the marker-only update.
  2. Boundary test coupling — fixed. The scheduling regression now observes Lit's isUpdatePending signal across crossing, same-side, identity, and zoomed-out transforms. The WebGL/overlay performance assertion is separate from the Lit scheduling contract.
  3. Near-identity floating-point residue — fixed. The boundary uses a 1e-6 identity tolerance, with a regression for the concrete 1.0000000000000002 wheel round-trip residue. The OpenSpec delta now defines this numerical-tolerance behavior.
  4. Chip markup/style complexity — fixed. The point count and conditional marker render as one text run; the two query-hook spans and shared .plot-indicator gap were removed. Component and Playwright tests assert the complete N points · Zoomed in presentation and reset state.

The audit's zoom-out/pan indicator and reset-control suggestions remain explicit non-goals in this change. The point-count live region remains intentional per the accepted accessibility requirement; punctuation/pluralization changes were not added without cross-AT evidence. I did not change PR labels or other metadata, so the label-gated E2E job remains governed by the repository workflow.

Verification: regression RED was 4/4 expected failures on the prior implementation; GREEN is focused Vitest 4/4, full core Vitest 1,389/1,389, isolated Playwright 1/1, strict OpenSpec validation, and fresh staged pnpm precommit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Show an indicator you are zoomed in

2 participants