Retry fmFetch when FileMaker bridge is briefly unavailable#289
Conversation
🦋 Changeset detectedLatest commit: b352a48 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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.
|
|
fixes #288 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch PR details, files changed, and diff
gh pr view 289 --repo proofsh/proofkit --json title,body,files,additions,deletions,headRefName
echo "=== PR DIFF ==="
gh pr diff 289 --repo proofsh/proofkitLength of output: 5498 The retry logic is well-structured. A few observations:
The One thing to be aware of:
The two tests cover the promise path well. However, the callback path error surface — the it("surfaces error asynchronously when callback path exhausts retries", async () => {
const { fmFetch } = await import("../src/main.ts");
const callback = vi.fn();
let caughtError: unknown;
vi.spyOn(globalThis, "setTimeout").mockImplementationOnce((fn) => {
try { fn(); } catch (e) { caughtError = e; }
return 0 as unknown as ReturnType<typeof setTimeout>;
});
fmFetch("LoadData", { id: "123" }, callback);
await vi.advanceTimersByTimeAsync(1750);
expect(callback).not.toHaveBeenCalled();
expect(caughtError).toBeInstanceOf(Error);
});…would prevent regressions in that code path. Overall: The implementation is solid and the changeset is properly scoped. The missing callback-path test is the main gap worth addressing. ✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds bounded retry logic inside ChangesfmFetch FileMaker Retry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
packages/webviewer/tests/main.test.ts (1)
21-51: ⚡ Quick winAdd callback-mode retry coverage.
Current suite validates only Promise-based
fmFetch. Since callback-mode retry/error behavior was also changed, add tests for callback success-after-retry and callback failure after retries to lock in both contracts.As per coding guidelines, test files should keep assertions inside
it()/test()and use async/await patterns; extending coverage here should follow that same rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webviewer/tests/main.test.ts` around lines 21 - 51, The test suite currently only covers Promise-based fmFetch behavior but does not validate callback-mode retry and error handling which was also modified. Add two new test cases after the existing tests: one using it() that tests callback-mode success after retry (similar structure to "retries when FileMaker is not yet available" but invoke the callback-based fmFetch method and verify the callback receives the response after retries), and another using it() that tests callback-mode failure after three retries (verify the callback receives an error after vi.advanceTimersByTimeAsync moves through the retry timeline). Both new tests should follow the established pattern of using async/await, advancing timers with vi.advanceTimersByTimeAsync(), and keeping all assertions inside the test body.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/webviewer/src/main.ts`:
- Around line 45-50: The error handling in the catch block for the
`_execScriptWithFileMakerRetry` call converts a catchable Promise rejection into
an uncaught global exception by rethrowing via setTimeout, which prevents
callers from handling the failure. Remove the setTimeout rethrow pattern and
instead surface the failure through a controllable channel appropriate for the
callback mode being used - either by allowing the Promise rejection to propagate
so callers can catch it with their own error handling, or by invoking an
explicit error callback to communicate the failure to the caller.
---
Nitpick comments:
In `@packages/webviewer/tests/main.test.ts`:
- Around line 21-51: The test suite currently only covers Promise-based fmFetch
behavior but does not validate callback-mode retry and error handling which was
also modified. Add two new test cases after the existing tests: one using it()
that tests callback-mode success after retry (similar structure to "retries when
FileMaker is not yet available" but invoke the callback-based fmFetch method and
verify the callback receives the response after retries), and another using it()
that tests callback-mode failure after three retries (verify the callback
receives an error after vi.advanceTimersByTimeAsync moves through the retry
timeline). Both new tests should follow the established pattern of using
async/await, advancing timers with vi.advanceTimersByTimeAsync(), and keeping
all assertions inside the test body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b090d8e-fd5f-41bd-8c08-ca9d8c2dd83b
📒 Files selected for processing (3)
.changeset/fuzzy-trees-fetch.mdpackages/webviewer/src/main.tspackages/webviewer/tests/main.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/webviewer/src/main.ts (1)
45-50:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid converting callback-path retry exhaustion into an uncaught global exception.
Lines 47-49 rethrow via
setTimeout, which makes failure uncatchable by callers and can crash the host runtime. Please surface this through a controllable channel (e.g., explicit error callback or Promise-returning API path) instead of async global throw.As per coding guidelines, "Handle errors appropriately in async code with try-catch blocks".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webviewer/src/main.ts` around lines 45 - 50, The error handling for _execScriptWithFileMakerRetry is using setTimeout to rethrow errors asynchronously, which creates an uncatchable global exception that can crash the runtime. Replace this setTimeout pattern with a controllable error handling approach by either passing the error to an explicit error callback parameter or by returning the promise rejection in a way that allows callers to handle the error through standard Promise error handling (e.g., by not catching it and letting it propagate, or by explicitly handling it with a proper callback mechanism). This ensures errors from the retry exhaustion are surfaced through a channel that callers can control rather than becoming an uncaught global exception.Source: Coding guidelines
🧹 Nitpick comments (3)
packages/webviewer/tests/main.test.ts (1)
43-51: ⚡ Quick winAdd callback-mode retry-exhaustion coverage.
Current tests cover Promise-mode exhaustion, but not the callback branch’s failure surface. Add one test for callback-mode exhaustion so future changes don’t regress that path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webviewer/tests/main.test.ts` around lines 43 - 51, Add a new test case alongside the existing Promise-mode retry exhaustion test to cover callback-mode retry exhaustion. Create a test that calls the fmFetch function from "../src/main.ts" with a callback parameter instead of using Promise-based handling, then advance timers by 1750ms to exhaust the three FileMaker availability retries, and verify that the callback receives the expected error indicating that 'window.FileMaker' was not available. This ensures the callback branch of the retry logic is covered and protected against future regressions.packages/webviewer/src/main.ts (2)
114-125: ⚡ Quick winRefactor the retry loop to a bounded
for...offlow.Line 114 uses an indexed infinite loop. It works, but this violates the TS loop guideline and makes termination implicit.
♻️ Proposed refactor
async function _execScriptWithFileMakerRetry(scriptName: string, data: unknown, cb: (arg0?: unknown) => void) { - for (let attempt = 0; ; attempt++) { + for (const delay of FM_FETCH_FILEMAKER_RETRY_DELAYS_MS) { try { _execScript(scriptName, data, cb); return; } catch (error) { - const delay = FM_FETCH_FILEMAKER_RETRY_DELAYS_MS[attempt]; - if (!(delay && isFileMakerUnavailableError(error))) { + if (!isFileMakerUnavailableError(error)) { throw error; } await wait(delay); } } + _execScript(scriptName, data, cb); }As per coding guidelines, "Prefer
for...ofloops over.forEach()and indexedforloops".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webviewer/src/main.ts` around lines 114 - 125, The retry loop uses an indexed infinite loop (for (let attempt = 0; ; attempt++)) which violates TypeScript coding guidelines. Refactor this loop to use a for...of iteration over the FM_FETCH_FILEMAKER_RETRY_DELAYS_MS array instead, making the loop bounds explicit and termination conditions clear. The loop should iterate through the delay values in the array, catching FileMaker unavailable errors and retrying with the corresponding delay, then throwing any other errors. This will replace the implicit termination of the current indexed infinite loop with an explicit bounded iteration.Source: Coding guidelines
5-5: ⚡ Quick winTie retryable-error classification to a single source of truth.
isFileMakerUnavailableErrordepends on a message fragment; if the thrown wording changes, retries silently stop. Consider deriving the thrown message and matcher from one shared constant (or an error code/type) to prevent drift.Also applies to: 103-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webviewer/src/main.ts` at line 5, The FILEMAKER_UNAVAILABLE_MESSAGE constant and the isFileMakerUnavailableError function (which checks for a message fragment) are decoupled, creating a maintenance risk where if one is updated the other silently becomes inconsistent. Refactor to use a single source of truth by ensuring the error message thrown when FileMaker is unavailable and the matcher in isFileMakerUnavailableError both reference the same FILEMAKER_UNAVAILABLE_MESSAGE constant, or alternatively consider using an error code or type-based check instead of message string matching to decouple the error classification from wording changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/webviewer/src/main.ts`:
- Around line 45-50: The error handling for _execScriptWithFileMakerRetry is
using setTimeout to rethrow errors asynchronously, which creates an uncatchable
global exception that can crash the runtime. Replace this setTimeout pattern
with a controllable error handling approach by either passing the error to an
explicit error callback parameter or by returning the promise rejection in a way
that allows callers to handle the error through standard Promise error handling
(e.g., by not catching it and letting it propagate, or by explicitly handling it
with a proper callback mechanism). This ensures errors from the retry exhaustion
are surfaced through a channel that callers can control rather than becoming an
uncaught global exception.
---
Nitpick comments:
In `@packages/webviewer/src/main.ts`:
- Around line 114-125: The retry loop uses an indexed infinite loop (for (let
attempt = 0; ; attempt++)) which violates TypeScript coding guidelines. Refactor
this loop to use a for...of iteration over the
FM_FETCH_FILEMAKER_RETRY_DELAYS_MS array instead, making the loop bounds
explicit and termination conditions clear. The loop should iterate through the
delay values in the array, catching FileMaker unavailable errors and retrying
with the corresponding delay, then throwing any other errors. This will replace
the implicit termination of the current indexed infinite loop with an explicit
bounded iteration.
- Line 5: The FILEMAKER_UNAVAILABLE_MESSAGE constant and the
isFileMakerUnavailableError function (which checks for a message fragment) are
decoupled, creating a maintenance risk where if one is updated the other
silently becomes inconsistent. Refactor to use a single source of truth by
ensuring the error message thrown when FileMaker is unavailable and the matcher
in isFileMakerUnavailableError both reference the same
FILEMAKER_UNAVAILABLE_MESSAGE constant, or alternatively consider using an error
code or type-based check instead of message string matching to decouple the
error classification from wording changes.
In `@packages/webviewer/tests/main.test.ts`:
- Around line 43-51: Add a new test case alongside the existing Promise-mode
retry exhaustion test to cover callback-mode retry exhaustion. Create a test
that calls the fmFetch function from "../src/main.ts" with a callback parameter
instead of using Promise-based handling, then advance timers by 1750ms to
exhaust the three FileMaker availability retries, and verify that the callback
receives the expected error indicating that 'window.FileMaker' was not
available. This ensures the callback branch of the retry logic is covered and
protected against future regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59b5954d-2e6b-4d2d-a70d-db32407c4897
📒 Files selected for processing (3)
.changeset/fuzzy-trees-fetch.mdpackages/webviewer/src/main.tspackages/webviewer/tests/main.test.ts
1 participant
Summary
fmFetchwhenwindow.FileMakeris temporarily unavailable.Closes #288
Testing
pnpm run ciSummary by CodeRabbit
Bug Fixes
fmFetchreliability by automatically retrying FileMaker script calls when FileMaker is briefly unavailable, using progressive backoff delays.New Features
fmFetchcallback usage now returns aPromise<void>to match the retry flow.Tests
Chores