Store GPS data as PostGIS metric points - #1111
Merged
Merged
Conversation
|
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 PostGIS-backed point/latitude/longitude/metadata to metric_stream, introduces a ChangesLocation Metrics Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
# Conflicts: # src/db/clickhouse-migrations.ts
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/apple-health/db-insertion.test.ts (1)
1296-1313:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert geometry and metadata land on the same
locationrow.These two
toContainEqualchecks would still pass if the writer accidentally split one sample into twolocationinserts—one withpoint/lat/lng and another withmetadata. The new storage model is a single point-valued row with a metadata blob, so this test should pin that down directly.Suggested test tightening
- expect(capture.values[1]).toContainEqual( - expect.objectContaining({ - providerId: "p1", - activityId: "act-1", - channel: "location", - point: "SRID=4326;POINT(-74.006 40.7128)", - latitude: 40.7128, - longitude: -74.006, - }), - ); - expect(capture.values[1]).toContainEqual( - expect.objectContaining({ - providerId: "p1", - activityId: "act-1", - channel: "location", - metadata: { horizontal_accuracy_m: 5.2 }, - }), - ); + const locationRow = capture.values[1]?.find((row) => row.channel === "location"); + expect(locationRow).toMatchObject({ + providerId: "p1", + activityId: "act-1", + channel: "location", + point: "SRID=4326;POINT(-74.006 40.7128)", + latitude: 40.7128, + longitude: -74.006, + metadata: { horizontal_accuracy_m: 5.2 }, + });🤖 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/providers/apple-health/db-insertion.test.ts` around lines 1296 - 1313, Replace the two separate toContainEqual assertions that separately check geometry and metadata with a single assertion that ensures there is one location row object that contains both the geometry fields and the metadata blob; specifically, update the assertion on capture.values[1] to assert an objectContaining that includes providerId: "p1", activityId: "act-1", channel: "location", point/latitude/longitude and metadata: { horizontal_accuracy_m: 5.2 } together so the test fails if the writer split them into two inserts.
🧹 Nitpick comments (2)
src/providers/apple-health/import.integration.test.ts (1)
863-867: ⚡ Quick winUse descriptive callback parameter names in the new location assertions.
Line 863 and Line 865 use
r; please rename to a descriptive name for consistency/readability.Suggested change
- const gpsRows = allMetrics.filter((r) => r.activityId === run?.id && r.channel === "location"); + const gpsRows = allMetrics.filter( + (metricRow) => metricRow.activityId === run?.id && metricRow.channel === "location", + ); @@ - expect(gpsRows.some((r) => r.latitude !== null && Math.abs(r.latitude - 40.7128) < 0.001)).toBe( + expect( + gpsRows.some( + (metricRow) => + metricRow.latitude !== null && Math.abs(metricRow.latitude - 40.7128) < 0.001, + ), + ).toBe( true, );As per coding guidelines, “Avoid acronyms and single-letter variables. Use descriptive names instead.”
🤖 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/providers/apple-health/import.integration.test.ts` around lines 863 - 867, Rename the terse callback parameter `r` in the location assertions to a descriptive name (e.g., `metric` or `row`) to improve readability: update the filter callback used to build `gpsRows` (allMetrics.filter((...) => ...)) and the some() callback (gpsRows.some(...)) so both use the chosen descriptive identifier while preserving the existing conditions (activityId === run?.id and channel === "location", and the latitude check against 40.7128).packages/server/src/repositories/activity-sensor-store.test-helpers.ts (1)
470-519: ⚡ Quick winAdd a shim-vs-production note above this query.
This helper now reconstructs
locationsamples directly from raw Postgres rows, but the production path still comes from ClickHouseanalytics.deduped_location. A brief comment here will keep future test changes from treating this SQL as the canonical analytics query.Based on learnings: "These tests cannot run ClickHouse SQL; production paths/integration tests should still use the real ClickHouse ActivitySensorStore ... ensure there is a clear comment explaining this distinction."
🤖 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/repositories/activity-sensor-store.test-helpers.ts` around lines 470 - 519, Add a short explanatory comment immediately above the SQL in getStream that this test helper reconstructs location samples from raw Postgres rows as a shim for tests and is not the production analytics query; explicitly mention that production reads deduped location data from ClickHouse (analytics.deduped_location) and that the real production path is implemented in the ClickHouse-backed ActivitySensorStore so future changes won't treat this Postgres SQL as canonical.
🤖 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 `@drizzle/0018_metric_stream_location_point.sql`:
- Around line 145-152: The cleanup DELETE currently targets any rows with
channel IN ('lat','lng','gps_accuracy') in fitness.metric_stream via a fresh
deleted_rows CTE using only LIMIT batch_size, which can remove rows that weren't
part of the preceding INSERT; instead, propagate the source row identifiers
(ctid or a PK column) from the INSERT CTE into the cleanup CTE and use those
exact identifiers in the DELETE so you only remove rows actually converted by
the INSERT path (do this for both affected branches referenced by the
INSERT/DELETE pair).
In `@packages/server/src/routers/health-kit-sync.test.ts`:
- Around line 1342-1353: The test currently uses execute.mock.calls.find(...)
which only checks the first matching "INSERT INTO fitness.metric_stream" call;
change to collect all matching calls by using execute.mock.calls.filter(...) (or
equivalent) and then assert across the combined payloads (e.g., join/concatenate
JSON.stringify of each call) or assert per-call that every matching insert
contains '"location"', 'ST_SetSRID', and 'horizontal_accuracy_m' and that none
contain '"lat"', '"lng"', or '"gps_accuracy"'; apply the same change for the
other similar blocks (the checks around lines with indices 1401-1409 and
1458-1466) and reference the metric_stream inserts produced by
pushWorkoutRoutes()/the code that triggers execute.mock.calls to ensure no
legacy lat/lng/gps_accuracy sneaks in.
In `@src/db/clickhouse-migrations.ts`:
- Around line 562-565: The materialized-view creation currently waits on
analytics.deduped_location but the native backfill only refreshes
analytics.deduped_sensor before recomputing analytics.activity_summary; update
the custom rebuild paths so they perform the same refresh+wait pair used by the
native backfill for deduped_sensor (i.e., call the refresh function for
"analytics","deduped_sensor" and then await
waitForClickHouseTable(client,"analytics","deduped_sensor") in the same spots
that already handle deduped_location) so both paths are consistent; keep the
existing waitForClickHouseTable calls for "analytics","deduped_location" and
"analytics","v_activity" as needed.
In `@src/db/metric-stream-location-point-migration.integration.test.ts`:
- Around line 118-143: The test only asserts coordinates but not the PostGIS
SRID, so modify the query to include public.ST_SRID(point) AS point_srid (or
call ST_SRID(point) in the SELECT) and add assertions that
locationResult.rows[0]?.point_srid === 4326 and
locationResult.rows[1]?.point_srid === 4326 to ensure the backfilled geometry
retains SRID 4326 for the metric_stream.location points.
---
Outside diff comments:
In `@src/providers/apple-health/db-insertion.test.ts`:
- Around line 1296-1313: Replace the two separate toContainEqual assertions that
separately check geometry and metadata with a single assertion that ensures
there is one location row object that contains both the geometry fields and the
metadata blob; specifically, update the assertion on capture.values[1] to assert
an objectContaining that includes providerId: "p1", activityId: "act-1",
channel: "location", point/latitude/longitude and metadata: {
horizontal_accuracy_m: 5.2 } together so the test fails if the writer split them
into two inserts.
---
Nitpick comments:
In `@packages/server/src/repositories/activity-sensor-store.test-helpers.ts`:
- Around line 470-519: Add a short explanatory comment immediately above the SQL
in getStream that this test helper reconstructs location samples from raw
Postgres rows as a shim for tests and is not the production analytics query;
explicitly mention that production reads deduped location data from ClickHouse
(analytics.deduped_location) and that the real production path is implemented in
the ClickHouse-backed ActivitySensorStore so future changes won't treat this
Postgres SQL as canonical.
In `@src/providers/apple-health/import.integration.test.ts`:
- Around line 863-867: Rename the terse callback parameter `r` in the location
assertions to a descriptive name (e.g., `metric` or `row`) to improve
readability: update the filter callback used to build `gpsRows`
(allMetrics.filter((...) => ...)) and the some() callback (gpsRows.some(...)) so
both use the chosen descriptive identifier while preserving the existing
conditions (activityId === run?.id and channel === "location", and the latitude
check against 40.7128).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f20a2c9-1f61-49c8-b9ff-ab0798044839
📒 Files selected for processing (45)
.github/workflows/deploy-web-stack.yml.github/workflows/test.ymlAGENTS.mdREADME.mddeploy/README.mddeploy/review-apps/review-compose.ymldeploy/stack.ymldocker-compose.e2e.ymldocker-compose.ymldocs/README.mddocs/altitude-provenance.mddocs/schema.dbmldocs/schema.pumldrizzle/0018_metric_stream_location_point.sqlpackages/server/src/repositories/activity-recording-repository.test.tspackages/server/src/repositories/activity-recording-repository.tspackages/server/src/repositories/activity-sensor-store.test-helpers.tspackages/server/src/repositories/clickhouse-activity-sensor-store.test.tspackages/server/src/repositories/clickhouse-activity-sensor-store.tspackages/server/src/repositories/health-kit-sync-repository.tspackages/server/src/repositories/intervals-repository.tspackages/server/src/routers/clickhouse-integration-test-helpers.test.tspackages/server/src/routers/clickhouse-integration-test-helpers.tspackages/server/src/routers/health-kit-sync-processors.tspackages/server/src/routers/health-kit-sync-schemas.tspackages/server/src/routers/health-kit-sync.test.tssrc/db/README.mdsrc/db/clickhouse-metric-stream-bootstrap.tssrc/db/clickhouse-migrations.test.tssrc/db/clickhouse-migrations.tssrc/db/metric-stream-location-point-migration.integration.test.tssrc/db/metric-stream-replica-identity.integration.test.tssrc/db/metric-stream-writer.test.tssrc/db/metric-stream-writer.tssrc/db/peerdb/metric-stream-cdc.sqlsrc/db/schema.tssrc/db/seed-dev-db.integration.test.tssrc/db/sensor-channels.tssrc/db/test-helpers.tssrc/providers/apple-health/db-insertion.test.tssrc/providers/apple-health/db-insertion.tssrc/providers/apple-health/import.integration.test.tssrc/providers/garmin.test.tssrc/providers/ride-with-gps-ext.test.tssrc/providers/ride-with-gps-sync.integration.test.ts
💤 Files with no reviewable changes (1)
- packages/server/src/repositories/health-kit-sync-repository.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/db/sensor-channels.ts (1)
9-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove
LOCATIONout of the scalar section and update storage docs.Line 9 says this block is scalar-only, but Line 20 adds a point-valued channel (
metric_stream.point). The header comments (Lines 4-6) also omit point storage, which is now outdated.Proposed minimal doc/classification fix
/** * Sensor sample channel constants. * * Each channel name identifies a type of measurement stored in the - * metric_stream table. Scalar channels use the `scalar` column; - * vector channels use the `vector` (real[]) column. + * metric_stream table. Scalar channels use the `scalar` column; + * vector channels use the `vector` (real[]) column; point channels + * use the `point` (PostGIS geometry) column. */ // ── Scalar channels (single numeric value) ────────────────── @@ /** Speed in m/s */ export const SPEED = "speed"; -/** Location point stored in metric_stream.point. */ -export const LOCATION = "location"; /** Altitude in meters */ export const ALTITUDE = "altitude"; @@ export const RR_INTERVAL_MS = "rr_interval_ms"; + +// ── Point channels (geometry) ─────────────────────────────── +/** Location point stored in metric_stream.point. */ +export const LOCATION = "location";🤖 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/sensor-channels.ts` around lines 9 - 20, The header and placement are inconsistent: remove LOCATION from the scalar channels block and add a new "Point channels" section (or move it under an existing non-scalar section) so that LOCATION is clearly classified as a point-valued channel (metric_stream.point); update the top-of-file header/comments to mention both scalar and point storage types (e.g., scalar numeric and point/location) and ensure the exported constant NAME LOCATION remains unchanged but is relocated under the new section to reflect its storage type.
🤖 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 `@src/db/clickhouse-migrations.ts`:
- Around line 181-191: Migration 0013 (id "0013_metric_stream_location_point")
only rebuilds downstream views but must fully rebuild
postgres_fitness.metric_stream so historical location rows get the new shape;
modify the migration (and the code around the earlier backfill at lines
~564-590) to drop postgres_fitness.metric_stream and
metric_stream_backfill_chunks before running
buildClickHouseBootstrapStatements(postgresConnectionString), then call
backfillNativeMetricStream(client, postgresConnectionString) and finally run
SYSTEM REFRESH/WAIT for analytics.deduped_sensor, analytics.deduped_location,
and analytics.activity_summary using runClickHouseMigrationStatement so the
recreated metric_stream is populated and the views are refreshed; consider
extracting this sequence into a helper named rebuildMetricStreamLocationPoint to
keep the logic together.
---
Outside diff comments:
In `@src/db/sensor-channels.ts`:
- Around line 9-20: The header and placement are inconsistent: remove LOCATION
from the scalar channels block and add a new "Point channels" section (or move
it under an existing non-scalar section) so that LOCATION is clearly classified
as a point-valued channel (metric_stream.point); update the top-of-file
header/comments to mention both scalar and point storage types (e.g., scalar
numeric and point/location) and ensure the exported constant NAME LOCATION
remains unchanged but is relocated under the new section to reflect its storage
type.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5925a46a-1d56-4a57-87e0-d1a1613afb9d
📒 Files selected for processing (13)
cspell.jsondeploy/README.mddeploy/stack.ymldocs/README.mddrizzle/0018_metric_stream_location_point.sqlpackages/server/src/repositories/activity-sensor-store.test-helpers.tspackages/server/src/routers/health-kit-sync.test.tssrc/db/clickhouse-migrations.test.tssrc/db/clickhouse-migrations.tssrc/db/metric-stream-location-point-migration.integration.test.tssrc/db/sensor-channels.tssrc/providers/apple-health/db-insertion.test.tssrc/providers/apple-health/import.integration.test.ts
✅ Files skipped from review due to trivial changes (4)
- docs/README.md
- cspell.json
- src/db/clickhouse-migrations.test.ts
- deploy/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- deploy/stack.yml
- src/providers/apple-health/import.integration.test.ts
- drizzle/0018_metric_stream_location_point.sql
- packages/server/src/routers/health-kit-sync.test.ts
- src/providers/apple-health/db-insertion.test.ts
- src/db/metric-stream-location-point-migration.integration.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/db/clickhouse-migrations.test.ts (1)
175-178: ⚡ Quick winReduce brittleness in the
system.tablescall-count assertionLine 178 hard-codes an implementation-level count (
33). This can break on harmless internal query-order/call-shape refactors. Prefer asserting requiredsystem.tableschecks instead of an exact total.Proposed test hardening
- const systemTableQueries = query.mock.calls.filter(([options]) => - String(options.query).includes("system.tables"), - ); - expect(systemTableQueries).toHaveLength(33); + const systemTableQueries = query.mock.calls + .map(([options]) => String(options.query)) + .filter((queryText) => queryText.includes("system.tables")); + expect(systemTableQueries.length).toBeGreaterThan(0); + expect(systemTableQueries.some((queryText) => queryText.includes("postgres_fitness"))).toBe(true); + expect(systemTableQueries.some((queryText) => queryText.includes("analytics"))).toBe(true);🤖 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-migrations.test.ts` around lines 175 - 178, The test hard-codes the total number of "system.tables" queries (expect(systemTableQueries).toHaveLength(33)), which is brittle; change this to assert the required checks instead—e.g., verify that systemTableQueries has at least one entry (expect(systemTableQueries.length).toBeGreaterThanOrEqual(1)) or assert that the filtered calls contain the specific expected query strings or table names by inspecting query.mock.calls via the existing filter (systemTableQueries) and checking for those required conditions (contains expected table names or a minimum count) rather than a fixed total.
🤖 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.
Nitpick comments:
In `@src/db/clickhouse-migrations.test.ts`:
- Around line 175-178: The test hard-codes the total number of "system.tables"
queries (expect(systemTableQueries).toHaveLength(33)), which is brittle; change
this to assert the required checks instead—e.g., verify that systemTableQueries
has at least one entry
(expect(systemTableQueries.length).toBeGreaterThanOrEqual(1)) or assert that the
filtered calls contain the specific expected query strings or table names by
inspecting query.mock.calls via the existing filter (systemTableQueries) and
checking for those required conditions (contains expected table names or a
minimum count) rather than a fixed total.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 650cef9c-6dfb-478e-8b14-478f7809c3bd
📒 Files selected for processing (7)
.github/workflows/review-app.ymldeploy/review-apps/README.mddocs/production-incident-baseline.mddocs/review-apps.mdsrc/db/clickhouse-migrations.test.tssrc/db/clickhouse-migrations.tssrc/db/sensor-channels.test.ts
✅ Files skipped from review due to trivial changes (3)
- deploy/review-apps/README.md
- src/db/sensor-channels.test.ts
- docs/production-incident-baseline.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/db/clickhouse-migrations.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/review-app.yml (2)
363-378: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove review-stack lifecycle orchestration out of ad-hoc shell and into Terraform-managed infrastructure flow.
The added
docker compose down/pull/uporchestration is CI shell-driven infra mutation. This should be modeled in Terraform-driven automation for this pipeline path.As per coding guidelines, "Prefer Terraform-native CI approaches — when automating infrastructure changes (compose deploys, DNS, server config), use Terraform with
templatefile(), providers, andterraform applyin CI rather than ad-hoc shell scripts."🤖 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 @.github/workflows/review-app.yml around lines 363 - 378, The CI step currently performing ad-hoc infra changes via the docker compose down/pull/up sequence and the pg_isready loop (the docker compose --env-file "$REVIEW_ENV_FILE" -p "$COMPOSE_PROJECT_NAME" -f deploy/review-apps/review-compose.yml down/pull/up lines and the for attempt ... pg_isready check) must be removed from the workflow and replaced with a Terraform-driven flow; update this job to invoke Terraform (init/plan/apply) against a review-stack module that uses templatefile()/providers to render and deploy the compose stack or equivalent resources, surface readiness via Terraform outputs or remote-exec/provisioner logic instead of the shell pg_isready loop, and ensure the workflow calls terraform apply with the appropriate workspace/vars instead of running docker compose directly.
353-394: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSplit
Deploy review stackinto granular named steps for observability.This block now contains teardown, pull, phased startup, readiness loops, migrations, seed, and app start in one script; failures are harder to localize in Actions UI.
Proposed workflow split (example)
- - name: Deploy review stack + - name: Tear down previous review stack if: steps.terraform.outputs.review_app_skipped != 'true' env: COMPOSE_PROJECT_NAME: dofek-review-pr-${{ github.event.pull_request.number }} DOCKER_HOST: ssh://root@${{ steps.terraform.outputs.review_server_ip }} REVIEW_ENV_FILE: ${{ github.workspace }}/.env.review - REVIEW_HOST: ${{ steps.terraform.outputs.review_host }} run: | set -euo pipefail + docker compose --env-file "$REVIEW_ENV_FILE" -p "$COMPOSE_PROJECT_NAME" -f deploy/review-apps/review-compose.yml down --remove-orphans --volumes - docker compose --env-file "$REVIEW_ENV_FILE" -p "$COMPOSE_PROJECT_NAME" -f deploy/review-apps/review-compose.yml down --remove-orphans --volumes + - name: Pull review stack images + if: steps.terraform.outputs.review_app_skipped != 'true' + env: + COMPOSE_PROJECT_NAME: dofek-review-pr-${{ github.event.pull_request.number }} + DOCKER_HOST: ssh://root@${{ steps.terraform.outputs.review_server_ip }} + REVIEW_ENV_FILE: ${{ github.workspace }}/.env.review + run: | + set -euo pipefail docker compose --env-file "$REVIEW_ENV_FILE" -p "$COMPOSE_PROJECT_NAME" -f deploy/review-apps/review-compose.yml pull web db clickhouse redis - docker compose --env-file "$REVIEW_ENV_FILE" -p "$COMPOSE_PROJECT_NAME" -f deploy/review-apps/review-compose.yml up -d db + # Keep remaining phases as separate steps: + # - Start DB + # - Wait for DB + # - Start ClickHouse/Redis + # - Wait for ClickHouse + # - Run migrate/seed + # - Start webAs per coding guidelines, "Keep workflow steps as granular as possible for visibility — prefer multiple small, clearly named CI steps over one large script step so the GitHub Actions visualization shows meaningful progress."
🤖 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 @.github/workflows/review-app.yml around lines 353 - 394, The "Deploy review stack" step is doing teardown, pull, phased startup, readiness loops, migrations, seed, and final app start in one large run block; split it into multiple smaller, named Actions steps (e.g., "Teardown review stack", "Pull images", "Start DB", "Wait for DB ready", "Start ClickHouse & Redis", "Wait for ClickHouse ready", "Run migrations", "Run seed", "Start web") that each reuse the same env vars (COMPOSE_PROJECT_NAME, REVIEW_ENV_FILE, REVIEW_HOST, DOCKER_HOST) and call the corresponding docker compose commands from the current run (the down, pull, up -d db, the db pg_isready loop, up -d clickhouse redis, the ClickHouse wget check loop, run --rm web migrate, run --rm web seed, and up -d web); ensure each step has a clear name and a short single-purpose run script so failures are visible in Actions UI.
🤖 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 `@drizzle/0018_metric_stream_location_point.sql`:
- Around line 63-111: The source_rows CTE currently self-joins lat, lng and gps
by sample key which can produce an N×M×G Cartesian expansion when a channel has
duplicate legacy rows; change to assign a row_number() over a partition by
(recorded_at, user_id, provider_id, source_type, activity_id, device_id) for
each channel (for aliases lat, lng, gps) and join on that ordinal as well as the
sample key so each physical legacy row maps 1:1 into the merged location row;
apply the same row_number+join fix to the second branch that also joins
lat/lng/gps (the other source_rows-like block referenced in the comment) to
preserve raw per-source cardinality.
---
Outside diff comments:
In @.github/workflows/review-app.yml:
- Around line 363-378: The CI step currently performing ad-hoc infra changes via
the docker compose down/pull/up sequence and the pg_isready loop (the docker
compose --env-file "$REVIEW_ENV_FILE" -p "$COMPOSE_PROJECT_NAME" -f
deploy/review-apps/review-compose.yml down/pull/up lines and the for attempt ...
pg_isready check) must be removed from the workflow and replaced with a
Terraform-driven flow; update this job to invoke Terraform (init/plan/apply)
against a review-stack module that uses templatefile()/providers to render and
deploy the compose stack or equivalent resources, surface readiness via
Terraform outputs or remote-exec/provisioner logic instead of the shell
pg_isready loop, and ensure the workflow calls terraform apply with the
appropriate workspace/vars instead of running docker compose directly.
- Around line 353-394: The "Deploy review stack" step is doing teardown, pull,
phased startup, readiness loops, migrations, seed, and final app start in one
large run block; split it into multiple smaller, named Actions steps (e.g.,
"Teardown review stack", "Pull images", "Start DB", "Wait for DB ready", "Start
ClickHouse & Redis", "Wait for ClickHouse ready", "Run migrations", "Run seed",
"Start web") that each reuse the same env vars (COMPOSE_PROJECT_NAME,
REVIEW_ENV_FILE, REVIEW_HOST, DOCKER_HOST) and call the corresponding docker
compose commands from the current run (the down, pull, up -d db, the db
pg_isready loop, up -d clickhouse redis, the ClickHouse wget check loop, run
--rm web migrate, run --rm web seed, and up -d web); ensure each step has a
clear name and a short single-purpose run script so failures are visible in
Actions UI.
🪄 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: ed389923-044a-4f88-9d61-2cf6bd3da3b5
📒 Files selected for processing (2)
.github/workflows/review-app.ymldrizzle/0018_metric_stream_location_point.sql
# Conflicts: # .github/workflows/review-app.yml # docs/altitude-provenance.md # src/db/metric-stream-writer.test.ts # src/db/sensor-channels.ts # src/providers/apple-health/db-insertion.test.ts # src/providers/apple-health/import.integration.test.ts # src/providers/garmin.test.ts # src/providers/ride-with-gps-ext.test.ts # src/providers/ride-with-gps-sync.integration.test.ts
Contributor
|
Review app is ready: This environment runs on a dedicated Hetzner server for PR #1111 and updates on each push. |
Asherlc
enabled auto-merge (squash)
May 10, 2026 01:41
This was referenced May 12, 2026
This was referenced May 19, 2026
This was referenced May 31, 2026
Merged
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
Store GPS samples as point-valued
locationmetric-stream rows with PostGIS geometry, denormalized latitude/longitude, and metadata JSON. Backfill legacylat,lng, andgps_accuracyrows into location rows, preserving matched FIT GPS accuracy as metadata and removing legacy scalar rows. Update providers, HealthKit sync, activity stream readers, ClickHouse mirrors, schema docs, and altitude/GPS accuracy provenance docs for the new representation.Verification
pnpm lint;pnpm tsc --noEmit;cd packages/server && pnpm tsc --noEmit;cd packages/web && pnpm tsc --noEmit;pnpm test --run --maxWorkers=4.Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes
Chores