Add session timeout for portals #1237
Conversation
DinithHerath
requested review from
Krishanx92,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In @portals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsx:
- Around line 67-69: The component assigns event handlers directly which
overwrites existing handlers; update SessionTimeout.jsx to use
document.addEventListener for the events currently assigned via
document.onclick, document.onmousemove, and document.onkeydown, registering the
resetIdleTimer function, and in the component cleanup (where handlers are
removed) use document.removeEventListener with the same resetIdleTimer reference
for "click", "mousemove", and "keydown" so listeners are added/removed safely
without clobbering other handlers.
In
@portals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsx:
- Around line 67-69: The component currently assigns
document.onclick/document.onmousemove/document.onkeydown directly which
overwrites other handlers; change these assignments to use
document.addEventListener('click', resetIdleTimer),
document.addEventListener('mousemove', resetIdleTimer), and
document.addEventListener('keydown', resetIdleTimer') where resetIdleTimer is
referenced, and ensure the effect cleanup removes the listeners with
document.removeEventListener for the same event types and the same
resetIdleTimer function so listeners are properly detached when SessionTimeout
unmounts.
In
@portals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsx:
- Around line 67-69: The code currently assigns event handlers directly
(document.onclick/document.onmousemove/document.onkeydown) which overwrites
existing handlers; replace those assignments by registering listeners with
document.addEventListener('click', resetIdleTimer),
document.addEventListener('mousemove', resetIdleTimer) and
document.addEventListener('keydown', resetIdleTimer) where handlers are set (use
the existing resetIdleTimer function reference), and update the cleanup to call
document.removeEventListener for the same event types and same resetIdleTimer
reference to avoid leaking or failing to remove listeners.
🧹 Nitpick comments (5)
portals/devportal/src/main/webapp/source/src/app/webWorkers/timer.worker.js (1)
27-31: Consider handling worker termination for cleaner resource management.The
setIntervalruns indefinitely without storing a reference. While the main thread can terminate the worker, consider adding anonmessagehandler to allow the main thread to signal a graceful stop, which could be useful for debugging or future enhancements.♻️ Optional: Add message handler for graceful stop
+let timerId = null; + +// Handle messages from main thread +// eslint-disable-next-line no-restricted-globals +self.onmessage = (event) => { + if (event.data === 'stop' && timerId) { + clearInterval(timerId); + timerId = null; + } +}; + // This is the timer function that will be executed in the background. -setInterval(() => { +timerId = setInterval(() => { // disable following rule because linter is unaware of the worker source // eslint-disable-next-line no-restricted-globals self.postMessage(''); }, TIMER_INTERVAL);portals/publisher/src/main/webapp/site/public/locales/en.json (1)
2476-2479: Standardize quoting + consider ICU pluralization for{time}; verify button wiring forcancel/ok.
- The message uses typographic quotes (
“Stay Logged In”) while other files often use straight quotes; consider standardizing to avoid inconsistent encoding/string comparisons.- If your i18n stack supports ICU MessageFormat, consider pluralization for
second(s)to avoid “1 seconds”.- Since the keys are
label.cancel/label.okbut the values areLogout/Stay Logged In, please double-check the dialog maps the right action to the right button.portals/devportal/src/main/webapp/site/public/locales/en.json (1)
612-615: Align SessionTimeout copy format across portals (quotes + pluralization).
- This message uses straight quotes while publisher uses typographic quotes; recommend picking one style and applying consistently across portals.
- Consider ICU pluralization for
{time}if supported to handle “1 second” correctly.portals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
135-135: UnnecessaryinjectIntlwrapper.The component uses only
FormattedMessagewhich doesn't require theintlprop. Since theintlprop is never consumed, theinjectIntlHOC adds unnecessary overhead.Proposed fix
-export default injectIntl(SessionTimeout); +export default SessionTimeout;portals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
24-129: Consider extracting shared SessionTimeout component.This component is nearly identical to the admin and devportal versions. The only differences are:
- Config import path/alias
confirmPrimaryprop (present here, absent in admin/devportal)Extracting to a shared library with configuration injection would reduce maintenance burden and ensure consistent behavior across portals.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 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/ProtectedApp.jsxportals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsxportals/admin/src/main/webapp/source/src/app/webWorkers/timer.worker.jsportals/devportal/src/main/webapp/site/public/locales/en.jsonportals/devportal/src/main/webapp/source/src/app/AppRouts.jsxportals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsxportals/devportal/src/main/webapp/source/src/app/webWorkers/timer.worker.jsportals/publisher/src/main/webapp/site/public/locales/en.jsonportals/publisher/src/main/webapp/source/src/app/ProtectedApp.jsxportals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsxportals/publisher/src/main/webapp/source/src/app/webWorkers/timer.worker.js
🧰 Additional context used
🧬 Code graph analysis (7)
portals/publisher/src/main/webapp/source/src/app/webWorkers/timer.worker.js (2)
portals/admin/src/main/webapp/source/src/app/webWorkers/timer.worker.js (1)
TIMER_INTERVAL(24-24)portals/devportal/src/main/webapp/source/src/app/webWorkers/timer.worker.js (1)
TIMER_INTERVAL(24-24)
portals/publisher/src/main/webapp/source/src/app/ProtectedApp.jsx (3)
portals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
SessionTimeout(24-134)portals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
SessionTimeout(24-133)portals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
SessionTimeout(24-127)
portals/devportal/src/main/webapp/source/src/app/AppRouts.jsx (3)
portals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
SessionTimeout(24-134)portals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
SessionTimeout(24-133)portals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
SessionTimeout(24-127)
portals/devportal/src/main/webapp/source/src/app/webWorkers/timer.worker.js (2)
portals/admin/src/main/webapp/source/src/app/webWorkers/timer.worker.js (1)
TIMER_INTERVAL(24-24)portals/publisher/src/main/webapp/source/src/app/webWorkers/timer.worker.js (1)
TIMER_INTERVAL(24-24)
portals/admin/src/main/webapp/source/src/app/ProtectedApp.jsx (3)
portals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
SessionTimeout(24-134)portals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
SessionTimeout(24-133)portals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
SessionTimeout(24-127)
portals/admin/src/main/webapp/source/src/app/webWorkers/timer.worker.js (2)
portals/devportal/src/main/webapp/source/src/app/webWorkers/timer.worker.js (1)
TIMER_INTERVAL(24-24)portals/publisher/src/main/webapp/source/src/app/webWorkers/timer.worker.js (1)
TIMER_INTERVAL(24-24)
portals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsx (3)
portals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsx (8)
SessionTimeout(24-134)openDialog(25-25)remainingTime(26-26)openDialogRef(27-27)idleTimeoutRef(30-30)idleWarningTimeoutRef(31-31)idleSecondsCounterRef(32-32)handleConfirmDialog(87-94)portals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsx (8)
SessionTimeout(24-133)openDialog(25-25)remainingTime(26-26)openDialogRef(27-27)idleTimeoutRef(30-30)idleWarningTimeoutRef(31-31)idleSecondsCounterRef(32-32)handleConfirmDialog(87-94)portals/publisher/src/main/webapp/source/src/app/components/Shared/ConfirmDialog.jsx (1)
ConfirmDialog(14-50)
🔇 Additional comments (14)
portals/admin/src/main/webapp/site/public/locales/en.json (1)
977-980: LGTM - Session timeout localization strings are well-structured.The new keys follow the existing naming conventions, and the message placeholder
{time}appropriately supports dynamic countdown display. The wording is clear and user-friendly.portals/devportal/src/main/webapp/source/src/app/webWorkers/timer.worker.js (1)
33-33: Empty export for bundler compatibility is fine.The
export default {}appears to be for ES module bundler compatibility. This is consistent with the parallel implementations in admin and publisher portals.portals/admin/src/main/webapp/site/public/locales/fr.json (1)
977-980: LGTM!French translations for the SessionTimeout dialog are correctly added and follow the established key naming convention. The
{time}placeholder in the message is properly preserved for dynamic value interpolation.portals/publisher/src/main/webapp/source/src/app/ProtectedApp.jsx (2)
47-47: LGTM!Import follows the established pattern and is correctly placed among other component imports.
216-216: LGTM!The
SessionTimeoutcomponent is appropriately placed inside theBasecomponent, within the themed context and error boundary. The component self-manages its activation viaConfigurations.sessionTimeout.enable.portals/admin/src/main/webapp/source/src/app/ProtectedApp.jsx (2)
32-32: LGTM!Import follows the established pattern and is consistent with the publisher portal implementation.
204-204: LGTM!The
SessionTimeoutcomponent is placed within theAppErrorBoundary, ensuring proper error handling. The component self-manages its activation state via configuration, so rendering it before the settings check is acceptable.portals/devportal/src/main/webapp/source/src/app/AppRouts.jsx (2)
29-29: LGTM!Import follows the established pattern and is consistent with other portal implementations.
75-75: LGTM!Good practice to conditionally render
SessionTimeoutonly for authenticated users. This ensures the session timeout logic is only active when there's an actual session to manage.portals/admin/src/main/webapp/source/src/app/webWorkers/timer.worker.js (1)
1-33: LGTM!The web worker implementation is correct and follows the established pattern used in the devportal and publisher portals. The ESLint disable comment is appropriate for the worker context where
selfis the global scope. The empty default export is a standard webpack convention for worker modules, and termination is properly handled by the consumingSessionTimeoutcomponent.portals/publisher/src/main/webapp/source/src/app/webWorkers/timer.worker.js (1)
19-33: LGTM - Simple and effective timer worker.The implementation correctly offloads the timer to a web worker to avoid blocking the main thread. The interval and message posting are straightforward.
Minor observation: The
export default {}(line 33) appears vestigial since the worker is instantiated vianew Worker(...)and communicates viapostMessage. It's harmless but could be removed for clarity if it's not needed for bundler compatibility.portals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
34-47: Core timeout logic is sound.The
handleTimeOutfunction correctly:
- Updates remaining time after warning threshold
- Opens dialog exactly at warning timeout
- Triggers logout exactly at idle timeout
The use of refs for mutable state accessed in closures is appropriate.
portals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
123-123: VerifyconfirmPrimaryprop inconsistency.The publisher version includes
confirmPrimarywhile admin and devportal do not. This affects button styling (contained vs text variant based on the ConfirmDialog contract). Confirm whether this difference is intentional.portals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
96-134: Implementation is correct.The dialog rendering, callback handling, and overall session timeout logic are implemented correctly. The component properly:
- Displays remaining countdown time
- Offers "Stay Logged In" and "Logout" options
- Resets idle timer or triggers logout based on user choice
portals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsx
Outdated
Show resolved
Hide resolved
portals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsx
Outdated
Show resolved
Hide resolved
portals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsx
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
portals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsx (4)
42-46: Consider using>=for the idle timeout check.If for any reason
idleSecondsCountskips past the exactidleTimeoutRef.currentvalue (e.g., due to timing variations), the logout won't trigger. Using>=ensures the logout always fires once the threshold is met or exceeded.Proposed fix
- if (idleSecondsCount === idleTimeoutRef.current) { + if (idleSecondsCount >= idleTimeoutRef.current) {
58-59: Add validation for configuration values.If
idleTimeoutoridleWarningTimeoutare undefined or non-numeric, the comparisons inhandleTimeOutwill behave unexpectedly. Consider adding default values or validation.Proposed fix
- idleTimeoutRef.current = Configurations.sessionTimeout.idleTimeout; - idleWarningTimeoutRef.current = Configurations.sessionTimeout.idleWarningTimeout; + idleTimeoutRef.current = Configurations.sessionTimeout.idleTimeout || 180; + idleWarningTimeoutRef.current = Configurations.sessionTimeout.idleWarningTimeout || 160;
67-69: Consider using passive event listeners for better scroll performance.The
mousemovelistener fires frequently. Adding{ passive: true }signals to the browser that the handler won't callpreventDefault(), which can improve scrolling performance.Proposed fix
- document.addEventListener('click', resetIdleTimer); - document.addEventListener('mousemove', resetIdleTimer); - document.addEventListener('keydown', resetIdleTimer); + const listenerOptions = { passive: true }; + document.addEventListener('click', resetIdleTimer, listenerOptions); + document.addEventListener('mousemove', resetIdleTimer, listenerOptions); + document.addEventListener('keydown', resetIdleTimer, listenerOptions);And in cleanup:
- document.removeEventListener('click', resetIdleTimer); - document.removeEventListener('mousemove', resetIdleTimer); - document.removeEventListener('keydown', resetIdleTimer); + document.removeEventListener('click', resetIdleTimer, listenerOptions); + document.removeEventListener('mousemove', resetIdleTimer, listenerOptions); + document.removeEventListener('keydown', resetIdleTimer, listenerOptions);
136-136: Remove unnecessaryinjectIntlwrapper.The component uses
FormattedMessagedeclaratively, which accesses the intl context directly. TheinjectIntlHOC is only needed when you need to access theintlobject imperatively (e.g.,intl.formatMessage()). Since theintlprop is never used, this wrapper adds unnecessary overhead.Proposed fix
Remove the import:
-import { injectIntl, FormattedMessage } from 'react-intl'; +import { FormattedMessage } from 'react-intl';Simplify the export:
-export default injectIntl(SessionTimeout); +export default SessionTimeout;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
portals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsxportals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsxportals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- portals/devportal/src/main/webapp/source/src/app/components/SessionTimeout.jsx
- portals/publisher/src/main/webapp/source/src/app/components/SessionTimeout.jsx
🔇 Additional comments (1)
portals/admin/src/main/webapp/source/src/app/components/SessionTimeout.jsx (1)
96-133: LGTM!The dialog implementation with react-intl is well-structured. The countdown display and localized messages are correctly implemented.
1 participant
This pull request introduces a session timeout feature for admin portal, which includes new components and updates to localization files. The most important changes include the addition of the
SessionTimeoutcomponent, updates to localization files to support session timeout messages, and the introduction of a web worker for handling the session timer. To use the session timeout please add the following configuration under the [1], [2] or [3].[1] https://github.com/wso2/apim-apps/blob/main/portals/admin/src/main/webapp/site/public/conf/settings.json
[2] https://github.com/wso2/apim-apps/blob/main/portals/publisher/src/main/webapp/site/public/conf/settings.json
[3] https://github.com/wso2/apim-apps/blob/main/portals/devportal/src/main/webapp/site/public/theme/settings.json
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.