feat(frontend): add analytics dashboard at /dashboard/analytics - #539
Conversation
9d44119 to
d9abfaa
Compare
|
@claude-review |
|
Claude finished @zaira-bibi's task in 2m 38s —— View job Claude Code Review — Analytics dashboard
Reviewed all 15 files against What's good 👍
Suggestions (all non-blocking)
Details are in the three inline comments. Notes
Overall: approve-worthy pending the author's take on the suggestions (none blocking). |
| const load = useCallback(async () => { | ||
| if (!token) return; | ||
| setState({ status: "loading" }); | ||
| try { | ||
| const points = await fetchLoginActivity(token, { days: DAYS }); | ||
| setState({ status: "ready", points }); | ||
| } catch (err) { | ||
| if (err instanceof ApiRequestError && err.status === 403) { | ||
| setState({ status: "forbidden" }); | ||
| } else { | ||
| setState({ status: "error", message: "We couldn't load analytics. Please try again." }); | ||
| } | ||
| } | ||
| }, [token]); | ||
|
|
||
| useEffect(() => { | ||
| load(); | ||
| }, [load]); |
There was a problem hiding this comment.
Suggestion (Low/Medium): the fetch effect has no cancellation guard. If the component unmounts mid-request, or token changes while a request is in flight, the .then/catch still calls setState. On rapid token changes an earlier slow response can also land after a newer one and overwrite it (stale-response race). React 19 no longer warns on post-unmount setState, but the stale-overwrite is still a real ordering bug.
A small active flag closes both:
useEffect(() => {
if (!token) return;
let active = true;
setState({ status: "loading" });
fetchLoginActivity(token, { days: DAYS })
.then((points) => {
if (active) setState({ status: "ready", points });
})
.catch((err) => {
if (!active) return;
if (err instanceof ApiRequestError && err.status === 403) {
setState({ status: "forbidden" });
} else {
setState({ status: "error", message: "We couldn't load analytics. Please try again." });
}
});
return () => {
active = false;
};
}, [token]);Minor for this page (token rarely changes here), so non-blocking — but it's a cheap, standard guard.
| <div className="space-y-6"> | ||
| <div className="grid grid-cols-1 sm:grid-cols-2 gap-4"> | ||
| <StatCard title="Total logins" value={total} hint={`last ${DAYS} days`} /> | ||
| <StatCard title="Busiest day" value={busiest?.value ?? "—"} hint={busiest?.label} /> |
There was a problem hiding this comment.
Nit (Low): the "Busiest day" hint renders a raw ISO date (busiest?.label → 2026-07-23). Consider a friendlier format (e.g. Jul 23) via Intl.DateTimeFormat / toLocaleDateString for the tile — the ISO label is fine on the chart axis where space is tight, but the stat tile has room. Purely cosmetic.
d9abfaa to
302982c
Compare
8705fe8 to
d9ba0f2
Compare
d9ba0f2 to
7ead835
Compare
7ead835 to
a49eee8
Compare
a49eee8 to
863ac4b
Compare
|
@claude-review |
|
Claude finished @zaira-bibi's task in 2m 7s —— View job Claude Code Review — Analytics dashboard (re-review)
Re-reviewed all 19 files against Previous review comments
I have not auto-resolved @abdulrafey1's threads — I'll leave that to a human reviewer, but from the code they're both satisfied. What's good 👍
Suggestions (all non-blocking, unchanged from last round)
Minor notes
Overall: approve-worthy. @abdulrafey1's feedback is resolved; only my three earlier non-blocking suggestions remain, at the author's discretion. |
regisb
left a comment
There was a problem hiding this comment.
I'm definitely not the right person to review this frontend PR but I approve of this.
863ac4b to
d365895
Compare
d365895 to
c29095f
Compare
abdulrafey1
left a comment
There was a problem hiding this comment.
I went through the PR with the help of claude, found some small issues, nothing major.
I think we all should perform claude-assisted in-depth code reviews for frontend so all of us can get a better idea of what are the common mistakes in the frontend code made by AI and how can we fix them
c29095f to
e44d63f
Compare
Milestone 1's last leg: login activity is visible in-app. A client component fetches GET /api/v1/analytics/login-activity once, zero-fills the sparse (zero-day-omitting) response into a continuous 30-day series, and renders it as a hand-rolled SVG bar chart plus two stat tiles (total logins, busiest day) — derived from the same windowed series so the headline numbers match the bars. The sidebar's Analytics nav entry is gated by asking the backend GET /api/v1/permissions/can?permission=analytics.read (via a new lib/permissions client), threaded through the dashboard layout as a boolean that fails closed — there is no admin bypass. On a 403 (defense in depth for direct navigation), AnalyticsPage redirects to /dashboard rather than showing a message, so it discloses nothing about the gated page. No new dependencies: BarChart and StatCard are added to the in-repo shadcn-style components/ui set (themed SVG, dark-mode-native). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e44d63f to
36b5be2
Compare
Closes: #487 — Analytics are not visible anywhere in the product UI
What
Makes login activity visible in-app at
/dashboard/analytics— a hand-rolled SVG bar chart plus two stat tiles, sourced from the existingGET /api/v1/analytics/login-activityread API — closing Milestone 1's last (UI) leg.Changes
lib/analytics— typedfetchLoginActivity(token, { days })client mirroringlib/llmcomponents/ui/BarChart— zero-dependency hand-rolled SVG bar chart primitive (shadcn recipe: themed, dark-mode-native,role="img")components/ui/StatCard— stat tile composed onCardapp/dashboard/analytics— client page (fetch-once, client-side zero-fill of the sparse response into a continuous 30-day series, two derived stats from that same series, four UI states: loading / 403 / error / ready-or-empty) + route wrapperlib/permissions—checkPermission(token, permission)client calling feat(permissions): add GET /permissions/can check endpoint #533'sGET /api/v1/permissions/cancanViewAnalyticsboolean the dashboard layout fetches once and fails closed on error), threaded through the layout + mobile sidebarHow to Test
make frontend.install.dev(if needed), thenmake test.frontend.vitest→ all green.cd frontend && bun run typecheckandmake lint.frontend→ clean.make backend.up.dev+make frontend.up.dev): grant a user a role withanalytics.readat the global scope (make cli -- roles assign-role <user> <role>), log in → the Analytics nav entry appears and/dashboard/analyticsrenders the bar chart + two stat tiles.analytics.read→ no nav entry; direct navigation redirects to the dashboard.Notes
gh stack sync --pruneonce feat(permissions): add GET /permissions/can check endpoint #533 lands to rebase ontomain).GET /permissions/can(not a permissions list on/user/me) — a UI-gating convenience, not a security boundary; endpoints still enforce their own permissions and return 403.This PR description was written with the assistance of an LLM (Claude).