Add Garmin dump imports and progress on iOS - #1603
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Pull Request Review SummaryThe implementation of the Garmin Dump Import feature is very clean, well-structured, and has excellent test coverage. I have identified one minor issue regarding the progress bar rendering for active imports:
Aside from this, the code looks great and is ready to merge once that minor fix is applied! LGTM! 🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds Garmin ZIP importing and generalizes file-import handling across server upload routes, shared mobile uploads, active-import progress, and web/mobile provider cards. The change also adds chunked-upload coverage, provider-card stories, and integration tests. ChangesFile import flow
Estimated code review effort: 5 (Critical) | ~100 minutes Possibly related PRs
Suggested labels: 🚥 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. |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
Mobile PreviewScan to open on device:
To test on device:
|
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? |
![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.
![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)
packages/mobile/lib/share-import.ts (1)
167-196: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftGarmin share imports still resolve to Apple Health. In
packages/mobile/app/providers/index.tsx:321-327, the shared-file path callsimportFile(sharedFileUri)without aproviderId, sopackages/mobile/lib/share-import.ts:167-196falls back toinferImportProviderFromFile(). That helper returnsapple-healthfor any.zipbefore it can ever reachgarmin-dump, so shared Garmin exports are routed to the wrong upload endpoint. Move Garmin detection ahead of the generic ZIP check or pass the provider through the share flow.🤖 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/mobile/lib/share-import.ts` around lines 167 - 196, Update inferImportProviderFromFile so Garmin exports are detected before the generic isAppleHealthLike ZIP check and return garmin-dump when the filename, extension, MIME type, or other existing Garmin identifying metadata matches. Preserve Apple Health detection for remaining ZIP imports, and ensure the shared-file import flow uses the corrected provider result.
🤖 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 `@packages/mobile/app/providers/index.test.tsx`:
- Around line 1278-1311: Strengthen the in-flight Garmin import test around
renderProvidersScreen and the garmin-card progress UI by asserting that the
progress bar reflects the mocked 64% activeImport.progress value. Query the
progress track or fill using the existing accessible/test identifier and verify
its rendered percentage or width, while retaining the current message and
import-only assertions.
In `@packages/mobile/app/providers/index.tsx`:
- Around line 678-702: Update the import progress mapping in the providerList
render around activeImportByProvider so server-resumed activeImport values are
converted to the ProviderCard syncProgress shape, mapping activeImport.progress
to percentage while preserving message. Keep localImportProgress precedence and
ensure both local and resumed imports provide percentage and message
consistently.
In `@packages/mobile/lib/share-import.ts`:
- Line 1: Update inferImportProviderFromFile to detect Garmin export ZIP/XML
files before the generic Apple Health classification, ensuring shared Garmin
archives resolve to "garmin-dump" while preserving "apple-health" for non-Garmin
ZIP/XML files.
In `@packages/server/src/routers/sync.ts`:
- Line 795: Replace the Date.now()-based fallback in the jobId construction with
a deterministic value incorporating providerId, while continuing to prefer
job.id when present. Ensure repeated polls of the same provider produce the same
fallback ID.
---
Outside diff comments:
In `@packages/mobile/lib/share-import.ts`:
- Around line 167-196: Update inferImportProviderFromFile so Garmin exports are
detected before the generic isAppleHealthLike ZIP check and return garmin-dump
when the filename, extension, MIME type, or other existing Garmin identifying
metadata matches. Preserve Apple Health detection for remaining ZIP imports, and
ensure the shared-file import flow uses the corrected provider result.
🪄 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: 0cb144ca-5f4d-445e-9be9-ec7c783c5b31
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
packages/mobile/app/providers/index.stories.tsxpackages/mobile/app/providers/index.test.tsxpackages/mobile/app/providers/index.tsxpackages/mobile/app/providers/provider-card.tsxpackages/mobile/lib/share-import.test.tspackages/mobile/lib/share-import.tspackages/mobile/package.jsonpackages/server/src/routers/sync.test.tspackages/server/src/routers/sync.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. |
Pull Request Review SummaryThis is an exceptionally high-quality, well-tested, and clean Pull Request! It successfully implements the Garmin Dump import flow, detects Garmin ZIP/XML exports correctly, and adds robust support for server-resumed active imports. Key Highlights:
Suggested Improvement:
Once that minor improvement is addressed, this PR is fully ready to merge! LGTM! 🚀 🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
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.
|
Addressed CodeRabbit review summary: #1603 (review) Fixed.
|
|
[Review Note] File: Pass |
|
🤖 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.
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
|
Addressed PR-level review note: #1603 (comment) Fixed.
|
|
🤖 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.
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.
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 59 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/mobile/app/providers/index.tsx (2)
288-317: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRun temporary-file cleanup when authentication is missing.
Line 290 returns before
finally, leaving document-picker cache copies on disk when the session is absent or expires. Move the token check insidetryso cleanup always runs and surface an actionable error.Proposed fix
const importFile = useCallback( async (fileUri: string, providerId?: ImportProviderId) => { - if (!sessionToken) return; try { + if (!sessionToken) { + throw new Error("Sign in before importing a file"); + } await importSharedFile(🤖 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/mobile/app/providers/index.tsx` around lines 288 - 317, Update importFile so the sessionToken check occurs inside its try block, ensuring deleteSharedFile runs through finally even when authentication is missing. Replace the early silent return with an actionable authentication error that follows the existing error handling path and preserves cleanup.
546-548: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the required query-state component for active-import failures.
Avoid consuming
(activeImports.data ?? [])whenactiveImports.errorexists, and render the failure throughQueryStatePanelrather than bespoke text.As per coding guidelines, “Treat loading, error, and empty as separate UI states” and use
QueryStatePanel.tsxfor screens and cards. As per path instructions, do not usequery.data ?? []whenquery.errorexists.Also applies to: 618-622
🤖 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/mobile/app/providers/index.tsx` around lines 546 - 548, Update the active-import UI flow around activeImportByProvider and its rendering branches to handle loading, error, and empty states separately. When activeImports.error exists, avoid reading activeImports.data via a fallback and render the failure with QueryStatePanel; retain normal data mapping and existing non-error behavior for successful queries.Sources: Coding guidelines, Path instructions
🤖 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 `@packages/mobile/app/providers/provider-card.tsx`:
- Around line 114-138: Update the provider card action rendering to track import
and sync activity separately: use the sync-specific state for the Sync button’s
disabled and loading behavior, and pass the import-specific importing state to
FileImportButton’s disabled/loading props. Anchor the changes to the
canRunManualSync/canImport action block and FileImportButton, preserving each
action’s own spinner behavior for dual-capability providers.
In `@packages/mobile/components/ProviderStatsBreakdown.tsx`:
- Around line 51-54: Update ProviderStatsBreakdown to display the total-record
value supplied by the API instead of calculating it on mobile. Update
packages/mobile/app/providers/index.test.tsx at lines 738-739 to provide and
assert the server-returned total.
In `@packages/mobile/lib/share-import.test.ts`:
- Around line 184-207: Update the chunk request assertions in the share import
test so the second request’s x-upload-id is compared directly with the first
request’s captured x-upload-id, while retaining the existing share-999 prefix
validation on the first request.
In `@packages/mobile/lib/share-import.ts`:
- Around line 282-286: Update the Garmin handling in
packages/mobile/lib/share-import.ts:282-286 to preserve .xml in x-file-ext
instead of relabeling Garmin XML as .zip. Update the Garmin provider picker
configuration in packages/mobile/app/providers/file-import-providers.ts:37-40 to
accept XML MIME types, keeping Garmin XML support consistent through inference,
selection, and upload.
In `@packages/server/src/routes/upload.ts`:
- Around line 551-569: Update the upload assembly flow around assembleChunks,
enqueueImport, and the catch handler so chunkDir cleanup is best-effort and
cannot prevent enqueueImport from running. In the failure path, also remove the
partially assembled assembledFile with best-effort cleanup, while preserving the
existing upload error status handling.
- Around line 541-549: Update the upload completion flow around
uploadStateStore.deleteUploadSession, setUploadStatus, and the following async
assembly block so it durably persists the assembly job or retains an idempotent
terminal upload session until queue insertion succeeds. Do not delete the
session or return the assembling response until the durable handoff is complete,
and ensure retries and process restarts can still resume or enqueue the import
without creating duplicates.
- Around line 443-447: Update the upload error handlers at the shown catch
blocks, including those around the handlers near lines 559-570 and 572-588, to
call captureException with the caught error before logging or returning the
failure response. Preserve the existing cleanup, logging, and HTTP response
behavior while ensuring every unexpected upload exception is reported to
telemetry.
- Around line 482-507: Update the chunked-upload flow around the upload session
and streamToFile calls to enforce limits across the entire upload: cap
chunkTotal, track each chunk’s actual byte count in the session, and atomically
reserve/reject cumulative bytes against config.maxUploadBytes before accepting
data. Ensure concurrent chunk requests cannot exceed the limit, and preserve the
existing forbidden and mismatched-total responses.
- Around line 399-408: Update the ownership check in the job-status handler
using getImportJobStatus so access is allowed only when status.userId is present
and matches userId. Return the existing 403 Forbidden response for missing or
mismatched ownership metadata, while preserving the 404 response for unknown
jobs and the success response for owned jobs.
In `@packages/web/src/components/file-import-configs.ts`:
- Around line 48-51: Update getFileImportConfig to verify providerId is an own
key of fileImportConfigs using Object.hasOwn before indexing it; preserve the
special apple_health handling and return undefined for inherited or unknown
properties.
In `@packages/web/src/components/FileImportProviderCard.tsx`:
- Around line 5-10: Update FileImportProviderCardProps to intersect
FileImportZoneProps only with the required providerId property, removing the
redundant stats, recentLogs, and showDetailsLink declarations.
In `@packages/web/src/components/FileImportZone.tsx`:
- Around line 219-226: Replace the non-form grouping element in the
FileImportZone drag-and-drop container with a div, preserving its aria-label and
drag event handlers; also update the matching closing fieldset tag to div.
---
Outside diff comments:
In `@packages/mobile/app/providers/index.tsx`:
- Around line 288-317: Update importFile so the sessionToken check occurs inside
its try block, ensuring deleteSharedFile runs through finally even when
authentication is missing. Replace the early silent return with an actionable
authentication error that follows the existing error handling path and preserves
cleanup.
- Around line 546-548: Update the active-import UI flow around
activeImportByProvider and its rendering branches to handle loading, error, and
empty states separately. When activeImports.error exists, avoid reading
activeImports.data via a fallback and render the failure with QueryStatePanel;
retain normal data mapping and existing non-error behavior for successful
queries.
🪄 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: adf19b75-9f73-4c65-a99f-da4075ca30cc
📒 Files selected for processing (23)
packages/mobile/app/providers/file-import-button.tsxpackages/mobile/app/providers/file-import-provider-card.tsxpackages/mobile/app/providers/file-import-providers.tspackages/mobile/app/providers/index.test.tsxpackages/mobile/app/providers/index.tsxpackages/mobile/app/providers/provider-card.tsxpackages/mobile/app/providers/styles.tspackages/mobile/components/ProviderStatsBreakdown.tsxpackages/mobile/lib/share-import.test.tspackages/mobile/lib/share-import.tspackages/server/src/routes/upload.test.tspackages/server/src/routes/upload.tspackages/web/src/components/DataSourcesPanel.test.tsxpackages/web/src/components/DataSourcesPanel.tsxpackages/web/src/components/FileImportButton.stories.tsxpackages/web/src/components/FileImportButton.tsxpackages/web/src/components/FileImportProviderCard.stories.tsxpackages/web/src/components/FileImportProviderCard.tsxpackages/web/src/components/FileImportZone.test.tsxpackages/web/src/components/FileImportZone.tsxpackages/web/src/components/file-import-configs.tspackages/web/src/pages/ProviderDetailPage.test.tsxpackages/web/src/pages/ProviderDetailPage.tsx
Summary of ReviewThe changes in this Pull Request are exceptionally clean, robust, and well-tested. All previously identified issues and edge cases have been fully addressed:
Excellent work! This PR is ready to be merged. LGTM 🚀 🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
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.
|
Follow-up for the latest CodeRabbit PR-level/outside-diff review items.\n\nFixed.\n- Moved the mobile missing-session check inside the import try/finally path so shared/document-picker temp files are still deleted and the user sees "Sign in before importing a file".\n- Active import query failures now render through QueryStatePanel and no longer read activeImports.data when activeImports.error exists.\n- Provider stats now include server-supplied totalRecords; shared providerStatsTotal prefers that API value and mobile tests provide/assert it.\n- Upload status now forbids missing ownership metadata instead of treating it as public.\n- Chunked uploads now cap chunk count, record actual per-chunk byte counts atomically, reject cumulative uploads over the route limit, and keep the upload session until assembly/enqueue completes.\n- Final chunk handling now enqueues before returning processing, makes chunk cleanup best-effort after enqueue, and removes partial assembled files on assembly/enqueue failure.\n- Verified older PR-level items remain addressed: Garmin ZIP inference happens before generic Apple Health ZIP handling, active import progress maps to ProviderCard percentage/message, and active import fallback job IDs are deterministic by provider.\n\nCommit: 680f293 |
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.
Pull Request Review SummaryThe changes in Key Observations:
Minor Suggestion:
Overall, the code is clean, secure, and ready to be merged! LGTM! 🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
|
[Review Note] File: To ensure that the chunk bytes TTL is kept in sync with the session TTL (especially if the session TTL is extended or updated), it would be a good idea to also expire the |
|
🤖 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.
|
🤖 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.
|
🤖 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.
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
|
Addressed CodeReviewBot comment #1603 (comment). Fixed.
|
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.
Summary
Previously, iOS classified every shared ZIP as Apple Health and could not resume import progress; this adds explicit Garmin Dump ZIP selection through the native document picker.
Garmin files now use the existing upload/status endpoints, while a user-scoped active-import query restores and polls provider-card progress across navigation or relaunch and surfaces queue errors.
The change includes the Expo document-picker dependency, a Storybook Garmin import state, and TDD coverage for mobile and server flows.
Validation
Validated with
pnpm test:mobile(825 tests), focused server router tests (108 tests),pnpm typecheck, and the fullpnpm lintpipeline.Summary by cubic
Adds Garmin dump and Kaya CSV imports with a shared file-import flow on iOS and web. Uploads are chunked and resumable with per-user progress; final chunks return processing, and provider stats show server-computed totals.
New Features
expo-document-pickerfor Apple Health ZIP/XML, Garmin ZIP, and Strong/Cronometer/Kaya CSVs; explicit provider selection; shared provider card with “Import file”; resumable uploads with status polling (Apple Health progress prioritized).totalRecordsfrom the server.sync.activeImportsreturns current‑user jobs with progress/messages.Bug Fixes
Written for commit 91b582f. Summary will update on new commits.
Summary by CodeRabbit