[Customer Portal][FE][Web] Refactor Architecture, Integrate DOMPurify, and Codebase Cleanup - #152
Conversation
Add dompurify (^3.3.1) to the customer-portal webapp dependencies and update pnpm-lock.yaml accordingly. The lockfile now contains the dompurify package entry, a @types/trusted-types entry, and related snapshot/optional dependency metadata.
Treat sloganListItems.icon as a React component and render it with its className prop instead of rendering a raw node. Update LoginSlogan to map items by assigning ItemIcon = item.icon and calling <ItemIcon className={item.iconClassName} />. Adjust ContactInfoCard tests to mock icons as functions (components) returning null instead of strings to match the new shape.
Treat icon props as components rather than pre-rendered elements. ContactRow was refactored to extract the icon component and render it with a size prop (also converted to a block-bodied component). Tests updated accordingly: ContactRow.test now passes the User component instead of <User />, and ProjectStatisticsCard tests provide icon as a function (() => null) instead of null. This standardizes icon usage and allows controlling icon props (e.g. size).
Instantiate stat icon components with a fixed size prop in ProjectStatisticsCard so icons render correctly (icon={<StatIcon size={24} />}) instead of passing the component reference. Simplify AllCasesList onClick handler to use optional chaining when invoking onCaseClick. Remove the obsolete AIInfoCard unit test file under case-creation-layout/header/__tests__.
Delete AIInfoCard.tsx, CaseCreationHeader.tsx, and the corresponding test (CaseCreationHeader.test.tsx) from apps/customer-portal/webapp/src/components/support/case-creation-layout/header. These files (the AI info card, the case creation header UI, and its unit test) were removed as part of a cleanup/refactor—update any imports or references to these components elsewhere in the codebase.
BackgroundTokenRefresh: add failure tracking, logging and a failure threshold that triggers sign-out to force re-authentication; use useRef and useLogger and include signOut in effect deps. support.ts: add getInitials and hasDisplayableContent helpers; improve formatSlaResponseTime to use Math.floor and singular/plural labels; tighten resolveColorFromTheme typing; enhance replaceInlineImageSources to handle single/double/unquoted src attributes, match attachments by id/sys_id, preserve quotes and sanitize output with DOMPurify; add normalization for ServiceNow timestamps and update formatCommentDate accordingly. support tests: update SLA formatting expectations and add comprehensive tests for initials, code wrapper stripping, displayable-content detection, inline image replacement, and comment date formatting.
Delete projectDetailsConstants.tsx (removes tab/contact/stat interfaces, activity helpers, enums and related UI icon imports). Simplify comments in supportConstants.ts (remove redundant parenthetical notes for MAX_ATTACHMENT_SIZE_BYTES and CASE_ATTACHMENTS_INITIAL_LIMIT). Adjust AppLayout.tsx to always use overflow: "auto" (remove special-case hidden overflow for case details pages) to ensure consistent scrolling behavior.
Rename loginScreenConstants.tsx to .ts and refactor the slogans list to use typed icon components instead of JSX: introduce SloganListItem (icon as ElementType, optional iconClassName) and update entries to pass icon components + class names. Add new projectDetailsConstants.ts defining types and constants for project details (tabs, contacts, stats, activity mapping/getRecentActivityItems, status/enum-like constants and related types), with imports for icons, colors, ProjectStatsResponse, and getSystemHealthColor.
Delete the BasicInformationSection component and its unit tests, and remove an associated CaseDetailsSection test. Files removed: - apps/customer-portal/webapp/src/components/support/case-creation-layout/sections/basic-information-section/BasicInformationSection.tsx - apps/customer-portal/webapp/src/components/support/case-creation-layout/sections/basic-information-section/__tests__/BasicInformationSection.test.tsx - apps/customer-portal/webapp/src/components/support/case-creation-layout/sections/case-details-section/__tests__/CaseDetailsSection.test.tsx This cleans up the Basic Information UI implementation and related tests (likely in preparation for a refactor or replacement).
Delete the CaseDetailsSection component and its associated unit tests for the rich text editor and attachments attachments list. Removed files: - apps/customer-portal/webapp/src/components/support/case-creation-layout/sections/case-details-section/CaseDetailsSection.tsx - apps/customer-portal/webapp/src/components/support/case-creation-layout/sections/case-details-section/rich-text-editor/__tests__/RichTextEditor.test.tsx - apps/customer-portal/webapp/src/components/support/case-creation-layout/sections/case-details-section/rich-text-editor/attachments/__tests__/AttachmentsListDisplay.test.tsx This cleanup removes the obsolete UI component and its tests from the customer portal frontend.
Introduce local CaseDetailsHeaderSkeleton and CaseDetailsSkeleton components with oxygen-ui Skeletons and layout (replacing the previous re-export) to provide a header-only and full header+action-row loading UI. Refactor CaseDetailsTabPanels to use a switch on activeTab (add explicit cases, clearer empty-state messages, and a default null) for readability and consistent early returns. Simplify OutstandingCasesList onClick by using optional chaining (onCaseClick?.(c)) to avoid creating unnecessary wrapper functions.
Replace the inline avatar+initials rendering in CaseDetailsDetailsPanel with a dedicated AssignedEngineerDisplay component and tidy imports/formatting. Remove the now-unused CaseDetailsSkeleton file. Update CaseDetailsTabPanels tests to wrap the component with ErrorBannerProvider and add a mock for @asgardeo/react's useAsgardeo; adjust imports accordingly.
Introduce AssignedEngineerDisplay component to render engineer avatar (initials) and name. Replace inline initials generation in CaseDetailsContent with shared getInitials util, simplify useGetCaseAttachments call (remove pagination args), and fix CaseDetailsSkeleton import path. Update tests to expect the generic error message (remove projecthub assertion).
Delete AttachmentsListDisplay.tsx, CodeBlockInsertDialog.tsx, and the associated CodeBlockInsertDialog.test.tsx from the case-creation rich-text-editor section. This removes legacy/unused UI components and their test to clean up the support case details editor implementation.
Delete the EditorContentArea component and its related unit tests. Removed files: EditorContentArea.tsx, EditorContentArea.test.tsx, and LinkInsertPopover.test.tsx. This cleans up the rich-text editor area and associated tests (likely due to refactor or replacement); update any imports that referenced the deleted component.
Delete ConversationSummary sidebar component, its unit tests, and the TextFormattingControls rich-text toolbar from the case-creation layout. Removes: ConversationSummary.tsx, ConversationSummary.test.tsx, and TextFormattingControls.tsx — cleaning up deprecated/unused UI components and associated tests.
Delete LinkInsertPopover.tsx, MarkdownEditorDialog.tsx and the corresponding MarkdownEditorDialog.test.tsx. These legacy rich-text editor components and their unit test were removed as part of cleanup/refactor to consolidate or replace markdown and link handling in the case details editor.
Delete the legacy rich-text editor and related subcomponents. Removed files: - apps/customer-portal/webapp/src/components/support/case-creation-layout/sections/case-details-section/rich-text-editor/RichTextEditor.tsx - apps/customer-portal/webapp/src/components/support/case-creation-layout/sections/case-details-section/rich-text-editor/markdown-editor/MarkdownEditorSection.tsx - apps/customer-portal/webapp/src/components/support/case-creation-layout/sections/case-details-section/rich-text-editor/toolbar/AlignmentControls.tsx This removes the inline editor implementation (toolbar, markdown editing section, and alignment controls). Update any imports/usages and related tests or documentation as needed.
Delete three toolbar components used by the case creation rich-text editor: BlockFormatControl.tsx, HistoryControls.tsx, and InsertControls.tsx. Files removed from apps/customer-portal/webapp/src/components/support/case-creation-layout/sections/case-details-section/rich-text-editor/toolbar. Update any references/usages or replace with the new toolbar implementation as needed.
Deleted two rich-text toolbar components (ListControls.tsx and MarkdownControl.tsx) from the case-creation editor. Added a new unit test ActivityCommentInput.test.tsx that mocks usePostComment and Asgardeo hooks and verifies ActivityCommentInput: renders placeholder and send button, keeps send disabled when empty, enables on input, and calls mutate with the expected payload and callbacks.
Add unit tests for AssignedEngineerDisplay and CaseDetailsContent. The new tests cover rendering of engineer name and avatar initials, handling null/undefined assignedEngineer, and CaseDetailsContent behaviors including loading skeleton, header/action row, focus mode toggle, attachment count, and tab panels. Also reformat UploadAttachmentModal callbacks to multiline useCallback forms and tidy dependency arrays (cosmetic changes only, no functional changes).
Add unit tests for ChatMessageCard and CommentBubble components (including license headers and dompurify mock). Update CaseDetailsActivityPanel.test to wrap the panel with ErrorBannerProvider and mock @asgardeo/react's useAsgardeo to avoid auth-related test failures. These changes improve coverage for the activity tab and fix setup issues in the existing panel test.
Move inline ActivityCommentInput and ChatMessageCard implementations into their own files and refactor CaseDetailsActivityPanel to consume them. The panel was reorganized to introduce an ActivityContent wrapper, clean up imports, and reuse hasDisplayableContent/formatCommentDate helpers. ActivityCommentInput now handles auth state and error banners when posting comments. Adds a TODO noting the component needs a full refactor.
Introduce a new CommentBubble component for case activity: renders avatar (derived initials), display name, timestamp, role chip, and a ChatMessageCard with sanitized HTML content (uses DOMPurify and support utils to strip wrappers/labels and replace inline images). Also add unit tests for attachments: AttachmentListItem.test.tsx (verifies rendering of name/meta and onDownload callback) and AttachmentsListSkeleton.test.tsx (verifies skeleton placeholders).
Add reusable AttachmentListItem and AttachmentsListSkeleton components and refactor CaseDetailsAttachmentsPanel to use them. Removes duplicated icon and skeleton markup from the panel, cleans up imports, and simplifies the isLoading expression. This reduces UI duplication, improves readability, and centralizes attachment row and loading state rendering.
Adjust indentation and line breaks of the ternary that chooses the file icon in SelectedFileDisplay.tsx. This is a purely stylistic change with no functional impact.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughRefactors icon handling to use component references, removes the RichTextEditor subsystem and related dialogs/tests, adds comment/activity UI (comment input, chat message card, comment bubble), extends support utilities (sanitization, date/initials), restructures several case creation/details form sections, and updates numerous tests and mocks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant UI as ActivityCommentInput (Client)
participant Auth as useAsgardeo (Auth)
participant API as usePostComment (Mutation)
participant Banner as ErrorBannerProvider / Toast
rect rgba(120, 200, 80, 0.5)
User->>UI: Type comment + press Enter / Click Send
end
UI->>Auth: check isSignedIn / isLoading
alt signed in
UI->>API: mutate({ caseId, content })
API-->>UI: pending / loading
API-->>UI: success
UI->>UI: clear input, optionally refresh comments (re-render)
else not signed in / auth loading
UI->>Banner: show sign-in-required or disable input
end
alt API error
API-->>Banner: show error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
dileepapeiris
added
the
Type/New Feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
`@apps/customer-portal/webapp/src/components/support/case-details/activity-tab/CommentBubble.tsx`:
- Around line 148-158: The "Support Engineer" Chip in CommentBubble is currently
shown whenever !isCurrentUser, which mislabels other non-current-user
commenters; update the conditional to check the comment author's role/type
instead (e.g., use comment.author.role === 'support' or author.type ===
'engineer') before rendering the Chip so only true support engineers get the
badge; locate the JSX in CommentBubble where the Chip is rendered and replace
the !isCurrentUser gate with a role-based predicate (fall back to existing logic
only if role/type is absent).
- Around line 43-56: The local deriveInitialsFromEmail function duplicates logic
and is email-specific; replace its usage with the centralized getInitials
utility from `@utils/support`. Remove deriveInitialsFromEmail and import
getInitials, then update places that call deriveInitialsFromEmail (e.g., where
CommentBubble uses comment.createdBy) to call getInitials(comment.createdBy) so
name-or-email inputs are handled correctly by the shared helper.
In
`@apps/customer-portal/webapp/src/components/support/case-details/attachments-tab/__tests__/AttachmentListItem.test.tsx`:
- Around line 23-31: The mock object `mockAttachment` in
AttachmentListItem.test.tsx doesn't match the `CaseAttachment` shape: add the
required `type` string property (e.g., "image/png" or "attachment") and change
`sizeBytes` from a number to a string (or undefined) to match `sizeBytes: string
| undefined` in responses.ts; update the mock definition accordingly so its
fields (id, name, size, sizeBytes, type, createdBy, createdOn, downloadUrl)
conform to the `CaseAttachment` interface.
In
`@apps/customer-portal/webapp/src/components/support/case-details/details-tab/__tests__/AssignedEngineerDisplay.test.tsx`:
- Around line 37-45: The two tests in AssignedEngineerDisplay.test.tsx fail
because getByText("--") finds two elements (Avatar and Typography) when
assignedEngineer is null/undefined; update both tests to use
screen.getAllByText("--") and assert that it returns two elements (length === 2)
or otherwise verify both elements are present; locate the tests using the helper
renderAssignedEngineer and replace getByText("--") assertions with
getAllByText("--") + length assertion to ensure both Avatar and Typography
render.
In
`@apps/customer-portal/webapp/src/components/support/case-details/details-tab/__tests__/CaseDetailsContent.test.tsx`:
- Line 17: The test imports userEvent from the wrong package; update the imports
in CaseDetailsContent.test.tsx by removing userEvent from the named import from
"@testing-library/react" and instead add an import for userEvent from
"@testing-library/user-event" (use the default export or named import as
appropriate), ensuring any usages of userEvent in the file still reference the
same symbol; verify tests import render and screen from "@testing-library/react"
and userEvent from "@testing-library/user-event".
In `@apps/customer-portal/webapp/src/providers/BackgroundTokenRefresh.tsx`:
- Around line 42-58: The refreshToken handler can repeatedly call signOut()
after failureCountRef reaches FAILURE_THRESHOLD; add a signingOutRef (ref
boolean) and check it before invoking signOut() inside refreshToken: if
signingOutRef.current is true, skip; if false set signingOutRef.current = true
just before calling signOut(), then call signOut() and in its .catch/.finally
ensure signingOutRef.current stays true until sign-out completes or reset
appropriately if needed; update references in refreshToken and any cleanup
effects so only one signOut() is in-flight.
In `@apps/customer-portal/webapp/src/utils/__tests__/support.test.ts`:
- Around line 424-436: The tests fail under non-English locales because
formatCommentDate uses toLocaleString(undefined,...); update formatCommentDate
to pass an explicit locale (e.g., 'en-US' or 'en') into toLocaleString (or
otherwise normalize output) so its string is consistent across CI environments,
then keep the existing tests; locate the function formatCommentDate and change
the toLocaleString call to toLocaleString('en-US', options) (or choose 'en') to
produce deterministic month/day/year text.
🧹 Nitpick comments (17)
apps/customer-portal/webapp/src/constants/loginScreenConstants.ts (1)
31-52: Consider defaulting the repeatediconClassName.All four items use the same
"text-muted-foreground"value. You could default this in the rendering component (LoginSlogan) and only specifyiconClassNamewhen overriding, reducing repetition here. Not a problem as-is — just a nit for DRYness.apps/customer-portal/webapp/src/components/support/support-overview-cards/OutstandingCasesList.tsx (1)
77-77: Clean simplification using optional chaining — minor UX note.When
onCaseClickisundefined, this still attaches a no-op arrow function as theonClickhandler, soForm.CardButtonmay render with interactive styling (pointer cursor, focus ring, hover effects) even though clicking does nothing. If this component is ever used withoutonCaseClick, consider guarding:onClick={onCaseClick ? () => onCaseClick(caseItem) : undefined}If
onCaseClickis always provided in practice, this is a non-issue.apps/customer-portal/webapp/src/components/support/all-cases/AllCasesList.tsx (1)
80-80: Same optional-chaining simplification — same UX caveat asOutstandingCasesList.Same note applies: if
onCaseClickcan beundefined, a no-oponClickkeeps the card looking interactive. See the comment onOutstandingCasesList.tsxfor the suggested guard pattern.apps/customer-portal/webapp/src/components/support/case-details/attachments-tab/SelectedFileDisplay.tsx (1)
38-43: Consider refactoring to use icon component references for consistency.While the multi-line formatting improves readability, this implementation still uses pre-rendered JSX elements. The PR's stated goal is to "standardize icon rendering by passing icon component references (React.ElementType) instead of pre-rendered JSX," and other components (ProjectStatisticsCard, ContactRow) have been updated to this pattern.
For consistency with the broader refactoring effort, consider storing the component reference and rendering it in JSX:
♻️ Proposed refactor to align with PR's icon component pattern
- const icon = - category === "archive" ? ( - <FileArchive size={24} aria-hidden /> - ) : ( - <File size={24} aria-hidden /> - ); + const IconComponent = category === "archive" ? FileArchive : File;Then update the rendering (line 70):
- {icon} + <IconComponent size={24} aria-hidden />apps/customer-portal/webapp/src/constants/projectDetailsConstants.ts (1)
17-24: Consolidate duplicate imports from the same module.
@wso2/oxygen-ui-icons-reactis imported twice. Merge into a single statement.♻️ Proposed fix
import { CircleAlert, Clock, Info, Rocket, Server, + User, + Shield, } from "@wso2/oxygen-ui-icons-react"; -import { User, Shield } from "@wso2/oxygen-ui-icons-react";apps/customer-portal/webapp/src/utils/support.ts (1)
345-351:hasDisplayableContentstrips HTML with a simple regex — acceptable here but be aware of edge cases.The
/<[^>]+>/gstrip is fine for the stated purpose (detecting empty backend entries). Note that a comment containing only inline images (no text) will returnfalse. If image-only comments should be considered displayable, this would need adjustment. Verify whether that scenario applies.apps/customer-portal/webapp/src/components/support/case-details/activity-tab/ChatMessageCard.tsx (2)
39-92: Document the sanitization contract forhtmlContent.The static analysis tools correctly flag
dangerouslySetInnerHTML(line 92). Per the PR context, the upstreamCommentBubblecomponent sanitizes HTML with DOMPurify before passing it here. However,ChatMessageCarditself has no guard — any caller passing unsanitized content would introduce an XSS vector.Consider adding a JSDoc note on the
htmlContentprop (or the component doc) stating that the caller must pass DOMPurify-sanitized HTML. This makes the contract explicit for future maintainers.export interface ChatMessageCardProps { + /** Must be pre-sanitized (e.g. via DOMPurify) before being passed. */ htmlContent: string;
46-47: Regex-based HTML stripping is approximate but acceptable for a threshold check.
/<[^>]+>/gwon't handle HTML entities (&etc.) or edge cases like<in text, soplainLengthcan be off. For a fuzzy "show more" threshold of 200 chars this is fine, but be aware it could miscount on entity-heavy content.apps/customer-portal/webapp/src/components/support/case-details/attachments-tab/AttachmentListItem.tsx (1)
35-49: Minor:"pdf"and"text"cases return identical icons — consider collapsing.Both cases return
<FileText size={20} aria-hidden />. You can use a fall-through pattern to reduce duplication:♻️ Suggested simplification
switch (category) { case "image": return <Image size={20} aria-hidden />; case "pdf": - return <FileText size={20} aria-hidden />; case "text": return <FileText size={20} aria-hidden />;apps/customer-portal/webapp/src/components/support/case-details/header/__tests__/CaseDetailsTabPanels.test.tsx (1)
97-104: Inconsistent indentation insideErrorBannerProviderwrapper.The
<CaseDetailsTabPanels>and its closing tag are indented at a different level than the wrapping<ErrorBannerProvider>. Minor formatting nit.Suggested fix
<ErrorBannerProvider> - <CaseDetailsTabPanels - activeTab={activeTab} - caseId={caseId} - data={options?.data ?? mockCaseDetails} - isError={options?.isError ?? false} - /> + <CaseDetailsTabPanels + activeTab={activeTab} + caseId={caseId} + data={options?.data ?? mockCaseDetails} + isError={options?.isError ?? false} + /> </ErrorBannerProvider>apps/customer-portal/webapp/src/components/support/case-details/activity-tab/__tests__/ActivityCommentInput.test.tsx (1)
23-73: Consider resettingmockMutatebetween tests to prevent cross-test pollution.
mockMutateis shared across tests but never cleared. If test order changes or new tests are added,toHaveBeenCalledWithassertions could pick up calls from previous tests.Suggested fix
+import { beforeEach } from "vitest"; + const mockMutate = vi.fn(); + +beforeEach(() => { + mockMutate.mockClear(); +});apps/customer-portal/webapp/src/components/support/case-details/activity-tab/CaseDetailsActivityPanel.tsx (2)
23-26: Duplicate import path and potentially unused import.
formatCommentDateandhasDisplayableContentare imported from@utils/supportin two separate statements. Additionally,formatCommentDatedoes not appear to be used in this file (it's used byCommentBubblewhich imports it separately).Suggested fix
-import { formatCommentDate } from "@utils/support"; -import ActivityCommentInput from "@case-details-activity/ActivityCommentInput"; -import CommentBubble from "@case-details-activity/CommentBubble"; -import { hasDisplayableContent } from "@utils/support"; +import { hasDisplayableContent } from "@utils/support"; +import ActivityCommentInput from "@case-details-activity/ActivityCommentInput"; +import CommentBubble from "@case-details-activity/CommentBubble";
158-233: Inconsistent indentation inActivityContent.The JSX inside
ActivityContent's return statement uses a deeper indentation level than the function body, likely carried over from when this was inline JSX in the parent component.apps/customer-portal/webapp/src/components/support/case-details/activity-tab/ActivityCommentInput.tsx (1)
86-91: Consider also handling Ctrl+Enter / Cmd+Enter as a send shortcut.Minor UX note: currently only bare
Enter(without Shift) triggers send. Some users expectCtrl+Enteras an alternative send shortcut in chat-like inputs. Not blocking, just a consideration for future iteration.apps/customer-portal/webapp/src/components/support/case-details/activity-tab/CommentBubble.tsx (3)
72-79: Double DOMPurify sanitization —replaceInlineImageSourcesalready sanitizes.
replaceInlineImageSources(insupport.ts, lines 375 and 397) already returnsDOMPurify.sanitize(…)output for both branches. Line 79 sanitizes again, which is redundant. It's not harmful (idempotent), but it adds unnecessary processing and obscures the sanitization contract.Either remove the outer sanitize here, or remove the sanitize inside
replaceInlineImageSourcesand let callers be responsible — but pick one.♻️ Proposed fix — drop the redundant sanitize
const withImages = replaceInlineImageSources( withoutLabel, comment.inlineAttachments, ); - const htmlContent = DOMPurify.sanitize(withImages); + const htmlContent = withImages; // already sanitized by replaceInlineImageSourcesAnd you can also remove the
import DOMPurify from "dompurify";on line 33 if no other usage remains.
97-105: Passdirectionto Stack prop instead of overriding viasx.Using
direction="row"onStackwithspacingand then overridingflexDirectionviasxcan cause spacing misalignment if the underlying implementation uses directional margins instead of CSSgap. Passing the direction directly ensuresspacingis calculated correctly.The same issue applies to the inner Stack on lines 130-138.
♻️ Proposed fix
<Stack - direction="row" + direction={isRight ? "row-reverse" : "row"} spacing={1.5} alignItems="flex-start" - sx={{ - flexDirection: isRight ? "row-reverse" : "row", - }} >Apply the same pattern to the inner
Stackat line 130:<Stack - direction="row" + direction={isRight ? "row-reverse" : "row"} spacing={1} alignItems="center" flexWrap="wrap" - sx={{ - flexDirection: isRight ? "row-reverse" : "row", - minHeight: 32, - }} + sx={{ minHeight: 32 }} >
64-69: Inconsistent return type annotation style.
ActivityCommentInput.tsximportstype { JSX } from "react"and usesJSX.Element, while this file uses an inlineimport("react").JSX.Element. Pick one style for consistency across the activity-tab components.♻️ Proposed fix
Add the import at the top of the file:
import type { JSX } from "react";Then simplify the return type:
-}: CommentBubbleProps): import("react").JSX.Element { +}: CommentBubbleProps): JSX.Element {
Update unit tests to better match runtime imports and avoid leaking globals. Add TriangleAlert icon mocks across header/filter/profile tests and adjust ErrorIndicator mock paths to @components/common/error-indicator. In useGetProjectSupportStats tests, set CUSTOMER_PORTAL_BACKEND_BASE_URL on window.config and restore the original config (using try/finally) to ensure no global state leakage and more robust async assertion handling.
Add required providers and test adjustments to stabilize unit tests and make SLA handling more robust. - AllCasesPage tests: wrap rendered component in LoaderProvider and import the provider. - DashboardPage tests: add a mock Alert component and update expected error message text to "Could not load dashboard statistics.". - ProjectDetails tests: import ReactElement and MockConfigProvider, add a renderWithProviders helper, and update render/rerender calls to use the provider so tests run with required config context. - support.test: correct SLA formatting expectations (fix comment) and add an explicit case for 2 days. - projectStats.ts: compute a normalized goodValue and accept both the constant and literal "good" (case-insensitive) when determining success status. These changes ensure tests run with the necessary context, align expectations with actual formatting logic, and make SLA status detection more tolerant of input variations.
Enhance and fix several unit tests: add missing Oxygen UI mocks (Box, Typography, Paper, Button, Menu, MenuItem) so components render correctly; correct mocked import paths for SubscriptionWidget and ActiveFilters to match project structure; update CasesTable expectation to reflect API-returned count (2) and clarify comment; adjust TimeTrackingStatCards text check to "Non-Billable Hours"; wrap ProjectCard renders with LoaderProvider via a renderWithLoader helper and import ReactElement to satisfy typing. These changes address failing tests caused by missing mocks, incorrect paths, and inaccurate assertions.
Update tests and component logic to fix mocking and initials handling. - Extend @wso2/oxygen-ui mock to include IconButton and Tooltip used by ErrorIndicator. - Correct ErrorIndicator mock import path to @components/common/error-indicator/ErrorIndicator. - Replace custom deriveInitialsFromEmail in CommentBubble with shared getInitials and simplify initials derivation logic. - Update attachment tests: switch from userEvent to fireEvent, add attachment `type`, and change `sizeBytes` to a string to match usage in component. - Replace async userEvent-based clicks in tests with synchronous fireEvent clicks. - Make tests for rendering "--" more robust by using getAllByText and asserting at least one match. These changes fix test failures caused by incorrect mocks, inconsistent test data types, and duplicate initials logic.
Refine unit tests for stability and completeness: - CaseDetailsDetailsPanel: loosen SLA regex matcher to accept singular and plural units by using a custom getByText predicate. - CasesOverviewStatCard: replace manual icon mocks with an importOriginal-based mock that preserves actual exports and supplies SVG placeholders for many icons (prevents missing-import issues from supportConstants). - LoaderContext: enable vitest fake timers in beforeEach, restore afterEach, and advance timers when hiding the loader to account for the test's 500ms hide delay. These changes make tests more robust and avoid brittle assertions or missing-icon failures.
Update tests to account for API totalRecords (41) and DOM text node splitting. Use a predicate with normalized whitespace in getByText to reliably match "Showing X of Y cases", and simplify displayedCount to Math.min(10, mockCases.length). Remove brittle expectations that assumed the API total matched the filtered list.
Provide a React Query client to ProjectDetails tests: add QueryClient and QueryClientProvider (with retries disabled) and wrap render/rerender calls so react-query hooks run in tests. Also add and mock useGetProjectTimeTrackingStat, ensuring it returns a settled state in beforeEach.
Add mocks for @asgardeo/react across several tests to avoid ESM buffer import issues; add mocks for case-creation-layout components in CreateCasePage tests; include Zap and Activity icon mocks used by supportConstants in SupportPage tests. Update markdownToHtml in richTextEditor to use {{...}} placeholders for fenced code, inline code, and code-block placeholders to avoid conflicts with emphasis regex and ensure reliable protection/restoration of code spans and blocks.
Improve unit test stability by adding/adjusting mocks to avoid needing full provider setups. Mock BackgroundTokenRefresh in AppLayout tests; mock useLogger and wrap CaseDetailsPage renders with MockConfigProvider so tests don't require Logger/Config providers. Update @wso2/oxygen-ui and @wso2/oxygen-ui-icons-react mocks in SupportPage tests to import and spread the original modules (including alpha exports), add a missing grey palette entry, and provide concrete icon components so UI pieces like RequestCard render correctly in tests.
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 (2)
apps/customer-portal/webapp/src/api/__tests__/useGetProjectSupportStats.test.tsx (1)
185-190:⚠️ Potential issue | 🟡 MinorConfig restoration not wrapped in
try/finally, unlike the previous test.If any assertion on lines 185–189 throws,
window.configwon't be restored, potentially polluting subsequent tests. This is inconsistent with thetry/finallypattern correctly applied in the "should fetch real data" test above.Proposed fix
- await waitFor(() => expect(result.current.isError).toBe(true)); - expect(result.current.error?.message).toContain( - "Error fetching support stats: Internal Server Error", - ); - expect(mockLogger.error).toHaveBeenCalled(); - (window as any).config = originalConfig; + try { + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(result.current.error?.message).toContain( + "Error fetching support stats: Internal Server Error", + ); + expect(mockLogger.error).toHaveBeenCalled(); + } finally { + (window as any).config = originalConfig; + }apps/customer-portal/webapp/src/pages/__tests__/DashboardPage.test.tsx (1)
132-156:⚠️ Potential issue | 🟡 Minor
AlertandPapermocks share the samedata-testid="error-banner"— fragile.Both the
Alertmock (line 133) and thePapermock (line 153) usedata-testid="error-banner". If the component under test ever renders both simultaneously,getByTestId("error-banner")will throw a "found multiple elements" error, producing a misleading failure.Consider giving
Papera distinct test ID (e.g.,"paper") to keep selectors unambiguous.Proposed fix
Paper: ({ children }: any) => ( - <div data-testid="error-banner" role="alert"> + <div data-testid="paper" role="alert"> {children} </div> ),
🤖 Fix all issues with AI agents
In `@apps/customer-portal/webapp/src/pages/__tests__/AllCasesPage.test.tsx`:
- Around line 186-197: The assertion in the status filter test hardcodes "41"
instead of using the mocked total count; update the expectation to use
mockCases.length (same approach as displayedCount uses Math.min) so the line
that builds the text comparator uses mockCases.length instead of 41 — adjust the
getByText callback where it checks for `Showing ${displayedCount} of 41 cases`
to `Showing ${displayedCount} of ${mockCases.length} cases`, referencing the
displayedCount and mockCases variables in AllCasesPage.test.tsx.
- Around line 169-177: The test hardcodes "41" which will break if mockCases
changes; update the assertion in AllCasesPage.test.tsx to use the
mockCases.length value (the same source used in the mock setup / totalRecords)
when checking the "Showing 1 of X cases" text, by replacing the literal 41 with
mockCases.length (reference the test file's usage of mockCases and the getByText
callback), and keep the existing "Showing 1 cases" assertion as-is since that
matches the mocked AllCasesList output.
🧹 Nitpick comments (10)
apps/customer-portal/webapp/src/utils/projectStats.ts (1)
55-57: Extract the"good"magic string into a constant.
SLA_STATUS.GOODis"All Good", sogoodValueresolves to"all good"— meaning the new"good"branch handles a distinct value. Hard-coding it here obscures that intent and makes it easy to miss if SLA status values change.Consider adding it to
SLA_STATUS(or a derived mapping) so both accepted values are discoverable in one place:Suggested approach
In
projectDetailsConstants.ts:export const SLA_STATUS = { GOOD: "All Good", + GOOD_SHORT: "Good", BAD: "Bad", } as const;Then in
projectStats.ts:- if (normalizedStatus === goodValue || normalizedStatus === "good") { + if (normalizedStatus === goodValue || normalizedStatus === SLA_STATUS.GOOD_SHORT.toLowerCase()) {apps/customer-portal/webapp/src/api/__tests__/useGetProjectSupportStats.test.tsx (1)
69-72:mockClearcalls are redundant withvi.clearAllMocks().
vi.clearAllMocks()on line 72 already clears all mocks (includingmockLogger.debugandmockLogger.error), making the explicit.mockClear()calls on lines 69–70 unnecessary.apps/customer-portal/webapp/src/components/common/filter-panel/__tests__/FilterPopover.test.tsx (1)
84-93: Nit: InconsistentXicon mock vs.mockIconhelper.
Filter,Search, andTriangleAlertall use themockIconhelper, butXis defined inline with a lowercasedata-testid("icon-x"vs. the"icon-X"thatmockIcon("X")would produce). Since no test in this file queries by the X icon'sdata-testid, it's harmless today, but usingmockIcon("X")(or renaming to keep the lowercase convention) would be more consistent.Suggested consistency fix
vi.mock("@wso2/oxygen-ui-icons-react", () => { const mockIcon = (name: string) => () => <span data-testid={`icon-${name}`} />; return { Filter: mockIcon("Filter"), Search: mockIcon("Search"), - X: () => <span data-testid="icon-x" />, + X: mockIcon("X"), TriangleAlert: mockIcon("TriangleAlert"), }; });apps/customer-portal/webapp/src/components/support/case-details/activity-tab/CommentBubble.tsx (2)
58-65: Redundant doubleDOMPurify.sanitizecall.
replaceInlineImageSources(in@utils/support) already returnsDOMPurify.sanitize(...)in all code paths. The secondDOMPurify.sanitize(withImages)on line 65 is therefore redundant. While idempotent and not a bug, it performs unnecessary DOM parsing on every render.♻️ Suggested fix
const withImages = replaceInlineImageSources( withoutLabel, comment.inlineAttachments, ); - const htmlContent = DOMPurify.sanitize(withImages); + const htmlContent = withImages;If you'd rather keep an explicit sanitization boundary in this component for defense-in-depth, consider removing the
DOMPurifyimport and the duplicate call fromreplaceInlineImageSourcesinstead, so the sanitization lives in one clear place.
56-65: Content processing runs on every render without memoization.The pipeline (
stripCodeWrapper→stripCustomerCommentAddedLabel→replaceInlineImageSources→ sanitize) includes regex replacements, DOM parsing in DOMPurify, and image-source matching. Unlikeinitials, this work isn't wrapped inuseMemo. For long comments with inline attachments, this could be noticeable on frequent re-renders (e.g., expand/collapse toggling theexpandedstate).♻️ Suggested fix — wrap in useMemo
- const rawContent = comment.content ?? ""; - const stripped = stripCodeWrapper(rawContent); - const withoutLabel = stripCustomerCommentAddedLabel(stripped); - const withImages = replaceInlineImageSources( - withoutLabel, - comment.inlineAttachments, - ); - const htmlContent = DOMPurify.sanitize(withImages); + const htmlContent = useMemo(() => { + const raw = comment.content ?? ""; + const stripped = stripCodeWrapper(raw); + const withoutLabel = stripCustomerCommentAddedLabel(stripped); + return replaceInlineImageSources(withoutLabel, comment.inlineAttachments); + }, [comment.content, comment.inlineAttachments]);apps/customer-portal/webapp/src/utils/__tests__/support.test.ts (1)
358-358: Test description says "null content" but passesundefined.Nit: the
itdescription reads"returns false for null content"but the actual value assigned isundefined as unknown as string. Consider either updating the description to say "undefined/missing content" or adding a separate case that actually passesnull as unknown as stringto cover both.apps/customer-portal/webapp/src/pages/__tests__/AllCasesPage.test.tsx (1)
170-172: Extract the repeated text-normalization matcher.The same
texthelper lambda is defined 4 times inside individualwaitForblocks. Consider extracting it to file scope.Proposed refactor
Define once at file scope (e.g. near
renderComponent):/** Matches an element whose full textContent (whitespace-normalised) equals `content`. */ const textContentEquals = (content: string, el: Element | null) => el?.textContent?.replace(/\s+/g, " ").trim() === content;Then use
textContentEqualsin eachwaitForinstead of re-declaringtext.Also applies to: 190-191, 240-241, 257-258
apps/customer-portal/webapp/src/pages/__tests__/CaseDetailsPage.test.tsx (1)
65-74: SharedQueryClientacross tests risks cache leakage.The
queryClientis created once at module scope and reused for every test. React Query caches results in the client, so a prior test's cache can leak into the next. Although the current tests are small and fully mocked, this becomes a latent issue as the suite grows.Create a fresh client per test or clear the cache in
beforeEach.Proposed refactor
+import { beforeEach } from "vitest"; + +let queryClient: QueryClient; + +beforeEach(() => { + queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); +}); + const renderWithProviders = (ui: ReactElement) => render( <QueryClientProvider client={queryClient}> <MockConfigProvider>{ui}</MockConfigProvider> </QueryClientProvider>, );Remove the module-level
const queryClient = …declaration.apps/customer-portal/webapp/src/pages/__tests__/ProjectDetails.test.tsx (2)
121-125: Same sharedQueryClientconcern — create per test to avoid cache leakage.Same pattern as
CaseDetailsPage.test.tsx: thequeryClientlives at module scope and is shared across all tests. Move creation intobeforeEachor reset the cache there.Proposed refactor
+let queryClient: QueryClient; + describe("ProjectDetails", () => { beforeEach(() => { vi.clearAllMocks(); + queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); mockUseGetProjectDetails.mockReturnValue({ ... });Remove the module-level
const queryClient = ….
206-212: Rerender manually duplicates provider nesting — acceptable but could be DRYer.The
rerender(...)call rebuilds the full provider tree by hand. If the provider stack grows, this gets out of sync withrenderWithProviders. One option: extract aWrappercomponent and pass it via RTL'swrapperoption sorerenderinherits it automatically.
Introduce three new stub React components for the support case creation layout: CaseCreationHeader, BasicInformationSection, and CaseDetailsSection. Each file defines a TypeScript props interface and exports a component that currently returns null; files include the project license header. These placeholders reserve component APIs and file locations for future implementation in apps/customer-portal/webapp/src/components/support/case-creation-layout.
Introduce a ConversationSummary stub component used in the case creation layout to avoid rendering/stub issues. Update tests to be less brittle: replace hardcoded totals with mockCases.length in AllCasesPage tests, relax strict UI assertions in CreateCasePage and SupportPage (check for presence of key elements like the create button/boxes/grids and use >=1 for icon counts), and adjust test names/expectations accordingly. Files changed: ConversationSummary.tsx (new), AllCasesPage.test.tsx, CreateCasePage.test.tsx, SupportPage.test.tsx.
Replace stub components with full UI implementations for the case creation flow. Adds CaseCreationHeader (back button, title, AI badge), BasicInformationSection (project/deployment/product fields, edit mode, selectable options, loading skeletons), CaseDetailsSection (title/description, issue type & severity selects, AI chips, severity color indicators, edit mode) and ConversationSummary (conversation stats, loading skeletons, error indicators and tips). Introduces stronger prop types, additional props for extra options, local edit state, and imports from the oxygen UI library and icons.
Introduce BasicInformationSection and CaseDetailsSection components for the case-creation layout and add unit tests. BasicInformationSection implements project/deployment/product fields with edit toggling, loading skeletons, auto-detected chips, and merging of metadata + extra options. CaseDetailsSection provides title/description, issue type and severity selects (renders severity color via getSeverityColor), edit toggling, and supports merging filters/extra values. Added tests for BasicInformationSection, CaseDetailsSection, and ConversationSummary (mocks utilities and verifies labels, loading states, skeletons, and rendered metadata). Components use @wso2/oxygen-ui primitives and are wired for accessibility and disabled/edit states.
Rename/move conversation summary to form-sections and remove old section files; update CreateCasePage and its tests to import components from the new form-sections paths. Also remove the hover underline style from ConversationSummary link and adjust license comment formatting. Deleted legacy BasicInformationSection and CaseDetailsSection files (paths updated to reflect the new layout).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/customer-portal/webapp/src/components/support/case-creation-layout/form-sections/conversation-summary-section/ConversationSummary.tsx (1)
105-118:⚠️ Potential issue | 🟡 Minor"View full conversation" looks clickable but has no handler.
This
Typographyis styled withcursor: pointerand loading/disabled states but lacks anonClickhandler or anchor element. Users will see a clickable-looking element that does nothing.If this is a planned future feature, consider adding a
// TODOcomment or usingcursor: defaultuntil the handler is wired up.
🤖 Fix all issues with AI agents
In
`@apps/customer-portal/webapp/src/components/support/case-creation-layout/form-sections/case-details-section/CaseDetailsSection.tsx`:
- Around line 226-234: The mapped item `label` can be undefined so you end up
rendering the literal "undefined" for the MenuItem key/value; update the mapping
in the issueTypes iteration (the code that computes `label` inside
CaseDetailsSection.tsx) to provide a safe fallback (e.g. use typeof type ===
"string" ? type : (type as { label?: string }).label ?? "Unknown Issue") or skip
items with no label, and then use that sanitized `label` for the MenuItem key
and value; ensure you update the `issueTypes.map` logic and the MenuItem usage
so `label` is never undefined.
🧹 Nitpick comments (8)
apps/customer-portal/webapp/src/components/support/case-creation-layout/form-sections/case-details-section/__tests__/CaseDetailsSection.test.tsx (1)
55-116: Consider adding interaction and state tests.All current tests verify static rendering. The component has meaningful interactive behavior (edit button toggles
isEditing, which controls field disabled state) that isn't covered. At minimum, a test verifying that fields are initially disabled and become enabled after clicking "Edit case details" would add valuable coverage.apps/customer-portal/webapp/src/pages/__tests__/CreateCasePage.test.tsx (3)
91-107: Stale/misleading comment on line 91.The comment
// Mock case-creation-layout components (paths may not exist in repo)suggests uncertainty about whether these modules exist. If they do exist (which they should, sinceCreateCasePageimports them), this comment is misleading and should be reworded to a straightforward mock description like the others in the file.
234-252: Second test is a near-duplicate of the first and lackswaitFor.This test asserts the same "Create Support Case" button that the first test already covers (line 227), without adding any unique assertion. Additionally, it queries the button synchronously without
waitFor, which could be flaky ifCreateCasePageperforms any async work before rendering the button (the first test useswaitForfor this exact reason).Consider either removing this test or differentiating it with meaningful additional assertions. If kept, wrap the assertion in
waitForfor consistency.
209-231: Test name is misleading — sections are all stubbed tonull.The test is titled "should render all sections correctly," but all four section components are mocked to render
null(lines 92–107). The assertions only verify the presence of genericbox/gridwrappers and the submit button — no section content is actually tested. Consider renaming to something like"should render the page shell and submit button"to accurately reflect what's being validated.apps/customer-portal/webapp/src/pages/__tests__/SupportPage.test.tsx (1)
225-228: Consider consistency in assertion style.Lines 227–228 use
toBeGreaterThanOrEqual(1)while line 306 still asserts an exact count withtoHaveLength(2). The relaxed checks here are fine for a loading state where the exact icon count is less meaningful, but if the intent is to reduce brittleness uniformly, consider applying the same relaxed style in the "Start New Chat" test (line 306) as well—or vice versa, keeping exact counts where the rendered structure is well-defined.apps/customer-portal/webapp/src/components/support/case-creation-layout/form-sections/basic-information-section/__tests__/BasicInformationSection.test.tsx (1)
80-89: The "enable editing" test doesn't verify the editing state change.After clicking the edit button, the test only asserts the button still exists. Consider asserting that a previously disabled control (e.g., the deployment
Select) becomes enabled.💡 Example assertion after the click
fireEvent.click(editButton); - expect(editButton).toBeInTheDocument(); + // Verify that a previously disabled select is now enabled + const deploymentSelect = screen.getByRole("combobox"); + expect(deploymentSelect).not.toBeDisabled();(Adjust the selector as needed to match the rendered DOM.)
apps/customer-portal/webapp/src/components/support/case-creation-layout/form-sections/case-details-section/CaseDetailsSection.tsx (1)
34-49:metadata?: unknownweakens type safety; consider a concrete type.The prop is typed as
unknownand immediately cast on line 73. Since the expected shape is known ({ issueTypes?: unknown[]; severityLevels?: ... }), defining it explicitly in the interface would catch misuse at compile time and remove the need for the internal cast.💡 Suggested type
export interface CaseDetailsSectionProps { // ... - metadata?: unknown; + metadata?: { + issueTypes?: (string | { label?: string })[]; + severityLevels?: { id: string; label: string; description?: string }[]; + } | null; // ... }apps/customer-portal/webapp/src/pages/CreateCasePage.tsx (1)
286-289:ConversationSummaryalways receivesundefinedmetadata — error indicators always shown.
metadata={undefined}andisLoading={false}means the sidebar will permanently display three error indicators instead of conversation stats. If this is intentional placeholder behavior, a brief comment would clarify intent.
Introduce a displayLabel (label ?? "--") and use it for MenuItem key, value and content. This prevents undefined/null labels from being rendered and ensures a consistent placeholder and stable keys for issue type menu items.
Exclude issueTypes with null/empty labels before rendering MenuItem options in CaseDetailsSection. This removes the previous fallback display of "--" for missing labels and ensures only valid, non-blank labels are used as MenuItem keys and values to avoid blank/duplicate options in the Issue Type select.
1021bc1
into
wso2-open-operations:customer-portal-milestone-1
Purpose
This PR focuses on resolving technical debt and cleaning up the codebase following recent rapid feature development. It implements several architectural improvements suggested by AI review tools (CodeRabbit/Copilot) to enhance type safety, security, and project maintainability.
Goals
DOMPurifyto ensure that any HTML content rendered within the application is properly sanitized against XSS attacks.Approach
LoginSlogan,ContactRow, andProjectStatisticsCardto accept icons asReact.ElementType.<User size={20} />, these components now receiveUserand render it internally as<Icon size={...} />. This ensures consistent icon scaling throughout the UI.dompurifyand@types/trusted-typesas dependencies. This infrastructure is essential for safely rendering rich text content or project descriptions that may contain HTML.AIInfoCardandCaseCreationHeaderalong with their associated tests, as these were temporary placeholders or redundant structures.AllCasesListto use optional chaining in event handlers, reducing potential runtime errors.Changes
LoginSlogan.tsx,ContactRow.tsx,ProjectStatisticsCard.tsxpackage.json,pnpm-lock.yamldompurifyand@types/trusted-types.AIInfoCard.tsx,CaseCreationHeader.tsx(and tests)AllCasesList.tsxonClickhandlers using optional chaining.*.test.tsx(Various)Summary by CodeRabbit
New Features
Bug Fixes
Improvements