refactor: enhance TypeScript definitions#185
Conversation
- Introduced a new `tsconfig.json` for TypeScript configuration. - Added `typecheck` script to `package.json` for type checking. - Updated `package.json` to include TypeScript and @types/node as dev dependencies. - Modified GitHub Actions workflow to include type checking step. This was tested by intentionally adding a typo to the definition file and running tsc until the output was what I expected
- Modified the `Callback` type to accept `Error | null` and optional `payload`. - Updated method signatures in various routes to use more specific argument types, replacing `any` with `DetailRoute` or `AthleteRouteArgs` where applicable. - Enhanced return types for methods in `RoutesRoutes` and `OAuthRoutes` for better type clarity. - Adjusted the `segments` property in `AthleteRouteResponse` to use `unknown[]` instead of `any[]` for stricter type enforcement.
- Replaced the inline athlete type in `ClubActivity` and `DetailedActivityResponse` with a new `MetaAthlete` interface for improved type consistency and clarity.
- Modified `UploadRouteArgs` to allow `file` as either a string or Buffer, and made `name` and `external_id` optional. - Renamed `athlete_id` to `id` in `AthleteRouteArgs` for consistency. - `AthleteRoutes` and `OAuthRoutes` no longer accept the `done` callback.
…ting - Added JSDoc comments for better type checking and documentation in `axiosUtility.js`. - Improved error handling with detailed parameter descriptions in `StatusCodeError` and `RequestError`. - Updated `httpRequest` function to include comprehensive parameter types and return types. - Refactored `rateLimiting.js` to include TypeScript type definitions for rate limit management, enhancing clarity and maintainability. - Added methods for parsing and updating rate limits with appropriate type annotations.
- Added comprehensive JSDoc comments and TypeScript type definitions across various files, including `axiosUtility.js`, `index.d.ts` - Updated method signatures to remove callback parameters, ensuring all API methods return Promises for consistency. - Updated README to reflect changes in API usage, emphasizing the Promise-based approach. - Renames listKudos to listKudoers to more accurately describe its return type
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughConverts callback-style APIs to Promise-based, adds TypeScript config and CI typecheck step, introduces an authenticated client factory, refactors HTTP utilities/errors and httpClient (uploads, multipart, polling, pagination), enriches rate-limit state, and updates docs, types, and tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
participant Caller as Consumer
participant Client as Strava Client
participant Http as HttpClient/httpRequest
participant API as Strava API
participant Rate as RateLimiting
end
Caller->>Client: call method (uses token)
Client->>Http: build request (Authorization, User-Agent, options)
Http->>API: perform HTTP request
API-->>Http: return response (headers + body)
Http->>Rate: updateRateLimits(response.headers)
Http-->>Client: resolve Promise(parsed body)
Client-->>Caller: return Promise result
sequenceDiagram
rect rgba(220,255,220,0.5)
participant Caller as Consumer
participant Uploads as Uploads.post
participant Http as HttpClient/httpRequest
participant API as Strava API
end
Caller->>Uploads: post(file, data_type, statusCallback?)
Uploads->>Http: POST multipart/form-data
Http->>API: create upload
API-->>Http: upload { id, status: processing }
Http-->>Uploads: return upload object
alt statusCallback provided
loop polling
Uploads->>Http: GET uploads/{id}
Http->>API: fetch status
API-->>Http: status update
Http-->>Uploads: status result
end
Uploads-->>Caller: resolve final upload result
else no polling
Uploads-->>Caller: return initial upload object
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
README.md (1)
313-313:⚠️ Potential issue | 🟡 MinorThe error handling example uses an internal path that npm consumers cannot access.
require('./axiosUtility')refers to a library-internal file. When installing via npm, consumers cannot import from internal paths. The README should either:
- Export
StatusCodeErrorandRequestErrorfrom the main entry point (index.js) and update the example to use the public API, or- Clarify that consumers must import directly from the internal file, or
- Show how consumers should catch and identify these errors using the thrown error's
nameproperty orinstanceofchecks without relying on importing the classes.lib/httpClient.js (1)
373-402:⚠️ Potential issue | 🟠 MajorMissing body-extraction path for
FullResponsewith abodyproperty.The request handler contract allows returning
FullResponse | string | object. When a request handler returns aFullResponsewith a truthybodyproperty, the current code skips lines 382–386 and falls through to line 402, returning the entire wrapper object (including headers) instead of extracting and parsing the body. Route modules expect the resolved value to be the API payload, not the{ headers, body }wrapper.Suggested fix: extract and parse `body` from FullResponse
// if the client auto-parsed JSON, response is already an object: if (typeof response === 'object' && response !== null && !(/** `@type` {FullResponse} */ (response).body)) { return response } + // If the response is a FullResponse with a body, extract and parse the body + if (typeof response === 'object' && response !== null && /** `@type` {FullResponse} */ (response).body) { + const body = /** `@type` {FullResponse} */ (response).body + if (typeof body === 'string') { + if (options.responseType === 'text') { + return body + } + if (options.responseType === 'json' || options.responseType === 'formdata') { + return /** `@type` {object} */ (JSONbig.parse(body)) + } + return body + } + return body + } + if (typeof response === 'string') {index.d.ts (3)
32-35:⚠️ Potential issue | 🟠 Major
CreatePushSubscriptionRouteArgsrequiresclient_idandclient_secret, but the implementation injects them from the authenticator.In
lib/pushSubscriptions.js(lines 67-70),client_idandclient_secretare populated fromauthenticator.getClientId()/authenticator.getClientSecret(), not from the caller's args. ExtendingApplicationBaseArgsforces callers to provide credentials they don't need to pass.Proposed fix
-export interface CreatePushSubscriptionRouteArgs extends ApplicationBaseArgs { +export interface CreatePushSubscriptionRouteArgs { callback_url: string; verify_token: string; }
19-26:⚠️ Potential issue | 🟡 Minor
Datetype used for fields that arrive as JSON strings.
ListPushSubscriptionResponse.created_at/updated_at(lines 24-25),SummaryAthlete.created_at/updated_at(lines 437-438), andDetailedActivity.start_date/start_date_local(lines 673-674) are typed asDate, but the Strava API returns ISO 8601 strings andJSON.parsedoes not auto-hydrate them intoDateobjects. These should bestringto match runtime behavior.Also applies to: 437-438, 673-674
907-926:⚠️ Potential issue | 🟠 Major
Strava.client()return type is incorrect — should not bevoid.The JSDoc comment states "Call with
new strava.client(token, request)", and the implementation (lines 38-66 of index.js) confirms this is a constructor function that mutatesthiswith properties likeaccess_token,athlete,athletes,activities, etc. When invoked withnew, the constructor implicitly returns the configured instance, notvoid. The type signature should reflect this by using TypeScript's constructor signature syntax (e.g.,new (token: string, request?: RequestHandler): <StravaInstance>) rather than declaring a method that returnsvoid.
🤖 Fix all issues with AI agents
In `@index.d.ts`:
- Around line 162-180: The SummarySegment interface currently marks
athlete_pr_effort and athlete_segment_stats as required but they can be absent
for summary segments; update the SummarySegment definition to make
athlete_pr_effort and athlete_segment_stats optional (i.e., add ? to those
property names) so their types SummaryPRSegmentEffort and SummarySegmentEffort
reflect that they may be undefined when the authenticated athlete has no
efforts.
- Around line 354-382: The current StreamSet and StreamsRoutes types are
incorrect: StreamSet models all streams as required and StreamsRoutes methods
return Promise<StreamSet[]>, but the API returns an array of individual stream
objects. Replace the StreamSet usage by defining a union type (e.g., Stream =
TimeStream | DistanceStream | LatLngStream | AltitudeStream |
SmoothVelocityStream | HeartrateStream | CadenceStream | PowerStream |
TemperatureStream | MovingStream | SmoothGradeStream) and update
StreamsRoutes.activity/route/effort/segment to return Promise<Stream[]>;
alternatively, if you intend to keep StreamSet, make all properties optional on
StreamSet and update the return types accordingly (but prefer the union
approach).
In `@lib/athlete.js`:
- Around line 56-59: The JSDoc cast is applied to the already-awaited value in
athlete.prototype.get (and similarly at the other return sites around lines 105
and 119); update each return to either cast the promise before awaiting (e.g.,
apply /** `@type` {Promise<SummaryAthlete>} */ to this.client.getEndpoint(...) and
then await it) or change the JSDoc to the unwrapped type and cast the resolved
value (e.g., /** `@type` {SummaryAthlete} */ after await); apply the same
consistent fix to the other methods that currently do the post-await Promise<T>
cast.
In `@lib/clubs.js`:
- Around line 23-33: The synchronous throw in clubs.prototype.get (and the
helper _listHelper used by listMembers, listActivities, listAdmins) causes
uncaught exceptions when callers expect a Promise; change these to consistently
return rejected Promises instead of throwing synchronously by either marking
clubs.prototype.get and _listHelper as async (so thrown errors become rejected
promises) or replacing the throw statements with return Promise.reject(new
Error(...)); ensure every code path in get, _listHelper, listMembers,
listActivities, and listAdmins returns a Promise so callers can safely use
.catch().
In `@lib/pushSubscriptions.js`:
- Around line 50-53: pushSubscriptions.prototype.create (and similarly
pushSubscriptions.prototype.delete) currently throws synchronously when required
args are missing; change this to return Promise.reject(new Error(...)) so
validation failures are delivered as rejected Promises consistent with
segments.js and callers using .catch/await will receive a promise rejection.
Locate the argument checks in pushSubscriptions.prototype.create and
pushSubscriptions.prototype.delete and replace the synchronous throw statements
with returns of Promise.reject(new Error('required args missing: callback_url
and verify_token are required')) (or the appropriate error message used in
segments.js) so behavior matches the rest of the module.
In `@lib/segmentEfforts.js`:
- Around line 23-25: Update the stale inline comment above the args.id
validation in the module (the comment currently reads "// require segment id")
to accurately reflect the check and error message by changing it to something
like "// require segment effort id" so it matches the thrown Error in the block
that validates args.id.
In `@lib/uploads.js`:
- Around line 67-73: The current falsy check in the loop over _allowedFormProps
uses if (argsRecord[key]) which drops valid numeric 0 values (notably for
trainer and commute); change the condition to explicitly test for presence such
as if (argsRecord[key] != null) or if (argsRecord[key] !== undefined) so that 0
is preserved when copying into args.formData, keeping the rest of the loop and
variables (argsRecord, _allowedFormProps, args.formData) unchanged.
In `@README.md`:
- Line 227: Fix the typo in the README where the description for
strava.oauth.getToken(code) reads "Used to token exchange" — update the phrase
to "Used for token exchange" so the entry for the symbol
strava.oauth.getToken(code) reads correctly.
- Line 284: The phrase "comma separated" in the README usage description for
strava.segments.explore(args) should be hyphenated as "comma-separated"; update
the README entry that documents strava.segments.explore(args) so the compound
adjective reads `"Expects arg `bounds` as a comma-separated string..."` (modify
the text around the function reference `strava.segments.explore(args)`
accordingly).
🧹 Nitpick comments (12)
tsconfig.json (2)
13-17:outDirandsourceMapare redundant whennoEmitis true.Since
noEmit: trueprevents any output,outDir: "dist"andsourceMap: truehave no effect. Consider removing them to reduce config noise.Proposed cleanup
// Emit "declaration": false, "declarationMap": false, "noEmit": true, - "outDir": "dist", // JavaScript Support "allowJs": true, "checkJs": true, // Language and Environment "target": "es2022", - "sourceMap": true,
27-28:skipLibCheck: falsewill type-check all.d.tsfiles innode_modules.This is thorough but can noticeably slow down type-checking, especially as dependencies grow. If typecheck times become a concern, flipping this to
trueis the standard recommendation.lib/athletes.js (1)
24-41:_listHelperis now constrained to only'stats'— consider simplifying.The
listTypeparameter is typed as the literal'stats', making the indirection through_listHelperunnecessary. If no other sub-resources will use this helper, inlining the logic intostats()would be simpler. If future expansion is planned (e.g.,'koms'), consider broadening the type.Also,
getPaginationQSis called for the stats endpoint, which typically returns a single object rather than a paginated list — this is pre-existing behavior but worth noting.lib/authenticator.js (1)
89-147: RepeatedfetchConfig()calls on every getter invocation when value is undefined.Each getter (e.g.,
getToken,getClientId) callsfetchConfig()every time the value is falsy, which re-reads the config file and environment variables. If the credential is legitimately absent, this I/O cost recurs on every call. A simpleconfigLoadedflag would avoid redundant reads.Sketch of a configLoaded guard
+var configLoaded = false + var fetchConfig = function (config) { if (config) { if (config.access_token) token = config.access_token if (config.client_id) clientId = config.client_id if (config.client_secret) clientSecret = config.client_secret if (config.redirect_uri) redirectUri = config.redirect_uri } else { readConfigFile() readEnvironmentVars() } + configLoaded = true }Then in each getter, replace
if (!token)withif (!configLoaded):getToken: function () { - if (!token) { + if (!configLoaded) { fetchConfig() } // ...And in
purge:purge: function () { token = undefined clientId = undefined clientSecret = undefined redirectUri = undefined + configLoaded = false }lib/rateLimiting.js (2)
157-182:updateRateLimitsclears all state when headers are missing — verify this is intentional.When
parseRateLimitsreturnsnull(e.g. a response lacking one of the four required headers),updateRateLimitscallsthis.clear(), which resets all counters to zero. This means a single response without complete rate-limit headers erases the previously known limit/usage state.If the intent is to be conservative (assume unknown = safe), this is fine. But if a consumer is tracking cumulative rate state and an intermediary error response omits headers, silently clearing could mask that the rate limit was already nearly exceeded.
Consider returning
nullwithout clearing state, or at least documenting the clearing behavior:Possible alternative: preserve state on missing headers
} else { - this.clear() + // Headers missing/invalid — preserve existing state }
46-46: Minor inconsistency:rlalias vsthisacross methods.
exceeded/fractionReached/readExceeded/readFractionReachedread state via the module-levelrlalias, whileupdateRateLimitsandclearmutate state viathis. This works becauserl === RateLimit, but mixing the two patterns can confuse future maintainers. Consider standardizing on one approach.Also applies to: 53-63, 158-159
lib/httpClient.js (1)
243-271:postUpload: solid upload implementation with one typedef nit.The validation, stream creation, and header merging look correct. Minor note: the typedef for
fileisstring|Buffer, butfs.createReadStreamtreats a Buffer as a file path (not file contents). Consider clarifying in the JSDoc thatfileis a file path.Suggested JSDoc clarification
- * `@property` {string|Buffer} [file] + * `@property` {string} file - File path to upload (e.g., 'data/your_file.gpx')index.js (1)
29-66: Constructor lacksnew-call guard
strava.clientis intended to be called withnew(as the JSDoc states), but nothing prevents it from being called as a plain function. Withoutnew,thisrefers to the caller's context (orundefinedin strict mode), silently corrupting state instead of throwing.Consider adding a guard or converting to a class:
🛡️ Proposed guard
strava.client = function (token, request = httpRequest) { + if (!(this instanceof strava.client)) { + return new strava.client(token, request) + } this.access_token = tokenlib/oauth.js (1)
75-89:json: trueis a no-op with the axios-basedhttpRequestThe
json: trueproperty (Lines 79, 101, 125) is a leftover from therequest-promiseAPI. The axios wrapper inaxiosUtility.jsalready defaultsresponseTypeto'json'and doesn't read ajsonproperty. It's also absent from theHttpRequestOptionstypedef. Consider removing it to avoid confusion.♻️ Cleanup in all three methods
// In getToken (line 79): method: 'POST', url: 'https://www.strava.com/oauth/token', - json: true, qs: { // In deauthorize (line 101): url: 'https://www.strava.com/oauth/deauthorize', method: 'POST', - json: true, simple: false, // In refreshToken (line 125): url: 'https://www.strava.com/oauth/token', method: 'POST', - json: true, simple: true,axiosUtility.js (1)
94-129:options.maxRedirects || 5treats explicit0as falsyLine 108: if a caller explicitly passes
maxRedirects: 0, the||operator falls through to5. Consider using nullish coalescing (??) instead if you want to allow0.This is pre-existing behavior, so feel free to defer.
♻️ Suggested fix
- maxRedirects: options.maxRedirects || 5, + maxRedirects: options.maxRedirects ?? 5,lib/uploads.js (1)
99-120: Unbounded recursive polling could exhaust the stack on a stuck upload
_checkrecursively calls itself every second until the upload is done. If the Strava API never transitions out of the "processing" state (e.g., a hung upload), this will recurse indefinitely. Consider adding a max-retry limit or converting to an iterative loop.This appears to be pre-existing behavior, so feel free to defer.
lib/pushSubscriptions.js (1)
86-98:list()silently succeeds with empty credentials if authenticator is not configured.Lines 90-91 fall back to
''whengetClientSecret()/getClientId()return null. This means a misconfigured client will send an API request with empty credentials and get an opaque Strava error instead of a clear local error. This is a pre-existing pattern applied consistently across all three methods, so it's not a blocker, but worth noting for future improvement.
|
Thanks, go ahead and address the Code Rabbit feedback and also bump the major version in package.json. You might as well bump axios in this branch as well. (see #186) |
…eScript definitions - Updated the `DetailedSegment` and `SummarySegment` interfaces in `index.d.ts` to make `athlete_pr_effort` and `athlete_segment_stats` optional properties, enhancing flexibility in type usage.
- Changed the `StreamSet` interface to a union type
Apply JSDoc @type to the promise then await it, instead of casting the awaited value as a Promise. Fixes get, update, and _listHelper.
Refactored the `get` and `_listHelper` methods in the clubs module to utilize async/await syntax, ensuring proper error resolution, since throw will no longer break .catch() chaining.
…ror handling Updated the `create` and `delete` methods in the pushSubscriptions module to utilize async/await syntax, ensuring proper error handling and consistency in promise resolution.
Modified the condition in the post method to check for null values instead of falsy values, ensuring that valid inputs `0` for `trainer` and `commute` are correctly processed.
…ethods Refactored the tests for the `create` and `delete` methods in the pushSubscriptions module to use async/await and assert.rejects for improved error handling. Updated error messages to provide clearer feedback on missing parameters.
Updated the _requestHelper method to improve response processing. Added checks for FullResponse objects with body, allowing for proper extraction and parsing of response payloads based on specified response types. This ensures that both wrapped and direct payloads are handled correctly.
Changed the type of `created_at` and `updated_at` fields to string in multiple interfaces for consistency. Added a new `StravaClientInstance` interface to encapsulate the Strava client structure for more accurate return typing.
Consolidated the logic in the _listHelper method to directly construct the endpoint for fetching athlete stats, improving readability and maintainability. It seems like the helper was in place to be used for another route, but it looks like Strava never developed this for the v3 api, so we can safely remove the code.
… fetches Introduced a boolean flag `configLoaded` to track the loading state of configuration. Updated the token, client ID, client secret, and redirect URI retrieval methods to check this flag before fetching the config, enhancing efficiency and preventing unnecessary calls to `fetchConfig`.
…ntext in rate limit methods Updated the rate limit methods to use 'this' context instead of a global variable reference, improving encapsulation and clarity. This change enhances the maintainability of the code by standardizing the object used for ratelimiting context.
…ypedef Clarified the description of the `file` property in the PostUploadArgs typedef to specify that it should be a file path string, enhancing the documentation for better understanding.
…keyword Added a check in the strava.client function to enforce instantiation with the 'new' keyword, preventing potential issues when the function is called without it. This change improves the robustness of the client initialization process.
…ethods Eliminated the redundant 'json' property from the request options in the getToken, deauthorize, and refreshToken methods. axios defaults to json. this is a leftover from request-promise
Modified the maxRedirects option in the httpRequest function to correctly handle a value of 0, ensuring that it is explicitly set to 0 instead of defaulting to 5. This change improves the flexibility of the request configuration.
…equirement Updated the get, listStarred, starSegment, listEfforts, explore, and _listHelper methods to be async, improving error handling. Introduced a new internal function, _requireSegmentId, to validate the presence of a segment ID in the arguments, ensuring consistent error management across these methods.
…or handling If _requireClientCredentials() throws, it will be an uncaught synchronous exception rather than a rejected Promise. A caller doing strava.pushSubscriptions.list().catch(handler) won't catch it.
…ror handling Updated the get and getFile methods to be asynchronous, allowing for better management of errors when validating route IDs. This change ensures that any exceptions thrown by _requireRouteId are properly caught by callers using .catch().
_requireRouteId requires args.id to be a string (typeof args.id !== 'string'), but Strava route IDs are numeric. A call like strava.routes.get({ id: 12345 }) will throw even though the endpoint construction (endpoint += args.id) handles numbers fine via concatenation. Other modules (e.g., lib/clubs.js, lib/activities.js) only check typeof args.id === 'undefined', accepting both strings and numbers.
Modified the type definition for access_token in both GetEndpointArgs and CheckArgs to explicitly allow for undefined values. Updated the handling of access_token in the uploads post method to default to undefined instead of an empty string, which is worse than omitting the header entirely.
Refactored the timeout error handling in the _check method to ensure that the callback doesn't get called with the same error twice.
…handling Updated the _typeHelper method to be asynchronous, allowing for better management of errors related to missing id in the arguments. This change ensures that any exceptions thrown are properly handled by callers using .catch().
Updated the postUpload method to utilize FormData and Blob for file uploads, ensuring proper multipart handling with boundaries. This change improves compatibility with Node 18+ and streamlines the process of appending form data and files.
…retrieval callbacks are no more!
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@index.d.ts`:
- Around line 46-48: The declared type for the "file" field currently allows
Buffer but the runtime in httpClient.js (postUpload) uses
fsPromises.readFile(args.file) and path.basename(args.file), which require a
file path string; update the declaration for the "file" property in index.d.ts
to be string only (remove Buffer) so it matches the implementation and avoids
passing Buffers to path.basename/readFile; ensure references to "file" in
related interfaces/types are updated to the string type.
- Around line 838-844: The AthleteRoutes.listActivities signature currently
returns Promise<DetailedActivity[]] but the Strava list endpoint returns
lightweight activity objects; add a new SummaryActivity interface (containing
only the fields returned by GET /athlete/activities such as id, name, distance,
moving_time, elapsed_time, total_elevation_gain, type, start_date,
start_date_local, utc_offset, external_id, upload_id, athlete, map,
average_speed/average_cadence/average_heartrate if present, and any other
list-only fields) and change the AthleteRoutes.listActivities return type to
Promise<SummaryActivity[]>; ensure you do not copy the DetailedActivity-only
required properties (segment_efforts, splits_metric, splits_standard, laps,
best_efforts) into SummaryActivity and keep DetailedActivity unchanged for
endpoints that need full details.
In `@lib/activities.js`:
- Around line 136-171: listKudoers currently delegates to _listHelper which only
builds cursor-style QS (getCursorPaginationQS) and drops page/per_page; fix by
making listKudoers build a page-based query string with
this.client.getPaginationQS when args contains page or per_page, append it to
'activities/<id>/kudos' and call this.client.getEndpoint(endpoint, args)
directly (avoid _listHelper), or alternatively update _listHelper to detect
page/per_page and choose between getPaginationQS and getCursorPaginationQS
before calling this.client.getEndpoint so kudoers pagination is preserved.
🧹 Nitpick comments (2)
index.d.ts (1)
718-724: SeveralDetailedActivityarray fields should be optional.
splits_metric,splits_standard,laps, andbest_effortsare currently required, but they may not be present for all activity types (e.g.,best_effortsonly appears on runs, splits only on distance-based activities). Marking them optional would prevent runtime mismatches.Suggested fix
- splits_metric: Split[]; - splits_standard: Split[]; - laps: Lap[]; - best_efforts: DetailedSegmentEffort[]; + splits_metric?: Split[]; + splits_standard?: Split[]; + laps?: Lap[]; + best_efforts?: DetailedSegmentEffort[];lib/routes.js (1)
47-51:_requireRouteIdnow accepts numeric IDs — previous issue resolved.The validation was relaxed from
typeof args.id !== 'string'totypeof args.id === 'undefined', matching the pattern in other modules and correctly accepting numeric route IDs.One minor nit: the JSDoc on Line 45 still says "When args.id is missing or not a string" but the check now only validates presence, not type.
JSDoc fix
- * `@throws` {Error} When args.id is missing or not a string + * `@throws` {Error} When args.id is missing
… SummaryActivity Removed redundant properties from DetailedActivity and extended it from SummaryActivity. Updated the listActivities method to return SummaryActivity[] for improved data handling.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@index.d.ts`:
- Around line 610-629: The Lap interface's average_cadence property must be made
optional because some activities omit cadence; update the Lap interface (symbol:
Lap) in index.d.ts to make average_cadence optional (e.g., change it from
required number to an optional number type) so callers handle absent cadence
safely and TypeScript reflects the API shape.
- Around line 670-681: The DetailedActivity interface currently requires several
fields that are not always present; update the interface so that photos,
segment_efforts, splits_metric, splits_standard, and best_efforts are optional
(e.g., add ? or allow undefined/null) to reflect non-running and manual
activities; locate the DetailedActivity declaration and change the properties
photos, segment_efforts, splits_metric, splits_standard, and best_efforts to
optional types (or permit null/undefined) so consumers won’t get type errors
when those fields are missing.
- Around line 795-840: The SummaryActivity interface currently requires
elev_high, elev_low, device_name, and upload_id_str but the API can omit them;
update the SummaryActivity declaration to mark those four properties as optional
using the ? modifier (i.e., elev_high?: number, elev_low?: number, device_name?:
string, upload_id_str?: string) so consumers won’t get type errors when the
fields are missing; locate and change these properties inside the
SummaryActivity interface.
- Around line 74-86: The SegmentListStarredArgs interface incorrectly exposes
non-supported query params for GET /segments/starred; update
SegmentListStarredArgs (extending BaseArgs) to only include page?: number and
per_page?: number, removing athlete_id, gender, age_group, weight_class,
following, club_id, date_range, start_date_local, and end_date_local; if those
removed fields are needed for a leaderboard-type endpoint, create a new
interface (e.g., SegmentLeaderboardArgs) containing those specific fields
instead of adding them to SegmentListStarredArgs.
🧹 Nitpick comments (1)
index.d.ts (1)
267-366: Consider a generic base to reduce repetition across 11 stream interfaces.All stream interfaces share the same shape (
original_size,resolution,series_type) and differ only in thetypeliteral anddataelement type. A generic base would cut ~100 lines of duplication.Example
interface BaseStream<T extends string, D> { type: T; original_size: number; resolution: "low" | "medium" | "high"; series_type: "distance" | "time"; data: D[]; } export type TimeStream = BaseStream<"time", number>; export type DistanceStream = BaseStream<"distance", number>; export type LatLngStream = BaseStream<"latlng", LatLng>; export type AltitudeStream = BaseStream<"altitude", number>; export type SmoothVelocityStream = BaseStream<"velocity_smooth", number>; export type HeartrateStream = BaseStream<"heartrate", number>; export type CadenceStream = BaseStream<"cadence", number>; export type PowerStream = BaseStream<"watts", number>; export type TemperatureStream = BaseStream<"temp", number>; export type MovingStream = BaseStream<"moving", boolean>; export type SmoothGradeStream = BaseStream<"grade_smooth", number>;
…ty interfaces Updated ActivityCommentsArgs and ActivityKudoersArgs to remove deprecated page and per_page properties, replacing them with page_size and after_cursor for cursor-based pagination. Adjusted README to reflect these changes and provide updated usage examples.
Eliminated deprecated properties from SegmentListStarredArgs to streamline the interface and improve clarity. This change enhances the overall structure of the type definitions.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@index.d.ts`:
- Around line 393-413: DetailedClub currently uses the wrong enum for sport
types (it references SportType), so change DetailedClub.sport_type to the
club-specific union used by SummaryClub (i.e., the string union "cycling" |
"running" | "triathlon" | "other") or extract that union into a shared type
(e.g., ClubSportType) and use it in both SummaryClub and DetailedClub; update
the type reference for DetailedClub.sport_type to the shared ClubSportType (or
the explicit union) and ensure any imports/exports are adjusted accordingly.
- Around line 732-748: The ActivityKudoersArgs type and related uses incorrectly
define cursor-based fields; change ActivityKudoersArgs to use page?: number and
per_page?: number and update the ActivitiesRoutes.listKudoers signature to
accept this corrected type, then update the implementation in
activities.prototype._listHelper (for the kudoers path) to use page-based
pagination (swap the call to HttpClient.prototype.getCursorPaginationQS for the
page-based query-string helper or build QS with page/per_page) so the GET
/activities/{id}/kudos request sends page and per_page parameters recognized by
the Strava API.
In `@lib/activities.js`:
- Around line 160-171: The listKudoers call is using cursor-style params but the
Strava kudoers endpoint expects page/per_page; update the ActivityKudoersArgs
type to use page and per_page (instead of page_size/after_cursor) and ensure
listKudoers passes those fields through to activities.prototype._listHelper (or,
alternatively, add explicit mapping in listKudoers from page/per_page -> the
cursor QS expected by getCursorPaginationQS). Refer to ActivityKudoersArgs, the
listKudoers callsite, and activities.prototype._listHelper when making the
change so the correct page-based params reach the API.
In `@README.md`:
- Around line 158-173: Documentation incorrectly claims "cursor-based pagination
only"; update the README to clarify that cursor-based pagination applies only to
certain list endpoints and that many endpoints still use page-based pagination.
Change the paragraph under "Pagination" to say cursor-based pagination is
supported for specific endpoints (e.g., listComments, listKudoers) while other
endpoints continue to accept page/per_page; explicitly list or link to the
endpoints/types that remain page-based (reference SegmentListStarredArgs,
SegmentEffortsArgs, ClubsRoutesListArgs, AthleteListArgs) so readers know which
APIs use page-based params, and keep the example for cursor usage separate and
labeled as a cursor-specific example.
… pagination documentation Eliminated the ApplicationBaseArgs interface from type definitions and revised the README to clarify pagination handling, emphasizing the transition from deprecated properties to the new page_size and after_cursor parameters. Updated usage examples to reflect these changes.
improves flexibility while page and per_page are still deprecated.
… optional properties Modified the DetailedClub interface to specify sport_type as a union of string literals. Updated various properties in the DetailedActivity, Lap, and SummaryActivity interfaces to be optional, enhancing flexibility in type definitions.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/httpClient.js (1)
363-432:⚠️ Potential issue | 🔴 CriticalRate limiting on successful responses depends on the fix in
axiosUtility.js.
_requestHelpersetsresolveWithFullResponse = true(line 370) and expects the response to be a{ headers, body }object (lines 381-399) for rate-limit updates. As flagged in theaxiosUtility.jsreview,httpRequestcurrently ignores this flag and returns onlyresponse.data. Until that's fixed,rateLimiting.updateRateLimits()will never fire on success—only on error paths (line 425-427).The response shape handling logic itself (lines 386-419) is well-structured and handles all the expected cases correctly once the upstream fix is in place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/httpClient.js` around lines 363 - 432, The success-path rate-limit update is never called because _requestHelper (HttpClient.prototype._requestHelper) sets options.resolveWithFullResponse = true and expects a FullResponse shape ({ headers, body }), but the upstream httpRequest implementation in axiosUtility.js currently returns only response.data; fix httpRequest to respect options.resolveWithFullResponse (or similarly named flag) so it returns the full response object (including headers) when set, allowing rateLimiting.updateRateLimits(...) inside _requestHelper to receive headers on successful calls; verify that resolveWithFullResponse (and any equivalent option) is propagated through the axios wrapper and that the returned object contains headers and body fields as expected.index.js (1)
22-27:⚠️ Potential issue | 🔴 Critical
axiosInstance.create()does not exist and will throw a runtime error — remove the unused assignment or switch toaxios.create().
axiosInstanceis the result ofaxios.create()fromaxiosUtility.js. Thecreate()method is only available on the top-levelaxiosexport, not on instances. CallingaxiosInstance.create()will throwTypeError: axiosInstance.create is not a functionwhen the module loads.Additionally,
strava.defaultRequestis never referenced elsewhere and is not exported, making this dead code.Either remove the assignment entirely, or replace
axiosInstance.create()withaxios.create()if this configuration is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 22 - 27, The code assigns strava.defaultRequest = axiosInstance.create(...), but axiosInstance (from axiosUtility.js) is an axios instance and has no create() method, and strava.defaultRequest is unused; either remove the entire assignment to eliminate dead code, or if you need a separate configured axios instance replace axiosInstance.create(...) with axios.create({ baseURL: 'https://www.strava.com/api/v3/', headers: { 'User-Agent': 'node-strava-v3 v' + version } }) and keep the name strava.defaultRequest; ensure you do not rely on axiosInstance.create anywhere else and remove the unused export/variable if you choose deletion.lib/gear.js (1)
20-30:⚠️ Potential issue | 🟠 MajorSynchronous
throwin non-asyncfunction breaks.catch()chaining.
gear.prototype.getis notasync, so thethrowon line 25 raises synchronously. A caller writingstrava.gear.get({}).catch(handler)will get an uncaught exception. This is the same issue that was already fixed inlib/clubs.js,lib/routes.js,lib/segments.js, andlib/activities.jsby marking methodsasync.Proposed fix
-gear.prototype.get = function (args) { +gear.prototype.get = async function (args) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/gear.js` around lines 20 - 30, The sync throw in gear.prototype.get causes uncaught exceptions for callers expecting a Promise; make the method async (change "gear.prototype.get = function (args) {" to "gear.prototype.get = async function (args) {" or equivalent) so any thrown Error becomes a rejected Promise and .catch() works, keep the existing return call to this.client.getEndpoint(...) and preserve the DetailedGear Promise type annotation.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@axiosUtility.js`:
- Around line 133-134: The updateDefaultHeaders function is merging headers into
axiosInstance.defaults.headers at the wrong level; instead of assigning directly
to the structured headers object, merge provided headers into
axiosInstance.defaults.headers.common so method-specific keys
(get/post/put/etc.) remain intact. Update the logic in updateDefaultHeaders to
Object.assign or spread into axiosInstance.defaults.headers.common (using
axiosInstance.defaults.headers.common as the target) so provided headers like
Authorization are applied idiomatically to all request methods.
- Around line 94-126: httpRequest currently always returns response.data which
drops response.headers so callers like _requestHelper that set
resolveWithFullResponse never receive headers for rate-limit updates; modify
httpRequest (the function using axiosInstance) to check
options.resolveWithFullResponse (or similar truthy flag) and, on success, return
{ headers: response.headers, body: response.data } when that flag is true,
otherwise continue returning response.data; keep existing error handling
(StatusCodeError / RequestError) unchanged so failures behave as before.
In `@index.d.ts`:
- Around line 656-667: The DetailedActivity interface currently declares laps as
required (laps: Lap[]) while other collection fields like splits_metric,
splits_standard, and best_efforts are optional; change laps to be optional
(e.g., laps?: Lap[]) so the type matches cases where the Strava API omits laps
(such as manual activities) and keeps consistency with the other optional fields
in DetailedActivity.
In `@index.js`:
- Around line 22-27: The code assigns strava.defaultRequest =
axiosInstance.create(...), but axiosInstance (from axiosUtility.js) is an axios
instance and has no create() method, and strava.defaultRequest is unused; either
remove the entire assignment to eliminate dead code, or if you need a separate
configured axios instance replace axiosInstance.create(...) with axios.create({
baseURL: 'https://www.strava.com/api/v3/', headers: { 'User-Agent':
'node-strava-v3 v' + version } }) and keep the name strava.defaultRequest;
ensure you do not rely on axiosInstance.create anywhere else and remove the
unused export/variable if you choose deletion.
In `@lib/athletes.js`:
- Around line 20-26: The synchronous throw in athletes.prototype.stats prevents
callers from catching errors via .catch; update the function to return a
rejected Promise instead of throwing (or make it async). Specifically, modify
athletes.prototype.stats so that when args.id is undefined it returns
Promise.reject(new Error('args must include an athlete id')) (or mark the
function async and throw inside) while keeping the existing endpoint
construction using this.client.getPaginationQS and the final return of
this.client.getEndpoint(endpoint, args).
In `@lib/gear.js`:
- Around line 20-30: The sync throw in gear.prototype.get causes uncaught
exceptions for callers expecting a Promise; make the method async (change
"gear.prototype.get = function (args) {" to "gear.prototype.get = async function
(args) {" or equivalent) so any thrown Error becomes a rejected Promise and
.catch() works, keep the existing return call to this.client.getEndpoint(...)
and preserve the DetailedGear Promise type annotation.
In `@lib/httpClient.js`:
- Around line 305-321: The getCursorPaginationQS function uses an explicit
undefined/null check for page_size but a truthy check for after_cursor, causing
inconsistency and excluding valid empty-string cursors; update
HttpClient.prototype.getCursorPaginationQS to set after_cursor the same way as
page_size (e.g., compute after_cursor = typeof args.after_cursor !== 'undefined'
? args.after_cursor : null and then use if (after_cursor !== null) {
qa.after_cursor = after_cursor }) so both parameters are handled consistently
(refer to function name getCursorPaginationQS and local variables page_size,
after_cursor, qa).
- Around line 363-432: The success-path rate-limit update is never called
because _requestHelper (HttpClient.prototype._requestHelper) sets
options.resolveWithFullResponse = true and expects a FullResponse shape ({
headers, body }), but the upstream httpRequest implementation in axiosUtility.js
currently returns only response.data; fix httpRequest to respect
options.resolveWithFullResponse (or similarly named flag) so it returns the full
response object (including headers) when set, allowing
rateLimiting.updateRateLimits(...) inside _requestHelper to receive headers on
successful calls; verify that resolveWithFullResponse (and any equivalent
option) is propagated through the axios wrapper and that the returned object
contains headers and body fields as expected.
In `@lib/oauth.js`:
- Around line 75-88: The oauth.getToken implementation is sending sensitive
credentials (client_secret, client_id, grant_type, code) via the options.qs
(query string); change the HTTP options to put these parameters into the POST
body instead of qs by replacing options.qs with options.data (or body) and
ensure the request Content-Type is set appropriately so httpRequest/axiosUtility
sends a form-encoded or JSON body as Strava expects; apply the same change to
the refreshToken function so refresh_token and client_secret are moved from qs
into the POST body when calling httpRequest.
In `@lib/routes.js`:
- Around line 41-51: The JSDoc for _requireRouteId is stale: it claims it throws
when args.id is "missing or not a string" but the implementation only checks for
undefined; either update the implementation to also validate type (e.g., ensure
typeof args.id === 'string') or change the JSDoc to only state it throws when
the id is missing; locate the _requireRouteId function and pick one fix (prefer
updating the JSDoc to remove "or not a string" if stricter validation was
intentionally removed) and make the text and `@throws` description consistent with
the actual behavior.
In `@lib/segmentEfforts.js`:
- Around line 20-30: The method segmentEfforts.prototype.get currently throws
synchronously when args.id is missing; change its declaration to an async
function (e.g., segmentEfforts.prototype.get = async function (args) { ... }) so
that thrown errors produce a rejected Promise callers can .catch, keep the same
validation for args.id and then return the call to
this.client.getEndpoint(endpoint, args) (preserve the existing JSDoc cast).
In `@lib/uploads.js`:
- Around line 143-156: uploads.prototype._uploadIsDone currently treats any
status other than the exact string 'Your activity is still being processed.' as
complete; update this to explicitly recognize terminal success statuses (e.g.,
'Your activity is ready.') and explicit error conditions by checking args.error
or known error status strings so transient errors are not marked as done; modify
_uploadIsDone to return false while args.error exists or status is a
non-terminal/processing value and return true only for explicit terminal success
statuses, using the function name uploads.prototype._uploadIsDone and the
properties args.status and args.error to locate the logic to change.
In `@test/pushSubscriptions.js`:
- Around line 7-16: The tests in pushSubscriptions.js leave the singleton
authenticator's configLoaded cache populated between tests; update the afterEach
block to purge that cache by calling authenticator.purge() (or delegate to the
existing testHelper.cleanupAuth() used elsewhere) so each test suite gets a
fresh auth state; locate the afterEach in pushSubscriptions.js and add the call
alongside the existing env cleanup and nock.cleanAll() to ensure isolation.
In `@tsconfig.json`:
- Around line 13-25: The tsconfig currently sets "noEmit": true while also
specifying "outDir": "dist" and "sourceMap": true which are redundant; remove
the "outDir" property and either remove "sourceMap" or set it to false in the
tsconfig so that the compiler settings are not misleading (update the JSON keys
around "noEmit", "outDir", and "sourceMap" accordingly).
🧹 Nitpick comments (6)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@index.d.ts`: - Around line 656-667: The DetailedActivity interface currently declares laps as required (laps: Lap[]) while other collection fields like splits_metric, splits_standard, and best_efforts are optional; change laps to be optional (e.g., laps?: Lap[]) so the type matches cases where the Strava API omits laps (such as manual activities) and keeps consistency with the other optional fields in DetailedActivity. In `@lib/httpClient.js`: - Around line 305-321: The getCursorPaginationQS function uses an explicit undefined/null check for page_size but a truthy check for after_cursor, causing inconsistency and excluding valid empty-string cursors; update HttpClient.prototype.getCursorPaginationQS to set after_cursor the same way as page_size (e.g., compute after_cursor = typeof args.after_cursor !== 'undefined' ? args.after_cursor : null and then use if (after_cursor !== null) { qa.after_cursor = after_cursor }) so both parameters are handled consistently (refer to function name getCursorPaginationQS and local variables page_size, after_cursor, qa). In `@lib/oauth.js`: - Around line 75-88: The oauth.getToken implementation is sending sensitive credentials (client_secret, client_id, grant_type, code) via the options.qs (query string); change the HTTP options to put these parameters into the POST body instead of qs by replacing options.qs with options.data (or body) and ensure the request Content-Type is set appropriately so httpRequest/axiosUtility sends a form-encoded or JSON body as Strava expects; apply the same change to the refreshToken function so refresh_token and client_secret are moved from qs into the POST body when calling httpRequest. In `@lib/uploads.js`: - Around line 143-156: uploads.prototype._uploadIsDone currently treats any status other than the exact string 'Your activity is still being processed.' as complete; update this to explicitly recognize terminal success statuses (e.g., 'Your activity is ready.') and explicit error conditions by checking args.error or known error status strings so transient errors are not marked as done; modify _uploadIsDone to return false while args.error exists or status is a non-terminal/processing value and return true only for explicit terminal success statuses, using the function name uploads.prototype._uploadIsDone and the properties args.status and args.error to locate the logic to change. In `@test/pushSubscriptions.js`: - Around line 7-16: The tests in pushSubscriptions.js leave the singleton authenticator's configLoaded cache populated between tests; update the afterEach block to purge that cache by calling authenticator.purge() (or delegate to the existing testHelper.cleanupAuth() used elsewhere) so each test suite gets a fresh auth state; locate the afterEach in pushSubscriptions.js and add the call alongside the existing env cleanup and nock.cleanAll() to ensure isolation. In `@tsconfig.json`: - Around line 13-25: The tsconfig currently sets "noEmit": true while also specifying "outDir": "dist" and "sourceMap": true which are redundant; remove the "outDir" property and either remove "sourceMap" or set it to false in the tsconfig so that the compiler settings are not misleading (update the JSON keys around "noEmit", "outDir", and "sourceMap" accordingly).lib/oauth.js (1)
75-88: Credentials sent as query parameters viaqs.
client_secretis sent as a query parameter (via theqsoption which maps to URL params inaxiosUtility). While this appears to be the existing behavior and Strava accepts it, sending secrets in query strings is generally discouraged as they can appear in server logs and proxy caches. Consider moving these to the request body in a future iteration.♻️ Send credentials in the POST body instead of query params
const options = { method: 'POST', url: 'https://www.strava.com/oauth/token', - qs: { + body: { code: authorizationCode, client_secret: authenticator.getClientSecret(), client_id: authenticator.getClientId(), grant_type: 'authorization_code' } }The same change should be applied to
refreshTokenas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/oauth.js` around lines 75 - 88, The oauth.getToken implementation is sending sensitive credentials (client_secret, client_id, grant_type, code) via the options.qs (query string); change the HTTP options to put these parameters into the POST body instead of qs by replacing options.qs with options.data (or body) and ensure the request Content-Type is set appropriately so httpRequest/axiosUtility sends a form-encoded or JSON body as Strava expects; apply the same change to the refreshToken function so refresh_token and client_secret are moved from qs into the POST body when calling httpRequest.tsconfig.json (1)
13-25:outDirandsourceMapare redundant whennoEmit: trueis set.Since
noEmit: trueprevents any output,outDir: "dist"andsourceMap: truehave no effect. They're harmless but add noise.🧹 Proposed cleanup
// Emit "declaration": false, "declarationMap": false, "noEmit": true, - "outDir": "dist", // JavaScript Support "allowJs": true, "checkJs": true, // Language and Environment "target": "es2022", - "sourceMap": true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsconfig.json` around lines 13 - 25, The tsconfig currently sets "noEmit": true while also specifying "outDir": "dist" and "sourceMap": true which are redundant; remove the "outDir" property and either remove "sourceMap" or set it to false in the tsconfig so that the compiler settings are not misleading (update the JSON keys around "noEmit", "outDir", and "sourceMap" accordingly).lib/httpClient.js (1)
305-321: Same inconsistency pattern ingetCursorPaginationQS.
page_sizeuses an explicit null check (line 314, includes0) whileafter_cursoruses a truthy check (line 316, excludes empty string""). For a cursor string, the truthy check is likely fine in practice, but the pattern is inconsistent withpage_size.Consistency fix
- if (after_cursor) { qa.after_cursor = after_cursor } + if (after_cursor !== null) { qa.after_cursor = after_cursor }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/httpClient.js` around lines 305 - 321, The getCursorPaginationQS function uses an explicit undefined/null check for page_size but a truthy check for after_cursor, causing inconsistency and excluding valid empty-string cursors; update HttpClient.prototype.getCursorPaginationQS to set after_cursor the same way as page_size (e.g., compute after_cursor = typeof args.after_cursor !== 'undefined' ? args.after_cursor : null and then use if (after_cursor !== null) { qa.after_cursor = after_cursor }) so both parameters are handled consistently (refer to function name getCursorPaginationQS and local variables page_size, after_cursor, qa).index.d.ts (1)
656-667:lapsis still required while all other conditional fields are optional.Most fields in
DetailedActivitywere correctly made optional per previous feedback. However,lapson Line 665 remains required. The Strava API may omitlapsfor certain activity types (e.g., manual activities). Consider making it optional for safety, consistent with the treatment ofsplits_metric,splits_standard, andbest_efforts.Suggested change
- laps: Lap[]; + laps?: Lap[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.d.ts` around lines 656 - 667, The DetailedActivity interface currently declares laps as required (laps: Lap[]) while other collection fields like splits_metric, splits_standard, and best_efforts are optional; change laps to be optional (e.g., laps?: Lap[]) so the type matches cases where the Strava API omits laps (such as manual activities) and keeps consistency with the other optional fields in DetailedActivity.test/pushSubscriptions.js (1)
7-16: Consider callingauthenticator.purge()inafterEachfor test isolation.The authenticator is a singleton with a
configLoadedcache. Without purging inafterEach, cached credentials persist across test suites. Other test files (e.g.,test/activities.js) usetestHelper.cleanupAuth()which callspurge(). Missing this here could cause test-order-dependent failures if a later suite expects fresh config loading.Suggested fix
+const strava = require('../') + describe('pushSubscriptions_test', function () { beforeEach(function () { process.env.STRAVA_CLIENT_ID = 'test-client-id' process.env.STRAVA_CLIENT_SECRET = 'test-client-secret' }) afterEach(function () { delete process.env.STRAVA_CLIENT_ID delete process.env.STRAVA_CLIENT_SECRET nock.cleanAll() + // Reset authenticator cache so credentials don't leak to other suites + strava.config({}) // or require('../lib/authenticator').purge() })Note: strava is already imported on Line 4. You could directly import the authenticator or use whichever cleanup mechanism
testHelper.cleanupAuth()uses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/pushSubscriptions.js` around lines 7 - 16, The tests in pushSubscriptions.js leave the singleton authenticator's configLoaded cache populated between tests; update the afterEach block to purge that cache by calling authenticator.purge() (or delegate to the existing testHelper.cleanupAuth() used elsewhere) so each test suite gets a fresh auth state; locate the afterEach in pushSubscriptions.js and add the call alongside the existing env cleanup and nock.cleanAll() to ensure isolation.lib/uploads.js (1)
143-156:_uploadIsDonerelies on an exact status string — fragile but functional.The completion check matches only
'Your activity is still being processed.'as "not done" and treats everything else (including errors) as done. This means a transient error response from the API would also be treated as "done." Consider also checking for error states explicitly.Possible improvement
uploads.prototype._uploadIsDone = function (args) { - var isDone = false - - switch (args.status) { - case 'Your activity is still being processed.': - isDone = false - break - - default: - isDone = true - } - - return isDone + return args.status !== 'Your activity is still being processed.' }If you want to distinguish transient errors from completion, you could also check
args.erroror match against known terminal statuses like'Your activity is ready.'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/uploads.js` around lines 143 - 156, uploads.prototype._uploadIsDone currently treats any status other than the exact string 'Your activity is still being processed.' as complete; update this to explicitly recognize terminal success statuses (e.g., 'Your activity is ready.') and explicit error conditions by checking args.error or known error status strings so transient errors are not marked as done; modify _uploadIsDone to return false while args.error exists or status is a non-terminal/processing value and return true only for explicit terminal success statuses, using the function name uploads.prototype._uploadIsDone and the properties args.status and args.error to locate the logic to change.
…veWithFullResponse Updated the httpRequest function to optionally return the full response object, including headers and body, when the resolveWithFullResponse option is set to true. This change aligns with an earlier commit
…asynchronous errors
…ronous error handling
Wording was slightly stale
…ynchronous error handling
|
@markstos this should be ready again, although, code rabbit wanted alot done. Sorry for the lengthy review. |
2 participants
axiosUtility.js,index.d.tsSummary by CodeRabbit
New Features
Breaking Changes
Improvements