[CSM Portal] Improve Operations tab UX: approval signal, stage labels, linked-record navigation, incident state guard - #1203
Merged
Conversation
…, linked-record navigation Reduces noise and improves scannability in the change-request approvals card and the change-request/incident detail pages: - Sort each approval stage's approvers so a meaningful status (approved, requested, rejected, etc.) surfaces first, and collapse "not required" approvers behind a default-collapsed disclosure instead of listing every one inline. - Give repeated stage labels within one change request's approvals a "(N of M)" suffix so a duplicate reads as an explained repeat rather than an unexplained one. - Replace the "Unknown approver" fallback with "Unnamed approver" (with the raw id available on hover) so a missing display name doesn't read as a data-integrity alarm. - Make linked-record references clickable when the target's route is known safely: a change request's linked case, and an incident's parent incident / change request / problem. Fields whose target type can't be determined safely (e.g. an incident's "caused by") are left as plain text rather than guessing at a possibly-wrong link.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThe PR adds change-request approval decisions across the entity service, portal backend, and webapp; introduces navigable linked-record references; and restricts incident state editing to legal transitions. Tests cover API validation, approval actions, navigation, and terminal states. ChangesChange request approval decisions
Entity reference navigation
Incident state transition guardrails
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ApprovalsUI
participant PortalAPI
participant EntityService
participant ServiceNow
User->>ApprovalsUI: Click Approve or Reject
ApprovalsUI->>PortalAPI: POST approval decision
PortalAPI->>EntityService: Forward decision request
EntityService->>ServiceNow: Submit decision
ServiceNow-->>EntityService: Return approval state
EntityService-->>PortalAPI: Return decision response
PortalAPI-->>ApprovalsUI: Complete mutation and invalidate caches
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)apps/csm-portal/backend/openapi.yamlTraceback (most recent call last): 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 1
🧹 Nitpick comments (2)
apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.test.tsx (1)
126-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the tooltip-exposed ID.
This test verifies the "Unnamed approver" fallback but doesn't assert the tooltip actually exposes
approver.id(a PR-stated goal). Consider adding a check for the tooltip title/content.🤖 Prompt for 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. In `@apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.test.tsx` around lines 126 - 145, Extend the test for the unnamed approver in ChangeRequestApprovals to assert that the rendered tooltip exposes the approver ID "no-name-id" in its title or content, while preserving the existing "Unnamed approver" and absence of "Unknown approver" assertions.apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.tsx (1)
95-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink the
not requiredtoggle to its region witharia-controls. The button already setsaria-expanded; add a stable id to the collapsibleBoxand reference it from theButtonso the disclosure relationship is explicit for assistive tech.🤖 Prompt for 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. In `@apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.tsx` around lines 95 - 163, Update the not-required disclosure in ApprovalStage by adding a stable id to the collapsible Box containing notRequiredApprovers and setting the toggle Button’s aria-controls to that id, while preserving the existing aria-expanded behavior.
🤖 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
`@apps/csm-portal/webapp/src/features/csm-operations/pages/CsmChangeRequestDetailPage.test.tsx`:
- Around line 81-85: Update the test case “renders a dash for the linked case
when there is no case reference” to assert the dash specifically within the
“Linked case” field/container rather than using the page-wide getAllByText
lookup. Preserve the existing null-case setup and verify that the linked-case
value renders “—”.
---
Nitpick comments:
In
`@apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.test.tsx`:
- Around line 126-145: Extend the test for the unnamed approver in
ChangeRequestApprovals to assert that the rendered tooltip exposes the approver
ID "no-name-id" in its title or content, while preserving the existing "Unnamed
approver" and absence of "Unknown approver" assertions.
In
`@apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.tsx`:
- Around line 95-163: Update the not-required disclosure in ApprovalStage by
adding a stable id to the collapsible Box containing notRequiredApprovers and
setting the toggle Button’s aria-controls to that id, while preserving the
existing aria-expanded behavior.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: aab26bff-fece-410f-9853-afae6059cbda
📒 Files selected for processing (7)
apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.test.tsxapps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.tsxapps/csm-portal/webapp/src/features/csm-operations/components/EntityRefLink.tsxapps/csm-portal/webapp/src/features/csm-operations/pages/CsmChangeRequestDetailPage.test.tsxapps/csm-portal/webapp/src/features/csm-operations/pages/CsmChangeRequestDetailPage.tsxapps/csm-portal/webapp/src/features/csm-operations/pages/CsmIncidentDetailPage.test.tsxapps/csm-portal/webapp/src/features/csm-operations/pages/CsmIncidentDetailPage.tsx
The Incident state Select offered all 6 states unconditionally with no transition guard, letting a user jump NEW -> CLOSED directly or reopen a CANCELLED incident. ServiceNow enforces no state-order rule for incidents in this org (checked business rules/ACLs: only role-gating), so this is a net-new CSM-platform guardrail, not a port of existing SN behavior. Adds getLegalNextIncidentStates() (standard ITSM lifecycle graph) to utils/incidents.ts and restricts EditIncidentDialog's State select to the incident's current state plus its legal next states. CLOSED and CANCELLED are terminal (no outgoing transitions), so the Select is disabled for those with a helper note.
rksk
changed the title
The page renders multiple empty fields as a dash, so asserting on any dash anywhere on the page didn't actually guard the linked-case fallback specifically.
Adds POST /change-requests/{id}/approvals/decision, proxying to the new
Ballerina cs-entity-service resource so an authorized approver can decide
their own pending approval on a change request. ServiceNow's existing
business rule cascades the change request's own state automatically, so
this service does not compute or set state itself.
Adds POST /change-requests/{id}/approvals/decision, proxying to the new
entity-service endpoint so an authorized approver can decide their own
pending approval. Any user with change-request access may attempt a
decision; ServiceNow itself enforces that only the caller's own pending
approval can be acted on.
Adds the Approve/Reject action to the Operations-tab change request
detail page's Approvals section, closing the gap where CR state was a
read-only Chip with no transition control. The action only renders on
the current user's own pending ("REQUESTED") approval row, determined
from the existing GET /approvals response; any other row (someone
else's pending approval, or one already decided) stays read-only.
ServiceNow enforces that only the caller's own pending approval can be
acted on, and its existing business rule cascades the change request's
own state automatically, so this is purely a new mutation plus a
targeted UI affordance - nothing computes or sets CR state client-side.
On success, both the approvals and change-request detail queries are
invalidated so the page reflects the new state/stage immediately.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/csm-portal/backend/openapi.yaml (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDecision
decision/statevalues are lowercase while every sibling approval-status enum in this feature is UPPERCASE.ChangeRequestApproval.statususes[APPROVED, REJECTED, PENDING]andChangeRequestApprover.statususes "APPROVED"/"NOT_REQUIRED"/"REQUESTED", but the new decision endpoint's contract uses "approved"/"rejected" throughout. This is a single design-choice root cause replicated across the contract definition and its Go mirrors; aligning casing now (before any consumer hardens against it) avoids a confusing, inconsistent API surface for this same feature area.
apps/csm-portal/backend/openapi.yaml#L1981-2049: change thedecisionenum andstatedescription to useAPPROVED/REJECTED(matchingChangeRequestApproval.status), or explicitly document why this endpoint intentionally diverges.apps/csm-portal/backend/openapi.yaml#L6103-6122: updateChangeRequestApprovalDecisionPayload.decisionenum andChangeRequestApprovalDecisionResponse.stateto UPPERCASE values consistent withChangeRequestApprovalStatus.entity-service/internal/domain/entity.go#L2009-2022: update theDecision/Statedoc comments and expected values to match the corrected casing (and consider typingStateasdomain.ChangeRequestApprovalStatusrather than a barestring, for consistency with the existing approvals model).entity-service/internal/service/sn_change_request_service.go#L858-909: updatechangeRequestApprovalDecisionsmap keys and thesnChangeRequestApprovalDecisionPayload/snChangeRequestApprovalDecisionResponsefield values to the corrected casing, verifying against the actual Choreo contract before changing wire values.🤖 Prompt for 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. In `@apps/csm-portal/backend/openapi.yaml` at line 1, Align the decision endpoint’s status casing across the OpenAPI contract and Go mirrors: update the decision/state enums and descriptions in the schemas, the Decision/State documentation in the domain model, and the decision map and payload/response values in changeRequestApprovalDecisions and related types to use APPROVED/REJECTED consistently; type State as domain.ChangeRequestApprovalStatus if compatible with the existing model, and verify the Choreo wire contract before changing serialized values.
🧹 Nitpick comments (1)
apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.test.tsx (1)
200-211: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the Approve/Reject assertion to the current user's row, not just a count.
This test only checks that exactly one "Approve"/"Reject" exists on screen; it doesn't verify they render specifically on the "Current User" row rather than "Other Approver"'s row. A logic inversion bug that attaches decision actions to the wrong approver would still pass this test since the count stays at 1.
♻️ Suggested refactor
fireEvent.click(screen.getByText("Authorize")); - expect(screen.getByText("Current User")).toBeInTheDocument(); - expect(screen.getByText("Other Approver")).toBeInTheDocument(); - expect(screen.getAllByText("Approve")).toHaveLength(1); - expect(screen.getAllByText("Reject")).toHaveLength(1); + const myRow = screen.getByText("Current User").closest("li, tr, div") as HTMLElement; + const otherRow = screen.getByText("Other Approver").closest("li, tr, div") as HTMLElement; + expect(within(myRow).getByText("Approve")).toBeInTheDocument(); + expect(within(myRow).getByText("Reject")).toBeInTheDocument(); + expect(within(otherRow).queryByText("Approve")).not.toBeInTheDocument(); + expect(within(otherRow).queryByText("Reject")).not.toBeInTheDocument();Adjust the container selector to match the actual row/list-item element used by
ChangeRequestApprovals.tsx.🤖 Prompt for 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. In `@apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.test.tsx` around lines 200 - 211, Update the test case “shows Approve/Reject only on the current user's own pending approval row” to select the actual approval row/list-item container used by ChangeRequestApprovals, then assert Approve and Reject are present within the “Current User” row and absent from the “Other Approver” row rather than checking global counts.
🤖 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 `@apps/csm-portal/backend/internal/handler/change_requests.go`:
- Around line 202-207: Update the approval decision handling around
DecideChangeRequestApproval to strictly decode a single JSON object, reject
empty, null, malformed, or unknown-field payloads, and accept only approved or
rejected decisions before forwarding to the entity. In
apps/csm-portal/backend/internal/handler/change_requests_test.go lines 309-318,
add 400-response cases covering empty/object-null payloads, unknown fields, and
unsupported decisions.
In
`@apps/csm-portal/webapp/src/features/csm-operations/components/EditIncidentDialog.tsx`:
- Around line 355-360: Update renderSelect to render opts.helperText as
FormHelperText inside the surrounding FormControl, preserving the existing
disabled-select behavior and displaying the terminal-state explanation when
isStateTerminal is true.
---
Outside diff comments:
In `@apps/csm-portal/backend/openapi.yaml`:
- Line 1: Align the decision endpoint’s status casing across the OpenAPI
contract and Go mirrors: update the decision/state enums and descriptions in the
schemas, the Decision/State documentation in the domain model, and the decision
map and payload/response values in changeRequestApprovalDecisions and related
types to use APPROVED/REJECTED consistently; type State as
domain.ChangeRequestApprovalStatus if compatible with the existing model, and
verify the Choreo wire contract before changing serialized values.
---
Nitpick comments:
In
`@apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.test.tsx`:
- Around line 200-211: Update the test case “shows Approve/Reject only on the
current user's own pending approval row” to select the actual approval
row/list-item container used by ChangeRequestApprovals, then assert Approve and
Reject are present within the “Current User” row and absent from the “Other
Approver” row rather than checking global counts.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: fb483fb8-27d4-4045-a048-47ea2a3e6911
📒 Files selected for processing (21)
apps/csm-portal/backend/cmd/server/main.goapps/csm-portal/backend/internal/entity/entity.goapps/csm-portal/backend/internal/handler/change_requests.goapps/csm-portal/backend/internal/handler/change_requests_test.goapps/csm-portal/backend/internal/handler/helpers_test.goapps/csm-portal/backend/openapi.yamlapps/csm-portal/webapp/src/api/backend/types.tsapps/csm-portal/webapp/src/features/csm-operations/api/useDecideChangeRequestApproval.test.tsxapps/csm-portal/webapp/src/features/csm-operations/api/useDecideChangeRequestApproval.tsapps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.test.tsxapps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.tsxapps/csm-portal/webapp/src/features/csm-operations/components/EditIncidentDialog.test.tsxapps/csm-portal/webapp/src/features/csm-operations/components/EditIncidentDialog.tsxapps/csm-portal/webapp/src/features/csm-operations/pages/CsmChangeRequestDetailPage.test.tsxapps/csm-portal/webapp/src/features/csm-operations/utils/__tests__/incidents.test.tsapps/csm-portal/webapp/src/features/csm-operations/utils/incidents.tsentity-service/internal/domain/entity.goentity-service/internal/handler/change_request_handler.goentity-service/internal/server/routes.goentity-service/internal/service/interfaces.goentity-service/internal/service/sn_change_request_service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/csm-portal/webapp/src/features/csm-operations/pages/CsmChangeRequestDetailPage.test.tsx
- apps/csm-portal/webapp/src/features/csm-operations/components/ChangeRequestApprovals.tsx
… select renderSelect accepted a helperText option but never rendered it, so the disabled terminal-state explanation was silently dropped.
…load DecideChangeRequestApproval only checked json.Valid, so any well-formed JSON body was forwarded to the entity service regardless of shape. Decode into a typed payload, reject unknown fields and trailing data, and only accept decision values of "approved" or "rejected".
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
# Conflicts: # apps/csm-portal/webapp/src/features/csm-operations/pages/CsmChangeRequestDetailPage.test.tsx
cloby99
approved these changes
Jul 23, 2026
5 tasks
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.
Purpose
A UX review of the CSM portal's Operations tab (change requests / incidents) found several issues that make the approval-stage view, detail pages, and incident state control harder to use than necessary:
stateas a read-onlyChipwith no way to act on it, even when the signed-in user had a pending approval waiting on them.NewtoClosedor reopen aCancelledincident.The first item required a new upstream endpoint (Ballerina/Go entity-service/CSM backend); the rest are display-layer / client-side validation only.
Goals
Approach
POST /change-requests/{id}/approvals/decision(proxied through a corresponding new ServiceNow scripted resource and Ballerina function) lets an approver decide their OWN pending approval row. ServiceNow's own existing business rule cascades the change request's own state automatically once the approval is decided — nothing here computes or sets CR state client-side, it purely submits the decision. InChangeRequestApprovals.tsx, an approver row now renders Approve/Reject buttons only when it's the signed-in user's own row with statusREQUESTED; any other row (someone else's pending approval, or one already decided) stays read-only. On success, both the approvals and change-request detail queries are invalidated so the page reflects the new state/stage immediately.ChangeRequestApprovals.tsx: within a stage, non-NOT_REQUIREDapprovers render first;NOT_REQUIREDapprovers collapse behind a default-collapsed "N not required" disclosure. Stage labels that repeat (case-insensitively) get a(1 of 2)/(2 of 2)suffix, in encounter order; single-occurrence stages are untouched. The missing-name fallback is now "Unnamed approver", with the approver's id shown in a tooltip.EntityRefLinkcomponent: renders a{id, name}reference as a clickable chip to${routeBase}/${id}when the caller can determine the target's route safely, or as plain text otherwise.CsmChangeRequestDetailPage.tsx(linked case →/cases/:id) andCsmIncidentDetailPage.tsx(parent incident →/operations/incidents/:id, change request →/operations/change-requests/:id, problem →/operations/problems/:id). An incident's "caused by" field is deliberately left as plain text since its target record type can't be determined from the data alone.getLegalNextIncidentStates(current)helper defining the incident lifecycle's legal transitions (New → In Progress/Cancelled;In Progress → On Hold/Resolved/Cancelled;On Hold → In Progress/Cancelled;Resolved → Closed/In Progress(reopen);Closed/Cancelledterminal).EditIncidentDialog.tsx's stateSelectnow only offers the current state plus its legal next states, and disables the control entirely once a terminal state is reached. This is a net-new CSM platform decision (ServiceNow itself enforces no transition order on incidents today), not a port of existing ServiceNow behavior.User stories
As a CS engineer with a pending change-request approval, I can approve or reject it directly from the detail page instead of going into ServiceNow. As a CS engineer reviewing a change request's approvals, I can quickly see which approver still needs to act without scanning past a dozen "not required" rows. As a CS engineer on an incident or change-request detail page, I can click through to a linked case/incident/change request/problem instead of copying an id to search for it manually. As a CS engineer updating an incident's state, I only see the transitions that are actually legal from where it is now, and I can tell at a glance when an incident is closed and no longer editable.
Release note
Operations tab: change-request approvers can now approve/reject their own pending approval directly from the detail page; approval stages surface the approvers that matter and collapse the rest; linked-record references on change request/incident pages are now clickable where the target route is known; incident state changes are now restricted to valid transitions.
Documentation
N/A — internal CS-engineer portal UI/workflow improvement, no user-facing product docs affected.
Automation tests
ChangeRequestApprovals.test.tsx: NOT_REQUIRED collapsing/sorting, duplicate-stage suffixing, friendlier unnamed-approver fallback, Approve/Reject button visibility rules (own pending row only), click → mutate args, disabled-while-pending.useDecideChangeRequestApproval.test.tsx(new): mutation hook behavior viarenderHook.CsmChangeRequestDetailPage.test.tsx: linked case renders as a clickable reference; dash assertion scoped to the "Linked case" field specifically.CsmIncidentDetailPage.test.tsx: parent incident / change request / problem render as clickable references; "caused by" stays plain text.utils/__tests__/incidents.test.ts(new): exhaustive transition-graph coverage for all 6 incident states, including both terminal states.EditIncidentDialog.test.tsx(new): confirms only legal next states render as options from a non-trivial state, and that the control is disabled once terminal.pnpm testpasses aside from 3 pre-existing, unrelated failing files.change_requests_test.gocovers the new handler (auth, UUID validation, body-size/JSON validation, forwarding, upstream error mapping).go test ./...passes for the new service method.Security checks
go vet,eslint, andtsc(viapnpm build) ran clean instead.Related PRs
This webapp change depends on a corresponding Go entity-service change (bundled in this same PR/branch) and a Ballerina entity-service change tracked separately in the private repo.
Test environment
Verified with
pnpm build/pnpm test/pnpm lint(webapp),go build/go test/go vet(entity-service), andmake build/make test/make vet(backend).Summary by CodeRabbit