Skip to content

fix(test): pin miner-ui jsdom localStorage over Node 26's broken global#7597

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-7592
Jul 21, 2026
Merged

fix(test): pin miner-ui jsdom localStorage over Node 26's broken global#7597
JSONbored merged 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-7592

Conversation

@bitfathers94

Copy link
Copy Markdown
Contributor

fix(test): pin miner-ui jsdom localStorage over Node 26's broken global

Node 26 predefines an experimental globalThis.localStorage accessor
(nodejs/node#60303) that returns undefined without --localstorage-file.
It exists on globalThis before jsdom's environment is installed, so
Vitest's populateGlobal skips copying jsdom's working Storage over it and
every bare localStorage call (theme-toggle.test.tsx, mirroring
theme-toggle.tsx) throws under Node 26 while passing on Node 22/24.

Point globalThis.localStorage at jsdom's real Storage from the raw JSDOM
window unconditionally: a no-op where the global already is that object,
the fix on Node 26+. A ??= guard would keep the broken accessor since it
already counts as present.

Closes #7592

Node 26 predefines an experimental globalThis.localStorage accessor
(nodejs/node#60303) that returns undefined without --localstorage-file.
It exists on globalThis before jsdom's environment is installed, so
Vitest's populateGlobal skips copying jsdom's working Storage over it and
every bare localStorage call (theme-toggle.test.tsx, mirroring
theme-toggle.tsx) throws under Node 26 while passing on Node 22/24.

Point globalThis.localStorage at jsdom's real Storage from the raw JSDOM
window unconditionally: a no-op where the global already is that object,
the fix on Node 26+. A ??= guard would keep the broken accessor since it
already counts as present.

Closes JSONbored#7592
@bitfathers94
bitfathers94 requested a review from JSONbored as a code owner July 20, 2026 23:38
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 20, 2026
@loopover-orb

loopover-orb Bot commented Jul 20, 2026

Copy link
Copy Markdown

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-21 00:35:20 UTC

1 file · 1 AI reviewer · no blockers · readiness 100/100 · CI green · unknown

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This adds an unconditional `Object.defineProperty` in vitest.setup.ts that re-points `globalThis.localStorage` at the real jsdom Storage object pulled off `globalThis.jsdom.window.localStorage`, working around Node 26's own experimental `localStorage` accessor that Vitest's `populateGlobal` won't overwrite because the property already exists. The reasoning in the comment is sound and matches the linked Node issue (nodejs/node#60303), and the guard is correctly a hard reassignment rather than `??=` since a `??=` would keep the broken Node accessor. The change is narrowly scoped to the exact failure described in #7592 and closes it directly.

Nits — 5 non-blocking
  • The fix depends on `globalThis.jsdom.window.localStorage` existing, which is an internal implementation detail of vitest-environment-jsdom rather than a documented public API — worth a one-line comment noting this is unstable across vitest-environment-jsdom versions and can silently become a no-op again if that internal shape changes.
  • No new/updated test was added to pin this behavior going forward; since CI here almost certainly runs on Node <26, this hunk is exercised as a no-op in CI and its actual fix path (Node 26) isn't verified by this PR's own test run.
  • Consider adding a small assertion in vitest.setup.ts (e.g. throw if `jsdomLocalStorage` is falsy) so a future vitest-environment-jsdom upgrade that changes the internal `globalThis.jsdom` shape fails loudly instead of silently reverting to the broken Node 26 accessor.
  • If feasible, add a Node-version-gated CI job (or document in CI config) that actually runs on Node 26 to validate this fix exercises its real branch, not just the Node 22/24 no-op branch.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #7592
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 63 registered-repo PR(s), 40 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor bitfathers94; Gittensor profile; 63 PR(s), 0 issue(s).
Improvement ℹ️ None detected risk: low · value: none
Review context
  • Author: bitfathers94
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 63 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/ desktop before /
before /
after /
after /
/ mobile before / (mobile)
before / (mobile)
after / (mobile)
after / (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

Scroll preview
Route Before (production) After (this PR's preview)
/ before / (scroll)
before / (scroll)

A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LoopOver approves — the gate is satisfied and CI is green.

@JSONbored
JSONbored merged commit c7d48ca into JSONbored:main Jul 21, 2026
8 checks passed
JSONbored added a commit that referenced this pull request Jul 21, 2026
… broken global (#7616)

* fix(test): pin loopover-ui + ui-kit jsdom localStorage over Node 26's broken global (#7612)

Node 26 predefines an experimental globalThis.localStorage accessor
(nodejs/node#60303) that returns undefined without --localstorage-file.
It exists on globalThis before jsdom's environment is installed, so
Vitest's populateGlobal skips copying jsdom's working Storage over it,
and every bare localStorage call throws under Node 26 while passing on
Node 22/24 -- the identical gap #7597 fixed in apps/loopover-miner-ui,
which never got mirrored into apps/loopover-ui or packages/loopover-ui-kit.

Confirmed via a full npm run test:ci on Node 26: 5 files / 26 tests
failed in apps/loopover-ui with "Cannot read properties of undefined
(reading 'clear')" on window.localStorage.clear() -- routes/index,
api/try-it, app-panels/onboarding-preview-card, lib/analytics-window,
lib/use-local-storage. packages/loopover-ui-kit has no test that
currently touches localStorage, so this guard is preventive there.

Point globalThis.localStorage at jsdom's real Storage from the raw
JSDOM window unconditionally: a no-op where the global already is that
object, the fix on Node 26+. All 5 previously-failing files now pass
on Node 26 (426/426 in apps/loopover-ui, 19/19 in ui-kit), with no
change on Node 22/24.

Closes #7612

* fix(test): satisfy prettier's line-wrap in the loopover-ui localStorage guard

npm run ui:lint flagged the previous wrap of the jsdom.window?.localStorage
chain; break before ?.window instead of before ?.localStorage to match
apps/loopover-ui's printWidth-100 prettier config. No behavior change.
JSONbored added a commit that referenced this pull request Jul 21, 2026
Every engines.node in this repo was open-ended (root and 4 of 5
engines-declaring workspaces at ">=22.0.0", loopover-miner at
">=22.13.0" -- both with no upper bound), and there was no root
.npmrc, so npm never warned or blocked a Node 23+ install locally.
That silent gap is exactly how the Node 26 jsdom/localStorage bug
(#7592/#7597, #7612) went undetected until someone happened to run
tests locally on a newer Node -- twice.

- Tighten engines.node to exclude Node 23+ on root + every workspace
  that already declares one (discovery-index, loopover-engine,
  loopover-ui-kit, loopover-miner, loopover-mcp), preserving each
  workspace's own floor (loopover-miner keeps >=22.13.0, aligned with
  unflagged node:sqlite support per its own test).
- Add a root .npmrc with engine-strict=true so an out-of-range Node
  becomes a hard install/run error instead of a silently-ignored
  warning. CI is unaffected: .github/actions/setup-workspace already
  strips any repo .npmrc before installing (a guard against a
  malicious fork PR's .npmrc), and pins its own Node via
  node-version-file: .nvmrc regardless.
- Add scripts/check-engines-nvmrc-sync.mjs (+ .d.mts + unit tests),
  asserting .nvmrc and every declared engines.node stay in lockstep,
  so a future .nvmrc bump that forgets to widen engines.node is
  caught mechanically. Wired into both the local test:ci aggregate
  and a real ci.yml step (gated like Branding drift check; .nvmrc
  added to the backend path filter so a .nvmrc-only change still
  triggers it).
- Update the 3 pre-existing tests that hard-asserted the old
  un-bounded engines.node strings (loopover-engine-scaffold,
  miner-package-skeleton, miner-run-state) to the new values.
- Document the new check in reference.md's CI table.

Closes #7613
JSONbored added a commit that referenced this pull request Jul 21, 2026
…rd (#7620) (#7624)

#7612's fix mirrored #7597's Node 26 jsdom/localStorage guard verbatim
into apps/loopover-ui and packages/loopover-ui-kit, on top of the
existing copy already in apps/loopover-miner-ui -- three copies of the
same ~15-line guard that can drift, and a fourth future jsdom
workspace wouldn't automatically inherit the fix.

Extract into test/helpers/vitest-jsdom-node26-localstorage.ts, exporting
one patchJsdomLocalStorageForNode26() function. All three vitest.setup.ts
files now import and call it instead of inlining the logic. No behavior
change -- same guard, same no-op-on-Node-22/24 / fix-on-Node-26+ semantics.

Verified on Node 26: all three consuming workspaces' full test suites
pass post-extraction (loopover-ui 79/79 files, ui-kit 3/3, ui-miner
28/28) -- the regression proof for a helper this thin, matching this
repo's existing test/helpers/*.ts convention of no dedicated test file.

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

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

theme-toggle tests fail under Node 26 — jsdom's localStorage is shadowed by Node's experimental global

2 participants