Skip to content

fix(review): add localStorage theme-forcing fallback for visual capture#4127

Merged
JSONbored merged 1 commit into
mainfrom
fix/visual-theme-viewport-4109-v2
Jul 8, 2026
Merged

fix(review): add localStorage theme-forcing fallback for visual capture#4127
JSONbored merged 1 commit into
mainfrom
fix/visual-theme-viewport-4109-v2

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Verified whether page.emulateMediaFeatures([{ name: "prefers-color-scheme", ... }]) actually changes the rendered theme for a real capture target, per fix(review): verify + fix theme-forcing and viewport matrix in visual capture #4109. Emulation.setEmulatedMedia (what emulateMediaFeatures maps to) only changes what CSS media queries and window.matchMedia report -- it cannot write localStorage and has zero effect on any theme mechanism that reads an explicit stored preference instead. This is reproducible today against gittensory's own UI: apps/gittensory-ui/src/components/site/theme-toggle.tsx forces dark mode unconditionally (never consulting prefers-color-scheme), so a light vs dark capture of gittensory's own site would render byte-identical today.
  • Adds the documented fallback: a new review.visual.theme_storage_key config field that ALSO forces theme via localStorage.setItem(key, theme) + a page reload before capture, alongside the existing emulateMediaFeatures call. Threaded through captureShot/captureScrollFrames (src/review/visual/shot.ts), capturePage/captureScrollGif/buildCapture (src/review/visual/capture.ts), the on-demand ?url= render route's &themeStorageKey= param, the R2 cache fingerprint (so a differently-keyed capture never collides), and the full config-as-code round trip (packages/gittensory-engine/src/focus-manifest.ts: type + empty default + parse + present-check + overlay + serialize, plus both .gittensory.yml.example and config/examples/gittensory.full.yml docs). Fully opt-in and additive -- omitted (every existing repo) is byte-identical to today.
  • Made an explicit, written decision on the viewport matrix per fix(review): verify + fix theme-forcing and viewport matrix in visual capture #4109's second question: kept the existing 1440x900/390x844 desktop+mobile pair rather than widening to metagraphed's 3-viewport manual convention (documented inline in shot.ts) -- a 3rd viewport would raise Browser Rendering wall-clock by 50% (4 -> 6 renders/route) for every repo, every review, not just the reviewer who wants tablet coverage, and gittensory's own pair already straddles a real breakpoint on each side.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint (via npm run test:ci)
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers (via npm run test:ci)
  • npm run build:mcp (via npm run test:ci)
  • npm run test:mcp-pack (via npm run test:ci)
  • npm run ui:openapi:check (via npm run test:ci; no API/schema change in this PR)
  • npm run ui:lint (via npm run test:ci)
  • npm run ui:typecheck (via npm run test:ci)
  • npm run ui:build (via npm run test:ci)
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

