feat: add analytics movement ranges and profile navigation - #445
feat: add analytics movement ranges and profile navigation#445MrParamecium wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 565b6e2224
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rollups = ( | ||
| self.query_store.rollups.list_rollups(start=start, end=end) | ||
| self.query_store.rollups.list_rollups(start=history_start, end=end) |
There was a problem hiding this comment.
When the movement window starts before the selected analytics range—such as a short date range combined with 1y—this fetch now includes anonymous rollups from history_start, but the transition loop only filters by metric and direct_dates. Consequently, lifecycle transitions before start are added to the response and then labeled with the selected period_start/period_end, inflating the reported transition counts for the default internal-excluded query.
Useful? React with 👍 / 👎.
| // The profile controller owns deep-link restoration. Avoid fetching the | ||
| // overview behind an already-open profile on a direct URL or browser back. | ||
| const profileRequested = /^\d+$/.test(state.userFilters.profile); | ||
| if (!profileRequested && (!state.sections.lifecycle.loaded || !state.sections.users.loaded)) refreshPrimary(); |
There was a problem hiding this comment.
For a profile opened directly in a new tab, this gate intentionally skips the overview requests. The profile Back button then takes the no-history fallback in closeProfile(), which merely unhides the overview and replaces the URL without emitting popstate or refreshing AdminAnalyticsValue; the user therefore lands on an empty, never-loaded Analytics overview. Ensure that the in-place fallback synchronizes this controller or triggers the primary load.
Useful? React with 👍 / 👎.
| window_days, _granularity = _MOVEMENT_WINDOWS[movement_range] | ||
| movement_start = end - timedelta(days=window_days) | ||
| current_time = _utc(now or datetime.now(UTC), "now") |
There was a problem hiding this comment.
The movement window is derived from the broader filter's exclusive end, so changing analyticsEnd to a historical date also moves the chart despite the new movement selector being documented and labeled as an independent, recent range. For example, a report filtered to August will show August 27–31 under “Recent 5-day movement” rather than the latest five UTC days; derive the movement endpoint from the current as_of date instead of the report range.
Useful? React with 👍 / 👎.
Review follow-ups on the movement-range/profile work. Backend (value_queries.py) — widening the daily scan to cover the movement window took the requested range away from three readers that assumed it: - transitions summed lifecycle_transition rollups from before `start` while still stamping the result `period_start=start`; the direct-rows loop and the weekly loop both filter, so the rollup loop now does too - availability.history described the movement history rather than the range the admin asked for, and its new coverage clause pinned status to "partial" for every 1Y view - a calendar bucket may open before the chart's own window, so its label is clamped to the window start instead of naming an excluded date The 180-day cap was a bare literal here and a named constant in the router; both now read one exported constant, and the daily scan span is derived from the movement table so a longer range cannot widen it unnoticed. Frontend — `analyticsProfile` had two writers. The value module kept a cached copy that nothing cleared on close, so the overview stayed blank behind a closed profile and the next filter edit put the closed profile back into the URL. The profile controller now owns the param outright and the overview guard reads the live URL; openAccountManagement clears it alongside analyticsUser. The range switch refreshes the chart without refetching priority users, and its roving tabindex is backed by radiogroup semantics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W2SXWfkLjG1SJBzVv8Embh
Every conflict was the cache-buster collision class (app.html plus the three test files that hard-pin `?v=` strings). Resolution: keep this branch's bumped versions for the assets it actually diffs -- styles.css 138, admin-analytics.js 6, admin-analytics-value.js 5, admin-tabs.js 5, each one past main -- and take main's app.js 129, which this branch does not touch. Resolved so CI and CodeQL can run: GitHub builds no merge commit for a conflicting PR, so no workflow had run against the review fixes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W2SXWfkLjG1SJBzVv8Embh
The router kept a private alias equal to the imported constant; use the shared name at its one call site instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W2SXWfkLjG1SJBzVv8Embh
2 participants
Summary
Validation
pytest -q dashboard/backend/tests(4240 passed, 156 skipped)pytest -q dashboard/backend/tests/domain/analytics/test_value_queries.py dashboard/backend/tests/test_admin_analytics_api.py dashboard/backend/tests/test_admin_analytics_frontend.py dashboard/backend/tests/test_admin_analytics_value_frontend.py(68 passed)node --check dashboard/frontend/js/admin-analytics.jsnode --check dashboard/frontend/js/admin-analytics-value.jsgit diff --checkScope notes
origin/main(which includes merged PR feat: expose admin user value analytics APIs #444).dashboard/storage/data/backtest.dband.superpowers/were intentionally not staged.