fix: removeFMTableNames types - #68
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/typegen
@proofkit/webviewer
commit: |
WalkthroughRefactors removeFMTableNames in packages/fmdapi/src/utils.ts to use a mapped-type approach for key stripping and updates its generic constraint. Adds unit and type-checking tests for this behavior and introduces test-specific TypeScript configurations. Adds a changeset entry documenting a patch release for @proofkit/fmdapi. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Utils as removeFMTableNames
Caller->>Utils: removeFMTableNames(obj: T)
activate Utils
Note right of Utils: Iterate Object.entries(obj)
loop for each [key, value]
Utils->>Utils: Compute mappedKey = key without "::..." suffix (if present)
Utils->>Utils: Assign newObj[mappedKey] = value
end
Utils-->>Caller: newObj as TransformedFields<T>
deactivate Utils
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
🧹 Nitpick comments (5)
packages/fmdapi/tests/tsconfig.typecheck.json (1)
2-13: Type-check config looks solid; consider trimming JSX and tightening compiler flags.
- "jsx": "react-jsx" is unnecessary for a pure type-check suite without JSX. Removing it avoids pulling React typings accidentally.
- Optional hardening that often catches subtle issues in mapped-type-heavy code:
- exactOptionalPropertyTypes: true
- verbatimModuleSyntax: true
Apply this diff if you agree:
"compilerOptions": { "strict": true, "target": "ES2022", "module": "ESNext", "moduleResolution": "Bundler", - "jsx": "react-jsx", "skipLibCheck": true, "noEmit": true, "isolatedModules": true, - "types": ["node"] + "types": ["node"], + "verbatimModuleSyntax": true, + "exactOptionalPropertyTypes": true },packages/fmdapi/src/utils.ts (1)
19-21: Minor: avoid intermediate array allocation.If you keep “first ::” semantics, prefer indexOf + slice over split to reduce allocations in hot paths.
- const mappedKey = ( - typeof key === "string" && key.includes("::") ? key.split("::")[1] : key - ) as keyof TransformedFields<T>; + const mappedKey = (() => { + if (typeof key !== "string") return key; + const i = key.indexOf("::"); + return (i === -1 ? key : key.slice(i + 2)) as keyof TransformedFields<T>; + })();.changeset/swift-swans-rush.md (1)
1-6: Changelog entry reads well and matches the patch scope.Clear and concise. If you adopt the “last ::” semantics (or any behavior change), consider explicitly noting it to help downstream consumers.
packages/fmdapi/tests/tsconfig.json (1)
1-10: Test tsconfig looks appropriate for Vitest; optional small hardening.
- Consider adding verbatimModuleSyntax: true to better align with “Bundler” resolution in modern setups.
- You can also exclude transpiled artifacts like ../coverage if produced by CI.
"compilerOptions": { "noEmit": true, "types": ["vitest", "node"], - "isolatedModules": true + "isolatedModules": true, + "verbatimModuleSyntax": true }, - "include": ["./**/*.ts"], - "exclude": ["../dist", "../schema"] + "include": ["./**/*.ts"], + "exclude": ["../dist", "../schema", "../coverage"]packages/fmdapi/tests/removeFMTableNames.test.ts (1)
5-20: Add a guard-rail test for multiple '::' segments and key collisions.To lock semantics and detect regressions related to the type/runtime split logic, add:
it("strips FM table prefixes from keys and preserves values", () => { // existing… }); + + it("handles multiple '::' segments and resolves collisions deterministically", () => { + const input = { + "A::B::name": "v1", + "A::name": "v2", + } as const; + const out = removeFMTableNames(input); + // Decide on semantics (first or last '::'); adjust expectation accordingly. + // If last '::': both map to "name" and the latter wins in insertion order. + expect(out).toEqual({ name: "v2" }); + });This locks ordering and “first vs last” behavior explicitly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.changeset/swift-swans-rush.md(1 hunks)packages/fmdapi/src/utils.ts(1 hunks)packages/fmdapi/tests/removeFMTableNames.test.ts(1 hunks)packages/fmdapi/tests/tsconfig.json(1 hunks)packages/fmdapi/tests/tsconfig.typecheck.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/fmdapi/tests/removeFMTableNames.test.ts (1)
packages/fmdapi/src/utils.ts (1)
removeFMTableNames(12-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (3)
packages/fmdapi/src/utils.ts (2)
8-11: Mapped-type remap is clean and precise.Key remapping via TransformedFields is concise and correctly preserves value types (with unions on collisions). LGTM.
12-14: Confirm acceptable inputs for T extends object (arrays, functions, class instances).Shifting from Record<string, any> to object admits arrays, functions, and instances. Runtime iterates keys and may produce surprising shapes for arrays (stringified indices) and will ignore non-enumerables on class instances.
- If only plain records are intended, consider narrowing to Record<PropertyKey, unknown> or add a runtime check to exclude arrays.
- Otherwise, add a test demonstrating expected behavior for arrays and symbol keys.
packages/fmdapi/tests/removeFMTableNames.test.ts (1)
5-20: Runtime behavior test is clear and valuable.Covers positive case and ensures prefixed keys are removed. Nice.
| type StripFMTableName<K extends PropertyKey> = K extends `${string}::${infer R}` | ||
| ? R | ||
| : K; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type/runtime mismatch: StripFMTableName uses “last ::” while runtime uses “first ::”.
Currently, the type-level mapper strips after the last '::' (due to ${string}::${infer R} greediness), while the runtime uses key.split("::")[1] (after the first). For keys containing multiple '::', the returned object’s keys won’t match its static type.
Pick ONE of the following to align behavior:
Option A — make runtime match the type (strip after the last '::'):
- const mappedKey = (
- typeof key === "string" && key.includes("::") ? key.split("::")[1] : key
- ) as keyof TransformedFields<T>;
+ const idx = key.lastIndexOf("::");
+ const mappedKey = (
+ typeof key === "string" && idx !== -1 ? key.slice(idx + 2) : key
+ ) as keyof TransformedFields<T>;Option B — make the type match the runtime (strip after the first '::'):
-type StripFMTableName<K extends PropertyKey> = K extends `${string}::${infer R}`
+type StripFMTableName<K extends PropertyKey> = K extends `${infer _}::${infer R}`
? R
: K;If you have domain certainty that keys never contain more than one '::', add a test to lock that invariant; otherwise, I recommend Option A (safer in the presence of extra qualifiers).
📝 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.
| type StripFMTableName<K extends PropertyKey> = K extends `${string}::${infer R}` | |
| ? R | |
| : K; | |
| type StripFMTableName<K extends PropertyKey> = K extends `${infer _}::${infer R}` | |
| ? R | |
| : K; |
| for (const key in obj) { | ||
| if (key.includes("::")) { | ||
| const newKey = key.split("::")[1]; | ||
| newObj[newKey as keyof TransformedFields<T>] = obj[key]; | ||
| } else { | ||
| newObj[key] = obj[key]; | ||
| } | ||
| const originalKey = key as keyof T; | ||
| const value = obj[originalKey]; | ||
| const mappedKey = ( | ||
| typeof key === "string" && key.includes("::") ? key.split("::")[1] : key | ||
| ) as keyof TransformedFields<T>; | ||
|
|
||
| // Use a temporary index signature cast to assign without any | ||
| (newObj as unknown as Record<PropertyKey, unknown>)[ | ||
| mappedKey as unknown as PropertyKey | ||
| ] = value as unknown; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid copying inherited enumerable properties; restrict to own keys.
for..in iterates inherited enumerable props. That can silently pull in prototype properties. Add a hasOwnProperty guard (minimal change), or iterate Object.keys/Reflect.ownKeys instead.
Minimal, surgical fix:
- for (const key in obj) {
+ for (const key in obj) {
+ if (!Object.prototype.hasOwnProperty.call(obj, key)) continue;
const originalKey = key as keyof T;
const value = obj[originalKey];If you want to also preserve symbol keys (currently dropped), consider Reflect.ownKeys:
- for (const key in obj) {
- const originalKey = key as keyof T;
- const value = obj[originalKey];
+ for (const k of Reflect.ownKeys(obj)) {
+ const key = k as keyof T;
+ const value = (obj as Record<PropertyKey, unknown>)[k];…and keep the mappedKey logic the same (only transform when typeof k === "string").
📝 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.
| for (const key in obj) { | |
| if (key.includes("::")) { | |
| const newKey = key.split("::")[1]; | |
| newObj[newKey as keyof TransformedFields<T>] = obj[key]; | |
| } else { | |
| newObj[key] = obj[key]; | |
| } | |
| const originalKey = key as keyof T; | |
| const value = obj[originalKey]; | |
| const mappedKey = ( | |
| typeof key === "string" && key.includes("::") ? key.split("::")[1] : key | |
| ) as keyof TransformedFields<T>; | |
| // Use a temporary index signature cast to assign without any | |
| (newObj as unknown as Record<PropertyKey, unknown>)[ | |
| mappedKey as unknown as PropertyKey | |
| ] = value as unknown; | |
| } | |
| for (const key in obj) { | |
| if (!Object.prototype.hasOwnProperty.call(obj, key)) continue; | |
| const originalKey = key as keyof T; | |
| const value = obj[originalKey]; | |
| const mappedKey = ( | |
| typeof key === "string" && key.includes("::") ? key.split("::")[1] : key | |
| ) as keyof TransformedFields<T>; | |
| // Use a temporary index signature cast to assign without any | |
| (newObj as unknown as Record<PropertyKey, unknown>)[ | |
| mappedKey as unknown as PropertyKey | |
| ] = value as unknown; | |
| } |
🤖 Prompt for AI Agents
In packages/fmdapi/src/utils.ts around lines 16 to 27, the for..in loop
currently iterates inherited enumerable properties; change it to only process
the object's own keys by either adding a hasOwnProperty guard inside the loop
(if (!Object.prototype.hasOwnProperty.call(obj, key)) continue;) or replace the
loop with Object.keys(obj).forEach(...) to iterate own string keys; if you need
to preserve symbol keys instead use Reflect.ownKeys(obj).forEach and keep the
existing mappedKey transformation logic unchanged.
| it("produces a type with stripped keys", () => { | ||
| type Input = { | ||
| "Customer::first_name": string; | ||
| last_name: string; | ||
| "Portal::recordId": number; | ||
| }; | ||
|
|
||
| const input: Input = { | ||
| "Customer::first_name": "Bob", | ||
| last_name: "Builder", | ||
| "Portal::recordId": 42, | ||
| }; | ||
| const out = removeFMTableNames(input); | ||
|
|
||
| // Type-level assertions via assignment | ||
| const first: string = out.first_name; | ||
| const last: string = out.last_name; | ||
| const recId: number = out.recordId; | ||
| expect(first).toBe("Bob"); | ||
| expect(last).toBe("Builder"); | ||
| expect(recId).toBe(42); | ||
|
|
||
| // Old keys should not exist on the resulting type | ||
| // @ts-expect-error - old FM-prefixed key should not exist on result type | ||
| out["Customer::first_name"]; | ||
| // @ts-expect-error - old FM-prefixed key should not exist on result type | ||
| out["Portal::recordId"]; | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ts-expect-error in runtime tests isn’t enforced by Vitest; wire up the dedicated type-check instead.
Vitest (via esbuild) doesn’t type-check by default, so the @ts-expect-error assertions here won’t fail builds. You already added a dedicated type-check config—great. Ensure CI runs it.
Run locally/CI:
Optionally, move the @ts-expect-error assertions to packages/fmdapi/tests/type/removeFMTableNames.typecheck.ts to keep runtime tests focused.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Type-check only:
npx -y tsc -p packages/fmdapi/tests/tsconfig.typecheck.json --noEmitLength of output: 556
Type-check setup is broken – CI will fail until it’s fixed
The dedicated type-check command (npx tsc -p packages/fmdapi/tests/tsconfig.typecheck.json --noEmit) currently errors out:
- TS2688: Cannot find type definition file for 'node'
- TS6053: File
packages/fmdapi/tests/type/removeFMTableNames.typecheck.tsnot found
You need to:
- Add the missing Node type defs (
npm install -D @types/node) sotsccan find thenodelibrary. - Either create the expected type-check file (
packages/fmdapi/tests/type/removeFMTableNames.typecheck.ts)—moving just the@ts-expect-errorlines from the runtime test into it—or updatetsconfig.typecheck.json’sfileslist to point at your existing.test.tsfile.
Once the above is addressed, ensure your CI invokes:
npx tsc -p packages/fmdapi/tests/tsconfig.typecheck.json --noEmitso that any stray or missing @ts-expect-error assertions will fail the build rather than slip through Vitest’s runtime tests.
🤖 Prompt for AI Agents
packages/fmdapi/tests/removeFMTableNames.test.ts lines 22-49: Type-check CI is
failing because Node type defs are missing and the type-check harness expects a
separate file; install dev dependency @types/node (npm i -D @types/node) so tsc
finds the node lib, then either create
packages/fmdapi/tests/type/removeFMTableNames.typecheck.ts containing the two
@ts-expect-error lines moved out of the runtime test OR update
packages/fmdapi/tests/tsconfig.typecheck.json’s "files" array to point at the
existing .test.ts file, and finally ensure CI invokes npx tsc -p
packages/fmdapi/tests/tsconfig.typecheck.json --noEmit so type-level
@ts-expect-error assertions fail the build if incorrect.
| "compilerOptions": { | ||
| "strict": true, | ||
| "target": "ES2022", | ||
| "module": "ESNext", | ||
| "moduleResolution": "Bundler", | ||
| "jsx": "react-jsx", | ||
| "skipLibCheck": true, | ||
| "noEmit": true, | ||
| "isolatedModules": true, | ||
| "types": ["node"] | ||
| }, | ||
| "files": ["./type/removeFMTableNames.typecheck.ts"] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure your toolchain TS version supports moduleResolution: "Bundler".
Bundler resolution requires a relatively recent TypeScript. Please confirm your CI/runner uses a compatible TS version; otherwise, this config will be ignored or fail.
You can verify with:
🏁 Script executed:
#!/bin/bash
# Expect TS >= 5.x supporting "Bundler" moduleResolution.
npx -y tsc -v
# Also prove the typecheck config compiles:
npx -y tsc -p packages/fmdapi/tests/tsconfig.typecheck.json --noEmitLength of output: 677
Fix TypeScript type‐check configuration errors
Verification shows your CI is using TypeScript 5.9.2, so moduleResolution: "Bundler" is supported. However, the type-check step is failing:
• TS2688: Cannot find type definition file for ‘node’.
• TS6053: File ‘packages/fmdapi/tests/type/removeFMTableNames.typecheck.ts’ not found.
To resolve:
- Ensure Node.js type definitions are installed for this project. For example, in
packages/fmdapi, run:or addnpm install --save-dev @types/node
"@types/node": "…"underdevDependencies. - Correct the path in
packages/fmdapi/tests/tsconfig.typecheck.jsonso it matches where the.typecheck.tsfile actually lives. For example, if the file is intests/types/, update:Otherwise, adjust to the correct subdirectory or move the file."compilerOptions": { "types": ["node"], … }, "files": [ - "./type/removeFMTableNames.typecheck.ts" + "./types/removeFMTableNames.typecheck.ts" ]
After making these fixes, re-run:
npx tsc -v
npx tsc -p packages/fmdapi/tests/tsconfig.typecheck.json --noEmit🤖 Prompt for AI Agents
packages/fmdapi/tests/tsconfig.typecheck.json lines 2-13: The typecheck config
is failing because Node.js types are missing (TS2688) and the referenced
typecheck file path is incorrect (TS6053); install or add "@types/node" to
devDependencies in packages/fmdapi (e.g., npm install --save-dev @types/node or
add the exact version to package.json) and update the "files" entry to the real
location of removeFMTableNames.typecheck.ts (or move the file to match the path)
so tsc can find it, then re-run the typecheck command.
1 participant
fix: removeFMTableNames types
fix: update FM table names for consistency
Summary by CodeRabbit
New Release
Refactor
Tests
Chores