Ran the full local gate: npm run test:ci (all steps green, including test:engine-parity, manifest:drift-check, selfhost:validate-observability, db:migrations:check/db:schema-drift:check -- no migration needed, this feature has no DB surface) and npm audit --audit-level=moderate (0 vulnerabilities). Additionally manually verified branch-level patch coverage of every changed line in src/review/visual/shot.ts, src/review/visual/capture.ts, and packages/gittensory-engine/src/focus-manifest.ts against the local v8 lcov report -- every new if/&&/ternary in the diff has both sides exercised.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A -- no auth/cookie/CORS/session surface touched.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A -- review.visual.* is a .gittensory.yml-only manifest field, like its preview/routes/themes/gif siblings; no OpenAPI/DB/MCP surface.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A -- no UI changes.)
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. (N/A -- backend-only capture-pipeline change, no visible UI.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. (Updated the two config example docs; CHANGELOG.md untouched.)

UI Evidence

N/A -- this PR has no visible UI surface; it changes the server-side visual-capture pipeline (src/review/visual/**) and its .gittensory.yml config schema only.

Notes

  • The empirical "is emulateMediaFeatures a no-op" question is answered by direct code inspection rather than a live network probe (sandboxed environment has no network access to a real deployed preview): Emulation.setEmulatedMedia's documented scope is CSS-media-query/matchMedia only, and apps/gittensory-ui's own theme-toggle source confirms it as a concrete, reproducible instance of a target this option cannot affect.
  • The viewport-matrix question is resolved by an explicit "keep as-is" decision with a written rationale (src/review/visual/shot.ts, above DESKTOP_VIEWPORT/MOBILE_VIEWPORT), per the issue's "either is acceptable, must be explicit" framing -- no viewport dimensions changed.

Closes #4109

…re (#4109)

Verified emulateMediaFeatures only affects CSS media queries/matchMedia, so
it has zero effect on a target whose theme is driven by a stored preference
instead -- reproducible today against gittensory-ui's own dark-mode-only
build. Adds an opt-in review.visual.theme_storage_key config that ALSO
forces the theme via localStorage.setItem + a reload, alongside the
existing emulateMediaFeatures call. The desktop/mobile viewport pair is
documented as a deliberate keep, not a silent inconsistency with
metagraphed's manual 3-viewport convention.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui f04bcb9 Commit Preview URL

Branch Preview URL
Jul 08 2026, 04:40 AM

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.67%. Comparing base (189f824) to head (f04bcb9).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4127   +/-   ##
=======================================
  Coverage   93.66%   93.67%           
=======================================
  Files         384      384           
  Lines       35936    35963   +27     
  Branches    13188    13196    +8     
=======================================
+ Hits        33660    33687   +27     
  Misses       1618     1618           
  Partials      658      658           
Files with missing lines Coverage Δ
packages/gittensory-engine/src/focus-manifest.ts 99.15% <100.00%> (+<0.01%) ⬆️
src/review/visual/capture.ts 89.68% <100.00%> (+0.33%) ⬆️
src/review/visual/shot.ts 92.61% <100.00%> (+0.80%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

loopover-orb Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-08 05:14:31 UTC

10 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR adds an opt-in localStorage theme-forcing fallback (`review.visual.theme_storage_key`) for capture targets whose theming reads a stored preference rather than `prefers-color-scheme`, backed by a concrete, reproducible finding against gittensory-ui's own dark-mode-only build. The change is correctly gated behind both `theme` and `themeStorageKey` being set at every call site (captureShot, captureScrollFrames, handleShot's on-demand route), is threaded consistently through the R2 cache fingerprint so differently-keyed captures never collide, and round-trips cleanly through the config-as-code parse/overlay/serialize path using the existing `pickOverlayNullable`/`parsePublicSafeText` conventions. Tests are extensive and exercise the real wiring (option forwarding, fingerprint divergence, on-demand URL param carryover, guard combinations) rather than fabricated states, and all CI checks are green.

Nits — 6 non-blocking
  • src/review/visual/shot.ts: the localStorage-set + reload block is duplicated verbatim between captureShot and captureScrollFrames — consider extracting a small `forceThemeViaLocalStorage(page, theme, key)` helper to avoid the two copies drifting.
  • packages/gittensory-engine/src/focus-manifest.ts parseVisualConfig: setting `theme_storage_key` without also setting `themes` silently no-ops (per the doc comment) with no parse warning — consider a warning similar to other visual-config combos so a misconfigured repo doesn't wonder why nothing changed.
  • src/review/visual/shot.ts's localStorage `catch` branch (privacy mode / disabled storage / cross-origin frame) has no test exercising the failure path — worth one to confirm capture still proceeds with a best-effort skip.
  • Verify the `page.reload` call resolves against a type that actually declares `reload` (the `ScreenshotPage` interface shown in the diff isn't visibly extended with it) — CI's typecheck passed so this is almost certainly fine, but couldn't confirm from the visible diff alone.
  • Consider adding the parser warning noted above (theme_storage_key configured without themes) to make the no-op explicit rather than silent.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #4109
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: 51 registered-repo PR(s), 43 merged, 505 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 51 PR(s), 505 issue(s).
Gate result ✅ Passing No configured blocker found.
Linked issue satisfaction

Addressed
The PR empirically verifies emulateMediaFeatures is a no-op for gittensory-ui's localStorage-driven theming, adds a configurable themeStorageKey fallback threaded through shot.ts/capture.ts/focus-manifest.ts/config examples with cache-key updates, and makes an explicit documented decision to keep the 2-viewport matrix with a stated cost rationale, matching all three requirements and deliverables i

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 51 PR(s), 505 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
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.

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 8, 2026
@JSONbored
JSONbored merged commit 1d2cb3f into main Jul 8, 2026
12 checks passed
@JSONbored
JSONbored deleted the fix/visual-theme-viewport-4109-v2 branch July 8, 2026 05:21
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. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

fix(review): verify + fix theme-forcing and viewport matrix in visual capture

1 participant