Fix weekly reports and analytics worker microbatches - #1190
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? |
Automated Checks (advisory, non-blocking)✅ All checks passed. Surmado Code Review — Free tier limit reachedYou've used all 10 free reviews this month. Deterministic checks (secrets, model strings) still ran above. Upgrade to the Paid plan for 100 reviews/month + $15 per additional 100: https://app.surmado.com/checkout?plan=pr_review_starter Or wait until your next monthly window for 10 more free reviews. Surmado Code Review (v1.2-mt) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis PR fixes a production OOM incident by introducing a microbatch-bounded ChangesBounded Activity Graph and Analytics Model Refactoring
Weekly Report Backend and Frontend Updates
Integration Test and Incident Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Multi-layer change with new macro logic (connected components, priority selection), refactoring of analytics models, backend aggregation logic migration (week boundary semantics), UI state handling, and deployment/architecture changes (entrypoint decoupling). Requires verification of ClickHouse query correctness, null-handling edge cases, and week boundary computation across multiple time-zone-aware functions. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses correctness and stability issues in weekly performance reporting and ClickHouse-backed analytics microbatches, including decoupling the BullMQ worker startup from analytics dbt builds to avoid worker restart loops during dbt/ClickHouse OOM scenarios.
Changes:
- Update weekly report aggregation and UI behavior for Sunday-start weeks and no-training/sleep edge cases.
- Bound analytics activity graph construction to each dbt microbatch window and stop running
dbt buildin theworkerentrypoint path. - Stabilize Ultrahuman integration tests by pinning the system clock to the asserted fixture date.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/providers/ultrahuman-sync.integration.test.ts | Pins Vitest fake Date to make the integration test deterministic across real-time days. |
| packages/web/src/components/WeeklyReportCard.tsx | Adds “No training” state and refines zone display logic for weeks without training/sleep data. |
| packages/web/src/components/WeeklyReportCard.test.tsx | Adds coverage for the new “No training” UI state. |
| packages/server/src/routers/weekly-report.ts | Adjusts weekly report ClickHouse query logic (week bucketing, load/vitals edge cases) and simplifies current/history selection. |
| packages/server/src/routers/weekly-report.test.ts | Updates/extends router tests to assert new weekly bucketing/load/vitals behavior. |
| packages/server/src/repositories/weekly-report-repository.ts | Mirrors weekly report query changes in repository implementation. |
| entrypoint.sh | Removes dbt build from the worker mode so BullMQ worker startup isn’t blocked by analytics builds. |
| docs/production-incident-baseline.md | Documents the production incident motivating the decoupling/bounding changes. |
| analytics/models/sources.yml | Adds missing dbt sources needed by the bounded activity graph macro (device/provider priority). |
| analytics/models/read_models/read_model_microbatch.sql.test.ts | Adds assertions for worker/dbt decoupling and bounded activity graph usage in microbatch models. |
| analytics/models/read_models/activity_sensor_sample.sql | Switches to bounded recursive activity graph CTEs instead of global views. |
| analytics/models/read_models/activity_location_sample.sql | Switches to bounded recursive activity graph CTEs instead of global views. |
| analytics/macros/bounded_activity_graph.sql | Introduces a bounded recursive activity graph macro keyed to dbt microbatch start/end. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 13 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: These changes include modifications to weekly report SQL logic (week boundaries, null handling, activity counting), a new 206-line dbt macro for bounded activity graphs, and UI logic for training status—core business logic with moderate blast radius that warrants human review.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@analytics/macros/bounded_activity_graph.sql`:
- Around line 158-166: The merged CTE currently uses the highest-priority
member's times (any(best.started_at)/any(best.ended_at)) which can narrow the
group's true time range; replace those with group-wide bounds so
current_activity.started_at/ended_at reflect the full merged component
timeframe. Concretely, in the merged AS select (and the second occurrence later
in the file) change the time columns to use min(best.started_at) AS started_at
and max(best.ended_at) AS ended_at (instead of any(...)) so downstream joins in
activity_sensor_sample.sql include all edge samples for the merged activity.
- Around line 80-94: The overlap calculation uses a 1-hour fallback for NULL
ended_at which is inconsistent with active_activity and
activity_sensor_sample.sql that use a 12-hour window; update the two occurrences
of coalesce(..., started_at + INTERVAL 1 HOUR) (both for left_activity and
right_activity in the greatest/least expressions, numerator and denominator) to
use INTERVAL 12 HOUR so NULL-ended activities are treated as started_at + 12
hours for overlap computation.
🪄 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: 3587250d-428b-435c-a556-8551c63f252c
📒 Files selected for processing (13)
analytics/macros/bounded_activity_graph.sqlanalytics/models/read_models/activity_location_sample.sqlanalytics/models/read_models/activity_sensor_sample.sqlanalytics/models/read_models/read_model_microbatch.sql.test.tsanalytics/models/sources.ymldocs/production-incident-baseline.mdentrypoint.shpackages/server/src/repositories/weekly-report-repository.tspackages/server/src/routers/weekly-report.test.tspackages/server/src/routers/weekly-report.tspackages/web/src/components/WeeklyReportCard.test.tsxpackages/web/src/components/WeeklyReportCard.tsxsrc/providers/ultrahuman-sync.integration.test.ts
💤 Files with no reviewable changes (1)
- entrypoint.sh
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Auto-approved: This PR fixes a production incident by bounding activity graph construction to microbatch windows to prevent ClickHouse OOMs, decouples the BullMQ worker from dbt builds, corrects weekly report week bucketing and training load aggregation, and adds corresponding tests—all changes are...
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: The changes touch core business logic (weekly report SQL bucketing, training load math), analytics pipeline (new bounded activity macro replacing global views), and infrastructure (entrypoint removal of dbt from worker), which have significant blast radius and risk of breaking production data...
Re-trigger cubic
…alth-summary # Conflicts: # docs/production-incident-baseline.md
|
Storybook previews for This comment updates automatically on each PR push. |
Summary
Verification
CLICKHOUSE_URL=http://default:health@127.0.0.1:$(docker compose port clickhouse 8123 | sed 's/.*://') pnpm lintpnpm tsc --noEmit;cd packages/server && pnpm tsc --noEmit;cd packages/web && pnpm tsc --noEmitpnpm test:changedhit a late-suite local ClickHouse memory limit inanalytics.v_sleep, while the failed file passed in isolation.Summary by cubic
Fixes weekly report Sunday bucketing and training load math. Bounds and aligns the activity graph per microbatch to stop
ClickHouseOOMs and keep theBullMQworker independent from analytics builds.bounded_activity_graph()and switches sensor/location read models to it; bounds to the microbatch window via__dbt_internal_microbatch_event_time_start/end, and selects canonical activities using provider/device priority; addsprovider_priorityanddevice_prioritysources.entrypoint.shno longer runsdbt buildforworker; theBullMQworker starts independently.Written for commit d67e3b4. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes