fix: propagate historical body measurement changes - #1966
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe daily body measurement model now uses source refresh watermarks, emits tombstones for removed measurements, and exposes lifecycle metadata. Weekly healthspan and server queries exclude deleted rows. A ClickHouse migration, integration coverage, and SQL assertions validate the lifecycle. ChangesBody measurement lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Assessment against linked issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
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 |
PR Summary by QodoPropagate historical body measurement corrections via CDC watermarks
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
|
Storybook previews for This comment updates automatically on each PR push. |
Code Review by Qodo
Context used✅ Compliance rules (platform):
171 rules✅ Skills:
fix-provider, write-tests, cloudflare 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/server/src/routers/clickhouse-integration-test-read-models-b.ts (1)
469-523: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMissing
FINALondaily_body_measurementreads in the same function that correctly uses it elsewhere.Lines 482 (
healthspan_activity_zone_minutes FINAL) and 509 correctly appendFINALfor aReplacingMergeTreetable, but the twodaily_body_measurementreads at lines 490 and 520 omit it, despitedaily_body_measurementusing the sameReplacingMergeTree(refresh_version)engine (per the dbt model config). WithoutFINAL, a stale un-merged pre-tombstone version (is_deleted = 0) of a row can coexist with its newer tombstoned version and still pass theWHERE is_deleted = 0filter added here — exactly the bug this PR is meant to prevent, just relocated into the test fixture.🐛 Proposed fix
SELECT user_id, toMonday(date) AS week_start - FROM ${databases.analytics}.daily_body_measurement + FROM ${databases.analytics}.daily_body_measurement FINAL WHERE is_deleted = 0 GROUP BY user_id, toMonday(date) ), @@ argMax(weight_kg, (recorded_at, refresh_version, measurement_id)) AS weight_kg, argMax(body_fat_pct, (recorded_at, refresh_version, measurement_id)) AS body_fat_pct - FROM ${databases.analytics}.daily_body_measurement + FROM ${databases.analytics}.daily_body_measurement FINAL WHERE is_deleted = 0 GROUP BY user_id, toMonday(date) ),🤖 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/clickhouse-integration-test-read-models-b.ts` around lines 469 - 523, Update both daily_body_measurement reads in buildTestHealthspanReadModelSelectSql—within week_keys and body_by_week—to append FINAL, matching the existing healthspan_activity_zone_minutes read while preserving the current filters and aggregations.packages/server/src/routers/clickhouse-integration-test-models.ts (1)
177-186: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTest table's
ORDER BYfordaily_body_measurementdiverges from production and undermines dedup-parity for exactly the scenario this PR fixes.Line 558 defines
order_byas(user_id, recorded_at, measurement_id), but the actual dbt model (analytics/models/read_models/daily_body_measurement.sql) usesorder_by='(user_id, measurement_id)'. Sincemeasurement_idis already a globally-unique UUID, includingrecorded_atin the test table's key serves no purpose, but it does change ReplacingMergeTree dedup semantics: if a test scenario corrects arecorded_attimestamp for an existingmeasurement_id(part of the "historical edits" behavior this PR targets), production would replace the row viaFINAL, while this test table would treat the two versions as distinct rows and never deduplicate them, even withFINAL. That's a material behavior gap for a table whose ordering directly affects test-vs-prod parity for corrections.🐛 Proposed fix
: shortViewName === "daily_body_measurement" - ? "(user_id, recorded_at, measurement_id)" + ? "(user_id, measurement_id)"🤖 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/clickhouse-integration-test-models.ts` around lines 177 - 186, The test table definition for daily_body_measurement must match production’s ReplacingMergeTree key: update its order_by configuration from (user_id, recorded_at, measurement_id) to (user_id, measurement_id), preserving deduplication when recorded_at changes for an existing measurement_id.
🤖 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/daily_body_measurement.sql`:
- Around line 12-153: Review the incremental flow around target_user_state,
existing_rows, and live_body to avoid repeatedly scanning full user history with
FINAL as volume grows. Add materialization-duration monitoring and reassess the
max_threads: 1 setting so daily_body_measurement can use increased parallelism
when performance requires it.
- Around line 13-26: The body_view_state CTE currently reads last_success_time
from system.view_refreshes for v_body_measurement, but that object is a plain
view and has no refresh watermark. Replace this dependency with a real upstream
watermark that advances for body measurements, and update the source_changes
filtering flow to use it; alternatively, convert v_body_measurement back to a
refreshable materialized view so its refresh timestamp is valid.
In `@src/db/clickhouse-migrations/0055_daily_body_measurement_lifecycle.ts`:
- Line 5: Update tableCountRowsSchema to validate string and numeric counts as
finite, nonnegative numbers rather than accepting arbitrary strings, ensuring
malformed values are rejected before the zero comparison. In the migration’s
table-count query flow, replace raw database result handling with
executeWithSchema() using tableCountRowsSchema so the validated result is used
and untyped SQL output is not consumed directly.
In `@src/db/daily-body-measurement-read-model.integration.test.ts`:
- Around line 164-179: Replace readDependentWeeklyWeight with a helper that
materializes and queries the production weekly_healthspan model using the
existing model/materialization utilities. Have the integration test assert the
model’s weekly output after both update and deletion lifecycle changes, rather
than reimplementing the daily_body_measurement aggregation.
---
Outside diff comments:
In `@packages/server/src/routers/clickhouse-integration-test-models.ts`:
- Around line 177-186: The test table definition for daily_body_measurement must
match production’s ReplacingMergeTree key: update its order_by configuration
from (user_id, recorded_at, measurement_id) to (user_id, measurement_id),
preserving deduplication when recorded_at changes for an existing
measurement_id.
In `@packages/server/src/routers/clickhouse-integration-test-read-models-b.ts`:
- Around line 469-523: Update both daily_body_measurement reads in
buildTestHealthspanReadModelSelectSql—within week_keys and body_by_week—to
append FINAL, matching the existing healthspan_activity_zone_minutes read while
preserving the current filters and aggregations.
🪄 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: d5e76b6a-bce9-4311-b7df-00ac1790f4e8
📒 Files selected for processing (16)
analytics/models/read_models/daily_body_measurement.sqlanalytics/models/read_models/read_model_microbatch.sql.test.tsanalytics/models/read_models/weekly_healthspan.sqldocs/production-incident-baseline.mdpackages/server/src/repositories/body-clickhouse.test.tspackages/server/src/repositories/body-clickhouse.tspackages/server/src/repositories/cycling-analytics-repository.integration.test.tspackages/server/src/repositories/cycling-analytics-repository.test.tspackages/server/src/repositories/cycling-analytics-repository.tspackages/server/src/routers/clickhouse-integration-test-models.tspackages/server/src/routers/clickhouse-integration-test-read-models-b.tssrc/db/clickhouse-migrations/0055_daily_body_measurement_lifecycle.test.tssrc/db/clickhouse-migrations/0055_daily_body_measurement_lifecycle.tssrc/db/clickhouse-migrations/registry.test.tssrc/db/clickhouse-migrations/registry.tssrc/db/daily-body-measurement-read-model.integration.test.ts
|
🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews. |
|
🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews. |
|
Addressed the two actionable outside-diff findings in 951fdbc: the isolated weekly-healthspan fixture now reads both daily_body_measurement inputs with FINAL, and its daily_body_measurement table uses the production |
|
🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews. |
Fixes #1769
Summary
Validation
pnpm typecheckpnpm lintdaily_body_measurement weekly_healthspanLocal infrastructure note
The issue-specific ClickHouse test passed. A broader cycling fixture was interrupted by a concurrent local ClickHouse container restart under Docker VM pressure; no product/workflow workaround was added, and the evidence is recorded in the production incident baseline. CI remains the broad integration gate.
Summary by cubic
Propagates historical body measurement edits and deletes into the serving model and recomputes only the affected weeks. Fixes #1769 by bounding per-user refreshes with CDC watermarks capped by the body view’s last success time, and by emitting tombstones to prevent stale data.
Bug Fixes
daily_body_measurementincrementally per user using_peerdb_synced_atwatermarks fromanalytics.body_measurement_sample, capped bysystem.view_refreshes.last_success_time(removes the prior 7-day overlap).is_deleted = 0).weekly_healthspanfilters deleted rows and rematerializes only weeks touched since its last refresh usingdaily_body_measurement.refreshed_at.Migration
0056_daily_body_measurement_lifecycleto addis_deleted,source_synced_at, and a materialized minmax index onrefreshed_at.Written for commit 7c38ddb. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests