Refactor: migrate connection-manager to credentials and intelligence to discovery - #121
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:
📝 WalkthroughWalkthroughReorganizes package boundaries and exports: renames/moves connector symbols into Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger as UniversalTrigger
participant OptSvc as OptimizationService
participant Registry as StaticRegistry
participant Resolver as ConnectionHintResolver
Trigger->>OptSvc: getHint(appName, objectName, connectionId?)
OptSvc->>Registry: read static hint for appName/objectName
Registry-->>OptSvc: static ObjectHint
OptSvc->>Resolver: if connectionId && resolver set, invoke resolver(appName, objectName, connectionId)
Resolver-->>OptSvc: resolver hint or undefined
OptSvc->>OptSvc: merge(staticHint, resolverHint if present)
OptSvc-->>Trigger: return merged ObjectHint
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/connections/connectors.service.ts (1)
33-36:⚠️ Potential issue | 🟡 MinorStale type-alias comment references old package.
Line 35 still says the alias comes from the connectors package, but it now comes from credentials. Please update the comment to prevent confusion.
Suggested doc fix
/** * Encrypted value blob stored in app_connection.value. - * Aliased from the connectors package so all callers share a single source of truth. + * Aliased from the credentials package so all callers share a single source of truth. */ export type ConnectionValueBlob = OAuthCredentialBlob;🤖 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 33 - 36, Update the doc comment above the Encrypted value blob declaration in connectors.service.ts to reflect the correct source package: replace the stale reference to "connectors" with "credentials" so it reads that the alias is from the credentials package and that callers share a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/modules/connections/api/connections.api.ts`:
- Around line 46-52: Update the frontend endpoint paths in
apps/web/src/modules/connections/api/connections.api.ts so they match the
backend controller namespace: replace any apiClient.get/post calls using
"/connection-manager/..." with "/connectors/..." (e.g. update the call inside
listActiveConnections and the other apiClient.* calls that fetch providers,
active connections, and related resources) so the routes align with the backend
`@Controller`('connectors') endpoints.
In `@engine/platform/piece-framework/src/discovery/optimization-registry.ts`:
- Around line 4-11: Add a reset function to avoid test pollution by clearing the
module-scoped resolver: implement and export clearOptimizationResolver() which
sets the customResolver (the module variable) back to null; keep the existing
setOptimizationResolver(resolver: ConnectionHintResolver) and
ConnectionHintResolver type unchanged so tests can set and then call
clearOptimizationResolver() to restore default state.
In `@engine/platform/piece-framework/src/discovery/universal-trigger.ts`:
- Line 5: The logger instance uses app: 'universal-engine' which is inconsistent
with the renamed class UniversalTrigger; update the IgtLogger instantiation (the
log variable created with new IgtLogger) to use app: 'universal-trigger' so
logs/filters align with the class name (change the app field value in the
IgtLogger call where log is created).
In `@engine/platform/piece-framework/src/piece.ts`:
- Around line 20-25: The header comment incorrectly says the polling types are
defined in packages/connection-manager; update the comment to state they are
defined in this piece-framework module (engine/platform/piece-framework) and
used by piece.poll() and the SchedulerWorker, and mention the actual types
defined here (PollWindow, PollRecord, PollPage, StreamDescriptor, etc.) so the
comment accurately reflects the current location and purpose of these types.
In `@TECHNICAL_DEBT.md`:
- Around line 9-15: There are duplicate "### 1." headings in TECHNICAL_DEBT.md
(e.g., the "AI Orchestrator & MCP Architecture Refactoring" section and the
later "### 1." at line ~28); update the markdown headings to use a consistent,
incremental numbering scheme (e.g., 1., 2., 3.) or convert to auto-numbered
lists so each high-priority item is uniquely numbered — locate headings by their
text like "AI Orchestrator & MCP Architecture Refactoring" and the subsequent
"### 1." entries and rename them sequentially to restore clear navigation.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 33-36: Update the doc comment above the Encrypted value blob
declaration in connectors.service.ts to reflect the correct source package:
replace the stale reference to "connectors" with "credentials" so it reads that
the alias is from the credentials package and that callers share a single source
of truth.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bddd4b0-d0ce-4b68-a46a-676583a1afad
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (57)
TECHNICAL_DEBT.mdapps/api/package.jsonapps/api/src/modules/ai/_services/orchestrator.service.spec.tsapps/api/src/modules/ai/_services/orchestrator.service.tsapps/api/src/modules/connections/connections.module.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/connections/connectors.controller.tsapps/api/src/modules/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/metadata/metadata-discovery.service.spec.tsapps/api/src/modules/metadata/metadata-discovery.service.tsapps/api/src/modules/scheduler/poll-sync-runner.spec.tsapps/api/src/modules/scheduler/poll-sync-runner.tsapps/api/src/modules/scheduler/scheduler.module.tsapps/api/src/modules/stitches/stitches.module.tsapps/web/src/modules/connections/api/connections.api.tsapps/web/src/modules/connections/hooks/useConnections.tsapps/web/src/modules/stitches/api/metadata.api.tsapps/worker/package.jsonapps/worker/src/modules/pipeline/delivery.service.spec.tsapps/worker/src/modules/pipeline/delivery.service.tsengine/application/pieces/quickbooks/package.jsonengine/application/pieces/quickbooks/src/triggers/quickbooks-polling.helper.tsengine/application/pieces/quickbooks/src/triggers/universal-trigger.tsengine/application/pieces/salesforce/package.jsonengine/application/pieces/salesforce/src/lib/discovery/index.tsengine/application/pieces/salesforce/src/lib/discovery/salesforce-bulk.adapter.spec.tsengine/application/pieces/salesforce/src/lib/discovery/salesforce-bulk.adapter.tsengine/application/pieces/salesforce/src/lib/discovery/salesforce-discovery.adapter.spec.tsengine/application/pieces/salesforce/src/lib/discovery/salesforce-discovery.adapter.tsengine/application/pieces/salesforce/src/lib/discovery/salesforce-query.adapter.spec.tsengine/application/pieces/salesforce/src/lib/discovery/salesforce-query.adapter.tsengine/application/pieces/salesforce/src/lib/trigger/universal-trigger.tsengine/platform/core/package.jsonengine/platform/credentials/package.jsonengine/platform/credentials/src/crypto/encryption.interface.tsengine/platform/credentials/src/crypto/encryption.service.spec.tsengine/platform/credentials/src/crypto/encryption.service.tsengine/platform/credentials/src/http-client/host-http-client.spec.tsengine/platform/credentials/src/http-client/host-http-client.tsengine/platform/credentials/src/index.tsengine/platform/credentials/src/oauth/token-manager.service.tsengine/platform/credentials/tsconfig.jsonengine/platform/credentials/tsconfig.spec.jsonengine/platform/credentials/vitest.config.tsengine/platform/piece-framework/package.jsonengine/platform/piece-framework/src/discovery/igt-logger.tsengine/platform/piece-framework/src/discovery/index.tsengine/platform/piece-framework/src/discovery/interfaces.tsengine/platform/piece-framework/src/discovery/optimization-registry.tsengine/platform/piece-framework/src/discovery/smart-cursor-selector.tsengine/platform/piece-framework/src/discovery/universal-trigger.spec.tsengine/platform/piece-framework/src/discovery/universal-trigger.tsengine/platform/piece-framework/src/piece.tspackages/infra-adapters/src/adapters/local-crypto.adapter.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 5 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 5 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
engine/platform/piece-framework/src/discovery/optimization-registry.ts (1)
74-76:⚠️ Potential issue | 🟡 MinorJSDoc is outdated after the DB-to-resolver migration.
The comment still references "DB-backed profile cache" but the implementation now uses a pluggable
ConnectionHintResolver. Update to reflect the current behavior.📝 Proposed fix
- * `@param` connectionId - Optional app_connection.id. When provided, queries the DB-backed - * profile cache (scoped per-connection for custom object support). - * When absent, falls straight through to the static registry. + * `@param` connectionId - Optional connection identifier. When provided and a custom resolver + * is registered, calls the resolver for per-connection hints. + * When absent (or no resolver set), falls back to the static registry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/platform/piece-framework/src/discovery/optimization-registry.ts` around lines 74 - 76, Update the outdated JSDoc for the connectionId parameter to reflect the new DB-to-resolver design: replace references to the "DB-backed profile cache" with the pluggable ConnectionHintResolver and explain that when a connectionId is provided the registry consults the ConnectionHintResolver (scoped per-connection for custom object support) and when absent it falls back to the static registry; ensure you mention the ConnectionHintResolver symbol and the connectionId parameter name in the comment near the optimization registry functions/classes so readers know the current behavior.engine/platform/piece-framework/src/discovery/universal-trigger.ts (1)
218-218: 🧹 Nitpick | 🔵 TrivialRemove unnecessary
as anycast.The
configparameter is already typed asUniversalTriggerConfig<any>, which includesobjectName,hint,queryAdapter,bulkAdapter, andexecuteStandardQuery. The cast is redundant and reduces type safety.♻️ Proposed fix
- const { objectName, hint, queryAdapter, bulkAdapter, executeStandardQuery } = config as any; + const { objectName, hint, queryAdapter, bulkAdapter, executeStandardQuery } = config;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/platform/piece-framework/src/discovery/universal-trigger.ts` at line 218, Remove the unnecessary "as any" cast on the destructuring of config in universal-trigger.ts: use the existing strongly-typed parameter (UniversalTriggerConfig<any>) directly when extracting objectName, hint, queryAdapter, bulkAdapter, and executeStandardQuery (i.e., replace "const { ... } = config as any" with "const { ... } = config") so you retain type safety and let TypeScript validate those properties on the config parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/platform/piece-framework/src/discovery/optimization-registry.ts`:
- Around line 84-86: The variable name dbHint is misleading because its value
comes from customResolver; rename dbHint to resolverHint (or customHint)
everywhere in the function where customResolver(appName, objectName,
connectionId) is awaited and merged with staticHint, updating the declaration,
the await assignment, the conditional (e.g., if (resolverHint &&
Object.keys(resolverHint).length > 0)), and the returned merged object (return {
...staticHint, ...resolverHint }) to keep naming consistent with customResolver.
- Line 3: The constant VALID_EXECUTION_PATHS is declared but unused; remove the
declaration "VALID_EXECUTION_PATHS" from optimization-registry.ts to eliminate
dead code (or alternatively, if you intended runtime checks for ExecutionPath
values, implement and call a validator that references VALID_EXECUTION_PATHS
where ExecutionPath strings are parsed/validated such as in any factory or
deserialization function that accepts ExecutionPath). Ensure any imports or
references to ExecutionPath remain valid after removal.
In `@engine/platform/piece-framework/src/discovery/universal-trigger.ts`:
- Line 298: The file ends with a closing brace '}' but lacks a trailing newline;
open universal-trigger.ts, add a single newline character at the end of the file
(after the final '}') so the file terminates with '\n', save and commit the
change to satisfy POSIX/EditorConfig conventions.
In `@TECHNICAL_DEBT.md`:
- Line 459: Add a single trailing newline at the end of TECHNICAL_DEBT.md so the
file ends with exactly one newline character (fixing markdownlint MD047); update
the file's EOF to include the newline, save/commit the change, and re-run
linting to confirm the warning is resolved.
---
Outside diff comments:
In `@engine/platform/piece-framework/src/discovery/optimization-registry.ts`:
- Around line 74-76: Update the outdated JSDoc for the connectionId parameter to
reflect the new DB-to-resolver design: replace references to the "DB-backed
profile cache" with the pluggable ConnectionHintResolver and explain that when a
connectionId is provided the registry consults the ConnectionHintResolver
(scoped per-connection for custom object support) and when absent it falls back
to the static registry; ensure you mention the ConnectionHintResolver symbol and
the connectionId parameter name in the comment near the optimization registry
functions/classes so readers know the current behavior.
In `@engine/platform/piece-framework/src/discovery/universal-trigger.ts`:
- Line 218: Remove the unnecessary "as any" cast on the destructuring of config
in universal-trigger.ts: use the existing strongly-typed parameter
(UniversalTriggerConfig<any>) directly when extracting objectName, hint,
queryAdapter, bulkAdapter, and executeStandardQuery (i.e., replace "const { ...
} = config as any" with "const { ... } = config") so you retain type safety and
let TypeScript validate those properties on the config parameter.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a52f9bfc-46b2-409a-9207-5607d8a9843c
📒 Files selected for processing (5)
TECHNICAL_DEBT.mdapps/web/src/modules/connections/api/connections.api.tsengine/platform/piece-framework/src/discovery/optimization-registry.tsengine/platform/piece-framework/src/discovery/universal-trigger.tsengine/platform/piece-framework/src/piece.ts
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add trailing newline at end of file.
The file should end with a newline character per standard conventions.
📝 Proposed fix
}
+📝 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
| } | |
| } | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/platform/piece-framework/src/discovery/universal-trigger.ts` at line
298, The file ends with a closing brace '}' but lacks a trailing newline; open
universal-trigger.ts, add a single newline character at the end of the file
(after the final '}') so the file terminates with '\n', save and commit the
change to satisfy POSIX/EditorConfig conventions.
|
|
||
| 1. **API Modernization**: Proposed: delete `ProviderRegistryService` and update `connectors.controller.ts` and `connectors.service.ts` to use `PieceAuth` definitions from `PieceRegistryService` for mapping `tokenUrl`, `authUrl`, and `clientId` dynamically. | ||
| 2. **Frontend Simplification**: Proposed: remove hardcoded `env` in `DynamicAuthForm.tsx` and `oauth-state.service.ts`; render environment via `uiSchema`/`vendorParams` so any custom field flows through generically. | ||
| 2. **Frontend Simplification**: Proposed: remove hardcoded `env` in `DynamicAuthForm.tsx` and `oauth-state.service.ts`; render environment via `uiSchema`/`vendorParams` so any custom field flows through generically. No newline at end of file |
Contributor
There was a problem hiding this comment.
Add trailing newline at end of file.
Static analysis (markdownlint MD047) indicates the file should end with a single newline character.
📝 Proposed fix
-2. **Frontend Simplification**: Proposed: remove hardcoded `env` in `DynamicAuthForm.tsx` and `oauth-state.service.ts`; render environment via `uiSchema`/`vendorParams` so any custom field flows through generically.
+2. **Frontend Simplification**: Proposed: remove hardcoded `env` in `DynamicAuthForm.tsx` and `oauth-state.service.ts`; render environment via `uiSchema`/`vendorParams` so any custom field flows through generically.
+📝 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
| 2. **Frontend Simplification**: Proposed: remove hardcoded `env` in `DynamicAuthForm.tsx` and `oauth-state.service.ts`; render environment via `uiSchema`/`vendorParams` so any custom field flows through generically. | |
| 2. **Frontend Simplification**: Proposed: remove hardcoded `env` in `DynamicAuthForm.tsx` and `oauth-state.service.ts`; render environment via `uiSchema`/`vendorParams` so any custom field flows through generically. | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 459-459: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TECHNICAL_DEBT.md` at line 459, Add a single trailing newline at the end of
TECHNICAL_DEBT.md so the file ends with exactly one newline character (fixing
markdownlint MD047); update the file's EOF to include the newline, save/commit
the change, and re-run linting to confirm the warning is resolved.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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 (1)
engine/platform/piece-framework/src/discovery/optimization-registry.ts (1)
73-75:⚠️ Potential issue | 🟡 MinorUpdate stale JSDoc comment.
The documentation still references "queries the DB-backed profile cache" but the implementation now uses the pluggable
customResolver. Update the comment to accurately describe the current behavior.📝 Proposed fix
/** * Look up execution hints for a specific object. * * `@param` appName - Canonical app name (used for static OPTIMIZATION_REGISTRY fallback). * `@param` objectName - Vendor object name e.g. 'rtms__Load__c'. - * `@param` connectionId - Optional app_connection.id. When provided, queries the DB-backed - * profile cache (scoped per-connection for custom object support). - * When absent, falls straight through to the static registry. + * `@param` connectionId - Optional app_connection.id. When provided, invokes the custom + * resolver (if set) for connection-scoped hints. + * When absent or no resolver is set, falls through to the static registry. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/platform/piece-framework/src/discovery/optimization-registry.ts` around lines 73 - 75, Update the stale JSDoc for the connectionId param in OptimizationRegistry (in optimization-registry.ts) to reflect that when connectionId is provided the registry will consult the pluggable customResolver (not a DB-backed profile cache) to resolve per-connection/custom-object profiles, and when absent it falls back to the static registry; specifically edit the JSDoc for the connectionId parameter near the OptimizationRegistry methods (or class) to mention customResolver and per-connection scoping instead of "DB-backed profile cache".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@engine/platform/piece-framework/src/discovery/optimization-registry.ts`:
- Around line 73-75: Update the stale JSDoc for the connectionId param in
OptimizationRegistry (in optimization-registry.ts) to reflect that when
connectionId is provided the registry will consult the pluggable customResolver
(not a DB-backed profile cache) to resolve per-connection/custom-object
profiles, and when absent it falls back to the static registry; specifically
edit the JSDoc for the connectionId parameter near the OptimizationRegistry
methods (or class) to mention customResolver and per-connection scoping instead
of "DB-backed profile cache".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d340cf39-3c5c-4033-b080-0ebff19b2aee
📒 Files selected for processing (1)
engine/platform/piece-framework/src/discovery/optimization-registry.ts
…com/pramodnarayana/nexiom into refactor/arch-credentials-discovery
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/platform/piece-framework/src/discovery/optimization-registry.ts`:
- Around line 63-65: defineHints currently overwrites entire ObjectHint entries
for an app instead of merging fields; change defineHints to preserve existing
per-object defaults by merging each object key: for each object name in hints,
merge the existing OPTIMIZATION_REGISTRY[appName][objectName] with the new
hints[objectName] (e.g., spread existing then spread new) so partial overrides
(like updating bulkThreshold) don't drop cursorPrecedence/requiredFields; ensure
you initialize OPTIMIZATION_REGISTRY[appName] = {} when missing and update
OPTIMIZATION_REGISTRY[appName] with the merged per-object entries.
- Around line 77-97: getHint currently returns references to registry-owned
objects (staticHint and merged resolverHint), so callers can mutate
module-scoped defaults; update getHint (and the merge path) to return a
defensive copy: create a new ObjectHint object when returning (for both the
static-only return and the {...staticHint, ...resolverHint} case) and clone any
array-valued properties that should be isolated (e.g., requiredFields, autoJoin,
cursorPrecedence) so the returned hint contains new arrays rather than
references to those stored in OPTIMIZATION_REGISTRY or resolverHint; ensure you
handle undefined staticHint/resolverHint gracefully by only cloning existing
arrays.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5cfc329-77d5-457b-a277-43f1f376dddc
📒 Files selected for processing (3)
apps/api/src/modules/connections/connectors.service.tsengine/platform/piece-framework/src/discovery/optimization-registry.tsengine/platform/piece-framework/src/discovery/universal-trigger.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 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/platform/piece-framework/src/discovery/optimization-registry.ts`:
- Around line 69-74: The incoming hints are being stored by reference which
allows external mutation to change module-wide defaults; extract the private
cloneHint method into a module-level function named cloneHint and replace all
internal usages (including the places where getHint(), defineHints(), and the
previous private cloneHint were called) so that defineHints() clones both the
incoming hints and the existing OPTIMIZATION_REGISTRY[appName][objectName]
before merging, and update the other call sites (the previous private cloneHint
callers around the code that currently invokes cloning) to use the new
module-level cloneHint so arrays like cursorPrecedence, autoJoin, and
requiredFields are deeply copied prior to storage.
- Around line 36-61: OPTIMIZATION_REGISTRY and per-app hint buckets are plain
objects and allow prototype pollution via defineHints(); change the registry and
any per-app bucket creation to use null-prototype maps (Object.create(null))
instead of object literals so inherited keys can't be abused, and update
defineHints() to create a null-prototype bucket for
OPTIMIZATION_REGISTRY[appName] (and any similar write sites) rather than
assigning a plain {}; keep getHint()/AppHints usage compatible but ensure any
existence checks don't rely on prototype inheritance (use hasOwnProperty on the
null-prototype map or explicit undefined checks).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7881e441-f742-40bb-bfc0-490cfc669801
📒 Files selected for processing (1)
engine/platform/piece-framework/src/discovery/optimization-registry.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 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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
Bug Fixes
Documentation
Refactor
New Features