[native] Bound FIT import memory with native streaming - #1646
Merged
Merged
Conversation
Replace whole-file JavaScript decoding with Garmin's native C++ decoder and backpressured batches so large and concurrent imports cannot multiply the complete parsed object graph in the Node heap.
Preserve current file-type routing and dropped-field telemetry while deriving counts from capped native metadata instead of reconstructing complete JavaScript message graphs.
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. |
![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}
Contributor
There was a problem hiding this comment.
Sorry @Asherlc, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughThis PR replaces JavaScript worker-thread FIT parsing with a native C++ streaming decoder that applies bounded batch limits and acknowledgement backpressure. FIT data flows through the canonical ChangesNative FIT streaming decoder and protocol
Queued FIT import job and persistence
Worker startup injection and coordination
Provider FIT route-through to import job
Build, CI, Docker, and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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 |
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.
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jobs/process-fit-file-import-job.ts (1)
211-286: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftDo not mutate committed metrics before the decoder protocol completes.
Callbacks persist weight rows and clear/write activity samples before
streamFitFilevalidates its final exit and message count. A lateFitDecoderErrortherefore leaves partial weight data or erases an activity’s previous samples; the job then becomes unrecoverable and deletes its source file.Stage each import and atomically replace the committed scope only after successful stream completion. Add a regression where metadata and one batch succeed before the decoder fails.
Also applies to: 331-394, 421-430
🤖 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 `@src/jobs/process-fit-file-import-job.ts` around lines 211 - 286, Change the FIT import flow around writeWeightBatch, beginActivityImport, and streamFitFile so decoder callbacks stage weight and activity metrics instead of mutating committed database rows during streaming. Only atomically replace the committed scope after streamFitFile completes validation of its exit status and message count; preserve existing data when decoding fails. Add a regression covering successful metadata and batch callbacks followed by a FitDecoderError.
🤖 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 @.github/workflows/test.yml:
- Around line 561-563: Restrict the GITHUB_TOKEN permissions for the
test-fit-decoder job by adding a job-level permissions configuration with
minimal read-only access, replacing reliance on default workflow permissions.
Apply this to the job containing the runs-on and VCPKG_ROOT settings, without
changing its existing execution behavior.
In `@docs/production-incident-baseline.md`:
- Around line 13388-13418: Update the incident claims in the documented Fix /
mitigation, Validation, and Remaining risk sections to include primary-source
citations. Link decoder behavior and protocol assertions to the relevant native
decoder README, protocol tests, regression tests, and CI artifacts; cite
authoritative Docker or incident output for disk-exhaustion and operational
measurements. Ensure every newly added implementation, test-result, and
operational claim has an appropriate repository or official-source reference,
including the additional affected sections.
In `@native/fit-decoder/AGENTS.md`:
- Around line 3-4: Update the instructions in AGENTS.md to explicitly require
reading the repository root README.md before changing the package, while
preserving the existing requirement to read the package README.md.
In `@native/fit-decoder/CMakePresets.json`:
- Around line 21-26: Update native/fit-decoder/CMakePresets.json at lines 21-26
to add a release testPresets entry with outputOnFailure enabled and the
execution directory inherited from the release configure preset. Update
.github/workflows/test.yml at line 584 and Dockerfile at line 44 to invoke the
native tests with ctest --preset release instead of explicit arguments.
In `@native/fit-decoder/README.md`:
- Around line 7-8: Update the README’s backpressure description to state that
the decoder waits for a continue acknowledgement only after metadata and batch
output messages, not after the final end message. Align the wording with the
precise contract documented around the metadata, batches, and termination
behavior.
- Around line 3-8: Add authoritative links in the README paragraphs covering
protocol limits, message ordering, acknowledgement behavior, and failure
semantics, pointing to the native implementation and protocol-verification
source files. Keep the existing claims and wording intact while making each
claim traceable to its primary source.
In `@native/fit-decoder/src/fit-decoder.cpp`:
- Around line 118-130: The numeric conversion logic should preserve exact
integer values for the raw parsed record instead of routing every field through
GetFLOAT64Value(). Update this field-value handling to detect integer-valued FIT
fields and serialize their raw values losslessly, while retaining floating-point
conversion for fields requiring FIT scaling such as semicircles; keep the
existing left_right_balance representation unchanged.
In `@packages/server/src/routes/webhooks.test.ts`:
- Around line 611-616: Update the webhook test assertions around mockStartWorker
and syncWebhookEvent to explicitly require that syncWebhookEvent was called
before comparing invocation order. Remove the Number.POSITIVE_INFINITY fallback
so the test fails when the targeted sync never runs, while preserving the
ordering assertion that mockStartWorker occurs first.
In `@packages/server/src/routes/webhooks.ts`:
- Line 162: Update the webhook event-processing loop to use workerStartRequested
as a guard before every startWorker invocation, not only the final fallback. Set
the flag when startup is requested so multi-event payloads trigger at most one
worker start, while preserving the fallback behavior when no targeted event
requests startup.
In `@src/fit/AGENTS.md`:
- Around line 13-14: Add a blank line immediately after the “Testing Strategy”
heading in AGENTS.md, before the Native protocol list item.
In `@src/fit/stream-decoder.ts`:
- Around line 235-295: Reject any protocol message received after the first end
message before the switch dispatches it, so records and weights cannot reach
their consumers after completion. Update the end handling around endMessageCount
to establish this guard and remove the now-unreachable duplicate-end check; add
a regression test covering messages after end, including batches that could
otherwise satisfy the final count check.
---
Outside diff comments:
In `@src/jobs/process-fit-file-import-job.ts`:
- Around line 211-286: Change the FIT import flow around writeWeightBatch,
beginActivityImport, and streamFitFile so decoder callbacks stage weight and
activity metrics instead of mutating committed database rows during streaming.
Only atomically replace the committed scope after streamFitFile completes
validation of its exit status and message count; preserve existing data when
decoding fails. Add a regression covering successful metadata and batch
callbacks followed by a FitDecoderError.
🪄 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: 50bc8808-ac68-4124-9e65-8b8ba95bf057
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (56)
.dockerignore.github/dependabot.yml.github/workflows/test.ymlDockerfileREADME.mddocs/production-incident-baseline.mdnative/fit-decoder/AGENTS.mdnative/fit-decoder/CLAUDE.mdnative/fit-decoder/CMakeLists.txtnative/fit-decoder/CMakePresets.jsonnative/fit-decoder/GEMINI.mdnative/fit-decoder/README.mdnative/fit-decoder/src/fit-decoder.cppnative/fit-decoder/tests/VerifyProtocol.cmakenative/fit-decoder/vcpkg-configuration.jsonnative/fit-decoder/vcpkg-ports/garmin-fit-sdk/CMakeLists.txtnative/fit-decoder/vcpkg-ports/garmin-fit-sdk/cmake/GenerateFitEnumNames.cmakenative/fit-decoder/vcpkg-ports/garmin-fit-sdk/portfile.cmakenative/fit-decoder/vcpkg-ports/garmin-fit-sdk/vcpkg.jsonnative/fit-decoder/vcpkg.jsonpackage.jsonpackages/server/src/routes/webhooks.test.tspackages/server/src/routes/webhooks.tssrc/fit/AGENTS.mdsrc/fit/README.mdsrc/fit/external-id.test.tssrc/fit/external-id.tssrc/fit/parser-edge.test.tssrc/fit/parser-worker-entry.test.tssrc/fit/parser-worker-entry.tssrc/fit/parser-worker.test.tssrc/fit/parser-worker.tssrc/fit/parser.test.tssrc/fit/parser.tssrc/fit/stream-decoder.test.tssrc/fit/stream-decoder.tssrc/fit/test-helpers.tssrc/index.test.tssrc/index.tssrc/jobs/enqueue-fit-file-import.test.tssrc/jobs/enqueue-fit-file-import.tssrc/jobs/garmin-import-progress.test.tssrc/jobs/process-fit-file-import-batch-job.tssrc/jobs/process-fit-file-import-job.test.tssrc/jobs/process-fit-file-import-job.tssrc/jobs/process-garmin-dump-import-job.tssrc/jobs/queues.test.tssrc/jobs/queues.tssrc/providers/coros.test.tssrc/providers/coros.tssrc/providers/suunto.test.tssrc/providers/suunto.tssrc/providers/types.tssrc/providers/wahoo.test.tssrc/providers/wahoo/activity-persister.tssrc/providers/wahoo/provider.ts
💤 Files with no reviewable changes (7)
- src/fit/parser-worker-entry.test.ts
- src/fit/parser-worker-entry.ts
- src/fit/parser-worker.test.ts
- src/fit/parser-edge.test.ts
- src/fit/test-helpers.ts
- src/fit/parser-worker.ts
- src/fit/parser.ts
Keep failed decoder sessions from partially replacing persisted data and let provider workers process bounded native batches without deadlocking on their own queue.
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.
Owner
Author
|
Addressed the outside-diff atomicity finding from CodeRabbit review in commit 2d31b12. The import job now spools every bounded record or weight batch to a private temporary directory while the decoder protocol runs. It performs no activity upsert, sample replacement, or metric publish until metadata, end, process exit, and the final message count all validate; only then does it replay one batch of at most 250 messages at a time. A late FitDecoderError regression proves no mutation occurs and the spool is removed. The process-fit mutation score is 88.85%; focused and full unit suites pass. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/server/src/routes/webhooks.ts (2)
196-200: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSet
workerStartRequestedbefore awaiting worker startup.If
startWorker()throws,workerStartRequestedremainsfalse, causing the event loop and the fallback block to redundantly attempt worker startup again for subsequent payloads.Proposed fix
if (provider.requiresWorkerForWebhookSync && !workerStartRequested) { + workerStartRequested = true; const { startWorker } = await import("../lib/start-worker.ts"); await startWorker(); - workerStartRequested = true; }🤖 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/routes/webhooks.ts` around lines 196 - 200, Update the worker startup branch guarded by provider.requiresWorkerForWebhookSync and !workerStartRequested to set workerStartRequested before awaiting startWorker(). Preserve the existing dynamic import and startup flow while ensuring a failed start does not leave the flag false for subsequent payloads.
238-243: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCall
captureException()for unexpected errors.Per the AGENTS.md rules for
**/*.ts, "Never silently swallow errors — every unexpected catch must callcaptureException()."Proposed fix
Ensure
captureExceptionis imported from@sentry/nodeat the top of the file, then update thecatchblock:} catch (err) { + captureException(err); logger.warn(`[webhook] Failed to start worker: ${err}`); }🤖 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/routes/webhooks.ts` around lines 238 - 243, Update the catch block surrounding startWorker in the webhook route to call captureException from `@sentry/node` for the caught unexpected error, while preserving the existing logger.warn message. Add the captureException import at the file’s imports.Source: Path instructions
src/jobs/process-fit-file-import-job.ts (1)
307-325: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep FIT paths out of weight metric identifiers. Garmin logical paths may contain account emails, so using
originalPathdirectly persists PII.
src/jobs/process-fit-file-import-job.ts#L307-L325: derive an opaque stable path hash for the external ID.src/jobs/process-fit-file-import-job.test.ts#L743-L768: use an email-bearing path and assert the generated rows contain no path or email.🤖 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 `@src/jobs/process-fit-file-import-job.ts` around lines 307 - 325, The weight external ID built in writeWeightBatch must not persist the potentially PII-bearing data.originalPath. Derive a stable opaque hash of the original path and use that hash in each externalId while preserving uniqueness with the timestamp. In src/jobs/process-fit-file-import-job.test.ts lines 743-768, update the fixture to include an email-bearing path and assert generated rows contain neither the path nor the email.
🤖 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 @.github/workflows/test.yml:
- Around line 591-594: Add explicit name fields to the artifact upload step
using fit-decoder-linux, the corresponding artifact download steps, and the
chmod steps in the referenced CI sections. Use concise names that identify the
artifact or permission stage so workflow failures clearly pinpoint the broken
step.
In `@native/fit-decoder/src/field-json.cpp`:
- Around line 19-20: Update IsFitTimestampField and its callers to require the
field’s declared FIT base type to be FIT_BASE_TYPE_UINT32 in addition to
matching timestamp names before invoking FitTimestampJson(). Preserve normal
serialization for developer fields with those names but different types,
avoiding name-only timestamp coercion.
In `@src/fit/AGENTS.md`:
- Around line 11-17: Add primary-source links to the claims in AGENTS.md: link
the raw-field data-retention statement to its implementation, the native
protocol testing statement to VerifyProtocol.cmake, and the adapter and
normalization testing statements to their referenced test files. Preserve the
existing claims and testing descriptions while adding only the appropriate
repository links.
In `@src/jobs/enqueue-fit-file-import.test.ts`:
- Around line 45-96: The in-process import test around
enqueueFitFileImportAndWait currently resolves immediately and cannot detect
premature cleanup. Make processFitFileImportJob return a deferred promise,
assert rm has not been called while that promise is pending, then resolve it and
verify the import result and cleanup occur afterward.
In `@src/jobs/enqueue-fit-file-import.ts`:
- Around line 65-69: Update the in-process import branch in
processFitFileImportJob to await processFitFileImportJob before the surrounding
finally cleanup executes. In src/jobs/enqueue-fit-file-import.ts lines 65-69,
use an awaited return; in src/jobs/enqueue-fit-file-import.test.ts lines 45-96,
replace the immediately resolved mock with a deferred promise and verify rm runs
only after that promise settles.
In `@src/jobs/process-fit-file-import-job.ts`:
- Around line 382-388: Require or validate that metricStreamPublisher supports
replaceRows before calling replaceMetricStreamBatch in the activity replacement
path, preserving the existing no-publisher behavior only when appropriate. In
src/jobs/process-fit-file-import-job.ts lines 382-388, enforce the
replacement-capable publisher contract before mutation; in
src/jobs/process-fit-file-import-job.test.ts lines 700-740, provide replaceRows
in the mock and test the capability contract instead of bypassing it.
---
Outside diff comments:
In `@packages/server/src/routes/webhooks.ts`:
- Around line 196-200: Update the worker startup branch guarded by
provider.requiresWorkerForWebhookSync and !workerStartRequested to set
workerStartRequested before awaiting startWorker(). Preserve the existing
dynamic import and startup flow while ensuring a failed start does not leave the
flag false for subsequent payloads.
- Around line 238-243: Update the catch block surrounding startWorker in the
webhook route to call captureException from `@sentry/node` for the caught
unexpected error, while preserving the existing logger.warn message. Add the
captureException import at the file’s imports.
In `@src/jobs/process-fit-file-import-job.ts`:
- Around line 307-325: The weight external ID built in writeWeightBatch must not
persist the potentially PII-bearing data.originalPath. Derive a stable opaque
hash of the original path and use that hash in each externalId while preserving
uniqueness with the timestamp. In src/jobs/process-fit-file-import-job.test.ts
lines 743-768, update the fixture to include an email-bearing path and assert
generated rows contain neither the path nor the email.
🪄 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: 65ab4921-604c-468a-9014-24b3d2590737
📒 Files selected for processing (33)
.github/workflows/test.ymlDockerfiledocs/production-incident-baseline.mdknip.jsonnative/fit-decoder/AGENTS.mdnative/fit-decoder/CMakeLists.txtnative/fit-decoder/CMakePresets.jsonnative/fit-decoder/README.mdnative/fit-decoder/src/field-json.cppnative/fit-decoder/src/field-json.hppnative/fit-decoder/src/fit-decoder.cppnative/fit-decoder/tests/FieldJsonTest.cpppackages/server/src/lib/start-worker.test.tspackages/server/src/lib/start-worker.tspackages/server/src/mcp/tools.tspackages/server/src/routers/sync.tspackages/server/src/routes/upload.tspackages/server/src/routes/webhooks.test.tspackages/server/src/routes/webhooks.tssrc/fit/AGENTS.mdsrc/fit/stream-decoder.test.tssrc/fit/stream-decoder.tssrc/jobs/enqueue-fit-file-import.test.tssrc/jobs/enqueue-fit-file-import.tssrc/jobs/process-fit-file-import-job.test.tssrc/jobs/process-fit-file-import-job.tssrc/providers/coros.test.tssrc/providers/coros.tssrc/providers/suunto.test.tssrc/providers/suunto.tssrc/providers/wahoo.test.tssrc/providers/wahoo/activity-persister.tssrc/providers/wahoo/provider.ts
Await in-process decoding before staged-file cleanup. Validate replacement publishers before mutation and close the remaining native, webhook, privacy, and CI review gaps.
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.
Owner
Author
|
Addressed all three outside-diff findings from CodeRabbit review.
Commit: e7f309e |
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.
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 (2)
packages/server/src/routes/webhooks.ts (1)
163-163: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA failed startup attempt is incorrectly treated as a running worker.
packages/server/src/routes/webhooks.ts#L163-L163: replace the boolean with a cached startup Promise.packages/server/src/routes/webhooks.ts#L197-L203: await that Promise before every worker-dependent targeted sync.packages/server/src/routes/webhooks.ts#L239-L246: use the Promise’s presence to avoid a second startup attempt.packages/server/src/routes/webhooks.test.ts#L713-L743: assert worker-dependentsyncWebhookEventis not called after startup rejects.🤖 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/routes/webhooks.ts` at line 163, Replace workerStartRequested with a cached startup Promise in packages/server/src/routes/webhooks.ts:163, await it before each worker-dependent targeted sync at packages/server/src/routes/webhooks.ts:197-203, and use its presence to prevent duplicate startup attempts at packages/server/src/routes/webhooks.ts:239-246. Update packages/server/src/routes/webhooks.test.ts:713-743 to assert syncWebhookEvent is not called when startup rejects.Sources: Coding guidelines, Path instructions
packages/server/src/routes/upload.ts (1)
108-129: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWorker-start failure leaves a queued import referencing a deleted file.
packages/server/src/routes/upload.ts#L108-L129: start the worker before adding the job, or safely remove the job before callers clean up its staged file.packages/server/src/routes/upload.test.ts#L418-L428: assertqueue.addis not called when startup fails.🤖 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/routes/upload.ts` around lines 108 - 129, The enqueueImport flow in packages/server/src/routes/upload.ts lines 108-129 must start the import worker before calling importQueue.add, so startup failures prevent a queued job from referencing a file that callers clean up; preserve the existing job payload and returned ID behavior after successful startup. In packages/server/src/routes/upload.test.ts lines 418-428, add coverage asserting queue.add is not called when startImportWorker fails.Source: Coding guidelines
🤖 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/server/src/routes/upload.test.ts`:
- Around line 418-428: Extend the “returns 500 when the import worker fails to
start” test to mock or access the upload queue and assert its add method was not
called after startWorker rejects. Keep the existing 500 response and startWorker
invocation assertions, ensuring the test reproduces the orphaned-job scenario.
In `@packages/server/src/routes/webhooks.test.ts`:
- Around line 713-743: Update the test “does not retry failed worker startup for
later targeted events” to retain a reference to the provider’s syncWebhookEvent
mock and assert it was never called after mockStartWorker rejects. Ensure the
test fails if either targeted event proceeds into worker-dependent
synchronization.
In `@src/providers/wahoo/provider.ts`:
- Line 294: Update the argument reference in the Wahoo sync flow around
options.metricStreamPublisher to safely access options when it is undefined,
preserving an undefined publisher value so normal workout processing continues
without crashing.
---
Outside diff comments:
In `@packages/server/src/routes/upload.ts`:
- Around line 108-129: The enqueueImport flow in
packages/server/src/routes/upload.ts lines 108-129 must start the import worker
before calling importQueue.add, so startup failures prevent a queued job from
referencing a file that callers clean up; preserve the existing job payload and
returned ID behavior after successful startup. In
packages/server/src/routes/upload.test.ts lines 418-428, add coverage asserting
queue.add is not called when startImportWorker fails.
In `@packages/server/src/routes/webhooks.ts`:
- Line 163: Replace workerStartRequested with a cached startup Promise in
packages/server/src/routes/webhooks.ts:163, await it before each
worker-dependent targeted sync at
packages/server/src/routes/webhooks.ts:197-203, and use its presence to prevent
duplicate startup attempts at packages/server/src/routes/webhooks.ts:239-246.
Update packages/server/src/routes/webhooks.test.ts:713-743 to assert
syncWebhookEvent is not called when startup rejects.
🪄 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: 397d9364-9f28-47b6-9cbd-ddc0086f801f
📒 Files selected for processing (24)
.github/workflows/test.ymlnative/fit-decoder/src/field-json.cppnative/fit-decoder/tests/FieldJsonTest.cpppackages/server/src/index.integration.test.tspackages/server/src/index.test.tspackages/server/src/index.tspackages/server/src/router.test.tspackages/server/src/router.tspackages/server/src/routers/sync.test.tspackages/server/src/routers/sync.tspackages/server/src/routes/upload.integration.test.tspackages/server/src/routes/upload.test.tspackages/server/src/routes/upload.tspackages/server/src/routes/webhooks.test.tspackages/server/src/routes/webhooks.tspackages/server/src/upload-auth.integration.test.tssrc/fit/AGENTS.mdsrc/jobs/enqueue-fit-file-import.test.tssrc/jobs/enqueue-fit-file-import.tssrc/jobs/process-fit-file-import-job.test.tssrc/jobs/process-fit-file-import-job.tssrc/jobs/queues.test.tssrc/providers/wahoo/provider.test.tssrc/providers/wahoo/provider.ts
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.
Owner
Author
|
Addressed the two outside-diff findings from #1646 (review) in 73eb1130e.
Validation: 137 route tests passed, the focused async-ordering mutation run killed all 17 mutants, lint passed, and all 26 typed workspace packages passed. |
Asherlc
changed the title
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
6 similar comments
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
|
🤖 Review skipped: Rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours. Upgrade to a paid plan for unlimited reviews. |
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
Summary by cubic
Replaced whole‑file JS FIT parsing with a native C++ streaming decoder to bound memory and backpressure large imports. Also gated sync/upload/webhook work behind worker startup and tightened the import lifecycle to finish in‑process decoding before cleanup.
New Features
native/fit-decoder(CMake +vcpkg, Garmin FIT C++ SDK) emitting NDJSON with per‑batch acks (max 250 messages / 512 KiB); CI job, Docker stage, and abuild:fit-decoderscript.fit-file-importworkflow viaenqueueFitFileImportAndWaitwith retries, progress, and aggregation.requiresWorkerForWebhookSync); FIT external IDs can fall back to a streaming SHA‑256 without buffering files.Bug Fixes
start-workernow resolves only after Docker confirms the worker is running.Written for commit 73eb113. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation