Repository navigation
Feat/add update page - #228
Rashmika998 merged 8 commits into
Conversation
Extend API models and client behavior to support richer case and project data shapes, time card responses, and UI helpers. - Update CaseDetails-related types to use reusable IdLabelRef shapes and add fields (parentCase, deployedProduct, issueType, status, conversation, etc.). - Add TimeCard and TimeCardSearchResponse models for project time cards. - Expand product/version response models and Products/Versions response interfaces. - Relax/rename DeploymentDocument fields and add PostDeploymentAttachmentResponse. - Add getTimeCardStateColorPath utility to map time card states to theme color paths. - Add getSupportOverviewChipSx and getPlainChipSx helpers (and import alpha) to produce consistent MUI sx for chips; adjust unit tests to cover these helpers. - Thread parentCaseId through CaseDetailsPage -> CreateCasePage and include parentCaseId in case creation payload when present. These changes prepare the app for richer backend payloads (products, time cards, enhanced case details) and provide consistent styling helpers for status chips.
Replace usages of case.state with case.status and switch account/project name to label in components and tests. Update test fixtures and expectations (including severity id type and UI text change to "Manage case status") and add QueryClientProvider to header tests. Add new ApiQueryKeys entries (deployment-attachments, products, product-versions-search, time-cards-search). Introduce several request interfaces for API endpoints: parentCaseId in CreateCaseRequest, PostDeploymentAttachmentRequest, PatchDeploymentProductRequest, PostDeploymentProductRequest, ProductVersionsSearchRequest, PatchDeploymentRequest, and TimeCardSearchRequest.
Introduce four new API hooks for the customer-portal app: usePostDeploymentAttachment, usePostDeploymentProduct, useSearchProductVersions, and useSearchProjectTimeCards. Each hook uses react-query with the AuthApiContext fetch function and Asgardeo auth checks, logs via useLogger, calls endpoints under CUSTOMER_PORTAL_BACKEND_BASE_URL, and includes error handling. Mutations invalidate related queries (deployment attachments/products). Queries support pagination/filters, have enabled guards for auth and required params, and use a 5-minute staleTime.
Introduce new React Query hooks for backend interactions: useGetProducts, useGetDeploymentDocuments, usePatchDeployment, and usePatchDeploymentProduct. Each hook uses the auth-aware fetch client, logging, and normalizes responses where needed; mutation hooks invalidate relevant query caches on success. Refactor AddProductModal tests to use QueryClientProvider, mock API hooks (useGetProducts, useSearchProductVersions, usePostDeploymentProduct), and add/adjust assertions for form behavior: version enabling, validation, submit flow (including cores/tps), API mutation calls, onSuccess handling, and form reset on close. Adjust test selectors and flows to match current UI behavior.
Replace local product/version inputs with API-backed selects in AddProductModal (useGetProducts, useSearchProductVersions) and submit using usePostDeploymentProduct; add projectId prop, simplify validation, reset form on close, and disable description/update fields. Enhance DeploymentCard with edit button and EditDeploymentModal integration, and pass projectId to product list. Rewrite DeploymentDocumentList to fetch documents via useGetDeploymentDocuments, show loading state, add UploadAttachmentModal, handle download links and normalize document fields. Update tests to mock new hooks, adjust render props, and add EditDeploymentModal tests. Minor fixes: format closedBy display in CaseDetailsDetailsPanel and add optional referenceType to PostCaseAttachmentRequest model.
Add a TimeCardsDateFilter component and integrate date-range filtering into ProjectTimeTracking (introduce start/end state, default 12-month range, and format helpers). Replace useGetTimeTrackingDetails with useSearchProjectTimeCards, adapt loading/error states, and map timeCards to TimeTrackingCard. Refactor TimeTrackingCard to consume the TimeCard model, simplify badge handling, use support styling utilities, and update tests accordingly. Extend UploadAttachmentModal to support deployment document uploads via deploymentId and usePostDeploymentAttachment, unify pending state handling, and adjust UI labels and upload logic.
Enable product management/editing in deployment UI: update DeploymentProductList to accept projectId, wire AddProductModal with projectId, add edit state and edit/delete IconButtons on product rows, and open a new ManageProductModal for editing product cores/TPS (invalidates deployment products query on success). Add a new EditDeploymentModal component for editing deployment name/type/description. Update time-tracking tests to use useSearchProjectTimeCards, mock the date filter, and add TimeCardsDateFilter tests.
📝 WalkthroughWalkthroughAdds server-driven deployment/product management and time-card search: new React Query hooks (fetch/mutate) for products, product versions, deployment documents, and time-cards; new/updated modals for editing deployments and managing products; refactored lists to fetch data; extended models/constants and updated multiple tests and UI wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as AddProductModal
participant API as Product Hooks
participant Backend
participant Mutation as PostDeploymentProduct
participant RQ as React Query
User->>UI: Open Add Product modal
UI->>API: useGetProducts() -> GET /products
API->>Backend: GET /products?offset=0&limit=10
Backend-->>API: products[]
API-->>UI: show product dropdown
User->>UI: Select product
UI->>API: useSearchProductVersions(productId) -> POST /products/{id}/versions/search
API->>Backend: POST payload { offset, limit }
Backend-->>API: versions[]
API-->>UI: show versions
User->>UI: Submit product details
UI->>Mutation: mutateAsync({deploymentId, body})
Mutation->>Backend: POST /deployments/{id}/products
alt success
Backend-->>Mutation: 200 OK
Mutation->>RQ: invalidate DEPLOYMENT_PRODUCTS
RQ-->>UI: refreshed list
UI-->>User: close modal
else error
Backend-->>Mutation: error
Mutation-->>UI: surface error
end
sequenceDiagram
participant User
participant UI as ProjectTimeTracking
participant Filter as TimeCardsDateFilter
participant API as useSearchProjectTimeCards
participant Backend
participant RQ as React Query
User->>UI: Mounts with projectId
UI->>Filter: render initial dates (12mo → today)
UI->>API: query POST /projects/{id}/time-cards/search { startDate, endDate, offset, limit }
API->>Backend: request
Backend-->>API: timeCards + totalRecords
API-->>RQ: cache (staleTime 5m)
API-->>UI: render time cards
User->>Filter: change date range
Filter->>UI: onStartDateChange/onEndDateChange
UI->>API: refetch with new dates
Backend-->>API: filtered timeCards
API-->>UI: updated UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
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/components/project-details/time-tracking/__tests__/ProjectTimeTracking.test.tsx (1)
188-205:⚠️ Potential issue | 🟡 MinorTerminology inconsistency: "time logs" vs "time cards".
The empty state renders
"No time logs available."(Line 204, matching the production code atProjectTimeTracking.tsxLine 117), but the rest of the PR consistently uses "time cards" terminology. TheTimeCardsDateFiltercomponent also displays "time logs" in its status text. Consider standardizing to "time cards" across the UI for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/time-tracking/__tests__/ProjectTimeTracking.test.tsx` around lines 188 - 205, The test and components use inconsistent terminology ("time logs" vs "time cards"); update the production UI and tests to use "time cards" consistently by changing the text in the ProjectTimeTracking component (the string rendered in ProjectTimeTracking.tsx) and any status text in TimeCardsDateFilter, then update the assertion in ProjectTimeTracking.test.tsx (the test that checks for "No time logs available.") to expect "No time cards available." so all UI strings and tests match the "time cards" terminology.apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentProductList.tsx (1)
201-207:⚠️ Potential issue | 🟡 Minor
Checkboxaria-label says "Batch select" butcheckedstate now reflectsisEditing.The
Checkboxwas previouslychecked={false}(static placeholder); it now showschecked={isEditing}. Itsaria-label="Batch select (not yet implemented)"no longer accurately describes its visual state to assistive technologies, since it's now reflecting the editing selection state.Consider either updating the label to describe its current role or keeping the checkbox hardcoded to
falseuntil batch-select is implemented.♻️ Proposed fix if editing-indicator intent is kept
<Checkbox sx={{ p: 0.5, mt: -0.5 }} checked={isEditing} disabled aria-disabled - aria-label="Batch select (not yet implemented)" + aria-label={isEditing ? `${name} selected for editing` : "Batch select (not yet implemented)"} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentProductList.tsx` around lines 201 - 207, The Checkbox in DeploymentProductList.tsx currently binds checked={isEditing} but still uses aria-label="Batch select (not yet implemented)"; either keep it a static placeholder by reverting checked to false until batch-select exists, or update the aria-label to reflect its current role (e.g., an editing-mode indicator) and ensure aria-disabled/aria-disabled state matches its interactivity; locate the Checkbox with props checked={isEditing}, aria-label and adjust either the checked prop or the aria-label to accurately describe the control.
🧹 Nitpick comments (25)
apps/customer-portal/webapp/src/components/support/case-details/header/__tests__/CaseDetailsTabPanels.test.tsx (2)
186-191: No test covers theCallsPanelhappy path for tab 3The updated test only validates the placeholder rendered when
projectisnull. The scenario where tab 3 receives a validprojectIdand renders<CallsPanel>is no longer exercised, leaving that branch untested.🧪 Suggested additional test
+ it("should render CallsPanel when activeTab is 3 and project is present", () => { + renderTabPanels(3); + // CallsPanel renders; the placeholder text must NOT appear + expect(screen.queryByText("Call requests will appear here.")).not.toBeInTheDocument(); + }); + it("should show Calls placeholder when activeTab is 3 and project is missing", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/support/case-details/header/__tests__/CaseDetailsTabPanels.test.tsx` around lines 186 - 191, Add a test that covers the happy path for CallsPanel when activeTab is 3 and a valid project exists: use the existing renderTabPanels helper with activeTab = 3 and pass mockCaseDetails (with project not null and a valid projectId) and then assert that the CallsPanel content is rendered (for example by checking for a CallsPanel-specific label/text or by mocking/spy-rendering the CallsPanel component). Update CaseDetailsTabPanels.test.tsx to include this new test alongside the existing null-project placeholder test so the branch that renders <CallsPanel> is exercised.
26-28: SharedQueryClientacross tests risks cache bleed-between-test pollutionThe
queryClientinstance is constructed once at module scope and reused by every test viarenderTabPanels. Although all explicitly called API hooks are mocked, child components rendered for tabs 0 and 3 (CaseDetailsActivityPanel,CallsPanel) may contain their own internaluseQuerycalls that aren't intercepted. Any cached state accumulated during one test can influence later ones, making the suite order-dependent.Recommended fix — move the client inside the helper (or reset in
afterEach):♻️ Option A — fresh client per render (preferred)
-const queryClient = new QueryClient({ - defaultOptions: { queries: { retry: false } }, -}); - function renderTabPanels( activeTab: number, caseId = "case-1", options?: { data?: CaseDetails; isError?: boolean }, ) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); return render( <QueryClientProvider client={queryClient}>♻️ Option B — module-level client cleared between tests
+import { afterEach } from "vitest"; + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } }, }); + +afterEach(() => { + queryClient.clear(); +});Also applies to: 136-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/support/case-details/header/__tests__/CaseDetailsTabPanels.test.tsx` around lines 26 - 28, The shared QueryClient instance (QueryClient) at module scope causes cache bleed between tests used by renderTabPanels; create a fresh QueryClient for each render (move new QueryClient(...) inside renderTabPanels) or reset/clear it in afterEach (e.g., call queryClient.clear() or recreate it in a beforeEach) so each test gets an isolated cache, and update references in the tests that call renderTabPanels and any utility that uses the module-level queryClient.apps/customer-portal/webapp/src/components/support/case-details/details-tab/__tests__/CaseDetailsContent.test.tsx (1)
36-47: Consider adding at least one assertion for the newly introduced fields.
type,parentCase,conversation,closedOn,closedBy,closeNotes, andhasAutoClosedare present in the mock but have no corresponding expectations in any of the five test cases. This is a minor coverage gap — especially fortype, which carries a user-visible label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/support/case-details/details-tab/__tests__/CaseDetailsContent.test.tsx` around lines 36 - 47, Add assertions in CaseDetailsContent.test.tsx to cover the newly included mock fields: verify the user-visible type label (e.g., assert "Incident" appears) and add expectations for parentCase, conversation, closedOn, closedBy, closeNotes, and hasAutoClosed as applicable in the five existing test cases; locate the test file and the CaseDetailsContent render calls and add checks using the component's visible text or aria roles (e.g., screen.getByText / queryByText) to assert presence or absence of those fields depending on the scenario so the mock values are actually asserted.apps/customer-portal/webapp/src/utils/projectDetails.ts (1)
39-47: Consider using constants for time card state values.Other color-mapping functions in this file (e.g.,
getDeploymentStatusColor,getSLAStatusColor) reference constants from@constants/projectDetailsConstantsrather than inline string literals. Using constants for"approved","submitted","rejected", and"draft"would improve consistency and make state values easier to refactor or search for.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/utils/projectDetails.ts` around lines 39 - 47, Replace the inline string literals in getTimeCardStateColorPath with the corresponding constants from `@constants/projectDetailsConstants` (e.g., use the exported time card state constants such as TIME_CARD_STATE_APPROVED, TIME_CARD_STATE_SUBMITTED, TIME_CARD_STATE_REJECTED, TIME_CARD_STATE_DRAFT), update the import at the top of the file to import those constants, and compare normalized to those constants instead of hardcoded strings so the color mappings remain consistent with getDeploymentStatusColor and getSLAStatusColor.apps/customer-portal/webapp/src/components/project-details/time-tracking/TimeCardsDateFilter.tsx (1)
88-101: No date range validation — "From" can exceed "To".There's no constraint preventing the start date from being set after the end date (or vice versa). Consider setting
inputProps={{ max: endDate }}on the start field andinputProps={{ min: startDate }}on the end field to provide native browser validation, or validate in the parent component.🛡️ Proposed fix to add date constraints
<TextField type="date" size="small" value={startDate} onChange={(e) => onStartDateChange(e.target.value)} sx={{ minWidth: 200 }} + inputProps={{ max: endDate || undefined }} InputProps={{ startAdornment: ( <InputAdornment position="start"> <Calendar size={16} /> </InputAdornment> ), }} /><TextField type="date" size="small" value={endDate} onChange={(e) => onEndDateChange(e.target.value)} sx={{ minWidth: 200 }} + inputProps={{ min: startDate || undefined }} InputProps={{ startAdornment: ( <InputAdornment position="start"> <Calendar size={16} /> </InputAdornment> ), }} />Also applies to: 111-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/time-tracking/TimeCardsDateFilter.tsx` around lines 88 - 101, The start/end date TextField controls (the one using startDate with onStartDateChange and the corresponding endDate with onEndDateChange in TimeCardsDateFilter) lack validation to prevent From > To; add native browser constraints by supplying inputProps={{ max: endDate }} on the start TextField and inputProps={{ min: startDate }} on the end TextField, and also add a defensive check in the change handlers (onStartDateChange/onEndDateChange) to clamp or reject values that would invert the range so parent state cannot become invalid.apps/customer-portal/webapp/src/models/responses.ts (1)
253-257: Good introduction ofIdLabelReffor reusable id/label references.This centralizes the common
{ id: string; label: string }pattern. Note that several inline occurrences remain in the file (e.g.,TimeCard.approvedBy,TimeCard.project,CaseListItem.project, etc.) that could also useIdLabelReffor consistency when convenient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/models/responses.ts` around lines 253 - 257, Several interfaces duplicate the { id: string; label: string } shape instead of reusing the new IdLabelRef; update each inline occurrence (e.g., TimeCard.approvedBy, TimeCard.project, CaseListItem.project, and any other fields with the same shape) to reference the IdLabelRef interface, adjusting their type annotations to IdLabelRef and removing the duplicated inline type definitions so the common IdLabelRef is used consistently across the file.apps/customer-portal/webapp/src/api/useGetProducts.ts (1)
40-43: Inconsistent export style: named export here vs default export inuseSearchProjectTimeCards.This hook uses a named export (
export function useGetProducts) whileuseSearchProjectTimeCardsusesexport default function. For consistency across the API hooks layer, consider aligning on one export style.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/api/useGetProducts.ts` around lines 40 - 43, The export style is inconsistent between hooks: useGetProducts is a named export but useSearchProjectTimeCards is a default export; update useGetProducts to match the established pattern by changing its declaration to a default export (i.e., export default function useGetProducts(...)) so other imports remain consistent, and update any import sites that relied on the named export if present; alternatively, if the project prefers named exports, change useSearchProjectTimeCards to a named export—ensure both hooks (useGetProducts and useSearchProjectTimeCards) use the same export style.apps/customer-portal/webapp/src/models/requests.ts (1)
113-115: Inlinepaginationtypes duplicatePaginationRequest— reuse the existing typeBoth
ProductVersionsSearchRequestandTimeCardSearchRequestinline{ limit?: number; offset?: number }directly, which is the exact shape already exported asPaginationRequest.♻️ Proposed refactor
// Request body for POST /products/:productId/versions/search. export interface ProductVersionsSearchRequest { - pagination?: { limit?: number; offset?: number }; + pagination?: PaginationRequest; }// Request body for project time cards search (POST /projects/:projectId/time-cards/search). export interface TimeCardSearchRequest { filters?: { startDate?: string; endDate?: string; }; - pagination?: { - limit?: number; - offset?: number; - }; + pagination?: PaginationRequest; }Also applies to: 156-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/models/requests.ts` around lines 113 - 115, ProductVersionsSearchRequest and TimeCardSearchRequest currently inline the pagination shape; replace those inline types with the exported PaginationRequest type (i.e., change pagination?: { limit?: number; offset?: number } to pagination?: PaginationRequest) so both interfaces reuse the existing PaginationRequest definition; update any related references within the same module to use PaginationRequest by name (ensure PaginationRequest is exported/visible in this file).apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/ProjectDeployments.test.tsx (1)
107-113: SharedQueryClientacross tests may cause cache pollution.The
queryClientis created once at module scope and reused across all tests. Cached data from one test can leak into subsequent tests. Consider creating a freshQueryClientinbeforeEachor withinrenderWithProviders.Suggested fix
-const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); - function renderWithProviders(ui: ReactElement) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); return render( <QueryClientProvider client={queryClient}>{ui}</QueryClientProvider>, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/ProjectDeployments.test.tsx` around lines 107 - 113, The test suite currently reuses a module-scoped QueryClient (queryClient) which can leak cache between tests; change renderWithProviders (or add a beforeEach) to create and use a fresh QueryClient instance for each test and pass that instance into QueryClientProvider instead of the shared queryClient (reference QueryClient, queryClient, renderWithProviders, QueryClientProvider); ensure defaultOptions (queries.retry = false) are preserved on the new instance so each test starts with an isolated cache.apps/customer-portal/webapp/src/components/project-details/time-tracking/__tests__/TimeCardsDateFilter.test.tsx (1)
21-56: Consider adding interaction tests for date input changes.The current tests only verify static rendering. Consider adding a test that simulates changing the start/end date inputs and verifies
onStartDateChange/onEndDateChangecallbacks are invoked with the correct values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/time-tracking/__tests__/TimeCardsDateFilter.test.tsx` around lines 21 - 56, Add an interaction test in TimeCardsDateFilter.test.tsx that mounts TimeCardsDateFilter with jest.fn() mocks for onStartDateChange and onEndDateChange, then use userEvent (imported from '@testing-library/user-event') to change the start and end date inputs (query them by label text or role) to new ISO date strings and assert the corresponding mock callbacks (onStartDateChange, onEndDateChange) were called with the new values; ensure you render the component with initial startDate/endDate and check the callbacks are invoked exactly as expected.apps/customer-portal/webapp/src/components/project-details/time-tracking/ProjectTimeTracking.tsx (1)
74-83: No pagination beyond 50 records.
limitis hardcoded to 50 withoffset: 0, and whiletotalRecordsis surfaced in the filter's "Showing X of Y" text, there's no mechanism to load additional records. If a project has more than 50 time cards in the date range, users will see a truncated list with no way to access the rest.Consider adding a "Load more" button or pagination controls, even if deferred to a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/time-tracking/ProjectTimeTracking.tsx` around lines 74 - 83, The current useSearchProjectTimeCards call hardcodes limit: 50 and offset: 0 causing the list to stop at 50 items; surface totalRecords but provide no way to fetch more. Update the component to support pagination or "Load more" by making limit and offset stateful (e.g., local state page/offset or nextOffset) and pass them into useSearchProjectTimeCards, render a "Load more" button when timeCards.length < totalRecords that increments offset (or page) to fetch the next page, and append new timeCards to the existing list from timeCardsData instead of always replacing; reference useSearchProjectTimeCards, timeCardsData, totalRecords, limit and offset when locating where to change.apps/customer-portal/webapp/src/components/project-details/time-tracking/__tests__/TimeTrackingCard.test.tsx (1)
51-63: Missing assertion fortotalTime: 0rendering.The incomplete card sets
totalTime: 0, which the component renders as"0h". Consider adding an assertion to verify this edge case is handled correctly, since it validates that zero isn't treated as a falsy/missing value.Suggested assertion
render(<TimeTrackingCard card={incompleteCard} />); expect(screen.getByText(/Approved by: --/)).toBeInTheDocument(); + expect(screen.getByText("0h")).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/time-tracking/__tests__/TimeTrackingCard.test.tsx` around lines 51 - 63, The test "should show fallback '--' for missing values" creates incompleteCard with totalTime: 0 but doesn't assert its rendering; update the test in TimeTrackingCard.test.tsx to also verify that the TimeTrackingCard component renders the zero value (e.g., "0h") for totalTime so zero isn't treated as missing. Locate the test using the describe/it block for TimeTrackingCard and the incompleteCard variable and add an assertion using screen.getByText or similar to check the "0h" output.apps/customer-portal/webapp/src/api/usePatchDeploymentProduct.ts (1)
64-68: Stale closure:isSignedIn/isAuthLoadingare captured at render time.The auth checks inside
mutationFnreference values from the enclosing render scope. If the user's auth state changes between render and mutation invocation, these stale values may produce incorrect results. The same pattern exists inusePostDeploymentProduct, so this is consistent with the codebase, but worth noting.A more robust approach would be to check auth state at call time (e.g., via the Asgardeo SDK's async methods) or rely on the authenticated fetch function (
fetchFn) to reject unauthorized requests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/api/usePatchDeploymentProduct.ts` around lines 64 - 68, The mutationFn inside usePatchDeploymentProduct currently reads isSignedIn/isAuthLoading from the render scope, which can be stale; update mutationFn to perform auth checks at call time instead of using those captured variables—either call the Asgardeo SDK async method (e.g., getAccessToken/getAuthenticatedUser or an isAuthenticated async helper) inside mutationFn before proceeding, or delegate auth enforcement to the provided fetchFn (ensure fetchFn rejects when unauthorized) so mutationFn does not rely on render-scoped isSignedIn/isAuthLoading; apply the same fix pattern to usePostDeploymentProduct to avoid stale closures.apps/customer-portal/webapp/src/components/project-details/deployments/AddProductModal.tsx (2)
110-114:handleTextChangeis recreated on every render.Unlike
handleProductChangeandhandleVersionChangewhich are wrapped inuseCallback,handleTextChangeis a plain function that returns a new handler on each render. This is a minor inconsistency.Optional: wrap in useCallback
- const handleTextChange = - (field: "cores" | "tps") => - (event: ChangeEvent<HTMLInputElement>) => { - setForm((prev) => ({ ...prev, [field]: event.target.value })); - }; + const handleTextChange = useCallback( + (field: "cores" | "tps") => + (event: ChangeEvent<HTMLInputElement>) => { + setForm((prev) => ({ ...prev, [field]: event.target.value })); + }, + [], + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/AddProductModal.tsx` around lines 110 - 114, handleTextChange is recreated on every render while handleProductChange and handleVersionChange use useCallback; wrap handleTextChange in useCallback to memoize the returned handler and match the other handlers. Specifically, convert handleTextChange into a useCallback that returns the (field: "cores" | "tps") => (event: ChangeEvent<HTMLInputElement>) => { setForm(prev => ({ ...prev, [field]: event.target.value })); } and include appropriate dependencies (e.g., [setForm]) so the callback is stable across renders.
289-328: Disabled placeholder fields for future features.The "Description" and "Initial Update Information" sections are entirely disabled with no functional purpose. This is fine as scaffolding for future work, but consider adding a brief comment or TODO to clarify intent, so future contributors know these are intentionally non-functional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/AddProductModal.tsx` around lines 289 - 328, Add a short inline comment or TODO above the disabled "Description" and "Initial Update Information" UI blocks in AddProductModal.tsx to indicate these TextField controls (ids "product-update-level" and "product-applied-on" and the surrounding description block) are intentionally scaffolding for future features; reference the section header text ("Initial Update Information" and the Description heading) and include a brief note about why they're disabled and when they will become active so future contributors understand intent.apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentCard.tsx (1)
131-134: Use the already-computedprojectIdvariable instead of re-evaluating the same expression.
projectIdis extracted at line 56 (const projectId = deployment.project?.id ?? ""), but the same expression is duplicated on line 133. Use the existing variable for consistency.♻️ Proposed fix
- <DeploymentProductList - deploymentId={deployment.id} - projectId={deployment.project?.id ?? ""} - /> + <DeploymentProductList + deploymentId={deployment.id} + projectId={projectId} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentCard.tsx` around lines 131 - 134, The DeploymentCard component already computes const projectId = deployment.project?.id ?? "" but the DeploymentProductList prop is re-evaluating deployment.project?.id ?? "" again; replace the duplicated expression with the existing projectId variable (i.e., pass projectId to DeploymentProductList along with deploymentId) to ensure consistency and avoid redundant computation in the DeploymentProductList JSX usage.apps/customer-portal/webapp/src/api/useGetDeploymentDocuments.ts (1)
24-35: Optional:normalizeDocumentscasts array items without runtime shape validation.All three branches (
raw as DeploymentDocument[],.attachments,.documents) perform unchecked casts. If the API returns a non-conforming shape, callers silently receive malformed objects with no runtime error. This is consistent with other hooks in the codebase, but worth noting as a future improvement point if a lightweight schema validator (e.g.,zod) is ever adopted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/api/useGetDeploymentDocuments.ts` around lines 24 - 35, normalizeDocuments currently performs unchecked casts (raw as DeploymentDocument[] and trusting .attachments/.documents) so malformed API responses can slip through; update normalizeDocuments to validate the runtime shape of each item before returning: keep the existing branching (Array, attachments, documents) but for each candidate array run a simple validation that each entry is an object with the expected keys/types for DeploymentDocument (e.g., check typeof on string/number fields and presence of required properties) and return only items that pass (or return [] if none); reference the function name normalizeDocuments and the DeploymentDocument type when implementing the per-item guard so callers receive only validated objects.apps/customer-portal/webapp/src/api/usePatchDeployment.ts (1)
97-104: Add"project-deployments"toApiQueryKeysfor consistency.The string
"project-deployments"is a functional query key used byuseGetProjectDeploymentsto fetch the project's deployments list. However, it is defined as a raw string literal here while all other query keys in the codebase are managed through theApiQueryKeysconstant. This inconsistency reduces discoverability and increases maintenance burden.Add
PROJECT_DEPLOYMENTS: "project-deployments"toApiQueryKeysinapps/customer-portal/webapp/src/constants/apiConstants.ts(alongside existing keys likePROJECT_DETAILS,PROJECT_CONTACTS, etc.), then reference it instead of the raw string inusePatchDeployment.tsline 99,usePostCreateDeployment.tsline 87, anduseGetProjectDeployments.tsline 38.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/api/usePatchDeployment.ts` around lines 97 - 104, Add a new constant PROJECT_DEPLOYMENTS: "project-deployments" to the ApiQueryKeys object (symbol: ApiQueryKeys) and replace raw string usages with the constant: change the literal "project-deployments" in usePatchDeployment (onSuccess), usePostCreateDeployment, and useGetProjectDeployments to reference ApiQueryKeys.PROJECT_DEPLOYMENTS so all query keys are centralized and consistent.apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentDocumentList.tsx (2)
58-63: Redundant query invalidation — hook already handles it.
handleAddSuccessmanually invalidatesDEPLOYMENT_ATTACHMENTS, butusePostDeploymentAttachmentalready does the same in its ownonSuccess(seeusePostDeploymentAttachment.tsLines 105–108). The double invalidation is harmless but unnecessary.Proposed simplification
const handleAddSuccess = () => { setIsAddModalOpen(false); - queryClient.invalidateQueries({ - queryKey: [ApiQueryKeys.DEPLOYMENT_ATTACHMENTS, deploymentId], - }); };If keeping only the hook's built-in invalidation is sufficient, you can also drop the
useQueryClientimport andqueryClientvariable entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentDocumentList.tsx` around lines 58 - 63, The handleAddSuccess function redundantly calls queryClient.invalidateQueries for ApiQueryKeys.DEPLOYMENT_ATTACHMENTS even though usePostDeploymentAttachment already invalidates that key; update handleAddSuccess to only close the modal by calling setIsAddModalOpen(false) and remove the manual invalidateQueries call, and then remove the now-unused useQueryClient import and queryClient variable from DeploymentDocumentList; keep usePostDeploymentAttachment's onSuccess as the single source of truth for invalidation.
160-175: Consider sanitizingdownloadUrlbefore using it as anhref.If
doc.downloadUrlever contains ajavascript:ordata:URI, it becomes an XSS vector. A quick guard ensures only safe protocols are used:Proposed guard
+ const isSafeUrl = doc.downloadUrl?.startsWith("https://") || doc.downloadUrl?.startsWith("http://"); - {doc.downloadUrl ? ( + {doc.downloadUrl && isSafeUrl ? ( <Button component="a" href={doc.downloadUrl}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentDocumentList.tsx` around lines 160 - 175, The Download button uses doc.downloadUrl directly which can open javascript: or data: URIs; validate/sanitize it in DeploymentDocumentList before assigning to href by building a safeUrl variable (e.g., try new URL(doc.downloadUrl, window.location.href) or check protocol) and only allow safe protocols such as http:, https:, blob:, or same-origin relative paths; if validation fails set safeUrl to null and render the disabled Button variant. Replace direct uses of doc.downloadUrl with safeUrl (and preserve target/rel) so untrusted URIs are never injected into the href.apps/customer-portal/webapp/src/components/project-details/deployments/EditDeploymentModal.tsx (2)
88-97: Double form reset on close — minor redundancy.
handleClose(Line 89) resets the form, and theuseEffecton!open(Line 95) also resets it. When the modal closes viahandleClose, both paths execute. This is harmless but could be simplified by relying on only one mechanism.Also applies to: 93-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/EditDeploymentModal.tsx` around lines 88 - 97, The form reset happens twice on close: handleClose calls setForm(INITIAL_FORM) and the useEffect watching open also resets the form when open becomes false; remove the redundancy by choosing one mechanism — either keep handleClose (remove the setForm call inside the useEffect) or keep the useEffect (remove setForm from handleClose). Update references: adjust handleClose, the useEffect that depends on open, and ensure setForm(INITIAL_FORM) remains called exactly once via either handleClose or the useEffect, preserving onClose() behavior.
197-208: Required field label uses manual*instead of therequiredprop.The "Deployment Name *" label manually appends an asterisk. Using MUI's
requiredprop onTextFieldadds built-in required-field semantics (accessibility, native validation). Same applies to "Deployment Type *" at Line 223.Proposed fix
<TextField id="edit-deployment-name" - label="Deployment Name *" + label="Deployment Name" + required placeholder="e.g., Production US-East"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/EditDeploymentModal.tsx` around lines 197 - 208, Replace the manual asterisk in the TextField labels with MUI's required prop: for the TextField with id "edit-deployment-name" (value tied to form.name and onChange via handleTextChange("name")) remove the " *" from the label and add required={true} (or required) so the field gets proper semantics and native validation; do the same for the "Deployment Type" TextField (the other TextField using handleTextChange for type) so both fields use the required prop instead of a manual "*" in the label.apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/AddProductModal.test.tsx (1)
29-31: SharedQueryClientcache not cleared between tests — potential for flaky results.The
queryClientis module-scoped and never reset. Cached queries from one test can leak into the next. Add aqueryClient.clear()inbeforeEach:Proposed fix
beforeEach(() => { vi.clearAllMocks(); + queryClient.clear(); vi.mocked(useGetProducts).mockReturnValue({Also applies to: 58-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/AddProductModal.test.tsx` around lines 29 - 31, The module-scoped QueryClient instance (queryClient) is shared across tests causing cache leakage; update the tests to clear or recreate it before each test by adding a beforeEach that calls queryClient.clear() (or re-initializes a new QueryClient with the same defaultOptions) so cached queries do not persist between runs; apply this change around the tests referencing queryClient (including the other occurrence noted) to ensure isolation.apps/customer-portal/webapp/src/components/project-details/deployments/ManageProductModal.tsx (1)
240-247: "Save Changes" is never disabled — allows no-op submissions.Unlike
EditDeploymentModalwhich disables the submit button via!isValid, this modal's Save button is always enabled (when not submitting). Consider disabling it when nothing has changed or whenproduct?.idis missing (thoughproductbeing null returnsnullat Line 125, so this is a lesser concern).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/ManageProductModal.tsx` around lines 240 - 247, The Save button in ManageProductModal is always enabled and allows no-op submissions; update the <Button> props so it is disabled when there are no changes or required data is missing: pass a disabled prop that checks either the form's dirty/valid flags (e.g., formik.dirty === false || !formik.isValid) or a local hasChanges boolean you set by diffing initial values vs current form values, and also include a check for missing product?.id (e.g., disabled={!hasChanges || !product?.id || isSubmitting}). Ensure handleSave remains unchanged and use the form manager (Formik or React Hook Form) flags where available to avoid adding extra state if possible.apps/customer-portal/webapp/src/components/support/case-details/attachments-tab/UploadAttachmentModal.tsx (1)
169-188: NoonErrorcallback onmutate— mutation failures are swallowed.Neither the deployment nor case
mutatecalls supply anonErrorhandler. If the API call fails, the user sees no feedback (the spinner disappears silently). Consider surfacing errors via a toast/alert or at minimum resetting the pending state visually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/support/case-details/attachments-tab/UploadAttachmentModal.tsx` around lines 169 - 188, Both postDeploymentAttachment.mutate and postAttachments.mutate are missing onError handlers so failures are swallowed; add an onError callback to each mutate call that resets any pending/loading state (e.g., setPending(false) or similar), prevents calling handleClose/onSuccess, and surfaces the error to the user via the app's notification/toast/alert utility (use your existing toast/snackbar helper) while including the error message/details for debugging.
Multiple small improvements across deployment, time-tracking, attachments, tests and models: - DeploymentDocumentList: disable Delete button and add title tooltip indicating deletion isn't implemented yet. - AddProductModal.test: use RTL's render wrapper pattern to provide QueryClientProvider. - DeploymentDocumentList.test: add a test to assert loading state shows a progress indicator. - EditDeploymentModal.test: wrap the Update button click in act to avoid async test timing issues. - TimeCardsDateFilter: add htmlFor/id attributes to date inputs for accessibility. - TimeTrackingCard: fall back to "--" using || instead of nullish coalescing for label. - UploadAttachmentModal: add read error state, reset it on clear, handle FileReader.onerror, show an error Alert, and switch to use isDeploymentMode when posting deployment attachments (pass deploymentId explicitly). - models/responses: make CaseDetails.conversation non-nullable (removed | null). These changes improve UX/error handling, accessibility, and test reliability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/AddProductModal.test.tsx (1)
29-31: Consider creating a freshQueryClientper test.The current module-level singleton is safe here because all hooks are fully mocked, but it's a fragile setup — any future test that exercises a real query could pollute the shared cache.
♻️ Suggested refactor
-const queryClient = new QueryClient({ - defaultOptions: { queries: { retry: false } }, -}); +let queryClient: QueryClient; ... beforeEach(() => { + queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); vi.clearAllMocks(); ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/AddProductModal.test.tsx` around lines 29 - 31, Replace the module-level QueryClient singleton with a fresh instance per test to avoid shared cache pollution: move the creation of queryClient (the QueryClient({ defaultOptions: { queries: { retry: false } }})) into a test setup hook (e.g., beforeEach) or a factory function used in each test, and update tests to use that per-test queryClient when rendering components or wrapping hooks (look for usages in AddProductModal.test.tsx and the queryClient variable) so each test gets an isolated cache.apps/customer-portal/webapp/src/models/responses.ts (2)
510-515:ProductItemhas bothlabel?andname?as optional — clarify which the API returns.Having both optional means consumers must always check both (
item.label ?? item.name). If the API consistently returns one or the other, consider making the canonical field required or providing a normalized accessor. At minimum, document which field the current API version returns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/models/responses.ts` around lines 510 - 515, ProductItem currently defines both label? and name? as optional which forces callers to check both; update the model to reflect the actual API shape by making the canonical field required (e.g., change either label or name to non-optional) or add a normalized accessor/derived property (e.g., getDisplayName or a computed field) and document which field the API returns in the ProductItem comment; modify the ProductItem interface and any consumers of ProductItem (search for ProductItem usages) to use the chosen canonical property or accessor.
464-475:DeploymentDocument— many optional dual-name fields increase fragility.Fields like
sizeBytes/size,uploadedAt/createdOn,uploadedBy/createdByeach have two optional variants for the same concept. The component code compensates withdoc.sizeBytes ?? doc.size, but this pushes API normalization into every consumer. Consider normalizing in the API hook or a transformer so the interface has a single canonical name per field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/models/responses.ts` around lines 464 - 475, The DeploymentDocument interface exposes duplicate optional names (sizeBytes/size, uploadedAt/createdOn, uploadedBy/createdBy); instead add a single canonical set of fields on DeploymentDocument (e.g., size, uploadedAt, uploadedBy) and normalize incoming API payloads in the API hook/transformer that returns DeploymentDocument (create a small helper like normalizeDeploymentDocument or transformDeploymentDocument called from fetchDeployments/useFetchDeployments) that maps sizeBytes→size, createdOn→uploadedAt, createdBy→uploadedBy (falling back if only the alternate exists), then update the hook return type to the normalized DeploymentDocument so consumers no longer need doc.sizeBytes ?? doc.size.apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/DeploymentDocumentList.test.tsx (1)
86-94: SharedQueryClientacross tests can leak cache state.The
queryClientis created once at module scope (Line 86) and reused across all tests. If any test triggers cache writes or invalidations, it could affect subsequent tests non-deterministically.Move the
QueryClientcreation intorenderWithProvidersor abeforeEachblock:Proposed fix
-const queryClient = new QueryClient({ - defaultOptions: { queries: { retry: false } }, -}); - function renderWithProviders(ui: ReactElement) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); return render( <QueryClientProvider client={queryClient}>{ui}</QueryClientProvider>, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/DeploymentDocumentList.test.tsx` around lines 86 - 94, The tests reuse a module-scoped QueryClient (queryClient) which can leak cache state; change to create a fresh QueryClient per test by moving the QueryClient instantiation into renderWithProviders (or into a beforeEach that sets a new queryClient) and pass that instance to QueryClientProvider; update renderWithProviders to create a new QueryClient() each call (or reference the per-test variable) and ensure any defaultOptions (queries.retry = false) are preserved so tests remain deterministic.apps/customer-portal/webapp/src/components/project-details/time-tracking/TimeTrackingCard.tsx (1)
119-121:totalTimenull/undefined guard is over-defensive for its type.
TimeCard.totalTimeis typed asnumber(non-nullable), so the!== undefined && !== nullcheck will always pass. If0his a valid display value, this is fine. If the intent is to guard against missing data, the type should benumber | null.Consider aligning the type with the runtime expectation — either make
totalTimenullable in the interface or simplify the guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/time-tracking/TimeTrackingCard.tsx` around lines 119 - 121, The runtime guard around totalTime is redundant because TimeCard.totalTime is declared as a non-nullable number; decide whether missing data is expected and then either (A) update the type to number | null (or number | undefined) in the TimeCard/props interface and keep the existing conditional render, or (B) simplify the JSX in TimeTrackingCard.tsx to render `${totalTime}h` directly (or render 0h when totalTime is 0) and remove the null/undefined checks; locate references to TimeCard.totalTime and the TimeTrackingCard component to update the prop type or adjust the render accordingly so type and runtime behavior are aligned.apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentDocumentList.tsx (2)
58-63: Redundant query invalidation —usePostDeploymentAttachmentalready invalidatesDEPLOYMENT_ATTACHMENTS.
handleAddSuccessmanually invalidates[DEPLOYMENT_ATTACHMENTS, deploymentId], but theusePostDeploymentAttachmenthook'sonSuccesscallback (inusePostDeploymentAttachment.ts) already does the same invalidation. React Query deduplicates, so this is harmless, but it's unnecessary code that may cause confusion about which layer owns cache invalidation.Consider removing the manual invalidation and keeping only
setIsAddModalOpen(false)inhandleAddSuccess:Proposed simplification
const handleAddSuccess = () => { setIsAddModalOpen(false); - queryClient.invalidateQueries({ - queryKey: [ApiQueryKeys.DEPLOYMENT_ATTACHMENTS, deploymentId], - }); };If you do this, the
queryClientimport anduseQueryClient()call (Line 34, 50) can also be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentDocumentList.tsx` around lines 58 - 63, handleAddSuccess currently calls queryClient.invalidateQueries for [ApiQueryKeys.DEPLOYMENT_ATTACHMENTS, deploymentId] even though usePostDeploymentAttachment already performs that invalidation; remove the redundant invalidateQueries call so handleAddSuccess only calls setIsAddModalOpen(false), and then delete the now-unused queryClient variable and the useQueryClient() invocation plus its import to avoid dead code; keep usePostDeploymentAttachment's onSuccess as the single place doing the DEPLOYMENT_ATTACHMENTS invalidation.
160-175: Download link opens in a new tab without user-controlled sanitization.
doc.downloadUrlis used directly ashref. If the URL comes from user-uploaded metadata or an untrusted API, this could be a vector forjavascript:protocol URLs. Therel="noopener noreferrer"is present (good), but consider validating that the URL useshttps:orhttp:before rendering the anchor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentDocumentList.tsx` around lines 160 - 175, doc.downloadUrl is rendered directly into the anchor href, which can allow unsafe schemes like javascript:; before using doc.downloadUrl in the Button component (the JSX block that checks {doc.downloadUrl ? ...}), validate/sanitize the URL: attempt to construct a new URL(doc.downloadUrl) in a try/catch and ensure url.protocol is "http:" or "https:" (or otherwise whitelist allowed schemes); if validation fails, render the disabled Button variant (the existing disabled branch) instead of using the untrusted href, so only safe http/https links are opened in a new tab.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/AddProductModal.test.tsx`:
- Around line 163-176: The test in AddProductModal.test.tsx verifies
mockMutateAsync and mockOnSuccess on successful submit but doesn't assert that
the modal is closed; add an assertion that mockOnClose (the onClose prop/mock
used when rendering the AddProductModal) is called after the waitFor that checks
mockMutateAsync and mockOnSuccess. Ensure you reference the same mocks used in
the test (mockMutateAsync, mockOnSuccess, mockOnClose) and place the mockOnClose
assertion immediately after the existing success assertions so the test confirms
the component triggers onClose on successful submission.
- Around line 203-204: The test is clicking the Add Product submit button
immediately which can be flaky; before calling fireEvent.click on the element
obtained as submitButton (via screen.getByRole("button", { name: "Add Product"
})), add a waitFor that asserts the button is enabled (e.g., waitFor(() =>
expect(submitButton).toBeEnabled())) so React state updates complete and
mockMutateAsync is invoked reliably before the click and subsequent waitFor
assertions.
---
Nitpick comments:
In
`@apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/AddProductModal.test.tsx`:
- Around line 29-31: Replace the module-level QueryClient singleton with a fresh
instance per test to avoid shared cache pollution: move the creation of
queryClient (the QueryClient({ defaultOptions: { queries: { retry: false } }}))
into a test setup hook (e.g., beforeEach) or a factory function used in each
test, and update tests to use that per-test queryClient when rendering
components or wrapping hooks (look for usages in AddProductModal.test.tsx and
the queryClient variable) so each test gets an isolated cache.
In
`@apps/customer-portal/webapp/src/components/project-details/deployments/__tests__/DeploymentDocumentList.test.tsx`:
- Around line 86-94: The tests reuse a module-scoped QueryClient (queryClient)
which can leak cache state; change to create a fresh QueryClient per test by
moving the QueryClient instantiation into renderWithProviders (or into a
beforeEach that sets a new queryClient) and pass that instance to
QueryClientProvider; update renderWithProviders to create a new QueryClient()
each call (or reference the per-test variable) and ensure any defaultOptions
(queries.retry = false) are preserved so tests remain deterministic.
In
`@apps/customer-portal/webapp/src/components/project-details/deployments/DeploymentDocumentList.tsx`:
- Around line 58-63: handleAddSuccess currently calls
queryClient.invalidateQueries for [ApiQueryKeys.DEPLOYMENT_ATTACHMENTS,
deploymentId] even though usePostDeploymentAttachment already performs that
invalidation; remove the redundant invalidateQueries call so handleAddSuccess
only calls setIsAddModalOpen(false), and then delete the now-unused queryClient
variable and the useQueryClient() invocation plus its import to avoid dead code;
keep usePostDeploymentAttachment's onSuccess as the single place doing the
DEPLOYMENT_ATTACHMENTS invalidation.
- Around line 160-175: doc.downloadUrl is rendered directly into the anchor
href, which can allow unsafe schemes like javascript:; before using
doc.downloadUrl in the Button component (the JSX block that checks
{doc.downloadUrl ? ...}), validate/sanitize the URL: attempt to construct a new
URL(doc.downloadUrl) in a try/catch and ensure url.protocol is "http:" or
"https:" (or otherwise whitelist allowed schemes); if validation fails, render
the disabled Button variant (the existing disabled branch) instead of using the
untrusted href, so only safe http/https links are opened in a new tab.
In
`@apps/customer-portal/webapp/src/components/project-details/time-tracking/TimeTrackingCard.tsx`:
- Around line 119-121: The runtime guard around totalTime is redundant because
TimeCard.totalTime is declared as a non-nullable number; decide whether missing
data is expected and then either (A) update the type to number | null (or number
| undefined) in the TimeCard/props interface and keep the existing conditional
render, or (B) simplify the JSX in TimeTrackingCard.tsx to render
`${totalTime}h` directly (or render 0h when totalTime is 0) and remove the
null/undefined checks; locate references to TimeCard.totalTime and the
TimeTrackingCard component to update the prop type or adjust the render
accordingly so type and runtime behavior are aligned.
In `@apps/customer-portal/webapp/src/models/responses.ts`:
- Around line 510-515: ProductItem currently defines both label? and name? as
optional which forces callers to check both; update the model to reflect the
actual API shape by making the canonical field required (e.g., change either
label or name to non-optional) or add a normalized accessor/derived property
(e.g., getDisplayName or a computed field) and document which field the API
returns in the ProductItem comment; modify the ProductItem interface and any
consumers of ProductItem (search for ProductItem usages) to use the chosen
canonical property or accessor.
- Around line 464-475: The DeploymentDocument interface exposes duplicate
optional names (sizeBytes/size, uploadedAt/createdOn, uploadedBy/createdBy);
instead add a single canonical set of fields on DeploymentDocument (e.g., size,
uploadedAt, uploadedBy) and normalize incoming API payloads in the API
hook/transformer that returns DeploymentDocument (create a small helper like
normalizeDeploymentDocument or transformDeploymentDocument called from
fetchDeployments/useFetchDeployments) that maps sizeBytes→size,
createdOn→uploadedAt, createdBy→uploadedBy (falling back if only the alternate
exists), then update the hook return type to the normalized DeploymentDocument
so consumers no longer need doc.sizeBytes ?? doc.size.
5011641
into
wso2-open-operations:customer-portal-milestone-1
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Improvements