Provide support to upgrade opaque apps to JWT from Admin portal#1273
Merged
Provide support to upgrade opaque apps to JWT from Admin portal#1273
Conversation
tharikaGitHub
requested review from
Krishanx92,
lasanthaS and
piyumaldk
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:
WalkthroughAdds an Application Settings UI: new components (ChangeAppOwner, UpgradeTokenType, UpgradeToJWTDialog, TabbedContentBase), refactors ListApplications into a tabbed flow, makes table components data-driven, adds API method to upgrade token types, and relocates many locale keys from Applications.Listing.* to ApplicationSettings.* while renaming a route label. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant ListApp as ListApplications
participant API as API
participant Tabbed as TabbedContentBase
participant UpgradeList as UpgradeTokenType
participant Dialog as UpgradeToJWTDialog
User->>ListApp: Open Application Settings
ListApp->>API: fetch applications (includes tokenType)
API-->>ListApp: returns apps
alt Upgradable apps exist
ListApp->>Tabbed: render tabs (Change Settings, Upgrade Legacy)
User->>Tabbed: select "Upgrade Legacy"
Tabbed->>UpgradeList: pass childProps (apiCall, pagination, search)
UpgradeList->>User: show upgradable apps
User->>UpgradeList: click "Upgrade"
UpgradeList->>Dialog: open confirmation
User->>Dialog: confirm
Dialog->>API: POST upgradeApplicationTokenType(appId)
API-->>Dialog: success/error
Dialog->>UpgradeList: updateList (refresh)
else No upgradable apps
ListApp->>User: render ChangeAppOwner view
end
sequenceDiagram
participant User as User
participant ChangeOwner as ChangeAppOwner
participant AppsTable as AppsTableContent
participant EditApp as EditApplication
participant API as API
User->>ChangeOwner: type search query
ChangeOwner->>AppsTable: pass filtered applicationList
AppsTable->>AppsTable: render columns via columns[].render(app)
User->>AppsTable: click Edit (Actions)
AppsTable->>EditApp: render EditApplication
User->>EditApp: submit owner change
EditApp->>API: POST change owner
API-->>EditApp: success
EditApp->>ChangeOwner: trigger updateList
ChangeOwner->>User: refreshed list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ApplicationTableHead.jsx (1)
65-77:⚠️ Potential issue | 🟠 MajorThis prop contract breaks the current callers.
ChangeAppOwner.jsxLine 205 andUpgradeTokenType.jsxLine 217 still render<ApplicationTableHead columnData={columnData} />. With this change, sortable columns will renderTableSortLabelwithorder={undefined}, and clicking them will call an undefinedonRequestSort. Please either wire the new sorting props through those callers in this PR or keep these props backward-compatible here with safe defaults/guards.🤖 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/ApplicationSettings/ApplicationTableHead.jsx` around lines 65 - 77, The prop-type change for ApplicationTableHead made onRequestSort, order and orderBy required which breaks callers (ChangeAppOwner.jsx and UpgradeTokenType.jsx) that still pass only columnData; restore backward-compatibility by making onRequestSort, order and orderBy optional in ApplicationTableHead and add safe defaults/guards inside the component (e.g., default onRequestSort to a no-op and only render/activate TableSortLabel or call onRequestSort when onRequestSort and order are provided) so existing callers that pass only columnData continue to work; update the propTypes for ApplicationTableHead and any internal usage sites (render logic that references order/orderBy/onRequestSort) accordingly.portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx (2)
57-63:⚠️ Potential issue | 🟠 MajorDon’t forward
apiCallas a zero-argument refresh callback.
apiCallrequirespageNo, but the new action flows treat their refresh prop asupdateList(). With the current signature, that makesoffset: pageNo * rowsPerPageevaluate toNaNafter an owner/token mutation. Pass a bound refresher like() => apiCall(page)here, or defaultpageNoto the current page before handing this function to children.Also applies to: 157-170
🤖 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/ApplicationSettings/ListApplications.jsx` around lines 57 - 63, The refresh callback is being passed as a zero-arg function into child actions even though apiCall(pageNo, ...) requires pageNo; this causes offset to compute NaN after mutations. Fix by either binding the current page when passing the refresher (e.g. pass () => apiCall(page)) or change apiCall to default its first parameter to the current page state (use page state as default for pageNo) so calls like updateList() work; update the call sites around apiCall and where updateList is passed (also at the other occurrence around lines 157-170) to use the bound refresher or the new default.
78-156:⚠️ Potential issue | 🟠 MajorConsolidate fetching into one state-driven path.
This component fetches in both
useEffectblocks and again inside the pagination/search handlers. That gives you duplicate requests on mount and page changes, and stale-parameter requests aftersetPage/setRowsPerPage, so an older response can overwrite the correct slice.🤖 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/ApplicationSettings/ListApplications.jsx` around lines 78 - 156, The component currently calls apiCall in multiple places (two useEffect blocks plus inside handleChangePage, handleChangeRowsPerPage, clearSearch, and filterApps) causing duplicate requests and race conditions when setState is async; consolidate fetching into one state-driven useEffect that depends on page, rowsPerPage, and searchQuery and invokes apiCall(page, rowsPerPage, searchQuery) (or appropriate args) to update setApplicationList; remove direct apiCall calls from handleChangePage, handleChangeRowsPerPage, clearSearch, and filterApps so those handlers only update state (e.g., handleChangePage should only call setPage(pageNo), handleChangeRowsPerPage should compute nextPage, call setRowsPerPage(nextRowsPerPage) and setPage(nextPage), clearSearch should setPage(0) and setSearchQuery('')), and ensure the single useEffect uses the latest state values (page/rowsPerPage/searchQuery) when calling apiCall to prevent stale responses from overwriting data.
🧹 Nitpick comments (1)
portals/admin/src/main/webapp/source/src/app/components/AdminPages/Addons/TabbedContentBase.jsx (1)
51-51: Avoid pinning the warning to tab index1.This component is otherwise generic, but the warning only appears for the second tab. If the tabs are reordered or this wrapper is reused elsewhere, the warning will show on the wrong tab. Drive it from tab metadata or an explicit prop such as
warningTabId/warningTabIndexinstead.🤖 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/AdminPages/Addons/TabbedContentBase.jsx` at line 51, The warning is currently pinned to the second tab via the hardcoded check "value === 1" in TabbedContentBase.jsx; change this to use explicit metadata or a prop so the component stays generic—add a prop such as "warningTabIndex" (or read a "warning" flag from each tab object in the tabs array) and replace "value === 1 ? warning : null" with a comparison against that prop or the tab's metadata (e.g., value === warningTabIndex or currentTab.warning). Update the component signature to accept the new prop (or expect the warning flag on tab objects) and adjust callers to pass the correct index/metadata.
🤖 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 374-385: The FormattedMessage IDs and locale keys use the
misspelled namespace "UpgradeTokeType" but the component/file is
"UpgradeTokenType.jsx"; rename all occurrences of
"ApplicationSettings.UpgradeTokeType" to "ApplicationSettings.UpgradeTokenType"
in the component (UpgradeTokenType.jsx) so FormattedMessage() uses the correct
id, and update the corresponding keys in both locale JSON files (en.json and
fr.json) to match the corrected namespace so IDs are consistent across source
and locales.
In
`@portals/admin/src/main/webapp/source/src/app/components/AdminPages/Addons/TabbedContentBase.jsx`:
- Around line 72-75: The tabs prop currently assumes each tab has an id and a
string label, but callers (e.g., ListApplications) pass no id and use a JSX node
label; update the TabbedContentBase component to accept label as any renderable
node (change PropTypes to label: PropTypes.node) and use a stable fallback key
when rendering Tab (e.g., key={tab.id || (typeof tab.label === 'string' ?
tab.label : undefined) || `tab-${index}`}), applying the same fix in the second
rendering block (the 107-116 equivalent) so keys are never undefined for static
tab sets.
- Around line 47-52: TabbedContentBase is declaring PaperProps in
propTypes/defaultProps but never forwards it to the underlying ContentBase, so
any caller customization is ignored; fix by accepting the PaperProps prop in the
TabbedContentBase component signature and pass it through to ContentBase (i.e.,
add PaperProps={PaperProps} to the ContentBase JSX), and do the same for the
other ContentBase usage in the file (the second occurrence around the tab
rendering) so both locations honor the prop; reference the PaperProps identifier
and the ContentBase component to locate where to forward the prop.
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsx`:
- Around line 206-217: The wrapper is passing editComponentProps to
AppsTableContent that don't match EditApplication's API: EditApplication expects
a triggerButtonText prop and title as an object (i18n descriptor), but here
editComponentProps only includes icon, applicationList, and a formatted string
title; update the editComponentProps passed from ChangeAppOwner.jsx to include
triggerButtonText and supply title as the same intl message descriptor object
(not the formatted string) used in EditApplication (keep icon and
applicationList unchanged) so the row action receives the complete contract
required by EditApplication.
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx`:
- Around line 209-279: The current logic derives upgradableApps by filtering
applicationList (a paginated slice), so the warning/tab and UpgradeTokenType
table only reflect the current page; instead, call the server (or use a provided
prop) to get a server-side legacy count/list and use that to decide UI and feed
the upgrade table. Concretely: stop computing upgradableApps from
applicationList; add/consume a server-provided legacyAppsCount or legacyAppsPage
endpoint (or extend the existing applications API to support tokenType=opaque
filter), update the condition that builds warning and the tabs (symbols:
applicationList, upgradableApps, oldestCreatedTime, timeAgo, tabs,
UpgradeTokenType, TabbedContentBase) to use that server-side count/list, and
ensure UpgradeTokenType fetches paginated legacy apps from the server rather
than relying on client-side filtering so totalApps remains correct.
- Around line 231-235: The Link component rendering the external docs URL (see
the Link element with href
`${Configurations.app.docUrl}api-security/key-management/tokens/jwt-tokens/`)
has a typo in the rel prop: `noreferre` should be `noreferrer`; update the rel
attribute to include the correct token (e.g., `noopener noreferrer`) so the
browser does not send the Referer header and still benefits from noopener
protection.
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeToJWTDialog.jsx`:
- Around line 48-70: The upgrade flow allows duplicate submissions and always
closes the dialog because handleUpgrade unconditionally calls handleClose() and
updateList() in finally; add a local boolean state (e.g., isUpgrading) used by
the Upgrade button to disable it while the POST via
restApi.upgradeApplicationTokenType(app.applicationId) is in flight, guard early
to return if isUpgrading to prevent duplicates, set isUpgrading=true before
calling the API and false after, and move handleClose() and updateList() into
the success path only (call them after Alert.success) while keeping the dialog
open on error so Alert.error can be seen; update references to handleUpgrade,
restApi.upgradeApplicationTokenType, Alert.success, Alert.error, handleClose,
and updateList accordingly.
---
Outside diff comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ApplicationTableHead.jsx`:
- Around line 65-77: The prop-type change for ApplicationTableHead made
onRequestSort, order and orderBy required which breaks callers
(ChangeAppOwner.jsx and UpgradeTokenType.jsx) that still pass only columnData;
restore backward-compatibility by making onRequestSort, order and orderBy
optional in ApplicationTableHead and add safe defaults/guards inside the
component (e.g., default onRequestSort to a no-op and only render/activate
TableSortLabel or call onRequestSort when onRequestSort and order are provided)
so existing callers that pass only columnData continue to work; update the
propTypes for ApplicationTableHead and any internal usage sites (render logic
that references order/orderBy/onRequestSort) accordingly.
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx`:
- Around line 57-63: The refresh callback is being passed as a zero-arg function
into child actions even though apiCall(pageNo, ...) requires pageNo; this causes
offset to compute NaN after mutations. Fix by either binding the current page
when passing the refresher (e.g. pass () => apiCall(page)) or change apiCall to
default its first parameter to the current page state (use page state as default
for pageNo) so calls like updateList() work; update the call sites around
apiCall and where updateList is passed (also at the other occurrence around
lines 157-170) to use the bound refresher or the new default.
- Around line 78-156: The component currently calls apiCall in multiple places
(two useEffect blocks plus inside handleChangePage, handleChangeRowsPerPage,
clearSearch, and filterApps) causing duplicate requests and race conditions when
setState is async; consolidate fetching into one state-driven useEffect that
depends on page, rowsPerPage, and searchQuery and invokes apiCall(page,
rowsPerPage, searchQuery) (or appropriate args) to update setApplicationList;
remove direct apiCall calls from handleChangePage, handleChangeRowsPerPage,
clearSearch, and filterApps so those handlers only update state (e.g.,
handleChangePage should only call setPage(pageNo), handleChangeRowsPerPage
should compute nextPage, call setRowsPerPage(nextRowsPerPage) and
setPage(nextPage), clearSearch should setPage(0) and setSearchQuery('')), and
ensure the single useEffect uses the latest state values
(page/rowsPerPage/searchQuery) when calling apiCall to prevent stale responses
from overwriting data.
---
Nitpick comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/AdminPages/Addons/TabbedContentBase.jsx`:
- Line 51: The warning is currently pinned to the second tab via the hardcoded
check "value === 1" in TabbedContentBase.jsx; change this to use explicit
metadata or a prop so the component stays generic—add a prop such as
"warningTabIndex" (or read a "warning" flag from each tab object in the tabs
array) and replace "value === 1 ? warning : null" with a comparison against that
prop or the tab's metadata (e.g., value === warningTabIndex or
currentTab.warning). Update the component signature to accept the new prop (or
expect the warning flag on tab objects) and adjust callers to pass the correct
index/metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: efe0f9fe-ff43-43a6-a39a-20c02fc1c4e7
📒 Files selected for processing (13)
portals/admin/src/main/webapp/site/public/locales/en.jsonportals/admin/src/main/webapp/site/public/locales/fr.jsonportals/admin/src/main/webapp/source/src/app/components/AdminPages/Addons/ContentBase.jsxportals/admin/src/main/webapp/source/src/app/components/AdminPages/Addons/TabbedContentBase.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ApplicationTableHead.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/AppsTableContent.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeToJWTDialog.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeTokenType.jsxportals/admin/src/main/webapp/source/src/app/components/Base/RouteMenuMapping.jsxportals/admin/src/main/webapp/source/src/app/data/api.jsportals/devportal/src/main/webapp/site/public/locales/en.json
portals/admin/src/main/webapp/source/src/app/components/AdminPages/Addons/TabbedContentBase.jsx
Show resolved
Hide resolved
portals/admin/src/main/webapp/source/src/app/components/AdminPages/Addons/TabbedContentBase.jsx
Outdated
Show resolved
Hide resolved
portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsx
Show resolved
Hide resolved
...als/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx
Outdated
Show resolved
Hide resolved
...als/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx
Show resolved
Hide resolved
...s/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeToJWTDialog.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx (1)
209-220:⚠️ Potential issue | 🔴 CriticalDon’t gate the upgrade flow on the current page slice.
applicationListcomes from a paginatedlimit/offsetAPI call, butupgradableAppsis derived by filtering only that slice. If the visible page happens to contain only JWT apps, the upgrade tab and warning disappear even while later pages still contain opaque apps. This needs a server-side legacy-app count/list instead of filtering the current page.Also applies to: 263-272
🤖 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/ApplicationSettings/ListApplications.jsx` around lines 209 - 220, The UI is incorrectly deciding upgrade visibility from the paginated applicationList slice (via upgradableApps and oldestCreatedTime); instead, call a server-side endpoint (or include fields in the applications API) that returns the total legacy/opaque-app count or the list/oldest createdTime for legacy apps, then use that server-provided value to drive the upgrade tab and warning; remove reliance on filtering applicationList in ListApplications.jsx (references: applicationList, upgradableApps, oldestCreatedTime, getTimeAgo) and update the logic in both places (current block and the similar block at lines 263-272) to use the new server-provided legacy count/list/oldestLegacyCreatedTime.
🤖 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/ApplicationSettings/ChangeAppOwner.jsx`:
- Around line 167-173: The IconButton in ChangeAppOwner.jsx uses
aria-label='delete' which miscommunicates the action to assistive tech; update
the IconButton's aria-label to the localized "Clear Search" label (reuse the
existing localization string used elsewhere in this component) so the accessible
name matches the clearSearch action, keeping the onClick={clearSearch} and the
HighlightOffRoundedIcon unchanged.
- Around line 206-225: The wrapper in ChangeAppOwner.jsx is violating
AppsTableContent's prop contract: AppsTableContent expects apps to be a Map and
requires toggleDeleteConfirmation, but the wrapper passes applicationList (an
array) and omits toggleDeleteConfirmation; update the wrapper to pass the
correct types and required callback by converting applicationList into the same
Map shape AppsTableContent expects (or adapt AppsTableContent if intended) and
supply the toggleDeleteConfirmation prop (a function matching AppsTableContent's
signature). Locate the AppsTableContent usage in ChangeAppOwner.jsx and adjust
the apps prop and add toggleDeleteConfirmation to align with the component's
declared API (AppsTableContent, apps, toggleDeleteConfirmation).
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx`:
- Around line 244-247: The first tab label in the tabs array is hardcoded
("Change Owner"); update ListApplications.jsx to use react-intl similar to the
other tabs by replacing the literal with an internationalized message using the
existing intl API or FormattedMessage—specifically change the tabs entry that
sets label: 'Change Owner' to use intl.formatMessage or <FormattedMessage
id="..." defaultMessage="Change Owner" /> and ensure the message id matches the
locale keys added in this PR so the ChangeAppOwner tab is localized.
---
Duplicate comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx`:
- Around line 209-220: The UI is incorrectly deciding upgrade visibility from
the paginated applicationList slice (via upgradableApps and oldestCreatedTime);
instead, call a server-side endpoint (or include fields in the applications API)
that returns the total legacy/opaque-app count or the list/oldest createdTime
for legacy apps, then use that server-provided value to drive the upgrade tab
and warning; remove reliance on filtering applicationList in
ListApplications.jsx (references: applicationList, upgradableApps,
oldestCreatedTime, getTimeAgo) and update the logic in both places (current
block and the similar block at lines 263-272) to use the new server-provided
legacy count/list/oldestLegacyCreatedTime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b580e442-468f-4098-be8b-aba64084dc2e
📒 Files selected for processing (7)
portals/admin/src/main/webapp/site/public/locales/en.jsonportals/admin/src/main/webapp/site/public/locales/fr.jsonportals/admin/src/main/webapp/source/src/app/components/AdminPages/Addons/TabbedContentBase.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeToJWTDialog.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeTokenType.jsx
🚧 Files skipped from review as they are similar to previous changes (4)
- portals/admin/src/main/webapp/source/src/app/components/AdminPages/Addons/TabbedContentBase.jsx
- portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeTokenType.jsx
- portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeToJWTDialog.jsx
- portals/admin/src/main/webapp/site/public/locales/en.json
portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsx
Show resolved
Hide resolved
portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsx
Show resolved
Hide resolved
...als/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx
Show resolved
Hide resolved
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 (2)
portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx (2)
108-117:⚠️ Potential issue | 🟠 MajorStale state:
apiCall(page)uses old page value.
setPage(nextPage)schedules a state update but doesn't immediately changepage. CallingapiCall(page)on line 114 fetches data for the old page, notnextPage.🐛 Proposed fix
function handleChangeRowsPerPage(event) { const nextRowsPerPage = event.target.value; const rowsPerPageRatio = rowsPerPage / nextRowsPerPage; const nextPage = Math.floor(page * rowsPerPageRatio); setPage(nextPage); setRowsPerPage(nextRowsPerPage); - apiCall(page).then((result) => { + apiCall(nextPage).then((result) => { setApplicationList(result); }); }🤖 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/ApplicationSettings/ListApplications.jsx` around lines 108 - 117, The handler handleChangeRowsPerPage uses the stale page state when calling apiCall(page); instead pass the computed nextPage (and ensure nextRowsPerPage is used if needed) to apiCall so the fetch uses the updated page—i.e., after computing nextRowsPerPage, rowsPerPageRatio and nextPage, call setPage(nextPage), setRowsPerPage(nextRowsPerPage) and then apiCall(nextPage). Use the local nextPage variable rather than the page state to avoid the async state update race.
122-128:⚠️ Potential issue | 🟠 MajorStale state:
apiCall(page, '', '')uses old page value.Same issue as
handleChangeRowsPerPage—setPage(0)doesn't updatepagesynchronously, so the API call fetches the wrong page.🐛 Proposed fix
function clearSearch() { setPage(0); setSearchQuery(''); - apiCall(page, '', '').then((result) => { + apiCall(0, '', '').then((result) => { setApplicationList(result); }); }🤖 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/ApplicationSettings/ListApplications.jsx` around lines 122 - 128, In clearSearch the apiCall uses the stale page variable because setPage(0) is async; change the API invocation to use the new page value directly (call apiCall(0, '', '') instead of apiCall(page, '', '')) so the request fetches page 0; update the then handler as-is to call setApplicationList(result). Reference: clearSearch, setPage, setSearchQuery, apiCall, setApplicationList.
🧹 Nitpick comments (1)
portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx (1)
78-88: Consider consolidating useEffect dependencies.Both useEffects perform the same action. Combined with handlers that also call
apiCalldirectly, this can cause redundant API calls whenpageorrowsPerPagechanges.♻️ Suggested consolidation
- useEffect(() => { - apiCall(page).then((result) => { - setApplicationList(result); - }); - }, [page]); - - useEffect(() => { - apiCall(page).then((result) => { - setApplicationList(result); - }); - }, [rowsPerPage]); + useEffect(() => { + apiCall(page).then((result) => { + setApplicationList(result); + }); + }, [page, rowsPerPage]);🤖 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/ApplicationSettings/ListApplications.jsx` around lines 78 - 88, The two identical useEffect blocks calling apiCall(page) should be consolidated into a single useEffect that depends on both page and rowsPerPage (e.g., useEffect(() => { apiCall(page, rowsPerPage).then(setApplicationList) }, [page, rowsPerPage])) so changes to either trigger exactly one fetch; update the apiCall invocation to accept/forward rowsPerPage if needed and remove or adjust any handlers that also call apiCall (pagination or rowsPerPage change handlers) to avoid double-fetches—look for useEffect, apiCall, setApplicationList, and any page/rowsPerPage change handlers to apply this change.
🤖 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 358-364: Update the malformed i18n key used in both locale files
and the component: change the key
ApplicationSettings.ListApplicationschange.app.owner.tab.title.tab.title to the
correct ApplicationSettings.ListApplications.change.app.owner.tab.title in
en.json and fr.json, and update the lookup in the ListApplications.jsx component
(where that key is referenced) to use the corrected key string so message
resolution succeeds.
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx`:
- Around line 150-156: The handler filterApps calls setPage(0) but then uses the
stale page value in apiCall(page); change the call to explicitly use 0 (i.e.,
call apiCall(0)) and keep setting the result into setApplicationList so the API
always requests the first page after filtering; reference functions: filterApps,
setPage, apiCall, setApplicationList.
- Around line 200-203: The intl message id used in the label inside the
ListApplications component is malformed; update the id string passed to
intl.formatMessage in the label (the call within
ApplicationSettings/ListApplications.jsx) by inserting the missing dot after
"ListApplications" and removing the duplicated ".tab.title" suffix so it becomes
a single, correct dotted key (e.g.,
"ApplicationSettings.ListApplications.change.app.owner.tab.title") to ensure
translation lookup succeeds.
---
Outside diff comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx`:
- Around line 108-117: The handler handleChangeRowsPerPage uses the stale page
state when calling apiCall(page); instead pass the computed nextPage (and ensure
nextRowsPerPage is used if needed) to apiCall so the fetch uses the updated
page—i.e., after computing nextRowsPerPage, rowsPerPageRatio and nextPage, call
setPage(nextPage), setRowsPerPage(nextRowsPerPage) and then apiCall(nextPage).
Use the local nextPage variable rather than the page state to avoid the async
state update race.
- Around line 122-128: In clearSearch the apiCall uses the stale page variable
because setPage(0) is async; change the API invocation to use the new page value
directly (call apiCall(0, '', '') instead of apiCall(page, '', '')) so the
request fetches page 0; update the then handler as-is to call
setApplicationList(result). Reference: clearSearch, setPage, setSearchQuery,
apiCall, setApplicationList.
---
Nitpick comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx`:
- Around line 78-88: The two identical useEffect blocks calling apiCall(page)
should be consolidated into a single useEffect that depends on both page and
rowsPerPage (e.g., useEffect(() => { apiCall(page,
rowsPerPage).then(setApplicationList) }, [page, rowsPerPage])) so changes to
either trigger exactly one fetch; update the apiCall invocation to
accept/forward rowsPerPage if needed and remove or adjust any handlers that also
call apiCall (pagination or rowsPerPage change handlers) to avoid
double-fetches—look for useEffect, apiCall, setApplicationList, and any
page/rowsPerPage change handlers to apply this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d381e20-126f-4f3c-86cd-edc46ca8a2d0
📒 Files selected for processing (5)
portals/admin/src/main/webapp/site/public/locales/en.jsonportals/admin/src/main/webapp/site/public/locales/fr.jsonportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeToJWTDialog.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsx
...als/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx
Show resolved
Hide resolved
...als/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsx (2)
159-173:⚠️ Potential issue | 🟡 MinorReuse the localized clear-search label for
aria-label.
aria-label='clear-search'still exposes an English token instead of the user-facing "Clear Search" string. Reusing the existing intl message here keeps the accessible name accurate and localizable.🤖 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/ApplicationSettings/ChangeAppOwner.jsx` around lines 159 - 173, The IconButton in ChangeAppOwner.jsx currently uses a hardcoded aria-label 'clear-search'; replace that with the localized string by calling intl.formatMessage with the same descriptor used in the Tooltip (the object with defaultMessage 'Clear Search' and id 'ApplicationSettings.ChangeAppOwner.clear.search') so the accessible name is localized; update the IconButton prop (aria-label) to use intl.formatMessage(...) and keep the onClick handler clearSearch unchanged.
206-223:⚠️ Potential issue | 🟠 MajorVerify this wrapper still matches
EditApplication’s prop contract.
triggerButtonTextandtitleare passed as already-formatted strings here. IfEditApplicationstill formats those props internally, the row action will fail at runtime; in that case these should be the intl descriptor objects instead.If
EditApplicationstill doesintl.formatMessage(title)or renders<FormattedMessage {...title} />, this wrapper needs to pass descriptors, not strings.#!/bin/bash file="portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/EditApplication.jsx" rg -n -C2 "propTypes|triggerButtonText|title|formatMessage|FormattedMessage" "$file"🤖 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/ApplicationSettings/ChangeAppOwner.jsx` around lines 206 - 223, The wrapper is passing triggerButtonText and title as pre-formatted strings to AppsTableContent/EditApplication but EditApplication may expect message descriptors (or to call intl.formatMessage/ render <FormattedMessage /> itself); check EditApplication (component name EditApplication) for usage of props.triggerButtonText and props.title — if it calls intl.formatMessage(title) or renders <FormattedMessage {...title} /> then change this wrapper to pass the original intl descriptor objects (not intl.formatMessage(...) results), otherwise leave as-is; update the props passed into AppsTableContent (the editComponentProps object) to match EditApplication’s prop contract so runtime formatting calls receive descriptors and not already-formatted strings.portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx (2)
108-116:⚠️ Potential issue | 🟠 MajorPass the target page/limit to these reloads.
After
setPage/setRowsPerPage, the captured state is still stale, so theseapiCall(...)calls can fetch the wrong slice. This shows up when changing page size or submitting/clearing search from a later page.Suggested fix
- function apiCall(pageNo, user = searchQuery, name = searchQuery) { + function apiCall(pageNo, user = searchQuery, name = searchQuery, limit = rowsPerPage) { setLoading(true); const restApi = new API(); return restApi .getApplicationList({ - limit: rowsPerPage, offset: pageNo * rowsPerPage, user, name, + limit, offset: pageNo * limit, user, name, }) @@ - const nextRowsPerPage = event.target.value; + const nextRowsPerPage = Number(event.target.value); const rowsPerPageRatio = rowsPerPage / nextRowsPerPage; const nextPage = Math.floor(page * rowsPerPageRatio); setPage(nextPage); setRowsPerPage(nextRowsPerPage); - apiCall(page).then((result) => { + apiCall(nextPage, searchQuery, searchQuery, nextRowsPerPage).then((result) => { setApplicationList(result); }); @@ - apiCall(page, '', '').then((result) => { + apiCall(0, '', '').then((result) => { setApplicationList(result); }); @@ - apiCall(page).then((result) => { + apiCall(0).then((result) => { setApplicationList(result); });Also applies to: 122-126, 150-155
🤖 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/ApplicationSettings/ListApplications.jsx` around lines 108 - 116, The apiCall invocations are using stale state after calling setPage/setRowsPerPage; update the calls to pass the computed page and limit directly (e.g., in handleChangeRowsPerPage use nextPage and nextRowsPerPage when calling apiCall) instead of relying on closure-captured page/rowsPerPage, and apply the same fix in the other handlers that reload data (the submit/clear search handlers that call setPage/setRowsPerPage then apiCall) so they pass the intended page and limit values into apiCall.
172-174:⚠️ Potential issue | 🔴 CriticalThe upgrade tab is still driven by the current page only.
applicationListis already paginated, so filtering it client-side here hides the JWT-upgrade flow whenever the current slice happens to contain only JWT apps. This needs a server-side legacy-app count/list; otherwise the new feature disappears even when later pages still contain opaque apps.Also applies to: 221-229
🤖 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/ApplicationSettings/ListApplications.jsx` around lines 172 - 174, The upgrade tab visibility/filtering currently uses the paginated client-side array applicationList (see upgradableApps in ListApplications.jsx), which hides legacy (non-JWT) apps when the current page slice contains only JWT apps; change the logic to rely on a server-side legacy-app indicator instead: add a new API call (e.g., getLegacyApplications or getLegacyAppCount) and store its result in component state (e.g., legacyApps or legacyCount), use that server-driven value to determine whether to show the upgrade tab and to populate the upgrade list, and remove the client-side filter usage of applicationList (the upgradableApps variable and similar logic around lines 221-229) so pagination does not hide legacy apps.
🤖 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/ApplicationSettings/UpgradeToJWTDialog.jsx`:
- Line 88: The Dialog currently always calls onClose={handleClose}, allowing
backdrop click or Escape to close it while submitting; update the Dialog's
onClose handler so it ignores close events when the component is in the
submitting state (e.g., onClose={(e, reason) => { if (!submitting && reason !==
'backdropClick' && reason !== 'escapeKeyDown') return; }} or more simply: if
submitting then return early, otherwise call handleClose). Ensure you reference
the Dialog component's onClose prop and the submitting state and keep
Cancel/Upgrade buttons disabled while submitting.
---
Duplicate comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsx`:
- Around line 159-173: The IconButton in ChangeAppOwner.jsx currently uses a
hardcoded aria-label 'clear-search'; replace that with the localized string by
calling intl.formatMessage with the same descriptor used in the Tooltip (the
object with defaultMessage 'Clear Search' and id
'ApplicationSettings.ChangeAppOwner.clear.search') so the accessible name is
localized; update the IconButton prop (aria-label) to use
intl.formatMessage(...) and keep the onClick handler clearSearch unchanged.
- Around line 206-223: The wrapper is passing triggerButtonText and title as
pre-formatted strings to AppsTableContent/EditApplication but EditApplication
may expect message descriptors (or to call intl.formatMessage/ render
<FormattedMessage /> itself); check EditApplication (component name
EditApplication) for usage of props.triggerButtonText and props.title — if it
calls intl.formatMessage(title) or renders <FormattedMessage {...title} /> then
change this wrapper to pass the original intl descriptor objects (not
intl.formatMessage(...) results), otherwise leave as-is; update the props passed
into AppsTableContent (the editComponentProps object) to match EditApplication’s
prop contract so runtime formatting calls receive descriptors and not
already-formatted strings.
In
`@portals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsx`:
- Around line 108-116: The apiCall invocations are using stale state after
calling setPage/setRowsPerPage; update the calls to pass the computed page and
limit directly (e.g., in handleChangeRowsPerPage use nextPage and
nextRowsPerPage when calling apiCall) instead of relying on closure-captured
page/rowsPerPage, and apply the same fix in the other handlers that reload data
(the submit/clear search handlers that call setPage/setRowsPerPage then apiCall)
so they pass the intended page and limit values into apiCall.
- Around line 172-174: The upgrade tab visibility/filtering currently uses the
paginated client-side array applicationList (see upgradableApps in
ListApplications.jsx), which hides legacy (non-JWT) apps when the current page
slice contains only JWT apps; change the logic to rely on a server-side
legacy-app indicator instead: add a new API call (e.g., getLegacyApplications or
getLegacyAppCount) and store its result in component state (e.g., legacyApps or
legacyCount), use that server-driven value to determine whether to show the
upgrade tab and to populate the upgrade list, and remove the client-side filter
usage of applicationList (the upgradableApps variable and similar logic around
lines 221-229) so pagination does not hide legacy apps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65b35848-6a97-48e4-9c9f-781d5eee5f46
📒 Files selected for processing (5)
portals/admin/src/main/webapp/site/public/locales/en.jsonportals/admin/src/main/webapp/site/public/locales/fr.jsonportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ChangeAppOwner.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/ListApplications.jsxportals/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeToJWTDialog.jsx
...s/admin/src/main/webapp/source/src/app/components/ApplicationSettings/UpgradeToJWTDialog.jsx
Outdated
Show resolved
Hide resolved
Member
Author
|
Note: The test case |
tharikaGitHub
force-pushed
the
new-main-after-460
branch
from
42e03fe to
2e1707d
Compare
Piumal1999
reviewed
Mar 17, 2026
| title={title} | ||
| help={help} | ||
| pageDescription={pageDescription} | ||
| warning={value === 1 ? warning : null} |
Contributor
There was a problem hiding this comment.
It looks like when the user switches tabs, the warning message appears above the tabs and causes both the tabs and the table to shift. Wouldn’t it be better to always display the warning banner in this page if there's a warning?
cc: @Induwara04
|
Piumal1999
approved these changes
Mar 18, 2026
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
4 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.
This PR is part of an improvement done with [1].
Some screenshots of the feature are shown below. The tabbed structure is visible only when Opaque Applications are present.
[1] wso2/api-manager#4670
Summary by CodeRabbit
New Features
Documentation
Chores