Repository navigation
refactor: move provider registry to code-first architecture - #68
Merged
pramodnarayana merged 6 commits intoFeb 25, 2026
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:
📝 WalkthroughWalkthroughMoves provider catalog from DB to a code-first in-memory registry; deletes provider schema/migrations and integration packages; adds tenant-scoped BYOA credential storage ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant ProviderRegistry
participant ConnectorsService
participant Database as CatalogDB
participant EncryptionService
participant OAuthProvider as OAuth Provider
rect rgba(100, 150, 200, 0.5)
Note over Client,OAuthProvider: Authorization Flow (tenant-scoped)
Client->>Controller: GET /connect/:provider?state=&tenantId=
Controller->>ProviderRegistry: getProvider(provider)
ProviderRegistry-->>Controller: ProviderDefinition
Controller->>ConnectorsService: getAuthorizationUrl(provider, state, tenantId)
ConnectorsService->>Database: SELECT app_credential WHERE tenant_id = X AND app_name = provider
Database-->>ConnectorsService: encryptedClientId/Secret
ConnectorsService->>EncryptionService: decryptClientSecret(encryptedSecret)
EncryptionService-->>ConnectorsService: clientSecret
ConnectorsService->>OAuthProvider: build authorize URL (client_id, redirect_uri, scopes)
OAuthProvider-->>ConnectorsService: Authorization URL
ConnectorsService-->>Controller: URL
Controller-->>Client: redirect URL
end
rect rgba(100, 200, 150, 0.5)
Note over Client,OAuthProvider: Token Exchange (tenant-scoped)
Client->>Controller: POST /connect/:provider/callback (code, tenantId)
Controller->>ProviderRegistry: getProvider(provider)
ProviderRegistry-->>Controller: ProviderDefinition
Controller->>ConnectorsService: exchangeCodeForTokens(provider, code, tenantId)
ConnectorsService->>Database: SELECT app_credential WHERE tenant_id = X AND app_name = provider
Database-->>ConnectorsService: encryptedClientId/Secret
ConnectorsService->>EncryptionService: decryptClientSecret(encryptedSecret)
EncryptionService-->>ConnectorsService: clientSecret
ConnectorsService->>OAuthProvider: POST token endpoint (code, client_id, client_secret)
OAuthProvider-->>ConnectorsService: tokens
ConnectorsService-->>Controller: TokenResponse
Controller-->>Client: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 19
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/connections/token-refresh.service.spec.ts (1)
50-66:⚠️ Potential issue | 🟡 MinorTest mocks a QuickBooks provider but calls
refresh('salesforce', ...).The mock at Line 52 sets
name: 'quickbooks'but the test at Line 63 callsclient.refresh('salesforce', ...). While functionally correct (the error message uses the argument, not the mock'sname), this is semantically confusing. Consider aligning the mock'snamewith the provider being tested, or use a generic name like'test_provider'to make it clear the name field isn't relevant to this test case.Proposed fix
(mockProviderRegistry.getProvider as Mock).mockReturnValue({ - name: 'quickbooks', - displayName: 'QuickBooks Online', - description: 'Accounting', + name: 'salesforce', + displayName: 'Salesforce', + description: 'CRM', logoUrl: '', - category: 'Accounting', + category: 'CRM', authType: 'OAUTH2' as const, - authorizeUrl: 'https://appcenter.intuit.com/connect/oauth2', + authorizeUrl: 'https://login.salesforce.com/services/oauth2/authorize', tokenUrl: '', scopes: [], });🤖 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.spec.ts` around lines 50 - 66, The test for token refresh is mocking a provider via mockProviderRegistry.getProvider but uses a different provider id when calling client.refresh('salesforce', ...), which is confusing; update the mock so its name matches the refresh call (e.g., set name: 'salesforce') or change both to a generic consistent identifier like 'test_provider' so mockProviderRegistry.getProvider and the client.refresh('...') argument align; ensure the assertion message remains the same and uses the same provider identifier referenced in the mock and the client.refresh call.apps/api/src/modules/connections/connectors.service.spec.ts (1)
190-206: 🧹 Nitpick | 🔵 TrivialInconsistent type assertion on Line 194.
This mock uses
as ProviderResultwhile all other similar mocks in this file (lines 93, 126, 166, 179, 226, 243) useas unknown as NonNullable<ProviderResult>. Pick one style for consistency.Suggested fix
mockProviderRegistry.getProvider.mockReturnValue({ name: 'salesforce', tokenUrl: 'https://login.salesforce.com/services/oauth2/token', - } as ProviderResult); + } as unknown as NonNullable<ProviderResult>);🤖 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.spec.ts` around lines 190 - 206, The mock return on mockProviderRegistry.getProvider inside the test "should successfully exchange a code for tokens" uses "as ProviderResult" which is inconsistent; change that assertion to match the other tests by casting the object to "as unknown as NonNullable<ProviderResult>" so the mock type aligns with the other mocks in this spec (refer to mockProviderRegistry.getProvider.mockReturnValue and the service.exchangeCodeForTokens test).
🤖 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/callback.controller.spec.ts`:
- Around line 155-158: Update the test that mocks
mockProviderRegistry.getProvider in the "should redirect with internal_error if
provider lookup fails" case to use a more accurate error message (e.g.,
"Unexpected registry error") instead of "DB error"; change the thrown Error
string in the mockImplementation for mockProviderRegistry.getProvider so the
test reflects the in-memory registry behavior.
In `@apps/api/src/modules/connections/connections/callback.controller.ts`:
- Around line 201-203: The persistConnection method currently accepts
_providerData but never uses it while authType is hardcoded to 'OAUTH2'; either
use providerData to determine the real auth type or remove the unused parameter
and update callers. Preferred fix: inside persistConnection (method name) read
authType from the ProviderDefinition argument (e.g., providerData.authType or
similar) instead of the hardcoded 'OAUTH2' and replace uses of the hardcoded
value; if ProviderDefinition doesn't include authType, add it to the type.
Alternate fix: remove the _providerData parameter from persistConnection and
remove the corresponding argument at each call site that passes providerData.
Ensure you update all references to authType in persistConnection accordingly.
In `@apps/api/src/modules/connections/connections/connectors.controller.spec.ts`:
- Around line 174-182: Replace the stale mock error message in
mockProviderRegistry.getAllProviders from 'DB connection failed' to a more
accurate string like 'Registry initialization error', and avoid invoking
action() twice by calling controller.getProviders() once inside a try/catch:
call the function, catch the thrown exception, then assert the caught error is
an InternalServerErrorException and that its message contains 'Failed to get
providers'; this keeps mockProviderRegistry.getAllProviders and
controller.getProviders as the referenced symbols to locate the test change.
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 65-83: Extract the duplicated credential lookup/validation into a
private helper (e.g., private async fetchAppCredential(tenantId: string,
providerName: string)) and replace the copies in getAuthorizationUrl and
exchangeCodeForTokens with calls to that helper; the helper should perform the
this.db.select().from(appCredentials).where(...and(eq(appCredentials.tenantId,
tenantId), eq(appCredentials.appName, providerName))) query, log the same error
via this.logger.error when credential is missing, and throw the same
NotFoundException, so future changes (caching or adding setupMetadata) are made
in one place.
- Around line 22-27: DefaultOAuthRefreshClient currently reads
clientId/clientSecret from process.env using normalized names, which breaks BYOA
because ConnectorsService stores OAuth credentials in the appCredentials DB;
update DefaultOAuthRefreshClient (in token-refresh.service.ts) to obtain
per-tenant clientId and clientSecret from the same source ConnectorsService uses
(e.g., call into ProviderRegistryService or a new method on ConnectorsService
that returns app credentials for a tenant/app) instead of process.env, and wire
that lookup through the token refresh flow so token refresh uses DB-stored
credentials for the given tenant.
In `@docs/architecture/credential_storage_architecture.md`:
- Around line 79-93: The doc flags application-layer-only tenant isolation as a
gap; add a tracked technical-debt entry in TECHNICAL_DEBT.md describing the
missing PostgreSQL RLS, target timeline, and owner, and implement a short-term
safeguard by creating a Drizzle query wrapper/middleware (e.g., a helper used by
ConnectorsService and any credential/connection accessors) that automatically
injects the tenant filter (eq(appCredentials.tenantId, tenantId)) into
credential queries and rejects queries that don't include that tenant predicate;
update usages of ConnectorsService, appCredentials accessors, and
credential/connection repository functions to call the wrapper so accidental
omission of the .where(eq(...)) is prevented.
In `@packages/connections/src/connectivity/provider-registry.ts`:
- Around line 13-14: The isAllowed function uses the `in` operator which matches
inherited prototype keys and can return true for names like "constructor";
update isAllowed to check own properties only (e.g., use
`Object.hasOwn(PROVIDER_REGISTRY, name)` or
`Object.prototype.hasOwnProperty.call(PROVIDER_REGISTRY, name)`) so it only
returns true for actual providers declared on PROVIDER_REGISTRY and not
inherited keys.
In `@packages/connections/src/connectivity/providers/index.ts`:
- Around line 11-14: The registry type is too permissive: change
PROVIDER_REGISTRY (currently declared as Record<string, ProviderDefinition>) to
use a stricter key union so typos are caught at compile time—declare a
ProviderName union (e.g., 'salesforce' | 'quickbooks') and retype
PROVIDER_REGISTRY as Record<ProviderName, ProviderDefinition>, keeping the
existing entries salesforce and quickbooks; if you must allow dynamic providers
instead, leave the type but update all lookup sites to handle undefined from
PROVIDER_REGISTRY safely.
In `@packages/connections/src/connectivity/providers/quickbooks.provider.ts`:
- Line 11: The logoUrl in quickbooks.provider.ts currently points to an external
third-party CDN ('https://cdn.activepieces.com/pieces/quickbooks.png'); replace
it with an internally hosted asset URL (upload the quickbooks.png to our own
CDN/storage or assets service) and update the logoUrl property to that internal
path in the QuickBooks provider export; optionally add a fallback/local relative
path or an assets helper reference used elsewhere in the codebase to ensure
availability if the external domain becomes unavailable.
- Line 16: The QuickBooks OAuth scopes array in quickbooks.provider.ts is
missing 'offline_access', so include 'offline_access' in the scopes array
(alongside 'com.intuit.quickbooks.accounting' and recommended OpenID scopes like
'openid', 'profile', 'email', 'phone' as needed) so the initial token exchange
returns a refresh_token; update the scopes constant/field named scopes in the
QuickBooks provider configuration to include 'offline_access' to enable
DefaultOAuthRefreshClient.refresh() to work.
In `@packages/connections/src/connectivity/types.ts`:
- Line 7: ProviderDefinition currently requires OAuth-only fields (authorizeUrl,
tokenUrl, scopes) for every AuthType (AuthType includes 'OAUTH2' | 'API_KEY' |
'BASIC'), forcing dummy values for API_KEY/BASIC providers; update the types so
OAuth fields are only required for OAUTH2 providers — either make
authorizeUrl/tokenUrl/scopes optional on ProviderDefinition or, preferably,
replace ProviderDefinition with a discriminated union keyed by authType (e.g.,
OAuth2Provider with authType 'OAUTH2' and required authorizeUrl/tokenUrl/scopes,
plus ApiKeyProvider and BasicProvider with authType 'API_KEY'/'BASIC' and no
OAuth fields) so code can safely narrow on authType and only access relevant
properties.
In `@packages/database/push-schema-local.ts`:
- Around line 4-6: The Client is being constructed with a hardcoded connection
string; change the Client({ connectionString: ... }) instantiation to read the
database URL from an environment variable (e.g., process.env.DATABASE_URL) and
optionally load .env via dotenv at module startup if not already loaded, and
provide a sensible fallback or throw a clear error if the env var is missing;
update the code that creates the "client" instance so it uses
process.env.DATABASE_URL (or process.env.NEXIOM_LOCAL_DATABASE_URL) instead of
the literal 'postgres://user:password@localhost:5432/nexiom_local' and ensure
any dotenv.config() call happens before Client is constructed.
- Around line 38-45: Move the call to client.connect() inside the try block and
ensure client.end() in the finally block is only called if the client was
successfully connected to avoid secondary errors; specifically, in the scope
where you create the Client instance referenced as client (and where you
currently call client.connect()), wrap client.connect() and await
client.query(query) inside the same try block, and in the finally block only
call client.end() if a successful connection flag or client._connected-like
state indicates the connection was opened (or guard with a boolean connected
variable set after await client.connect()) so that client.end() is not invoked
on an uninitialized/unconnected client.
- Around line 11-36: The SQL in push-schema-local.ts creates "app_credential"
with a removed column and FK: drop the "provider_id" column definition from the
CREATE TABLE statement and remove the entire DO $$...END $$; block that adds the
"app_credential_provider_id_provider_id_fk" foreign key; keep the rest of the
table columns (id, tenant_id, app_name, client_id, encrypted_client_secret,
setup_metadata, created_at, updated_at) and the existing unique index
"tenant_app_credential_unique_idx" so the raw SQL matches the current Drizzle
schema in app-credential.ts.
In `@packages/database/push-schema.ts`:
- Around line 38-45: The catch in the try/catch around client.query(query) logs
errors but swallows them, causing main() to resolve successfully; change the
catch in push-schema.ts to re-throw the caught error (or throw a new Error with
context) after logging so the promise rejects and the process exits non-zero,
ensuring the finally still calls client.end(); reference the client.query(query)
call and the surrounding try/catch in this file when applying the change.
- Around line 11-36: Remove the stale provider_id column and its FK block from
the app_credential DDL: delete the "provider_id" column definition in the CREATE
TABLE for app_credential and remove the DO $$ ... ALTER TABLE ... ADD CONSTRAINT
"app_credential_provider_id_provider_id_fk" ... block that references
public.provider(id); keep the other columns, the CREATE UNIQUE INDEX
"tenant_app_credential_unique_idx" and any setup_metadata/secret columns intact
so the SQL matches the source-of-truth schema (app_credential) and no longer
depends on the provider table.
- Around line 4-6: The hardcoded PostgreSQL credentials in the Client
instantiation (the const client = new Client({ connectionString:
'postgres://admin:password123@localhost:5432/nexiom_master' })) must be replaced
with using the DATABASE_URL environment variable like the pattern in
packages/database/src/client.ts; update the Client creation to read
process.env.DATABASE_URL (with a sensible fallback or throw if missing) and
remove the plaintext string so credentials are not stored in source control,
ensuring the code still constructs the same Client instance (variable name
client) and behaves the same when DATABASE_URL is present.
In `@packages/database/src/schema/app-credential.ts`:
- Around line 4-23: The schema misses a foreign key on tenantId and relies
solely on an application-level updatedAt hook: add a foreign key constraint from
appCredentials.tenantId to the tenants primary key (e.g., tenants.id) so
deletes/updates cascade or restrict as your tenant lifecycle requires, and
implement a DB-level mechanism for updatedAt (either a DB trigger or a default
GENERATED/ON UPDATE clause) rather than only using the $onUpdate handler on
updatedAt; reference the appCredentials definition, tenantId field,
uniqueIndex('tenant_app_credential_unique_idx'), and updatedAt/$onUpdate when
making the changes.
In `@pnpm-workspace.yaml`:
- Line 4: Remove the trailing blank line at the end of pnpm-workspace.yaml: open
pnpm-workspace.yaml, delete the extra empty line(s) at EOF so the file does not
contain a blank line after the final YAML content (ensure the file ends cleanly
with a single newline if your editor requires it).
---
Outside diff comments:
In `@apps/api/src/modules/connections/connections/token-refresh.service.spec.ts`:
- Around line 50-66: The test for token refresh is mocking a provider via
mockProviderRegistry.getProvider but uses a different provider id when calling
client.refresh('salesforce', ...), which is confusing; update the mock so its
name matches the refresh call (e.g., set name: 'salesforce') or change both to a
generic consistent identifier like 'test_provider' so
mockProviderRegistry.getProvider and the client.refresh('...') argument align;
ensure the assertion message remains the same and uses the same provider
identifier referenced in the mock and the client.refresh call.
In `@apps/api/src/modules/connections/connectors.service.spec.ts`:
- Around line 190-206: The mock return on mockProviderRegistry.getProvider
inside the test "should successfully exchange a code for tokens" uses "as
ProviderResult" which is inconsistent; change that assertion to match the other
tests by casting the object to "as unknown as NonNullable<ProviderResult>" so
the mock type aligns with the other mocks in this spec (refer to
mockProviderRegistry.getProvider.mockReturnValue and the
service.exchangeCodeForTokens test).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (40)
TECHNICAL_DEBT.mdapps/api/src/db/schema.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.tsdocs/architecture/credential_storage_architecture.mdintegrations/quickbooks/eslint.config.mjsintegrations/quickbooks/package.jsonintegrations/quickbooks/src/auth/config.tsintegrations/quickbooks/src/index.tsintegrations/quickbooks/tsconfig.jsonintegrations/salesforce/eslint.config.mjsintegrations/salesforce/package.jsonintegrations/salesforce/src/auth/config.tsintegrations/salesforce/src/index.tsintegrations/salesforce/tsconfig.jsonpackages/connections/src/connectivity/provider-registry.tspackages/connections/src/connectivity/providers/index.tspackages/connections/src/connectivity/providers/quickbooks.provider.tspackages/connections/src/connectivity/providers/salesforce.provider.tspackages/connections/src/connectivity/types.tspackages/database/drizzle/0000_unique_sharon_carter.sqlpackages/database/drizzle/0002_careless_gideon.sqlpackages/database/drizzle/meta/0000_snapshot.jsonpackages/database/drizzle/meta/0001_snapshot.jsonpackages/database/drizzle/meta/0002_snapshot.jsonpackages/database/drizzle/meta/_journal.jsonpackages/database/push-schema-local.tspackages/database/push-schema.tspackages/database/src/client.tspackages/database/src/index.tspackages/database/src/schema/app-credential.tspackages/database/src/schema/provider.tspackages/database/src/schema/tenant.tspnpm-workspace.yaml
💤 Files with no reviewable changes (17)
- integrations/salesforce/src/index.ts
- integrations/quickbooks/src/index.ts
- integrations/quickbooks/src/auth/config.ts
- integrations/quickbooks/eslint.config.mjs
- packages/database/drizzle/0002_careless_gideon.sql
- packages/database/drizzle/meta/0002_snapshot.json
- integrations/salesforce/eslint.config.mjs
- integrations/quickbooks/tsconfig.json
- packages/database/drizzle/meta/_journal.json
- integrations/salesforce/src/auth/config.ts
- packages/database/drizzle/meta/0001_snapshot.json
- integrations/salesforce/tsconfig.json
- packages/database/drizzle/meta/0000_snapshot.json
- packages/database/drizzle/0000_unique_sharon_carter.sql
- integrations/salesforce/package.json
- integrations/quickbooks/package.json
- packages/database/src/schema/provider.ts
Comment on lines
22
to
27
| constructor( | ||
| @Inject('DRIZZLE_DB') private readonly db: DrizzleDb, | ||
| private readonly crypto: EncryptionService, | ||
| private readonly providerRegistry: ProviderRegistryService, | ||
| private readonly configService: ConfigService, | ||
| ) {} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify token-refresh.service.ts still uses env-based credential lookup
rg -n 'process\.env' --type=ts -C3 -g '*token-refresh*'Repository: pramodnarayana/nexiom
Length of output: 1016
🏁 Script executed:
#!/bin/bash
# Find token-refresh.service.ts file
fd -t f 'token-refresh.service.ts' -x cat -n {}Repository: pramodnarayana/nexiom
Length of output: 2571
🏁 Script executed:
#!/bin/bash
# Find connectors.service.ts and examine how it handles credentials
fd -t f 'connectors.service.ts' -x cat -n {}Repository: pramodnarayana/nexiom
Length of output: 8130
Token refresh flow still uses environment variables, breaking BYOA consistency.
The DefaultOAuthRefreshClient in token-refresh.service.ts (lines 28–37) reads clientId and clientSecret from process.env using a normalized name-based pattern (e.g., GOOGLE_CLIENT_ID), while ConnectorsService now fetches all OAuth credentials from the appCredentials database table for each tenant. Token refreshes will fail at runtime for any tenant whose credentials exist only in the database, not in environment variables. This architectural mismatch must be resolved in this PR to maintain the BYOA pattern consistency.
🤖 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 22 - 27,
DefaultOAuthRefreshClient currently reads clientId/clientSecret from process.env
using normalized names, which breaks BYOA because ConnectorsService stores OAuth
credentials in the appCredentials DB; update DefaultOAuthRefreshClient (in
token-refresh.service.ts) to obtain per-tenant clientId and clientSecret from
the same source ConnectorsService uses (e.g., call into ProviderRegistryService
or a new method on ConnectorsService that returns app credentials for a
tenant/app) instead of process.env, and wire that lookup through the token
refresh flow so token refresh uses DB-stored credentials for the given tenant.
Comment on lines
+4
to
+6
| const client = new Client({ | ||
| connectionString: 'postgres://admin:password123@localhost:5432/nexiom_master' | ||
| }); |
Contributor
There was a problem hiding this comment.
Never commit plaintext credentials; use DATABASE_URL env var instead.
The connection string hardcodes admin:password123 directly in source. Even for local dev, this is a credential leak risk once pushed to a repository. Use the same DATABASE_URL pattern already established in packages/database/src/client.ts.
🔒 Proposed fix
+import { config } from 'dotenv';
+config();
+
async function main() {
+ if (!process.env.DATABASE_URL) {
+ throw new Error('DATABASE_URL environment variable is required');
+ }
const client = new Client({
- connectionString: 'postgres://admin:password123@localhost:5432/nexiom_master'
+ connectionString: process.env.DATABASE_URL,
});📝 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 client = new Client({ | |
| connectionString: 'postgres://admin:password123@localhost:5432/nexiom_master' | |
| }); | |
| import { config } from 'dotenv'; | |
| config(); | |
| async function main() { | |
| if (!process.env.DATABASE_URL) { | |
| throw new Error('DATABASE_URL environment variable is required'); | |
| } | |
| const client = new Client({ | |
| connectionString: process.env.DATABASE_URL, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/database/push-schema.ts` around lines 4 - 6, The hardcoded
PostgreSQL credentials in the Client instantiation (the const client = new
Client({ connectionString:
'postgres://admin:password123@localhost:5432/nexiom_master' })) must be replaced
with using the DATABASE_URL environment variable like the pattern in
packages/database/src/client.ts; update the Client creation to read
process.env.DATABASE_URL (with a sensible fallback or throw if missing) and
remove the plaintext string so credentials are not stored in source control,
ensuring the code still constructs the same Client instance (variable name
client) and behaves the same when DATABASE_URL is present.
Comment on lines
+38
to
+45
| try { | ||
| await client.query(query); | ||
| console.log('Successfully created app_credential table and constraints'); | ||
| } catch (e) { | ||
| console.error('Error executing query', e); | ||
| } finally { | ||
| await client.end(); | ||
| } |
Contributor
There was a problem hiding this comment.
Silent error swallowing gives a false exit-zero on DDL failure.
The catch block logs the error but doesn't re-throw it, so main() resolves and the Node process exits with code 0 even when the DDL failed. CI/CD pipelines or manual runs will report success while the schema was never applied.
🐛 Proposed fix
try {
await client.query(query);
console.log('Successfully created app_credential table and constraints');
} catch (e) {
console.error('Error executing query', e);
+ throw e;
} finally {
await client.end();
}📝 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
| try { | |
| await client.query(query); | |
| console.log('Successfully created app_credential table and constraints'); | |
| } catch (e) { | |
| console.error('Error executing query', e); | |
| } finally { | |
| await client.end(); | |
| } | |
| try { | |
| await client.query(query); | |
| console.log('Successfully created app_credential table and constraints'); | |
| } catch (e) { | |
| console.error('Error executing query', e); | |
| throw e; | |
| } finally { | |
| await client.end(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/database/push-schema.ts` around lines 38 - 45, The catch in the
try/catch around client.query(query) logs errors but swallows them, causing
main() to resolve successfully; change the catch in push-schema.ts to re-throw
the caught error (or throw a new Error with context) after logging so the
promise rejects and the process exits non-zero, ensuring the finally still calls
client.end(); reference the client.query(query) call and the surrounding
try/catch in this file when applying the change.
Comment on lines
+4
to
+23
| export const appCredentials = pgTable('app_credential', { | ||
| id: uuid('id').defaultRandom().primaryKey(), | ||
| tenantId: uuid('tenant_id').notNull(), | ||
| appName: varchar('app_name', { length: 100 }).notNull(), | ||
|
|
||
| // The OAuth Client ID | ||
| clientId: text('client_id').notNull(), | ||
|
|
||
| // The OAuth Client Secret (Encrypted via EncryptionService) | ||
| encryptedClientSecret: text('encrypted_client_secret').notNull(), | ||
|
|
||
| // Any extra fields required by the vendor (e.g., custom domains) | ||
| setupMetadata: jsonb('setup_metadata').default({}), | ||
|
|
||
| createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(), | ||
| updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().notNull().$onUpdate(() => new Date()), | ||
| }, (table) => [ | ||
| // A tenant can only have one set of global BYOA credentials per application | ||
| uniqueIndex('tenant_app_credential_unique_idx').on(table.tenantId, table.appName), | ||
| ]); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clean schema definition — well-aligned with the BYOA credential model.
The composite unique index on (tenantId, appName) correctly enforces the one-credential-per-app-per-tenant invariant. Column naming is clear, especially encryptedClientSecret which signals the encryption expectation.
Two observations:
tenantIdhas no foreign key constraint. This means orphaned credentials can exist if a tenant is deleted. This is likely a consequence of the cross-package schema fragmentation documented inTECHNICAL_DEBT.md#3, but should be addressed when schemas are unified.$onUpdate(() => new Date())is application-level only — direct SQL updates (e.g., manual migrations, debugging) won't updateupdated_at. Consider adding a DB-level trigger if you expect any writes outside Drizzle ORM.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/database/src/schema/app-credential.ts` around lines 4 - 23, The
schema misses a foreign key on tenantId and relies solely on an
application-level updatedAt hook: add a foreign key constraint from
appCredentials.tenantId to the tenants primary key (e.g., tenants.id) so
deletes/updates cascade or restrict as your tenant lifecycle requires, and
implement a DB-level mechanism for updatedAt (either a DB trigger or a default
GENERATED/ON UPDATE clause) rather than only using the $onUpdate handler on
updatedAt; reference the appCredentials definition, tenantId field,
uniqueIndex('tenant_app_credential_unique_idx'), and updatedAt/$onUpdate when
making the changes.
pramodnarayana
marked this pull request as draft
February 23, 2026 13:25
pramodnarayana
marked this pull request as ready for review
February 23, 2026 13:25
pramodnarayana
marked this pull request as draft
February 23, 2026 13:25
pramodnarayana
marked this pull request as ready for review
February 23, 2026 13:25
pramodnarayana
marked this pull request as draft
February 23, 2026 13:26
pramodnarayana
marked this pull request as ready for review
February 23, 2026 13:27
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/modules/connections/connectors.service.spec.ts (1)
229-261:⚠️ Potential issue | 🟠 MajorMock providers missing
authType: 'OAUTH2'— tests pass for the wrong reason.Both the "token exchange fails" test (Line 229) and "network/timeout errors" test (Line 246) omit
authTypefrom the mock provider. The service check at Line 155 (provider.authType !== 'OAUTH2') will throwInternalServerErrorExceptionbeforefetchis ever called. The tests pass accidentally because the exception type matches, but the actual error-handling paths for vendor HTTP failures and network errors are not exercised.🐛 Proposed fix — add `authType: 'OAUTH2'` to both mocks
it('should throw InternalServerErrorException if the token exchange fails', async () => { mockProviderRegistry.getProvider.mockReturnValue({ name: 'salesforce', + authType: 'OAUTH2', tokenUrl: 'https://login.salesforce.com/services/oauth2/token', } as unknown as NonNullable<ProviderResult>); ... it('should throw InternalServerErrorException on network/timeout errors', async () => { mockProviderRegistry.getProvider.mockReturnValue({ name: 'salesforce', + authType: 'OAUTH2', tokenUrl: 'https://login.salesforce.com/services/oauth2/token', } as unknown as NonNullable<ProviderResult>);🤖 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.spec.ts` around lines 229 - 261, The two tests that exercise error paths are returning mock providers without authType, causing provider.authType !== 'OAUTH2' to short-circuit in exchangeCodeForTokens; update the mockProviderRegistry.getProvider returns in both "should throw InternalServerErrorException if the token exchange fails" and "should throw InternalServerErrorException on network/timeout errors" to include authType: 'OAUTH2' so the service.exchangeCodeForTokens flow proceeds to call fetch and actually exercises the HTTP error and network-error handling paths.apps/api/src/modules/connections/connections/token-refresh.service.spec.ts (1)
43-44: 🛠️ Refactor suggestion | 🟠 MajorRemove stale environment variable stubs.
QUICKBOOKS_CLIENT_IDandQUICKBOOKS_CLIENT_SECRETare no longer used byDefaultOAuthRefreshClient— credentials are now fetched viaConnectorsService. These stubs are dead test setup from the env-based era and should be removed to avoid confusion.Suggested fix
// Mock the global fetch vi.stubGlobal('fetch', vi.fn()); - - // Stub environment variables for Quickbooks (used in tests) - vi.stubEnv('QUICKBOOKS_CLIENT_ID', 'test_client_id'); - vi.stubEnv('QUICKBOOKS_CLIENT_SECRET', 'test_client_secret');🤖 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.spec.ts` around lines 43 - 44, Remove the stale environment stubs for QUICKBOOKS_CLIENT_ID and QUICKBOOKS_CLIENT_SECRET in the token-refresh.service.spec.ts test setup: these vi.stubEnv calls are dead because DefaultOAuthRefreshClient now obtains credentials from ConnectorsService, so delete the vi.stubEnv('QUICKBOOKS_CLIENT_ID', ...) and vi.stubEnv('QUICKBOOKS_CLIENT_SECRET', ...) lines and ensure tests rely on mocked ConnectorsService behavior (or other relevant test doubles) instead of env vars.apps/api/src/modules/connections/connections/connectors.controller.spec.ts (1)
139-153: 🧹 Nitpick | 🔵 TrivialRemove stale
createdAt/updatedAtfields from mock—not part ofProviderDefinitiontype.The
ProviderDefinitiontype (defined inpackages/connections/src/connectivity/types.ts) comprisesBaseProviderDefinition,OAuth2Provider,ApiKeyProvider, orBasicProviderand does not includecreatedAtorupdatedAtfields. Removing them from the mock clarifies the actual contract and eliminates misleading DB-era artifacts.Suggested cleanup
(mockProviderRegistry.getAllProviders as Mock).mockReturnValue([ { name: 'salesforce', displayName: 'Salesforce', authType: 'OAUTH2', description: 'CRM platform', logoUrl: 'https://logo.com/sf.png', category: 'CRM', scopes: [], uiSchema: {}, authorizeUrl: '', tokenUrl: '', - createdAt: new Date(), - updatedAt: new Date(), }, ]);🤖 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.spec.ts` around lines 139 - 153, The mock returned by (mockProviderRegistry.getAllProviders as Mock).mockReturnValue includes stale createdAt/updatedAt fields not present on the ProviderDefinition type; remove those fields from the mock object (the salesforce provider entry) so the mock shape matches ProviderDefinition (as defined in packages/connections/src/connectivity/types.ts) and only include the actual properties like name, displayName, authType, description, logoUrl, category, scopes, uiSchema, authorizeUrl, tokenUrl, etc.
♻️ Duplicate comments (3)
packages/database/src/schema/app-credential.ts (1)
1-30: Well-structured BYOA credential schema with proper FK constraint.The foreign key from
tenantIdtotenants.idwithonDelete('cascade')correctly addresses data integrity for tenant lifecycle management. The composite unique index on(tenantId, appName)enforces the one-credential-per-app-per-tenant invariant.Note:
$onUpdate(() => new Date())on Line 20 remains application-level only — direct SQL writes won't updateupdated_at. This was flagged in a prior review and is a known trade-off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database/src/schema/app-credential.ts` around lines 1 - 30, The updatedAt column in appCredentials uses .$onUpdate(() => new Date()) which only updates at the application layer; to ensure updated_at is maintained on direct SQL writes, remove the .$onUpdate handler from the updatedAt definition in appCredentials and add a DB migration that creates a PostgreSQL trigger/function to set updated_at = now() on row UPDATE for the app_credential table (or alternatively keep .$onUpdate but also create that trigger); reference the updatedAt field, appCredentials table and the existing app_credential table name when implementing the migration/trigger.apps/api/src/modules/connections/connections/connectors.controller.spec.ts (1)
174-186: Previous review feedback addressed. The error message is now'Registry initialization error'and the test uses a try/catch pattern withexpect.unreachable, which is valid.🤖 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.spec.ts` around lines 174 - 186, The reviewer comment is a duplicate and should be cleared; mark the duplicate review comment as resolved or remove the redundant test-related feedback from the PR discussion. No code change required in getProviders or mockProviderRegistry.getAllProviders — just resolve the duplicate review thread associated with the controller.getProviders test to avoid confusion.apps/api/src/modules/connections/connections/callback.controller.spec.ts (1)
155-158: Previous review feedback addressed. Error message changed from'DB error'to'Unexpected registry error'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/connections/connections/callback.controller.spec.ts` around lines 155 - 158, The test for "should redirect with internal_error if provider lookup fails" currently duplicates an earlier change and hardcodes the thrown Error message; update the spec so it either throws the exact error used elsewhere or, better, does not assert on the exact error string—only on the redirect behavior and presence of internal_error. Modify the mockProviderRegistry.getProvider mock in this test to throw a generic Error (e.g., new Error('provider lookup failed')) or keep the current throw but remove any assertions that check the literal message, ensuring the test only asserts that the controller redirects with internal_error and uses mockProviderRegistry.getProvider and the test name to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/connections/connections/token-refresh.service.spec.ts`:
- Around line 60-78: The provider mock used in token-refresh.service.spec.ts for
the "should throw an error if the provider lacks a tokenUrl" test returns a full
ProviderDefinition shape while other tests use partial casts; make the mocks
consistent by providing a complete ProviderDefinition object for every mocked
provider used by mockProviderRegistry.getProvider (include name, displayName,
description, logoUrl, category, authType, authorizeUrl, tokenUrl, scopes)
instead of using "as unknown as ProviderDefinition" partials so the
token-refresh error path and type-checking are not masked; update all provider
mocks in this spec (and any nearby tests) to the full-shape form.
- Line 36: Replace the unsafe "as any" cast on mockConnectorsService with a
proper ConnectorsService typed cast and add the ConnectorsService import;
specifically, import { ConnectorsService } from '../connectors.service' at the
top of token-refresh.service.spec.ts and change the mock injection of
mockConnectorsService to be cast as ConnectorsService (e.g., via a safe
double-cast such as unknown->ConnectorsService) so the test uses a properly
typed mock for ConnectorsService.
In `@apps/api/src/modules/connections/connections/token-refresh.service.ts`:
- Around line 33-44: The credential retrieval
(connectorsService.fetchAppCredential and connectorsService.decryptClientSecret)
is currently lumped into the same try/catch as the token exchange fetch, making
failures indistinguishable; wrap the credential-fetch/decrypt steps in their own
try/catch inside the token refresh method (e.g., the method in
token-refresh.service) and on error throw or rethrow a descriptive error like
"Failed to retrieve app credential for tenantId/appName: <inner error>" (or log
a clear pre-fetch message) before proceeding to the fetch() token exchange so
upstream callers can tell credential errors apart from token-exchange failures.
In `@packages/connections/src/connectivity/token-manager.service.ts`:
- Around line 158-162: The call to oauthClient.refresh forwards
connection.tenantId with an unsafe cast (connection.tenantId as string) even
though connection is Record<string, any]; guard tenantId the same way
oldPayload.refreshToken is guarded: validate that connection.tenantId exists and
is a string before calling oauthClient.refresh, and if missing throw or return a
clear error (or early-return) so you never pass undefined to
oauthClient.refresh; update the code in token-manager.service.ts around the
oauthClient.refresh call to use the validated tenantId variable instead of the
unsafe cast.
In `@packages/connections/src/connectivity/types.ts`:
- Around line 1-4: The file imports the runtime `db` just to derive `DrizzleDb`,
creating an unnecessary runtime dependency; update it to use a type-only export
from the database package instead — have `@nexiom/database` export a `DrizzleDb`
(or `NodePgDatabase<DbSchema>`) type alias in its client module, then replace
the runtime import of `db` in this file with a type-only import (e.g., `import
type { DrizzleDb } from '@nexiom/database'`) and export `DrizzleDb` from this
module; ensure references to the `db` symbol are removed so no runtime import
remains.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connections/connectors.controller.spec.ts`:
- Around line 139-153: The mock returned by
(mockProviderRegistry.getAllProviders as Mock).mockReturnValue includes stale
createdAt/updatedAt fields not present on the ProviderDefinition type; remove
those fields from the mock object (the salesforce provider entry) so the mock
shape matches ProviderDefinition (as defined in
packages/connections/src/connectivity/types.ts) and only include the actual
properties like name, displayName, authType, description, logoUrl, category,
scopes, uiSchema, authorizeUrl, tokenUrl, etc.
In `@apps/api/src/modules/connections/connections/token-refresh.service.spec.ts`:
- Around line 43-44: Remove the stale environment stubs for QUICKBOOKS_CLIENT_ID
and QUICKBOOKS_CLIENT_SECRET in the token-refresh.service.spec.ts test setup:
these vi.stubEnv calls are dead because DefaultOAuthRefreshClient now obtains
credentials from ConnectorsService, so delete the
vi.stubEnv('QUICKBOOKS_CLIENT_ID', ...) and
vi.stubEnv('QUICKBOOKS_CLIENT_SECRET', ...) lines and ensure tests rely on
mocked ConnectorsService behavior (or other relevant test doubles) instead of
env vars.
In `@apps/api/src/modules/connections/connectors.service.spec.ts`:
- Around line 229-261: The two tests that exercise error paths are returning
mock providers without authType, causing provider.authType !== 'OAUTH2' to
short-circuit in exchangeCodeForTokens; update the
mockProviderRegistry.getProvider returns in both "should throw
InternalServerErrorException if the token exchange fails" and "should throw
InternalServerErrorException on network/timeout errors" to include authType:
'OAUTH2' so the service.exchangeCodeForTokens flow proceeds to call fetch and
actually exercises the HTTP error and network-error handling paths.
---
Duplicate comments:
In `@apps/api/src/modules/connections/connections/callback.controller.spec.ts`:
- Around line 155-158: The test for "should redirect with internal_error if
provider lookup fails" currently duplicates an earlier change and hardcodes the
thrown Error message; update the spec so it either throws the exact error used
elsewhere or, better, does not assert on the exact error string—only on the
redirect behavior and presence of internal_error. Modify the
mockProviderRegistry.getProvider mock in this test to throw a generic Error
(e.g., new Error('provider lookup failed')) or keep the current throw but remove
any assertions that check the literal message, ensuring the test only asserts
that the controller redirects with internal_error and uses
mockProviderRegistry.getProvider and the test name to locate the code to change.
In `@apps/api/src/modules/connections/connections/connectors.controller.spec.ts`:
- Around line 174-186: The reviewer comment is a duplicate and should be
cleared; mark the duplicate review comment as resolved or remove the redundant
test-related feedback from the PR discussion. No code change required in
getProviders or mockProviderRegistry.getAllProviders — just resolve the
duplicate review thread associated with the controller.getProviders test to
avoid confusion.
In `@packages/database/src/schema/app-credential.ts`:
- Around line 1-30: The updatedAt column in appCredentials uses .$onUpdate(() =>
new Date()) which only updates at the application layer; to ensure updated_at is
maintained on direct SQL writes, remove the .$onUpdate handler from the
updatedAt definition in appCredentials and add a DB migration that creates a
PostgreSQL trigger/function to set updated_at = now() on row UPDATE for the
app_credential table (or alternatively keep .$onUpdate but also create that
trigger); reference the updatedAt field, appCredentials table and the existing
app_credential table name when implementing the migration/trigger.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
apps/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/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/connections/src/connectivity/provider-registry.tspackages/connections/src/connectivity/providers/index.tspackages/connections/src/connectivity/providers/quickbooks.provider.tspackages/connections/src/connectivity/token-manager.service.tspackages/connections/src/connectivity/types.tspackages/database/src/index.tspackages/database/src/schema/app-credential.tspackages/database/src/schema/tenant.tspackages/database/src/utils/tenant-guard.tspnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- pnpm-workspace.yaml
pramodnarayana
marked this pull request as draft
February 23, 2026 14:31
pramodnarayana
marked this pull request as ready for review
February 23, 2026 14:32
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/connections/token-refresh.service.spec.ts (2)
41-45:⚠️ Potential issue | 🟡 Minor
vi.unstubAllEnvs()is dead code — no env stubs exist in this suite.The migration away from environment-based credential lookup means
vi.stubEnv()is never called here. TheunstubAllEnvs()call is a leftover from the old approach and should be removed.🧹 Proposed fix
afterEach(() => { vi.restoreAllMocks(); vi.unstubAllGlobals(); - vi.unstubAllEnvs(); });🤖 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.spec.ts` around lines 41 - 45, The afterEach teardown calls vi.unstubAllEnvs(), which is dead code in this test suite because no tests call vi.stubEnv(); remove the vi.unstubAllEnvs() call from the afterEach block so teardown only restores mocks and globals (leave vi.restoreAllMocks() and vi.unstubAllGlobals() intact), updating the afterEach in token-refresh.service.spec.ts accordingly.
74-152: 🧹 Nitpick | 🔵 TrivialAdd a test for the credential retrieval failure path (inner
catchinrefresh).The inner
try/catchintoken-refresh.service.ts(lines 38–54) catchesfetchAppCredential/decryptClientSecretfailures and throws"Failed to retrieve app credential for tenantId/appName: ...". This new code path has no test coverage in this spec. Without it, a bug in credential error wrapping would go undetected.it('should throw an error containing the credential failure message when fetchAppCredential rejects', async () => { (mockProviderRegistry.getProvider as Mock).mockReturnValue({ name: 'quickbooks', displayName: 'QuickBooks', description: 'Accounting', logoUrl: '', category: 'Accounting', authType: 'OAUTH2', authorizeUrl: 'https://appcenter.intuit.com/connect/oauth2', tokenUrl: 'https://oauth.platform.intuit.com/oauth2/v1/tokens/bearer', scopes: ['com.intuit.quickbooks.accounting'], }); mockConnectorsService.fetchAppCredential.mockRejectedValue( new Error('Credential not found'), ); await expect( client.refresh('testTenant', 'quickbooks', 'refresh123'), ).rejects.toThrow('Failed to retrieve app credential for tenantId/appName: Credential not found'); });🤖 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.spec.ts` around lines 74 - 152, Add a unit test for the credential-retrieval failure path in the refresh method: mock provider via mockProviderRegistry.getProvider to return the quickbooks provider, make mockConnectorsService.fetchAppCredential.reject with new Error('Credential not found') (and optionally mock decryptClientSecret if needed), then call client.refresh('testTenant','quickbooks','refresh123') and assert it rejects with an error whose message contains "Failed to retrieve app credential for tenantId/appName: Credential not found" (this exercises the inner try/catch in token-refresh.service.ts around fetchAppCredential/decryptClientSecret).
♻️ Duplicate comments (1)
apps/api/src/modules/connections/connections/connectors.controller.spec.ts (1)
172-184: LGTM — error-path test correctly addresses the previous review.The
try/catch+expect.unreachable()pattern is the idiomatic Vitest approach for asserting typed exceptions, andexpect.unreachablehas been a stable part of the Vitest API since v0.34.5. The stale'DB connection failed'message is replaced with'Registry initialization error', and the double-invocation anti-pattern is gone.🤖 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.spec.ts` around lines 172 - 184, The test in connectors.controller.spec.ts is already correct—no code change required; keep the current try/catch pattern that calls controller.getProviders() and asserts via expect.unreachable and the caught error checks for InternalServerErrorException and the 'Failed to get providers' message, leaving mockProviderRegistry.getAllProviders.mockImplementation(() => { throw new Error('Registry initialization error'); }) as-is; no further edits to the test or controller.getProviders() invocation are needed.
🤖 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/token-refresh.service.ts`:
- Around line 34-49: Move the untyped top-level declaration "let credential;"
into the inner try block and declare it as a const when assigning from
this.connectorsService.fetchAppCredential so it gets a proper inferred type;
keep clientId: string and clientSecret: string in the outer scope, and continue
to assign clientId = credential.clientId and clientSecret = await
this.connectorsService.decryptClientSecret(...) inside that inner try; this
removes the implicit any while preserving the current control flow around
fetchAppCredential, decryptClientSecret, clientId, and clientSecret.
In `@apps/api/src/modules/connections/connectors.service.spec.ts`:
- Around line 199-201: Replace the inline type NonNullable<ReturnType<typeof
mockProviderRegistry.getProvider>> with the existing test alias
NonNullable<ProviderResult> for consistency; locate the object cast where
mockProviderRegistry.getProvider is referenced and change the cast to use
NonNullable<ProviderResult> (keeping the object structure unchanged).
In `@packages/connections/src/connectivity/token-manager.service.ts`:
- Around line 157-159: The current check in token-manager.service.ts only
verifies typeof connection.tenantId === 'string' so empty strings pass and later
cause opaque vendor errors in oauthClient.refresh; update the validation around
connection.tenantId (the block that currently throws TypeError) to also reject
empty or all-whitespace values (e.g., check length or trim()) and throw a clear
TypeError like 'Invalid connection: tenantId is missing, empty or not a string'
so oauthClient.refresh always receives a non-empty tenantId.
In `@packages/connections/src/connectivity/types.ts`:
- Line 1: Remove the DrizzleDb re-export from types.ts (the ProviderDefinition /
provider/auth domain file) and update any consumers (e.g.,
token-manager.service.ts) to import DrizzleDb directly from the `@nexiom/database`
package instead of from ProviderDefinition/types; specifically, delete the
export line "export type { DrizzleDb } from '@nexiom/database';" in
packages/connections/src/connectivity/types.ts and replace any usages that
reference that exported symbol with direct imports from `@nexiom/database` so
ProviderDefinition and related types remain decoupled from the DB client type.
In `@packages/database/src/client.ts`:
- Line 5: schemaBundle currently only spreads tenantSchema so the new
appCredentials table is not included, causing db.query.appCredentials to be
undefined; update the schemaBundle construction to include/import appCredentials
alongside tenantSchema (e.g., merge appCredentials into schemaBundle) so the
DrizzleDb type and db.query.appCredentials are defined, ensuring any references
to appCredentials (db.query.appCredentials.*) resolve at runtime.
In `@packages/database/src/schema/app-credential.ts`:
- Around line 19-20: The updatedAt column (defined as updatedAt:
timestamp('updated_at', { withTimezone: true }).defaultNow().notNull()) only
sets a default on insert and never changes on updates; modify the updatedAt
definition to include an on-update handler (use .$onUpdate(() => new Date()) or
the Drizzle equivalent) so it is refreshed on every row update, leaving
createdAt unchanged; update the timestamp(...) chain for updatedAt to include
.$onUpdate and ensure .defaultNow().notNull() remain.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connections/token-refresh.service.spec.ts`:
- Around line 41-45: The afterEach teardown calls vi.unstubAllEnvs(), which is
dead code in this test suite because no tests call vi.stubEnv(); remove the
vi.unstubAllEnvs() call from the afterEach block so teardown only restores mocks
and globals (leave vi.restoreAllMocks() and vi.unstubAllGlobals() intact),
updating the afterEach in token-refresh.service.spec.ts accordingly.
- Around line 74-152: Add a unit test for the credential-retrieval failure path
in the refresh method: mock provider via mockProviderRegistry.getProvider to
return the quickbooks provider, make
mockConnectorsService.fetchAppCredential.reject with new Error('Credential not
found') (and optionally mock decryptClientSecret if needed), then call
client.refresh('testTenant','quickbooks','refresh123') and assert it rejects
with an error whose message contains "Failed to retrieve app credential for
tenantId/appName: Credential not found" (this exercises the inner try/catch in
token-refresh.service.ts around fetchAppCredential/decryptClientSecret).
---
Duplicate comments:
In `@apps/api/src/modules/connections/connections/connectors.controller.spec.ts`:
- Around line 172-184: The test in connectors.controller.spec.ts is already
correct—no code change required; keep the current try/catch pattern that calls
controller.getProviders() and asserts via expect.unreachable and the caught
error checks for InternalServerErrorException and the 'Failed to get providers'
message, leaving mockProviderRegistry.getAllProviders.mockImplementation(() => {
throw new Error('Registry initialization error'); }) as-is; no further edits to
the test or controller.getProviders() invocation are needed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
apps/api/src/modules/connections/connections/connectors.controller.spec.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.tspackages/connections/src/connectivity/token-manager.service.tspackages/connections/src/connectivity/types.tspackages/database/src/client.tspackages/database/src/schema/app-credential.ts
| options?: Array<{ label: string; value: string }>; | ||
| }>; | ||
| } | ||
| export type { DrizzleDb } from '@nexiom/database'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DrizzleDb re-export is semantically out of place in a provider-types file.
types.ts defines provider/auth domain shapes; proxying a DB client type through it couples every consumer of ProviderDefinition et al. to the database package. Consumers that actually need DrizzleDb (e.g., token-manager.service.ts) should import it directly from @nexiom/database once the type is exported there.
♻️ Proposed fix
-export type { DrizzleDb } from '@nexiom/database';
-
/** Auth types supported by Nexiom providers. */
export type AuthType = 'OAUTH2' | 'API_KEY' | 'BASIC';In token-manager.service.ts (and any other consumer of DrizzleDb):
-import { DrizzleDb } from './types';
+import type { DrizzleDb } from '@nexiom/database';📝 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
| export type { DrizzleDb } from '@nexiom/database'; | |
| /** Auth types supported by Nexiom providers. */ | |
| export type AuthType = 'OAUTH2' | 'API_KEY' | 'BASIC'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/connections/src/connectivity/types.ts` at line 1, Remove the
DrizzleDb re-export from types.ts (the ProviderDefinition / provider/auth domain
file) and update any consumers (e.g., token-manager.service.ts) to import
DrizzleDb directly from the `@nexiom/database` package instead of from
ProviderDefinition/types; specifically, delete the export line "export type {
DrizzleDb } from '@nexiom/database';" in
packages/connections/src/connectivity/types.ts and replace any usages that
reference that exported symbol with direct imports from `@nexiom/database` so
ProviderDefinition and related types remain decoupled from the DB client type.
Comment on lines
+19
to
+20
| createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(), | ||
| updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().notNull(), |
Contributor
There was a problem hiding this comment.
updatedAt will never be updated — $onUpdate is missing.
updatedAt only has .defaultNow(), which sets the value at insert time. Without .$onUpdate(() => new Date()), Drizzle will never overwrite the column on subsequent updates, so updatedAt will permanently equal createdAt. The AI-generated summary incorrectly states that this column "auto-updates on row update."
🐛 Proposed fix
- updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().notNull(),
+ updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().$onUpdate(() => new Date()).notNull(),📝 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
| createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(), | |
| updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().notNull(), | |
| createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(), | |
| updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().$onUpdate(() => new Date()).notNull(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/database/src/schema/app-credential.ts` around lines 19 - 20, The
updatedAt column (defined as updatedAt: timestamp('updated_at', { withTimezone:
true }).defaultNow().notNull()) only sets a default on insert and never changes
on updates; modify the updatedAt definition to include an on-update handler (use
.$onUpdate(() => new Date()) or the Drizzle equivalent) so it is refreshed on
every row update, leaving createdAt unchanged; update the timestamp(...) chain
for updatedAt to include .$onUpdate and ensure .defaultNow().notNull() remain.
pramodnarayana
marked this pull request as draft
February 23, 2026 15:00
pramodnarayana
marked this pull request as ready for review
February 23, 2026 15:01
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/modules/connections/connectors.service.spec.ts (1)
194-225: 🧹 Nitpick | 🔵 TrivialAdd assertions to verify credential decryption is wired correctly.
This test validates the token exchange response and the fetch call structure, but doesn't verify the new BYOA credential flow — specifically that
decryptwas called with the encrypted secret from the DB, and that the decryptedclient_secretandclient_idare included in the request body. Without these, a regression in credential fetching/decryption could silently pass.♻️ Suggested additions after line 213
expect(result).toEqual(mockTokens); + expect(mockEncryptionService.decrypt).toHaveBeenCalledWith( + 'encrypted-secret', + ); expect(fetch).toHaveBeenCalledWith( 'https://login.salesforce.com/services/oauth2/token', { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, - // eslint-disable-next-line `@typescript-eslint/no-unsafe-assignment` - body: expect.stringContaining('grant_type=authorization_code'), + body: expect.stringContaining('client_secret=test-client-secret'), // eslint-disable-next-line `@typescript-eslint/no-unsafe-assignment` signal: expect.any(AbortSignal), }, );🤖 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.spec.ts` around lines 194 - 225, The test needs assertions that the BYOA credential decryption is invoked and its plaintext results are sent in the token request: in the existing "should successfully exchange a code for tokens" test, mock the credentials lookup used by exchangeCodeForTokens (the same credentials repo/mock the service uses) to return an object containing the encrypted secret, spy/mock the decrypt function (decrypt) to return a decrypted client_id/client_secret, then assert decrypt was called with the stored encrypted secret and assert the fetch body (the POST body used by exchangeCodeForTokens) contains the decrypted client_id and client_secret (e.g., expect.stringContaining('client_id=...') and expect.stringContaining('client_secret=...')); keep the rest of the test unchanged and use the same identifiers exchangeCodeForTokens, mockProviderRegistry.getProvider, fetch, and decrypt to locate where to add these assertions.packages/database/src/client.ts (1)
38-42: 🧹 Nitpick | 🔵 TrivialProxy type is
NodePgDatabase<DbSchema>whilegetDb()returnsDrizzleDb.The proxy is cast as
NodePgDatabase<DbSchema>, butgetDb()returnsDrizzleDb(which isReturnType<typeof drizzle<DbSchema>>). While these are likely compatible at runtime with the node-postgres driver, usingDrizzleDbconsistently avoids subtle type drift if thedrizzlefunction's return type changes across versions.♻️ Suggested consistency fix
-export const db = new Proxy({} as NodePgDatabase<DbSchema>, { - get(_target, prop) { - return getDb()[prop as keyof NodePgDatabase<DbSchema>]; - } +export const db = new Proxy({} as DrizzleDb, { + get(_target, prop) { + return getDb()[prop as keyof DrizzleDb]; + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database/src/client.ts` around lines 38 - 42, The proxy is currently cast as NodePgDatabase<DbSchema> but getDb() returns a DrizzleDb (ReturnType<typeof drizzle<DbSchema>>), so change the proxy's declared type to the DrizzleDb return type to keep types consistent: update the export const db = new Proxy(...) cast to the DrizzleDb type (or a local type alias like DrizzleDb = ReturnType<typeof drizzle<DbSchema>>) and ensure any necessary drizzle import/type alias is added so the proxy signature matches getDb()'s actual return type rather than NodePgDatabase<DbSchema>.apps/api/src/modules/connections/connections/token-refresh.service.ts (1)
67-73:⚠️ Potential issue | 🔴 CriticalImport and throw
OAuthRefreshErrorto enable automatic REVOKED status marking.Lines 67-73 in
token-refresh.service.tsthrow a plainErrorwith a.statusproperty instead ofOAuthRefreshError. ThehandleRefreshErrormethod intoken-manager.service.ts(line 196) checkserror instanceof OAuthRefreshErrorto decide whether to mark connections asREVOKEDon 400/401 responses. Since a plainErrorwill never pass thisinstanceofcheck, connections with revoked vendor tokens are never automatically markedREVOKED.Replace the plain
ErrorwithOAuthRefreshError:🐛 Proposed fix
+import { + OAuthRefreshClient, + OAuthRefreshError, + ProviderRegistryService, +} from '@nexiom/connections'; -import { - OAuthRefreshClient, - ProviderRegistryService, -} from '@nexiom/connections'; if (!response.ok) { - const err = new Error( + const err = new OAuthRefreshError( `OAuth Refresh failed: ${response.status} ${response.statusText || ''}`.trim(), - ) as Error & { status: number }; - err.status = response.status; + response.status, + ); throw err; }🤖 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 67 - 73, Replace the plain Error thrown in token-refresh.service.ts with the OAuthRefreshError so handleRefreshError can detect revoked tokens; import OAuthRefreshError at the top of the file, construct an OAuthRefreshError instance (preserving the message built from response.status and response.statusText), set its status property to response.status, and throw that OAuthRefreshError instead of the plain Error in the block that currently creates err and throws it.
🤖 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/token-refresh.service.spec.ts`:
- Around line 163-167: The test mock in token-refresh.service.spec.ts provides a
text() method but the production code (token-refresh.service.ts) reads
response.statusText; update the mocked fetch response (the Mock assigned to
globalThis.fetch in the spec) to include statusText: 'Unauthorized' (or the
appropriate string) instead of the unused text() function so the mock accurately
reflects the shape the service reads (locate the mock where (globalThis.fetch as
Mock).mockResolvedValue is used).
In `@apps/api/src/modules/connections/connectors.service.spec.ts`:
- Around line 185-187: The test currently overrides
mockEncryptionService.decrypt with a synchronous throw via mockImplementation,
but decrypt is async; change that override to use mockRejectedValue with an
Error('decryption failed') so the mock represents an async rejection; locate the
override in connectors.service.spec.ts where
mockEncryptionService.decrypt.mockImplementation(...) is set and replace it with
mockRejectedValue to align with the beforeEach mockResolvedValue and async
behavior.
- Around line 38-48: The test currently defines mockDbWhere and mockDb but never
asserts the arguments passed to the where call, so add an assertion in the
success tests to verify the DB query is scoped to the correct tenant and
provider; specifically, after exercising the service (e.g., calling the method
that uses mockDb.select().from().where()), inspect mockDbWhere.mock.calls and
assert it was called with the expected tenantId and provider name (or the
Drizzle eq(...) expression shape your service constructs), referencing
mockDbWhere and mockDb.where to ensure tenant isolation is enforced in the spec.
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 55-61: fetchAppCredential currently throws a NestJS
NotFoundException which leaks HTTP-specific exceptions into background callers
like DefaultOAuthRefreshClient used by token-refresh.service.ts; change
fetchAppCredential to return null when no credential is found (or throw a new
domain-specific error class, e.g., AppCredentialNotFoundError) instead of
NotFoundException, and update callers
(DefaultOAuthRefreshClient/token-refresh.service) to detect null or catch the
domain error and translate it into the appropriate error handling for their
context; ensure the method signature and any call sites are adjusted to handle a
null return or to catch the new domain-specific error.
In `@packages/database/src/client.ts`:
- Line 10: Remove the unused top-level variable tempDbInstance (declared as let
tempDbInstance: ReturnType<typeof drizzle> | undefined;) from the file—it's a
leftover from refactoring and not referenced anywhere; simply delete that
declaration and run the build/typecheck to ensure no remaining references to
tempDbInstance or unintended impacts on functions that use drizzle.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connections/token-refresh.service.ts`:
- Around line 67-73: Replace the plain Error thrown in token-refresh.service.ts
with the OAuthRefreshError so handleRefreshError can detect revoked tokens;
import OAuthRefreshError at the top of the file, construct an OAuthRefreshError
instance (preserving the message built from response.status and
response.statusText), set its status property to response.status, and throw that
OAuthRefreshError instead of the plain Error in the block that currently creates
err and throws it.
In `@apps/api/src/modules/connections/connectors.service.spec.ts`:
- Around line 194-225: The test needs assertions that the BYOA credential
decryption is invoked and its plaintext results are sent in the token request:
in the existing "should successfully exchange a code for tokens" test, mock the
credentials lookup used by exchangeCodeForTokens (the same credentials repo/mock
the service uses) to return an object containing the encrypted secret, spy/mock
the decrypt function (decrypt) to return a decrypted client_id/client_secret,
then assert decrypt was called with the stored encrypted secret and assert the
fetch body (the POST body used by exchangeCodeForTokens) contains the decrypted
client_id and client_secret (e.g., expect.stringContaining('client_id=...') and
expect.stringContaining('client_secret=...')); keep the rest of the test
unchanged and use the same identifiers exchangeCodeForTokens,
mockProviderRegistry.getProvider, fetch, and decrypt to locate where to add
these assertions.
In `@packages/database/src/client.ts`:
- Around line 38-42: The proxy is currently cast as NodePgDatabase<DbSchema> but
getDb() returns a DrizzleDb (ReturnType<typeof drizzle<DbSchema>>), so change
the proxy's declared type to the DrizzleDb return type to keep types consistent:
update the export const db = new Proxy(...) cast to the DrizzleDb type (or a
local type alias like DrizzleDb = ReturnType<typeof drizzle<DbSchema>>) and
ensure any necessary drizzle import/type alias is added so the proxy signature
matches getDb()'s actual return type rather than NodePgDatabase<DbSchema>.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/modules/connections/connections/callback.controller.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.tspackages/connections/src/connectivity/token-manager.service.tspackages/connections/src/connectivity/types.tspackages/database/src/client.ts
Comment on lines
+55
to
+61
| if (!credential) { | ||
| this.logger.error( | ||
| `Missing OAuth app credential for ${providerName} on tenant ${tenantId}`, | ||
| ); | ||
| throw new NotFoundException( | ||
| `Platform administrator has not configured ${providerName} integration.`, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
NotFoundException leaks into non-HTTP callers.
fetchAppCredential is a public method also called by DefaultOAuthRefreshClient via token-refresh.service.ts, which runs in the token-manager context (a background/worker path, not an HTTP request). Throwing a NestJS NotFoundException from there is semantically incorrect for non-controller flows. The inner catch in token-refresh.service.ts (lines 49–53) wraps it into a generic Error, which mitigates the issue, but the coupling is fragile — any new caller would need the same wrapping.
Consider returning null from the helper and letting each caller decide the appropriate error type, or having the helper throw a domain-specific error that callers can translate.
🤖 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 55 - 61,
fetchAppCredential currently throws a NestJS NotFoundException which leaks
HTTP-specific exceptions into background callers like DefaultOAuthRefreshClient
used by token-refresh.service.ts; change fetchAppCredential to return null when
no credential is found (or throw a new domain-specific error class, e.g.,
AppCredentialNotFoundError) instead of NotFoundException, and update callers
(DefaultOAuthRefreshClient/token-refresh.service) to detect null or catch the
domain error and translate it into the appropriate error handling for their
context; ensure the method signature and any call sites are adjusted to handle a
null return or to catch the new domain-specific error.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)apps/api/src/modules/connections/connectors.service.spec.ts (1)
81-145: 🧹 Nitpick | 🔵 TrivialMissing test coverage for
InternalServerErrorExceptionwhen provider isn't OAuth2.
getAuthorizationUrlthrowsInternalServerErrorExceptionwhenprovider.authType !== 'OAUTH2' || !provider.authorizeUrl(line 103 of the service). There's no test case covering this path. Similarly, the same gap exists forexchangeCodeForTokens(service line 162).🧪 Suggested test additions
it('should throw InternalServerErrorException if provider is not OAUTH2 or missing authorizeUrl', async () => { mockProviderRegistry.getProvider.mockReturnValue({ name: 'some-provider', authType: 'API_KEY', } as unknown as NonNullable<ProviderResult>); await expect( service.getAuthorizationUrl('some-provider', 'state', testTenantId), ).rejects.toThrow(InternalServerErrorException); });Add an equivalent test in the
exchangeCodeForTokensblock for the!provider.tokenUrlbranch.🤖 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.spec.ts` around lines 81 - 145, Add unit tests covering the InternalServerErrorException paths: call service.getAuthorizationUrl with a provider from mockProviderRegistry whose authType !== 'OAUTH2' (e.g., 'API_KEY') or with missing authorizeUrl and assert it rejects with InternalServerErrorException; likewise add a test in the exchangeCodeForTokens suite that returns a provider lacking OAUTH2 or missing tokenUrl and assert exchangeCodeForTokens throws InternalServerErrorException. Reference getAuthorizationUrl, exchangeCodeForTokens, and provider.authorizeUrl/provider.tokenUrl/provider.authType when locating where to add these tests.apps/api/src/modules/connections/connections/token-refresh.service.spec.ts (1)
73-116: 🧹 Nitpick | 🔵 TrivialMissing test for
decryptClientSecretrejection path in therefreshflow.The test at lines 118–140 covers
fetchAppCredentialrejection. But whenfetchAppCredentialsucceeds anddecryptClientSecretsubsequently rejects, the innertry/catchintoken-refresh.service.ts(lines 54–57) also catches it and re-throws as"Failed to retrieve app credential for tenantId/appName: ...". This code path has no dedicated spec coverage.🧪 Suggested test addition
it('should throw an error if decryptClientSecret fails', async () => { (mockProviderRegistry.getProvider as Mock).mockReturnValue({ name: 'quickbooks', displayName: 'QuickBooks', description: 'Accounting', logoUrl: '', category: 'Accounting', authType: 'OAUTH2', authorizeUrl: 'https://appcenter.intuit.com/connect/oauth2', tokenUrl: 'https://oauth.url', scopes: ['com.intuit.quickbooks.accounting'], }); mockConnectorsService.fetchAppCredential.mockResolvedValue({ clientId: 'mock-client-id', encryptedClientSecret: 'mock-encrypted-secret', }); mockConnectorsService.decryptClientSecret.mockRejectedValue( new Error('decryption failed'), ); await expect( client.refresh('testTenant', 'quickbooks', 'refresh123'), ).rejects.toThrow( 'Failed to retrieve app credential for tenantId/appName: decryption failed', ); });🤖 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.spec.ts` around lines 73 - 116, Add a unit test covering the decryptClientSecret rejection path in the token-refresh service: call client.refresh with a provider (e.g., quickbooks) while mocking fetchAppCredential to resolve a credential and mocking mockConnectorsService.decryptClientSecret to reject (e.g., new Error('decryption failed')), then assert that client.refresh rejects with the same wrapped error message produced by the refresh method ("Failed to retrieve app credential for tenantId/appName: decryption failed"); reference the refresh method in token-refresh.service.ts and the mocks mockConnectorsService.fetchAppCredential and mockConnectorsService.decryptClientSecret to locate where to add this spec.
♻️ Duplicate comments (2)
apps/api/src/modules/connections/connectors.service.ts (1)
65-80: 🧹 Nitpick | 🔵 Trivial
decryptClientSecretstill throws an HTTP-specific exception from a shared public method.
InternalServerErrorExceptionis a NestJS HTTP concern. Thetoken-refresh.service(non-HTTP path) mitigates this by wrapping the call in atry/catchthat re-throws as a plainError, but any future caller of this public method must know to do the same wrapping — a latent coupling risk. Consider throwing a domain-specific error (e.g.,AppCredentialError) instead, letting each caller map it to the appropriate HTTP or non-HTTP 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 65 - 80, The decryptClientSecret method currently throws an HTTP-specific InternalServerErrorException from a shared service; change it to throw a domain-specific error (e.g., AppCredentialError) instead so callers can map to HTTP or non-HTTP concerns. Update decryptClientSecret (and its catch) to log the failure via this.logger.error (including providerName and tenantId) and then throw new AppCredentialError with a clear message; create the AppCredentialError class (or reuse an existing domain error) and ensure token-refresh.service and any HTTP controllers map AppCredentialError to the appropriate HTTP exception or plain Error as needed. Keep crypto.decrypt usage unchanged and only replace the thrown exception type and add/ensure the domain error class is exported for callers to catch.apps/api/src/modules/connections/connectors.service.spec.ts (1)
117-123: 🧹 Nitpick | 🔵 TrivialTenant isolation assertion only verifies column name, not the actual value.
expect(safeStringify(whereArg)).toContain('tenant_id')confirms the column is referenced but not that the correcttestTenantIdvalue is bound. A credential query using the wrong tenant ID would still pass this assertion.🔍 Suggested stronger assertion
expect(safeStringify(whereArg)).toContain('tenant_id'); +expect(safeStringify(whereArg)).toContain(testTenantId); +expect(safeStringify(whereArg)).toContain('salesforce');The same pattern should be applied in the
exchangeCodeForTokenssuccess test (lines 226–227).🤖 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.spec.ts` around lines 117 - 123, The current assertion only checks that the where clause references 'tenant_id' (expect(safeStringify(whereArg)).toContain('tenant_id')) but not that it binds the expected testTenantId; update the assertion on mockDbWhere/mockDbWhere.mock.calls[0][0] (whereArg) to assert that the actual tenant value/testTenantId is present (e.g., inspect the object or its JSON via safeStringify toContain testTenantId or assert whereArg.tenant_id === testTenantId) and apply the same stronger assertion pattern in the exchangeCodeForTokens success test where mockDbWhere is used.
🤖 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/token-refresh.service.ts`:
- Line 6: Replace the internal dist import of OAuthRefreshError with the package
public entry: locate the import in token-refresh.service.ts that currently reads
from '@nexiom/connections/dist/connectivity/token-manager.service' and change it
to import { OAuthRefreshError } from '@nexiom/connections' so the code uses the
package's public API (referencing the OAuthRefreshError symbol).
- Around line 73-76: Replace the two-step creation and cast of err with a single
constructor call that passes the HTTP status directly: instead of creating
OAuthRefreshError with only the message and then setting err.status, call new
OAuthRefreshError(message, response.status) so the status parameter
(response.status) is provided to the OAuthRefreshError constructor; remove the
unnecessary type cast and the subsequent err.status assignment (references:
OAuthRefreshError, err, response.status, response.statusText in
token-refresh.service.ts).
---
Outside diff comments:
In `@apps/api/src/modules/connections/connections/token-refresh.service.spec.ts`:
- Around line 73-116: Add a unit test covering the decryptClientSecret rejection
path in the token-refresh service: call client.refresh with a provider (e.g.,
quickbooks) while mocking fetchAppCredential to resolve a credential and mocking
mockConnectorsService.decryptClientSecret to reject (e.g., new Error('decryption
failed')), then assert that client.refresh rejects with the same wrapped error
message produced by the refresh method ("Failed to retrieve app credential for
tenantId/appName: decryption failed"); reference the refresh method in
token-refresh.service.ts and the mocks mockConnectorsService.fetchAppCredential
and mockConnectorsService.decryptClientSecret to locate where to add this spec.
In `@apps/api/src/modules/connections/connectors.service.spec.ts`:
- Around line 81-145: Add unit tests covering the InternalServerErrorException
paths: call service.getAuthorizationUrl with a provider from
mockProviderRegistry whose authType !== 'OAUTH2' (e.g., 'API_KEY') or with
missing authorizeUrl and assert it rejects with InternalServerErrorException;
likewise add a test in the exchangeCodeForTokens suite that returns a provider
lacking OAUTH2 or missing tokenUrl and assert exchangeCodeForTokens throws
InternalServerErrorException. Reference getAuthorizationUrl,
exchangeCodeForTokens, and
provider.authorizeUrl/provider.tokenUrl/provider.authType when locating where to
add these tests.
---
Duplicate comments:
In `@apps/api/src/modules/connections/connectors.service.spec.ts`:
- Around line 117-123: The current assertion only checks that the where clause
references 'tenant_id' (expect(safeStringify(whereArg)).toContain('tenant_id'))
but not that it binds the expected testTenantId; update the assertion on
mockDbWhere/mockDbWhere.mock.calls[0][0] (whereArg) to assert that the actual
tenant value/testTenantId is present (e.g., inspect the object or its JSON via
safeStringify toContain testTenantId or assert whereArg.tenant_id ===
testTenantId) and apply the same stronger assertion pattern in the
exchangeCodeForTokens success test where mockDbWhere is used.
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 65-80: The decryptClientSecret method currently throws an
HTTP-specific InternalServerErrorException from a shared service; change it to
throw a domain-specific error (e.g., AppCredentialError) instead so callers can
map to HTTP or non-HTTP concerns. Update decryptClientSecret (and its catch) to
log the failure via this.logger.error (including providerName and tenantId) and
then throw new AppCredentialError with a clear message; create the
AppCredentialError class (or reuse an existing domain error) and ensure
token-refresh.service and any HTTP controllers map AppCredentialError to the
appropriate HTTP exception or plain Error as needed. Keep crypto.decrypt usage
unchanged and only replace the thrown exception type and add/ensure the domain
error class is exported for callers to catch.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
apps/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/database/src/client.ts
| OAuthRefreshClient, | ||
| ProviderRegistryService, | ||
| } from '@nexiom/connections'; | ||
| import { OAuthRefreshError } from '@nexiom/connections/dist/connectivity/token-manager.service'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if OAuthRefreshError is exported from `@nexiom/connections` main entry
# and whether the package.json exports field exists
# Test 1: Find and check the main index for OAuthRefreshError export
echo "=== Checking packages/connections/src/index.ts ==="
fd -t f 'index.ts' packages/connections/src -x cat -n {} 2>/dev/null || echo "No index.ts found"
echo ""
echo "=== Checking packages/connections/package.json ==="
fd -t f 'package.json' packages/connections -max-depth 1 -x cat {} 2>/dev/null || echo "No package.json found"Repository: pramodnarayana/nexiom
Length of output: 1381
🏁 Script executed:
#!/bin/bash
# Check the directory structure and package.json location
echo "=== Directory structure of packages/connections ==="
ls -la packages/connections/ 2>/dev/null || echo "Directory not found"
echo ""
echo "=== Find all index.ts files in packages/connections ==="
find packages/connections -name 'index.ts' -type f
echo ""
echo "=== Check package.json location ==="
find packages/connections -name 'package.json' -type f
echo ""
echo "=== Check if OAuthRefreshError is exported from token-manager.service.js ==="
fd -t f 'token-manager.service.ts' packages/connections -x grep -n 'export.*OAuthRefreshError' {}Repository: pramodnarayana/nexiom
Length of output: 880
🏁 Script executed:
#!/bin/bash
# Get the full content of the main index.ts
echo "=== packages/connections/src/index.ts ==="
cat -n packages/connections/src/index.ts
echo ""
echo "=== packages/connections/package.json ==="
cat packages/connections/package.json
echo ""
echo "=== Check token-manager.service.ts for OAuthRefreshError export ==="
grep -n 'export.*class OAuthRefreshError' packages/connections/src/connectivity/token-manager.service.ts
</parameter>Repository: pramodnarayana/nexiom
Length of output: 1291
🏁 Script executed:
#!/bin/bash
# Verify that OAuthRefreshError can be imported from `@nexiom/connections`
echo "=== Checking if OAuthRefreshError is in token-manager.service.ts export ==="
cat -n packages/connections/src/connectivity/token-manager.service.ts | head -30Repository: pramodnarayana/nexiom
Length of output: 1398
Import OAuthRefreshError from the package's main entry instead of the compiled dist output path.
OAuthRefreshError is imported from @nexiom/connections/dist/connectivity/token-manager.service instead of from @nexiom/connections. While OAuthRefreshError is re-exported from the package's main entry via the index.ts file, importing directly from dist output paths should be avoided: it creates a dependency on the internal build structure, and any future restructuring of the package's output layout could break this import.
Change the import to:
import { OAuthRefreshError } from '@nexiom/connections';This uses the intended public API and ensures resilience to internal build changes.
🤖 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` at
line 6, Replace the internal dist import of OAuthRefreshError with the package
public entry: locate the import in token-refresh.service.ts that currently reads
from '@nexiom/connections/dist/connectivity/token-manager.service' and change it
to import { OAuthRefreshError } from '@nexiom/connections' so the code uses the
package's public API (referencing the OAuthRefreshError symbol).
Comment on lines
73
to
76
| const err = new OAuthRefreshError( | ||
| `OAuth Refresh failed: ${response.status} ${response.statusText || ''}`.trim(), | ||
| ) as Error & { status: number }; | ||
| ) as OAuthRefreshError & { status: number }; | ||
| err.status = response.status; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
OAuthRefreshError already accepts status in its constructor — the cast and post-creation assignment are unnecessary.
The class is defined as constructor(message: string, public status?: number), so status can be passed directly as the second argument:
♻️ Proposed simplification
- const err = new OAuthRefreshError(
- `OAuth Refresh failed: ${response.status} ${response.statusText || ''}`.trim(),
- ) as OAuthRefreshError & { status: number };
- err.status = response.status;
- throw err;
+ throw new OAuthRefreshError(
+ `OAuth Refresh failed: ${response.status} ${response.statusText || ''}`.trim(),
+ response.status,
+ );🤖 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 73 - 76, Replace the two-step creation and cast of err with a single
constructor call that passes the HTTP status directly: instead of creating
OAuthRefreshError with only the message and then setting err.status, call new
OAuthRefreshError(message, response.status) so the status parameter
(response.status) is provided to the OAuthRefreshError constructor; remove the
unnecessary type cast and the subsequent err.status assignment (references:
OAuthRefreshError, err, response.status, response.statusText in
token-refresh.service.ts).
pramodnarayana
marked this pull request as draft
February 23, 2026 16:45
pramodnarayana
marked this pull request as ready for review
February 23, 2026 16:45
This was referenced Feb 25, 2026
This was referenced Mar 5, 2026
This was referenced Mar 16, 2026
Merged
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
Architecture & Infrastructure
Removals