Fix UI freeze and timestamp format in Usage Report (Issue #4787)#1275
Fix UI freeze and timestamp format in Usage Report (Issue #4787)#1275Kavindu21123 wants to merge 1 commit intowso2:mainfrom
Conversation
Kavindu21123
requested review from
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
|
|
WalkthroughModified the Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 You can get early access to new features in CodeRabbit.Enable the |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
portals/admin/src/main/webapp/source/src/app/components/APISettings/UsageReport.jsx (1)
239-248: Add defensive validation withisValid()before callingvalueOf().The current null/empty check at line 239 is insufficient. MUI X DatePicker v7 can invoke
onChangewith an invalid Dayjs object (whereisValid() === false) during partial date input, causing lines 247–248 to send"NaN"timestamps. The proposed guard prevents this:Recommended fix
- if (!selectedStartDate || !selectedEndDate) { + if ( + !selectedStartDate + || !selectedEndDate + || !selectedStartDate.isValid?.() + || !selectedEndDate.isValid?.() + ) { return; }Alternatively, update the
onChangehandlers to leverage MUI'scontextparameter (more idiomatic for v7):onChange={(newValue, context) => { if (context.validationError == null && newValue?.isValid?.()) { setSelectedStartDate(newValue); } }}🤖 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/APISettings/UsageReport.jsx` around lines 239 - 248, The null/empty checks before calling valueOf() are insufficient because MUI X DatePicker v7 can pass invalid Dayjs objects; update the guard around selectedStartDate/selectedEndDate used before calling valueOf() (the block that constructs startTime/endTime for API.getTransactionCount) to also verify each has an isValid() === true (e.g., only call selectedStartDate.valueOf() and selectedEndDate.valueOf() when the Dayjs object exists and isValid()); alternatively, fix the DatePicker onChange handlers that set selectedStartDate/selectedEndDate to only set when the provided newValue has no validation errors (use the context param) and newValue?.isValid?.() so invalid dates never reach the API call.
🤖 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/APISettings/UsageReport.jsx`:
- Around line 253-256: In fetchTransactionData, remove the rethrow inside the
catch for the api.getTransactionCount() promise: currently the catch logs the
error then does "throw error", which causes an unhandled rejection because
fetchTransactionData()'s promise is not awaited/returned from the useEffect
call; update the catch in the api.getTransactionCount() chain to only log the
error (console.error) and not rethrow so the promise resolves and the finally
block still runs (i.e., edit the catch in the fetchTransactionData function that
wraps api.getTransactionCount()).
---
Nitpick comments:
In
`@portals/admin/src/main/webapp/source/src/app/components/APISettings/UsageReport.jsx`:
- Around line 239-248: The null/empty checks before calling valueOf() are
insufficient because MUI X DatePicker v7 can pass invalid Dayjs objects; update
the guard around selectedStartDate/selectedEndDate used before calling valueOf()
(the block that constructs startTime/endTime for API.getTransactionCount) to
also verify each has an isValid() === true (e.g., only call
selectedStartDate.valueOf() and selectedEndDate.valueOf() when the Dayjs object
exists and isValid()); alternatively, fix the DatePicker onChange handlers that
set selectedStartDate/selectedEndDate to only set when the provided newValue has
no validation errors (use the context param) and newValue?.isValid?.() so
invalid dates never reach the API call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2aebe8c9-350d-4786-bacd-ab5209c7b47a
📒 Files selected for processing (1)
portals/admin/src/main/webapp/source/src/app/components/APISettings/UsageReport.jsx
| .catch((error) => { | ||
| console.error('Error fetching transaction count:', error); | ||
| throw error; | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript Promises, what happens when an error is thrown inside catch() and the resulting Promise is not awaited or otherwise handled?
💡 Result:
If you throw inside a .catch() handler, the promise returned by that .catch() becomes rejected with the thrown error (equivalent to returning a rejected promise). [1]
If you then don’t await it or attach another rejection handler (e.g. another .catch()), it becomes an unhandled promise rejection:
- Browsers: the global
unhandledrejectionevent fires, and if it isn’t handled, you’ll typically see an error logged to the console. [2] - Node.js: by default (
--unhandled-rejections=throw, the default since Node 15), Node emitsunhandledRejection, and if you don’t handle that event, it raises it as an uncaught exception (which commonly terminates the process). [3]
Sources: [1] MDN Promise.prototype.catch() (www-igm.univ-mlv.fr), [2] MDN unhandledrejection event (developer.mozilla.org), [3] Node.js CLI --unhandled-rejections docs (nodejs.org)
Citations:
- 1: https://www-igm.univ-mlv.fr/~forax/MDN/developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch.html?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Window/unhandledrejection_event?utm_source=openai
- 3: https://nodejs.org/download/release/v22.17.0/docs/api/cli.html?utm_source=openai
🏁 Script executed:
cat -n portals/admin/src/main/webapp/source/src/app/components/APISettings/UsageReport.jsx | sed -n '240,270p'Repository: wso2/apim-apps
Length of output: 1166
Remove the rethrow in this catch block to prevent unhandled Promise rejections.
The promise chain from api.getTransactionCount() (line 246) is not awaited or returned by fetchTransactionData(). When useEffect calls fetchTransactionData() at line 264 without awaiting, the promise becomes unhandled. Rethrowing in the catch block at line 255 creates a rejected promise that has no handler, resulting in an unhandled rejection error at runtime.
Since the error is already logged, simply removing the throw allows the promise to resolve, preventing the unhandled rejection while preserving the error logging and the finally block execution.
Proposed fix
.catch((error) => {
console.error('Error fetching transaction count:', error);
- throw error;
})📝 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.
| .catch((error) => { | |
| console.error('Error fetching transaction count:', error); | |
| throw error; | |
| }) | |
| .catch((error) => { | |
| console.error('Error fetching transaction count:', error); | |
| }) |
🤖 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/APISettings/UsageReport.jsx`
around lines 253 - 256, In fetchTransactionData, remove the rethrow inside the
catch for the api.getTransactionCount() promise: currently the catch logs the
error then does "throw error", which causes an unhandled rejection because
fetchTransactionData()'s promise is not awaited/returned from the useEffect
call; update the catch in the api.getTransactionCount() chain to only log the
error (console.error) and not rethrow so the promise resolves and the finally
block still runs (i.e., edit the catch in the fetchTransactionData function that
wraps api.getTransactionCount()).
2 participants
Description
This PR fixes two bugs in the Admin Portal's Usage Report component that prevented the "View" button from functioning correctly, addressing Issue #4787.
Root Causes & Fixes:
fetchTransactionDatafunction was usingdayjs().unix(), which sends the timestamp to the backend in seconds. The backend requires milliseconds. Changed.unix()to.valueOf()to send the correct millisecond epoch.setLoading(true)was triggered, but the API call was skipped. This caused the UI to hang indefinitely because thefinallyblock was never reached. Added a guard clauseif (!selectedStartDate || !selectedEndDate) { return; }at the beginning of the function to prevent this.Related Issue
Fixes wso2/api-manager#4787
How to Test
[apim.transaction_counter]indeployment.toml.Summary by CodeRabbit