feat: architecture separation initial commit - #113
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new package Changes
Sequence Diagram(s)sequenceDiagram
participant NestApp as NestJS App
participant HostClient as HostHttpClient
participant Framework as Piece Framework
participant Proxy as httpClient Proxy
rect rgba(200,200,255,0.5)
NestApp->>HostClient: module init -> onModuleInit()
HostClient->>Framework: initializeHttpClient(this)
Framework->>Proxy: set internal client instance
end
rect rgba(200,255,200,0.5)
participant PieceCode as Piece consumer
PieceCode->>Proxy: httpClient.sendRequest(request)
Proxy->>Framework: forward bound call -> client.sendRequest(...)
Framework->>HostClient: delegated sendRequest call
HostClient-->>PieceCode: response (parsed per responseType)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/pieces/salesforce/src/lib/trigger/universal-trigger.ts (1)
4-8:⚠️ Potential issue | 🔴 CriticalCritical: Broken dependency across multiple pieces — intelligence module imports will fail at runtime.
Multiple files across at least 2 pieces import from
@nexiom/connectors/intelligence, but their respectivepackage.jsonfiles remove@nexiom/connectorsas a dependency:
- Salesforce piece: 5 files affected (universal-trigger.ts, salesforce-query.adapter.ts, salesforce-query.adapter.spec.ts, salesforce-discovery.adapter.ts, salesforce-bulk.adapter.ts)
- Quickbooks piece: 3 files affected (universal-trigger.ts, quickbooks-query.adapter.ts, quickbooks-polling.helper.ts)
The
@nexiom/piece-frameworkreplacement does not re-exportUniversalTriggerEngine,optimizationService,IgtLogger,QuerySpec,ObjectHint, orObjectSchemafrom the intelligence module. This will cause module resolution failures at runtime.Required resolution:
- Retain
@nexiom/connectorsas a dependency in affectedpackage.jsonfiles (alongside@nexiom/piece-framework), OR- Move intelligence module exports to
@nexiom/piece-framework, OR- Create a separate
@nexiom/connectors-intelligencepackage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pieces/salesforce/src/lib/trigger/universal-trigger.ts` around lines 4 - 8, The imports of UniversalTriggerEngine, optimizationService, IgtLogger, QuerySpec, ObjectHint, and ObjectSchema from '@nexiom/connectors/intelligence' will fail because '@nexiom/connectors' was removed from package.json; restore runtime resolution by re-adding '@nexiom/connectors' as a dependency to the affected package.json files (at least the Salesforce and Quickbooks pieces) and run install/build; search for files importing those symbols (e.g., universal-trigger.ts, salesforce-query.adapter.ts, salesforce-discovery.adapter.ts, salesforce-bulk.adapter.ts, quickbooks universal-trigger.ts/quickbooks-query.adapter.ts/quickbooks-polling.helper.ts) to verify imports are unchanged, or alternatively implement one of the other approved resolutions (re-export these symbols from `@nexiom/piece-framework` or create a new `@nexiom/connectors-intelligence` package) and update import paths accordingly before running tests.packages/engine/src/pieces/piece-loader.service.ts (1)
116-128:⚠️ Potential issue | 🟠 MajorStrengthen
isPieceto enforce requiredPiecefields.The runtime validator at lines 116-128 does not check for
auth(required) andcategories(required) fields defined in thePieceinterface. Invalid pieces lacking these fields will pass the guard and cause failures downstream when code assumes the full contract.Proposed fix
private isPiece(value: unknown): value is Piece { if (typeof value !== 'object' || value === null) return false; const v = value as Record<string, unknown>; return ( typeof v['name'] === 'string' && typeof v['displayName'] === 'string' && typeof v['description'] === 'string' && typeof v['logoUrl'] === 'string' && + typeof v['auth'] === 'object' && + v['auth'] !== null && + Array.isArray(v['categories']) && typeof v['actions'] === 'object' && v['actions'] !== null && typeof v['triggers'] === 'object' && v['triggers'] !== null ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/src/pieces/piece-loader.service.ts` around lines 116 - 128, The isPiece type guard currently misses validating two required Piece fields so invalid objects slip through; update the isPiece(value: unknown): value is Piece function to also check that v['auth'] is an object (and not null) and that v['categories'] is an array (and has at least one string or is an array of strings per your Piece contract), and ensure both checks use typeof/Array.isArray validations consistent with the existing pattern so the guard only returns true when name, displayName, description, logoUrl, actions, triggers, auth, and categories all meet their expected shapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/master/tasks.md`:
- Line 627: The task description contains a duplicated word "disk disk"; update
the checklist item text that currently reads "pulls/fetches mapped Git Shard
repositories (e.g. `fluxnex-shard-001`) onto the local disk disk every 5
minutes." to remove the extra "disk" so it reads "onto the local disk every 5
minutes." Locate the string in the docs/architecture/master/tasks.md content
(the checklist entry) and correct the typo, preserving the rest of the phrasing
and the 5-minute interval.
- Around line 615-633: The Phase summary table and totals in the docs are stale
after adding new tasks T053 and T054 (Phase 8); update the summary section to
add a "Phase 8" row (reflecting the new T053 · engine and T054 · worker entries)
and recalculate the overall totals so the phase table and totals match the
current task list (ensure references to T053 and T054 appear in the new row and
any aggregate counts/percentages are adjusted accordingly).
In `@packages/connectors/src/http-client/host-http-client.spec.ts`:
- Around line 10-12: Replace the value import of NormalizedRecord and
VendorResponse with a type-only import: change the existing import that brings
those symbols from '@nexiom/piece-framework' into a single type import (import
type { NormalizedRecord, VendorResponse } from '@nexiom/piece-framework') while
leaving any other value imports intact; this ensures NormalizedRecord and
VendorResponse are treated as type-only (they are only used as annotations in
the tests).
In `@packages/connectors/src/http-client/host-http-client.ts`:
- Line 12: The import groups should separate type-only imports from runtime
imports: change the single import that brings in HttpClient, HttpRequest,
HttpResponse, and HttpMethod into two imports so that HttpClient, HttpRequest,
and HttpResponse are imported with "import type" (type-only) and HttpMethod is
imported as a normal runtime import; update the import statement that currently
references HttpClient/HttpRequest/HttpResponse/HttpMethod to use "import type {
HttpClient, HttpRequest, HttpResponse } from '@nexiom/piece-framework';" and
"import { HttpMethod } from '@nexiom/piece-framework';" so runtime usage of
HttpMethod remains available while the other symbols are treated as types.
In `@packages/piece-framework/package.json`:
- Around line 1-27: The package manifest is missing a declared dependency for
`@nestjs/common` while src/piece.ts imports InternalServerErrorException; either
add "@nestjs/common" to the package.json dependencies (so consumers/installers
get it) or replace InternalServerErrorException in src/piece.ts with a local
error type/utility to decouple from Nest; update
packages/piece-framework/package.json "dependencies" to include `@nestjs/common`
with an appropriate semver OR modify the symbol InternalServerErrorException
usage in src/piece.ts to throw a package-local Error subclass (e.g.,
PieceInternalServerError) and remove the external import.
In `@packages/piece-framework/src/http-client.ts`:
- Around line 35-37: initializeHttpClient currently allows overwriting the
singleton _httpClientInstance; change it to guard against re-assignment by
checking _httpClientInstance before setting it and failing fast (throw an
informative error) if it's already initialized. Locate the initializeHttpClient
function and the _httpClientInstance symbol and modify the function to validate
that _httpClientInstance is unset (or null/undefined) before assigning the
provided HttpClient; if already set, throw an Error like "HttpClient already
initialized" to prevent silent mid-process transport swaps.
- Around line 39-43: The httpClient Proxy throws if accessed before
initialization because initializeHttpClient(...) is never called in production;
to fix, call initializeHttpClient(new HostHttpClient(...)) during application
bootstrap so the singleton is set before any piece uses
httpClient.sendRequest(); add this call in the NestJS startup path (e.g., in
PiecesModule onModuleInit / module bootstrap or a dedicated HTTP integration
module that runs before triggers/actions) and ensure HostHttpClient is
constructed with the platform host/context used in production.
In `@packages/piece-framework/tsconfig.json`:
- Around line 14-17: The tsconfig.json contains an invalid trailing comma after
the "include" array which breaks JSON parsing; open the tsconfig.json file and
remove the comma immediately following the closing bracket of the "include"
array so the file is valid JSON (ensure the "include": ["src/**/*"] entry ends
without a trailing comma).
In `@packages/piece-framework/tsconfig.spec.json`:
- Around line 3-7: The tsconfig.spec.json currently sets compilerOptions.types
to only ["vitest/globals"], which overrides the base and drops "node"; update
the types array to include "node" alongside "vitest/globals" so Node globals
remain available for tests (i.e., change compilerOptions.types to include both
"vitest/globals" and "node").
In `@packages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.ts`:
- Around line 1-3: The package is missing the `@nexiom/connectors` dependency
referenced by the import of ObjectHint in quickbooks-polling.helper.ts; add
"@nexiom/connectors" to the dependencies in the quickbooks package.json
(matching the version used across the monorepo), run the package manager to
update lockfiles, and ensure TypeScript can resolve the import so the import of
ObjectHint (and any other symbols from `@nexiom/connectors`) in
quickbooks-polling.helper.ts compiles successfully.
In `@packages/pieces/salesforce/package.json`:
- Line 28: The package.json change removed `@nexiom/connectors` which breaks
imports in packages/pieces/salesforce/src/lib/trigger/universal-trigger.ts (it
imports UniversalTriggerEngine, optimizationService, and IgtLogger from
`@nexiom/connectors/intelligence`); restore `@nexiom/connectors` as a dependency in
packages/pieces/salesforce/package.json (alongside `@nexiom/piece-framework`) so
those symbols remain resolvable, then run install to verify the imports in
UniversalTriggerEngine/optimizationService/IgtLogger resolve.
---
Outside diff comments:
In `@packages/engine/src/pieces/piece-loader.service.ts`:
- Around line 116-128: The isPiece type guard currently misses validating two
required Piece fields so invalid objects slip through; update the isPiece(value:
unknown): value is Piece function to also check that v['auth'] is an object (and
not null) and that v['categories'] is an array (and has at least one string or
is an array of strings per your Piece contract), and ensure both checks use
typeof/Array.isArray validations consistent with the existing pattern so the
guard only returns true when name, displayName, description, logoUrl, actions,
triggers, auth, and categories all meet their expected shapes.
In `@packages/pieces/salesforce/src/lib/trigger/universal-trigger.ts`:
- Around line 4-8: The imports of UniversalTriggerEngine, optimizationService,
IgtLogger, QuerySpec, ObjectHint, and ObjectSchema from
'@nexiom/connectors/intelligence' will fail because '@nexiom/connectors' was
removed from package.json; restore runtime resolution by re-adding
'@nexiom/connectors' as a dependency to the affected package.json files (at
least the Salesforce and Quickbooks pieces) and run install/build; search for
files importing those symbols (e.g., universal-trigger.ts,
salesforce-query.adapter.ts, salesforce-discovery.adapter.ts,
salesforce-bulk.adapter.ts, quickbooks
universal-trigger.ts/quickbooks-query.adapter.ts/quickbooks-polling.helper.ts)
to verify imports are unchanged, or alternatively implement one of the other
approved resolutions (re-export these symbols from `@nexiom/piece-framework` or
create a new `@nexiom/connectors-intelligence` package) and update import paths
accordingly before running tests.
🪄 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: 13dfb78c-29b1-4658-a351-6475e298fff4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
.husky/pre-commitapps/api/package.jsonapps/api/src/modules/connections/connections/callback.controller.spec.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/connections/connections/token-refresh.service.spec.tsapps/api/src/modules/connections/connections/token-refresh.service.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/scheduler/poll-sync-runner.tsapps/api/src/modules/stitches/metadata-discovery.service.tsapps/api/src/modules/trigger/dlq-processor.service.spec.tsapps/api/src/modules/trigger/poller.service.spec.tsapps/api/src/modules/trigger/redis-trigger-store.tsapps/api/src/modules/trigger/trigger-executor.service.spec.tsapps/api/src/modules/trigger/trigger-executor.service.tsapps/api/src/modules/trigger/webhooks.controller.spec.tsapps/worker/package.jsonapps/worker/src/modules/pipeline/delivery.service.spec.tsapps/worker/src/modules/pipeline/delivery.service.tsdocs/architecture/master/tasks.mdpackages/connectors/package.jsonpackages/connectors/src/http-client/host-http-client.spec.tspackages/connectors/src/http-client/host-http-client.tspackages/connectors/src/index.tspackages/connectors/src/intelligence/interfaces.tspackages/connectors/src/intelligence/universal-trigger-engine.spec.tspackages/engine/package.jsonpackages/engine/src/pieces/piece-loader.service.tspackages/engine/src/pieces/piece-registry.service.spec.tspackages/engine/src/pieces/piece-registry.service.tspackages/engine/src/pieces/pieces.module.tspackages/engine/src/state/cursor-manager.service.spec.tspackages/engine/src/state/cursor-manager.service.tspackages/engine/src/state/cursor-manager.types.tspackages/piece-framework/package.jsonpackages/piece-framework/src/action.tspackages/piece-framework/src/auth.spec.tspackages/piece-framework/src/auth.tspackages/piece-framework/src/canonical/index.tspackages/piece-framework/src/http-client.tspackages/piece-framework/src/index.tspackages/piece-framework/src/piece.tspackages/piece-framework/src/property.tspackages/piece-framework/src/retryable-exception.tspackages/piece-framework/src/trigger.tspackages/piece-framework/tsconfig.jsonpackages/piece-framework/tsconfig.spec.jsonpackages/pieces/quickbooks/package.jsonpackages/pieces/quickbooks/src/index.tspackages/pieces/quickbooks/src/lib/auth.tspackages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.tspackages/pieces/quickbooks/src/triggers/universal-trigger.tspackages/pieces/salesforce/package.jsonpackages/pieces/salesforce/src/index.tspackages/pieces/salesforce/src/lib/auth.tspackages/pieces/salesforce/src/lib/common/index.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.spec.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-bulk.adapter.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-discovery.adapter.spec.tspackages/pieces/salesforce/src/lib/intelligence/salesforce-discovery.adapter.tspackages/pieces/salesforce/src/lib/sf-fetch.tspackages/pieces/salesforce/src/lib/trigger/salesforce-polling.helper.tspackages/pieces/salesforce/src/lib/trigger/universal-trigger.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/piece-framework/src/piece.ts (1)
304-313:⚠️ Potential issue | 🟠 MajorPrototype keys can break duplicate detection here.
These reducers build lookup maps on
{}and checkacc[name]by truthiness. A first action/trigger namedtoString,constructor, or__proto__will already look “present” viaObject.prototype, and__proto__can mutate the returned map. UseObject.create(null)plus an own-property check.🛠️ Suggested change
const actionsMap = params.actions.reduce( (acc, action) => { - if (acc[action.name]) { + if (Object.prototype.hasOwnProperty.call(acc, action.name)) { throw new PieceInternalServerError(`Duplicate action name: ${action.name}`); } acc[action.name] = action; return acc; }, - {} as Record<string, Action>, + Object.create(null) as Record<string, Action>, );const triggersMap = (params.triggers || []).reduce( (acc, trigger: Trigger) => { // Guard: skip entries that are not valid objects with a non-empty name if ( typeof trigger !== 'object' || @@ - if (acc[trigger.name]) { + if (Object.prototype.hasOwnProperty.call(acc, trigger.name)) { throw new PieceInternalServerError(`Duplicate trigger name: ${trigger.name}`); } acc[trigger.name] = trigger; return acc; }, - {} as Record<string, Trigger>, + Object.create(null) as Record<string, Trigger>, );Also applies to: 315-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/piece-framework/src/piece.ts` around lines 304 - 313, The duplicate-name detection in the actions map reducer uses a plain {} and checks acc[action.name] by truthiness, which breaks for inherited prototype keys like "toString" or "__proto__" (and allows __proto__ to mutate the map); change the accumulator initial value to Object.create(null) and use an own-property check such as Object.prototype.hasOwnProperty.call(acc, action.name) (or acc.hasOwnProperty if you first ensure a plain object) before throwing the PieceInternalServerError; apply the same Object.create(null) + own-property check fix to the analogous reducer that builds the triggers map (the other map in the same file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/master/tasks.md`:
- Line 615: Rename the Phase 8 heading to use a single consistent label across
sections by standardizing both occurrences to the same string (e.g., change
"Phase 8 — Fleet Sharded Custom Logic" and "Fleet Sharding" to the unified label
"Phase 8 — Fleet Sharding"); locate the two headings by their current text and
update them to the chosen phrase so all references and planning sections use the
identical Phase 8 name.
In `@packages/connectors/src/http-client/host-http-client.ts`:
- Around line 42-47: The onModuleInit in HostHttpClient currently swallows all
errors from initializeHttpClient; change it to only ignore an explicitly
idempotent re-init and rethrow any other failures: call
initializeHttpClient(this) inside try/catch, inspect the caught error (by error
type or message) and if it indicates "already initialized" (or a dedicated
IdempotentInitError from initializeHttpClient) return silently, otherwise
rethrow the error so real initialization failures (tokenManager/db/redis not set
up) are not hidden.
In `@packages/engine/src/pieces/piece-loader.service.ts`:
- Around line 130-132: The type guard isPiece currently only verifies the first
element of v['categories'], allowing mixed-type arrays; update the condition in
the isPiece guard to ensure v['categories'] is an array with length > 0 and that
every element is a string (e.g., replace the single-element typeof check with an
all-elements check using Array.prototype.every) so the guard truly narrows
categories to PieceCategory[].
In `@packages/piece-framework/src/http-client.ts`:
- Around line 10-20: The HttpRequest.interface exposes responseType but the host
implementation currently always calls response.text() and JSON.parse (in the
host HTTP client read/parse response code), so callers asking for binary/stream
will get wrong data; update the host client (the response handling function in
packages/connectors/src/http-client/host-http-client.ts that reads the fetch
Response) to respect HttpRequest.responseType by switching on its value (e.g.,
'json' -> response.json(), 'text' -> response.text(), 'arraybuffer'|'binary' ->
response.arrayBuffer(), 'stream' -> response.body) and return the
appropriately-typed payload, or if you prefer to remove the feature, delete
responseType from the HttpRequest interface and any usages; ensure the chosen
approach updates types/signatures where response is returned and add tests
covering json/text/binary/stream paths.
In `@packages/piece-framework/tsconfig.json`:
- Around line 9-16: The package build tsconfig currently includes
"vitest/globals" in "types" and "src/**/*" in "include", which pulls tests into
the production build; remove the vitest type reference from the "types" array
and restrict "include" to production source (e.g., "src/**/*.ts" or a dedicated
src folder excluding spec files), and rely on tsconfig.spec.json for test
typing; update the tsconfig.json's "types" and "include" entries accordingly so
spec files and vitest types are not part of the emitted dist.
In `@packages/piece-framework/tsconfig.spec.json`:
- Around line 2-8: The spec tsconfig currently inherits outDir and
tsBuildInfoFile from the package tsconfig which causes tsc -p
packages/piece-framework/tsconfig.spec.json to write into the package build
outputs; update packages/piece-framework/tsconfig.spec.json to set "noEmit":
true and override "tsBuildInfoFile" to a dedicated file (e.g.
"./dist/tsconfig.spec.tsbuildinfo") under the "compilerOptions" so the spec
build won’t clobber the package dist and will use an isolated build-info file.
In `@packages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.ts`:
- Line 6: The parseLimit function currently coerces blank strings to 0 via
Number(), creating unbounded queries; fix it by validating the incoming limit
(the limit?: number | string param) before coercion: if typeof limit ===
'string' then if limit.trim() === '' treat it as invalid (either return
undefined to omit MAXRESULTS or throw a validation error) and only call Number()
on non-empty strings, then keep the existing NaN and negative checks; update
parseLimit so blank/whitespace-only strings do not fall through into a numeric 0
and thereby suppress the MAXRESULTS clause.
---
Outside diff comments:
In `@packages/piece-framework/src/piece.ts`:
- Around line 304-313: The duplicate-name detection in the actions map reducer
uses a plain {} and checks acc[action.name] by truthiness, which breaks for
inherited prototype keys like "toString" or "__proto__" (and allows __proto__ to
mutate the map); change the accumulator initial value to Object.create(null) and
use an own-property check such as Object.prototype.hasOwnProperty.call(acc,
action.name) (or acc.hasOwnProperty if you first ensure a plain object) before
throwing the PieceInternalServerError; apply the same Object.create(null) +
own-property check fix to the analogous reducer that builds the triggers map
(the other map in the same file).
🪄 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: 661520ba-fd1d-433e-905d-51e124db8e32
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/worker/src/modules/pipeline/fanout.service.spec.tsapps/worker/src/modules/pipeline/normalization.service.spec.tsapps/worker/src/modules/pipeline/replica.service.spec.tsdocs/architecture/master/tasks.mdpackages/connectors/src/http-client/host-http-client.spec.tspackages/connectors/src/http-client/host-http-client.tspackages/engine/src/pieces/piece-loader.service.tspackages/piece-framework/src/http-client.tspackages/piece-framework/src/piece.tspackages/piece-framework/tsconfig.jsonpackages/piece-framework/tsconfig.spec.jsonpackages/pieces/quickbooks/package.jsonpackages/pieces/quickbooks/src/triggers/quickbooks-polling.helper.tspackages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.tspackages/pieces/salesforce/package.json
|
|
||
| --- | ||
|
|
||
| ## Phase 8 — Fleet Sharded Custom Logic |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a single Phase 8 label across sections for consistency.
Line 615 uses “Fleet Sharded Custom Logic” while Line 648 uses “Fleet Sharding.” Consider standardizing to one name to avoid ambiguity in planning references.
Also applies to: 648-648
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/master/tasks.md` at line 615, Rename the Phase 8 heading to
use a single consistent label across sections by standardizing both occurrences
to the same string (e.g., change "Phase 8 — Fleet Sharded Custom Logic" and
"Fleet Sharding" to the unified label "Phase 8 — Fleet Sharding"); locate the
two headings by their current text and update them to the chosen phrase so all
references and planning sections use the identical Phase 8 name.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.ts (1)
55-60:⚠️ Potential issue | 🟠 MajorBlank string limits still drop the adapter’s default page size.
parseLimit(undefined)still returns100, but''/ whitespace now returnsundefined, so Line 24 omitsMAXRESULTSentirely instead of using the existing default. A cleared input therefore behaves differently from an omitted one and can unexpectedly expand the query. Normalize blank strings to the default branch, or reject them, rather than returningundefinedhere.Proposed fix
- private static parseLimit(limit?: number | string): number | undefined { - if (limit === undefined) return 100; - if (limit === 0) return 0; - - if (typeof limit === 'string' && limit.trim() === '') { - return undefined; - } + private static parseLimit(limit?: number | string): number { + if (typeof limit === 'string') { + limit = limit.trim(); + if (limit === '') return 100; + } + if (limit === undefined) return 100; + if (limit === 0) return 0; const parsed = Number(limit);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.ts` around lines 55 - 60, The parseLimit function treats blank/whitespace strings as undefined which causes MAXRESULTS to be omitted; change parseLimit (in quickbooks-query.adapter.ts) so that empty or whitespace-only string inputs are normalized to the same default as undefined (return 100) instead of returning undefined (or alternatively throw for invalid input), ensuring cleared input behaves like omitted input and preserves the adapter's default page size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/connectors/src/http-client/host-http-client.spec.ts`:
- Line 1: The test file imports Vitest helpers but omits afterEach, causing a
ReferenceError where afterEach is used; update the import line that currently
lists "describe, expect, it, vi, beforeAll" to also include "afterEach" so the
teardown hook is defined (ensure the import statement includes afterEach
alongside describe, expect, it, vi, beforeAll to match usage later in the file).
In `@packages/engine/src/pieces/piece-loader.service.ts`:
- Around line 130-132: The runtime type guard in piece-loader.service.ts is
erroneously rejecting valid pieces by requiring v['categories'].length > 0;
update the check used by isPiece() (or the local validator around
v['categories']) to allow an empty array (i.e., remove the length > 0
requirement) and only assert Array.isArray(v['categories']) &&
v['categories'].every(c => typeof c === 'string'), so pieces defaulted to [] by
createPiece() are accepted during loading.
In `@packages/piece-framework/tsconfig.spec.json`:
- Around line 6-9: Add a vitest.config.ts that enables Vitest globals to match
the tsconfig.spec.json declaration: create a vitest.config.ts exporting
defineConfig(...) and set test.globals: true, environment: 'node', include to
'src/**/*.spec.ts', and coverage settings (provider 'v8', reporters,
include/exclude patterns) so runtime globals align with the "vitest/globals"
types declared in tsconfig.spec.json; ensure the file exports the configuration
as default using defineConfig from 'vitest/config'.
In `@packages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.ts`:
- Around line 1-6: Several tests pass an extra objectName property that no
longer exists on the QBOQuerySpec type; open the spec that constructs
QBOQuerySpec literals and remove the objectName field from each of the eight
failing test cases (the object literals that are passed where QBOQuerySpec is
expected), ensuring each test object only uses cursorField, cursorValue,
cursorIdField, cursorIdValue, and limit; alternatively revert the QBOQuerySpec
narrowing if you prefer to keep objectName.
---
Duplicate comments:
In `@packages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.ts`:
- Around line 55-60: The parseLimit function treats blank/whitespace strings as
undefined which causes MAXRESULTS to be omitted; change parseLimit (in
quickbooks-query.adapter.ts) so that empty or whitespace-only string inputs are
normalized to the same default as undefined (return 100) instead of returning
undefined (or alternatively throw for invalid input), ensuring cleared input
behaves like omitted input and preserves the adapter's default page size.
🪄 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: e18dbf16-d05c-4ddb-b034-78aed7cdf6b4
📒 Files selected for processing (9)
docs/architecture/master/tasks.mdpackages/connectors/src/http-client/host-http-client.spec.tspackages/connectors/src/http-client/host-http-client.tspackages/engine/src/pieces/piece-loader.service.tspackages/piece-framework/src/http-client.tspackages/piece-framework/src/piece.tspackages/piece-framework/tsconfig.jsonpackages/piece-framework/tsconfig.spec.jsonpackages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/connectors/src/http-client/host-http-client.spec.ts`:
- Around line 18-25: The test's prototype spy sendRequestSpy is only restored at
the end of individual test flows, which can leak across tests if an assertion
fails; add a suite-level cleanup in the outer describe (paired with the existing
beforeAll initializeHttpClient call) that always restores sendRequestSpy (and
any other spies set in the suite) in an afterAll/afterEach hook so restoration
runs regardless of test failures; locate where sendRequestSpy is created in the
spec (and where initializeHttpClient/HostHttpClient are used) and move or add
the spy restoration to a top-level afterAll (or afterEach) to guarantee cleanup
on failure paths.
In `@packages/piece-framework/vitest.config.ts`:
- Line 7: The Vitest config's include pattern (the include array in
vitest.config.ts) only matches '*.spec.ts' and will miss tests named
'*.test.ts'; update the include value to cover both patterns (e.g., include
'*.spec.*' and '*.test.*' or explicit patterns like 'src/**/*.spec.ts' and
'src/**/*.test.ts') so that functions/tests discovered by Vitest (defined by the
include array) do not get silently skipped.
In `@packages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.ts`:
- Line 24: The parseLimit function currently types its return as number |
undefined but never returns undefined; change its signature to return number,
ensure all code paths return a number (e.g., 100, 0, or parsed value) and remove
any unreachable undefined returns; then simplify the consumer check in
quickbooks-query.adapter.ts (the block using safeLimit, e.g., the if currently
written as if (safeLimit !== 0 && safeLimit !== undefined)) to just check if
(safeLimit !== 0). Update any other callers/types that rely on parseLimit to
reflect the new number return type.
🪄 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: bb930ed4-e578-4c7b-bb2d-107573ccf4df
📒 Files selected for processing (5)
packages/connectors/src/http-client/host-http-client.spec.tspackages/engine/src/pieces/piece-loader.service.tspackages/piece-framework/vitest.config.tspackages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.spec.tspackages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.ts
💤 Files with no reviewable changes (1)
- packages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/connectors/src/http-client/host-http-client.spec.ts`:
- Around line 199-209: Add a test that covers non-ok HTTP responses for
HostHttpClient.sendRequest: spy on global fetch to return ok: false (e.g.,
status 404) with a JSON body and assert the client's behavior (either that
client.sendRequest rejects with an error or returns the expected error
structure). Update host-http-client.spec.ts to include this case using the same
client instance and HttpMethod.GET so we validate how sendRequest handles non-ok
responses.
🪄 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: 58fd3180-cb6a-4031-ad81-3a95a52c69d0
📒 Files selected for processing (3)
packages/connectors/src/http-client/host-http-client.spec.tspackages/piece-framework/vitest.config.tspackages/pieces/quickbooks/src/triggers/quickbooks-query.adapter.ts
This was referenced Apr 6, 2026
Merged
This was referenced Apr 17, 2026
This was referenced Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
Chores
Refactor
Bug Fixes