Fix activity source attribution for grouped workouts - #1468
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? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughAdds per-member ChangesSubsource-aware activity source attribution
Test database template naming isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ClickHouse as ClickHouse deduped_activities
participant Postgres as fitness.v_activity
participant Repository as ActivityRepository
participant Model as ActivitySourceAttribution
participant WebMobile as Web and Mobile UI
ClickHouse->>Postgres: source_external_ids with subsource and memberActivityId
Postgres->>Repository: absent_source_external_ids and source_external_ids
Repository->>Model: parsed SourceExternalIdEntry[]
Model->>Model: toSourceLinks() groups active and absent links
Model-->>WebMobile: sourceLinks[]
WebMobile->>WebMobile: render removed, URL, or plain labels
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
|
Storybook previews for This comment updates automatically on each PR push. |
Mobile PreviewScan to open on device:
To test on device:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 14 files
Confidence score: 3/5
drizzle/0040_v_activity_source_subsources.sqlwritessource_external_ids.memberActivityIdas a JSON number, whilededuped_activities.sqlappears to compare/serialize it as a string; that type mismatch can break joins/deduping and lead to duplicate or missing activity records after deploy. Align the type on both sides (store as string or cast consistently in ClickHouse) and run a quick backfill/query check before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
analytics/models/read_models/deduped_activities.sql (1)
43-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
subsourceto absent source links too
Removed grouped members lose upstream-app attribution becauseActivitySourceAttributionreadssubsourcefor absent entries, butabsent_source_linksnever populates it. Plumb the source-app field intoabsent_group_membersand setsubsourceinanalytics/models/read_models/deduped_activities.sql,drizzle/0040_v_activity_source_subsources.sql, anddrizzle/_views/01_v_activity.sql.🤖 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 `@analytics/models/read_models/deduped_activities.sql` around lines 43 - 59, The absent source link mapping is missing the source-app attribution field, so update the absent-member pipeline to carry and emit `subsource` end to end. Add `subsource` to the `absent_group_members` data flow, then include it in the `absent_source_links` CTE in `deduped_activities.sql` using the existing `ActivitySourceAttribution`/source-app naming so absent entries can be read consistently. Make the matching schema/view changes in `drizzle/0040_v_activity_source_subsources.sql` and `drizzle/_views/01_v_activity.sql` so the field is populated everywhere the absent-source path is built.
♻️ Duplicate comments (1)
drizzle/_views/01_v_activity.sql (1)
181-196: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDuplicate of the migration-file gap:
absent_source_linksmissessubsourcehere too.Since this is the generated view SQL that
drizzle/0040_v_activity_source_subsources.sqlproduces, fixing the migration (see comment there) should update this file identically. Flagging here for completeness since this is the file actually applied viaCREATE OR REPLACE VIEW.🤖 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 `@drizzle/_views/01_v_activity.sql` around lines 181 - 196, The absent_source_links CTE is still missing subsource in the JSON payload, matching the same gap from the migration-generated view. Update the absent_source_links aggregation in v_activity.sql to include the subsource field alongside providerId, externalId, memberActivityId, and providerAbsentAt, using the same shape and naming as the rest of the activity source/view helpers so the CREATE OR REPLACE VIEW output stays consistent.
🤖 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
`@docs/superpowers/plans/2026-07-03-activity-source-attribution-domain-model.md`:
- Around line 1-3: The opening note in the Activity Source Attribution Domain
Model TDD Plan still contains an agent-only “superpowers:*” directive, which is
not allowed in docs. Update the document header so it reads as plain,
self-contained plan text with no references to agent skills or workflow
instructions, or move that instruction into AGENTS.md; keep the rest of the plan
unchanged.
In `@drizzle/0040_v_activity_source_subsources.sql`:
- Around line 207-222: The absent source link aggregation in absent_source_links
is missing subsource, so tombstoned entries lose the original source attribution
and fall back to the generic provider label in
ActivitySourceAttribution.#toSourceLink. Update the JSONB_BUILD_OBJECT in the
absent_source_links CTE to mirror source_external_ids by carrying subsource from
t.raw->>'sourceName' or t.source_name, keeping the existing ordering and
grouping intact so the raw source metadata survives deduplication.
In `@packages/server/src/models/activity-source-attribution.ts`:
- Around line 91-101: The `toSourceLinks` logic in `ActivitySourceAttribution`
is filtering absent entries using only `providerId`, which causes removed
subsources under the same provider to disappear when one active subsource
exists. Update the deduping key in `toSourceLinks` (and any related lookup/set
logic) to consider both `providerId` and `subsource`, so `#activeEntries` and
`#absentEntries` can coexist for different grouped apps. Add a regression test
in `activity-source-attribution.test.ts` or `activity.test.ts` that creates an
active and an absent entry with the same `providerId` but different `subsource`
values and asserts both source links render separately, including the removed
marker for the absent one.
---
Outside diff comments:
In `@analytics/models/read_models/deduped_activities.sql`:
- Around line 43-59: The absent source link mapping is missing the source-app
attribution field, so update the absent-member pipeline to carry and emit
`subsource` end to end. Add `subsource` to the `absent_group_members` data flow,
then include it in the `absent_source_links` CTE in `deduped_activities.sql`
using the existing `ActivitySourceAttribution`/source-app naming so absent
entries can be read consistently. Make the matching schema/view changes in
`drizzle/0040_v_activity_source_subsources.sql` and
`drizzle/_views/01_v_activity.sql` so the field is populated everywhere the
absent-source path is built.
---
Duplicate comments:
In `@drizzle/_views/01_v_activity.sql`:
- Around line 181-196: The absent_source_links CTE is still missing subsource in
the JSON payload, matching the same gap from the migration-generated view.
Update the absent_source_links aggregation in v_activity.sql to include the
subsource field alongside providerId, externalId, memberActivityId, and
providerAbsentAt, using the same shape and naming as the rest of the activity
source/view helpers so the CREATE OR REPLACE VIEW output stays consistent.
🪄 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: 6eb80476-aeec-4bbe-9cb7-0b514ed9257b
📒 Files selected for processing (16)
analytics/models/read_models/deduped_activities.sqlanalytics/models/read_models/read_model_microbatch.sql.test.tsdocs/production-incident-baseline.mddocs/superpowers/plans/2026-07-03-activity-source-attribution-domain-model.mddrizzle/0040_v_activity_source_subsources.sqldrizzle/_views/01_v_activity.sqlpackages/mobile/app/activity/[id].test.tsxpackages/mobile/app/activity/[id].tsxpackages/server/src/models/activity-source-attribution.test.tspackages/server/src/models/activity-source-attribution.tspackages/server/src/models/activity.test.tspackages/server/src/repositories/activity-repository.tspackages/web/src/pages/ActivityDetailPage.test.tsxpackages/web/src/pages/ActivityDetailPage.tsxsrc/db/test-helpers.test.tssrc/db/test-helpers.ts
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Includes database migration, business logic changes, and UI updates for activity source attribution. These are high-impact changes that require human review to ensure correctness and safety.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
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)
docs/superpowers/plans/2026-07-03-activity-source-attribution-domain-model.md (1)
97-109: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a real sequential migration name and run the migration.
drizzle/YYYY_v_activity_source_attribution_model.sqlis a placeholder, and Task 5 only runslint:migrations. As per path instructions, manual Drizzle migrations must be sequentially numbered, andpnpm migratemust run after creating them.🤖 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/superpowers/plans/2026-07-03-activity-source-attribution-domain-model.md` around lines 97 - 109, Task 5 still uses a placeholder migration name and only mentions linting, so update the migration to a real sequential Drizzle filename in the source-attribution model change and make sure the new migration is actually run. Use the migration and SQL view artifacts referenced by the task, especially drizzle/_views/01_v_activity.sql and the new drizzle migration file, and add the required pnpm migrate step after creating it. Keep the typed source-attribution projection changes in sync with the migration so the read model and tests reflect the applied schema.Source: Path instructions
packages/server/src/models/activity-source-attribution.ts (1)
133-177: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
subsourceinpartialAbsentSources()—packages/web/src/pages/ActivitiesPage.tsxformatsProviderAbsentSourcedirectly, so droppingentry.subsourcehere regresses grouped labels back to generic provider names.partialAbsenceSummary()/#formatRemovedSources()are not on the current UI path; if they stay, they should use the sameproviderSourceLabel(providerId, subsource)helper.🤖 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/models/activity-source-attribution.ts` around lines 133 - 177, `partialAbsentSources()` is dropping the `subsource` field from `ProviderAbsentSource`, which breaks grouped labels in `ActivitiesPage`. Update `ActivitySourceAttribution.partialAbsentSources()` to preserve `entry.subsource` alongside `providerId` and `providerAbsentAt`, and make sure any remaining formatting paths like `partialAbsenceSummary()` and `#formatRemovedSources()` use `providerSourceLabel(providerId, subsource)` instead of plain provider names.
🤖 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 `@vitest.config.ts`:
- Around line 85-86: The coverage config is excluding the production migration
runner instead of fixing the missing test coverage. Remove the blanket exclusion
for src/db/clickhouse-migrations.ts from vitest.config.ts and restore focused
unit tests around runClickHouseMigrations to cover the branching for
requiresPreviouslyAppliedMigrationId, migration.run, and statement execution by
mocking the ClickHouse client. Keep exclusions limited to legitimate test-only
or fixture paths.
---
Outside diff comments:
In
`@docs/superpowers/plans/2026-07-03-activity-source-attribution-domain-model.md`:
- Around line 97-109: Task 5 still uses a placeholder migration name and only
mentions linting, so update the migration to a real sequential Drizzle filename
in the source-attribution model change and make sure the new migration is
actually run. Use the migration and SQL view artifacts referenced by the task,
especially drizzle/_views/01_v_activity.sql and the new drizzle migration file,
and add the required pnpm migrate step after creating it. Keep the typed
source-attribution projection changes in sync with the migration so the read
model and tests reflect the applied schema.
In `@packages/server/src/models/activity-source-attribution.ts`:
- Around line 133-177: `partialAbsentSources()` is dropping the `subsource`
field from `ProviderAbsentSource`, which breaks grouped labels in
`ActivitiesPage`. Update `ActivitySourceAttribution.partialAbsentSources()` to
preserve `entry.subsource` alongside `providerId` and `providerAbsentAt`, and
make sure any remaining formatting paths like `partialAbsenceSummary()` and
`#formatRemovedSources()` use `providerSourceLabel(providerId, subsource)`
instead of plain provider names.
🪄 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: bb491ddd-94ec-452c-ab9b-ccbe4c5a3e07
📒 Files selected for processing (13)
analytics/models/read_models/deduped_activities.sqldocs/superpowers/plans/2026-07-03-activity-source-attribution-domain-model.mddrizzle/0040_v_activity_source_subsources.sqldrizzle/_views/01_v_activity.sqlpackages/server/src/models/activity-source-attribution.test.tspackages/server/src/models/activity-source-attribution.tspackages/server/src/repositories/activity-repository.tssrc/db/clickhouse-migrations.test.tssrc/db/clickhouse-migrations/0034_move_metric_stream_to_ingest.test.tssrc/db/clickhouse-migrations/custom-runs.test.tssrc/db/clickhouse-migrations/registry.test.tssrc/db/clickhouse-migrations/sql.test.tsvitest.config.ts
💤 Files with no reviewable changes (5)
- src/db/clickhouse-migrations/sql.test.ts
- src/db/clickhouse-migrations/0034_move_metric_stream_to_ingest.test.ts
- src/db/clickhouse-migrations/registry.test.ts
- src/db/clickhouse-migrations.test.ts
- src/db/clickhouse-migrations/custom-runs.test.ts
|
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. |
|
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. |
|
Addressed the remaining actionable outside-diff review feedback.
|
|
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. |
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
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. |
Preserves member-level activity source attribution through the SQL read models, server model, and web/mobile activity detail screens.
Web and mobile now render distinct Strong via Apple Health, WHOOP via Apple Health, and WHOOP Cloud sources for grouped workouts.
Adds the incident baseline entry and the follow-up issue-TDD plan for #1467.
Verification: focused server/web/mobile/analytics tests, lint with local ClickHouse URL, migration policy, and root/server/web/mobile typechecks passed; full
CI=1 pnpm testwas attempted but failed after local ClickHouse was stopped, with onlyECONNREFUSED 127.0.0.1:8123failures.Summary by cubic
Fixes source attribution for grouped workouts by preserving per-member subsources and member IDs across SQL and server models. Web and mobile now show distinct source links like Strong (via Apple Health), WHOOP (via Apple Health), and WHOOP (Cloud), including removed sources and correct provider names; also adds a short TDD plan doc and updates the incident baseline.
Bug Fixes
subsourceand stringmemberActivityIdfor active and absent members in ClickHousededuped_activitiesand Postgresv_activity; keep removed subsources when another subsource is active.ActivitySourceAttribution: key byproviderId+subsource, includesubsourceon absent entries, emitmemberActivityIdas string, and label providers by name when no URL exists; do not collapse links by provider.sourceLinksin order, including multiple Apple Health subsources and removed entries; add focused tests.Migration
0040_v_activity_source_subsources.sqlto rebuildfitness.v_activity. Deploy analytics SQL and refresh the read model.Written for commit 36d6b59. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation