Fix heart-rate zone stats when resting heart rate is invalid - #1084
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 a CloudBeaver service with DNS, Terraform provisioning, Swarm stack, deploy workflow inputs, and data-source JSON. Separately, rewrites ClickHouse native metric_stream backfill to split Timescale chunks into 6‑hour ranges, use overlap-based completion checks, and de-duplicate inserts by row id. ChangesCloudBeaver Service Deployment
Metric Stream Backfill Fix
Sequence Diagram(s)sequenceDiagram
participant Deploy as Deploy Workflow
participant Terraform as Terraform
participant SSH as SSH/SCP
participant Docker as Docker Swarm
participant Traefik as Traefik Router
participant CloudBeaver as CloudBeaver Service
Deploy->>Terraform: Apply DNS records & server provisioning
Terraform->>Terraform: Create A records, provision data volume
Terraform->>SSH: remote-exec to create /mnt/dofek-data/cloudbeaver/.../.dbeaver
Deploy->>SSH: Upload cloudbeaver-data-sources.json to data volume
Deploy->>Docker: Deploy Swarm stack with cloudbeaver service
Docker->>Docker: Pull pinned dbeaver/cloudbeaver image
Docker->>CloudBeaver: Start container with bind-mount
Traefik->>CloudBeaver: Route requests using CLOUDBEAVER_HOST_RULE to port 8978
sequenceDiagram
participant Migration as Migration Runner
participant Timescale as Timescale (Source)
participant Splitter as Range Splitter
participant ClickHouse as ClickHouse (Target)
participant Tracker as Completion Tracker
Migration->>Timescale: Fetch Timescale metric_stream chunks
Timescale-->>Migration: Return chunk identifiers
Migration->>Splitter: splitMetricStreamBackfillChunk(chunk)
Splitter-->>Migration: Return 6-hour MetricStreamBackfillRange segments
loop For each 6-hour range
Migration->>ClickHouse: Run INSERT ... LEFT JOIN existing_metric_stream (within bounds)
ClickHouse-->>Migration: Insert new rows only (existing_metric_stream.id IS NULL)
Migration->>Tracker: Record completion row with lower_bound/upper_bound
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/server.tf`:
- Around line 238-240: The triggers_replace for the CloudBeaver datasource
syncers only includes filesha256("${path.module}/cloudbeaver-data-sources.json")
so the provisioner won't re-run when the underlying volume is replaced; update
both triggers_replace arrays (the ones containing
filesha256("${path.module}/cloudbeaver-data-sources.json") at the two
occurrences) to also include the volume/mount-alias ID (e.g. add the mount alias
or volume resource id such as mount_alias.<name>.id or aws_ebs_volume.<name>.id)
so Terraform will detect the volume replacement and re-run the datasources_sync
provisioner. Ensure you add the same volume id expression to both trigger
arrays.
In `@src/db/clickhouse-migrations.test.ts`:
- Around line 345-356: The test mock for ClickHouse queries must return the
expected shape for the system.databases query (include { engine: "Atomic" }) so
shouldReplacePostgresFitnessDatabase() follows the Atomic branch; update the
vi.fn() mockImplementation that matches queryText (currently checking
"system.tables") to also detect "system.databases" and return [{ engine:
"Atomic" }]; additionally adjust the split-duration expectation for the 2.5-hour
case from one-hour chunks to six-hour chunks to match the updated splitting
logic (update the assertions that check chunk size/split intervals used in the
test cases that reference the 2.5-hour input, including the blocks that branch
on "0006_backfill_native_metric_stream" and "metric_stream_backfill_chunks").
🪄 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: 062619f8-6440-4dbd-84f2-503321bb0836
📒 Files selected for processing (11)
.github/workflows/deploy-web-environment.yml.github/workflows/deploy-web-stack.yml.github/workflows/deploy-web.ymldeploy/README.mddeploy/cloudbeaver-data-sources.jsondeploy/dns.tfdeploy/server.tfdeploy/stack.ymldocs/production-incident-baseline.mdsrc/db/clickhouse-migrations.test.tssrc/db/clickhouse-migrations.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Adds CloudBeaver to the production/staging Docker Swarm deployment (with DNS, Traefik routing, and persistent workspace/bootstrap config) and hardens the ClickHouse metric_stream backfill migration to be more resilient to timeouts and retries.
Changes:
- Introduces a new
cloudbeaverswarm service with Traefik routing, persistent workspace storage, and Terraform-synced datasource configuration. - Updates ClickHouse native
metric_streambackfill to operate in 6-hour ranges, treat broader completion ranges as covering subranges, and avoid duplicate inserts on retry. - Extends deployment workflows/Terraform/DNS and adds/updates tests + incident documentation for the backfill timeout.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/db/clickhouse-migrations.ts |
Split Timescale chunks into 6-hour backfill ranges; broaden completion-range matching; add anti-join to avoid duplicate inserts on retry. |
src/db/clickhouse-migrations.test.ts |
Adds test coverage for 6-hour range splitting and broader completion-range matching. |
docs/production-incident-baseline.md |
Updates incident entry describing the deploy timeout and the mitigations. |
deploy/stack.yml |
Adds CloudBeaver service with Traefik routing and workspace bind mount. |
deploy/server.tf |
Ensures CloudBeaver persistent directories exist and syncs datasource JSON into the workspace for prod/staging. |
deploy/dns.tf |
Adds Cloudflare DNS records for CloudBeaver prod/staging hostnames. |
deploy/cloudbeaver-data-sources.json |
Adds preconfigured Postgres + ClickHouse datasource definitions for CloudBeaver. |
deploy/README.md |
Documents CloudBeaver service/state path and URL. |
.github/workflows/deploy-web.yml |
Passes staging CloudBeaver host rule into the environment deploy workflow. |
.github/workflows/deploy-web-stack.yml |
Adds CloudBeaver host-rule env plumbing, pulls CloudBeaver image, and validates the new bind-mount path. |
.github/workflows/deploy-web-environment.yml |
Adds cloudbeaver_host_rule input and forwards it to the stack deploy workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Asherlc
changed the title
Contributor
|
Review app is ready: This environment runs on a dedicated Hetzner server for PR #1084 and updates on each push. |
…rlc/fix-deploy-fail # Conflicts: # docs/production-incident-baseline.md
Asherlc
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/repositories/activity-repository.ts`:
- Around line 445-453: The CASE fallback currently returns the literal 60 which
can violate the invariant resting_hr < up.max_hr; change the ELSE branch in the
resting_hr CASE (the one referencing rhr.resting_hr and up.resting_hr) to
compute a safe fallback that is capped below up.max_hr (use the smaller of 60
and up.max_hr minus one) and ensure it remains positive (at least 1); apply the
identical change to the other CASE occurrence that uses the same fallback so
both preserve resting_hr < up.max_hr.
🪄 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: 19ed7ded-aba9-4529-8732-c9d26acf064b
📒 Files selected for processing (2)
packages/server/src/repositories/activity-repository.test.tspackages/server/src/repositories/activity-repository.ts
✅ Files skipped from review due to trivial changes (1)
- packages/server/src/repositories/activity-repository.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server/src/routers/settings.integration.test.ts (1)
214-248: ⚡ Quick winAssert provider-row deletion to match test intent.
The test name says provider data is wiped, but there’s no assertion for
fitness.provider. Add a provider count check forSETTINGS_TEST_USER_ID(or provider id) aftersettings.deleteAllUserData.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/routers/settings.integration.test.ts` around lines 214 - 248, Add an assertion that provider rows are deleted by including a count query for fitness.provider in the same Promise.all and capturing it (e.g., add providersAfter to the destructured array alongside activitiesAfter,...,userSettingsAfter), then assert providersAfter.rows[0].count equals 0 after calling settings.deleteAllUserData; reference SETTINGS_TEST_USER_ID for the WHERE clause and the settings.deleteAllUserData call to ensure the test checks provider-row deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/routers/router-data.integration.test.ts`:
- Around line 1336-1338: The test currently silently returns when
highestHeartRateActivityId is falsy (variable highestHeartRateActivityId), which
masks fixture regressions; replace the early return with an explicit
failure—e.g., assert/throw or use the test framework's fail/expect—to
immediately fail the test if highestHeartRateActivityId is undefined or null
before calling query, ensuring the failing precondition is reported clearly.
---
Nitpick comments:
In `@packages/server/src/routers/settings.integration.test.ts`:
- Around line 214-248: Add an assertion that provider rows are deleted by
including a count query for fitness.provider in the same Promise.all and
capturing it (e.g., add providersAfter to the destructured array alongside
activitiesAfter,...,userSettingsAfter), then assert providersAfter.rows[0].count
equals 0 after calling settings.deleteAllUserData; reference
SETTINGS_TEST_USER_ID for the WHERE clause and the settings.deleteAllUserData
call to ensure the test checks provider-row deletion.
🪄 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: 217e4f78-6dc6-408d-9b80-21daf05e7561
📒 Files selected for processing (7)
docs/production-incident-baseline.mdpackages/server/src/repositories/activity-repository.test.tspackages/server/src/routers/router-data.integration.test.tspackages/server/src/routers/router.integration.test.tspackages/server/src/routers/settings.integration.test.tssrc/db/clickhouse-client.test.tssrc/db/clickhouse.ts
✅ Files skipped from review due to trivial changes (1)
- docs/production-incident-baseline.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/repositories/activity-repository.test.ts
…rlc/fix-deploy-fail
This was referenced May 2, 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
Fixes integration CI failures where HR zone totals were
0due to invalid resting-heart-rate parameters flowing into zone computations.Root cause
#findHeartRateZoneParamsusedCOALESCE(rhr.resting_hr, 60)and could pass derived resting HR values that are>= max_hr(or otherwise invalid) directly into zone calculations. That produced non-sensical zone math and zero total seconds.Fix
Hardened the SQL parameter selection in
packages/server/src/repositories/activity-repository.tsto only accept resting HR when it is positive and below max HR. If derived resting HR is invalid, the query now falls back touser_profile.resting_hrwhen valid; otherwise defaults to60.Added regression coverage in
packages/server/src/repositories/activity-repository.test.tsto assert heart-rate zone calls use fallback60when resting HR is invalid.Validation
Not run in this pass to keep changes minimal and avoid extra CI-impacting re-checks in this workspace.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests