VPR-158 fix(raps): confirm role template saves and keep nav highlighted - #308
Open
rlorenzo wants to merge 2 commits into
Open
VPR-158 fix(raps): confirm role template saves and keep nav highlighted#308rlorenzo wants to merge 2 commits into
rlorenzo wants to merge 2 commits into
Conversation
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #308 +/- ##
==========================================
- Coverage 41.88% 41.83% -0.05%
==========================================
Files 986 993 +7
Lines 49488 49735 +247
Branches 5843 5857 +14
==========================================
+ Hits 20726 20806 +80
- Misses 27852 28016 +164
- Partials 910 913 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughWalkthroughThe change centralizes left-navigation matching, adds child-page metadata, hardens RAPS role-loading and submission flows, and adds session-backed status notifications shown after redirects. ChangesNavigation highlighting
RAPS role interactions and notifications
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟡 Moderate · up to The PR improves role-template saving and navigation highlighting, but the current implementation can lose save confirmation when storage fails, render confirmation in an inaccessible form, and miss navigation highlighting for some deployed URL configurations. These bounded correctness and usability issues should be fixed before merging. Sequence Diagram(s)sequenceDiagram
participant RolePage as RAPS role page
participant Storage as sessionStorage
participant Templates as Templates Vue app
participant Renderer as status notification renderer
RolePage->>Storage: queueStatusNotification(message)
Templates->>Storage: showQueuedStatusNotification()
Storage-->>Templates: retrieve and remove queued message
Templates->>Renderer: display status notification
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/Classes/LeftNavHighlight.cs`:
- Around line 71-75: Update the absolute-URL guard in the URL handling logic of
LeftNavHighlight so root-relative values beginning with "/" are excluded from
the Uri.TryCreate check. Continue rejecting empty and genuinely external
absolute URLs while allowing root-relative URLs to reach the existing matching
branch.
In `@web/Views/Shared/Components/LeftNav/Default.cshtml`:
- Line 7: Update the LeftNavHighlight.FindActive call to include
Request.PathBase together with Request.Path when constructing the request path
input, so it matches the application-base-prefixed paths returned by Url.Content
and correctly applies leftNavActiveSecondary.
In `@web/wwwroot/js/qtable.js`:
- Around line 38-42: The queued-message flow should return and clear the message
from showQueuedStatusNotification in web/wwwroot/js/qtable.js:38-42 without
calling showStatusNotification or creating global DOM. In
web/Areas/RAPS/Views/Roles/Templates.cshtml:150-150, consume that return value
into Vue app state and render it with q-banner, including role="status" and the
page’s accessible classes.
- Around line 30-31: Update queueStatusNotification to catch and suppress
failures from putItemInStorage, keeping notification persistence best effort so
callers can continue to location.href even when storage is unavailable.
🪄 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: 7a39f6b9-211c-46d3-b430-877a0bfc47c2
📒 Files selected for processing (11)
eslint.config.mjstest/Classes/LeftNavHighlightTests.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Views/Roles/DelegateRoles.cshtmlweb/Areas/RAPS/Views/Roles/TemplateRoles.cshtmlweb/Areas/RAPS/Views/Roles/Templates.cshtmlweb/Classes/LeftNavHighlight.csweb/Classes/NavMenuItem.csweb/Views/Shared/Components/LeftNav/Default.cshtmlweb/wwwroot/js/qtable.jsweb/wwwroot/js/site.js
rlorenzo
force-pushed
the
fix/VPR-158-role-template-improvements
branch
from
August 12, 2026 22:47
bc07c19 to
4bad52c
Compare
- Saving roles on a template now returns to the listing with a success message instead of silently staying put - Role Templates stays highlighted on the edit and apply pages, which have no nav entry of their own, via NavMenuItem.ChildPageURLs - Move nav match logic out of the Razor view into LeftNavHighlight so it is unit testable, and cover the existing instance-link behaviour - Disable submit until both role reads succeed; a failed read left the selection empty and saving wiped the template's roles - Apply the same missing-confirmation fix to the delegate roles page
rlorenzo
force-pushed
the
fix/VPR-158-role-template-improvements
branch
from
August 13, 2026 01:16
4bad52c to
f81b664
Compare
- The guidelines named q-banner as the only way a Razor page shows a status message, so a reviewer reading them flags the established status toast as non-compliant, and a developer following them would build a banner where every existing call site uses a toast - Record when each applies and why Notify cannot be used in the Razor pages, in both the bot-facing instructions and DESIGN.md - Point the Copilot instructions at DESIGN.md, which they never mentioned, leaving Copilot to write UI against the curated subset with no idea the design system exists
rlorenzo
force-pushed
the
fix/VPR-158-role-template-improvements
branch
from
August 13, 2026 14:11
9c35962 to
6e855f3
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes both defects reported in VPR-158.
What was wrong
being awaited and the page stayed put, so there was no way to tell the
save had worked.
add-roles page.
LeftNav/Default.cshtmlmatched by exact path only, andRoleTemplateRoles/RoleTemplateApplyhave no nav entry of their own.Changes
NavMenuItem.ChildPageURLslets a nav item declare pages it covers, and"Role Templates" declares
RoleTemplateRolesandRoleTemplateApply.LeftNavHighlightso it isunit testable. 11 tests cover the new child-page case plus behaviour that
already existed with no coverage: instance-link promotion, secondary
highlight, query-string stripping, and
/2PathBase resolution.queued success message (
queueStatusNotification/showQueuedStatusNotification, because a toast cannot survive navigation).Also included, beyond the ticket's scope
These are in this PR already. Calling them out because a reviewer expecting
only the two reported defects would otherwise be surprised by them.
TemplateRoles. If either initial GET failed,selectedRolesstayed empty and submitting sent an empty list, wiping thetemplate's roles. With the new redirect that wipe would have been followed
by a success message, so the guard became necessary rather than optional.
Submit is now disabled until both reads succeed, mirroring the guard
already in
DelegateRoles.cshtml. This also removes a latent crash:.filter()was called on the result of a failed fetch.DelegateRoles.cshtmlhad the identical missing-confirmation bug andis fixed the same way (message only; that page has no listing to return
to).
eslint.config.mjsdeclares the two new JS globals. Without it thecshtml lint gate fails on
no-undef.Verification
RoleListunaffected, save redirects and the message renders withrole="status"/aria-live="polite", 0 console errors. The submit usedan unchanged selection so the PUT was a data no-op, confirmed afterwards
by the template still holding its 7 roles.
CA1502onRAPSController.Navshows up in lint output. It reads 35 bothwith and without this change, so it is pre-existing, not introduced here.