Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR rebrandsall references from "FM HTTP" to "FM MCP" throughout the stack. Configuration keys, environment variables, adapter names, type names, documentation, CLI prompts, and error messages are updated to reflect the FileMaker MCP server concept while preserving the underlying HTTP transport mechanism. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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)
📝 Coding Plan
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli-old/src/generators/fmdapi.ts (1)
254-263:⚠️ Potential issue | 🟠 MajorDrop the legacy
fmHttpblock before writing the merged config.
readJsonConfigFile()returns raw JSONC objects, so spreadingexistingConfighere can preserve a stalefmHttpproperty on disk. Re-running this helper against a config that still has the old key can leave bothfmHttpandfmMcpin the same entry, which breaks the clean-break rename and the new schema.Suggested fix
- const existingConfig = configArray[existingConfigIndex] as FmdapiDataSourceConfig; + const { fmHttp: _legacyFmHttp, ...existingConfig } = configArray[ + existingConfigIndex + ] as FmdapiDataSourceConfig & { fmHttp?: unknown }; configArray[existingConfigIndex] = { ...existingConfig, ...newConfig, layouts: existingConfig.layouts ?? [], fmMcp: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli-old/src/generators/fmdapi.ts` around lines 254 - 263, When merging configs in the block that assigns configArray[existingConfigIndex] (using existingConfig and newConfig from readJsonConfigFile), explicitly remove any legacy fmHttp key so it is not carried forward; build the merged object from {...existingConfig, ...newConfig} but delete or omit existingConfig.fmHttp (and/or newConfig.fmHttp) before assignment, and ensure fmMcp is constructed as shown (fmMcp: { enabled: true, ...(existingConfig.fmMcp ?? {}), ...(newConfig.fmMcp ?? {}) }) so the final object contains fmMcp but no fmHttp.packages/cli/src/generators/fmdapi.ts (1)
254-263:⚠️ Potential issue | 🟠 MajorDrop the legacy
fmHttpblock before writing the merged config.This merge has the same problem as the mirrored old-CLI implementation:
existingConfigcomes from raw JSONC, so spreading it can keep a stalefmHttpproperty in the saved config. That leaves the rename half-applied and can produce entries with bothfmHttpandfmMcp.Suggested fix
- const existingConfig = configArray[existingConfigIndex] as FmdapiDataSourceConfig; + const { fmHttp: _legacyFmHttp, ...existingConfig } = configArray[ + existingConfigIndex + ] as FmdapiDataSourceConfig & { fmHttp?: unknown }; configArray[existingConfigIndex] = { ...existingConfig, ...newConfig, layouts: existingConfig.layouts ?? [], fmMcp: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/generators/fmdapi.ts` around lines 254 - 263, The merged config currently spreads raw existingConfig (from configArray[existingConfigIndex]) which can leave a stale legacy fmHttp property alongside the new fmMcp block; before assigning back to configArray[existingConfigIndex] in the code that builds the merged object (using existingConfig, newConfig, layouts, and fmMcp), explicitly remove or omit the fmHttp key from the merged result (e.g., construct the merged object without copying fmHttp or delete merged.fmHttp) so only fmMcp remains; ensure the code that sets configArray[existingConfigIndex] references existingConfig, newConfig, layouts and fmMcp but does not propagate fmHttp.
🧹 Nitpick comments (3)
packages/typegen/src/constants.ts (1)
32-36: Unify hostnames indefaultFmMcpBaseUrlacross packages.Two packages define
defaultFmMcpBaseUrlwith different hostnames:
packages/typegen/src/constants.tsuses"http://127.0.0.1:1365"packages/webviewer/src/fm-bridge.tsuses"http://localhost:1365"While functionally equivalent on localhost,
localhostand127.0.0.1can differ in CORS origin matching, SSL/TLS certificate validation, and cookie handling. Using the same hostname consistently across the codebase reduces risk of subtle environment-specific issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typegen/src/constants.ts` around lines 32 - 36, The constant defaultFmMcpBaseUrl in packages/typegen/src/constants.ts currently uses "http://127.0.0.1:1365" which differs from the hostname used elsewhere; update the defaultFmMcpBaseUrl value to use the same hostname as in packages/webviewer (i.e., "http://localhost:1365") so that defaultFmMcpBaseUrl and any related constants (e.g., fmMcpBaseUrl) are consistent across packages, and search for other occurrences of defaultFmMcpBaseUrl or hardcoded 127.0.0.1 to make them uniform.packages/cli/tests/resolve-init.test.ts (1)
144-174: Test name does not match the updated terminology.The test name on line 144 still says
"uses local fm http for webviewer setup when available"but the test now verifieslocal-fm-mcpmode. Consider updating the test name to"uses local fm mcp for webviewer setup when available"for consistency.📝 Proposed test name fix
- it("uses local fm http for webviewer setup when available", async () => { + it("uses local fm mcp for webviewer setup when available", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/resolve-init.test.ts` around lines 144 - 174, Update the test title string in the test case that asserts fileMaker.mode === "local-fm-mcp" — change the description from "uses local fm http for webviewer setup when available" to "uses local fm mcp for webviewer setup when available" so the test name matches the verified behavior; this affects the it(...) block that calls resolveInitRequest(...) and asserts request.fileMaker toMatchObject with mode "local-fm-mcp" and fileName "LocalFile.fmp12".packages/webviewer/src/fm-bridge.ts (1)
8-14: Standardize default FM MCP base URL across monorepo.
packages/webviewer/src/fm-bridge.tsuseslocalhost:1365, whilepackages/typegen/src/constants.ts,packages/cli/, andpackages/cli-old/all use127.0.0.1:1365. Though both resolve to the same address, this inconsistency can cause subtle CORS/cookie behavior differences and debugging confusion. Standardize the monorepo on one form.🤖 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 8 - 14, The default FM MCP URLs in fm-bridge.ts are using "localhost:1365" while the rest of the monorepo uses "127.0.0.1:1365"; update the constants defaultFmMcpBaseUrl and defaultWsUrl to use "127.0.0.1:1365" (and "ws://127.0.0.1:1365/ws" respectively) so the default values match the other packages and avoid CORS/cookie inconsistencies.
🤖 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/cli-old/src/helpers/fmMcp.ts`:
- Line 33: getFmMcpStatus currently defaults to defaultBaseUrl which is still
sourced from the old FM_HTTP_BASE_URL runtime config; update the source of
defaultBaseUrl to use the new runtime config variable (the renamed HTTP base
URL) so the FM_HTTP_BASE_URL name is not referenced at runtime. Locate the
defaultBaseUrl declaration at the top of this module and change its
environment/config key to the new name, and ensure getFmMcpStatus (and any other
functions referencing defaultBaseUrl) now read the renamed config variable
instead of FM_HTTP_BASE_URL.
In `@packages/typegen/src/server/createDataApiClient.ts`:
- Around line 206-214: The default environment name for fmMcp is still the
legacy "FM_HTTP_BASE_URL" causing the MCP client to fall back to the old
variable; update the defaultEnvNames.fmMcpBaseUrl constant in
packages/typegen/src/constants.ts to the new env var name used for MCP (replace
the legacy string with the new MCP-specific env name), then ensure code paths
that use getEnvName/defaultEnvNames (e.g., baseUrlEnvName in
createDataApiClient.ts) now resolve to the new name so fmMcpObj?.baseUrl ||
process.env[baseUrlEnvName] uses the correct env var.
- Around line 217-225: The error message for the missing connectedFileName
currently points to the hard-coded FM_CONNECTED_FILE_NAME; update the
createDataApiClient branch that returns the missing-file error (the block
checking connectedFileName) to reference the resolved env var name from
envNames.fmMcp.connectedFileName (and include that value in the message and/or
suspectedField/details) so projects using a custom env name get the correct
guidance; locate the connectedFileName check in createDataApiClient and
substitute the hard-coded string with envNames.fmMcp.connectedFileName.
In `@packages/typegen/src/types.ts`:
- Around line 193-204: The help text for scriptName in the fmMcpFieldObject
still refers to "HTTP proxy"; update the description for the scriptName field
(inside fmMcpFieldObject) to mention the FM MCP proxy or the MCP service rather
than "HTTP proxy" so generated schema/docs reflect the rename (also review
similar wording in baseUrl description to ensure consistency with FM MCP
naming).
---
Outside diff comments:
In `@packages/cli-old/src/generators/fmdapi.ts`:
- Around line 254-263: When merging configs in the block that assigns
configArray[existingConfigIndex] (using existingConfig and newConfig from
readJsonConfigFile), explicitly remove any legacy fmHttp key so it is not
carried forward; build the merged object from {...existingConfig, ...newConfig}
but delete or omit existingConfig.fmHttp (and/or newConfig.fmHttp) before
assignment, and ensure fmMcp is constructed as shown (fmMcp: { enabled: true,
...(existingConfig.fmMcp ?? {}), ...(newConfig.fmMcp ?? {}) }) so the final
object contains fmMcp but no fmHttp.
In `@packages/cli/src/generators/fmdapi.ts`:
- Around line 254-263: The merged config currently spreads raw existingConfig
(from configArray[existingConfigIndex]) which can leave a stale legacy fmHttp
property alongside the new fmMcp block; before assigning back to
configArray[existingConfigIndex] in the code that builds the merged object
(using existingConfig, newConfig, layouts, and fmMcp), explicitly remove or omit
the fmHttp key from the merged result (e.g., construct the merged object without
copying fmHttp or delete merged.fmHttp) so only fmMcp remains; ensure the code
that sets configArray[existingConfigIndex] references existingConfig, newConfig,
layouts and fmMcp but does not propagate fmHttp.
---
Nitpick comments:
In `@packages/cli/tests/resolve-init.test.ts`:
- Around line 144-174: Update the test title string in the test case that
asserts fileMaker.mode === "local-fm-mcp" — change the description from "uses
local fm http for webviewer setup when available" to "uses local fm mcp for
webviewer setup when available" so the test name matches the verified behavior;
this affects the it(...) block that calls resolveInitRequest(...) and asserts
request.fileMaker toMatchObject with mode "local-fm-mcp" and fileName
"LocalFile.fmp12".
In `@packages/typegen/src/constants.ts`:
- Around line 32-36: The constant defaultFmMcpBaseUrl in
packages/typegen/src/constants.ts currently uses "http://127.0.0.1:1365" which
differs from the hostname used elsewhere; update the defaultFmMcpBaseUrl value
to use the same hostname as in packages/webviewer (i.e.,
"http://localhost:1365") so that defaultFmMcpBaseUrl and any related constants
(e.g., fmMcpBaseUrl) are consistent across packages, and search for other
occurrences of defaultFmMcpBaseUrl or hardcoded 127.0.0.1 to make them uniform.
In `@packages/webviewer/src/fm-bridge.ts`:
- Around line 8-14: The default FM MCP URLs in fm-bridge.ts are using
"localhost:1365" while the rest of the monorepo uses "127.0.0.1:1365"; update
the constants defaultFmMcpBaseUrl and defaultWsUrl to use "127.0.0.1:1365" (and
"ws://127.0.0.1:1365/ws" respectively) so the default values match the other
packages and avoid CORS/cookie inconsistencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3270e047-c7c8-4155-a58f-0bc2fa2e9781
📒 Files selected for processing (68)
.changeset/cli-webviewer-scaffold-fm-mcp.md.changeset/fm-http-to-fm-mcp.md.changeset/fm-http-typegen-flow.md.changeset/fm-mcp-typegen-flow.md.changeset/pre.json_artifacts/domain_map.yaml_artifacts/skill_spec.mdapps/docs/content/docs/cli/webviewer/getting-started.mdxapps/docs/content/docs/cli/webviewer/overview.mdxapps/docs/content/docs/typegen/config.mdxpackages/cli-old/src/cli/add/data-source/filemaker.tspackages/cli-old/src/generators/fmdapi.tspackages/cli-old/src/helpers/fmMcp.tspackages/cli-old/template/vite-wv/proofkit-typegen.config.jsoncpackages/cli-old/template/vite-wv/scripts/filemaker.jspackages/cli-old/template/vite-wv/scripts/launch-fm.jspackages/cli-old/template/vite-wv/scripts/upload.jspackages/cli-old/template/vite-wv/src/App.tsxpackages/cli-old/tests/init-scaffold-contract.test.tspackages/cli/src/cli/add/data-source/filemaker.tspackages/cli/src/core/context.tspackages/cli/src/core/resolveInitRequest.tspackages/cli/src/core/types.tspackages/cli/src/generators/fmdapi.tspackages/cli/src/helpers/fmMcp.tspackages/cli/src/services/live.tspackages/cli/src/utils/projectFiles.tspackages/cli/template/fm-addon/ProofKitWV/de.xmlpackages/cli/template/fm-addon/ProofKitWV/en.xmlpackages/cli/template/fm-addon/ProofKitWV/es.xmlpackages/cli/template/fm-addon/ProofKitWV/fr.xmlpackages/cli/template/fm-addon/ProofKitWV/it.xmlpackages/cli/template/fm-addon/ProofKitWV/ja.xmlpackages/cli/template/fm-addon/ProofKitWV/ko.xmlpackages/cli/template/fm-addon/ProofKitWV/nl.xmlpackages/cli/template/fm-addon/ProofKitWV/pt.xmlpackages/cli/template/fm-addon/ProofKitWV/sv.xmlpackages/cli/template/fm-addon/ProofKitWV/zh.xmlpackages/cli/template/vite-wv/proofkit-typegen.config.jsoncpackages/cli/template/vite-wv/scripts/filemaker.jspackages/cli/template/vite-wv/scripts/launch-fm.jspackages/cli/template/vite-wv/scripts/upload.jspackages/cli/template/vite-wv/src/App.tsxpackages/cli/tests/init-scaffold-contract.test.tspackages/cli/tests/resolve-init.test.tspackages/cli/tests/test-layer.tspackages/fmdapi/src/adapters/fm-mcp.tspackages/fmdapi/src/index.tspackages/fmdapi/tests/fm-mcp-adapter.test.tspackages/typegen/live-fm-mcp-output/client/contacts.tspackages/typegen/live-fm-mcp-output/client/index.tspackages/typegen/live-fm-mcp-output/contacts.tspackages/typegen/proofkit-typegen.fm-mcp.local.jsoncpackages/typegen/skills/getting-started/SKILL.mdpackages/typegen/skills/typegen-setup/SKILL.mdpackages/typegen/src/buildLayoutClient.tspackages/typegen/src/constants.tspackages/typegen/src/fmodata/downloadMetadata.tspackages/typegen/src/getEnvValues.tspackages/typegen/src/server/createDataApiClient.tspackages/typegen/src/typegen.tspackages/typegen/src/types.tspackages/typegen/tests/getEnvValues.test.tspackages/typegen/tests/typegen.test.tspackages/typegen/tests/utils/mock-fetch.tspackages/typegen/typegen.schema.jsonpackages/webviewer/src/fm-bridge.tspackages/webviewer/tests/vite-plugins.test.ts
💤 Files with no reviewable changes (1)
- .changeset/fm-http-typegen-flow.md
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| }, | ||
| "baseUrl": { | ||
| "description": "Base URL of the local FM HTTP server. Defaults to \"http://127.0.0.1:1365\". Can also be set via FM_HTTP_BASE_URL env var.", | ||
| "description": "Base URL of the local FM MCP server. Defaults to \"http://127.0.0.1:1365\". Can also be set via FM_HTTP_BASE_URL env var.", |
Contributor
There was a problem hiding this comment.
The environment variable name in the schema description is incorrect. It references the old FM_HTTP_BASE_URL but should reference FM_MCP_BASE_URL to match the actual implementation in constants.ts and the code that reads this environment variable.
"description": "Base URL of the local FM MCP server. Defaults to \"http://127.0.0.1:1365\". Can also be set via FM_MCP_BASE_URL env var.",
Suggested change
| "description": "Base URL of the local FM MCP server. Defaults to \"http://127.0.0.1:1365\". Can also be set via FM_HTTP_BASE_URL env var.", | |
| "description": "Base URL of the local FM MCP server. Defaults to \"http://127.0.0.1:1365\". Can also be set via FM_MCP_BASE_URL env var.", |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
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.
Closes #178
Renames all FM HTTP references to FM MCP — adapter, config fields, types, templates, and docs. Clean break, no deprecated aliases.
Summary by CodeRabbit
Release Notes
New Features
Chores
fmHttptofmMcpacross project files.FM_HTTP_BASE_URLrenamed toFM_MCP_BASE_URL.