feat: DB-driven piece registry (Tier 4A) - #85
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:
📝 WalkthroughWalkthroughAdds a DB-backed pieces registry with runtime dynamic imports, injects pieces into the PieceRegistry via DI, enriches/connects provider uiSchema and vendorParams into the connectors OAuth flow, introduces a DynamicAuthForm frontend component, and updates migrations, schema exports, tests, and tooling to support pieces and connections. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as ConnectAppCard/DynamicAuthForm
participant API as ConnectorsController
participant PRS as PieceRegistryService
participant Loader as PieceLoaderService
participant DB as Database
User->>Frontend: Open connect dialog
Frontend->>API: GET /providers (metadata)
API->>PRS: request provider + uiSchema
PRS->>Loader: ensure pieces loaded
Loader->>DB: SELECT enabled pieces
DB-->>Loader: pieces rows
Loader->>Loader: dynamic import piece packages
Loader-->>PRS: return loaded pieces
PRS-->>API: provider metadata with uiSchema
API-->>Frontend: provider metadata (includes uiSchema)
User->>Frontend: Submit form (creds + vendorParams)
Frontend->>API: POST /exchange (vendorParams included)
API->>PRS: validate vendorParams against piece auth props
PRS-->>API: validation result
API->>API: perform token exchange, store connection
API-->>Frontend: success
sequenceDiagram
participant Seeder as DatabaseManager.seed
participant DB
participant Loader as PieceLoaderService
participant Registry as PieceRegistryService
Seeder->>DB: Query enabled pieces
DB-->>Loader: pieces rows
Loader->>Loader: dynamic import each piece package
Loader-->>Registry: pieces[]
Registry->>Registry: register pieces (duplicate-name check)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 14
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/db/db-cli.ts (1)
4-11: 🧹 Nitpick | 🔵 TrivialESM compatibility fix looks correct, but verify
.envloading precedence.The
fileURLToPath/import.meta.urlapproach correctly handles ESM's lack of__dirname. However, the.envloading order may be surprising:dotenv.config({ path: path.resolve(__dirname, '../../../../.env') }); // Root .env - loaded first dotenv.config({ path: path.resolve(__dirname, '../../.env') }); // API .env - loaded secondBy default,
dotenv.configdoes not override existing environment variables. This means root-level.envvalues take precedence overapps/api/.envvalues, which is typically the opposite of what's expected (local overrides global).If the intent is for
apps/api/.envto override root values, consider reversing the order or using{ override: true }on the second call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/db/db-cli.ts` around lines 4 - 11, The dotenv calls use __dirname and load the root .env before the apps/api .env so root values currently win; to make the API .env override root variables either swap the two dotenv.config(...) calls so dotenv.config({ path: path.resolve(__dirname, '../../.env') }) runs after the root load, or keep the order and add { override: true } to the second call (the dotenv.config invocation referencing '../../.env'); update the calls near __dirname/fileURLToPath so the intended precedence is enforced.apps/api/drizzle/0000_tricky_hawkeye.sql (1)
1-170:⚠️ Potential issue | 🔴 CriticalDo not mutate the already-applied
0000migration.Any environment that has already recorded
0000_tricky_hawkeyewill never rerun it, so it will missapp_connection,pieces, the new foreign keys, and the changed constraints/indexes entirely. These changes need a new numbered migration, with0000left immutable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/drizzle/0000_tricky_hawkeye.sql` around lines 1 - 170, The migration file 0000_tricky_hawkeye.sql must not be mutated; create a new sequential migration (e.g., 0001_...) that contains the additional DDL currently appended to 0000: the CREATE TABLE for app_connection and pieces, the ALTER TABLE foreign keys (app_connection_tenant_id_organization_id_fk, role_permission_*, account_userId_user_id_fk, etc.), the index/index-unique changes (tenant_external_id_unique_idx, member_user_org_unique WHERE clause, organization_slug_unique_idx WHERE clause, idx_role_permission_unique using COALESCE), and the INSERT INTO pieces; leave 0000_tricky_hawkeye.sql exactly as-is and ensure the new migration applies these schema changes so environments that already executed 0000 will receive them.
🤖 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/drizzle/0000_tricky_hawkeye.sql`:
- Around line 158-160: Add a composite index to support the query in
connectors.controller.ts (the listing that filters by tenant_id and status and
orders by created_at DESC, id DESC) so the DB can use an index to avoid sorting;
create a btree index on app_connection with columns (tenant_id, status,
created_at DESC, id DESC) (e.g. name it tenant_status_created_at_id_idx) and add
that statement to the SQL migration alongside the other app_connection indexes.
In `@apps/api/src/db/database-manager.spec.ts`:
- Around line 74-85: The mock for insert (drizzleMocks.insert via
makeInsertChain) returns a plain object from values(), but production Drizzle's
values() is thenable and may be awaited directly; update makeInsertChain so
values returns a thenable Promise (or an object with a then method) that
resolves to the expected terminal value (e.g., undefined or [{id: 'mock-id'}])
and also exposes the chainable methods onConflictDoNothing, onConflictDoUpdate,
and returning so both awaiting db.insert(...).values(...) and method-chaining
paths work correctly.
In `@apps/api/src/db/database-manager.ts`:
- Around line 350-368: Remove the duplicate piece INSERTs from the seed() method
in database-manager.ts: delete the block that inserts into schema.pieces (the
salesforce and quickbooks entries using .onConflictDoNothing()) and rely on the
consolidated migration (0000_tricky_hawkeye.sql) as the source of truth for
initial piece data; ensure any test fixtures or docs that expect seed() to
populate those pieces are updated accordingly if needed.
In `@apps/api/src/modules/connections/connections/connectors.controller.ts`:
- Around line 54-63: validateVendorParams currently only validates against
piece.auth.props and silently ignores extra keys, and exchangeCode uses
piece.auth.props while getProviders exposes p.uiSchema; update
validateVendorParams (and the other call sites that use it) to choose the
authoritative schema: prefer p.uiSchema when present (fall back to
piece.auth.props), validate that every vendorParams key exists in that schema,
call assertPropValue for declared keys, and throw/reject on any extra keys so
undeclared params are not persisted into valueBlob.data; adjust
exchangeCode/getProviders call sites to pass the same schema (p.uiSchema ||
piece.auth.props) into validateVendorParams to ensure mirror validation.
- Around line 101-136: validateExchangeBody currently assumes inputs are strings
and calls string methods (e.g., displayName?.trim(), toKebabSlug) which can
throw for non-string payloads; add runtime type checks at the start of
validateExchangeBody to reject non-string fields (providerName, code, clientId,
clientSecret, state, displayName) by throwing BadRequestException when typeof
!== 'string' or values are missing, then proceed with trimming, length checks
using MAX_DISPLAY_NAME_LENGTH and MAX_EXTERNAL_ID_LENGTH and calling toKebabSlug
on a confirmed string to generate externalId.
In `@apps/api/src/modules/pieces/piece-loader.service.ts`:
- Around line 75-83: The isPiece type guard currently only checks name,
displayName, and triggers; update it to validate all required Piece fields to
match the Piece interface by additionally asserting that logoUrl and description
are strings and that actions (and triggers) are non-null objects (or
arrays/records as appropriate). Modify the isPiece function to check typeof
v['logoUrl'] === 'string', typeof v['description'] === 'string', and that
v['actions'] is an object (and not null) in addition to the existing checks;
keep createPiece() usage compatibility but ensure the guard reflects the full
interface (Piece, isPiece).
In `@apps/web/src/modules/connections/components/ConnectAppCard.tsx`:
- Around line 41-56: The state type defaultCreds currently declares env but
handleOpenChange only sets clientId; update handleOpenChange to include env when
calling setDefaultCreds by reading the env field from the
getConnectionCredentials response (use
getConnectionCredentials(connection.id).then(creds => setDefaultCreds({
clientId: creds.clientId, env: creds.env }))...), or alternatively remove env
from the defaultCreds type and drop any usage of defaultCreds?.env elsewhere
(e.g., the place that passes env into the connect flow); adjust setDefaultCreds
calls and any consumers (refer to defaultCreds, setDefaultCreds,
getConnectionCredentials, handleOpenChange, and the code path that reads
defaultCreds?.env) accordingly.
- Around line 70-86: The connect flow drops vendorParams: update the connect
callback in useConnections.ts to destructure vendorParams from its parameters
(alongside providerName, clientId, clientSecret, displayName, env), extend the
pendingCredentials type/interface to include vendorParams:
Record<string,string>, and include vendorParams when building popupUrl query
params so the OAuth popup receives them; ensure handleSuccess/exchangeOAuthCode
continue to accept and forward vendorParams from the popup response.
In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx`:
- Around line 121-127: handleCopy currently only handles the fulfilled promise
from navigator.clipboard.writeText and ignores failures; update handleCopy to
handle rejections (use .catch or convert to async/await with try/catch) so
clipboard permission or other errors are surfaced, and provide user feedback on
failure (e.g., set an error state or show a toast/alert) while still returning
early if callbackUrl is falsy; reference navigator.clipboard.writeText,
handleCopy, setCopied and callbackUrl when locating the code to change.
- Around line 52-56: renderFieldControl is passing field.onChange directly into
the Input component, but Input.onChange receives a ChangeEvent rather than a raw
value; wrap the handler so you call field.onChange with event.target.value (and
for prop.type === 'NUMBER' convert to a number or empty string appropriately)
instead of passing the function directly. Locate the renderFieldControl
function, replace onChange={field.onChange} with a wrapper that extracts
e.target.value and forwards the correct type to field.onChange, and keep using
field.value for the value prop.
In `@packages/connectors/src/oauth/types.ts`:
- Around line 11-12: The new optional field uiSchema: Record<string, any> is a
backward-compatible addition to enable dynamic UI generation for provider auth
forms; keep the change as-is for now but, if this pattern expands, replace the
broad Record<string, any> with a more specific JSON Schema type (e.g., a
dedicated JsonSchema or UISchema interface/alias) and update all usages of
uiSchema to use that new type to improve type safety while preserving
optionality.
In `@packages/database/src/schema/pieces.ts`:
- Around line 12-28: Add an updatedAt timestamp column to the pieces table to
track modifications: modify the pieces pgTable definition (symbol: pieces) to
include an updatedAt field (similar to createdAt) using timestamp('updated_at',
{ withTimezone: true }) and set a sensible default (e.g., defaultNow()) and
notNull(); if you need automatic updates on row changes, ensure your application
updates updatedAt in the PieceRegistryService update flows or add a DB trigger
to set updated_at on UPDATE.
In `@TECHNICAL_DEBT.md`:
- Around line 294-297: The PieceLoaderService currently performs runtime
installs (shelling out to npm) which mutates shared state; change
PieceLoaderService so it never runs npm install during request handling—instead
have it verify the requested piece (in methods on PieceLoaderService) exists in
./plugins and is valid, and if missing return a clear error/exception
instructing callers to trigger the out-of-band admin installer; remove or gate
any code paths that call npm install/@nexiom/piece-* from inside
PieceLoaderService, and add a boolean check/log message in the loader (e.g., in
loadPiece / ensurePieceInstalled) to fail fast with a descriptive error when an
artifact is not preinstalled so that plugin installation is handled by a
separate admin job/service.
- Line 296: The "Sandboxing" item incorrectly suggests worker_threads provide
crash isolation; update the sentence to clarify that worker_threads run in the
same Node.js process and do not protect against fatal crashes, and replace or
augment the example "worker_threads" with true isolation options such as
"child_process", containers, or a separate worker service; specifically change
the "Sandboxing" section wording that mentions `worker_threads` so it states
that true isolation requires separate processes/containers (e.g., using
`child_process` or containerized workers) rather than `worker_threads`.
---
Outside diff comments:
In `@apps/api/drizzle/0000_tricky_hawkeye.sql`:
- Around line 1-170: The migration file 0000_tricky_hawkeye.sql must not be
mutated; create a new sequential migration (e.g., 0001_...) that contains the
additional DDL currently appended to 0000: the CREATE TABLE for app_connection
and pieces, the ALTER TABLE foreign keys
(app_connection_tenant_id_organization_id_fk, role_permission_*,
account_userId_user_id_fk, etc.), the index/index-unique changes
(tenant_external_id_unique_idx, member_user_org_unique WHERE clause,
organization_slug_unique_idx WHERE clause, idx_role_permission_unique using
COALESCE), and the INSERT INTO pieces; leave 0000_tricky_hawkeye.sql exactly
as-is and ensure the new migration applies these schema changes so environments
that already executed 0000 will receive them.
In `@apps/api/src/db/db-cli.ts`:
- Around line 4-11: The dotenv calls use __dirname and load the root .env before
the apps/api .env so root values currently win; to make the API .env override
root variables either swap the two dotenv.config(...) calls so dotenv.config({
path: path.resolve(__dirname, '../../.env') }) runs after the root load, or keep
the order and add { override: true } to the second call (the dotenv.config
invocation referencing '../../.env'); update the calls near
__dirname/fileURLToPath so the intended precedence is enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 256ebe92-1f03-4ebc-96c8-9b75a1f3396d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
TECHNICAL_DEBT.mdapps/api/drizzle/0000_tricky_hawkeye.sqlapps/api/drizzle/0001_daffy_carlie_cooper.sqlapps/api/drizzle/0002_lively_paper_doll.sqlapps/api/drizzle/0003_create_storage_registry.sqlapps/api/drizzle/0004_workspace_id_unique.sqlapps/api/drizzle/meta/0000_snapshot.jsonapps/api/drizzle/meta/0001_snapshot.jsonapps/api/drizzle/meta/0002_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/package.jsonapps/api/src/db/database-manager.spec.tsapps/api/src/db/database-manager.tsapps/api/src/db/db-cli.tsapps/api/src/db/schema.tsapps/api/src/modules/connections/connections.module.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/pieces/piece-loader.service.tsapps/api/src/modules/pieces/pieces.module.tsapps/api/src/modules/trigger/piece-registry.service.spec.tsapps/api/src/modules/trigger/piece-registry.service.tsapps/api/src/modules/trigger/trigger.module.tsapps/web/src/modules/connections/components/ConnectAppCard.tsxapps/web/src/modules/connections/components/DynamicAuthForm.tsxpackages/connectors/package.jsonpackages/connectors/src/oauth/providers/salesforce.tspackages/connectors/src/oauth/types.tspackages/database/src/client.tspackages/database/src/index.tspackages/database/src/schema/pieces.tspackages/pieces/quickbooks/package.jsonpackages/pieces/quickbooks/src/index.tspackages/pieces/quickbooks/src/lib/auth.tspackages/pieces/quickbooks/src/triggers/universal-trigger.tspackages/pieces/quickbooks/tsconfig.lib.jsonturbo.json
💤 Files with no reviewable changes (7)
- apps/api/drizzle/0002_lively_paper_doll.sql
- packages/connectors/src/oauth/providers/salesforce.ts
- apps/api/drizzle/0003_create_storage_registry.sql
- apps/api/drizzle/meta/0001_snapshot.json
- apps/api/drizzle/0001_daffy_carlie_cooper.sql
- apps/api/drizzle/0004_workspace_id_unique.sql
- apps/api/drizzle/meta/0002_snapshot.json
Comment on lines
+158
to
+160
| CREATE INDEX "app_name_idx" ON "app_connection" USING btree ("app_name");--> statement-breakpoint | ||
| CREATE INDEX "tenant_status_idx" ON "app_connection" USING btree ("tenant_id","status");--> statement-breakpoint | ||
| CREATE UNIQUE INDEX "tenant_external_id_unique_idx" ON "app_connection" USING btree ("tenant_id","external_id");--> statement-breakpoint |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add an index that matches the active-connections listing query.
apps/api/src/modules/connections/connections/connectors.controller.ts Lines 205-247 filter on tenant_id + status and then order by created_at DESC, id DESC. The current indexes still force a sort for every page read, which will degrade once app_connection grows.
Suggested index
CREATE INDEX "app_name_idx" ON "app_connection" USING btree ("app_name");--> statement-breakpoint
CREATE INDEX "tenant_status_idx" ON "app_connection" USING btree ("tenant_id","status");--> statement-breakpoint
+CREATE INDEX "tenant_status_created_at_id_idx"
+ ON "app_connection" USING btree ("tenant_id","status","created_at" DESC,"id" DESC);--> statement-breakpoint
CREATE UNIQUE INDEX "tenant_external_id_unique_idx" ON "app_connection" USING btree ("tenant_id","external_id");--> statement-breakpoint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/drizzle/0000_tricky_hawkeye.sql` around lines 158 - 160, Add a
composite index to support the query in connectors.controller.ts (the listing
that filters by tenant_id and status and orders by created_at DESC, id DESC) so
the DB can use an index to avoid sorting; create a btree index on app_connection
with columns (tenant_id, status, created_at DESC, id DESC) (e.g. name it
tenant_status_created_at_id_idx) and add that statement to the SQL migration
alongside the other app_connection indexes.
Comment on lines
+121
to
+127
| const handleCopy = () => { | ||
| if (!callbackUrl) return; | ||
| navigator.clipboard.writeText(callbackUrl).then(() => { | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
Clipboard API failure silently ignored.
navigator.clipboard.writeText returns a Promise but only .then() is handled. If clipboard access is denied (some browsers require secure context or permissions), the error is silently swallowed and the user sees no feedback.
🛡️ Proposed fix to handle clipboard errors
const handleCopy = () => {
if (!callbackUrl) return;
navigator.clipboard.writeText(callbackUrl).then(() => {
setCopied(true);
setTimeout(() => setCopied(false), 2000);
- });
+ }).catch(() => {
+ // Fallback or user feedback on clipboard failure
+ console.warn('Clipboard write failed');
+ });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleCopy = () => { | |
| if (!callbackUrl) return; | |
| navigator.clipboard.writeText(callbackUrl).then(() => { | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }); | |
| }; | |
| const handleCopy = () => { | |
| if (!callbackUrl) return; | |
| navigator.clipboard.writeText(callbackUrl).then(() => { | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }).catch(() => { | |
| // Fallback or user feedback on clipboard failure | |
| console.warn('Clipboard write failed'); | |
| }); | |
| }; |
🤖 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
121 - 127, handleCopy currently only handles the fulfilled promise from
navigator.clipboard.writeText and ignores failures; update handleCopy to handle
rejections (use .catch or convert to async/await with try/catch) so clipboard
permission or other errors are surfaced, and provide user feedback on failure
(e.g., set an error state or show a toast/alert) while still returning early if
callbackUrl is falsy; reference navigator.clipboard.writeText, handleCopy,
setCopied and callbackUrl when locating the code to change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
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/connections/connectors.controller.ts (1)
333-355:⚠️ Potential issue | 🟠 Major
getConnectionCredentials()never returns the savedenv.The web flow now reads
creds.envto prefill reconnect dialogs, but this endpoint only selectsvalueand returns{ clientId, hasClientSecret }. Existing connections will silently fall back to the default environment instead of the one stored inmetadata.Also applies to: 380-383
🤖 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 333 - 355, The endpoint is not returning the saved environment so update the DB select and response handling in getConnectionCredentials (and the similar block around lines 380-383): include appConnections.metadata in the select, parse connection.metadata (if present) to extract an env string (defaulting to '' or a sane default when missing), and return that env alongside clientId and hasClientSecret; ensure you also handle JSON parse errors safely and apply the same change to the other query path referenced in the review.
♻️ Duplicate comments (3)
packages/database/src/schema/pieces.ts (1)
29-30: 🧹 Nitpick | 🔵 TrivialMake
updatedAtadvance on updates.
defaultNow()only sets the insert timestamp. Ifenabledorversionchanges later, this column stays stale unless every update path remembers to set it manually. The rest of the schema already uses.$onUpdate(() => new Date())for this pattern.Suggested fix
- /** Last modification time. Must be set explicitly by application code on update. */ - updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().notNull(), + /** Last modification time. */ + updatedAt: timestamp('updated_at', { withTimezone: true }) + .defaultNow() + .notNull() + .$onUpdate(() => new Date()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database/src/schema/pieces.ts` around lines 29 - 30, The updatedAt column uses .defaultNow() so it only sets on INSERT; modify the timestamp definition for updatedAt (the updatedAt property in packages/database/src/schema/pieces.ts) to also call .$onUpdate(() => new Date()) so it advances automatically on UPDATEs while preserving .defaultNow() and .notNull() behavior; locate the updatedAt timestamp('updated_at', { withTimezone: true }) definition and append .$onUpdate(() => new Date()) to it.apps/api/src/db/database-manager.spec.ts (1)
74-91:⚠️ Potential issue | 🟠 MajorReturn a real Promise from
values()instead of addingthento a plain object.Line 80 now trips Biome's
noThenPropertyrule, and this stub still doesn't behave like a normal Promise if a caller relies on rejection or chaining. Attach the chain methods toPromise.resolve(...)instead.Suggested fix
const makeInsertChain = () => ({ values: vi.fn().mockImplementation(() => { - const chain = { - // Thenable: allows `await db.insert(t).values(...)` - then: (resolve: (v: undefined) => void) => resolve(undefined), - // Chain methods - onConflictDoNothing: vi.fn().mockResolvedValue(undefined), - onConflictDoUpdate: vi.fn().mockReturnValue({ - returning: vi.fn().mockResolvedValue([{ id: 'mock-id' }]), - }), - returning: vi.fn().mockResolvedValue([{ id: 'mock-id' }]), - }; - return chain; + const returning = vi.fn().mockResolvedValue([{ id: 'mock-id' }]); + return Object.assign(Promise.resolve(undefined), { + onConflictDoNothing: vi.fn().mockResolvedValue(undefined), + onConflictDoUpdate: vi.fn().mockReturnValue({ returning }), + returning, + }); }), });🤖 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 74 - 91, The test stub for makeInsertChain currently returns a plain object with a then property; update values (the method returned by makeInsertChain and assigned to drizzleMocks.insert) to return a real Promise (e.g., return Promise.resolve(chain) or make values async and return chain) and attach the chain methods (onConflictDoNothing, onConflictDoUpdate, returning) to that chain object before resolving so callers get a genuine Promise that supports proper chaining and rejection semantics.apps/api/src/modules/connections/connections/connectors.controller.ts (1)
54-59:⚠️ Potential issue | 🟠 MajorServer-side vendor-param validation still doesn't mirror the published schema.
getProviders()exposesp.uiSchema || piece.auth.props, butexchangeCode()still validates onlypiece.auth.props, andvalidateVendorParams()returns early when no schema is present. Providers backed only byuiSchemacan therefore still persist uncheckedvendorParamsintovalueBlob.data.Also applies to: 197-203, 491-495
🤖 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 54 - 59, validateVendorParams currently returns early if schema is falsy and only validates against piece.auth.props, allowing vendors with only p.uiSchema to bypass validation; update validateVendorParams to accept and use either piece.auth.props or p.uiSchema (i.e., treat schema = schema ?? vendorDescriptor.uiSchema) and perform validation even when the original schema arg is undefined, and update exchangeCode to call validateVendorParams with the schema selection logic used in getProviders (use p.uiSchema || piece.auth.props) so vendorParams are validated against the same schema source that getProviders exposes.
🤖 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/db-cli.ts`:
- Line 12: The dotenv.config call currently uses override: true which allows
checked-in .env to overwrite shell/CI vars; change the loading order to first
call dotenv.config for the workspace/root .env (without override), then call
dotenv.config for the API-specific .env using override: false so existing
process.env values (from the shell/CI) are preserved; update the code path that
constructs the two path.resolve references and ensure the second dotenv.config
uses override: false to prevent destructive commands (drop/fresh/reset) from
being redirected by checked-in env files.
In `@apps/api/src/modules/pieces/piece-loader.service.ts`:
- Around line 64-69: The exported piece name mismatch must be treated as invalid
rather than returned; in PieceLoaderService (the method that currently checks
exported.name !== expectedName and logs a warn) change the behavior to reject
the export—either throw a clear error (e.g., InvalidPieceExportError) or return
a sentinel (e.g., null/undefined) so callers skip registration—so that
PieceRegistryService does not register a piece under the wrong key; also update
the log to error-level and include both expectedName and exported.name for
diagnostics and ensure callers of this method handle the thrown error or null
result.
In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx`:
- Around line 13-21: The UiPropType/UiSchemaProp in DynamicAuthForm.tsx
currently restricts field types to SHORT_TEXT, LONG_TEXT, SECRET_TEXT, NUMBER,
CHECKBOX but the registry can return DROPDOWN, STATIC_DROPDOWN, JSON (and
possibly others); update the type handling in UiPropType and all
render/validation logic in DynamicAuthForm (functions/components that map
uiSchema to controls) to either (a) match the shared connector property
definitions by adding DROPDOWN, STATIC_DROPDOWN and JSON to UiPropType and
implement corresponding controls/validation (select for DROPDOWN/STATIC_DROPDOWN
using provided options; JSON editor/textarea with JSON parsing/validation), or
(b) explicitly detect unsupported types at render/validation time and return a
clear error/reject the schema so the UI does not render an invalid control;
ensure the changes are applied consistently wherever uiSchema is used (including
the areas noted around lines 24-36, 39-69, 72-85, and 245-266) and reference
UiPropType and UiSchemaProp when updating type definitions and render/validation
branches.
- Around line 78-80: The validation currently sets shape.clientSecret = isUpdate
? z.string().optional() : z.string().min(1, ...) which lets reconnect
submissions omit the secret even though /connectors/oauth-exchange requires it;
update the logic so clientSecret is required for reconnects (e.g. change to
consider a reconnect flag: shape.clientSecret = isUpdate && !isReconnect ?
z.string().optional() : z.string().min(1, 'Client secret is required')), and
make the same change for the duplicate block around lines 200-208 so reconnect
flows to /connectors/oauth-exchange always provide a non-empty clientSecret.
- Around line 106-131: The form isn't reset when async credentials arrive and
the environment Select is uncontrolled; inside DynamicAuthForm call
form.reset(…) within a useEffect that depends on the defaultValues prop (and any
cred-loading props) to rehydrate the form when defaults change, using the same
mergedDefaults object you build; also convert the environment Select to a
controlled component by wiring its value and change handler to the
react-hook-form field (use field.onChange for onValueChange and field.value for
value) so the select updates when form.reset runs.
In `@apps/web/src/modules/connections/hooks/useConnections.ts`:
- Around line 14-15: The hook currently stores vendorParams in
pendingCredentials but then either appends them to popupUrl (leaking secrets) or
ignores them when handling the OAuth response; update the logic in
useConnections (reference pendingCredentials, handleSuccess, popupUrl, and the
createConnection/callback invocation points) to stop serializing vendorParams
into popupUrl and instead merge vendorParams from pendingCredentials into the
payload passed to handleSuccess/createConnection so user-entered params are
preserved without exposing SECRET_TEXT in the GET URL; ensure all places noted
(lines handling popup creation and the handleSuccess flow) read
pendingCredentials?.vendorParams and combine it with the popup/callback payload
before continuing.
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts`:
- Around line 113-122: The mkOptions helper currently returns an
IdentityModuleOptions object missing the required betterAuthConfig field; update
mkOptions to include a minimal mock betterAuthConfig (e.g., the required shape
used by the module) or explicitly document why it's omitted, so tests don't rely
on an incomplete IdentityModuleOptions; locate mkOptions in
drizzle-user.adapter.spec.ts and add a minimal betterAuthConfig property to the
returned object (matching the interface shape expected by IdentityModuleOptions)
or add a comment above mkOptions noting intentional omission.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connections/connectors.controller.ts`:
- Around line 333-355: The endpoint is not returning the saved environment so
update the DB select and response handling in getConnectionCredentials (and the
similar block around lines 380-383): include appConnections.metadata in the
select, parse connection.metadata (if present) to extract an env string
(defaulting to '' or a sane default when missing), and return that env alongside
clientId and hasClientSecret; ensure you also handle JSON parse errors safely
and apply the same change to the other query path referenced in the review.
---
Duplicate comments:
In `@apps/api/src/db/database-manager.spec.ts`:
- Around line 74-91: The test stub for makeInsertChain currently returns a plain
object with a then property; update values (the method returned by
makeInsertChain and assigned to drizzleMocks.insert) to return a real Promise
(e.g., return Promise.resolve(chain) or make values async and return chain) and
attach the chain methods (onConflictDoNothing, onConflictDoUpdate, returning) to
that chain object before resolving so callers get a genuine Promise that
supports proper chaining and rejection semantics.
In `@apps/api/src/modules/connections/connections/connectors.controller.ts`:
- Around line 54-59: validateVendorParams currently returns early if schema is
falsy and only validates against piece.auth.props, allowing vendors with only
p.uiSchema to bypass validation; update validateVendorParams to accept and use
either piece.auth.props or p.uiSchema (i.e., treat schema = schema ??
vendorDescriptor.uiSchema) and perform validation even when the original schema
arg is undefined, and update exchangeCode to call validateVendorParams with the
schema selection logic used in getProviders (use p.uiSchema || piece.auth.props)
so vendorParams are validated against the same schema source that getProviders
exposes.
In `@packages/database/src/schema/pieces.ts`:
- Around line 29-30: The updatedAt column uses .defaultNow() so it only sets on
INSERT; modify the timestamp definition for updatedAt (the updatedAt property in
packages/database/src/schema/pieces.ts) to also call .$onUpdate(() => new
Date()) so it advances automatically on UPDATEs while preserving .defaultNow()
and .notNull() behavior; locate the updatedAt timestamp('updated_at', {
withTimezone: true }) definition and append .$onUpdate(() => new Date()) to it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0675554-5ebe-4d36-be49-050313a91e17
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
TECHNICAL_DEBT.mdapps/api/drizzle/0001_add_composite_index_and_pieces_updated_at.sqlapps/api/drizzle/meta/_journal.jsonapps/api/package.jsonapps/api/src/db/database-manager.spec.tsapps/api/src/db/database-manager.tsapps/api/src/db/db-cli.tsapps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/pieces/piece-loader.service.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.tspackages/connectors/src/oauth/types.tspackages/database/src/schema/pieces.tspackages/identity/src/adapters/drizzle-user.adapter.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/web/src/modules/connections/components/DynamicAuthForm.tsx (2)
40-42:⚠️ Potential issue | 🟡 MinorEmpty optional NUMBER fields coerce to
0and bypass the submit filter.
z.coerce.number()converts empty strings to0viaNumber(''). The submit filter at lines 217-220 checks for empty strings, but by that point the value is already0, which passes through tovendorParams.Consider using
z.preprocessto convert empty strings toundefinedbefore coercion:🛡️ Proposed fix
if (prop.type === 'NUMBER') { - const field = z.coerce.number(); - return prop.required ? field : field.optional(); + const base = z.preprocess( + (v) => (v === '' || v === undefined ? undefined : v), + z.coerce.number(), + ); + return prop.required ? base : base.optional(); }🤖 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 40 - 42, The NUMBER schema branch currently uses z.coerce.number(), which coerces empty strings to 0 and bypasses the submit filter that expects empty values to be filtered before building vendorParams; change the schema to use z.preprocess to convert '' (empty string) to undefined before applying z.coerce.number(), and keep the optional handling (return prop.required ? field : field.optional()) so that empty optional fields become undefined and are correctly filtered out by the submit logic and not turned into 0.
268-282:⚠️ Potential issue | 🟡 MinorPlaceholder text contradicts the required validation.
The placeholder "(Unchanged — re-enter to reconnect)" suggests the user can leave
clientSecretblank in update mode, but the validation at line 135 makes it always required. This will confuse users who expect to keep their existing secret.Consider updating the placeholder to clarify that re-entry is mandatory:
🛡️ Proposed fix
- <Input type="password" {...field} value={field.value as string || ''} placeholder={isUpdate ? '(Unchanged — re-enter to reconnect)' : ''} /> + <Input type="password" {...field} value={field.value as string || ''} placeholder={isUpdate ? '(Required — re-enter to reconnect)' : ''} />🤖 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 268 - 282, The placeholder for the clientSecret input in DynamicAuthForm is misleading for update flows; when isUpdate is true the form validation still requires clientSecret (the validation rule/schema for clientSecret), so update the placeholder to reflect that re-entry is mandatory (e.g. "(Required — please re-enter to reconnect)") or alternatively adjust the clientSecret validation to allow empty values on update; locate the FormField with name="clientSecret" and the form validation rule/schema that enforces required and make the chosen change (update placeholder text when isUpdate is true, or change the validation logic to conditionally require clientSecret only when not isUpdate).
🤖 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 518-531: The code redundantly calls
this.providerRegistry.getProvider a second time as providerForSchema; reuse the
already-fetched providerData (from earlier lines where providerData was
validated) instead of calling getProvider again. Replace references to
providerForSchema?.uiSchema with providerData?.uiSchema and keep the existing
piece lookup (pieceRegistry.getPiece) and authProps extraction unchanged, then
pass those uiSchemaProps and authProps into validateVendorParams so only one
provider lookup is performed.
In `@apps/web/src/modules/connections/hooks/useConnections.ts`:
- Around line 14-15: Introduce a shared local type (e.g., PendingCredential or
ConnectionCredential) that captures the shape { clientId: string; clientSecret?:
string; displayName: string; env?: string; vendorParams?: Record<string,string>
} and replace the inline type used for pendingCredentials (useRef) and the
inline type used in the connect-related variable at lines ~96-104; update the
declarations for pendingCredentials and the connect variable to reference this
new type so both use the same definition and future changes stay in sync (keep
the same property names: clientId, clientSecret, displayName, env,
vendorParams).
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts`:
- Around line 114-129: The mkOptions helper currently uses two "as" type
assertions that bypass structural checks; remove the trailing "as
BetterAuthAdapterConfig" and "as IdentityModuleOptions" and make the function
signature mkOptions(): IdentityModuleOptions (already present) return a fully
conforming object literal instead of asserting types—ensure the betterAuthConfig
property contains all required fields of BetterAuthAdapterConfig and that the
top-level object satisfies IdentityModuleOptions (dbToken, constants with
systemTenantId/ownerRoleId/adminRoleId/memberRoleId, etc.) so the compiler
enforces completeness without any "as" casts.
---
Duplicate comments:
In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx`:
- Around line 40-42: The NUMBER schema branch currently uses z.coerce.number(),
which coerces empty strings to 0 and bypasses the submit filter that expects
empty values to be filtered before building vendorParams; change the schema to
use z.preprocess to convert '' (empty string) to undefined before applying
z.coerce.number(), and keep the optional handling (return prop.required ? field
: field.optional()) so that empty optional fields become undefined and are
correctly filtered out by the submit logic and not turned into 0.
- Around line 268-282: The placeholder for the clientSecret input in
DynamicAuthForm is misleading for update flows; when isUpdate is true the form
validation still requires clientSecret (the validation rule/schema for
clientSecret), so update the placeholder to reflect that re-entry is mandatory
(e.g. "(Required — please re-enter to reconnect)") or alternatively adjust the
clientSecret validation to allow empty values on update; locate the FormField
with name="clientSecret" and the form validation rule/schema that enforces
required and make the chosen change (update placeholder text when isUpdate is
true, or change the validation logic to conditionally require clientSecret only
when not isUpdate).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ac72b7e-88a9-4313-837f-8cf81b360a0d
📒 Files selected for processing (10)
apps/api/src/db/database-manager.spec.tsapps/api/src/db/db-cli.tsapps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/pieces/piece-loader.service.tsapps/web/components.jsonapps/web/src/modules/connections/components/DynamicAuthForm.tsxapps/web/src/modules/connections/hooks/useConnections.tsapps/web/src/shared/components/ui/textarea.tsxpackages/database/src/schema/pieces.tspackages/identity/src/adapters/drizzle-user.adapter.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/src/modules/connections/components/DynamicAuthForm.tsx (1)
40-57:⚠️ Potential issue | 🟠 MajorOptional
NUMBERandJSONfields still don't round-trip the empty state correctly.For
NUMBER,field.optional()wraps the preprocess schema, so clearing the input still falls through intoz.coerce.number()instead of becomingundefined.JSONhas the same problem:z.string().refine(...).optional()still runs the refine on''. On top of that, the number input still renders a real0back as empty because it uses|| ''. That makes optional fields fail validation when blank and hides legitimate zero values.♻️ Proposed fix
if (prop.type === 'NUMBER') { - const field = z.preprocess( - (v) => (v === '' ? undefined : v), - z.coerce.number(), - ); - return prop.required ? field : field.optional(); + return z.preprocess( + (v) => (v === '' ? undefined : v), + prop.required ? z.coerce.number() : z.coerce.number().optional(), + ); } if (prop.type === 'JSON') { - const field = z.string().refine( - (v) => { try { JSON.parse(v); return true; } catch { return false; } }, - { message: `${prop.displayName ?? key} must be valid JSON` }, - ); - return prop.required ? field : field.optional(); + return z.preprocess( + (v) => (v === '' ? undefined : v), + (prop.required ? z.string() : z.string().optional()).refine( + (v) => v === undefined || (() => { try { JSON.parse(v); return true; } catch { return false; } })(), + { message: `${prop.displayName ?? key} must be valid JSON` }, + ), + ); } @@ - value={(field.value as string) || ''} + value={field.value ?? ''}In Zod, does `z.preprocess(v => v === '' ? undefined : v, z.coerce.number()).optional()` accept an empty string as missing, or must the inner schema be optional (`z.preprocess(..., z.coerce.number().optional())`)? Also, what is the recommended pattern for optional JSON text inputs that should treat `''` as undefined?Also applies to: 116-124
🤖 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 40 - 57, The optional NUMBER and JSON schemas must treat empty string as missing before the inner validator runs and the input rendering must preserve actual zero values; change z.preprocess(..., z.coerce.number()) to make the inner schema optional (e.g. z.preprocess(v => v === '' ? undefined : v, z.coerce.number().optional())) and for JSON wrap the string/refine as optional inside the preprocess (or preprocess into undefined then use z.string().optional().refine(...)), so refine/coercion never runs for ''. Also update the input rendering that uses || '' to instead use a nullish check that preserves 0 (e.g. value === 0 ? '0' : value ?? '') so legitimate zeroes are displayed correctly; apply these fixes in DynamicAuthForm where prop.type === 'NUMBER' and prop.type === 'JSON' and in the value-binding logic that currently uses || ''.
🤖 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/web/src/modules/connections/hooks/useConnections.ts`:
- Line 23: The hook currently stores a single pendingCredentials in useRef which
allows a second connect() to overwrite the first; change pendingCredentials to
be keyed by the OAuth popup/session state (e.g., use a Map<string,
PendingCredential>) or alternatively block new connect() while a pending flow
exists. Update connect() to generate/return a unique state and save pending
credentials under that state, and update handleSuccess(state, code) to look up
and remove the matching PendingCredential (instead of reading a single ref).
Also ensure any cleanup paths (timeouts, cancelation) remove the entry from the
map and that error paths log/handle missing state entries.
- Around line 8-13: The PendingCredential type currently allows clientSecret to
be optional which lets invalid reconnect flows compile; change clientSecret?:
string to clientSecret: string in the PendingCredential definition and update
any other occurrences of the optional clientSecret shape (the other
PendingCredential-like declarations referenced around the second and third
occurrences) so the type requires clientSecret, and then fix any call sites
(e.g., places that construct pending credentials before calling
exchangeOAuthCode) to provide a clientSecret value or handle the flow so a valid
clientSecret is always passed to functions like exchangeOAuthCode.
---
Duplicate comments:
In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx`:
- Around line 40-57: The optional NUMBER and JSON schemas must treat empty
string as missing before the inner validator runs and the input rendering must
preserve actual zero values; change z.preprocess(..., z.coerce.number()) to make
the inner schema optional (e.g. z.preprocess(v => v === '' ? undefined : v,
z.coerce.number().optional())) and for JSON wrap the string/refine as optional
inside the preprocess (or preprocess into undefined then use
z.string().optional().refine(...)), so refine/coercion never runs for ''. Also
update the input rendering that uses || '' to instead use a nullish check that
preserves 0 (e.g. value === 0 ? '0' : value ?? '') so legitimate zeroes are
displayed correctly; apply these fixes in DynamicAuthForm where prop.type ===
'NUMBER' and prop.type === 'JSON' and in the value-binding logic that currently
uses || ''.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0712b83c-95e2-4843-8694-4b48789878eb
📒 Files selected for processing (4)
apps/api/src/modules/connections/connections/connectors.controller.tsapps/web/src/modules/connections/components/DynamicAuthForm.tsxapps/web/src/modules/connections/hooks/useConnections.tspackages/identity/src/adapters/drizzle-user.adapter.spec.ts
|
|
||
| // Store credentials temporarily while the popup is open | ||
| const pendingCredentials = useRef<{ clientId: string; clientSecret?: string; displayName: string; env?: string } | null>(null); | ||
| const pendingCredentials = useRef<PendingCredential | null>(null); |
Contributor
There was a problem hiding this comment.
One in-flight OAuth attempt can overwrite another.
pendingCredentials is a single ref for the whole hook. If the user starts provider B before provider A's popup returns, the second connect() call replaces the first credential set, and handleSuccess() will exchange A's code with B's clientId/clientSecret/displayName/vendorParams. Either key pending state by popup/session state or block a new connect() while one is already pending.
Also applies to: 39-70, 113-123
🤖 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` at line 23, The
hook currently stores a single pendingCredentials in useRef which allows a
second connect() to overwrite the first; change pendingCredentials to be keyed
by the OAuth popup/session state (e.g., use a Map<string, PendingCredential>) or
alternatively block new connect() while a pending flow exists. Update connect()
to generate/return a unique state and save pending credentials under that state,
and update handleSuccess(state, code) to look up and remove the matching
PendingCredential (instead of reading a single ref). Also ensure any cleanup
paths (timeouts, cancelation) remove the entry from the map and that error paths
log/handle missing state entries.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/web/src/modules/connections/components/DynamicAuthForm.tsx (1)
14-25:⚠️ Potential issue | 🟠 MajorStop treating unknown runtime property types as generic strings.
ProviderResponse.uiSchemastill arrives asRecord<string, unknown>, and piece-backed auth props can emit types outside this narrowed union (for exampleMULTI_SELECT_DROPDOWN,OBJECT,FILE,DYNAMIC,CUSTOM_AUTH). The cast here hides that, so unsupported props fall through to the generic string/text-input path and then fail backend validation. Please add a runtime type guard and reject unsupported schemas until those controls are implemented.Also applies to: 171-176
🤖 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 14 - 25, The code currently casts ProviderResponse.uiSchema entries to UiSchemaProp/UiPropType which hides unknown runtime types (e.g., MULTI_SELECT_DROPDOWN, OBJECT, FILE, DYNAMIC, CUSTOM_AUTH) and lets them fall through to the generic string input; replace that cast with a runtime type guard for UiSchemaProp/UiPropType that explicitly checks the prop.type against the allowed union values, and when a prop.type is not one of the supported UiPropType values return/reject/skip the schema (e.g., surface a validation/error state or exclude the prop) so unsupported controls are rejected until implementations exist; update the same check where similar casting occurs (the other instance around the 171–176 area) to reuse the guard and ensure unsupported types are not treated as SHORT_TEXT/STRING.
🤖 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/web/src/modules/connections/components/ConnectAppCard.tsx`:
- Line 1: ConnectAppCard currently passes an inline object as the defaultValues
prop to DynamicAuthForm which is recreated on every render; wrap that object in
useMemo and pass the memoized value instead so DynamicAuthForm doesn't reset
during unrelated rerenders. Import useMemo from React (in addition to useState),
create a memo like const memoizedDefaultValues = useMemo(() => ({ ...existing
inline defaultValues logic }), [/* include only the actual deps that should
change the defaults, e.g. connectionSettings, app.id, or props used to build
defaults */]); and replace the inline defaultValues prop with
memoizedDefaultValues when rendering DynamicAuthForm.
In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx`:
- Around line 52-58: The JSON branch in DynamicAuthForm.tsx rejects blank
textareas because z.string().refine runs on '' (empty string) and
field.optional() only skips undefined; update the JSON field creation (the const
field variable for prop.type === 'JSON') to preprocess empty strings the same
way as the number branch — e.g., transform '' to undefined before validation or
short-circuit the refine to return true when v is ''/whitespace — then return
prop.required ? field : field.optional() so optional JSON props can be left
empty. Ensure you change only the logic around the const field (the
refine/transform) for JSON handling.
- Around line 11-12: The file references React.ReactElement but doesn't import
the React namespace, so add an explicit import for the ReactElement type from
'react' (e.g., import { ReactElement, useState, useMemo, useEffect } from
'react') and then update the component's return type annotation (the function
that currently returns React.ReactElement on line with the component definition)
to use the imported ReactElement type; ensure you only import the type (not the
whole namespace) to satisfy TypeScript and keep existing hooks like
useState/useMemo/useEffect unchanged.
- Around line 40-50: The NUMBER schema currently always uses
z.coerce.number().optional() so preprocessing ''→undefined bypasses required
checks; update the logic in DynamicAuthForm.tsx to build the inner schema based
on prop.required: use z.coerce.number() (non-optional) when prop.required is
true and z.coerce.number().optional() when false, then wrap that with
z.preprocess((v) => v === '' ? undefined : v, innerSchema) and finally if
prop.required is false call .optional() on the whole field; reference the
existing symbols prop.type === 'NUMBER', z.preprocess, and z.coerce.number() to
locate and modify this behavior.
In `@apps/web/src/modules/connections/hooks/useConnections.ts`:
- Around line 117-126: The pendingCredentials.current guard is never cleared if
the user manually closes the OAuth popup; modify the connect flow to clear
pendingCredentials.current when the popup is dismissed by adding a cleanup path:
after calling the popup/openPopup from useOAuthPopup (or where popupRef is set),
either subscribe to an onClose callback from useOAuthPopup or start a
short-interval poll checking popupRef.current?.closed and when closed clear
pendingCredentials.current (and optionally show a toast). Ensure this cleanup
runs on both manual close and normal success/error paths so
pendingCredentials.current is reliably reset for subsequent connect() calls.
---
Duplicate comments:
In `@apps/web/src/modules/connections/components/DynamicAuthForm.tsx`:
- Around line 14-25: The code currently casts ProviderResponse.uiSchema entries
to UiSchemaProp/UiPropType which hides unknown runtime types (e.g.,
MULTI_SELECT_DROPDOWN, OBJECT, FILE, DYNAMIC, CUSTOM_AUTH) and lets them fall
through to the generic string input; replace that cast with a runtime type guard
for UiSchemaProp/UiPropType that explicitly checks the prop.type against the
allowed union values, and when a prop.type is not one of the supported
UiPropType values return/reject/skip the schema (e.g., surface a
validation/error state or exclude the prop) so unsupported controls are rejected
until implementations exist; update the same check where similar casting occurs
(the other instance around the 171–176 area) to reuse the guard and ensure
unsupported types are not treated as SHORT_TEXT/STRING.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eea8b602-ae5a-4534-8bc7-aca86dae7f76
📒 Files selected for processing (3)
apps/web/src/modules/connections/components/ConnectAppCard.tsxapps/web/src/modules/connections/components/DynamicAuthForm.tsxapps/web/src/modules/connections/hooks/useConnections.ts
pramodnarayana
marked this pull request as draft
March 8, 2026 16:51
pramodnarayana
marked this pull request as ready for review
March 8, 2026 16:51
This was referenced Mar 9, 2026
This was referenced Mar 17, 2026
Merged
Merged
This was referenced Mar 25, 2026
Merged
Merged
This was referenced Apr 8, 2026
Merged
This was referenced Jun 9, 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
Database
Documentation
Chores