Conversation
…il certain event happens.
🦋 Changeset detectedLatest commit: b1c90d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces a new wait utility module to the SDK with two helper functions: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds two utility helpers — Key points:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant waitForCondition
participant conditionFn
participant wait
Caller->>waitForCondition: waitForCondition(conditionFn, { intervalMs, timeoutMs })
loop Until condition met or timeout
waitForCondition->>conditionFn: conditionFn()
alt condition returns true
conditionFn-->>waitForCondition: true
waitForCondition-->>Caller: resolves (void)
else condition returns false or throws
conditionFn-->>waitForCondition: false / Error (swallowed)
waitForCondition->>waitForCondition: check Date.now() - startTime >= timeoutMs
alt timed out
waitForCondition-->>Caller: rejects (Error: "Timeout while waiting for condition")
else not timed out
waitForCondition->>wait: wait(intervalMs)
wait-->>waitForCondition: resolves after intervalMs
end
end
end
Last reviewed commit: b1c90d3 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| while (true) { | ||
| try { | ||
| if (await conditionFn()) { | ||
| // Condition is met, resolve the promise and exit the loop | ||
| return; | ||
| } | ||
| } catch { | ||
| // Ignore errors from the condition function and continue retrying until timeout | ||
| } | ||
|
|
||
| if (Date.now() - startTime >= timeoutMs) { | ||
| throw new Error("Timeout while waiting for condition"); | ||
| } | ||
|
|
||
| await wait(intervalMs); | ||
| } |
There was a problem hiding this comment.
Extra conditionFn call after timeout boundary is crossed
After each wait(intervalMs) completes, the loop immediately calls conditionFn() again before checking the timeout. This means that once wait() returns at a moment where Date.now() - startTime >= timeoutMs, a full execution of conditionFn() still happens before the timeout is detected.
For the intended usage (e.g., ensIndexerClient.config() which is an HTTP request that may take several seconds to time out or return), this can cause the total elapsed time to significantly overshoot timeoutMs — by up to intervalMs + conditionFn_execution_time.
A simple fix is to check the deadline at the top of the loop, before calling conditionFn, while still always running the condition at least once on the very first iteration:
const startTime = Date.now();
while (true) {
// Check timeout at the start of each iteration (except the very first)
// so we don't invoke conditionFn after the deadline has already elapsed.
if (Date.now() - startTime >= timeoutMs) {
throw new Error("Timeout while waiting for condition");
}
try {
if (await conditionFn()) {
return;
}
} catch {
// Ignore errors from the condition function and continue retrying until timeout
}
await wait(intervalMs);
}Note: this slight change means that a timeoutMs: 0 would reject immediately without ever calling conditionFn. If "at least one check before timing out" is a hard requirement, a do/while or a separate first-call-then-loop pattern would preserve that while still preventing post-deadline conditionFn invocations.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ensnode-sdk/src/shared/wait.ts`:
- Around line 3-11: The JSDoc for the wait function (`wait(ms: number):
Promise<void>`) contains a redundant `@returns` tag; remove the `@returns` line
from the comment block so only the summary and the `@param ms` remain, leaving
the function signature and implementation (`export async function wait`)
unchanged and keeping the doc concise and valid.
- Around line 32-43: Remove the redundant `@returns` JSDoc tag from the JSDoc
block for the waitForCondition function: keep the summary describing the
behavior and retain other useful tags (e.g., `@param`) but delete the duplicate
`@returns` line so the documentation isn't repetitive for the exported async
function waitForCondition.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.changeset/slick-files-rush.mdpackages/ensnode-sdk/src/index.tspackages/ensnode-sdk/src/shared/wait.test.tspackages/ensnode-sdk/src/shared/wait.ts
There was a problem hiding this comment.
Pull request overview
Adds generic waiting utilities to @ensnode/ensnode-sdk to support workflows that need to pause execution until a dependency becomes available (e.g., waiting for an API to come up), and exposes them as part of the public SDK surface.
Changes:
- Introduces
wait(ms)andwaitForCondition(conditionFn, options)helpers insrc/shared/wait.ts. - Adds a Vitest suite covering expected
wait/waitForConditionbehavior. - Exports the new helpers from the package root and adds a changeset for a minor bump.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/ensnode-sdk/src/shared/wait.ts | New public waiting helpers (wait, waitForCondition) with defaults. |
| packages/ensnode-sdk/src/shared/wait.test.ts | Unit tests for timing, retry, error swallowing, and timeout cases. |
| packages/ensnode-sdk/src/index.ts | Re-exports the new shared module from the SDK entrypoint. |
| .changeset/slick-files-rush.md | Announces the new helpers in release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { | ||
| intervalMs = DEFAULT_WAIT_FOR_CONDITION_OPTIONS.intervalMs, | ||
| timeoutMs = DEFAULT_WAIT_FOR_CONDITION_OPTIONS.timeoutMs, | ||
| } = options || {}; |
There was a problem hiding this comment.
waitForCondition should validate intervalMs/timeoutMs as finite numbers (and typically intervalMs > 0, timeoutMs >= 0). As written, passing NaN (or a non-finite) timeoutMs makes the timeout check always false, resulting in an infinite loop; intervalMs <= 0 (or NaN, which becomes 0 in setTimeout) can create a tight retry loop.
| try { | ||
| if (await conditionFn()) { | ||
| // Condition is met, resolve the promise and exit the loop | ||
| return; | ||
| } | ||
| } catch { | ||
| // Ignore errors from the condition function and continue retrying until timeout | ||
| } |
There was a problem hiding this comment.
The catch {} unconditionally swallows all errors from conditionFn. Since this is exported as a public helper, it becomes impossible for callers to fail fast on non-transient errors (e.g. misconfiguration) or propagate cancellation errors; consider either rethrowing by default, or adding an option (e.g. ignoreErrors / shouldRetryOnError(error)) and documenting the behavior explicitly.
| } | ||
|
|
||
| if (Date.now() - startTime >= timeoutMs) { | ||
| throw new Error("Timeout while waiting for condition"); |
There was a problem hiding this comment.
The timeout error message is quite generic. Since this is likely to surface in operational logs, consider including at least the timeoutMs value (and possibly the last observed error from conditionFn, if you add error handling options) to make debugging easier.
| throw new Error("Timeout while waiting for condition"); | |
| throw new Error(`Timeout while waiting for condition after ${timeoutMs} ms`); |
|
|
||
| describe("wait", () => { | ||
| beforeEach(() => { | ||
| vi.useFakeTimers(); |
There was a problem hiding this comment.
This test suite asserts on Date.now() deltas later, but uses vi.useFakeTimers() without pinning now like other timer-based tests in this package (e.g. vi.useFakeTimers({ shouldAdvanceTime: true, now: ... })). Pinning now improves determinism across environments and makes the Date.now()-based assertions less brittle.
| vi.useFakeTimers(); | |
| vi.useFakeTimers({ now: new Date("2020-01-01T00:00:00Z") }); |
| "@ensnode/ensnode-sdk": minor | ||
| --- | ||
|
|
||
| Added waiting helpers that allow halting runtime until certain event happens. |
There was a problem hiding this comment.
Grammar in the changeset message: "until certain event happens" is missing an article. Consider changing it to "until a certain event happens" (or similar) since this text may appear in release notes.
| Added waiting helpers that allow halting runtime until certain event happens. | |
| Added waiting helpers that allow halting runtime until a certain event happens. |
|
Closing this PR as after reading feedback it feels like re-implementing a well known package: |
Initially, I wanted to implement some helpers myself, but then, after seeing AI PR feedback, I figured I was rebuilding `p-retry` in a way... Here is the (now closed) PR: #1709
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
wait()that allows waiting a certain amount of timewaitForCondition()that allows waiting until a certain condition turnstureWhy
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)