Open
Conversation
DDH13
requested review from
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
WalkthroughThis change introduces a set of vhost validation and endpoint selection utilities to consistently handle WebSocket and HTTP URL selection across API portal components. Helper functions are created to validate protocol availability, select appropriate hosts, and manage UI state based on supported protocols. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Environments.jsx (1)
315-343:⚠️ Potential issue | 🟠 MajorWS copy button should use the WS helper.
The WS row shows
pickFirstEnabledWSUrl(...), but the copy action still usespickFirstEnabledUrl(...), which can copy the HTTP/default URL instead of WS/WSS.🛠️ Suggested fix
- navigator.clipboard.writeText(pickFirstEnabledUrl( - selectedEndpoint.URLs, - )).then(onCopy); + navigator.clipboard.writeText(pickFirstEnabledWSUrl( + selectedEndpoint.URLs, + )).then(onCopy);🤖 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/Environments.jsx` around lines 315 - 343, The copy handler for the WebSocket row is using pickFirstEnabledUrl instead of the WebSocket helper, so update the IconButton onClick handler to write the WebSocket URL by calling pickFirstEnabledWSUrl(selectedEndpoint.URLs) (keeping the existing promise then(onCopy) behavior); locate the IconButton onClick that currently calls navigator.clipboard.writeText(pickFirstEnabledUrl(...)) and replace the inner call to use pickFirstEnabledWSUrl so the copied value matches the displayed InputBase value.
🧹 Nitpick comments (3)
portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/AsyncApiConsole/AsyncApiUI.jsx (2)
78-105: Consider preferring secure endpoints by default.
getPreferredEndpointcurrently favors ws/http over wss/https. If both exist, selecting the secure endpoint first is generally safer.🛠️ Suggested tweak
- if (apiType === CONSTANTS.API_TYPES.WS) { - // prefer ws, but if ws does not exist fall back to wss - if (URLsObj.ws) return URLsObj.ws; - if (URLsObj.wss) return URLsObj.wss; + if (apiType === CONSTANTS.API_TYPES.WS) { + // prefer wss, but if wss does not exist fall back to ws + if (URLsObj.wss) return URLsObj.wss; + if (URLsObj.ws) return URLsObj.ws; return ''; } - // for non-WS APIs prefer http, but if http does not exist fall back to https - if (URLsObj.http) return URLsObj.http; - if (URLsObj.https) return URLsObj.https; + // for non-WS APIs prefer https, but if https does not exist fall back to http + if (URLsObj.https) return URLsObj.https; + if (URLsObj.http) return URLsObj.http; return '';🤖 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/AsyncApiConsole/AsyncApiUI.jsx` around lines 78 - 105, getPreferredEndpoint currently prefers non-secure endpoints (ws/http) over secure ones; change it to prefer secure endpoints first by checking URLsObj.wss before URLsObj.ws for apiType CONSTANTS.API_TYPES.WS and checking URLsObj.https before URLsObj.http for non-WS types (keep returning '' if none). Update any initialEndpoint / useEffect usage remains the same since they rely on getPreferredEndpoint (ensure setEndpoint gets the new preferred secure value).
245-259: Guard against null/undefined URL sets in disabled/empty logic.
Object.keys(URLs)andObject.values(URLs)will throw ifURLsis null/undefined. A small guard keeps this resilient if upstream ever passes a missing value.🛠️ Suggested tweak
- disabled={Object.keys(URLs).length === 0 || !Object.values(URLs).some(Boolean)} + disabled={!URLs || Object.keys(URLs).length === 0 || !Object.values(URLs).some(Boolean)} ... - {Object.keys(URLs).length === 0 || !Object.values(URLs).some(Boolean) ? ( + {!URLs || Object.keys(URLs).length === 0 || !Object.values(URLs).some(Boolean) ? (🤖 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/AsyncApiConsole/AsyncApiUI.jsx` around lines 245 - 259, The code uses Object.keys(URLs) and Object.values(URLs) which will throw if URLs is null/undefined; fix by normalizing URLs to a safe object before use (e.g., const safeURLs = URLs || {}) or by using fallback expressions in-place (Object.keys(URLs || {}) and Object.values(URLs || {})) and update the disabled prop, the conditional rendering check, and the map over Object.entries(URLs || {}) so the select, the onChange handler reference (handleServerChange) and MenuItem rendering tolerate missing URL sets.portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Environments.jsx (1)
121-143: Make URL preference explicit to avoid protocol mix‑ups.
pickFirstEnabledUrlcurrently depends on object value order, which can return ws/wss even when http/https are present. Consider explicitly preferring https/http and falling back to wss/ws only when needed (so WS‑only APIs still work).🛠️ Suggested refactor
const pickFirstEnabledUrl = (urls) => { if (!urls || typeof urls !== 'object') { return ''; } - // Get all truthy values and return the first one, or '' if none are found. - const firstUrl = Object.values(urls).find( - (val) => typeof val === 'string' && val.trim() !== '', - ); - return firstUrl || ''; + const preferred = ['https', 'http', 'wss', 'ws']; + for (const key of preferred) { + const val = urls[key]; + if (typeof val === 'string' && val.trim() !== '') { + return val; + } + } + return ''; };Based on learnings: For WSO2 APIM devportal, the backend populates selectedEndpoint.URLs based on API type (HTTP has http/https only, WS has ws/wss only, GraphQL has all four), so explicit preference avoids picking the wrong protocol for the HTTP row.
🤖 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/Environments.jsx` around lines 121 - 143, pickFirstEnabledUrl relies on object value order and may return ws/wss when http/https exist; change it to explicitly check keys in preferred order (e.g., ['https','http','wss','ws']) and return the first non-empty string, rather than using Object.values; also update pickFirstEnabledWSUrl to prefer secure websockets first (check ['wss','ws'] in that order) so WS endpoints return secure variants when available. Ensure both functions still handle missing/invalid urls and return '' as the fallback.
🤖 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/publisher/src/main/webapp/source/src/app/components/Apis/Details/Environments/Environments.jsx`:
- Around line 3161-3169: The WS dropdowns in Environments.jsx are mapping
row.vhosts directly, which can produce blank options when a vhost lacks ws/wss;
update the mapping to first filter row.vhosts using the same
hasValidWebSocketPorts predicate (or equivalent helper) when api.isWebSocket()
is true, then map the filtered list to MenuItem using getHostValue(vhost,
api.isWebSocket()); apply the same change to the other occurrences noted (around
the blocks that render vhost MenuItem lists at the lines referenced) so only
vhosts with valid WebSocket ports are shown.
In
`@portals/publisher/src/main/webapp/source/src/app/components/Shared/Environments/Vhosts.jsx`:
- Around line 44-57: getHostValue currently returns vhost.wsHost or
vhost.wssHost which can be null when both WS and WSS are disabled; update
getHostValue (referencing the function name getHostValue and helper checks
wsDisabled and wssDisabled) to detect the case where the resolved host is falsy
and return a safe fallback (explicit empty string or null) instead of passing
through a null value, ensuring callers get a predictable empty value rather than
a blank option.
- Around line 5-17: wsDisabled and wssDisabled currently only return true when
both host and port are null; change their logic to treat any missing/empty host
or port as disabled by using OR checks and also account for empty
strings/undefined. Update the functions wsDisabled(vhost) and wssDisabled(vhost)
to return true when vhost?.wsPort is null/undefined/empty OR vhost?.wsHost is
null/undefined/empty (and likewise for wssPort/wssHost) so a missing single
field marks the socket type disabled.
---
Outside diff comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Environments.jsx`:
- Around line 315-343: The copy handler for the WebSocket row is using
pickFirstEnabledUrl instead of the WebSocket helper, so update the IconButton
onClick handler to write the WebSocket URL by calling
pickFirstEnabledWSUrl(selectedEndpoint.URLs) (keeping the existing promise
then(onCopy) behavior); locate the IconButton onClick that currently calls
navigator.clipboard.writeText(pickFirstEnabledUrl(...)) and replace the inner
call to use pickFirstEnabledWSUrl so the copied value matches the displayed
InputBase value.
---
Nitpick comments:
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/AsyncApiConsole/AsyncApiUI.jsx`:
- Around line 78-105: getPreferredEndpoint currently prefers non-secure
endpoints (ws/http) over secure ones; change it to prefer secure endpoints first
by checking URLsObj.wss before URLsObj.ws for apiType CONSTANTS.API_TYPES.WS and
checking URLsObj.https before URLsObj.http for non-WS types (keep returning ''
if none). Update any initialEndpoint / useEffect usage remains the same since
they rely on getPreferredEndpoint (ensure setEndpoint gets the new preferred
secure value).
- Around line 245-259: The code uses Object.keys(URLs) and Object.values(URLs)
which will throw if URLs is null/undefined; fix by normalizing URLs to a safe
object before use (e.g., const safeURLs = URLs || {}) or by using fallback
expressions in-place (Object.keys(URLs || {}) and Object.values(URLs || {})) and
update the disabled prop, the conditional rendering check, and the map over
Object.entries(URLs || {}) so the select, the onChange handler reference
(handleServerChange) and MenuItem rendering tolerate missing URL sets.
In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Details/Environments.jsx`:
- Around line 121-143: pickFirstEnabledUrl relies on object value order and may
return ws/wss when http/https exist; change it to explicitly check keys in
preferred order (e.g., ['https','http','wss','ws']) and return the first
non-empty string, rather than using Object.values; also update
pickFirstEnabledWSUrl to prefer secure websockets first (check ['wss','ws'] in
that order) so WS endpoints return secure variants when available. Ensure both
functions still handle missing/invalid urls and return '' as the fallback.
Comment on lines
3161
to
+3169
| {row.vhosts?.map( | ||
| (vhost) => ( | ||
| <MenuItem value={api.isWebSocket() | ||
| ? vhost.wsHost : vhost.host}> | ||
| {api.isWebSocket() | ||
| ? vhost.wsHost : vhost.host} | ||
| <MenuItem | ||
| key={getHostValue(vhost, | ||
| api.isWebSocket())} | ||
| value={getHostValue( | ||
| vhost,api.isWebSocket())}> | ||
| {getHostValue( | ||
| vhost,api.isWebSocket())} |
There was a problem hiding this comment.
Filter WS vhost options here too to avoid blank entries.
These dropdowns still list all vhosts even in WebSocket mode; if a vhost lacks ws/wss, it can show up as an empty option despite the hasValidHosts gating. Apply the same hasValidWebSocketPorts filter used elsewhere.
🛠️ Suggested fix (apply similarly in the other WS dropdowns)
- {row.vhosts?.map((vhost) => (
+ {row.vhosts
+ .filter((vhost) => !api.isWebSocket() || hasValidWebSocketPorts(vhost))
+ .map((vhost) => (
<MenuItem
key={getHostValue(vhost, api.isWebSocket())}
value={getHostValue(vhost, api.isWebSocket())}
>
{getHostValue(vhost, api.isWebSocket())}
</MenuItem>
))}Also applies to: 3595-3602, 3819-3826
🤖 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/Environments/Environments.jsx`
around lines 3161 - 3169, The WS dropdowns in Environments.jsx are mapping
row.vhosts directly, which can produce blank options when a vhost lacks ws/wss;
update the mapping to first filter row.vhosts using the same
hasValidWebSocketPorts predicate (or equivalent helper) when api.isWebSocket()
is true, then map the filtered list to MenuItem using getHostValue(vhost,
api.isWebSocket()); apply the same change to the other occurrences noted (around
the blocks that render vhost MenuItem lists at the lines referenced) so only
vhosts with valid WebSocket ports are shown.
Comment on lines
+5
to
+17
| /** | ||
| * Returns true when websocket (ws) host/port is missing. | ||
| * @param {Object} vhost VHost object | ||
| * @returns {boolean} | ||
| */ | ||
| export const wsDisabled = (vhost) => vhost?.wsPort === null && vhost?.wsHost === null; | ||
|
|
||
| /** | ||
| * Returns true when secure websocket (wss) host/port is missing. | ||
| * @param {Object} vhost VHost object | ||
| * @returns {boolean} | ||
| */ | ||
| export const wssDisabled = (vhost) => vhost?.wssPort === null && vhost?.wssHost === null; |
There was a problem hiding this comment.
Treat partial/empty WS/WSS data as disabled.
Using && only flags disabled when both host and port are null, so a missing host or port is still considered valid and can yield blank/invalid URLs. Consider treating any missing/empty field as disabled.
🛠️ Suggested fix
-export const wsDisabled = (vhost) => vhost?.wsPort === null && vhost?.wsHost === null;
+export const wsDisabled = (vhost) =>
+ vhost?.wsPort == null || vhost?.wsHost == null || vhost.wsHost === '';
-export const wssDisabled = (vhost) => vhost?.wssPort === null && vhost?.wssHost === null;
+export const wssDisabled = (vhost) =>
+ vhost?.wssPort == null || vhost?.wssHost == null || vhost.wssHost === '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Returns true when websocket (ws) host/port is missing. | |
| * @param {Object} vhost VHost object | |
| * @returns {boolean} | |
| */ | |
| export const wsDisabled = (vhost) => vhost?.wsPort === null && vhost?.wsHost === null; | |
| /** | |
| * Returns true when secure websocket (wss) host/port is missing. | |
| * @param {Object} vhost VHost object | |
| * @returns {boolean} | |
| */ | |
| export const wssDisabled = (vhost) => vhost?.wssPort === null && vhost?.wssHost === null; | |
| /** | |
| * Returns true when websocket (ws) host/port is missing. | |
| * `@param` {Object} vhost VHost object | |
| * `@returns` {boolean} | |
| */ | |
| export const wsDisabled = (vhost) => | |
| vhost?.wsPort == null || vhost?.wsHost == null || vhost.wsHost === ''; | |
| /** | |
| * Returns true when secure websocket (wss) host/port is missing. | |
| * `@param` {Object} vhost VHost object | |
| * `@returns` {boolean} | |
| */ | |
| export const wssDisabled = (vhost) => | |
| vhost?.wssPort == null || vhost?.wssHost == null || vhost.wssHost === ''; |
🤖 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/Shared/Environments/Vhosts.jsx`
around lines 5 - 17, wsDisabled and wssDisabled currently only return true when
both host and port are null; change their logic to treat any missing/empty host
or port as disabled by using OR checks and also account for empty
strings/undefined. Update the functions wsDisabled(vhost) and wssDisabled(vhost)
to return true when vhost?.wsPort is null/undefined/empty OR vhost?.wsHost is
null/undefined/empty (and likewise for wssPort/wssHost) so a missing single
field marks the socket type disabled.
Comment on lines
+44
to
+57
| /** | ||
| * Get the host value based on the API type and available ports. | ||
| * @param {Object} vhost VHost object | ||
| * @param {boolean} isWebSocket Whether the API is a websocket API | ||
| * @returns {string} | ||
| */ | ||
| export const getHostValue = (vhost, isWebSocket) => { | ||
| if (!isWebSocket) { | ||
| return vhost.host; | ||
| } | ||
| if (wsDisabled(vhost) && !wssDisabled(vhost)) { | ||
| return vhost.wssHost; | ||
| } | ||
| return vhost.wsHost; |
There was a problem hiding this comment.
Return a safe fallback when both WS and WSS are unavailable.
If both protocols are disabled, vhost.wsHost can be null, which later becomes a blank option/value. Consider returning an explicit empty string (or null) instead.
🛠️ Suggested fix
export const getHostValue = (vhost, isWebSocket) => {
if (!isWebSocket) {
return vhost.host;
}
if (wsDisabled(vhost) && !wssDisabled(vhost)) {
return vhost.wssHost;
}
+ if (wsDisabled(vhost) && wssDisabled(vhost)) {
+ return '';
+ }
return vhost.wsHost;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Get the host value based on the API type and available ports. | |
| * @param {Object} vhost VHost object | |
| * @param {boolean} isWebSocket Whether the API is a websocket API | |
| * @returns {string} | |
| */ | |
| export const getHostValue = (vhost, isWebSocket) => { | |
| if (!isWebSocket) { | |
| return vhost.host; | |
| } | |
| if (wsDisabled(vhost) && !wssDisabled(vhost)) { | |
| return vhost.wssHost; | |
| } | |
| return vhost.wsHost; | |
| /** | |
| * Get the host value based on the API type and available ports. | |
| * `@param` {Object} vhost VHost object | |
| * `@param` {boolean} isWebSocket Whether the API is a websocket API | |
| * `@returns` {string} | |
| */ | |
| export const getHostValue = (vhost, isWebSocket) => { | |
| if (!isWebSocket) { | |
| return vhost.host; | |
| } | |
| if (wsDisabled(vhost) && !wssDisabled(vhost)) { | |
| return vhost.wssHost; | |
| } | |
| if (wsDisabled(vhost) && wssDisabled(vhost)) { | |
| return ''; | |
| } | |
| return vhost.wsHost; | |
| }; |
🤖 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/Shared/Environments/Vhosts.jsx`
around lines 44 - 57, getHostValue currently returns vhost.wsHost or
vhost.wssHost which can be null when both WS and WSS are disabled; update
getHostValue (referencing the function name getHostValue and helper checks
wsDisabled and wssDisabled) to detect the case where the resolved host is falsy
and return a safe fallback (explicit empty string or null) instead of passing
through a null value, ensuring callers get a predictable empty value rather than
a blank option.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Improvements