Open
Conversation
… feature/refactor-integration-config
Contributor
WalkthroughCentralizes per-domain integration model files into a single Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Config as packages/configs/integrations/src/config.ts
participant Framework as `@o2s/framework` (createIntegrationConfig)
participant App as apps/api-harmonization AppConfig
participant Runtime as Runtime
Dev->>Config: edit domain assignments (e.g., tickets: Zendesk)
Config->>Framework: call createIntegrationConfig({ ... })
Framework-->>Config: validate domains and assemble integrations
Config-->>App: export `integrations`
App->>Runtime: import integrations and initialize
Runtime-->>App: use integration namespaces/types at runtime
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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: 2
🧹 Nitpick comments (1)
packages/framework/src/config/create-integration-config.ts (1)
21-40: MakeDOMAIN_KEYSexhaustive from a single typed source.This list is easy to drift out of sync with
ApiConfig['integrations']. If a new domain is added toApiConfigand not added here, the helper will silently omit it and still cast the result as complete.♻️ One way to keep it exhaustive
-const DOMAIN_KEYS: DomainKey[] = [ - 'articles', - 'auth', - 'billingAccounts', - 'cache', - 'carts', - 'checkout', - 'cms', - 'customers', - 'invoices', - 'notifications', - 'orders', - 'organizations', - 'payments', - 'products', - 'resources', - 'search', - 'tickets', - 'users', -]; +const DOMAIN_KEYS = { + articles: true, + auth: true, + billingAccounts: true, + cache: true, + carts: true, + checkout: true, + cms: true, + customers: true, + invoices: true, + notifications: true, + orders: true, + organizations: true, + payments: true, + products: true, + resources: true, + search: true, + tickets: true, + users: true, +} satisfies Record<DomainKey, true>; ... - for (const domain of DOMAIN_KEYS) { + for (const domain of Object.keys(DOMAIN_KEYS) as DomainKey[]) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/config/create-integration-config.ts` around lines 21 - 40, Replace the hard-coded DOMAIN_KEYS array with a keys list derived from the ApiConfig['integrations'] type so it cannot drift out of sync: compute keys at runtime via Object.keys on a typed dummy (e.g. Object.keys({} as ApiConfig['integrations'])) and cast to (keyof ApiConfig['integrations'])[] or import a single canonical integrations object and call Object.keys on that, then assign to DOMAIN_KEYS so DOMAIN_KEYS (the symbol) is typed as (keyof ApiConfig['integrations'])[] and will always be exhaustive relative to the ApiConfig type.
🤖 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/configs/integrations/src/config.ts`:
- Around line 7-8: The runtime domain map and the exported namespace are
duplicated: remove the second source of truth by deriving the exported domain
bindings from the single createIntegrationConfig result (the const result =
createIntegrationConfig({...}) used for integrations. Replace the manual export
imports like "export import Articles = Mocked.Integration.Articles" with
types/aliases derived from result (or generate per-domain adapter types from the
same config) so that symbols such as result, createIntegrationConfig, Articles
and Mocked.Integration.* are the single source of truth and cannot drift
independently.
In `@packages/framework/src/config/create-integration-config.ts`:
- Around line 7-19: The IntegrationWithDomain type is too permissive (Config:
Record<string, unknown>), letting malformed integration configs bypass
compile-time checks and forcing runtime casts in createIntegrationConfig; change
IntegrationWithDomain.Config to Partial<ApiConfig['integrations']> so the type
aligns with concrete integrations, remove the unnecessary cast in the
createIntegrationConfig logic that treats Config as ApiConfig['integrations'],
and ensure IntegrationConfigInput continues to map DomainKey to
IntegrationWithDomain so callers get compile-time validation of optional fields
like service/controller.
---
Nitpick comments:
In `@packages/framework/src/config/create-integration-config.ts`:
- Around line 21-40: Replace the hard-coded DOMAIN_KEYS array with a keys list
derived from the ApiConfig['integrations'] type so it cannot drift out of sync:
compute keys at runtime via Object.keys on a typed dummy (e.g. Object.keys({} as
ApiConfig['integrations'])) and cast to (keyof ApiConfig['integrations'])[] or
import a single canonical integrations object and call Object.keys on that, then
assign to DOMAIN_KEYS so DOMAIN_KEYS (the symbol) is typed as (keyof
ApiConfig['integrations'])[] and will always be exhaustive relative to the
ApiConfig type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f5be25e-73ab-4ab0-a6de-12974533f806
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
apps/api-harmonization/src/app.config.tspackages/configs/integrations/src/config.tspackages/configs/integrations/src/models/articles.tspackages/configs/integrations/src/models/auth.tspackages/configs/integrations/src/models/billing-accounts.tspackages/configs/integrations/src/models/cache.tspackages/configs/integrations/src/models/carts.tspackages/configs/integrations/src/models/checkout.tspackages/configs/integrations/src/models/cms.tspackages/configs/integrations/src/models/customers.tspackages/configs/integrations/src/models/index.tspackages/configs/integrations/src/models/invoices.tspackages/configs/integrations/src/models/notifications.tspackages/configs/integrations/src/models/orders.tspackages/configs/integrations/src/models/organizations.tspackages/configs/integrations/src/models/payments.tspackages/configs/integrations/src/models/products.tspackages/configs/integrations/src/models/resources.tspackages/configs/integrations/src/models/search.tspackages/configs/integrations/src/models/tickets.tspackages/configs/integrations/src/models/users.tspackages/framework/package.jsonpackages/framework/src/config/create-integration-config.tspackages/framework/src/config/index.tspackages/integrations/zendesk/scripts/oas/help-center-oas.yaml
💤 Files with no reviewable changes (18)
- packages/configs/integrations/src/models/orders.ts
- packages/configs/integrations/src/models/search.ts
- packages/configs/integrations/src/models/users.ts
- packages/configs/integrations/src/models/carts.ts
- packages/configs/integrations/src/models/products.ts
- packages/configs/integrations/src/models/billing-accounts.ts
- packages/configs/integrations/src/models/invoices.ts
- packages/configs/integrations/src/models/auth.ts
- packages/configs/integrations/src/models/payments.ts
- packages/configs/integrations/src/models/articles.ts
- packages/configs/integrations/src/models/notifications.ts
- packages/configs/integrations/src/models/tickets.ts
- packages/configs/integrations/src/models/checkout.ts
- packages/configs/integrations/src/models/customers.ts
- packages/configs/integrations/src/models/organizations.ts
- packages/configs/integrations/src/models/cache.ts
- packages/configs/integrations/src/models/cms.ts
- packages/configs/integrations/src/models/resources.ts
Comment on lines
+7
to
+8
| // When swapping a domain, update BOTH the assignment below AND the matching export import | ||
| const result = createIntegrationConfig({ |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the second source of truth for domain bindings.
The runtime map and the exported domain namespaces can drift independently here. If articles is switched to another provider in Lines 8-27 but export import Articles = Mocked.Integration.Articles is missed in Lines 34-51, consumers will compile against Mocked types while integrations.articles points somewhere else. That breaks the PR’s “single swap point” goal and makes future provider swaps unsafe. Please derive both surfaces from one source of truth, or keep/generate per-domain adapters so they cannot diverge.
Also applies to: 33-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/configs/integrations/src/config.ts` around lines 7 - 8, The runtime
domain map and the exported namespace are duplicated: remove the second source
of truth by deriving the exported domain bindings from the single
createIntegrationConfig result (the const result =
createIntegrationConfig({...}) used for integrations. Replace the manual export
imports like "export import Articles = Mocked.Integration.Articles" with
types/aliases derived from result (or generate per-domain adapter types from the
same config) so that symbols such as result, createIntegrationConfig, Articles
and Mocked.Integration.* are the single source of truth and cannot drift
independently.
Comment on lines
+7
to
+19
| export type IntegrationWithDomain = { | ||
| Config: Record<string, unknown>; | ||
| Integration: Record<string, unknown>; | ||
| }; | ||
|
|
||
| type DomainKey = keyof ApiConfig['integrations']; | ||
|
|
||
| /** | ||
| * Input for createIntegrationConfig. Each domain maps to an integration that provides that domain. | ||
| */ | ||
| export type IntegrationConfigInput = { | ||
| [D in DomainKey]: IntegrationWithDomain; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Helper typing:\n'
rg -n -C2 "IntegrationWithDomain|Record<string, unknown>|ApiConfig\\['integrations'\\]" packages/framework/src/config/create-integration-config.ts
printf '\nConcrete integration Config exports:\n'
rg -n -C2 "export const Config: Partial<ApiConfig\\['integrations'\\]>" packages/integrationsRepository: o2sdev/openselfservice
Length of output: 4537
🏁 Script executed:
sed -n '50,78p' packages/framework/src/config/create-integration-config.tsRepository: o2sdev/openselfservice
Length of output: 895
Tighten IntegrationWithDomain.Config; the current Record<string, unknown> type bypasses safety checks.
All concrete integrations export Config: Partial<ApiConfig['integrations']>, but the helper accepts Record<string, unknown>, allowing malformed configs to pass the presence check at line 63–65 and the cast at line 67 without validation. Only at runtime could the framework discover missing fields like service or controller.
Change IntegrationWithDomain.Config to Partial<ApiConfig['integrations']> to align the helper's input type with actual integration implementations:
Suggested fix
export type IntegrationWithDomain = {
- Config: Record<string, unknown>;
+ Config: Partial<ApiConfig['integrations']>;
Integration: Record<string, unknown>;
};At the implementation, the cast at line 67 becomes unnecessary:
- const domainConfig = (integration.Config as Record<string, unknown>)[domain];
+ const domainConfig = integration.Config[domain];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/framework/src/config/create-integration-config.ts` around lines 7 -
19, The IntegrationWithDomain type is too permissive (Config: Record<string,
unknown>), letting malformed integration configs bypass compile-time checks and
forcing runtime casts in createIntegrationConfig; change
IntegrationWithDomain.Config to Partial<ApiConfig['integrations']> so the type
aligns with concrete integrations, remove the unnecessary cast in the
createIntegrationConfig logic that treats Config as ApiConfig['integrations'],
and ensure IntegrationConfigInput continues to map DomainKey to
IntegrationWithDomain so callers get compile-time validation of optional fields
like service/controller.
Contributor
Coverage Report for packages/configs/vitest-config
File CoverageNo changed files found. |
marcinkrasowski
approved these changes
Mar 11, 2026
Collaborator
marcinkrasowski
left a comment
marcinkrasowski
left a comment
There was a problem hiding this comment.
implement suggested changes, then merge
… feature/refactor-integration-config
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/cli/create-o2s-app/src/scaffold/transform-integration-configs.ts (2)
119-123: Consider adding a safeguard for failed import injection.The
replace()call relies on an exact string match with the Mocked import line. If the source file's formatting differs (e.g., double quotes, extra whitespace), the replacement silently fails—leaving the file without the necessary integration imports while proceeding to update domain assignments.Consider verifying the replacement succeeded or using a more flexible pattern:
💡 Optional safeguard
- updatedContent = updatedContent.replace( - `import * as Mocked from '${MOCKED_IMPORT}';`, - `import * as Mocked from '${MOCKED_IMPORT}';\n${newImports}`, - ); + const mockedImportPattern = /import\s+\*\s+as\s+Mocked\s+from\s+['"]@o2s\/integrations\.mocked\/integration['"];/; + if (!mockedImportPattern.test(updatedContent)) { + throw new Error(`Could not locate Mocked import in ${CONFIG_FILE_PATH}`); + } + updatedContent = updatedContent.replace( + mockedImportPattern, + (match) => `${match}\n${newImports}`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/create-o2s-app/src/scaffold/transform-integration-configs.ts` around lines 119 - 123, The replace call that injects newImports into updatedContent using the exact string `import * as Mocked from '${MOCKED_IMPORT}';` can silently fail when formatting differs; modify the logic around updatedContent.replace (the usage that references MOCKED_IMPORT and newImports) to verify the replacement succeeded (compare pre/post strings or check indexOf for newImports) and, if it didn't, attempt a more flexible insertion using a regex that matches import * as Mocked from with either single/double quotes and optional whitespace, and as a final fallback insert newImports at a safe location (e.g., after the first import block or at top) or log/throw an error so the failure is explicit. Ensure you update the code paths that use updatedContent.replace and the variables MOCKED_IMPORT and newImports so the injection is robust and failures are surfaced.
134-139: Static analysis ReDoS warning is a false positive in this context.The
modulevariable originates fromintegrationModules(internal CLI metadata), not from untrusted user input. The regex pattern is also anchored (^...$) with simple quantifiers (\s+,\s*) that don't create catastrophic backtracking scenarios. Since this runs during scaffolding (not in a request-handling hot path), the risk is negligible.For defense in depth, consider validating that module names match an expected pattern (e.g., alphanumeric with camelCase) before interpolating into the regex, but this is optional given the controlled input source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/create-o2s-app/src/scaffold/transform-integration-configs.ts` around lines 134 - 139, The regex interpolation of module (from integrationModules) can trigger a ReDoS warning; to fix, validate/sanitize module before using it in the RegExp: ensure the module string matches an allowed identifier pattern (e.g., alphanumeric/camelCase) or, more robustly, escape regex metacharacters in module prior to building the RegExp used in the domainMatch check (the code that constructs new RegExp(`^(\\s+${module}:\\s*)Mocked(,)$`) and later assigns lines[i] when domainMatch is truthy). If validation fails, skip the regex match for that module to avoid injecting unexpected input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli/create-o2s-app/src/scaffold/transform-integration-configs.ts`:
- Around line 119-123: The replace call that injects newImports into
updatedContent using the exact string `import * as Mocked from
'${MOCKED_IMPORT}';` can silently fail when formatting differs; modify the logic
around updatedContent.replace (the usage that references MOCKED_IMPORT and
newImports) to verify the replacement succeeded (compare pre/post strings or
check indexOf for newImports) and, if it didn't, attempt a more flexible
insertion using a regex that matches import * as Mocked from with either
single/double quotes and optional whitespace, and as a final fallback insert
newImports at a safe location (e.g., after the first import block or at top) or
log/throw an error so the failure is explicit. Ensure you update the code paths
that use updatedContent.replace and the variables MOCKED_IMPORT and newImports
so the injection is robust and failures are surfaced.
- Around line 134-139: The regex interpolation of module (from
integrationModules) can trigger a ReDoS warning; to fix, validate/sanitize
module before using it in the RegExp: ensure the module string matches an
allowed identifier pattern (e.g., alphanumeric/camelCase) or, more robustly,
escape regex metacharacters in module prior to building the RegExp used in the
domainMatch check (the code that constructs new
RegExp(`^(\\s+${module}:\\s*)Mocked(,)$`) and later assigns lines[i] when
domainMatch is truthy). If validation fails, skip the regex match for that
module to avoid injecting unexpected input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0064858c-dda5-4ae4-8505-58f030ba94e4
📒 Files selected for processing (3)
packages/cli/create-o2s-app/src/scaffold/transform-app-config.tspackages/cli/create-o2s-app/src/scaffold/transform-integration-configs.tspackages/cli/create-o2s-app/src/utils/logger.ts
✅ Files skipped from review due to trivial changes (2)
- packages/cli/create-o2s-app/src/utils/logger.ts
- packages/cli/create-o2s-app/src/scaffold/transform-app-config.ts
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
2 participants
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.
Key Changes
1.Single config file instead of 18 model files
-Before: 18 files in packages/configs/integrations/src/models/ (e.g. tickets.ts, articles.ts), each with the same pattern.
-After: One config.ts that maps each domain to an integration. Swapping integrations is done by changing the integration reference in that file.
2. createIntegrationConfig helper
-New packages/framework/src/config/create-integration-config.ts:
-Accepts a domain → integration map.
-Checks at runtime that each integration provides the domain it’s assigned to.
-Returns integrations typed as ApiConfig['integrations'].
-Removes the need for Config.tickets! and similar non-null assertions.
3. Simplified app.config.ts
-Before: 18 imports and a manual integrations object built from each domain’s config.
-After: Single import of integrations from @o2s/configs.integrations and direct use in AppConfig.
Fixes #760
Summary by CodeRabbit
Refactor
New Features
Documentation
Chores