[server] Fix duplicate Garmin dump activities - #1673
Conversation
Reuse uniquely matching Garmin summaries when dump FIT IDs differ. Include years in shared datetime labels and document safe Docker cleanup for local validation.
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? |
|
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. |
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
Reviewer's GuideAdds an exact-identity lookup for Garmin dump activities to reuse existing summaries when FIT external IDs differ, updates shared date-time formatting to include the year, and documents safe Docker disk cleanup for running required validations. Sequence diagram for Garmin dump exact-identity reuse in FIT importsequenceDiagram
participant processFitFileImportJob
participant providerActivitySync
participant db
participant metricStreamWriter
processFitFileImportJob->>providerActivitySync: findUniqueProviderActivityByExactIdentity(db, identity)
providerActivitySync->>db: executeWithSchema(sql SELECT ... LIMIT 2)
db-->>providerActivitySync: [{ id } | [] | multiple]
alt [unique exact identity match]
providerActivitySync-->>processFitFileImportJob: { id: summaryActivityId }
processFitFileImportJob->>metricStreamWriter: replaceMetricStreamBatch(db, { activityId: summaryActivityId }, streams, "file")
else [absent or ambiguous identity]
providerActivitySync-->>processFitFileImportJob: undefined
processFitFileImportJob->>providerActivitySync: upsertProviderActivity(db, insertValues, updateValues)
providerActivitySync->>db: sql INSERT ... ON CONFLICT DO UPDATE
db-->>providerActivitySync: { id: newActivityId }
providerActivitySync-->>processFitFileImportJob: { id: newActivityId }
processFitFileImportJob->>metricStreamWriter: replaceMetricStreamBatch(db, { activityId: newActivityId }, streams, "file")
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Relying on strict equality for
startedAt/endedAtinfindUniqueProviderActivityByExactIdentitycould be fragile if provider timestamps differ even slightly between summary and FIT data (e.g., precision/rounding or timezone normalization); consider normalizing or using a small tolerance window to make matching more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Relying on strict equality for `startedAt`/`endedAt` in `findUniqueProviderActivityByExactIdentity` could be fragile if provider timestamps differ even slightly between summary and FIT data (e.g., precision/rounding or timezone normalization); consider normalizing or using a small tolerance window to make matching more robust.
## Individual Comments
### Comment 1
<location path="AGENTS.md" line_range="83" />
<code_context>
+- **Docker cleanup is in scope for required validation**: When Docker disk exhaustion blocks lint, tests, or builds, remove disposable resources created by the current workspace and prune rebuildable build cache first (`docker builder prune -af`). Prune unused images only if more space is required. Preserve running containers and named volumes belonging to other workspaces; never run `docker volume prune` or `docker system prune --volumes` without explicit user approval. Follow [Docker's pruning guidance](https://docs.docker.com/engine/manage-resources/pruning/) and the local recovery steps in [`docs/testing.md`](docs/testing.md#docker-disk-recovery).
</code_context>
<issue_to_address>
**nitpick (typo):** Consider adding "the" before "rebuildable build cache" for grammatical consistency.
In this sentence, "prune rebuildable build cache first" should be "prune the rebuildable build cache first" for more natural phrasing and to match `docs/testing.md` ("prune the rebuildable build cache").
```suggestion
- **Docker cleanup is in scope for required validation**: When Docker disk exhaustion blocks lint, tests, or builds, remove disposable resources created by the current workspace and prune the rebuildable build cache first (`docker builder prune -af`). Prune unused images only if more space is required. Preserve running containers and named volumes belonging to other workspaces; never run `docker volume prune` or `docker system prune --volumes` without explicit user approval. Follow [Docker's pruning guidance](https://docs.docker.com/engine/manage-resources/pruning/) and the local recovery steps in [`docs/testing.md`](docs/testing.md#docker-disk-recovery).
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
📝 WalkthroughWalkthroughAdds exact provider-activity identity reuse for Garmin FIT imports, includes the year in datetime formatting, and documents safe Docker disk-recovery steps for required validation. ChangesProvider activity identity reuse
Datetime output update
Docker disk recovery guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FITImportJob
participant ProviderActivitySync
participant ActivityDatabase
participant MetricStreamStore
FITImportJob->>ProviderActivitySync: Find Garmin activity by exact identity
ProviderActivitySync->>ActivityDatabase: Query active matching rows
ActivityDatabase-->>ProviderActivitySync: Return matching activity rows
ProviderActivitySync-->>FITImportJob: Return one id or undefined
FITImportJob->>MetricStreamStore: Replace metrics for resolved activity
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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. |
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![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 `@src/db/provider-activity-sync.ts`:
- Line 43: Update providerActivityIdSchema to validate id with Zod’s strict UUID
string format instead of accepting any string, preserving the existing object
schema and runtime boundary behavior.
In `@src/jobs/process-fit-file-import-job.test.ts`:
- Around line 34-36: Update the provider-activity-sync module mock in
process-fit-file-import-job.test.ts to include an upsertProviderActivity export
alongside findUniqueProviderActivityByExactIdentity, wiring it to the existing
mock implementation used by the test upsert flow.
🪄 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: c9bbe054-ea68-483e-a4cb-328014e81b61
📒 Files selected for processing (9)
AGENTS.mddocs/testing.mdpackages/format/src/format.test.tspackages/format/src/format.tssrc/db/provider-activity-sync.integration.test.tssrc/db/provider-activity-sync.test.tssrc/db/provider-activity-sync.tssrc/jobs/process-fit-file-import-job.test.tssrc/jobs/process-fit-file-import-job.ts
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
Asherlc
changed the title
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Review follow-up: the exact timestamp equality is intentional. The supplied Garmin summary and FIT session resolve to the same millisecond timestamps, while a tolerance window could merge two distinct activities; absent or ambiguous matches deliberately fall back to preserving both raw imports. The lookup therefore stays conservative and is covered by the one-millisecond mismatch integration assertion. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
🤖 Review failed: an error occurred during analysis. |
Summary
Validation
pnpm lintpnpm test:unit(11,814 passed)