Conversation
…bleIteratorLinkPlugin
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
the
size:L
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new configurable option, refreshTokenDelayInSeconds, to DurableIteratorLinkPlugin and integrates it into token refresh and retry scheduling, replacing a fixed 2000ms retry. Updates tests to use vitest fake timers and exercise additional delay-driven refresh cycles. Also performs a minor docs cleanup in DurableIteratorOptions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant P as DurableIteratorLinkPlugin
participant TP as Token Provider
participant WS as WebSocket
C->>P: instantiate({ url, refreshTokenBeforeExpireInSeconds, refreshTokenDelayInSeconds? })
P->>TP: request initial token
TP-->>P: return token
P->>WS: connect(url, token)
P->>P: scheduleRefresh(timeout = max(delayMs, timeToExpiry))
alt Proactive refresh (timeToExpiry >= delayMs)
P->>TP: refresh token
TP-->>P: new token
P->>WS: update/reconnect with new token
else Retry path (delay-driven)
Note over P: wait delayMs
P->>TP: retry refresh token
TP-->>P: token or failure
opt Success
P->>WS: update/reconnect with new token
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a refreshTokenDelayInSeconds option to the DurableIteratorLinkPlugin, allowing control over the minimum delay between token refresh attempts. The implementation correctly adds the option and uses it to schedule subsequent refresh operations. The tests have also been updated to cover the new functionality. I have one suggestion regarding the use of this new option to also control the retry delay for failed refresh attempts, which could lead to unexpected behavior.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
More templates
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/durable-iterator/src/client/plugin.ts (3)
55-60: Clarify and bound the delay option (doc).State that values must be non‑negative, and that non‑finite or negative values fall back to the default (2s).
/** * Minimum delay between token refresh attempts. * - * @default 2 (seconds) + * Must be non-negative. Non-finite or negative values are treated as the default (2 seconds). + * + * @default 2 (seconds) */ refreshTokenDelayInSeconds?: Value<Promisable<number>, [tokenPayload: DurableIteratorTokenPayload, options: StandardLinkInterceptorOptions<T>]>
138-141: Sanitize delay to avoid NaN/negative values causing tight or undefined retry loops.Clamp to a safe default when user-supplied resolver returns non‑finite or negative.
- const delayMilliseconds = await value(this.refreshTokenDelayInSeconds, tokenAndPayload.payload, options) * 1000 + const rawDelaySeconds = await value(this.refreshTokenDelayInSeconds, tokenAndPayload.payload, options) + const delayMilliseconds = (Number.isFinite(rawDelaySeconds) && rawDelaySeconds >= 0 ? rawDelaySeconds : 2) * 1000
146-193: Avoid work after teardown: short‑circuit the refresh cycle when finished.When the iterator is returned/aborted, the timer callback may still run and perform updateToken/reconnect. Add early returns to avoid post‑teardown side effects.
refreshTokenBeforeExpireTimeoutId = setTimeout( - async () => { + async () => { + if (isFinished) return // retry until success or finished const newTokenAndPayload = await retry({ times: Number.POSITIVE_INFINITY, delay: delayMilliseconds }, async (exit) => { try { const output = await next() return this.validateToken(output, options.path) } catch (err) { if (isFinished) { exit(err) } throw err } }) const canProactivelyUpdateToken = newTokenAndPayload.payload.chn === tokenAndPayload.payload.chn && stringifyJSON(newTokenAndPayload.payload.tags) === stringifyJSON(tokenAndPayload.payload.tags) tokenAndPayload = newTokenAndPayload await refreshTokenBeforeExpire() // recursively call + if (isFinished) return /** * The next refresh cycle doesn't depend on the logic below, * so we place it last to avoid interfering with recursion. */ if (canProactivelyUpdateToken) { /** * Proactively update the token before expiration * to avoid reconnecting when the old token expires. */ await durableClient.updateToken({ token: tokenAndPayload.token }) } else { /** * Proactive update requires the same channel and tags. * If they differ, we must reconnect instead to make new token effective. */ websocket.reconnect() } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/durable-iterator/src/client/plugin.test.ts(4 hunks)packages/durable-iterator/src/client/plugin.ts(3 hunks)packages/durable-iterator/src/iterator.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/durable-iterator/src/iterator.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/durable-iterator/src/client/plugin.ts (4)
packages/shared/src/value.ts (3)
Value(1-1)fallback(17-19)value(3-12)packages/durable-iterator/src/schemas.ts (1)
DurableIteratorTokenPayload(6-6)packages/client/src/adapters/standard/link.ts (1)
StandardLinkInterceptorOptions(9-12)packages/shared/src/json.ts (1)
stringifyJSON(9-12)
packages/durable-iterator/src/client/plugin.test.ts (4)
packages/shared/src/index.ts (1)
sleep(22-22)packages/durable-iterator/src/client/iterator.ts (1)
getClientDurableIteratorToken(74-80)packages/durable-iterator/src/schemas.ts (1)
parseDurableIteratorToken(52-60)packages/durable-iterator/src/consts.ts (1)
DURABLE_ITERATOR_TOKEN_PARAM(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: publish-commit
- GitHub Check: lint
🔇 Additional comments (4)
packages/durable-iterator/src/client/plugin.ts (1)
77-86: Constructor changes look good.New option is threaded through with a sensible default of 2s.
packages/durable-iterator/src/client/plugin.test.ts (3)
37-66: Test wiring LGTM.New mock and plugin option are correctly introduced and exercised.
188-201: Timing assertions align with new min‑delay semantics.Shorter pre‑expiry waits (2×500ms) and a 2s delay for the extra cycle match the implementation.
211-220: Good coverage: asserts delay resolver is invoked.Verifying both call count and arguments strengthens confidence in the new option.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/durable-iterator/src/client/plugin.ts (3)
55-61: Good option add; clamp to non-negative to avoid busy-loop/instant retriesIf a user returns 0 or a negative value, the retry loop below will spin too fast or schedule immediately. Clamp to >= 0 when materializing milliseconds.
Apply this diff at the usage site:
-const delayMilliseconds = await value(this.refreshTokenDelayInSeconds, tokenAndPayload.payload, options) * 1000 +const delayMilliseconds = Math.max( + 0, + (await value(this.refreshTokenDelayInSeconds, tokenAndPayload.payload, options)) * 1000, +)
139-140: Materialize delay as non-negative millisecondsStrengthen against negative returns and fractional seconds.
Apply:
-const delayMilliseconds = await value(this.refreshTokenDelayInSeconds, tokenAndPayload.payload, options) * 1000 +const delayMilliseconds = Math.max( + 0, + (await value(this.refreshTokenDelayInSeconds, tokenAndPayload.payload, options)) * 1000, +)
189-193: Guard against negative “time until refresh” and improve readabilityCompute msUntilRefresh once and clamp to >= 0; then take max with the min spacing. This avoids negative setTimeout delays if clocks skew or payload exp is near.
Apply:
- Math.max( - refreshTokenBeforeExpireTimeoutId === undefined ? 0 : delayMilliseconds, - ((tokenAndPayload.payload.exp - beforeSeconds) * 1000) - Date.now(), - ), + Math.max( + refreshTokenBeforeExpireTimeoutId === undefined ? 0 : delayMilliseconds, + Math.max(((tokenAndPayload.payload.exp - beforeSeconds) * 1000) - Date.now(), 0), + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/durable-iterator/src/client/plugin.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/durable-iterator/src/client/plugin.ts (4)
packages/shared/src/value.ts (3)
Value(1-1)fallback(17-19)value(3-12)packages/durable-iterator/src/schemas.ts (1)
DurableIteratorTokenPayload(6-6)packages/client/src/adapters/standard/link.ts (1)
StandardLinkInterceptorOptions(9-12)packages/shared/src/json.ts (1)
stringifyJSON(9-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: publish-commit
🔇 Additional comments (2)
packages/durable-iterator/src/client/plugin.ts (2)
77-86: Wiring and defaults look correctPrivate field and constructor wiring for refreshTokenDelayInSeconds with default 2s are consistent with docs.
149-161: Decouple retry delay from min refresh spacingUsing refreshTokenDelayInSeconds for network retry delay changes semantics; long spacing (e.g., 60s) would slow retries for a single failed refresh. Keep retry delay at 2000ms as before, and reserve refreshTokenDelayInSeconds only for scheduling the next refresh cycle.
Apply:
-const newTokenAndPayload = await retry({ times: Number.POSITIVE_INFINITY, delay: delayMilliseconds }, async (exit) => { +const newTokenAndPayload = await retry({ times: Number.POSITIVE_INFINITY, delay: 2000 }, async (exit) => {
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/durable-iterator/src/client/plugin.test.ts (2)
40-41: New option is wired correctly; add a stronger assertion.
Looks good. Consider asserting the last call torefreshTokenDelayInSecondsuses the latest token (e.g., fromurl3) to ensure scheduling uses the refreshed payload each cycle.Example:
expect(refreshTokenDelayInSeconds).toHaveBeenLastCalledWith( parseDurableIteratorToken(new URL(url3).searchParams.get(DURABLE_ITERATOR_TOKEN_PARAM)!), expect.objectContaining({ path: ['durableIterator'] }), )Also applies to: 66-69
340-345: Don’t register afterEach inside a test; useprocess.onceor local cleanup.
RegisteringafterEachduring test execution is brittle. Preferprocess.onceor a try/finally.Apply this diff:
-const unhandledRejectionHandler = vi.fn() -process.on('unhandledRejection', unhandledRejectionHandler) -afterEach(() => { - process.off('unhandledRejection', unhandledRejectionHandler) -}) +const unhandledRejectionHandler = vi.fn() +process.once('unhandledRejection', unhandledRejectionHandler)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/durable-iterator/src/client/plugin.test.ts(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/durable-iterator/src/client/plugin.test.ts (3)
packages/durable-iterator/src/client/iterator.ts (1)
getClientDurableIteratorToken(74-80)packages/durable-iterator/src/schemas.ts (1)
parseDurableIteratorToken(52-60)packages/durable-iterator/src/consts.ts (1)
DURABLE_ITERATOR_TOKEN_PARAM(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: publish-commit
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (8)
packages/durable-iterator/src/client/plugin.test.ts (8)
7-7: LGTM: Vitest imports are appropriate for fake-timer driven tests.
16-17: Good call capturing the real setTimeout.
This avoids deadlocks while fake timers are active.
203-232: LGTM: Refresh flow assertions align with delay-based scheduling.
The staged advances and token/url changes validate both before-expire and delay timers.Also applies to: 233-242
278-278: LGTM: Explicitly disabling refresh via NaN is covered.
321-336: LGTM: Retry path for invalid tokens is asserted clearly.
Counts and URL stability checks look right.
351-354: LGTM: Ensures in-flight retry rejects after return and is surfaced via unhandledRejection.
The sequencing with timers and real waits is appropriate.Also applies to: 356-358
395-408: LGTM: Reconnect on channel mismatch is validated and timers resume.
444-461: LGTM: Reconnect on tags mismatch is validated and timers resume.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/durable-iterator/src/client/plugin.test.ts (1)
340-345: Do not register afterEach inside a test; leaks unhandledRejection handler.Defining
afterEachinside anitdoes not clean up for the current test and leaves a process-level listener attached, risking cross-test interference.Apply this diff to remove the nested hook:
- const unhandledRejectionHandler = vi.fn() - process.on('unhandledRejection', unhandledRejectionHandler) - afterEach(() => { - process.off('unhandledRejection', unhandledRejectionHandler) - }) + const unhandledRejectionHandler = vi.fn() + process.on('unhandledRejection', unhandledRejectionHandler)Then, ensure explicit cleanup at the end of this test (right before it returns). Add:
process.off('unhandledRejection', unhandledRejectionHandler)I can provide a full patch wrapping the body in try/finally if you prefer.
🧹 Nitpick comments (1)
packages/durable-iterator/src/client/plugin.test.ts (1)
16-17: Avoid saving real setTimeout; prefer Vitest timer APIs to prevent real 1s sleeps.Capturing and using realSetTimeout slows tests and can introduce flakiness. Prefer
vi.runAllTimersAsync()(orvi.advanceTimersByTimeAsync) to flush pending timers/microtasks under fake timers.Apply this diff to drop the saved real timer:
-const realSetTimeout = globalThis.setTimeout -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/durable-iterator/src/client/plugin.test.ts(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/durable-iterator/src/client/plugin.test.ts (3)
packages/durable-iterator/src/client/iterator.ts (1)
getClientDurableIteratorToken(74-80)packages/durable-iterator/src/schemas.ts (1)
parseDurableIteratorToken(52-60)packages/durable-iterator/src/consts.ts (1)
DURABLE_ITERATOR_TOKEN_PARAM(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: publish-commit
- GitHub Check: lint
🔇 Additional comments (11)
packages/durable-iterator/src/client/plugin.test.ts (11)
7-7: LGTM: vitest utilities import.Good move to centralize timer control in tests.
151-159: Resolved: real timers restored after each test.This addresses the earlier review about leaking fake timers. Consider also flushing timers instead of sleeping (see next comment).
Apply this refinement to avoid a real 1s wait:
- await new Promise(resolve => realSetTimeout(resolve, 1000)) // await for all promises resolved - expect(vi.getTimerCount()).toBe(0) // every is cleanup + await vi.runAllTimersAsync() // flush pending timers & microtasks under fake timers + expect(vi.getTimerCount()).toBe(0) // all timers cleaned up vi.useRealTimers()
162-162: LGTM: deterministic refresh window.Setting refreshTokenBeforeExpireInSeconds to 9 with TTL 10s makes the first refresh fire at +1s, matching the expectations.
192-192: LGTM: asserts refresh timer is armed.This validates initial scheduling after iterator resolution.
203-208: LGTM: staged advancement verifies no premature refresh.Advancing half the window and asserting stable state is correct.
209-232: LGTM: refresh cycle assertions (new token + timer restart).Good checks for URL/token rotation, handler calls, and timer lifecycle.
233-243: LGTM: validates both before-expire and delay callbacks.Ensures new option is exercised and called with the correct payload/context.
278-278: LGTM: confirms refresh disabled when NaN.Right behavior when opting out of scheduled refresh.
395-408: LGTM: reconnect on channel mismatch path.Checks for
reconnect()and updated URL, plus timer restart, look correct.
444-461: LGTM: reconnect on tags mismatch path.Mirrors the channel mismatch case; assertions are solid.
65-69: refreshTokenDelayInSeconds option correctly supported in plugin Option is defined in DurableIteratorLinkPluginOptions, wired in the constructor with a default of 2s, and applied in the scheduling logic.
| vi.advanceTimersByTime(2000) // wait next retry | ||
| expect(vi.getTimerCount()).toBe(0) // refresh token executed | ||
| await new Promise(resolve => realSetTimeout(resolve, 10)) // wait for token refresh trigger | ||
| await output.return() // cleanup | ||
|
|
||
| await sleep(2000) | ||
| vi.advanceTimersByTime(2000) // wait handler throw | ||
| await new Promise(resolve => realSetTimeout(resolve, 10)) // wait for token refresh reject | ||
| expect(unhandledRejectionHandler).toHaveBeenCalledTimes(1) | ||
| expect(unhandledRejectionHandler.mock.calls[0]![0]).toEqual( |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Ensure the unhandledRejection listener is removed.
After the final assertions, call process.off('unhandledRejection', unhandledRejectionHandler) to prevent leaking the listener to subsequent tests.
🤖 Prompt for AI Agents
In packages/durable-iterator/src/client/plugin.test.ts around lines 351 to 359,
the test adds an unhandledRejection listener but never removes it, leaking the
listener into subsequent tests; after the final assertions add a call to remove
the listener by invoking process.off('unhandledRejection',
unhandledRejectionHandler) (or process.removeListener) to unregister the handler
and prevent test leakage.
1 participant
Summary by CodeRabbit
New Features
Tests
Documentation