Add support for defining a maximum session timeout for a user session #9587
Conversation
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
Contributor
|
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 maximum session timeout support to admin session management: new API path constants, form fields and validation, UI checkbox and numeric input, PATCH/revert payload keys, i18n keys/translations, SCSS for disabled state and grid layout. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Admin UI
participant SessionPage as Session Management Page
participant Backend as Backend API / Config Store
AdminUI->>SessionPage: Open session management page
SessionPage->>Backend: GET /session-config
Backend-->>SessionPage: Return config (idle, rememberMe, enableMaximumSessionTimeout?, maximumSessionTimeout?)
SessionPage-->>AdminUI: Render form (idleTimeout, rememberMe, enable max timeout + number)
AdminUI->>SessionPage: Toggle enable / enter maximum timeout
SessionPage->>SessionPage: Validate (integer, >= 0 if enabled)
AdminUI->>SessionPage: Submit form
SessionPage->>Backend: PATCH include paths: /enableMaximumSessionTimeoutPeriod, /maximumSessionTimeoutPeriod (with values)
Backend-->>SessionPage: 200 OK
SessionPage-->>AdminUI: Show success / updated state
AdminUI->>SessionPage: Revert changes
SessionPage->>Backend: PATCH revert removing /enableMaximumSessionTimeoutPeriod and /maximumSessionTimeoutPeriod
Backend-->>SessionPage: 200 OK
SessionPage-->>AdminUI: Reverted state reflected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@features/admin.session-management.v1/pages/session-management.tsx`:
- Line 242: The payload currently forces "0" when values.maximumSessionTimeout
is undefined, which would immediately invalidate sessions; instead change the
submission so it does not send the string "0" — either (A) add form validation
(e.g., in the Formik schema/submit guard) to require maximumSessionTimeout when
the associated checkbox is enabled and prevent submit if empty, or (B) when
building the payload (replace the current "value":
values.maximumSessionTimeout?.toString() || "0"), send a nullable/omitted value
(e.g., undefined/null or simply omit the field) so the API receives no
zero-minute override; locate and update the code that constructs the request
using values.maximumSessionTimeout to implement one of these fixes.
- Around line 207-213: The validation for maximumSessionTimeout incorrectly
skips when values.maximumSessionTimeout is falsy (so 0 or "" bypasses checks)
and uses < 0 instead of <= 0; update the validate logic inside the block guarded
by values.enableMaximumSessionTimeout to explicitly check for
null/undefined/empty-string (e.g., treat "" or undefined as missing), then run
FormValidation.isInteger on the parsed/typed value and reject values <= 0,
assigning the message to error.maximumSessionTimeout; apply the same pattern to
idleSessionTimeout and rememberMePeriod so empty or zero inputs are properly
validated when their checkboxes are enabled.
features/admin.session-management.v1/pages/session-management.tsx
Outdated
Show resolved
Hide resolved
features/admin.session-management.v1/pages/session-management.tsx
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9587 +/- ##
=======================================
Coverage 55.88% 55.88%
=======================================
Files 42 42
Lines 1020 1020
Branches 247 254 +7
=======================================
Hits 570 570
Misses 416 416
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@features/admin.session-management.v1/pages/session-management.tsx`:
- Around line 440-468: The maximumSessionTimeout input uses
SessionManagementConstants.SESSION_MANAGEMENT_CONFIG_FIELD_MIN_LENGTH (0) for
its min, allowing a 0-minute timeout; update the Field.Input for
maximumSessionTimeout to use a minimum of 1 (e.g., set min to 1 instead of
SESSION_MANAGEMENT_CONFIG_FIELD_MIN_LENGTH) and ensure the form validation for
maximumSessionTimeout explicitly rejects 0 so submissions cannot set a
zero-minute timeout (adjust any related minLength/validation logic for
maximumSessionTimeout accordingly).
- Around line 238-242: The PATCH entry currently uses
values.maximumSessionTimeout?.toString(), which yields undefined when the
timeout is unset and can produce an invalid payload for the REPLACE operation;
update the code that builds the patches (the object containing
SessionManagementConstants.REPLACE_OPERATION and
SessionManagementConstants.MAXIMUM_SESSION_TIMEOUT_PATH) to only include this
patch when values.maximumSessionTimeout is defined/feature-enabled, or else
supply an explicit safe fallback (e.g., an empty string or a known API-accepted
sentinel) so the payload never contains an undefined value; locate the patch
construction that references values.maximumSessionTimeout, guard it with a
conditional, or replace the value expression with a deterministic fallback.
🧹 Nitpick comments (2)
features/admin.session-management.v1/pages/session-management.scss (1)
26-28: Fragile positional selector — prefer a class-based approach.
:nth-child(3)breaks silently if fields are reordered or new ones are inserted before the divider. Consider adding a utility class (e.g.,.full-width-row) to the<Divider>in the TSX and targeting that instead.- > :nth-child(3) { - grid-column: 1 / -1; - } + > .full-width-row { + grid-column: 1 / -1; + }features/admin.session-management.v1/pages/session-management.tsx (1)
192-204: Same falsy-guard pattern allows0to bypass validation for existing fields.
values?.idleSessionTimeoutandvalues?.rememberMePeriodare also falsy when0, skipping the integer/range check. While less critical than the maximum session timeout (since these fields are always visible and required), a0-minute idle timeout is likely undesirable. Consider applying the same fix pattern here for consistency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@features/admin.session-management.v1/pages/session-management.tsx`:
- Around line 415-439: The checkbox's listen currently calls
setSessionManagementConfig which mutates initialValues and, with
enableReinitialize={true}, causes form edits to be lost; instead create a
dedicated state (e.g., isMaxSessionEnabled) initialized from
sessionManagementConfig?.enableMaximumSessionTimeout, change the Field.Checkbox
listen to update setIsMaxSessionEnabled rather than setSessionManagementConfig,
and on save/submit merge isMaxSessionEnabled into the payload so
sessionManagementConfig remains unchanged while the user edits the form; also
add a useEffect to sync isMaxSessionEnabled when sessionManagementConfig is
refreshed from the server.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds UI + i18n support to configure an optional maximum session timeout for user sessions in the session management settings, aligning with the referenced product issue.
Changes:
- Introduces new form controls (toggle + numeric input) for maximum session timeout and includes them in update/revert PATCH operations.
- Extends i18n namespace typings and en-US strings for the new fields and validation message.
- Adjusts session-management form layout/styling to accommodate the new controls.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/i18n/src/translations/en-US/portals/session-management.ts | Adds en-US labels/hints/placeholders + validation message for the new maximum timeout fields. |
| modules/i18n/src/models/namespaces/session-management-ns.ts | Extends the Session Management i18n namespace type with new keys. |
| features/admin.session-management.v1/pages/session-management.tsx | Adds new toggle/input, validation, and PATCH updates/removals for maximum session timeout. |
| features/admin.session-management.v1/pages/session-management.scss | Updates grid layout to place the divider on a full-width row and adds disabled styling. |
| features/admin.session-management.v1/models/session-management.ts | Extends form/API response models for the new config fields. |
| features/admin.session-management.v1/constants/session-management.ts | Adds PATCH paths for the new config fields. |
| .changeset/fast-carpets-kneel.md | Declares package version bumps for the feature release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
features/admin.session-management.v1/pages/session-management.tsx
Outdated
Show resolved
Hide resolved
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.
Purpose
This pull request introduces support for defining a maximum session timeout for user sessions in the session management feature. It adds new configuration options, updates the relevant data models, enhances the UI to allow enabling and setting a maximum session timeout, and localizes the new fields and validation messages.
These changes collectively allow administrators to enforce a maximum session lifetime, improving security and compliance for user sessions.
Related Issue
UI Screenshot
Summary by CodeRabbit
New Features
Validation
Localization
Style
Chores