fix: replace sequential await-in-loop with Promise.all for concurrent execution#10480
fix: replace sequential await-in-loop with Promise.all for concurrent execution#10480soccerthomas wants to merge 7 commits into
Conversation
…administrator-wizard
…enticator-settings
…ound-provisioning-settings
…enterprise-login-config
…ector-listing-page
|
|
📝 WalkthroughWalkthroughFive internal async helpers are refactored from sequential loops to concurrent ChangesSequential-to-concurrent async refactor
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ 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 |
|
Hi — this is my first contribution to this project. I've addressed the async-await-in-loop violations flagged in wso2/product-is#27874. Fixed 5 real violations with Promise.all and documented why the remaining 9 flagged locations were left unchanged. Would appreciate a review when you have time! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/admin.administrators.v1/wizard/add-administrator-wizard.tsx (1)
276-330: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSuccess alert and wizard close fire even when role updates fail.
Each task's
.catchhandles the error without re-throwing, soPromise.allalways resolves. As a result, the success alert at Line 319 pluscloseWizard()/onUserUpdate()run unconditionally, and any error set viasetAlert(...)is immediately discarded because the wizard closes. Under concurrent execution this is more pronounced: multiple failing tasks overwrite each other'ssetAlert, and the user ends up seeing a success despite partial/total failure.Separately,
setIsSubmitting(false)inside the per-task.finallyclears the loading state as soon as the first task settles, while other requests and the success path are still pending. Consider tracking failures and resetting submit state once afterPromise.all.♻️ Suggested handling
setIsSubmitting(true); - await Promise.all(roleIds.map(async (roleId: string): Promise<void> => { - await updateUsersForRoleFunction(roleId, roleData) - .catch((error: AxiosError<HttpErrorResponseDataInterface>) => { + let hasError: boolean = false; + + await Promise.all(roleIds.map(async (roleId: string): Promise<void> => { + await updateUsersForRoleFunction(roleId, roleData) + .catch((error: AxiosError<HttpErrorResponseDataInterface>) => { + hasError = true; if (!error.response || error.response.status === 401) { setAlert({ description: t( "extensions:manage.users.wizard.addAdmin.internal.updateRole.error.description" ), level: AlertLevels.ERROR, message: t( "extensions:manage.users.wizard.addAdmin.internal.updateRole.error.message" ) }); } else if (error.response && error.response.data && error.response.data.detail) { setAlert({ description: t( "extensions:manage.users.wizard.addAdmin.internal.updateRole.error.description", { description: error.response.data.detail } ), level: AlertLevels.ERROR, message: t( "extensions:manage.users.wizard.addAdmin.internal.updateRole.error.message" ) }); } else { // Generic error message setAlert({ description: t( "extensions:manage.users.wizard.addAdmin.internal.updateRole.genericError" + ".description" ), level: AlertLevels.ERROR, message: t( "extensions:manage.users.wizard.addAdmin.internal.updateRole.genericError.message" ) }); } - }) - .finally(() => { - setIsSubmitting(false); - }); + }); })); + setIsSubmitting(false); + + if (hasError) { + return; + } + dispatch(addAlert({ description: t( "extensions:manage.users.wizard.addAdmin.internal.updateRole.success.description" ), level: AlertLevels.SUCCESS, message: t( "extensions:manage.users.wizard.addAdmin.internal.updateRole.success.message" ) })); closeWizard(); onUserUpdate();🤖 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 `@features/admin.administrators.v1/wizard/add-administrator-wizard.tsx` around lines 276 - 330, The role update flow in add-administrator-wizard.tsx is treating failed updates as success because the per-role catch in the Promise.all chain swallows errors, so the success alert, closeWizard(), and onUserUpdate() run unconditionally. Update the submit handler around updateUsersForRoleFunction so failures are tracked or re-thrown, only dispatch the success alert and close the wizard after all role updates succeed, and move setIsSubmitting(false) to a single place after the overall Promise.all completes or fails rather than in each task’s finally.
🧹 Nitpick comments (1)
features/admin.administrators.v1/wizard/add-administrator-wizard.tsx (1)
237-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: annotate the variable type alongside the return type.
assignUserRolesnow has thePromise<void>return type, but theconstbinding still relies on inference. Per coding guidelines, async functions should carry thePromise<T>annotation on both the variable and the function signature.♻️ Suggested annotation
- const assignUserRoles = async (users: UserBasicInterface[], roles: RolesInterface[]): Promise<void> => { + const assignUserRoles: (users: UserBasicInterface[], roles: RolesInterface[]) => Promise<void> = + async (users: UserBasicInterface[], roles: RolesInterface[]): Promise<void> => {🤖 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 `@features/admin.administrators.v1/wizard/add-administrator-wizard.tsx` at line 237, The async helper assignUserRoles should explicitly annotate both the const binding and the function signature with Promise<void> instead of relying on inference. Update the declaration of assignUserRoles in add-administrator-wizard.tsx so the variable itself is typed as an async function returning Promise<void>, matching the existing return type annotation on the function.Source: Coding guidelines
🤖 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
`@features/admin.connections.v1/components/edit/settings/authenticator-settings.tsx`:
- Around line 438-446: The fetchAuthenticators helper is using Promise.all over
fetchAuthenticator, but a handled API failure can leave that promise pending and
block the pane loading state. Update fetchAuthenticator so it always settles on
every error path, or have it return null/undefined on failure and filter those
results in fetchAuthenticators, ensuring the effect that updates
isFederatedAuthenticatorFetchRequestLoading can complete even when one
authenticator fetch fails.
In
`@features/admin.connections.v1/components/edit/settings/outbound-provisioning-settings.tsx`:
- Around line 235-243: `fetchConnector()` in `OutboundProvisioningSettings` must
always settle with an `OutboundProvisioningConnectorWithMetaInterface` result or
a handled fallback, because the current error branches only dispatch alerts and
can leave `fetchConnectors()`/`Promise.all(...)` waiting indefinitely. Update
the error handling inside `fetchConnector()` so every failure path returns a
resolved value or explicitly rejects after alerting, and make sure the caller in
`fetchConnectors()` still aggregates the connector list correctly.
In `@features/admin.server-configurations.v1/pages/connector-listing-page.tsx`:
- Around line 284-301: Move the initial connector selection logic out of the
parallel category loading path in connector-listing-page.tsx:
loadCategoryConnectors() should only fetch and return category data, not update
selectedConnector. Compute the default selection after Promise.all completes,
using the deterministic category order and the first eligible connector, so the
choice does not depend on response timing or multiple concurrent
!selectedConnector checks.
---
Outside diff comments:
In `@features/admin.administrators.v1/wizard/add-administrator-wizard.tsx`:
- Around line 276-330: The role update flow in add-administrator-wizard.tsx is
treating failed updates as success because the per-role catch in the Promise.all
chain swallows errors, so the success alert, closeWizard(), and onUserUpdate()
run unconditionally. Update the submit handler around updateUsersForRoleFunction
so failures are tracked or re-thrown, only dispatch the success alert and close
the wizard after all role updates succeed, and move setIsSubmitting(false) to a
single place after the overall Promise.all completes or fails rather than in
each task’s finally.
---
Nitpick comments:
In `@features/admin.administrators.v1/wizard/add-administrator-wizard.tsx`:
- Line 237: The async helper assignUserRoles should explicitly annotate both the
const binding and the function signature with Promise<void> instead of relying
on inference. Update the declaration of assignUserRoles in
add-administrator-wizard.tsx so the variable itself is typed as an async
function returning Promise<void>, matching the existing return type annotation
on the function.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 1a7b73c3-cca0-4797-a1c1-e2fadc5e3eea
📒 Files selected for processing (5)
features/admin.administrators.v1/wizard/add-administrator-wizard.tsxfeatures/admin.connections.v1/components/edit/settings/authenticator-settings.tsxfeatures/admin.connections.v1/components/edit/settings/outbound-provisioning-settings.tsxfeatures/admin.console-settings.v1/hooks/use-enterprise-login-config.tsfeatures/admin.server-configurations.v1/pages/connector-listing-page.tsx
| const dynamicCategories: GovernanceConnectorCategoryInterface[] = await Promise.all( | ||
| categories | ||
| .filter((category: GovernanceConnectorCategoryInterface) => | ||
| !serverConfigurationConfig.predefinedConnectorCategories.includes(category.id)) | ||
| .map(async (category: GovernanceConnectorCategoryInterface): | ||
| Promise<GovernanceConnectorCategoryInterface | null> => { | ||
| const connectorCategory: GovernanceConnectorCategoryInterface | null = | ||
| await loadCategoryConnectors(category.id); | ||
|
|
||
| // Filter out the SIFT connector from sub-organizations. | ||
| if (isSubOrganization() && connectorCategory?.connectors.length > 0) { | ||
| connectorCategory.connectors = | ||
| connectorCategory?.connectors?.filter((connector: GovernanceConnectorInterface) => | ||
| connector.id !== ServerConfigurationsConstants.SIFT_CONNECTOR_ID); | ||
| } | ||
|
|
||
| // Filter out the SIFT connector from sub-organizations. | ||
| if (isSubOrganization() && connectorCategory?.connectors.length > 0) { | ||
| connectorCategory.connectors = | ||
| connectorCategory?.connectors?.filter((connector: GovernanceConnectorInterface) => | ||
| connector.id !== ServerConfigurationsConstants.SIFT_CONNECTOR_ID); | ||
| } | ||
| connectorCategory && dynamicConnectorCategoryArray.push(connectorCategory); | ||
| } | ||
| } | ||
| return connectorCategory; | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Move initial connector selection out of the parallel fetch path.
After this change, every loadCategoryConnectors() call starts with the same render-scoped selectedConnector value, so multiple requests can all pass the !selectedConnector check at Line 350 and race to update it. That makes the default selection depend on response timing instead of a stable category order.
Suggested fix
const dynamicConnectorCategoryArray: GovernanceConnectorCategoryInterface[] =
dynamicCategories.filter(Boolean);
+
+ if (!selectedConnector && dynamicConnectorCategoryArray[0]?.connectors?.[0]) {
+ setSelectorConnector(dynamicConnectorCategoryArray[0].connectors[0] as GovernanceConnectorWithRef);
+ }// Then remove the `!selectedConnector && setSelectorConnector(...)` side effect
// from `loadCategoryConnectors()` so each Promise stays pure.🤖 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 `@features/admin.server-configurations.v1/pages/connector-listing-page.tsx`
around lines 284 - 301, Move the initial connector selection logic out of the
parallel category loading path in connector-listing-page.tsx:
loadCategoryConnectors() should only fetch and return category data, not update
selectedConnector. Compute the default selection after Promise.all completes,
using the deterministic category order and the first eligible connector, so the
choice does not depend on response timing or multiple concurrent
!selectedConnector checks.
There was a problem hiding this comment.
The selectedConnector race condition is pre-existing behavior outside the scope of this fix. This PR only addresses the async-await-in-loop violations flagged in #27874.
There was a problem hiding this comment.
@soccerthomas Fair point on scope. I'll clarify one nuance for the record: strictly speaking, the race isn't entirely pre-existing — in the previous sequential for...of implementation, loadCategoryConnectors() calls resolved one at a time, so the !selectedConnector check always ended up selecting the first connector from the first category in the array (deterministic). With Promise.all, all calls fire concurrently, so whichever category's request resolves first now determines the default selection, which can vary between requests depending on network timing.
That said, I agree it's a narrow, low-impact UX side effect (default connector selection) rather than a functional break tied to the async-await-in-loop fix this PR targets. I won't insist on it being addressed here — happy to leave it as-is for this PR. If you'd like, I can open a follow-up issue to track making the initial selection deterministic (e.g., picking it from dynamicConnectorCategoryArray[0] after Promise.all resolves) so it doesn't get lost. Let me know if you'd like me to file that.
🧠 Learnings used
Learnt from: PasinduYeshan
Repo: wso2/identity-apps PR: 9589
File: features/admin.connections.v1/components/edit/settings/outbound-provisioning-settings.tsx:575-578
Timestamp: 2026-02-15T17:06:33.975Z
Learning: In the identity-apps repository, prefer using the data-componentid attribute for component identification in tests. The data-testid attribute is deprecated. Use data-componentid for new components, while existing components may continue using data-testid until they are updated.
Learnt from: ShanChathusanda93
Repo: wso2/identity-apps PR: 9619
File: features/admin.issuer-usage-scope.v1/pages/issuer-usage-scope-configuration.tsx:74-76
Timestamp: 2026-02-18T10:58:59.745Z
Learning: In WSO2 Identity Apps, for any feature that modifies server configurations via the /api/server/v1/configs/ endpoint (e.g., Issuer Usage Scope), perform permission checks using featureConfig?.server?.scopes?.update instead of creating dedicated scope configurations. Apply this guideline to all relevant TSX files under features that interact with server config endpoints.
Learnt from: VivekVinushanth
Repo: wso2/identity-apps PR: 9651
File: features/admin.cds.v1/components/profile-attribute.tsx:25-27
Timestamp: 2026-02-25T11:23:23.686Z
Learning: In TSX files across the repo, when implementing dynamic key-value inputs and there is no equivalent component in wso2is/form (e.g., dynamic values editor), it is acceptable to reuse the legacy DynamicField as a temporary solution until an Oxygen UI alternative is built. Avoid creating a custom react-final-form FieldArray unless absolutely necessary. This guidance applies to components like profile-attribute.tsx and similar dynamic-field scenarios.
Learnt from: Lakshan-Banneheke
Repo: wso2/identity-apps PR: 10203
File: features/admin.approval-workflows.v1/pages/approval-workflows.tsx:232-235
Timestamp: 2026-04-28T04:55:21.209Z
Learning: In wso2/identity-apps, FeatureLockedBanner `sx` props are intentionally allowed to include hardcoded pixel values (e.g., `marginTop: '-20px'`) when used consistently across the existing pages (such as approval-workflows, console-settings, remote-userstores). During code review, do not flag these specific hardcoded pixel values in `FeatureLockedBanner` `sx` as a violation of any guideline that would otherwise require referencing theme values.
Learnt from: Lakshan-Banneheke
Repo: wso2/identity-apps PR: 10215
File: features/admin.groups.v1/components/wizard/create-group-wizard.tsx:603-611
Timestamp: 2026-04-29T15:34:05.529Z
Learning: When reviewing code in wso2/identity-apps, do not flag `TierLimitReachErrorModal` usages of `actionLabel` and `header` as “unused” when these props are currently not consumed by the call site. Keep the props passed through for consistency with other call sites and to support future extensibility (the component is imported from `wso2is/admin.core.v1/components/modals`).
Hi @soccerthomas, thanks for the contribution. Could you please have a look at the coderabbitai bot comments and resolve those? |
|
And let's enhance the PR title too |
soccerthomas
changed the title
3 participants
Purpose
Replace sequential
awaitcalls insidefor...ofloops withPromise.allin 5 files where the async calls are independent. Each loop was running API calls one at a time — this change lets them run concurrently, reducing total wait time from the sum of all calls to the duration of the slowest single call.Files changed:
features/admin.administrators.v1/wizard/add-administrator-wizard.tsxfeatures/admin.connections.v1/components/edit/settings/authenticator-settings.tsxfeatures/admin.connections.v1/components/edit/settings/outbound-provisioning-settings.tsxfeatures/admin.console-settings.v1/hooks/use-enterprise-login-config.tsfeatures/admin.server-configurations.v1/pages/connector-listing-page.tsxThe remaining 9 locations flagged by the
async-await-in-looplint rule were left unchanged — they are while loops, already parallelized, or intentionally sequential.Related Issues
Related PRs
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.