[improvement][Pt.01] Reduce circular dependencies between features#9613
Draft
JayaShakthi97 wants to merge 8 commits intowso2:masterfrom
Draft
[improvement][Pt.01] Reduce circular dependencies between features#9613JayaShakthi97 wants to merge 8 commits intowso2:masterfrom
JayaShakthi97 wants to merge 8 commits intowso2:masterfrom
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughApp-level initialization now fetches and merges action and connection resource endpoints and passes the combined endpoints into the sign-in flow via a new optional parameter; core package no longer imports action/connection endpoint modules and service endpoint typing was adjusted. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 3
🧹 Nitpick comments (3)
features/admin.authentication.v1/hooks/use-sign-in.ts (1)
89-95: Consider using a stronger type forresourceEndpoints.
Record<string, any>is very permissive. SinceServiceResourceEndpointsInterfaceis already used elsewhere in the codebase (and imported in bothapp.tsxandprotected-app.tsx), using it here would provide compile-time safety and eliminate theas anycasts at Lines 366/369/385/388.Proposed type improvement
onSignIn: ( response: BasicUserInfo, onTenantResolve: (tenantDomain: string) => void, onSignInSuccessRedirect: (idToken: DecodedIDTokenPayload) => void, onAppReady: () => void, - resourceEndpoints?: Record<string, any> + resourceEndpoints?: ServiceResourceEndpointsInterface ) => Promise<void>;And similarly for the internal functions at Lines 219-220 and 246-247.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/admin.authentication.v1/hooks/use-sign-in.ts` around lines 89 - 95, The onSignIn callback currently types resourceEndpoints as Record<string, any>; change it to use the existing ServiceResourceEndpointsInterface (replace Record<string, any> with ServiceResourceEndpointsInterface on the onSignIn signature) and update the internal helper function signatures that accept resourceEndpoints (the internal functions referenced around the onSignIn implementation) to the same ServiceResourceEndpointsInterface so you can remove the unsafe "as any" casts at the call sites (the casts near where onSignIn is invoked/forwarded).features/admin.core.v1/configs/app.ts (1)
368-379: Empty-string endpoint placeholders are a silent failure risk.These placeholders (
actions: "",authenticatorTags: "", etc.) satisfy the type contract but will produce broken API calls if any code path uses them before the app layer merges the real endpoints. This is acceptable for the decoupling goal, but consider whether a more defensive approach (e.g., logging a warning or throwing on access) would help catch misconfiguration earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/admin.core.v1/configs/app.ts` around lines 368 - 379, The config currently sets endpoint properties like actions, authenticatorTags, authenticators, customAuthenticators, extensions, fidoConfigs, identityProviders, localAuthenticators, multiFactorAuthenticators to empty strings which silently breaks callers; replace these silent placeholders with a defensive mechanism: add a helper on the same config class (e.g., getEndpoint(name: string) or resolveEndpoint(name: string)) that either logs a clear warning (including the endpoint name and deployment info from getDeploymentConfig()) or throws when accessed, update each property (actions, authenticatorTags, authenticators, customAuthenticators, extensions, fidoConfigs, identityProviders, localAuthenticators, multiFactorAuthenticators) to call that helper instead of using "" so misconfiguration is detected immediately, and optionally add an initialization validation method (e.g., validateEndpoints or a call in the constructor) that checks required endpoints and surfaces errors early; keep the existing me value and resolveServerHost usage as-is.apps/console/src/app.tsx (1)
206-214: Endpoint merge logic is duplicated acrossapp.tsxandprotected-app.tsx.Both files construct the same merged endpoint object:
const serviceResourceEndpoints: ServiceResourceEndpointsInterface = { ...Config.getServiceResourceEndpoints(), ...getActionsResourceEndpoints(Config.resolveServerHost()), ...getConnectionResourceEndpoints(Config.resolveServerHost()) };Extract a shared helper function (e.g.,
getServiceResourceEndpoints()in a config module) to keep this logic in one place and avoid divergence if the set of merged endpoints changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/console/src/app.tsx` around lines 206 - 214, Extract the duplicated endpoint-merge logic into a single helper function named getServiceResourceEndpoints (or similar) in the shared config module; implement it to return the merged ServiceResourceEndpointsInterface by combining Config.getServiceResourceEndpoints(), getActionsResourceEndpoints(Config.resolveServerHost()), and getConnectionResourceEndpoints(Config.resolveServerHost()), then replace the inline merge in both app.tsx and protected-app.tsx with a call to this new helper and pass its result to dispatch(setServiceResourceEndpoints(...)) and setResourceEndpoints(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/admin.authentication.v1/hooks/use-sign-in.ts`:
- Around line 219-220: The default resourceEndpoints from
Config.getServiceResourceEndpoints() returns empty strings for
actions/authenticatorTags/authenticators and some organization pages
(organization-edit.tsx, organization-list.tsx,
organization-switch-breadcrumb.tsx) call onSignIn() with only 4 args relying on
that default; either confirm those pages work without these endpoints or update
their calls to pass the merged endpoints like the console app does: compute
mergedEndpoints = { ...getActionsResourceEndpoints(),
...getConnectionResourceEndpoints(), ...Config.getServiceResourceEndpoints() }
and pass mergedEndpoints into onSignIn(resourceEndpoints) (or adjust
use-sign-in.ts onSignIn() to accept and consume the merged shape), ensuring the
same dispatch/context behavior used at lines where resourceEndpoints is read
(lines ~366,369,385,388) receives the non-empty values.
In `@features/admin.core.v1/configs/app.ts`:
- Around line 347-348: Remove the duplicated spread of
getApplicationTemplatesResourcesEndpoints(this.resolveServerHost()) in the
configuration object: locate the two consecutive spreads of
getApplicationTemplatesResourcesEndpoints(...) and keep only one, so the
resources are not redundantly merged; ensure any intended calls to
getActionsResourceEndpoints or getConnectionResourceEndpoints are restored if
they were meant to be different, and run tests to verify no keys were
accidentally omitted.
In `@features/admin.core.v1/models/config.ts`:
- Around line 745-746: The extends list currently contains a duplicated
SMSTemplateResourceEndpointsInterface entry; locate the type or interface
declaration whose extends/intersection includes
SMSTemplateResourceEndpointsInterface twice and remove the duplicate OR replace
the second occurrence with the correct intended interface name (if another
interface was meant there). Update the extends/intersection to contain unique
members only (e.g., keep a single SMSTemplateResourceEndpointsInterface and add
the correct replacement if needed) and run TypeScript checks to confirm the fix.
---
Nitpick comments:
In `@apps/console/src/app.tsx`:
- Around line 206-214: Extract the duplicated endpoint-merge logic into a single
helper function named getServiceResourceEndpoints (or similar) in the shared
config module; implement it to return the merged
ServiceResourceEndpointsInterface by combining
Config.getServiceResourceEndpoints(),
getActionsResourceEndpoints(Config.resolveServerHost()), and
getConnectionResourceEndpoints(Config.resolveServerHost()), then replace the
inline merge in both app.tsx and protected-app.tsx with a call to this new
helper and pass its result to dispatch(setServiceResourceEndpoints(...)) and
setResourceEndpoints(...).
In `@features/admin.authentication.v1/hooks/use-sign-in.ts`:
- Around line 89-95: The onSignIn callback currently types resourceEndpoints as
Record<string, any>; change it to use the existing
ServiceResourceEndpointsInterface (replace Record<string, any> with
ServiceResourceEndpointsInterface on the onSignIn signature) and update the
internal helper function signatures that accept resourceEndpoints (the internal
functions referenced around the onSignIn implementation) to the same
ServiceResourceEndpointsInterface so you can remove the unsafe "as any" casts at
the call sites (the casts near where onSignIn is invoked/forwarded).
In `@features/admin.core.v1/configs/app.ts`:
- Around line 368-379: The config currently sets endpoint properties like
actions, authenticatorTags, authenticators, customAuthenticators, extensions,
fidoConfigs, identityProviders, localAuthenticators, multiFactorAuthenticators
to empty strings which silently breaks callers; replace these silent
placeholders with a defensive mechanism: add a helper on the same config class
(e.g., getEndpoint(name: string) or resolveEndpoint(name: string)) that either
logs a clear warning (including the endpoint name and deployment info from
getDeploymentConfig()) or throws when accessed, update each property (actions,
authenticatorTags, authenticators, customAuthenticators, extensions,
fidoConfigs, identityProviders, localAuthenticators, multiFactorAuthenticators)
to call that helper instead of using "" so misconfiguration is detected
immediately, and optionally add an initialization validation method (e.g.,
validateEndpoints or a call in the constructor) that checks required endpoints
and surfaces errors early; keep the existing me value and resolveServerHost
usage as-is.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9613 +/- ##
=======================================
Coverage 55.88% 55.88%
=======================================
Files 42 42
Lines 1020 1020
Branches 231 254 +23
=======================================
Hits 570 570
Misses 450 450
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
… and SMS template resource endpoint interface
JayaShakthi97
force-pushed
the
feature/refactor-admin-connections-v1
branch
from
d0e6725 to
5716901
Compare
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.
Purpose
This pull request refactors how service resource endpoints are managed and injected throughout the authentication and app initialization flows. Instead of relying on the core config to include all resource endpoints (including actions and connections), these are now dynamically merged and passed where needed. This decouples certain admin modules from the core, simplifies dependency management, and makes the system more modular.
The most important changes are:
Endpoint Management Refactor:
Removed
@wso2is/admin.actions.v1and@wso2is/admin.connections.v1as hard dependencies from the core package, and instead import their endpoint configs only in the app layers where needed. (features/admin.core.v1/package.json,pnpm-lock.yaml,features/admin.core.v1/configs/app.ts,apps/console/src/app.tsx,apps/console/src/protected-app.tsx) [1] [2] [3] [4] [5] [6] [7] [8]The core
Config.getServiceResourceEndpoints()no longer includes actions and connections endpoints. These are now merged at the app level and passed explicitly where needed. (features/admin.core.v1/configs/app.ts,apps/console/src/app.tsx,apps/console/src/protected-app.tsx) [1] [2] [3] [4]Authentication Flow Update:
resourceEndpointsparameter, ensuring the correct set of endpoints is used after login, organization switch, or tenant change. (features/admin.authentication.v1/hooks/use-sign-in.ts,apps/console/src/protected-app.tsx) [1] [2] [3] [4] [5] [6] [7]Type and Interface Adjustments:
ServiceResourceEndpointsInterfaceto remove direct dependencies on actions and connections endpoint interfaces, and instead add the relevant endpoint keys as plain strings. (features/admin.core.v1/models/config.ts) [1] [2] [3] [4] [5]Default/Placeholder Endpoints:
features/admin.core.v1/configs/app.ts,features/admin.core.v1/models/config.ts) [1] [2]These changes make the codebase more modular and maintainable by reducing tight coupling between the core and feature modules, and by ensuring that endpoint configuration is managed in a flexible, context-aware manner.
Before
Now
Related Issues
Related PRs
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Summary by CodeRabbit
Refactor
Chores