Conversation
- expose `@proofkit/webviewer/nextjs` - add App Router and Pages Router bridge helpers - update docs and package exports
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 5b4d168 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 |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds Next.js-specific FM Bridge support for local development. It extracts core URL/script building utilities, introduces Next.js components ( ChangesNext.js FM Bridge Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- bump rolldown and related oxc/rolldown packages - refresh webviewer next/react entries
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
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: 2
🧹 Nitpick comments (3)
packages/webviewer/tests/nextjs.test.ts (1)
169-193: ⚡ Quick winConvert
.then()chains toasync/awaitin theResolvedFmBridgeScriptsuiteBoth tests in this describe block use
.then()onnextjsModulePromiseinstead ofasync/await, inconsistent with the pattern used in the rest of the test file and the project's test coding guidelines.♻️ Proposed refactor
- it("renders null when script props are null", () => { - return nextjsModulePromise.then(({ ResolvedFmBridgeScript }) => { - expect(ResolvedFmBridgeScript({ script: null })).toBeNull(); - }); - }); + it("renders null when script props are null", async () => { + const { ResolvedFmBridgeScript } = await nextjsModulePromise; + expect(ResolvedFmBridgeScript({ script: null })).toBeNull(); + }); - it("renders next/script with provided props", () => { - return nextjsModulePromise.then(({ ResolvedFmBridgeScript }) => { - expect( - ResolvedFmBridgeScript({ - script: { - strategy: "beforeInteractive", - src: "http://localhost:1365/fm-mock.js?fileName=Contacts&wsUrl=ws%3A%2F%2Flocalhost%3A1365%2Fws", - }, - }), - ).toMatchObject({ - props: { - strategy: "beforeInteractive", - src: "http://localhost:1365/fm-mock.js?fileName=Contacts&wsUrl=ws%3A%2F%2Flocalhost%3A1365%2Fws", - }, - }); - }); - }); + it("renders next/script with provided props", async () => { + const { ResolvedFmBridgeScript } = await nextjsModulePromise; + expect( + ResolvedFmBridgeScript({ + script: { + strategy: "beforeInteractive", + src: "http://localhost:1365/fm-mock.js?fileName=Contacts&wsUrl=ws%3A%2F%2Flocalhost%3A1365%2Fws", + }, + }), + ).toMatchObject({ + props: { + strategy: "beforeInteractive", + src: "http://localhost:1365/fm-mock.js?fileName=Contacts&wsUrl=ws%3A%2F%2Flocalhost%3A1365%2Fws", + }, + }); + });As per coding guidelines: "Use
async/awaitsyntax instead of promise chains for better readability."🤖 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/nextjs.test.ts` around lines 169 - 193, Update the two tests in the "ResolvedFmBridgeScript" suite to use async/await instead of .then() on nextjsModulePromise: mark each test callback async, await nextjsModulePromise to get ResolvedFmBridgeScript, and then perform the same assertions (removing the returned promise chain). Keep the same inputs and expectations for ResolvedFmBridgeScript({ script: ... }) and for the null case.packages/webviewer/src/nextjs.ts (2)
64-70: ⚡ Quick winReplace the double type assertion with a properly typed cast
script as unknown as Record<string, unknown>completely bypasses TypeScript's prop-type checking for theScriptcomponent. Per coding guidelines, prefer type narrowing over type assertions. The root cause is likely thatNextFmBridgeScriptPropsisn't declared to extendnext/script'sScriptProps. Aligning the interface with the actual component props (or usingComponentPropsWithoutRef<typeof Script>as the target type) eliminates the need for the double cast.♻️ Proposed refactor
-import Script from "next/script"; +import Script, { type ScriptProps } from "next/script";-export interface NextFmBridgeScriptProps { - strategy: "beforeInteractive"; - src?: string; - id?: string; - dangerouslySetInnerHTML?: { - __html: string; - }; -} +export type NextFmBridgeScriptProps = Pick<ScriptProps, + "strategy" | "src" | "id" | "dangerouslySetInnerHTML" +>;Then the assertion simplifies to a single, narrower cast:
- return createElement(Script, script as unknown as Record<string, unknown>); + return createElement(Script, script as ScriptProps);As per coding guidelines: "Leverage TypeScript's type narrowing instead of type assertions."
🤖 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/nextjs.ts` around lines 64 - 70, The component ResolvedFmBridgeScript currently uses a double-cast "script as unknown as Record<string, unknown>" which bypasses TypeScript checks; update the type for the incoming prop (ResolvedFmBridgeScriptProps / NextFmBridgeScriptProps) to extend next/script's ScriptProps or use ComponentPropsWithoutRef<typeof Script> as the proper script type, then replace the double assertion with a single, correctly-typed cast (or no cast if you make the prop already the correct type) when calling createElement(Script, ...), ensuring Script receives the correct prop type.
64-79: 💤 Low valueAdd explicit return type annotations to exported component functions
Neither
ResolvedFmBridgeScriptnorFmBridgeScriptdeclare a return type. For exported React components, the return type carries useful documentation value and prevents silent regressions if the implementation changes.♻️ Proposed annotations
-export const ResolvedFmBridgeScript = ({ script }: ResolvedFmBridgeScriptProps) => { +export const ResolvedFmBridgeScript = ({ script }: ResolvedFmBridgeScriptProps): React.ReactElement | null => {-export const FmBridgeScript = async (options: FmBridgeOptions = {}) => { +export const FmBridgeScript = async (options: FmBridgeOptions = {}): Promise<React.ReactElement | null> => {As per coding guidelines: "Use explicit types for function parameters and return values when they enhance clarity."
🤖 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/nextjs.ts` around lines 64 - 79, Both exported functions lack explicit return types; add explicit React return annotations to prevent regressions: annotate ResolvedFmBridgeScript signature to return React.ReactElement | null and annotate the async FmBridgeScript to return Promise<React.ReactElement | null>. Ensure any necessary React types are imported and update the function signatures for ResolvedFmBridgeScript and FmBridgeScript accordingly (keeping parameters ResolvedFmBridgeScriptProps and FmBridgeOptions).
🤖 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/package.json`:
- Around line 59-62: The package.json currently lists next and react under
"peerDependencies" which causes spurious warnings for consumers that don't use
the ./nextjs subpath; add a top-level "peerDependenciesMeta" object in
package.json with "next": { "optional": true } and "react": { "optional": true }
to mark those peers as optional so package managers won't warn for Vite-only
consumers, ensuring the change is valid JSON and coexists with the existing
"peerDependencies" entry.
In `@packages/webviewer/src/nextjs.ts`:
- Around line 31-62: Wrap the await discoverConnectedFileName(baseUrl) call
inside getFmBridgeScriptProps in a try-catch so any thrown errors are swallowed
and treated as "no connected file" (i.e., leave resolvedFileName undefined/null)
so the function falls through to the existing fallback branch that returns
strategy: "beforeInteractive" with id: fallbackScriptId and
dangerouslySetInnerHTML using buildNoConnectedFilesRuntimeScript(baseUrl); keep
the rest of the logic (baseUrl, wsUrl, debug, buildMockScriptUrl) unchanged and
don't rethrow the error.
---
Nitpick comments:
In `@packages/webviewer/src/nextjs.ts`:
- Around line 64-70: The component ResolvedFmBridgeScript currently uses a
double-cast "script as unknown as Record<string, unknown>" which bypasses
TypeScript checks; update the type for the incoming prop
(ResolvedFmBridgeScriptProps / NextFmBridgeScriptProps) to extend next/script's
ScriptProps or use ComponentPropsWithoutRef<typeof Script> as the proper script
type, then replace the double assertion with a single, correctly-typed cast (or
no cast if you make the prop already the correct type) when calling
createElement(Script, ...), ensuring Script receives the correct prop type.
- Around line 64-79: Both exported functions lack explicit return types; add
explicit React return annotations to prevent regressions: annotate
ResolvedFmBridgeScript signature to return React.ReactElement | null and
annotate the async FmBridgeScript to return Promise<React.ReactElement | null>.
Ensure any necessary React types are imported and update the function signatures
for ResolvedFmBridgeScript and FmBridgeScript accordingly (keeping parameters
ResolvedFmBridgeScriptProps and FmBridgeOptions).
In `@packages/webviewer/tests/nextjs.test.ts`:
- Around line 169-193: Update the two tests in the "ResolvedFmBridgeScript"
suite to use async/await instead of .then() on nextjsModulePromise: mark each
test callback async, await nextjsModulePromise to get ResolvedFmBridgeScript,
and then perform the same assertions (removing the returned promise chain). Keep
the same inputs and expectations for ResolvedFmBridgeScript({ script: ... }) and
for the null case.
🪄 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: 5b104c27-1ea2-4e91-b0f8-c8233b3ba222
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.changeset/nextjs-fm-bridge.mdapps/docs/content/docs/webviewer/fm-bridge.mdxapps/docs/content/docs/webviewer/package.mdxpackages/webviewer/package.jsonpackages/webviewer/src/fm-bridge.tspackages/webviewer/src/nextjs-runtime.d.tspackages/webviewer/src/nextjs.tspackages/webviewer/tests/nextjs.test.tspackages/webviewer/tsconfig.jsonpackages/webviewer/vite.config.ts
Comment on lines
+59
to
+62
| "peerDependencies": { | ||
| "next": ">=13.0.0", | ||
| "react": ">=18.0.0" | ||
| }, |
There was a problem hiding this comment.
Add peerDependenciesMeta to mark next and react as optional peers.
next and react are only consumed by the ./nextjs subpath export. Without peerDependenciesMeta, every installer of @proofkit/webviewer — including Vite-only consumers — will receive a spurious peer-dependency warning about missing next and react.
peerDependenciesMeta lets you mark a peer dependency as optional so npm will not emit a warning if the package is not installed on the host. When set to true, the selected peer dependency will be marked as optional by the package manager; therefore, consumers omitting it will no longer be reported as an error.
🛠️ Proposed fix
"peerDependencies": {
"next": ">=13.0.0",
"react": ">=18.0.0"
},
+ "peerDependenciesMeta": {
+ "next": {
+ "optional": true
+ },
+ "react": {
+ "optional": true
+ }
+ },📝 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
| "peerDependencies": { | |
| "next": ">=13.0.0", | |
| "react": ">=18.0.0" | |
| }, | |
| "peerDependencies": { | |
| "next": ">=13.0.0", | |
| "react": ">=18.0.0" | |
| }, | |
| "peerDependenciesMeta": { | |
| "next": { | |
| "optional": true | |
| }, | |
| "react": { | |
| "optional": true | |
| } | |
| }, |
🤖 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/package.json` around lines 59 - 62, The package.json
currently lists next and react under "peerDependencies" which causes spurious
warnings for consumers that don't use the ./nextjs subpath; add a top-level
"peerDependenciesMeta" object in package.json with "next": { "optional": true }
and "react": { "optional": true } to mark those peers as optional so package
managers won't warn for Vite-only consumers, ensuring the change is valid JSON
and coexists with the existing "peerDependencies" entry.
- fall back to inline script props when discovery throws - add test for fetch rejection path
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
@proofkit/webviewer/nextjsfor Next.js App Router and Pages Router dev integration.FmBridgeScript,getFmBridgeScriptProps(), andResolvedFmBridgeScriptfor beforeInteractive injection or inline fallback.Testing
pnpm run cinot run here.packages/webviewer/tests/nextjs.test.ts.packages/webviewer/package.jsonandpackages/webviewer/tsconfig.jsonupdates.Summary by CodeRabbit
New Features
fmBridgehelper for local development integration with FileMaker, supporting both App Router and Pages Router architectures.Documentation
fmBridgedocumentation with Next.js-specific setup instructions and configuration options.