refactor(uploads): remove statusCallback in favor of maxStatusChecks#188
Merged
wesleyschlenker merged 1 commit intomainfrom Feb 26, 2026
Merged
refactor(uploads): remove statusCallback in favor of maxStatusChecks#188wesleyschlenker merged 1 commit intomainfrom
wesleyschlenker merged 1 commit intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces callback-based upload status polling with promise-based behavior: Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant post as post()
participant _check as _check()
participant API as Upload API
Client->>post: post(uploadData, { maxStatusChecks: N })
post->>API: POST /uploads
API-->>post: initial Upload response
alt maxStatusChecks > 0
post->>_check: _check({ uploadId, maxStatusChecks: N })
loop Poll until done or maxChecks reached
_check->>API: GET /uploads/:id/status
API-->>_check: status response
_check->>_check: evaluate status (done? / continue)
end
_check-->>post: final Upload or throw timeout
end
post-->>Client: resolve/reject promise with Upload result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/uploads.js`:
- Around line 19-20: The JSDoc for maxStatusChecks is ambiguous about its
default: update the comment for maxStatusChecks near the post() API to
explicitly state that the function returns immediately when maxStatusChecks is
omitted or set to 0, and only uses the 300-attempts (~5 min at 1s) behavior when
a positive number is provided (e.g., "If > 0, poll until upload is done or up to
the provided number of attempts (commonly 300 ≈ 5 min at 1s). If omitted or 0,
post() returns immediately after posting."). Mention both the immediate-return
default and the positive-number polling behavior and reference maxStatusChecks
and post() in the JSDoc so callers aren’t misled.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mdindex.d.tslib/uploads.jstest/uploads.js
💤 Files with no reviewable changes (1)
- index.d.ts
wesleyschlenker
force-pushed
the
deps-and-eslint
branch
from
a7a0eff to
f8464b3
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/uploads.js (1)
19-20: JSDoc formaxStatusChecksnow clearly states both behaviors — past concern addressed.The updated wording at line 19 explicitly states the immediate-return default and the positive-number polling behavior, resolving the ambiguity flagged in the prior review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/uploads.js` around lines 19 - 20, The review comment is a duplicate noting that the JSDoc for maxStatusChecks now clarifies behavior; no code change required—remove the duplicated review comment or mark it "resolved" to avoid noise in the PR; reference the JSDoc entry for maxStatusChecks (and adjacent formData) in lib/uploads.js when confirming resolution.
🧹 Nitpick comments (3)
lib/uploads.js (1)
57-57:var self = thisis unnecessary inasyncmethods.Both
post()(line 57) and_check()(line 99) declarevar self = this, but since both areasyncfunctions,thisis lexically stable — no callback escapes the async context that would rebind it.♻️ Proposed cleanup
In
post():uploads.prototype.post = async function (args) { - var self = this // ... - return await self._check(checkArgs) + return this._check(checkArgs) }In
_check():uploads.prototype._check = async function (args) { var endpoint = 'uploads/' + args.id - var self = this // ... - if (self._uploadIsDone(response)) { + if (this._uploadIsDone(response)) {Also applies to: 97-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/uploads.js` at line 57, Remove the redundant "var self = this" declarations in the async methods post() and _check() and update any references that use self to use this directly (since async functions preserve lexical this); specifically edit the functions post() and _check() to delete the var self = this line and replace occurrences of self.something with this.something so behavior remains identical.test/uploads.js (2)
71-84: Real 1-second sleep makes this test slow and potentially brittle on constrained CI.
_checkinlib/uploads.jshas a hardcodedawait setTimeout(1000)between retries, so this test always takes ≥ 1 second. With mocha's default 2-second timeout, there's a narrow margin for CI overhead. Consider either:
- Making the inter-poll delay injectable in
_check(e.g. viaargs.delayMs, defaulting to 1000) so tests can passdelayMs: 0, or- Using fake timers (e.g.
sinon.useFakeTimers) to tick through the delay without real wall-clock time.♻️ Sketch of an injectable-delay approach in
lib/uploads.js/** * `@typedef` {Object} CheckArgs * `@property` {string} id * `@property` {string | undefined} [access_token] * `@property` {number} [maxChecks] + * `@property` {number} [delayMs] - Polling interval in ms. Defaults to 1000. */ // In _check(): - await setTimeout(1000) + await setTimeout(args.delayMs != null ? args.delayMs : 1000)Then in the test:
maxStatusChecks: 2, + delayMs: 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uploads.js` around lines 71 - 84, The test is slowed by a hardcoded 1s sleep inside _check in lib/uploads.js; modify _check to accept an optional delay parameter (e.g. args.delayMs) that defaults to 1000 and use that instead of the fixed setTimeout(1000), and propagate this option through the uploads.post call (respecting existing maxStatusChecks) so tests can call uploads.post(..., { delayMs: 0 }) to eliminate the real sleep; alternatively, add support to uploads.post to accept delayMs and pass it down to _check so existing callers are unaffected.
20-85: No test coverage for the timeout path.When
maxStatusChecksis exhausted before the upload completes,_checkthrows'Upload status check timed out after N attempts...'. Consider adding a test that mocks only "still processing" responses and asserts the returned promise rejects with that error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uploads.js` around lines 20 - 85, Add a new test for the timeout path where strava.uploads.post exhausts maxStatusChecks and rejects: mock the initial POST to /api/v3/uploads returning id_str and then mock the subsequent GET /api/v3/uploads/:id to always return status "Your activity is still being processed." for N times, call strava.uploads.post with maxStatusChecks set to N (or lower) and assert the returned promise rejects with the exact timeout message thrown by _check (e.g., "Upload status check timed out after N attempts..."); reference strava.uploads.post and the internal _check timeout behavior when creating the test so it specifically exercises the maxStatusChecks timeout branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/uploads.js`:
- Around line 19-20: The review comment is a duplicate noting that the JSDoc for
maxStatusChecks now clarifies behavior; no code change required—remove the
duplicated review comment or mark it "resolved" to avoid noise in the PR;
reference the JSDoc entry for maxStatusChecks (and adjacent formData) in
lib/uploads.js when confirming resolution.
---
Nitpick comments:
In `@lib/uploads.js`:
- Line 57: Remove the redundant "var self = this" declarations in the async
methods post() and _check() and update any references that use self to use this
directly (since async functions preserve lexical this); specifically edit the
functions post() and _check() to delete the var self = this line and replace
occurrences of self.something with this.something so behavior remains identical.
In `@test/uploads.js`:
- Around line 71-84: The test is slowed by a hardcoded 1s sleep inside _check in
lib/uploads.js; modify _check to accept an optional delay parameter (e.g.
args.delayMs) that defaults to 1000 and use that instead of the fixed
setTimeout(1000), and propagate this option through the uploads.post call
(respecting existing maxStatusChecks) so tests can call uploads.post(..., {
delayMs: 0 }) to eliminate the real sleep; alternatively, add support to
uploads.post to accept delayMs and pass it down to _check so existing callers
are unaffected.
- Around line 20-85: Add a new test for the timeout path where
strava.uploads.post exhausts maxStatusChecks and rejects: mock the initial POST
to /api/v3/uploads returning id_str and then mock the subsequent GET
/api/v3/uploads/:id to always return status "Your activity is still being
processed." for N times, call strava.uploads.post with maxStatusChecks set to N
(or lower) and assert the returned promise rejects with the exact timeout
message thrown by _check (e.g., "Upload status check timed out after N
attempts..."); reference strava.uploads.post and the internal _check timeout
behavior when creating the test so it specifically exercises the maxStatusChecks
timeout branch.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mdindex.d.tslib/uploads.jstest/uploads.js
💤 Files with no reviewable changes (1)
- index.d.ts
markstos
approved these changes
Feb 25, 2026
Collaborator
markstos
left a comment
markstos
left a comment
There was a problem hiding this comment.
Seems reasonable to me. I'm approving, but there was one nit I picked for possible refinements before self-merge.
wesleyschlenker
force-pushed
the
deps-and-eslint
branch
from
f8464b3 to
a2de421
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/uploads.js (1)
87-87: Consider updating test name for clarity.The test name "should upload a GPX file without status callback" references the removed
statusCallbackconcept. Consider renaming to something like "should upload a GPX file without polling" or "should return immediately without maxStatusChecks" to better reflect the current API design.✏️ Suggested rename
- it('should upload a GPX file without status callback', async () => { + it('should return immediately when maxStatusChecks is not set', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uploads.js` at line 87, Update the test's description string to reflect the current API by renaming the `it` block whose title currently reads "should upload a GPX file without status callback" to a clearer name such as "should upload a GPX file without polling" or "should return immediately without maxStatusChecks"; locate the `it` test case with that exact old title in the test file and replace the string only (no behavior changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/uploads.js`:
- Line 87: Update the test's description string to reflect the current API by
renaming the `it` block whose title currently reads "should upload a GPX file
without status callback" to a clearer name such as "should upload a GPX file
without polling" or "should return immediately without maxStatusChecks"; locate
the `it` test case with that exact old title in the test file and replace the
string only (no behavior changes).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mdindex.d.tslib/uploads.jstest/uploads.js
💤 Files with no reviewable changes (1)
- index.d.ts
wesleyschlenker
force-pushed
the
deps-and-eslint
branch
from
a2de421 to
5e5687a
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/uploads.js (1)
78-79: Consider using<= 0for robustness.While the past discussion concluded to use
=== 0for readability, a negative value (e.g., user error) would currently proceed to_check()and make one API call before timing out. Using<= 0would handle this edge case more gracefully by returning immediately.♻️ Suggested change
const maxChecks = args.maxStatusChecks != null ? args.maxStatusChecks : 0 - if (maxChecks === 0) { + if (maxChecks <= 0) { return response }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/uploads.js` around lines 78 - 79, The check for maxChecks should treat negative or zero values the same to avoid making an unnecessary API call; change the conditional that uses maxChecks (computed from args.maxStatusChecks) from testing === 0 to <= 0 so the branch returns immediately instead of calling _check(), thereby preventing a single unwanted API request when a negative value is passed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/uploads.js`:
- Around line 78-79: The check for maxChecks should treat negative or zero
values the same to avoid making an unnecessary API call; change the conditional
that uses maxChecks (computed from args.maxStatusChecks) from testing === 0 to
<= 0 so the branch returns immediately instead of calling _check(), thereby
preventing a single unwanted API request when a negative value is passed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mdindex.d.tslib/uploads.jstest/uploads.js
💤 Files with no reviewable changes (1)
- index.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/uploads.js
Collaborator
Author
|
@markstos the AI overlord disagrees with you. Now, who to trust: the artificial or the organic? lol |
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
2 participants
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.
There was actually one more unaddressed callback after I reviewed this morning. setting maxStatusChecks as a static parameter seemed like the most logical resolution for this, but let me know if you'd prefer a different approach.
Summary by CodeRabbit
Documentation
Breaking Changes
Tests