Add dashboard ClickHouse read models - #1253
Merged
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughAdds three incremental ClickHouse dbt read models (recovery_read_model, strain_read_model, healthspan_read_model), a migration to create their tables, test/bootstrap updates to populate and validate them, and switches multiple server routes and tests to consume these read models. Documentation and the DBT safe-model list were updated. ChangesNamed Dashboard Read Models for Recovery, Strain, and Healthspan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 28 files
Confidence score: 2/5
- There is high user-impact risk in
packages/server/src/routers/healthspan-query.ts: week-rounding the access window can both expose out-of-window data and exclude valid in-window data for limited entitlements. - Timezone handling appears regressed in
packages/server/src/routers/recovery.tsandpackages/server/src/routers/sleep-need.ts, with UTC-based bucketing likely shifting daily load/ACWR and sleep-need results for non-UTC users. - Given multiple medium-to-high severity findings (6–8/10) with strong confidence, this is likely not safe to merge without targeted fixes and validation.
- Pay close attention to
packages/server/src/routers/healthspan-query.ts,packages/server/src/routers/recovery.ts,packages/server/src/routers/sleep-need.ts- entitlement window accuracy, week aggregation correctness, and user-local day boundary logic need verification.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@analytics/models/read_models/healthspan_read_model.sql`:
- Around line 73-80: The vo2_by_week CTE is averaging vo2max into latest_vo2max
which loses ordering; replace the order-insensitive avg(vo2max) with an
order-sensitive aggregate (e.g., argMax(vo2max, activity_date) or
anyLast(vo2max) depending on ClickHouse convention) so vo2_by_week.latest_vo2max
returns the actual last measurement per user/week from {{
ref('activity_vo2max_estimate') }}; keep GROUP BY user_id,
toMonday(activity_date) and ensure activity_date is used as the ordering
argument so later latestNonNullValue() can recover the true latest value.
- Around line 1-9: The incremental model currently appends every run because the
full week_keys set is always recomputed and cross-joined with refresh_clock;
change the SQL so when is_incremental() is true you filter week_keys to only
new/dirty rows (e.g., WHERE (user_id, week_start) NOT IN (SELECT user_id,
week_start FROM analytics.healthspan_read_model) or join against
analytics.healthspan_read_model to exclude existing keys) and only add the new
refresh_version for those rows (i.e., put the CROSS JOIN refresh_clock /
refresh_version assignment inside the {% if is_incremental() %} branch or
conditionally apply it), ensuring incremental_strategy='append' actually appends
only new targets; also update vo2_by_week so latest_vo2max uses a within-week
"latest" aggregator such as argMax(vo2max, activity_date) (replace avg(vo2max)
AS latest_vo2max) so that latestNonNullValue(...) over weeks receives a per-week
latest value instead of an average.
In `@analytics/models/read_models/recovery_read_model.sql`:
- Around line 1-9: The model analytics.recovery_read_model is configured as
incremental (materialized='incremental', incremental_strategy='append',
engine='ReplacingMergeTree(refresh_version)') but lacks an is_incremental()
guard, causing full snapshots to be appended every run; modify the model SQL to
include an incremental filter using is_incremental() that only selects/returns
rows for new or changed (user_id, date) partitions (e.g., filter by source
change timestamp or a date range of affected input rows) so you append only
deltas, or alternatively change incremental_strategy to one that enforces
single-current-row semantics (e.g., use insert_overwrite semantics or ensure a
unique key and merge behavior) and update the model body to emit only the
intended partition/key rows when not a full-refresh.
In `@analytics/models/read_models/strain_read_model.sql`:
- Around line 1-9: The incremental append currently regenerates the full
(user_id, date) history because date_series is computed from min(date) through
greatest(max(date), today()) without an is_incremental() guard; fix it by adding
an is_incremental() branch that reads the current max(date) from {{ this }}
(SELECT max(date) FROM {{ this }}) and bounds the date_series to only the
affected window (use greatest(min(date), date_sub(existing_max_date, INTERVAL
<lookback> DAY)) as the start and greatest(max(date), today()) as the end) so
each run only appends the recent window; use a 6-day lookback for acute and
27-day for chronic, keep the existing incremental_strategy='append' and
refresh_version logic unchanged.
In `@docs/production-incident-baseline.md`:
- Around line 10123-10143: This incident entry is missing the required
"Follow-Up Work" section; add a concise Follow-Up Work paragraph listing
concrete next actions such as: (1) run post-deploy Axiom/Sentry checks to verify
request latency improvements and confirm the UTC daily bucketing in
analytics.strain_read_model is acceptable, (2) add a short-term dashboard
latency alert (route-facing metrics) and a ticket to verify reads from
analytics.recovery_read_model and analytics.healthspan_read_model are stable,
(3) update the production incident baseline and runbook to reflect the new
dbt-owned incremental models and empty-table deploy step, and (4) if no further
action is planned, state "none currently" with a brief rationale.
In `@packages/server/src/routers/recovery.ts`:
- Around line 276-285: The query is converting strain.date through
toDateTime->toTimeZone which can shift day-granular dates; remove that
conversion and use the raw day-granular value instead (e.g., use strain.date
directly or toString(toDate(strain.date)) for the AS date field) in the
workload-ratio SQL fragment that builds the SELECT (the expression producing "AS
date" from strain.date), and make the same change to the similar expression
later (lines referenced around the second occurrence). Ensure the WHERE filters
still use strain.date unchanged so output dates and window semantics stay
consistent.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ee28f90-e6c7-499f-bd91-71d38f3d64ca
📒 Files selected for processing (28)
analytics/README.mdanalytics/models/read_models/healthspan_read_model.sqlanalytics/models/read_models/read_model_microbatch.sql.test.tsanalytics/models/read_models/recovery_read_model.sqlanalytics/models/read_models/strain_read_model.sqldeploy/README.mddocs/production-incident-baseline.mdentrypoint.shpackages/server/src/repositories/stress-repository.test.tspackages/server/src/repositories/stress-repository.tspackages/server/src/routers/clickhouse-integration-test-helpers.test.tspackages/server/src/routers/clickhouse-integration-test-helpers.tspackages/server/src/routers/healthspan-query.test.tspackages/server/src/routers/healthspan-query.tspackages/server/src/routers/healthspan.test.tspackages/server/src/routers/mobile-dashboard.test.tspackages/server/src/routers/mobile-dashboard.tspackages/server/src/routers/recovery-settings-sleep-need-sport-settings.test.tspackages/server/src/routers/recovery.test.tspackages/server/src/routers/recovery.tspackages/server/src/routers/sleep-need.test.tspackages/server/src/routers/sleep-need.tspackages/server/src/routers/strength-stress.test.tspackages/server/src/routers/stress.test.tssrc/db/clickhouse-migrations.test.tssrc/db/clickhouse-migrations/0026_create_named_dashboard_read_model_tables.tssrc/db/clickhouse-migrations/registry.test.tssrc/db/clickhouse-migrations/registry.ts
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: This PR adds new ClickHouse read models and switches multiple core dashboard API routes to use them, involving new business logic, schema changes, and complex queries that could affect data accuracy and system behavior, so human review is needed to verify correctness and data integrity.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: This PR introduces new ClickHouse read models that precompute dashboard metrics (recovery scores, strain, healthspan) and rewrites multiple API routes to query them instead of computing on request, which involves complex business logic in SQL (z-scores, sigmoid functions, windowed loads, weekly...
Re-trigger cubic
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)
packages/server/src/routers/recovery.ts (1)
594-600:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winParse
strainTargetread-model dates withdateStringSchemainstead ofz.string().Line 595 uses
z.string()for a DB date boundary. If a non-date string ever crosses this boundary, thedaysAgocomputation (Line 655) becomesNaNand silently distorts acute/chronic load outputs.Suggested fix
const loads = await sensorStore.query( z.object({ - date: z.string(), + date: dateStringSchema, daily_load: z.coerce.number(), }),As per coding guidelines, "Use
dateStringSchemaandtimestampStringSchemafromsrc/lib/typed-sql.tsto handle DB-to-JS type normalization."🤖 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/server/src/routers/recovery.ts` around lines 594 - 600, Replace the loose z.string() used for the read-model date with the stricter dateStringSchema from src/lib/typed-sql.ts so DB date strings are normalized and validated before daysAgo math; import dateStringSchema and update the z.object that currently has date: z.string() (the schema used for the strain/read-model row with daily_load) to use dateStringSchema instead of z.string().
♻️ Duplicate comments (1)
packages/server/src/routers/recovery.ts (1)
276-290:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse raw
strain.dateinworkloadRatiooutput to prevent day-shift mismatches.On Line 276, converting a day-granular
DatethroughtoDateTime(...)->toTimeZone(...)->toDate(...)can shift the day for non-UTC users, while Lines 283-284 still filter with unshiftedstrain.date. That can return rows inside the filter window but label them as a different calendar day.Suggested fix
- toString(toDate(toTimeZone(toDateTime(strain.date), {timezone:String}))) AS date, + toString(strain.date) AS date, strain.daily_load AS daily_load, strain.acute_load_7d AS acute_load, strain.chronic_load_28d AS chronic_load, strain.workload_ratio AS workload_ratio @@ { userId: ctx.userId, - timezone: ctx.timezone, endDate: input.endDate, outputWindowStart: dateWindowStartString(input.endDate, input.days), },🤖 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/server/src/routers/recovery.ts` around lines 276 - 290, The SELECT is converting strain.date through toDateTime(...)->toTimeZone(...)->toDate(...) which can shift days; change the expression used for the date output that feeds workload_ratio to use the raw day-granular strain.date (e.g., toString(strain.date) or strain.date) instead of the timezone-shifted value so the label/ordering (ORDER BY date ASC) and the WHERE filters (strain.date > ... AND strain.date <= ...) remain consistent with workload_ratio; update the SELECT's date alias (the expression currently using toDate(toTimeZone(toDateTime(strain.date), {timezone:String}))) to use strain.date and keep the rest of the query (userId, timezone, endDate, outputWindowStart) unchanged.
🤖 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 `@packages/server/src/routers/recovery.ts`:
- Around line 594-600: Replace the loose z.string() used for the read-model date
with the stricter dateStringSchema from src/lib/typed-sql.ts so DB date strings
are normalized and validated before daysAgo math; import dateStringSchema and
update the z.object that currently has date: z.string() (the schema used for the
strain/read-model row with daily_load) to use dateStringSchema instead of
z.string().
---
Duplicate comments:
In `@packages/server/src/routers/recovery.ts`:
- Around line 276-290: The SELECT is converting strain.date through
toDateTime(...)->toTimeZone(...)->toDate(...) which can shift days; change the
expression used for the date output that feeds workload_ratio to use the raw
day-granular strain.date (e.g., toString(strain.date) or strain.date) instead of
the timezone-shifted value so the label/ordering (ORDER BY date ASC) and the
WHERE filters (strain.date > ... AND strain.date <= ...) remain consistent with
workload_ratio; update the SELECT's date alias (the expression currently using
toDate(toTimeZone(toDateTime(strain.date), {timezone:String}))) to use
strain.date and keep the rest of the query (userId, timezone, endDate,
outputWindowStart) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fae38c72-2e0f-4fa5-bab1-8f8a5a3ea24e
📒 Files selected for processing (7)
analytics/models/read_models/healthspan_read_model.sqlanalytics/models/read_models/read_model_microbatch.sql.test.tsanalytics/models/read_models/recovery_read_model.sqlanalytics/models/read_models/strain_read_model.sqldocs/production-incident-baseline.mdpackages/server/src/routers/recovery.test.tspackages/server/src/routers/recovery.ts
There was a problem hiding this comment.
3 issues found across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Requires human review: This PR adds new dbt models with business logic (z-scores, sigmoid scoring), creates ClickHouse tables, and switches multiple API routes to read from them, which requires careful review of SQL correctness, migration ordering, and potential data integrity issues.
Re-trigger cubic
This was referenced Jun 5, 2026
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.
Summary
FINALwhere freshness depends onReplacingMergeTree.Verification
set -a; . ./.env.local; set +a; pnpm lintpnpm tsc --noEmitcd packages/server && pnpm tsc --noEmitcd packages/web && pnpm tsc --noEmitset -a; . ./.env.local; set +a; pnpm test --runSummary by cubic
Adds ClickHouse dashboard read models for recovery, strain, and healthspan, and switches related routes to read compact tables to cut latency and database load. Incremental builds are now micro-batched and bounded per user/day/week to avoid backfills; healthspan uses VO2 started date and strain date filters are timezone-safe.
New Features
analytics.recovery_read_model,analytics.strain_read_model,analytics.healthspan_read_modelusingReplacingMergeTree(refresh_version)with bounded incremental materialization; routes useFINALwhere needed.Migration
0026_create_named_dashboard_read_model_tables.ts, then rebuild dbt models with the updated safe model list.Written for commit dec58db. Summary will update on new commits.
Summary by CodeRabbit