Merged
Conversation
WalkthroughThe PR refactors the Enkryptify SDK with a modular architecture: removes the old client implementation, introduces authentication providers (environment and token-based), adds in-memory caching with TTL, implements token exchange support, expands error handling, and includes a logging system. Configuration structure is redesigned for flexibility, and comprehensive test coverage is added. Changes
Sequence DiagramssequenceDiagram
participant Client as Application
participant Enkryptify
participant Cache as SecretCache
participant API as EnkryptifyApi
participant Auth as TokenExchangeManager
participant Store as Token Store
Client->>Enkryptify: new Enkryptify(config)
Enkryptify->>Store: storeToken(provider, token)
Enkryptify->>Cache: init cache (if enabled)
Note over Enkryptify: initialized with workspace,<br/>project, environment
Client->>Enkryptify: get(key)
alt cache enabled and hit
Enkryptify->>Cache: get(key)
Cache-->>Enkryptify: cached value
else cache miss or disabled
Enkryptify->>Auth: ensureToken()
Auth->>Store: retrieveToken()
alt token exchange enabled
Auth->>Auth: POST /v1/auth/exchange
Auth->>Store: storeToken(provider, jwt)
end
Enkryptify->>API: fetchSecret(workspace, project, key)
API->>API: fetch with Authorization header
API-->>Enkryptify: Secret
alt cache enabled
Enkryptify->>Cache: set(key, value)
end
end
Enkryptify-->>Client: value: string
sequenceDiagram
participant Client as Application
participant Enkryptify
participant Cache as SecretCache
participant API as EnkryptifyApi
Client->>Enkryptify: preload()
Enkryptify->>API: fetchAllSecrets(workspace, project, environment)
API-->>Enkryptify: Secret[]
loop for each secret
Enkryptify->>Cache: set(secretName, resolvedValue)
end
Enkryptify-->>Client: Promise resolved
Note over Client: subsequent gets hit cache<br/>without API calls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 7
🧹 Nitpick comments (5)
tests/api.test.ts (1)
97-105: Consider consolidating duplicate awaits to avoid re-executing the rejected promise.The test awaits
api.fetchAllSecrets()twice, which triggers two separate API calls due to how Promises work with.rejects. Eachexpect(...).rejects.toThrow()invocation re-runs the expression. This could cause flaky behavior if the mock state changes between calls.♻️ Proposed fix to use a single call
it("throws ApiError on 500 with status in message", async () => { fetchMock.mockResolvedValue( new Response("Internal Server Error", { status: 500, statusText: "Internal Server Error" }), ); const api = new EnkryptifyApi("https://api.example.com", createAuth("tok")); - await expect(api.fetchAllSecrets("ws", "prj", "env")).rejects.toThrow(ApiError); - await expect(api.fetchAllSecrets("ws", "prj", "env")).rejects.toThrow("HTTP 500"); + const promise = api.fetchAllSecrets("ws", "prj", "env"); + await expect(promise).rejects.toThrow(ApiError); + await expect(promise).rejects.toThrow("HTTP 500"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api.test.ts` around lines 97 - 105, The test calls api.fetchAllSecrets("ws","prj","env") twice causing two executions; change the test to call EnkryptifyApi.fetchAllSecrets once, store the resulting promise in a variable (e.g. const p = api.fetchAllSecrets(...)) and then assert on that single promise with await expect(p).rejects.toThrow(ApiError) and await expect(p).rejects.toThrow("HTTP 500") so the mocked response is consumed only once and both assertions check the same rejection.src/api.ts (1)
26-59: Consider handling JSON parse errors for robustness.If the server returns a non-JSON response with a 2xx status (e.g., empty body on 204, or malformed response),
response.json()will throw. While this is an edge case, wrapping the JSON parsing could provide a clearer error message.Also, the
Content-Type: application/jsonheader on GET requests is unnecessary (no request body), though it's harmless.♻️ Optional: Add JSON parse error handling
async `#request`<T>(method: string, endpoint: string): Promise<T> { const token = retrieveToken(this.#auth); const url = `${this.#baseUrl}${endpoint}`; const response = await fetch(url, { method, headers: { Authorization: `Bearer ${token}`, - "Content-Type": "application/json", }, }); // ... error handling unchanged ... - return response.json() as Promise<T>; + try { + return await response.json() as T; + } catch { + throw new ApiError(response.status, "Invalid JSON response", method, endpoint); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api.ts` around lines 26 - 59, The `#request`<T> method should guard against empty or invalid JSON responses: if response.status === 204 or response.headers.get("content-length") === "0" (or no body), return undefined as Promise<T>; otherwise attempt to parse JSON inside a try/catch around response.json() and on parse failure throw an ApiError (or wrap the original error) with details (status, method, endpoint and the JSON parse error message) so callers get a clear error; also optionally remove the "Content-Type" header for GET calls if desired.tests/enkryptify.test.ts (1)
170-177: Environment variable cleanup should useafterEachfor safety.If the test fails before Line 175, the environment variable may leak to subsequent tests. Consider using the same restoration pattern from the "token resolution" describe block.
♻️ Proposed fix using afterEach for cleanup
describe("Enkryptify.fromEnv()", () => { + const originalEnv = process.env.ENKRYPTIFY_TOKEN; + + afterEach(() => { + if (originalEnv !== undefined) { + process.env.ENKRYPTIFY_TOKEN = originalEnv; + } else { + delete process.env.ENKRYPTIFY_TOKEN; + } + }); + it("returns valid auth provider when env var is set", () => { process.env.ENKRYPTIFY_TOKEN = "ek_test"; const auth = Enkryptify.fromEnv(); expect(auth._brand).toBe("EnkryptifyAuthProvider"); - delete process.env.ENKRYPTIFY_TOKEN; }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/enkryptify.test.ts` around lines 170 - 177, The test mutates process.env.ENKRYPTIFY_TOKEN inline and deletes it inside the test, which can leak if the test fails; move the setup/teardown to use a beforeEach/afterEach pair (or at least afterEach) surrounding the "Enkryptify.fromEnv()" spec so that process.env.ENKRYPTIFY_TOKEN is set before the test and reliably deleted/restored in afterEach; update the test block referencing Enkryptify.fromEnv() to use afterEach cleanup (or restore original value) instead of deleting inside the it handler.src/auth.ts (1)
21-27: Consider adding empty token validation for consistency withEnvAuthProvider.
TokenAuthProvideraccepts any token string without validation, whileEnvAuthProvidervalidates that the token is not empty. If an empty string is passed, the error will only surface later during API calls viaretrieveToken. For consistency and fail-fast behavior, consider validating the token here.Note: If token format validation is intentionally deferred to
Enkryptify.#validateTokenFormat, this is acceptable—just ensure the public API documentation clarifies this.♻️ Proposed fix to add validation
export class TokenAuthProvider implements EnkryptifyAuthProvider { readonly _brand = "EnkryptifyAuthProvider" as const; constructor(token: string) { + if (!token) { + throw new EnkryptifyError( + "Token must be a non-empty string.\n" + + "Docs: https://docs.enkryptify.com/sdk/auth", + ); + } storeToken(this, token); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth.ts` around lines 21 - 27, TokenAuthProvider currently stores any token without validation; add the same empty-string check used by EnvAuthProvider in the TokenAuthProvider constructor so it throws early for an empty or all-whitespace token, then call storeToken(this, token) only after the check; reference TokenAuthProvider, EnvAuthProvider, and storeToken when making the change (or, if you intentionally defer format validation to Enkryptify.#validateTokenFormat, add a short comment documenting that decision instead of the check).src/enkryptify.ts (1)
121-141: Makecache: falsebehavior consistent.Line 124 disables cache reads, but Lines 260-262 still write the fetched value back, so a one-off cache bypass mutates local state anyway. If
cache: falseis meant as a true per-call bypass, thread the flag through to the single-fetch path; otherwise rename/document it as read-only.♻️ Proposed refactor
async get(key: string, options?: { cache?: boolean }): Promise<string> { this.#guardDestroyed(); const useCache = this.#cacheEnabled && options?.cache !== false; @@ - return this.#fetchAndCacheSingle(key); + return this.#fetchAndCacheSingle(key, useCache); } @@ - async `#fetchAndCacheSingle`(key: string): Promise<string> { + async `#fetchAndCacheSingle`(key: string, cacheResult = true): Promise<string> { @@ - if (this.#cacheEnabled && this.#cache) { + if (cacheResult && this.#cacheEnabled && this.#cache) { this.#cache.set(key, value); }Also applies to: 238-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/enkryptify.ts` around lines 121 - 141, The get method computes useCache from options.cache but still allows fetched values to be written back; fix by threading the per-call cache flag into the fetch paths (get → `#fetchAndCacheSingle` and get → `#fetchAndCacheAll`) as a parameter like writeToCache (or a boolean cacheWrite), and update those methods to honor it (only write to this.#cache or set this.#eagerLoaded when writeToCache is true). Locate usages in get, `#fetchAndCacheSingle`, and `#fetchAndCacheAll` and ensure the flag is passed and checked before any mutations of this.#cache or eager-loaded state so cache: false truly bypasses local writes (alternatively, rename options.cache to readOnlyCache and update callers/docs if you prefer the other semantics).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 222-223: The README's "Typecheck" section currently shows the
command "pnpm typecheck" which conflicts with CONTRIBUTING.md's "pnpm check";
decide which command is canonical and make them consistent—e.g., change the
README's "pnpm typecheck" line to "pnpm check" (or update CONTRIBUTING.md
instead) and also verify the corresponding npm script exists in package.json
(script name "check" or "typecheck") so both docs match the actual script.
In `@src/cache.ts`:
- Around line 18-20: Change the expiration check in the cache to treat entries
as expired when Date.now() is greater than or equal to entry.expiresAt (not
strictly greater), so that ttl = 0 and exact-boundary reads don't return stale
data; update the condition in the code that references entry.expiresAt,
Date.now(), this.#store.delete(key) and the early return to use >= while leaving
the null check for entry.expiresAt unchanged.
- Around line 42-43: The size getter currently returns this.#store.size without
removing expired items, so update the Cache class to prune expired entries
before reporting size: implement or reuse a helper like pruneExpiredEntries()
(or add expiry cleanup logic used by get()/has()) and call it at the start of
the size() getter so expired entries are removed from this.#store before
returning the count.
In `@src/enkryptify.ts`:
- Around line 144-157: getFromCache currently throws SecretNotFoundError on a
local cache miss which is incorrect; change the behavior so a cache miss is
reported with a distinct error (e.g., CacheMissError or CacheMiss) instead of
SecretNotFoundError, or alternately return undefined and update the method
signature to allow optional returns; specifically modify getFromCache to detect
value === undefined and either throw a new CacheMissError(key) (define
CacheMissError class) or return undefined, leaving SecretNotFoundError reserved
for real backend misses and keeping EnkryptifyError usage unchanged for
config/disabled-cache cases.
In `@src/errors.ts`:
- Around line 55-60: The RateLimitError constructor currently only parses
numeric delta-seconds and treats 0 as falsy, so update the constructor in
RateLimitError (errors.ts) to support both delta-seconds and HTTP-date formats:
first try parseInt(retryAfter, 10) and accept 0 as valid (check for
Number.isFinite or !Number.isNaN), and if that fails attempt
Date.parse(retryAfter) to compute seconds = Math.ceil((parsedDate -
Date.now())/1000) (ensure non-negative); set retrySeconds to null only if
neither parse yields a valid non-negative integer, and use retrySeconds !== null
when building the message so a 0 value shows "Retry after 0 seconds." instead of
the generic text.
In `@src/token-exchange.ts`:
- Around line 69-79: The scheduled refresh in `#scheduleRefresh` sets a timeout
that calls this.#exchange() without awaiting it, causing floating promises and
possible unhandled rejections; change the timeout callback to handle the promise
(either make the callback async and await this.#exchange() inside a try/catch,
or call this.#exchange().catch(err => {/*log/handle*/})), keep the this.#jwt =
null behavior, and ensure any errors are logged/handled so rejections from
`#exchange`() do not go unhandled; leave the `#refreshTimer.unref`() behavior
intact.
- Around line 56-58: The code currently computes refreshMs = (data.expiresIn -
60) * 1000 and calls this.#scheduleRefresh(refreshMs), which can be
zero/negative when data.expiresIn <= 60; clamp the computed delay to a safe
minimum before scheduling to prevent immediate or past-due refresh loops by
introducing a MIN_REFRESH_DELAY_SECONDS (or MS) constant and using something
like refreshSeconds = Math.max(data.expiresIn - 60, MIN_REFRESH_DELAY_SECONDS)
(or equivalent in ms) and then call this.#scheduleRefresh(refreshSeconds *
1000); ensure you reference and use data.expiresIn, refreshMs (or the renamed
variable), and this.#scheduleRefresh when implementing the change.
---
Nitpick comments:
In `@src/api.ts`:
- Around line 26-59: The `#request`<T> method should guard against empty or
invalid JSON responses: if response.status === 204 or
response.headers.get("content-length") === "0" (or no body), return undefined as
Promise<T>; otherwise attempt to parse JSON inside a try/catch around
response.json() and on parse failure throw an ApiError (or wrap the original
error) with details (status, method, endpoint and the JSON parse error message)
so callers get a clear error; also optionally remove the "Content-Type" header
for GET calls if desired.
In `@src/auth.ts`:
- Around line 21-27: TokenAuthProvider currently stores any token without
validation; add the same empty-string check used by EnvAuthProvider in the
TokenAuthProvider constructor so it throws early for an empty or all-whitespace
token, then call storeToken(this, token) only after the check; reference
TokenAuthProvider, EnvAuthProvider, and storeToken when making the change (or,
if you intentionally defer format validation to Enkryptify.#validateTokenFormat,
add a short comment documenting that decision instead of the check).
In `@src/enkryptify.ts`:
- Around line 121-141: The get method computes useCache from options.cache but
still allows fetched values to be written back; fix by threading the per-call
cache flag into the fetch paths (get → `#fetchAndCacheSingle` and get →
`#fetchAndCacheAll`) as a parameter like writeToCache (or a boolean cacheWrite),
and update those methods to honor it (only write to this.#cache or set
this.#eagerLoaded when writeToCache is true). Locate usages in get,
`#fetchAndCacheSingle`, and `#fetchAndCacheAll` and ensure the flag is passed and
checked before any mutations of this.#cache or eager-loaded state so cache:
false truly bypasses local writes (alternatively, rename options.cache to
readOnlyCache and update callers/docs if you prefer the other semantics).
In `@tests/api.test.ts`:
- Around line 97-105: The test calls api.fetchAllSecrets("ws","prj","env") twice
causing two executions; change the test to call EnkryptifyApi.fetchAllSecrets
once, store the resulting promise in a variable (e.g. const p =
api.fetchAllSecrets(...)) and then assert on that single promise with await
expect(p).rejects.toThrow(ApiError) and await expect(p).rejects.toThrow("HTTP
500") so the mocked response is consumed only once and both assertions check the
same rejection.
In `@tests/enkryptify.test.ts`:
- Around line 170-177: The test mutates process.env.ENKRYPTIFY_TOKEN inline and
deletes it inside the test, which can leak if the test fails; move the
setup/teardown to use a beforeEach/afterEach pair (or at least afterEach)
surrounding the "Enkryptify.fromEnv()" spec so that process.env.ENKRYPTIFY_TOKEN
is set before the test and reliably deleted/restored in afterEach; update the
test block referencing Enkryptify.fromEnv() to use afterEach cleanup (or restore
original value) instead of deleting inside the it handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Lite
Run ID: effd1d96-6c69-4eef-91cc-063d6d8b534f
📒 Files selected for processing (22)
.github/workflows/release.yml.oxlintrc.json.releaserc.jsonCONTRIBUTING.mdREADME.mdsrc/api.tssrc/auth.tssrc/cache.tssrc/client.tssrc/enkryptify.tssrc/errors.tssrc/index.tssrc/internal/token-store.tssrc/logger.tssrc/token-exchange.tssrc/types.tstests/api.test.tstests/auth.test.tstests/cache.test.tstests/client.test.tstests/enkryptify.test.tstests/logger.test.ts
💤 Files with no reviewable changes (2)
- src/client.ts
- tests/client.test.ts
Comment on lines
222
to
+223
| # Typecheck | ||
| pnpm check | ||
| pnpm typecheck |
There was a problem hiding this comment.
Keep the documented typecheck command consistent.
This now diverges from CONTRIBUTING.md, which still tells contributors to run pnpm check. One of the two docs is stale, so following both instructions will fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 222 - 223, The README's "Typecheck" section currently
shows the command "pnpm typecheck" which conflicts with CONTRIBUTING.md's "pnpm
check"; decide which command is canonical and make them consistent—e.g., change
the README's "pnpm typecheck" line to "pnpm check" (or update CONTRIBUTING.md
instead) and also verify the corresponding npm script exists in package.json
(script name "check" or "typecheck") so both docs match the actual script.
Comment on lines
+18
to
+20
| if (entry.expiresAt !== null && Date.now() > entry.expiresAt) { | ||
| this.#store.delete(key); | ||
| return undefined; |
There was a problem hiding this comment.
Expire entries exactly at the TTL boundary.
Using > lets a value survive when Date.now() equals expiresAt, so ttl = 0 and exact-boundary reads can return stale data for one extra tick.
🛠️ Proposed fix
- if (entry.expiresAt !== null && Date.now() > entry.expiresAt) {
+ if (entry.expiresAt !== null && Date.now() >= entry.expiresAt) {
this.#store.delete(key);
return undefined;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (entry.expiresAt !== null && Date.now() > entry.expiresAt) { | |
| this.#store.delete(key); | |
| return undefined; | |
| if (entry.expiresAt !== null && Date.now() >= entry.expiresAt) { | |
| this.#store.delete(key); | |
| return undefined; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cache.ts` around lines 18 - 20, Change the expiration check in the cache
to treat entries as expired when Date.now() is greater than or equal to
entry.expiresAt (not strictly greater), so that ttl = 0 and exact-boundary reads
don't return stale data; update the condition in the code that references
entry.expiresAt, Date.now(), this.#store.delete(key) and the early return to use
>= while leaving the null check for entry.expiresAt unchanged.
Comment on lines
+42
to
+43
| get size(): number { | ||
| return this.#store.size; |
There was a problem hiding this comment.
Prune expired entries before reporting size.
size reads the raw Map count, but expiry is only enforced in get() and has(). With a finite TTL, expired entries stay counted until each key is touched again.
🛠️ Proposed fix
get size(): number {
+ const now = Date.now();
+ for (const [key, entry] of this.#store) {
+ if (entry.expiresAt !== null && now >= entry.expiresAt) {
+ this.#store.delete(key);
+ }
+ }
return this.#store.size;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| get size(): number { | |
| return this.#store.size; | |
| get size(): number { | |
| const now = Date.now(); | |
| for (const [key, entry] of this.#store) { | |
| if (entry.expiresAt !== null && now >= entry.expiresAt) { | |
| this.#store.delete(key); | |
| } | |
| } | |
| return this.#store.size; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cache.ts` around lines 42 - 43, The size getter currently returns
this.#store.size without removing expired items, so update the Cache class to
prune expired entries before reporting size: implement or reuse a helper like
pruneExpiredEntries() (or add expiry cleanup logic used by get()/has()) and call
it at the start of the size() getter so expired entries are removed from
this.#store before returning the count.
Comment on lines
+144
to
+157
| getFromCache(key: string): string { | ||
| this.#guardDestroyed(); | ||
|
|
||
| if (!this.#cacheEnabled || !this.#cache) { | ||
| throw new EnkryptifyError( | ||
| "Cache is disabled. Enable caching in the config or use get() to fetch from the API.\n" + | ||
| "Docs: https://docs.enkryptify.com/sdk/configuration#caching", | ||
| ); | ||
| } | ||
|
|
||
| const value = this.#cache.get(key); | ||
| if (value === undefined) { | ||
| throw new SecretNotFoundError(key, this.#workspace, this.#environment); | ||
| } |
There was a problem hiding this comment.
Don't report a local cache miss as SecretNotFoundError.
getFromCache() never hits the API, but Lines 155-156 currently throw an error that says the secret is missing from the workspace/environment. On a cold or expired cache that is simply untrue and makes downstream error handling impossible to distinguish from a real backend miss.
🩹 Proposed fix
const value = this.#cache.get(key);
if (value === undefined) {
- throw new SecretNotFoundError(key, this.#workspace, this.#environment);
+ throw new EnkryptifyError(
+ `Secret "${key}" is not present in the local cache. Call get() or preload() first.\n` +
+ "Docs: https://docs.enkryptify.com/sdk/configuration#caching",
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/enkryptify.ts` around lines 144 - 157, getFromCache currently throws
SecretNotFoundError on a local cache miss which is incorrect; change the
behavior so a cache miss is reported with a distinct error (e.g., CacheMissError
or CacheMiss) instead of SecretNotFoundError, or alternately return undefined
and update the method signature to allow optional returns; specifically modify
getFromCache to detect value === undefined and either throw a new
CacheMissError(key) (define CacheMissError class) or return undefined, leaving
SecretNotFoundError reserved for real backend misses and keeping EnkryptifyError
usage unchanged for config/disabled-cache cases.
Comment on lines
+55
to
+60
| constructor(retryAfter?: string | null) { | ||
| const parsed = retryAfter ? parseInt(retryAfter, 10) : null; | ||
| const retrySeconds = parsed !== null && !Number.isNaN(parsed) ? parsed : null; | ||
| super( | ||
| `Rate limited (HTTP 429). ` + | ||
| `${retrySeconds ? `Retry after ${retrySeconds} seconds.` : "Please retry later."}\n` + |
There was a problem hiding this comment.
Parse both forms of Retry-After.
Line 56 only handles numeric delta-seconds. Retry-After can also be an HTTP-date, and the truthy check on Line 60 also turns a valid 0 into the generic “Please retry later.” message.
🩹 Proposed fix
constructor(retryAfter?: string | null) {
- const parsed = retryAfter ? parseInt(retryAfter, 10) : null;
- const retrySeconds = parsed !== null && !Number.isNaN(parsed) ? parsed : null;
+ let retrySeconds: number | null = null;
+ if (retryAfter) {
+ const seconds = Number.parseInt(retryAfter, 10);
+ if (!Number.isNaN(seconds)) {
+ retrySeconds = Math.max(0, seconds);
+ } else {
+ const retryDate = Date.parse(retryAfter);
+ if (!Number.isNaN(retryDate)) {
+ retrySeconds = Math.max(0, Math.ceil((retryDate - Date.now()) / 1000));
+ }
+ }
+ }
super(
`Rate limited (HTTP 429). ` +
- `${retrySeconds ? `Retry after ${retrySeconds} seconds.` : "Please retry later."}\n` +
+ `${retrySeconds !== null ? `Retry after ${retrySeconds} seconds.` : "Please retry later."}\n` +
"Docs: https://docs.enkryptify.com/sdk/troubleshooting#rate-limiting",
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| constructor(retryAfter?: string | null) { | |
| const parsed = retryAfter ? parseInt(retryAfter, 10) : null; | |
| const retrySeconds = parsed !== null && !Number.isNaN(parsed) ? parsed : null; | |
| super( | |
| `Rate limited (HTTP 429). ` + | |
| `${retrySeconds ? `Retry after ${retrySeconds} seconds.` : "Please retry later."}\n` + | |
| constructor(retryAfter?: string | null) { | |
| let retrySeconds: number | null = null; | |
| if (retryAfter) { | |
| const seconds = Number.parseInt(retryAfter, 10); | |
| if (!Number.isNaN(seconds)) { | |
| retrySeconds = Math.max(0, seconds); | |
| } else { | |
| const retryDate = Date.parse(retryAfter); | |
| if (!Number.isNaN(retryDate)) { | |
| retrySeconds = Math.max(0, Math.ceil((retryDate - Date.now()) / 1000)); | |
| } | |
| } | |
| } | |
| super( | |
| `Rate limited (HTTP 429). ` + | |
| `${retrySeconds !== null ? `Retry after ${retrySeconds} seconds.` : "Please retry later."}\n` + | |
| "Docs: https://docs.enkryptify.com/sdk/troubleshooting#rate-limiting", | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/errors.ts` around lines 55 - 60, The RateLimitError constructor currently
only parses numeric delta-seconds and treats 0 as falsy, so update the
constructor in RateLimitError (errors.ts) to support both delta-seconds and
HTTP-date formats: first try parseInt(retryAfter, 10) and accept 0 as valid
(check for Number.isFinite or !Number.isNaN), and if that fails attempt
Date.parse(retryAfter) to compute seconds = Math.ceil((parsedDate -
Date.now())/1000) (ensure non-negative); set retrySeconds to null only if
neither parse yields a valid non-negative integer, and use retrySeconds !== null
when building the message so a 0 value shows "Retry after 0 seconds." instead of
the generic text.
Comment on lines
+56
to
+58
| // Refresh 60 seconds before expiry | ||
| const refreshMs = (data.expiresIn - 60) * 1000; | ||
| this.#scheduleRefresh(refreshMs); |
There was a problem hiding this comment.
Handle edge case where expiresIn is less than or equal to 60 seconds.
If the server returns an expiresIn of 60 or less, refreshMs becomes zero or negative, causing immediate or past-due refresh scheduling. This could trigger rapid consecutive refresh attempts.
🛡️ Proposed fix to add a minimum refresh delay
// Refresh 60 seconds before expiry
- const refreshMs = (data.expiresIn - 60) * 1000;
- this.#scheduleRefresh(refreshMs);
+ const refreshMs = Math.max((data.expiresIn - 60) * 1000, 5000); // At least 5s
+ this.#scheduleRefresh(refreshMs);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Refresh 60 seconds before expiry | |
| const refreshMs = (data.expiresIn - 60) * 1000; | |
| this.#scheduleRefresh(refreshMs); | |
| // Refresh 60 seconds before expiry | |
| const refreshMs = Math.max((data.expiresIn - 60) * 1000, 5000); // At least 5s | |
| this.#scheduleRefresh(refreshMs); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/token-exchange.ts` around lines 56 - 58, The code currently computes
refreshMs = (data.expiresIn - 60) * 1000 and calls
this.#scheduleRefresh(refreshMs), which can be zero/negative when data.expiresIn
<= 60; clamp the computed delay to a safe minimum before scheduling to prevent
immediate or past-due refresh loops by introducing a MIN_REFRESH_DELAY_SECONDS
(or MS) constant and using something like refreshSeconds =
Math.max(data.expiresIn - 60, MIN_REFRESH_DELAY_SECONDS) (or equivalent in ms)
and then call this.#scheduleRefresh(refreshSeconds * 1000); ensure you reference
and use data.expiresIn, refreshMs (or the renamed variable), and
this.#scheduleRefresh when implementing the change.
Comment on lines
+69
to
+79
| #scheduleRefresh(ms: number): void { | ||
| if (this.#refreshTimer) { | ||
| clearTimeout(this.#refreshTimer); | ||
| } | ||
| this.#refreshTimer = setTimeout(() => { | ||
| this.#jwt = null; | ||
| this.#exchange(); | ||
| }, ms); | ||
| // Don't keep the process alive just for token refresh | ||
| this.#refreshTimer.unref?.(); | ||
| } |
There was a problem hiding this comment.
Unhandled promise rejection in scheduled refresh.
The #exchange() call inside setTimeout is not awaited, creating a floating promise. If the refresh fails, the rejection won't be caught, potentially causing unhandled rejection warnings in Node.js.
🛡️ Proposed fix to handle the promise
`#scheduleRefresh`(ms: number): void {
if (this.#refreshTimer) {
clearTimeout(this.#refreshTimer);
}
this.#refreshTimer = setTimeout(() => {
this.#jwt = null;
- this.#exchange();
+ this.#exchange().catch(() => {
+ // Error already logged and handled in `#exchange`
+ });
}, ms);
// Don't keep the process alive just for token refresh
this.#refreshTimer.unref?.();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #scheduleRefresh(ms: number): void { | |
| if (this.#refreshTimer) { | |
| clearTimeout(this.#refreshTimer); | |
| } | |
| this.#refreshTimer = setTimeout(() => { | |
| this.#jwt = null; | |
| this.#exchange(); | |
| }, ms); | |
| // Don't keep the process alive just for token refresh | |
| this.#refreshTimer.unref?.(); | |
| } | |
| `#scheduleRefresh`(ms: number): void { | |
| if (this.#refreshTimer) { | |
| clearTimeout(this.#refreshTimer); | |
| } | |
| this.#refreshTimer = setTimeout(() => { | |
| this.#jwt = null; | |
| this.#exchange().catch(() => { | |
| // Error already logged and handled in `#exchange` | |
| }); | |
| }, ms); | |
| // Don't keep the process alive just for token refresh | |
| this.#refreshTimer.unref?.(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/token-exchange.ts` around lines 69 - 79, The scheduled refresh in
`#scheduleRefresh` sets a timeout that calls this.#exchange() without awaiting it,
causing floating promises and possible unhandled rejections; change the timeout
callback to handle the promise (either make the callback async and await
this.#exchange() inside a try/catch, or call this.#exchange().catch(err =>
{/*log/handle*/})), keep the this.#jwt = null behavior, and ensure any errors
are logged/handled so rejections from `#exchange`() do not go unhandled; leave the
`#refreshTimer.unref`() behavior intact.
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 by CodeRabbit
Release Notes
New Features
getFromCache(),preload(), anddestroy()Documentation
Chores