Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
WalkthroughRefactors multiple Airtable actions and loaders to return a unified MCPResponse envelope; adds a new createRecords action supporting batch creation; updates client types for multi-record POSTs; and converts error handling to structured MCPResponse objects with try/catch-wrapped API calls. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Action/Loader
participant Auth as OAuth Check
participant Perm as Permissioning
participant Airtable as Airtable API
Client->>Handler: invoke(props)
Handler->>Auth: check ctx.client
alt No OAuth
Auth-->>Handler: missing
Handler-->>Client: MCPResponse { error, status: 401 }
else OAuth present
Handler->>Perm: validatePermissions(...)
alt Permission denied
Perm-->>Handler: denied / envelope error
Handler-->>Client: MCPResponse { error, status: 403 }
else Permission granted
Handler->>Airtable: try { HTTP request (GET/POST/PATCH/DELETE) }
alt HTTP OK
Airtable-->>Handler: JSON payload
Handler-->>Client: MCPResponse { data: parsedJSON }
else HTTP error
Airtable-->>Handler: non-OK
Handler-->>Client: MCPResponse { error: message, status }
end
end
end
Note over Handler: catch -> MCPResponse { error, status: 500 }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
airtable/actions/updateRecords.ts (1)
64-135: Handle the MCPResponse envelope fromvalidatePermissions.
ctx.invoke["airtable"].loaders.permissioning.validatePermissionsnow returns anMCPResponse. AccessinghasPermissiondirectly on the envelope skips both error handling and the actual payload, so a denied or failed validation now falls through and the update runs anyway. Please unwrap the response, propagate anyerrorearly, and then examine the returneddata. For example:- const validationResult = await ctx.invoke["airtable"].loaders.permissioning.validatePermissions({ … }); - if ("hasPermission" in validationResult && !validationResult.hasPermission) { - return { error: validationResult.message || "Access denied", status: 403 }; - } + const validationResponse = + await ctx.invoke["airtable"].loaders.permissioning.validatePermissions({ … }); + if ("error" in validationResponse) { + return { + error: validationResponse.error, + status: validationResponse.status ?? 403, + }; + } + const validationResult = validationResponse.data; + if ( + validationResult && + "hasPermission" in validationResult && + !validationResult.hasPermission + ) { + return { + error: validationResult.message || "Access denied", + status: 403, + }; + }This preserves the new MCP contract and keeps unauthorized updates from proceeding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
airtable/actions/createField.ts(4 hunks)airtable/actions/createRecords.ts(1 hunks)airtable/actions/createTable.ts(4 hunks)airtable/actions/deleteRecords.ts(3 hunks)airtable/actions/updateField.ts(4 hunks)airtable/actions/updateRecords.ts(4 hunks)airtable/actions/updateTable.ts(4 hunks)airtable/loaders/getBaseSchema.ts(2 hunks)airtable/loaders/getRecord.ts(2 hunks)airtable/loaders/listBases.ts(2 hunks)airtable/loaders/listRecords.ts(2 hunks)airtable/loaders/searchRecords.ts(2 hunks)airtable/manifest.gen.ts(2 hunks)airtable/utils/client.ts(1 hunks)airtable/utils/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
airtable/actions/updateTable.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)Table(35-46)
airtable/actions/updateField.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)Field(26-32)
airtable/loaders/listRecords.ts (1)
airtable/utils/types.ts (4)
MCPResponse(13-13)ListRecordsResponse(82-85)ValidationResult(190-195)ValidationFilterResult(175-182)
airtable/loaders/listBases.ts (1)
airtable/utils/types.ts (4)
MCPResponse(13-13)ListBasesResponse(49-56)ValidationResult(190-195)ValidationFilterResult(175-182)
airtable/loaders/searchRecords.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)ListRecordsResponse(82-85)
airtable/actions/deleteRecords.ts (1)
airtable/utils/types.ts (1)
MCPResponse(13-13)
airtable/utils/client.ts (1)
airtable/utils/types.ts (2)
CreateRecordBody(87-90)AirtableRecord(19-23)
airtable/actions/createField.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)Field(26-32)
airtable/loaders/getBaseSchema.ts (1)
airtable/utils/types.ts (4)
MCPResponse(13-13)BaseSchemaResponse(59-62)ValidationResult(190-195)ValidationFilterResult(175-182)
airtable/actions/createRecords.ts (1)
airtable/utils/types.ts (4)
FieldSet(15-17)MCPResponse(13-13)AirtableRecord(19-23)CreateRecordBody(87-90)
airtable/actions/createTable.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)Table(35-46)
airtable/loaders/getRecord.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)AirtableRecord(19-23)
airtable/actions/updateRecords.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)AirtableRecord(19-23)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
airtable/actions/updateTable.ts (1)
51-63: UnwrapvalidatePermissionsMCPResponse before proceedingLine 52:
validatePermissionsnow returns an MCPResponse envelope. As written, any{ error, status }response (including outright permission failures) falls through because"hasPermission"is never inspected and the API call executes anyway. Please unwrap the envelope, bail out on the error branch, and only readdata.hasPermissionwhen present.- const validationResult = await ctx.invoke["airtable"].loaders.permissioning - .validatePermissions({ + const validationResponse = await ctx.invoke["airtable"].loaders.permissioning + .validatePermissions({ mode: "check", baseId: props.baseId, tableIdOrName: props.tableId, }); - - if ("hasPermission" in validationResult && !validationResult.hasPermission) { + if ("error" in validationResponse) { + return { + error: validationResponse.error ?? "Access denied", + status: validationResponse.status ?? 403, + }; + } + const validationResult = validationResponse.data; + if ( + validationResult && + "hasPermission" in validationResult && + !validationResult.hasPermission + ) { return { error: validationResult.message || "Access denied", status: 403, }; }airtable/actions/deleteRecords.ts (1)
40-52: Return early onvalidatePermissionsMCPResponse errorsLine 42: Like the other modules, this still assumes
validatePermissionsreturns a bare result. Any{ error, status }envelope skips your check, so deletions can run without confirmed access. Please unwrap the MCPResponse, return the error/status when present, and only inspectdata.hasPermissionthereafter.- const validationResult = await ctx.invoke["airtable"].loaders.permissioning - .validatePermissions({ + const validationResponse = await ctx.invoke["airtable"].loaders.permissioning + .validatePermissions({ mode: "check", baseId: props.baseId, tableIdOrName: props.tableId, }); - - if ("hasPermission" in validationResult && !validationResult.hasPermission) { + if ("error" in validationResponse) { + return { + error: validationResponse.error ?? "Access denied", + status: validationResponse.status ?? 403, + }; + } + const validationResult = validationResponse.data; + if ( + validationResult && + "hasPermission" in validationResult && + !validationResult.hasPermission + ) { return { error: validationResult.message || "Access denied", status: 403, }; }
♻️ Duplicate comments (3)
airtable/loaders/getRecord.ts (1)
42-56: MCPResponse still needs unwrapping before the permission checkLine 44: Per the MCP refactor,
validatePermissionsreturns an MCPResponse. Treating it as a bare result means loader errors and permission denials slip through. Please unwrap the response, return early on the error branch, and only inspectdata.hasPermissiononce confirmed.- const validationResult = await ctx.invoke["airtable"].loaders.permissioning - .validatePermissions({ + const validationResponse = await ctx.invoke["airtable"].loaders.permissioning + .validatePermissions({ mode: "check", baseId: props.baseId, tableIdOrName: props.tableId, }); - - if ( - "hasPermission" in validationResult && !validationResult.hasPermission - ) { + if ("error" in validationResponse) { + return { + error: validationResponse.error ?? "Access denied", + status: validationResponse.status ?? 403, + }; + } + const validationResult = validationResponse.data; + if ( + validationResult && + "hasPermission" in validationResult && + !validationResult.hasPermission + ) { return { error: validationResult.message || "Access denied", status: 403, }; }airtable/actions/createRecords.ts (1)
96-104: Preserve per-recordtypecastin the request payloadLine 98: We expose
RecordToCreate.typecast, but the mapping drops it, so callers relying on per-record typecasting silently stop working. Carry the flag through toCreateRecordBodywhen present.- const recordsToCreate: CreateRecordBody[] = records.map((record) => ({ - fields: record.fields, - })); + const recordsToCreate: CreateRecordBody[] = records.map((record) => { + const recordBody: CreateRecordBody = { fields: record.fields }; + if (record.typecast !== undefined) { + recordBody.typecast = record.typecast; + } + return recordBody; + });airtable/loaders/searchRecords.ts (1)
106-114: EscapesearchTermbefore buildingfilterByFormula.
Line 109 and Line 113 injectsearchTerminto the formula verbatim. When the user includes"or\, Airtable rejects the request with HTTP 422, so search stops working for those inputs. Escape these characters first and reuse the sanitized value everywhere you template the formula.- let filterByFormula = ""; + let filterByFormula = ""; + const escapedSearchTerm = searchTerm + .replace(/\\/g, "\\\\") + .replace(/"/g, '\\"'); if (searchFields && searchFields.length > 0) { const fieldFormulas = searchFields.map( - (field) => `SEARCH("${searchTerm}", {${field}})`, + (field) => `SEARCH("${escapedSearchTerm}", {${field}})`, ); filterByFormula = `OR(${fieldFormulas.join(",")})`; } else { - filterByFormula = `SEARCH("${searchTerm}", {*})`; + filterByFormula = `SEARCH("${escapedSearchTerm}", {*})`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
airtable/actions/createField.ts(4 hunks)airtable/actions/createRecords.ts(1 hunks)airtable/actions/createTable.ts(4 hunks)airtable/actions/deleteRecords.ts(3 hunks)airtable/actions/updateField.ts(4 hunks)airtable/actions/updateRecords.ts(4 hunks)airtable/actions/updateTable.ts(4 hunks)airtable/loaders/getBaseSchema.ts(2 hunks)airtable/loaders/getRecord.ts(2 hunks)airtable/loaders/listBases.ts(2 hunks)airtable/loaders/listRecords.ts(2 hunks)airtable/loaders/searchRecords.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airtable/actions/createTable.ts
🧰 Additional context used
🧬 Code graph analysis (11)
airtable/loaders/getRecord.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)AirtableRecord(19-23)
airtable/actions/createField.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)Field(26-32)
airtable/actions/createRecords.ts (1)
airtable/utils/types.ts (4)
FieldSet(15-17)MCPResponse(13-13)AirtableRecord(19-23)CreateRecordBody(87-90)
airtable/loaders/getBaseSchema.ts (2)
airtable/utils/types.ts (4)
MCPResponse(13-13)BaseSchemaResponse(59-62)ValidationResult(190-195)ValidationFilterResult(175-182)airtable/mod.ts (1)
Props(30-54)
airtable/actions/updateRecords.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)AirtableRecord(19-23)
airtable/actions/updateTable.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)Table(35-46)
airtable/actions/deleteRecords.ts (1)
airtable/utils/types.ts (1)
MCPResponse(13-13)
airtable/loaders/listRecords.ts (2)
airtable/utils/types.ts (4)
MCPResponse(13-13)ListRecordsResponse(82-85)ValidationResult(190-195)ValidationFilterResult(175-182)airtable/mod.ts (1)
Props(30-54)
airtable/actions/updateField.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)Field(26-32)
airtable/loaders/searchRecords.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)ListRecordsResponse(82-85)
airtable/loaders/listBases.ts (1)
airtable/utils/types.ts (4)
MCPResponse(13-13)ListBasesResponse(49-56)ValidationResult(190-195)ValidationFilterResult(175-182)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airtable/actions/createRecords.ts(1 hunks)airtable/loaders/getRecord.ts(2 hunks)airtable/loaders/listRecords.ts(2 hunks)airtable/loaders/searchRecords.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
airtable/loaders/getRecord.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)AirtableRecord(19-23)
airtable/actions/createRecords.ts (1)
airtable/utils/types.ts (4)
FieldSet(15-17)MCPResponse(13-13)AirtableRecord(19-23)CreateRecordBody(87-90)
airtable/loaders/listRecords.ts (1)
airtable/utils/types.ts (3)
MCPResponse(13-13)ListRecordsResponse(82-85)ValidationFilterResult(175-182)
airtable/loaders/searchRecords.ts (1)
airtable/utils/types.ts (2)
MCPResponse(13-13)ListRecordsResponse(82-85)
🪛 GitHub Actions: ci
airtable/loaders/getRecord.ts
[error] 51-51: Type '{ error: string | undefined; status: any; }' is not assignable to type 'MCPResponse'.
[error] 57-57: Property 'data' does not exist on type 'PermissionCheck | ValidationFilterResult | FilteredOperationResult'.
airtable/actions/createRecords.ts
[error] 67-67: Property 'status' does not exist on type 'ValidationFilterResult | ValidationErrorResult | { error: string; }'.
[error] 71-71: Property 'data' does not exist on type 'PermissionCheck | ValidationFilterResult | FilteredOperationResult'.
[error] 53-53: Property 'status' does not exist on type 'ValidationFilterResult | ValidationErrorResult | { error: string; }'.
airtable/loaders/listRecords.ts
[error] 97-97: Property 'status' does not exist on type 'ValidationFilterResult | ValidationErrorResult | { error: string; }'.
[error] 101-101: Property 'data' does not exist on type 'PermissionCheck | ValidationFilterResult | FilteredOperationResult'.
[error] 105-105: Property 'status' does not exist on type 'PermissionCheck | ValidationFilterResult | FilteredOperationResult'.
| const validationResponse = await ctx.invoke["airtable"].loaders.permissioning | ||
| .validatePermissions({ | ||
| mode: "check", | ||
| baseId: props.baseId, | ||
| tableIdOrName: props.tableId, | ||
| }); | ||
|
|
||
| // Desembrulhar o MCPResponse envelope | ||
| if ("error" in validationResponse) { | ||
| return { | ||
| error: validationResponse.error ?? "Access denied", | ||
| status: validationResponse.status ?? 403, | ||
| }; | ||
| } | ||
|
|
||
| const validationResult = validationResponse.data; | ||
| if ( | ||
| validationResult && | ||
| "hasPermission" in validationResult && | ||
| !validationResult.hasPermission | ||
| ) { | ||
| return { | ||
| error: validationResult.message || "Access denied", | ||
| status: 403, | ||
| }; | ||
| } | ||
|
|
||
| const { baseId, tableId, records, typecast } = props; | ||
|
|
||
| if (!records || records.length === 0) { | ||
| return { | ||
| error: "At least one record is required", | ||
| status: 400, | ||
| }; | ||
| } | ||
|
|
||
| if (records.length > 10) { | ||
| return { | ||
| error: "Maximum 10 records can be created at once", | ||
| status: 400, | ||
| }; | ||
| } | ||
|
|
||
| const invalidRecords = records.filter((record) => | ||
| !record.fields || typeof record.fields !== "object" | ||
| ); | ||
| if (invalidRecords.length > 0) { | ||
| return { | ||
| error: "All records must have a valid 'fields' object", | ||
| status: 400, | ||
| }; | ||
| } | ||
|
|
||
| try { | ||
| const recordsToCreate: CreateRecordBody[] = records.map((record) => { | ||
| const recordBody: CreateRecordBody = { | ||
| fields: record.fields, | ||
| }; | ||
| if (record.typecast !== undefined) { | ||
| recordBody.typecast = record.typecast; | ||
| } | ||
| return recordBody; | ||
| }); |
There was a problem hiding this comment.
Fix permission loader typing mismatch.
validatePermissions is still typed as PermissionCheck | ValidationFilterResult | ValidationErrorResult, so it has neither .data nor .status. Accessing those members (lines 63‑79) causes the build to fail, as shown in CI, and the permission result never unwraps correctly. Please update the permission loader/manifest to actually return an MCPResponse before reading .data, or adjust this call site to handle the existing union shape (gate on error and fall back to the original object). Until then the action won’t compile or enforce permission errors.
🧰 Tools
🪛 GitHub Actions: ci
[error] 67-67: Property 'status' does not exist on type 'ValidationFilterResult | ValidationErrorResult | { error: string; }'.
[error] 71-71: Property 'data' does not exist on type 'PermissionCheck | ValidationFilterResult | FilteredOperationResult'.
🤖 Prompt for AI Agents
In airtable/actions/createRecords.ts around lines 56 to 118, the call to
validatePermissions assumes an MCPResponse with .data and .status but the loader
currently returns a union (PermissionCheck | ValidationFilterResult |
ValidationErrorResult), so accessing .data/.status breaks the build; either
update the permission loader/manifest to return an MCPResponse or change this
call site to narrow the union before unwrapping: first check for the presence of
"error" and return using its error/status, then check for "hasPermission" (the
legacy shape) and handle its message/hasPermission fields, and finally if an
MCPResponse shape is possible check for "data" and use data/status; ensure all
branches return a consistent error/status shape so the action compiles and
permission failures are enforced.
| try { | ||
| const validationResponse = await ctx.invoke["airtable"].loaders | ||
| .permissioning.validatePermissions({ | ||
| mode: "check", | ||
| baseId: props.baseId, | ||
| tableIdOrName: props.tableId, | ||
| }); | ||
|
|
||
| const response = await ctx.client["GET /v0/:baseId/:tableId/:recordId"]( | ||
| props, | ||
| ); | ||
| // Desembrulhar o MCPResponse envelope | ||
| if ("error" in validationResponse) { | ||
| return { | ||
| error: validationResponse.error, | ||
| status: validationResponse.status ?? 403, | ||
| }; | ||
| } | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Error getting record: ${response.statusText}`); | ||
| } | ||
| const validationResult = validationResponse.data; | ||
| if ( | ||
| validationResult && | ||
| "hasPermission" in validationResult && | ||
| !validationResult.hasPermission | ||
| ) { | ||
| return { | ||
| error: validationResult.message || "Access denied", | ||
| status: 403, | ||
| }; | ||
| } | ||
|
|
||
| const response = await ctx.client["GET /v0/:baseId/:tableId/:recordId"]( | ||
| props, | ||
| ); | ||
|
|
||
| return await response.json(); | ||
| if (!response.ok) { | ||
| return { | ||
| error: `Error getting record: ${response.statusText}`, | ||
| status: response.status, | ||
| }; | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| return { | ||
| data, | ||
| }; | ||
| } catch (err) { | ||
| return { | ||
| error: `Error getting record: ${ | ||
| err instanceof Error ? err.message : String(err) | ||
| }`, | ||
| status: 500, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Restore correct permission unwrap.
Here too validatePermissions still returns the legacy union (PermissionCheck | ValidationFilterResult | ValidationErrorResult), so .data/.status are undefined. The new branch therefore fails type-checking (see CI) and would never block permission errors. Please either change the loader/manifest to emit MCPResponse<PermissionCheck> or update this code to inspect the existing union before proceeding.
🧰 Tools
🪛 GitHub Actions: ci
[error] 51-51: Type '{ error: string | undefined; status: any; }' is not assignable to type 'MCPResponse'.
[error] 57-57: Property 'data' does not exist on type 'PermissionCheck | ValidationFilterResult | FilteredOperationResult'.
🤖 Prompt for AI Agents
In airtable/loaders/getRecord.ts around lines 41 to 91, the code assumes
validatePermissions returns an MCPResponse with .data as PermissionCheck but the
actual return is a union (PermissionCheck | ValidationFilterResult |
ValidationErrorResult), so .data/.status may be undefined and permission
failures are not handled; update the unwrap logic to inspect the union shape:
first check for the error-shaped ValidationErrorResult (e.g., presence of
.error/.status) and return accordingly, then check for ValidationFilterResult
and handle its semantics, and finally check for PermissionCheck and use its
hasPermission/message fields to allow or block access; alternatively, if you
prefer changing the manifest/loader, adjust validatePermissions to emit
MCPResponse<PermissionCheck> so this file can safely use .data as
PermissionCheck.
| try { | ||
| const validationResponse = await ctx.invoke["airtable"].loaders | ||
| .permissioning.validatePermissions({ | ||
| mode: "filter", | ||
| props: { ...props, tableIdOrName: props.tableId }, | ||
| }); | ||
|
|
||
| // Desembrulhar o MCPResponse envelope | ||
| if ("error" in validationResponse) { | ||
| return { | ||
| error: validationResponse.error ?? "Access denied", | ||
| status: validationResponse.status ?? 403, | ||
| }; | ||
| } | ||
|
|
||
| const propsValidationResult = validationResponse.data; | ||
| if (!propsValidationResult) { | ||
| return { | ||
| error: "Permission validation failed", | ||
| status: validationResponse.status ?? 500, | ||
| }; | ||
| } | ||
|
|
||
| if ("error" in propsValidationResult && propsValidationResult.error) { | ||
| return { | ||
| error: propsValidationResult.error, | ||
| status: 403, | ||
| }; | ||
| } | ||
|
|
||
| const filterResult = propsValidationResult as ValidationFilterResult; | ||
| const validatedProps = (filterResult.filteredProps || props) as Props; | ||
|
|
||
| const response = await ctx.client["GET /v0/:baseId/:tableId"]( | ||
| validatedProps, | ||
| ); | ||
|
|
||
| if (!response.ok) { | ||
| return { | ||
| error: `Error listing records: ${response.statusText}`, | ||
| status: response.status, | ||
| }; | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| return { | ||
| data, | ||
| }; | ||
| } catch (err) { | ||
| return { | ||
| error: `Error listing records: ${ | ||
| err instanceof Error ? err.message : String(err) | ||
| }`, | ||
| status: 500, | ||
| }; |
There was a problem hiding this comment.
Handle the actual validation result shape.
validatePermissions still exposes ValidationFilterResult | ValidationErrorResult | { error: string; }, so reading .status and .data (lines 94‑106) won’t compile and skips error envelopes. Please update the permission loader/manifest to return MCPResponse<ValidationFilterResult> (with .data) or adjust this block to work with the current union before using the result.
🧰 Tools
🪛 GitHub Actions: ci
[error] 97-97: Property 'status' does not exist on type 'ValidationFilterResult | ValidationErrorResult | { error: string; }'.
[error] 101-101: Property 'data' does not exist on type 'PermissionCheck | ValidationFilterResult | FilteredOperationResult'.
[error] 105-105: Property 'status' does not exist on type 'PermissionCheck | ValidationFilterResult | FilteredOperationResult'.
🤖 Prompt for AI Agents
In airtable/loaders/listRecords.ts around lines 86 to 140, the code assumes
validationResponse has .data and .status but validatePermissions returns a union
(ValidationFilterResult | ValidationErrorResult | { error: string }), so
accessing .data/.status unguarded will not compile and can skip error envelopes;
update either the permission loader signature to return
MCPResponse<ValidationFilterResult> or (recommended) change this block to narrow
the union before using .data/.status: check for the various shapes (e.g., if
("error" in validationResponse && validationResponse.error) return that
envelope; else if ("data" in validationResponse) use validationResponse.data;
else if ("status" in validationResponse) use validationResponse.status or
default values), add explicit type guards/casts to obtain ValidationFilterResult
before treating it as such, and ensure all branches return proper { error,
status } or { data } to satisfy the compiler.
| try { | ||
| const validationResult = await ctx.invoke["airtable"].loaders.permissioning | ||
| .validatePermissions({ | ||
| mode: "check", | ||
| baseId: props.baseId, | ||
| tableIdOrName: props.tableId, | ||
| }); | ||
|
|
||
| if ( | ||
| "hasPermission" in validationResult && !validationResult.hasPermission | ||
| ) { | ||
| return { | ||
| error: validationResult.message || "Access denied", | ||
| status: 403, | ||
| }; | ||
| } | ||
|
|
||
| const { searchTerm, searchFields, ...otherProps } = props; | ||
|
|
||
| // Escape backslashes and double quotes in searchTerm to prevent formula injection | ||
| const escapedSearchTerm = searchTerm | ||
| .replace(/\\/g, "\\\\") | ||
| .replace(/"/g, '\\"'); | ||
|
|
||
| let filterByFormula = ""; | ||
| if (searchFields && searchFields.length > 0) { | ||
| const fieldFormulas = searchFields.map( | ||
| (field) => `SEARCH("${escapedSearchTerm}", {${field}})`, | ||
| ); | ||
| filterByFormula = `OR(${fieldFormulas.join(",")})`; | ||
| } else { | ||
| filterByFormula = `SEARCH("${escapedSearchTerm}", {*})`; | ||
| } | ||
|
|
||
| const response = await ctx.client["GET /v0/:baseId/:tableId"]({ | ||
| ...otherProps, | ||
| filterByFormula, | ||
| }); | ||
|
|
||
| if ("hasPermission" in validationResult && !validationResult.hasPermission) { | ||
| return new Response(validationResult.message || "Access denied", { | ||
| status: 403, | ||
| }); | ||
| } | ||
|
|
||
| const { searchTerm, searchFields, ...otherProps } = props; | ||
|
|
||
| let filterByFormula = ""; | ||
| if (searchFields && searchFields.length > 0) { | ||
| const fieldFormulas = searchFields.map( | ||
| (field) => `SEARCH("${searchTerm}", {${field}})`, | ||
| ); | ||
| filterByFormula = `OR(${fieldFormulas.join(",")})`; | ||
| } else { | ||
| filterByFormula = `SEARCH("${searchTerm}", {*})`; | ||
| if (!response.ok) { | ||
| return { | ||
| error: `Error searching records: ${response.statusText}`, | ||
| status: response.status, | ||
| }; | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| return { | ||
| data, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Short-circuit on permission errors.
validatePermissions can return a { error: string } result. Because we only branch on hasPermission, any validation error is ignored and the search executes with denied access. Add an error guard before the hasPermission check so we return a 403 on permission failures.
- const validationResult = await ctx.invoke["airtable"].loaders.permissioning
- .validatePermissions({
+ const validationResult = await ctx.invoke["airtable"].loaders.permissioning
+ .validatePermissions({
mode: "check",
baseId: props.baseId,
tableIdOrName: props.tableId,
});
+ if ("error" in validationResult && validationResult.error) {
+ return {
+ error: validationResult.error ?? "Access denied",
+ status: 403,
+ };
+ }
+
if (
"hasPermission" in validationResult && !validationResult.hasPermission
) {📝 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.
| try { | |
| const validationResult = await ctx.invoke["airtable"].loaders.permissioning | |
| .validatePermissions({ | |
| mode: "check", | |
| baseId: props.baseId, | |
| tableIdOrName: props.tableId, | |
| }); | |
| if ( | |
| "hasPermission" in validationResult && !validationResult.hasPermission | |
| ) { | |
| return { | |
| error: validationResult.message || "Access denied", | |
| status: 403, | |
| }; | |
| } | |
| const { searchTerm, searchFields, ...otherProps } = props; | |
| // Escape backslashes and double quotes in searchTerm to prevent formula injection | |
| const escapedSearchTerm = searchTerm | |
| .replace(/\\/g, "\\\\") | |
| .replace(/"/g, '\\"'); | |
| let filterByFormula = ""; | |
| if (searchFields && searchFields.length > 0) { | |
| const fieldFormulas = searchFields.map( | |
| (field) => `SEARCH("${escapedSearchTerm}", {${field}})`, | |
| ); | |
| filterByFormula = `OR(${fieldFormulas.join(",")})`; | |
| } else { | |
| filterByFormula = `SEARCH("${escapedSearchTerm}", {*})`; | |
| } | |
| const response = await ctx.client["GET /v0/:baseId/:tableId"]({ | |
| ...otherProps, | |
| filterByFormula, | |
| }); | |
| if ("hasPermission" in validationResult && !validationResult.hasPermission) { | |
| return new Response(validationResult.message || "Access denied", { | |
| status: 403, | |
| }); | |
| } | |
| const { searchTerm, searchFields, ...otherProps } = props; | |
| let filterByFormula = ""; | |
| if (searchFields && searchFields.length > 0) { | |
| const fieldFormulas = searchFields.map( | |
| (field) => `SEARCH("${searchTerm}", {${field}})`, | |
| ); | |
| filterByFormula = `OR(${fieldFormulas.join(",")})`; | |
| } else { | |
| filterByFormula = `SEARCH("${searchTerm}", {*})`; | |
| if (!response.ok) { | |
| return { | |
| error: `Error searching records: ${response.statusText}`, | |
| status: response.status, | |
| }; | |
| } | |
| const data = await response.json(); | |
| return { | |
| data, | |
| }; | |
| try { | |
| const validationResult = await ctx.invoke["airtable"].loaders.permissioning | |
| .validatePermissions({ | |
| mode: "check", | |
| baseId: props.baseId, | |
| tableIdOrName: props.tableId, | |
| }); | |
| if ("error" in validationResult && validationResult.error) { | |
| return { | |
| error: validationResult.error ?? "Access denied", | |
| status: 403, | |
| }; | |
| } | |
| if ( | |
| "hasPermission" in validationResult && !validationResult.hasPermission | |
| ) { | |
| return { | |
| error: validationResult.message || "Access denied", | |
| status: 403, | |
| }; | |
| } | |
| const { searchTerm, searchFields, ...otherProps } = props; | |
| // Escape backslashes and double quotes in searchTerm to prevent formula injection | |
| const escapedSearchTerm = searchTerm | |
| .replace(/\\/g, "\\\\") | |
| .replace(/"/g, '\\"'); | |
| let filterByFormula = ""; | |
| if (searchFields && searchFields.length > 0) { | |
| const fieldFormulas = searchFields.map( | |
| (field) => `SEARCH("${escapedSearchTerm}", {${field}})`, | |
| ); | |
| filterByFormula = `OR(${fieldFormulas.join(",")})`; | |
| } else { | |
| filterByFormula = `SEARCH("${escapedSearchTerm}", {*})`; | |
| } | |
| const response = await ctx.client["GET /v0/:baseId/:tableId"]({ | |
| ...otherProps, | |
| filterByFormula, | |
| }); | |
| if (!response.ok) { | |
| return { | |
| error: `Error searching records: ${response.statusText}`, | |
| status: response.status, | |
| }; | |
| } | |
| const data = await response.json(); | |
| return { | |
| data, | |
| }; |
🤖 Prompt for AI Agents
In airtable/loaders/searchRecords.ts around lines 87 to 136, the
validatePermissions result can be an error object which is currently ignored;
add a guard that checks if ("error" in validationResult) and if so return {
error: validationResult.error || "Access denied", status: 403 } before the
existing hasPermission branch so permission validation failures short-circuit
and do not allow the search to proceed.
1 participant
What is this Contribution About?
Please provide a brief description of the changes or enhancements you are proposing in this pull request.
Issue Link
Please link to the relevant issue that this pull request addresses:
Loom Video
Demonstration Link
Summary by CodeRabbit
New Features
Refactor