refactor packages architecture - #158
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR restructures encryption service delivery and consolidates domain packages. The ChangesEncryption Service Abstraction and Domain Core Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/credentials/src/index.ts (1)
2-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the entrypoint header to match the new public surface.
Line 5 still advertises
@soopa/credentialsas the encryption entrypoint, but Line 9 says the crypto utilities moved to@soopa/security. That top-level API comment is now misleading.Proposed diff
- * - `@soopa/credentials` — runtime services (encryption, token manager) + * - `@soopa/credentials` — runtime services (token manager, OAuth refresh helpers)🤖 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/credentials/src/index.ts` around lines 2 - 10, Update the top-of-file header comment in packages/credentials/src/index.ts so it accurately reflects the public surface: remove or stop advertising encryption/crypto as provided by `@soopa/credentials` and instead state that `@soopa/credentials` exposes runtime services (e.g., token manager) while crypto utilities are provided by `@soopa/security`; keep the mention of `@soopa/piece-framework` for piece/action/trigger/auth/property definitions. Locate the header block in index.ts and edit the entrypoints list and any descriptive lines referencing encryption to reference `@soopa/security` for crypto and `@soopa/credentials` only for runtime services.apps/api/src/modules/connections/connections.module.ts (1)
54-77:⚠️ Potential issue | 🔴 CriticalFix ENCRYPTION_SERVICE wiring: register
EncryptionModule.forRootAsync(...)at bootstrap
apps/api/src/modules/connections/connections.module.tsandpackages/pipeline/src/pipeline-core.module.tsboth injectENCRYPTION_SERVICEintoTokenManagerService, but neither module importsEncryptionModule.apps/api/src/app/app.module.tsandapps/worker/src/app.module.tsalso do not registerEncryptionModule.forRootAsync(...).ENCRYPTION_SERVICEis only provided byEncryptionModule.forRootAsync(...)inpackages/security/src/encryption.module.ts; repo-wide it’s only referenced in docs/specs, so Nest will fail to resolve the token at startup.Add/import
EncryptionModule.forRootAsync(...)once in the API and worker app modules (or a shared global/infra module that’s actually imported) soENCRYPTION_SERVICEexists in the DI graph.🤖 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/connections/connections.module.ts` around lines 54 - 77, TokenManagerService is injecting ENCRYPTION_SERVICE but no module registers that provider; import and register EncryptionModule.forRootAsync(...) into the application DI graph so ENCRYPTION_SERVICE is available. Update the top-level application module(s) that bootstrap the API and worker (where TokenManagerService is ultimately used) to import EncryptionModule.forRootAsync(...) (or add a shared/global InfraModule that imports EncryptionModule.forRootAsync(...) and then import that module into the app modules) so the ENCRYPTION_SERVICE token is provided before TokenManagerService is constructed.
🤖 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 `@packages/credentials/src/oauth/token-manager.service.ts`:
- Line 6: The import mixes a type and a value from `@soopa/security` which breaks
with verbatimModuleSyntax; change the combined import to a type-only import for
IEncryptionService and keep ENCRYPTION_SERVICE as a value import (e.g., use an
import type { IEncryptionService } for the interface and a regular import {
ENCRYPTION_SERVICE } for the token). Apply this change in
token-manager.service.ts and the other credentials OAuth files that import
IEncryptionService/ENCRYPTION_SERVICE so the type is erased at emit and the
runtime token remains a value import.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connections.module.ts`:
- Around line 54-77: TokenManagerService is injecting ENCRYPTION_SERVICE but no
module registers that provider; import and register
EncryptionModule.forRootAsync(...) into the application DI graph so
ENCRYPTION_SERVICE is available. Update the top-level application module(s) that
bootstrap the API and worker (where TokenManagerService is ultimately used) to
import EncryptionModule.forRootAsync(...) (or add a shared/global InfraModule
that imports EncryptionModule.forRootAsync(...) and then import that module into
the app modules) so the ENCRYPTION_SERVICE token is provided before
TokenManagerService is constructed.
In `@packages/credentials/src/index.ts`:
- Around line 2-10: Update the top-of-file header comment in
packages/credentials/src/index.ts so it accurately reflects the public surface:
remove or stop advertising encryption/crypto as provided by `@soopa/credentials`
and instead state that `@soopa/credentials` exposes runtime services (e.g., token
manager) while crypto utilities are provided by `@soopa/security`; keep the
mention of `@soopa/piece-framework` for piece/action/trigger/auth/property
definitions. Locate the header block in index.ts and edit the entrypoints list
and any descriptive lines referencing encryption to reference `@soopa/security`
for crypto and `@soopa/credentials` only for runtime services.
🪄 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: 95953bfe-ca57-4574-8ee7-04488642bec2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
apps/api/package.jsonapps/api/src/modules/connections/connections.module.tsapps/api/src/modules/connections/connections/credential.controller.tsapps/api/src/modules/connections/connections/registry-token-refresh.service.spec.tsapps/api/src/modules/connections/connections/registry-token-refresh.service.tsapps/api/src/modules/stitches/stitches.module.tsapps/worker/package.jsonpackages/credentials/package.jsonpackages/credentials/src/crypto/encryption.interface.tspackages/credentials/src/crypto/encryption.service.spec.tspackages/credentials/src/crypto/encryption.service.tspackages/credentials/src/index.tspackages/credentials/src/oauth/token-manager.service.spec.tspackages/credentials/src/oauth/token-manager.service.tspackages/credentials/src/oauth/token-refresh.service.spec.tspackages/credentials/src/oauth/token-refresh.service.tspackages/domain/core/package.jsonpackages/domain/core/tsconfig.lib.jsonpackages/domain/core/vitest.config.tspackages/domain/tms/package.jsonpackages/domain/tms/src/index.tspackages/domain/tms/src/schema/tms-identifier-validator.tspackages/domain/tms/src/schema/tms-provisioner.tspackages/domain/tms/src/schema/tms-schema.tspackages/domain/tms/src/tms-normalized-writer.tspackages/domain/tms/src/tms-target-builder.tspackages/domain/tms/tsconfig.jsonpackages/domain/tms/tsconfig.lib.jsonpackages/domain/tms/vitest.config.tspackages/pipeline/package.jsonpackages/pipeline/src/delivery/delivery.service.tspackages/pipeline/src/delivery/use-cases/claim-delivery.use-case.tspackages/pipeline/src/pipeline-core.module.tspackages/pipeline/src/replication/registry-replication.service.spec.tspackages/pipeline/src/replication/registry-replication.service.tspackages/pipeline/src/replication/registry-token-refresh.service.integration.spec.tspackages/pipeline/src/replication/registry-token-refresh.service.tspackages/pipeline/src/replication/replica.service.spec.tspackages/pipeline/src/replication/replica.service.tspackages/pipeline/src/shared/adapters/outbound-gateway.adapter.tspackages/pipeline/src/shared/adapters/registry-replication.adapter.tspackages/pipeline/src/shared/adapters/replica-state.adapter.tspackages/pipeline/src/shared/domain.tspackages/pipeline/src/shared/events/domain-events.tspackages/pipeline/src/shared/logic/delivery-status.evaluator.spec.tspackages/pipeline/src/shared/logic/delivery-status.evaluator.tspackages/pipeline/src/shared/ports/outbound-gateway.port.tspackages/pipeline/src/shared/ports/queue-publisher.port.tspackages/pipeline/src/shared/ports/registry-replication.port.tspackages/pipeline/src/shared/ports/replica-state.port.tspackages/pipeline/src/shared/ports/state-store.port.tspackages/security/eslint.config.mjspackages/security/package.jsonpackages/security/src/adapters/aws-kms.adapter.tspackages/security/src/adapters/local-crypto.adapter.tspackages/security/src/aws-kms.adapter.spec.tspackages/security/src/constants.tspackages/security/src/encryption.module.spec.tspackages/security/src/encryption.module.tspackages/security/src/index.tspackages/security/src/interfaces/encryption-service.interface.tspackages/security/src/local-crypto.adapter.spec.tspackages/security/tsconfig.jsonpackages/security/vitest.config.ts
💤 Files with no reviewable changes (18)
- packages/credentials/src/crypto/encryption.interface.ts
- packages/domain/tms/tsconfig.lib.json
- packages/domain/core/package.json
- packages/domain/core/vitest.config.ts
- packages/credentials/src/crypto/encryption.service.ts
- packages/credentials/src/crypto/encryption.service.spec.ts
- packages/domain/tms/src/schema/tms-schema.ts
- packages/domain/tms/src/tms-target-builder.ts
- packages/domain/tms/src/index.ts
- packages/domain/tms/src/schema/tms-identifier-validator.ts
- packages/domain/tms/vitest.config.ts
- apps/worker/package.json
- packages/domain/core/tsconfig.lib.json
- apps/api/src/modules/stitches/stitches.module.ts
- packages/domain/tms/tsconfig.json
- packages/domain/tms/package.json
- packages/domain/tms/src/tms-normalized-writer.ts
- packages/domain/tms/src/schema/tms-provisioner.ts
| import type { DrizzleDb } from '@soopa/database'; | ||
|
|
||
| import { EncryptionService } from '../crypto/encryption.interface.js'; | ||
| import { IEncryptionService, ENCRYPTION_SERVICE } from '@soopa/security'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TypeScript compiler flags that affect type-only imports =="
fd -HI '^tsconfig.*\.json$' -x sh -c '
echo "--- $1"
rg -n "\"(verbatimModuleSyntax|importsNotUsedAsValues|preserveValueImports)\"" "$1" || true
' sh {}
echo
echo "== Current IEncryptionService imports in the touched credentials files =="
rg -n 'import\s*\{[^}]*IEncryptionService[^}]*\}\s*from\s*["@'\'']`@soopa/security`["@'\'']' packages/credentials/src
echo
echo "== Security package exports for the DI token and interface =="
rg -n 'IEncryptionService|ENCRYPTION_SERVICE' packages/security/srcRepository: pramodnarayana/nexiom
Length of output: 4777
Use import type for IEncryptionService across the credentials OAuth files.
@soopa/security exports ENCRYPTION_SERVICE as a value, but IEncryptionService only as a type (packages/security/src/index.ts). With verbatimModuleSyntax: true enabled in apps/web/tsconfig*.json, these must be import type imports to avoid invalid value imports.
Proposed diff pattern
-import { IEncryptionService, ENCRYPTION_SERVICE } from '`@soopa/security`';
+import { ENCRYPTION_SERVICE } from '`@soopa/security`';
+import type { IEncryptionService } from '`@soopa/security`';-import { IEncryptionService } from '`@soopa/security`';
+import type { IEncryptionService } from '`@soopa/security`';📝 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
| import { IEncryptionService, ENCRYPTION_SERVICE } from '@soopa/security'; | |
| import { ENCRYPTION_SERVICE } from '`@soopa/security`'; | |
| import type { IEncryptionService } from '`@soopa/security`'; |
🤖 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/credentials/src/oauth/token-manager.service.ts` at line 6, The
import mixes a type and a value from `@soopa/security` which breaks with
verbatimModuleSyntax; change the combined import to a type-only import for
IEncryptionService and keep ENCRYPTION_SERVICE as a value import (e.g., use an
import type { IEncryptionService } for the interface and a regular import {
ENCRYPTION_SERVICE } for the token). Apply this change in
token-manager.service.ts and the other credentials OAuth files that import
IEncryptionService/ENCRYPTION_SERVICE so the type is erased at emit and the
runtime token remains a value import.
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
Refactoring
Removals