Dynamic schema migration - #170
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 33 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces a new ChangesPlugin pipeline hooks refactor + migrator package + canonical type explorer
Sequence Diagram(s)sequenceDiagram
participant PluginHotReloader as PluginHotReloaderService
participant QueueService as IQueueService
participant MigrationWorker as MigrationWorkerService
participant TenantDB as TenantDatabase
participant MigrationRunner as MigrationRunnerPort
PluginHotReloader->>PluginHotReloader: hotLoadPiece(packageName)
PluginHotReloader->>PluginHotReloader: check piece.migrationsFolder
alt piece.migrationsFolder exists
PluginHotReloader->>QueueService: send(TenantProvisionQueue, event)
QueueService-->>MigrationWorker: PluginMigrationEvent
MigrationWorker->>TenantDB: connect per tenant
MigrationWorker->>MigrationRunner: runMigrations(tenantDb, migrationsFolder)
MigrationRunner->>TenantDB: pg_advisory_lock(schemaHash)
MigrationRunner->>TenantDB: read _journal.json, query applied hashes
MigrationRunner->>TenantDB: execute pending .sql migrations
MigrationRunner->>TenantDB: insert hashes into __drizzle_migrations
MigrationRunner->>TenantDB: pg_advisory_unlock(schemaHash)
MigrationRunner-->>MigrationWorker: complete
end
sequenceDiagram
participant Controller as ConnectionExplorerController
participant ListTypesUC as ListNormalizedTypesUseCase
participant ListDataUC as ListConnectionDataUseCase
participant ExplorerRepo as DrizzleExplorerRepositoryAdapter
participant DB as TenantDatabase
Controller->>ListTypesUC: execute(connectionId, objectType)
ListTypesUC->>ExplorerRepo: listNormalizedTypes(tenantId, schema, connectionId, objectType)
ExplorerRepo->>DB: SELECT DISTINCT canonicalType<br/>FROM normalized_entity<br/>JOIN replica WHERE connectionId AND objectType
DB-->>ExplorerRepo: string[]
ExplorerRepo-->>ListTypesUC: string[]
ListTypesUC-->>Controller: string[]
Controller->>ListDataUC: execute(..., tab=normalized, canonicalType)
ListDataUC->>ExplorerRepo: listConnectionNormalized(..., canonicalType)
ExplorerRepo->>ExplorerRepo: resolveTableName(canonicalType)
alt polymorphic or ALL
ExplorerRepo->>DB: queryPolymorphicNormalizedEntities(...)
else canonical type specified
ExplorerRepo->>DB: queryCanonicalTable(tableName, ...)
end
DB-->>ExplorerRepo: ExplorerPage<Record<string, unknown>>
ExplorerRepo-->>ListDataUC: ExplorerPage
ListDataUC-->>Controller: ExplorerPage
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 18
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/trace/adapters/outbound/drizzle-explorer.adapter.ts (1)
589-599:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalized object-type source breaks canonical-type discovery flow.
Line 589 switches normalized tab object values to
normalizedEntity.canonicalType, butlistNormalizedTypes(...)resolves canonical types by filteringreplicaEntity.entityType = objectType. That makes the next/normalized/types?objectType=...request use the wrong key and can return empty results.Suggested fix
normalized: async (tenantDb, schema, connectionId) => { const rows = await tenantDb - .selectDistinct({ type: schema.normalizedEntity.canonicalType }) - .from(schema.normalizedEntity) - .innerJoin( - schema.replicaEntity, - eq(schema.normalizedEntity.replicaId, schema.replicaEntity.id), - ) - .where(eq(schema.replicaEntity.dataSourceId, connectionId)); - return rows.map((r) => (r.type ? r.type.toLowerCase() : 'Uncategorized')); + .selectDistinct({ type: schema.replicaEntity.entityType }) + .from(schema.replicaEntity) + .innerJoin( + schema.normalizedEntity, + eq(schema.normalizedEntity.replicaId, schema.replicaEntity.id), + ) + .where(eq(schema.replicaEntity.dataSourceId, connectionId)); + return rows.map((r) => r.type ?? 'Uncategorized'); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/trace/adapters/outbound/drizzle-explorer.adapter.ts` around lines 589 - 599, The normalized async function is selecting canonicalType from normalizedEntity, but the downstream listNormalizedTypes function filters results using replicaEntity.entityType. This mismatch causes the canonical type discovery flow to fail. Change the selectDistinct field from schema.normalizedEntity.canonicalType to schema.replicaEntity.entityType in the normalized function so that the returned object type values match the field that listNormalizedTypes uses for filtering when making subsequent /normalized/types?objectType=... requests.packages/pipeline/src/shared/fakes/normalization-repository.fake.ts (1)
6-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid hardcoded
objectTypein fake repository responses.Line 22 always returning
'DEFAULT'can mask objectType-sensitive behavior and make tests pass when production would receivenullor a real value.Proposed fix
- public inboundRequests: { schemaName: string; traceId: string; payload: any }[] = []; + public inboundRequests: { + schemaName: string; + traceId: string; + payload: Record<string, unknown>; + objectType?: string | null; + }[] = []; async fetchInboundRequest(schemaName: string, traceId: string, tx: TxContext): Promise<{ request: Record<string, unknown>; objectType?: string | null } | null> { const req = this.inboundRequests.find(r => r.schemaName === schemaName && r.traceId === traceId); - return req ? { request: req.payload, objectType: 'DEFAULT' } : null; + return req ? { request: req.payload, objectType: req.objectType ?? null } : null; }Also applies to: 20-22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pipeline/src/shared/fakes/normalization-repository.fake.ts` around lines 6 - 7, The NormalizationRepositoryFake class has a method that always returns a hardcoded 'DEFAULT' value for the objectType field, which prevents tests from detecting issues where production code might receive null or different values. Locate the method returning responses (around line 20-22) that builds the fake repository response and replace the hardcoded 'DEFAULT' objectType with either a configurable property that can be set per test scenario, or derive it from the input parameters being passed to the method so different tests can verify behavior with different objectType values.packages/dbmanager/src/impl/sql-database-manager.ts (1)
443-453:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd idempotent backfill for
normalized_entity.updated_aton existing schemas.Line 443 adds
updated_atonly inCREATE TABLE IF NOT EXISTS, so already-provisioned schemas will miss this column. Downstream code already readsnormalized_entity.updated_at, which can fail at runtime on migrated tenants.Suggested fix
// Idempotently add published_at to pre-existing schemas. await this.db.$client.query(` ALTER TABLE "${schemaName}".normalized_entity ADD COLUMN IF NOT EXISTS published_at TIMESTAMPTZ; `); + await this.db.$client.query(` + ALTER TABLE "${schemaName}".normalized_entity + ADD COLUMN IF NOT EXISTS updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(); + `);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dbmanager/src/impl/sql-database-manager.ts` around lines 443 - 453, The CREATE TABLE IF NOT EXISTS block defines updated_at column for the normalized_entity table, but this column is only added to new schemas. Existing schemas that already have the normalized_entity table will be missing this column, causing runtime failures in downstream code that reads normalized_entity.updated_at. Add an idempotent ALTER TABLE statement after the existing published_at ALTER TABLE block that adds the updated_at TIMESTAMPTZ column to the normalized_entity table if it does not already exist, following the same pattern as the ALTER TABLE for published_at.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.husky/pre-commit:
- Line 8: The git add command on line 8 uses the glob pattern
plugins/**/drizzle/migrations/* which only matches files directly in the
migrations directory but excludes nested subdirectories like meta. Modify the
glob pattern to plugins/**/drizzle/migrations/**/* to recursively include all
files and subdirectories within migrations, ensuring that metadata files like
_journal.json are properly staged.
In `@apps/web/src/modules/trace/pages/ConnectionDataExplorerPage.tsx`:
- Around line 494-500: The load function now accepts a currentCanonicalType
parameter to maintain canonical type filtering, but some callers at lines 600
and 668 are not passing this parameter when invoking load. Update all calls to
the load function to include the currentCanonicalType argument so that the
canonical filter is consistently preserved when reloading data, preventing the
issue where the normalized view reloads unfiltered data while the canonical
dropdown still shows a selected type.
- Around line 509-544: The normalized tab is fetching data twice during
canonical type initialization because the first useEffect (starting at line 509)
calls load after setting selectedCanonicalType, and then the second useEffect
(starting at line 537) immediately triggers again due to the
selectedCanonicalType state change and calls load again with the same
parameters. Remove the explicit load call on line 519 from the first useEffect
after setting selectedCanonicalType, since the second useEffect will
automatically trigger and handle the load call when selectedCanonicalType
changes, eliminating the duplicate fetch.
In `@packages/migrator/package.json`:
- Around line 20-23: Add drizzle-orm as a direct dependency in the dependencies
section of packages/migrator/package.json alongside the existing `@nestjs/common`
and pg dependencies. The drizzle-migration-runner.adapter.ts file dynamically
imports drizzle-orm in the runQuery function, and without it declared as an
explicit dependency in package.json, runtime failures will occur when
runMigrations passes a Drizzle db object that triggers the execute code path.
In `@packages/migrator/src/adapters/drizzle-migration-runner.adapter.ts`:
- Around line 79-84: The catch block around the fs.readFile call for journalPath
is catching all errors and treating them as a missing journal file, which masks
real I/O or permission failures. Modify the catch block to check if the caught
error has a code property equal to ENOENT before logging the warning and
returning. For any other error codes, the error should be re-thrown or handled
appropriately to ensure that real I/O or permission failures do not silently
fail and leave schemas unmigrated.
- Around line 114-125: The migration hash insertion into the
__drizzle_migrations table is happening after the transaction commits, creating
a race condition where a crash between commit and insert will cause the
migration to be re-applied. For transactional migrations (when
disableTransaction is false), move the INSERT INTO __drizzle_migrations
statement inside the transaction by including it as part of the content passed
to the runTransaction function call, ensuring both the migration execution and
journal write are atomic. The non-transactional path (disableTransaction = true)
can keep the INSERT outside since it's not wrapped in a transaction anyway.
- Around line 17-23: The runQuery function is calling pool.query() repeatedly,
which obtains a different connection from the pool each time. This breaks
transaction boundaries (BEGIN/COMMIT/ROLLBACK) and advisory locks that require
the same backend session. Instead of passing the pool as the runner throughout
the migration, obtain a single client connection from the pool before starting
the migration and use that same client for all operations in runQuery,
transaction fallback (around lines 33-49), lock acquisition (around line 65),
and lock release (around lines 131-133). Ensure the client connection is
properly released after the migration completes to return it to the pool.
In `@packages/pipeline/src/plugin-hooks/pipeline-hook-broker.service.spec.ts`:
- Around line 18-20: The mockPieceRegistry currently always returns a piece with
appHooks defined, so the code paths for handling pieces without appHooks are
never tested. Add new explicit test cases in the test suite for the
PipelineHookBrokerService that cover scenarios where getPiece returns a piece
object without the appHooks property. For the getWebhookResponse method, verify
it returns null when appHooks is not present, and for the provisionDomain
method, verify it skips execution silently without errors when appHooks is
absent. This will ensure the optional-hook behavior is properly exercised and
validated.
In `@packages/pipeline/src/plugin-hooks/pipeline-hook-broker.service.ts`:
- Around line 11-21: The getHooks() method unconditionally throws an error when
appHooks is missing, which prevents optional hook paths like provisionDomain and
getWebhookResponse from running their own skip/null logic. Remove the second
error throw in the getHooks() method that validates the hooks object, and
instead allow the method to return undefined or null when appHooks is not
defined, so that callers can handle the absence of hooks with their own
conditional logic.
In `@plugins/domain/tms/drizzle/migrations/meta/_journal.json`:
- Line 1: The migration journal at _journal.json is empty and lacks entries for
the required schema changes (data_source_id and updated_at columns) needed by
the TMS writers. Create a new migration file in the migrations directory that
includes ALTER TABLE statements to add these required columns to the relevant
TMS tables, with appropriate defaults and backfill logic. Then add a
corresponding entry to the _journal.json file to register this migration with a
new version number and metadata so that existing tenant databases will properly
upgrade their schemas and prevent runtime failures in typed upserts.
In `@plugins/domain/tms/src/schema/tms-provisioner.ts`:
- Line 31: The `data_source_id` column being added with NOT NULL constraint in
the schema file will not be created on already-provisioned tenant tables since
the CREATE TABLE IF NOT EXISTS statement is skipped for existing tables. Add a
separate migration file that explicitly handles the column addition with an
ALTER TABLE ADD COLUMN statement for the `data_source_id` column as nullable,
includes a backfill operation to populate existing rows with an appropriate
default value, and then uses a subsequent ALTER TABLE statement to set the
column as NOT NULL. This ensures existing tenant tables are properly updated
during rollout before the NOT NULL constraint is enforced.
In `@plugins/domain/tms/src/schema/tms-schema.ts`:
- Line 78: The unique constraint on the TMS table is insufficient after adding
the dataSourceId column. Currently, the table has a UNIQUE constraint only on
source_id, which allows the same source_id to exist with different
dataSourceIds, causing cross-connection collisions. Update the unique constraint
on the TMS table schema to be composite, changing from UNIQUE(source_id) to
UNIQUE(data_source_id, source_id) to ensure that the combination of both columns
is unique and prevents collisions across different data sources.
In `@plugins/domain/tms/src/writers/tms-tp.writer.ts`:
- Around line 5-6: The usDotNumber field in the tpWriter is reading from the
wrong data property. Change the field assignment for usDotNumber from
`str(data['usdot'])` to `str(data['usDotNumber'])` to match the actual property
name emitted by the revenova-piece normalizer. This aligns with the database
schema definition and maintains consistency with other camelCase field mappings
like mcNumber and stateDotNumber in the same writer.
In `@plugins/domain/tms/src/writers/tms-writer-utils.ts`:
- Around line 55-56: The ON CONFLICT clause in the SQL statement only targets
the source_id column, which is insufficient for multi-source write operations
and can cause row overwrites when different data sources share the same
source_id. Modify the ON CONFLICT clause to use a composite key that includes
both source_id and data_source_id columns, so the conflict detection properly
respects the new identity model that depends on both fields together rather than
just source_id alone.
In `@plugins/salesforce/revenova-piece/src/normalizeRevenovaToTms.spec.ts`:
- Around line 26-32: The tests in normalizeRevenovaToTms.spec.ts are mocking the
evaluate function with mockEvaluate.mockReturnValueOnce(), which means they
don't exercise the actual JSONata mapping logic and won't catch regressions in
the expressions themselves. Add at least two new test cases that don't mock the
evaluate function and instead pass real test data through the actual
normalizeRevenovaToTms function to verify the real JSONata mapping works
correctly for both the Account and rtms__TransportationProfile__c object types,
ensuring the correct canonical types and data shapes are produced.
In `@plugins/salesforce/revenova-piece/src/upsertRevenovaObject.ts`:
- Around line 143-146: The normalization of context.objectType in the code block
containing parsedDoctype, context.objectType.startsWith, and substring
operations does not validate whether the normalized value is non-empty before
using it to override parsedDoctype. If context.objectType is a value like "sf:"
with no actual type name, the substring operation will result in an empty
string, causing parsedDoctype to be set to an empty value instead of falling
back to the original parsed value. Add a guard condition after the
substring/normalization to verify the resulting normalized value is non-empty
before assigning it to parsedDoctype, otherwise keep the fallback to the
original parsed value. Apply this same fix to all occurrences mentioned in the
comment.
In `@sdk/framework/src/piece.ts`:
- Around line 295-296: The `migrationsFolder` property was added to the `Piece`
class but is missing from the `CreatePieceParams` interface and the
`createPiece()` function's return mapping. Add the optional `migrationsFolder?:
string` property to the `CreatePieceParams` interface to allow it to be passed
as a parameter, and then ensure the `createPiece()` function properly maps this
parameter to the `migrationsFolder` field in the returned `Piece` object so that
pieces created via `createPiece()` can include migration metadata.
In `@sdk/registry/src/pieces/migration-worker.service.ts`:
- Around line 105-111: The migrationsFolder resolution in the
getTenantDbConnection call is vulnerable to path traversal attacks because
event.migrationsFolder can contain absolute paths or .. segments that escape the
plugin root. Validate and constrain the baseFolder value before using it with
path.resolve() by ensuring it contains no absolute path indicators and no ..
segments, then verify that the final resolved migrationsFolder path stays within
the event.pluginLocation directory. You can use path.relative() to check if the
resolved path is actually contained within the plugin location, rejecting any
paths that would traverse outside of it before passing to
this.migrator.runMigrations().
---
Outside diff comments:
In `@apps/api/src/modules/trace/adapters/outbound/drizzle-explorer.adapter.ts`:
- Around line 589-599: The normalized async function is selecting canonicalType
from normalizedEntity, but the downstream listNormalizedTypes function filters
results using replicaEntity.entityType. This mismatch causes the canonical type
discovery flow to fail. Change the selectDistinct field from
schema.normalizedEntity.canonicalType to schema.replicaEntity.entityType in the
normalized function so that the returned object type values match the field that
listNormalizedTypes uses for filtering when making subsequent
/normalized/types?objectType=... requests.
In `@packages/dbmanager/src/impl/sql-database-manager.ts`:
- Around line 443-453: The CREATE TABLE IF NOT EXISTS block defines updated_at
column for the normalized_entity table, but this column is only added to new
schemas. Existing schemas that already have the normalized_entity table will be
missing this column, causing runtime failures in downstream code that reads
normalized_entity.updated_at. Add an idempotent ALTER TABLE statement after the
existing published_at ALTER TABLE block that adds the updated_at TIMESTAMPTZ
column to the normalized_entity table if it does not already exist, following
the same pattern as the ALTER TABLE for published_at.
In `@packages/pipeline/src/shared/fakes/normalization-repository.fake.ts`:
- Around line 6-7: The NormalizationRepositoryFake class has a method that
always returns a hardcoded 'DEFAULT' value for the objectType field, which
prevents tests from detecting issues where production code might receive null or
different values. Locate the method returning responses (around line 20-22) that
builds the fake repository response and replace the hardcoded 'DEFAULT'
objectType with either a configurable property that can be set per test
scenario, or derive it from the input parameters being passed to the method so
different tests can verify behavior with different objectType values.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d08a2e4-c424-4899-b044-3537e1676a20
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (76)
.husky/pre-commitapps/api/src/modules/dbmanager/dbmanager.module.tsapps/api/src/modules/trace/adapters/outbound/drizzle-explorer.adapter.tsapps/api/src/modules/trace/connection-explorer.controller.tsapps/api/src/modules/trace/core/ports/outbound/explorer-repository.port.tsapps/api/src/modules/trace/core/use-cases/explorer/list-connection-data.use-case.tsapps/api/src/modules/trace/core/use-cases/explorer/list-normalized-types.use-case.tsapps/api/src/modules/trace/trace.module.tsapps/tenant-provisioner/package.jsonapps/tenant-provisioner/src/modules/provisioner/provisioner.module.tsapps/tenant-provisioner/src/modules/provisioner/tenant-provision.worker.spec.tsapps/tenant-provisioner/src/modules/provisioner/tenant-provision.worker.tsapps/web/src/modules/trace/api/data-explorer.api.tsapps/web/src/modules/trace/components/TraceViewerPanel.tsxapps/web/src/modules/trace/pages/ConnectionDataExplorerPage.tsxapps/web/src/modules/trace/pages/DataExplorerPage.tsxapps/worker/src/core/use-cases/app-installer/app-installer.spec.tsapps/worker/src/core/use-cases/outbox/process-outbox.use-case.spec.tspackage.jsonpackages/database/src/schema/tenant/pipeline.tspackages/dbmanager/src/impl/sql-database-manager.tspackages/migrator/package.jsonpackages/migrator/src/adapters/drizzle-migration-runner.adapter.tspackages/migrator/src/index.tspackages/migrator/src/migrator.module.tspackages/migrator/src/ports/migration-runner.port.tspackages/migrator/tsconfig.build.jsonpackages/migrator/tsconfig.jsonpackages/pipeline/src/index.tspackages/pipeline/src/normalization/normalization.service.tspackages/pipeline/src/plugin-hooks/application-shard.types.tspackages/pipeline/src/plugin-hooks/pipeline-hook-broker.service.spec.tspackages/pipeline/src/plugin-hooks/pipeline-hook-broker.service.tspackages/pipeline/src/shared/adapters/outbound/drizzle-normalization.adapter.tspackages/pipeline/src/shared/fakes/normalization-repository.fake.tspackages/pipeline/src/shared/ports/normalization.repository.port.tspackages/pipeline/src/test-utils/mock-db-manager.tspackages/provision/src/test-utils/mock-db-manager.tspackages/queue/src/events/plugin-migration.event.tsplugins/domain/tms/drizzle/migrations/meta/_journal.jsonplugins/domain/tms/package.jsonplugins/domain/tms/src/schema/tms-provisioner.tsplugins/domain/tms/src/schema/tms-schema.tsplugins/domain/tms/src/tms-normalized-writer.tsplugins/domain/tms/src/tms-target-builder.tsplugins/domain/tms/src/writers/tms-address.writer.tsplugins/domain/tms/src/writers/tms-carrier.writer.tsplugins/domain/tms/src/writers/tms-customer.writer.tsplugins/domain/tms/src/writers/tms-factoring.writer.tsplugins/domain/tms/src/writers/tms-tp.writer.tsplugins/domain/tms/src/writers/tms-vendor.writer.tsplugins/domain/tms/src/writers/tms-writer-utils.tsplugins/quickbooks/core/package.jsonplugins/salesforce/core/package.jsonplugins/salesforce/core/src/adapters/native-fetch.adapter.spec.tsplugins/salesforce/core/src/adapters/native-fetch.adapter.tsplugins/salesforce/core/src/index.tsplugins/salesforce/revenova-piece/src/index.tsplugins/salesforce/revenova-piece/src/normalizeRevenovaToTms.spec.tsplugins/salesforce/revenova-piece/src/normalizeRevenovaToTms.tsplugins/salesforce/revenova-piece/src/normalizeRevenovaToTmsMapping.tsplugins/salesforce/revenova-piece/src/upsertRevenovaObject.spec.tsplugins/salesforce/revenova-piece/src/upsertRevenovaObject.tssdk/framework/src/app-hooks.tssdk/framework/src/index.tssdk/framework/src/normalizer.tssdk/framework/src/piece.tssdk/framework/src/pipeline-plugin.types.tssdk/registry/package.jsonsdk/registry/src/pieces/migration-worker.service.spec.tssdk/registry/src/pieces/migration-worker.service.tssdk/registry/src/pieces/pieces.module.tssdk/registry/src/pieces/plugin-hot-reloader.service.spec.tssdk/registry/src/pieces/plugin-hot-reloader.service.tstest/integration/setup/global-setup.tsturbo.json
💤 Files with no reviewable changes (3)
- packages/pipeline/src/plugin-hooks/application-shard.types.ts
- plugins/salesforce/revenova-piece/src/normalizeRevenovaToTmsMapping.ts
- sdk/framework/src/app-hooks.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 18 file(s) based on 18 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 18 file(s) based on 18 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/domain/tms/src/schema/tms-provisioner.ts (1)
31-31:⚠️ Potential issue | 🔴 CriticalMigration file missing for existing tenant table updates.
The migration journal references "0000_add_data_source_id_and_updated_at" but no actual migration file exists in
plugins/domain/tms/drizzle/migrations/. The provisioner'sCREATE TABLE IF NOT EXISTSstatements only apply to new tenant schemas; existing tenants won't receive:
- The new
data_source_idcolumn- Updated unique constraints from
(source_id)to(data_source_id, source_id)This will cause INSERT/UPDATE failures when the application tries to set
data_source_idon existing tenant tables. Create the missing migration file with:
ALTER TABLE ... ADD COLUMN data_source_id VARCHAR(255);(initially nullable)- Backfill existing rows with appropriate values
ALTER TABLE ... ALTER COLUMN data_source_id SET NOT NULL;ALTER TABLE ... DROP CONSTRAINTold unique constraint andADD CONSTRAINTwith new composite keyAlso applies to: lines 54, 63, 72, 80, 88, 98 (all tables affected)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/domain/tms/src/schema/tms-provisioner.ts` at line 31, A migration file referenced in the migration journal ("0000_add_data_source_id_and_updated_at") is missing from the `plugins/domain/tms/drizzle/migrations/` directory. Create this missing migration file with ALTER TABLE statements that add the data_source_id column (initially as nullable VARCHAR(255)) to all affected tables (those referenced in the schema provisioner at lines 31, 54, 63, 72, 80, 88, 98). The migration must then backfill existing rows with appropriate data_source_id values, alter the column to SET NOT NULL, drop the existing unique constraint on (source_id) only, and add a new composite unique constraint on (data_source_id, source_id) to ensure existing tenants receive all schema updates that the CREATE TABLE IF NOT EXISTS statements provide to new tenants.apps/web/src/modules/trace/pages/ConnectionDataExplorerPage.tsx (1)
509-531:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalized tab fails to load data when no canonical types exist.
When
listConnectionNormalizedTypesreturns an empty array,selectedCanonicalTypeis set to''. The second effect (lines 534-539) requiresselectedCanonicalType !== ''to trigger a load, so no data is ever fetched. Users will see an empty or perpetually loading state.The duplicate-fetch fix (from the prior review) removed the
load()call entirely, but the empty-types case still needs explicit handling.🐛 Proposed fix to handle empty canonical types
listConnectionNormalizedTypes(stitch.id, objectType).then(types => { if (!active) return; setCanonicalTypes(types); const newSelected = types.length > 0 ? types[0] : ''; setSelectedCanonicalType(newSelected); setPage(1); + // When no canonical types exist, load without filter; otherwise second effect handles it + if (newSelected === '') { + void load(1, appliedFilters, objectType, ''); + } }).catch(err => { console.error('Failed to load canonical types', err); if (active) { setCanonicalTypes([]); setSelectedCanonicalType(''); setPage(1); + void load(1, appliedFilters, objectType, ''); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/modules/trace/pages/ConnectionDataExplorerPage.tsx` around lines 509 - 531, When listConnectionNormalizedTypes returns an empty array in the useEffect hook, selectedCanonicalType is set to an empty string which prevents the subsequent effect from triggering a load (that effect requires selectedCanonicalType !== ''). Fix this by explicitly calling the load function with appropriate parameters (load with argument 1, appliedFilters, and objectType) in the then block when types.length is 0, similar to how load is called in the else branch. This ensures data is fetched even when no canonical types exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/domain/tms/drizzle/migrations/meta/_journal.json`:
- Line 1: The _journal.json file contains an entry referencing the migration tag
"0000_add_data_source_id_and_updated_at", but no corresponding SQL migration
file exists in the migrations directory. To fix this, either create the actual
migration SQL file with a name matching the migration tag (following the naming
convention used by your migration tool) in the migrations directory, or remove
the orphaned entry from the journal entries array in _journal.json. Ensure
migration consistency by keeping the journal and actual migration files in sync.
---
Outside diff comments:
In `@apps/web/src/modules/trace/pages/ConnectionDataExplorerPage.tsx`:
- Around line 509-531: When listConnectionNormalizedTypes returns an empty array
in the useEffect hook, selectedCanonicalType is set to an empty string which
prevents the subsequent effect from triggering a load (that effect requires
selectedCanonicalType !== ''). Fix this by explicitly calling the load function
with appropriate parameters (load with argument 1, appliedFilters, and
objectType) in the then block when types.length is 0, similar to how load is
called in the else branch. This ensures data is fetched even when no canonical
types exist.
In `@plugins/domain/tms/src/schema/tms-provisioner.ts`:
- Line 31: A migration file referenced in the migration journal
("0000_add_data_source_id_and_updated_at") is missing from the
`plugins/domain/tms/drizzle/migrations/` directory. Create this missing
migration file with ALTER TABLE statements that add the data_source_id column
(initially as nullable VARCHAR(255)) to all affected tables (those referenced in
the schema provisioner at lines 31, 54, 63, 72, 80, 88, 98). The migration must
then backfill existing rows with appropriate data_source_id values, alter the
column to SET NOT NULL, drop the existing unique constraint on (source_id) only,
and add a new composite unique constraint on (data_source_id, source_id) to
ensure existing tenants receive all schema updates that the CREATE TABLE IF NOT
EXISTS statements provide to new tenants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49a105d1-f429-4c8d-b53b-9b4e714c1084
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.husky/pre-commitapps/web/src/modules/trace/pages/ConnectionDataExplorerPage.tsxpackages/migrator/package.jsonpackages/migrator/src/adapters/drizzle-migration-runner.adapter.tspackages/migrator/tsconfig.jsonpackages/pipeline/src/plugin-hooks/pipeline-hook-broker.service.spec.tspackages/pipeline/src/plugin-hooks/pipeline-hook-broker.service.tspackages/pipeline/tsconfig.build.jsonplugins/domain/tms/drizzle/migrations/meta/_journal.jsonplugins/domain/tms/src/schema/tms-provisioner.tsplugins/domain/tms/src/schema/tms-schema.tsplugins/domain/tms/src/writers/tms-tp.writer.tsplugins/domain/tms/src/writers/tms-writer-utils.tsplugins/salesforce/revenova-piece/src/normalizeRevenovaToTms.spec.tsplugins/salesforce/revenova-piece/src/upsertRevenovaObject.tssdk/framework/src/piece.tssdk/registry/src/pieces/migration-worker.service.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/dbmanager/src/impl/sql-database-manager.ts (2)
82-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale docstring: DO-block guards no longer exist.
The docstring claims "All DDL inside is idempotent (CREATE IF NOT EXISTS + DO $$ BEGIN guards)" for column renames/additions, but the DO-block migrations that applied schema alterations to existing tables have been removed. Calling
migrateReplicaTableson a pre-existing schema will now skip tables that already exist without adding any missing columns.Update the docstring to reflect the current behavior or restore the idempotent ALTER logic if migration of existing tenants is still required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dbmanager/src/impl/sql-database-manager.ts` around lines 82 - 91, The docstring for the `migrateReplicaTables` method incorrectly claims that DDL operations use "DO $$ BEGIN guards" for idempotency, but this logic has been removed. Update the docstring to accurately describe the current behavior: that the method will only provision tables using CREATE IF NOT EXISTS and will not modify existing table schemas to add missing columns. If idempotent schema migration of existing tenants is still required, you will need to restore the ALTER TABLE logic that was previously removed from the `provisionReplicaTables` method call.
93-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale docstring: migration claims no longer valid.
The docstring promises "existing tenants receive newly provisioned objects" including "schema_name columns on outbox tables," but the DO-block migrations responsible for adding columns to existing tables have been removed. With only
CREATE TABLE IF NOT EXISTS, existing tables won't be altered.Either update the docstring to accurately describe the new behavior (provision-only, no schema evolution) or implement an alternative migration path for existing tenants.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dbmanager/src/impl/sql-database-manager.ts` around lines 93 - 108, The docstring for the migrateToStandardActive method describes functionality that the current implementation does not actually provide. The docstring claims the method will add schema_name columns to outbox tables and other schema evolution tasks, but the implementation only uses CREATE TABLE IF NOT EXISTS which will not alter existing tables. Either update the docstring to accurately describe the current behavior (that it only provisions new tables without modifying existing tenant schemas), or implement the actual migration logic with DO-block migrations that will alter existing tables to add the missing columns and indexes as originally intended.apps/api/src/modules/trace/adapters/outbound/drizzle-explorer.adapter.ts (2)
326-337: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDuplicate
extractRowshelper should be extracted to a private method.The same
extractRowsfunction is defined identically in bothqueryPolymorphicNormalizedEntitiesandqueryCanonicalTable. Extract it once as a private class method to eliminate duplication.♻️ Proposed refactor
Add a private method at class level:
private extractRows(res: unknown): Record<string, unknown>[] { if (Array.isArray(res)) return res as Record<string, unknown>[]; if ( res && typeof res === 'object' && 'rows' in res && Array.isArray((res as { rows: unknown[] }).rows) ) { return (res as { rows: Record<string, unknown>[] }).rows; } return []; }Then replace inline definitions in both
queryPolymorphicNormalizedEntitiesandqueryCanonicalTablewiththis.extractRows(...).Also applies to: 382-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/trace/adapters/outbound/drizzle-explorer.adapter.ts` around lines 326 - 337, The extractRows helper function is duplicated in both the queryPolymorphicNormalizedEntities and queryCanonicalTable methods. Extract this function as a private class method named extractRows that takes an unknown parameter and returns Record<string, unknown>[]. Then replace the inline function definitions in both queryPolymorphicNormalizedEntities and queryCanonicalTable with calls to this.extractRows(...) to eliminate the duplication while maintaining the same functionality.
96-112: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRepeated
buildTenantSchemacalls create unnecessary overhead.
buildTenantSchema(schemaName)is called 4 times in this method—once at line 79, and then again at lines 98, 102, and 105. Each call constructs new schema objects. Destructure both tables once and reuse them.♻️ Proposed fix
async listConnectionReplica( tenantId: string, schemaName: string, connectionId: string, page: number, limit: number, objectType?: string, filters?: import('../../filter-parser.js').FilterGroup, ): Promise<ExplorerPage<Record<string, unknown>>> { const tenantDb = await this.dbManager.getTenantDb(tenantId); assertValidSchemaName(schemaName); - const { replicaEntity } = buildTenantSchema(schemaName); + const { replicaEntity, normalizedEntity } = buildTenantSchema(schemaName); const filterWhere = buildDrizzleFilter(filters, replicaEntity); const finalWhere = and( eq(replicaEntity.dataSourceId, connectionId), objectType ? eq(replicaEntity.entityType, objectType) : undefined, filterWhere, ); const offset = (page - 1) * limit; const [countRes, dataResRaw] = await Promise.all([ tenantDb .select({ count: sql`count(*)` }) .from(replicaEntity) .where(finalWhere), tenantDb .select({ replica: replicaEntity, - normalizedId: buildTenantSchema(schemaName).normalizedEntity.id, + normalizedId: normalizedEntity.id, }) .from(replicaEntity) .leftJoin( - buildTenantSchema(schemaName).normalizedEntity, + normalizedEntity, eq( replicaEntity.id, - buildTenantSchema(schemaName).normalizedEntity.replicaId, + normalizedEntity.replicaId, ), ) .where(finalWhere) .orderBy(desc(replicaEntity.createdAt)) .limit(limit) .offset(offset), ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/trace/adapters/outbound/drizzle-explorer.adapter.ts` around lines 96 - 112, The buildTenantSchema(schemaName) function is being called multiple times throughout this method (at least 4 times), creating unnecessary overhead by constructing new schema objects each time. Store the result of buildTenantSchema(schemaName) in a variable early in the method, then reuse that variable in place of all subsequent calls to buildTenantSchema(schemaName) in the same method. This will eliminate redundant schema object construction while maintaining the same functionality.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/api/src/modules/trace/adapters/outbound/drizzle-explorer.adapter.ts`:
- Around line 326-337: The extractRows helper function is duplicated in both the
queryPolymorphicNormalizedEntities and queryCanonicalTable methods. Extract this
function as a private class method named extractRows that takes an unknown
parameter and returns Record<string, unknown>[]. Then replace the inline
function definitions in both queryPolymorphicNormalizedEntities and
queryCanonicalTable with calls to this.extractRows(...) to eliminate the
duplication while maintaining the same functionality.
- Around line 96-112: The buildTenantSchema(schemaName) function is being called
multiple times throughout this method (at least 4 times), creating unnecessary
overhead by constructing new schema objects each time. Store the result of
buildTenantSchema(schemaName) in a variable early in the method, then reuse that
variable in place of all subsequent calls to buildTenantSchema(schemaName) in
the same method. This will eliminate redundant schema object construction while
maintaining the same functionality.
In `@packages/dbmanager/src/impl/sql-database-manager.ts`:
- Around line 82-91: The docstring for the `migrateReplicaTables` method
incorrectly claims that DDL operations use "DO $$ BEGIN guards" for idempotency,
but this logic has been removed. Update the docstring to accurately describe the
current behavior: that the method will only provision tables using CREATE IF NOT
EXISTS and will not modify existing table schemas to add missing columns. If
idempotent schema migration of existing tenants is still required, you will need
to restore the ALTER TABLE logic that was previously removed from the
`provisionReplicaTables` method call.
- Around line 93-108: The docstring for the migrateToStandardActive method
describes functionality that the current implementation does not actually
provide. The docstring claims the method will add schema_name columns to outbox
tables and other schema evolution tasks, but the implementation only uses CREATE
TABLE IF NOT EXISTS which will not alter existing tables. Either update the
docstring to accurately describe the current behavior (that it only provisions
new tables without modifying existing tenant schemas), or implement the actual
migration logic with DO-block migrations that will alter existing tables to add
the missing columns and indexes as originally intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: abd1fa23-654c-43f6-95f1-b8ee7cfc5c90
⛔ Files ignored due to path filters (1)
packages/dbmanager/src/impl/__snapshots__/sql-database-manager.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
apps/api/src/modules/trace/adapters/outbound/drizzle-explorer.adapter.tsapps/web/src/modules/trace/pages/ConnectionDataExplorerPage.tsxpackages/dbmanager/src/impl/sql-database-manager.tspackages/migrator/tsconfig.jsonpackages/pipeline/src/shared/fakes/normalization-repository.fake.tsplugins/domain/tms/drizzle/migrations/meta/_journal.json
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
Release Notes
New Features
Bug Fixes
Chores
updatedAttimestamps; TMS uniqueness is now enforced by(data source, source)pairing.