Skip to content

fix(frontend): storage-clear sign-out, safe lookups, real YTD sparkline - #1344

Merged
IAmJSD merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:ui/issue-317
Oct 24, 2026
Merged

IAmJSD merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:ui/issue-317

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Three low-severity correctness findings from the 2026-09-02 audit. A tab stayed signed in after another tab ran localStorage.clear(), a provider named constructor rendered function Object() { [native code] } as a coverage card heading, and the YTD Savings sparkline followed the trend chart's 7d/30d/90d/All toggle while the number above it stayed year-to-date. This PR fixes all three.

Closes #317

What changed

  • frontend/src/api/client.ts: installStorageListener treats e.key === null (the clear() event) as a clear-all. It drops the in-memory token, API key and CSRF token and reloads. A tab that is already signed out is not reloaded (A11-023).
  • New frontend/src/lib/own-property.ts exports ownValue(table, key), a hasOwnProperty-guarded lookup. It is used for PROVIDER_DISPLAY_NAMES in inventory.ts, and for PAYMENT_ORDER (3 sites) and PAYMENT_DISPLAY_LABELS (2 sites) in recommendations.ts (A12-048).
  • frontend/src/dashboard.ts: new loadYtdSparkline(), called from loadDashboard next to loadSavingsTrendChart. It fetches /history/analytics from 1 January 00:00 UTC to now with the same provider/account scope (hourly, daily or weekly interval depending on elapsed days), with a generation guard and a skip when the tile is absent. loadSavingsTrendChart no longer touches the YTD sparkline (A12-073).
  • Tests: crosstab-session.test.ts, own-property.test.ts, inventory.test.ts, dashboard.test.ts, and a new Playwright spec tests-e2e/crosstab-storage-clear.spec.ts.

