feat(dashboard): add weekly users metrics for projects#1412
Open
mantrakp04 wants to merge 1 commit intodevfrom
Open
feat(dashboard): add weekly users metrics for projects#1412mantrakp04 wants to merge 1 commit intodevfrom
mantrakp04 wants to merge 1 commit intodevfrom
Conversation
- Introduced a new API endpoint to fetch weekly and daily user metrics for managed projects. - Updated the dashboard to utilize this new endpoint, replacing the previous daily active users data. - Created a new component to visualize weekly users metrics in the project cards. - Refactored existing components to accommodate the new data structure and ensure proper rendering of user activity charts. This change enhances the analytics capabilities of the dashboard, providing better insights into user engagement over time.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR migrates metrics display from Daily Active Users (DAU) to Weekly Users across the backend API and dashboard frontend. The backend endpoint restructures ClickHouse queries to fetch weekly user counts and daily user series separately, updating the response schema. The dashboard frontend updates to consume the new schema and passes weekly user metrics through component props to render a redesigned weekly users metric instead of the prior DAU sparkline. ChangesWeekly Users Metrics Migration
Sequence DiagramsequenceDiagram
participant Dashboard as Dashboard Client
participant API as Backend API Endpoint
participant CH as ClickHouse
participant Card as ProjectCard
participant Metric as ProjectWeeklyUsersMetric
Dashboard->>API: GET /internal/projects-weekly-users
activate API
API->>CH: Query weekly user counts
activate CH
CH-->>API: weeklyUsers per project
deactivate CH
API->>CH: Query daily distinct users
activate CH
CH-->>API: dailyUsers per project/day
deactivate CH
API-->>Dashboard: { projects: { [id]: { weekly_users, daily_users } } }
deactivate API
Dashboard->>Dashboard: Update state:<br/>projectWeeklyUsers<br/>projectWeeklyUsersChart
Dashboard->>Card: weeklyUsers=<count><br/>weeklyUsersChart=<data>
Card->>Metric: weeklyUsers=<count><br/>data=<data>
Metric->>Metric: Render weekly count<br/>+ daily activity chart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
Greptile SummaryThis PR renames the
Confidence Score: 3/5The dashboard and component changes are safe, but the backend route has a failure-mode where a transient error on the second ClickHouse query silently zeroes out the weekly count that was already successfully fetched. The two ClickHouse queries share a single try/catch: if the daily query fails after the weekly query has already returned data, the handler discards the weekly results and returns all-zero counts. This is an observable regression in reliability relative to the single-query original. apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx — the shared try/catch and sequential queries both warrant a closer look before merging. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant Dashboard as Dashboard (page-client.tsx)
participant API as Backend /internal/projects-weekly-users
participant CH as ClickHouse
Browser->>Dashboard: Load projects page
Dashboard->>API: GET /internal/projects-weekly-users
API->>CH: Query 1 — weekly uniq users (7-day window)
CH-->>API: rows [{projectId, weeklyUsers}]
API->>CH: Query 2 — daily uniq users per day (7-day window)
CH-->>API: dailyRows [{projectId, day, dailyUsers}]
API-->>Dashboard: {projects: {id: {weekly_users, daily_users[]}}}
Dashboard->>Dashboard: Split into weeklyUsersMap + weeklyUsersChartMap
Dashboard-->>Browser: Render ProjectCard with ProjectWeeklyUsersMetric
Note over Browser: Shows X users/wk headline + 7-day sparkline chart
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
79
to
125
| @@ -92,13 +124,13 @@ export const GET = createSmartRouteHandler({ | |||
| }, | |||
| format: "JSONEachRow", | |||
Contributor
There was a problem hiding this comment.
The weekly and daily queries are independent and fired sequentially. Since neither depends on the other's result, they can be issued concurrently with Promise.all, roughly halving the round-trip time for this endpoint.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx
Line: 79-125
Comment:
**Sequential ClickHouse queries double API latency**
The weekly and daily queries are independent and fired sequentially. Since neither depends on the other's result, they can be issued concurrently with `Promise.all`, roughly halving the round-trip time for this endpoint.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
57
to
+63
|
|
||
| const byProject: Record<string, { date: string, activity: number }[]> = {}; | ||
| const byProject: Record<string, { weekly_users: number, daily_users: { date: string, activity: number }[] }> = {}; | ||
| for (const id of projectIds) { | ||
| byProject[id] = emptySeries(); | ||
| byProject[id] = { | ||
| weekly_users: 0, | ||
| daily_users: emptySeries(), | ||
| }; |
Contributor
There was a problem hiding this comment.
Map instead
byProject is a plain Record<string, …> indexed by ClickHouse-returned project IDs, which violates the team rule of using Map<K, V> for dynamic-key collections to avoid prototype pollution. A project ID equal to __proto__, constructor, or toString would silently corrupt the object. Switching to new Map<string, …>() with map.set / map.get eliminates this class of risk.
Rule Used: Use Map<A, B> instead of plain objects when using ... (source)
Learned From
stack-auth/stack-auth#769
stack-auth/stack-auth#835
stack-auth/stack-auth#839
+4 more
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx
Line: 57-63
Comment:
**Plain object with dynamic keys — use `Map` instead**
`byProject` is a plain `Record<string, …>` indexed by ClickHouse-returned project IDs, which violates the team rule of using `Map<K, V>` for dynamic-key collections to avoid prototype pollution. A project ID equal to `__proto__`, `constructor`, or `toString` would silently corrupt the object. Switching to `new Map<string, …>()` with `map.set` / `map.get` eliminates this class of risk.
**Rule Used:** Use Map<A, B> instead of plain objects when using ... ([source](https://app.greptile.com/review/custom-context?memory=cd0e08f7-0df2-43c8-8c71-97091bba4120))
**Learned From**
[stack-auth/stack-auth#769](https://github.com/stack-auth/stack-auth/pull/769)
[stack-auth/stack-auth#835](https://github.com/stack-auth/stack-auth/pull/835)
[stack-auth/stack-auth#839](https://github.com/stack-auth/stack-auth/pull/839)
*+4 more*
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds project-level “weekly users” analytics to the dashboard by introducing a new internal backend endpoint that returns both a weekly unique-user count and a 7-day daily series, and updating the Projects page/cards to consume and visualize that data.
Changes:
- Added
/internal/projects-weekly-usersbackend endpoint returning{ weekly_users, daily_users[] }per managed project. - Updated the Projects page client to fetch and map the new response into per-project weekly counts + chart series.
- Replaced the project-card sparkline with a new weekly-users metric component and updated labeling/gradient IDs accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/dashboard/src/components/project-weekly-users-metric.tsx | Renames/reworks the metric widget to display weekly users and a daily users sparkline. |
| apps/dashboard/src/components/project-card.tsx | Switches the card footer visualization to the new weekly users metric component and new props. |
| apps/dashboard/src/app/(main)/(protected)/(outside-dashboard)/projects/page-client.tsx | Fetches the new weekly-users endpoint and plumbs weekly count + daily series into ProjectCard. |
| apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx | Implements the new internal API endpoint and ClickHouse queries for weekly and daily user metrics. |
Comments suppressed due to low confidence (1)
apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx:107
- The weekly and daily ClickHouse queries are wrapped in a single try/catch. If the weekly query succeeds but the daily query throws (or vice versa), the catch returns the all-zero fallback and discards any partial results already fetched. Consider handling these queries independently (e.g., separate try/catch blocks or Promise.allSettled) so one failing query doesn’t zero-out the other metric.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
133
to
+140
| const body = await response.json(); | ||
| if (body == null || typeof body !== "object" || !("projects" in body) || body.projects == null || typeof body.projects !== "object") { | ||
| console.warn("[projects-dau] unexpected body", body); | ||
| console.warn("[projects-weekly-users] unexpected body", body); | ||
| return; | ||
| } | ||
| const map = new Map<string, { date: string, activity: number }[]>(); | ||
| for (const [projectId, series] of Object.entries(body.projects as Record<string, unknown>)) { | ||
| if (!Array.isArray(series)) continue; | ||
| const weeklyUsersMap = new Map<string, number>(); | ||
| const weeklyUsersChartMap = new Map<string, { date: string, activity: number }[]>(); | ||
| for (const [projectId, value] of Object.entries(body.projects)) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx (1)
76-141: ⚡ Quick winTwo independent ClickHouse queries run sequentially, and a failure of the second query discards results from the first.
Because both queries share a single
try/catch, if the weekly-users query succeeds androwsis populated but the daily-users query throws, the catch block returns the zero-initializedbyProject— silently losing the successfully fetchedweekly_usersdata. Parallelizing withPromise.allalso halves the latency.♻️ Proposed parallel execution
- let rows: { projectId: string, weeklyUsers: number }[] = []; - let dailyRows: { projectId: string, day: string, dailyUsers: number }[] = []; try { const clickhouseClient = getClickhouseAdminClient(); - const result = await clickhouseClient.query({ - query: `...weekly...`, - query_params: { ... }, - format: "JSONEachRow", - }); - rows = await result.json(); - - const dailyResult = await clickhouseClient.query({ - query: `...daily...`, - query_params: { ... }, - format: "JSONEachRow", - }); - dailyRows = await dailyResult.json(); + const [weeklyResult, dailyResult] = await Promise.all([ + clickhouseClient.query({ + query: `...weekly...`, + query_params: { ... }, + format: "JSONEachRow", + }), + clickhouseClient.query({ + query: `...daily...`, + query_params: { ... }, + format: "JSONEachRow", + }), + ]); + const [rows, dailyRows]: [ + { projectId: string, weeklyUsers: number }[], + { projectId: string, day: string, dailyUsers: number }[] + ] = await Promise.all([weeklyResult.json(), dailyResult.json()]); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx` around lines 76 - 141, The current single try/catch wraps two sequential ClickHouse queries (the weekly query that sets rows and the daily query that sets dailyRows), so if the second query fails you silently return the zeroed byProject and lose the successful weekly result; fix this by executing both queries concurrently (use Promise.all to run clickhouseClient.query(...) for the weekly and daily queries in parallel), await both responses and call .json() for each result, or alternatively isolate each query in its own try/catch so a failure in the daily query does not discard rows from the weekly query; refer to clickhouseClient.query, rows, dailyRows, and the outer try/catch/captureError to locate where to change execution to parallel or split error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx`:
- Around line 142-144: The loop assigns into
byProject[row.projectId].weekly_users without checking that
byProject[row.projectId] exists; add a defensive guard in the rows iteration
(and mirror it in the dailyRows loop) to skip or initialize when
byProject[row.projectId] is undefined to avoid TypeError. Specifically, inside
the for (const row of rows) { ... } and for (const row of dailyRows) { ... }
blocks, check if (byProject[row.projectId]) before setting
weekly_users/daily_users (or create a new entry if your logic requires
initialization), and ensure you reference the same property names weekly_users
and daily_users on the validated object.
In
`@apps/dashboard/src/app/`(main)/(protected)/(outside-dashboard)/projects/page-client.tsx:
- Around line 126-170: Remove the inner try/catch and the console.warn bail-outs
so errors propagate to runAsynchronously; specifically, in the runAsynchronously
callback that calls appInternals.sendRequest("/internal/projects-weekly-users",
...) remove the catch block and replace the response failure and unexpected-body
console.warns with thrown Errors (or throw new Error with a descriptive message)
so runAsynchronously can handle logging/alerts, while preserving the logic that
builds weeklyUsersMap and weeklyUsersChartMap and only calls
setProjectWeeklyUsers / setProjectWeeklyUsersChart when cancelled is false.
---
Nitpick comments:
In `@apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx`:
- Around line 76-141: The current single try/catch wraps two sequential
ClickHouse queries (the weekly query that sets rows and the daily query that
sets dailyRows), so if the second query fails you silently return the zeroed
byProject and lose the successful weekly result; fix this by executing both
queries concurrently (use Promise.all to run clickhouseClient.query(...) for the
weekly and daily queries in parallel), await both responses and call .json() for
each result, or alternatively isolate each query in its own try/catch so a
failure in the daily query does not discard rows from the weekly query; refer to
clickhouseClient.query, rows, dailyRows, and the outer try/catch/captureError to
locate where to change execution to parallel or split error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1f34ad5-ba86-47ea-b522-335d59f4130d
📒 Files selected for processing (4)
apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsxapps/dashboard/src/app/(main)/(protected)/(outside-dashboard)/projects/page-client.tsxapps/dashboard/src/components/project-card.tsxapps/dashboard/src/components/project-weekly-users-metric.tsx
Comment on lines
142
to
+144
| for (const row of rows) { | ||
| byProject[row.projectId].weekly_users = Number(row.weeklyUsers); | ||
| } |
Contributor
There was a problem hiding this comment.
Unguarded property access on byProject[row.projectId] — potential TypeError if ClickHouse returns an unexpected row.
byProject is a Record<string, …> and TypeScript will not flag byProject[row.projectId] as possibly undefined. At runtime, if the ClickHouse result includes a projectId outside projectIds (e.g., due to a misbehaving ClickHouse node or stale query cache), line 143 throws TypeError: Cannot set properties of undefined. The same guard should be applied to the dailyRows loop at line 147 as a belt-and-suspenders measure.
🛡️ Proposed defensive guard
for (const row of rows) {
+ if (!(row.projectId in byProject)) continue;
byProject[row.projectId].weekly_users = Number(row.weeklyUsers);
}
const dailyIndex = new Map<string, Map<string, number>>();
for (const row of dailyRows) {
+ if (!(row.projectId in byProject)) continue;
const dayKey = row.day.split("T")[0];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx`
around lines 142 - 144, The loop assigns into
byProject[row.projectId].weekly_users without checking that
byProject[row.projectId] exists; add a defensive guard in the rows iteration
(and mirror it in the dailyRows loop) to skip or initialize when
byProject[row.projectId] is undefined to avoid TypeError. Specifically, inside
the for (const row of rows) { ... } and for (const row of dailyRows) { ... }
blocks, check if (byProject[row.projectId]) before setting
weekly_users/daily_users (or create a new entry if your logic requires
initialization), and ensure you reference the same property names weekly_users
and daily_users on the validated object.
Comment on lines
126
to
170
| runAsynchronously(async () => { | ||
| try { | ||
| const response = await appInternals.sendRequest("/internal/projects-dau", {}, "client"); | ||
| const response = await appInternals.sendRequest("/internal/projects-weekly-users", {}, "client"); | ||
| if (!response.ok) { | ||
| console.warn("[projects-dau] request failed", response.status, await response.text()); | ||
| console.warn("[projects-weekly-users] request failed", response.status, await response.text()); | ||
| return; | ||
| } | ||
| const body = await response.json(); | ||
| if (body == null || typeof body !== "object" || !("projects" in body) || body.projects == null || typeof body.projects !== "object") { | ||
| console.warn("[projects-dau] unexpected body", body); | ||
| console.warn("[projects-weekly-users] unexpected body", body); | ||
| return; | ||
| } | ||
| const map = new Map<string, { date: string, activity: number }[]>(); | ||
| for (const [projectId, series] of Object.entries(body.projects as Record<string, unknown>)) { | ||
| if (!Array.isArray(series)) continue; | ||
| const weeklyUsersMap = new Map<string, number>(); | ||
| const weeklyUsersChartMap = new Map<string, { date: string, activity: number }[]>(); | ||
| for (const [projectId, value] of Object.entries(body.projects)) { | ||
| if (value == null || typeof value !== "object") { | ||
| continue; | ||
| } | ||
| const weeklyUsers = "weekly_users" in value ? value.weekly_users : undefined; | ||
| if (typeof weeklyUsers === "number") { | ||
| weeklyUsersMap.set(projectId, weeklyUsers); | ||
| } | ||
| const dailyUsers = "daily_users" in value ? value.daily_users : undefined; | ||
| if (!Array.isArray(dailyUsers)) { | ||
| continue; | ||
| } | ||
| const points: { date: string, activity: number }[] = []; | ||
| for (const point of series) { | ||
| if (point != null && typeof point === "object" && "date" in point && "activity" in point && typeof (point as any).date === "string" && typeof (point as any).activity === "number") { | ||
| points.push({ date: (point as any).date, activity: (point as any).activity }); | ||
| for (const point of dailyUsers) { | ||
| if (point != null && typeof point === "object" && "date" in point && "activity" in point) { | ||
| const date = point.date; | ||
| const activity = point.activity; | ||
| if (typeof date === "string" && typeof activity === "number") { | ||
| points.push({ date, activity }); | ||
| } | ||
| } | ||
| } | ||
| map.set(projectId, points); | ||
| weeklyUsersChartMap.set(projectId, points); | ||
| } | ||
| if (!cancelled) { | ||
| setProjectDau(map); | ||
| setProjectWeeklyUsers(weeklyUsersMap); | ||
| setProjectWeeklyUsersChart(weeklyUsersChartMap); | ||
| } | ||
| } catch (e) { | ||
| console.warn("[projects-dau] fetch error", e); | ||
| console.warn("[projects-weekly-users] fetch error", e); | ||
| } |
Contributor
There was a problem hiding this comment.
Inner try-catch-all with console.warn violates the project guidelines and silences every error from runAsynchronously.
The catch block at line 168 catches every possible error and swallows it with console.warn, which is exactly the pattern the guidelines prohibit: "NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error) (or similar). Use runAsynchronously or runAsynchronouslyWithAlert instead as it deals with error logging." Because all errors are consumed internally, runAsynchronously never sees them and provides no logging benefit. The console.warn calls for a failed HTTP response (line 130) and unexpected body shape (line 135) have the same problem — they silently bail out instead of propagating through runAsynchronously.
🛡️ Proposed fix — let `runAsynchronously` handle error logging
useEffect(() => {
let cancelled = false;
runAsynchronously(async () => {
- try {
const response = await appInternals.sendRequest("/internal/projects-weekly-users", {}, "client");
if (!response.ok) {
- console.warn("[projects-weekly-users] request failed", response.status, await response.text());
- return;
+ throw new Error(`[projects-weekly-users] request failed: ${response.status} ${await response.text()}`);
}
const body = await response.json();
if (body == null || typeof body !== "object" || !("projects" in body) || body.projects == null || typeof body.projects !== "object") {
- console.warn("[projects-weekly-users] unexpected body", body);
- return;
+ throw new Error("[projects-weekly-users] unexpected response body");
}
const weeklyUsersMap = new Map<string, number>();
const weeklyUsersChartMap = new Map<string, { date: string, activity: number }[]>();
for (const [projectId, value] of Object.entries(body.projects)) {
// ... existing parsing logic unchanged ...
}
if (!cancelled) {
setProjectWeeklyUsers(weeklyUsersMap);
setProjectWeeklyUsersChart(weeklyUsersChartMap);
}
- } catch (e) {
- console.warn("[projects-weekly-users] fetch error", e);
- }
});
return () => {
cancelled = true;
};
}, [appInternals, rawProjects.length]);As per coding guidelines: "NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error) (or similar). Use runAsynchronously or runAsynchronouslyWithAlert instead as it deals with error logging."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/dashboard/src/app/`(main)/(protected)/(outside-dashboard)/projects/page-client.tsx
around lines 126 - 170, Remove the inner try/catch and the console.warn
bail-outs so errors propagate to runAsynchronously; specifically, in the
runAsynchronously callback that calls
appInternals.sendRequest("/internal/projects-weekly-users", ...) remove the
catch block and replace the response failure and unexpected-body console.warns
with thrown Errors (or throw new Error with a descriptive message) so
runAsynchronously can handle logging/alerts, while preserving the logic that
builds weeklyUsersMap and weeklyUsersChartMap and only calls
setProjectWeeklyUsers / setProjectWeeklyUsersChart when cancelled is false.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| const response = await appInternals.sendRequest("/internal/projects-weekly-users", {}, "client"); | ||
| if (!response.ok) { | ||
| console.warn("[projects-dau] request failed", response.status, await response.text()); | ||
| console.warn("[projects-weekly-users] request failed", response.status, await response.text()); |
There was a problem hiding this comment.
Missing error state handling when projects-weekly-users API request fails causes UI to conflate failed data loads with legitimate zero activity
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change enhances the analytics capabilities of the dashboard, providing better insights into user engagement over time.
Summary by CodeRabbit