test(web): make test localStorage shim writable

[Skords-01]

Summary

Adds writable: true to the Object.defineProperty(globalThis, "localStorage", …) descriptor in apps/web/src/test/setup.ts. Object.defineProperty defaults writable to false, which made the shim a read-only data slot and broke any test that does direct global reassignment in its own beforeEach (globalThis.localStorage = makeLS()). Seven test files use that pattern and were failing in CI with TypeError: Cannot assign to read only property 'localStorage' of object '#<Object>':

• apps/web/src/core/lib/featureFlags.test.ts
• apps/web/src/core/lib/insightsEngine.test.ts
• apps/web/src/modules/nutrition/lib/nutritionStorage.test.ts
• apps/web/src/modules/nutrition/lib/shoppingListStorage.test.ts
• apps/web/src/modules/nutrition/lib/waterStorage.test.ts
• apps/web/src/modules/nutrition/domain/nutritionBackup.test.ts
• apps/web/src/shared/lib/storage/typedStore.test.ts

These failures were the "pre-existing on main" breakage flagged on the Test coverage (vitest) and umbrella check jobs of PR #2217 and PR #2218. configurable: true stays so the existing reset path (ensureTestLocalStorage → re-define on next call) keeps working. Inline comment (Ukrainian, per Hard Rule #15) explains the rationale.

No production code changes — test bootstrap only.

Governing Skill

• Primary skill: n/a (one-line test infra fix)
• Secondary skill (if truly needed): n/a

Playbook

• Primary playbook: n/a
• Why this playbook: n/a
• If no playbook matched, why: Single-property descriptor patch in vitest setup; no rollout, no contract change, no governed surface — outside the scope of any current playbook.

Verification

# 1. Targeted repro on the seven broken test files
pnpm --filter @sergeant/web exec vitest run \
  src/core/lib/featureFlags.test.ts \
  src/core/lib/insightsEngine.test.ts \
  src/modules/nutrition/lib/nutritionStorage.test.ts \
  src/modules/nutrition/lib/shoppingListStorage.test.ts \
  src/modules/nutrition/lib/waterStorage.test.ts \
  src/modules/nutrition/domain/nutritionBackup.test.ts \
  src/shared/lib/storage/typedStore.test.ts
# → 7 files / 88 tests pass

# 2. Full apps/web suite (regression sweep)
pnpm --filter @sergeant/web exec vitest run
# → 221 files / 2197 tests pass under Node 20.20.2

# 3. Lint + typecheck on the touched file
pnpm --filter @sergeant/web exec eslint --max-warnings=0 src/test/setup.ts
pnpm --filter @sergeant/web exec tsc --noEmit
# → both green

Note: running vitest directly via pnpm --filter @sergeant/web exec vitest requires @sergeant/db-schema to be pre-built (pnpm --filter @sergeant/db-schema build) because the umbrella pnpm test goes through Turborepo (test depends on ^build) but the direct invocation does not. CI uses the Turbo path, so this is only a local-dev nuance.

Additional checks:

• Local smoke / manual validation completed (full apps/web vitest sweep)
• Surface-specific checks completed (7 previously-failing files now green; lint + tsc green)

Docs and Governance

• I updated docs that changed with the behavior, contract, workflow, or rollout.
• I checked whether AGENTS.md needed an update.
• I checked whether a playbook or skill needed an update.
• I checked whether governance docs or review docs needed an update.

Updated docs:

• n/a — test bootstrap only, no contract/workflow/rollout change.

Risk and Rollout

• User-visible risk: none. Test-only file (apps/web/src/test/setup.ts); not bundled into web/server/mobile output.
• Rollout / deploy order: merge anytime; no migration, no deploy coupling.
• Backout plan: revert the single-line addition (writable: true) — return-to-prior-state is identical to the pre-PR descriptor.

Hard Rule #15

• I read AGENTS.md before coding.
• Internal docs I touched are in Ukrainian (the new explanatory comment).
• I did not use --no-verify.

Audit-freeze (until 2026-06-02)

• This PR does not add new top-level audit/initiative/playbook/ADR files (or override is justified below).

Reviewer Notes

