Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
eluce2
changed the title
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces ExecutionContext/fetch plumbing with an Effect-driven DI layer (FMODataLayer); adds service types, client runtime, effect helpers, mutation/read helper modules, and MockFMServerConnection testing utilities; migrates builders, Database, EntitySet, and request/response flows to layer/config-driven, Effect-based pipelines. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(135,206,235,0.5)
participant Builder as Builder (Query/Mutation)
end
rect rgba(144,238,144,0.5)
participant Effect as Effect Pipeline
participant Layer as FMODataLayer
end
rect rgba(255,182,193,0.5)
participant HTTP as HttpClient
participant Parser as Response Parser
end
Builder->>Effect: execute(options) (uses layer)
activate Effect
Effect->>Layer: access HttpClient, ODataConfig, Logger
Effect->>HTTP: requestFromService(url, opts)
activate HTTP
HTTP->>HTTP: apply retry / spans
HTTP-->>Parser: raw response
deactivate HTTP
Parser->>Effect: parsed data or classified error
Effect-->>Builder: Result<T>
deactivate Effect
sequenceDiagram
participant Test as Test Code
participant Mock as MockFMServerConnection
participant Router as Router Fetch
participant Spy as RequestSpy
participant DB as Database Instance
participant Builder as Query Builder
Test->>Mock: new MockFMServerConnection()
Test->>Mock: addRoute(MockRoute)
Mock->>Router: register route
Test->>Mock: db = mock.database("db")
DB->>Builder: Builder created from db
Builder->>Router: HTTP request
Router->>Spy: record request (if enabled)
Router-->>Builder: mock response
Builder-->>Test: Result<T>
Test->>Spy: spy.forUrl(pattern)
Spy-->>Test: calls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/fmodata/src/client/query/query-builder.ts (1)
264-275:⚠️ Potential issue | 🟠 MajorSerialize ORM filters with the effective
useEntityIdsvalue.
where()bakes theFilterExpressionwiththis.config.useEntityIds, butexecute()andgetQueryString()can override that flag later per request. A query built under one mode and executed under the other will mix field names and FMFIDs in the same URL, which can produce invalid OData. Please defertoODataFilter()until the finaluseEntityIdsvalue is known.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/query/query-builder.ts` around lines 264 - 275, The where() method currently converts a FilterExpression to a string immediately using this.config.useEntityIds; instead, keep the raw FilterExpression (or the original string) in queryOptions (e.g., store as filterExpression or reuse queryOptions.filter but as a FilterExpression type) and do not call FilterExpression.toODataFilter there; update getQueryString() and execute() to detect if queryOptions.filter is a FilterExpression and call toODataFilter(effectively using the final useEntityIds value determined at request time, including any overrides) to produce the actual filter string before building the URL so the final serialization uses the correct effective useEntityIds.packages/fmodata/src/client/batch-builder.ts (1)
94-97:⚠️ Potential issue | 🟠 Major
addRequest()returnsthisinstead of a new type with the extended builder tuple, breaking type safety.
Buildersis fixed when the batch is constructed (line 67). When you calladdRequest<T>()and returnthis, the type remainsBatchBuilder<Builders>even though the runtimebuildersarray has grown. This meansexecute()returnsBatchResult<ExtractTupleTypes<Builders>>based on the original tuple, not the dynamically added builders. For example:const batch = db.batch([]); // Type: BatchBuilder<[]> batch.addRequest(someBuilder); // Still type: BatchBuilder<[]> const result = await batch.execute(); // Type: BatchResult<[]> ≠ runtime resultReturn a new type that extends the tuple:
BatchBuilder<[...Builders, ExecutableBuilder<T>]>to keep the type system in sync with runtime additions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/batch-builder.ts` around lines 94 - 97, The addRequest method currently returns this and therefore preserves the original generic tuple type (BatchBuilder<Builders>) while mutating the runtime builders array; change its signature to return a new generic tuple type (BatchBuilder<[...Builders, ExecutableBuilder<T>]>), update the implementation to push the builder into the internal builders array and return the object typed as BatchBuilder<[...Builders, ExecutableBuilder<T>]> (or construct and return a new BatchBuilder instance with the extended tuple type) so that execute() and BatchResult<ExtractTupleTypes<...>> reflect the added builder; update references to addRequest, BatchBuilder, Builders, ExecutableBuilder, execute, ExtractTupleTypes and BatchResult accordingly.
🧹 Nitpick comments (12)
packages/fmodata/src/validation.ts (3)
358-359: Inconsistent variable naming with the selectedFields branch.The all-fields branch uses
allIssuesAll/failedFieldsAll(lines 358-359), while the selectedFields branch usesallIssues/failedFields(lines 177-178). Since these are in separate branches with no collision risk, using the same names (allIssues/failedFields) would improve consistency and readability.♻️ Suggested fix
- const allIssuesAll: StandardSchemaV1.Issue[] = []; - const failedFieldsAll: string[] = []; + const allIssues: StandardSchemaV1.Issue[] = []; + const failedFields: string[] = [];Then update all references (
allIssuesAll→allIssues,failedFieldsAll→failedFields) in lines 377-416.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/validation.ts` around lines 358 - 359, Rename the variables in the all-fields branch to match the selectedFields branch by replacing allIssuesAll → allIssues and failedFieldsAll → failedFields, and update every reference to those symbols (currently lines around where allIssuesAll and failedFieldsAll are used, previously suggested in lines 377-416) so the all-fields branch uses the same variable names as the selectedFields branch for consistency and readability.
406-416: Same redundantcausepattern as the selectedFields branch.Consistent with the earlier suggestion,
cause: allIssuesAll(orallIssuesafter renaming) duplicates data already passed as the second parameter.♻️ Suggested fix
error: new ValidationError( - `Validation failed for field${failedFieldsAll.length > 1 ? "s" : ""} '${failedFieldsAll.join("', '")}'`, - allIssuesAll, - { field: failedFieldsAll[0], value: record, cause: allIssuesAll }, + `Validation failed for field${failedFields.length > 1 ? "s" : ""} '${failedFields.join("', '")}'`, + allIssues, + { field: failedFields[0], value: record }, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/validation.ts` around lines 406 - 416, The ValidationError construction in validation.ts is passing the same issues twice by setting cause: allIssuesAll while also supplying allIssuesAll as the second argument; update the error creation in the block that returns valid: false so it does not duplicate data—remove the cause: allIssuesAll property (or set cause to a distinct value such as the offending record or undefined) in the new ValidationError call that uses ValidationError(..., allIssuesAll, { field: failedFieldsAll[0], value: record, ... }), keeping the message, second-argument issues array (allIssuesAll), and contextual metadata (field/ value) intact.
239-249: Redundantcauseassignment duplicates the issues array.The
allIssuesarray is already passed as the second parameter toValidationError. Settingcause: allIssuesduplicates this data. Thecauseproperty is typically used for error chaining (linking to an originating error), not for repeating already-available information.♻️ Suggested fix
return { valid: false, error: new ValidationError( `Validation failed for field${failedFields.length > 1 ? "s" : ""} '${failedFields.join("', '")}'`, allIssues, - { field: failedFields[0], value: record, cause: allIssues }, + { field: failedFields[0], value: record }, ), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/validation.ts` around lines 239 - 249, The ValidationError construction in the validation routine is redundantly setting cause: allIssues while already passing allIssues as the second parameter; remove the cause property from the ValidationError options so only the error details are passed once. Locate the ValidationError instantiation (uses symbols allIssues, failedFields, record) and delete the cause: allIssues entry from the options object, leaving field and value (or other relevant metadata) intact.packages/fmodata/src/testing.ts (2)
123-128: Consider documenting first-match-wins behavior.Route matching uses
Array.find(), so the first matching route wins. This is a common and sensible approach, but it could be helpful to document this in theMockRouteinterface oraddRoutemethod JSDoc so test authors know that more specific routes should be added first.📝 Suggested documentation addition
/** * Add a route to the mock. Routes added after construction are picked up * automatically by subsequent requests. + * Routes are matched in order; the first matching route wins. */ addRoute(route: MockRoute): this {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/testing.ts` around lines 123 - 128, Update the docs/comments to state that route matching is first-match-wins because it uses Array.find(): explicitly mention this behavior in the MockRoute interface JSDoc and/or the addRoute method JSDoc so test authors know to register more specific routes before generic ones; reference the matching logic that uses routes.find and the urlPattern/method checks to clarify what constitutes a "match."
163-175: Accept header extraction could miss array-style headers.The code handles
Headers, plain objects, andRequestinputs, but the array-style headers ([["Accept", "value"]]) are silently ignored on line 170. While this is an edge case, it could cause unexpected behavior if a consumer uses array headers.📝 Suggested fix for array headers
if (init.headers instanceof Headers) { acceptHeader = init.headers.get("Accept") ?? ""; } else if (!Array.isArray(init.headers)) { acceptHeader = (init.headers as Record<string, string>).Accept ?? (init.headers as Record<string, string>).accept ?? ""; + } else { + // Array-style headers: [["Accept", "value"]] + const acceptEntry = init.headers.find(([key]) => key.toLowerCase() === "accept"); + acceptHeader = acceptEntry?.[1] ?? ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/testing.ts` around lines 163 - 175, The Accept header extraction misses array-style headers when init.headers is an array; update the logic around acceptHeader so that when init?.headers is an Array (e.g., Array<[string,string]>) you iterate the entries and pick the first header where key equals "Accept" or "accept" to set acceptHeader; keep existing handling for Request, Headers, and plain object paths and then compute shouldStripAnnotations based on acceptHeader.includes("odata.metadata=none").packages/fmodata/tests/list-methods.test.ts (1)
5-8: Consider per-test mock setup to prevent future state bleed.
mock/dbare currently module-scoped. If this suite grows and starts mutating routes or spy state, tests can become order-dependent. Optional: move setup intobeforeEach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/tests/list-methods.test.ts` around lines 5 - 8, Tests use a module-scoped MockFMServerConnection (mock), DB_NAME and db which can cause state bleed between tests; move creation of MockFMServerConnection, mock.addRoute({ urlPattern: DB_NAME, ... }) and mock.database(DB_NAME) into a per-test setup (beforeEach) and reset/teardown in afterEach to ensure each test gets a fresh MockFMServerConnection and route state, referencing the MockFMServerConnection constructor, mock.addRoute and mock.database calls when updating the test file.packages/fmodata/src/index.ts (1)
117-118: Avoid expanding the barrel surface fromindex.ts.These new re-exports increase coupling to the barrel entrypoint. Consider exposing and consuming these through explicit subpath exports (for example
@proofkit/fmodata/servicesand@proofkit/fmodata/types) instead of growingindex.ts.As per coding guidelines,
**/index.{js,jsx,ts,tsx}: Avoid barrel files (index files that re-export everything).Also applies to: 130-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/index.ts` around lines 117 - 118, The index.ts barrel now re-exports FMODataLayer, HttpClient, ODataConfig, and ODataLogger which expands the barrel surface; remove these re-exports from index.ts and instead expose them via explicit subpath exports (e.g., create/use a dedicated entry like ./services and ./types and add corresponding export entries in the package's "exports" mapping), update any internal imports to import from "./services" or the package subpath (e.g., `@proofkit/fmodata/services`) rather than from the root barrel, and ensure the unique symbols FMODataLayer, HttpClient, ODataConfig, and ODataLogger are exported only from the services/types modules and referenced there.packages/fmodata/src/errors.ts (1)
236-238: Use narrowing instead of a direct type assertion forname.The cast on Line 237 weakens type safety and can over-classify arbitrary objects. Narrow
namevia runtime checks instead.Proposed refactor
- if (error && typeof error === "object" && "name" in error) { - const name = (error as { name: string }).name; - if (name === "NetworkError" || name === "TimeoutError") { - return true; - } - } + if (error && typeof error === "object") { + const name = Reflect.get(error, "name"); + if (typeof name === "string" && (name === "NetworkError" || name === "TimeoutError")) { + return true; + } + }As per coding guidelines,
**/*.{ts,tsx}: Leverage TypeScript's type narrowing instead of type assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/errors.ts` around lines 236 - 238, Replace the direct type assertion (error as { name: string }).name with runtime narrowing: after confirming error is an object and "name" in error, check that (error as unknown)['name'] is a string (e.g. typeof (error as Record<string, unknown>)['name'] === "string") and then assign that value to a const name without asserting its type; update the conditional that compares name to "NetworkError" or "TimeoutError" to use this narrowed name value (symbols: error and name in errors.ts).packages/fmodata/tests/insert.test.ts (1)
113-137: Keep the default-update branch type-only, or give it a count-specific mock.The
countResultbranch is only used inexpectTypeOf, but it still performs a realexecute()against the insert-shaped mock configured above. That makes the test depend on the wrong response contract while still not asserting the default update runtime path. Either assert the type directly onexecutewithout awaiting it, or add a dedicated count-returningPATCHmock and check the returnedupdatedCount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/tests/insert.test.ts` around lines 113 - 137, The test currently awaits countResult which hits the insert-shaped mock; either keep the "default-update" branch type-only by not awaiting execute and performing the type assertion directly on the returned promise (e.g. call db.from(contacts).update(...).byId(...).execute() and run expectTypeOf on that promise result type), or add a dedicated PATCH/count mock on MockFMServerConnection that returns a count-shaped response and then await countResult and assert countResult.data.updatedCount; update the test around the countResult variable and the MockFMServerConnection routes (referencing MockFMServerConnection, db.from(...).update, .byId, and .execute) accordingly.packages/fmodata/tests/field-id-transforms.test.ts (1)
477-479: Consider removing or convertingconsole.logto a test-appropriate assertion.The
console.logstatement in the error branch may clutter test output. If this information is important, consider using a dedicated test logger or converting it to an assertion that documents the expected behavior.if (result.error) { // If validation failed, check raw response to ensure transformation occurred - console.log("Note: Validation failed for expanded data (expected - dynamic fields)"); + // Validation failure is expected for expanded data with dynamic fields + expect(result.error).toBeDefined(); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/tests/field-id-transforms.test.ts` around lines 477 - 479, Remove the stray console.log in the error branch of the test that checks expanded data (the block that inspects result.error) and either delete it or replace it with a test assertion that documents the expected behavior (e.g., assert that transformation occurred on the raw response or assert that validation failed for dynamic fields); update the branch around result.error in the field-id-transforms.test.ts test to use expect/other test helpers instead of console.log so test output remains clean and meaningful.packages/fmodata/tests/use-entity-ids-override.test.ts (1)
86-120: Consider extracting repeatedlocalContactsTOdefinition to reduce duplication.The
localContactsTOtable occurrence is defined identically in multiple test blocks (Lines 87-96, 123-132, 157-166). While per-test isolation is valid, extracting this to a shared constant at the describe block level would reduce repetition without compromising test independence.+ // Shared table occurrence for tests that need local scope + const localContactsTO = fmTableOccurrence( + "contacts", + { + id: textField().primaryKey().entityId("FMFID:1"), + name: textField().entityId("FMFID:2"), + }, + { + entityId: "FMTID:100", + }, + ); + it("should work with insert operations", async () => { - const localContactsTO = fmTableOccurrence( - "contacts", - { - id: textField().primaryKey().entityId("FMFID:1"), - name: textField().entityId("FMFID:2"), - }, - { - entityId: "FMTID:100", - }, - ); - const mock = new MockFMServerConnection({ enableSpy: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/tests/use-entity-ids-override.test.ts` around lines 86 - 120, Extract the repeated localContactsTO definition (the fmTableOccurrence call that uses textField().primaryKey().entityId("FMFID:1"), textField().entityId("FMFID:2"), and entityId: "FMTID:100") into a single const declared at the describe block level and reuse that constant in each it block; if any test mutates localContactsTO, ensure tests get a fresh instance by cloning or creating a helper function (e.g., makeLocalContactsTO()) that returns the fmTableOccurrence to preserve per-test isolation while removing duplication.packages/fmodata/tests/mock.test.ts (1)
16-16: Consider using vitest'sassertinstead ofnode:console.Line 16 imports
assertfromnode:console, but Line 19 already imports fromvitestwhich provides assertion utilities. Theassertfrom vitest (already imported on Line 19) would be more consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/tests/mock.test.ts` at line 16, Replace the Node console assert import with Vitest's assert: remove or change the import statement that reads import { assert } from "node:console" and use the already-available vitest assertion utilities (the existing vitest import) so tests use vitest's assert instead of node:console's; update any references to ensure they import from vitest (or rely on the current vitest import) rather than node:console.
🤖 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/fmodata/src/client/entity-set.ts`:
- Around line 111-113: The child builders (created in list(), get(), and
navigate()) currently bake request-scoped defaults by calling
builder.select(allColumns, systemColumns) and resolving relation/source IDs
using this.config.useEntityIds (e.g., assigned to selectedBuilder), which
prevents execute-time overrides like execute({ includeSpecialColumns: false })
or switching ID modes; change this by removing the eager injection/resolution:
do not pass systemColumns or pre-resolve IDs when creating the child builder
(leave systemColumns/ID-mode undefined on the builder), and instead surface
these defaults in the execute path so execute merges this.config and runtime
options before building the final select/navigation SQL; update builder.select
usage (and any pre-resolved navigation path code) in list(), get(), navigate()
and the other similar occurrences (the other two spots flagged) so defaults are
applied at execute-time rather than baked into selectedBuilder.
In `@packages/fmodata/src/client/filemaker-odata.ts`:
- Around line 321-327: The current code always wraps the pipeline with
withRetryPolicy using retryPolicy (variable retryPolicy), which can replay
unsafe write operations; change the logic in the filemaker-odata request flow so
you only call withRetryPolicy(pipeline, retryPolicy) when finalOptions.method is
one of the safe/idempotent methods (e.g., "GET", "HEAD", "OPTIONS", and
optionally "PUT") and retryPolicy is present; for write methods ("POST",
"PATCH", "DELETE") return withSpan(pipeline, "fmodata.request", {...}) without
applying withRetryPolicy; keep the tracing span wrapping via withSpan and
preserve the same attributes ("fmodata.url": url, "fmodata.method":
finalOptions.method ?? "GET") regardless of whether retries were applied.
In `@packages/fmodata/src/client/insert-builder.ts`:
- Around line 185-195: The insert request currently spreads mergedOptions after
method/headers/body so callers can override method, body, and required headers
(Content-Type/Prefer); to fix, ensure mergedOptions cannot clobber these by
extracting and excluding method, headers, and body from mergedOptions before
spreading (e.g., const { method, headers, body, ...rest } = mergedOptions) and
then pass ...rest into requestFromService while building headers by merging
caller headers into your headers with caller headers first and your headers last
(so Content-Type and Prefer from preferHeader win), and always set method:
"POST" and body: JSON.stringify(transformedData) explicitly when invoking
requestFromService (reference mergedOptions, preferHeader, transformedData, and
the requestFromService call that returns responseData).
In `@packages/fmodata/src/client/record-builder.ts`:
- Around line 248-250: The builder currently computes and stores frozen
identifiers early (see getSingleField and navigate setting
mutableBuilder.operationParam via
column.getFieldIdentifier(this.config.useEntityIds)), which breaks when
execute() is given a different request-scoped useEntityIds; change the builder
to store the column/relation metadata (e.g., keep operationColumn and a flag
indicating the operation type) instead of a precomputed operationParam, and in
execute() derive the final identifier by calling
column.getFieldIdentifier(requestUseEntityIds) (or the equivalent relation
identifier method) at request-build time before constructing the URL; update the
similar logic referenced around lines ~455-471 as well so all path segments are
resolved during execute(), not at builder construction.
In `@packages/fmodata/src/client/update-builder.ts`:
- Around line 163-183: buildUrl currently always calls
this.queryBuilder.getQueryString() without the resolved useEntityIds flag,
causing mismatched naming modes; update the function signature of buildUrl to
accept a boolean useEntityIds and in execute() pass mergedOptions.useEntityIds
when calling buildUrl; inside buildUrl call this.queryBuilder.getQueryString({
useEntityIds }) instead of the no-arg variant, and keep the same logic that
trims the leading /{tableId} or /{tableName} from the returned queryString to
form the final URL.
In `@packages/fmodata/src/effect.ts`:
- Around line 50-56: runAsResult currently only catches typed failures via
Effect.catchAll and lets defects (exceptions thrown inside
Effect.gen/Effect.sync, e.g. InvalidLocationHeaderError thrown by
parseLocationHeader used in insert-builder) escape as rejected promises; update
runAsResult to convert defects into the Result error channel by using
Effect.catchAllCause to inspect the Cause and map defects into a typed
FMODataErrorType (or wrap the risky call sites instead using
Effect.try/Effect.tryCatch around parseLocationHeader in insert-builder), so
that Effect.runPromise always resolves to Result<T> rather than rejecting;
specifically modify the runAsResult function (or wrap parseLocationHeader calls)
to handle defects and return Result.error for those cases.
In `@packages/fmodata/tests/validation.test.ts`:
- Around line 78-82: The test's expand callback currently uses an explicit "(b:
any)" annotation which disables type inference and bypasses type checking;
remove the ": any" so the callback is "b => b.select({ name: users.name,
fake_field: users.fake_field })" (i.e., use expand(users, b => ...) ) to let
TypeScript infer the builder type for the expand/select chain (references:
expand, select, users, contacts, db, execute).
---
Outside diff comments:
In `@packages/fmodata/src/client/batch-builder.ts`:
- Around line 94-97: The addRequest method currently returns this and therefore
preserves the original generic tuple type (BatchBuilder<Builders>) while
mutating the runtime builders array; change its signature to return a new
generic tuple type (BatchBuilder<[...Builders, ExecutableBuilder<T>]>), update
the implementation to push the builder into the internal builders array and
return the object typed as BatchBuilder<[...Builders, ExecutableBuilder<T>]> (or
construct and return a new BatchBuilder instance with the extended tuple type)
so that execute() and BatchResult<ExtractTupleTypes<...>> reflect the added
builder; update references to addRequest, BatchBuilder, Builders,
ExecutableBuilder, execute, ExtractTupleTypes and BatchResult accordingly.
In `@packages/fmodata/src/client/query/query-builder.ts`:
- Around line 264-275: The where() method currently converts a FilterExpression
to a string immediately using this.config.useEntityIds; instead, keep the raw
FilterExpression (or the original string) in queryOptions (e.g., store as
filterExpression or reuse queryOptions.filter but as a FilterExpression type)
and do not call FilterExpression.toODataFilter there; update getQueryString()
and execute() to detect if queryOptions.filter is a FilterExpression and call
toODataFilter(effectively using the final useEntityIds value determined at
request time, including any overrides) to produce the actual filter string
before building the URL so the final serialization uses the correct effective
useEntityIds.
---
Nitpick comments:
In `@packages/fmodata/src/errors.ts`:
- Around line 236-238: Replace the direct type assertion (error as { name:
string }).name with runtime narrowing: after confirming error is an object and
"name" in error, check that (error as unknown)['name'] is a string (e.g. typeof
(error as Record<string, unknown>)['name'] === "string") and then assign that
value to a const name without asserting its type; update the conditional that
compares name to "NetworkError" or "TimeoutError" to use this narrowed name
value (symbols: error and name in errors.ts).
In `@packages/fmodata/src/index.ts`:
- Around line 117-118: The index.ts barrel now re-exports FMODataLayer,
HttpClient, ODataConfig, and ODataLogger which expands the barrel surface;
remove these re-exports from index.ts and instead expose them via explicit
subpath exports (e.g., create/use a dedicated entry like ./services and ./types
and add corresponding export entries in the package's "exports" mapping), update
any internal imports to import from "./services" or the package subpath (e.g.,
`@proofkit/fmodata/services`) rather than from the root barrel, and ensure the
unique symbols FMODataLayer, HttpClient, ODataConfig, and ODataLogger are
exported only from the services/types modules and referenced there.
In `@packages/fmodata/src/testing.ts`:
- Around line 123-128: Update the docs/comments to state that route matching is
first-match-wins because it uses Array.find(): explicitly mention this behavior
in the MockRoute interface JSDoc and/or the addRoute method JSDoc so test
authors know to register more specific routes before generic ones; reference the
matching logic that uses routes.find and the urlPattern/method checks to clarify
what constitutes a "match."
- Around line 163-175: The Accept header extraction misses array-style headers
when init.headers is an array; update the logic around acceptHeader so that when
init?.headers is an Array (e.g., Array<[string,string]>) you iterate the entries
and pick the first header where key equals "Accept" or "accept" to set
acceptHeader; keep existing handling for Request, Headers, and plain object
paths and then compute shouldStripAnnotations based on
acceptHeader.includes("odata.metadata=none").
In `@packages/fmodata/src/validation.ts`:
- Around line 358-359: Rename the variables in the all-fields branch to match
the selectedFields branch by replacing allIssuesAll → allIssues and
failedFieldsAll → failedFields, and update every reference to those symbols
(currently lines around where allIssuesAll and failedFieldsAll are used,
previously suggested in lines 377-416) so the all-fields branch uses the same
variable names as the selectedFields branch for consistency and readability.
- Around line 406-416: The ValidationError construction in validation.ts is
passing the same issues twice by setting cause: allIssuesAll while also
supplying allIssuesAll as the second argument; update the error creation in the
block that returns valid: false so it does not duplicate data—remove the cause:
allIssuesAll property (or set cause to a distinct value such as the offending
record or undefined) in the new ValidationError call that uses
ValidationError(..., allIssuesAll, { field: failedFieldsAll[0], value: record,
... }), keeping the message, second-argument issues array (allIssuesAll), and
contextual metadata (field/ value) intact.
- Around line 239-249: The ValidationError construction in the validation
routine is redundantly setting cause: allIssues while already passing allIssues
as the second parameter; remove the cause property from the ValidationError
options so only the error details are passed once. Locate the ValidationError
instantiation (uses symbols allIssues, failedFields, record) and delete the
cause: allIssues entry from the options object, leaving field and value (or
other relevant metadata) intact.
In `@packages/fmodata/tests/field-id-transforms.test.ts`:
- Around line 477-479: Remove the stray console.log in the error branch of the
test that checks expanded data (the block that inspects result.error) and either
delete it or replace it with a test assertion that documents the expected
behavior (e.g., assert that transformation occurred on the raw response or
assert that validation failed for dynamic fields); update the branch around
result.error in the field-id-transforms.test.ts test to use expect/other test
helpers instead of console.log so test output remains clean and meaningful.
In `@packages/fmodata/tests/insert.test.ts`:
- Around line 113-137: The test currently awaits countResult which hits the
insert-shaped mock; either keep the "default-update" branch type-only by not
awaiting execute and performing the type assertion directly on the returned
promise (e.g. call db.from(contacts).update(...).byId(...).execute() and run
expectTypeOf on that promise result type), or add a dedicated PATCH/count mock
on MockFMServerConnection that returns a count-shaped response and then await
countResult and assert countResult.data.updatedCount; update the test around the
countResult variable and the MockFMServerConnection routes (referencing
MockFMServerConnection, db.from(...).update, .byId, and .execute) accordingly.
In `@packages/fmodata/tests/list-methods.test.ts`:
- Around line 5-8: Tests use a module-scoped MockFMServerConnection (mock),
DB_NAME and db which can cause state bleed between tests; move creation of
MockFMServerConnection, mock.addRoute({ urlPattern: DB_NAME, ... }) and
mock.database(DB_NAME) into a per-test setup (beforeEach) and reset/teardown in
afterEach to ensure each test gets a fresh MockFMServerConnection and route
state, referencing the MockFMServerConnection constructor, mock.addRoute and
mock.database calls when updating the test file.
In `@packages/fmodata/tests/mock.test.ts`:
- Line 16: Replace the Node console assert import with Vitest's assert: remove
or change the import statement that reads import { assert } from "node:console"
and use the already-available vitest assertion utilities (the existing vitest
import) so tests use vitest's assert instead of node:console's; update any
references to ensure they import from vitest (or rely on the current vitest
import) rather than node:console.
In `@packages/fmodata/tests/use-entity-ids-override.test.ts`:
- Around line 86-120: Extract the repeated localContactsTO definition (the
fmTableOccurrence call that uses textField().primaryKey().entityId("FMFID:1"),
textField().entityId("FMFID:2"), and entityId: "FMTID:100") into a single const
declared at the describe block level and reuse that constant in each it block;
if any test mutates localContactsTO, ensure tests get a fresh instance by
cloning or creating a helper function (e.g., makeLocalContactsTO()) that returns
the fmTableOccurrence to preserve per-test isolation while removing duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8feaec2f-52a8-4fb7-9fea-553d632d72c8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (47)
packages/fmodata/package.jsonpackages/fmodata/skills/fmodata-client/SKILL.mdpackages/fmodata/src/cli/commands/query.tspackages/fmodata/src/client/batch-builder.tspackages/fmodata/src/client/builders/shared-types.tspackages/fmodata/src/client/builders/table-utils.tspackages/fmodata/src/client/database.tspackages/fmodata/src/client/delete-builder.tspackages/fmodata/src/client/entity-set.tspackages/fmodata/src/client/filemaker-odata.tspackages/fmodata/src/client/insert-builder.tspackages/fmodata/src/client/query/query-builder.tspackages/fmodata/src/client/query/url-builder.tspackages/fmodata/src/client/record-builder.tspackages/fmodata/src/client/schema-manager.tspackages/fmodata/src/client/update-builder.tspackages/fmodata/src/client/webhook-builder.tspackages/fmodata/src/effect.tspackages/fmodata/src/errors.tspackages/fmodata/src/index.tspackages/fmodata/src/services.tspackages/fmodata/src/testing.tspackages/fmodata/src/types.tspackages/fmodata/src/validation.tspackages/fmodata/tests/batch-error-messages.test.tspackages/fmodata/tests/batch.test.tspackages/fmodata/tests/delete.test.tspackages/fmodata/tests/errors.test.tspackages/fmodata/tests/expands.test.tspackages/fmodata/tests/field-id-transforms.test.tspackages/fmodata/tests/filters.test.tspackages/fmodata/tests/fmids-validation.test.tspackages/fmodata/tests/include-special-columns.test.tspackages/fmodata/tests/insert.test.tspackages/fmodata/tests/list-methods.test.tspackages/fmodata/tests/metadata.test.tspackages/fmodata/tests/mock.test.tspackages/fmodata/tests/navigate.test.tspackages/fmodata/tests/query-strings.test.tspackages/fmodata/tests/record-builder-select-expand.test.tspackages/fmodata/tests/scripts.test.tspackages/fmodata/tests/typescript.test.tspackages/fmodata/tests/update.test.tspackages/fmodata/tests/use-entity-ids-override.test.tspackages/fmodata/tests/validation.test.tspackages/fmodata/tests/webhooks.test.tspackages/fmodata/vite.config.ts
💤 Files with no reviewable changes (1)
- packages/fmodata/src/client/builders/shared-types.ts
|
No open human review comments were found in this PR to create a plan for. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/fmodata/src/client/record-builder.ts (2)
289-297:⚠️ Potential issue | 🟡 MinorEntity-ID path segments are resolved at builder construction, not execution.
getSingleField()freezesoperationParamusingthis.config.useEntityIdsat builder construction time (line 292), butexecute()accepts a request-scopeduseEntityIdsoverride. If a caller builds the builder under one mode and executes under another, the field identifier part of the URL uses the stale mode while the rest of the request uses the new one.Consider storing the column reference and resolving the identifier at execute time:
mutableBuilder.operation = "getSingleField"; mutableBuilder.operationColumn = column; - mutableBuilder.operationParam = column.getFieldIdentifier(this.config.useEntityIds); + // Defer identifier resolution to execute() timeThen in
execute()andgetRequestConfig(), resolve using the effectiveuseEntityIds:if (this.operation === "getSingleField" && this.operationColumn) { - url += `/${this.operationParam}`; + url += `/${this.operationColumn.getFieldIdentifier(mergedOptions.useEntityIds ?? this.config.useEntityIds)}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/record-builder.ts` around lines 289 - 297, getSingleField currently resolves and stores operationParam at builder construction using this.config.useEntityIds, which can become stale if execute() is called with a different request-scoped useEntityIds; instead, keep the column reference and resolve the identifier at execution time. Change getSingleField to set operation = "getSingleField" and store operationColumn = column (remove resolving operationParam here), keeping the navigation flags isNavigateFromEntitySet, navigateRelation, and navigateSourceTableName as-is; then update execute() and getRequestConfig() to derive the effective useEntityIds (request override or this.config.useEntityIds) and compute the field identifier from operationColumn.getFieldIdentifier(effectiveUseEntityIds) when building the URL or request payload.
500-518:⚠️ Potential issue | 🟡 MinorSame entity-ID freeze issue in
navigate().Line 502 resolves
relationIdusingthis.config.useEntityIdsat builder construction time. If the resultingQueryBuilderis later executed with a differentuseEntityIdsoverride, the navigation path segment will use the stale identifier.Consider deferring the relation identifier resolution to the
QueryBuilder's execute time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/record-builder.ts` around lines 500 - 518, The relation identifier is being resolved eagerly in RecordBuilder.navigate (see resolveTableId and relationId) using this.config.useEntityIds, causing stale IDs if QueryBuilder execution overrides useEntityIds; instead, stop resolving to a frozen relationId in RecordBuilder.navigate — store the raw relationName (and for the source table/store the original table reference rather than a resolved ID) and move the call to resolveTableId into the QueryBuilder execution path (e.g., QueryBuilder.execute or the method that builds the request URL) so resolution uses the effective useEntityIds at execute time; update any places that expect relationId to instead call resolveTableId(targetTable, relationName, effectiveUseEntityIds) during execution.
♻️ Duplicate comments (4)
packages/fmodata/src/effect.ts (1)
50-57:⚠️ Potential issue | 🟠 Major
runAsResultmay leak defects as rejected promises.
Effect.catchAllonly catches typed failures in the error channel, not defects (exceptions thrown insideEffect.gen/Effect.sync). If code likeparseLocationHeader()throws inside a generator, the exception becomes a defect that bypassescatchAll, causingrunPromiseto reject rather than returningResult.error.Consider using
Effect.catchAllCauseto also handle defects:export function runAsResult<T>(effect: Effect.Effect<T, FMODataErrorType>): Promise<Result<T>> { return Effect.runPromise( effect.pipe( Effect.map((data): Result<T> => ({ data, error: undefined })), - Effect.catchAll((error) => Effect.succeed<Result<T>>({ data: undefined, error })), + Effect.catchAllCause((cause) => { + const error = Cause.failureOption(cause).pipe( + Option.getOrElse(() => { + // Handle defects by extracting the first defect + const defect = Cause.defects(cause)[0]; + return defect instanceof Error ? defect : new Error(String(defect)); + }), + ); + return Effect.succeed<Result<T>>({ data: undefined, error: error as FMODataErrorType }); + }), ), ); }Alternatively, wrap risky synchronous calls at call sites with
Effect.try.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/effect.ts` around lines 50 - 57, runAsResult currently uses Effect.catchAll so defects (exceptions thrown inside Effect.sync or Effect.gen) bypass the handler and cause Effect.runPromise to reject; update the implementation of runAsResult (the function named runAsResult that calls Effect.runPromise and uses Effect.catchAll) to handle defects as well—either replace Effect.catchAll with Effect.catchAllCause and map both failures and causes into the Result error path, or explicitly wrap risky sync calls at their sites with Effect.try/Effect.tryOrElse (e.g., where parseLocationHeader might throw) so that exceptions become typed failures that catchAll will handle; ensure the final returned Promise always resolves to a Result<T> by converting defects to Result.error rather than letting runPromise reject.packages/fmodata/src/client/entity-set.ts (2)
303-305:⚠️ Potential issue | 🟠 MajorDefer navigation ID-mode resolution until request build time.
Line 303 and Line 304 resolve names with
this.config.useEntityIdsduringnavigate(), so laterexecute({ useEntityIds: ... })can produce mixed path/query modes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/entity-set.ts` around lines 303 - 305, navigate() is resolving table IDs eagerly using this.config.useEntityIds (via calls to resolveTableId and producing resolvedRelation/resolvedSourceName), which causes mixed ID/name modes when execute({ useEntityIds: ... }) is later called; change navigate() to stop calling resolveTableId and instead store the raw targetTable, relationName and source table name (e.g., keep occurrence/getTableName values) and perform resolveTableId only during request build/execute (inside execute or the request builder) using the runtime flag passed to execute({ useEntityIds: ... }) so all path/query resolution is consistent with the execute-time useEntityIds setting.
124-127:⚠️ Potential issue | 🟠 MajorDo not bake
includeSpecialColumnsdefaults into child builders.Line 124 and Line 195 eagerly pass system columns into
select(...), which makesexecute({ includeSpecialColumns: false })ineffective on those builders.Also applies to: 195-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/entity-set.ts` around lines 124 - 127, The child builders are being created with baked-in system column defaults by passing systemColumns into builder.select(...) (see selectedBuilder and the applyNavigationContext(...) call), which prevents execute({ includeSpecialColumns: false }) from working; remove the eager systemColumns argument so the select() call does not hardcode ROWID/ROWMODID and instead allows includeSpecialColumns to be applied at execution time (or alternatively read includeSpecialColumns from the execution options when constructing the final projection), ensuring the QueryBuilder returned by selectedBuilder does not permanently include system columns.packages/fmodata/src/client/insert-builder.ts (1)
151-161:⚠️ Potential issue | 🟠 MajorPrevent execute options from clobbering the insert request contract.
Line 160 spreads
...mergedOptionsaftermethod,headers, andbody, so callers can override POST semantics and required headers.Suggested fix
- const responseData = yield* requestFromService<any>(url, { - method: "POST", - headers: { - "Content-Type": "application/json", - Prefer: preferHeader, - // biome-ignore lint/suspicious/noExplicitAny: Type assertion for headers object - ...((mergedOptions as any)?.headers || {}), - }, - body: JSON.stringify(transformedData), - ...mergedOptions, - }); + const { method: _method, body: _body, headers: customHeaders, ...requestOptions } = mergedOptions; + const headers = new Headers(customHeaders as HeadersInit | undefined); + headers.set("Content-Type", "application/json"); + headers.set("Prefer", preferHeader); + + const responseData = yield* requestFromService<any>(url, { + ...requestOptions, + method: "POST", + headers, + body: JSON.stringify(transformedData), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/insert-builder.ts` around lines 151 - 161, The request currently spreads ...mergedOptions after method/headers/body so callers can override POST, Content-Type, and Prefer; fix by preventing mergedOptions from clobbering those fields: destructure mergedOptions (e.g. const { method, headers: optHeaders, body, ...restOptions } = mergedOptions as any) and then call requestFromService with ...restOptions and explicit method: "POST", body: JSON.stringify(transformedData), and headers: { "Content-Type": "application/json", Prefer: preferHeader, ...((optHeaders || {})) } (or alternatively spread mergedOptions first then explicitly set method/body/headers after) so requestFromService and the insert builder (insert-builder.ts, requestFromService, mergedOptions) cannot override required semantics.
🧹 Nitpick comments (3)
packages/fmodata/src/client/builders/index.ts (1)
6-8: Please avoid widening this barrel further.Adding more re-exports here increases the public surface of an already broad
index.tsentry point. I’d keepmutation-helpersandread-builder-stateon concrete module paths instead of extending the barrel again.As per coding guidelines,
**/index.{js,jsx,ts,tsx}: Avoid barrel files (index files that re-export everything).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/builders/index.ts` around lines 6 - 8, The barrel index.ts currently re-exports "./query-string-builder", widening the public surface; remove the export of "query-string-builder" from this file (leave exports for "mutation-helpers" and "read-builder-state" intact) and update any consumers to import from the concrete module path "packages/fmodata/src/client/builders/query-string-builder" (or its package-relative path) instead of from the barrel; ensure CI compiles and all imports referencing query-string-builder are updated to the explicit module path.packages/fmodata/src/client/query/index.ts (1)
5-6: Avoid routingExpandConfigthrough anotherindex.tsalias.Re-exporting this from the query barrel couples the query entry point back to builder internals and grows the barrel surface again. Prefer a concrete export path for the shared type.
As per coding guidelines,
**/index.{js,jsx,ts,tsx}: Avoid barrel files (index files that re-export everything).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/query/index.ts` around lines 5 - 6, Remove the re-export of ExpandConfig from the query index (the export type { ExpandConfig } line) and stop routing that shared builder type through this barrel; instead update any consumers to import ExpandConfig directly from the builders' shared-types module (i.e., the canonical shared-types export in the builders package) so the query entry point no longer couples to builder internals and the barrel surface is not expanded.packages/fmodata/src/effect.ts (1)
177-187: Consider simplifying the attributes conditional.The ternary
attributes ? attributes : undefinedis redundant sinceundefinedis already the falsy case.return effect.pipe( Effect.withSpan(name, { - attributes: attributes ? attributes : undefined, + attributes, }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/effect.ts` around lines 177 - 187, In withSpan, simplify the redundant conditional by passing the attributes value directly into Effect.withSpan; update the call in the withSpan function (the Effect.withSpan(name, { attributes: ... }) object) to use the shorthand property (attributes) so that undefined is passed naturally instead of using the ternary attributes ? attributes : undefined.
🤖 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/fmodata/src/client/batch-builder.ts`:
- Around line 142-157: In failAllResults, firstErrorIndex is hardcoded to 0
which is invalid when errorCount (derived from this.builders.length) is 0;
change firstErrorIndex to be null when there are no failed items (e.g. set
firstErrorIndex = errorCount === 0 ? null : 0) and update the BatchResult typing
or cast accordingly so firstErrorIndex accepts number | null; refer to function
name failAllResults, variable errorCount, and property firstErrorIndex to locate
and adjust the code.
In `@packages/fmodata/src/client/builders/mutation-helpers.ts`:
- Around line 51-52: When mode === "byId" in the build path that returns
`/${databaseName}/${tableId}('${recordId}')`, validate that recordId is present
and non-empty and otherwise throw a clear error (fail fast) similar to the
filter-mode branch; update the function in mutation-helpers.ts that uses
mode/databaseName/tableId/recordId to check recordId and raise an exception (or
return an error) instead of emitting `'undefined'`.
In `@packages/fmodata/src/client/builders/read-builder-state.ts`:
- Around line 55-75: In cloneRecordReadBuilderState, when incoming changes
includes selectedFields or fieldMapping you must clone those inputs instead of
keeping the caller's reference; change the branches that set selectedFields and
fieldMapping to assign a shallow copy (e.g., selectedFields =
changes?.selectedFields ? [...changes.selectedFields] : undefined and
fieldMapping = changes?.fieldMapping ? { ...changes.fieldMapping } : undefined)
so the returned state does not retain mutable references to caller-provided
arrays/maps while preserving the existing cloning of expandConfigs.
- Around line 25-39: cloneQueryReadBuilderState currently clones expandConfigs
but leaves fieldMapping copied by reference via the spread of state; update
cloneQueryReadBuilderState to clone fieldMapping as well so callers don't
observe mutations: when building the returned object set fieldMapping to a
shallow copy of changes.fieldMapping if provided or a shallow copy of
state.fieldMapping otherwise (e.g. use an object spread to create a new object),
preserving the existing behavior for other fields and keeping the function name
and returned type QueryReadBuilderState<TSchema> intact.
In `@packages/fmodata/src/client/delete-builder.ts`:
- Around line 120-123: The call to requestFromService lets caller options in
mergedOptions override DELETE semantics; change the options construction so
mergedOptions cannot override method or provide a body: spread mergedOptions
first (or remove body from it), then explicitly set method: "DELETE" and ensure
body is cleared (e.g., set body to undefined) before passing to
requestFromService; update the code around requestFromService(url, {
...mergedOptions, method: "DELETE", body: undefined }) or remove
mergedOptions.body beforehand to guarantee requestFromService always sees a
DELETE with no body.
In `@packages/fmodata/src/client/query/query-builder.ts`:
- Around line 341-343: The code currently calls
expression.toODataFilter(this.config.useEntityIds) and patches the query with a
rendered filter string, which locks in compile-time naming; instead, store the
FilterExpression object (e.g., keep the variable expression or a new property
like this.pendingFilterExpression) rather than calling toODataFilter here, stop
patching a concrete filter string in patchQueryOptions, and defer rendering to
buildQueryString(...) where you have access to the resolved runtime flag
(mergedOptions.useEntityIds) used by execute(); update buildQueryString to
detect a pending FilterExpression and call
expression.toODataFilter(resolvedUseEntityIds) there so filter naming respects
execute-time overrides.
In `@packages/fmodata/src/client/update-builder.ts`:
- Around line 177-182: The PATCH call allows callers to override critical
request invariants because ...mergedOptions is spread last; fix by ensuring
method, headers, and body cannot be overridden: when calling
requestFromService(url, { ... }), either spread ...mergedOptions first and then
explicitly set method: "PATCH", headers, body: JSON.stringify(transformedData)
last, or remove method/headers/body from mergedOptions (e.g., omit those keys)
before merging; update the call site that constructs the request (the
requestFromService invocation in update-builder.ts that uses url, mergedOptions,
headers, and transformedData) so the PATCH invariants are always enforced.
---
Outside diff comments:
In `@packages/fmodata/src/client/record-builder.ts`:
- Around line 289-297: getSingleField currently resolves and stores
operationParam at builder construction using this.config.useEntityIds, which can
become stale if execute() is called with a different request-scoped
useEntityIds; instead, keep the column reference and resolve the identifier at
execution time. Change getSingleField to set operation = "getSingleField" and
store operationColumn = column (remove resolving operationParam here), keeping
the navigation flags isNavigateFromEntitySet, navigateRelation, and
navigateSourceTableName as-is; then update execute() and getRequestConfig() to
derive the effective useEntityIds (request override or this.config.useEntityIds)
and compute the field identifier from
operationColumn.getFieldIdentifier(effectiveUseEntityIds) when building the URL
or request payload.
- Around line 500-518: The relation identifier is being resolved eagerly in
RecordBuilder.navigate (see resolveTableId and relationId) using
this.config.useEntityIds, causing stale IDs if QueryBuilder execution overrides
useEntityIds; instead, stop resolving to a frozen relationId in
RecordBuilder.navigate — store the raw relationName (and for the source
table/store the original table reference rather than a resolved ID) and move the
call to resolveTableId into the QueryBuilder execution path (e.g.,
QueryBuilder.execute or the method that builds the request URL) so resolution
uses the effective useEntityIds at execute time; update any places that expect
relationId to instead call resolveTableId(targetTable, relationName,
effectiveUseEntityIds) during execution.
---
Duplicate comments:
In `@packages/fmodata/src/client/entity-set.ts`:
- Around line 303-305: navigate() is resolving table IDs eagerly using
this.config.useEntityIds (via calls to resolveTableId and producing
resolvedRelation/resolvedSourceName), which causes mixed ID/name modes when
execute({ useEntityIds: ... }) is later called; change navigate() to stop
calling resolveTableId and instead store the raw targetTable, relationName and
source table name (e.g., keep occurrence/getTableName values) and perform
resolveTableId only during request build/execute (inside execute or the request
builder) using the runtime flag passed to execute({ useEntityIds: ... }) so all
path/query resolution is consistent with the execute-time useEntityIds setting.
- Around line 124-127: The child builders are being created with baked-in system
column defaults by passing systemColumns into builder.select(...) (see
selectedBuilder and the applyNavigationContext(...) call), which prevents
execute({ includeSpecialColumns: false }) from working; remove the eager
systemColumns argument so the select() call does not hardcode ROWID/ROWMODID and
instead allows includeSpecialColumns to be applied at execution time (or
alternatively read includeSpecialColumns from the execution options when
constructing the final projection), ensuring the QueryBuilder returned by
selectedBuilder does not permanently include system columns.
In `@packages/fmodata/src/client/insert-builder.ts`:
- Around line 151-161: The request currently spreads ...mergedOptions after
method/headers/body so callers can override POST, Content-Type, and Prefer; fix
by preventing mergedOptions from clobbering those fields: destructure
mergedOptions (e.g. const { method, headers: optHeaders, body, ...restOptions }
= mergedOptions as any) and then call requestFromService with ...restOptions and
explicit method: "POST", body: JSON.stringify(transformedData), and headers: {
"Content-Type": "application/json", Prefer: preferHeader, ...((optHeaders ||
{})) } (or alternatively spread mergedOptions first then explicitly set
method/body/headers after) so requestFromService and the insert builder
(insert-builder.ts, requestFromService, mergedOptions) cannot override required
semantics.
In `@packages/fmodata/src/effect.ts`:
- Around line 50-57: runAsResult currently uses Effect.catchAll so defects
(exceptions thrown inside Effect.sync or Effect.gen) bypass the handler and
cause Effect.runPromise to reject; update the implementation of runAsResult (the
function named runAsResult that calls Effect.runPromise and uses
Effect.catchAll) to handle defects as well—either replace Effect.catchAll with
Effect.catchAllCause and map both failures and causes into the Result error
path, or explicitly wrap risky sync calls at their sites with
Effect.try/Effect.tryOrElse (e.g., where parseLocationHeader might throw) so
that exceptions become typed failures that catchAll will handle; ensure the
final returned Promise always resolves to a Result<T> by converting defects to
Result.error rather than letting runPromise reject.
---
Nitpick comments:
In `@packages/fmodata/src/client/builders/index.ts`:
- Around line 6-8: The barrel index.ts currently re-exports
"./query-string-builder", widening the public surface; remove the export of
"query-string-builder" from this file (leave exports for "mutation-helpers" and
"read-builder-state" intact) and update any consumers to import from the
concrete module path "packages/fmodata/src/client/builders/query-string-builder"
(or its package-relative path) instead of from the barrel; ensure CI compiles
and all imports referencing query-string-builder are updated to the explicit
module path.
In `@packages/fmodata/src/client/query/index.ts`:
- Around line 5-6: Remove the re-export of ExpandConfig from the query index
(the export type { ExpandConfig } line) and stop routing that shared builder
type through this barrel; instead update any consumers to import ExpandConfig
directly from the builders' shared-types module (i.e., the canonical
shared-types export in the builders package) so the query entry point no longer
couples to builder internals and the barrel surface is not expanded.
In `@packages/fmodata/src/effect.ts`:
- Around line 177-187: In withSpan, simplify the redundant conditional by
passing the attributes value directly into Effect.withSpan; update the call in
the withSpan function (the Effect.withSpan(name, { attributes: ... }) object) to
use the shorthand property (attributes) so that undefined is passed naturally
instead of using the ternary attributes ? attributes : undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b0befae-3f51-4d54-9e6d-9b1a6700623f
📒 Files selected for processing (26)
packages/fmodata/src/client/batch-builder.tspackages/fmodata/src/client/builders/index.tspackages/fmodata/src/client/builders/mutation-helpers.tspackages/fmodata/src/client/builders/read-builder-state.tspackages/fmodata/src/client/builders/response-processor.tspackages/fmodata/src/client/database.tspackages/fmodata/src/client/delete-builder.tspackages/fmodata/src/client/entity-set.tspackages/fmodata/src/client/insert-builder.tspackages/fmodata/src/client/query/expand-builder.tspackages/fmodata/src/client/query/index.tspackages/fmodata/src/client/query/query-builder.tspackages/fmodata/src/client/query/response-processor.tspackages/fmodata/src/client/query/types.tspackages/fmodata/src/client/record-builder.tspackages/fmodata/src/client/response-processor.tspackages/fmodata/src/client/runtime.tspackages/fmodata/src/client/schema-manager.tspackages/fmodata/src/client/update-builder.tspackages/fmodata/src/client/webhook-builder.tspackages/fmodata/src/effect.tspackages/fmodata/src/errors.tspackages/fmodata/src/services.tspackages/fmodata/tests/effect-layer-execution.test.tspackages/fmodata/tests/mutation-helpers.test.tspackages/fmodata/tests/response-processor.test.ts
💤 Files with no reviewable changes (3)
- packages/fmodata/src/client/query/expand-builder.ts
- packages/fmodata/src/client/response-processor.ts
- packages/fmodata/src/client/query/response-processor.ts
✅ Files skipped from review due to trivial changes (1)
- packages/fmodata/tests/mutation-helpers.test.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/fmodata/src/validation.ts (2)
591-600:⚠️ Potential issue | 🟠 MajorGuard
responsebefore readingresponse.value.Line 591 dereferences
response.valuebefore the null/empty-response check, sovalidateSingleResponse(undefined, ...)throws aTypeErrorinstead of returning the documented result union. Move the!responsebranch ahead of the count checks, or use optional chaining here. As per coding guidelines, Use optional chaining (?.) and nullish coalescing (??) for safer property access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/validation.ts` around lines 591 - 600, The code dereferences response.value before checking whether response is null, causing a TypeError; update the checks in validateSingleResponse to guard against null/undefined by either moving the if (!response) branch above the count check or using optional chaining and nullish coalescing—e.g., use response?.value?.length when testing for > 1 and (response?.value?.length ?? 0) when testing for === 0—and keep the RecordCountMismatchError and mode === "exact" logic unchanged so behavior remains identical for non-null responses.
296-314:⚠️ Potential issue | 🟠 MajorPrefix expanded-relation issue paths before rethrowing.
These branches forward
itemValidation.error.issuesunchanged, so callers get nested paths like["name"]instead of["users", 0, "name"]or["users", "name"]. That makes the aggregatedValidationError.issuespayload ambiguous once expands are involved.Also applies to: 321-338, 463-480, 488-505
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/validation.ts` around lines 296 - 314, The validation branches rethrow itemValidation.error.issues unchanged, so issue paths lose their expanded-context; update those error issues before constructing the new ValidationError: map over itemValidation.error.issues and prepend the expand path (use expandConfig.relation and the index i for array expands) to each issue.path (e.g., newPath = [expandConfig.relation, i, ...oldPath] or [expandConfig.relation, ...oldPath] when not indexing), then pass the transformed issues into the ValidationError constructor instead of the raw itemValidation.error.issues; apply the same fix for the other similar branches referenced (around validateRecord returns at the other ranges).packages/fmodata/tests/field-id-transforms.test.ts (1)
475-496:⚠️ Potential issue | 🟠 MajorDon't let this test pass on the error path.
The
if (result.error)branch only asserts that an error exists, so a regression that breaks expanded-relation transformation still produces a passing test. If failure is expected here, make it a dedicated error test; otherwise fail fast onresult.errorand keep the transformation assertions unconditional.Suggested change
- if (result.error) { - expect(result.error).toBeDefined(); - } else { - expect(result.data).toBeDefined(); - expect(result.data).toHaveLength(1); - if (result.data?.[0]) { - const contact = result.data[0]; - expect(contact).toMatchObject({ - PrimaryKey: "contact-1", - name: "Contact One", - id_user: "550e8400-e29b-41d4-a716-446655440001", - }); - // Check expanded relation was transformed - expect(contact.users).toHaveLength(1); - expect(contact.users[0]).toMatchObject({ - id: "550e8400-e29b-41d4-a716-446655440001", - name: "Alice", - }); - } - } + expect(result.error).toBeUndefined(); + if (!result.data?.[0]) { + throw new Error("Expected expanded relation data"); + } + const contact = result.data[0]; + expect(contact).toMatchObject({ + PrimaryKey: "contact-1", + name: "Contact One", + id_user: "550e8400-e29b-41d4-a716-446655440001", + }); + expect(contact.users).toHaveLength(1); + expect(contact.users[0]).toMatchObject({ + id: "550e8400-e29b-41d4-a716-446655440001", + name: "Alice", + });packages/fmodata/src/client/batch-builder.ts (1)
216-224:⚠️ Potential issue | 🟡 MinorSet
firstErrorIndexwhen the batch is truncated.If the first failure is a missing parsed response,
errorCountincrements butfirstErrorIndexstaysnull. That leaves the summary metadata inconsistent withresults.Suggested fix
if (!parsed) { - const failedAtIndex = firstErrorIndex ?? i; + if (firstErrorIndex === null) { + firstErrorIndex = i; + } + const failedAtIndex = firstErrorIndex; results.push({ data: undefined, error: new BatchTruncatedError(i, failedAtIndex), status: 0, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/batch-builder.ts` around lines 216 - 224, When encountering a missing parsed response inside the batch loop (the branch that pushes a BatchTruncatedError into results), ensure you set firstErrorIndex when it is currently null so summary metadata matches results; specifically, update the variable firstErrorIndex (used later for summary) to i when it is null before incrementing errorCount and continuing. Locate the branch that creates new BatchTruncatedError and modify the logic around firstErrorIndex, errorCount, and results to assign firstErrorIndex = i (or the null-coalesced equivalent) so the summary reflects the true first failure index.packages/fmodata/src/client/update-builder.ts (1)
206-243:⚠️ Potential issue | 🟠 Major
toRequest()still hard-codes the builder's ID mode.
execute()resolvesshouldUseIdsfrom call-site options, butgetRequestConfig()/toRequest()always usethis.config.useEntityIdsfor both the URL and body transformation. Batched updates built viaBatchBuilder.execute({ useEntityIds: ... })can therefore serialize table IDs and field names in a different mode than the actual request options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/update-builder.ts` around lines 206 - 243, getRequestConfig() and toRequest() must resolve the effective "use entity IDs" from the call-site options instead of always using this.config.useEntityIds: add an optional parameter (e.g., options?: ExecuteOptions or useEntityIds?: boolean) to getRequestConfig and thread that through to toRequest (or accept options in toRequest and pass it into getRequestConfig), compute const shouldUseIds = options?.useEntityIds ?? this.config.useEntityIds, then use shouldUseIds for resolveMutationTableId(this.table, shouldUseIds, "ExecutableUpdateBuilder"), for transformFieldNamesToIds(this.data, this.table) and for buildMutationUrl(..., useEntityIds: shouldUseIds, ...), ensuring URL and body transformation use the same resolved flag so Batched updates respect the per-call option.
🧹 Nitpick comments (2)
packages/fmodata/tests/use-entity-ids-override.test.ts (1)
29-40: Consider moving helper outside describe block for consistency.The
makeLocalContactsTOfactory is defined inside the describe block butcontactsTOis at module level. Both serve similar purposes. This is a minor inconsistency but acceptable given the test isolation needs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/tests/use-entity-ids-override.test.ts` around lines 29 - 40, The factory function makeLocalContactsTO is defined inside the describe block while contactsTO is defined at module scope; extract makeLocalContactsTO to module scope (move its declaration out of the describe) so both helpers share the same scope and consistency; ensure any references to makeLocalContactsTO inside the describe remain unchanged and that imports/exports (if any) still resolve after moving the function.packages/fmodata/src/client/entity-set.ts (1)
136-136: Extract the default page size into a shared constant.
list()now hard-codes1000in three places, whilepackages/fmodata/src/client/query/query-builder.tsalready defines the same default. Pulling this into one shared constant will keep the implicit page size from drifting between entry points.As per coding guidelines, "Use meaningful variable names instead of magic numbers - extract constants with descriptive names".
Also applies to: 150-152, 168-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/entity-set.ts` at line 136, The hard-coded page size 1000 in EntitySet.list (seen in the selectedBuilder assignment using this.applyNavigationContext(...).top(1000)) should be replaced with a shared constant; import or re-export the existing default from packages/fmodata/src/client/query/query-builder.ts (the QueryBuilder default page size constant) or create a single shared constant (e.g., DEFAULT_PAGE_SIZE) and use it wherever .top(1000) is used in this file (also replace the occurrences around lines 150-152 and 168). Update the selectedBuilder call and the other two .top(1000) calls to use that constant name so the implicit page size is defined in one place.
🤖 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/fmodata/src/client/batch-builder.ts`:
- Around line 178-184: The batch assembly currently calls the synchronous
builders.map(b => b.toRequest(...)) which bypasses new async
validation/transforms; update the pipeline generator to invoke the builders'
async request preparation (e.g., an async
toRequest/prepareRequest/validateAndTransform method on the builder) and await
them before formatting the batch. Specifically, in the Effect.gen block where
pipeline is created (symbol: pipeline), replace the synchronous map with an
Effect-level parallel/sequence (e.g., Effect.all or Effect.forEach) that yields
the array of Request objects from each builder's async preparation method, then
pass the awaited requests into formatBatchRequestFromNative as before; keep
using yield* Effect.tryPromise for the formatBatchRequestFromNative call.
In `@packages/fmodata/src/client/query/query-builder.ts`:
- Around line 755-761: The request config path (toRequest() ->
getRequestConfig()) ignores per-call batch/time overrides (like useEntityIds and
includeSpecialColumns) so BatchBuilder.execute(options) can render URLs with
wrong naming mode; update toRequest() and getRequestConfig() to accept an
overrides/options parameter and merge those into the builder state before
building the query string (i.e., merge options.useEntityIds and
options.includeSpecialColumns into this.config / this.readState or pass them
into urlBuilder.build), and ensure BatchBuilder.execute(options) passes its
options through to toRequest(); reference functions: toRequest,
getRequestConfig, BatchBuilder.execute, urlBuilder.build, readState, and config.
In `@packages/fmodata/src/client/update-builder.ts`:
- Around line 179-186: callerHeaders (a HeadersInit) is being spread into an
object which loses entries for Headers instances or tuple arrays; fix by
normalizing callerHeaders via the Headers constructor in update-builder.ts
before merging: create a Headers from callerHeaders (e.g., new
Headers(callerHeaders)) and copy its entries into the final headers set used in
the requestFromService call (using Headers.forEach or for..of and .set()) so
that all header types (object, Headers, tuple[]) are preserved when building the
headers passed to requestFromService.
In `@packages/fmodata/src/effect.ts`:
- Around line 50-62: In runAsResult, replace the unsafe instanceof Error cast
with a proper domain-type check: use isFMODataError(defect) to determine if the
thrown defect already matches FMODataErrorType and return it directly, otherwise
wrap non-domain defects with new BuilderInvariantError("runAsResult",
String(defect)) and return that as the FMODataErrorType; update the catch block
in the runAsResult function so it calls isFMODataError(defect) and only treats
the defect as FMODataErrorType when that guard succeeds, using
BuilderInvariantError for all other cases.
- Around line 79-88: The cast erases environment requirements; remove the "as
Effect.Effect<T, FMODataErrorType>" assertion on the provided variable and
instead constrain the effect's environment so the layer satisfies it. Change the
effect parameter type from Effect.Effect<T, FMODataErrorType, R> to
Effect.Effect<T, FMODataErrorType, FMODataLayer> (or make R extend FMODataLayer)
and let const provided = Effect.provide(withOptionalSpan(effect, spanName,
attributes), layer) infer its correct type without assertions.
---
Outside diff comments:
In `@packages/fmodata/src/client/batch-builder.ts`:
- Around line 216-224: When encountering a missing parsed response inside the
batch loop (the branch that pushes a BatchTruncatedError into results), ensure
you set firstErrorIndex when it is currently null so summary metadata matches
results; specifically, update the variable firstErrorIndex (used later for
summary) to i when it is null before incrementing errorCount and continuing.
Locate the branch that creates new BatchTruncatedError and modify the logic
around firstErrorIndex, errorCount, and results to assign firstErrorIndex = i
(or the null-coalesced equivalent) so the summary reflects the true first
failure index.
In `@packages/fmodata/src/client/update-builder.ts`:
- Around line 206-243: getRequestConfig() and toRequest() must resolve the
effective "use entity IDs" from the call-site options instead of always using
this.config.useEntityIds: add an optional parameter (e.g., options?:
ExecuteOptions or useEntityIds?: boolean) to getRequestConfig and thread that
through to toRequest (or accept options in toRequest and pass it into
getRequestConfig), compute const shouldUseIds = options?.useEntityIds ??
this.config.useEntityIds, then use shouldUseIds for
resolveMutationTableId(this.table, shouldUseIds, "ExecutableUpdateBuilder"), for
transformFieldNamesToIds(this.data, this.table) and for buildMutationUrl(...,
useEntityIds: shouldUseIds, ...), ensuring URL and body transformation use the
same resolved flag so Batched updates respect the per-call option.
In `@packages/fmodata/src/validation.ts`:
- Around line 591-600: The code dereferences response.value before checking
whether response is null, causing a TypeError; update the checks in
validateSingleResponse to guard against null/undefined by either moving the if
(!response) branch above the count check or using optional chaining and nullish
coalescing—e.g., use response?.value?.length when testing for > 1 and
(response?.value?.length ?? 0) when testing for === 0—and keep the
RecordCountMismatchError and mode === "exact" logic unchanged so behavior
remains identical for non-null responses.
- Around line 296-314: The validation branches rethrow
itemValidation.error.issues unchanged, so issue paths lose their
expanded-context; update those error issues before constructing the new
ValidationError: map over itemValidation.error.issues and prepend the expand
path (use expandConfig.relation and the index i for array expands) to each
issue.path (e.g., newPath = [expandConfig.relation, i, ...oldPath] or
[expandConfig.relation, ...oldPath] when not indexing), then pass the
transformed issues into the ValidationError constructor instead of the raw
itemValidation.error.issues; apply the same fix for the other similar branches
referenced (around validateRecord returns at the other ranges).
---
Nitpick comments:
In `@packages/fmodata/src/client/entity-set.ts`:
- Line 136: The hard-coded page size 1000 in EntitySet.list (seen in the
selectedBuilder assignment using this.applyNavigationContext(...).top(1000))
should be replaced with a shared constant; import or re-export the existing
default from packages/fmodata/src/client/query/query-builder.ts (the
QueryBuilder default page size constant) or create a single shared constant
(e.g., DEFAULT_PAGE_SIZE) and use it wherever .top(1000) is used in this file
(also replace the occurrences around lines 150-152 and 168). Update the
selectedBuilder call and the other two .top(1000) calls to use that constant
name so the implicit page size is defined in one place.
In `@packages/fmodata/tests/use-entity-ids-override.test.ts`:
- Around line 29-40: The factory function makeLocalContactsTO is defined inside
the describe block while contactsTO is defined at module scope; extract
makeLocalContactsTO to module scope (move its declaration out of the describe)
so both helpers share the same scope and consistency; ensure any references to
makeLocalContactsTO inside the describe remain unchanged and that
imports/exports (if any) still resolve after moving the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fd18f28-26c6-4f56-abcf-815e37a6f974
📒 Files selected for processing (23)
packages/fmodata/package.jsonpackages/fmodata/src/client/batch-builder.tspackages/fmodata/src/client/builders/mutation-helpers.tspackages/fmodata/src/client/builders/read-builder-state.tspackages/fmodata/src/client/delete-builder.tspackages/fmodata/src/client/entity-set.tspackages/fmodata/src/client/filemaker-odata.tspackages/fmodata/src/client/insert-builder.tspackages/fmodata/src/client/query/query-builder.tspackages/fmodata/src/client/query/url-builder.tspackages/fmodata/src/client/record-builder.tspackages/fmodata/src/client/update-builder.tspackages/fmodata/src/effect.tspackages/fmodata/src/errors.tspackages/fmodata/src/index.tspackages/fmodata/src/testing.tspackages/fmodata/src/validation.tspackages/fmodata/tests/field-id-transforms.test.tspackages/fmodata/tests/insert.test.tspackages/fmodata/tests/list-methods.test.tspackages/fmodata/tests/mock.test.tspackages/fmodata/tests/use-entity-ids-override.test.tspackages/fmodata/tests/validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/fmodata/src/index.ts
- packages/fmodata/src/client/query/url-builder.ts
- packages/fmodata/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/fmodata/src/client/entity-set.ts (1)
136-140:⚠️ Potential issue | 🟠 Major
includeSpecialColumnsis still baked into child builders inlist()andget().Line 137 and Line 208 eagerly apply
ROWID/ROWMODIDfrom config beforeexecute()options are known. That can still make execute-time overrides ineffective for schema-default selection flows.💡 Directional fix
- const selectedBuilder = this.applyNavigationContext( - this.config.includeSpecialColumns - ? builder.select(allColumns, { ROWID: true, ROWMODID: true }) - : builder.select(allColumns), - ).top(1000); + const selectedBuilder = this.applyNavigationContext(builder.select(allColumns)).top(1000);- const selectedBuilder = this.applyNavigationContext( - this.config.includeSpecialColumns - ? builder.select(allColumns, { ROWID: true, ROWMODID: true }) - : builder.select(allColumns), - ); + const selectedBuilder = this.applyNavigationContext(builder.select(allColumns));Then apply
includeSpecialColumnswhen building the final request inexecute()(after merging runtime options).Also applies to: 207-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fmodata/src/client/entity-set.ts` around lines 136 - 140, The child builders in list() and get() currently bake config.includeSpecialColumns into the builder via builder.select(..., { ROWID: true, ROWMODID: true }) inside applyNavigationContext, preventing execute() runtime options from overriding selection; change the flow to stop adding ROWID/ROWMODID in the child builders (remove the select with special columns in the code paths building selectedBuilder in list() and get()), and instead apply includeSpecialColumns when constructing the final request inside execute() after merging runtime options so that execute() can decide whether to include ROWID/ROWMODID; look for applyNavigationContext, builder.select, list(), get(), execute(), and config.includeSpecialColumns to make this change.
🤖 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/fmodata/src/errors.ts`:
- Around line 115-116: The ValidationError constructor currently defaults the
Error `cause` to the `issues` array; change it so `cause` is taken only from
options (const cause = options?.cause) and only pass a cause to super when an
actual cause is provided (call super(message) if cause is undefined, otherwise
super(message, { cause })); leave this.issues = issues untouched and do not
assign the issues array as the Error cause to preserve proper error-chain
semantics for the ValidationError class.
---
Duplicate comments:
In `@packages/fmodata/src/client/entity-set.ts`:
- Around line 136-140: The child builders in list() and get() currently bake
config.includeSpecialColumns into the builder via builder.select(..., { ROWID:
true, ROWMODID: true }) inside applyNavigationContext, preventing execute()
runtime options from overriding selection; change the flow to stop adding
ROWID/ROWMODID in the child builders (remove the select with special columns in
the code paths building selectedBuilder in list() and get()), and instead apply
includeSpecialColumns when constructing the final request inside execute() after
merging runtime options so that execute() can decide whether to include
ROWID/ROWMODID; look for applyNavigationContext, builder.select, list(), get(),
execute(), and config.includeSpecialColumns to make this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 582aa99e-c55d-48cd-aa4b-016b59b22a05
📒 Files selected for processing (3)
packages/fmodata/src/client/builders/expand-builder.tspackages/fmodata/src/client/entity-set.tspackages/fmodata/src/errors.ts
Comment on lines
+115
to
+116
| const cause = options?.cause ?? issues; | ||
| super(message, { cause }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether downstream code assumes `error.cause` is Error-like.
# Expected: no hard assumptions that `cause` always has Error properties when handling ValidationError.
rg -nP --type=ts -C3 '\bValidationError\b|\.cause\b|cause\?\.(message|stack|name)|instanceof\s+Error'Repository: proofgeist/proofkit
Length of output: 50375
🏁 Script executed:
# Check the ValidationError class implementation
sed -n '100,130p' packages/fmodata/src/errors.tsRepository: proofgeist/proofkit
Length of output: 1020
🏁 Script executed:
# Search for places where .cause property is accessed on ValidationError
rg -n 'error\.cause' packages/fmodata/src/ --type ts -A 2 -B 2Repository: proofgeist/proofkit
Length of output: 1511
🏁 Script executed:
# Search for specific patterns that treat cause as Error or array-like
rg -n '\.cause\s*instanceof|cause\?.flatten|cause\[' packages/ --type tsRepository: proofgeist/proofkit
Length of output: 1206
Remove the default of cause to the issues array in ValidationError.
The current code sets cause to an array when no underlying exception exists, which breaks error-chain consistency. Downstream code expects error.cause to be an Error-like object (or undefined), not an array. Additionally, when ValidationError instances are chained internally, passing an array as the cause option violates JavaScript error semantics.
Keep issues on this.issues only, and pass cause to the parent constructor only when an actual cause is provided:
Suggested fix
- const cause = options?.cause ?? issues;
- super(message, { cause });
+ super(message, options?.cause ? { cause: options.cause } : undefined);📝 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 cause = options?.cause ?? issues; | |
| super(message, { cause }); | |
| super(message, options?.cause ? { cause: options.cause } : undefined); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/fmodata/src/errors.ts` around lines 115 - 116, The ValidationError
constructor currently defaults the Error `cause` to the `issues` array; change
it so `cause` is taken only from options (const cause = options?.cause) and only
pass a cause to super when an actual cause is provided (call super(message) if
cause is undefined, otherwise super(message, { cause })); leave this.issues =
issues untouched and do not assign the issues array as the Error cause to
preserve proper error-chain semantics for the ValidationError class.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/fmodata/src/client/update-builder.ts`:
- Around line 167-171: The error-mapping currently uses "instanceof Error" then
force-casts results to FMODataErrorType (in the handler that constructs new
BuilderInvariantError("ExecutableUpdateBuilder.execute", ...)) which lets
non-BuilderInvariantError values slip through; change the runtime check to
"instanceof BuilderInvariantError" so TypeScript can narrow the union correctly
and remove the unsafe casts. Apply the same change to the other error mapping
(the block around the referenced 286–291 region), ensuring both mappings return
a value of type FMODataErrorType without using "as any" or "as
FMODataErrorType".
In `@packages/fmodata/src/effect.ts`:
- Around line 30-35: The fromResult and fromValidation functions currently cast
unknown rejections to FMODataErrorType with assertions; replace that with a
small domain-aware error-narrowing helper that uses isFMODataError and
BuilderInvariantError: when the catch receives e, if isFMODataError(e) return e,
otherwise wrap/convert the unknown error into a BuilderInvariantError (or
another appropriate FMODataErrorType) and return that; then call that helper
from the Effect.tryPromise catch handlers in fromResult and fromValidation so
the error channel only emits properly narrowed FMODataErrorType instances
instead of blind casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cdd7ad24-4348-48a2-8054-1306a946aa5e
📒 Files selected for processing (2)
packages/fmodata/src/client/update-builder.tspackages/fmodata/src/effect.ts
Replace manual error-threading boilerplate in all builders with Effect pipelines. Effect is used as an internal implementation detail — the public API (Result<T>) is unchanged. Changes: - Add `effect` dependency, remove unused `neverthrow` - Add `src/effect.ts` with bridge utilities (fromResult, makeRequestEffect, runAsResult, tryEffect, fromValidation) - Refactor `_makeRequest` into an Effect pipeline with extracted helpers (_classifyError, _parseHttpError, _checkEmbeddedODataError) - Refactor InsertBuilder.execute() to use Effect.gen pipeline - Refactor UpdateBuilder.execute() to use Effect.gen pipeline - Refactor DeleteBuilder.execute() to use Effect.gen pipeline - Refactor QueryBuilder.execute() to use Effect pipe composition - Refactor BatchBuilder.execute() to use Effect.gen pipeline All 873 tests pass with zero type errors. https://claude.ai/code/session_01VdwR8FRDc9f1qS68z2Sfzo
…ng spans Phase 2 of Effect.ts integration: - Add opt-in RetryPolicy for transient errors (SchemaLockedError, NetworkError, TimeoutError, HTTP 5xx) with exponential backoff and jitter via Effect Schedule - Accumulate all validation errors instead of fail-fast, returning all field issues in a single ValidationError - Add Effect.withSpan tracing to all builder execute() methods and HTTP requests for zero-cost observability All changes remain non-breaking — public API (Result<T>, error classes, type guards) is unchanged. https://claude.ai/code/session_01VdwR8FRDc9f1qS68z2Sfzo
…te tests Phase 3 of Effect.ts integration: - Define Effect Services (HttpClient, ODataConfig, ODataLogger) for dependency injection - Create MockFMServerConnection with router-style fetch handler and request spy - Export testing utilities via @proofkit/fmodata/testing subpath - Migrate 22 test files from createMockClient/fetchHandler to MockFMServerConnection - All 870 tests passing, 0 type errors, publint passes https://claude.ai/code/session_01VdwR8FRDc9f1qS68z2Sfzo
…r pattern Replace constructor parameter threading (ExecutionContext, databaseName, useEntityIds, etc.) with Effect's Context.Tag + Layer pattern across all builders. Each builder now receives an FMODataLayer and extracts config synchronously via extractConfigFromLayer() for non-Effect methods. Key changes: - All builders (Query, Record, Insert, Update, Delete, Batch) use layer-based DI - SchemaManager and WebhookManager converted to use Effect pipelines - Database.getMetadata/listTableNames/runScript use requestFromService - Removed deprecated makeRequestEffect() and runWithContext() from effect.ts - Simplified ExecutionContext interface to just _getLayer() - Removed unused BuilderConfig interface from shared-types.ts - createDatabaseLayer() enables database-scoped config overrides https://claude.ai/code/session_01EpwtyTQeyjVu3Qykf6aMBd
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
2 participants
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.
feat(fmodata): use Effect.ts internally for error handling pipelines
Replace manual error-threading boilerplate in all builders with Effect
pipelines. Effect is used as an internal implementation detail — the
public API (Result) is unchanged.
Changes:
effectdependency, remove unusedneverthrowsrc/effect.tswith bridge utilities (fromResult, makeRequestEffect,runAsResult, tryEffect, fromValidation)
_makeRequestinto an Effect pipeline with extracted helpers(_classifyError, _parseHttpError, _checkEmbeddedODataError)
All 873 tests pass with zero type errors.
https://claude.ai/code/session_01VdwR8FRDc9f1qS68z2Sfzo
feat(fmodata): add retry policies, validation accumulation, and tracing spans
Phase 2 of Effect.ts integration:
with exponential backoff and jitter via Effect Schedule
All changes remain non-breaking — public API (Result, error classes, type guards) is unchanged.
https://claude.ai/code/session_01VdwR8FRDc9f1qS68z2Sfzo
feat(fmodata): add Effect services, MockFMServerConnection, and migrate tests
Phase 3 of Effect.ts integration:
https://claude.ai/code/session_01VdwR8FRDc9f1qS68z2Sfzo
fix(fmodata): limit retries to idempotent HTTP methods
fix(fmodata): address remaining PR review findings
fix(fmodata): harden write requests and batch response handling
refactor(fmodata): migrate all builders to Effect DI via Context/Layer pattern
Replace constructor parameter threading (ExecutionContext, databaseName,
useEntityIds, etc.) with Effect's Context.Tag + Layer pattern across all
builders. Each builder now receives an FMODataLayer and extracts config
synchronously via extractConfigFromLayer() for non-Effect methods.
Key changes:
https://claude.ai/code/session_01EpwtyTQeyjVu3Qykf6aMBd
fix(fmodata): resolve DI branch lint regressions
Summary by CodeRabbit
New Features
Improvements
Tests