feat(connectors): unified auth ui - #86
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughMigrates from a legacy ProviderRegistry to a piece-based model, replacing env with vendorParams across OAuth flows and Redis-backed state, adds resolveOAuth2Url and piece schema/category changes, moves Salesforce/QuickBooks into pieces, and updates controllers, services, frontend forms, and many tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Web Client
participant API as API Server
participant PieceReg as Piece Registry
participant OAuth as OAuth Provider
participant Redis as Redis
participant DB as Database
Note over Client,API: Initiation includes vendorParams (safe vs secret split)
Client->>API: GET /initiate?provider=mock-piece&vendorParams={...}
API->>PieceReg: getPiece("mock-piece")
PieceReg-->>API: Piece{auth.authorizeUrl (template), props}
API->>API: validate vendorParams vs piece.auth.props
API->>API: resolvedUrl = resolveOAuth2Url(authorizeUrl, vendorParams)
API-->>Client: redirect to resolvedUrl
Note over OAuth,API: Callback with code & state
OAuth-->>Client: redirect /callback?code=...&state=...
Client->>API: GET /callback?code=...&state=...
API->>Redis: verifyState(state) -> getdel(oauth:state:<stateId>) (returns vendorParams)
API->>PieceReg: getPiece("mock-piece")
PieceReg-->>API: Piece{auth.tokenUrl (template)}
API->>API: tokenUrl = resolveOAuth2Url(tokenUrl, vendorParams)
API->>OAuth: POST tokenUrl (code, client creds)
OAuth-->>API: tokenResponse
API->>API: piece.auth.validateConnectResponse?(tokenResponse)
API->>DB: store credential blob { data: tokenResponse, vendorParams }
API-->>Client: popup success
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
packages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.ts (1)
48-62:⚠️ Potential issue | 🔴 CriticalSOQL injection vulnerability via
tieBreakerValueand missing validation fortieBreakerField.The
cursorValueis validated viavalidateCursor(which restricts to safe characters), buttieBreakerValueis interpolated directly into the query without similar validation. A value containing single quotes (e.g.,O'Brien) could break the query or enable SOQL injection.Additionally,
tieBreakerFieldis not validated against the schema likecursorFieldis (lines 49-52), which could allow invalid field names or injection.🔒 Proposed fix to validate tie-breaker inputs
private buildWhereClause(schema: ObjectSchema, spec: QuerySpec): string { const cursorFieldDef = schema.fields.find(f => f.name === spec.cursorField); if (!cursorFieldDef) { throw new Error(`Invalid cursorField: '${spec.cursorField}' not found on object '${spec.objectName}'`); } const isStringType = ['string', 'id', 'reference'].includes(cursorFieldDef.type.toLowerCase()); const formattedCursorValue = isStringType ? `'${spec.cursorValue}'` : spec.cursorValue; - const tbFormatted = spec.tieBreakerField && spec.tieBreakerValue ? `'${spec.tieBreakerValue}'` : null; + let tbFormatted: string | null = null; + if (spec.tieBreakerField && spec.tieBreakerValue) { + const tbFieldDef = schema.fields.find(f => f.name === spec.tieBreakerField); + if (!tbFieldDef) { + throw new Error(`Invalid tieBreakerField: '${spec.tieBreakerField}' not found on object '${spec.objectName}'`); + } + this.validateCursor(spec.tieBreakerValue); + tbFormatted = `'${spec.tieBreakerValue}'`; + } if (spec.tieBreakerField && tbFormatted) { return `(${spec.cursorField} > ${formattedCursorValue} OR (${spec.cursorField} = ${formattedCursorValue} AND ${spec.tieBreakerField} > ${tbFormatted}))`; } return `${spec.cursorField} > ${formattedCursorValue}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.ts` around lines 48 - 62, In buildWhereClause: validate tieBreakerField against the schema (like cursorField) by finding it in schema.fields and throwing if missing, and validate/sanitize tieBreakerValue the same way cursorValue is handled (reuse validateCursor or the same escaping logic) before interpolating; ensure you detect tieBreakerField's type (e.g., string/id/reference) and format/escape tbFormatted safely (escape single quotes or use a common formatter) so neither tieBreakerField nor tieBreakerValue can inject SOQL when building the returned clause.apps/web/src/modules/connections/pages/ConnectionsPage.tsx (1)
68-71: 🧹 Nitpick | 🔵 TrivialConsider removing the empty header container.
The empty
<div></div>element serves no purpose after the header content was removed. Consider cleaning it up or adding placeholder content if the header will be restored.♻️ Suggested cleanup
<div className="flex items-center justify-between gap-4"> - <div> - </div> <Button🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modules/connections/pages/ConnectionsPage.tsx` around lines 68 - 71, In ConnectionsPage remove the empty header container: delete the stray <div></div> between the header area and the Button in the JSX (the empty element shown near the top of the component) or replace it with meaningful placeholder content if the header will be restored; ensure the surrounding layout (the parent div with className="flex items-center justify-between gap-4") still renders correctly after removing that empty element.packages/connectors/src/framework/piece.ts (1)
41-42:⚠️ Potential issue | 🟠 MajorMake
authrequired at piece construction time.
Piece.authis now mandatory and downstream code already dereferencespiece.auth.type, butCreatePieceParams.authis still optional and the non-null assertion hides missing configs until runtime. Fail fast here instead of registering invalid pieces.Proposed fix
export interface CreatePieceParams { name?: string; displayName: string; logoUrl: string; authors?: string[]; categories?: any[]; - auth?: PieceAuthProperty; + auth: PieceAuthProperty; actions: Action[]; triggers: Trigger[]; description?: string; minimumSupportedRelease?: string; maximumSupportedRelease?: string; } @@ export function createPiece(params: CreatePieceParams): Piece { + if (!params.auth) { + throw new InternalServerErrorException( + `Piece "${params.name ?? params.displayName}" is missing auth`, + ); + } + @@ - auth: params.auth!, + auth: params.auth, categories: (params.categories ?? []).map(String),Also applies to: 92-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/connectors/src/framework/piece.ts` around lines 41 - 42, CreatePieceParams currently defines auth as optional even though Piece.auth is required and consumers read piece.auth.type; change the CreatePieceParams.auth? optional property to a required auth: PieceAuthProperty so missing auth is a compile-time error. Update the other occurrence of the same optional auth declaration (the block around the 92-94 region) to the same required auth: PieceAuthProperty type, and ensure any factory/constructor functions that accept CreatePieceParams now expect and validate the provided auth value (symbols to locate: CreatePieceParams, PieceAuthProperty, and any create/register piece functions in piece.ts).apps/web/src/modules/connections/components/DynamicAuthForm.tsx (1)
321-344:⚠️ Potential issue | 🟠 MajorSkip unsupported
uiSchemaprops in the render loop.The schema/default builders already ignore unknown
prop.types, but this loop renders every entry. That creates fields with no matching form-shape handling, so they can appear usable and then be dropped or mis-serialized on submit. Filter withisSupportedUiPropType()here too, or render an explicit unsupported-field state instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx` around lines 321 - 344, The dynamic UI loop in DynamicAuthForm.tsx currently maps every entry in provider.uiSchema into a FormField, which allows unsupported prop types to render with no proper controls; update the mapping to first filter entries using isSupportedUiPropType(prop.type) (or, if you prefer, detect unsupported types and render an explicit unsupported-field state) before calling renderFieldControl and creating the FormField so only supported uiSchema props are rendered as interactive controls; reference provider.uiSchema, isSupportedUiPropType, renderFieldControl, and FormField when making the change.apps/api/src/modules/connections/connectors.service.ts (1)
377-381:⚠️ Potential issue | 🟠 MajorDon’t leave
ACTIVEconnections behind when namespace provisioning fails.
applyPlan()runs after the transaction commits, so a failure here leavesapp_connectionandconnectionStorageRegistrypersisted even though the workspace schema does not exist. Consider writing the connection in a pending state and promoting it toACTIVEonly afterapplyPlan()succeeds, or add compensating cleanup on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/connectors.service.ts` around lines 377 - 381, The current flow persists app_connection and connectionStorageRegistry as ACTIVE before calling this.dbManager.applyPlan (namespace provisioning), so if applyPlan fails the DB records remain while the schema is missing; change the flow to create the connection and registry in a PENDING state (or with a status flag) inside the transaction, then call this.dbManager.applyPlan(workspaceSchemaName, SchemaPlan.NAMESPACE_ONLY) and only after it succeeds update/promote the corresponding app_connection and connectionStorageRegistry rows to ACTIVE (or perform an explicit compensating delete of both records if applyPlan throws), updating the methods that set connection status and any callers of createConnection/createConnector to handle the PENDING→ACTIVE transition.apps/api/src/modules/connections/connections/token-refresh.service.ts (1)
155-174:⚠️ Potential issue | 🟠 MajorKeep a fallback for pre-
vendorParamsconnection blobs during refresh.
getCredentials()now falls back straight to{}when the decrypted blob predates this migration and only carries the legacy environment field. Once those existing connections refresh,resolveOAuth2Url()can target the wrong host or fail outright for non-default environments. Please read the legacy field here or backfill stored blobs before switching refresh traffic to this code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/connections/token-refresh.service.ts` around lines 155 - 174, getCredentials() currently returns vendorParams = valueBlob.vendorParams ?? {}, which drops legacy environment info and can break resolveOAuth2Url(); update getCredentials() to preserve/backfill that legacy field by checking for valueBlob.environment (or other legacy keys) when vendorParams is missing and merging it into the returned vendorParams (e.g., vendorParams = { ...(valueBlob.vendorParams ?? {}), ...(valueBlob.environment ? { environment: valueBlob.environment } : {}) }), so resolveOAuth2Url() can still read the intended host; optionally add a TODO to persist the backfilled blob later.apps/api/src/modules/connections/connections/connectors.controller.ts (2)
515-577:⚠️ Potential issue | 🟠 MajorUse the signed state's vendor params as the source of truth during token exchange.
After
verifyState()you only checktenantId; the token exchange and persisted blob still trustbody.vendorParams. A client can initiate OAuth with one environment and then POST a different one here, which breaks the integrity of the signed state and can route the token request to the wrong endpoint. If callback-only fields also need to survive, keep them separate from the state-bound input params instead of letting the request body override them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/connections/connectors.controller.ts` around lines 515 - 577, The code currently uses the request body’s vendorParams during the token exchange and when building the ConnectionValueBlob, allowing an attacker to swap those values; after calling oauthStateService.verifyState(...) use the verified decodedState (e.g., decodedState.vendorParams) as the authoritative vendorParams for the call to connectorsService.exchangeCodeForTokens(...) and when constructing valueBlob instead of vendorParams from the request body, and if there are callback-only fields that must survive the redirect keep them in a separate field (e.g., callbackFields) that is merged only from decodedState and never overwritten by body input.
52-72:⚠️ Potential issue | 🟠 MajorDo not let unsupported vendor params bypass schema validation.
validateVendorParams()currently accepts too much: ifschemais missing it returns immediately, and even when a schema exists it never enforces declared option sets for dropdown-style fields. That lets callers send arbitrary URL-template inputs undervendorParams, which is exactly the data later used to resolve OAuth endpoints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/connections/connectors.controller.ts` around lines 52 - 72, validateVendorParams currently returns early when schema is undefined and doesn't enforce allowed option sets for enum/dropdown fields; update it so that if schema is undefined but vendorParams is non-empty it throws a BadRequestException (i.e., no vendor params are allowed without an explicit schema), and ensure declared-field validation enforces allowed options by enhancing assertPropValue (or calling a new helper from validateVendorParams) to reject values not in prop.enum/allowedOptions for AnyProperty enum/dropdown types; reference validateVendorParams and assertPropValue to locate where to add the checks and throw BadRequestException on any disallowed/undeclared vendorParams.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/connections/connections/connectors.controller.ts`:
- Around line 433-458: The parsed vendorParams from vendorParamsJson is
currently cast to Record<string,string> without validation before being passed
into oauthStateService.generateState and connectorsService.getAuthorizationUrl;
update initiateOAuth to fully validate the parsed object keys/values (ensure
it's a plain object, all values are strings, no nested objects/arrays, and
optionally enforce a max number of keys/total length) and throw a
BadRequestException on invalid input, then only pass the validated vendorParams
into generateState and getAuthorizationUrl; this validation should occur right
after JSON.parse and before calling generateState/getAuthorizationUrl so both
oauthStateService.generateState and connectorsService.getAuthorizationUrl
receive sanitized vendorParams.
In `@apps/api/src/modules/connections/connections/token-refresh.service.spec.ts`:
- Around line 16-28: Add a regression test that verifies templated token URLs
are resolved from stored vendorParams: create a MOCK_OAUTH2_PIECE-like fixture
where auth.tokenUrl is templated (e.g. "https://{env}.mock.com/token"), add a
credentials object that includes vendorParams with the env value, and exercise
the same refresh flow in token-refresh.service.spec.ts (the existing tests using
MOCK_OAUTH2_PIECE and the refresh method). Assert that the refresh call uses the
resolved URL (https://<env>.mock.com/token) rather than the literal template;
this will catch regressions in template substitution when reading vendorParams
during refresh.
In `@apps/api/src/modules/connections/oauth-state.service.ts`:
- Around line 39-56: The generateState function currently embeds arbitrary
vendorParams into the signed JWT state; instead, change generateState to create
a random opaque stateId (e.g., uuid), store vendorParams server-side (DB/cache)
keyed by that stateId, and sign only { tenantId, provider,
purpose:'oauth_state_handshake', stateId } in the JWT; update the
state-consumption path (the function that validates/parses the JWT on
callback—e.g., the validate/consume state handler) to read stateId from the JWT
and fetch the vendorParams from storage, then delete/expire the stored params to
avoid leakage.
In `@apps/web/src/modules/connections/api/connections.api.ts`:
- Around line 57-66: The vendorParams typing in getConnectionCredentials (and
the similar read/write contract around lines 70-79) currently uses
Record<string, string>, which drops boolean semantics; change the type to
Record<string, string | boolean> (or a Value union like string | boolean) for
vendorParams in the Promise return type and the apiClient.get generic so boolean
checkbox values are preserved on round-trip, and update the corresponding
write/update method signatures in this file to use the same union type so
frontend and backend contracts remain aligned.
In `@apps/web/src/modules/connections/components/ConnectAppCard.tsx`:
- Around line 149-155: The current defaultValues in ConnectAppCard.tsx uses raw
defaultCreds?.vendorParams (set as strings), which breaks typed controls; update
the useMemo block (defaultValues) to coerce vendorParams back to their proper
types based on provider.uiSchema before spreading them: implement a small helper
that iterates vendorParams keys, looks up the corresponding schema/type in
provider.uiSchema (e.g., "boolean", "number", "integer"), and converts string
values like "true"/"false" to booleans and numeric strings to numbers (leaving
other types as-is), then spread the coerced params instead of the raw
defaultCreds?.vendorParams so checkboxes and numeric inputs render correctly on
reconnect.
In `@apps/web/src/modules/connections/hooks/useConnections.ts`:
- Around line 127-136: The popup URL currently serializes the entire
vendorParams into the URL (popupUrl) which can leak SECRET_TEXT values; modify
the useConnections hook so that before appending vendorParams to popupUrl you
create a URL-safe bag (e.g., safeVendorParams) that excludes secret fields —
filter vendorParams by metadata or by key list to remove SECRET_TEXT entries (or
only include explicitly non-secret fields) and only encode
JSON.stringify(safeVendorParams); alternatively omit vendorParams entirely from
the URL and rely on pendingCredentials + handleSuccess to merge secret fields on
the server; update the popup URL construction (popupUrl, vendorParams,
connect()) to use the filtered/omitted bag and ensure
handleSuccess/pendingCredentials still receive secrets via secure channels.
In `@packages/connectors/src/framework/auth.ts`:
- Around line 15-27: The resolveOAuth2Url function currently inserts
user-controlled vendorParams directly into template; validate each substituted
value (from vendorParams) before replacing: for each key referenced in the
template, ensure the value is non-empty and does not contain unsafe reserved URL
delimiters (e.g., '@', '/', '?', '#', ':') or other characters that can change
URL structure, and if such characters are present either reject with a clear
Error mentioning the key or safely percent-encode the value (e.g., via
encodeURIComponent) depending on intended URL context; update resolveOAuth2Url
to perform this validation/encoding for every placeholder replacement so
authUrl/tokenUrl cannot be reshaped by raw user input.
In `@packages/connectors/src/intelligence/interfaces.ts`:
- Around line 84-85: Extract the inline return payload used by checkApiLimits
into a new exported type (e.g., ApiRateLimit or RateLimitInfo) and update the
checkApiLimits signature to return Promise<ApiRateLimit | null>; locate the
declaration for checkApiLimits in interfaces.ts (the checkApiLimits?: (auth:
TAuth, store: TriggerStore) => Promise<{ remaining: number; total: number } |
null> line), add a named export for the new type, and replace the inline object
type with that type so implementations and tests can import and reuse the
contract.
In `@packages/connectors/src/intelligence/universal-trigger-engine.spec.ts`:
- Around line 237-240: The test "should block polling if API-limit gating
triggers" is brittle because it relies on ambient SF_API_LIMIT_THRESHOLD; set
process.env.SF_API_LIMIT_THRESHOLD to a deterministic value (e.g. "0.05" or
another threshold that makes remaining 100/total 10000 trigger) at the start of
the test and restore the previous value after the test finishes (or use a local
before/after hook) so assertions remain deterministic; modify the spec around
the createConfig call in universal-trigger-engine.spec.ts to explicitly set and
then reset SF_API_LIMIT_THRESHOLD for this test.
In `@packages/connectors/src/intelligence/universal-trigger-engine.ts`:
- Around line 105-110: The preflight call to checkApiLimits can hang because
it's an external connector callback; wrap the await of checkApiLimits in a
bounded timeout (e.g., via a Promise.race or existing timeout helper) so that if
it doesn't resolve within a short interval you treat it as a timeout, log the
timeout similarly to the catch (include context and error/timeout marker), set
apiLimits to null and return true (fail open) just like the catch path; update
the code around the existing apiLimits/checkApiLimits usage in
universal-trigger-engine (the try/catch block that calls checkApiLimits and the
subsequent log.debug) to implement this timeout behavior.
In `@packages/pieces/salesforce/package.json`:
- Around line 4-13: The package.json currently points to non-existent build
outputs ("main", "types", and "exports" entries reference dist/src/index.*);
either set "rootDir": "src" in tsconfig.lib.json so the compiler emits
dist/src/index.js and dist/src/index.d.ts, or update the package.json entries
("main", "types", and the "exports" mapping) to reference dist/index.js and
dist/index.d.ts instead — choose one approach and make the paths consistent
across the "main", "types", and "exports" fields.
In `@packages/pieces/salesforce/src/lib/auth.ts`:
- Around line 3-9: salesforceAuth currently hardcodes production endpoints and
skips validating the token response; restore the environment-aware config and
instance_url check: update the salesforceAuth PieceAuth.OAuth2 definition to
accept an environment selector (e.g., 'production' vs 'sandbox') and use
templated authUrl/tokenUrl values (use https://login.salesforce.com/... for
production and https://test.salesforce.com/... for sandbox) instead of fixed
URLs, keep scopes/refresh handling, and add post-token validation to ensure the
returned token response includes instance_url (throw or surface an error if
missing) because the rest of the code (the Salesforce client reads instance_url
on every API call) depends on it.
In `@packages/pieces/salesforce/src/lib/trigger/salesforce-polling.helper.ts`:
- Around line 39-41: The columns construction interpolates unvalidated dateField
and extraColumns into SOQL (variables: dateField, extraColumns, columns, soql,
url), enabling SOQL injection; validate and sanitize dateField and each entry in
extraColumns against a strict Salesforce field-name pattern (e.g., allow only
letters, digits, underscores and optional __r/__c suffixes) and reject or throw
if any item fails, then build the columns string from the validated list; also
replace the hardcoded API version string "v59.0" with the shared SF_API_VERSION
import from ../sf-fetch.js so the URL uses
`${auth.instance_url}/services/data/${SF_API_VERSION}/query?q=${soql}`.
- Around line 43-46: Replace the native fetch call with the shared sfFetch
helper so Salesforce auth-related errors (e.g., SalesforceAuthError on
expired/revoked tokens) are handled consistently: import and call sfFetch
instead of fetch (keeping the Authorization header with `Bearer
${auth.access_token}`), remove or adapt the manual response.ok check to use
sfFetch's error handling, and ensure any thrown errors still surface
appropriately instead of the generic `new Error('Salesforce SOQL query failed')`
(update the code surrounding the fetch call that references `auth.access_token`
and the response/error handling to use sfFetch).
In `@packages/pieces/salesforce/src/lib/trigger/universal-trigger.ts`:
- Around line 165-166: The current explicit cast on checkSalesforceLimits (used
for checkApiLimits alongside executeCountQuery) weakens type safety by using
any; remove the cast and introduce a shared typed function signature (e.g.,
CheckApiLimitsFn) that matches the engine's expected shape (auth with
access_token/instance_url and TriggerStore) and returns Promise<{
remaining:number; total:number } | null>, then change the checkSalesforceLimits
declaration to conform to that type and update the checkApiLimits assignment to
use the typed function reference so TypeScript enforces compatibility between
checkSalesforceLimits and the expected callback.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connections/connectors.controller.ts`:
- Around line 515-577: The code currently uses the request body’s vendorParams
during the token exchange and when building the ConnectionValueBlob, allowing an
attacker to swap those values; after calling oauthStateService.verifyState(...)
use the verified decodedState (e.g., decodedState.vendorParams) as the
authoritative vendorParams for the call to
connectorsService.exchangeCodeForTokens(...) and when constructing valueBlob
instead of vendorParams from the request body, and if there are callback-only
fields that must survive the redirect keep them in a separate field (e.g.,
callbackFields) that is merged only from decodedState and never overwritten by
body input.
- Around line 52-72: validateVendorParams currently returns early when schema is
undefined and doesn't enforce allowed option sets for enum/dropdown fields;
update it so that if schema is undefined but vendorParams is non-empty it throws
a BadRequestException (i.e., no vendor params are allowed without an explicit
schema), and ensure declared-field validation enforces allowed options by
enhancing assertPropValue (or calling a new helper from validateVendorParams) to
reject values not in prop.enum/allowedOptions for AnyProperty enum/dropdown
types; reference validateVendorParams and assertPropValue to locate where to add
the checks and throw BadRequestException on any disallowed/undeclared
vendorParams.
In `@apps/api/src/modules/connections/connections/token-refresh.service.ts`:
- Around line 155-174: getCredentials() currently returns vendorParams =
valueBlob.vendorParams ?? {}, which drops legacy environment info and can break
resolveOAuth2Url(); update getCredentials() to preserve/backfill that legacy
field by checking for valueBlob.environment (or other legacy keys) when
vendorParams is missing and merging it into the returned vendorParams (e.g.,
vendorParams = { ...(valueBlob.vendorParams ?? {}), ...(valueBlob.environment ?
{ environment: valueBlob.environment } : {}) }), so resolveOAuth2Url() can still
read the intended host; optionally add a TODO to persist the backfilled blob
later.
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 377-381: The current flow persists app_connection and
connectionStorageRegistry as ACTIVE before calling this.dbManager.applyPlan
(namespace provisioning), so if applyPlan fails the DB records remain while the
schema is missing; change the flow to create the connection and registry in a
PENDING state (or with a status flag) inside the transaction, then call
this.dbManager.applyPlan(workspaceSchemaName, SchemaPlan.NAMESPACE_ONLY) and
only after it succeeds update/promote the corresponding app_connection and
connectionStorageRegistry rows to ACTIVE (or perform an explicit compensating
delete of both records if applyPlan throws), updating the methods that set
connection status and any callers of createConnection/createConnector to handle
the PENDING→ACTIVE transition.
In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx`:
- Around line 321-344: The dynamic UI loop in DynamicAuthForm.tsx currently maps
every entry in provider.uiSchema into a FormField, which allows unsupported prop
types to render with no proper controls; update the mapping to first filter
entries using isSupportedUiPropType(prop.type) (or, if you prefer, detect
unsupported types and render an explicit unsupported-field state) before calling
renderFieldControl and creating the FormField so only supported uiSchema props
are rendered as interactive controls; reference provider.uiSchema,
isSupportedUiPropType, renderFieldControl, and FormField when making the change.
In `@apps/web/src/modules/connections/pages/ConnectionsPage.tsx`:
- Around line 68-71: In ConnectionsPage remove the empty header container:
delete the stray <div></div> between the header area and the Button in the JSX
(the empty element shown near the top of the component) or replace it with
meaningful placeholder content if the header will be restored; ensure the
surrounding layout (the parent div with className="flex items-center
justify-between gap-4") still renders correctly after removing that empty
element.
In `@packages/connectors/src/framework/piece.ts`:
- Around line 41-42: CreatePieceParams currently defines auth as optional even
though Piece.auth is required and consumers read piece.auth.type; change the
CreatePieceParams.auth? optional property to a required auth: PieceAuthProperty
so missing auth is a compile-time error. Update the other occurrence of the same
optional auth declaration (the block around the 92-94 region) to the same
required auth: PieceAuthProperty type, and ensure any factory/constructor
functions that accept CreatePieceParams now expect and validate the provided
auth value (symbols to locate: CreatePieceParams, PieceAuthProperty, and any
create/register piece functions in piece.ts).
In `@packages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.ts`:
- Around line 48-62: In buildWhereClause: validate tieBreakerField against the
schema (like cursorField) by finding it in schema.fields and throwing if
missing, and validate/sanitize tieBreakerValue the same way cursorValue is
handled (reuse validateCursor or the same escaping logic) before interpolating;
ensure you detect tieBreakerField's type (e.g., string/id/reference) and
format/escape tbFormatted safely (escape single quotes or use a common
formatter) so neither tieBreakerField nor tieBreakerValue can inject SOQL when
building the returned clause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9736695e-10b5-4064-a6b7-ff27c5a91361
📒 Files selected for processing (52)
TECHNICAL_DEBT.mdapps/api/src/db/database-manager.spec.tsapps/api/src/modules/connections/connections.module.tsapps/api/src/modules/connections/connections/callback.controller.spec.tsapps/api/src/modules/connections/connections/callback.controller.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/connections/connections/token-refresh.service.spec.tsapps/api/src/modules/connections/connections/token-refresh.service.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/connections/oauth-state.service.spec.tsapps/api/src/modules/connections/oauth-state.service.tsapps/api/src/modules/trigger/piece-registry.service.spec.tsapps/web/src/modules/connections/api/connections.api.tsapps/web/src/modules/connections/components/ConnectAppCard.tsxapps/web/src/modules/connections/components/DynamicAuthForm.tsxapps/web/src/modules/connections/hooks/useConnections.tsapps/web/src/modules/connections/pages/ConnectionsPage.tsxpackages/connectors/src/apps/salesforce/index.tspackages/connectors/src/apps/salesforce/triggers/new-record.tspackages/connectors/src/apps/salesforce/triggers/salesforce-polling.helper.tspackages/connectors/src/apps/salesforce/triggers/updated-record.tspackages/connectors/src/framework/auth.spec.tspackages/connectors/src/framework/auth.tspackages/connectors/src/framework/piece.tspackages/connectors/src/index.tspackages/connectors/src/intelligence/index.tspackages/connectors/src/intelligence/interfaces.tspackages/connectors/src/intelligence/universal-trigger-engine.spec.tspackages/connectors/src/intelligence/universal-trigger-engine.tspackages/connectors/src/oauth/provider-registry.tspackages/connectors/src/oauth/providers/index.tspackages/connectors/src/oauth/providers/quickbooks.tspackages/connectors/src/oauth/providers/salesforce.tspackages/connectors/src/oauth/types.tspackages/pieces/quickbooks/src/index.tspackages/pieces/quickbooks/src/lib/auth.tspackages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.tspackages/pieces/salesforce/package.jsonpackages/pieces/salesforce/src/index.tspackages/pieces/salesforce/src/lib/auth.tspackages/pieces/salesforce/src/lib/common/index.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.spec.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-discovery.adapter.spec.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-discovery.adapter.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.tspackages/pieces/salesforce/src/lib/sf-fetch.tspackages/pieces/salesforce/src/lib/trigger/salesforce-polling.helper.tspackages/pieces/salesforce/src/lib/trigger/universal-trigger.tspackages/pieces/salesforce/tsconfig.lib.json
💤 Files with no reviewable changes (11)
- packages/pieces/salesforce/src/lib/sf-fetch.ts
- packages/connectors/src/oauth/providers/index.ts
- apps/api/src/modules/connections/connections.module.ts
- packages/connectors/src/apps/salesforce/triggers/new-record.ts
- packages/connectors/src/oauth/providers/quickbooks.ts
- packages/connectors/src/oauth/providers/salesforce.ts
- packages/connectors/src/oauth/types.ts
- packages/connectors/src/apps/salesforce/triggers/updated-record.ts
- packages/connectors/src/oauth/provider-registry.ts
- packages/connectors/src/apps/salesforce/triggers/salesforce-polling.helper.ts
- packages/connectors/src/apps/salesforce/index.ts
pramodnarayana
force-pushed
the
feat/connection-auth-ui-unification
branch
from
March 9, 2026 07:40
261bfbd to
4e23c95
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.ts (2)
37-42: 🧹 Nitpick | 🔵 TrivialConsider logging when fallback branch is reached.
The
return []at line 41 would only execute ifcheckpoint.statehas an unexpected value (e.g., corrupted stored data). Adding a warning log would help diagnose such edge cases.🔍 Proposed observability improvement
if (checkpoint.state === 'IN_PROGRESS' || checkpoint.state === 'AWAITING_RESULTS') { return this.checkJobStatusAndDownload(auth, store, storeKey, checkpoint); } + log.warn('Unexpected bulk job checkpoint state', { state: checkpoint.state, jobId: checkpoint.jobId }); return []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.ts` around lines 37 - 42, Add a warning log before the fallback `return []` so unexpected checkpoint states are observable: in the method that checks `checkpoint.state` (which currently calls `checkJobStatusAndDownload` for 'IN_PROGRESS' or 'AWAITING_RESULTS'), log a warning (e.g., `this.logger.warn` or the class logger) that includes the unexpected `checkpoint.state` value and identifying info such as `storeKey` and any `checkpoint.jobId`/job identifier, then return the empty array as before.
127-129: 🧹 Nitpick | 🔵 TrivialConsider accepting
storeKeyas a parameter for consistency.The
checkpointmethod hardcodes'igt_bulk_job_checkpoint'while other methods receivestoreKeyas a parameter (lines 48, 86). Similarly,downloadResultsat line 148 also hardcodes the key. This inconsistency could cause subtle bugs if the store key is ever changed or made configurable.♻️ Proposed refactor for consistency
- private async checkpoint(store: TriggerStore, data: BulkJobCheckpoint): Promise<void> { - await store.put('igt_bulk_job_checkpoint', data); + private async checkpoint(store: TriggerStore, storeKey: string, data: BulkJobCheckpoint): Promise<void> { + await store.put(storeKey, data); }Then update callers at lines 78 and 108 to pass
storeKey, and updatedownloadResultsto acceptstoreKeyas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.ts` around lines 127 - 129, The checkpoint method currently hardcodes the key 'igt_bulk_job_checkpoint'; change the signature of private async checkpoint(store: TriggerStore, data: BulkJobCheckpoint) to accept storeKey: string (e.g., checkpoint(store: TriggerStore, storeKey: string, data: BulkJobCheckpoint)) and replace the hardcoded key in store.put with the passed storeKey; similarly update downloadResults to accept storeKey and use it instead of the literal; then update all callers that invoke checkpoint and downloadResults (the places that currently call checkpoint(...) and downloadResults(...), including the two call sites noted in the diff) to pass the same storeKey they use elsewhere so the key usage is consistent across TriggerStore interactions.apps/api/src/db/database-manager.spec.ts (1)
90-93:⚠️ Potential issue | 🟡 MinorResolve the mocked insert promise to the builder, not
undefined.The test comment states that
values()should support bothawait values()andvalues().returning(...), but the mock resolves toundefinedwhen awaited. While current production code never captures the awaited result, this test fidelity gap could mask bugs if code patterns change.Proposed fix
- const promise = Promise.resolve(undefined) as Promise<undefined> & - typeof chain; + const promise = Promise.resolve(chain) as Promise<typeof chain> & + typeof chain; Object.assign(promise, chain); return promise;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/db/database-manager.spec.ts` around lines 90 - 93, The mocked insert promise resolves to undefined; change it to resolve to the builder object so awaiting values() returns the chain as described by the test. Locate the mock where const promise = Promise.resolve(undefined) as Promise<undefined> & typeof chain; and replace it so the promise resolves the builder (e.g., Promise.resolve(chain) with the appropriate type assertion), ensuring values() supports both await values() and values().returning(...) by returning the same chain object when awaited.apps/api/src/modules/connections/oauth-state.service.spec.ts (1)
103-122: 🧹 Nitpick | 🔵 TrivialClarify the vendorParams storage design in the test.
Line 119 asserts
decoded.vendorParamsisundefinedwhile the test passes{ realmId: 'test-123' }togenerateState. This suggests vendorParams are intentionally stored in Redis rather than the JWT payload. Consider adding a brief comment explaining this design choice to prevent future confusion:Proposed clarifying comment
// We can manually decode to verify contents without verifying signature const decoded = jwt.decode(stateToken) as jwt.JwtPayload; expect(decoded.tenantId).toBe(mockTenantId); expect(decoded.provider).toBe(mockProvider); + // vendorParams are stored in Redis, not in the JWT payload expect(decoded.vendorParams).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/oauth-state.service.spec.ts` around lines 103 - 122, The test calls service.generateState(mockTenantId, mockProvider, { realmId: 'test-123' }) but asserts decoded.vendorParams is undefined; add a short clarifying comment above the assertion explaining that generateState persists vendorParams (e.g., realmId) to Redis (or another store) rather than embedding them in the JWT payload, so jwt.decode(stateToken) will not show vendorParams — reference generateState, stateToken, jwt.decode, and vendorParams in the comment to make the design explicit.apps/api/src/modules/connections/connectors.service.ts (1)
322-346:⚠️ Potential issue | 🔴 CriticalOnly compensate rows created by this request.
This rollback runs after an
onConflictDoUpdate/onConflictDoNothing, so on a reconnect path the returnedconnectionIdcan belong to an already-working connection. IfapplyPlan()fails for that existing namespace, the cleanup here deletes the liveappConnectionsrow and itsconnectionStorageRegistryentry, turning a transient provisioning error into credential loss.Track whether this request actually created the connection/storage records, and only delete those on failure. Even better, skip
applyPlan()entirely when the storage namespace already existed.Also applies to: 363-403
♻️ Duplicate comments (4)
apps/web/src/modules/connections/hooks/useConnections.ts (1)
127-140:⚠️ Potential issue | 🔴 CriticalRegex-based secret filtering is still not safe enough for popup URLs.
This only excludes params whose names match
/secret|password|token|key/i. ASECRET_TEXTfield namedcertificate,privatePem, etc. still lands in browser history/logs, while non-secret fields whose names includekeyget stripped. Pass an explicit URL-safe vendor-param bag from the form/schema instead of guessing here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modules/connections/hooks/useConnections.ts` around lines 127 - 140, The current popup URL builds by filtering vendorParams with a regex which can both leak SECRET_TEXT fields (e.g., certificate, privatePem) and incorrectly drop safe fields (e.g., fields containing "key"); update the code that constructs popupUrl (referencing popupUrl, vendorParams, providerName, clientId) to accept and use an explicit safe vendor-params bag provided by the form/schema (e.g., safeVendorParams) instead of guessing via regex, and remove the Object.entries(...filter(.../secret|password|token|key/i)) logic; keep SECRET_TEXT merging behavior in handleSuccess/pendingCredentials and ensure only the explicit safeVendorParams are JSON-encoded and appended to the URL.apps/web/src/modules/connections/components/DynamicAuthForm.tsx (1)
190-196:⚠️ Potential issue | 🟠 MajorPreserve checkbox/number vendor params instead of flattening them to strings.
This submit path still narrows all extra fields to
Record<string, string>, even though the form supportsCHECKBOXandNUMBER. That losesfalse/numeric semantics on round-trip and will repopulate generic fields incorrectly. Keep the original primitive values in the submitted contract.Also applies to: 262-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx` around lines 190 - 196, The onSubmit signature and submission code currently force vendorParams to Record<string, string>, which strips numeric and boolean primitives (CHECKBOX/NUMBER) and breaks repopulation; update the contract and submit handling to preserve original primitive types (e.g., change vendorParams to Record<string, string | number | boolean> or Record<string, unknown>) and ensure the form serialization logic in DynamicAuthForm (the onSubmit handler and any place that flattens form values) no longer coerces values to strings but returns the original boolean/number/string values so checkboxes and numeric fields round-trip correctly.apps/web/src/modules/connections/components/ConnectAppCard.tsx (1)
153-161:⚠️ Potential issue | 🟡 MinorFinish the
vendorParamsround-trip for checkbox and number fields.This coercion still mis-prefills some stored values:
'1'/'0'checkboxes do not round-trip correctly, andNumber('')turns an empty optional numeric field into0. Reopening a saved connection can therefore show a value the user never set.Suggested fix
- if (type === 'CHECKBOX') { - coercedVendorParams[key] = value === 'true' || value === true; - } else if (type === 'NUMBER') { - const num = Number(value); - coercedVendorParams[key] = !Number.isNaN(num) ? num : value; + if (type === 'CHECKBOX') { + coercedVendorParams[key] = + value === true || + value === 1 || + value === 'true' || + value === '1'; + } else if (type === 'NUMBER') { + if (value === '') { + coercedVendorParams[key] = value; + } else { + const num = typeof value === 'number' ? value : Number(value); + coercedVendorParams[key] = !Number.isNaN(num) ? num : value; + } } else { coercedVendorParams[key] = value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modules/connections/components/ConnectAppCard.tsx` around lines 153 - 161, The vendorParams coercion incorrectly turns '1'/'0' checkboxes into wrong values and converts empty-number strings into 0; update the loop in ConnectAppCard.tsx (the block that inspects provider.uiSchema, coercedVendorParams, defaultCreds.vendorParams, and key/type) so that for type === 'CHECKBOX' you treat '1' and '0' as true/false in addition to 'true'/'false' and boolean inputs (e.g., value === '1' || value === 1 => true, value === '0' || value === 0 => false), and for type === 'NUMBER' you preserve empty strings (if value === '' keep ''), otherwise attempt Number(value) and only assign the numeric value when it is not NaN; if conversion yields NaN leave the original value unchanged.apps/api/src/modules/connections/connections/connectors.controller.ts (1)
455-493:⚠️ Potential issue | 🟠 MajorValidate
vendorParamsagainst the piece schema before redirecting.The Zod parse only proves this is a small string map. Undeclared keys, missing required fields, and invalid enum/checkbox values are still first rejected later in
oauth-exchange, after they have already been signed into state and used to resolve the authorize URL. RunvalidateVendorParams(...)here so bothgenerateState()andgetAuthorizationUrl()only see provider-approved values.Suggested fix
let vendorParams: Record<string, string> = {}; if (vendorParamsJson) { try { const parsed: unknown = JSON.parse(vendorParamsJson); const parseResult = z .record(z.string(), z.string()) .refine((obj) => Object.keys(obj).length <= 15, { message: 'vendorParams cannot exceed 15 keys', }) .safeParse(parsed); if (!parseResult.success) { throw new BadRequestException( `Invalid vendorParams format: ${parseResult.error.issues[0]?.message}`, ); } vendorParams = parseResult.data; } catch (err) { if (err instanceof BadRequestException) { throw err; } throw new BadRequestException('Invalid vendorParams JSON format'); } } + + const piece = this.pieceRegistry.getPiece(providerName); + if (!piece) { + throw new NotFoundException(`Provider "${providerName}" is not registered`); + } + const authProps = + piece.auth && 'props' in piece.auth + ? (piece.auth.props as Record<string, AnyProperty>) + : undefined; + validateVendorParams(authProps, vendorParams); let authorizeUrl: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/connections/connectors.controller.ts` around lines 455 - 493, The vendorParams map is only type-checked as string->string but not validated against the provider schema, so call validateVendorParams(...) on the parsed vendorParams before using it; replace the vendorParams passed into oauthStateService.generateState(...) and connectorsService.getAuthorizationUrl(...) with the validated result, and if validateVendorParams returns/throws validation errors, convert them into a BadRequestException (mirroring the existing error handling) so only provider-approved values are signed into state and used to build the authorize URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/connections/connections/connectors.controller.ts`:
- Around line 163-165: The provider-name validation is inconsistent between
initiateOAuth() (allows [\w-]) and validateExchangeBody() (allows [a-z0-9-]);
unify them by extracting a single exported constant regex (e.g.,
VALID_PROVIDER_NAME_REGEX = /^[A-Za-z0-9_-]+$/) and use that constant in both
initiateOAuth and validateExchangeBody (and the other occurrence around lines
439-440) so the same validation rule is enforced everywhere.
- Around line 226-227: The two temporary stdout logs in the connectors
controller (the console.log calls that print '--- GET PROVIDERS CALLED ---' and
providers.map((p) => p.name)) should be removed or replaced with Nest's logger;
update the method handling GET /connectors/providers (e.g., the controller
method that builds the providers array) to either delete those console.log
statements or call this.logger.debug with the same messages/values (use
this.logger.debug('GET PROVIDERS', providers.map(p => p.name)) or similar) so
logs go through the controller's logger instead of stdout.
In `@apps/api/src/modules/connections/connections/token-refresh.service.spec.ts`:
- Around line 209-252: Add a regression test to token-refresh.service.spec.ts
that verifies the legacy decrypted field environment is mapped into vendorParams
for templated token URLs: create a fixture where mockEncryptionService.decrypt
resolves to a JSON string containing { environment: 'sandbox', clientId,
clientSecret, accessToken, refreshToken } (no explicit vendorParams), keep the
piece tokenUrl templated like 'https://{env}.mock.com/token', call
client.refresh(...) as in the existing test, and assert globalThis.fetch was
called with 'https://sandbox.mock.com/token'; this ensures the mapping logic in
the token refresh flow (the code in token-refresh.service that converts
decrypted.environment into vendorParams) continues to work.
In `@apps/api/src/modules/connections/connectors.service.spec.ts`:
- Around line 182-194: The test's title "should throw
InternalServerErrorException if authType is not OAUTH2" does not match the
assertion which expects BadRequestException for service.getAuthorizationUrl;
update the spec to be consistent by either renaming the it(...) description to
mention BadRequestException (e.g., "should throw BadRequestException if authType
is not OAUTH2") or change the expect(...).toThrow(...) to
InternalServerErrorException if that is the intended behavior of
getAuthorizationUrl; ensure the change references the test that calls
service.getAuthorizationUrl('mock-piece', 'state', 'test-client-id') and the
mockPieceRegistry.getPiece stub.
- Around line 242-259: The test description is inconsistent: update the it()
description for the test around exchangeCodeForTokens to state it "should throw
BadRequestException if authType is not OAUTH2" (matching the assertion that
rejects.toThrow(BadRequestException)); ensure the text change mirrors the same
fix you applied for getAuthorizationUrl so the message and the assertion refer
to the same exception type (BadRequestException) and the test remains targeted
at service.exchangeCodeForTokens.
In `@apps/api/src/modules/connections/oauth-state.service.ts`:
- Around line 67-74: generateState only writes Redis when vendorParams exist and
verifyState treats missing entries as valid, allowing replay; fix by always
persisting a nonce marker for every state token in generateState (even if
vendorParams is empty) using the same key `oauth:state:${stateId}`, and change
verifyState to atomically consume the marker instead of accepting cache
misses—use a single atomic fetch-and-delete (Redis GETDEL or a small EVAL
script) against the key to retrieve vendorParams and delete the state in one
operation, and fail verification if the atomic operation returns null; update
references to redis.set/redis.get/redis.del and the functions generateState and
verifyState accordingly.
In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx`:
- Around line 90-109: The dropdown options unwrapping misses the double-nested
shape where prop.options is an object whose options property itself contains an
options array; update the logic around rawOptions / NestedOptions to check for
and extract rawOptions.options.options (in addition to rawOptions.options and
rawOptions array) so optionsBody gets assigned the inner array, i.e. augment the
conditional that populates optionsBody (referencing prop, rawOptions,
NestedOptions, and optionsBody) to handle the extra nesting before rendering
SelectItem.
In `@packages/connectors/src/framework/auth.spec.ts`:
- Around line 24-28: Add a test that passes undefined for vendorParams to verify
resolveOAuth2Url handles it: update or add an it(...) case calling
resolveOAuth2Url(template, undefined) and expect the same URL result; if the
function currently types vendorParams as Record<string,string>, either adjust
the function signature to accept vendorParams?: Record<string,string> or add a
guard inside resolveOAuth2Url to treat undefined the same as an empty object
(e.g., defaulting vendorParams to {} before using it) so the new test passes.
- Around line 1-35: Add a new unit test in auth.spec.ts for resolveOAuth2Url
that passes a template with a placeholder value containing unsafe characters
(e.g., vendorParams = { tenant: 'bad@/?:#' } for template
'https://{tenant}.example.com') and assert that resolveOAuth2Url throws the
validation error from auth.ts rejecting characters '@/?#:'. Reference
resolveOAuth2Url in the test and assert both that an exception is thrown and
that the error message mentions the invalid characters or the specific
validation failure so the behavior in auth.ts (lines checking for `@/`?#:) is
covered.
In `@packages/connectors/src/framework/piece.ts`:
- Around line 13-14: The categories field and all related types were loosened to
any[] and stringified, which defeats the new PieceCategory enum; change all
occurrences of categories typing from any[] or string[] to PieceCategory[]
(e.g., the interface property, CreatePieceParams.categories, and any function
params/returns) and remove the String(...) coercion in the place that builds
categories (referenced near the String(...) call around Line 94). Instead
validate incoming values against the PieceCategory enum (either by filtering to
known enum members or throwing a clear error on invalid entries) so only genuine
PieceCategory values are stored/passed to the registry/UI.
In `@packages/connectors/src/index.ts`:
- Around line 4-15: The top-level entrypoint removed previously exported
OAuth/provider/Salesforce symbols, breaking existing imports; restore
backward-compatible deprecated re-exports in packages/connectors/src/index.ts
for the removed symbols (e.g., re-export the OAuth/provider and Salesforce
connector types/classes such as OAuthProvider, Provider, SalesforceConnector or
any previously exported names) by forwarding them to their new modules and add a
deprecation comment, or if you intend a breaking release, update package version
and changelog to mark this as breaking. Ensure you modify the exports in
index.ts (the central export file) to either re-export the old symbols or
document/version the change as breaking.
In `@packages/connectors/src/intelligence/index.ts`:
- Around line 1-6: The package removed exports from the
`@nexiom/connectors/intelligence` entrypoint and therefore broke consumers that
imported the Salesforce helper utilities from that module; restore backwards
compatibility by re-adding deprecated re-exports to
packages/connectors/src/intelligence/index.ts that forward the Salesforce helper
symbols (the Salesforce-specific helper modules that were previously reachable
from this entrypoint) for one release, or alternatively document and publish
this as a breaking change (major bump); ensure the index file explicitly
re-exports the Salesforce helper identifiers under the same names so existing
imports continue to compile, and add a deprecation comment indicating they will
be removed in the next major release.
In `@packages/connectors/src/intelligence/universal-trigger-engine.ts`:
- Around line 107-114: The timeout detection is brittle because it relies on
matching the error.message string; replace that by creating and throwing a
dedicated TimeoutError (or a uniquely-identified Timeout symbol-wrapped Error)
from the timeout promise used with Promise.race (the spot that currently does
reject(new Error('TIMEOUT'))), then detect it via instanceof TimeoutError (or by
checking the unique symbol/property) in the catch block where apiLimits,
checkApiLimits, and log.debug are referenced; update the reject call and the
catch branch (isTimeout detection) accordingly so timeout handling is robust.
In `@packages/pieces/quickbooks/src/lib/auth.ts`:
- Around line 11-27: The new auth property "environment" changes persisted
shape; preserve legacy "useSandbox" by reading it when "environment" is missing:
in the QuickBooks auth handling code (where Property.StaticDropdown
'environment' is defined and anywhere auth is parsed/used—e.g., functions that
build API base URL or polling helpers), treat props.environment as
props.useSandbox ? 'test' : 'login' when environment is undefined, and ensure
any save/update logic continues to accept and retain useSandbox until a
migration/backfill runs; alternatively add an explicit backfill path to populate
environment from useSandbox for stored records during startup or migration.
In `@packages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.ts`:
- Around line 26-30: The ORDER BY currently only sorts by spec.cursorField which
can cause unstable pagination when multiple rows share the same cursor; update
the query construction where query is built (use of selectClause,
spec.objectName and spec.cursorField) to include the tie-breaker field
(spec.tieBreakerField) in the ORDER BY (e.g. ORDER BY spec.cursorField ASC,
spec.tieBreakerField ASC) and ensure any logic that relies on calculateLimit or
buildWhereClause remains compatible with the dual-field ordering.
- Around line 74-81: The calculateLimit function currently silently truncates
fractional limits with Math.floor which hides bad input; update calculateLimit
to validate that the provided limit is an integer (no fractional part) and throw
an error for non-integer or negative values instead of flooring them.
Specifically, in calculateLimit, after parsing Number(limit) check
Number.isFinite(parsed) and parsed >= 0 and Number.isInteger(parsed); if parsed
is fractional throw an Error(`Invalid limit: ${limit} — must be an integer >=
0`), keep the special cases for undefined (return 200) and 0 (return 0), and
return parsed as an integer when valid.
In `@packages/pieces/salesforce/src/lib/sf-fetch.ts`:
- Around line 102-109: The error-handling block that sets errMsg (the local
variable in packages/pieces/salesforce/src/lib/sf-fetch.ts) should avoid
JSON.stringify on arbitrary thrown objects; instead, if err is an Error or has a
string 'message' property use that, otherwise set a sanitized placeholder like
'Non-error thrown object' or String(err) truncated, and remove the
JSON.stringify route to prevent leaking request/auth payloads; update the branch
that currently does "JSON.stringify(err)" to check for (err && typeof (err as
any).message === 'string') and use that, falling back to a safe static message.
In `@packages/pieces/salesforce/src/lib/trigger/salesforce-polling.helper.ts`:
- Around line 52-67: The current cursor (stored at cursorKey) only saves the
last dateField string and resumes with WHERE ${dateField} > ${since}, which can
skip records sharing the same timestamp; change to a compound cursor storing
both the last record's dateField and Id (e.g., serialize {sinceDate, sinceId}
into store.put(cursorKey,...)) and update the resume logic to parse that stored
cursor and build the SOQL WHERE clause as: (dateField > sinceDate) OR (dateField
= sinceDate AND Id > sinceId), and keep ORDER BY ${dateField} ASC, Id ASC and
LIMIT 200; set the new cursor from the batch's last record using last[dateField]
and last.Id before calling store.put(cursorKey,...). Ensure code locations: the
soql construction, parsing/serializing the cursor, and the place that uses
store.put(cursorKey, ...) are updated (refer to variables soql, dateField,
object, SF_API_VERSION, store.put, cursorKey, and the last record extraction).
- Around line 39-56: The function runSalesforce currently interpolates the
object parameter directly into the SOQL; call the existing validation helper
assertSafeSalesforceObject(object) early (e.g., after extracting opts and before
building columns/soql) to validate/sanitize the object name, so the object is
safe to use in the FROM ${object} clause; update runSalesforce to invoke
assertSafeSalesforceObject(object) prior to composing the soql URL.
In `@TECHNICAL_DEBT.md`:
- Around line 305-316: The TECHNICAL_DEBT.md entry incorrectly marks the
Frontend UI Unification item as completed; update the entry in TECHNICAL_DEBT.md
for the block referencing connectors.controller.ts and DynamicAuthForm.tsx to
reflect that this work is proposed in PR `#86` and still in progress — change
"Completed: 2026-03-09" to "Status: Proposed / In progress (see PR `#86`)",
replace definitive language like "Fully deleted `ProviderRegistryService`" and
"Removed the hardcoded `env` fields" with tentative phrasing such as "Proposed:
delete `ProviderRegistryService` and update
connectors.controller.ts/connectors.service.ts to use `PieceAuth` from
`PieceRegistryService`" and "Proposed: remove hardcoded `env` in
DynamicAuthForm.tsx and oauth-state.service.ts; render environment via
uiSchema/vendorParams", and remove any statements that assert the PR has already
merged so the debt item remains under active/in-progress until the PR lands.
---
Outside diff comments:
In `@apps/api/src/db/database-manager.spec.ts`:
- Around line 90-93: The mocked insert promise resolves to undefined; change it
to resolve to the builder object so awaiting values() returns the chain as
described by the test. Locate the mock where const promise =
Promise.resolve(undefined) as Promise<undefined> & typeof chain; and replace it
so the promise resolves the builder (e.g., Promise.resolve(chain) with the
appropriate type assertion), ensuring values() supports both await values() and
values().returning(...) by returning the same chain object when awaited.
In `@apps/api/src/modules/connections/oauth-state.service.spec.ts`:
- Around line 103-122: The test calls service.generateState(mockTenantId,
mockProvider, { realmId: 'test-123' }) but asserts decoded.vendorParams is
undefined; add a short clarifying comment above the assertion explaining that
generateState persists vendorParams (e.g., realmId) to Redis (or another store)
rather than embedding them in the JWT payload, so jwt.decode(stateToken) will
not show vendorParams — reference generateState, stateToken, jwt.decode, and
vendorParams in the comment to make the design explicit.
In `@packages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.ts`:
- Around line 37-42: Add a warning log before the fallback `return []` so
unexpected checkpoint states are observable: in the method that checks
`checkpoint.state` (which currently calls `checkJobStatusAndDownload` for
'IN_PROGRESS' or 'AWAITING_RESULTS'), log a warning (e.g., `this.logger.warn` or
the class logger) that includes the unexpected `checkpoint.state` value and
identifying info such as `storeKey` and any `checkpoint.jobId`/job identifier,
then return the empty array as before.
- Around line 127-129: The checkpoint method currently hardcodes the key
'igt_bulk_job_checkpoint'; change the signature of private async
checkpoint(store: TriggerStore, data: BulkJobCheckpoint) to accept storeKey:
string (e.g., checkpoint(store: TriggerStore, storeKey: string, data:
BulkJobCheckpoint)) and replace the hardcoded key in store.put with the passed
storeKey; similarly update downloadResults to accept storeKey and use it instead
of the literal; then update all callers that invoke checkpoint and
downloadResults (the places that currently call checkpoint(...) and
downloadResults(...), including the two call sites noted in the diff) to pass
the same storeKey they use elsewhere so the key usage is consistent across
TriggerStore interactions.
---
Duplicate comments:
In `@apps/api/src/modules/connections/connections/connectors.controller.ts`:
- Around line 455-493: The vendorParams map is only type-checked as
string->string but not validated against the provider schema, so call
validateVendorParams(...) on the parsed vendorParams before using it; replace
the vendorParams passed into oauthStateService.generateState(...) and
connectorsService.getAuthorizationUrl(...) with the validated result, and if
validateVendorParams returns/throws validation errors, convert them into a
BadRequestException (mirroring the existing error handling) so only
provider-approved values are signed into state and used to build the authorize
URL.
In `@apps/web/src/modules/connections/components/ConnectAppCard.tsx`:
- Around line 153-161: The vendorParams coercion incorrectly turns '1'/'0'
checkboxes into wrong values and converts empty-number strings into 0; update
the loop in ConnectAppCard.tsx (the block that inspects provider.uiSchema,
coercedVendorParams, defaultCreds.vendorParams, and key/type) so that for type
=== 'CHECKBOX' you treat '1' and '0' as true/false in addition to 'true'/'false'
and boolean inputs (e.g., value === '1' || value === 1 => true, value === '0' ||
value === 0 => false), and for type === 'NUMBER' you preserve empty strings (if
value === '' keep ''), otherwise attempt Number(value) and only assign the
numeric value when it is not NaN; if conversion yields NaN leave the original
value unchanged.
In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx`:
- Around line 190-196: The onSubmit signature and submission code currently
force vendorParams to Record<string, string>, which strips numeric and boolean
primitives (CHECKBOX/NUMBER) and breaks repopulation; update the contract and
submit handling to preserve original primitive types (e.g., change vendorParams
to Record<string, string | number | boolean> or Record<string, unknown>) and
ensure the form serialization logic in DynamicAuthForm (the onSubmit handler and
any place that flattens form values) no longer coerces values to strings but
returns the original boolean/number/string values so checkboxes and numeric
fields round-trip correctly.
In `@apps/web/src/modules/connections/hooks/useConnections.ts`:
- Around line 127-140: The current popup URL builds by filtering vendorParams
with a regex which can both leak SECRET_TEXT fields (e.g., certificate,
privatePem) and incorrectly drop safe fields (e.g., fields containing "key");
update the code that constructs popupUrl (referencing popupUrl, vendorParams,
providerName, clientId) to accept and use an explicit safe vendor-params bag
provided by the form/schema (e.g., safeVendorParams) instead of guessing via
regex, and remove the Object.entries(...filter(.../secret|password|token|key/i))
logic; keep SECRET_TEXT merging behavior in handleSuccess/pendingCredentials and
ensure only the explicit safeVendorParams are JSON-encoded and appended to the
URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f535ca92-d4a9-46d5-92f7-eeff12810e3b
📒 Files selected for processing (52)
TECHNICAL_DEBT.mdapps/api/src/db/database-manager.spec.tsapps/api/src/modules/connections/connections.module.tsapps/api/src/modules/connections/connections/callback.controller.spec.tsapps/api/src/modules/connections/connections/callback.controller.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/connections/connections/token-refresh.service.spec.tsapps/api/src/modules/connections/connections/token-refresh.service.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/connections/oauth-state.service.spec.tsapps/api/src/modules/connections/oauth-state.service.tsapps/api/src/modules/trigger/piece-registry.service.spec.tsapps/web/src/modules/connections/api/connections.api.tsapps/web/src/modules/connections/components/ConnectAppCard.tsxapps/web/src/modules/connections/components/DynamicAuthForm.tsxapps/web/src/modules/connections/hooks/useConnections.tsapps/web/src/modules/connections/pages/ConnectionsPage.tsxpackages/connectors/src/apps/salesforce/index.tspackages/connectors/src/apps/salesforce/triggers/new-record.tspackages/connectors/src/apps/salesforce/triggers/salesforce-polling.helper.tspackages/connectors/src/apps/salesforce/triggers/updated-record.tspackages/connectors/src/framework/auth.spec.tspackages/connectors/src/framework/auth.tspackages/connectors/src/framework/piece.tspackages/connectors/src/index.tspackages/connectors/src/intelligence/index.tspackages/connectors/src/intelligence/interfaces.tspackages/connectors/src/intelligence/universal-trigger-engine.spec.tspackages/connectors/src/intelligence/universal-trigger-engine.tspackages/connectors/src/oauth/provider-registry.tspackages/connectors/src/oauth/providers/index.tspackages/connectors/src/oauth/providers/quickbooks.tspackages/connectors/src/oauth/providers/salesforce.tspackages/connectors/src/oauth/types.tspackages/pieces/quickbooks/src/index.tspackages/pieces/quickbooks/src/lib/auth.tspackages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.tspackages/pieces/salesforce/package.jsonpackages/pieces/salesforce/src/index.tspackages/pieces/salesforce/src/lib/auth.tspackages/pieces/salesforce/src/lib/common/index.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.spec.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-discovery.adapter.spec.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-discovery.adapter.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.tspackages/pieces/salesforce/src/lib/sf-fetch.tspackages/pieces/salesforce/src/lib/trigger/salesforce-polling.helper.tspackages/pieces/salesforce/src/lib/trigger/universal-trigger.tspackages/pieces/salesforce/tsconfig.lib.json
💤 Files with no reviewable changes (10)
- packages/connectors/src/apps/salesforce/index.ts
- packages/connectors/src/oauth/providers/salesforce.ts
- packages/connectors/src/oauth/providers/quickbooks.ts
- packages/connectors/src/apps/salesforce/triggers/new-record.ts
- packages/connectors/src/oauth/types.ts
- packages/connectors/src/apps/salesforce/triggers/updated-record.ts
- packages/connectors/src/apps/salesforce/triggers/salesforce-polling.helper.ts
- packages/connectors/src/oauth/providers/index.ts
- apps/api/src/modules/connections/connections.module.ts
- packages/connectors/src/oauth/provider-registry.ts
Comment on lines
163
to
165
| if (!/^[a-z0-9-]+$/u.test(safeProviderName)) { | ||
| throw new BadRequestException('Invalid provider name format'); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep provider-name validation consistent across both OAuth steps.
initiateOAuth() accepts [\w-], but validateExchangeBody() only allows [a-z0-9-]. A registered piece name with _ or uppercase characters can start the flow and then fail during oauth-exchange. Please share one validator/regex so the contract cannot drift.
Also applies to: 439-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/connections/connections/connectors.controller.ts` around
lines 163 - 165, The provider-name validation is inconsistent between
initiateOAuth() (allows [\w-]) and validateExchangeBody() (allows [a-z0-9-]);
unify them by extracting a single exported constant regex (e.g.,
VALID_PROVIDER_NAME_REGEX = /^[A-Za-z0-9_-]+$/) and use that constant in both
initiateOAuth and validateExchangeBody (and the other occurrence around lines
439-440) so the same validation rule is enforced everywhere.
pramodnarayana
marked this pull request as draft
March 9, 2026 08:24
…rity, and lint fixes
- connectors.controller.ts: validate vendorParams against provider auth.props schema
before signing OAuth state; add getProviderDefinition() to ConnectorsService
- ConnectAppCard.tsx: fix coercion for CHECKBOX ('1'/'0') and NUMBER (preserve empty strings)
- DynamicAuthForm.tsx: preserve boolean/number primitive types in buildVendorParams
- useConnections.ts: replace regex filter with explicit safeVendorParams bag
- salesforce-polling.helper.ts: extract parseCursor/buildWhereClause/updateCursor helpers
to reduce cognitive complexity from 16 to ~5
- connectors.controller.spec.ts: stub getProviderDefinition on mock service
- token-refresh.service.spec.ts: fix legacy env template key from {env} to {environment}
- connectors/src/index.ts: suppress no-extraneous-class lint on SalesforceConnector stub
- DynamicAuthForm.tsx: replace any casts with typed NestedNestedOptions interface
pramodnarayana
marked this pull request as ready for review
March 9, 2026 09:34
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.ts (1)
6-12:⚠️ Potential issue | 🟠 MajorConstrain
environmentto the supported literals ('login' | 'test').The
QuickBooksAuth.props.environmentis typed asstringbut the auth schema enforces only two values viaStaticDropdown:'login'and'test'. This type mismatch allows invalid values to compile. Additionally, the same environment resolution logic (env ?? useSandbox ? 'test' : 'login') is duplicated in bothpackages/pieces/quickbooks/src/index.tsandpackages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.ts, creating drift risk. Use a literal union type here and extract the shared resolution logic to a single function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.ts` around lines 6 - 12, Change QuickBooksAuth.props.environment from a plain string to a literal union 'login' | 'test' to match the StaticDropdown values and prevent invalid compile-time values; update the QuickBooksAuth interface in quickbooks-polling.helper.ts accordingly. Remove the duplicated resolution expression (env ?? useSandbox ? 'test' : 'login') from quickbooks-polling.helper.ts and packages/pieces/quickbooks/src/index.ts and replace both with a single shared helper (e.g., resolveQuickBooksEnvironment or getQuickBooksEnv) exported from a new or existing common module so both files call that function to compute the final environment. Ensure callers import and use that helper and update any local references to the old expression to use the helper's return type (the 'login' | 'test' union).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/db/database-manager.spec.ts`:
- Around line 89-93: The test currently resolves the mocked values() to the
fluent builder via "const promise = Promise.resolve(chain) as Promise<typeof
chain> & typeof chain;", which incorrectly allows "const q = await values();
q.returning(...)" — change the mock so values() still returns the fluent "chain"
object synchronously for chaining, but the awaited promise resolves to a plain
execution result (e.g., an object shaped like the real insert result) instead of
the builder. Concretely: keep the "chain" variable as the builder with
.values/.returning/.onConflict... methods, but make the promise be
Promise.resolve(executionResult) (and update the type assertion to the execution
result type rather than typeof chain) so await values() yields a non-builder
result while chaining before await still works.
In `@apps/api/src/modules/connections/connections/connectors.controller.ts`:
- Around line 566-572: The current check in connectors.controller.ts uses
BadRequestException when a provider isn't found: replace the thrown
BadRequestException with NotFoundException so missing registry entries return
404; update the block around pieceRegistry.getPiece(restOfBody.providerName) to
throw new NotFoundException(`Provider "${restOfBody.providerName}" is not
registered`) instead of BadRequestException and ensure any imports reference
NotFoundException from `@nestjs/common` if not already present.
- Line 555: Remove the unnecessary alias restOfBody and replace its usage with
body: in the method containing the line "const restOfBody = body;" delete that
assignment, then update all subsequent references to restOfBody in the same
function (connector-related logic in connectors.controller.ts) to use body
directly to eliminate the indirection.
In `@apps/api/src/modules/connections/connections/token-refresh.service.spec.ts`:
- Around line 239-244: The test uses client.refresh(...) with piece name
'mock-templated-oauth' but mockPieceRegistry.getPiece is set up with
TEMPLATED_PIECE derived from MOCK_OAUTH2_PIECE whose name is 'mock-oauth2', so
update the test to use the correct piece name or make the mock validate the
requested name; specifically, change the piece name arguments in the failing
tests that call client.refresh(...) (and any other occurrences at the same
tests) from 'mock-templated-oauth' to 'mock-oauth2' (or rename TEMPLATED_PIECE
to 'mock-templated-oauth') so the call to mockPieceRegistry.getPiece and the
TEMPLATED_PIECE identity align with the piece name used in the test.
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 386-411: When applyPlan (this.dbManager.applyPlan) fails, the
current rollback performs two separate deletes (this.db.delete on
connectionStorageRegistry and appConnections) which can leave partial cleanup if
one delete succeeds and the other fails; change the rollback to execute both
deletes inside a single atomic transaction (use the project's DB transaction API
around the two deletes so they either both commit or both roll back), still log
the applyError via this.logger.error and then throw the same
InternalServerErrorException if the transaction fails.
In `@packages/connectors/src/intelligence/index.ts`:
- Around line 7-13: The current empty stubs (SalesforcePollingHelper,
SalesforceQueryAdapter, SalesforceSFUtils) compile but will fail at runtime;
replace them with true re-exports from the new package by exporting the real
symbols from '@nexiom/piece-salesforce' (e.g., export { SalesforcePollingHelper,
SalesforceQueryAdapter, SalesforceSFUtils } from '@nexiom/piece-salesforce') so
runtime calls work, and ensure the package dependency on
`@nexiom/piece-salesforce` is declared (or, if you intend to keep breaking
behavior, change the package major version and add a clear migration note
instead).
In `@packages/connectors/src/intelligence/universal-trigger-engine.ts`:
- Around line 29-32: The code reads SF_API_LIMIT_THRESHOLD and logs
Salesforce-specific low-limit warnings while verifyApiLimitsSafe/checkApiLimits
are provider-agnostic; update the config so the threshold and provider label are
moved into UniversalEngineConfig (or a generic connector-level setting) and stop
referencing SF_API_LIMIT_THRESHOLD directly in parseLimitThreshold; modify
parseLimitThreshold usage and the caller in UniversalTriggerEngine (where
lowLimitThreshold is computed before calling verifyApiLimitsSafe) to read the
threshold from the new UniversalEngineConfig property (and pass a
provider-agnostic label or connector name alongside objectName into
verifyApiLimitsSafe) so QuickBooks/other connectors no longer inherit
Salesforce-branded config or alerts.
- Around line 114-117: The Promise.race call that sets apiLimits leaves the
setTimeout active when checkApiLimits resolves first; update the apiLimits
assignment to store the timeout id before racing and ensure the timeout is
cleared when checkApiLimits settles (e.g., create let timeoutId =
setTimeout(...), build the timeout Promise using that id, then after awaiting
Promise.race or by attaching .finally(() => clearTimeout(timeoutId)) to the
checkApiLimits Promise) so the timer handle is cleared promptly; specifically
modify the code around the apiLimits = await Promise.race([...]) line to capture
and clear the timeout for the TimeoutError path.
In `@packages/pieces/quickbooks/src/index.ts`:
- Around line 19-21: Validate and normalize the auth.props['environment'] value
before using it: read the raw value, trim and reject empty strings, then only
accept explicit values (e.g., 'test' for sandbox or 'login'/'production' for
live); derive useSandbox from that validated env (env === 'test') and only fall
back to auth.props['useSandbox'] when env is absent/invalid, then pass the
resulting useSandbox and companyId into quickbooksCommon.getApiUrl — update the
logic around env, useSandbox, and the call to quickbooksCommon.getApiUrl
accordingly.
In `@packages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.ts`:
- Around line 87-89: Extract the environment normalization into quickbooksCommon
by adding a helper (e.g., resolveEnvironment or normalizeEnvironment) that
accepts the auth.props structure and returns 'test' or 'login' (or a boolean
indicating sandbox) and then replace the inline fallback logic in
executeQuickBooksFetch with a call to that helper before calling
quickbooksCommon.getApiUrl; also update the duplicate logic in the other file
(the code in packages/pieces/quickbooks/src/index.ts) to use the same helper so
both polling (executeQuickBooksFetch) and custom API calls share the single
source of truth.
In `@packages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.ts`:
- Around line 6-7: The SalesforceAuth type is imported from a trigger-specific
module (salesforce-polling.helper) which couples the intelligence layer to
trigger internals; extract the SalesforceAuth shape into a neutral shared module
(e.g., salesforce-types or salesforce-common) and export it there, then update
the import in salesforce-bulk.adapter.ts to import SalesforceAuth from that new
neutral module (and update salesforce-polling.helper.ts to import the same type
from the neutral module or re-export it if necessary), ensuring no
trigger-specific code is referenced by the intelligence layer.
- Around line 41-46: The code currently logs and returns [] when encountering an
unexpected checkpoint.state, leaving the bad checkpoint in TriggerStore so the
poll never recovers; change this branch to reset/remove the stored checkpoint
for the affected storeKey (e.g., call the TriggerStore delete/remove method or
overwrite the entry with the initial/default checkpoint state) so future polls
can re-initialize normal processing, log that you reset the checkpoint
(including storeKey and jobId), and ensure the delete/put call is awaited and
errors are handled/logged rather than silently ignored.
- Line 243: The header quote normalization regex in the headers assignment is
wrong: in the expression that maps rows[0] (the headers) and calls replaceAll,
replace the current regex used in the replaceAll call so it matches a leading OR
trailing quote (not a single anchored group). Update the regex used in the
headers mapping (the replaceAll on h inside the rows[0].map) from the faulty
pattern to one that matches either ^" or "$ (e.g., use a pattern that alternates
the two anchors) so quoted header values like "Id" become Id after trim.
In `@packages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.ts`:
- Around line 61-69: The current tie-breaker handling in the block that checks
spec.tieBreakerField silently proceeds when spec.tieBreakerValue is missing;
update the logic in salesforce-query.adapter.ts (the block referencing
spec.tieBreakerField, spec.tieBreakerValue, tbFieldDef, validateCursor and
tbFormatted) to explicitly detect when tieBreakerField is set but
tieBreakerValue is falsy and surface it—either by throwing a clear Error or by
emitting a warning via the existing logger—so callers are alerted; if throwing,
include the objectName and tieBreakerField in the message, and if warning,
include the same context and continue using cursor-only pagination.
In `@packages/pieces/salesforce/src/lib/sf-fetch.ts`:
- Around line 11-18: Move the type import so it appears at the top of the module
before any exports: relocate "import type { TriggerStore }" above the exported
SF_API_VERSION and checkSalesforceLimits declaration to follow conventional
module ordering; update any references to TriggerStore in the
checkSalesforceLimits signature accordingly to avoid linter warnings.
In `@packages/pieces/salesforce/src/lib/trigger/salesforce-polling.helper.ts`:
- Around line 40-47: parseCursor currently falls back to returning the raw
string when JSON.parse succeeds but lacks the expected keys, which can inject
untrusted JSON into SOQL; change parseCursor so that if parsed JSON does not
contain both sinceDate and sinceId it does NOT reuse raw but instead returns {
sinceDate: fallbackDate, sinceId: '' } (same behavior as when JSON.parse
throws). Update the function parseCursor to only return parsed.sinceDate/sinceId
when both are strings and otherwise treat the value as a reset to fallbackDate.
- Around line 94-95: The WHERE clause currently uses formattedSince wrapped in
single quotes which produces an invalid SOQL comparison; change the construction
so formattedSince is an unquoted ISO8601 DateTime literal (e.g., use sinceDate
or its ISO string without surrounding quotes) before calling
buildWhereClause(dateField, formattedSince, sinceId) so the final SOQL reads
like CreatedDate > 2005-10-08T01:02:03Z; ensure the value passed from
formattedSince/sinceDate is not quoted and remains compliant with SOQL DateTime
literal formatting.
---
Outside diff comments:
In `@packages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.ts`:
- Around line 6-12: Change QuickBooksAuth.props.environment from a plain string
to a literal union 'login' | 'test' to match the StaticDropdown values and
prevent invalid compile-time values; update the QuickBooksAuth interface in
quickbooks-polling.helper.ts accordingly. Remove the duplicated resolution
expression (env ?? useSandbox ? 'test' : 'login') from
quickbooks-polling.helper.ts and packages/pieces/quickbooks/src/index.ts and
replace both with a single shared helper (e.g., resolveQuickBooksEnvironment or
getQuickBooksEnv) exported from a new or existing common module so both files
call that function to compute the final environment. Ensure callers import and
use that helper and update any local references to the old expression to use the
helper's return type (the 'login' | 'test' union).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e400de80-791b-4cf4-b843-c2306f376043
📒 Files selected for processing (24)
TECHNICAL_DEBT.mdapps/api/src/db/database-manager.spec.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/connections/connections/token-refresh.service.spec.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/connections/oauth-state.service.spec.tsapps/api/src/modules/connections/oauth-state.service.tsapps/web/src/modules/connections/components/ConnectAppCard.tsxapps/web/src/modules/connections/components/DynamicAuthForm.tsxapps/web/src/modules/connections/hooks/useConnections.tspackages/connectors/src/framework/auth.spec.tspackages/connectors/src/framework/auth.tspackages/connectors/src/framework/piece.tspackages/connectors/src/index.tspackages/connectors/src/intelligence/index.tspackages/connectors/src/intelligence/universal-trigger-engine.tspackages/pieces/quickbooks/src/index.tspackages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.tspackages/pieces/salesforce/src/lib/sf-fetch.tspackages/pieces/salesforce/src/lib/trigger/salesforce-polling.helper.ts
Contributor
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 (1)
apps/api/src/modules/connections/connectors.service.ts (1)
371-419:⚠️ Potential issue | 🔴 CriticalRollback now deletes pre-existing connections on reconnect failures.
Because the registry insert is
onConflictDoNothing(), reconnecting an existing connection gives you no signal that the namespace already existed. IfapplyPlan()then fails, Lines 398-410 still delete bothconnectionStorageRegistryandappConnections, which removes a previously working connection on a transient provisioning error. Only provision and roll back rows created in this call.🛠️ One safe direction
- await tx + const insertedRegistry = await tx .insert(connectionStorageRegistry) .values({ connectionId: connection.id, workspaceId: schemaName, databaseHostId: 'primary-cluster', regionContext: finalRegionContext, }) .onConflictDoNothing({ target: connectionStorageRegistry.connectionId, - }); + }) + .returning({ connectionId: connectionStorageRegistry.connectionId }); - return { schemaName, connectionId: connection.id }; + return { + schemaName, + connectionId: connection.id, + createdRegistry: insertedRegistry.length > 0, + }; }); - try { - await this.dbManager.applyPlan( - workspaceSchemaName.schemaName, - SchemaPlan.NAMESPACE_ONLY, - ); - } catch (applyError) { + if (workspaceSchemaName.createdRegistry) { + try { + await this.dbManager.applyPlan( + workspaceSchemaName.schemaName, + SchemaPlan.NAMESPACE_ONLY, + ); + } catch (applyError) { + // rollback only for newly created rows + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/connectors.service.ts` around lines 371 - 419, The rollback block currently unconditionally deletes rows from connectionStorageRegistry and appConnections, which can remove pre-existing connections when insert used onConflictDoNothing(); fix by tracking which rows this call actually created and only delete those: detect/record whether the registry insert and appConnections insert created new records (e.g., use the insert/returning result or pre-check existence of connectionStorageRegistry via connection.id) inside the initial db.transaction that inserts connection/appConnections (refer to connectionStorageRegistry, appConnections, connection.id and the transaction scope around applyPlan), persist flags or the created record ids (e.g., createdRegistry=true/false, createdAppConnection=true/false), and in the catch that handles applyPlan failure only delete the rows for which the corresponding created flag is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/connections/connections/connectors.controller.ts`:
- Around line 393-402: When hydrating reconnect credentials in
connectors.controller.ts (the parsed variable and vendorParams assignment),
preserve legacy top-level parsed.environment by merging it into vendorParams: if
parsed.vendorParams is missing or doesn't contain an environment key but
parsed.environment exists, create/augment vendorParams with environment:
parsed.environment (so vendorParams becomes parsed.vendorParams || {
environment: parsed.environment } and if parsed.vendorParams exists without
environment, set vendorParams.environment = parsed.environment). This aligns
with token-refresh.service.ts mapping and prevents losing prefilled vendor
params for existing Salesforce/QuickBooks connections.
In `@apps/api/src/modules/connections/connections/token-refresh.service.spec.ts`:
- Around line 82-100: The test assertions still expect error messages using
"Provider" terminology; update them to match piece-based naming if the
implementation uses "Piece". In token-refresh.service.spec.ts change the
expected error strings for client.refresh calls (the case when
mockPieceRegistry.getPiece returns undefined and when a piece has an empty
tokenUrl) to use "Piece" (e.g., "Piece not found for refresh: unknown_app" and
"Piece mock-oauth2 does not support OAuth refresh or lacks a token url") so the
test messages align with the Piece API and the MOCK_OAUTH2_PIECE references.
In `@packages/connectors/src/intelligence/universal-trigger-engine.ts`:
- Around line 108-109: The structured log/warning currently emits the connector
label under the key objectName; update the emission to use connectorName instead
so the payload is accurate: locate the places in universal-trigger-engine.ts
where objectName is included in the log payload (e.g., the structured warning
calls near the function signature that includes objectName and checkApiLimits
and the similar emissions around the 132-137 region) and replace the payload
key/value to use connectorName (or rename the parameter to connectorName
consistently) so logs reflect the connector label instead of objectName.
- Around line 29-30: The code reads config.apiLimitThreshold directly into
lowLimitThreshold which bypasses the clamping done in parseLimitThreshold(), so
normalize any provided config.apiLimitThreshold before use: update
parseLimitThreshold to accept an optional input (or create a small
normalizeApiLimitThreshold helper) and call it with config.apiLimitThreshold
when computing lowLimitThreshold (i.e. replace the direct use of
config.apiLimitThreshold with the parsed/clamped result), ensuring values like
2, -1, NaN are coerced/clamped to the valid threshold range used elsewhere.
In
`@packages/pieces/salesforce/src/lib/intelligence/salesforce-discovery.adapter.spec.ts`:
- Around line 20-24: The mock SalesforeAuthError in the test file is just a bare
Error subclass; update the mock in the vi.mock block (where sfFetch,
SF_API_VERSION and SalesforceAuthError are defined) so the mocked
SalesforceAuthError sets this.name = 'SalesforceAuthError' in its constructor to
match the real sf-fetch.ts implementation, ensuring future tests that assert
error.name behave correctly.
In `@packages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.ts`:
- Around line 29-30: The ORDER BY clause builder appends spec.tieBreakerField
without validating it unless spec.tieBreakerValue is truthy, so validate the
field unconditionally before injecting it; locate the code that constructs the
query (the block that checks spec.tieBreakerValue and the conditional that
appends `, ${spec.tieBreakerField} ASC`) and move or duplicate the
schema/field-name validation so it runs whenever spec.tieBreakerField is present
(including the warning path where tieBreakerValue is absent), and only append
spec.tieBreakerField to the ORDER BY when that validation succeeds (same fix
should be applied to the similar logic in the block covering lines 61-77).
- Around line 26-33: The SELECT must always include the tie-breaker column when
tie-breaker paging is enabled: update the logic that builds selectClause (used
where query is constructed) so that if spec.tieBreakerField is set and not
already present in requestedFields/selectClause you append it to the selected
columns before composing query (keep deduplication to avoid duplicates); ensure
this change happens near where selectClause is created/used (refer to
selectClause, spec.tieBreakerField, requestedFields, buildWhereClause and the
code that assigns query and calls calculateLimit) so subsequent pages can supply
spec.tieBreakerValue.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 371-419: The rollback block currently unconditionally deletes rows
from connectionStorageRegistry and appConnections, which can remove pre-existing
connections when insert used onConflictDoNothing(); fix by tracking which rows
this call actually created and only delete those: detect/record whether the
registry insert and appConnections insert created new records (e.g., use the
insert/returning result or pre-check existence of connectionStorageRegistry via
connection.id) inside the initial db.transaction that inserts
connection/appConnections (refer to connectionStorageRegistry, appConnections,
connection.id and the transaction scope around applyPlan), persist flags or the
created record ids (e.g., createdRegistry=true/false,
createdAppConnection=true/false), and in the catch that handles applyPlan
failure only delete the rows for which the corresponding created flag is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04f41241-592d-4c95-b097-48e3613c3536
📒 Files selected for processing (20)
apps/api/src/db/database-manager.spec.tsapps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/connections/connections/token-refresh.service.spec.tsapps/api/src/modules/connections/connectors.service.tspackages/connectors/src/intelligence/index.tspackages/connectors/src/intelligence/interfaces.tspackages/connectors/src/intelligence/universal-trigger-engine.tspackages/pieces/quickbooks/src/index.tspackages/pieces/quickbooks/src/lib/common.tspackages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.tspackages/pieces/salesforce/package.jsonpackages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.spec.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-discovery.adapter.spec.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-discovery.adapter.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.tspackages/pieces/salesforce/src/lib/salesforce-types.tspackages/pieces/salesforce/src/lib/sf-fetch.tspackages/pieces/salesforce/src/lib/trigger/index.tspackages/pieces/salesforce/src/lib/trigger/salesforce-polling.helper.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/modules/connections/connectors.service.ts (1)
196-206: 🧹 Nitpick | 🔵 TrivialToken exchange lacks timeout handling for slow vendors.
The
fetchcall usesAbortSignal.timeout(10000)which is good, but the error from a timeout will be a genericDOMException. Consider catching and wrapping timeout errors specifically to provide clearer diagnostics.♻️ Explicit timeout error handling
try { this.logger.log(`Exchanging OAuth code for ${providerName}...`); const response = await fetch(tokenUrl, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, body: new URLSearchParams({ grant_type: 'authorization_code', code, redirect_uri: redirectUri, client_id: clientId, client_secret: clientSecret, }).toString(), signal: AbortSignal.timeout(10000), }); // ... } catch (error) { + if (error instanceof DOMException && error.name === 'TimeoutError') { + throw new InternalServerErrorException( + `Token exchange timed out for ${providerName}`, + ); + } this.handleExchangeException(providerName, error); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/connectors.service.ts` around lines 196 - 206, The fetch-based token exchange using AbortSignal.timeout(10000) should catch timeout-specific failures and rethrow a clearer error; wrap the existing fetch(tokenUrl, ...) call in a try/catch, detect AbortError/timeout (e.g., error.name === 'AbortError' or DOMException with the abort name) and throw a new, descriptive error like "Token exchange timed out after 10s" (or attach contextual info such as tokenUrl, clientId, code) so callers of the token exchange receive explicit timeout diagnostics instead of a generic DOMException; leave non-timeout errors to propagate or rewrap with their original details.packages/pieces/salesforce/src/lib/sf-fetch.ts (1)
98-120:⚠️ Potential issue | 🟠 MajorPreserve abort signal semantics by immediately re-throwing caller cancellations.
When a caller cancels via
init.signal.abort(), theonCallerAborthandler triggerscontroller.abort(), causing fetch to throw an AbortError. This error is caught at line 100 and marked asisTransientError = true, which causes it to enter the retry loop. After exhausting retries, the code callsbuildSalesforceError(undefined)and returns a generic error with status 0, losing the original abort semantics.The proposed fix is correct: check
init.signal?.abortedbefore marking as transient and immediately re-throw to preserve the abort behavior.🔧 Minimal fix
try { response = await executeFetchWithTimeout(url, init); } catch (err: unknown) { + if (init.signal?.aborted) { + throw err; + } const errMsg = parseNetworkErrorMsg(err); console.debug(`[sfFetch] Transient network error encountered: ${errMsg}`); isTransientError = true; }If you want cancellation to stop immediately during backoff as well, make the retry wait abort-aware too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pieces/salesforce/src/lib/sf-fetch.ts` around lines 98 - 120, In the catch block around executeFetchWithTimeout in sf-fetch.ts, immediately re-throw if the caller aborted by checking init.signal?.aborted (preserving the original AbortError) instead of treating it as a transient error; only run parseNetworkErrorMsg and set isTransientError = true for non-abort errors so that isRetriableError and the retry loop aren’t entered for caller cancellations (optionally make the backoff sleep abort-aware by wiring init.signal into the retry wait).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/connections/connections/connectors.controller.ts`:
- Around line 53-64: The STATIC_DROPDOWN branch currently casts prop to a nested
shape and trusts prop.options.options exists; replace the unsafe type assertion
with a defensive type guard: first check that prop is an object, that 'options'
in prop and prop.options is an object, and that
Array.isArray(prop.options.options) before using it; then assign opts =
prop.options.options and build the allowed Set and validate val as before
(references: prop, propWithOptions, opts, STATIC_DROPDOWN). If the nested shape
is invalid, skip validation or throw a clear BadRequestException indicating
malformed parameter options.
- Around line 656-662: Remove the unused vendorParams field from the incoming
request body type so only decodedState.vendorParams is the source of truth;
update the request body interface/type (the one supplying
body.clientId/body.clientSecret) to delete vendorParams, and ensure
ConnectionValueBlob construction continues to use decodedState.vendorParams and
not body.vendorParams (symbols: ConnectionValueBlob, decodedState, vendorParams,
body).
In `@apps/api/src/modules/connections/connections/token-refresh.service.ts`:
- Around line 169-178: The code uses a double cast through unknown to read a
legacy environment field when building vendorParams (see vendorParams and
valueBlob in token-refresh.service.ts); replace the cast chain by declaring a
small local type or interface (e.g., LegacyValueBlob { environment?: unknown })
and narrow valueBlob to that type (or use a type guard like 'in' to check for
"environment") before reading and String()-coercing it, then spread the optional
environment property into vendorParams—this removes the (valueBlob as unknown as
Record<string, unknown>) casts while preserving the existing behavior.
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 425-454: The rollback currently logs only providerName and the
rollback error; update the rollback logging to include the specific connection
identifier and context by adding workspaceProvisionInfo.connectionId (and if
helpful workspaceProvisionInfo.createdRegistry/createdAppConnection) to the
this.logger.error call inside the catch for the transaction so the log shows
which connectionId was being cleaned (references: workspaceProvisionInfo,
connectionId, createdRegistry, createdAppConnection, connectionStorageRegistry,
appConnections, providerName, this.logger.error); keep the transaction and catch
as-is but augment the error payload/message to include the connection id and
flags for easier debugging.
In `@packages/connectors/src/intelligence/universal-trigger-engine.ts`:
- Around line 29-33: The current fallback connectorLabel = config.connectorName
?? objectName is misleading for throttling/alerting; update the logic so when
config.checkApiLimits is true you require an explicit config.connectorName
(throw an error or return early) or else fall back to a clear generic label
(e.g., "unknown-connector" or "connector") instead of objectName; change the
assignment used by verifyApiLimitsSafe (and related code paths using
parseLimitThreshold/verifyApiLimitsSafe) to enforce this check so alerts use the
real connector name.
- Around line 115-122: The Promise.race timeout leaves the underlying request
running; update checkApiLimits to accept an AbortSignal and thread a
controller.signal into the call so the HTTP request is cancelled on timeout:
replace the existing timeout race (timeoutId / timeoutPromise) with an
AbortController, set a timer to call controller.abort() (and throw/relay a
TimeoutError), pass controller.signal into checkApiLimits(auth, store, signal),
and ensure you clear the timer and avoid orphaned requests by aborting/clearing
in the finally path; also update any downstream functions (e.g.,
checkSalesforceLimits / executeFetchWithTimeout) to forward the signal
parameter.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 196-206: The fetch-based token exchange using
AbortSignal.timeout(10000) should catch timeout-specific failures and rethrow a
clearer error; wrap the existing fetch(tokenUrl, ...) call in a try/catch,
detect AbortError/timeout (e.g., error.name === 'AbortError' or DOMException
with the abort name) and throw a new, descriptive error like "Token exchange
timed out after 10s" (or attach contextual info such as tokenUrl, clientId,
code) so callers of the token exchange receive explicit timeout diagnostics
instead of a generic DOMException; leave non-timeout errors to propagate or
rewrap with their original details.
In `@packages/pieces/salesforce/src/lib/sf-fetch.ts`:
- Around line 98-120: In the catch block around executeFetchWithTimeout in
sf-fetch.ts, immediately re-throw if the caller aborted by checking
init.signal?.aborted (preserving the original AbortError) instead of treating it
as a transient error; only run parseNetworkErrorMsg and set isTransientError =
true for non-abort errors so that isRetriableError and the retry loop aren’t
entered for caller cancellations (optionally make the backoff sleep abort-aware
by wiring init.signal into the retry wait).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0708f93-6937-4617-94ca-2ffd0d174d76
📒 Files selected for processing (9)
apps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/connections/connections/token-refresh.service.spec.tsapps/api/src/modules/connections/connections/token-refresh.service.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tspackages/connectors/src/intelligence/universal-trigger-engine.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-discovery.adapter.spec.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-query.adapter.tspackages/pieces/salesforce/src/lib/sf-fetch.ts
pramodnarayana
marked this pull request as draft
March 9, 2026 14:06
pramodnarayana
marked this pull request as ready for review
March 9, 2026 14:06
This was referenced Mar 10, 2026
This was referenced Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Breaking Changes