[Test PR] Implemented API Key For Federated APIs#1338
Open
dpiyumal2319 wants to merge 12 commits into
Open
Conversation
- Added Loading state for api key actions - Enabled Subscription/API key pages across for the federated apis # Conflicts: # portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/AddEditGWEnvironment.jsx # portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyGenerate.jsx # portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx
dpiyumal2319
requested review from
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
|
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:
📝 WalkthroughWalkthroughAdds an admin UI for gateway plan-to-local-plan mapping with validation plumbing; introduces in-progress UI state and spinners for API key actions in the devportal; and replaces hardcoded gateway checks for subscription UI with feature-catalog-driven gating in the publisher. ChangesAdmin Portal Gateway Plan Mapping
DevPortal API Key In-Progress UI Feedback
Publisher Portal Feature-Catalog-Driven Subscriptions
Sequence Diagram(s)sequenceDiagram
participant User
participant GatewayConfig as GatewayConfiguration
participant PlanMapping as GatewayPlanMapping
participant AddEdit as AddEditGWEnvironment
participant API as Backend API
User->>GatewayConfig: Open environment editor
GatewayConfig->>PlanMapping: Render mapping configs (Accordion)
User->>PlanMapping: Edit mapping field
PlanMapping->>AddEdit: setAdditionalProperties('plan_mapping.<id>', value)
AddEdit->>AddEdit: Update additionalProperties, clear field error
User->>AddEdit: Click Save
AddEdit->>API: POST/PUT environment config
alt Validation Success
API-->>AddEdit: 200 OK
AddEdit->>GatewayConfig: Clear planMappingErrors
GatewayConfig->>PlanMapping: No field errors
else Validation Failure (federated)
API-->>AddEdit: 400 with federated errors (code 900520)
AddEdit->>AddEdit: extractPlanMappingErrors -> planMappingErrors
AddEdit->>GatewayConfig: Pass planMappingErrors
GatewayConfig->>PlanMapping: Display helperText errors for fields
end
sequenceDiagram
participant User
participant Listing as ApiKeyListing
participant Associate as ApiKeyAssociation
participant Generate as ApiKeyGenerate
participant API as Backend API
User->>Associate: Click Associate
Associate->>Associate: Set isAssociating=true
Associate->>API: POST associate
API-->>Associate: 200 OK
Associate->>Associate: Set isAssociating=false
Listing->>Listing: Refresh UI
User->>Generate: Click Regenerate(keyX)
Generate->>Generate: Set isRegenerating=true, regeneratingKeyUUID=keyX
Generate->>API: PUT regenerate keyX
API-->>Generate: 200 OK
Generate->>Generate: Clear isRegenerating, regeneratingKeyUUID
Listing->>Listing: Refresh keys, remove spinner on keyX
User->>Listing: Click Revoke(keyY)
Listing->>Listing: Set isRevoking=true for keyY
Listing->>API: DELETE keyY
API-->>Listing: 200 OK
Listing->>Listing: Set isRevoking=false, refresh keys
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 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 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Subscriptions/Subscriptions.jsx (1)
187-221:⚠️ Potential issue | 🟠 MajorInconsistent gating leaves editable controls without a save path.
At Line 221, save/cancel are hidden when subscription management is unsupported, but
SubscriptionAvailability(Line 211-220) is still rendered and editable. That creates a dead-end UI state for those gateways.💡 Suggested fix
- {tenants !== 0 && settings.crossTenantSubscriptionEnabled && - !isSubValidationDisabled && ( + {isSubscriptionManagementSupported() + && tenants !== 0 + && settings.crossTenantSubscriptionEnabled + && !isSubValidationDisabled && ( <SubscriptionAvailability api={api} availability={availability} setAvailability={setAvailability} tenantList={tenantList} setTenantList={setTenantList} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Subscriptions/Subscriptions.jsx` around lines 187 - 221, The SubscriptionAvailability component is rendered and editable even when subscription management is unsupported, which hides the save/cancel controls; update the render gating so SubscriptionAvailability is shown only when isSubscriptionManagementSupported() is true (same gate used for SubscriptionPoliciesManage) and still respects tenants, settings.crossTenantSubscriptionEnabled, and !isSubValidationDisabled, or alternatively move the save/cancel controls into a common parent that wraps both SubscriptionPoliciesManage and SubscriptionAvailability; adjust references to SubscriptionAvailability, SubscriptionPoliciesManage, and isSubscriptionManagementSupported() accordingly so the UI never exposes editable availability without a save path.portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/index.jsx (1)
650-664:⚠️ Potential issue | 🟡 MinorPrevent API Keys menu from rendering on MCP pages.
At Line 650, the menu can render for MCP pages, but there is no MCP API-keys route (only
/apis/:apiUuid/api-keysis mounted). This can create a dead link to a not-found route.💡 Proposed fix
- {user && showCredentials && api?.securityScheme?.includes('api_key') && ( + {user && !isMCPServer && showCredentials && api?.securityScheme?.includes('api_key') && ( <LeftMenuItem text={( <FormattedMessage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/index.jsx` around lines 650 - 664, The API Keys menu can render on MCP pages which don't have the per-API route, causing a dead link; update the conditional that renders LeftMenuItem (the block using user, showCredentials, api?.securityScheme and pathPrefix) to also require a per-API identifier or route context — e.g. check api?.uuid (or api?.id) is truthy or ensure pathPrefix includes '/apis/' — so the LeftMenuItem for API Keys is only rendered when the per-API route (/apis/:apiUuid/api-keys) exists.portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyListing.jsx (1)
241-268:⚠️ Potential issue | 🟡 MinorKeep revocation state active until the full revoke flow finishes.
Line 247 clears
isRevokingbefore the follow-up key-list refresh completes, which can re-enable actions prematurely. Move reset logic to afinallyblock so UI stays consistent through both revoke and refresh steps.Suggested fix
const handleConfirmRevoke = () => { setRevokeConfirmOpen(false); setIsRevoking(true); const restApi = new API(); restApi.revokeAPIBoundAPIKey(apiUUID, selectedKeyForRevoke.keyUUID) .then(() => { - setIsRevoking(false); setRevokeSuccessOpen(true); // Refresh the API keys list return restApi.getApiApiKeys(apiUUID); }) .then((result) => { const apiKeyList = result.body; setApiKeys(apiKeyList); }) .catch((error) => { if (process.env.NODE_ENV !== 'production') { console.log(error); } - setIsRevoking(false); setRevokeErrorMessage( error.message || intl.formatMessage({ id: 'Apis.Details.APIKeys.ApiKeyListing.error.revokeFailed', defaultMessage: 'Failed to revoke API key. Please try again.', }), ); setRevokeErrorOpen(true); - }); + }) + .finally(() => { + setIsRevoking(false); + }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyListing.jsx` around lines 241 - 268, The revoke flow clears isRevoking too early in handleConfirmRevoke; keep the revocation state until both revokeAPIBoundAPIKey and the subsequent getApiApiKeys refresh complete by removing setIsRevoking(false) from the first .then and instead resetting it in a final .finally block (or after both promise chains) so setIsRevoking(false) runs regardless of success or error; update references in handleConfirmRevoke to call restApi.revokeAPIBoundAPIKey(apiUUID, selectedKeyForRevoke.keyUUID) then getApiApiKeys(apiUUID) and move setIsRevoking(false) into the .finally to ensure UI stays disabled during the full revoke+refresh flow and to still set revoke success/error state as before.
🧹 Nitpick comments (4)
portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx (1)
51-51: AddpropTypesforGatewayConfiguration(including newisReadOnly).Line 51 and Lines 354-355 introduced/used props that currently rely only on defaults. Adding
propTypeswill resolve the current static-analysis warnings and make the new API surface explicit.💡 Proposed patch
import React, { useEffect } from 'react'; +import PropTypes from 'prop-types'; ... GatewayConfiguration.defaultProps = { ... validating: false, }; + +GatewayConfiguration.propTypes = { + gatewayConfigurations: PropTypes.arrayOf(PropTypes.shape({ + name: PropTypes.string, + type: PropTypes.string, + values: PropTypes.array, + label: PropTypes.string, + tooltip: PropTypes.string, + required: PropTypes.bool, + default: PropTypes.any, + defaultValue: PropTypes.any, + updateDisabled: PropTypes.bool, + })), + additionalProperties: PropTypes.object, + setAdditionalProperties: PropTypes.func, + gatewayId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]), + hasErrors: PropTypes.func, + validating: PropTypes.bool, + isReadOnly: PropTypes.bool, +};Also applies to: 354-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx` at line 51, Import PropTypes and add a GatewayConfiguration.propTypes declaration listing the props used (at minimum hasErrors, validating, isReadOnly) with PropTypes.bool for those three; include any other props referenced in the component (e.g., props used around lines 354-355) with appropriate PropTypes (string, func, object, etc.) so the component's API is explicit and static-analysis warnings are resolved. Ensure the propTypes assignment is applied to the GatewayConfiguration function/class (e.g., GatewayConfiguration.propTypes = { hasErrors: PropTypes.bool, validating: PropTypes.bool, isReadOnly: PropTypes.bool, ... }) and keep existing defaultProps as-is.portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyGenerate.jsx (1)
254-264: Preferfinallyfor regeneration state cleanup.
setIsRegenerating(false)andsetRegeneratingKeyUUID(null)are duplicated in both success and error paths; moving them to a.finally()block will keep this flow easier to maintain.♻️ Suggested cleanup
restApi.regenerateApiApiKey(apiUUID, keyData.keyUUID) .then((response) => { const regeneratedKey = { keyName: response.body.keyName, apikey: response.body.apikey, validityPeriod: response.body.validityPeriod, }; setRegeneratedApiKey(regeneratedKey); setRegenerateModalOpen(true); - setIsRegenerating(false); - setRegeneratingKeyUUID(null); setTimeout(() => { refreshApiKeys(); }, 500); }) .catch((error) => { console.error('Error regenerating key:', error); - setIsRegenerating(false); - setRegeneratingKeyUUID(null); Alert.error(intl.formatMessage({ id: 'Apis.Details.APIKeys.ApiKeyGenerate.alert.regenerateFailed', defaultMessage: 'Failed to regenerate API Key. Please try again.', })); - }); + }) + .finally(() => { + setIsRegenerating(false); + setRegeneratingKeyUUID(null); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyGenerate.jsx` around lines 254 - 264, Move the duplicated cleanup calls into a .finally() to avoid repeating state resets: in the promise chain that handles key regeneration (the block that currently calls setIsRegenerating(false) and setRegeneratingKeyUUID(null) in both the .then and .catch), remove those two calls from the .then and .catch handlers and add a .finally(() => { setIsRegenerating(false); setRegeneratingKeyUUID(null); }); keeping refreshApiKeys() in the successful .then and the error logging/Alert.error in .catch so behavior is unchanged.portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx (1)
643-646: Consolidate gateway vendor checks to use extracted booleans for consistency.The new booleans at lines 643–645 improve readability, but raw
(api.gatewayVendor === 'wso2' || !api.gatewayVendor)checks persist at lines 791, 804, and 824. Replace them withisWSO2Gatewayto avoid maintenance drift where future changes may inconsistently update only one pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx` around lines 643 - 646, There are repeated raw checks of the gateway vendor using (api.gatewayVendor === 'wso2' || !api.gatewayVendor) scattered after the new booleans; replace each such occurrence with the extracted isWSO2Gateway boolean to keep logic consistent; update all usages in TryOutController.jsx (where isFederatedApiKeyFlow and shouldShowIntegratedAuthSection are defined) so the code references isWSO2Gateway instead of repeating the raw api.gatewayVendor expression and ensure behavior remains identical.portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyListing.jsx (1)
477-491: Avoid showing two icons during revoke loading.The button keeps
startIcon={<Block />}while also rendering an inline spinner in the label, which creates duplicated visual indicators. Prefer a conditionalstartIconand keep label text-only in loading state.Suggested refactor
<Button variant='outlined' size='small' color='error' - startIcon={<Block />} + startIcon={isRevoking && selectedKeyForRevoke?.keyUUID === keyData.keyUUID + ? <CircularProgress size={16} color='inherit' /> + : <Block />} onClick={() => handleRevokeKey(keyData)} disabled={isRevoking && selectedKeyForRevoke?.keyUUID === keyData.keyUUID} > {isRevoking && selectedKeyForRevoke?.keyUUID === keyData.keyUUID ? ( - <> - <CircularProgress size={16} sx={{ mr: 1 }} /> - <FormattedMessage - id='Apis.Details.APIKeys.ApiKeyListing.button.revoking' - defaultMessage='Revoking...' - /> - </> + <FormattedMessage + id='Apis.Details.APIKeys.ApiKeyListing.button.revoking' + defaultMessage='Revoking...' + /> ) : ( <FormattedMessage id='Apis.Details.APIKeys.ApiKeyListing.button.revoke' defaultMessage='Revoke' /> )} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyListing.jsx` around lines 477 - 491, The button currently shows both startIcon={<Block />} and an inline CircularProgress when revoking; update the JSX so the startIcon is conditional based on isRevoking/selectedKeyForRevoke (e.g., show <Block /> when not revoking, and show the CircularProgress as the startIcon or null when revoking) and remove the inline CircularProgress from the label; adjust the label to render only text ("Revoke" or "Revoking...") while keeping the existing onClick handler (handleRevokeKey) and disabled logic that checks isRevoking and selectedKeyForRevoke?.keyUUID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@portals/admin/src/main/webapp/site/public/locales/en.json`:
- Around line 594-600: Add the missing locale entry
"GatewayEnvironments.PlanMapping.helper" to the en.json catalog used by
GatewayPlanMapping.jsx; locate the defaultMessage text used in the
GatewayPlanMapping.jsx component for that id and add a matching key/value pair
(e.g., set the value to the component's defaultMessage) so the message id
resolves from the locale file instead of falling back to defaultMessage.
In
`@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayPlanMapping.jsx`:
- Around line 47-50: The generated DOM IDs for TextField become duplicated
because rest mappings can be mirrored into async via groupedValuesForDisplay and
mappingValue.id is reused; update the mapping/id creation so IDs incorporate the
API type to guarantee uniqueness—e.g., when constructing groupedValuesForDisplay
or when rendering (the code that reads mappingValue.id for the TextField),
append or prefix the apiType (like `${apiType}-${mappingValue.id}`) or
regenerate a unique id for mirrored entries; ensure any code that mirrors rest
into async (groupedValuesForDisplay) performs a deep copy and adjusts the id
field, and update the TextField id usage to use the new apiType-aware id (refer
to groupedValuesForDisplay and the place that reads mappingValue.id for the
TextField).
---
Outside diff comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyListing.jsx`:
- Around line 241-268: The revoke flow clears isRevoking too early in
handleConfirmRevoke; keep the revocation state until both revokeAPIBoundAPIKey
and the subsequent getApiApiKeys refresh complete by removing
setIsRevoking(false) from the first .then and instead resetting it in a final
.finally block (or after both promise chains) so setIsRevoking(false) runs
regardless of success or error; update references in handleConfirmRevoke to call
restApi.revokeAPIBoundAPIKey(apiUUID, selectedKeyForRevoke.keyUUID) then
getApiApiKeys(apiUUID) and move setIsRevoking(false) into the .finally to ensure
UI stays disabled during the full revoke+refresh flow and to still set revoke
success/error state as before.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/index.jsx`:
- Around line 650-664: The API Keys menu can render on MCP pages which don't
have the per-API route, causing a dead link; update the conditional that renders
LeftMenuItem (the block using user, showCredentials, api?.securityScheme and
pathPrefix) to also require a per-API identifier or route context — e.g. check
api?.uuid (or api?.id) is truthy or ensure pathPrefix includes '/apis/' — so the
LeftMenuItem for API Keys is only rendered when the per-API route
(/apis/:apiUuid/api-keys) exists.
In
`@portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Subscriptions/Subscriptions.jsx`:
- Around line 187-221: The SubscriptionAvailability component is rendered and
editable even when subscription management is unsupported, which hides the
save/cancel controls; update the render gating so SubscriptionAvailability is
shown only when isSubscriptionManagementSupported() is true (same gate used for
SubscriptionPoliciesManage) and still respects tenants,
settings.crossTenantSubscriptionEnabled, and !isSubValidationDisabled, or
alternatively move the save/cancel controls into a common parent that wraps both
SubscriptionPoliciesManage and SubscriptionAvailability; adjust references to
SubscriptionAvailability, SubscriptionPoliciesManage, and
isSubscriptionManagementSupported() accordingly so the UI never exposes editable
availability without a save path.
---
Nitpick comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx`:
- Line 51: Import PropTypes and add a GatewayConfiguration.propTypes declaration
listing the props used (at minimum hasErrors, validating, isReadOnly) with
PropTypes.bool for those three; include any other props referenced in the
component (e.g., props used around lines 354-355) with appropriate PropTypes
(string, func, object, etc.) so the component's API is explicit and
static-analysis warnings are resolved. Ensure the propTypes assignment is
applied to the GatewayConfiguration function/class (e.g.,
GatewayConfiguration.propTypes = { hasErrors: PropTypes.bool, validating:
PropTypes.bool, isReadOnly: PropTypes.bool, ... }) and keep existing
defaultProps as-is.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyGenerate.jsx`:
- Around line 254-264: Move the duplicated cleanup calls into a .finally() to
avoid repeating state resets: in the promise chain that handles key regeneration
(the block that currently calls setIsRegenerating(false) and
setRegeneratingKeyUUID(null) in both the .then and .catch), remove those two
calls from the .then and .catch handlers and add a .finally(() => {
setIsRegenerating(false); setRegeneratingKeyUUID(null); }); keeping
refreshApiKeys() in the successful .then and the error logging/Alert.error in
.catch so behavior is unchanged.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyListing.jsx`:
- Around line 477-491: The button currently shows both startIcon={<Block />} and
an inline CircularProgress when revoking; update the JSX so the startIcon is
conditional based on isRevoking/selectedKeyForRevoke (e.g., show <Block /> when
not revoking, and show the CircularProgress as the startIcon or null when
revoking) and remove the inline CircularProgress from the label; adjust the
label to render only text ("Revoke" or "Revoking...") while keeping the existing
onClick handler (handleRevokeKey) and disabled logic that checks isRevoking and
selectedKeyForRevoke?.keyUUID.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsx`:
- Around line 643-646: There are repeated raw checks of the gateway vendor using
(api.gatewayVendor === 'wso2' || !api.gatewayVendor) scattered after the new
booleans; replace each such occurrence with the extracted isWSO2Gateway boolean
to keep logic consistent; update all usages in TryOutController.jsx (where
isFederatedApiKeyFlow and shouldShowIntegratedAuthSection are defined) so the
code references isWSO2Gateway instead of repeating the raw api.gatewayVendor
expression and ensure behavior remains identical.
🪄 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: CHILL
Plan: Pro
Run ID: fa22ca30-41a4-42a8-9b93-83159b586ea8
📒 Files selected for processing (12)
portals/admin/src/main/webapp/site/public/locales/en.jsonportals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsxportals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayPlanMapping.jsxportals/devportal/src/main/webapp/site/public/locales/en.jsonportals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyAssociation.jsxportals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyGenerate.jsxportals/devportal/src/main/webapp/source/src/app/components/Apis/Details/APIKeys/ApiKeyListing.jsxportals/devportal/src/main/webapp/source/src/app/components/Apis/Details/ApiConsole/ApiConsole.jsxportals/devportal/src/main/webapp/source/src/app/components/Apis/Details/index.jsxportals/devportal/src/main/webapp/source/src/app/components/Shared/ApiTryOut/TryOutController.jsxportals/publisher/src/main/webapp/source/src/app/components/Apis/Details/Subscriptions/Subscriptions.jsxportals/publisher/src/main/webapp/source/src/app/components/Apis/Details/index.jsx
💤 Files with no reviewable changes (1)
- portals/publisher/src/main/webapp/source/src/app/components/Apis/Details/index.jsx
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)
portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx (1)
166-196:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential excessive re-renders due to
additionalPropertiesin dependency array.The
useEffectdepends onadditionalPropertiesbut also mutates it viasetAdditionalProperties. Each mutation triggers a re-render with updatedadditionalProperties, causing the effect to re-run. While it should eventually stabilize (once no more properties need clearing), this can cause several unnecessary render cycles on initial mount or when configurations change.Consider restructuring to avoid this pattern—e.g., computing properties to clear and performing a single batch update, or removing
additionalPropertiesfrom the dependency array if the cleanup should only run whengatewayConfigurationschanges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx` around lines 166 - 196, The effect currently lists additionalProperties in its dependency array and calls setAdditionalProperties per-key, causing repeated re-renders; instead, when gatewayConfigurations change compute the set of property keys to remove by using getAllNestedGatewayConfigPropertyNames and scanning additionalProperties and nested configs (use clearUnusedGatewayConfigProperties logic to decide nested clears), build a single update object (or list of keys) and call setAdditionalProperties once to apply all removals in batch; then remove additionalProperties from the useEffect dependency array so the cleanup runs only when gatewayConfigurations changes and avoid repeated effect re-invocations.
🧹 Nitpick comments (2)
portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayPlanMapping.jsx (1)
85-105: 💤 Low valuePrefer theme palette over hardcoded color values.
The
tableStylesobject uses hardcoded hex colors (#8A94A6,#EEF1F6,#2F3441) which won't adapt to theme changes (e.g., dark mode) and may diverge from the design system.♻️ Suggested theme-aware styling
- const tableStyles = { + const tableStyles = (theme) => ({ '& .MuiTableCell-head': { fontWeight: 500, - color: '#8A94A6', + color: theme.palette.text.secondary, fontSize: '0.8rem', - borderBottom: '1px solid `#EEF1F6`', + borderBottom: `1px solid ${theme.palette.divider}`, px: 2, py: 1, }, '& .MuiTableCell-body': { fontSize: '0.75rem', - borderBottom: '1px solid `#EEF1F6`', - color: '#2F3441', + borderBottom: `1px solid ${theme.palette.divider}`, + color: theme.palette.text.primary, px: 2, py: 1.5, verticalAlign: 'middle', }, '& .MuiTableRow-root:last-of-type .MuiTableCell-body': { borderBottom: 'none', }, - }; + });Then use it as a function in the
sxprop:-<Table size='small' sx={tableStyles}> +<Table size='small' sx={(theme) => tableStyles(theme)}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayPlanMapping.jsx` around lines 85 - 105, Replace the hardcoded hex colors in the tableStyles object with theme palette references so styles adapt to theme (e.g., dark mode); update the selectors '& .MuiTableCell-head', '& .MuiTableCell-body', and '& .MuiTableRow-root:last-of-type .MuiTableCell-body' to pull colors from the theme (e.g., theme.palette.text.secondary, theme.palette.divider, theme.palette.text.primary or appropriate semantic tokens) and convert tableStyles into a function that accepts theme (tableStyles = (theme) => ({ ... })) so it can be passed to the sx prop where GatewayPlanMapping uses it; ensure px/py values remain but replace '#8A94A6', '#EEF1F6', and '#2F3441' with the chosen theme palette properties.portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx (1)
21-21: 💤 Low valueConsider exporting
PLAN_MAPPING_PROPERTY_PREFIXfrom a shared location.This constant is duplicated in
GatewayPlanMapping.jsx(line 19). If the prefix ever changes, both files need to be updated, creating a maintenance risk.♻️ Suggested approach
Export the constant from one file and import it in the other:
// In GatewayPlanMapping.jsx - export it -const PLAN_MAPPING_PROPERTY_PREFIX = 'plan_mapping.'; +export const PLAN_MAPPING_PROPERTY_PREFIX = 'plan_mapping.';// In GatewayConfiguration.jsx - import it -import GatewayPlanMapping from './GatewayPlanMapping'; +import GatewayPlanMapping, { PLAN_MAPPING_PROPERTY_PREFIX } from './GatewayPlanMapping'; -const PLAN_MAPPING_PROPERTY_PREFIX = 'plan_mapping.';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx` at line 21, Duplicate constant PLAN_MAPPING_PROPERTY_PREFIX should be centralized: create a single export (e.g., export const PLAN_MAPPING_PROPERTY_PREFIX = 'plan_mapping.' ) in a shared module (shared constants or utils) and replace the local declarations in both GatewayConfiguration.jsx and GatewayPlanMapping.jsx with an import of that exported symbol; update any references to PLAN_MAPPING_PROPERTY_PREFIX in those files to use the imported constant and remove the duplicated local definitions.
🤖 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
`@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx`:
- Around line 166-196: The effect currently lists additionalProperties in its
dependency array and calls setAdditionalProperties per-key, causing repeated
re-renders; instead, when gatewayConfigurations change compute the set of
property keys to remove by using getAllNestedGatewayConfigPropertyNames and
scanning additionalProperties and nested configs (use
clearUnusedGatewayConfigProperties logic to decide nested clears), build a
single update object (or list of keys) and call setAdditionalProperties once to
apply all removals in batch; then remove additionalProperties from the useEffect
dependency array so the cleanup runs only when gatewayConfigurations changes and
avoid repeated effect re-invocations.
---
Nitpick comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx`:
- Line 21: Duplicate constant PLAN_MAPPING_PROPERTY_PREFIX should be
centralized: create a single export (e.g., export const
PLAN_MAPPING_PROPERTY_PREFIX = 'plan_mapping.' ) in a shared module (shared
constants or utils) and replace the local declarations in both
GatewayConfiguration.jsx and GatewayPlanMapping.jsx with an import of that
exported symbol; update any references to PLAN_MAPPING_PROPERTY_PREFIX in those
files to use the imported constant and remove the duplicated local definitions.
In
`@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayPlanMapping.jsx`:
- Around line 85-105: Replace the hardcoded hex colors in the tableStyles object
with theme palette references so styles adapt to theme (e.g., dark mode); update
the selectors '& .MuiTableCell-head', '& .MuiTableCell-body', and '&
.MuiTableRow-root:last-of-type .MuiTableCell-body' to pull colors from the theme
(e.g., theme.palette.text.secondary, theme.palette.divider,
theme.palette.text.primary or appropriate semantic tokens) and convert
tableStyles into a function that accepts theme (tableStyles = (theme) => ({ ...
})) so it can be passed to the sx prop where GatewayPlanMapping uses it; ensure
px/py values remain but replace '#8A94A6', '#EEF1F6', and '#2F3441' with the
chosen theme palette properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9858a193-62ab-45e7-b38e-78d44dda0119
📒 Files selected for processing (2)
portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsxportals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayPlanMapping.jsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx (1)
21-21: ⚡ Quick win
PLAN_MAPPING_PROPERTY_PREFIXis defined identically in three files.The string
'plan_mapping.'is now declared as a module-level constant inGatewayPlanMapping.jsx(line 17),AddEditGWEnvironment.jsx(line 83), andGatewayConfiguration.jsx(line 21). Any future rename requires three coordinated edits.Extract the constant to a single shared location (e.g.,
PlatformGatewayUtils.jsor a newGatewayConstants.js) and import it in all three files:- const PLAN_MAPPING_PROPERTY_PREFIX = 'plan_mapping.'; + import { PLAN_MAPPING_PROPERTY_PREFIX } from './PlatformGatewayUtils';Apply the same change to
GatewayPlanMapping.jsxandAddEditGWEnvironment.jsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx` at line 21, The constant PLAN_MAPPING_PROPERTY_PREFIX ('plan_mapping.') is duplicated across GatewayConfiguration.jsx, GatewayPlanMapping.jsx, and AddEditGWEnvironment.jsx—extract it into a single shared module (e.g., GatewayConstants.js or PlatformGatewayUtils.js) exporting PLAN_MAPPING_PROPERTY_PREFIX, then replace the local declarations in the three components with an import of that exported constant; update imports in GatewayConfiguration.jsx, GatewayPlanMapping.jsx, and AddEditGWEnvironment.jsx to reference the new module and remove the duplicated definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/AddEditGWEnvironment.jsx`:
- Around line 1259-1267: The else branch silently swallows errors when
response?.body is falsy; update the catch in the promise to provide user
feedback by calling Alert.error with a meaningful fallback (e.g., error.message
or a generic "Save failed" plus response.status/statusText) while still clearing
plan mapping errors via setPlanMappingErrors({}) and turning off the spinner via
setSaving(false); locate the catch handler that references setPlanMappingErrors
and extractPlanMappingErrors and ensure Alert.error is invoked in the else path
using available symbols (error, response) so the user sees a visible failure
notification.
---
Nitpick comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsx`:
- Line 21: The constant PLAN_MAPPING_PROPERTY_PREFIX ('plan_mapping.') is
duplicated across GatewayConfiguration.jsx, GatewayPlanMapping.jsx, and
AddEditGWEnvironment.jsx—extract it into a single shared module (e.g.,
GatewayConstants.js or PlatformGatewayUtils.js) exporting
PLAN_MAPPING_PROPERTY_PREFIX, then replace the local declarations in the three
components with an import of that exported constant; update imports in
GatewayConfiguration.jsx, GatewayPlanMapping.jsx, and AddEditGWEnvironment.jsx
to reference the new module and remove the duplicated definitions.
🪄 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: CHILL
Plan: Pro
Run ID: e27a4f5e-7d31-4f04-94be-6d3480a9903c
📒 Files selected for processing (3)
portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/AddEditGWEnvironment.jsxportals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayConfiguration.jsxportals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/GatewayPlanMapping.jsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/AddEditGWEnvironment.jsx (1)
381-381: ⚡ Quick winLocalize the fallback plan-mapping error text.
Line 381 introduces a hardcoded user-facing English string. Route this through
intl.formatMessageto keep portal localization consistent.Suggested refactor
-const extractPlanMappingErrors = (responseBody) => { +const extractPlanMappingErrors = (responseBody, formatMessage) => { const rowErrors = {}; @@ - rowErrors[fieldKey] = errorItem.description || 'Invalid external plan assignment'; + rowErrors[fieldKey] = errorItem.description || formatMessage({ + id: 'GatewayEnvironments.AddEditGWEnvironment.plan.mapping.invalid.assignment', + defaultMessage: 'Invalid external plan assignment', + }); } }); return rowErrors; };- setPlanMappingErrors(extractPlanMappingErrors(response.body)); + setPlanMappingErrors(extractPlanMappingErrors(response.body, intl.formatMessage));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/AddEditGWEnvironment.jsx` at line 381, Replace the hardcoded fallback string assigned to rowErrors[fieldKey] in AddEditGWEnvironment.jsx with a localized message via intl.formatMessage; specifically, change the assignment rowErrors[fieldKey] = 'Invalid external plan assignment' to rowErrors[fieldKey] = intl.formatMessage({ id: 'gateway.planMappingInvalid', defaultMessage: 'Invalid external plan assignment' }) (or use an existing message id if one is already defined), ensuring intl is available in the component props or via injectIntl/useIntl before making this replacement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/AddEditGWEnvironment.jsx`:
- Around line 378-381: The loop over responseBody.error calls startsWith on
errorItem.message which can be a non-string; update the handler in the
responseBody.error.forEach callback to first check typeof errorItem.message ===
'string' (or coerce safely) before using startsWith with
PLAN_MAPPING_PROPERTY_PREFIX, and only then assign rowErrors[fieldKey] =
errorItem.description || 'Invalid external plan assignment'; ensure fieldKey
references the validated string value (e.g., const fieldKey = errorItem.message)
so non-string values are skipped and no runtime exception is thrown.
---
Nitpick comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/AddEditGWEnvironment.jsx`:
- Line 381: Replace the hardcoded fallback string assigned to
rowErrors[fieldKey] in AddEditGWEnvironment.jsx with a localized message via
intl.formatMessage; specifically, change the assignment rowErrors[fieldKey] =
'Invalid external plan assignment' to rowErrors[fieldKey] = intl.formatMessage({
id: 'gateway.planMappingInvalid', defaultMessage: 'Invalid external plan
assignment' }) (or use an existing message id if one is already defined),
ensuring intl is available in the component props or via injectIntl/useIntl
before making this replacement.
🪄 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: CHILL
Plan: Pro
Run ID: 670cf00f-7119-4ad2-ae06-56373196a5df
📒 Files selected for processing (2)
portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/AddEditGWEnvironment.jsxportals/devportal/src/main/webapp/source/src/app/components/Apis/Details/index.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/index.jsx
Comment on lines
+378
to
+381
| responseBody.error.forEach((errorItem) => { | ||
| const fieldKey = errorItem?.message; | ||
| if (fieldKey && fieldKey.startsWith(PLAN_MAPPING_PROPERTY_PREFIX)) { | ||
| rowErrors[fieldKey] = errorItem.description || 'Invalid external plan assignment'; |
There was a problem hiding this comment.
Guard errorItem.message type before calling startsWith.
Line 380 can throw if errorItem.message is a non-string truthy value. Add a string type guard to keep error handling resilient to payload drift.
Suggested fix
responseBody.error.forEach((errorItem) => {
const fieldKey = errorItem?.message;
- if (fieldKey && fieldKey.startsWith(PLAN_MAPPING_PROPERTY_PREFIX)) {
+ if (typeof fieldKey === 'string' && fieldKey.startsWith(PLAN_MAPPING_PROPERTY_PREFIX)) {
rowErrors[fieldKey] = errorItem.description || 'Invalid external plan assignment';
}
});🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 380-380: Prefer using an optional chain expression instead, as it's more concise and easier to read.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@portals/admin/src/main/webapp/source/src/app/components/GatewayEnvironments/AddEditGWEnvironment.jsx`
around lines 378 - 381, The loop over responseBody.error calls startsWith on
errorItem.message which can be a non-string; update the handler in the
responseBody.error.forEach callback to first check typeof errorItem.message ===
'string' (or coerce safely) before using startsWith with
PLAN_MAPPING_PROPERTY_PREFIX, and only then assign rowErrors[fieldKey] =
errorItem.description || 'Invalid external plan assignment'; ensure fieldKey
references the validated string value (e.g., const fieldKey = errorItem.message)
so non-string values are skipped and no runtime exception is thrown.
|
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.
What changed
Why
These changes add the remaining UI support needed for federated gateway API key flows across the Admin, Devportal, and Publisher portals.
Impact