Conversation
eluce2
force-pushed
the
graphite-base/150
branch
from
17b40aa to
ee3c1f2
Compare
This was referenced Mar 17, 2026
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughWalkthroughAdds a FileMaker WebViewer Vite plugin with HTTP-based connectedFiles discovery, WebSocket URL resolution, and mock-script injection; exposes the plugin via package exports, adds tests and Vitest config, updates Vite build input, and clarifies a documentation prerequisite for Path D. Changes
Sequence DiagramsequenceDiagram
participant Dev as rgba(66,133,244,0.5) Dev Server
participant Plugin as rgba(15,157,88,0.5) fm-bridge Plugin
participant FmHttp as rgba(219,68,55,0.5) FM HTTP Server
participant Html as rgba(244,180,0,0.5) HTML Transformer
Dev->>Plugin: start in serve mode
Plugin->>Plugin: check options.fileName
alt fileName not provided
Plugin->>FmHttp: GET /connectedFiles
FmHttp-->>Plugin: connectedFiles array
Plugin->>Plugin: validate & pick first fileName
end
Plugin->>Plugin: resolveWsUrl(fmHttpBaseUrl, wsUrl)
Plugin->>Plugin: buildMockScriptTag(baseUrl,fileName,wsUrl,debug)
Plugin->>Html: transformIndexHtml -> inject script tag
Html-->>Dev: return HTML with fm-mock script injected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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)
📝 Coding Plan
Comment |
eluce2
force-pushed
the
feat/fm-http-typegen
branch
from
ee3c1f2 to
675c83e
Compare
eluce2
force-pushed
the
codex/fm-bridge-vite-plugin
branch
from
b344ed7 to
2b33374
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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
🤖 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/webviewer/src/fm-bridge.ts`:
- Around line 46-53: discoverConnectedFileName currently awaits
fetch(connectedFilesUrl) without a timeout which can block transformIndexHtml;
wrap the fetch in an AbortController with a short timeout (e.g., 3–10s), pass
controller.signal to fetch(connectedFilesUrl), clear the timer on success, and
handle the abort case by throwing the existing error message (preserving the
original error as cause) so stalled requests fail fast when fm-http is
unresponsive; update the try/catch around fetch in discoverConnectedFileName
accordingly and ensure any AbortError is propagated with the same user-facing
message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cdb4a5ec-b3c6-4540-bc51-11e1a798ba7d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/typegen/skills/getting-started/SKILL.mdpackages/webviewer/package.jsonpackages/webviewer/src/fm-bridge.tspackages/webviewer/src/vite-plugins.tspackages/webviewer/tests/vite-plugins.test.tspackages/webviewer/vite.config.tspackages/webviewer/vitest.config.ts
Collaborator
Author
eluce2
force-pushed
the
codex/fm-bridge-vite-plugin
branch
from
83e2fee to
7eafe6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/webviewer/src/fm-bridge.ts (1)
125-143: Avoid duplicateconnectedFileslookups by caching the in-flight discovery.
configureServerandtransformIndexHtmlcan both trigger discovery whenresolvedFileNameis unset. Caching one in-flight promise avoids duplicate network calls and racey retries.Refactor sketch
export const fmBridge = (options: FmBridgeOptions = {}): Plugin => { const baseUrl = trimToNull(options.fmHttpBaseUrl) ?? defaultFmHttpBaseUrl; const wsUrl = resolveWsUrl(options); const debug = options.debug === true; let resolvedFileName: string | null = trimToNull(options.fileName); let isServeMode = true; + let discoveryPromise: Promise<string> | null = null; + + const ensureResolvedFileName = async (): Promise<string> => { + if (resolvedFileName) return resolvedFileName; + discoveryPromise ??= discoverConnectedFileName(baseUrl) + .then((fileName: string) => { + resolvedFileName = fileName; + return fileName; + }) + .finally(() => { + discoveryPromise = null; + }); + return discoveryPromise; + }; return { @@ async configureServer() { if (!isServeMode) { return; } if (resolvedFileName) { return; } - resolvedFileName = await discoverConnectedFileName(baseUrl); + await ensureResolvedFileName(); }, async transformIndexHtml() { if (!isServeMode) { return; } if (!resolvedFileName) { - resolvedFileName = await discoverConnectedFileName(baseUrl); + await ensureResolvedFileName(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webviewer/src/fm-bridge.ts` around lines 125 - 143, Both configureServer and transformIndexHtml can call discoverConnectedFileName(baseUrl) concurrently when resolvedFileName is unset; introduce a shared in-flight promise (e.g., resolvedFileNamePromise) that is assigned before awaiting discoverConnectedFileName so both functions await the same promise, then set resolvedFileName from its result and clear the promise on error or success. Update configureServer and transformIndexHtml to check resolvedFileName first, then check/assign resolvedFileNamePromise before awaiting discoverConnectedFileName(baseUrl), and ensure error handling clears the promise so future attempts can retry.
🤖 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/webviewer/src/fm-bridge.ts`:
- Around line 71-74: The call to response.json() inside fmBridge can throw a
generic parse error and lose context when /connectedFiles returns non-JSON; wrap
the JSON parsing in a try-catch around the await response.json() (referencing
connectedFilesUrl, response, and payload) and on parse failure throw a new Error
that includes fmBridge context, the connectedFilesUrl, and the original error
message (optionally include the raw response body via response.text() to aid
debugging) before continuing with the existing Array.isArray(payload) check.
---
Nitpick comments:
In `@packages/webviewer/src/fm-bridge.ts`:
- Around line 125-143: Both configureServer and transformIndexHtml can call
discoverConnectedFileName(baseUrl) concurrently when resolvedFileName is unset;
introduce a shared in-flight promise (e.g., resolvedFileNamePromise) that is
assigned before awaiting discoverConnectedFileName so both functions await the
same promise, then set resolvedFileName from its result and clear the promise on
error or success. Update configureServer and transformIndexHtml to check
resolvedFileName first, then check/assign resolvedFileNamePromise before
awaiting discoverConnectedFileName(baseUrl), and ensure error handling clears
the promise so future attempts can retry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28e39cc4-ddd9-41f2-8c25-b088445cd4d2
📒 Files selected for processing (2)
packages/webviewer/src/fm-bridge.tspackages/webviewer/tests/vite-plugins.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/webviewer/tests/vite-plugins.test.ts
Comment on lines
+71
to
+74
| const payload = (await response.json()) as unknown; | ||
| if (!Array.isArray(payload)) { | ||
| throw new Error(`fmBridge expected an array response from ${connectedFilesUrl}.`); | ||
| } |
There was a problem hiding this comment.
Handle invalid JSON payloads with a descriptive error.
If /connectedFiles returns non-JSON, response.json() throws a generic parse error and loses fmBridge context.
Proposed fix
- const payload = (await response.json()) as unknown;
+ let payload: unknown;
+ try {
+ payload = await response.json();
+ } catch (error) {
+ throw new Error(
+ `fmBridge received invalid JSON from ${connectedFilesUrl}. Ensure fm-http returned a JSON array.`,
+ { cause: error },
+ );
+ }As per coding guidelines, "Handle errors appropriately in async code with try-catch blocks".
📝 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
| const payload = (await response.json()) as unknown; | |
| if (!Array.isArray(payload)) { | |
| throw new Error(`fmBridge expected an array response from ${connectedFilesUrl}.`); | |
| } | |
| let payload: unknown; | |
| try { | |
| payload = await response.json(); | |
| } catch (error) { | |
| throw new Error( | |
| `fmBridge received invalid JSON from ${connectedFilesUrl}. Ensure fm-http returned a JSON array.`, | |
| { cause: error }, | |
| ); | |
| } | |
| if (!Array.isArray(payload)) { | |
| throw new Error(`fmBridge expected an array response from ${connectedFilesUrl}.`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webviewer/src/fm-bridge.ts` around lines 71 - 74, The call to
response.json() inside fmBridge can throw a generic parse error and lose context
when /connectedFiles returns non-JSON; wrap the JSON parsing in a try-catch
around the await response.json() (referencing connectedFilesUrl, response, and
payload) and on parse failure throw a new Error that includes fmBridge context,
the connectedFilesUrl, and the original error message (optionally include the
raw response body via response.text() to aid debugging) before continuing with
the existing Array.isArray(payload) check.
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
New Features
Tests
Documentation
Chores