Step-chain sync dedup + per-user rate limits - #1360
Merged
Merged
Conversation
Add sync request dedup for Garmin/WHOOP step-chain sync: - Create SyncApiQuery type + stable key for comparing API requests - Add resolver plugin system for provider-specific query mapping - Dedup at enqueue time: check for pending jobs before adding - Scan pending jobs when planning steps to skip already-queued API calls - Switch Garmin/WHOOP rate limit tracking from provider-wide to per-user scope - Remove skipRemainingAfterRateLimit from WHOOP checkpoint (obsoleted by dedup) - Make Garmin applyRateLimitToCheckpoint a no-op (obsoleted by dedup) Code quality fixes: - Extract duplicated resolveScopedUserId to src/lib/user-context.ts - Replace side-effect resolver registration with explicit function call - Register resolvers alongside providers using provider.id - Add dofek/jobs/sync-request-query-registration to package.json exports - Fix misleading docstring in garmin/sync-checkpoint.ts - Add collision-risk comment on SHA-256 job ID truncation - Add TODO comment about queue scanning performance
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 sync request query resolution, request deduplication, provider resolver wiring, Garmin and Whoop request planning, scoped rate-limit metadata, and a debounced web hook export. ChangesSync Request Deduplication and Scoped Rate Limits
Web debounce cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 |
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
…message mismatch - Change buildSyncRequestJobId separator from ':' to '-' (BullMQ rejects colons) - Add createProviderRateLimitFetch wrapping in Garmin authSetup().automatedLogin() - Update test expectations for shared resolveScopedUserId error message
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/whoop/sync-orchestrator.ts (1)
458-463: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReport unexpected sync failures to Sentry before returning.
This branch still turns non-rate-limit exceptions into
SyncResult.errorswithoutcaptureException(err), so provider failures disappear from monitoring even though the sync continues. As per coding guidelines, "Always report errors to Sentry" and path instructions, "Never silently swallow errors — every unexpected catch must callcaptureException()."🤖 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/providers/whoop/sync-orchestrator.ts` around lines 458 - 463, In the non-rate-limit error path in sync-orchestrator’s unexpected exception handling, the error is being added to SyncResult.errors without being reported to Sentry first. Update the branch in the sync flow around the isWhoopRateLimitError check to call captureException(err) before pushing to errors and returning, so provider failures are always surfaced to monitoring. Use the existing err handling in the catch block and keep the rate-limit checkpoint behavior unchanged.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 `@src/jobs/sync-request-job.test.ts`:
- Around line 1-72: The current tests cover request resolution and job ID
stability, but they miss the dedup fall-through handled by
enqueueSyncJobWithRequestDedup and the finished-job branch in
sync-request-job.ts. Add a test that mocks getJob returning a completed or
failed job and assert the re-enqueue path is taken, so the behavior of
enqueueSyncJobWithRequestDedup is pinned down. Use the existing sync-request-job
test suite and the enqueueSyncJobWithRequestDedup/getJob symbols to place the
new coverage alongside the current resolveWhoopSyncRequestQuery and
buildSyncRequestJobId tests.
In `@src/jobs/sync-request-job.ts`:
- Around line 17-28: The sync request job flow in syncRequestJob should not
reuse a BullMQ jobId that still exists in a completed or failed state, since
that can prevent addJob from enqueueing a new sync. In the existing getJob/
existing.getState duplicate-check path, remove the finished job before returning
to the fallback, or generate a fresh jobId when the state is in
DUPLICATE_REQUEST_JOB_STATES, so queue.add via addJob can create the new job.
In `@src/jobs/sync-request-query-registration.ts`:
- Around line 5-20: Add a colocated unit test for
registerProviderSyncRequestResolver to lock down the provider-id dispatch
behavior. Cover the supported branches in registerProviderSyncRequestResolver by
verifying that provider.id values for "garmin" and "whoop" load the correct
resolver modules and call registerSyncRequestQueryResolver with the expected
resolver, and add an unsupported-provider case that confirms no registration
happens. Mock the dynamic imports and the registerSyncRequestQueryResolver
dependency so the test is isolated and clearly exercises the wiring in
sync-request-query-registration.ts.
In `@src/lib/user-context.ts`:
- Around line 3-8: Add a colocated unit test for resolveScopedUserId() in
src/lib/user-context.test.ts to cover all three behaviors: returning the
explicit userId argument, falling back to getTokenUserId() when no argument is
provided, and throwing when neither source yields a value. Use the
resolveScopedUserId and getTokenUserId symbols to locate the helper, and mock
the token-context dependency so the fallback and missing-user paths are isolated
and deterministic.
In `@src/providers/garmin/provider.ts`:
- Around line 113-116: The token refresh path in Garmin provider is bypassing
the scoped rate-limit wrapper because `GarminConnectClient.fromTokens()` is
still being called with `this.#baseFetchFn` instead of the wrapped fetch used by
`sync()`. Update `#resolveTokens(..., fetchFn)` and the `fromTokens()` call in
`GarminProvider` so refresh requests use the same scoped `fetchFn` that tracks
adaptive 429s and can surface `GarminRateLimitError`. Keep the change consistent
across the token-resolution flow so user-scoped throttling applies to both
initial sync and token refresh.
In `@src/providers/garmin/sync-request-query.ts`:
- Around line 8-13: The sync-request resolution in sync-request-query should not
map a checkpoint with phase "done" to the same connectapi/activities request as
a fresh job, since that can collide with the no-checkpoint key. Update the logic
in the request builder to return null when checkpoint.phase === "done", while
keeping the existing fresh-job behavior for missing checkpoints. Use the
checkpoint handling branch in the query resolver to distinguish completed
step-chain jobs from new syncs.
In `@src/providers/garmin/sync-step-plan.test.ts`:
- Around line 318-320: The test is hardcoding the serialized pending-query key,
which duplicates the private format used by syncApiQueryKey() and makes the
assertion brittle. Update the pending set construction in sync-step-plan.test.ts
to derive the expected key from garminSyncStepToApiQuery() and syncApiQueryKey()
instead of embedding the literal string, so the test stays aligned with the
planner’s query-key serialization.
In `@src/providers/whoop/sync-step-plan.test.ts`:
- Around line 255-258: The pending sync key fixture in the sync-step-plan test
is hardcoded to a serialized query string, which tightly couples the test to the
current JSON ordering. Update the test to build the expected key using
syncApiQueryKey() so it matches the same contract used by the planner and stays
resilient to serialization changes. Use the shared key builder in the
pendingQueryMocks.listPendingSyncRequestQueryKeys setup instead of the literal
string.
---
Outside diff comments:
In `@src/providers/whoop/sync-orchestrator.ts`:
- Around line 458-463: In the non-rate-limit error path in sync-orchestrator’s
unexpected exception handling, the error is being added to SyncResult.errors
without being reported to Sentry first. Update the branch in the sync flow
around the isWhoopRateLimitError check to call captureException(err) before
pushing to errors and returning, so provider failures are always surfaced to
monitoring. Use the existing err handling in the catch block and keep the
rate-limit checkpoint behavior unchanged.
🪄 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: d9f7dc72-6fb4-49d2-ad5e-5e42ab62fd25
📒 Files selected for processing (36)
package.jsonpackages/garmin-connect/src/client.tspackages/server/src/routers/sync-helpers.tspackages/whoop-whoop/src/client.tssrc/jobs/enqueue-sync-job.test.tssrc/jobs/enqueue-sync-job.tssrc/jobs/process-scheduled-sync-job.test.tssrc/jobs/process-sync-job.test.tssrc/jobs/provider-registration.tssrc/jobs/sync-request-job.test.tssrc/jobs/sync-request-job.tssrc/jobs/sync-request-query-registration.tssrc/lib/sync-api-query.test.tssrc/lib/sync-api-query.tssrc/lib/sync-request-query.test.tssrc/lib/sync-request-query.tssrc/lib/sync-request-queue.tssrc/lib/user-context.tssrc/providers/garmin.test.tssrc/providers/garmin/provider.tssrc/providers/garmin/sync-api-query.tssrc/providers/garmin/sync-checkpoint.test.tssrc/providers/garmin/sync-checkpoint.tssrc/providers/garmin/sync-request-query.tssrc/providers/garmin/sync-step-plan.test.tssrc/providers/garmin/sync-step-plan.tssrc/providers/whoop.test.tssrc/providers/whoop/provider.tssrc/providers/whoop/sync-api-query.test.tssrc/providers/whoop/sync-api-query.tssrc/providers/whoop/sync-checkpoint.test.tssrc/providers/whoop/sync-checkpoint.tssrc/providers/whoop/sync-orchestrator.tssrc/providers/whoop/sync-request-query.tssrc/providers/whoop/sync-step-plan.test.tssrc/providers/whoop/sync-step-plan.ts
💤 Files with no reviewable changes (2)
- src/providers/whoop/sync-checkpoint.test.ts
- src/providers/whoop/sync-checkpoint.ts
- Remove completed/failed jobs before re-adding with same jobId
- Pass rate-limit fetchFn through to Garmin token refresh
- Add dedup, resolver-dispatch, and resolveScopedUserId tests
- Return null for done checkpoint in Garmin query resolver
- Use computed syncApiQueryKey in Garmin and WHOOP step-plan tests
- Add captureException for non-rate-limit errors in WHOOP orchestrator
- Narrow registerProviderSyncRequestResolver param to { id: string }
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jobs/sync-request-job.ts (1)
17-25: 🎯 Functional Correctness | 🟠 MajorTreat any nonterminal BullMQ state as a dedup hit.
DUPLICATE_REQUEST_JOB_STATESmisses valid pending states likewaiting-children, so an existing job can still be removed and re-added. AGENTS.md says to fix the root cause and avoid workarounds; keep the existing job unlessgetState()returnscompletedorfailed.🤖 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/sync-request-job.ts` around lines 17 - 25, The duplicate-request dedupe logic in sync-request-job should treat every nonterminal BullMQ state as a cache hit instead of relying on DUPLICATE_REQUEST_JOB_STATES. Update the existing-job check in sync-request-job.ts so that when getJob(nextOptions.jobId) finds a job and existing.getState() returns anything other than completed or failed, the function returns the existing job and does not remove/recreate it. Keep the change localized around the job-state handling in the sync request job flow, and preserve removal only for truly terminal jobs.src/providers/garmin/sync-step-plan.ts (1)
183-193: 🩺 Stability & Availability | 🟠 MajorExclude the current sync job from pending request keys —
listPendingSyncRequestQueryKeys("garmin", userId)includes the job being processed, so a fresh sync can treatconnectapi/activities?start=0as pending and skip the initialactivities_liststep. Pass the current job id through the lookup and filter it out at the source.🤖 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/providers/garmin/sync-step-plan.ts` around lines 183 - 193, The pending-request lookup is currently including the sync job that is actively running, which can cause `activities_list` to be skipped for a fresh Garmin sync. Update `buildGarminSyncStepPlan` to pass the current job identifier into `listPendingSyncRequestQueryKeys`, and filter that job out inside the pending-keys query so `planSyncStepIfRequestNotPending` only sees other jobs’ keys.
🤖 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.
Outside diff comments:
In `@src/jobs/sync-request-job.ts`:
- Around line 17-25: The duplicate-request dedupe logic in sync-request-job
should treat every nonterminal BullMQ state as a cache hit instead of relying on
DUPLICATE_REQUEST_JOB_STATES. Update the existing-job check in
sync-request-job.ts so that when getJob(nextOptions.jobId) finds a job and
existing.getState() returns anything other than completed or failed, the
function returns the existing job and does not remove/recreate it. Keep the
change localized around the job-state handling in the sync request job flow, and
preserve removal only for truly terminal jobs.
In `@src/providers/garmin/sync-step-plan.ts`:
- Around line 183-193: The pending-request lookup is currently including the
sync job that is actively running, which can cause `activities_list` to be
skipped for a fresh Garmin sync. Update `buildGarminSyncStepPlan` to pass the
current job identifier into `listPendingSyncRequestQueryKeys`, and filter that
job out inside the pending-keys query so `planSyncStepIfRequestNotPending` only
sees other jobs’ keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a30c2204-2496-40c4-babc-f9f9a4078a50
📒 Files selected for processing (15)
cspell.jsonpackages/garmin-connect/src/client.tssrc/jobs/sync-request-job.test.tssrc/jobs/sync-request-job.tssrc/jobs/sync-request-query-registration.test.tssrc/jobs/sync-request-query-registration.tssrc/lib/user-context.test.tssrc/providers/garmin.test.tssrc/providers/garmin/provider.tssrc/providers/garmin/sync-request-query.tssrc/providers/garmin/sync-step-plan.test.tssrc/providers/garmin/sync-step-plan.tssrc/providers/whoop/provider.tssrc/providers/whoop/sync-orchestrator.tssrc/providers/whoop/sync-step-plan.test.ts
…dis connection errors The step-chain sync dedup code introduced calls to listPendingSyncRequestQueryKeys (sync-request-queue.ts) which creates a BullMQ Queue backed by ioredis. Three test files (garmin.test.ts, whoop.test.ts, sync-checkpoint.test.ts) imported step-plan/orchestrator modules that transitively trigger this, but lacked the mock — causing all 180 of their unit tests to fail with 'Connection is closed' from ioredis.
…Stryker mutants Add 4 test cases covering the surviving conditional-expression and object-literal mutants in whoop/sync-request-query.ts: - bootstrap phase with cursorMs - bootstrap phase with null cursor (returns null) - api phase with out-of-bounds step index (returns null) - done phase with valid api steps (returns null, not continuing) These cover every branch in the function, raising the mutation score from 61.54% to well above the 75% break threshold.
The enqueueSyncJobWithRequestDedup function (introduced by the sync dedup PR) calls getJob on the queue before addJob. The mock only provided add, so getJob was undefined and calling it threw, preventing addJob (and thus queueAdd) from ever being reached.
…error classes Shard 9: Add resolveGarminSyncRequestQuery test covering all 4 branches (no checkpoint, phase=done, step past end, valid step) — kills 15 surviving mutants in garmin/sync-request-query.ts. Shard 15: Add scope/userId assertions to GarminRateLimitError and WhoopRateLimitError tests — kills 8 surviving mutants (EqualityOperator, ConditionalExpression, LogicalOperator) in both error constructors.
…uncedValue The useDebouncedValue hook used window.setTimeout. After JSDOM teardown in tests, the timer callback fired, called setDebouncedValue, which triggered React 19 dispatchSetState -> resolveUpdatePriority. React 19 client dev mode accesses window there, causing ReferenceError. Fix: add useRef mount guard so setDebouncedValue is never called after unmount, and use setTimeout/clearTimeout without window. prefix.
Both shards had survivors where createProviderRateLimitFetch options
object was mutated to {} by Stryker (ObjectLiteral + ArrowFunction).
No test verified the options were passed through, so empty options
didn't change behavior in any existing test.
Fix: mock createProviderRateLimitFetch with a spy that wraps the
original, then assert options.scope, options.userId, and
options.createRateLimitError were populated. This kills the ObjectLiteral
and ArrowFunction mutants, raising total score to 75%.
Typecheck failed because vi.mock factory returning vi.fn() doesn't change the module's TS type at import sites, and as assertions are banned by Biome plugin. Fix: use vi.mocked() helper + toHaveBeenCalledWith with expect.objectContaining instead of manually filtering mock.calls. This avoids any type-narrowing issues and the banned as casts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/web/src/pages/ProviderDetailPage.tsx`:
- Around line 390-402: Add a colocated regression test for the debounce hook
behavior in ProviderDetailPage by exercising the unmount path with fake timers
and confirming no delayed state update occurs after cleanup. Use the same
hook/component area around the useEffect debounce logic and mountedRef guard,
and create the test as ProviderDetailPage.test.tsx so it verifies the timer is
cleared and setDebouncedValue is not called after unmount.
🪄 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: dccdab70-3826-468e-9b62-30ed49438a86
📒 Files selected for processing (9)
packages/garmin-connect/src/client.test.tspackages/server/src/mcp/route.test.tspackages/web/src/pages/ProviderDetailPage.tsxpackages/whoop-whoop/src/client.test.tssrc/jobs/sync-request-job.test.tssrc/providers/garmin.test.tssrc/providers/garmin/sync-checkpoint.test.tssrc/providers/garmin/sync-request-query.test.tssrc/providers/whoop.test.ts
Shard 6 (garmin/provider.ts ArrowFunction at line 132): createRateLimitError callback survived when mutated to () => undefined because expect.any(Function) matches any function. Fix: invoke the callback from the mock's recorded args and assert it returns a GarminRateLimitError. Also kills the OptionalChaining no-cov at line 136. Shard 5 (sync-request-queue.ts, 3 survived + 7 no cov): listProviderSyncJobsForUser had 3 survivors (ArrowFunction, ArrayDeclaration, MethodExpression) — no test exercised the function. Added colocated unit tests with mocked BullMQ queue that verify: - getJobs called with correct pending states - filter returns only matching user IDs - empty results for no-match or no-job cases. listPendingSyncRequestQueryKeys tests cover the for-loop body, if (query) branches, and key collection (killing 6 no-cov mutants).
Same pattern as the garmin fix — invoke the createRateLimitError callback from recorded mock args and assert it returns a WhoopRateLimitError instead of just checking expect.any(Function).
Export useDebouncedValue hook and add colocated tests for: - Initial value returned immediately - Value updates after delay - Timeout cleared on unmount (no stale state update)
Add colocated unit tests for whoopSyncStepToApiQuery and garminSyncStepToApiQuery exercising every switch case with exact return value assertions. Shard 1 (whoop): 47.62% -> 100% Shard 8 (garmin): 47.83% -> 100%
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/web/src/pages/ProviderDetailPage.tsx (1)
388-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mountedRefguard is dead code —clearTimeoutalready prevents the stale update.The cleanup runs
clearTimeout(timer)on both unmount and[value, delayMs]changes, so the timer callback only fires while the effect is active — i.e.mountedRef.currentis alwaystruewhen it runs. Theif (mountedRef.current)false branch is unreachable, including in the "no update after unmount" test (which passes purely because ofclearTimeout). Drop the ref and keep the cleanup.♻️ Proposed simplification
export function useDebouncedValue<T>(value: T, delayMs: number): T { const [debouncedValue, setDebouncedValue] = useState(value); - const mountedRef = useRef(true); useEffect(() => { - mountedRef.current = true; - const timer = setTimeout(() => { - if (mountedRef.current) { - setDebouncedValue(value); - } - }, delayMs); - return () => { - mountedRef.current = false; - clearTimeout(timer); - }; + const timer = setTimeout(() => setDebouncedValue(value), delayMs); + return () => clearTimeout(timer); }, [value, delayMs]); return debouncedValue; }As per coding guidelines: "Apply minimum fix: only perform the minimum fix required... do not add extra error handling, validation, or infrastructure unless explicitly requested."
🤖 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/web/src/pages/ProviderDetailPage.tsx` around lines 388 - 406, The `useDebouncedValue` hook in `ProviderDetailPage` has dead `mountedRef` logic because the `clearTimeout` cleanup already prevents stale updates. Remove the `mountedRef` ref and the `if (mountedRef.current)` guard, and keep the effect focused on scheduling the timeout and clearing it in the cleanup.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/web/src/pages/ProviderDetailPage.tsx`:
- Line 388: The exported useDebouncedValue hook is only present for the test and
is not part of the real public API, so remove the export from ProviderDetailPage
and validate the behavior through the existing public surface (such as
useDebouncedFilters/SyncHistory interactions) instead. If useDebouncedValue is
intended to be reusable, move it out of ProviderDetailPage into a shared hooks
module with a real production consumer, and keep the export there for that
reason.
In `@src/lib/sync-request-queue.test.ts`:
- Around line 76-82: The mock for sync request query resolution is using an
empty string to stand in for a missing sinceIso value, which hides the
absent-value path. Update the mock in sync-request-queue.test.ts around
mockResolveSyncRequestQuery and mockSyncApiQueryKey to preserve sinceIso as
undefined or omit the field when it is missing, and keep the key builder
handling the undefined case explicitly instead of coercing it to an empty
string.
- Around line 69-89: Add a regression test in listPendingSyncRequestQueryKeys to
cover duplicate query keys: mock getJobs so two jobs resolve through
resolveSyncRequestQuery to the same query, then verify syncApiQueryKey produces
the same string and the returned Set contains only one entry. Use the existing
listPendingSyncRequestQueryKeys, mockResolveSyncRequestQuery, and
mockSyncApiQueryKey helpers so the test locks in the dedup behavior.
In `@src/providers/garmin/sync-api-query.test.ts`:
- Around line 5-15: The Garmin request key for activities_list is currently
collapsing all pages because the query builder hardcodes the start filter to 0.
Update garminSyncStepToApiQuery in sync-api-query.ts to use step.offset for the
start value so syncApiQueryKey() can distinguish follow-up pages queued by
sync-orchestrator.ts. Keep the activities_list path the same, but ensure the
generated filters reflect the step offset.
---
Outside diff comments:
In `@packages/web/src/pages/ProviderDetailPage.tsx`:
- Around line 388-406: The `useDebouncedValue` hook in `ProviderDetailPage` has
dead `mountedRef` logic because the `clearTimeout` cleanup already prevents
stale updates. Remove the `mountedRef` ref and the `if (mountedRef.current)`
guard, and keep the effect focused on scheduling the timeout and clearing it in
the cleanup.
🪄 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: fe3c7c57-ccc5-468f-afb2-8eab6dfe9e1d
📒 Files selected for processing (7)
packages/web/src/pages/ProviderDetailPage.test.tsxpackages/web/src/pages/ProviderDetailPage.tsxsrc/lib/sync-request-queue.test.tssrc/providers/garmin.test.tssrc/providers/garmin/sync-api-query.test.tssrc/providers/whoop.test.tssrc/providers/whoop/sync-api-query.test.ts
- Extract useDebouncedValue to hooks/ dir, remove test-only export - Remove dead mountedRef from useDebouncedValue (clearTimeout suffices) - Remove empty-string sentinel in sync-request-queue.test.ts mock - Add dedup regression test for listPendingSyncRequestQueryKeys - Propagate step.offset in garminSyncStepToApiQuery (activities_list)
CI typecheck failed because vitest globals aren't configured in packages/web tsconfig — must import test lifecycle hooks explicitly.
This was referenced Jun 25, 2026
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
1 participant
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
Add step-chain sync job dedup for Garmin/WHOOP and switch rate-limit tracking from provider-wide to per-user scope.
Core changes
SyncApiQuerytype +syncApiQueryKey()— stable key for comparing provider API requests across queued jobs (sorted filter keys enforces determinism)registerSyncRequestQueryResolver) — providers register a function that maps job data → API query for dedup keyingenqueueSyncJobWithRequestDedup) — checks for pending jobs byprovider:user:queryKeyjob ID before addingplanSyncStepIfRequestNotPending) —listPendingSyncRequestQueryKeys()scans active/waiting/delayed jobs and skips steps already queuedcreateProviderRateLimitFetchfrom constructor (provider scope) tosync()call (user scope withuserId)skipRemainingAfterRateLimitfrom WHOOP checkpoint; GarminapplyRateLimitToCheckpointis now a no-op — both obsoleted by the dedup approachCode quality fixes
resolveScopedUserId→src/lib/user-context.tsregisterProviderSyncRequestResolver()called alongside provider registration usingprovider.iddofek/jobs/sync-request-query-registrationto rootpackage.jsonexportsgarmin/sync-checkpoint.tsSummary by cubic
Adds request-level dedup for Garmin and WHOOP step-chain syncs and switches rate limiting to per-user scope to cut duplicate API calls and avoid cross-user throttling. Also extracts a reusable debounce hook with tests to prevent stale updates after unmount.
New Features
SyncApiQuerykeys and job IDs; enqueue-time dedup checksgetJob(), reuses pending jobs, and removes completed/failed before re-adding.listPendingSyncRequestQueryKeys().dofek/jobs/sync-request-query-registration.useDebouncedValuetopackages/web/src/hookswith tests.Refactors
scopeanduserId.activities_listdedup keys byoffset.skipRemainingAfterRateLimit; non-rate-limit errors are captured.resolveScopedUserId; BullMQ job IDs use-../jobs/sync-request-query-registrationto package exports.Written for commit 7e97f3d. Summary will update on new commits.
Summary by CodeRabbit
./jobs/sync-request-query-registration.