[CSM Portal] case detail: SLA tab - #1052
Conversation
Add a case-detail SLA tab that lists the case's SLA records, consuming the task-SLA search endpoint (POST /task-slas/search filtered by the case id). A CaseSlaTable (definition, target, colored stage chip, business time left/elapsed, elapsed %, start/stop times in the viewer's timezone; loading/empty/error + refresh) renders the mapped rows. Stage label is derived from the stage value and hasBreached from the elapsed percentage (the authoritative flag isn't exposed upstream yet). Removes the dead, never-populated 3-clock SLA scaffolding this replaces (list/dashboard SLA fields untouched).
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR replaces the CSM case SLA clock flow with task-SLA types, mapping, fetching, and table rendering. It also enables the SLA tab and removes the old SLA clock chip, banner, and timeline widgets. ChangesSLA Model and UI Replacement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CaseSlaTable
participant useGetCsmCaseSlas
participant API
CaseSlaTable->>useGetCsmCaseSlas: call with caseId
useGetCsmCaseSlas->>API: POST /slas/search with taskIds and pagination
API-->>useGetCsmCaseSlas: res.slas
useGetCsmCaseSlas-->>CaseSlaTable: { caseId, count, slas }
CaseSlaTable->>CaseSlaTable: render rows and status
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 resume |
✅ Action performedReviews resumed. |
The task-SLA search response now returns businessElapsedPercentage as a number (was a string). Type it as number|null, use it directly in the mapping, and derive hasBreached from the numeric value (no more parse). Fixtures updated.
Match the renamed entity/BFF route (was /task-slas/search).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/csm-portal/webapp/src/features/csm-cases/utils/caseSlaMapping.ts (1)
46-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWiden
SlaStageto admit unknown backend values.normalizeStage()can still return an unmapped string, soCaseSla.stageshould reflect that instead of relying on an unsafe cast.CaseSlaTable.tsxalready falls back to the raw label and a default chip color, but widening the type keeps future exhaustive logic honest.🤖 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-cases/utils/caseSlaMapping.ts` around lines 46 - 63, `normalizeStage()` can return backend values that are not part of the current `SlaStage` union, so `CaseSla.stage` is typed too narrowly and depends on an unsafe cast. Widen `SlaStage` in the `caseSlaMapping` types to allow unknown string values while preserving the existing known-stage mapping logic in `normalizeStage()`. Update any related `CaseSla` typing and keep `CaseSlaTable.tsx` using its raw-label/default-chip fallback so future exhaustive handling stays type-safe.apps/csm-portal/webapp/src/features/csm-cases/components/CaseSlaTable.tsx (2)
88-89: 🎯 Functional Correctness | 🔵 TrivialTotal count vs. rendered rows can diverge.
useGetCsmCaseSlasfetches a single page (TASK_SLA_PAGE_LIMIT, offset 0) but returns the server's fulltotalascount. If a case has more SLA records than the page limit, the "{count} total" chip would overstate what's actually rendered, with no pagination affordance to see the rest. Likely a rare edge case given typical SLA record counts, but worth a quick sanity check onTASK_SLA_PAGE_LIMITsizing.Also applies to: 199-199
🤖 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-cases/components/CaseSlaTable.tsx` around lines 88 - 89, The `count` derived in `useGetCsmCaseSlas` can overstate the number of rendered rows because the query only fetches one page (`TASK_SLA_PAGE_LIMIT` with offset 0) while using the server’s total. Update `CaseSlaTable` so the displayed total matches the actual loaded `slas` length unless full pagination is added, and apply the same fix wherever the count is shown in the related SLA summary/chip rendering.
34-35: 📐 Maintainability & Code Quality | 🔵 TrivialCross-feature import for
formatRelativeTime.Importing a formatting helper from
@features/csm-dashboard/utils/abtDashboardinto thecsm-casesfeature couples the two feature modules. Consider movingformatRelativeTimeto a shared@utilslocation alongsideformatAbsoluteForUser.🤖 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-cases/components/CaseSlaTable.tsx` around lines 34 - 35, The `CaseSlaTable` import of `formatRelativeTime` is coupling `csm-cases` to `csm-dashboard`. Move `formatRelativeTime` out of `@features/csm-dashboard/utils/abtDashboard` into a shared `@utils` module, ideally alongside `formatAbsoluteForUser`, then update `CaseSlaTable` to import both helpers from the shared location. Keep the feature module free of cross-feature utility dependencies.apps/csm-portal/webapp/src/features/csm-cases/pages/CsmCaseDetailPage.tsx (1)
1210-1211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
activeprop is alwaystruehere — the lazy-gate does nothing.
CaseSlaTableonly ever mounts whenactiveTab === "sla", soactive={activeTab === "sla"}always evaluates totrueat this call site. The laziness is already achieved by the conditional mount; theenabled/activeplumbing inCaseSlaTable/useGetCsmCaseSlas(designed to keep the component mounted and toggle fetching) is dead weight here, and it makes theactive={false}scenario tested inCaseSlaTable.test.tsxunreachable in production.Either drop the
activeprop and calluseGetCsmCaseSlas(caseId)unconditionally insideCaseSlaTable(since mount-gating already provides the laziness), or keep the table permanently mounted (hidden viasx={{ display: activeTab === "sla" ? "block" : "none" }}) so theactivetoggle is meaningful.♻️ Simplify by relying on mount-based laziness
- {activeTab === "sla" && - caseId && <CaseSlaTable caseId={caseId} active={activeTab === "sla"} />} + {activeTab === "sla" && caseId && <CaseSlaTable caseId={caseId} />}🤖 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-cases/pages/CsmCaseDetailPage.tsx` around lines 1210 - 1211, The `CaseSlaTable` call site in `CsmCaseDetailPage` makes the `active` prop redundant because the component only mounts when `activeTab === "sla"`, so the lazy-gate never changes behavior. Simplify by either removing the `active` prop and letting `CaseSlaTable`/`useGetCsmCaseSlas` fetch on mount, or by keeping `CaseSlaTable` mounted across tabs and hiding it with styling so `active` can actually toggle. Update the related `CaseSlaTable` and `useGetCsmCaseSlas` plumbing consistently with the chosen approach.
🤖 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.
Nitpick comments:
In `@apps/csm-portal/webapp/src/features/csm-cases/components/CaseSlaTable.tsx`:
- Around line 88-89: The `count` derived in `useGetCsmCaseSlas` can overstate
the number of rendered rows because the query only fetches one page
(`TASK_SLA_PAGE_LIMIT` with offset 0) while using the server’s total. Update
`CaseSlaTable` so the displayed total matches the actual loaded `slas` length
unless full pagination is added, and apply the same fix wherever the count is
shown in the related SLA summary/chip rendering.
- Around line 34-35: The `CaseSlaTable` import of `formatRelativeTime` is
coupling `csm-cases` to `csm-dashboard`. Move `formatRelativeTime` out of
`@features/csm-dashboard/utils/abtDashboard` into a shared `@utils` module,
ideally alongside `formatAbsoluteForUser`, then update `CaseSlaTable` to import
both helpers from the shared location. Keep the feature module free of
cross-feature utility dependencies.
In `@apps/csm-portal/webapp/src/features/csm-cases/pages/CsmCaseDetailPage.tsx`:
- Around line 1210-1211: The `CaseSlaTable` call site in `CsmCaseDetailPage`
makes the `active` prop redundant because the component only mounts when
`activeTab === "sla"`, so the lazy-gate never changes behavior. Simplify by
either removing the `active` prop and letting `CaseSlaTable`/`useGetCsmCaseSlas`
fetch on mount, or by keeping `CaseSlaTable` mounted across tabs and hiding it
with styling so `active` can actually toggle. Update the related `CaseSlaTable`
and `useGetCsmCaseSlas` plumbing consistently with the chosen approach.
In `@apps/csm-portal/webapp/src/features/csm-cases/utils/caseSlaMapping.ts`:
- Around line 46-63: `normalizeStage()` can return backend values that are not
part of the current `SlaStage` union, so `CaseSla.stage` is typed too narrowly
and depends on an unsafe cast. Widen `SlaStage` in the `caseSlaMapping` types to
allow unknown string values while preserving the existing known-stage mapping
logic in `normalizeStage()`. Update any related `CaseSla` typing and keep
`CaseSlaTable.tsx` using its raw-label/default-chip fallback so future
exhaustive handling stays type-safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7725660f-9718-44b5-82a4-fd6bb630c019
📒 Files selected for processing (12)
apps/csm-portal/webapp/src/constants/apiConstants.tsapps/csm-portal/webapp/src/features/csm-cases/api/useGetCsmCaseDetail.tsapps/csm-portal/webapp/src/features/csm-cases/api/useGetCsmCaseSlas.tsapps/csm-portal/webapp/src/features/csm-cases/components/CaseActionBar.test.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CaseDetailWidgets.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CaseSlaTable.test.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CaseSlaTable.tsxapps/csm-portal/webapp/src/features/csm-cases/pages/CsmCaseDetailPage.tsxapps/csm-portal/webapp/src/features/csm-cases/types/csmCases.tsapps/csm-portal/webapp/src/features/csm-cases/utils/caseSlaMapping.test.tsapps/csm-portal/webapp/src/features/csm-cases/utils/caseSlaMapping.tsapps/csm-portal/webapp/src/features/csm-dashboard/utils/abtDashboard.ts
💤 Files with no reviewable changes (3)
- apps/csm-portal/webapp/src/features/csm-cases/components/CaseActionBar.test.tsx
- apps/csm-portal/webapp/src/features/csm-cases/components/CaseDetailWidgets.tsx
- apps/csm-portal/webapp/src/features/csm-dashboard/utils/abtDashboard.ts
- Widen SlaStage to an open enum (known literals + string) and key the label/color maps off a KnownSlaStage subset, removing the unsafe cast for unmapped backend stages. - Use the rendered row count for the header (was the response total, which could diverge from the shown page). - Drop the always-true 'active' prop on CaseSlaTable; the conditional mount already gates the lazy fetch. Removed the now-unreachable inactive test.
|
Addressed 3 of the 4 nitpicks in f721cf6:
Skipping the 4th (cross-feature |
The task-SLA search response wraps results under "slas" (not "taskSlas"). Match the BE/entity contract (cs-tools wso2-open-operations#1054): TaskSlaSearchResponse.slas and read res.slas in the hook.
The task-SLA search endpoint rejects a page size over 50 ("Limit cannot
exceed 50"). Request limit 50 (was 100), which covers a case's SLA count.
Add a determinate LinearProgress in the Business time left column, filled by the elapsed % (clamped 0-100) and colored by breach/threshold (error when breached or >=100%, warning >=75%, else success), with the time-left label beneath it.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/csm-portal/webapp/src/features/csm-cases/components/CaseSlaTable.test.tsx (1)
56-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the header refresh button.
Coverage exercises loading, empty, populated, and error/retry states, but there's no assertion that clicking the header
IconButton(aria-label "Refresh SLAs") callsrefetch, only the error-state retry button is covered.✅ Suggested additional test
+ it("refresh button triggers refetch", () => { + const refetch = vi.fn(); + const list: CaseSlaList = { caseId: "case-1", count: 1, slas: [SLA_ROW] }; + mockResult({ data: list, refetch }); + render(<CaseSlaTable caseId="case-1" />); + fireEvent.click(screen.getByRole("button", { name: /refresh slas/i })); + expect(refetch).toHaveBeenCalled(); + });🤖 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-cases/components/CaseSlaTable.test.tsx` around lines 56 - 94, Add a test in CaseSlaTable.test.tsx to cover the header IconButton with aria-label "Refresh SLAs" and verify it calls the query refetch function. Reuse the existing mockResult setup and the CaseSlaTable render path, but assert the refresh button behavior independently from the error-state retry button so the component’s header action is covered.
🤖 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-cases/api/useGetCsmCaseSlas.ts`:
- Line 27: The SLA search hook is hiding backend results by always capping
`/slas/search` at 50 and returning `count: slas.length`, which can underreport
totals in the SLA chip and silently drop rows. Update `useGetCsmCaseSlas` to use
the backend `TaskSlaSearchResponse.total` as the source of truth, and expose a
separate rendered/loaded count from the fetched page size. If more than 50 SLAs
can exist, add pagination or a load-more flow so `count`, `total`, and the table
contents stay consistent.
---
Nitpick comments:
In
`@apps/csm-portal/webapp/src/features/csm-cases/components/CaseSlaTable.test.tsx`:
- Around line 56-94: Add a test in CaseSlaTable.test.tsx to cover the header
IconButton with aria-label "Refresh SLAs" and verify it calls the query refetch
function. Reuse the existing mockResult setup and the CaseSlaTable render path,
but assert the refresh button behavior independently from the error-state retry
button so the component’s header action is covered.
🪄 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: 179fe13f-380f-4ded-8c9f-c61572a0740a
📒 Files selected for processing (7)
apps/csm-portal/webapp/src/constants/apiConstants.tsapps/csm-portal/webapp/src/features/csm-cases/api/useGetCsmCaseSlas.tsapps/csm-portal/webapp/src/features/csm-cases/components/CaseSlaTable.test.tsxapps/csm-portal/webapp/src/features/csm-cases/components/CaseSlaTable.tsxapps/csm-portal/webapp/src/features/csm-cases/pages/CsmCaseDetailPage.tsxapps/csm-portal/webapp/src/features/csm-cases/types/csmCases.tsapps/csm-portal/webapp/src/features/csm-cases/utils/caseSlaMapping.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/csm-portal/webapp/src/constants/apiConstants.ts
- apps/csm-portal/webapp/src/features/csm-cases/utils/caseSlaMapping.ts
- apps/csm-portal/webapp/src/features/csm-cases/types/csmCases.ts
- apps/csm-portal/webapp/src/features/csm-cases/pages/CsmCaseDetailPage.tsx
The header count now uses the search response 'total' (not the fetched page size), so it doesn't underreport when a case has more than the 50-row page. When fewer rows are shown than the total, a 'Showing first N of M' caption makes the truncation explicit. Adds a header-refresh-button test.
2 participants
Purpose
Internal CS engineers viewing a case have no way to see the case's SLAs (the response/resolution/workaround clocks). This adds a case-detail SLA tab that lists them. Tracked on the internal delivery board (no public issue).
Goals
Show each SLA record for a case — definition, target, stage, business time left, business elapsed time, elapsed %, and start/stop times — so a CS engineer can gauge SLA status at a glance.
Approach
Webapp-only. The SLA tab renders a
CaseSlaTablefed by a lazy query:useGetCsmCaseSlas(caseId, {enabled})—POST /slas/searchwith{ filters: { taskIds: [caseId] }, pagination: { limit: 100, offset: 0 } }, loaded only when the SLA tab is open, with refresh.caseSlaMapping.ts(pure, unit-tested) maps eachTaskSlaView→ the table's row model:definition ← slaDefinition.name,target ← slaDefinition.target, normalizedstage(+ a humanizedstageLabel, since the endpoint sends no label),businessTimeLeftLabel/businessElapsedLabel, numericbusinessElapsedPercent,startTime,stopTime ← endTime.hasBreachedis derived asbusinessElapsedPercent >= 100— a UI proxy, since the authoritative breach flag isn't exposed upstream yet (documented inline).CaseSlaTable— MUI table: definition, target, colored stage chip (hasBreachedforces error), business time left/elapsed, elapsed %, start/stop times in the viewer's timezone; loading/empty/error states + refresh.UI screenshot omitted deliberately to avoid embedding real customer/user data.
User stories
As a CS engineer, I want to see a case's SLA status on the case page so I can gauge response/resolution progress at a glance.
Release note
Case detail now has an SLA tab listing the case's SLA records (definition, target, stage, business time left/elapsed, elapsed %, start/stop times).
Documentation
N/A — internal CS-engineer portal; no external product documentation impact.
Training
N/A. ## Certification
N/A. ## Marketing
N/A.
Automation tests
Security checks
Samples
N/A
Related PRs
Migrations (if applicable)
N/A — no schema or data migration.
Test environment
macOS; Node + pnpm; Chrome.
Learning
Reused the case-detail feature's existing query/table conventions; kept the SLA table UI unchanged and isolated the wire-format mapping in a pure, testable module.
Summary by CodeRabbit
New Features
Bug Fixes