Why this approach

  • YTD sparkline: the issue offered a separate YTD series or a relabel. The tile's number is a YTD figure, so the line under it should be too. 1 January UTC matches the backend's yearStart in calculateCommitmentMetrics. The cost is one extra analytics request per Home load and per chip change, not per range click.
  • Lookups: a shared helper is greppable and mirrors the own-property guard already in navigation.ts (fix(frontend,server): resolve every route to its own page on direct load and refresh cloud-commitments-cli#1854). Main also has private own() copies in lib/plan-service-ids.ts and commitmentOptions.ts. This PR does not switch those over; they can move to ownValue in a follow-up. Object.create(null) tables would work too, but the protection would be invisible at the call site.
  • Storage listener: skipping the reload for signed-out tabs avoids wiping a half-typed login form when another tab clears storage.

Screenshots

Screenshots were taken before this branch was merged with main; before = main at a14dc8f. After is this branch. Both are headless Chromium at 1440x900 against the repo's mocked e2e API.

Before After
crosstab-clear before crosstab-clear after
coverage-constructor before coverage-constructor after
ytd-sparkline before ytd-sparkline after
  1. Cross-tab clear: on main, after another tab runs localStorage.clear(), this tab still shows the signed-in Home view. On the branch it signs out and lands on CUDly Login.
  2. Coverage heading: on main, a section for provider constructor is titled function Object() { [native code] }. On the branch it falls through to the uppercase fallback, CONSTRUCTOR.
  3. YTD sparkline, trend range set to 7d: on main the sparkline shows the 7-day wiggle from the trend chart. On the branch it still shows the year-to-date growth that matches the $27,500 figure.

Verification

Run on this branch's head f4bd10c2.

  • Merged with main at 5b52c4e; conflicts in frontend/src/__tests__/dashboard.test.ts resolved, see the merge commit.
  • npm run lint: 0 errors. Typecheck and build clean.
  • Jest: 3877 of 3878 passed, 1 skipped. Before the merge, the new and changed tests (20) also passed with --randomize. A full-file random seed of the dashboard/inventory suites fails 2 tests on unmodified main as well, so that order dependence is not from this branch.
  • New and changed tests: crosstab-session.test.ts (a null key now signs out, and a signed-out tab does not reload), own-property.test.ts (constructor, toString, __proto__, hasOwnProperty, valueOf and '' return undefined), inventory.test.ts (providers constructor and toString render CONSTRUCTOR/TOSTRING), dashboard.test.ts (YTD request starts 1 January UTC with the chip scope and draws the series, a 7-day trend click no longer redraws the tile, a failure hides it, a missing tile skips the fetch; the chip-change test now expects 2 analytics calls).
  • Playwright: 201 passed, including the new crosstab-storage-clear.spec.ts (two pages in one context; clear() in tab B puts tab A on CUDly Login).
  • Secrets scan clean, planted key flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS.

Notes for review

  • Home now makes two /history/analytics calls (trend and YTD) per load and per chip change. If an extra call is not wanted, the alternative from the issue is to relabel the sparkline (for example "trend for selected range").
  • dashboard.ts was already over the 500-line guideline (1194 lines on main) and is 1223 on this branch. loadYtdSparkline sits next to attachSparkline, which is module-private.
  • riexchange.ts providerLabel (PROVIDER_LABELS[provider]) has the same prototype-lookup shape. It is left alone because the issue scopes itself to code outside the RI-exchange module, and RI-exchange audit issues own that file.
  • Main's Trends tab (trends.ts:167, PROVIDER_LABELS[r.provider] ?? r.provider) has the same shape too. It is not changed here and can use ownValue in a follow-up.
  • If a clear() happens while a signed-in tab is mid-edit, that tab reloads. This matches the existing per-key sign-out.

Overlaps

Textual conflicts with the open branches for #744 (#1381, dashboard.ts, savings-chart summary near loadSavingsTrendChart) and #730 (#1371, inventory.ts imports and section header).

Note for the UI owner

This changes UI code that @cristim owns. IAmJSD reviewed the before/after screenshots and asked for it to be merged; any design point you disagree with can be a follow-up.

🤖 Generated with Claude Code

…s, real YTD sparkline

- Cross-tab sign-out ignored the storage event localStorage.clear()
  fires (key === null), so a tab whose storage was wiped elsewhere kept
  a signed-in view. A null key now clears in-memory auth and reloads,
  unless the tab is already signed out (A11-023).
- PROVIDER_DISPLAY_NAMES, PAYMENT_ORDER and PAYMENT_DISPLAY_LABELS were
  indexed by API strings through the prototype chain, so a provider
  named "constructor" rendered "function Object() { [native code] }" as
  a card heading. Lookups go through a new lib/own-property ownValue()
  helper (A12-048).
- The YTD Savings tile's sparkline reused the trend chart's points, so
  the 7d/30d/90d/All toggle reshaped a line labelled YTD. It now plots
  its own 1 January (UTC, as the backend's ytd_savings uses) to now
  series with the same chip scope (A12-073).

Refs LeanerCloud#317

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/s Hours type/bug Defect labels Oct 24, 2026
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Conflicts:
- frontend/src/__tests__/dashboard.test.ts: import block. Kept this
  branch's loadYtdSparkline import from ../dashboard and main's separate
  formatTrendTooltipTitle import from ../modules/chart-utils (LeanerCloud#1323).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): APPROVE f4bd10c2d690b137a6038d4f7e9f340e90eaeab1

Blocking:

  • none
    Non-blocking:
  • frontend/src/lib/own-property.ts:9: main now has private copies of this helper in commitmentOptions.ts:102 (own, fix(frontend): align history Revoke and dual-control gates with backend #1347) and lib/plan-service-ids.ts:21. This PR adds a third, exported copy without switching either over. Consolidate here or in a follow-up.
  • frontend/src/trends.ts:167: main's new Trends tab (feat(frontend): add a Trends sub-tab for the analytics trends endpoint #1317) uses PROVIDER_LABELS[r.provider] ?? r.provider, the same prototype-lookup shape. A provider named "constructor" puts Object's function source into the By-provider table as text. It landed after the branch, so a follow-up is fine.
  • frontend/src/dashboard.ts:1185: the YTD sparkline refreshes only via loadDashboard, so chip changes and auto-refresh are covered. It costs one more /history/analytics call per Home load, which the body already says.
    Attacked: The only conflict was the dashboard.test.ts import block. Against parent 1 (main 5b52c4e) the merge only adds main's changes (formatTrendTooltipTitle import, outlined-cancel test). Against parent 2 (2c78990) it only adds main's changes. No main tests were dropped or loosened, and there are no follow-up commits. It still merges cleanly into origin/main 51aa5b1. loadSavingsTrendChart no longer writes a YTD sparkline. The range toggle calls only the trend chart. The generation guard covers both the success and error paths. attachSparkline uses DOM APIs only, with no innerHTML or inline style. The storage listener returns early on a null key only when the tab is signed out. Jest for crosstab-session, dashboard, inventory, own-property and recommendations: 599 passed, 1 skipped.

@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at f4bd10c2 merged onto current main (9d5eb2ed; merge-tree 02bd5b30; Linux, Node, headless Chromium):

  • This branch's own checks passed at this head: lint, typecheck, build, the full jest suite, the full Playwright e2e suite, and the git-secrets checks (see Verification in the PR body).
  • The combined tree of every UI PR in this merge wave (tree dc090cc2, main plus 17 PRs) passes npm ci, lint, typecheck, build, jest (1 skipped, 4097 passed, 4098 total), and the full Playwright suite (232 passed).
  • CI is green on f4bd10c2 (CI Success, Run pre-commit hooks).
  • The independent review above approves this exact head.
  • Nothing was applied to any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 24, 2026 19:02
@IAmJSD
IAmJSD merged commit d169b00 into LeanerCloud:main Oct 24, 2026
29 checks passed
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Marked ready and merged by IAmJSD's agent, at IAmJSD's request (IAmJSD reviewed the before/after screenshots; cristim is traveling), after the independent review and local verification above, all at f4bd10c2.

IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 24, 2026
Conflicts:
- frontend/src/app.ts: import block; kept this branch's wireTablistKeyboard
  import and main's initPurchasingNav import (LeanerCloud#1336).
- frontend/src/inventory.ts: import block; kept this branch's syncTabStops
  import and main's ownValue import (LeanerCloud#1344).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 24, 2026
No conflicts. Checked loadSavingsTrendChart in dashboard.ts: the sparse
(<2 points) path from LeanerCloud#188 sits after main's empty-state path and is
independent of main's new loadYtdSparkline (LeanerCloud#1344), so both behaviours
are kept as merged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 24, 2026
Conflicts:
- frontend/src/dashboard.ts (loadSavingsTrendChart empty and error
  paths): kept this branch's removeChartTable(SAVINGS_TREND_TABLE_ID) and
  took main's removal of attachSparkline('ytd', []) there; since LeanerCloud#1344 the
  YTD sparkline has its own series loaded by loadYtdSparkline(), so the
  trend chart's empty/error state no longer clears it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 24, 2026
No textual conflicts. Checked main's changes next to this branch's:
LeanerCloud#1344 moved the YTD sparkline into loadYtdSparkline(), which looks up
.kpi-tile-spark[data-spark-key="ytd"]; lib/kpi-tile.ts still renders
that slot for the YTD tile. CSS added by LeanerCloud#1336/LeanerCloud#1332/LeanerCloud#1388 (sticky
section nav z-index 5, table scroll shadows z-index 9, sort buttons)
sits below the dialog z-index ladder and does not touch card or dialog
surfaces.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/few Limited audience priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(frontend): low correctness findings from the 2026-09-02 audit

1 participant