• Most important to review: confirm no existing test in apps/web deliberately relies on globalThis.localStorage being read-only (e.g., to assert the shim resists overwriting). I grep'd and found none — every reassignment is intentional test-supplied mock seeding — and the full 2197-test sweep passes, but worth a second pair of eyes.
• The descriptor still has configurable: true, so the ensureTestLocalStorage() re-define path on the next afterEach/setup tick keeps functioning identically.
• This unblocks the Test coverage (vitest) and umbrella check jobs that were red on PRs docs(docs): close audit items 5 and 7 with PR 2215/2216 evidence #2217 and perf(web): lazy-load hub views + split markdown/auth/zod vendors (audit #9, #10) #2218 (both shipped despite that, since the failure was confirmed pre-existing — see the comment thread on perf(web): lazy-load hub views + split markdown/auth/zod vendors (audit #9, #10) #2218 for the per-job breakdown).

Link to Devin session: https://app.devin.ai/sessions/468732c16b1b4805b00ee2d0d47865f1
Requested by: @Skords-01

---

Summary by cubic

Make the localStorage test shim writable so tests can reassign it without errors. This fixes CI failures in the web vitest suite.

• Bug Fixes
  • Set writable: true on globalThis.localStorage in apps/web/src/test/setup.ts; kept configurable: true.
  • Restores tests that do globalThis.localStorage = … (7 files) and removes the read-only TypeError in CI.
  • Test-only change; no production impact.

^{Written for commit 1a2a4e3. Summary will update on new commits.}

Summary by CodeRabbit

• Tests
  • Updated test setup configuration to improve compatibility with storage functionality mocking in individual test files, preventing errors during test execution.

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
sergeant	Ready	Preview, Comment	May 8, 2026 9:11am

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

The PR adds writable: true to the Object.defineProperty descriptor for globalThis.localStorage in the test setup utility. This allows test files to directly reassign the mocked localStorage object without triggering read-only property errors.

Changes

Test Infrastructure

Layer / File(s)	Summary
localStorage Mock Configuration apps/web/src/test/setup.ts	The ensureTestLocalStorage() function is updated to include writable: true in the property descriptor alongside configurable: true, enabling tests to reassign globalThis.localStorage directly without TypeError when overriding the mock.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

• Skords-01/Sergeant#1745: Both PRs modify test-time mocking of browser storage using Object.defineProperty on global/localStorage, adjusting property descriptors to enable storage object swapping in tests.

Suggested labels

size/M

Poem

🐰 A tiny tweak, yet oh so grand,
The localStorage now writable stands!
No more read-only, tests run free,
Mocks upon mocks, in harmony! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: making the test localStorage shim writable by adding writable: true to the Object.defineProperty descriptor.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch devin/1778231087-fix-test-localStorage-writable

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

⏱️ CI Pipeline Duration Report

Based on the last 50 successful runs on the default branch.

Overall Pipeline

Metric	Value
p50	6m 26s
p95	7m 55s
p99	9m 3s
Current run	14m 42s
vs p95	+85.7%

Trend (last 20 runs): ▃▃▁▂▃▃▃▂▃▃▂▂▄▃▃▆▅▄█▆

Per-Job Breakdown

Job	p50	p95	p99	Current	vs p95
Accessibility (axe-core)	2m 5s	2m 21s	2m 23s	0s	-100.0%
Commit messages (commitlint)	0s	0s	0s	33s	N/A
Critical-flow E2E (Playwright)	1m 36s	1m 44s	1m 44s	7m 36s	+338.5%
Migration lint (AGENTS rule	0s	0s	0s	7s	N/A
Pipeline duration (p95 trend)	26s	27s	27s	—	—
Secret scan (gitleaks)	8s	11s	11s	10s	-9.1%
Smoke E2E (Playwright)	1m 26s	1m 40s	1m 40s	—	—
Test coverage (vitest)	2m 4s	2m 33s	2m 33s	10m 10s	+298.7%
Workflow lint (actionlint)	7s	7s	7s	5s	-28.6%
check	4m 12s	4m 54s	5m 6s	14m 23s	+193.5%
tsconfig strict guard (PR-1.A)	5s	14s	14s	7s	-50.0%

⚠️ Warning: Current run (14m 42s) exceeds p95 + 20% threshold (9m 30s). Consider reviewing slow jobs.