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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #302 +/- ##
==========================================
- Coverage 45.04% 45.04% -0.01%
==========================================
Files 941 941
Lines 49175 49176 +1
Branches 6594 6594
==========================================
Hits 22152 22152
- Misses 26079 26080 +1
Partials 944 944
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
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 (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe session timeout view rejects failed responses, reschedules polling after errors, uses ChangesSession Timeout Handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Session renewal now preserves the IIS PathBase and surfaces renewal failures without closing the timeout dialog. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ 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/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
0f34d9d to
ba86fee
Compare
ba86fee to
bc04fe0
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
New usage of AbortSignal.timeout() can throw on browsers that don’t support it, which would break session polling/refresh unless feature-detected or safely guarded.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
web/Views/Shared/Components/SessionTimeout/Default.cshtml:73
- Same compatibility concern here: calling
AbortSignal.timeout()directly can throw on browsers that don’t implement it, which would prevent the user from refreshing the session at all. Feature-detectAbortSignal.timeoutand fall back to a regularfetch()when unavailable.
fetch('@Url.Content("~/RefreshSession")', { signal: AbortSignal.timeout(10000) })
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
- 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
bc04fe0 to
f783416
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated code still formats midnight/noon as 0:xx in the session expiration time display, which is user-visible and should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
web/Views/Shared/Components/SessionTimeout/Default.cshtml:82
- Same 12-hour formatting issue here: at midnight/noon this will render
0:xxinstead of12:xx. Adjust the hour conversion tohours % 12 || 12.
this.sessionExpireTime = (d.getHours() > 12 ? d.getHours() - 12 : d.getHours()) + ":"
+ ("0" + d.getMinutes()).slice(-2)
+ (d.getHours() >= 12 ? " PM" : " AM")
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
On TEST and PROD, Refresh Session did nothing:
extendSessionfetched a bare/RefreshSession, which under the/2PathBase lands on legacy VIPER 1 and returns HTML. It now uses@Url.Content("~/RefreshSession"), matchingloginin the same file.Also in the same dialog:
try/catchcould not catch an async rejection, and a failed poll scheduled no follow-up, so checking stopped for the life of the page. It now reschedules.Not verifiable locally (no PathBase). Confirm on TEST that the rendered markup contains
/2/RefreshSessionand the button extends the session.