feat(dashboard): add weekly users metrics for projects#1412
Open
mantrakp04 wants to merge 4 commits intodevfrom
Open
feat(dashboard): add weekly users metrics for projects#1412mantrakp04 wants to merge 4 commits intodevfrom
mantrakp04 wants to merge 4 commits 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new internal GET API that returns per-project 7-day weekly user counts and a 7-day daily activity series, updates the dashboard to fetch and display weekly-users metrics instead of DAU, swaps the metric component on project cards, and adds unit tests for the backend merge helper. ChangesWeekly Users Metrics Replacement
Sequence DiagramsequenceDiagram
participant Dashboard as Dashboard Page
participant API as /internal/projects-weekly-users API
participant ClickHouse as ClickHouse
participant Card as ProjectCard
Dashboard->>API: GET /internal/projects-weekly-users
API->>ClickHouse: Query weekly_users by project
API->>ClickHouse: Query daily_users by project & day
ClickHouse-->>API: Return aggregated rows
API->>API: applyProjectWeeklyUsersRows merges rows into per-project map
API-->>Dashboard: 200 { projects: { projectId: { weekly_users, daily_users } } }
Dashboard->>Dashboard: Update state (weeklyUsers, weeklyUsersChart)
Dashboard->>Card: Pass weeklyUsers, weeklyUsersChart
Card->>Card: Render ProjectWeeklyUsersMetric
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 SummaryReplaces the old daily-active-users endpoint (
Confidence Score: 5/5Safe to merge; the two previously raised issues (sequential queries, plain-object Map) are both resolved, and new logic is covered by tests. The backend correctly parallelises ClickHouse queries, uses a Map for all dynamic-key lookups, and exports a testable pure helper. The dashboard changes are straightforward state-splitting with consistent co-updates. No remaining logic bugs or regressions found. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Dashboard as Dashboard (page-client)
participant API as /internal/projects-weekly-users
participant CH as ClickHouse
Dashboard->>API: GET /internal/projects-weekly-users
API->>API: listManagedProjectIds(user)
par concurrent queries
API->>CH: Weekly uniq users per project (7-day window)
API->>CH: Daily uniq users per project (7-day window)
end
CH-->>API: weeklyRows
CH-->>API: dailyRows
API->>API: applyProjectWeeklyUsersRows(byProject, weeklyRows, dailyRows)
API-->>Dashboard: { projects: { [id]: { weekly_users, daily_users[] } } }
Dashboard->>Dashboard: split into weeklyUsersMap + weeklyUsersChartMap
Dashboard->>Dashboard: render ProjectCard → ProjectWeeklyUsersMetric
Reviews (3): Last reviewed commit: "fix(api): adjust date conversion to UTC ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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.
![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
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
mantrakp04
force-pushed
the
fix/daily-to-weekly-active-user
branch
from
4763cac to
12ccfae
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/stack-shared/src/config/schema-fuzzer.test.ts (1)
373-377: ⚡ Quick winAdd the nested-object regression variant for this validation.
This test currently covers only dot-notation. Add a second assertion for nested-object payload (
payments.products.<id>.prices) so both accepted override forms are locked down by tests.🤖 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 `@packages/stack-shared/src/config/schema-fuzzer.test.ts` around lines 373 - 377, Add a second assertion to the test that exercises the nested-object payload form: call assertNoConfigOverrideErrors(branchConfigSchema, { payments: { products: { free: { prices: "include-by-default" } } } }) and assert it rejects with the same validation message; keep the existing dot-notation assertion and mirror the expected regex (/payments\.products\.free\.prices must not be one of the following values: include-by-default/) so both override forms (dot-notation and nested-object) are covered by the test.
🤖 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 `@examples/demo/src/app/payments-demo/api/create-checkout-url/route.ts`:
- Around line 32-33: Wrap the JSON parsing and body validation in a try/catch so
JSON parse/validation failures produce a 400 response instead of bubbling as a
500: replace the direct call to const body = readBody(await request.json()) with
a guarded sequence that calls await request.json() and readBody(...) inside a
try block and on error returns new Response(JSON.stringify({error: err.message
|| 'Invalid request body'}), {status: 400}); keep the call to user.listTeams()
(teams = await user.listTeams()) after successful parsing so downstream logic
only runs for valid input.
In `@examples/demo/src/app/payments-demo/api/send-test-email/route.ts`:
- Around line 25-27: The handler currently calls request.json() and
readCount(body) without error handling, causing malformed input to produce 500s;
wrap the JSON parsing and readCount invocation in a try/catch around the lines
with request.json() and readCount, detect validation/parsing errors, and return
an explicit 400 response with a deterministic structured error body (e.g., {
error: "invalid_request", message: "<short description>" }) instead of throwing;
ensure the catch only converts input/validation errors (from request.json() or
readCount) to 400 while rethrowing or letting other unexpected errors propagate.
In `@examples/demo/src/app/payments-demo/page.tsx`:
- Around line 147-159: Wrap the click handler's async work (the call to
createCheckoutUrl and subsequent window.location.assign) with
runAsynchronouslyWithAlert so any rejection surfaces to the user; keep the
existing setLoading(true) at the start and ensure setLoading(false) runs in
finally, but move the createCheckoutUrl invocation and window.location.assign
into the runAsynchronouslyWithAlert callback (or pass a function that calls
them) so errors are caught and shown via the alert helper instead of being
silently swallowed.
- Line 177: The return currently builds the URL with plain string interpolation;
update the return in the function that produces the internal projects URL to
follow repo convention by using the urlString template tag
(urlString`http://${host}:${portPrefix}01/projects/internal`) or by encoding
dynamic segments with encodeURIComponent (e.g., encodeURIComponent(host) and
encodeURIComponent(portPrefix)) before concatenation so the URL construction for
the host/portPrefix variables matches project standards.
In `@packages/stack-shared/src/config/schema.ts`:
- Around line 1152-1154: The current guard in schema.ts uses the regex
/^payments\.products\.[^.]+\.prices$/ against a flat dot-notation variable named
key and so misses nested-object overrides; update the validation so that before
applying this regex and returning Result.error(...) you either (a) normalize
nested override paths into dot-notation (flatten the nested object keys into the
same "payments.products.<id>.prices" strings) or (b) recursively traverse the
payments.products tree to locate any prices fields and validate their values,
and in both cases ensure any match with value === "include-by-default" triggers
the same Result.error message; reference the existing key variable, the regex
/^payments\.products\.[^.]+\.prices$/, and the Result.error(...) call when
implementing the change.
---
Nitpick comments:
In `@packages/stack-shared/src/config/schema-fuzzer.test.ts`:
- Around line 373-377: Add a second assertion to the test that exercises the
nested-object payload form: call
assertNoConfigOverrideErrors(branchConfigSchema, { payments: { products: { free:
{ prices: "include-by-default" } } } }) and assert it rejects with the same
validation message; keep the existing dot-notation assertion and mirror the
expected regex (/payments\.products\.free\.prices must not be one of the
following values: include-by-default/) so both override forms (dot-notation and
nested-object) are covered by the test.
🪄 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: 602f04a3-dc57-4ddf-ba1c-3fb4555326ea
📒 Files selected for processing (7)
examples/demo/src/app/payments-demo/api/config-check/route.tsexamples/demo/src/app/payments-demo/api/create-checkout-url/route.tsexamples/demo/src/app/payments-demo/api/send-test-email/route.tsexamples/demo/src/app/payments-demo/page.tsxexamples/demo/src/components/header.tsxpackages/stack-shared/src/config/schema-fuzzer.test.tspackages/stack-shared/src/config/schema.ts
…d tests - Introduced the `applyProjectWeeklyUsersRows` function to process weekly and daily user metrics for projects. - Created a new test file to validate the functionality of the new helper, ensuring it correctly applies user data and handles unknown projects. - Updated the existing route to utilize the new function for better code organization and clarity. This change enhances the backend's ability to manage and analyze user engagement metrics for projects.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/backend/src/app/api/latest/internal/projects-weekly-users/route.test.tsx (1)
4-67: ⚡ Quick winConsider one additional case: out-of-window daily rows.
The route's helper relies on
project.daily_users.map(...)looking up only dates already present inemptySeries, so daily rows whosedayis outside the 7-day window are silently dropped. A second test case feeding adailyRowsentry with adaynot in the initialdaily_usersarray would pin down that contract and protect against accidental future changes (e.g., switching toArray.from(dailyIndex.entries())).🧪 Suggested additional test
+ it("ignores daily rows with dates outside the pre-filled window", () => { + const byProject = new Map([ + ["project-a", { + weekly_users: 0, + daily_users: [ + { date: "2026-05-01", activity: 0 }, + { date: "2026-05-02", activity: 0 }, + ], + }], + ]); + + applyProjectWeeklyUsersRows( + byProject, + [{ projectId: "project-a", weeklyUsers: 1 }], + [ + { projectId: "project-a", day: "2026-04-15", dailyUsers: 42 }, // out of window + { projectId: "project-a", day: "2026-05-02", dailyUsers: 3 }, + ], + ); + + expect(byProject.get("project-a")?.daily_users).toMatchInlineSnapshot(); + });🤖 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.test.tsx` around lines 4 - 67, Add a test to ensure applyProjectWeeklyUsersRows ignores dailyRows whose day is outside the initialized 7-day window: in the existing spec for applyProjectWeeklyUsersRows, add a dailyRows entry with a day not present in the project's daily_users array (e.g., a date before/after the window) and assert that the project's daily_users remains unchanged for that date (i.e., the out-of-window row is not inserted/updated). Reference applyProjectWeeklyUsersRows, the dailyRows input, and the project's daily_users/emptySeries to locate where to add the case and the expectation.apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx (2)
171-184: 💤 Low valueCatch-all is acceptable here, but document the intent.
The
try { ... } catch (error) { captureError(...); return empty; }block technically matches the "NEVER try-catch-all" shape from the guidelines, but unlike the disallowed pattern it (a) usescaptureErrorso failures aren't silently swallowed and (b) deliberately degrades gracefully so an analytics outage doesn't break the projects page. A short comment stating that intent (e.g., "ClickHouse failures must not break the projects list; report and return zeros") would help future readers and reviewers avoid re-flagging this.📝 Suggested comment
+ // Analytics is non-critical for rendering the projects list: on ClickHouse + // (or unexpected) failures we report the error and fall back to zeroed + // metrics so the dashboard remains usable. } catch (error) { const captureId = error instanceof ClickHouseError🤖 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 171 - 184, Add a short explanatory comment above the catch block that captures ClickHouseError (the block using captureError, StackAssertionError, ClickHouseError, projectsResponse and projectIds) stating that ClickHouse/analytics failures are intentionally not propagated so the projects list degrades gracefully: we log the error via captureError with context and return an empty/zeroed projectsResponse to avoid breaking the projects page. Keep the existing captureError call and return behavior unchanged; only add the one-line intent comment.
107-107: 💤 Low value
Object.fromEntries(byProject)with arbitrary project IDs — confirmyupRecordand JSON serialization handle reserved keys.If a managed
projectIdwere ever literally"__proto__"(or"constructor"/"toString"),Object.fromEntriescreates it as an own data property (per spec viaCreateDataPropertyOrThrow), but downstream consumers —yupRecordvalidation,JSON.stringify, and the frontend'sObject.entries(body.projects)parsing — should be confirmed safe with such keys. The unit test inroute.test.tsxexercises"__proto__"against the helper directly, but doesn't cover the full route's response-schema validation or JSON round-trip.In practice managed project IDs are CUIDs/UUIDs so this is unlikely to bite, but consider either (a) extending the test to round-trip through
JSON.stringify+yupRecordvalidation, or (b) returning a[projectId, data][]array shape instead of a record — which is also more consistent with the "Use ES6 maps instead of records wherever you can" guideline.As per coding guidelines: "Use ES6 maps instead of records wherever you can."
Also applies to: 191-191
🤖 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` at line 107, The current projectsResponse implementation returns Object.fromEntries(byProject) which can create reserved prototype-named keys; change the route to return an array of [projectId, data] pairs instead of a plain record (i.e. return Array.from(byProject) or the equivalent) and update any consumers/response schema validation (the yupRecord usage and the route response schema) to accept that array shape; also update the test in route.test.tsx to assert a full JSON.stringify -> parse -> validation round-trip for the new array shape (or, if you prefer to keep a record, explicitly serialize via Object.entries and ensure yupRecord and JSON round-trip tests cover "__proto__" edge-case), referencing projectsResponse, byProject, yupRecord, and body.projects in your changes.apps/dashboard/src/app/(main)/(protected)/(outside-dashboard)/projects/page-client.tsx (1)
75-122: 💤 Low valueConsider consolidating the two near-identical fetch effects.
The
useEffectat lines 75–122 (/internal/projects+ onboarding statuses) and the new one at lines 124–169 (/internal/projects-weekly-users) share the sameappInternals/rawProjects.lengthdependency array, the samecancelledcleanup pattern, and similar response-shape validation. Extracting a tinyuseFetchInternal<T>(path, parse)hook (or just a shared validator helper) would reduce duplication and centralize the response validation pattern. Not blocking — only worth doing if more such fetches are anticipated.Also applies to: 124-169
🤖 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 75 - 122, The two near-duplicate useEffect blocks that call appInternals.sendRequest (one handling "/internal/projects" and the other "/internal/projects-weekly-users") should be consolidated to remove duplication: extract a small reusable helper or hook (e.g., useFetchInternal or fetchInternalWithValidation) that accepts the request path and a parse/validate function, reuses the cancelled cleanup pattern and runAsynchronouslyWithAlert wrapper, and returns loading + parsed data; replace the existing effects to call this helper and then call setProjectStatuses / setWeeklyUsers (or respective setters) and setLoadingProjectStatuses from the helper results, keeping existing response-shape validation logic (the instanceof/object checks and isProjectOnboardingStatus) inside the shared validator to centralize error handling and avoid repeating appInternals, cancelled, and dependency arrays.
🤖 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 85-98: Update the SQL usage of toDate(event_at) in the query to
explicitly specify UTC (i.e. toDate(event_at, 'UTC')) so the timezone alignment
is explicit and resilient to schema changes; locate the query that aggregates
over the window defined by since, WINDOW_DAYS and ONE_DAY_MS (the same logic
that builds emptySeries) and replace the implicit toDate call with
toDate(event_at, 'UTC') to ensure consistent day-binning.
---
Nitpick comments:
In
`@apps/backend/src/app/api/latest/internal/projects-weekly-users/route.test.tsx`:
- Around line 4-67: Add a test to ensure applyProjectWeeklyUsersRows ignores
dailyRows whose day is outside the initialized 7-day window: in the existing
spec for applyProjectWeeklyUsersRows, add a dailyRows entry with a day not
present in the project's daily_users array (e.g., a date before/after the
window) and assert that the project's daily_users remains unchanged for that
date (i.e., the out-of-window row is not inserted/updated). Reference
applyProjectWeeklyUsersRows, the dailyRows input, and the project's
daily_users/emptySeries to locate where to add the case and the expectation.
In `@apps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsx`:
- Around line 171-184: Add a short explanatory comment above the catch block
that captures ClickHouseError (the block using captureError,
StackAssertionError, ClickHouseError, projectsResponse and projectIds) stating
that ClickHouse/analytics failures are intentionally not propagated so the
projects list degrades gracefully: we log the error via captureError with
context and return an empty/zeroed projectsResponse to avoid breaking the
projects page. Keep the existing captureError call and return behavior
unchanged; only add the one-line intent comment.
- Line 107: The current projectsResponse implementation returns
Object.fromEntries(byProject) which can create reserved prototype-named keys;
change the route to return an array of [projectId, data] pairs instead of a
plain record (i.e. return Array.from(byProject) or the equivalent) and update
any consumers/response schema validation (the yupRecord usage and the route
response schema) to accept that array shape; also update the test in
route.test.tsx to assert a full JSON.stringify -> parse -> validation round-trip
for the new array shape (or, if you prefer to keep a record, explicitly
serialize via Object.entries and ensure yupRecord and JSON round-trip tests
cover "__proto__" edge-case), referencing projectsResponse, byProject,
yupRecord, and body.projects in your changes.
In
`@apps/dashboard/src/app/`(main)/(protected)/(outside-dashboard)/projects/page-client.tsx:
- Around line 75-122: The two near-duplicate useEffect blocks that call
appInternals.sendRequest (one handling "/internal/projects" and the other
"/internal/projects-weekly-users") should be consolidated to remove duplication:
extract a small reusable helper or hook (e.g., useFetchInternal or
fetchInternalWithValidation) that accepts the request path and a parse/validate
function, reuses the cancelled cleanup pattern and runAsynchronouslyWithAlert
wrapper, and returns loading + parsed data; replace the existing effects to call
this helper and then call setProjectStatuses / setWeeklyUsers (or respective
setters) and setLoadingProjectStatuses from the helper results, keeping existing
response-shape validation logic (the instanceof/object checks and
isProjectOnboardingStatus) inside the shared validator to centralize error
handling and avoid repeating appInternals, cancelled, and dependency arrays.
🪄 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: 8405b0ca-45ad-491d-94db-2da1fc63f78a
📒 Files selected for processing (3)
apps/backend/src/app/api/latest/internal/projects-weekly-users/route.test.tsxapps/backend/src/app/api/latest/internal/projects-weekly-users/route.tsxapps/dashboard/src/app/(main)/(protected)/(outside-dashboard)/projects/page-client.tsx
- Updated the SQL query in the projects weekly users API to convert event dates to UTC using `toDate(event_at, 'UTC')`. - This change ensures consistent date handling across different time zones, improving the accuracy of user metrics. This fix enhances the reliability of the analytics data retrieved from the backend.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/src/app/(main)/(protected)/(outside-dashboard)/projects/page-client.tsx (1)
124-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit loading and error state for the weekly-users fetch.
The new effect populates
projectWeeklyUsers/projectWeeklyUsersChartonly on success and has no companion loading or error state. When the request is in-flight or fails,ProjectCardreceivesweeklyUsers={undefined}(and an empty chart) for every project — visually identical to "this project genuinely has zero weekly activity". This violates the dashboard guideline ("always carefully handle loading and error states. Be explicit with these states and ensure errors are never silently swallowed") and matches the failure mode flagged by Vercel's reviewer.Concretely:
- A failed/in-flight
/internal/projects-weekly-userscall is indistinguishable from "no activity" in the UI.runAsynchronouslywill surface the thrown errors to logs/alerts, but the rendered project cards still mislead the user.Suggested approach: track
loadingWeeklyUsers/weeklyUsersErrorstate, pass them down toProjectCard(e.g., via a discriminatedweeklyUsersStateprop), and render a skeleton while loading and an explicit error indicator (with retry) on failure — mirroring the pattern already used forprojectStatuses/loadingProjectStatusesabove.As per coding guidelines: "When building frontend code, always carefully handle loading and error states. Be explicit with these states and ensure errors are never silently swallowed."
🤖 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 124 - 169, The effect that fetches weekly users (useEffect with runAsynchronously) must track loading and error states so ProjectCard can distinguish "loading/error" from "zero activity": add state variables like loadingWeeklyUsers (boolean) and weeklyUsersError (Error | null) alongside setProjectWeeklyUsers and setProjectWeeklyUsersChart; set loadingWeeklyUsers = true before the request, on success set loadingWeeklyUsers = false and clear weeklyUsersError, and on any thrown error catch it, set weeklyUsersError to the Error and loadingWeeklyUsers = false (do not rethrow silently); update where ProjectCard is rendered to accept a discriminated weeklyUsersState prop (e.g., {status: "loading"} | {status: "error", error: Error} | {status: "ready", data: Map...}) and pass the maps when ready; also expose a retry callback that re-triggers the fetch so the UI can retry on error.
🤖 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.
Outside diff comments:
In
`@apps/dashboard/src/app/`(main)/(protected)/(outside-dashboard)/projects/page-client.tsx:
- Around line 124-169: The effect that fetches weekly users (useEffect with
runAsynchronously) must track loading and error states so ProjectCard can
distinguish "loading/error" from "zero activity": add state variables like
loadingWeeklyUsers (boolean) and weeklyUsersError (Error | null) alongside
setProjectWeeklyUsers and setProjectWeeklyUsersChart; set loadingWeeklyUsers =
true before the request, on success set loadingWeeklyUsers = false and clear
weeklyUsersError, and on any thrown error catch it, set weeklyUsersError to the
Error and loadingWeeklyUsers = false (do not rethrow silently); update where
ProjectCard is rendered to accept a discriminated weeklyUsersState prop (e.g.,
{status: "loading"} | {status: "error", error: Error} | {status: "ready", data:
Map...}) and pass the maps when ready; also expose a retry callback that
re-triggers the fetch so the UI can retry on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fc8bf5f-1cdc-4786-85b8-a5da46c8f88b
📒 Files selected for processing (1)
apps/dashboard/src/app/(main)/(protected)/(outside-dashboard)/projects/page-client.tsx
BilalG1
reviewed
May 6, 2026
Comment on lines
+128
to
+129
| const [weeklyResult, dailyResult] = await Promise.all([ | ||
| clickhouseClient.query({ |
Collaborator
There was a problem hiding this comment.
we can combine these two queries into one to reduce clickhouse memory usage for bigger projects. here's a possible combined query but should double check it:
SELECT
project_id AS projectId,
toDate(event_at, 'UTC') AS day,
uniqExact(assumeNotNull(user_id)) AS users
FROM analytics_internal.events
WHERE event_type = '$token-refresh'
AND project_id IN {projectIds:Array(String)}
AND branch_id = {branchId:String}
AND user_id IS NOT NULL
AND event_at >= {since:DateTime}
AND event_at < {untilExclusive:DateTime}
AND coalesce(CAST(data.is_anonymous, 'Nullable(UInt8)'), 0) = 0
GROUP BY GROUPING SETS ((projectId), (projectId, day))
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
New Features
Updates
Tests