Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import { injectIntl } from 'react-intl';
import API from 'AppData/api';
import MCPServer from 'AppData/MCPServer';
import Subscription from 'AppData/Subscription';
import CONSTANTS from 'AppData/Constants';
import NoApi from 'AppComponents/Apis/Listing/NoApi';
import Loading from 'AppComponents/Base/Loading/Loading';
Expand Down Expand Up @@ -69,6 +70,8 @@
this.count = 100;
this.rowsPerPage = 10;
this.pageType = null;
// Incremented per load so a slower earlier request cannot overwrite a newer one.
this.apiLoadRequestId = 0;
}

/**
Expand All @@ -82,8 +85,8 @@
* @param {JSON} prevProps props from previous component instance
*/
componentDidUpdate(prevProps) {
const { subscriptions, searchText } = this.props;
if (subscriptions.length !== prevProps.subscriptions.length) {
const { refreshKey, searchText } = this.props;
if (refreshKey !== prevProps.refreshKey) {
this.getData();
} else if (searchText !== prevProps.searchText) {
this.page = 0;
Expand All @@ -95,15 +98,27 @@
getData = () => {
const { intl, entityType } = this.props;
const isMCPServersRoute = entityType === 'MCP';
const requestId = ++this.apiLoadRequestId;
this.xhrRequest()
Comment on lines +101 to 102

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Set loading before starting every reload.

When refreshKey changes after a successful subscription, this method keeps the previous data visible until all status lookups finish. The old SubscriptionPolicySelect remains actionable during that interval. A user can submit a second subscription request for the same API.

Set loading: true immediately after allocating requestId. The existing finally block will clear it only for the current request.

Proposed fix
 const requestId = ++this.apiLoadRequestId;
+this.setState({ loading: true });
 this.xhrRequest()
📝 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
const requestId = ++this.apiLoadRequestId;
this.xhrRequest()
const requestId = ++this.apiLoadRequestId;
this.setState({ loading: true });
this.xhrRequest()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@portals/devportal/src/main/webapp/source/src/app/components/Apis/Listing/APICardView.jsx`
around lines 101 - 102, Update the reload flow in the method containing
apiLoadRequestId so it sets loading to true immediately after allocating
requestId and before starting xhrRequest(). Preserve the existing finally
behavior that clears loading only for the current request.

.then((data) => {
if (requestId !== this.apiLoadRequestId) {
return undefined;
}
const { body } = data;
const { list, pagination } = body;
const { total } = pagination;
this.count = total;
this.setState({ data: this.updateUnsubscribedAPIsList(list) });
return this.resolveSubscribedIds(list).then((subscribedIds) => {
if (requestId !== this.apiLoadRequestId) {
return;
}
this.setState({ data: this.updateUnsubscribedAPIsList(list, subscribedIds) });
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
})
.catch((error) => {
if (requestId !== this.apiLoadRequestId) {
return;
}
const { response } = error;
const { setTenantDomain } = this.props;
if (response && response.body.code === 901300) {
Expand All @@ -120,47 +135,71 @@
}
})
.finally(() => {
this.setState({ loading: false });
if (requestId === this.apiLoadRequestId) {
this.setState({ loading: false });
}
});
};

/**
* Resolve which entities in the given page are already subscribed by this application.
*
* Get List of the Ids of all APIs that have been already subscribed
*
* @returns {*} Ids of respective APIs
* @param {Array} list a page of APIs or MCP Servers
* @returns {Promise<Set<string>>} ids of the entities in this page that are already subscribed
* @memberof APICardView
*/
getIdsOfSubscribedEntities() {
const { subscriptions } = this.props;

// Get arrays of the API Ids and remove all null/empty references by executing 'fliter(Boolean)'
const subscribedAPIIds = subscriptions.map((sub) => sub.apiId).filter(Boolean);

return subscribedAPIIds;
}
resolveSubscribedIds = (list) => {
const { applicationId } = this.props;
const subscribedIds = new Set();
if (!applicationId || !list || list.length === 0) {
return Promise.resolve(subscribedIds);
}
const client = new Subscription();
return Promise.all(list.map((entity) => client.getSubscriptions(entity.id, applicationId, 1, 0, 'ALL')
.then((response) => {
const subList = (response && response.body && response.body.list) || [];

Check warning on line 160 in portals/devportal/src/main/webapp/source/src/app/components/Apis/Listing/APICardView.jsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Prefer using an optional chain expression instead, as it's more concise and easier to read.

See more on https://sonarcloud.io/project/issues?id=wso2_apim-apps&issues=AZ_L-fvYpAlu9Xgqbu58&open=AZ_L-fvYpAlu9Xgqbu58&pullRequest=1410
if (subList.length > 0) {
subscribedIds.add(entity.id);
}
})))
.then(() => subscribedIds);
};

changePage = (page) => {
const { intl, entityType } = this.props;
const isMCPServersRoute = entityType === 'MCP';
this.page = page;
this.setState({ loading: true });
const requestId = ++this.apiLoadRequestId;
this.xhrRequest()
.then((data) => {
if (requestId !== this.apiLoadRequestId) {
return undefined;
}
const { body } = data;
const { list } = body;
this.setState({
data: this.updateUnsubscribedAPIsList(list),
return this.resolveSubscribedIds(list).then((subscribedIds) => {
if (requestId !== this.apiLoadRequestId) {
return;
}
this.setState({
data: this.updateUnsubscribedAPIsList(list, subscribedIds),
});
});
})
.catch(() => {
if (requestId !== this.apiLoadRequestId) {
return;
}
Alert.error(intl.formatMessage({
defaultMessage: isMCPServersRoute ? 'Error While Loading MCP Servers' : 'Error While Loading APIs',
id: isMCPServersRoute ? 'Apis.Listing.MCPServerCardView.error.loading' : 'Apis.Listing.ApiTableView.error.loading',
}));
})
.finally(() => {
this.setState({ loading: false });
if (requestId === this.apiLoadRequestId) {
this.setState({ loading: false });
}
});
};

Expand Down Expand Up @@ -194,15 +233,14 @@
* @returns {Array} filtered list of apis
* @memberof APICardView
*/
updateUnsubscribedAPIsList(list) {
const subscribedIds = this.getIdsOfSubscribedEntities();
updateUnsubscribedAPIsList(list, subscribedIds) {
const listLocal = list.filter((api) => !(api.throttlingPolicies.length === 1
&& api.throttlingPolicies[0].includes(CONSTANTS.DEFAULT_SUBSCRIPTIONLESS_PLAN)));
for (let i = 0; i < listLocal.length; i++) {
const policyList = listLocal[i].throttlingPolicies
.filter((policy) => !policy.includes(CONSTANTS.DEFAULT_SUBSCRIPTIONLESS_PLAN));
listLocal[i].throttlingPolicies = policyList;
if (!((!subscribedIds.includes(listLocal[i].id) && !listLocal[i].advertiseInfo.advertised)
if (!((!subscribedIds.has(listLocal[i].id) && !listLocal[i].advertiseInfo.advertised)
&& listLocal[i].isSubscriptionAvailable)) {
listLocal[i].throttlingPolicies = null;
}
Expand Down Expand Up @@ -389,7 +427,7 @@
intl: PropTypes.shape({
formatMessage: PropTypes.func,
}).isRequired,
subscriptions: PropTypes.arrayOf(PropTypes.shape({})),
refreshKey: PropTypes.number,
searchText: PropTypes.string,
handleSubscribe: PropTypes.func.isRequired,
applicationId: PropTypes.string.isRequired,
Expand All @@ -399,7 +437,7 @@
};

APICardView.defaultProps = {
subscriptions: [],
refreshKey: 0,
searchText: '',
apisNotFound: false,
setTenantDomain: () => {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,8 @@ class SubscriptionsBase extends React.Component {
pseudoMcpSubscriptions: false,
dialogSubscriptions: null,
dialogMcpSubscriptions: null,
dialogRefreshKey: 0,
dialogMcpRefreshKey: 0,
};
this.checkSubValidationDisabled = this.checkSubValidationDisabled.bind(this);
this.checkMcpSubValidationDisabled = this.checkMcpSubValidationDisabled.bind(this);
Expand Down Expand Up @@ -302,8 +304,6 @@ class SubscriptionsBase extends React.Component {
this.searchTextTmp = '';
this.mounted = false;
this.subscriptionsRequestId = 0;
this.dialogLoadRequestId = 0;
this.mcpDialogLoadRequestId = 0;

this.resetAccumulation();
}
Expand All @@ -325,8 +325,6 @@ class SubscriptionsBase extends React.Component {
componentWillUnmount() {
this.mounted = false;
this.subscriptionsRequestId += 1;
this.dialogLoadRequestId += 1;
this.mcpDialogLoadRequestId += 1;
}

handleOpenDialog() {
Expand Down Expand Up @@ -560,23 +558,21 @@ class SubscriptionsBase extends React.Component {
}

/**
* Update the full, unpaginated list backing a subscribe dialog (API or MCP).
* Refresh the contents of a subscribe dialog (API or MCP).
* @param {boolean} isMcp whether this is refreshing the MCP Server dialog
* @returns {Promise<void>}
* @memberof Subscriptions
*/
updateDialogSubscriptions(isMcp) {
const requestId = isMcp ? ++this.mcpDialogLoadRequestId : ++this.dialogLoadRequestId;
return this.ensureLoaded(Infinity, Infinity).then(() => {
const currentRequestId = isMcp ? this.mcpDialogLoadRequestId : this.dialogLoadRequestId;
const dialogOpen = isMcp ? this.state.openMcpDialog : this.state.openDialog;
if (!this.mounted || requestId !== currentRequestId || !dialogOpen) {
return;
}
const filtered = this.combinedSubscriptions.filter(isMcp ? isMcpSubscription : isApiSubscription);
this.setState(isMcp ? { dialogMcpSubscriptions: filtered } : { dialogSubscriptions: filtered });
this.refreshDerivedState();
});
// The empty array renders the card view instead of the spinner; the refresh key reloads it.
const dialogOpen = isMcp ? this.state.openMcpDialog : this.state.openDialog;
if (!this.mounted || !dialogOpen) {
return Promise.resolve();
}
this.setState((prevState) => (isMcp
? { dialogMcpSubscriptions: [], dialogMcpRefreshKey: prevState.dialogMcpRefreshKey + 1 }
: { dialogSubscriptions: [], dialogRefreshKey: prevState.dialogRefreshKey + 1 }));
return Promise.resolve();
}

/**
Expand Down Expand Up @@ -841,6 +837,8 @@ class SubscriptionsBase extends React.Component {
pseudoMcpSubscriptions,
dialogSubscriptions,
dialogMcpSubscriptions,
dialogRefreshKey,
dialogMcpRefreshKey,
} = this.state;

if (!isAuthorize) {
Expand Down Expand Up @@ -1054,8 +1052,8 @@ class SubscriptionsBase extends React.Component {
{dialogSubscriptions ? (
<APIList
apisNotFound={apisNotFound}
subscriptions={dialogSubscriptions}
applicationId={applicationId}
refreshKey={dialogRefreshKey}
handleSubscribe={(appInner, api, policy) => this.handleSubscribe(appInner, api, policy)}
searchText={searchText}
entityType='API'
Expand Down Expand Up @@ -1154,8 +1152,8 @@ class SubscriptionsBase extends React.Component {
{dialogMcpSubscriptions ? (
<APIList
apisNotFound={apisNotFound}
subscriptions={dialogMcpSubscriptions}
applicationId={applicationId}
refreshKey={dialogMcpRefreshKey}
handleSubscribe={(appInner, api, policy) => this.handleSubscribe(appInner, api, policy)}
searchText={searchText}
entityType='MCP'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,13 @@ export default class Subscription extends Resource {
* @param applicationId id of the application
* @param limit subscription count to return
* @param offset subscription list offset
* @param match ALL to require both apiId and applicationId to match, ANY (default) otherwise
* @returns {promise} With all subscription for given applicationId or apiId.
*/
getSubscriptions(apiId, applicationId, limit = 25, offset = 0) {
getSubscriptions(apiId, applicationId, limit = 25, offset = 0, match = undefined) {
var promise_get = this.client.then((client) => {
return client.apis["Subscriptions"].get_subscriptions(
{ apiId: apiId, applicationId: applicationId, limit, offset });
{ apiId: apiId, applicationId: applicationId, limit, offset, ...(match && { match }) });
}
);
return promise_get;
Expand Down
Loading