fix(session): use ~/ so Refresh Session keeps the /2 PathBase - #302
fix(session): use ~/ so Refresh Session keeps the /2 PathBase#302rlorenzo wants to merge 1 commit into
Conversation
Bundle ReportBundle size has no change ✅ |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #302 +/- ##
=======================================
Coverage 41.88% 41.88%
=======================================
Files 986 986
Lines 49488 49488
Branches 5843 5843
=======================================
Hits 20726 20726
Misses 27852 27852
Partials 910 910
Flags with carried forward coverage won't be shown. Click here to find out more. |
📝 WalkthroughWalkthroughThe session timeout view now rejects failed responses, reschedules polling after errors, uses ChangesSession Timeout Handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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}
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/Views/Shared/Components/SessionTimeout/Default.cshtml`:
- Around line 79-86: Replace the renewal failure `$q.notify` handling in the
session-timeout dialog with stored failure state, then conditionally render a
`q-banner` for that state using `class="bg-warning text-dark"` and
`role="alert"`, while preserving the existing failure message and retry/login
behavior.
🪄 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: 387615ea-156b-410e-9d38-cac711f62305
📒 Files selected for processing (1)
web/Views/Shared/Components/SessionTimeout/Default.cshtml
98e47d7 to
e05b9b3
Compare
There was a problem hiding this comment.
Pull request overview
Fixes the “Refresh Session does nothing on TEST/PROD under /2 PathBase” issue by ensuring the session-extension request targets VIPER 2 (not legacy VIPER 1 at /) and by making the polling / refresh flows handle non-OK and failed responses without silently stalling.
Changes:
- Update
extendSessionto call@Url.Content("~/RefreshSession")so the generated URL includes the IIS sub-application PathBase (e.g.,/2/RefreshSession). - Treat non-OK HTTP responses as failures (reject) rather than flowing a
Responseobject into the “success” handler. - Replace the ineffective synchronous
try/catcharound an async fetch chain with a real.catchthat reschedules session polling; add an in-dialog error banner when session extension fails.
e05b9b3 to
cd69eaa
Compare
8df5d47 to
0f34d9d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
web/Views/Shared/Components/SessionTimeout/Default.cshtml:75
- The try/catch around clearTimeout is unnecessary: clearTimeout does not throw for an unknown/invalid timeout id, so this swallows errors without benefit and adds noise. Removing it also aligns with the earlier fix that removed ineffective try/catch usage.
try {
clearTimeout(this.sessionTimeoutCheckEventId)
}
catch(e) { void e }
web/Views/Shared/Components/SessionTimeout/Default.cshtml:63
- Use an explicit seconds-to-milliseconds conversion instead of the magic number
300000so it’s immediately clear this is a 5-minute retry interval and consistent with thenextCheck * 1000usage above.
this.sessionTimeoutCheckEventId = window.setTimeout(this.checkSessionTimeout, 300000)
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 `@web/Views/Shared/Components/SessionTimeout/Default.cshtml`:
- Around line 55-64: Update the catch handler for checkSessionTimeout so a
failed poll retries promptly rather than always waiting 300000 milliseconds; cap
the retry using the last known session-expiry deadline or a short bounded
backoff that preserves near-expiry warning behavior, and add a test covering a
failed poll near expiry.
- Around line 48-55: Update the secondsUntilTimeout condition in the
session-timeout polling logic to use <= 300 so the warning is shown at exactly
five minutes remaining. Add a boundary test covering secondsUntilTimeout === 300
and verify that the warning state and nextCheck behavior are set correctly.
- Around line 44-45: Update the fetch calls in checkSessionTimeout and
extendSession to use a supported timeout via AbortController or
AbortSignal.timeout, aborting requests that exceed the timeout and routing those
failures through the existing catch handlers so polling restarts and renewal
reports sessionExtendFailed.
🪄 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: b30e247d-03e3-46c8-ac09-ccbe5dd740eb
📒 Files selected for processing (1)
web/Views/Shared/Components/SessionTimeout/Default.cshtml
- Reject non-OK responses instead of passing the Response to the success handler, which rendered "extended to Invalid Date" while the session was never extended - Report a failed extend in a role="alert" q-banner, and leave the dialog up offering both Refresh Session and Log in, so a user whose refresh keeps failing still has a way to re-authenticate - Replace the poll's dead try/catch, which cannot catch an async rejection, with a catch that reschedules the next check
0f34d9d to
ba86fee
Compare
The bug
On TEST and PROD, the session timeout warning appears ("Your session will expire at 10:30 AM. Click Refresh Session to continue working.") and clicking Refresh Session does nothing. The console shows:
Root cause
extendSessionfetched a bare root-relative"/RefreshSession". TEST and PROD run VIPER 2 as an IIS sub-application under a/2PathBase, with legacy VIPER 1 at/. So the call landed on legacy VIPER 1, which returned an HTML page.r.json()threw, and with no.catchon the chain it surfaced as an unhandled rejection, so the button looked dead.This is pre-existing on
main,Development, and every feature branch. It is not a regression. It does not reproduce locally because local development has no PathBase, so the bare path happens to resolve correctly.Changes
extendSessionnow uses@Url.Content("~/RefreshSession"), which includes the PathBase. This matches theloginmethod at the bottom of the same file.Responseobject straight through, rendering "Your session has been extended to Invalid Date" while the session was never extended.role="alert"q-bannerinside the dialog using the shared.error-surfacetreatment, and leaves the warning dialog on screen so the user can retry. Per DESIGN.md, Razor pages useq-bannerwith accessible classes rather than a toast, and an actionable error belongs next to the control that failed rather than in something transient. The banner is suppressed once the session actually expires, so the expired message and a stale failure message cannot show together.try { ... } catch (e) { void e }was dead code: a synchronouscatchcannot catch an async rejection. It is replaced with a real.catchthat reschedules the next check. Beyond making the failure non-silent, this fixes a second bug: a rejected poll previously scheduled no follow-up, so session checking stopped for the remaining life of the page.checkSessionTimeout's target is deliberately unchanged. It reads the sharedViper.dbo.SessionTimeoutrow through the legacy VIPER 1 CFM endpoint at the domain root, so its root-relative URL is correct.Verification
Run locally, one at a time:
npm run lint -- --fix web/Views/Shared/Components/SessionTimeout/Default.cshtmlnpm run verify:buildnpm run test:backendWhat is not verified locally. Local development has no PathBase. There is no
UsePathBasecall inProgram.cs; the/2prefix comes only from IIS sub-application hosting (see the comment atweb/Classes/Scheduler/HangfireExtensions.cs:111). Locally,"/RefreshSession"and@Url.Content("~/RefreshSession")therefore render to the identical string, so a local run cannot demonstrate the fix. The fix rests on the documented behaviour ofUrl.Content("~/..."), which prependsRequest.PathBase. Confirming the TEST symptom is resolved requires a TEST deploy: check that the rendered markup contains/2/RefreshSessionand that the button extends the session.