Skip to content

fix(ui): migrate the pre-rebrand analytics window preference forward - #8728

Closed
tryeverything24 wants to merge 1 commit into
JSONbored:mainfrom
tryeverything24:fix-analytics-window-migration-8699
Closed

fix(ui): migrate the pre-rebrand analytics window preference forward#8728
tryeverything24 wants to merge 1 commit into
JSONbored:mainfrom
tryeverything24:fix-analytics-window-migration-8699

Conversation

@tryeverything24

Copy link
Copy Markdown
Contributor

Closes #8699

What

app.analytics.tsx called useLocalStorage<AnalyticsWindowDays>(ANALYTICS_WINDOW_STORAGE_KEY, DEFAULT_ANALYTICS_WINDOW_DAYS) with no third legacyKey argument. The key was renamed from "gittensory.analytics.windowDays" to "loopover.analytics.windowDays" in the rebrand commit (75450f1d5) that gave app.runs.tsx and app.workbench.tsx their one-time legacyKey migration — this file was missed. Any returning user who had picked a 30d/90d analytics window before the cutover silently reverted to the 7-day default.

How

apps/loopover-ui/src/routes/app.analytics.tsx now passes "gittensory.analytics.windowDays" as the third useLocalStorage argument, matching the app.runs.tsx / app.workbench.tsx pattern exactly. The hook already implements the one-time fallback: if the new key is absent it reads the legacy key, adopts the value, and writes it forward to the new key (leaving the legacy key in place).

Deliverables

  • The useLocalStorage call for the analytics window preference includes the "gittensory.analytics.windowDays" legacy key.
  • New test in apps/loopover-ui/src/routes/app.analytics.test.tsx: seeds localStorage["gittensory.analytics.windowDays"] = "30", mounts ProductAnalytics, and asserts the toggle group shows 30d selected (data-state="on", 7d "off"), the dashboard is fetched for ?days=30, and localStorage["loopover.analytics.windowDays"] is backfilled with "30" — mirroring the migration tests used for the runs/workbench keys in use-local-storage.test.ts. A companion test pins the no-keys case: 7d default, and no value invented under either key.

Before / after

On unfixed main (fix stashed), the new migration test fails exactly as the issue describes — the seeded 30d preference is ignored and the default wins:

× migrates a pre-rebrand gittensory window preference forward instead of resetting to 7d
  AssertionError: expected 'off' to be 'on'   // 7d is "on", 30d is "off"

With the fix:

✓ src/routes/app.analytics.test.tsx (4 tests)
✓ src/lib/use-local-storage.test.ts (11 tests)

…legacyKey fallback

app.analytics.tsx called useLocalStorage with no third legacyKey
argument, unlike its rebrand siblings app.runs.tsx and app.workbench.tsx.
The storage key was renamed from gittensory.analytics.windowDays to
loopover.analytics.windowDays in the same rebrand commit (75450f1)
that added the legacyKey migration elsewhere, so a returning user who
had picked a 30d/90d window silently reverted to the 7-day default.

Pass gittensory.analytics.windowDays as the legacy key: the hook reads
it once when the new key is absent, adopts the value, and backfills the
new key, exactly like the runs/workbench view preferences.

Closes JSONbored#8699
@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 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-26 01:19:44 UTC

2 files · 1 AI reviewer · no blockers · CI failing · blocked

🛑 Suggested Action - Fix Blockers

Review summary
This is a minimal, well-scoped fix: it adds the missing third `legacyKey` argument ("gittensory.analytics.windowDays") to the `useLocalStorage` call in app.analytics.tsx, mirroring the existing pattern in app.runs.tsx/app.workbench.tsx, so returning users' pre-rebrand 30d/90d preference is picked up instead of silently reverting to the 7-day default. The change is a one-line fix plus two well-targeted tests (migration case and no-keys-present case) that exercise the real hook behavior rather than fabricating state, and it closes the linked issue #8699 directly. The diff is coherent, narrowly scoped, and the tests assert against actual rendered DOM state and the real `useApiResource` call args rather than mocking around the fix.

Nits — 4 non-blocking
  • The inline comment on app.analytics.tsx:175-176 duplicates prose already in the PR description; a shorter one-liner matching the terser style of app.runs.tsx's own legacyKey comment (if any) would keep the diff leaner.
  • Test file doesn't verify that the legacy key (`gittensory.analytics.windowDays`) is left in place after migration, only that the new key is backfilled — worth asserting explicitly since the PR description claims 'leaving the legacy key in place.'
  • Consider asserting `window.localStorage.getItem("gittensory.analytics.windowDays")` is still `"30"` after migration in the first test, to fully validate the 'leaves legacy key in place' claim from the description.
  • If app.runs.tsx/app.workbench.tsx have a shorter/no comment for their legacyKey argument, align app.analytics.tsx's comment length for consistency across the three call sites.

CI checks failing

  • validate
  • validate-code

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 #8699
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: 88 registered-repo PR(s), 40 merged, 4 issue(s).
Contributor context ✅ Confirmed Gittensor contributor tryeverything24; Gittensor profile; 88 PR(s), 4 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff adds the missing "gittensory.analytics.windowDays" legacyKey argument to the useLocalStorage call in app.analytics.tsx, matching the sibling pattern, and adds a migration test that seeds the legacy key, mounts ProductAnalytics, verifies the 30d toggle is selected, and confirms the new key is backfilled, plus a companion default-case test.

Review context
  • Author: tryeverything24
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: JavaScript, TypeScript, Python, HTML, C++, Java, PHP, C#
  • Official Gittensor activity: 88 PR(s), 4 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
/app/analytics/test desktop before /app/analytics/test
before /app/analytics/test
after /app/analytics/test
after /app/analytics/test
/app/analytics/test mobile before /app/analytics/test (mobile)
before /app/analytics/test (mobile)
after /app/analytics/test (mobile)
after /app/analytics/test (mobile)
/app/analytics desktop before /app/analytics
before /app/analytics
after /app/analytics
after /app/analytics
/app/analytics mobile before /app/analytics (mobile)
before /app/analytics (mobile)
after /app/analytics (mobile)
after /app/analytics (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)
/app/analytics/test before /app/analytics/test (scroll)
before /app/analytics/test (scroll)
after /app/analytics/test (scroll)
after /app/analytics/test (scroll)
/app/analytics before /app/analytics (scroll)
before /app/analytics (scroll)
after /app/analytics (scroll)
after /app/analytics (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 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

This pull request changes UI/visual code but its screenshot evidence is incomplete. Every required viewport × theme combination needs its own before/after image pair in a labeled table row (e.g. "Desktop · Light | before | after"). Still missing: Desktop · Dark, Tablet · Dark, Mobile · Dark.

Please resubmit with the remaining rows filled in.

See https://github.com/JSONbored/loopover/blob/main/.claude/skills/contributing-to-loopover/SKILL.md for the exact format and examples. This is an automated maintenance action.

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.

fix(ui): analytics window preference has no rebrand-migration legacyKey fallback, silently resetting returning users to the 7-day default

1 participant