Remove ClickHouse full refresh read models - #1174
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? |
Contributor
Reviewer's GuideReplaces ClickHouse full-refresh materialized read models with a combination of standard views, incremental tables, and a dirty-key pipeline for sensor data, rewires all analytics queries to be activity-window based rather than activity_id-based, and updates migrations, tests, CDC setup, jobs, and docs to match the new design and forbid REFRESH-based workflows. Entity relationship diagram for incremental deduped sensor tableserDiagram
sensor_scalar_sample {
UUID id
UUID user_id
DateTime64 recorded_at
String channel
Float scalar
UInt16 provider_priority
Int8 _peerdb_is_deleted
}
sensor_dirty_key {
UUID user_id
String channel
DateTime64 recorded_at
DateTime64 min_peerdb_synced_at
DateTime64 max_peerdb_synced_at
DateTime64 processed_at
UInt64 dirty_version
}
deduped_sensor {
UUID user_id
DateTime64 recorded_at
String channel
Float scalar
String provider_id
UUID source_metric_stream_id
UInt16 provider_priority
UInt8 is_deleted
UInt64 refresh_version
}
sensor_scalar_sample ||--o{ sensor_dirty_key : queues_changes
sensor_scalar_sample ||--o{ deduped_sensor : recomputes_best_sample
sensor_dirty_key ||--o{ deduped_sensor : drives_incremental_refresh
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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:
📝 WalkthroughWalkthroughConverts analytics from refreshable materialized views to an incremental deduped-sensor pipeline: new scalar ingestion + dirty-key ReplacingMergeTree, a batched recompute (processDedupedSensorDirtyKeys), convert read-models to CREATE VIEW, migrate consumers to user_id + recorded_at windows with is_deleted filtering, and wire post-sync draining and tests. ChangesDeduped Sensor Infrastructure & Analytics Architecture
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
|
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new activity-window join pattern on
analytics.deduped_sensor(user_id + [started_at, ended_at/12h] range) is duplicated across many queries (power curves, VO2max, efficiency, PMC, intervals, etc.); consider extracting a shared helper or view to reduce the risk of subtle divergence in those predicates over time. - The maximum dirty-key batch limits for sensor recompute (
maxSensorDirtyKeyBatchesinprocess-post-sync-jobandclickHouseTestSensorDirtyKeyMaxBatchesin the integration helpers) are both hard-coded to 1000; it may be safer to centralize these thresholds or derive the test limit from the production constant to keep behavior aligned.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new activity-window join pattern on `analytics.deduped_sensor` (user_id + [started_at, ended_at/12h] range) is duplicated across many queries (power curves, VO2max, efficiency, PMC, intervals, etc.); consider extracting a shared helper or view to reduce the risk of subtle divergence in those predicates over time.
- The maximum dirty-key batch limits for sensor recompute (`maxSensorDirtyKeyBatches` in `process-post-sync-job` and `clickHouseTestSensorDirtyKeyMaxBatches` in the integration helpers) are both hard-coded to 1000; it may be safer to centralize these thresholds or derive the test limit from the production constant to keep behavior aligned.
## Individual Comments
### Comment 1
<location path="src/db/clickhouse-deduped-sensor.ts" line_range="238-247" />
<code_context>
+ FROM analytics.sensor_scalar_sample FINAL`);
+}
+
+function buildPendingDirtyKeySql(limit: number): string {
+ return `SELECT
+ user_id,
+ channel,
+ recorded_at,
+ min(min_peerdb_synced_at) AS min_peerdb_synced_at,
+ max(max_peerdb_synced_at) AS max_peerdb_synced_at,
+ max(dirty_version) AS max_dirty_version
+ FROM analytics.sensor_dirty_key
+ GROUP BY user_id, channel, recorded_at
+ HAVING maxIf(dirty_version, processed_at IS NULL) > maxIf(dirty_version, processed_at IS NOT NULL)
+ ORDER BY max_peerdb_synced_at ASC
+ LIMIT ${limit}`;
</code_context>
<issue_to_address>
**issue (bug_risk):** Dirty-key selection and marking logic can miss updates due to a race between the two INSERTs
Using the same `pendingKeySql` in two separate INSERTs creates a race:
1. You compute `pendingKeySql` and use it to recompute `analytics.deduped_sensor`.
2. Before the second `INSERT INTO analytics.sensor_dirty_key ... SELECT ... FROM (pendingKeySql)` runs, new `sensor_dirty_key` rows can arrive for keys in that batch.
3. In the second INSERT, `max_dirty_version` now includes these newer rows, so the HAVING clause treats them as processed, even though the recompute in (1) never saw their `dirty_version`.
To fix this, either:
- Constrain the marking step to `dirty_version <= max_dirty_version` as of the first query, or
- Materialize the pending keys (e.g., temp table / `dirty_batch_id`) and drive both recompute and marking from that snapshot.
As written, concurrent ingestion can cause updates to be marked processed without being applied to `deduped_sensor`.
</issue_to_address>
### Comment 2
<location path="src/db/clickhouse-deduped-sensor.ts" line_range="308-309" />
<code_context>
"analytics.activity_trend_daily",
]) {
await client.command({
- query: `CREATE MATERIALIZED VIEW IF NOT EXISTS ${viewName}
-REFRESH EVERY 1 MINUTE
</code_context>
<issue_to_address>
**suggestion (performance):** Recomputing deduped rows scans the full sensor_scalar_sample table on every batch, which will be expensive at scale
In `buildDedupedSensorRecomputeInsertSql`, this pattern:
```sql
LEFT JOIN (
SELECT *
FROM analytics.sensor_scalar_sample FINAL
) AS samples
ON samples.user_id = pending_keys.user_id
AND samples.channel = pending_keys.channel
AND samples.recorded_at = pending_keys.recorded_at
```
applies `FINAL` to the entire `sensor_scalar_sample` table without a predicate, so each dirty-key batch forces a full-table read and collapse.
To avoid that, either:
```sql
LEFT JOIN (
SELECT *
FROM analytics.sensor_scalar_sample FINAL
WHERE (user_id, channel, recorded_at) IN (
SELECT user_id, channel, recorded_at FROM pending_keys
)
) AS samples ON ...
```
or join directly to `analytics.sensor_scalar_sample FINAL` (without `SELECT * FROM (...)`) and let ClickHouse use the join keys for index pruning. The current subquery shape will likely be a major bottleneck as the table grows.
</issue_to_address>
### Comment 3
<location path="packages/server/src/routers/clickhouse-integration-test-helpers.test.ts" line_range="172" />
<code_context>
),
).toBe(true);
- expect(commands.filter((command) => command === "SELECT 1")).toHaveLength(11);
+ expect(commands.filter((command) => command === "SELECT 1")).toHaveLength(0);
});
});
</code_context>
<issue_to_address>
**suggestion (testing):** Integration helpers tests don’t exercise the new REBUILD/dirty-key behavior
The expectations here correctly reflect that we no longer issue `SYSTEM REFRESH/WAIT VIEW` or emit the `SELECT 1` stub, but the new behaviors in `clickhouse-integration-test-helpers.ts` are not exercised.
Please add integration tests that:
- Cover `rewriteClickHouseTestCommand` rewriting `CREATE VIEW IF NOT EXISTS analytics.foo` into a table + precomputed SELECT, and `REBUILD TEST ANALYTICS TABLE analytics.foo` into `TRUNCATE` + `INSERT` using that SELECT.
- Cover `syncClickHouseTestActivitySensorStoreWithClient` calling `processDedupedSensorDirtyKeys` in a loop until it returns 0, and throwing when the test max‑batch limit is exceeded.
This will keep the test harness aligned with the production incremental pipeline behavior.
Suggested implementation:
```typescript
).toBe(true);
expect(commands.filter((command) => command === "SELECT 1")).toHaveLength(0);
});
describe("rewriteClickHouseTestCommand", () => {
it("rewrites CREATE VIEW into test table + precomputed SELECT and REBUILD into TRUNCATE + INSERT", () => {
const viewName = "analytics.foo";
const createViewCommand = `CREATE VIEW IF NOT EXISTS ${viewName} AS SELECT col1, col2 FROM source_table`;
const rewrittenCreate = rewriteClickHouseTestCommand(createViewCommand);
// Creates test table instead of view
expect(rewrittenCreate).toMatch(/CREATE TABLE IF NOT EXISTS analytics_test_foo/);
// Uses a precomputed SELECT based on the original query
expect(rewrittenCreate).toMatch(/SELECT col1, col2 FROM source_table/);
const rebuildCommand = `REBUILD TEST ANALYTICS TABLE ${viewName}`;
const rewrittenRebuild = rewriteClickHouseTestCommand(rebuildCommand);
// REBUILD uses TRUNCATE + INSERT ... SELECT from the precomputed query
expect(rewrittenRebuild).toMatch(/TRUNCATE TABLE analytics_test_foo/);
expect(rewrittenRebuild).toMatch(/INSERT INTO analytics_test_foo/);
expect(rewrittenRebuild).toMatch(/SELECT col1, col2 FROM source_table/);
});
});
describe("syncClickHouseTestActivitySensorStoreWithClient", () => {
it("calls processDedupedSensorDirtyKeys until it returns 0", async () => {
const client: any = {
processDedupedSensorDirtyKeys: jest
.fn()
.mockResolvedValueOnce(5) // first batch
.mockResolvedValueOnce(2) // second batch
.mockResolvedValueOnce(0), // done
};
await syncClickHouseTestActivitySensorStoreWithClient(client);
expect(client.processDedupedSensorDirtyKeys).toHaveBeenCalledTimes(3);
});
it("throws when the test max-batch limit is exceeded", async () => {
const client: any = {
processDedupedSensorDirtyKeys: jest.fn().mockResolvedValue(1),
};
await expect(
syncClickHouseTestActivitySensorStoreWithClient(client, { maxBatches: 5 }),
).rejects.toThrow(/max batch/i);
});
});
});
```
To fully implement the requested behavior, you will also need to:
1. Ensure the helpers are imported into this test file, e.g. at the top of `clickhouse-integration-test-helpers.test.ts`:
- Add `rewriteClickHouseTestCommand` and `syncClickHouseTestActivitySensorStoreWithClient` to the named imports from `./clickhouse-integration-test-helpers` (or whatever the existing import path is).
- Example (adjust to match existing import style):
```ts
import {
rewriteClickHouseTestCommand,
syncClickHouseTestActivitySensorStoreWithClient,
} from "./clickhouse-integration-test-helpers";
```
2. Align the tests with the actual implementation details:
- If `rewriteClickHouseTestCommand` uses a different naming convention for the test table (e.g. `analytics_test_analytics_foo` or a different prefix), adjust the `toMatch` expectations accordingly.
- If the precomputed selection is stored under a different name or structure, ensure the regex expectations match the real SQL that `rewriteClickHouseTestCommand` emits.
3. Confirm the `syncClickHouseTestActivitySensorStoreWithClient` API:
- If it does **not** take an options object as the second parameter, or the option is named differently (e.g. `maxBatchesForTests`, `maxSyncBatches`), update the call in the second test and the corresponding `toThrow` expectation to match the real signature and error message.
- If the function returns a value instead of throwing on exceeding the max-batch limit, adjust the assertion to `resolves` and assert on the return value instead of using `rejects.toThrow`.
4. If your project uses a different test runner or assertion library (e.g. `vitest` instead of `jest`), replace `jest.fn()` and the Jest-specific `expect(...).rejects` syntax with the equivalents from your configured test framework.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Review app is ready: This environment runs on a dedicated Hetzner server for PR #1174 and updates on each 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/schema.md (1)
170-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale documentation contradicts the new architecture.
Line 170 describes the old activity-scoped deduplication strategy ("per (activity_id, channel), the provider with the most samples wins"), which directly contradicts the new activity-agnostic strategy documented in lines 74-78 and 127. The new strategy uses
(user_id, channel, recorded_at)keys with explicit priority tables, not activity-scoped sample counts.Remove this line or update it to reflect the new incremental dirty-key-driven deduplication pipeline.
Suggested fix
Remove the stale sentence or replace it with:
-Sensor sample dedup: per (activity_id, channel), the provider with the most samples wins. This ensures the most granular source (e.g., BLE at 50Hz vs API at 1Hz) is automatically preferred. +Sensor sample dedup is described in the **sensor_sample** section above: `analytics.deduped_sensor` selects the best sample per `(user_id, channel, recorded_at)` using explicit provider and device priority, with activity association determined by time-window queries.🤖 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 `@docs/schema.md` at line 170, The sentence claiming "Sensor sample dedup: per (activity_id, channel), the provider with the most samples wins" is stale and contradicts the new activity-agnostic deduplication; remove or replace that sentence so the doc reflects the current strategy: describe the incremental dirty-key-driven pipeline using (user_id, channel, recorded_at) keys and the explicit provider priority tables (i.e., mention the activity-agnostic dedup logic and priority tables instead of activity-scoped sample counts).src/db/clickhouse-read-models.ts (1)
977-1019: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this module before adding more read-model builders.
This change pushes
src/db/clickhouse-read-models.tspast the repo's 1000-line limit. Move the new SQL builders into a sibling module instead of growing this file further.As per coding guidelines "Max 1000 lines per file: No TypeScript file should exceed 1000 lines. If a file is approaching that limit, proactively split it into smaller, focused modules before it grows further."
🤖 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 `@src/db/clickhouse-read-models.ts` around lines 977 - 1019, This file has grown past the 1000-line limit due to the new SQL builder functions; extract the newly added read-model builders (at least buildProviderStatsCreateReadModelStatements, buildProviderStatsReadModelStatements, buildActivityTrendDailyCreateReadModelStatements, buildActivityTrendDailyReadModelStatements, buildAnalyticsFitnessReadModelStatements, and buildBodyMeasurementReadModelStatements) into a new sibling module (e.g., clickhouse-read-models-helpers or clickhouse-read-models-extra), export them there, and replace their original definitions with imports (or re-exports) in the current module so existing callers keep working; ensure buildAnalyticsFitnessReadModelStatements still composes the moved helpers by importing buildActivityReadModelSql, buildActivityMembersReadModelSql, buildSleepReadModelSql, buildBodyMeasurementReadModelSql, buildDailyMetricsReadModelSql, and the provider-stats/provider-trend builders from the new module and update any exports/tests accordingly.
🤖 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 `@packages/server/src/repositories/clickhouse-activity-sensor-store.test.ts`:
- Around line 60-65: Remove the test case titled "does not refresh the body
measurement view" from the clickhouse-activity-sensor-store.test suite: delete
the it(...) block that uses makeStore(), calls store.refreshBodyMeasurements(),
and asserts expect(command).not.toHaveBeenCalled(); this test asserts absence of
removed behavior and should be dropped per guidelines (look for the it block
referencing makeStore, store.refreshBodyMeasurements, and command).
In `@packages/server/src/repositories/clickhouse-activity-sensor-store.ts`:
- Around line 51-52: The code currently sets windowEndedAt to new
Date().toISOString() when window.endedAt is missing, which causes unbounded
scans; update the fallback used in clickhouse-activity-sensor-store.ts so
windowEndedAt is capped (e.g. use the same bounded fallback constant or helper
used elsewhere in this PR such as MAX_ACTIVITY_WINDOW or a
clampToMaxWindow(window.startedAt) helper) instead of wall-clock now, or
alternatively require the caller to supply an explicit end; adjust the
windowStartedAt/windowEndedAt assignment and any callers of getStream() or zone
query logic to use this capped end value.
- Around line 392-395: The downsampling divisor should use bucketized division
intDiv(total, maxPoints) instead of intDiv(total + maxPoints - 1, maxPoints)
because the latter collapses the maxPoints+1 case (e.g., total=501,
maxPoints=500) into 2; update the WHERE clause that currently reads "WHERE
row_number % greatest(1, intDiv(total + toUInt64({maxPoints:UInt32}) - 1,
toUInt64({maxPoints:UInt32}))) = 0" to use greatest(1, intDiv(total,
toUInt64({maxPoints:UInt32}))) so row selection preserves ~maxPoints entries,
and add a regression test in the clickhouse-activity-sensor-store tests that
asserts total = maxPoints + 1 returns roughly maxPoints rows.
- Around line 193-195: The SQL in the query that builds the JOIN with "INNER
JOIN analytics.v_activity AS activity" currently places non-equi predicates
(deduped_samples.recorded_at >= activity.started_at and
deduped_samples.recorded_at <= coalesce(activity.ended_at, activity.started_at +
INTERVAL 12 HOUR)) in the ON clause; change the JOIN to use only the equi-key
(activity.user_id = deduped_samples.user_id) and move the two
recorded_at/time-window predicates out of the ON into the query filter/windowing
logic (e.g., add them to the WHERE or implement a CROSS JOIN/windowing step that
enforces the activity window) so that the join remains an equi-join and the
time-range filtering happens after or as part of the windowing stage.
In `@packages/server/src/repositories/efficiency-repository.ts`:
- Around line 253-258: The LEFT JOIN in efficiency-repository.ts that uses
non-equi predicates on ds (the block using "LEFT JOIN analytics.deduped_sensor
ds ON ds.user_id = ea.user_id AND ds.recorded_at >= ea.started_at AND
ds.recorded_at <= coalesce(...)" ) must be replaced by a CTE/subquery that
pre-filters and aggregates sensor samples per activity_id using the time-range
and channel/is_deleted filters, then LEFT JOIN the aggregated result back to the
activities by equality on activity_id (or activity id + user_id) so zero-sample
activities are preserved; implement a new subquery/CTE (e.g.,
sensor_samples_by_activity) that SELECTs activity_id, COUNT(*) as sample_count
FROM analytics.deduped_sensor JOIN/WHERE against activity time bounds, channels
and is_deleted=0, GROUP BY activity_id, then LEFT JOIN that aggregated CTE to ea
using only equality predicates.
In `@packages/server/src/routers/clickhouse-integration-test-helpers.test.ts`:
- Around line 101-108: Remove the absence-only assertions that assert deleted
behavior is gone (the expect(...).toBe(false) checks that scan setupCommands for
"CREATE VIEW IF NOT EXISTS analytics_test_" and ".v_daily_metrics"); delete
these negative checks (both the one shown and the similar occurrence noted at
the other location) and ensure the test retains only positive assertions that
verify the expected current commands/outputs (i.e., keep or add explicit expects
for the commands we do want to see, using setupCommands and any existing helper
assertions).
In `@packages/server/src/routers/healthspan-query.ts`:
- Around line 147-153: The LEFT JOIN to analytics.deduped_sensor (alias ds) that
includes non-equi time predicates (ds.recorded_at >= am.started_at AND
ds.recorded_at <= coalesce(am.ended_at, am.started_at + INTERVAL 12 HOUR))
should be changed to an INNER JOIN so the non-equi predicates are evaluated
correctly; update the join in the query where ds is joined to am (and ensure the
GROUP BY on am.activity_id, am.duration_minutes, am.max_hr, am.ftp,
am.resting_hr remains unchanged) to use INNER JOIN analytics.deduped_sensor AS
ds with the same ON predicates.
In `@src/db/clickhouse-deduped-sensor.ts`:
- Around line 378-381: The dirty_version computation currently uses now64(9)
which can advance the watermark incorrectly; change the second argument of
greatest(...) to use the `_peerdb_synced_at` clock converted to the same UInt64
nanosecond timestamp type so the comparison stays on the `_peerdb_synced_at`
timeline. Specifically, replace the toUInt64(toUnixTimestamp64Nano(now64(9)))
part used in computing dirty_version (alongside pending_keys.max_dirty_version +
1) with an equivalent conversion of `_peerdb_synced_at` so dirty_version remains
aligned with the `_peerdb_synced_at` clock.
In `@src/db/clickhouse-migrations.test.ts`:
- Around line 45-48: Remove the "absence-only" assertions that check for legacy
refresh/materialized-view strings (e.g., expect(sql).not.toContain("REFRESH
EVERY"), expect(sql).not.toContain("SYSTEM REFRESH VIEW"),
expect(sql).not.toContain("SYSTEM WAIT VIEW") and any not.toHaveBeenCalledWith
checks) and keep only positive assertions that validate current expected output
(for example the existing expect(sql).toContain("CREATE TABLE IF NOT EXISTS
analytics.sensor_scalar_sample")); apply the same change to the other listed
ranges (72-79, 100-101, 226-232, 477-482, 588-602, 889-893) by removing
negated/refusal-style checks tied to removed refresh paths while retaining or
strengthening assertions that confirm the new SQL/behavior produced by the
functions under test.
In `@src/db/clickhouse-read-models.ts`:
- Around line 953-974: The query currently aggregates directly over the
analytics.v_activity × analytics.deduped_sensor join which double-counts a
sensor row if it joins to overlapping activities; introduce a sample-distinct
CTE (e.g., select distinct on the deduped sample identity: samples.user_id,
samples.recorded_at, samples.channel, samples.scalar or the unique samples.id if
present) and use that CTE (alias it e.g., distinct_samples) in place of
analytics.deduped_sensor in the main query so all sample-based metrics (avg_hr,
max_hr, avg_power, countIf*, total_samples, etc.) are computed from deduplicated
samples, while preserving activity_count (uniqExact(activity.id)) from the
activity side; update FROM/INNER JOIN to join activity to distinct_samples and
compute the CAST/avgIf/maxIf/countIf expressions against distinct_samples.
In `@src/db/clickhouse-resting-heart-rate-materialized-view.ts`:
- Around line 26-42: The LEFT JOIN is matching soft-deleted rows from
analytics.v_activity (alias activity) which causes deleted workouts to exclude
valid sleep heart-rate samples; fix by excluding deleted activities in the
join—either add a predicate filtering out activity.is_deleted (or the equivalent
soft-delete flag) inside the activity subquery or include AND
activity.is_deleted = 0 in the ON clause so samples, sleep_windows and the
activity alias behave correctly (ensure you reference activity.activity_id,
analytics.v_activity and samples when updating the join).
---
Outside diff comments:
In `@docs/schema.md`:
- Line 170: The sentence claiming "Sensor sample dedup: per (activity_id,
channel), the provider with the most samples wins" is stale and contradicts the
new activity-agnostic deduplication; remove or replace that sentence so the doc
reflects the current strategy: describe the incremental dirty-key-driven
pipeline using (user_id, channel, recorded_at) keys and the explicit provider
priority tables (i.e., mention the activity-agnostic dedup logic and priority
tables instead of activity-scoped sample counts).
In `@src/db/clickhouse-read-models.ts`:
- Around line 977-1019: This file has grown past the 1000-line limit due to the
new SQL builder functions; extract the newly added read-model builders (at least
buildProviderStatsCreateReadModelStatements,
buildProviderStatsReadModelStatements,
buildActivityTrendDailyCreateReadModelStatements,
buildActivityTrendDailyReadModelStatements,
buildAnalyticsFitnessReadModelStatements, and
buildBodyMeasurementReadModelStatements) into a new sibling module (e.g.,
clickhouse-read-models-helpers or clickhouse-read-models-extra), export them
there, and replace their original definitions with imports (or re-exports) in
the current module so existing callers keep working; ensure
buildAnalyticsFitnessReadModelStatements still composes the moved helpers by
importing buildActivityReadModelSql, buildActivityMembersReadModelSql,
buildSleepReadModelSql, buildBodyMeasurementReadModelSql,
buildDailyMetricsReadModelSql, and the provider-stats/provider-trend builders
from the new module and update any exports/tests accordingly.
🪄 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: 903ed73e-fd70-484d-a0fd-5cfa777c7ebd
📒 Files selected for processing (37)
docs/clickhouse-body-measurement-staleness-runbook.mddocs/clickhouse-metric-stream.mddocs/production-incident-baseline.mddocs/schema.mdpackages/server/src/lib/current-strain.tspackages/server/src/repositories/clickhouse-activity-sensor-analytics.tspackages/server/src/repositories/clickhouse-activity-sensor-store.test.tspackages/server/src/repositories/clickhouse-activity-sensor-store.tspackages/server/src/repositories/cycling-advanced-repository.tspackages/server/src/repositories/efficiency-repository.tspackages/server/src/repositories/intervals-repository.tspackages/server/src/repositories/pmc-repository.tspackages/server/src/repositories/training-repository.tspackages/server/src/routers/clickhouse-integration-test-helpers.test.tspackages/server/src/routers/clickhouse-integration-test-helpers.tspackages/server/src/routers/healthspan-query.tspackages/server/src/routers/router-data.integration.test.tssrc/db/clickhouse-cdc.test.tssrc/db/clickhouse-cdc.tssrc/db/clickhouse-deduped-sensor.test.tssrc/db/clickhouse-deduped-sensor.tssrc/db/clickhouse-metric-stream-bootstrap.tssrc/db/clickhouse-migrations.test.tssrc/db/clickhouse-migrations.tssrc/db/clickhouse-read-model-refresh.test.tssrc/db/clickhouse-read-model-refresh.tssrc/db/clickhouse-read-models.tssrc/db/clickhouse-resting-heart-rate-materialized-view.tssrc/db/clickhouse-sql-helpers.tssrc/db/clickhouse.test.tssrc/db/clickhouse.tssrc/db/peerdb/metric-stream-cdc.sqlsrc/jobs/process-post-sync-job.test.tssrc/jobs/process-post-sync-job.tssrc/jobs/worker.test.tssrc/jobs/worker.tssrc/personalization/refit.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/server/src/repositories/training-repository.ts`:
- Around line 140-147: The short-circuit in getHrZones uses
this.#loadRawActivityCount(..., "AND ended_at IS NOT NULL") which excludes open
activities and causes an early return ({ maxHr: null, weeks: [] }) even when HR
samples exist; remove the "AND ended_at IS NOT NULL" filter from the
rawActivityCount call (i.e., call `#loadRawActivityCount` without that guard) so
open activities are counted consistently with the main query that uses
coalesce(am.ended_at, am.started_at + INTERVAL 12 HOUR), ensuring the
short-circuit no longer wrongly returns empty results when only in-progress
endurance activities exist.
🪄 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: b7e315b4-6a1f-4c4e-8462-5ca04bc7528a
📒 Files selected for processing (7)
docs/production-incident-baseline.mdpackages/server/src/repositories/efficiency-repository.test.tspackages/server/src/repositories/efficiency-repository.tspackages/server/src/repositories/training-repository.test.tspackages/server/src/repositories/training-repository.tspackages/server/src/routers/efficiency.test.tspackages/server/src/routers/training-access-window.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/server/src/repositories/pmc-repository.ts`:
- Around line 90-104: The sample_counts CTE is unbounded because it only filters
by samples.user_id; update the CTE to also restrict samples.recorded_at to the
requested window (the same queryDays bounds used by the outer query) so the
aggregation is limited to the date range. Specifically, add the queryDays
start/end predicates (the same variables used elsewhere in this repo query
construction) to the sample_counts WHERE clause so samples.recorded_at is
between the window (or <= coalesce(activity.ended_at, activity.started_at +
INTERVAL 12 HOUR) AND >= queryDays.start), keeping the CTE scoped to the
requested time range.
🪄 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: 3321a625-0b05-4fe1-97da-73cd9e89e353
📒 Files selected for processing (12)
docs/production-incident-baseline.mdpackages/server/src/repositories/clickhouse-activity-sensor-analytics.tspackages/server/src/repositories/clickhouse-activity-sensor-store.test.tspackages/server/src/repositories/cycling-advanced-repository.test.tspackages/server/src/repositories/cycling-advanced-repository.tspackages/server/src/repositories/efficiency-repository.test.tspackages/server/src/repositories/efficiency-repository.tspackages/server/src/repositories/pmc-repository.test.tspackages/server/src/repositories/pmc-repository.tspackages/server/src/repositories/training-repository.test.tspackages/server/src/repositories/training-repository.tspackages/server/src/routers/training-access-window.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/web/src/components/DofekChart.test.tsx`:
- Around line 169-170: Remove the negative absence assertion asserting the
removed loading skeleton: delete the line that calls
expect(screen.queryByTestId("loading-skeleton")).toBeNull() in the DofekChart
test so the test only keeps the positive empty-state assertion
(expect(screen.getByText("No data available")).toBeDefined()); this targets the
assertion in packages/web/src/components/DofekChart.test.tsx that references
test id "loading-skeleton".
🪄 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: 054c7f0e-35d6-405b-a6f9-d7b9bdd808f2
📒 Files selected for processing (5)
docs/production-incident-baseline.mdpackages/server/src/repositories/pmc-repository.test.tspackages/server/src/repositories/pmc-repository.tspackages/web/src/components/DofekChart.test.tsxpackages/web/src/components/DofekChart.tsx
This was referenced May 25, 2026
Merged
This was referenced Jun 3, 2026
This was referenced Jun 24, 2026
This was referenced Jul 6, 2026
Merged
This was referenced Jul 18, 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
analytics.deduped_sensorwith an incremental dirty-key pipeline.REFRESH EVERYor manual refresh commands.Testing
Summary by Sourcery
Replace ClickHouse full-refresh analytics read models with incremental pipelines and standard views, and rewire activity analytics to consume activity-windowed sensor data without relying on REFRESH EVERY semantics.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests