Conversation
95891e8 to
b893056
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds a session-timeout status model and API, prevents timeout polling from extending authentication cookies, updates the session-timeout view, and replaces the previous status model and service method. ChangesSession timeout status and service
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This replaces Razor timeout polling with a non-renewing session-status API, updates warning behavior, and preserves session refresh status responses. No concrete current-head merge-blocking risk is identified. Sequence Diagram(s)sequenceDiagram
participant SessionTimeoutView
participant SessionTimeoutController
participant SessionTimeoutService
participant VIPERContext
SessionTimeoutView->>SessionTimeoutController: GET /api/sessionTimeout
SessionTimeoutController->>SessionTimeoutService: GetStatus(VIPERContext)
SessionTimeoutService->>VIPERContext: Read session timeout record
VIPERContext-->>SessionTimeoutService: Session timeout data
SessionTimeoutService-->>SessionTimeoutController: SessionTimeoutStatus
SessionTimeoutController-->>SessionTimeoutView: Return status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 9 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@web/Controllers/SessionTimeoutController.cs`:
- Around line 66-70: Replace the filtered generic catch in the session-timeout
handling with separate catch blocks for SqlException and
InvalidOperationException. In both blocks, preserve the existing Logger.Error
call and Status fallback response.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a02c865c-16f7-4431-9a5e-4f20891d1ebd
📒 Files selected for processing (6)
test/Controllers/SessionTimeoutControllerTests.csweb/Classes/SessionTimeoutStatus.csweb/Classes/Utilities/SessionTimeoutService.csweb/Controllers/SessionTimeoutController.csweb/Views/Shared/Components/SessionTimeout/Default.cshtmlweb/Views/Shared/Components/SessionTimeout/SessionTimeout.cs
b893056 to
d056d7b
Compare
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## fix/session-refresh-pathbase #304 +/- ##
================================================================
+ Coverage 45.04% 45.07% +0.02%
================================================================
Files 941 942 +1
Lines 49176 49202 +26
Branches 6594 6595 +1
================================================================
+ Hits 22152 22177 +25
+ Misses 26080 26079 -1
- Partials 944 946 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
d056d7b to
6e97020
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Replaces the legacy ColdFusion session-timeout polling endpoint with a first-party ASP.NET Core API, and updates the session-timeout UI to use it while avoiding polling for anonymous visitors.
Changes:
- Add
/api/sessionTimeoutcontroller returning session timeout status derived from the auth cookie and DB record. - Update the session timeout component and client script to poll the new API and improve warning reset/formatting.
- Remove the old
SessionTimeoutCheckmodel and add a small test to prevent inheriting session-extending controller bases/filters.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/Views/Shared/Components/SessionTimeout/SessionTimeout.cs | Stops rendering the timeout UI for anonymous users to avoid false “expired” dialogs. |
| web/Views/Shared/Components/SessionTimeout/Default.cshtml | Switches polling to the new API, adds reset behavior, and centralizes time formatting. |
| web/Models/SessionTimeoutCheck.cs | Removes legacy model no longer used by the new polling contract. |
| web/Controllers/SessionTimeoutController.cs | Introduces the new read-only polling endpoint and ensures no-store caching. |
| web/Classes/Utilities/SessionTimeoutService.cs | Exposes timeout duration for reuse by the new controller. |
| web/Classes/SessionTimeoutStatus.cs | Adds the JSON contract returned to the session-timeout dialog. |
| test/Controllers/SessionTimeoutControllerTests.cs | Adds regression tests to ensure the polling controller doesn’t extend sessions via base types/filters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
633ad99 to
3ffa268
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/Controllers/SessionTimeoutControllerTests.cs`:
- Around line 14-31: Add execution tests for
SessionTimeoutController.GetSessionTimeout covering a stored expiry, an
authenticated user without a stored row, an anonymous user, and each fallback
exception path. Assert both SecondsUntilTimeout and the returned offset
timestamp contract, using the existing test setup and collaborators rather than
only reflection-based checks.
In `@web/Controllers/SessionTimeoutController.cs`:
- Line 52: Update SessionTimeoutController.GetSessionTimeout() to use a
read-only SessionTimeouts query with AsNoTracking(), filtering by LoginId and
Service, instead of calling SessionTimeoutService.GetSessionTimeout() and its
Find-based path.
In `@web/Views/Shared/Components/SessionTimeout/Default.cshtml`:
- Around line 47-51: Update the secondsUntilTimeout boundary in the session
timeout polling condition to use <= 300 instead of < 300, ensuring the warning
and 15-second pre-expiry polling path also run when the API returns exactly 300
seconds.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ad6355cf-63f9-459e-91c1-76b73e3f1971
📒 Files selected for processing (6)
.review-pr-ignored-304test/Controllers/SessionTimeoutControllerTests.csweb/Classes/Utilities/SessionTimeoutService.csweb/Controllers/SessionTimeoutController.csweb/Models/SessionTimeoutCheck.csweb/Views/Shared/Components/SessionTimeout/Default.cshtml
💤 Files with no reviewable changes (1)
- web/Models/SessionTimeoutCheck.cs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
web/Controllers/SessionTimeoutController.cs:56
secondsUntilTimeoutcan become negative if the storedSessionTimeoutDateTimeis already in the past (clock skew, delayed poll, stale DB row). Returning a negative value is surprising for the client contract and can lead to inconsistent UI behavior; clamp it to 0 on the server.
return Status(record.SessionTimeoutDateTime,
(int)(record.SessionTimeoutDateTime - DateTime.Now).TotalSeconds);
3ffa268 to
f959924
Compare
f959924 to
4c01b44
Compare
4c01b44 to
f8e26f4
Compare
0b52272 to
551ed0c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🟡 Changes recommended
The new controller’s remarks are still ambiguous/contradictory around “ApiController” vs [ApiController], which can mislead maintainers and risk regressions to the non-extending guarantee.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
551ed0c to
94632f3
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes align with the stated security/session requirements and are backed by targeted unit tests that pin the “must not extend session” invariants and core status-mapping branches.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
- Keep the endpoint read-only by deriving from ControllerBase, since ApiController and AreaController both write a fresh expiry on every action and would stop the session ever timing out - Read the user from the auth cookie, replacing the legacy endpoint's unauthenticated loginID query parameter - Tell an authenticated user with no row apart from a dead session, which legacy could not do and so reported as expired - Skip the poll for anonymous visitors, who would otherwise be told on public pages that a session they never had has expired - Stand the warning down when the session is extended elsewhere, and render midnight as 12 AM rather than 0 AM - Drop the ViewComponent's hardcoded localhost and double slash, now that the poll is same-origin - Delete the unused SessionTimeoutCheck model, which was added as this endpoint's DTO in 2023 and never wired up
94632f3 to
81c4a61
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The endpoint migration and “no session/cookie renewal” requirements are implemented and backed by targeted unit tests that pin the critical invariants.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
Replaces the Razor dialog's poll to legacy
seconds_until_timeout_v2.cfmwithGET /api/sessionTimeout. The CFM tookloginIDas an unauthenticated query parameter, so anyone could read anyone's expiry. The new endpoint takes no parameters and derives the user from the cookie.The endpoint must never extend the session.
ApiControllerandAreaControllerboth renew it on every action, soSessionTimeoutControllerinheritsControllerBase. A reflection test pins that. Because the poll is now same-origin it also passes through cookie authentication, soOnCheckSlidingExpirationdeclines to slide the auth cookie for this endpoint; otherwise an open tab could keep a login alive past its 12 hours. It is[AllowAnonymous](main's FallbackPolicy would otherwise redirect an expired cookie to CAS) so a caller with no session gets 0 seconds and a Log in button.SessionTimeoutService.GetStatusbuilds the response for both this endpoint andRefreshSession, so both carry the local UTC offset and neither leaks the entity. A user with a valid cookie but no row (pages served by a plainControllernever write one) gets a full window instead of "expired". DB errors return 600 seconds like the CFM did. The three branches are unit tested.Also: anonymous visitors no longer poll; the warning clears itself when the session is extended in another tab; midnight renders as 12 AM.
The SPA still uses the CFM. #306 migrates it.