Add data health diagnostics and simplify auth logging - #787
Conversation
httpBatchStreamLink responses were being buffered by the compression middleware (zlib Z_NO_FLUSH default), causing the web dashboard to appear stuck in a loading state until all 19 batch queries completed. Switch to httpBatchLink which returns a single JSON response and works correctly with compression. Also adds: - sync.dataHealth diagnostic query comparing materialized view vs base table row counts to identify stale views - Warning log when dailyMetrics.trends returns all nulls (empty view) - Session resolution logging for all /api/auth/me requests (not just mobile) https://claude.ai/code/session_01EqYnYbGt92Aw7Vqun8b3KK
|
Storybook preview for This comment updates automatically on each PR push. |
Mobile Preview
To test on device:
|
The baseline migration crash-looped on production: the migration tracking table (drizzle.__drizzle_migrations) was empty after the squash rollout, so the runner tried to execute the baseline against a DB that already had all tables, failing with "relation already exists" every ~7 seconds. This put PostgreSQL into recovery mode, breaking auth (/api/auth/me → 500) and all data queries. The existing skip logic only checked appliedSet.size > 0. Now it also checks information_schema for existing tables in the fitness schema, covering the case where migration tracking was reset but the DB still has data. https://claude.ai/code/session_01EqYnYbGt92Aw7Vqun8b3KK
There was a problem hiding this comment.
Pull request overview
Adds diagnostic support for detecting stale fitness materialized views and streamlines /api/auth/me logging behavior.
Changes:
- Added
sync.dataHealthdiagnostic query to compare base-table vs view row counts and flag stale/empty views. - Added warning logging to
DailyMetricsRepository.getTrends()when trends return all nulls (potentially indicating stale views). - Simplified
/api/auth/melogging (removed mobile user-agent detection) and updated tests accordingly; switched web client tohttpBatchLink.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web/src/lib/trpc.ts | Switches tRPC client link from streaming batch to standard batch link. |
| packages/server/src/routes/auth/session.ts | Logs resolved userId for all /api/auth/me requests (removes mobile-only logging). |
| packages/server/src/routes/auth.test.ts | Updates tests to validate unified /api/auth/me logging behavior. |
| packages/server/src/routers/sync.ts | Adds dataHealth diagnostic endpoint comparing base tables vs materialized views. |
| packages/server/src/repositories/daily-metrics-repository.ts | Adds warning when trends query returns all-null results (stale-view hint). |
Comments suppressed due to low confidence (1)
packages/server/src/routes/auth/session.ts:50
- After
if (rows.length === 0) return,rows[0]can’t be undefined, so the subsequentconst row = rows[0]; if (!row) { ... }check is redundant. Consider removing the second check (or collapsing to a singleconst row = rows[0]; if (!row) ...) to simplify control flow around this new logging.
if (rows.length === 0) {
res.status(401).json({ error: "User not found" });
return;
}
logger.info(`[auth] /me resolved userId=${session.userId}`);
const row = rows[0];
if (!row) {
res.status(401).json({ error: "User not found" });
return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Diagnostic: compare materialized view row counts vs base table row counts. | ||
| * Helps identify when views are empty/stale but base tables have data. */ | ||
| dataHealth: protectedProcedure.query(async ({ ctx }) => { | ||
| const { sql: sqlTag } = await import("drizzle-orm"); | ||
| const { executeWithSchema } = await import("../lib/typed-sql.ts"); |
There was a problem hiding this comment.
The new dataHealth procedure isn’t covered by the existing syncRouter tests (there are already extensive tests in packages/server/src/routers/sync.test.ts, e.g. for providerStats). Please add unit tests for dataHealth to validate the returned counts/shape and that hasStaleViews toggles correctly when baseTable>0 but view=0.
| dataHealth: protectedProcedure.query(async ({ ctx }) => { | ||
| const { sql: sqlTag } = await import("drizzle-orm"); | ||
| const { executeWithSchema } = await import("../lib/typed-sql.ts"); | ||
|
|
There was a problem hiding this comment.
dataHealth uses await import(...) for drizzle-orm and executeWithSchema even though these are standard server-side dependencies. Consider switching to normal top-level imports for consistency with the rest of this file and to avoid adding an extra async step on every request.
|
|
||
| const [baseMetrics, viewMetrics, baseSleep, viewSleep, baseActivity, viewActivity] = | ||
| await Promise.all([ | ||
| executeWithSchema( | ||
| ctx.db, | ||
| countSchema, | ||
| sqlTag`SELECT count(*)::int AS count FROM fitness.daily_metrics WHERE user_id = ${ctx.userId}`, | ||
| ), | ||
| executeWithSchema( | ||
| ctx.db, | ||
| countSchema, | ||
| sqlTag`SELECT count(*)::int AS count FROM fitness.v_daily_metrics WHERE user_id = ${ctx.userId}`, | ||
| ), | ||
| executeWithSchema( | ||
| ctx.db, | ||
| countSchema, | ||
| sqlTag`SELECT count(*)::int AS count FROM fitness.sleep_session WHERE user_id = ${ctx.userId}`, | ||
| ), | ||
| executeWithSchema( | ||
| ctx.db, | ||
| countSchema, | ||
| sqlTag`SELECT count(*)::int AS count FROM fitness.v_sleep WHERE user_id = ${ctx.userId}`, | ||
| ), | ||
| executeWithSchema( | ||
| ctx.db, | ||
| countSchema, | ||
| sqlTag`SELECT count(*)::int AS count FROM fitness.activity WHERE user_id = ${ctx.userId}`, | ||
| ), | ||
| executeWithSchema( | ||
| ctx.db, | ||
| countSchema, | ||
| sqlTag`SELECT count(*)::int AS count FROM fitness.v_activity WHERE user_id = ${ctx.userId}`, | ||
| ), | ||
| ]); | ||
|
|
||
| const health = { | ||
| dailyMetrics: { | ||
| baseTable: baseMetrics[0]?.count ?? 0, | ||
| materializedView: viewMetrics[0]?.count ?? 0, | ||
| }, | ||
| sleep: { baseTable: baseSleep[0]?.count ?? 0, materializedView: viewSleep[0]?.count ?? 0 }, | ||
| activity: { | ||
| baseTable: baseActivity[0]?.count ?? 0, | ||
| materializedView: viewActivity[0]?.count ?? 0, | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
The 6 executeWithSchema calls are largely duplicated (same schema, only table name changes). This increases the chance of inconsistencies when adding/removing tables later. Consider defining a small table/view config list and mapping it into Promise.all, then building health from the results.
| const [baseMetrics, viewMetrics, baseSleep, viewSleep, baseActivity, viewActivity] = | |
| await Promise.all([ | |
| executeWithSchema( | |
| ctx.db, | |
| countSchema, | |
| sqlTag`SELECT count(*)::int AS count FROM fitness.daily_metrics WHERE user_id = ${ctx.userId}`, | |
| ), | |
| executeWithSchema( | |
| ctx.db, | |
| countSchema, | |
| sqlTag`SELECT count(*)::int AS count FROM fitness.v_daily_metrics WHERE user_id = ${ctx.userId}`, | |
| ), | |
| executeWithSchema( | |
| ctx.db, | |
| countSchema, | |
| sqlTag`SELECT count(*)::int AS count FROM fitness.sleep_session WHERE user_id = ${ctx.userId}`, | |
| ), | |
| executeWithSchema( | |
| ctx.db, | |
| countSchema, | |
| sqlTag`SELECT count(*)::int AS count FROM fitness.v_sleep WHERE user_id = ${ctx.userId}`, | |
| ), | |
| executeWithSchema( | |
| ctx.db, | |
| countSchema, | |
| sqlTag`SELECT count(*)::int AS count FROM fitness.activity WHERE user_id = ${ctx.userId}`, | |
| ), | |
| executeWithSchema( | |
| ctx.db, | |
| countSchema, | |
| sqlTag`SELECT count(*)::int AS count FROM fitness.v_activity WHERE user_id = ${ctx.userId}`, | |
| ), | |
| ]); | |
| const health = { | |
| dailyMetrics: { | |
| baseTable: baseMetrics[0]?.count ?? 0, | |
| materializedView: viewMetrics[0]?.count ?? 0, | |
| }, | |
| sleep: { baseTable: baseSleep[0]?.count ?? 0, materializedView: viewSleep[0]?.count ?? 0 }, | |
| activity: { | |
| baseTable: baseActivity[0]?.count ?? 0, | |
| materializedView: viewActivity[0]?.count ?? 0, | |
| }, | |
| }; | |
| const healthChecks = [ | |
| { | |
| key: "dailyMetrics", | |
| baseTable: "fitness.daily_metrics", | |
| materializedView: "fitness.v_daily_metrics", | |
| }, | |
| { | |
| key: "sleep", | |
| baseTable: "fitness.sleep_session", | |
| materializedView: "fitness.v_sleep", | |
| }, | |
| { | |
| key: "activity", | |
| baseTable: "fitness.activity", | |
| materializedView: "fitness.v_activity", | |
| }, | |
| ] as const; | |
| const countForTable = (tableName: string) => | |
| sqlTag`SELECT count(*)::int AS count FROM ${sqlTag.raw(tableName)} WHERE user_id = ${ctx.userId}`; | |
| const countTargets = healthChecks.flatMap(({ key, baseTable, materializedView }) => [ | |
| { key, target: "baseTable" as const, tableName: baseTable }, | |
| { key, target: "materializedView" as const, tableName: materializedView }, | |
| ]); | |
| const counts = await Promise.all( | |
| countTargets.map(({ tableName }) => | |
| executeWithSchema(ctx.db, countSchema, countForTable(tableName)), | |
| ), | |
| ); | |
| const health = countTargets.reduce< | |
| Record<string, { baseTable: number; materializedView: number }> | |
| >((acc, { key, target }, index) => { | |
| const count = counts[index]?.[0]?.count ?? 0; | |
| if (!acc[key]) { | |
| acc[key] = { baseTable: 0, materializedView: 0 }; | |
| } | |
| acc[key][target] = count; | |
| return acc; | |
| }, {}); |
| const result = rows[0] ?? null; | ||
| if (result && result.latest_date === null && result.avg_resting_hr === null) { | ||
| logger.warn( | ||
| `[daily-metrics] Trends query returned all nulls for user ${this.userId} (days=${days}, endDate=${endDate}). ` + | ||
| "Materialized view fitness.v_daily_metrics may be empty — check sync.dataHealth.", | ||
| ); |
There was a problem hiding this comment.
This warning triggers whenever the trends query returns an all-null aggregate row, which is also the expected SQL result when the user legitimately has no rows in fitness.v_daily_metrics for the requested window. That can create noisy/false-positive warnings for new/inactive users. Consider only warning when you can confirm base-table data exists (e.g., a cheap EXISTS/COUNT on fitness.daily_metrics for the same user) or otherwise tighten the condition so it specifically indicates a stale view rather than simply “no data”.
Mobile Preview
To test on device:
|
Fixes CI diff-coverage failure by adding tests for: - sync.dataHealth diagnostic query (row counts, stale view detection) - DailyMetricsRepository.getTrends warning when view returns all nulls https://claude.ai/code/session_01EqYnYbGt92Aw7Vqun8b3KK
Mobile Preview
To test on device:
|
Mobile Preview
To test on device:
|
…aming Reverts httpBatchLink back to httpBatchStreamLink (which E2E tests depend on) and instead fixes the root cause: compression middleware's default Z_NO_FLUSH buffers streamed tRPC chunks until the internal zlib buffer fills (~16KB). Setting flush to Z_SYNC_FLUSH ensures each chunk is delivered to the client immediately, so streaming results appear incrementally instead of all at once after all queries complete. https://claude.ai/code/session_01EqYnYbGt92Aw7Vqun8b3KK
Mobile Preview
To test on device:
|
…warning - Replace dynamic imports in dataHealth with top-level imports for drizzle-orm sql and executeWithSchema (CR: consistency) - Extract duplicated executeWithSchema calls into a healthChecks config list with map/reduce (CR: DRY) - Only warn on all-null trends when the base table actually has data, avoiding false-positive noise for new/inactive users (CR: noisy log) https://claude.ai/code/session_01EqYnYbGt92Aw7Vqun8b3KK
Mobile Preview
To test on device:
|
|
Preview torn down 🗑️ |
Summary
This PR adds diagnostic tooling to detect stale materialized views in the fitness data schema and simplifies authentication logging by removing mobile-specific detection.
Key Changes
New
dataHealthdiagnostic endpoint (sync.dataHealth): Compares row counts between base tables and materialized views (daily_metrics, sleep, activity) to identify when views are empty but base tables contain data. Logs warnings when stale views are detected.Enhanced daily metrics repository: Added logging in
getTrends()to warn when the query returns all nulls, suggesting a stale materialized view. Includes helpful diagnostic message pointing users tosync.dataHealth.Simplified auth logging: Removed mobile user-agent detection (
Darwin/CFNetworkchecks) from/api/auth/meendpoint. Now logs userId for all requests uniformly instead of only for mobile clients.Updated test suite: Replaced four mobile-specific user-agent tests with a single unified test that verifies userId logging for all requests.
TRPC client update: Changed from
httpBatchStreamLinktohttpBatchLinkfor more predictable batch request handling.Implementation Details
The
dataHealthendpoint uses parallel queries to fetch counts from both base tables and their corresponding materialized views, then identifies discrepancies. This helps diagnose sync issues where data exists in source tables but hasn't been materialized into views.https://claude.ai/code/session_01EqYnYbGt92Aw7Vqun8b3KK