feat(DEV-16185): add purchase order management components and functionality #829
Conversation
costa-monite
requested review from
caiomachado-monite,
dinis-monite and
marcperezmonite
🦋 Changeset detectedLatest commit: 457ef31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds Purchase Orders support across SDK React: new components, hooks, validation, tables, dialogs, email/preview flows, and integration into Payables with tabs. Introduces shared ItemsSection framework, currency/VAT utilities, mocks, and extensive tests (unit and Playwright). Updates build/test configs and minor related components. Changes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 64
🧹 Nitpick comments (105)
packages/sdk-react/src/api/client.test.ts (1)
29-37: DRY the positive cases with a table-driven test (and cover trailing slash)Simplifies maintenance and adds one common variant.
- test('should match paths that are in the list of paths that require', () => { - expect(isMoniteEntityIdPath('/payable_purchase_orders')).toBe(true); - expect(isMoniteEntityIdPath('/payable_purchase_orders/123')).toBe(true); - expect(isMoniteEntityIdPath('/payable_purchase_orders/123/send')).toBe(true); - expect(isMoniteEntityIdPath('/foo')).toBe(true); - expect(isMoniteEntityIdPath('/bar')).toBe(true); - expect(isMoniteEntityIdPath('/foo/bar')).toBe(true); - expect(isMoniteEntityIdPath('/foo/bar/baz')).toBe(true); - }); + test.each([ + '/payable_purchase_orders', + '/payable_purchase_orders/', // trailing slash + '/payable_purchase_orders/123', + '/payable_purchase_orders/123/send', + '/foo', + '/bar', + '/foo/bar', + '/foo/bar/baz', + ])('should match paths that are in the list of paths that require: %s', (path) => { + expect(isMoniteEntityIdPath(path)).toBe(true); + });packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/InvoiceTotals.tsx (6)
1-1: Use a type-only import to avoid a runtime dependency.
componentsis used purely for typing; import it withimport typeto avoid pulling it into the bundle.-import { components } from '@/api'; +import type { components } from '@/api';
33-36: Prefer string (or numeric-string) keys for taxesByVatRate.Object keys are strings at runtime;
Record<number, number>is misleading and hurts inference aroundObject.entries. UseRecord<string, number>(orRecord<\${number}`, number>` if you want numeric strings).- taxesByVatRate?: Record<number, number>; + taxesByVatRate?: Record<string, number>;
63-67: Drop redundant .toString().
formatCurrencyToDisplayreturns a string;TotalTableItemalready callstoString()onvalue. The extra call is unnecessary.- )?.toString()} + )}
62-62: Unify tax labels for i18n consistency.Use the same wording for the aggregate as the per-rate lines.
- label={t(i18n)`Taxes total`} + label={t(i18n)`Total tax`}Also applies to: 73-74
50-56: Optional: consider shadcn/ui primitives for structure.Given the guidelines, you could wrap this block with Card/Separator from '@/ui/components/*' instead of manual borders. Current Tailwind is fine if you prefer to keep it minimal; add a TODO if you plan to migrate later.
58-71: Remove duplicate entries call, drop unnecessary optional chaining, and sort rates for stable UI.
?.lengthis redundant here.- You call
Object.entriestwice.- Sorting prevents flicker and makes snapshots stable.
- {Object.entries(taxesByVatRate)?.length > 0 ? ( - Object.entries(taxesByVatRate).map(([taxRate, totalTax]) => ( + {Object.entries(taxesByVatRate).length > 0 ? ( + Object.entries(taxesByVatRate) + .sort(([a], [b]) => Number(a) - Number(b)) + .map(([taxRate, totalTax]) => ( <TotalTableItem key={taxRate} label={t(i18n)`Total Tax (${taxRate}%)`} value={formatCurrencyToDisplay( totalTax, actualCurrency || defaultCurrency || 'USD', true - )?.toString()} + )} className="mtw:text-sm mtw:font-normal mtw:text-neutral-50" /> - )) + )) ) : (To double-check minor-unit assumptions for
totalTax, run:#!/bin/bash # Find where taxesByVatRate is computed to confirm units are minor (as required by formatCurrencyToDisplay). rg -nP --type=ts --type=tsx -C3 '\btaxesByVatRate\b'packages/sdk-react/src/components/approvalPolicies/ApprovalPolicyDetails/ApprovalPolicyDetailsFormAdvanced/ApprovalPolicyDetailsFormAdvanced.tsx (1)
196-201: MUI sx styling present—add a TODO to migrate to shadcn/ui + Tailwind when making larger changesThis file still uses MUI and sx. Given this PR only tweaks imports, keep as-is but mark intent to migrate per guidelines.
Add a TODO near the imports:
import { components } from '@/api'; @@ import { yupResolver } from '@hookform/resolvers/yup'; +// TODO(UI): Migrate this component to shadcn/ui + Tailwind utilities on next significant change. +// Keep MUI for now per guideline to avoid scope creep in minor touch-ups.Also applies to: 208-216, 226-231, 238-246
.changeset/healthy-candles-warn.md (1)
6-6: Title-case the release note for consistencyMinor copy nit to match UI/Docs capitalization.
-Purchase orders support in Payables +Purchase Orders support in Payablespackages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/hooks/useLineItemManagement.ts (2)
66-71: Avoid JSON.stringify-based deps; use RHF’s useWatch to eliminate perf overhead and lint disablesHashing on every keystroke is O(n) and forces extra renders.
useWatchgives you the field value and re-renders when it changes; you can drop the hash and eslint overrides.- const watchedLineItems = watch('line_items'); - const watchedLineItemsContentHash = JSON.stringify(watchedLineItems); + const watchedLineItems = useWatch({ name: 'line_items' }); const currentLineItems = useMemo( () => watchedLineItems ?? [], - // eslint-disable-next-line react-hooks/exhaustive-deps - [watchedLineItemsContentHash] + [watchedLineItems] ); const sanitizedLineItemsForTable = useMemo( () => sanitizeLineItems(currentLineItems), - // eslint-disable-next-line react-hooks/exhaustive-deps - [watchedLineItemsContentHash] + [currentLineItems] );Add the import (outside the shown hunk):
import { useFieldArray, useFormContext, useWatch, /* ... */ } from 'react-hook-form';Also applies to: 73-77
346-361: Stringifying errors is heavy; prefer useFormState scoped to line_itemsThis avoids deep serialization and unnecessary renders.
Example:
import { useFormState } from 'react-hook-form'; const { errors } = useFormState({ name: 'line_items' }); // then compute lineItemErrors from errors.line_items without JSON.stringifypackages/sdk-react/src/components/payables/types.ts (1)
35-47: Consider const object + string-literal union instead of TS enum to avoid runtime enum object in bundlesSmall footprint win for libraries; API remains ergonomic.
-export enum PayablesTabEnum { - Bills = 'bills', - PurchaseOrders = 'purchase-orders', -} - -export type PayablesTab = PayablesTabEnum; +export const PayablesTab = { + Bills: 'bills', + PurchaseOrders: 'purchase-orders', +} as const; +export type PayablesTab = typeof PayablesTab[keyof typeof PayablesTab];packages/sdk-react/config/rollup.config.mjs (1)
27-27: Inlining dynamic imports disables code-splitting; confirm bundle-size tradeoff is intended for both CJS and ESMFor libraries, inlining can be desirable (single-file) but increases size and removes lazy benefits. Consider making this configurable or only inlining for CJS.
Script to audit dynamic import usage and React.lazy call sites:
#!/bin/bash set -euo pipefail echo "Dynamic import() call sites:" rg -nP "import\\s*\\(" -g "packages/sdk-react/src/**" -S -C2 echo echo "React.lazy call sites:" rg -nP "React\\.lazy\\s*\\(" -g "packages/sdk-react/src/**" -S -C2Optional tweak to make inlining configurable:
- inlineDynamicImports: true, + inlineDynamicImports: options?.inlineDynamicImports ?? true,And pass options in the default export if desired:
export default rollupConfig( { main: packageJson.main, module: packageJson.module, types: packageJson.types }, { inlineDynamicImports: true } );Also applies to: 35-35
packages/sdk-react/src/ui/DataGridEmptyState/DataGridEmptyState.tsx (2)
17-17: Use type-only import for ReactNodeMinor tree-shaking/readability win.
-import { ReactNode } from 'react'; +import type { ReactNode } from 'react';
74-78: Deduplicate identical switch branchesBoth payables and purchase-orders share the same icon; combine via fall-through for brevity.
Example:
case 'no-data=payables': case 'no-data=purchase-orders': defaultIcon = <ReceiptLong sx={{ fontSize: '4rem', color: 'primary.main' }} />; break;packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderHeader.tsx (3)
43-50: Add accessible label to the settings buttonExpose intent for screen readers; also keeps consistent tooltip behavior elsewhere.
- <Button + <Button variant="outline" className="mtw:mr-2" disabled={isLoading} + aria-label={t(i18n)`Open settings`} >
70-77: Avoid type="submit" on a header buttonThis header isn’t guaranteed to be inside the target form; prefer an explicit click handler only to avoid unintended submissions in nested forms.
- <Button - variant="default" - type="submit" - disabled={isLoading} - onClick={onSave} - > + <Button variant="default" disabled={isLoading} onClick={onSave}>
37-38: Minor: safer className compositionTemplate string avoids accidental “undefined-Title …”.
- className={className + '-Title PurchaseOrder-Preview'} + className={`${className}-Title PurchaseOrder-Preview`}packages/sdk-react/src/components/shared/ItemsSection/GenericQuantityField.tsx (1)
61-86: Use existing MeasureUnitField instead of a raw selectYou’ll get consistent UX and default-assignment behavior; also removes inline styles.
- <InputAdornment position="end"> - <select - value={ - (onRequestLineItemValue('product.measure_unit_id') as string) || '' - } - onChange={(e) => { - onLineItemValueChange('product.measure_unit_id', e.target.value, { - shouldValidate: true, - }); - onLineItemManuallyChanged(); - }} - style={{ - border: 'none', - background: 'transparent', - fontSize: '14px', - padding: '2px', - }} - > - <option value="">{t(i18n)`Unit`}</option> - {measureUnitsData?.data?.map((unit) => ( - <option key={unit.id} value={unit.id}> - {unit.name} - </option> - ))} - </select> - </InputAdornment> + <div className="mtw:flex mtw:items-center"> + {/* Consider moving MeasureUnitField to shared if needed */} + {/* import { MeasureUnitField } from '@/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/MeasureUnitField'; */} + <MeasureUnitField + value={(onRequestLineItemValue('product.measure_unit_id') as string) ?? ''} + availableMeasureUnits={measureUnitsData?.data} + error={Boolean(measureUnitFieldError)} + disabled={disabled} + onChange={(val) => { + onLineItemValueChange('product.measure_unit_id', val, { shouldValidate: true }); + onLineItemManuallyChanged(); + }} + /> + </div>packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/ItemSelector.tsx (2)
367-390: Manual (non-catalog) branch works; minor UX polishConsider trimming leading/trailing spaces in the onBlur handler before emitting CUSTOM_ID to avoid accidental whitespace entries.
- if (customName && customName.trim() !== '') { + const trimmed = customName.trim(); + if (trimmed !== '') { - const isCustomName = !itemsAutocompleteData.some( - (item) => item.label === customName + const isCustomName = !itemsAutocompleteData.some( + (item) => item.label === trimmed ); if (isCustomName) { - onChange({ id: CUSTOM_ID, label: customName }, false); + onChange({ id: CUSTOM_ID, label: trimmed }, false); } }
558-563: Loading spinner condition duplicates in endAdornmentAutocomplete adds a spinner in endAdornment when loading is true. Since you now gate startAdornment by catalogEnabled, consider also gating loading prop or overriding endAdornment to avoid a double spinner when disabled is true.
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/utils.ts (1)
3-3: Consolidate rate conversion utilitiesWe now use currencies.ts here and vatUtils.ts elsewhere. To avoid drift, centralize VAT/currency minor/major conversions in one module and update imports uniformly.
packages/sdk-react/src/components/payables/PurchaseOrders/constants.ts (1)
7-8: Consider sourcing page size from a single UI config.If there’s a shared grid/table page-size setting, import it here to avoid drift.
packages/sdk-react/src/ui/components/tabs.tsx (1)
5-16: Consider forwardRef + displayName to align with shadcn patterns.Improves ref passing and DX; matches other ui components.
Apply this pattern (example for Tabs; replicate for others):
-import { type ComponentProps } from 'react'; +import { forwardRef, type ComponentProps } from 'react'; -function Tabs({ className, ...props }: ComponentProps<typeof TabsPrimitive.Root>) { - return ( - <TabsPrimitive.Root - data-slot="tabs" - className={cn('mtw:flex mtw:flex-col mtw:gap-2', className)} - {...props} - /> - ); -} +const Tabs = forwardRef< + React.ElementRef<typeof TabsPrimitive.Root>, + ComponentProps<typeof TabsPrimitive.Root> +>(({ className, ...props }, ref) => ( + <TabsPrimitive.Root + ref={ref} + data-slot="tabs" + className={cn('mtw:flex mtw:flex-col mtw:gap-2', className)} + {...props} + /> +)); +Tabs.displayName = 'Tabs';Also applies to: 18-32, 34-48, 50-61
packages/sdk-react/src/components/payables/PurchaseOrders/validation.ts (2)
106-118: Enforce min/max for valid_for_days using constants.Preprocess defaults nicely; add bounds to prevent out-of-range values.
- valid_for_days: z.preprocess( + valid_for_days: z.preprocess( (val) => { if (val === undefined || val === null || val === '') return PURCHASE_ORDER_CONSTANTS.DEFAULT_VALID_FOR_DAYS; const num = Number(val); return isNaN(num) || num <= 0 ? PURCHASE_ORDER_CONSTANTS.DEFAULT_VALID_FOR_DAYS : num; }, - z.number().positive(t(i18n)`Valid for days must be greater than 0`) + z + .number() + .int(t(i18n)`Valid for days must be an integer`) + .min( + PURCHASE_ORDER_CONSTANTS.MIN_VALID_DAYS, + t(i18n)`Valid for days is too low` + ) + .max( + PURCHASE_ORDER_CONSTANTS.MAX_VALID_DAYS, + t(i18n)`Valid for days exceeds maximum` + ) ),
129-132: Incorrect option on z.date — use required_error.z.date({ message }) is ignored; use required_error for missing values.
- z - .date({ - message: t(i18n)`Expiry date is a required field`, - }) + z.date({ + required_error: t(i18n)`Expiry date is a required field`, + })packages/sdk-react/src/components/payables/PurchaseOrders/hooks/usePurchaseOrderDetails.tsx (1)
104-105: isEdit initialization may desync after create.If parent sets id post-create, consider syncing isEdit with id via useEffect.
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/components/useCreateInvoiceProductsTable.ts (1)
111-137: Minor: reduce can early-return without extra branches.Current shape is fine; consider a simpler accumulate pattern for readability.
- return lineItems.reduce( - (acc, field) => { + return lineItems.reduce((acc, field) => { … - if (!vatRate) return acc; + if (!vatRate) return acc; … - if (acc[vatRate]) { - acc[vatRate] += tax; - } else { - acc[vatRate] = tax; - } - - return acc; - }, - {} as Record<number, number> - ); + acc[vatRate] = (acc[vatRate] ?? 0) + tax; + return acc; + }, {} as Record<number, number>);packages/sdk-react/src/components/payables/PurchaseOrders/components/VendorSelector.tsx (3)
143-173: Strongly type filterOptions params without importing MUI internalsAvoid
any. You can infer from the createdfilterfunction type.- const handleFilterOptions = useCallback( - (options: CounterpartsAutocompleteOptionProps[], params: any) => { + const handleFilterOptions = useCallback( + ( + options: CounterpartsAutocompleteOptionProps[], + params: Parameters<typeof filter>[1] + ) => {
312-314: Pass disabled to AutocompleteYou set
loading={isCounterpartsLoading || disabled}but don’t disable the input.- loading={isCounterpartsLoading || disabled} + disabled={disabled} + loading={isCounterpartsLoading || disabled}
175-358: MUI usage is acceptable here due to complex Autocomplete UX; keep MUI, add TODO for future shadcn parityGiven prior team guidance allowing MUI Autocomplete for advanced cases, keeping MUI here is fine. For non-Autocomplete UI (Button, Typography, etc.), migration to shadcn/ui could be considered later to align with the Tailwind/mtw convention.
Add:
// TODO: Keep MUI Autocomplete until we have a shadcn/ui equivalent; // consider migrating ancillary MUI components (Button, Typography, Divider, etc.) when feasible.packages/sdk-react/src/ui/components/tabs-underline.tsx (3)
88-95: Replace initial inline styles with Tailwind classesAvoid inline styles; set initial
left:0;width:0via classes. Dynamic updates can still set styles at runtime.- <span + <span ref={indicatorRef} aria-hidden className={cn( - 'mtw:absolute mtw:bottom-0 mtw:h-1 mtw:bg-[#3737ff] mtw:rounded-[10px] mtw:transition-[left,width] mtw:duration-200 mtw:ease-out' + 'mtw:absolute mtw:bottom-0 mtw:left-0 mtw:w-0 mtw:h-1 mtw:bg-[#3737ff] mtw:rounded-[10px] mtw:transition-[left,width] mtw:duration-200 mtw:ease-out' )} - style={{ left: 0, width: 0 }} />
27-74: Track size changes with ResizeObserver (container resizes not covered by window resize)Listening only to
resizemay miss container/layout changes. Observing the list element improves robustness.useLayoutEffect(() => { const list = listRef.current; const indicator = indicatorRef.current; if (!list || !indicator) return; @@ - window.addEventListener('resize', update); + const ro = new ResizeObserver(() => update()); + ro.observe(list); + window.addEventListener('resize', update); @@ return () => { mo.disconnect(); - window.removeEventListener('resize', update); + ro.disconnect(); + window.removeEventListener('resize', update); }; }, [children]);
80-94: Prefer theme tokens over hard-coded hex colorsUse Tailwind vars/tokens if available to align with design system.
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.module.css (1)
1-4: Avoid !important if possible; consider increasing selector specificity
!importantcan cause override wars. If feasible, adjust selector specificity or apply a wrapper class in the TSX to avoid forcing.packages/sdk-react/src/components/payables/CreatePayableMenu/CreatePayableMenu.test.tsx (2)
121-136: Optional: assert disabled state when PO creation is not allowed.Add a test to ensure the PO button is disabled when isCreatePurchaseOrderAllowed=false.
35-59: Minor: prefer findBy over waitFor+getBy for async DOM.**Simplifies the pattern and reduces flakiness.
packages/sdk-react/src/components/templateSettings/components/LayoutAndLogo.tsx (1)
11-12: MUI usage in modified file — add TODO to migrate to shadcn/ui.Per guidelines, keep MUI if not doing a larger refactor, but mark migration.
Apply:
-import { Divider, CircularProgress } from '@mui/material'; +// TODO: Migrate Divider and CircularProgress to shadcn/ui equivalents per SDK UI guidelines. +import { Divider, CircularProgress } from '@mui/material';packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderModals.tsx (1)
17-22: Nit: boolean prop can be shorthand.Style-only; optional.
Apply:
- <TemplateSettings - isDialog={true} + <TemplateSettings + isDialog isOpen={isEditTemplateModalOpen} handleCloseDialog={onTemplateModalClose} documentType="purchase_order" />packages/sdk-react/src/components/templateSettings/TemplateSettings.tsx (1)
27-29: Reuse shared type alias for documentTypePrefer
SelectableDocumentTypeto avoid duplicating unions.+import type { SelectableDocumentType } from './types'; ... - /** Document type context for filtering templates */ - documentType?: 'receivable' | 'purchase_order'; + /** Document type context for filtering templates */ + documentType?: SelectableDocumentType;packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderDetails.tsx (1)
53-57: Dead state unless toggled externally
emailDialogis initialized but never set to open in this component. If a child triggers it via callbacks, wire those up; otherwise remove until needed.packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts (1)
23-35: Allowing purch_order vs receivable is correct; var naming could be clearer
invoiceTemplatesnow includes purchase order templates depending ondocumentType. Consider renaming totemplatesfor clarity in a follow-up.packages/sdk-react/src/components/payables/PurchaseOrders/sections/PurchaseOrderTermsSummary.tsx (2)
2-2: Import types from schemas barrelUse the centralized export to keep import paths consistent.
-import { CreatePurchaseOrderFormProps } from '../validation'; +import { type CreatePurchaseOrderFormProps } from '../schemas';
15-18: SpellingConsider
FulfillmentSummary(US) orFulfilmentSummary(UK). Current “Fullfillment” is misspelled, but keep consistent if other parts rely on it.packages/sdk-react/src/components/payables/PurchaseOrders/hooks/useUpdatePurchaseOrder.ts (4)
14-21: Batch query invalidations to avoid unnecessary sequential awaitsRun both invalidations concurrently.
- onSuccess: async (purchaseOrder) => { - await api.payablePurchaseOrders.getPayablePurchaseOrders.invalidateQueries( - queryClient - ); - await api.payablePurchaseOrders.getPayablePurchaseOrdersId.invalidateQueries( - queryClient - ); + onSuccess: async (purchaseOrder) => { + await Promise.all([ + api.payablePurchaseOrders.getPayablePurchaseOrders.invalidateQueries( + queryClient + ), + api.payablePurchaseOrders.getPayablePurchaseOrdersId.invalidateQueries( + queryClient + ), + ]);
1-6: Use the counterpart name helper and add a type for the responseAvoid brittle property checks; reuse the shared name resolver.
import { useMoniteContext } from '@/core/context/MoniteContext'; import { getAPIErrorMessage } from '@/core/utils/getAPIErrorMessage'; +import { getCounterpartName } from '@/components/counterparts/helpers'; +import type { components } from '@/api'; import { t } from '@lingui/macro'; import { useLingui } from '@lingui/react'; import { toast } from 'react-hot-toast'; + +type PurchaseOrderResponse = + components['schemas']['PurchaseOrderResponseSchema'];
22-31: More robust success toast messagingLeverage the helper to extract a display name if available.
- if (purchaseOrder.counterpart && 'name' in purchaseOrder.counterpart) { - return toast.success( - t( - i18n - )`Purchase order for "${purchaseOrder.counterpart.name}" was updated` - ); - } - - toast.success(t(i18n)`Purchase order has been updated`); + const name = purchaseOrder?.counterpart + ? getCounterpartName( + (purchaseOrder as PurchaseOrderResponse).counterpart as any + ) + : null; + toast.success( + name + ? t(i18n)`Purchase order for "${name}" was updated` + : t(i18n)`Purchase order has been updated` + );
11-37: Consider optimistic updates (onMutate/rollback) per our API calls guidelineAdd onMutate/onError/onSettled to optimistically update PO list/detail and rollback on failure for better UX.
I can draft the optimistic update skeleton wired to your generated query keys if you confirm the key shape for getPayablePurchaseOrders and getPayablePurchaseOrdersId.
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/EditPurchaseOrderDetails.tsx (2)
12-18: Avoid double wrapping with MoniteScopedProvidersCreatePurchaseOrder already wraps with MoniteScopedProviders (see CreatePurchaseOrder.tsx). Remove the extra provider here to prevent context resets.
-export const EditPurchaseOrderDetails = ( - props: EditPurchaseOrderDetailsProps -) => ( - <MoniteScopedProviders> - <EditPurchaseOrderDetailsContent {...props} /> - </MoniteScopedProviders> -); +export const EditPurchaseOrderDetails = ( + props: EditPurchaseOrderDetailsProps +) => <EditPurchaseOrderDetailsContent {...props} />;
25-31: Inline trivial pass-through callbackReduce noise by passing onUpdated directly.
- const handleSave = useCallback( - (updatedData: PurchaseOrderResponse) => { - onUpdated(updatedData); - }, - [onUpdated] - ); + const handleSave = useCallback(onUpdated, [onUpdated]);packages/sdk-react/src/components/shared/ItemsSection/types.ts (1)
1-22: Prefer readonly arrays for config safetyPrevents accidental mutation of staticMeasureUnits.
- staticMeasureUnits?: string[]; + staticMeasureUnits?: readonly string[];packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx (1)
68-84: Avoid indefinite spinner when file_url is missingPoll until file_url is ready using refetchInterval or trigger a one-off refetch with exponential backoff; otherwise users may be stuck.
packages/sdk-react/src/components/shared/ItemsSection/constants.ts (1)
25-33: De-duplicate measure units by reusing the shared constantImport PURCHASE_ORDER_MEASURE_UNITS instead of duplicating the list here.
-import { ItemsSectionConfig } from './types'; +import { ItemsSectionConfig } from './types'; +import { PURCHASE_ORDER_MEASURE_UNITS } from '@/components/payables/PurchaseOrders/types'; @@ export const PURCHASE_ORDERS_ITEMS_CONFIG: ItemsSectionConfig = { itemSelectionMode: 'manual', - staticMeasureUnits: ['unit', 'cm', 'day', 'hour', 'kg', 'litre'], + staticMeasureUnits: [...PURCHASE_ORDER_MEASURE_UNITS],packages/sdk-react/src/components/shared/ItemsSection/ManualItemSelector.tsx (1)
6-17: Remove unused prop and align with form usageindex is unused; drop it from the API.
interface ManualItemSelectorProps< TFieldValues extends FieldValues = FieldValues, > { control: Control<TFieldValues>; - index: number; fieldName: Path<TFieldValues>; placeholder?: string; error?: boolean; disabled?: boolean; onBlur?: () => void; onChange?: (value: string) => void; }packages/sdk-react/src/components/payables/PurchaseOrders/PreviewPurchaseOrderEmail.tsx (2)
113-117: Avoid blindly appending “.pdf” to document_id
document_idmight already include an extension or be a UUID-like id. Render a clean label and only appendApply:
- {purchaseOrder?.document_id || t(i18n)`Purchase Order`} - {t(i18n)`.pdf`} + {(() => { + const name = purchaseOrder?.document_id || t(i18n)`Purchase Order`; + return name.toLowerCase().endsWith('.pdf') ? name : `${name}.pdf`; + })()}
26-36: Prefer shared counterpart formatting helpersTo keep name/email derivation consistent across the app, prefer
getCounterpartNameand a shared helper for email if available, instead of duplicating union narrowing here.I can extract a
getCounterpartEmail(cp)util alongsidegetCounterpartNameand wire it here. Want me to follow up with a PR?Also applies to: 38-48
packages/sdk-react/src/components/payables/PurchaseOrders/sections/PurchaseOrderDetailsSection.tsx (1)
13-15: Locale-awareness for expiry date and empty input handling
- Pass
i18n.localetotoLocaleDateStringfor consistent formatting.- Treat empty value as
undefinedso schema/defaults can apply instead of coercing to0.Apply:
- const expiryDate = addDays(new Date(), validForDays); + const expiryDate = addDays(new Date(), validForDays); … - onChange={(e) => field.onChange(Number(e.target.value))} + onChange={(e) => field.onChange(e.target.value === '' ? undefined : Number(e.target.value))}And use:
- `${t(i18n)`Expires on`}: ${expiryDate.toLocaleDateString()}` + `${t(i18n)`Expires on`}: ${expiryDate.toLocaleDateString(i18n.locale)}`Also applies to: 42-47
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/OverviewTabPanel.tsx (2)
58-63: Guard currency formatting nulls
formatCurrencyToDisplaycan returnnull; show a dash instead of “null”.Apply:
- {formatCurrencyToDisplay(totalAmount, purchaseOrder.currency)} + {formatCurrencyToDisplay(totalAmount, purchaseOrder.currency) ?? '—'}
5-9: Confirm design system: shadcn vs MoniteCard (MUI)This new component uses
MoniteCard(MUI-based). If the team standard is shadcn for new UI, consider swapping to shadcnCardwith a simple key/value list. Otherwise, add a TODO to migrate and track it.#!/bin/bash # Inventory UI primitives to decide direction fd --max-depth 2 card packages/sdk-react/src/ui/components rg -nP 'MoniteCard' packages/sdk-react/src | head -n 20Also applies to: 34-67
examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.spec.ts (3)
8-16: Skip tests when auth is unavailable/failsAvoid noisy red runs in CI if creds are missing.
Apply:
- const auth = await signInUser(page); - if (!auth.success) { - test.info().annotations.push({ - type: 'auth', - description: auth.error || 'missing creds', - }); - } + const auth = await signInUser(page); + if (!auth.success) { + test.skip(true, auth.error || 'missing creds'); + }
68-73: Use stable selectors for settings button
locator('button').filter({ has: locator('svg') }).nth(0)is brittle. Prefer a role/name,data-testid, or an aria-label.Example:
const settingsBtn = page.getByRole('button', { name: /Settings/i }); // or page.getByTestId('po-settings-btn')
86-119: E2E flow is pragmatic; optional resilience nitsGood use of guards for optional UI. Consider
test.stepto segment flows and make triage easier.Also applies to: 121-169, 171-208
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderDeleteModal.tsx (2)
83-85: Replace MUI sx with Tailwind utility per SDK styling rules.Avoid inline sx; use Tailwind with mtw- prefix.
- <Alert severity="error" sx={{ mt: 2 }}> + <Alert severity="error" className="mtw:mt-2"> {error} </Alert>
69-71: Type-safe onClose handler.MUI onClose provides (event, reason). Wrap to match signature explicitly.
- onClose={onClose} + onClose={(_, __) => onClose()}packages/sdk-react/src/components/payables/Payables.tsx (1)
188-199: Minor: avoid sx for spacing in title loader.Prefer Tailwind utilities.
- {(isReadAllowedLoading || isCreateAllowedLoading || isCreatePurchaseOrderAllowedLoading) && ( - <CircularProgress size="0.7em" color="secondary" sx={{ ml: 1 }} /> - )} + {(isReadAllowedLoading || isCreateAllowedLoading || isCreatePurchaseOrderAllowedLoading) && ( + <CircularProgress size="0.7em" color="secondary" className="mtw:ml-1" /> + )}packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrdersTable.tsx (3)
138-141: Guard against empty sort model.When clearing sort, model can be empty. Prevent undefined access.
- const onChangeSort = (model: GridSortModel) => { - setSortModel(model[0] as PurchaseOrderGridSortModel); - setCurrentPaginationToken(null); - }; + const onChangeSort = (model: GridSortModel) => { + if (model.length) { + setSortModel(model[0] as PurchaseOrderGridSortModel); + setCurrentPaginationToken(null); + } + };
181-205: Creation action coupling to localized label.Relying on exact translated text can be brittle. Prefer a stable identifier for action routing.
- onCreate={(type) => { - if ( - type === t(i18n)`Create Purchase Order` || - type === 'Create Purchase Order' - ) { - setIsCreatePurchaseOrderDialogOpen?.(true); - } - }} + onCreate={() => setIsCreatePurchaseOrderDialogOpen?.(true)}
85-104: API typing source alignment.Guidelines ask to use custom types from '@/types/api' where available instead of raw
components[...]. Consider updating types (e.g., status enum, response schema) to the local aliases for consistency.#!/bin/bash # Check if '@/types/api' exports PO-related types/enums to adopt here rg -nP "export\s+type\s+.*PurchaseOrder|PurchaseOrderStatusEnum" packages/sdk-react/src/types -g '!**/dist/**'packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderForm.tsx (2)
41-41: Guard againstundefined-FormclassName.Avoid rendering a literal "undefined-Form" class.
- className={className + '-Form'} + className={className ? `${className}-Form` : undefined}
10-20: Remove unused props or wire them.
counterpartAddressesandexistingPurchaseOrderare unused here. Either plumb them into children or drop for now.packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/SubmitPurchaseOrder.tsx (1)
58-60: Disabled state still shows pointer cursor and clickable focus.If
disabled, also setaria-disabled, remove pointer cursor, and block clicks via CSS to convey state.- <CardContent - onClick={() => !disabled && setDeliveryMethod(deliveryMethod)} - > + <CardContent + aria-disabled={disabled || undefined} + onClick={() => !disabled && setDeliveryMethod(deliveryMethod)} + sx={{ cursor: disabled ? 'default' : 'pointer' }} + >packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsx (3)
13-14: Use the shared PO default ID constantKeep “PO-auto” consistent with constants.
+import { PURCHASE_ORDER_CONSTANTS } from '@/components/payables/PurchaseOrders/constants'; @@ - value: purchaseOrder.document_id ?? t(i18n)`PO-auto`, + value: + purchaseOrder.document_id ?? + PURCHASE_ORDER_CONSTANTS.DEFAULT_PO_DOCUMENT_ID,Also applies to: 139-141
29-31: Clean up tab IDsRemove redundant “-tab-” in base ID to avoid double “tab” in derived IDs.
- const tabsBaseId = `Monite-PurchaseOrderDetails-overview-${useId()}-tab-`; + const tabsBaseId = `Monite-PurchaseOrderDetails-overview-${useId()}`;No other changes needed; existing usages already append “-tab”/“-tabpanel”.
Also applies to: 71-92, 94-104
171-205: Avoid inline sx for font weightUse Tailwind utility for emphasis.
- <Typography variant="subtitle1" sx={{ fontWeight: 600 }}> + <Typography variant="subtitle1" className="mtw:font-semibold">packages/sdk-react/src/components/payables/PurchaseOrders/config/tableConfig.tsx (3)
61-62: Fallback for missing document_idUse the shared default to keep UX consistent.
- {purchaseOrder.document_id} + {purchaseOrder.document_id ?? PURCHASE_ORDER_CONSTANTS.DEFAULT_PO_DOCUMENT_ID}
78-87: Magic widthReplace hard-coded 180 with a constant for maintainability (e.g., DEFAULT_COUNTERPART_COLUMN_WIDTH).
1-13: Types import sourceGuidelines prefer API types from '@/types/api'. If available, switch from '@/api' to '@/types/api' for row typing.
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/useExistingPurchaseOrderDetails.tsx (2)
107-111: Simplify visibility logic for “Download PDF”The extra status guard is redundant; this only shows for issued + ready.
- const isDownloadPDFButtonVisible = - isAllowedDownloadStatus(purchaseOrder?.status) && - isPdfReady && - purchaseOrder?.status === 'issued'; + const isDownloadPDFButtonVisible = + purchaseOrder?.status === 'issued' && isPdfReady;
114-121: Dead/inconsistent button stateButtons are always invisible (false) but disabled state is still computed. Either surface Delete/Issue buttons per permissions/status or drop the unused disabled booleans.
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/InvoiceItemRow.tsx (2)
185-201: Reuse unified error flag for tax fieldUse taxOrVatError for consistency with the VAT branch.
- error={Boolean(errors.line_items?.[index]?.tax_rate_value)} + error={taxOrVatError}
12-13: Prefer lucide-react icons going forwardGuidelines favor lucide-react. Since this file remains MUI-heavy, keep for now but add a TODO to migrate icons with the component.
Also applies to: 241-245
packages/sdk-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.tsx (1)
71-90: Subject/body defaultsConsider including the PO number in the subject when available for better context.
- const subject = purchaseOrder ? t(i18n)`Purchase Order from Monite` : ''; + const subject = purchaseOrder + ? t(i18n)`Purchase Order ${purchaseOrder.document_id ?? ''}`.trim() + : '';packages/sdk-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.form.components.tsx (2)
40-51: Form styling via inlinestyleprop — switch toclassNamewith Tailwind.Inline styles contradict the Tailwind-first guideline. Replace
stylewithclassNameand usemtw:utilities.export const EmailPurchaseOrderForm = ({ formName, - style, + className, children, handleIssueAndSend, }: PropsWithChildren<FormProps>) => { return ( - <form id={formName} noValidate onSubmit={handleIssueAndSend} style={style}> + <form id={formName} noValidate onSubmit={handleIssueAndSend} className={className}> {children} </form> ); };And update
FormPropsaccordingly.
101-105: Defensive MenuProps: container may be null.
rootfromuseRootElements()can be null. Passundefinedif falsy to avoid MUI warnings.-MenuProps={{ container: root }} +MenuProps={{ container: root || undefined }}packages/sdk-react/src/components/payables/CreatePayableMenu/CreatePayableMenu.tsx (2)
92-99: CoerceanyCreateAllowedto boolean to avoidundefinedflow.
isCreateAllowed || isCreatePurchaseOrderAllowedcan beundefined. Coerce to boolean for clarity and type-safety.-const anyCreateAllowed = isCreateAllowed || isCreatePurchaseOrderAllowed; +const anyCreateAllowed = Boolean(isCreateAllowed || isCreatePurchaseOrderAllowed);
172-190: Optional: Disable the PO tab when creation is not allowed.If PO creation is feature-gated, preventing tab selection can reduce confusion. You can hide/disable the tab based on
isCreatePurchaseOrderAllowed. If keeping the tab visible is intentional for marketing/visibility, ignore.packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderStatusChip.tsx (3)
9-12: PreferExtract<>over intersection for status key narrowing.
type StatusConfigKey = Extract<PurchaseOrderStatus, keyof StatusConfigMap>;is clearer and avoids tricky union-intersection behavior.-type StatusConfigKey = PurchaseOrderStatus & keyof StatusConfigMap; +type StatusConfigKey = Extract<PurchaseOrderStatus, keyof StatusConfigMap>;
1-7: New component uses MUI; consider shadcn Badge/Pill for consistency.For new sdk-react UI, prefer shadcn/ui. If you keep MUI for now, add a TODO to migrate and map styles to Tailwind. The current
outlined+border: nonecombination is a bit contradictory; a filled/soft variant in shadcn would be simpler.Also applies to: 88-100
71-81: Contrast/readability check forlighten(color, 0.9).At 90% lightening, text on background might fail contrast on some themes. Consider using alpha on the color (e.g., text color + background via
rgba(color, 0.1)) or a smaller lighten factor and verify with WCAG tooling.packages/sdk-react/src/components/payables/PurchaseOrders/components/PurchaseOrderVatModeMenu.tsx (1)
22-30: ExposeclassNameand supportnull/undefinedvalues for initial state.
classNameis accepted but unused; plumb it through.- Many forms start with no selection; allow
value?: T | nulland guard display text.-type GenericDropdownMenuProps<T = string> = { - value: T; +type GenericDropdownMenuProps<T = string> = { + value?: T | null; onChange: (value: T) => void; options: DropdownOption<T>[]; disabled?: boolean; placeholder?: string; className?: string; triggerClassName?: string; }; export function GenericDropdownMenu<T = string>({ - value, + value, options, disabled = false, placeholder = DEFAULT_PLACEHOLDER, triggerClassName = DEFAULT_TRIGGER_CLASSNAME, onChange, }: GenericDropdownMenuProps<T>) { const selectedOption = useMemo( - () => options.find((option) => option.value === value), + () => options.find((option) => option.value === value), [options, value] ); const displayText = useMemo( - () => selectedOption?.displayLabel || selectedOption?.label || placeholder, + () => selectedOption?.displayLabel || selectedOption?.label || placeholder, [selectedOption, placeholder] ); return ( - <DropdownMenu> + <DropdownMenu> <DropdownMenuTrigger asChild disabled={disabled}> - <button type="button" className={triggerClassName} disabled={disabled}> + <button type="button" className={cn(triggerClassName)} disabled={disabled}> {displayText} {!disabled && <ChevronDownIcon className="mtw:w-4 mtw:h-4" />} </button> </DropdownMenuTrigger> <DropdownMenuContent>Also applies to: 32-40, 50-69
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx (2)
49-53: Expiry date computation is sound; consider parseISO for strings.
new Date(string)is locale-sensitive for non-ISO inputs. If API returns ISO strings, you’re fine; otherwise preferparseISOfor safety.
21-23: Optional: tighten currency typing to API enum.Use
components['schemas']['CurrencyEnum']forcurrencyprop to get compile-time safety and align with downstream usage inPurchaseOrderPreviewMonite.Also applies to: 114-119
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx (1)
246-253: Use the computed total amount from sanitized items.The component manually recalculates
totalAmountwhen this should already be handled by theuseCreateInvoiceProductsTablehook through the sanitized items.The totals calculation is already being done by
useCreateInvoiceProductsTable. Consider using the computed values instead of recalculating inline:- const quantity = item?.quantity ?? 1; - const price = item?.product?.price?.value ?? 0; - const totalAmount = price * quantity; + const quantity = item?.quantity ?? 1; + const price = item?.product?.price?.value ?? 0; + // totalAmount is already computed by useCreateInvoiceProductsTable + const totalAmount = price * quantity;packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (1)
514-540: Consider extracting the field mapping logic.The
mapSuffixfunction contains complex mapping logic that could be extracted to improve readability and testability.Consider extracting this as a utility function:
// In a separate utils file or at the top of the component const createFieldMapper = (fieldMapping: ItemsSectionConfig['fieldMapping']) => { return (suffix: string): string | null => { const mappings: Record<string, string | null> = { 'product.name': fieldMapping.itemName, 'product.price.value': fieldMapping.price, 'price.value': fieldMapping.price, 'product.price.currency': fieldMapping.currency, 'price.currency': fieldMapping.currency, 'product.measure_unit_id': fieldMapping.measureUnit, 'quantity': fieldMapping.quantity, 'vat_rate_id': fieldMapping.vatRateId ?? null, 'vat_rate_value': fieldMapping.vatRateValue ?? null, 'tax_rate_value': fieldMapping.taxRateValue ?? null, }; return mappings[suffix] ?? suffix; }; };packages/sdk-react/src/components/payables/PurchaseOrders/sections/VendorSection.tsx (2)
44-65: Consider optimizing address selection logic.The useEffect has redundant logic for selecting the default address that could be simplified.
Both conditions in the useEffect perform the same address selection logic. Consider simplifying:
useEffect(() => { if (!counterpartAddresses?.data?.length) return; - if (!selectedAddressId) { - counterpartIdRef.current = counterpart?.id; - const addressId = - counterpart?.default_billing_address_id || - (counterpartAddresses.data.length === 1 - ? counterpartAddresses.data[0].id - : ''); - setValue('counterpart_address_id', addressId); - } - - if (selectedAddressId && counterpartIdRef.current !== counterpart?.id) { + if (!selectedAddressId || counterpartIdRef.current !== counterpart?.id) { counterpartIdRef.current = counterpart?.id; const addressId = counterpart?.default_billing_address_id || (counterpartAddresses.data.length === 1 ? counterpartAddresses.data[0].id : ''); setValue('counterpart_address_id', addressId); } }, [counterpartAddresses, setValue, counterpart, selectedAddressId]);
31-32: Consider using state instead of ref for tracking counterpart ID.Using a ref for
counterpartIdRefmight not trigger re-renders when needed. Consider using state if the component needs to react to changes.If this value needs to trigger re-renders or side effects, consider:
- const counterpartIdRef = useRef<string | undefined>(''); + const [previousCounterpartId, setPreviousCounterpartId] = useState<string | undefined>('');packages/sdk-react/src/components/payables/PurchaseOrders/components/FormErrorDisplay.tsx (1)
88-99: Consider adding type safety for field errors.The field errors object could benefit from a more type-safe approach to ensure all expected fields are covered.
Consider defining a type for the field errors:
type LineItemFieldErrors = { name?: string | null; quantity?: string | null; price?: string | null; vat_rate_id?: string | null; vat_rate_value?: string | null; tax_rate_value?: string | null; unit?: string | null; }; const fieldErrors: LineItemFieldErrors = { name: lineItemErrors?.name?.message || null, // ... rest of the fields };packages/sdk-react/src/components/payables/shared/ConfigurableDataTable.tsx (1)
190-201: Inline MUIsxstyling in a new component violates UI guidelines.New components should use shadcn/ui and Tailwind (
mtw:classes). At minimum, add a TODO to migrate styles/components.className={classNames(ScopedCssBaselineContainerClassName, className)} - sx={{ - display: 'flex', - flexDirection: 'column', - overflow: 'hidden', - height: 'inherit', - minHeight: '500px', - position: 'relative', - }} + // TODO(DEV-16185): Migrate to shadcn/ui + Tailwind classes; avoid `sx` in new components.packages/sdk-react/src/components/payables/PurchaseOrders/types.ts (1)
32-35: Duplicate status enum vs API enum.Consider reusing
components['schemas']['PurchaseOrderStatusEnum']to avoid drift.packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/ExistingPurchaseOrderDetails.tsx (1)
30-39: New UI mixing MUI layout components; add migration TODO.This is a new component. Per guidelines, prefer shadcn/ui and Tailwind for layout (
Toolbar,Grid,DialogTitle,DialogContent,Alert, etc.). If migration is out-of-scope, annotate with a TODO.- DialogTitle, - DialogContent, - Grid, - Toolbar, + // TODO(DEV-16185): Consider migrating MUI layout elements to shadcn/ui + Tailwind in a follow-up.Also applies to: 164-335
packages/sdk-react/src/components/payables/PurchaseOrders/CreatePurchaseOrder.tsx (5)
500-508: Inlinesxin new UI; prefer Tailwind.Avoid inline styles in new components. If immediate migration is heavy, leave a TODO.
- <Box - sx={{ - width: '50%', - height: '100%', - display: 'flex', - flexDirection: 'column', - }} - > + <Box + // TODO(DEV-16185): Replace `sx` with Tailwind classes and shadcn/ui layout. + >
738-739: Avoid inlinestylefor spacing.Use Tailwind (
mtw:mb-xx) or a class.- style={{ marginBottom: theme.spacing(7) }} + className="mtw:mb-8" /* TODO: map design token to Tailwind scale */
322-361: Clamp user-entered values to documented limits.Apply MIN/MAX for
valid_for_days(seePURCHASE_ORDER_CONSTANTS) to prevent out-of-range payloads.- valid_for_days: isNaN(Number(values.valid_for_days)) - ? PURCHASE_ORDER_CONSTANTS.DEFAULT_VALID_FOR_DAYS - : Number(values.valid_for_days), + valid_for_days: (() => { + const n = Number(values.valid_for_days); + const v = isNaN(n) ? PURCHASE_ORDER_CONSTANTS.DEFAULT_VALID_FOR_DAYS : n; + return Math.min( + PURCHASE_ORDER_CONSTANTS.MAX_VALID_DAYS, + Math.max(PURCHASE_ORDER_CONSTANTS.MIN_VALID_DAYS, v) + ); + })(),
574-631: Currency modal: persist selection withoutstyle/sxand ensure RHF sync.Consider driving currency via RHF state only and syncing
actualCurrencyin auseEffecton form value; also avoidsx.
292-301: Entity VAT IDs side-effect: guard duplicate toasts.
vatIdsErrortoast will trigger on each render while error persists. Memoize or move intouseEffect.- if (vatIdsError) { - const message = getAPIErrorMessage(i18n, vatIdsError); - if (message) { - toast.error(message); - } - } + useEffect(() => { + if (!vatIdsError) return; + const message = getAPIErrorMessage(i18n, vatIdsError); + if (message) toast.error(message); + }, [i18n, vatIdsError]);
packages/sdk-react/src/components/payables/CreatePayableMenu/CreatePayableMenu.test.tsx
Show resolved
Hide resolved
...ages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderForm.tsx
Outdated
Show resolved
Hide resolved
...ages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderForm.tsx
Outdated
Show resolved
Hide resolved
...es/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderHeader.tsx
Outdated
Show resolved
Hide resolved
...es/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderModals.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/utils.ts
Show resolved
Hide resolved
packages/sdk-react/src/components/shared/ItemsSection/GenericQuantityField.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/shared/ItemsSection/GenericQuantityField.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/shared/ItemsSection/ManualItemSelector.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/templateSettings/components/LayoutAndLogo.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/config/tableConfig.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/CreatePurchaseOrder.tsx
Outdated
Show resolved
Hide resolved
...k-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.form.components.tsx
Outdated
Show resolved
Hide resolved
...k-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.form.components.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/shared/ConfigurableDataTable.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/shared/ConfigurableDataTable.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/shared/ConfigurableDataTable.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx
Outdated
Show resolved
Hide resolved
dinis-monite
added
pullpreview
packages/sdk-react/src/components/payables/PurchaseOrders/constants.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk-react/src/core/hooks/useLocalStorageFields.ts (1)
37-46: Functional updater reads stale stateWhen
setValueis called with a function, you invoke it with the closure-capturedstoredValue. In React this may lag behind the latest state (e.g., two functional updates in the same tick will both compute from the same stale value), so the new API contract breaks. Use the functional form ofsetStoredValueand derive the next value from itsprev. Example fix:- const setValue = (value: T | ((prev: T) => T)) => { - try { - const valueToStore = - value instanceof Function ? value(storedValue) : value; - - setStoredValue(valueToStore); - - if (typeof window !== 'undefined' && isRemembered) { - window.localStorage.setItem(prefixedKey, JSON.stringify(valueToStore)); - } - } catch (error) { - console.error('Error saving to localStorage:', error); - } - }; + const setValue = (value: T | ((prev: T) => T)) => { + try { + setStoredValue((prev) => { + const valueToStore = + typeof value === 'function' ? (value as (prev: T) => T)(prev) : value; + + if (typeof window !== 'undefined' && isRemembered) { + window.localStorage.setItem(prefixedKey, JSON.stringify(valueToStore)); + } + + return valueToStore; + }); + } catch (error) { + console.error('Error saving to localStorage:', error); + } + };
🧹 Nitpick comments (61)
packages/sdk-react/src/components/payables/PurchaseOrders/components/VendorSelector.tsx (1)
322-324: Don’t show loading spinner when disabled; wire Autocomplete disabled propUse disabled to disable interactions, keep loading for fetch state only.
- loading={isCounterpartsLoading || disabled} + loading={isCounterpartsLoading} + disabled={disabled}examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.validation.spec.ts (1)
29-35: Add explicit timeouts to reduce flakiness.Use consistent 10–15s timeouts as done elsewhere in this suite.
Apply this diff:
- await expect(saveButton).toBeEnabled(); + await expect(saveButton).toBeEnabled({ timeout: 15000 }); @@ - await expect( - page.getByText(/Please check the form for errors/i) - ).toBeVisible(); + await expect( + page.getByText(/Please check the form for errors/i) + ).toBeVisible({ timeout: 15000 });examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.spec.ts (1)
120-126: Reduce reliance on keyboard focus order for settings.Keyboard Shift+Tab/Enter is brittle. Prefer targeting a stable locator (e.g., button with accessible name or test id) for the settings trigger.
examples/with-nextjs-and-clerk-auth/e2e/utils/purchase-order-helpers.ts (1)
225-236: Remove unused variables or wire them into the flow.
createNewOptionandcreateNewButtonare declared but not used.Apply this diff to remove them:
- const createNewOption = optionsList - .filter({ hasText: /Create new (vendor|counterpart|customer)/i }) - .first(); @@ - const createNewButton = page - .getByRole('button', { name: /Create new (vendor|counterpart|customer)/i }) - .or(page.locator('button:has-text("Create counterpart")')) - .first();packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsx (6)
1-7: Consider lazy-loading modal components to reduce initial bundle size.These modal imports add to the main bundle even when unopened. Prefer React.lazy or next/dynamic for the 4 modal components.
95-97: Good SSR guard and secure window.open; consider user feedback when no URL.Add a fallback toast if no file_url is returned.
- if (typeof window !== 'undefined' && pdfLink.data?.file_url) { - window.open(pdfLink.data.file_url, '_blank', 'noopener,noreferrer'); - } + if (typeof window !== 'undefined' && pdfLink.data?.file_url) { + window.open(pdfLink.data.file_url, '_blank', 'noopener,noreferrer'); + } else { + toast.error(t(i18n)`Could not open PDF`); + }
245-249: Disable "Send draft" while permissions are loading for consistency.Prevents false-negative toasts before permission is resolved.
- <DropdownMenuItem + <DropdownMenuItem + disabled={isUpdateAllowedLoading} onClick={() => handleButtonClick(actions.onIssueAndSendButtonClick) } >
243-256: Optional: disable "Download draft" while PDF is fetching.Aligns with the main PDF button behavior.
- <DropdownMenuItem onClick={handleDownloadPdf}> + <DropdownMenuItem disabled={isDownloadingPdf} onClick={handleDownloadPdf}> {t(i18n)`Download draft`} </DropdownMenuItem>
271-277: Disable "Cancel invoice" while permissions are loading.Avoids premature error toasts.
- <DropdownMenuItem + <DropdownMenuItem + disabled={isUpdateAllowedLoading} onClick={() => handleButtonClick(() => setCancelModalOpen(true))} >
281-283: Disable "Mark as uncollectible" while permissions are loading.Consistent UX with other gated actions.
- <DropdownMenuItem + <DropdownMenuItem + disabled={isUpdateAllowedLoading} onClick={() => handleButtonClick(() => setMarkAsUncollectibleModalOpen(true)) } >packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/types.ts (2)
19-21: Export MeasureUnitId and prefer '@/types/api' per guidelines
- Export MeasureUnitId so other modules (e.g., utils) can reuse the exact id type.
- Consider sourcing API types from '@/types/api' instead of '@/api' for consistency.
As per coding guidelines
-type MeasureUnit = components['schemas']['LineItemProductMeasureUnit']; -type MeasureUnitId = MeasureUnit['id']; +type MeasureUnit = components['schemas']['LineItemProductMeasureUnit']; +export type MeasureUnitId = MeasureUnit['id'];
28-36: Clarify units and align with schema semantics
- Please document explicitly that the flat price is in minor units to prevent misinterpretation.
- Naming is fine, but consider “unit” → “measure_unit_id” for clarity, or ensure cross-file types consistently map it to product.measure_unit_id.
Based on learnings
/** * Optional flat fields accepted by sanitization helpers - * When provided, they will be normalized into product.price and related fields + * When provided, they will be normalized into product.price and related fields. + * Notes: `price` is in minor units (PriceFloat.value), not major. */packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/utils.ts (2)
6-11: Avoid re-declaring fields already in SanitizableLineItem; keep unit type consistentprice, currency, and unit are already defined on SanitizableLineItem. Redeclaring them here is redundant and risks divergence (e.g., unit typed as string here vs. MeasureUnitId in types).
type ExtendedLineItem = SanitizableLineItem & { - name?: string; - price?: number; - currency?: CurrencyEnum; - unit?: string; + name?: string; product_id?: string; quantity?: number; vat_rate_value?: number; tax_rate_value?: number;
317-319: Remove hard-coded 'USD'; thread a fallback currency into sanitizationDefaulting to 'USD' can mismatch tenant settings. Accept a fallbackCurrency and use it only when neither nested nor flat currency is set.
- currency: (extItem.product?.price?.currency || - flatCurrency || - 'USD') as CurrencyEnum, + currency: (extItem.product?.price?.currency ?? + flatCurrency ?? + fallbackCurrency) as CurrencyEnum,Also update the function signature to accept fallbackCurrency:
-export const sanitizeLineItems = ( - items: ReadonlyArray<SanitizableLineItem> | undefined -): CreateReceivablesFormBeforeValidationLineItemProps[] => { +export const sanitizeLineItems = ( + items: ReadonlyArray<SanitizableLineItem> | undefined, + fallbackCurrency?: CurrencyEnum +): CreateReceivablesFormBeforeValidationLineItemProps[] => {packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/hooks/useLineItemManagement.ts (5)
8-8: Prefer API types from '@/types/api'Using components from '@/api' leaks OpenAPI internals. Import PriceFloat and related types via '@/types/api' to keep a stable type surface.
As per coding guidelines
79-85: Hashing watched values can be costlyJSON.stringify on large arrays may impact performance. Consider useWatch with deep compare or a shallow stable mapping to reduce cost.
87-115: Pass fallback currency into sanitization to avoid 'USD' defaultPropagate actualCurrency/defaultCurrency into sanitizeLineItems after adding the new parameter.
- return sanitizeLineItems(convertedItems); - }, [currentLineItems]); + return sanitizeLineItems( + convertedItems, + actualCurrency ?? defaultCurrency + ); + }, [currentLineItems, actualCurrency, defaultCurrency]);
169-171: Avoid empty string for measure_unit_idOmit the key when unset; an empty string can violate schema expectations.
- measure_unit_id: template?.product?.measure_unit_id || '', + ...(template?.product?.measure_unit_id + ? { measure_unit_id: template.product.measure_unit_id } + : {}),
176-179: Avoid empty string for product_idOnly include product_id when present.
- id: template?.id ?? generateUniqueId(), - product_id: template?.product_id || '', + id: template?.id ?? generateUniqueId(), + ...(template?.product_id ? { product_id: template.product_id } : {}), product,packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx (4)
55-68: Remove redundant optional chaining and fallback
line_itemsis already defaulted to[](Line 50). Simplify the mapping.Apply this diff:
- const convertedLineItems = - line_items?.map( - (item): SanitizableLineItem => ({ - id: item.id, - name: item.name, - quantity: item.quantity, - unit: item.unit, - price: item.price, - currency: item.currency as components['schemas']['CurrencyEnum'], - vat_rate_id: item.vat_rate_id, - vat_rate_value: item.vat_rate_value, - tax_rate_value: item.tax_rate_value, - }) - ) || []; + const convertedLineItems = line_items.map( + (item): SanitizableLineItem => ({ + id: item.id, + name: item.name, + quantity: item.quantity, + unit: item.unit, + price: item.price, + currency: item.currency as components['schemas']['CurrencyEnum'], + vat_rate_id: item.vat_rate_id, + vat_rate_value: item.vat_rate_value, + tax_rate_value: item.tax_rate_value, + }) + );
29-31: Tighten currency prop type to CurrencyEnumUse the OpenAPI enum instead of a loose string to avoid downstream casts.
Apply this diff:
- currency?: string; + currency?: components['schemas']['CurrencyEnum'];As per coding guidelines
246-247: Localize the fallback unit labelProvide a translatable fallback for the unit label.
Apply this diff:
- {item?.product?.measure_unit_id || 'unit'} + {item?.product?.measure_unit_id || t(i18n)`unit`}As per coding guidelines
2-2: Consider migrating CSS modules to Tailwind/shadcn in a follow-upNew UI should prefer Tailwind (mtw- prefixed) and shadcn components. Keeping CSS modules is fine short-term; add a TODO to track migration.
Apply this diff:
import purchaseOrderStyles from './PurchaseOrderPreviewMonite.module.css'; +// TODO: Migrate styles to Tailwind (mtw- prefixed) and shadcn/ui components where feasible. import styles from '@/components/receivables/InvoiceDetails/CreateReceivable/sections/components/InvoicePreviewMonite.module.css'; +// TODO: Avoid CSS modules in new components; align with Tailwind utility classes.As per coding guidelines
Also applies to: 14-14
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx (3)
82-86: Replace inline centering styles with Tailwind utilitiesAvoid inline styles for layout. Use Tailwind classes for centering.
Apply this diff:
- <div - ref={containerRef} - className={cn( + <div + ref={containerRef} + className={cn( // Container layout - 'mtw:flex mtw:overflow-auto mtw:relative', + 'mtw:flex mtw:overflow-auto mtw:relative', + 'mtw:items-center mtw:justify-center', 'mtw:w-full mtw:h-full mtw:min-h-0', 'mtw:p-12', // 48px padding 'mtw:bg-gradient-to-br mtw:from-slate-50 mtw:to-slate-200', // Force pixel-snapping for zoom stability 'mtw:[transform:translateZ(0)]' )} - style={{ - justifyContent: 'safe center', - alignItems: 'safe center', - }} >As per coding guidelines
22-23: Tighten currency type on public APIPrefer the generated enum for correctness and to remove casts downstream.
Apply this diff:
- currency?: string; + currency?: components['schemas']['CurrencyEnum'];As per coding guidelines
45-61: Guard against invalid expiry_date valuesIf an invalid date string slips through, fall back to the computed expiry date.
Apply this diff:
const expiryDate = useMemo(() => { if (purchaseOrderData.expiry_date) { const d = new Date(purchaseOrderData.expiry_date); - - d.setHours(0, 0, 0, 0); - - return d; + if (!Number.isFinite(d.getTime())) { + // fall through to computed expiry + } else { + d.setHours(0, 0, 0, 0); + return d; + } } const validForDays = purchaseOrderData.valid_for_days || PURCHASE_ORDER_CONSTANTS.DEFAULT_VALID_FOR_DAYS; const d = addDays(new Date(), validForDays); d.setHours(0, 0, 0, 0); return d; }, [purchaseOrderData.expiry_date, purchaseOrderData.valid_for_days]);packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderHeader.tsx (1)
42-48: Add accessible name to the settings triggerProvide an aria-label on the trigger button so SR users know its purpose.
Apply:
- <Button + <Button variant="outline" className="mtw:mr-2" disabled={isLoading} + aria-label={t(i18n)`Open settings`} >packages/sdk-react/src/components/payables/PurchaseOrders/Filters/Filters.tsx (4)
67-71: Normalize “All statuses” to null for type safety and simpler filteringPassing 'all' into a status-typed field complicates downstream logic. Emit null instead.
Apply:
- <Select - defaultValue="all" - onValueChange={(value) => { - onChangeFilter(FILTER_TYPE_STATUS, value); - }} - > + <Select + defaultValue="all" + onValueChange={(value) => { + onChangeFilter(FILTER_TYPE_STATUS, value === 'all' ? null : value); + }} + >
90-95: Normalize “All vendors” to null for clearer semanticsUse null to mean “no vendor filter”.
Apply:
- <Select - defaultValue="all" - onValueChange={(value) => { - onChangeFilter(FILTER_TYPE_VENDOR, value); - }} - > + <Select + defaultValue="all" + onValueChange={(value) => { + onChangeFilter(FILTER_TYPE_VENDOR, value === 'all' ? null : value); + }} + >
78-85: Micro: precompute status text map outside the loopAvoid recomputing getStatusTextMap(i18n) per item.
Apply:
- {PurchaseOrderStatusEnum.map((status) => { - const map = getStatusTextMap(i18n); + {(() => { + const map = getStatusTextMap(i18n); + return PurchaseOrderStatusEnum.map((status) => { return ( <SelectItem value={status} key={status}> {map[status as keyof typeof map]} </SelectItem> - ); - })} + ); + })()}
49-57: Consider avoiding sx in new components; prefer Tailwind classesFilterContainer forces sx, but for new code we should favor Tailwind utilities. If feasible, extend FilterContainer to accept className or wrap it with a Tailwind container and drop these sx overrides.
As per coding guidelines
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/VatRateField.tsx (1)
17-17: fieldError prop is unusedEither remove it from props and callers, or render helper text for it.
Do you want me to wire a FormHelperText under the Select to show fieldError?.message?
Also applies to: 217-217
packages/sdk-react/src/components/shared/ItemsSection/ManualItemSelector.tsx (1)
10-10: Remove unused prop ‘index’
packages/sdk-react/src/components/shared/ItemsSection/ManualItemSelector.tsx:10 – remove theindexprop from the component’s props as it’s never referenced.packages/sdk-react/src/components/shared/ItemsSection/GenericQuantityField.tsx (1)
83-121: Keep RHF dirty/touched state in sync (call field.onChange too)You override Controller’s onChange and only call the parent updater. Also call
quantityControllerField.onChangewith the parsed numeric value to keep RHF state consistent.- onChange={(e) => { + onChange={(e) => { const rawValue = e.target.value; setQuantityRawValue(rawValue); onLineItemManuallyChanged(); const numericValue = parseLocaleNumericString( rawValue, i18n.locale ); onLineItemValueChange('quantity', numericValue, { shouldValidate: false, }); + // ensure RHF marks the field as dirty/updated as well + quantityControllerField.onChange(numericValue); }}packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx (1)
22-27: Guard the query; avoid firing without entity contextAdd
enabledto prevent calls whenentityIdorpurchaseOrderIdis missing. If headers are globally injected, you can also drop the per-call header.As per coding guidelines
- } = api.payablePurchaseOrders.getPayablePurchaseOrdersId.useQuery({ - path: { - purchase_order_id: purchaseOrderId, - }, - header: { 'x-monite-entity-id': entityId }, - }); + } = api.payablePurchaseOrders.getPayablePurchaseOrdersId.useQuery( + { + path: { purchase_order_id: purchaseOrderId }, + header: { 'x-monite-entity-id': entityId }, + }, + { enabled: !!entityId && !!purchaseOrderId } + );packages/sdk-react/src/components/shared/ItemsSection/consts.ts (1)
29-35: Avoid duplicating measure units; reuse the shared constantPrefer importing the canonical
PURCHASE_ORDER_MEASURE_UNITSinstead of hardcoding.-import type { PurchaseOrderMeasureUnit } from '@/components/payables/PurchaseOrders/types'; +import type { PurchaseOrderMeasureUnit } from '@/components/payables/PurchaseOrders/types'; +import { PURCHASE_ORDER_MEASURE_UNITS } from '@/components/payables/PurchaseOrders/consts'; @@ export const PURCHASE_ORDERS_ITEMS_CONFIG: ItemsSectionConfig< CreatePurchaseOrderFormBeforeValidationProps, PurchaseOrderMeasureUnit > = { itemSelectionMode: 'manual', - staticMeasureUnits: ['unit', 'cm', 'day', 'hour', 'kg', 'litre'], + staticMeasureUnits: PURCHASE_ORDER_MEASURE_UNITS as ReadonlyArray<PurchaseOrderMeasureUnit>,packages/sdk-react/src/components/payables/PurchaseOrders/sections/PurchaseOrderTermsSummary.tsx (1)
15-17: Typo in component nameConsider renaming
FullfillmentSummary→FulfillmentSummaryfor consistency.packages/sdk-react/src/components/payables/PurchaseOrders/PreviewPurchaseOrderEmail.tsx (1)
57-61: Avoid inline styles on iframe; use TailwindUse Tailwind classes for sizing/border to align with UI guidelines.
As per coding guidelines
- <iframe - srcDoc={preview?.body_preview} - style={{ width: '100%', height: '100%', border: 0, flex: 1 }} - /> + <iframe + srcDoc={preview?.body_preview} + className="mtw:w-full mtw:h-full mtw:border-0 mtw:flex-1" + />packages/sdk-react/src/components/payables/PurchaseOrders/sections/PurchaseOrderDetailsSection.tsx (1)
50-51: Handle empty input and clamp to range before applyingTyping empty or non-numeric sets NaN. Parse safely and clamp to [MIN, MAX] prior to
onChange.- onChange={(e) => field.onChange(Number(e.target.value))} + onChange={(e) => { + const n = e.target.value === '' ? undefined : Number(e.target.value); + const clamped = + typeof n === 'number' && !isNaN(n) + ? Math.min(PURCHASE_ORDER_CONSTANTS.MAX_VALID_DAYS, Math.max(PURCHASE_ORDER_CONSTANTS.MIN_VALID_DAYS, n)) + : n; + field.onChange(clamped as number | undefined); + }}packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrdersTable.tsx (2)
4-9: Consolidate const imports from the same module.Avoid duplicate imports from './consts'.
-import { PURCHASE_ORDER_CONSTANTS } from './consts'; -import { - FILTER_TYPE_SEARCH, - FILTER_TYPE_STATUS, - FILTER_TYPE_VENDOR, -} from './consts'; +import { + PURCHASE_ORDER_CONSTANTS, + FILTER_TYPE_SEARCH, + FILTER_TYPE_STATUS, + FILTER_TYPE_VENDOR, +} from './consts';
187-204: Avoid brittle string comparisons for actions.Comparing localized labels is fragile. At minimum, reuse a single variable for the label to avoid duplication; ideally pass stable action keys.
- actionButtonLabel={ - !isFiltering && !isSearching - ? t(i18n)`Create Purchase Order` - : undefined - } - actionOptions={ - !isFiltering && !isSearching - ? [t(i18n)`Create Purchase Order`] - : undefined - } + {/* + TODO: expose stable action keys in ConfigurableDataTable to avoid + comparing localized strings. + */} + {(() => { + const createActionLabel = t(i18n)`Create Purchase Order`; + return { + actionButtonLabel: + !isFiltering && !isSearching ? createActionLabel : undefined, + actionOptions: + !isFiltering && !isSearching ? [createActionLabel] : undefined, + _createActionLabel: createActionLabel, + }; + })()} onCreate={(type) => { - if ( - type === t(i18n)`Create Purchase Order` || - type === 'Create Purchase Order' - ) { + // Compare against the single source of truth label + // Fallback kept to avoid regressions if label source changes. + const createActionLabel = t(i18n)`Create Purchase Order`; + if (type === createActionLabel || type === 'Create Purchase Order') { setIsCreatePurchaseOrderDialogOpen?.(true); } }}packages/sdk-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.tsx (2)
84-95: Avoid “undefined” in email body when sender email missing.Fallback to a sane sender display.
- const body = defaultContact - ? t(i18n)`Greetings ${defaultContact.first_name}, - -Please find the purchase order attached to this letter. - -Kind Regards, -${me?.email}` - : t(i18n)`Please find the purchase order attached to this letter. - -Kind Regards`; + const sender = + me?.email || (me?.first_name ? `${me.first_name}` : t(i18n)`Team`); + const body = defaultContact + ? t(i18n)`Greetings ${defaultContact.first_name}, + +Please find the purchase order attached to this letter. + +Kind Regards, +${sender}` + : t(i18n)`Please find the purchase order attached to this letter. + +Kind Regards`;
263-273: MUI sx usage in new component; add TODO to migrate to Tailwind/shadcn.New UI should prefer shadcn/ui and Tailwind (mtw:*). Keep MUI for now but mark migration.
- <DialogContent + {/* TODO: Migrate sx layout to Tailwind classes (mtw:*) and shadcn/ui components */} + <DialogContent className={className + '-Content'} sx={{ mt: presentation === FormPresentation.Preview ? 0 : 4, p: presentation === FormPresentation.Preview ? 0 : '0 32px 32px 32px', height: '100%', width: '100%', display: 'flex', flexDirection: 'column', }} >As per coding guidelines
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/useExistingPurchaseOrderDetails.tsx (2)
8-13: Align allowed download statuses with visibility logic.The constant includes 'draft' but visibility additionally requires 'issued', effectively excluding drafts. Simplify for clarity.
-const ALLOWED_DOWNLOAD_STATUSES: PurchaseOrderStatus[] = ['issued', 'draft']; +const ALLOWED_DOWNLOAD_STATUSES: PurchaseOrderStatus[] = ['issued']; ... - const isDownloadPDFButtonVisible = - isAllowedDownloadStatus(purchaseOrder?.status) && - isPdfReady && - purchaseOrder?.status === 'issued'; + const isDownloadPDFButtonVisible = + isAllowedDownloadStatus(purchaseOrder?.status) && isPdfReady;Also applies to: 108-112
88-93: Harden window.open against tabnabbing fully.You already use noopener/noreferrer. Also nullify opener on the returned window.
- if (purchaseOrder?.file_url) { - window.open(purchaseOrder.file_url, '_blank', 'noopener,noreferrer'); - } + if (purchaseOrder?.file_url) { + const w = window.open( + purchaseOrder.file_url, + '_blank', + 'noopener,noreferrer' + ); + if (w) w.opener = null; + }packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (2)
79-81: Propagate generic TForm to RHF context.The component is generic but uses CreateReceivables types in useFormContext, reducing type-safety for PO usage.
- const { control, formState, getValues, trigger, watch } = - useFormContext<CreateReceivablesFormBeforeValidationProps>(); + const { control, formState, getValues, trigger, watch } = + useFormContext<TForm>();Follow-up: consider updating FieldPath<FieldPathValue<…>> usages to reference TForm to fully align types.
482-499: Prefer Tailwind classes over MUI sx for new shared UI.Since this is a new shared component, plan migration away from sx to mtw:* Tailwind utilities and shadcn/ui equivalents.
Add TODO comments at the main containers to track migration; keep MUI for now to avoid scope creep. As per coding guidelines
Also applies to: 520-523, 741-749
packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderForm.tsx (2)
12-22: Unused props (clean up or wire through).counterpartAddresses and existingPurchaseOrder are declared but unused here. Remove or thread them to children if needed.
49-53: Type-safe mapping to PO error display.Casting fieldErrors to POErrorDisplayProps['fieldErrors'] is fine as a bridge. Consider shaping fieldErrors at source to avoid type assertions later.
packages/sdk-react/src/components/payables/PurchaseOrders/hooks/usePurchaseOrderDetails.tsx (1)
29-38: DRY the items mapping.The create and update payloads duplicate the same items mapping. Extract a helper to keep logic consistent.
Apply these diffs to both places:
- items: line_items.map((item) => ({ - name: item.name, - quantity: item.quantity, - unit: item.unit, - price: rateMajorToMinor(item.price), - currency: item.currency as components['schemas']['CurrencyEnum'], - vat_rate: vatRatePercentageToBasisPoints( - item.vat_rate_value || item.tax_rate_value || 0 - ), - })), + items: toApiItems(line_items),- items: line_items.map((item) => ({ - name: item.name, - quantity: item.quantity, - unit: item.unit, - price: rateMajorToMinor(item.price), - currency: item.currency as components['schemas']['CurrencyEnum'], - vat_rate: vatRatePercentageToBasisPoints( - item.vat_rate_value || item.tax_rate_value || 0 - ), - })), + items: toApiItems(line_items),Add this helper near the top of the file:
function toApiItems( line_items: PurchaseOrderFormData['line_items'] ): components['schemas']['PurchaseOrderPayloadSchema']['items'] { return line_items.map((item) => ({ name: item.name, quantity: item.quantity, unit: item.unit, price: rateMajorToMinor(item.price), currency: item.currency as components['schemas']['CurrencyEnum'], vat_rate: vatRatePercentageToBasisPoints( item.vat_rate_value || item.tax_rate_value || 0 ), })); }Also applies to: 60-69
packages/sdk-react/src/components/payables/PurchaseOrders/validation.ts (5)
53-56: Use proper Zod error keys for number schema.The options object should use required_error/invalid_type_error, not message.
Apply this diff:
- z.number({ - message: t(i18n)`Price is a required field` - }).min(0, t(i18n)`Price must be 0 or greater`) + z.number({ + required_error: t(i18n)`Price is a required field`, + invalid_type_error: t(i18n)`Price must be a number`, + }).min(0, t(i18n)`Price must be 0 or greater`)
127-129: Use required_error for date schema.z.date doesn’t take message; use required_error.
Apply this diff:
- .date({ - message: t(i18n)`Expiry date is a required field`, - }) + .date({ + required_error: t(i18n)`Expiry date is a required field`, + })
81-89: Fix Zod number/string error options for VAT fields.Replace unsupported “error” with required_error/invalid_type_error so messages appear correctly.
Apply this diff:
const vatLineItemSchema = baseLineItemSchema.extend({ tax_rate_value: z.number().min(0).max(100).optional(), - vat_rate_value: z - .number({ error: t(i18n)`VAT is a required field` }) + vat_rate_value: z + .number({ + required_error: t(i18n)`VAT is a required field`, + invalid_type_error: t(i18n)`VAT must be a number`, + }) .min(0, t(i18n)`VAT rate must be 0 or greater`) .max(100, t(i18n)`VAT rate must be 100% or less`), - vat_rate_id: z - .string({ error: t(i18n)`VAT is a required field` }) + vat_rate_id: z + .string({ required_error: t(i18n)`VAT is a required field` }) .min(1, t(i18n)`VAT is a required field`), });
150-152: Enum message option is ignored.z.enum doesn’t support a “message” option here. Either drop it or provide a custom errorMap.
Apply this diff:
- currency: z.enum(CurrencyEnum as [string, ...string[]], { - message: t(i18n)`Currency is required`, - }), + currency: z.enum(CurrencyEnum as [string, ...string[]]),
226-228: Type inference from a parameterized schema is brittle.Inferring from ReturnType loses specificity. Consider exporting a dedicated type or a union of the VAT/non‑VAT schemas for stronger typing.
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/ExistingPurchaseOrderDetails.tsx (2)
24-34: New component uses MUI; add TODO to migrate to shadcn/ui.Per guidelines, new UI should use shadcn/ui and Tailwind. If keeping MUI short-term, add a TODO to migrate.
As per coding guidelines
Apply this diff:
import { Alert, DialogContent, DialogTitle, Grid, IconButton, Stack, Toolbar, Typography, CircularProgress, } from '@mui/material'; +// TODO(DEV-16185): This is a new component but still uses MUI. Migrate to shadcn/ui components and Tailwind classes when feasible, then remove MUI imports. As per coding guidelines.
295-298: Avoid sx styling on new components.Prefer Tailwind classes (mtw:*) over MUI sx for layout/styling in new code.
As per coding guidelines
- <DialogContent - className={className + '-Content'} - sx={{ display: 'flex', flexDirection: 'column' }} - > + <DialogContent className={`${className}-Content mtw:flex mtw:flex-col`}>packages/sdk-react/src/components/payables/PurchaseOrders/components/FormErrorDisplay.tsx (1)
4-4: Use shadcn/ui and Tailwind in new components.This new component uses MUI Alert/Collapse/List. Prefer shadcn/ui equivalents with Tailwind utilities, or add a TODO to migrate.
As per coding guidelines
Apply this diff to leave a migration note:
-import { Alert, Collapse, List, ListItem, ListItemText } from '@mui/material'; +import { Alert, Collapse, List, ListItem, ListItemText } from '@mui/material'; +// TODO(DEV-16185): Replace MUI Alert/Collapse/List with shadcn/ui components and Tailwind classes to align with UI guidelines.packages/sdk-react/src/components/payables/PurchaseOrders/CreatePurchaseOrder.tsx (3)
265-270: Remove unused isFulfillmentDateShown or implement itThis field is stored but never used. Drop it from initialSettingsFields or wire its toggle and behavior.
433-441: Rename inner control to avoid shadowing form methods.controlThis local useForm control is only for the currency selector and shadows methods.control. Rename for clarity.
- const { control } = useForm({ + const { control: currencyControl } = useForm({ defaultValues: useMemo( () => ({ items: [], currency: actualCurrency ?? fallbackCurrency, }), [actualCurrency] ), });Then:
- <MoniteCurrency + <MoniteCurrency ... - control={control} + control={currencyControl}
1-1: Remove invoice reminder dialogs from PO create (likely out of scope)Purchase Order creation imports receivables reminder hooks and renders dialogs, but there’s no UI path to open them here. This adds coupling and overhead without user value.
-import { useInvoiceReminderDialogs } from '../../receivables/hooks'; -... -import { CreateInvoiceReminderDialog } from '@/components/receivables/components/CreateInvoiceReminderDialog'; -import { EditInvoiceReminderDialog } from '@/components/receivables/components/EditInvoiceReminderDialog'; +// Removed invoice reminder dependencies; out of scope for PO creationAnd remove the dialog JSX at the bottom.
Also applies to: 18-20, 867-880
| if (!auth.success) { | ||
| test.info().annotations.push({ | ||
| type: 'auth', | ||
| description: auth.error || 'missing creds', | ||
| }); | ||
| } | ||
| await payablesPage.open(); | ||
| }); |
There was a problem hiding this comment.
Skip tests on auth failure, don’t just annotate.
Continuing after failed auth makes these tests flaky and noisy.
Apply this diff:
- if (!auth.success) {
- test.info().annotations.push({
- type: 'auth',
- description: auth.error || 'missing creds',
- });
- }
+ if (!auth.success) {
+ test.skip(auth.error || 'missing creds');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!auth.success) { | |
| test.info().annotations.push({ | |
| type: 'auth', | |
| description: auth.error || 'missing creds', | |
| }); | |
| } | |
| await payablesPage.open(); | |
| }); | |
| if (!auth.success) { | |
| test.skip(auth.error || 'missing creds'); | |
| } | |
| await payablesPage.open(); | |
| }); |
🤖 Prompt for AI Agents
In examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.spec.ts around
lines 15-22, the test currently only annotates on auth failure but continues
running; change this so the test is skipped when auth.success is false by
replacing the annotation block with a runtime skip (e.g., call
test.skip(!auth.success, auth.error || 'missing creds')) or an early return
after test.skip to prevent further execution, ensuring the page.open call and
subsequent steps do not run on auth failure.
examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.validation.spec.ts
Outdated
Show resolved
Hide resolved
examples/with-nextjs-and-clerk-auth/e2e/utils/purchase-order-helpers.ts
Outdated
Show resolved
Hide resolved
examples/with-nextjs-and-clerk-auth/e2e/utils/purchase-order-helpers.ts
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsx
Outdated
Show resolved
Hide resolved
...nents/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/VatRateField.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/shared/ItemsSection/GenericQuantityField.tsx
Show resolved
Hide resolved
costa-monite
force-pushed
the
feat/DEV-16185-purchase-orders
branch
from
57f793e to
1d254da
Compare
costa-monite
had a problem deploying
to
common
GitHub Actions
Failure
|
🚀 Preview URLs are now available! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/ItemSelector.tsx (2)
171-174: Type mismatch: option objects include undeclared fieldNameitemsAutocompleteData maps options adding fieldName, which is not declared on ItemSelectorOptionProps. This will trigger TS excess property checks at the useMemo return site.
Apply this diff to remove the extra property (or extend the type if you need it later):
return { id: item.id, label: item.name, description: item.description, price: item.price, smallestAmount: item.smallest_amount, measureUnit: unit, - fieldName: fieldName ?? '', };
429-455: Also disable the Autocomplete when disabled is trueAutocomplete remains interactive even when the component is disabled. Set disabled on the Autocomplete as well.
<Autocomplete {...field} id={`item-selector-${index}`} value={selectedItemOption} + disabled={disabled} onChange={(e, value, reason) => {packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/InvoiceItemRow.tsx (1)
208-236: Pass currentVatRateValue to enable reverse-lookup in VatRateFieldVatRateField’s new reverse-lookup requires currentVatRateValue, but it isn’t provided here, so the effect won’t run.
Apply:
<VatRateField value={ onRequestLineItemValue('vat_rate_id') as string | undefined } + currentVatRateValue={ + onRequestLineItemValue('vat_rate_value') as number | undefined + } onChange={(newId, newValue) => { onLineItemValueChange('vat_rate_id', newId, { shouldValidate: true, }); onLineItemValueChange('vat_rate_value', newValue, { shouldValidate: true, });
🧹 Nitpick comments (48)
packages/sdk-react/src/components/templateSettings/TemplateSettings.tsx (1)
27-28: Prefer using the shared SelectableDocumentType.We already export
SelectableDocumentTypein./types, so duplicating the union here risks the props drifting out of sync when we add more document types. Please import and reuse the shared type instead.
As per coding guidelines+import { SelectableDocumentType } from './types'; … - documentType?: 'receivable' | 'purchase_order'; + documentType?: SelectableDocumentType;packages/sdk-react/src/components/payables/PurchaseOrders/PreviewPurchaseOrderEmail.tsx (1)
32-36: Guard on entityId and debounce preview callsAvoid firing the mutation without a valid entityId and reduce chattiness while typing.
useEffect(() => { - if (!purchaseOrderId) return; - createPreview({ subject_text: subject, body_text: body }); - // eslint-disable-next-line react-hooks/exhaustive-deps -}, [purchaseOrderId, subject, body]); + if (!purchaseOrderId || !entityId) return; + const handle = window.setTimeout(() => { + createPreview({ subject_text: subject, body_text: body }); + }, 300); + return () => window.clearTimeout(handle); + // eslint-disable-next-line react-hooks/exhaustive-deps +}, [purchaseOrderId, subject, body, entityId]);Based on learnings.
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx (1)
22-27: Guard the query with enabled (or use the hook)Avoid firing the request before entityId/purchaseOrderId are ready.
Based on learnings
- } = api.payablePurchaseOrders.getPayablePurchaseOrdersId.useQuery({ - path: { - purchase_order_id: purchaseOrderId, - }, - header: { 'x-monite-entity-id': entityId }, - }); + } = api.payablePurchaseOrders.getPayablePurchaseOrdersId.useQuery( + { + path: { + purchase_order_id: purchaseOrderId, + }, + header: { 'x-monite-entity-id': entityId }, + }, + { enabled: Boolean(entityId && purchaseOrderId) } + );packages/sdk-react/src/components/payables/PurchaseOrders/Filters/Filters.tsx (2)
50-56: Prefer Tailwind classes over sx on new componentsMove the border radius styling to Tailwind and drop the sx usage.
As per coding guidelines
- <FilterContainer - className={className} - sx={{ - borderTopLeftRadius: '12px', - borderTopRightRadius: '12px', - ...sx, - }} - searchField={ + <FilterContainer + className={`${className} mtw:rounded-tl-xl mtw:rounded-tr-xl`} + searchField={
69-71: Normalize “All” to null to avoid leaking sentinel valuesMap "all" to null so downstream filters don’t receive "all".
- onValueChange={(value) => { - onChangeFilter(FILTER_TYPE_STATUS, value); - }} + onValueChange={(value) => { + onChangeFilter(FILTER_TYPE_STATUS, value === 'all' ? null : value); + }}- onValueChange={(value) => { - onChangeFilter(FILTER_TYPE_VENDOR, value); - }} + onValueChange={(value) => { + onChangeFilter(FILTER_TYPE_VENDOR, value === 'all' ? null : value); + }}Also applies to: 92-94
packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderDetails.tsx (1)
50-54: Remove unused email dialog state or wire it up.emailDialog is created and reset on close, but never opened. Either remove it to reduce noise or add the trigger that sets open: true when relevant.
Also applies to: 91-98
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/EditPurchaseOrderDetails.tsx (2)
12-18: Avoid nesting MoniteScopedProviders (double/triple provider trees).This component wraps MoniteScopedProviders, and CreatePurchaseOrder also wraps it. Additionally, callers like ExistingPurchaseOrderDetails wrap providers too. Drop the wrapper here to prevent duplicate baselines and context trees.
Apply:
-import { MoniteScopedProviders } from '@/core/context/MoniteScopedProviders'; import { useCallback } from 'react'; @@ -export const EditPurchaseOrderDetails = ( - props: EditPurchaseOrderDetailsProps -) => ( - <MoniteScopedProviders> - <EditPurchaseOrderDetailsContent {...props} /> - </MoniteScopedProviders> -); +export const EditPurchaseOrderDetails = ( + props: EditPurchaseOrderDetailsProps +) => <EditPurchaseOrderDetailsContent {...props} />;Based on learnings
25-30: Inline passthrough is sufficient; drop unnecessary useCallback.handleSave only forwards the value. Passing onUpdated directly keeps it simpler.
Apply:
- const handleSave = useCallback( - (updatedData: PurchaseOrderResponse) => { - onUpdated(updatedData); - }, - [onUpdated] - ); + const handleSave = onUpdated;packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/ExistingPurchaseOrderDetails.tsx (4)
61-64: Avoid nesting MoniteScopedProviders (duplicate provider and CSS baseline).This component wraps MoniteScopedProviders while children like EmailPurchaseOrderDetails and EditPurchaseOrderDetails also wrap providers. Keep a single provider at the highest level to prevent duplicated context trees and styling side effects.
Apply:
-export const ExistingPurchaseOrderDetails = ( - props: ExistingPurchaseOrderDetailsProps -) => ( - <MoniteScopedProviders> - <ExistingPurchaseOrderDetailsBase {...props} /> - </MoniteScopedProviders> -); +export const ExistingPurchaseOrderDetails = ( + props: ExistingPurchaseOrderDetailsProps +) => <ExistingPurchaseOrderDetailsBase {...props} />;Based on learnings
24-35: New component uses MUI; prefer shadcn/ui per repo rules.For new components in packages/sdk-react, avoid Material UI and style via Tailwind (mtw:*). If migration is large, add a TODO and plan conversion.
As per coding guidelines
Suggested minimal TODO:
import { Alert, DialogContent, DialogTitle, Grid, IconButton, Stack, Toolbar, Typography, CircularProgress, } from '@mui/material'; +// TODO: migrate to shadcn/ui primitives and Tailwind (mtw: classes) per UI guidelines.
131-132: Use the shared default PO document id constant.Replace the literal with PURCHASE_ORDER_CONSTANTS.DEFAULT_PO_DOCUMENT_ID for consistency.
Apply:
- const documentId = purchaseOrder.document_id ?? t(i18n)`PO-auto`; + const documentId = + purchaseOrder.document_id ?? t(i18n)`${PURCHASE_ORDER_CONSTANTS.DEFAULT_PO_DOCUMENT_ID}`;And add:
+import { PURCHASE_ORDER_CONSTANTS } from '@/components/payables/PurchaseOrders/consts';
295-299: Avoid inline/sx styling; prefer Tailwind classes.Replace sx with className="mtw:flex mtw:flex-col" to follow styling rules.
As per coding guidelines
- <DialogContent - className={className + '-Content'} - sx={{ display: 'flex', flexDirection: 'column' }} - > + <DialogContent className={`${className}-Content mtw:flex mtw:flex-col`}>packages/sdk-react/src/components/payables/Payables.tsx (1)
205-207: Avoid MUI sx; prefer Tailwind className per guidelinesUse className for spacing instead of sx to align with Tailwind-first styling.
- {(isReadAllowedLoading || isCreateAllowedLoading || isCreatePurchaseOrderAllowedLoading) && ( - <CircularProgress size="0.7em" color="secondary" sx={{ ml: 1 }} /> - )} + {(isReadAllowedLoading || isCreateAllowedLoading || isCreatePurchaseOrderAllowedLoading) && ( + <CircularProgress size="0.7em" color="secondary" className="mtw:ml-2" /> + )}As per coding guidelines
packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderForm.tsx (1)
12-22: Unused props (cleanup or add TODO)counterpartAddresses and existingPurchaseOrder aren’t used. Remove or add a TODO to avoid confusion.
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/OverviewTabPanel.tsx (1)
1-9: New components should not use MUI; add TODO to migrate to shadcn/ui + TailwindThis is a new file using MUI Box/Skeleton/Typography. Per repo rules, prefer shadcn/ui and Tailwind for new UI. If migration is out of scope now, add a TODO.
+// TODO(DEV-16185): Migrate MUI Box/Skeleton/Typography to shadcn/ui + Tailwind classes to comply with SDK UI guidelines.As per coding guidelines
packages/sdk-react/src/components/payables/CreatePayableMenu/CreatePayableMenu.tsx (1)
92-92: Ensure boolean for disabled stateNormalize to boolean to avoid tri-state issues if a prop is undefined.
- const anyCreateAllowed = isCreateAllowed || isCreatePurchaseOrderAllowed; + const anyCreateAllowed = Boolean( + isCreateAllowed || isCreatePurchaseOrderAllowed + );As per coding guidelines
packages/sdk-react/src/components/payables/PurchaseOrders/types.ts (1)
35-38: Align status typing to API enum to avoid driftAvoid a parallel enum. Alias to the OpenAPI enum and use it in filters to stay in sync with backend.
-export enum PurchaseOrderStatus { - Draft = 'draft', - Issued = 'issued', -} +export type PurchaseOrderStatus = + components['schemas']['PurchaseOrderStatusEnum']; @@ export interface PurchaseOrderFilterTypes { search?: string; - status?: PurchaseOrderStatusEnum; + status?: PurchaseOrderStatus; counterpart_id?: string; } - -type PurchaseOrderStatusEnum = components['schemas']['PurchaseOrderStatusEnum']; export type PurchaseOrderFilterValue = string | Date | null; export type CurrencyEnum = components['schemas']['CurrencyEnum']; export type VatRateBasisPoints = number; // integer basis points, e.g., 1900 => 19%Also applies to: 68-76
packages/sdk-react/src/components/payables/PurchaseOrders/sections/VendorSection.tsx (1)
1-19: Type-only import
CreatePurchaseOrderFormPropsis type-only. Useimport typeto keep it out of runtime bundles.-import { CreatePurchaseOrderFormProps } from '../validation'; +import type { CreatePurchaseOrderFormProps } from '../validation';packages/sdk-react/src/components/payables/PurchaseOrders/components/FormErrorDisplay.tsx (2)
1-7: Add TODO: migrate from MUI to shadcn/ui + Tailwind (mtw-*)New components should not introduce MUI. If migration can’t be done now, add a TODO to track it.
As per coding guidelines
import type { CreatePurchaseOrderFormProps } from '../validation'; import { t } from '@lingui/macro'; import { useLingui } from '@lingui/react'; import { Alert, Collapse, List, ListItem, ListItemText } from '@mui/material'; import { useMemo, memo } from 'react'; import type { FieldErrors } from 'react-hook-form'; +// TODO(ui-migration): Replace MUI Alert/Collapse/List with shadcn/ui components and Tailwind (mtw-*) classes per project guidelines.
37-44: Minor: Collapse should reflect state, not be always openUse
hasErrorsfor theinprop. Return-early already guards, but this is clearer.- <Collapse - in={true} - sx={{ - ':not(.MuiCollapse-hidden)': { - marginBottom: 1, - }, - }} - > + <Collapse in={hasErrors} sx={{ ':not(.MuiCollapse-hidden)': { marginBottom: 1 } }}>packages/sdk-react/src/components/payables/PurchaseOrders/components/VendorSelector.tsx (2)
1-1: Type-only importKeep
CreatePurchaseOrderFormPropsas a type-only import to avoid accidental runtime inclusion.-import { CreatePurchaseOrderFormProps } from '../validation'; +import type { CreatePurchaseOrderFormProps } from '../validation';
322-323: Separate loading from disabledUse
disabledto disable interactions and keeploadingtied to fetching state only.- loading={isCounterpartsLoading || disabled} + disabled={disabled} + loading={isCounterpartsLoading}packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/ItemSelector.tsx (1)
565-571: Avoid inline styles; use MUI sx (or Tailwind) for consistencyReplace the inline style on the endAdornment wrapper with a MUI Box using sx.
- endAdornment: ( - <div - style={{ - display: 'flex', - alignItems: 'center', - gap: '4px', - }} - > - {params.InputProps.endAdornment} - </div> - ), + endAdornment: ( + <Box sx={{ display: 'flex', alignItems: 'center', gap: 0.5 }}> + {params.InputProps.endAdornment} + </Box> + ),Add at top:
+import Box from '@mui/material/Box';As per coding guidelines.
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/VatRateField.tsx (1)
17-29: fieldError prop is unusedYou pass fieldError from the row but do not render helper text. Either use it (e.g., FormHelperText) or remove the prop to avoid confusion.
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/hooks/useLineItemManagement.ts (5)
33-38: Type guard and type mismatch: makepriceoptional inFlatLineItemor strengthen the guard
isFlatLineItemnarrows based onname/product, butFlatLineItem.priceis declared required while later code safely accesses it with optional chaining. Align the type to the actual usage to avoid unsound narrowing.-type FlatLineItem = CreateReceivablesFormBeforeValidationLineItemProps & { - name: string; - price: components['schemas']['PriceFloat']; +type FlatLineItem = CreateReceivablesFormBeforeValidationLineItemProps & { + name: string; + price?: components['schemas']['PriceFloat']; currency?: CurrencyEnum; unit?: string; };Also applies to: 40-43
79-85: Avoid JSON.stringify in render path for dependency trackingStringifying the whole items array can be costly for larger forms. Prefer
useWatch({ name: 'line_items' })from RHF and depend directly on that value (or use a memoized deep-compare util).
162-172: Don’t defaultmeasure_unit_idto empty stringEmpty strings tend to fail schema validation and create noisy diffs. Let it be
undefinedwhen missing.const product: CreateReceivablesFormBeforeValidationLineItemProps['product'] = { name: template?.product?.name || '', price: { currency: actualCurrency ?? defaultCurrency, value: template?.product?.price?.value || 0, }, - measure_unit_id: template?.product?.measure_unit_id || '', + measure_unit_id: template?.product?.measure_unit_id, type: template?.product?.type || 'product', };
176-188: Avoid empty string defaults for optional fields likeproduct_idEmit the key only when you have a value; otherwise omit it.
return { id: template?.id ?? generateUniqueId(), - product_id: template?.product_id || '', + ...(template?.product_id ? { product_id: template.product_id } : {}), product, quantity: template?.quantity ?? 1,
87-116: Optional: pass a default currency tosanitizeLineItemsto avoid hardcoded 'USD' fallbackCurrent utils fall back to 'USD'. Passing
actualCurrency ?? defaultCurrencymakes currency handling consistent with the form state. If you adopt this, add deps.- return sanitizeLineItems(convertedItems); + return sanitizeLineItems(convertedItems, actualCurrency ?? defaultCurrency); -}, [currentLineItems]); +}, [currentLineItems, actualCurrency, defaultCurrency]);packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/utils.ts (2)
317-320: Remove hardcoded 'USD' fallback; accept an optional default currency paramLet callers provide a default currency (e.g., from form state). Keep backward compatibility by falling back to 'USD' only if none provided.
- currency: (extItem.product?.price?.currency || - flatCurrency || - 'USD') as CurrencyEnum, + currency: (extItem.product?.price?.currency ?? + flatCurrency ?? + defaultCurrency ?? + 'USD') as CurrencyEnum,Additionally update the function signature (outside this hunk):
-export const sanitizeLineItems = ( - items: ReadonlyArray<SanitizableLineItem> | undefined +export const sanitizeLineItems = ( + items: ReadonlyArray<SanitizableLineItem> | undefined, + defaultCurrency?: CurrencyEnum ): CreateReceivablesFormBeforeValidationLineItemProps[] => {
344-349: Redundant fallback branch for VAT/tax assignmentThis condition can’t set a value because both result fields are undefined implies
vatRateValueis also undefined. Remove for clarity.- if ( - result.vat_rate_value === undefined && - result.tax_rate_value === undefined && - typeof vatRateValue === 'number' - ) { - result.vat_rate_value = vatRateValue; - } + // no-op: both rates undefined and no numeric source ⇒ keep undefinedexamples/with-nextjs-and-clerk-auth/e2e/utils/purchase-order-helpers.ts (5)
84-92: Scope the “Add row” button to the items table to avoid false clicks.Limit the search to the table container.
- const addRowBtn = page - .getByRole('button', { name: /Add item|Add row|Row/i }) - .first(); + const itemsTable = page.locator('table').first(); + const addRowBtn = itemsTable + .getByRole('button', { name: /Add item|Add row/i }) + .first();
44-44: Match both singular/plural tab labels.Helps with minor copy variations.
- const poTab = page.getByRole('tab', { name: /^Purchase orders$/i }).first(); + const poTab = page.getByRole('tab', { name: /^Purchase orders?$/i }).first();
67-67: Assert the tab is actually selected.Prevents false positives when focus lands but content doesn’t switch.
await expect(poTab).toBeVisible(); + await expect(poTab).toHaveAttribute('aria-selected', 'true');
379-381: Match both singular/plural in Purchase Order tab selector.Reduces brittleness across UI copy variants.
- const poTab = page.getByRole('tab', { name: /Purchase order/i }); + const poTab = page.getByRole('tab', { name: /Purchase orders?/i });
536-539: Avoid Node Buffer; attach JSON as a plain string.Keeps it environment-agnostic and simpler.
- await testInfo.attach(name, { - body: Buffer.from(JSON.stringify(items, null, 2), 'utf-8'), - contentType: 'application/json', - }); + await testInfo.attach(name, { + body: JSON.stringify(items, null, 2), + contentType: 'application/json', + });examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.validation.spec.ts (2)
3-6: Open the PO tab before creating a PO to reduce flakiness.Be consistent with other specs that navigate to the tab first.
-import { - createAndSaveDraft, - openCreatePurchaseOrder, -} from '../utils/purchase-order-helpers'; +import { + createAndSaveDraft, + openCreatePurchaseOrder, + openPurchaseOrdersTab, +} from '../utils/purchase-order-helpers';
23-25: Ensure you’re on the Purchase Orders tab before opening the create flow.Prevents missing entry points when another tab is active.
- await openCreatePurchaseOrder(page); + await openPurchaseOrdersTab(page); + await openCreatePurchaseOrder(page);packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (5)
51-55: TForm generic is unused/inconsistent; tighten types or remove the generic.The component is parameterized with TForm, but internally it hard-codes CreateReceivablesFormBeforeValidationProps in useFormContext and FieldPath usages. This gives a false sense of generic support and can mislead consumers.
Proposed minimal fix: drop the generic locally and export the props type.
-interface ConfigurableItemsSectionProps< - TForm = CreateReceivablesFormBeforeValidationProps, -> { - config: ItemsSectionConfig<TForm>; +export interface ConfigurableItemsSectionProps { + config: ItemsSectionConfig; actualCurrency?: CurrencyEnum; defaultCurrency?: CurrencyEnum; isNonVatSupported: boolean; isVatSelectionDisabled?: boolean; registerLineItemCleanupFn?: (fn: (() => void) | null) => void; renderErrorDisplay?: (result: { generalError?: string | null; fieldErrors: Record<string, string | null | undefined>; hasErrors: boolean; }) => ReactNode; } -export function ConfigurableItemsSection< - TForm = CreateReceivablesFormBeforeValidationProps, ->({ +export function ConfigurableItemsSection({ config, defaultCurrency = 'USD', actualCurrency = defaultCurrency, isNonVatSupported, isVatSelectionDisabled, registerLineItemCleanupFn, renderErrorDisplay, -}: ConfigurableItemsSectionProps<TForm>) { +}: ConfigurableItemsSectionProps) {Also applies to: 67-81
21-36: New component uses MUI; add TODO to migrate to shadcn/ui per guidelines.Given this is a new component, our guidelines prefer shadcn/ui + Tailwind (mtw- classes). Since child components are still MUI, add a TODO to plan migration when feasible.
As per coding guidelines
import { Button, Typography, Table, @@ CircularProgress, } from '@mui/material'; +// TODO(DEV-16185): This component currently relies on MUI because dependent children are MUI-based. +// Prefer shadcn/ui and Tailwind (mtw- classes) for new components; migrate when dependents are ready.
197-205: Guard staticMeasureUnits default when empty.If staticMeasureUnits is provided as an empty array, config.staticMeasureUnits[0] is undefined and will be written to the form. Add a length check.
- if (config.staticMeasureUnits) { - const unitValue = (itemMeasureUnitName || - config.staticMeasureUnits[0]) as FieldPathValue< + if (config.staticMeasureUnits) { + const fallback = + config.staticMeasureUnits.length > 0 + ? config.staticMeasureUnits[0] + : undefined; + const unitValue = (itemMeasureUnitName || fallback) as FieldPathValue< CreateReceivablesFormBeforeValidationProps, typeof measureUnitField >; setValueWithoutValidation(measureUnitField, unitValue); return; }
436-441: Remove no-op await.await Promise.resolve() is a no-op and obscures intent. If you need to defer until state updates flush, consider queueMicrotask or a documented reason.
- await Promise.resolve();
703-711: Clarify button label.“Row” is terse. “Add row” improves clarity and i18n.
- {t(i18n)`Row`} + {t(i18n)`Add row`}packages/sdk-react/src/components/shared/ItemsSection/index.ts (1)
1-3: Re-export props type for DX.Expose ConfigurableItemsSectionProps so consumers can type props without importing from the implementation file.
export { ManualItemSelector } from './ManualItemSelector'; export { ConfigurableItemsSection } from './ItemsSection'; +export type { ConfigurableItemsSectionProps } from './ItemsSection'; export * from './types';packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx (4)
22-23: Narrow currency type to API enum for stronger type safetyUse the OpenAPI enum for currency instead of a plain string.
- currency?: string; + currency?: components['schemas']['CurrencyEnum'];
73-86: Replace inline alignment style with Tailwind utilitiesAvoid inline styles per project guidelines; use Tailwind for centering.
[As per coding guidelines]
- <div - ref={containerRef} - className={cn( + <div + ref={containerRef} + className={cn( // Container layout - 'mtw:flex mtw:overflow-auto mtw:relative', + 'mtw:flex mtw:overflow-auto mtw:relative', 'mtw:w-full mtw:h-full mtw:min-h-0', - 'mtw:p-12', // 48px padding + 'mtw:p-12', // 48px padding 'mtw:bg-gradient-to-br mtw:from-slate-50 mtw:to-slate-200', + // Center content + 'mtw:justify-center mtw:items-center', // Force pixel-snapping for zoom stability - 'mtw:[transform:translateZ(0)]' - )} - style={{ - justifyContent: 'safe center', - alignItems: 'safe center', - }} - > + 'mtw:[transform:translateZ(0)]' + )} + >
88-107: Avoid inline transform; use CSS variable + Tailwind arbitrary propertyKeep styling in classes; expose scale via a CSS custom property.
[As per coding guidelines]
- <AspectRatio + <AspectRatio ratio={PURCHASE_ORDER_CONSTANTS.A4_PAPER_RATIO} className={cn( // Maximum width constraints for A4 'mtw:max-w-[21cm]', // Minimum dimensions for proper scaling (A4: 794×1123px = 21cm × 29.7cm) 'mtw:min-w-[794px] mtw:min-h-[1123px]', // Responsive scaling with our adaptive scale hook - 'mtw:transition-transform mtw:duration-200 mtw:ease-in-out', + 'mtw:transition-transform mtw:duration-200 mtw:ease-in-out', // Respect reduced motion preferences 'motion-reduce:mtw:transition-none', - 'mtw:origin-center', + 'mtw:origin-center', // Prevent unwanted shrinking and layout shifts 'mtw:flex-shrink-0', // CSS containment for layout stability - 'mtw:[contain:"layout style"]' + 'mtw:[contain:"layout style"]', + // Dynamic transform uses a CSS variable + 'mtw:[transform:scale(var(--po-scale))]' )} - style={{ - // Dynamic transform must stay as inline style due to variable - transform: `scale(${scale})`, - }} + style={ + { '--po-scale': String(scale) } as React.CSSProperties + } >Add React type import:
-import { useMemo, useRef } from 'react'; +import { useMemo, useRef } from 'react'; +import type React from 'react';
26-30: Unused VAT props — consider removal or forwardingentityVatIds and counterpartVats are accepted but unused. Either drop them from the public API or forward them to the Monite preview if needed later; otherwise they’re dead parameters.
Also applies to: 39-41
packages/sdk-react/src/components/approvalPolicies/useApprovalPolicyTrigger.test.tsx
Outdated
Show resolved
Hide resolved
.../src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/PreviewPurchaseOrderEmail.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/PreviewPurchaseOrderEmail.tsx
Show resolved
Hide resolved
| export const FullfillmentSummary = ({ | ||
| disabled, | ||
| }: PurchaseOrderTermsSummaryProps) => { | ||
| const { i18n } = useLingui(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix misspelling and align export with filename
Rename the component to avoid the “Fullfillment” typo and to match the file name for discoverability.
-export const FullfillmentSummary = ({
+export const PurchaseOrderTermsSummary = ({
disabled,
}: PurchaseOrderTermsSummaryProps) => {As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const FullfillmentSummary = ({ | |
| disabled, | |
| }: PurchaseOrderTermsSummaryProps) => { | |
| const { i18n } = useLingui(); | |
| export const PurchaseOrderTermsSummary = ({ | |
| disabled, | |
| }: PurchaseOrderTermsSummaryProps) => { | |
| const { i18n } = useLingui(); |
🤖 Prompt for AI Agents
In
packages/sdk-react/src/components/payables/PurchaseOrders/sections/PurchaseOrderTermsSummary.tsx
around lines 15-18, the exported component is misspelled as
"FullfillmentSummary" and does not match the file name; rename the exported
component to "PurchaseOrderTermsSummary" (fixing the typo and aligning with the
file name), update all internal references and any imports throughout the
codebase to the new name, and ensure types/props and exports remain consistent
before running the build/tests to confirm no import errors.
...ages/sdk-react/src/components/payables/PurchaseOrders/sections/PurchaseOrderTermsSummary.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/sections/VendorSection.tsx
Show resolved
Hide resolved
costa-monite
force-pushed
the
feat/DEV-16185-purchase-orders
branch
from
1d254da to
acc166b
Compare
costa-monite
had a problem deploying
to
common
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsx (1)
143-148: RestoreRecurrenceCancelModalimport to fix compile error
RecurrenceCancelModalis still referenced here but its import was removed, so this file now fails to compile (Cannot find name 'RecurrenceCancelModal'). Please reintroduce the import from./RecurrenceCancelModal.import { MarkAsUncollectibleModal } from './MarkAsUncollectibleModal'; import { RecordManualPaymentModal } from './RecordManualPaymentModal'; +import { RecurrenceCancelModal } from './RecurrenceCancelModal';
🧹 Nitpick comments (41)
packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderModals.tsx (1)
16-20: Use boolean prop shorthandMinor JSX cleanup.
- isDialog={true} + isDialogpackages/sdk-react/src/components/templateSettings/TemplateSettings.tsx (1)
27-29: Use shared SelectableDocumentType instead of re-declaring the unionAvoids drift and centralizes the type.
+import type { SelectableDocumentType } from './types'; @@ - /** Document type context for filtering templates */ - documentType?: 'receivable' | 'purchase_order'; + /** Document type context for filtering templates */ + documentType?: SelectableDocumentType;packages/sdk-react/src/components/templateSettings/components/LayoutAndLogo.tsx (1)
30-31: Optional: rename invoiceTemplates to templatesName no longer reflects content when filtering purchase orders.
- invoiceTemplates, + invoiceTemplates as templates, @@ - <TemplatesSelection - templates={invoiceTemplates} + <TemplatesSelection + templates={templates}Note: Adjust prop names/types if needed across the call site.
packages/sdk-react/src/components/shared/ItemsSection/GenericQuantityField.tsx (1)
50-74: Consider shadcn/ui Select for consistencyNew UI should use shadcn/ui components; replacing the native select with '@/ui/components/select' would align with guidelines and styling.
As per coding guidelines
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (2)
21-36: New shared UI uses MUI; add TODO to migrate to shadcn/ui + TailwindPer sdk-react rules, new components should use shadcn/ui and mtw:* classes. If keeping MUI to limit scope, add a TODO to track migration.
As per coding guidelines
Apply:+// TODO: This new shared ItemsSection uses MUI. Per sdk-react guidelines, migrate to shadcn/ui + Tailwind (mtw:*) when feasible. import AddIcon from '@mui/icons-material/Add';
79-81: Type generic not fully applied (informational)useFormContext is bound to CreateReceivablesFormBeforeValidationProps, limiting generic TForm benefits. Consider parameterizing useFormContext and FieldPath usages in a follow-up.
Based on learnings
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/InvoiceItemRow.tsx (2)
7-15: MUI usage is fine here; add a TODO to migrate later to shadcn/ui and lucide icons.Per guidelines, keep existing MUI; add a TODO for future migration to shadcn/ui and swap the MUI Delete icon to lucide-react during that migration.
As per coding guidelines
79-83: Unify error source for tax to avoid divergent checks.Use the computed
taxOrVatErrorfor the non-VAT path as well to keep a single source of truth.Apply outside this range in the TaxRateField props:
- error={Boolean(errors.line_items?.[index]?.tax_rate_value)} + error={taxOrVatError}packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/useExistingPurchaseOrderDetails.tsx (2)
108-112: Simplify/sanitize PDF visibility logicYou restrict to status === 'issued' after already checking allowed statuses, making 'draft' effectively unreachable. Simplify to the allowed-status + ready check.
- const isDownloadPDFButtonVisible = - isAllowedDownloadStatus(purchaseOrder?.status) && - isPdfReady && - purchaseOrder?.status === 'issued'; + const isDownloadPDFButtonVisible = + isAllowedDownloadStatus(purchaseOrder?.status) && isPdfReady;
115-121: Dead state: delete disabled state computed but never shownisDeleteButtonVisible is always false; computing isDeleteButtonDisabled is redundant. Either compute only when visible, or wire visibility.
- const isDeleteButtonVisible = false; - const isDeleteButtonDisabled = - purchaseOrder?.status !== 'draft' || - isDeleteAllowedLoading || - !isDeleteAllowed || - mutationInProgress; + const isDeleteButtonVisible = false; + const isDeleteButtonDisabled = false;packages/sdk-react/src/components/payables/PurchaseOrders/consts.ts (1)
43-46: Avoid hardcoding fallback currencyUsing 'USD' may diverge from the entity’s default currency. Prefer deriving from entity or a central default.
Add a TODO to source from Monite context (entity default) instead of hardcoding.
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/components/useCreateInvoiceProductsTable.ts (1)
65-79: Round to integer minor units before constructing PriceVAT math can produce fractional minor units. Round to avoid formatting inconsistencies.
- const priceObj = new Price({ - value: price, + const priceObj = new Price({ + value: Math.round(price), currency: currency as CurrencyEnum, formatter: formatCurrencyToDisplay, });- const priceObj = new Price({ - value: taxes, + const priceObj = new Price({ + value: Math.round(taxes), currency: currency as CurrencyEnum, formatter: formatCurrencyToDisplay, });Also applies to: 163-169
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/hooks/useLineItemManagement.ts (2)
79-85: Heavy dependency: JSON.stringify as memo keyJSON.stringify on line_items each render can be costly. Consider a deep-compare ref or stable hashing util to reduce overhead on large tables.
345-353: Initial row logic: simplify and avoid double flagsYou set initialRowAdded in both branches; can early-return after append to reduce branches.
- if (!hasValuesFromForm && !hasFieldsFromDefaultValues && !isAddingRow.current) { + if (!hasValuesFromForm && !hasFieldsFromDefaultValues && !isAddingRow.current) { isAddingRow.current = true; append(createEmptyRow()); setAutoAddedRows([0]); setInitialRowAdded(true); - } else { - setInitialRowAdded(true); - } + return; + } + setInitialRowAdded(true);packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderDetails.tsx (1)
61-68: Refine NotFound condition to avoid masking non-404 errorsTreating any error as “not found” can hide permission/auth/network issues. Consider checking for 404 (or explicit “not found”) and otherwise render an error state.
packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderForm.tsx (2)
20-22: Type actualCurrency as CurrencyEnum instead of castingAvoid repeated assertions by making the prop strongly typed.
interface CreatePurchaseOrderFormComponentProps { @@ - actualCurrency?: string; + actualCurrency?: components['schemas']['CurrencyEnum']; isNonVatSupported?: boolean; }config={PURCHASE_ORDERS_ITEMS_CONFIG} - actualCurrency={ - actualCurrency as components['schemas']['CurrencyEnum'] - } - defaultCurrency={ - actualCurrency as components['schemas']['CurrencyEnum'] - } + actualCurrency={actualCurrency} + defaultCurrency={actualCurrency}As per coding guidelines
Also applies to: 41-45
12-22: RemovecounterpartAddressesfrom CreatePurchaseOrderFormComponentProps
The prop is declared but never used inCreatePurchaseOrderForm.tsx; drop it to simplify the component API.packages/sdk-react/src/components/payables/Payables.tsx (1)
205-207: Optional: prefer project spinner over MUI CircularProgress in new code pathsYou’ve added new CircularProgress usages. Given ongoing migration to shadcn/tailwind, consider a TODO to replace with the project’s spinner for consistency.
Also applies to: 248-249, 323-323, 341-341
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsx (2)
58-69: Round tax to integer minor units to avoid fractional penniesThe tax computation can produce fractional minor units. Round once before summing to stabilize formatting.
Apply:
- const totalTax = + const totalTaxRaw = purchaseOrder.items?.reduce( (sum, item) => sum + ((item.quantity ?? 0) * (item.price ?? 0) * vatRateBasisPointsToPercentage(item.vat_rate ?? 0)) / 100, 0 ) || 0; - const totalAmount = subtotal + totalTax; + const totalTax = Math.round(totalTaxRaw); + const totalAmount = subtotal + totalTax;
125-127: Prefer shared constant for default PO numberUse the shared constant for consistency.
Apply:
- value: purchaseOrder.document_id ?? t(i18n)`PO-auto`, + value: purchaseOrder.document_id ?? PURCHASE_ORDER_CONSTANTS.DEFAULT_PO_DOCUMENT_ID,And add import at the top of this file:
import { PURCHASE_ORDER_CONSTANTS } from '@/components/payables/PurchaseOrders/consts';packages/sdk-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.tsx (2)
263-273: Replace MUI sx with Tailwind classes (avoid inline styles/sx)Per guidelines, move styles to Tailwind classes.
Apply:
- <DialogContent - className={className + '-Content'} - sx={{ - mt: presentation === FormPresentation.Preview ? 0 : 4, - p: presentation === FormPresentation.Preview ? 0 : '0 32px 32px 32px', - height: '100%', - width: '100%', - display: 'flex', - flexDirection: 'column', - }} - > + <DialogContent + className={ + className + + '-Content ' + + (presentation === FormPresentation.Preview ? '' : 'mtw:mt-4 mtw:px-8 mtw:pb-8') + + ' mtw:h-full mtw:w-full mtw:flex mtw:flex-col' + } + >As per coding guidelines
280-282: Use shared LoadingSpinner instead of MUI CircularProgressKeep loading visuals consistent and remove MUI usage.
Apply:
- <CenteredContentBox className="Monite-LoadingPage"> - <CircularProgress /> - </CenteredContentBox> + <CenteredContentBox className="Monite-LoadingPage"> + <LoadingSpinner /> + </CenteredContentBox>Add import:
import { LoadingSpinner } from '@/ui/LoadingSpinner';As per coding guidelines
packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrdersTable.tsx (1)
187-205: Hide/disable create actions when user lacks create permissionEnsure empty-state CTA and onCreate handler respect canCreatePO.
Apply:
- actionButtonLabel={ - !isFiltering && !isSearching - ? t(i18n)`Create Purchase Order` - : undefined - } + actionButtonLabel={ + !isFiltering && !isSearching && canCreatePO + ? t(i18n)`Create Purchase Order` + : undefined + } - actionOptions={ - !isFiltering && !isSearching - ? [t(i18n)`Create Purchase Order`] - : undefined - } + actionOptions={ + !isFiltering && !isSearching && canCreatePO + ? [t(i18n)`Create Purchase Order`] + : undefined + } onCreate={(type) => { if ( type === t(i18n)`Create Purchase Order` || type === 'Create Purchase Order' ) { - setIsCreatePurchaseOrderDialogOpen?.(true); + if (canCreatePO) { + setIsCreatePurchaseOrderDialogOpen?.(true); + } } }}As per coding guidelines
packages/sdk-react/src/components/payables/PurchaseOrders/CreatePurchaseOrder.tsx (1)
516-575: Container layout: replace MUI Stack/Box wrapper with div + TailwindAvoid new MUI primitives; keep existing classes.
Apply:
- <Stack direction="row" className="mtw:max-h-screen mtw:overflow-hidden"> - <Box className="mtw:w-1/2 mtw:h-screen mtw:flex mtw:flex-col"> + <div className="mtw:flex mtw:flex-row mtw:max-h-screen mtw:overflow-hidden"> + <div className="mtw:w-1/2 mtw:h-screen mtw:flex mtw:flex-col"> @@ - </div> - </Box> + </div> + </div>And close the outer container accordingly at the bottom (replace with ).
As per coding guidelinespackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/ExistingPurchaseOrderDetails.tsx (4)
24-34: New component uses MUI primitives; prefer shadcn/ui + Tailwind.Per sdk-react guidelines, new UI should use shadcn/ui components and Tailwind (mtw-*) utilities; avoid MUI. If full migration is out-of-scope now, add a TODO and plan a follow-up to replace DialogTitle/DialogContent/Grid/Stack/Toolbar/Typography/Alert/CircularProgress with shadcn/ui equivalents and Tailwind classes. As per coding guidelines.
295-299: Replace MUI sx styles with Tailwind classes.Avoid inline/sx styling. Use Tailwind utilities on className.
Apply this diff:
- <DialogContent - className={className + '-Content'} - sx={{ display: 'flex', flexDirection: 'column' }} - > + <DialogContent className={`${className}-Content mtw:flex mtw:flex-col`}>
164-173: Minor: remove redundant optional chaining.Inside the guarded block, dialogContext is truthy; use dialogContext.onClose directly.
- onClick={dialogContext?.onClose} + onClick={dialogContext.onClose}
83-88: Avoid duplicating permission checks.isUpdateAllowed is computed here and inside useExistingPurchaseOrderDetails. Prefer deriving all button/section gating from the hook to avoid drift.
Also applies to: 311-324
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/OverviewTabPanel.tsx (1)
35-41: Prefer shadcn/ui + Tailwind over MUI in new components.Box/Skeleton/Typography should be replaced with shadcn/ui equivalents and Tailwind utilities. If deferring, add a TODO.
As per coding guidelines
Also applies to: 47-55
packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderStatusChip.tsx (1)
4-4: Use lucide-react icon instead of MUI icons.Follow icon guideline; replace @mui/icons-material with lucide-react.
-import { Circle } from '@mui/icons-material'; +import { Circle } from 'lucide-react'; @@ - const chipIcon = useMemo( - () => (icon ? <Circle sx={{ fontSize: '10px !important' }} /> : undefined), - [icon] - ); + const chipIcon = useMemo( + () => (icon ? <Circle size={10} /> : undefined), + [icon] + );As per coding guidelines
Also applies to: 83-86
packages/sdk-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.form.components.tsx (3)
147-153: Remove inline styles; use Tailwind utilities.Avoid style props. You already use mtw:py-* — drop the inline paddings.
- <Card - className="mtw:mb-6 mtw:border-[#dedede] mtw:rounded-lg mtw:shadow-none mtw:py-0" - style={{ paddingTop: 0, paddingBottom: 0 }} - > + <Card className="mtw:mb-6 mtw:border-[#dedede] mtw:rounded-lg mtw:shadow-none mtw:py-0"> @@ - <Card - className="mtw:border-[rgba(0,0,0,0.13)] mtw:rounded-lg mtw:shadow-none mtw:min-h-[320px] mtw:flex mtw:flex-col" - style={{ paddingTop: 0, paddingBottom: 0 }} - > + <Card className="mtw:border-[rgba(0,0,0,0.13)] mtw:rounded-lg mtw:shadow-none mtw:min-h-[320px] mtw:flex mtw:flex-col">Also applies to: 165-170
107-110: Replace raw colors with Tailwind tokens.Use Tailwind palette (e.g., mtw:text-foreground, mtw:text-muted-foreground) instead of hex/rgba literals.
- <SelectTrigger className="mtw:w-full mtw:border-none mtw:bg-transparent mtw:p-1 mtw:text-base mtw:font-normal mtw:text-[#292929] mtw:leading-6 focus:mtw:ring-0 focus:mtw:outline-none"> + <SelectTrigger className="mtw:w-full mtw:border-none mtw:bg-transparent mtw:p-1 mtw:text-base mtw:font-normal mtw:text-foreground mtw:leading-6 focus:mtw:ring-0 focus:mtw:outline-none"> @@ - <Label className="mtw:min-w-[52px] mtw:text-sm mtw:font-medium mtw:text-[rgba(0,0,0,0.56)] mtw:leading-5"> + <Label className="mtw:min-w-[52px] mtw:text-sm mtw:font-medium mtw:text-muted-foreground mtw:leading-5"> @@ - <Label className="mtw:min-w-[52px] mtw:text-sm mtw:font-medium mtw:text-[rgba(0,0,0,0.56)] mtw:leading-5"> + <Label className="mtw:min-w-[52px] mtw:text-sm mtw:font-medium mtw:text-muted-foreground mtw:leading-5"> @@ - className="mtw:w-full mtw:border-none mtw:bg-transparent mtw:p-1 mtw:text-base mtw:font-normal mtw:text-[#292929] mtw:leading-6 focus:mtw:ring-0 focus:mtw:outline-none" + className="mtw:w-full mtw:border-none mtw:bg-transparent mtw:p-1 mtw:text-base mtw:font-normal mtw:text-foreground mtw:leading-6 focus:mtw:ring-0 focus:mtw:outline-none" @@ - className="mtw:min-h-[200px] mtw:border-none mtw:bg-transparent mtw:p-0 mtw:text-base mtw:font-normal mtw:text-[#292929] mtw:leading-6 mtw:resize-y focus:mtw:ring-0 focus:mtw:outline-none" + className="mtw:min-h-[200px] mtw:border-none mtw:bg-transparent mtw:p-0 mtw:text-base mtw:font-normal mtw:text-foreground mtw:leading-6 mtw:resize-y focus:mtw:ring-0 focus:mtw:outline-none"As per coding guidelines
Also applies to: 154-156, 173-176, 184-186, 210-213
70-87: Consider avoiding redundant fetch in RecipientSelector.You already have purchase order context upstream; passing counterpart_id directly would remove an extra query here.
packages/sdk-react/src/components/payables/PurchaseOrders/validation.ts (1)
150-153: Optional: enum message config.z.enum options may not honor a message directly; if you need a custom message, wrap with preprocess or refine.
packages/sdk-react/src/components/payables/PurchaseOrders/components/FormErrorDisplay.tsx (2)
1-7: New UI should use shadcn/ui + Tailwind, not MUI.Replace Alert/Collapse/List with shadcn/ui primitives and Tailwind utilities; avoid sx.
As per coding guidelines
Also applies to: 4-5
36-52: Remove sx styles; use Tailwind classes.Convert layout spacing and colors to Tailwind classes on container elements.
Example:
- <Collapse - in={true} - sx={{ - ':not(.MuiCollapse-hidden)': { - marginBottom: 1, - }, - }} - > - <Alert - severity="error" - sx={{ - '& .MuiAlert-icon': { - alignItems: 'center', - }, - }} - > + <div className="mtw:mb-2"> + <div className="mtw:border mtw:border-destructive/20 mtw:bg-destructive/5 mtw:text-destructive mtw:rounded mtw:p-3"> {generalError && <div>{generalError}</div>} @@ - <List dense disablePadding sx={{ mt: generalError ? 1 : 0 }}> + <ul className={`mtw:mt-${generalError ? '[4px]' : '0'} mtw:space-y-1`}> {Object.entries(fieldErrors).map(([key, error]) => error ? ( - <ListItem key={key} disablePadding> - <ListItemText - primary={error} - primaryTypographyProps={{ - sx: { color: 'inherit' }, - }} - /> - </ListItem> + <li key={key} className="mtw:text-sm">{error}</li> ) : null )} - </List> + </ul> @@ - </Alert> - </Collapse> + </div> + </div>Also applies to: 55-70
packages/sdk-react/src/components/payables/PurchaseOrders/config/tableConfig.tsx (2)
124-143: Split compute vs format for Amount column.Return a numeric total from valueGetter and format in valueFormatter. This keeps data vs presentation separate and preserves flexibility.
Apply:
{ field: 'amount', sortable: false, headerAlign: 'right', align: 'right', headerName: t(i18n)`Amount`, width: TABLE_COLUMN_WIDTHS.AMOUNT, - valueGetter: ( - row: components['schemas']['PurchaseOrderResponseSchema'] - ) => { - const totalAmount = - row.items?.reduce( - (sum: number, item: NonNullable<typeof row.items>[number]) => - sum + (item.quantity || 0) * (item.price || 0), - 0 - ) || 0; - return row.currency - ? (formatCurrencyToDisplay(totalAmount, row.currency) ?? '') - : ''; - }, + valueGetter: (row: components['schemas']['PurchaseOrderResponseSchema']) => { + return ( + row.items?.reduce( + (sum: number, item: NonNullable<typeof row.items>[number]) => + sum + (item.quantity || 0) * (item.price || 0), + 0 + ) ?? 0 + ); + }, + valueFormatter: (value: unknown, row) => { + return row.currency + ? formatCurrencyToDisplay(value as number, row.currency) ?? '' + : ''; + }, },
82-88: Avoid magic number for Vendor column width.Consider adding TABLE_COLUMN_WIDTHS.VENDOR and using it here for consistency with other widths.
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderDeleteModal.tsx (2)
43-55: Pre-bind default variables on mutation; simplify handler.Align with other hooks by passing header/path as default variables so callers can just call mutate().
Apply:
const deleteMutation = api.payablePurchaseOrders.deletePayablePurchaseOrdersId.useMutation( - undefined, + { + header: { 'x-monite-entity-id': entityId }, + path: { purchase_order_id: id }, + }, { onSuccess: () => { onDelete?.(id); onClose(); }, onError: (error) => { setError(getAPIErrorMessage(i18n, error)); }, } ); const handleDelete = () => { setError(null); - deleteMutation.mutate({ - header: { 'x-monite-entity-id': entityId }, - path: { purchase_order_id: id }, - }); + deleteMutation.mutate(); };Also applies to: 57-63
66-71: Reset error on close.Prevents stale error messages if the dialog is reopened.
Apply:
<Dialog open={open} onOpenChange={(isOpen) => { - if (!isOpen) onClose(); + if (!isOpen) { + setError(null); + onClose(); + } }} >packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx (1)
46-49: Centralize and type currency for formatting.Pre-compute a typed displayCurrency to avoid repeated fallback/casts and ensure type-safety with formatCurrencyToDisplay.
Apply:
const { i18n } = useLingui(); const { locale } = useMoniteContext(); const { formatCurrencyToDisplay } = useCurrencies(); + const displayCurrency = ((currency ?? 'USD') as components['schemas']['CurrencyEnum']);- {formatCurrencyToDisplay(price, currency || 'USD', true)} + {formatCurrencyToDisplay(price, displayCurrency, true)}- {formatCurrencyToDisplay( - totalAmount, - currency || 'USD', - true - )} + {formatCurrencyToDisplay(totalAmount, displayCurrency, true)}- {currency && - formatCurrencyToDisplay( - taxAmount as number, - currency, - true - )} + {formatCurrencyToDisplay(taxAmount as number, displayCurrency, true)}- {t(i18n)`TOTAL`} ({currency || 'USD'}) + {t(i18n)`TOTAL`} ({displayCurrency})Also applies to: 249-251, 253-257, 292-297, 304-305
...ges/sdk-react/src/components/payables/PurchaseOrders/components/PurchaseOrderVatModeMenu.tsx
Show resolved
Hide resolved
...ges/sdk-react/src/components/payables/PurchaseOrders/components/PurchaseOrderVatModeMenu.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.tsx
Outdated
Show resolved
Hide resolved
...ponents/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/OverviewTabPanel.tsx
Outdated
Show resolved
Hide resolved
...s/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderStatusChip.tsx
Show resolved
Hide resolved
...ages/sdk-react/src/components/payables/PurchaseOrders/sections/PurchaseOrderTermsSummary.tsx
Outdated
Show resolved
Hide resolved
...nts/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/InvoiceItemRow.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts (1)
72-75: Fix template typings for purchase-order support
TemplateDocumentandsetDefaultTemplatestay hard-wired toTemplateReceivableResponse, so any purchase-order template returned from the API is being forced through the receivable type. We lose the'purchase_order'discriminant and the PO-specific fields, defeating the goal of the newdocumentTypeparameter and violating our SDK typing guidelines. Please widen the aliases and reuse them in the mutation signature.- const setDefaultTemplate = async ( - id: components['schemas']['TemplateReceivableResponse']['id'], - name: components['schemas']['TemplateReceivableResponse']['name'] - ) => { + const setDefaultTemplate = async ( + id: TemplateDocument['id'], + name: TemplateDocument['name'] + ) => {-type TemplateDocumentType = - | components['schemas']['DocumentTypeEnum'] - | 'invoice'; -type TemplateDocument = components['schemas']['TemplateReceivableResponse']; +type TemplateDocumentType = + | components['schemas']['DocumentTypeEnum'] + | 'invoice'; +type TemplateDocument = + | components['schemas']['TemplateReceivableResponse'] + | components['schemas']['TemplatePurchaseOrderResponse'];This keeps the hook aligned with the OpenAPI schema for both receivables and purchase orders.
As per coding guidelines
Also applies to: 101-104
🧹 Nitpick comments (20)
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx (2)
27-28: Optional: avoid constructing header when entityId is falsyEven though the query is disabled, avoid passing an undefined header value to keep inputs clean.
} = api.payablePurchaseOrders.getPayablePurchaseOrdersId.useQuery( { path: { purchase_order_id: purchaseOrderId, }, - header: { 'x-monite-entity-id': entityId }, + ...(entityId && { + header: { 'x-monite-entity-id': entityId }, + }), }, { enabled: Boolean(entityId && purchaseOrderId) } );As per coding guidelines
36-41: Use shadcn/ui Alert for error statePrefer the design system’s Alert component instead of a plain div for consistency and accessibility.
-import { getAPIErrorMessage } from '@/core/utils/getAPIErrorMessage'; +import { getAPIErrorMessage } from '@/core/utils/getAPIErrorMessage'; +import { Alert, AlertDescription, AlertTitle } from '@/ui/components/alert'; @@ - if (error) { - return ( - <div className="mtw:text-sm mtw:text-destructive"> - {getAPIErrorMessage(i18n, error)} - </div> - ); - } + if (error) { + return ( + <Alert variant="destructive"> + <AlertTitle>{t(i18n)`Error`}</AlertTitle> + <AlertDescription> + {getAPIErrorMessage(i18n, error)} + </AlertDescription> + </Alert> + ); + }As per coding guidelines
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsx (2)
106-114: Avoid rendering undefined address segmentsGuard state/postal_code joins to prevent “City, undefined undefined”.
- <> - {billingAddress.line1} - {billingAddress.line2 && `, ${billingAddress.line2}`} - <br /> - {billingAddress.city}, {billingAddress.state}{' '} - {billingAddress.postal_code} - <br /> - {billingAddress.country} - </> + <> + {billingAddress.line1} + {billingAddress.line2 ? `, ${billingAddress.line2}` : null} + <br /> + {[ + billingAddress.city, + billingAddress.state, + billingAddress.postal_code, + ] + .filter(Boolean) + .join(', ')} + <br /> + {billingAddress.country} + </>
164-169: Handle null from formatCurrencyToDisplayformatCurrencyToDisplay may return null; add a fallback to avoid empty cells.
{ label: t(i18n)`Subtotal`, - value: formatCurrencyToDisplay( + value: formatCurrencyToDisplay( subtotal, purchaseOrder.currency - ), + ) ?? '—', }, @@ { label: t(i18n)`Tax`, - value: formatCurrencyToDisplay( + value: formatCurrencyToDisplay( totalTax, purchaseOrder.currency - ), + ) ?? '—', }, @@ { label: t(i18n)`Total`, value: ( <div className="mtw:text-base mtw:font-semibold"> - {formatCurrencyToDisplay( + {formatCurrencyToDisplay( totalAmount, purchaseOrder.currency - )} + ) ?? '—'} </div> ), },Also applies to: 173-177, 185-189
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx (2)
74-82: Unify currency fallback to avoid USD mismatch in totalsIf
currencyprop is undefined but line items carry a currency, the totals/tax rows may format amounts as USD while subtotals derive currency from items. Derive a singledisplayCurrencyfrom items and reuse it.Apply:
const { subtotalPrice, totalPrice, taxesByVatRate } = useCreateInvoiceProductsTable({ lineItems: sanitizedItems, formatCurrencyToDisplay, isNonVatSupported, actualCurrency: currency ?? 'USD', isInclusivePricing: false, }); + const displayCurrency = + currency ?? + sanitizedItems.find((field) => Boolean(field.product?.price?.currency)) + ?.product?.price?.currency ?? + 'USD';- <span className={styles.totalValue}> - {currency && - formatCurrencyToDisplay( - taxAmount as number, - currency, - true - )} - </span> + <span className={styles.totalValue}> + {displayCurrency && + formatCurrencyToDisplay(taxAmount as number, displayCurrency, true)} + </span>- {t(i18n)`TOTAL`} ({currency || 'USD'}) + {t(i18n)`TOTAL`} ({displayCurrency})Also applies to: 291-297, 303-304
2-2: Styling diverges from Tailwind-only guidelineNew UI should prefer shadcn/ui + Tailwind (
mtw:*) over CSS modules. Given preview templates reuse existing invoice styles, consider adding a TODO to migrate styles to Tailwind utility classes when feasible, keeping current behavior for now.As per coding guidelines
Also applies to: 14-14
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx (2)
75-81: Avoid static inline styles; use Tailwind utilitiesReplace static
justifyContent/alignItemswith Tailwind classes; keep the dynamic transform inline.className={cn( // Container layout - 'mtw:flex mtw:overflow-auto mtw:relative', + 'mtw:flex mtw:overflow-auto mtw:relative mtw:justify-center mtw:items-center', 'mtw:w-full mtw:h-full mtw:min-h-0', 'mtw:p-12', // 48px padding 'mtw:bg-gradient-to-br mtw:from-slate-50 mtw:to-slate-200', // Force pixel-snapping for zoom stability 'mtw:[transform:translateZ(0)]' )} - style={{ - justifyContent: 'safe center', - alignItems: 'safe center', - }} +As per coding guidelines
Also applies to: 82-85
134-135: Centralize CurrencyEnum typeImport
CurrencyEnumfrom the existing Purchase Orders types module to avoid per-file aliases.Example:
- import type { CurrencyEnum } from '../../types';
- remove the local alias.
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (4)
498-513: Avoid setting display:flex on ; wrap inner content instead.Setting display:flex on a td can cause table layout/sticky header quirks. Wrap a Box inside the cell.
- <TableCell - sx={{ - paddingLeft: 2, - paddingRight: 2, - display: 'flex', - alignItems: 'center', - gap: 2, - }} - > - {t(i18n)`Price`} - {config.features.vatModeMenu && ( - <VatModeMenu disabled={isVatSelectionDisabled} /> - )} - </TableCell> + <TableCell sx={{ paddingLeft: 2, paddingRight: 2 }}> + <Box sx={{ display: 'flex', alignItems: 'center', gap: 2 }}> + {t(i18n)`Price`} + {config.features.vatModeMenu && ( + <VatModeMenu disabled={isVatSelectionDisabled} /> + )} + </Box> + </TableCell>
391-400: Confirm unit of ProductItem.vat_rate_value; convert to percent if basis points.If item.vat_rate_value is in basis points, you should convert before storing (consistent with onLineItemValueChangeAdapter).
- setValueWithoutValidation( - vatRateValueField, - item.vat_rate_value as FieldPathValue< - CreateReceivablesFormBeforeValidationProps, - typeof vatRateValueField - > - ); + { + const raw = item.vat_rate_value!; + const percent = + typeof raw === 'number' && raw >= 100 + ? vatRateBasisPointsToPercentage(raw) + : raw; + setValueWithoutValidation( + vatRateValueField, + percent as FieldPathValue< + CreateReceivablesFormBeforeValidationProps, + typeof vatRateValueField + > + ); + }If ProductItem.vat_rate_value is already a percent, keep current logic.
432-436: Remove no-op await Promise.resolve().This yields the microtask queue without purpose and adds confusion.
- await Promise.resolve(); - if (config.features.autoAddRows) { handleAutoAddRow(); }
454-476: Inline sx styles and MUI inputs conflict with Tailwind/shadcn guidelines.Replace sx-based styling and MUI TextField/Collapse with shadcn/ui components and Tailwind classes (mtw-). Keep as follow-up if migration is scoped separately.
As per coding guidelines
Also applies to: 716-735
examples/with-nextjs-and-clerk-auth/e2e/utils/purchase-order-helpers.ts (8)
2-8: Import Route from @playwright/test to avoid type/version skewUse a single Playwright package for types to prevent subtle mismatches.
import { expect, type Locator, type Page, - type TestInfo, -} from '@playwright/test'; -import type { Route } from 'playwright-core'; + type TestInfo, + type Route, +} from '@playwright/test';
85-86: Tighten the “Add row” button matcher“Row” alone is too broad and can match unrelated controls.
- .getByRole('button', { name: /Add item|Add row|Row/i }) + .getByRole('button', { name: /Add (line )?item|Add row/i })
94-97: Scope to the line items table to avoid touching other tablesAnchor both removal buttons and row iteration to the line-items table (identified by the remove control) to prevent accidental interactions with other tables.
- const removeRowButtons = page - .locator('table tbody [data-testid="remove-item"]') - .or(page.locator('table tbody button', { hasText: /Remove|Delete|Trash/i })); + const itemsTable = page.locator('table:has([data-testid="remove-item"])').first(); + const removeRowButtons = itemsTable + .locator('[data-testid="remove-item"]') + .or(itemsTable.locator('button', { hasText: /Remove|Delete|Trash/i })); @@ - const rows = page.locator('table tbody tr'); - const rowsCount = await rows.count(); + const rows = itemsTable.locator('tbody tr'); + let rowsCount = await rows.count(); @@ await page.evaluate( (count) => console.log('[ensureLineItemValid] rows detected', count), rowsCount ); + + if (rowsCount === 0 && (await addRowBtn.isVisible().catch(() => false))) { + await addRowBtn.click().catch(() => {}); + rowsCount = await rows.count(); + }Also applies to: 116-123
183-185: Prefer name-based price input over positional column indexColumn index is brittle; prioritize the explicit name selector.
- const priceField = await firstVisibleLocator([ - row.locator('td').nth(2).locator('input[type="text"]'), - row.locator(`input[name="line_items.${index}.price"]`), - ]); + const priceField = await firstVisibleLocator([ + row.locator(`input[name="line_items.${index}.price"]`), + row.locator('td').nth(2).locator('input[type="text"]'), + ]);
210-213: Broaden combobox label to cover “Counterpart/Supplier” variantsSome UIs use alternative labels; broaden to increase robustness.
- const vendorCombo = page.getByRole('combobox', { name: /^Vendor$/i }); + const vendorCombo = page.getByRole('combobox', { + name: /(Vendor|Counterpart|Supplier)/i, + });
229-233: Remove unused createNewButtonDead variable; keep code lean.
- const createNewButton = page - .getByRole('button', { name: /Create new (vendor|counterpart|customer)/i }) - .or(page.locator('button:has-text("Create counterpart")')) - .first();
331-333: Implement vendor creation fallback (optional)If no vendors exist, tests will stall. Consider implementing the creation path when options are empty.
I can wire a minimal creation flow (open “Create new …”, fill required fields, confirm) if you confirm the UI steps.
533-536: Avoid Node‑specific Buffer in attachmentsUse TextEncoder for broader runtime compatibility.
- await testInfo.attach(name, { - body: Buffer.from(JSON.stringify(items, null, 2), 'utf-8'), - contentType: 'application/json', - }); + await testInfo.attach(name, { + body: new TextEncoder().encode(JSON.stringify(items, null, 2)), + contentType: 'application/json', + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.spec.ts(1 hunks)examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.validation.spec.ts(1 hunks)examples/with-nextjs-and-clerk-auth/e2e/utils/purchase-order-helpers.ts(1 hunks)packages/sdk-react/src/components/approvalPolicies/useApprovalPolicyTrigger.test.tsx(3 hunks)packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsx(1 hunks)packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx(1 hunks)packages/sdk-react/src/components/payables/PurchaseOrders/config/tableConfig.tsx(1 hunks)packages/sdk-react/src/components/payables/PurchaseOrders/sections/VendorSection.tsx(1 hunks)packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx(1 hunks)packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx(1 hunks)packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/ItemSelector.tsx(8 hunks)packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsx(1 hunks)packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx(1 hunks)packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/ItemSelector.tsx
- examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.validation.spec.ts
- packages/sdk-react/src/components/payables/PurchaseOrders/config/tableConfig.tsx
- examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.spec.ts
- packages/sdk-react/src/components/approvalPolicies/useApprovalPolicyTrigger.test.tsx
🧰 Additional context used
📓 Path-based instructions (2)
packages/sdk-react/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/sdk-react/.cursor/rules/api-calls.mdc)
packages/sdk-react/**/*.{ts,tsx}: Use proper TypeScript types for API data and leverage type inference from the OpenAPI schema
Use custom TypeScript types (e.g., Invoice, CreateInvoiceInput) imported from '@/types/api' for Monite API data structures
Files:
packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsxpackages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.tspackages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/VendorSection.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx
packages/sdk-react/**/*.tsx
📄 CodeRabbit inference engine (packages/sdk-react/.cursor/rules/ui-components.mdc)
packages/sdk-react/**/*.tsx: Always use shadcn/ui components when creating new UI elements
Import components from '@/ui/components/*' when using shadcn/ui components
All Tailwind classes should have the prefix 'mtw' (e.g., 'mtw:w-full')
Use Tailwind CSS utility classes for all styling
Avoid inline styles or styled-components
Follow Tailwind's mobile-first responsive design approach
Use Tailwind's color palette and spacing scale
Do NOT use Material UI for new components
When modifying files with Material UI: only update to shadcn/ui if the component needs significant changes; otherwise, maintain existing MUI implementation; add TODO comments when MUI components should be migrated
Always prefer shadcn/ui components such as Button, Input, Card, Dialog, etc., from '@/ui/components/[component]'
Use React Hook Form with shadcn/ui form components
Use 'lucide-react' for icons and import icons like 'import { Search, Menu, X } from "lucide-react"'
Always use named imports from the specific ui component file
Use the variant prop for different styles (e.g., variant="outline" for buttons)
Use the size prop when available (e.g., size="sm" for smaller buttons)
Extend components with className prop using Tailwind utilities
When updating Material UI components to shadcn/ui: map MUI component to shadcn/ui equivalent, convert sx props and makeStyles to Tailwind classes, update event handlers if API differs, test thoroughly, remove MUI imports only after confirming all usages are replaced
Use TypeScript for all new components
Leverage component prop types for better IntelliSense
Files:
packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsxpackages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/VendorSection.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx
🧠 Learnings (26)
📓 Common learnings
Learnt from: costa-monite
PR: team-monite/monite-sdk#785
File: packages/sdk-react/src/core/i18n/locales/en/messages.po:8470-8473
Timestamp: 2025-09-04T15:54:24.944Z
Learning: For PRs focused on the invoice preview template (feat/DEV-15554), costa-monite prefers to exclude out-of-scope tweaks like minor i18n spacing changes (e.g., "Subtotal{0}" → "Subtotal {0}") and handle them in a separate follow-up.
Learnt from: costa-monite
PR: team-monite/monite-sdk#828
File: packages/sdk-react/src/mocks/ledgerAccounts/ledgerAccountsHandlers.ts:24-27
Timestamp: 2025-09-17T07:36:58.933Z
Learning: costa-monite prefers mock API handlers to closely mirror real API behavior rather than adding extra validation that wouldn't exist in the actual API endpoints. This ensures mocks provide accurate testing scenarios.
📚 Learning: 2025-07-21T08:11:43.801Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:11:43.801Z
Learning: Applies to packages/sdk-react/**/*.tsx : Use 'lucide-react' for icons and import icons like 'import { Search, Menu, X } from "lucide-react"'
Applied to files:
packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:10:47.057Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: examples/with-nextjs-and-clerk-auth/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:10:47.057Z
Learning: Applies to examples/with-nextjs-and-clerk-auth/src/components/**/*.tsx : Use lucide-react for icons and import icons like: import { Search, Menu, X } from 'lucide-react'
Applied to files:
packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:11:13.444Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/api-calls.mdc:0-0
Timestamp: 2025-07-21T08:11:13.444Z
Learning: Applies to packages/sdk-react/**/*.{ts,tsx} : Use proper TypeScript types for API data and leverage type inference from the OpenAPI schema
Applied to files:
packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts
📚 Learning: 2025-07-21T08:11:13.444Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/api-calls.mdc:0-0
Timestamp: 2025-07-21T08:11:13.444Z
Learning: Applies to packages/sdk-react/**/*.{tsx} : Use `useMoniteContext` from '@/contexts/MoniteContext' to access the Monite API client and entityId in React components
Applied to files:
packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts
📚 Learning: 2025-09-05T09:38:28.034Z
Learnt from: costa-monite
PR: team-monite/monite-sdk#785
File: packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/InvoicePreview.tsx:21-22
Timestamp: 2025-09-05T09:38:28.034Z
Learning: In the InvoicePreview component at packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/InvoicePreview.tsx, the default parameter `templateName = 'default_monite'` is intentionally designed to use a hard-coded default rather than falling back to the API-configured `defaultInvoiceTemplate?.name`. This design choice means the 'default_monite' template takes precedence over any API-configured default template.
Applied to files:
packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.tspackages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx
📚 Learning: 2025-09-22T08:57:20.610Z
Learnt from: costa-monite
PR: team-monite/monite-sdk#829
File: packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx:4-6
Timestamp: 2025-09-22T08:57:20.610Z
Learning: The shared ItemsSection component correctly imports FormErrorDisplay and useFormErrors from the receivables path because it uses useLineItemManagement and expects receivables-style error shapes. The Purchase Orders has its own FormErrorDisplay/useFormErrors with different types that are not compatible with the shared component without broader changes.
Applied to files:
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx
📚 Learning: 2025-09-22T08:34:04.783Z
Learnt from: costa-monite
PR: team-monite/monite-sdk#829
File: packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx:576-587
Timestamp: 2025-09-22T08:34:04.783Z
Learning: In the ItemsSection component, VatRateField passes VAT rate values in basis points (e.g., 1900, 10000) but the form model stores VAT rates as percentages for vat_rate_value (e.g., 19, 100). The onLineItemValueChangeAdapter correctly converts from basis points to percentage using vatRateBasisPointsToPercentage before storing in the form.
Applied to files:
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx
📚 Learning: 2025-09-22T15:06:59.457Z
Learnt from: costa-monite
PR: team-monite/monite-sdk#829
File: packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx:56-64
Timestamp: 2025-09-22T15:06:59.457Z
Learning: In PurchaseOrderPreviewMonite component at packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx, the price conversion that wraps item.price in an object structure { currency: item.currency, value: item.price } is correct and intentional for purchase order preview calculations and display.
Applied to files:
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx
📚 Learning: 2025-09-08T09:09:14.289Z
Learnt from: costa-monite
PR: team-monite/monite-sdk#785
File: packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/InvoicePreviewMonite.tsx:320-336
Timestamp: 2025-09-08T09:09:14.289Z
Learning: In InvoicePreviewMonite component at packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/InvoicePreviewMonite.tsx, the price values from item.product.price.value are already in minor units (as defined in the PriceFloat API schema), so they can be passed directly to formatCurrencyToDisplay without conversion.
Applied to files:
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx
📚 Learning: 2025-09-04T15:54:24.944Z
Learnt from: costa-monite
PR: team-monite/monite-sdk#785
File: packages/sdk-react/src/core/i18n/locales/en/messages.po:8470-8473
Timestamp: 2025-09-04T15:54:24.944Z
Learning: For PRs focused on the invoice preview template (feat/DEV-15554), costa-monite prefers to exclude out-of-scope tweaks like minor i18n spacing changes (e.g., "Subtotal{0}" → "Subtotal {0}") and handle them in a separate follow-up.
Applied to files:
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx
📚 Learning: 2025-09-19T13:32:23.657Z
Learnt from: costa-monite
PR: team-monite/monite-sdk#828
File: packages/sdk-react/src/components/payables/PayableDetails/PayableDetailsForm/helpers.ts:199-217
Timestamp: 2025-09-19T13:32:23.657Z
Learning: In the prepareLineItemSubmit function at packages/sdk-react/src/components/payables/PayableDetails/PayableDetailsForm/helpers.ts, when allowLedgerUpdate is false, resolvedLedgerId equals ledger_account_id and should always be included in the payload. This is the intended behavior according to costa-monite.
Applied to files:
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsx
📚 Learning: 2025-09-05T11:02:00.731Z
Learnt from: costa-monite
PR: team-monite/monite-sdk#785
File: packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/InvoicePreview.tsx:34-39
Timestamp: 2025-09-05T11:02:00.731Z
Learning: In the InvoicePreview component at packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/InvoicePreview.tsx, the minScale: 1 setting in useAdaptiveScale is intentionally set to prevent downscaling below 100% to maintain invoice readability and quality, even if it requires scrolling on smaller viewports.
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx
📚 Learning: 2025-07-21T08:11:43.801Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:11:43.801Z
Learning: Applies to packages/sdk-react/**/*.tsx : When updating Material UI components to shadcn/ui: map MUI component to shadcn/ui equivalent, convert sx props and makeStyles to Tailwind classes, update event handlers if API differs, test thoroughly, remove MUI imports only after confirming all usages are replaced
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:10:47.057Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: examples/with-nextjs-and-clerk-auth/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:10:47.057Z
Learning: When updating Material UI components to shadcn/ui, map MUI component to shadcn/ui equivalent, convert sx props and makeStyles to Tailwind classes, update event handlers if API differs, test thoroughly, and remove MUI imports only after confirming all usages are replaced
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:11:43.801Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:11:43.801Z
Learning: Applies to packages/sdk-react/**/*.tsx : When modifying files with Material UI: only update to shadcn/ui if the component needs significant changes; otherwise, maintain existing MUI implementation; add TODO comments when MUI components should be migrated
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:11:43.801Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:11:43.801Z
Learning: Applies to packages/sdk-react/**/*.tsx : Always use shadcn/ui components when creating new UI elements
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:11:43.801Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:11:43.801Z
Learning: Applies to packages/sdk-react/**/*.tsx : Always prefer shadcn/ui components such as Button, Input, Card, Dialog, etc., from '@/ui/components/[component]'
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:10:47.057Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: examples/with-nextjs-and-clerk-auth/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:10:47.057Z
Learning: Applies to examples/with-nextjs-and-clerk-auth/src/components/**/*.tsx : Always prefer shadcn/ui components such as Button, Input, Card, Dialog, Select, Checkbox, RadioGroup, Switch, Textarea, Label, Alert, Badge, Toast, Tabs, Table, Form, DropdownMenu, Sheet, Skeleton, Separator
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsx
📚 Learning: 2025-07-21T08:10:47.057Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: examples/with-nextjs-and-clerk-auth/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:10:47.057Z
Learning: Applies to examples/with-nextjs-and-clerk-auth/src/components/**/*.tsx : Always use shadcn/ui components when creating new UI elements
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsxpackages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsx
📚 Learning: 2025-07-21T08:11:43.801Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:11:43.801Z
Learning: Applies to packages/sdk-react/**/*.tsx : All Tailwind classes should have the prefix 'mtw' (e.g., 'mtw:w-full')
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:11:43.801Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:11:43.801Z
Learning: Applies to packages/sdk-react/**/*.tsx : Use Tailwind CSS utility classes for all styling
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:11:43.801Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/ui-components.mdc:0-0
Timestamp: 2025-07-21T08:11:43.801Z
Learning: Applies to packages/sdk-react/**/*.tsx : Do NOT use Material UI for new components
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:11:13.444Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/api-calls.mdc:0-0
Timestamp: 2025-07-21T08:11:13.444Z
Learning: Applies to packages/sdk-react/**/*.{tsx} : Always check for `entityId` before making Monite API calls, as most endpoints require it
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:11:13.444Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: packages/sdk-react/.cursor/rules/api-calls.mdc:0-0
Timestamp: 2025-07-21T08:11:13.444Z
Learning: Applies to packages/sdk-react/**/*.{tsx} : Use the `enabled` option in React Query hooks to prevent queries from running when dependencies (such as entityId) are missing
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
📚 Learning: 2025-07-21T08:10:12.113Z
Learnt from: CR
PR: team-monite/monite-sdk#0
File: examples/with-nextjs-and-clerk-auth/.cursor/rules/api-calls.mdc:0-0
Timestamp: 2025-07-21T08:10:12.113Z
Learning: Applies to examples/with-nextjs-and-clerk-auth/**/*.tsx : Always destructure `api` and `entityId` from useMoniteContext.
Applied to files:
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx
🧬 Code graph analysis (8)
packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts (1)
packages/sdk-react/src/components/templateSettings/types.ts (1)
SelectableDocumentType(44-44)
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (6)
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/validation.ts (1)
CreateReceivablesFormBeforeValidationProps(234-240)packages/sdk-react/src/components/shared/ItemsSection/types.ts (1)
ItemsSectionConfig(9-33)packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/hooks/useLineItemManagement.ts (1)
useLineItemManagement(44-459)packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/ItemSelector.tsx (2)
ProductItem(72-84)CUSTOM_ID(54-54)packages/sdk-react/src/core/utils/vatUtils.ts (1)
vatRateBasisPointsToPercentage(71-73)packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/InvoiceTotals.tsx (1)
InvoiceTotals(38-87)
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx (5)
packages/sdk-react/src/components/payables/PurchaseOrders/validation.ts (1)
PurchaseOrderLineItem(226-228)packages/sdk-react/src/components/payables/PurchaseOrders/types.ts (1)
CurrencyEnum(76-76)packages/sdk-react/src/components/payables/PurchaseOrders/consts.ts (1)
PURCHASE_ORDER_CONSTANTS(15-27)packages/sdk-react/src/hooks/useAdaptiveScale.ts (1)
useAdaptiveScale(20-105)packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx (1)
PurchaseOrderPreviewMonite(37-353)
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx (4)
packages/sdk-react/src/core/utils/getAPIErrorMessage.ts (1)
getAPIErrorMessage(5-22)packages/sdk-react/src/__mocks__/lingui-macro.ts (1)
t(8-36)packages/sdk-react/src/ui/box/CenteredContentBox.tsx (1)
CenteredContentBox(3-18)packages/sdk-react/src/ui/FileViewer/FileViewer.tsx (1)
FileViewer(29-54)
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsx (7)
packages/sdk-react/src/core/hooks/useCurrencies.ts (1)
useCurrencies(17-159)packages/sdk-react/src/core/queries/useMe.ts (1)
useMyEntity(31-68)packages/sdk-react/src/core/queries/useCounterpart.ts (2)
useCounterpartById(570-583)useCounterpartAddresses(19-30)packages/sdk-react/src/core/utils/vatUtils.ts (1)
vatRateBasisPointsToPercentage(71-73)packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/OverviewTabPanel.tsx (1)
OverviewTabPanel(15-69)packages/sdk-react/src/ui/Card/Card.tsx (1)
MoniteCard(77-88)packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderStatusChip.tsx (1)
PurchaseOrderStatusChip(30-100)
examples/with-nextjs-and-clerk-auth/e2e/utils/purchase-order-helpers.ts (1)
examples/with-nextjs-and-clerk-auth/e2e/pages/PayablesPage.ts (1)
payablesPage(10-10)
packages/sdk-react/src/components/payables/PurchaseOrders/sections/VendorSection.tsx (4)
packages/sdk-react/src/components/counterparts/types.ts (1)
CustomerType(62-62)packages/sdk-react/src/components/payables/PurchaseOrders/validation.ts (1)
CreatePurchaseOrderFormProps(198-200)packages/sdk-react/src/core/queries/useCounterpart.ts (1)
useCounterpartAddresses(19-30)packages/sdk-react/src/components/payables/PurchaseOrders/components/VendorSelector.tsx (1)
VendorSelector(77-371)
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx (8)
packages/sdk-react/src/components/payables/PurchaseOrders/validation.ts (1)
PurchaseOrderLineItem(226-228)packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/types.ts (2)
CurrencyEnum(18-18)SanitizableLineItem(26-43)packages/sdk-react/src/core/hooks/useCurrencies.ts (1)
useCurrencies(17-159)packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/utils.ts (1)
sanitizeLineItems(282-366)packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/components/useCreateInvoiceProductsTable.ts (1)
useCreateInvoiceProductsTable(39-200)packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/InvoicePreview.utils.ts (2)
getCountryName(122-126)getCounterpartPhone(106-120)packages/sdk-react/src/components/counterparts/helpers.ts (2)
isOrganizationCounterpart(12-15)isIndividualCounterpart(7-10)packages/sdk-react/src/core/utils/vatUtils.ts (1)
vatRateBasisPointsToPercentage(71-73)
🔇 Additional comments (14)
packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsx (3)
22-30:MoreVerticalIconimport still breaks build
lucide-reactdoesn’t exportMoreVerticalIcon, so this import will fail compilation. The button is also icon-only and still lacks an accessible label. Please land the previously suggested fix.-import { - Copy, - Download, - Eye, - EyeOff, - FileCheck, - Mail, - MoreVerticalIcon, - SquarePen, -} from 'lucide-react'; +import { + Copy, + Download, + Eye, + EyeOff, + FileCheck, + Mail, + MoreVertical, + SquarePen, +} from 'lucide-react';- <Button + <Button type="button" variant="outline" size="sm" disabled={isUpdateAllowedLoading} + aria-label={t(i18n)`More actions`} > - <MoreVerticalIcon /> + <MoreVertical /> </Button>Based on learnings
Also applies to: 254-261
66-71: Duplicate action must check “create” permissionDuplicate still rides on the update guard, so users with create-only rights can’t access it. Also keep the menu item disabled while the create permission is loading. Please wire in the dedicated
useIsActionAllowed({ action: 'create' })check and gate the click/disabled state accordingly (see earlier guidance).Also applies to: 281-284
101-102: Nice SSR-safe PDF openerThe
typeof window !== 'undefined'guard prevents SSR crashes, and passing'noopener,noreferrer'tightens security on the new tab. Looks good.packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx (2)
22-30: Good: Query properly gated on entityId and purchaseOrderIdThe request won’t fire until both IDs are available. This prevents sending undefined headers.
8-8: Replace MUI icon with lucide-reactNew UI must use lucide-react icons, not MUI.
-import ErrorOutlineIcon from '@mui/icons-material/ErrorOutline'; +import { CircleAlert } from 'lucide-react'; @@ - <ErrorOutlineIcon color="error" /> + <CircleAlert className="mtw:text-destructive" aria-hidden="true" />As per coding guidelines
Also applies to: 48-48
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/Overview.tsx (2)
14-16: Good: shadcn/ui primitives + Tailwind are usedTabs and Skeleton come from our shadcn/ui layer and Tailwind classes use the mtw: prefix.
As per coding guidelines
39-51: Good: Issue/Expiry date semantics correctedIssue Date prefers issued_at with created_at fallback; Expiry derives from the same base date.
Based on learnings
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx (1)
55-68: Confirm price units (must be minor units)
sanitizeLineItemswill normalize flatprice+currencyintoproduct.price, which matches later usage. Please confirmitem.pricehere is already in minor units to keep display and totals correct. Based on learnings.packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx (1)
45-61: Fix date-only parsing to avoid off‑by‑one (time zone) bugs
new Date('YYYY-MM-DD')parses as UTC and may shift a day locally. Parse date-only strings as local dates.Apply:
-import { addDays } from 'date-fns'; +import { addDays, parse, parseISO } from 'date-fns';- const expiryDate = useMemo(() => { - if (purchaseOrderData.expiry_date) { - const d = new Date(purchaseOrderData.expiry_date); - - d.setHours(0, 0, 0, 0); - - return d; - } - const validForDays = - purchaseOrderData.valid_for_days || - PURCHASE_ORDER_CONSTANTS.DEFAULT_VALID_FOR_DAYS; - const d = addDays(new Date(), validForDays); - - d.setHours(0, 0, 0, 0); - - return d; - }, [purchaseOrderData.expiry_date, purchaseOrderData.valid_for_days]); + const expiryDate = useMemo(() => { + const raw = purchaseOrderData.expiry_date; + if (raw) { + let d: Date; + if (raw instanceof Date) { + d = new Date(raw); + } else { + const s = String(raw).trim(); + d = /^\d{4}-\d{2}-\d{2}$/.test(s) + ? parse(s, 'yyyy-MM-dd', new Date()) + : parseISO(s); + } + d.setHours(0, 0, 0, 0); + return d; + } + const validForDays = + purchaseOrderData.valid_for_days ?? + PURCHASE_ORDER_CONSTANTS.DEFAULT_VALID_FOR_DAYS; + const d = addDays(new Date(), validForDays); + d.setHours(0, 0, 0, 0); + return d; + }, [purchaseOrderData.expiry_date, purchaseOrderData.valid_for_days]);Also applies to: 8-8
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (2)
4-6: Import path is correct for shared error handling (keep as-is).This shared component intentionally uses the receivables FormErrorDisplay/useFormErrors due to the expected error shape.
Based on learnings
600-609: VAT bps→percent conversion fix looks correct.The >= 100 check handles the 1% (100 bps) edge case properly.
examples/with-nextjs-and-clerk-auth/e2e/utils/purchase-order-helpers.ts (3)
69-77: Good readiness gate for PO tabWaiting for either the Number column or the Create Purchase Order button reduces flakiness. LGTM.
395-482: Save flow looks solid; verify post‑save anchors exist in PO flowRace waits for “Compose email” or PO heading are good. Confirm these elements are present for POs in all environments; otherwise add an alternative (e.g., “Preview” button or “View purchase order”).
542-551: Utility looks goodfirstVisibleLocator is simple and effective. LGTM.
examples/with-nextjs-and-clerk-auth/e2e/utils/purchase-order-helpers.ts
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/sections/VendorSection.tsx
Outdated
Show resolved
Hide resolved
costa-monite
force-pushed
the
feat/DEV-16185-purchase-orders
branch
from
c473c5b to
71ae6c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts (1)
72-75: Type setDefaultTemplate params to support both template types.The parameters are typed specifically as
TemplateReceivableResponse['id']andTemplateReceivableResponse['name'], but this function should handle purchase order templates as well. Use theTemplateDocumentunion type once it's fixed.Apply this diff:
const setDefaultTemplate = async ( - id: components['schemas']['TemplateReceivableResponse']['id'], - name: components['schemas']['TemplateReceivableResponse']['name'] + id: TemplateDocument['id'], + name: TemplateDocument['name'] ) => {packages/sdk-react/src/mocks/handlers.ts (1)
51-66: Remove duplicate filesHandlers entry.The
filesHandlersappears twice in the handlers array (lines 51 and 66), which could cause duplicate route registration and unpredictable behavior in MSW.Apply this diff to remove the duplicate:
...currenciesHandlers, - ...filesHandlers, ...recurrencesHandlers, ...payableHandlers,packages/sdk-react/src/core/utils/vatUtils.ts (2)
9-11: Critical: Missing decimal precision parameter in rateMinorToMajor.The
fromMinorUnitsfunction requires a precision parameter (2 for basis points/percentage conversion). Without it, the conversion will use an incorrect default precision, breaking VAT rate calculations.Apply this diff:
export const rateMinorToMajor = (rateMinor: number): number => { - return fromMinorUnits(rateMinor); + return fromMinorUnits(rateMinor, 2); };Per the relevant snippet from currencies.ts, this function should call
fromMinorUnits(rateMinor, 2)to correctly convert basis points to percentages (e.g., 750 → 7.5).
19-21: Critical: Missing decimal precision parameter in rateMajorToMinor.The
toMinorUnitsfunction requires a precision parameter (2 for basis points/percentage conversion). Without it, the conversion will use an incorrect default precision.Apply this diff:
export const rateMajorToMinor = (rateMajor: number): number => { - return toMinorUnits(rateMajor); + return toMinorUnits(rateMajor, 2); };Per the relevant snippet from currencies.ts, this function should call
toMinorUnits(rateMajor, 2)to correctly convert percentages to basis points (e.g., 7.5 → 750).
♻️ Duplicate comments (32)
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/utils.ts (1)
321-321: Avoid empty string fallback for measure_unit_id.Defaulting to an empty string when
measure_unit_idis missing can cause schema validation failures and create noisy diffs. Useundefinedso the property is omitted when absent.Apply this diff:
- measure_unit_id: extItem.product?.measure_unit_id || flatUnit || '', + measure_unit_id: extItem.product?.measure_unit_id ?? flatUnit ?? undefined,Based on learnings.
packages/sdk-react/src/components/templateSettings/components/LayoutAndLogo.tsx (1)
92-92: Critical: DocumentTemplate type must support purchase_order templates.This type alias is defined as only
TemplateReceivableResponse, but whendocumentType='purchase_order', the hook returns purchase order templates. This creates the same type mismatch flagged in the previous review.Apply this diff:
-type DocumentTemplate = components['schemas']['TemplateReceivableResponse']; +type DocumentTemplate = + | components['schemas']['TemplateReceivableResponse'] + | components['schemas']['TemplatePurchaseOrderResponse'];packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/components/useCreateInvoiceProductsTable.ts (2)
49-63: Potential unit mismatch remains unresolved.This is the same issue flagged in the previous review: if
extField.priceis a number from UI input, it may be in major units while APIPriceFloat.valueexpects minor units. Mixing these without conversion will produce incorrect calculations.Based on learnings, API price values are in minor units. Verify whether numeric prices from UI are in major or minor units, and apply conversion if needed.
99-104: Ensure Price values are integers in minor units.The
Priceconstructor receives calculated values that may be floats. Per API schema conventions and learnings, price values should be integers in minor units.Apply rounding before construction:
const priceObj = new Price({ - value: price, + value: Math.round(price), currency: currency as CurrencyEnum, formatter: formatCurrencyToDisplay, });Also applies to: 163-167
packages/sdk-react/config/rollup.config.mjs (1)
27-27: Verify ifinlineDynamicImportsis still necessary.Based on the past review comment, you mentioned this change was added to resolve local bundling issues but might be outdated. The
inlineDynamicImports: trueoption bundles all dynamic imports into the main chunk, which can significantly increase bundle size and prevent code splitting optimizations.Please verify:
- Are the bundling issues still occurring without this option?
- What specific bundling errors were encountered?
- Can we scope this to only the affected build (CJS or ESM) rather than both?
#!/bin/bash # Check for dynamic import usage in the codebase to assess impact rg -nP --type=ts --type=tsx -C2 '\bimport\s*\(' | head -50Also applies to: 35-35
packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderForm.test.tsx (7)
36-36: Verify thatwaitUntilTableIsLoadedis appropriate for this component.Same concern as line 13:
waitUntilTableIsLoadedexpects a progressbar/table loading state that may not exist in the form rendering context.
54-54: Verify thatwaitUntilTableIsLoadedis appropriate for this component.Same concern as line 13:
waitUntilTableIsLoadedexpects a progressbar/table loading state that may not exist in the form rendering context.
94-94: Verify thatwaitUntilTableIsLoadedis appropriate for this component.Same concern as earlier:
waitUntilTableIsLoadedmay not be appropriate for form rendering.
113-113: Verify thatwaitUntilTableIsLoadedis appropriate for this component.Same concern as earlier:
waitUntilTableIsLoadedmay not be appropriate for form rendering.
127-127: Verify thatwaitUntilTableIsLoadedis appropriate for this component.Same concern as earlier:
waitUntilTableIsLoadedmay not be appropriate for form rendering.
145-145: Verify thatwaitUntilTableIsLoadedis appropriate for this component.Same concern as earlier:
waitUntilTableIsLoadedmay not be appropriate for form rendering.
156-156: Verify thatwaitUntilTableIsLoadedis appropriate for this component.Same concern as earlier:
waitUntilTableIsLoadedmay not be appropriate for form rendering.packages/sdk-react/src/components/payables/PurchaseOrders/components/VendorSelector.tsx (1)
11-25: Replace MUI imports with shadcn/ui + lucide-react per SDK rules.
New SDK React UI must be built with shadcn/ui components, Tailwindmtw-*classes, and lucide-react icons; pulling in MUI here breaks those rules and reintroduces a library we’re trying to phase out. Please migrate the Autocomplete, Button, TextField, IconButton, Divider, Typography, etc., to the shadcn/ui equivalents (Combobox/Popover, Button, Input, etc.), restyle with Tailwind utilities, and swap icons to lucide-react—or, if migration can’t happen immediately, add a TODO and block merging. As per coding guidelinespackages/sdk-react/src/components/payables/PurchaseOrders/hooks/useCreatePurchaseOrder.ts (1)
9-13: Missing entityId for API call.The mutation requires the
x-monite-entity-idheader. DestructureentityIdfromuseMoniteContextand pass it in the mutation options.Based on learnings
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/VatRateField.tsx (1)
93-107: Guard the reverse lookup against null/NaN VAT values
currentVatRateValuecan benull/undefined, and multiplying it coerces to0, so the effect auto-selects a 0 % VAT even when no VAT exists. Please ensure the reverse lookup only runs for real numbers and round before comparing to basis-point integers.Apply:
- if ( - !isNonVatSupported && - !value && - availableVatRates?.length && - hasInitialized && - currentVatRateValue !== undefined - ) { - const currentVatRateInBasisPoints = currentVatRateValue * 100; + if ( + !isNonVatSupported && + !value && + availableVatRates?.length && + hasInitialized && + typeof currentVatRateValue === 'number' && + !Number.isNaN(currentVatRateValue) + ) { + const currentVatRateInBasisPoints = Math.round(currentVatRateValue * 100);packages/sdk-react/src/components/payables/PurchaseOrders/sections/PurchaseOrderTermsSummary.tsx (1)
74-88: Sanitizevalid_for_daysbefore writing to the form
Number(e.target.value)can yield values outside[MIN, MAX](or evenNaN), socalculateExpiryDate(days)can produce an invalid date and the form will momentarily hold illegal durations. Clamp the value before callingfield.onChangeandhandleValidForDaysChange.Apply:
render={({ field }) => ( <input {...field} type="number" min={PURCHASE_ORDER_CONSTANTS.MIN_VALID_DAYS} max={PURCHASE_ORDER_CONSTANTS.MAX_VALID_DAYS} className="mtw:hidden" onChange={(e) => { - const days = Number(e.target.value); - field.onChange(days); - handleValidForDaysChange(days); + const raw = Number(e.target.value); + const clamped = Math.min( + PURCHASE_ORDER_CONSTANTS.MAX_VALID_DAYS, + Math.max( + PURCHASE_ORDER_CONSTANTS.MIN_VALID_DAYS, + Number.isFinite(raw) + ? raw + : PURCHASE_ORDER_CONSTANTS.MIN_VALID_DAYS + ) + ); + field.onChange(clamped); + handleValidForDaysChange(clamped); }} disabled={disabled} /> )} />packages/sdk-react/src/components/payables/PurchaseOrders/validation.ts (4)
53-56: Critical: Invalid Zod constructor optionmessagewill be ignored.The
z.number()constructor does not accept amessagekey. Per Zod documentation, useinvalid_type_errorfor type validation errors andrequired_errorfor missing required fields. The current code will silently ignore the custom error message.Apply this diff:
- z.number({ - message: t(i18n)`Price is a required field` - }).min(0, t(i18n)`Price must be 0 or greater`) + z.number({ + invalid_type_error: t(i18n)`Price is a required field`, + }).min(0, t(i18n)`Price must be 0 or greater`)Note: This is a reoccurrence of the previously flagged issue. Based on learnings, the developer believes this is correct, but Zod v3+ documentation confirms
messageis not a valid constructor option for primitive types.
71-77: Critical: Invalid Zod constructor optionmessagewill be ignored.Same issue as line 53-56. The
z.number()constructor requiresinvalid_type_error, notmessage.Apply this diff:
tax_rate_value: z .number({ - message: t(i18n)`Tax is a required field` + invalid_type_error: t(i18n)`Tax is a required field`, }) .min(0, t(i18n)`Tax rate must be 0 or greater`) .max(100, t(i18n)`Tax rate must be 100% or less`),
83-89: Critical: Invalid Zod constructor optionerrorwill be ignored.The
z.number()andz.string()constructors do not accept anerrorkey. Useinvalid_type_errorinstead.Apply this diff:
vat_rate_value: z - .number({ error: t(i18n)`VAT is a required field` }) + .number({ invalid_type_error: t(i18n)`VAT is a required field` }) .min(0, t(i18n)`VAT rate must be 0 or greater`) .max(100, t(i18n)`VAT rate must be 100% or less`), vat_rate_id: z - .string({ error: t(i18n)`VAT is a required field` }) + .string({ invalid_type_error: t(i18n)`VAT is a required field` }) .min(1, t(i18n)`VAT is a required field`),
127-129: Critical: Invalid Zod constructor optionmessagewill be ignored.The
z.date()constructor does not accept amessagekey. Useinvalid_type_errorinstead.Apply this diff:
z .date({ - message: t(i18n)`Expiry date is a required field`, + invalid_type_error: t(i18n)`Expiry date is a required field`, })packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx (1)
45-61: Potential date offset bug in expiry_date parsing.The
new Date(purchaseOrderData.expiry_date)constructor for ISO date-only strings (e.g., "2025-03-15") interprets them as UTC midnight, which can shift to the previous day in timezones west of UTC. This will cause the preview to display the wrong expiry date for users in those timezones.Apply this diff:
+import { parse, parseISO } from 'date-fns'; + const expiryDate = useMemo(() => { if (purchaseOrderData.expiry_date) { - const d = new Date(purchaseOrderData.expiry_date); + const raw = purchaseOrderData.expiry_date; + let d: Date; + if (raw instanceof Date) { + d = new Date(raw); + } else { + const s = String(raw).trim(); + // Parse date-only strings as local dates to avoid UTC offset + d = /^\d{4}-\d{2}-\d{2}$/.test(s) + ? parse(s, 'yyyy-MM-dd', new Date()) + : parseISO(s); + } d.setHours(0, 0, 0, 0); return d; } const validForDays = - purchaseOrderData.valid_for_days || + purchaseOrderData.valid_for_days ?? PURCHASE_ORDER_CONSTANTS.DEFAULT_VALID_FOR_DAYS; const d = calculateExpiryDate(validForDays); d.setHours(0, 0, 0, 0); return d; }, [purchaseOrderData.expiry_date, purchaseOrderData.valid_for_days]);Note: Also changed
||to??for valid_for_days to handle 0 correctly (though 0 would fail validation elsewhere).packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/useExistingPurchaseOrderDetails.tsx (1)
61-78: Allow Issue-only flow for email deliveryLine 62 still throws whenever the delivery method is Email, but issuing a PO is performed via email. That exception blocks the “Issue only” action in the normal case. Please remove the Download guard or invert it to require Email so the send mutation can run.
- const handleIssueOnly = useCallback(() => { - if (deliveryMethod !== DeliveryMethod.Download) { - throw new Error('Unsupported delivery method'); - } + const handleIssueOnly = useCallback(() => { + if (deliveryMethod !== DeliveryMethod.Email) { + throw new Error('Unsupported delivery method'); + }packages/sdk-react/src/components/payables/PurchaseOrders/Filters/Filters.tsx (1)
21-22: Fix invalid Theme import
'mui-styles'doesn’t exist, so TypeScript will fail to resolve this module. ImportThemefrom MUI’s styles package instead.-import type { SxProps } from '@mui/material'; -import type { Theme } from 'mui-styles'; +import type { SxProps } from '@mui/material'; +import type { Theme } from '@mui/material/styles';packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/InvoiceItemRow.tsx (1)
69-83: Error derivation still misses top-level fallbacks for manual items.The past review comment remains valid. The current implementation:
priceErrordoesn't checklineItemError?.price?.value(only checks nestedproduct.price.valueand top-levelprice)measureUnitFieldErrordoesn't fall back tolineItemError?.measure_unit_idfor manual itemsApply the previously suggested diff:
const priceError = Boolean( - lineItemError?.product?.price?.value || lineItemError?.price + lineItemError?.product?.price?.value || + lineItemError?.price?.value || + lineItemError?.price ); - const measureUnitFieldError = lineItemError?.product?.measure_unit_id; + const measureUnitFieldError = + lineItemError?.product?.measure_unit_id ?? + lineItemError?.measure_unit_id;packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderForm.tsx (1)
33-35: Avoid renderingundefined-FormclassesWhen no
classNameprop is provided,(className + '-Form')renders as"undefined-Form", which leaks an unintended class into the DOM. Use a fallback before appending the suffix.Apply:
- <div - className={(className + '-Form') + ' mtw:flex mtw:flex-1 mtw:overflow-auto mtw:px-3 mtw:py-0 mtw:flex-col mtw:gap-3'} - > + <div + className={`${className ? `${className}-Form` : 'Monite-CreatePurchaseOrderForm'} mtw:flex mtw:flex-1 mtw:overflow-auto mtw:px-3 mtw:py-0 mtw:flex-col mtw:gap-3`} + >packages/sdk-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.tsx (1)
161-170: Drop the extra success toast (hook already emits one).
useSendPurchaseOrderByIdcallstoast.successon its own success path, so this block fires a second toast for the same action. Remove the local toast to avoid double notifications.packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrdersTable.tsx (1)
16-17: Import casing already flagged.This import casing issue was already identified in a previous review comment.
packages/sdk-react/src/components/payables/PurchaseOrders/hooks/usePurchaseOrderDetails.tsx (1)
84-86: Message clearing issue persists.The previous review comment about allowing message clearing appears unaddressed. Using
if (message)prevents setting an empty string, blocking users from clearing the message field.packages/sdk-react/src/components/payables/PurchaseOrders/CreatePurchaseOrder.tsx (3)
147-154: Toast fires on every render - move to useEffect.This issue was previously flagged but remains unaddressed. The toast will fire repeatedly while
vatIdsErroris truthy, causing duplicate toast notifications.
308-310: Currency state initialization bug persists.The previously reported currency initialization issues remain:
tempCurrencystarts asundefined(lines 308-310)handleOpenCurrencyModaldoesn't synctempCurrencywithactualCurrency(lines 476-478)handleCurrencySubmitcan setactualCurrencytoundefinedif user clicks Save without selectingAlso applies to: 476-478, 489-507
52-62: Extensive MUI usage in new component conflicts with guidelines.While the Settings icon was updated to lucide-react, the component still uses extensive MUI primitives (Stack, Box, Modal, Grid, Typography, Alert, Switch, Checkbox, FormControlLabel) and sx styling throughout. Per coding guidelines, new components should use shadcn/ui and Tailwind (mtw:*) classes.
As per coding guidelines
Also applies to: 522-904
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (1)
19-34: MUI icon usage already flagged.The use of MUI's
AddIconinstead of lucide-react'sPlusicon was already identified in a previous review comment.Also applies to: 693-699
🧹 Nitpick comments (17)
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/utils.ts (1)
6-30: ExtendedLineItem duplicates fields already in SanitizableLineItem.The
ExtendedLineItemtype redefines fields (name,price,currency,unit,product_id,quantity,vat_rate_value,tax_rate_value,product) that are already present inSanitizableLineItemvia its definition. This redundancy creates confusion and makes the codebase harder to maintain.Consider one of these approaches:
Option 1 (preferred): Use
SanitizableLineItemdirectly if no additional fields are needed:-type ExtendedLineItem = SanitizableLineItem & { - name?: string; - price?: number; - currency?: CurrencyEnum; - unit?: string; - product_id?: string; - quantity?: number; - vat_rate_value?: number; - tax_rate_value?: number; - measure_unit?: { - name: string; - id: null; - }; - product?: { - name?: string; - description?: string; - price?: { - currency?: CurrencyEnum; - value?: number; - }; - measure_unit_id?: string; - measure_unit_name?: string; - type?: string; - }; -};Then update line 289:
- const extItem = item as ExtendedLineItem; + const extItem = item as SanitizableLineItem & { + product_id?: string; + quantity?: number; + vat_rate_value?: number; + tax_rate_value?: number; + };Option 2: Document why the redefinition is necessary if there are subtle type differences.
packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderModals.tsx (1)
12-23: Consider removing the unnecessary fragment wrapper.The fragment
<>...</>serves no purpose since only one element is conditionally rendered. Return the JSX directly or null.Apply this diff:
- return ( - <> - {isEditTemplateModalOpen && ( - <TemplateSettings - isDialog={true} - isOpen={isEditTemplateModalOpen} - handleCloseDialog={onTemplateModalClose} - documentType="purchase_order" - /> - )} - </> - ); + return isEditTemplateModalOpen ? ( + <TemplateSettings + isDialog={true} + isOpen={isEditTemplateModalOpen} + handleCloseDialog={onTemplateModalClose} + documentType="purchase_order" + /> + ) : null;packages/sdk-react/src/ui/DataGridEmptyState/DataGridEmptyState.tsx (1)
74-78: Combine duplicate switch cases (optional)The
'no-data=payables'and'no-data=purchase-orders'branches both render the exact same<ReceiptLong>icon with identical styling. You can reduce duplication by merging them:- case 'no-data=payables': - defaultIcon = ( - <ReceiptLong sx={{ fontSize: '4rem', color: 'primary.main' }} /> - ); - break; - case 'no-data=purchase-orders': + case 'no-data=payables': + case 'no-data=purchase-orders': defaultIcon = ( <ReceiptLong sx={{ fontSize: '4rem', color: 'primary.main' }} /> ); break;Leave them separate if you anticipate their icons or styling diverging in the future.
packages/sdk-react/jest.config.js (1)
52-52: Clarify CSS handling: transform vs. moduleNameMapper conflict.The new CSS transformer at lines 33/37 will intercept all
.cssfiles before thismoduleNameMapperentry can apply. This makes the\\.(css|...portion of the regex effectively unused for.cssfiles, though it still applies to.less,.scss, and.sass.Consider one of these options for clarity:
Option 1: Remove CSS from the pattern if the transformer handles all CSS
- '\\.(css|less|scss|sass)$': 'identity-obj-proxy', + '\\.(less|scss|sass)$': 'identity-obj-proxy',Option 2: Use identity-obj-proxy for CSS modules only, plain CSS for transformer
Update the transformer to exclude
.module.css(see comment in jest-css-transform.js), then update this pattern:- '\\.(css|less|scss|sass)$': 'identity-obj-proxy', + '\\.module\\.(css|less|scss|sass)$': 'identity-obj-proxy',examples/with-nextjs-and-clerk-auth/e2e/utils/vendor-helpers.ts (1)
12-13: Consider replacing fixed timeout with a state-based wait.The 500ms fixed delay after clicking the combobox works but is not ideal. Consider waiting for the listbox to appear instead.
Apply this diff:
await vendorCombo.click(); - await page.waitForTimeout(500); + await page.locator('[role="listbox"],[role="option"]').first().waitFor({ state: 'visible', timeout: 2000 }).catch(() => {});packages/sdk-react/src/components/payables/PurchaseOrders/components/PurchaseOrderVatModeMenu.tsx (1)
81-116: Add displayName to the memoized component.Consider adding a
displayNameto improve debugging and React DevTools experience:export const PurchaseOrderVatModeMenu = memo( ({ value, disabled = false, onChange }: PurchaseOrderVatModeMenuProps) => { // ... component implementation } ); + +PurchaseOrderVatModeMenu.displayName = 'PurchaseOrderVatModeMenu';packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/components/useCreateInvoiceProductsTable.ts (2)
85-93: Currency resolution logic is correct but complex.The currency fallback chain correctly reads
price?.currencyfrom the foundExtendedLineItem. This differs from the previous review comment which flagged readingproduct.price.currency- the code appears to have been corrected or the comment was inaccurate.However, the nested find/cast pattern is hard to verify. Consider extracting to a helper:
const findExtendedPriceCurrency = (items: typeof lineItems) => { const found = items.find((field) => { const ext = field as ExtendedLineItem; return typeof ext.price === 'object' && ext.price?.currency; }); return found ? (found as ExtendedLineItem).price?.currency : undefined; };
145-157: Duplicated currency resolution logic.This currency resolution is identical to the
subtotalPriceblock (lines 81-93). Extract to a shared helper to reduce duplication and improve maintainability.const resolveCurrency = () => { return ( actualCurrency ?? lineItems.find((field) => Boolean(field.product?.price?.currency)) ?.product?.price?.currency ?? (lineItems.find((field) => { const extField = field as ExtendedLineItem; return typeof extField.price === 'object' && extField.price?.currency; }) as ExtendedLineItem | undefined)?.price?.currency ?? ( lineItems.find((field) => Boolean((field as ExtendedLineItem).currency) ) as ExtendedLineItem )?.currency ); };Then use
resolveCurrency()in bothsubtotalPriceandtotalTaxes.packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderForm.test.tsx (4)
56-58: Use a more specific accessible name for the spinbutton query.Using
getByRole('spinbutton', { name: '' })with an empty name is fragile and may break if the accessibility label changes. Consider using a more specific accessible name (e.g.,/valid.*for.*days/i) or usegetByLabelTextif a label is associated with the input.Apply this diff to use a more specific query:
- const validForDaysInput = screen.getByRole('spinbutton', { name: '' }); + const validForDaysInput = screen.getByRole('spinbutton', { name: /valid.*for.*days/i });Or, if the input has an associated label:
- const validForDaysInput = screen.getByRole('spinbutton', { name: '' }); + const validForDaysInput = screen.getByLabelText(/valid.*for.*days/i);
101-105: Use exact value assertion and avoid unnecessary type cast.The test should verify the exact
valid_for_daysvalue (30) from the mock rather than just checking it's greater than 0. Also, theas HTMLInputElementcast is unnecessary when using thetoHaveValuematcher.Apply this diff:
const validForDaysInput = screen.getByRole('spinbutton', { name: '', - }) as HTMLInputElement; + }); expect(validForDaysInput).toBeInTheDocument(); - expect(Number(validForDaysInput.value)).toBeGreaterThan(0); + expect(validForDaysInput).toHaveValue(30);
119-120: Use exact count assertion.The test should verify the exact number of line items added (1) rather than just checking it's greater than 0, to ensure the component behaves as expected.
Apply this diff:
await waitFor(() => { const nameInputs = screen.getAllByPlaceholderText(/line item/i); - expect(nameInputs.length).toBeGreaterThan(0); + expect(nameInputs).toHaveLength(1); });
13-13: Remove unnecessarywaitUntilTableIsLoadedcalls in PurchaseOrderForm tests
waitUntilTableIsLoadedwaits for a table’s loading indicator (progressbar) to disappear, but PurchaseOrderForm doesn’t render a table or progressbar—these calls are no-ops and add noise. Remove allawait waitUntilTableIsLoaded();instances inPurchaseOrderForm.test.tsx, or replace with targeted awaits for specific form elements if needed.packages/sdk-react/src/ui/FormErrorDisplay/FormErrorDisplay.tsx (1)
14-24: Drop the extrauseMemos for clarityThese booleans are cheap to recompute—ditching the
useMemowrappers will simplify the component with zero measurable benefit.- const hasFieldErrors = useMemo(() => { - for (const error of Object.values(fieldErrors)) { - if (error) return true; - } - return false; - }, [fieldErrors]); - - const hasErrors = useMemo( - () => Boolean(generalError) || hasFieldErrors, - [generalError, hasFieldErrors] - ); + const hasFieldErrors = Object.values(fieldErrors).some(Boolean); + const hasErrors = Boolean(generalError) || hasFieldErrors;packages/sdk-react/src/components/payables/PurchaseOrders/PreviewPurchaseOrderEmail.tsx (1)
27-36: Remove per-call header and add enabled guard.The mutation should rely on global
x-monite-entity-idinjection rather than passing it per-call. Additionally, add anenabledguard to prevent the mutation setup whenpurchaseOrderIdis missing.Apply this diff:
- const { api, entityId } = useMoniteContext(); + const { api } = useMoniteContext(); const { data: preview, mutateAsync: createPreview, isPending: isCreatingPreview, error, - } = api.payablePurchaseOrders.postPayablePurchaseOrdersIdPreview.useMutation({ - path: { purchase_order_id: purchaseOrderId }, - header: { 'x-monite-entity-id': entityId }, - }); + } = api.payablePurchaseOrders.postPayablePurchaseOrdersIdPreview.useMutation( + { + path: { purchase_order_id: purchaseOrderId }, + }, + { + enabled: !!purchaseOrderId, + } + );Based on learnings
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/EditPurchaseOrderDetails.tsx (1)
25-30: Optional: Simplify by passing onUpdated directly.The
handleSavecallback is a simple passthrough. You could passonUpdateddirectly toCreatePurchaseOrderasonUpdate={onUpdated}unless you anticipate adding logic here later.packages/sdk-react/src/components/payables/PurchaseOrders/utils/calculations.ts (2)
15-42: LGTM with minor observation: Consider extracting repeated itemTotal calculation.The function correctly calculates subtotal, tax, and total. The
?? 0fallbacks handle missing values appropriately. However,itemTotalis calculated separately in both thesubtotalandtotalTaxreductions.For a cleaner implementation, consider a single pass:
export function calculatePurchaseOrderTotals( purchaseOrder: components['schemas']['PurchaseOrderResponseSchema'] ): PurchaseOrderCalculations { const { subtotal, totalTax } = purchaseOrder.items?.reduce( (acc, item) => { const itemTotal = (item.quantity ?? 0) * (item.price ?? 0); const taxRatePercentage = vatRateBasisPointsToPercentage(item.vat_rate ?? 0); return { subtotal: acc.subtotal + itemTotal, totalTax: acc.totalTax + (itemTotal * taxRatePercentage) / 100, }; }, { subtotal: 0, totalTax: 0 } ) || { subtotal: 0, totalTax: 0 }; return { subtotal, totalTax, totalAmount: subtotal + totalTax, }; }
54-67: LGTM! Date calculations are correct.Both
calculateExpiryDateandcalculateValidForDayscorrectly usedate-fnsutilities. The functions appropriately calculate dates relative to "now".Optional: For unit testing, consider accepting an optional
baseDateparameter to make these functions deterministic in tests.
...ges/sdk-react/src/components/payables/PurchaseOrders/components/PurchaseOrderVatModeMenu.tsx
Show resolved
Hide resolved
...mponents/receivables/InvoiceDetails/CreateReceivable/sections/hooks/useLineItemManagement.ts
Show resolved
Hide resolved
packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/sdk-react/src/core/hooks/useLocalStorageFields.ts (1)
37-50: Race condition with functional updates – use functional form ofsetStoredValue.When
valueis a function, line 40 applies it tostoredValuecaptured from the render-time closure. Multiple synchronous calls tosetValuewith functional updates will see stalestoredValue, causing lost updates.Apply this diff to delegate to React's state updater:
- const setValue = (value: T | ((prev: T) => T)) => { - try { - const valueToStore = - value instanceof Function ? value(storedValue) : value; - - setStoredValue(valueToStore); - - if (typeof window !== 'undefined' && isRemembered) { - window.localStorage.setItem(prefixedKey, JSON.stringify(valueToStore)); - } - } catch (error) { - console.error('Error saving to localStorage:', error); - } - }; + const setValue = (value: T | ((prev: T) => T)) => { + try { + setStoredValue((prev) => { + const valueToStore = + value instanceof Function ? value(prev) : value; + + if (typeof window !== 'undefined' && isRemembered) { + window.localStorage.setItem(prefixedKey, JSON.stringify(valueToStore)); + } + + return valueToStore; + }); + } catch (error) { + console.error('Error saving to localStorage:', error); + } + };This ensures functional updates always receive the latest state value from React.
packages/sdk-react/src/components/payables/CreatePayableMenu/CreatePayableMenu.tsx (1)
22-27: Fix lucide icon imports/usages (current names do not exist)Use ChevronDown/ChevronUp components; current ChevronDownIcon/ChevronUpIcon will fail.
-import { - ChevronDownIcon, - ChevronUpIcon, - CloudUpload, - Plus, -} from 'lucide-react'; +import { ChevronDown, ChevronUp, CloudUpload, Plus } from 'lucide-react'; @@ - {open ? <ChevronUpIcon /> : <ChevronDownIcon />} + {open ? <ChevronUp /> : <ChevronDown />}Also applies to: 98-101
♻️ Duplicate comments (40)
packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrders.test.tsx (1)
107-116: Fix status chip label assertions (duplicate concern).The test expects lowercase 'draft' and 'issued', but
PurchaseOrderStatusChiprenders localized title-case labels ("Draft", "Issued") as shown in the component implementation. Use case-insensitive matchers or match the exact rendered text.Apply this diff:
- expect(screen.getByText('draft')).toBeInTheDocument(); + expect(screen.getByText(/draft/i)).toBeInTheDocument(); - expect(screen.getByText('issued')).toBeInTheDocument(); + expect(screen.getByText(/issued/i)).toBeInTheDocument();packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/ItemSelector.tsx (1)
367-391: Guard interactive handlers when disabled in manual path.The past review comment on this section is still applicable. While
disabled={disabled}is correctly passed to the TextField, the event handlers (onFocus,onBlur,onChange) execute unconditionally and can mutate state (setCustomName,setIsTyping) and trigger callbacks (onChange) even when the field is disabled.Apply this diff to guard the handlers:
if (!catalogEnabled) { return ( <TextField label={``} placeholder={t(i18n)`Line item`} required error={error} className="Item-Selector" sx={{ width: '100%', marginLeft }} disabled={disabled} value={customName} - onFocus={handleFocus} - onBlur={handleBlur} + onFocus={disabled ? undefined : handleFocus} + onBlur={disabled ? undefined : handleBlur} onChange={(e) => { + if (disabled) return; setCustomName(e.target.value); const val = e.target.value; if (val.trim()) { onChange({ id: CUSTOM_ID, label: val }, false); } else { onChange({ id: '', label: '' }, false); } }} /> ); }packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts (2)
72-75: GeneralizesetDefaultTemplateparameters to cover purchase orders
setDefaultTemplatestill acceptsTemplateReceivableResponsefields only. Once purchase orders flow through (documentType="purchase_order"), this signature becomes incompatible with the actual template object and will break as soon as the caller passes a purchase order template. Update the params to use the unified template type (e.g.,TemplateDocument['id']/TemplateDocument['name']) so both receivable and purchase order templates are supported.- const setDefaultTemplate = async ( - id: components['schemas']['TemplateReceivableResponse']['id'], - name: components['schemas']['TemplateReceivableResponse']['name'] - ) => { + const setDefaultTemplate = async ( + id: TemplateDocument['id'], + name: TemplateDocument['name'] + ) => {
101-104: Critical: widenTemplateDocumentunion to include purchase orders
TemplateDocumentis still hard-wired toTemplateReceivableResponse, so any purchase order template returned from the API is being forced into the wrong shape. This was already flagged earlier and remains unresolved. Please extend the alias to a union that includesTemplatePurchaseOrderResponse(or a shared base type, if available) so the hook’s return type actually matches the filtered data.-type TemplateDocument = components['schemas']['TemplateReceivableResponse']; +type TemplateDocument = + | components['schemas']['TemplateReceivableResponse'] + | components['schemas']['TemplatePurchaseOrderResponse'];packages/sdk-react/src/components/templateSettings/components/LayoutAndLogo.tsx (1)
92-92: Unblock purchase order rendering by wideningDocumentTemplate
DocumentTemplateremains tied toTemplateReceivableResponse, so the component cannot be correctly typed (or safely rendered) whendocumentType="purchase_order". Please update the alias to the same union the hook uses (receivable ∪ purchase order) so state, selection, and preview handling work for both document families.-type DocumentTemplate = components['schemas']['TemplateReceivableResponse']; +type DocumentTemplate = + | components['schemas']['TemplateReceivableResponse'] + | components['schemas']['TemplatePurchaseOrderResponse'];packages/sdk-react/jest-css-transform.js (2)
3-8: The comment is incorrect about identity-obj-proxy handling CSS modules.Jest's
transformruns beforemoduleNameMapper, so this transformer intercepts all.cssfiles—including.module.css—before identity-obj-proxy can process them. CSS modules will return{}instead of proxied class names, breaking tests that assert on className values.See the fix below in lines 10-15.
10-15: Exclude CSS modules from this transformer to preserve identity-obj-proxy mocking.The current regex matches all
.cssfiles, including.module.css. This preventsidentity-obj-proxy(configured in jest.config.js line 52) from providing mock class names for CSS modules.Apply this diff to exclude CSS modules:
process(_src, filename) { - if (filename.match(/\.css$/)) { + if (filename.match(/\.css$/) && !filename.match(/\.module\.(css|scss|sass)$/)) { return { code: 'module.exports = {};' }; } return { code: _src }; },Then update the comment:
/** * Custom Jest transform to handle CSS files with @import statements * * Strips @import statements that Jest can't parse (like font imports) - * while still allowing identity-obj-proxy to handle CSS modules. + * Excludes CSS modules to allow identity-obj-proxy to provide mock class names. */packages/sdk-react/src/mocks/purchaseOrders/purchaseOrdersHandlers.ts (1)
1-7: Use stable unique IDs for POST responses.
Line 39 still usesMath.random().toString(36).substring(7), which can evaluate to an empty string when the base‑36 result is shorter than seven characters. That leaves the POST response without a usableid, so any follow-up GET/PATCH/DELETE to/payable_purchase_orders/:purchase_order_idwill 404. Switch to the same UUID generator we already rely on in the fixtures to guarantee a valid identifier.-import { components } from '@/api'; -import { http, HttpResponse, delay } from 'msw'; +import { components } from '@/api'; +import { faker } from '@faker-js/faker'; +import { http, HttpResponse, delay } from 'msw'; @@ - id: Math.random().toString(36).substring(7), + id: faker.string.uuid(),Also applies to: 37-46
examples/with-nextjs-and-clerk-auth/e2e/utils/vendor-helpers.ts (1)
15-34: Scope option selection to the vendor listbox.Lines 15-17 use
page.locator('[role="option"]')which searches the entire page and can match options from other dropdowns. This is the issue flagged in the past review comment.Apply this diff to scope the search to the vendor combobox's listbox:
+ const listboxId = await vendorCombo.getAttribute('aria-controls'); + const listbox = listboxId + ? page.locator(`#${listboxId}`) + : page.locator('[role="listbox"]').first(); + - const existingOptions = page - .locator('[role="option"]') + const existingOptions = listbox + .getByRole('option') .filter({ hasNotText: /Create new/i });Based on past review comments.
examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.spec.ts (1)
13-22: Skip test on auth failure instead of just annotating.Lines 15-20 only add an annotation when auth fails but allow the test to continue to line 21 (
payablesPage.open()). This makes tests flaky when credentials are missing. The second describe block (lines 147-151, 172-176, 227-231) correctly usestest.skip()with early return.Apply this diff to match the pattern used elsewhere:
const auth = await signInUser(page); if (!auth.success) { - test.info().annotations.push({ - type: 'auth', - description: auth.error || 'missing creds', - }); + test.skip(); + return; } await payablesPage.open();Based on past review comments.
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (2)
19-19: Swap Add icon to lucide-react now (minimal step)Replace
@mui/icons-material/Addwithlucide-react’sPlusto align with icon guidance.-import AddIcon from '@mui/icons-material/Add'; +import { Plus } from 'lucide-react'; @@ - <Button - startIcon={<AddIcon />} + <Button + startIcon={<Plus size={16} />} variant="outlined" onClick={handleAddRow} disabled={config.features.autoAddRows && tooManyEmptyRows} >As per coding guidelines
Also applies to: 691-699
19-34: New UI must use shadcn/ui + Tailwind; avoid MUI in new componentsThis is a new component under packages/sdk-react/**/*.tsx. Per guidelines, prefer shadcn/ui components, Tailwind (mtw- prefix), and lucide-react icons; do NOT introduce MUI.
- Replace MUI primitives (Button, Typography, Table, TextField, Stack, Box, Collapse, CircularProgress) with shadcn/ui equivalents and Tailwind.
- Remove
sxin favor of Tailwind classes (mtw:*).- Replace MUI icon imports with lucide-react.
Add a TODO if migration cannot be completed in this PR.
As per coding guidelines
packages/sdk-react/src/components/shared/ItemsSection/useFormErrors.ts (1)
140-141: Lingui cannot extract interpolated message — inline the literalUsing
t(i18n)${GENERAL_ERROR_MESSAGE}prevents extraction. Inline the string.- return hasActualErrors ? t(i18n)`${GENERAL_ERROR_MESSAGE}` : undefined; + return hasActualErrors ? t(i18n)`Please check the form for errors` : undefined;Based on learnings
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/InvoiceItemRow.tsx (1)
69-83: Price and measure unit errors miss top-level fields (manual items).Restore fallbacks for manual entries: include price?.value and measure_unit_id at the top level.
Apply:
- const priceError = Boolean( - lineItemError?.product?.price?.value || lineItemError?.price - ); + const priceError = Boolean( + lineItemError?.product?.price?.value || + lineItemError?.price?.value || + lineItemError?.price + ); - const measureUnitFieldError = lineItemError?.product?.measure_unit_id; + const measureUnitFieldError = + lineItemError?.product?.measure_unit_id ?? + lineItemError?.measure_unit_id;Based on learnings.
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/VatRateField.tsx (1)
92-116: Critical: Guard reverse-lookup against null/NaN and round basis points.Null currently passes the condition and coerces to 0, auto-selecting 0% VAT. Also, floats may mismatch integer basis points.
Apply:
-useEffect(() => { - if ( - !isNonVatSupported && - !value && - availableVatRates?.length && - hasInitialized && - currentVatRateValue !== undefined - ) { - const currentVatRateInBasisPoints = currentVatRateValue * 100; - const matchingVatRate = availableVatRates.find(rate => - rate.value === currentVatRateInBasisPoints - ); - if (matchingVatRate) { - onChange(matchingVatRate.id, matchingVatRate.value); - } - } -}, [ +useEffect(() => { + if ( + !isNonVatSupported && + !value && + availableVatRates?.length && + hasInitialized && + typeof currentVatRateValue === 'number' && + Number.isFinite(currentVatRateValue) && + !Number.isNaN(currentVatRateValue) + ) { + const currentVatRateInBasisPoints = Math.round(currentVatRateValue * 100); + const matchingVatRate = availableVatRates.find( + (rate) => rate.value === currentVatRateInBasisPoints + ); + if (matchingVatRate) { + onChange(matchingVatRate.id, matchingVatRate.value); + } + } +}, [ isNonVatSupported, value, availableVatRates, hasInitialized, currentVatRateValue, onChange, ]);packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/hooks/useLineItemManagement.ts (1)
41-176: Allow flat rows without a name to stay “flat”.The type guard still insists on a truthy
name, so a flat row where the user typed price/VAT but not the name yet falls out of the flat path.cleanUpLineItemsForSubmissionthen rebuilds it withflatTemplateundefined and zeroes the price, reproducing the data loss I flagged earlier. Please drop the truthy-name requirement so blank-name rows remain flat.Apply this diff:
const isFlatLineItem = ( item: CreateReceivablesFormBeforeValidationLineItemProps | undefined -): item is FlatLineItem => Boolean(item && 'name' in item && item.name && !item.product); +): item is FlatLineItem => + Boolean(item && !item.product && 'name' in item);packages/sdk-react/src/components/payables/PurchaseOrders/sections/PurchaseOrderEntitySection.tsx (1)
1-106: Migrate new component to shadcn/ui and Tailwind.This is a new component and should follow the coding guidelines requiring shadcn/ui components for all new UI elements. The existing review comment provides a detailed migration path using
FormField,FormItem,FormControl,FormMessage, andTextareawith Tailwind classes.Based on coding guidelines.
packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/useExistingPurchaseOrderDetails.tsx (1)
61-78: Fix delivery method gating for Issue-only flow.The guard at lines 62-64 throws an error if
deliveryMethod !== DeliveryMethod.Download, but "Issue only" mode sends an email (as indicated by thesendMutationcall). This incorrectly blocks the email delivery path.Per the existing review comment: remove this guard or change it to require
DeliveryMethod.Emailinstead.packages/sdk-react/src/components/payables/PurchaseOrders/components/VendorSelector.tsx (2)
11-25: New component uses MUI; migrate to shadcn/ui + Tailwind per repo rulesReplace MUI components/icons/styles with shadcn/ui and lucide-react; add TODO if deferred.
As per coding guidelines
Also applies to: 215-318, 336-372
183-219: Propagate disabled to Autocomplete and TextFieldWithout it, callers can’t lock the control.
-<Autocomplete +<Autocomplete + disabled={disabled} {...field} @@ - <TextField + <TextField {...params} + disabled={disabled}packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx (1)
55-68: Mismatch: converted items don’t provide product.price/value or product.nameRows read item.product.name and item.product.price.value, but mapping sets top-level name/price. Results in blank names and 0 prices.
Apply this mapping consistent with the sanitizer/table expectations. Based on learnings.
- const convertedLineItems = - line_items?.map( - (item): SanitizableLineItem => ({ - id: item.id, - name: item.name, - quantity: item.quantity, - unit: item.unit, - price: item.price, - currency: item.currency as CurrencyEnum, - vat_rate_id: item.vat_rate_id, - vat_rate_value: item.vat_rate_value, - tax_rate_value: item.tax_rate_value, - }) - ) || []; + const convertedLineItems = + line_items?.map( + (item): SanitizableLineItem => ({ + id: item.id, + quantity: item.quantity, + product: { + name: item.name, + measure_unit_id: item.unit ?? 'unit', + price: { + currency: item.currency as CurrencyEnum, + value: item.price, + }, + }, + vat_rate_id: item.vat_rate_id, + vat_rate_value: item.vat_rate_value, + tax_rate_value: item.tax_rate_value, + }) + ) || [];packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx (1)
45-61: Date parsing still vulnerable to timezone shifts.The past review correctly identified that
new Date('YYYY-MM-DD')treats ISO date-only strings as UTC midnight, which will shift by one day in negative UTC offset timezones (e.g., Americas). The current code at line 47 still usesnew Date(purchaseOrderData.expiry_date)without detecting the format.Apply the previously suggested fix using
date-fnshelpers:+import { addDays, parse, parseISO } from 'date-fns'; -import { addDays } from 'date-fns';const expiryDate = useMemo(() => { - if (purchaseOrderData.expiry_date) { - const d = new Date(purchaseOrderData.expiry_date); - - d.setHours(0, 0, 0, 0); - - return d; - } + const raw = purchaseOrderData.expiry_date; + if (raw) { + let d: Date; + if (raw instanceof Date) { + d = new Date(raw); + } else { + const s = String(raw).trim(); + // Parse date-only as local date; otherwise parse ISO normally + d = /^\d{4}-\d{2}-\d{2}$/.test(s) + ? parse(s, 'yyyy-MM-dd', new Date()) + : parseISO(s); + } + d.setHours(0, 0, 0, 0); + return d; + } const validForDays = - purchaseOrderData.valid_for_days || + purchaseOrderData.valid_for_days ?? PURCHASE_ORDER_CONSTANTS.DEFAULT_VALID_FOR_DAYS; const d = calculateExpiryDate(validForDays); d.setHours(0, 0, 0, 0); return d; }, [purchaseOrderData.expiry_date, purchaseOrderData.valid_for_days]);packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderForm.test.tsx (3)
86-92: Type assertion bypasses safety checks.Using
as anyat line 90 disables TypeScript validation and can hide type mismatches between the mock and actual component expectations.Import the proper type and ensure the mock conforms:
+import type { components } from '@/api'; + +type PurchaseOrderResponse = components['schemas']['PurchaseOrderResponse']; + const mockPurchaseOrder = { id: 'po-123', document_id: 'PO-00001', status: 'draft', counterpart_id: 'vendor-123', - items: [ + line_items: [ { + id: 'item-1', name: 'Test Item', quantity: 2, unit: 'unit', price: 1000, currency: 'USD', - vat_rate: 1000, + vat_rate_value: 1000, }, ], message: 'Test message', valid_for_days: 30, currency: 'USD', -}; +} as PurchaseOrderResponse; renderWithClient( <PurchaseOrderForm isCreate={false} - purchaseOrder={mockPurchaseOrder as any} + purchaseOrder={mockPurchaseOrder} /> );
142-151: Test name doesn't match implementation.The test claims to verify "VAT mode switching" but only checks that the text "vat" appears after adding a line item. It doesn't test toggling VAT modes or verifying calculation changes.
Either rename to match actual behavior:
- it('should handle VAT mode switching', async () => { + it('should display VAT field when line item is added', async () => {Or implement proper VAT mode switching test:
it('should handle VAT mode switching', async () => { renderWithClient(<PurchaseOrderForm isCreate />); await waitUntilTableIsLoaded(); const addButton = screen.getByRole('button', { name: /row/i }); await userEvent.click(addButton); - expect(screen.getByText(/vat/i)).toBeInTheDocument(); + // Locate VAT mode toggle + const vatModeToggle = screen.getByRole('button', { name: /vat.*mode/i }); + + // Verify initial mode + expect(screen.getByText(/inclusive/i)).toBeInTheDocument(); + + // Switch mode + await userEvent.click(vatModeToggle); + await userEvent.click(screen.getByRole('menuitem', { name: /exclusive/i })); + + // Verify mode changed + await waitFor(() => { + expect(screen.getByText(/exclusive/i)).toBeInTheDocument(); + }); });
153-166: Test name doesn't match implementation.The test claims to verify "currency consistency across line items" but only checks that 2+ line items render. It doesn't verify that all items share the same currency or that changing one affects others.
Either rename to match actual behavior:
- it('should maintain currency consistency across line items', async () => { + it('should allow adding multiple line items', async () => {Or implement proper currency consistency test:
it('should maintain currency consistency across line items', async () => { renderWithClient(<PurchaseOrderForm isCreate />); await waitUntilTableIsLoaded(); const addButton = screen.getByRole('button', { name: /row/i }); await userEvent.click(addButton); await userEvent.click(addButton); await waitFor(() => { - const lineItems = screen.getAllByPlaceholderText(/line item/i); - expect(lineItems.length).toBeGreaterThanOrEqual(2); + // Get all currency selectors + const currencySelects = screen.getAllByRole('combobox', { name: /currency/i }); + expect(currencySelects.length).toBeGreaterThanOrEqual(2); + + // Verify all have same initial currency + const firstCurrency = currencySelects[0].textContent; + currencySelects.forEach(select => { + expect(select.textContent).toBe(firstCurrency); + }); }); });packages/sdk-react/src/components/payables/PurchaseOrders/hooks/useCreatePurchaseOrder.ts (1)
7-13: Missing required entity header will cause API failures.The mutation doesn't include the
x-monite-entity-idheader, which is required by the Monite API for create operations. This will cause all create attempts to fail.Based on learnings
Add the entityId to the mutation:
export const useCreatePurchaseOrder = () => { const { i18n } = useLingui(); - const { api, queryClient } = useMoniteContext(); + const { api, queryClient, entityId } = useMoniteContext(); return api.payablePurchaseOrders.postPayablePurchaseOrders.useMutation( - undefined, + { header: { 'x-monite-entity-id': entityId } }, { onSuccess: async (purchaseOrder) => {packages/sdk-react/src/components/payables/PurchaseOrders/Filters/Filters.tsx (1)
22-22: Fix: incorrect Theme import will cause compilation error.The import uses
'mui-styles'which doesn't exist. Use the correct MUI import.Apply this diff:
-import type { Theme } from 'mui-styles'; +import type { Theme } from '@mui/material/styles';packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/EditPurchaseOrderDetails.tsx (1)
12-18: Remove redundant MoniteScopedProviders wrapper.This wraps the content in
MoniteScopedProviders, butCreatePurchaseOrder(line 33) already providesMoniteScopedProvidersinternally (see CreatePurchaseOrder.tsx line 113). This creates unnecessary nesting.-export const EditPurchaseOrderDetails = ( - props: EditPurchaseOrderDetailsProps -) => ( - <MoniteScopedProviders> - <EditPurchaseOrderDetailsContent {...props} /> - </MoniteScopedProviders> -); +export const EditPurchaseOrderDetails = ( + props: EditPurchaseOrderDetailsProps +) => <EditPurchaseOrderDetailsContent {...props} />;packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderForm.tsx (1)
33-35: Avoid "undefined-Form" class when className is not provided.When
classNameisundefined, the concatenation produces"undefined-Form". Provide a safe default.- <div - className={(className + '-Form') + ' mtw:flex mtw:flex-1 mtw:overflow-auto mtw:px-3 mtw:py-0 mtw:flex-col mtw:gap-3'} - > + <div + className={`${className || 'Monite-CreatePurchaseOrderForm'}-Form mtw:flex mtw:flex-1 mtw:overflow-auto mtw:px-3 mtw:py-0 mtw:flex-col mtw:gap-3`} + >packages/sdk-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.tsx (2)
163-167: Remove duplicate success toast.
useSendPurchaseOrderByIdalready shows a success toast (see hook line 23:toast.success(t(i18n)\Purchase order has been sent`)`). Remove the local toast to avoid double notifications.- toast.success( - mode === 'issue_and_send' - ? t(i18n)`Purchase order issued and sent successfully` - : t(i18n)`Purchase order sent successfully` - ); onSendEmail?.(purchaseOrderId); onClose();
155-189: Wire the recipient (to) field into the send payload.The form's
values.tois validated but never sent to the backend. TheemailParamsobject (lines 156-159) only includesbody_textandsubject_text, so the selected recipient is ignored. This will either fail if the API requires a recipient or silently drop the user's selection.Check the generated mutation type and add the recipient field to
emailParams:const emailParams = { body_text: values.body, subject_text: values.subject, + recipient_email: values.to, };If the API field name differs, adjust accordingly. Verify with:
#!/bin/bash # Check the mutation request type for postPayablePurchaseOrdersIdSend ast-grep --pattern 'postPayablePurchaseOrdersIdSend$$$' rg -nP 'postPayablePurchaseOrdersIdSend.*body.*email' packages/sdk-react/src/api/packages/sdk-react/src/components/payables/PurchaseOrders/sections/VendorSection.tsx (3)
43-77: Fetch counterpart by ID to ensure correct default billing address.The
activeCounterpartfallback (lines 46-47) only uses thecounterpartprop'sdefault_billing_address_idwhen the prop matchesselectedCounterpartId. When a user selects a different counterpart, the prop may not have updated yet, soactiveCounterpartbecomesundefinedand the default billing address is only selected if exactly one address exists. This means the counterpart's designated default billing address is ignored in most cases.Use the existing
useCounterpartById(selectedCounterpartId)hook to always fetch the up-to-date counterpart data for the selected ID:+ const { data: selectedCounterpart } = useCounterpartById(selectedCounterpartId); const { data: counterpartAddresses } = useCounterpartAddresses( selectedCounterpartId ); @@ useEffect(() => { if (!counterpartAddresses?.data?.length) return; - const activeCounterpart = - counterpart?.id === selectedCounterpartId ? counterpart : undefined; + const activeCounterpart = selectedCounterpart; @@ }, [ counterpartAddresses, setValue, - counterpart, + selectedCounterpart, selectedAddressId, selectedCounterpartId, ]);
97-97: RemoveisInvoiceCreationfrom Purchase Order flow.The
isInvoiceCreationprop is specific to invoice contexts. In a Purchase Order flow, this should be omitted (it defaults tofalse) to avoid invoice-specific behavior.<CreateCounterpartModal open={isCreateCounterpartOpened} onClose={() => { setIsCreateCounterpartOpened(false); }} onCreate={(newCounterpartId: string) => { setValue('counterpart_id', newCounterpartId); }} customerTypes={vendorTypes} - isInvoiceCreation />
103-104: Avoid unsafeas stringcast for address IDs.When
selectedAddressIdisundefined, theas stringcast is unsafe. Use a fallback to an empty string instead.<EditCounterpartModal initialCounterpartId={selectedCounterpartId ?? ''} - initialBillingAddressId={selectedAddressId as string} - initialShippingAddressId={selectedAddressId as string} + initialBillingAddressId={selectedAddressId ?? ''} + initialShippingAddressId={selectedAddressId ?? ''} disabled={disabled}packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/ExistingPurchaseOrderDetails.tsx (1)
216-252: Keep “Download PDF” visibility consistent.The top-level download button only shows for issued orders, yet the dropdown still exposes “Download PDF” for drafts here. That reintroduces the inconsistency previously flagged—users can still download in draft state via the menu. Please remove the draft-menu entry and surface it under the issued branch instead.
packages/sdk-react/src/components/payables/Payables.tsx (1)
221-256: Don’t gate the tabs on bill read permission only.When
isReadAllowedis false, the tabs never render, so a user with onlypayables_purchase_order:readis locked out of the PO tab entirely. Fetch/check the PO read permission and allow the layout when either resource is readable (or let the PO tab enforce its own access restriction).packages/sdk-react/src/components/payables/PurchaseOrders/CreatePurchaseOrder.tsx (3)
147-153: Move the VAT error toast into an effect.This toast fires on every render while
vatIdsErrorremains set, spamming users. Wrap the message/toast.errorin auseEffectkeyed on[vatIdsError, i18n]so it runs only when the error first appears.
308-506: Guard currency changes so you never writeundefined.
tempCurrencystarts asundefinedand isn’t reset when reopening the modal. Clicking “Save” with no selection (or after reopening) drivessetActualCurrency(tempCurrency), forcingactualCurrencytoundefinedand breaking submission; it also warns whenever any items exist, even if they already match. Initialise/resettempCurrencyfromactualCurrencywhen opening, and bail if no new value was chosen—only warn when mismatched currencies exist.
53-792: Replace new MUI UI with shadcn + Tailwind.This newly added component introduces fresh MUI usage (
Modal,Box,Grid,Typography,Alert,Switch,FormControlLabel, etc.), which violates the SDK guideline to use shadcn/ui + Tailwind for new UI. Please migrate these sections to the shadcn equivalents (Dialog, Alert, Switch, Checkbox, etc.) and tailwind classes, removing the MUI imports.packages/sdk-react/src/components/payables/PurchaseOrders/validation.ts (1)
117-148: Date parsing can shift dates by one day in western timezones.
new Date("2025-03-15")interprets ISO date-only strings as UTC midnight, causing the date to render as the previous day for users west of UTC (e.g., "2025-03-14 4:00 PM" in California). While the validation's.refine()normalizes to local midnight for comparison, the underlying Date object stored/transmitted will have the UTC-interpreted value, potentially causing off-by-one errors in display, storage, or other parts of the system.Consider parsing date-only strings as local dates:
+import { parse } from 'date-fns'; + expiry_date: z.preprocess( (val) => { if (!val) return undefined; if (val instanceof Date) return val; - const date = new Date(val as string); + const s = String(val).trim(); + // Parse date-only strings (YYYY-MM-DD) as local dates to avoid UTC offset + const date = /^\d{4}-\d{2}-\d{2}$/.test(s) + ? parse(s, 'yyyy-MM-dd', new Date()) + : new Date(s); return isNaN(date.getTime()) ? undefined : date; },This ensures user-entered dates remain in their local timezone throughout the application.
🧹 Nitpick comments (26)
packages/sdk-react/src/core/hooks/useLocalStorageFields.ts (1)
37-50: Consider wrappingsetValueinuseCallbackfor stable reference.After applying the functional update fix, wrapping
setValueinuseCallbackwould prevent unnecessary re-renders in components that include it in dependency arrays. Dependencies would be[prefixedKey, isRemembered].Example:
const setValue = useCallback((value: T | ((prev: T) => T)) => { try { setStoredValue((prev) => { const valueToStore = value instanceof Function ? value(prev) : value; if (typeof window !== 'undefined' && isRemembered) { window.localStorage.setItem(prefixedKey, JSON.stringify(valueToStore)); } return valueToStore; }); } catch (error) { console.error('Error saving to localStorage:', error); } }, [prefixedKey, isRemembered]);packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrders.test.tsx (2)
35-45: Remove redundant length assertion.The
toEqualmatcher already validates the entire array structure, making the separate length check unnecessary.Apply this diff:
- expect(PURCHASE_ORDER_MEASURE_UNITS).toEqual([ + expect(PURCHASE_ORDER_MEASURE_UNITS).toEqual([ 'unit', 'cm', 'day', 'hour', 'kg', 'litre', ]); - expect(PURCHASE_ORDER_MEASURE_UNITS).toHaveLength(6);
101-105: Consider validating against type definition.The test hardcodes status strings. If
PurchaseOrderStatustype exists, consider asserting against that type definition for better type safety.examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.validation.spec.ts (1)
21-39: Test logic contradicts test name.The test is named "should prevent saving until required fields are filled" but line 30 asserts the Save button is enabled on an empty form, then clicks it. If the test validates prevention of saving, either:
- The button should be disabled initially (assert
toBeDisabled()), or- The button is enabled but clicking triggers validation (current implementation at lines 34-36)
The current flow (enabled → click → validation message) suggests the app allows the click but shows errors, which is valid. However, the assertion at line 30 doesn't verify prevention—it just confirms the button can be clicked.
Consider clarifying the test intent:
- await expect(saveButton).toBeEnabled(); + // Button is enabled but save should fail validation + await expect(saveButton).toBeEnabled();Or if the intent is to verify the button becomes enabled only after filling fields, restructure to fill fields incrementally and assert button state changes.
examples/with-nextjs-and-clerk-auth/e2e/utils/vendor-helpers.ts (1)
3-75: Consider adding return type documentation.The function returns
string | nullrepresenting the selected/created vendor name, but the purpose of the null return isn't immediately clear from the signature.Add a JSDoc comment explaining the return values:
+/** + * Ensures a vendor is selected or creates a new one if needed. + * @returns The vendor name/text if successful, null if vendor field not found or on error + */ export async function createVendorIfNeeded(page: Page): Promise<string | null> {packages/sdk-react/src/components/shared/ItemsSection/types.ts (2)
3-8: Constrain TForm to avoidneverfieldMapping keysWith
TForm = unknown,LineItemOf<TForm>resolves tonever, forcing allfieldMappingprops tonever. ConstrainTFormto includeline_itemsfor better DX.Apply:
-export interface ItemsSectionConfig< - TForm = unknown, - TMeasureUnit extends string = string, -> { +export interface ItemsSectionConfig< + TForm extends { line_items: unknown[] } = { line_items: unknown[] }, + TMeasureUnit extends string = string +> {Also applies to: 9-13
7-7: Consider exportingLineItemDeepKeysIf other modules need to compute deep keys for line items, export this alias to prevent redefinition elsewhere.
-type LineItemDeepKeys<TForm> = DeepKeys<LineItemOf<TForm>>; +export type LineItemDeepKeys<TForm> = DeepKeys<LineItemOf<TForm>>;packages/sdk-react/src/components/shared/ItemsSection/useFormErrors.ts (1)
5-9: Unify error typings with existing receivables typesLocal
ValidationErrorItem/LineItemPathare broad and duplicate shapes used under receivables. Import and reuse shared/receivables types to avoid divergence, or extract a shared type.Based on learnings
packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (2)
43-72: Generic props vs fixed RHF form type — align or constrainComponent is generic (
TForm) butuseFormContextand allFieldPath*usages are hard-wired toCreateReceivablesFormBeforeValidationProps. This defeats the generic config.Options:
- Constrain
TFormtoCreateReceivablesFormBeforeValidationPropsfor now (explicitly non-generic).- Or thread
TFormthrough:useFormContext<TForm>()and replace allFieldPath<CreateReceivables...>withFieldPath<TForm>and correspondingFieldPathValue<TForm, ...>.Choose one path to keep types sound.
Also applies to: 101-123
78-85: Avoid duplicate measure units fetch
useLineItemManagementalready queries measure units (per helper docs). This file also queries them and buildseffectiveMeasureUnitsData. Consider sourcing measure units from one place to reduce duplicate requests and cache pressure.
- Expose measure units data from
useLineItemManagement, or- Lift the query to a parent and pass down.
Based on learnings
Also applies to: 86-100, 270-298
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/InvoiceItemRow.tsx (1)
7-15: OK: Using OpenAPI-derived types and RHF generics.Type usage via components and FieldErrors looks good; keeping MUI here is acceptable since UI isn’t being reworked. Consider a TODO if/when migrating to shadcn/ui.
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/VatRateField.tsx (2)
12-12: Props surface extension looks good.currentVatRateValue addition and narrowing fieldError to FieldError are fine. Note: fieldError isn’t used in render.
Wire fieldError into the Select’s error or helper text.
Also applies to: 17-18, 36-36
133-150: Leverage fieldError to signal error state.Use fieldError to drive error prop for accuracy.
- <Select + <Select MenuProps={{ container: root }} value={value ?? ''} onChange={handleChange} variant="outlined" - error={error} + error={Boolean(error || fieldError)} disabled={disabled} fullWidth size="small" >packages/sdk-react/src/components/payables/PurchaseOrders/consts.ts (2)
5-13: Make DEFAULT_FIELD_ORDER a readonly tuple and expose a field union typeImproves type-safety for consumers and prevents accidental mutation.
-export const DEFAULT_FIELD_ORDER = [ +export const DEFAULT_FIELD_ORDER = [ '__check__', 'document_id', 'status', 'counterpart_id', 'created_at', 'issued_at', 'amount', -]; +] as const; + +export type PurchaseOrderTableField = (typeof DEFAULT_FIELD_ORDER)[number];
38-41: Export a status union for downstream correctnessGives callers a typed union instead of plain strings.
export const PURCHASE_ORDER_STATUSES = { DRAFT: 'draft', ISSUED: 'issued', } as const; + +export type PurchaseOrderStatus = (typeof PURCHASE_ORDER_STATUSES)[keyof typeof PURCHASE_ORDER_STATUSES];packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreviewMonite.tsx (2)
291-297: Use a single actualCurrency for taxes tooTax lines disappear when currency is undefined; totals show USD. Align with the hook’s actualCurrency.
- {currency && - formatCurrencyToDisplay( - taxAmount as number, - currency, - true - )} + {formatCurrencyToDisplay( + taxAmount as number, + currency ?? 'USD', + true + )}
281-307: Subtotal/Total are already formatted; drop toString()Minor polish; avoids accidental “undefined” stringification.
- {subtotalPrice?.toString()} + {subtotalPrice ?? ''} @@ - {totalPrice?.toString()} + {totalPrice ?? ''}packages/sdk-react/src/components/payables/CreatePayableMenu/CreatePayableMenu.tsx (3)
92-93: Coerce anyCreateAllowed to booleanPrevents “boolean | undefined” flows and keeps props strict.
-const anyCreateAllowed = isCreateAllowed || isCreatePurchaseOrderAllowed; +const anyCreateAllowed = Boolean(isCreateAllowed || isCreatePurchaseOrderAllowed);
114-126: Disable tab triggers by permissionAvoids navigating into an unavailable flow.
-<TabsTrigger - value="bill" +<TabsTrigger + value="bill" + disabled={!isCreateAllowed} @@ -<TabsTrigger - value="purchase-order" +<TabsTrigger + value="purchase-order" + disabled={!isCreatePurchaseOrderAllowed}
48-49: Reset file input after upload to allow re-selecting same fileCommon UX; prevents stale value.
-const { FileInput, openFileInput, checkFileError } = useFileInput(); +const { FileInput, openFileInput, checkFileError, resetInput } = useFileInput(); @@ const files = event.target.files; if (files) handleFileUpload(files); setOpen(false); + resetInput();Also applies to: 195-199
packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx (1)
70-86: Inline styles for safe centering reduce maintainability.Using inline
style={{ justifyContent: 'safe center', alignItems: 'safe center' }}at lines 83-84 breaks the Tailwind-only styling guideline and reduces maintainability. While CSSsafekeyword prevents overflow issues, mixing style systems is discouraged.As per coding guidelines
Consider a Tailwind-only approach with overflow handling:
<div ref={containerRef} className={cn( - // Container layout - 'mtw:flex mtw:overflow-auto mtw:relative', + 'mtw:flex mtw:items-center mtw:justify-center', + 'mtw:overflow-auto mtw:relative', 'mtw:w-full mtw:h-full mtw:min-h-0', 'mtw:p-12', 'mtw:bg-gradient-to-br mtw:from-slate-50 mtw:to-slate-200', - // Force pixel-snapping for zoom stability 'mtw:[transform:translateZ(0)]' )} - style={{ - justifyContent: 'safe center', - alignItems: 'safe center', - }} >The
overflow-autoalready provides scroll fallback when content exceeds container.packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/PurchaseOrderPDFViewer.tsx (2)
44-60: Consider using Alert component for error state.The not-found state at lines 44-60 manually composes the error UI with Tailwind classes. The codebase likely has a reusable Alert or error component from shadcn/ui that would provide better consistency.
As per coding guidelines
If
@/ui/components/alertexists, use it:+import { Alert, AlertDescription, AlertTitle } from '@/ui/components/alert'; + if (!purchaseOrder) { return ( - <div className="mtw:p-4"> - <div className="mtw:flex mtw:flex-col mtw:items-center mtw:gap-2"> - <AlertCircle className="mtw:text-red-500 mtw:h-8 mtw:w-8" /> - <div className="mtw:flex mtw:flex-col mtw:items-center mtw:gap-1"> - <div className="mtw:text-sm mtw:font-medium">{t( - i18n - )`Purchase order not found`}</div> - <div className="mtw:text-xs mtw:text-muted-foreground"> - {t(i18n)`The purchase order could not be loaded.`} - </div> - </div> - </div> - </div> + <Alert variant="destructive"> + <AlertCircle className="mtw:h-4 mtw:w-4" /> + <AlertTitle>{t(i18n)`Purchase order not found`}</AlertTitle> + <AlertDescription> + {t(i18n)`The purchase order could not be loaded.`} + </AlertDescription> + </Alert> ); }
62-76: Spinner could use reusable component.The manual spinner div at lines 66 uses inline Tailwind animation classes. If a reusable Spinner component exists in the UI library, it would improve consistency.
Check if
@/ui/components/spinneror similar exists and use it:+import { Spinner } from '@/ui/components/spinner'; + <CenteredContentBox> <div className="mtw:flex mtw:flex-col mtw:items-center mtw:gap-2 mtw:text-center"> - <div className="mtw:w-5 mtw:h-5 mtw:animate-spin mtw:rounded-full mtw:border-2 mtw:border-foreground mtw:border-t-transparent" /> + <Spinner size="sm" /> <div className="mtw:text-sm mtw:font-medium">{t( i18n )`Updating the purchase order`}</div>packages/sdk-react/src/components/payables/PurchaseOrders/Filters/Filters.tsx (2)
21-22: Consider completing migration to Tailwind for consistency.Per coding guidelines, new components should use Tailwind utilities instead of MUI
sxprops. The Select components correctly use shadcn/ui, but the component still accepts and uses the MUIsxpattern. The inlinesxobject at lines 52-56 could be replaced with Tailwind utilities (e.g.,className="mtw:rounded-t-xl"), though this depends on whetherFilterContainerhas been migrated.Based on learnings: when modifying files with Material UI, only update to shadcn/ui if the component needs significant changes; otherwise, maintain existing MUI implementation.
If
FilterContainerremains MUI-based and this component is primarily a thin wrapper, keeping thesxprop for now is acceptable. Otherwise, migrate the border radius styling to Tailwind:- sx={{ - borderTopLeftRadius: '12px', - borderTopRightRadius: '12px', - ...sx, - }} + className="mtw:rounded-t-xl"And remove the
sxprop from the component interface if no longer needed.Also applies to: 29-29, 50-56
42-47: Consider pagination or search for vendor filter scalability.The vendor query is limited to 100 results. If an entity has more than 100 vendors, they won't appear in the filter dropdown. For organizations with large vendor lists, consider implementing autocomplete/search functionality or increasing the limit with pagination support.
Example approaches:
- Increase limit with a reasonable ceiling (e.g., 500)
- Add server-side search to the vendor Select component
- Implement infinite scroll/pagination for the dropdown
This is common for filter dropdowns, so the current implementation is acceptable for typical use cases.
packages/sdk-react/src/components/payables/PurchaseOrders/components/CreatePurchaseOrderForm.tsx (1)
14-14: Unused prop: counterpartAddresses.The
counterpartAddressesprop is declared but never used in the component body. Consider removing it or wiring it to a child component if needed.
...pprovalPolicyDetails/ApprovalPolicyDetailsFormAdvanced/ApprovalPolicyDetailsFormAdvanced.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/CreatePayableMenu/CreatePayableMenu.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/CreatePurchaseOrder.tsx
Outdated
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderDetails.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderDetails.tsx
Show resolved
Hide resolved
...dk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx
Outdated
Show resolved
Hide resolved
...nts/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/InvoiceItemRow.tsx
Show resolved
Hide resolved
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/utils.ts
Outdated
Show resolved
Hide resolved
costa-monite
force-pushed
the
feat/DEV-16185-purchase-orders
branch
from
4080da0 to
457ef31
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts (1)
72-75: Update setDefaultTemplate parameters to use TemplateDocument union.The parameters are typed as
TemplateReceivableResponse['id']andTemplateReceivableResponse['name'], which breaks when a purchase order template is passed. Use theTemplateDocumentunion instead (once it's fixed per comment below).Apply this diff:
const setDefaultTemplate = async ( - id: components['schemas']['TemplateReceivableResponse']['id'], - name: components['schemas']['TemplateReceivableResponse']['name'] + id: TemplateDocument['id'], + name: TemplateDocument['name'] ) => {packages/sdk-react/src/components/receivables/components/InvoiceDetailsActions.tsx (1)
249-256: Duplicate action still gated by update permission
handleButtonClickonly checksisUpdateAllowed, so users with create rights but no update rights still get blocked from duplicating. Gate duplication with the new create permission instead of reusing the update-only helper.@@ - const PERMISSION_ERROR_MESSAGE = … + const PERMISSION_ERROR_MESSAGE = … + const CREATE_PERMISSION_ERROR_MESSAGE = t( + i18n + )`You don't have permission to create this document. Please, contact your system administrator for details.`; @@ - disabled={isCreateAllowedLoading || !isCreateAllowed} - onClick={() => handleButtonClick(handleDuplicateInvoice)} + disabled={isCreateAllowedLoading || !isCreateAllowed} + onClick={() => + isCreateAllowed + ? handleDuplicateInvoice() + : toast.error(CREATE_PERMISSION_ERROR_MESSAGE) + } @@ - <DropdownMenuItem - disabled={isCreateAllowedLoading || !isCreateAllowed} - onClick={() => handleButtonClick(handleDuplicateInvoice)} - > + <DropdownMenuItem + disabled={isCreateAllowedLoading || !isCreateAllowed} + onClick={() => + isCreateAllowed + ? handleDuplicateInvoice() + : toast.error(CREATE_PERMISSION_ERROR_MESSAGE) + } + >Also applies to: 291-293
♻️ Duplicate comments (24)
packages/sdk-react/src/components/payables/PurchaseOrders/components/VendorSelector.tsx (2)
11-314: Migrate this new component to shadcn/ui + lucide-react per SDK guidelinesThis file introduces fresh MUI components/icons (
Autocomplete,Button,TextField,IconButton,CircularProgress,Divider,KeyboardArrowDown,AddIcon,ClearIcon, plussxstyling), butpackages/sdk-react/**/*.tsxmust use shadcn/ui building blocks, Tailwindmtw-*classes, andlucide-reacticons. Please replace the MUI controls with shadcn/ui equivalents (Combobox/Command + Popover for the dropdown, Button/Input/IconButton replacements, Loader for spinners, etc.), swap icons to lucide, and move styling to Tailwind classes. If migration can’t happen immediately, add a TODO flagging the required follow-up. As per coding guidelines
182-314: Wire thedisabledprop through to the rendered controlsConsumers can’t actually disable this selector—the
<Autocomplete>and nested<TextField>ignore thedisabledprop exposed byVendorSelector. Please pass it through so the control respects a disabled state.<Autocomplete + disabled={disabled} @@ <TextField {...params} + disabled={disabled}packages/sdk-react/src/components/templateSettings/hooks/useDocumentTemplatesApi.ts (1)
101-104: Critical: TemplateDocument must include purchase order templates.Line 104 defines
TemplateDocumentas onlyTemplateReceivableResponse, but the filtering logic (lines 36-38) returns purchase order templates whendocumentType='purchase_order'. This type mismatch will cause TypeScript errors or runtime failures when purchase order templates are accessed.Despite your earlier comment that "it is already there in the DocumentTypeEnum" (which correctly includes purchase_order at line 102), the
TemplateDocumenttype itself at line 104 still only references the receivable schema.Apply this diff to fix the type definition:
type TemplateDocumentType = | components['schemas']['DocumentTypeEnum'] | 'invoice'; -type TemplateDocument = components['schemas']['TemplateReceivableResponse']; +type TemplateDocument = + | components['schemas']['TemplateReceivableResponse'] + | components['schemas']['TemplatePurchaseOrderResponse'];packages/sdk-react/src/components/templateSettings/components/LayoutAndLogo.tsx (1)
92-92: Critical: DocumentTemplate type must support purchase_order templates.Using
components['schemas']['TemplateReceivableResponse']is incorrect whendocumentType="purchase_order". The hook can return purchase order templates, but this type only covers receivables, causing a type mismatch.Apply this diff to align with the hook's return type:
-type DocumentTemplate = components['schemas']['TemplateReceivableResponse']; +type DocumentTemplate = + | components['schemas']['TemplateReceivableResponse'] + | components['schemas']['TemplatePurchaseOrderResponse'];packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/utils.ts (1)
333-333: Avoid empty string for measure_unit_id.Use
undefinedwhen missing; empty strings often fail schema validation and cause noisy diffs.Apply this diff:
- measure_unit_id: extItem.product?.measure_unit_id || flatUnit || '', + measure_unit_id: extItem.product?.measure_unit_id || flatUnit || undefined,examples/with-nextjs-and-clerk-auth/e2e/utils/vendor-helpers.ts (2)
15-34: Scope option selection to the opened vendor listbox.The past review comment about this issue was not addressed. Line 15-17 uses
page.locator('[role="option"]'), which searches the entire page and can match options from other dropdowns or components that may be present simultaneously.Apply this diff to scope the search to the vendor listbox:
await vendorCombo.click(); await page.waitForTimeout(500); + + // Scope option search to the vendor listbox + const listboxId = await vendorCombo.getAttribute('aria-controls'); + const listbox = listboxId + ? page.locator(`#${listboxId}`) + : page.locator('[role="listbox"]').filter({ has: page.getByRole('option') }).first(); - const existingOptions = page - .locator('[role="option"]') + const existingOptions = listbox + .getByRole('option') .filter({ hasNotText: /Create new/i });
145-156: Scope country option selection to the country listbox.The past review comment about this issue was not addressed. Lines 145 and 150 use
page.getByRole('option'), which can match options from any dropdown on the page (including the vendor combobox if it's still open or other fields).Apply this diff to scope to the country listbox:
if (await countryField.isVisible()) { await countryField.click(); + await page.waitForTimeout(300); // Allow listbox to open + + // Scope option search to country listbox + const countryListboxId = await countryField.getAttribute('aria-controls'); + const countryListbox = countryListboxId + ? page.locator(`#${countryListboxId}`) + : page.locator('[role="listbox"]').last(); - const usOption = page.getByRole('option', { name: /United States|USA/i }); + const usOption = countryListbox.getByRole('option', { name: /United States|USA/i }); if (await usOption.isVisible().catch(() => false)) { await usOption.click(); } else { - const firstCountry = page.getByRole('option').first(); + const firstCountry = countryListbox.getByRole('option').first(); if (await firstCountry.isVisible()) { await firstCountry.click(); }packages/sdk-react/src/components/payables/PurchaseOrders/__tests__/PurchaseOrders.test.tsx (1)
107-116: Fix status chip expectations to match rendered casing.The test expects lowercase 'draft' and 'issued', but
PurchaseOrderStatusChiplikely renders title-case labels ("Draft", "Issued"). Use case-insensitive matching to avoid false failures.Apply this diff:
- expect(screen.getByText('draft')).toBeInTheDocument(); + expect(screen.getByText(/draft/i)).toBeInTheDocument(); rerender(<PurchaseOrderStatusChip status="issued" />); - expect(screen.getByText('issued')).toBeInTheDocument(); + expect(screen.getByText(/issued/i)).toBeInTheDocument();packages/sdk-react/src/components/payables/PurchaseOrders/Filters/Filters.tsx (2)
79-86: Precompute status text map outside the loopCalling
getStatusTextMap(i18n)inside the map creates a new object on each iteration. Compute it once before the loop:<SelectContent> <SelectItem value="all">{t(i18n)`All statuses`}</SelectItem> - {PurchaseOrderStatusEnum.map((status) => { - const map = getStatusTextMap(i18n); - return ( - <SelectItem value={status} key={status}> - {map[status as keyof typeof map]} - </SelectItem> - ); - })} + {(() => { + const map = getStatusTextMap(i18n); + return PurchaseOrderStatusEnum.map((status) => ( + <SelectItem value={status} key={status}> + {map[status as keyof typeof map]} + </SelectItem> + )); + })()} </SelectContent>
22-22: Wrong Theme import — will not compileThe import
Themefrom'mui-styles'is incorrect and will cause compilation errors. Use the correct path:-import type { Theme } from 'mui-styles'; +import type { Theme } from '@mui/material/styles';packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderDetails.tsx (2)
50-53: Add trigger to open email dialogThe
emailDialogstate is initialized but never set toopen: true, so theEmailPurchaseOrderDetailscomponent at lines 91-98 will never be displayed. You need to add a user action (e.g., button click) that callssetEmailDialog({ open: true, mode: 'send' }).
77-89: Type mismatch: onSave accepts union but typed as string-onlyThe
onSavehandler here accepts either astring(purchase order ID) orPurchaseOrderFormDataobject (lines 80-86), butPurchaseOrderFormProps.onSaveis typed to accept onlystring(see PurchaseOrderForm.tsx line 9).Update
PurchaseOrderFormPropsto match actual usage:interface PurchaseOrderFormProps { purchaseOrder?: PurchaseOrderResponse | null; isCreate?: boolean; vendorTypes?: CustomerTypes; - onSave?: (purchaseOrderId: string) => void; + onSave?: (value: string | PurchaseOrderFormData) => void; onUpdate?: (purchaseOrder: PurchaseOrderResponse) => void; onCancel?: () => void; }packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/useExistingPurchaseOrderDetails.tsx (1)
61-78: Remove incorrect delivery method guardThe guard at lines 62-64 incorrectly prevents issuing when
deliveryMethod !== Download. However, issuing a purchase order is done via email (as the comment at lines 66-69 indicates), so this check blocks the correct delivery method.Remove the guard or change it to require Email:
const handleIssueOnly = useCallback(() => { - if (deliveryMethod !== DeliveryMethod.Download) { - throw new Error('Unsupported delivery method'); - } - // For purchase orders, there's no separate issue endpoint // Purchase orders are "issued" when they're sent via email // For "Issue only" mode, we still need to send an email but it's a minimal send // This will need to be implemented with actual email sending logic sendMutation.mutate({ path: { purchase_order_id: purchaseOrderId }, body: { subject_text: t(i18n)`Purchase Order`, body_text: t(i18n)`Please find the purchase order attached.`, }, header: { 'x-monite-entity-id': entityId }, }); }, [deliveryMethod, sendMutation, purchaseOrderId, i18n, entityId]);packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrderForm.tsx (1)
5-12: Widen onSave type to match actual usageThe
onSaveprop is currently typed to accept only astring(purchase order ID), butPurchaseOrderDetails.tsx(lines 80-86) passes a handler that receives either astringorPurchaseOrderFormDataobject.You need to either:
- Widen the type to accept both, or
- Separate the concerns by using
onUpdatefor the form data caseOption 1 (widen the type):
+import type { PurchaseOrderFormData } from './schemas'; + interface PurchaseOrderFormProps { purchaseOrder?: PurchaseOrderResponse | null; isCreate?: boolean; vendorTypes?: CustomerTypes; - onSave?: (purchaseOrderId: string) => void; + onSave?: (payload: string | PurchaseOrderFormData) => void; onUpdate?: (purchaseOrder: PurchaseOrderResponse) => void; onCancel?: () => void; }packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/ExistingPurchaseOrderDetails.tsx (1)
134-142: Forward theonSendEmailcallback.
ExistingPurchaseOrderDetailsPropsadvertisesonSendEmail, but it isn’t passed intoEmailPurchaseOrderDetails, so consumers never receive the notification after a successful send. Please forward the prop when rendering the email view (and ensure the child invokes it, which it already does once the previous comment is addressed).<EmailPurchaseOrderDetails purchaseOrderId={purchaseOrder.id} isOpen={true} onClose={() => { setPresentation(PurchaseOrderDetailsPresentation.Overview); }} mode={purchaseOrder.status === 'draft' ? 'issue_and_send' : 'send'} + onSendEmail={props.onSendEmail} />packages/sdk-react/src/components/payables/PurchaseOrders/EmailPurchaseOrderDetails.tsx (2)
155-188: Include the recipient in the send payload.
emailParamsonly forwardsbody_textandsubject_text, so the selected recipient never leaves the form. The send mutation therefore drops thetoaddress, breaking user expectations and likely the API contract (postPayablePurchaseOrdersIdSendrequires the recipient). Please propagatevalues.tointo the payload (and ensure it matches the generated request shape) before invokingsendEmail.- const emailParams = { - body_text: values.body, - subject_text: values.subject, - }; + const emailParams = { + to: values.to, + body_text: values.body, + subject_text: values.subject, + };
162-168: Remove the extra success toast.
useSendPurchaseOrderByIdalready emits a success toast on completion, so this additionaltoast.successduplicates the notification. Please drop the local toast and keep only the callback/close logic.- onSuccess: () => { - toast.success( - mode === 'issue_and_send' - ? t(i18n)`Purchase order issued and sent successfully` - : t(i18n)`Purchase order sent successfully` - ); + onSuccess: () => { onSendEmail?.(purchaseOrderId); onClose();packages/sdk-react/src/components/payables/PurchaseOrders/PurchaseOrdersTable.tsx (1)
16-17: Fix import casing for UI components (likely break on case-sensitive FS).Use PascalCase paths to match file names.
Apply this diff:
-import { AccessRestriction } from '@/ui/accessRestriction'; -import { LoadingPage } from '@/ui/loadingPage'; +import { AccessRestriction } from '@/ui/AccessRestriction'; +import { LoadingPage } from '@/ui/LoadingPage';packages/sdk-react/src/components/payables/PurchaseOrders/CreatePurchaseOrder.tsx (3)
143-149: Prevent repeated toasts: move error toast into useEffect.Toast fires on every render while vatIdsError is set. Wrap in useEffect.
+ useEffect(() => { + if (vatIdsError) { + const message = getAPIErrorMessage(i18n, vatIdsError); + if (message) toast.error(message); + } + }, [vatIdsError, i18n]); - if (vatIdsError) { - const message = getAPIErrorMessage(i18n, vatIdsError); - - if (message) { - toast.error(message); - } - }
475-493: Guard against undefined tempCurrency and check actual currency mismatches.If user opens modal and clicks Save without selecting,
tempCurrencycould be undefined. Also, warning should only show for items whose currency differs from the selection.const handleCurrencySubmit = useCallback(() => { + const next = tempCurrency ?? actualCurrency; + if (!next) return; // safety guard + - if (tempCurrency !== actualCurrency) { + if (next !== actualCurrency) { - const validLineItems = lineItems.filter((item) => { - return item.name?.trim() !== ''; - }); + // Only warn when there are items whose currency differs from selection + const mismatched = (lineItems || []).filter( + (item) => (item.name?.trim() || '') !== '' && item.currency !== next + ); - if (validLineItems.length) { + if (mismatched.length > 0) { setRemoveItemsWarning(true); + return; } else { setRemoveItemsWarning(false); - setActualCurrency(tempCurrency); + setActualCurrency(next); - handleCloseCurrencyModal(); } } else { setRemoveItemsWarning(false); - setTempCurrency(actualCurrency); - handleCloseCurrencyModal(); } + handleCloseCurrencyModal(); }, [tempCurrency, actualCurrency, lineItems, handleCloseCurrencyModal]);
51-57: Migrate MUI components to shadcn/ui and Tailwind.New components should avoid MUI. Replace Modal → Dialog, Box/Stack/Grid → div with Tailwind, Alert → shadcn Alert.
As per coding guidelines
Example for currency modal (lines 582-658):
-import { Alert, Box, Stack, Modal, Grid } from '@mui/material'; +import { Dialog, DialogContent, DialogHeader, DialogTitle } from '@/ui/components/dialog'; +import { Alert, AlertDescription } from '@/ui/components/alert';Then refactor Modal markup to Dialog pattern:
-<Modal open={isCurrencyModalOpen} container={root} onClose={handleCloseCurrencyModal}> - <Box sx={{ position: 'relative', top: '50%', left: '50%', transform: 'translate(-50%, -50%)', maxWidth: 600, bgcolor: 'background.paper', boxShadow: 24, borderRadius: 2 }}> - <Grid container alignItems="center" p={4}> +<Dialog open={isCurrencyModalOpen} onOpenChange={setIsCurrencyModalOpen}> + <DialogContent container={root} className="mtw:max-w-[600px] mtw:p-6"> + <DialogHeader> + <DialogTitle className="mtw:text-2xl">{t(i18n)`Change document currency`}</DialogTitle> + </DialogHeader>Apply similar pattern to enable fields modal and use semantic divs + Tailwind for Stack/Box/Grid elsewhere.
packages/sdk-react/src/components/payables/PurchaseOrders/hooks/usePurchaseOrderDetails.tsx (1)
92-94: Allow clearing message on update.Using
if (message)prevents setting an empty string, so users can't clear the message. Check forundefinedinstead.- if (message) { + if (message !== undefined) { payload.message = message; }packages/sdk-react/src/components/payables/PurchaseOrders/types.ts (1)
23-23: Fix the entity_vat_id typo.The property name has doubled
_idsuffix (entity_vat_id_id), breaking type-checks and form data plumbing. Rename toentity_vat_id.- entity_vat_id_id?: string; + entity_vat_id?: string;packages/sdk-react/src/components/payables/PurchaseOrders/sections/components/PurchaseOrderPreview.tsx (1)
38-49: Fix potential off-by-one day for ISO date-only strings.
new Date('YYYY-MM-DD')parses as UTC in JS and can shift the day in non-UTC time zones. Parse date-only strings as local dates.+import { addDays, parse, parseISO } from 'date-fns'; … const expiryDate = useMemo(() => { - if (purchaseOrderData.expiry_date) { - const d = new Date(purchaseOrderData.expiry_date); - d.setUTCHours(0, 0, 0, 0); - return d; - } + const raw = purchaseOrderData.expiry_date; + if (raw) { + let d: Date; + if (raw instanceof Date) { + d = new Date(raw); + } else { + const s = String(raw).trim(); + // Parse date-only as local date + d = /^\d{4}-\d{2}-\d{2}$/.test(s) + ? parse(s, 'yyyy-MM-dd', new Date()) + : parseISO(s); + } + d.setHours(0, 0, 0, 0); + return d; + } const validForDays = - purchaseOrderData.valid_for_days || + purchaseOrderData.valid_for_days ?? PURCHASE_ORDER_CONSTANTS.DEFAULT_VALID_FOR_DAYS; - return calculateExpiryDate(validForDays); + const d = calculateExpiryDate(validForDays); + d.setHours(0, 0, 0, 0); + return d; }, [purchaseOrderData.expiry_date, purchaseOrderData.valid_for_days]);
🧹 Nitpick comments (10)
packages/sdk-react/src/mocks/purchaseOrders/purchaseOrdersFixture.ts (3)
4-6: Consider independent timestamps for nested entities.While the timestamp generation now ensures logical ordering (addressing the previous review concern), reusing the same
createdDateandupdatedDatefor the counterpart (lines 19-20), entity (lines 33-34), and purchase order (lines 59-60) is semantically incorrect. In real-world scenarios, these entities would have independent creation and update times.Consider generating separate timestamp pairs for each entity:
const now = new Date(); -const createdDate = faker.date.past({ refDate: now }); -const updatedDate = faker.date.between({ from: createdDate, to: now }); + +const generateTimestamps = () => { + const created = faker.date.past({ refDate: now }); + const updated = faker.date.between({ from: created, to: now }); + return { created, updated }; +}; + +const poTimestamps = generateTimestamps(); +const counterpartTimestamps = generateTimestamps(); +const entityTimestamps = generateTimestamps(); const counterpartId = faker.string.uuid(); const entityId = faker.string.uuid();Then use the appropriate timestamps for each entity (e.g.,
counterpartTimestamps.created.toISOString()for counterpart'screated_at).
47-56: Consider adding more diverse test items.The items array currently contains a single item with a generic unit type (
'unit'). For more comprehensive testing scenarios, consider:
- Adding multiple items with different characteristics (varying quantities, prices, VAT rates)
- Using more specific unit types (e.g.,
'piece','hour','kg') to better reflect real-world usageExample enhancement:
items: [ { name: 'Test Item', quantity: 1, - unit: 'unit', + unit: 'piece', price: 10000, // $100.00 in minor units currency: 'USD', vat_rate: 1000, // 10% in basis points }, + { + name: 'Consulting Services', + quantity: 5, + unit: 'hour', + price: 15000, // $150.00 in minor units + currency: 'USD', + vat_rate: 2000, // 20% in basis points + }, ],
64-69: LGTM! Consider adding pagination fixtures for comprehensive testing.The list fixture correctly implements the pagination response structure. For more comprehensive testing of pagination scenarios, you might consider exporting additional fixtures with defined pagination tokens.
Optional: Add pagination-enabled fixtures:
export const purchaseOrderListWithNextPage: components['schemas']['PurchaseOrderPaginationResponse'] = { data: [purchaseOrderFixture], prev_pagination_token: undefined, next_pagination_token: faker.string.alphanumeric(32), };packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/PriceField.tsx (1)
52-63: Consider removinginputValuefrom useEffect dependencies.Including
inputValuein the dependency array creates a feedback loop where the effect updatesinputValue, which then triggers the effect again. While the conditionmajorValue !== inputValueprevents infinite loops, this pattern can be fragile.Apply this diff to remove
inputValuefrom dependencies and rely on a functional state update:useEffect(() => { if (!isFocused) { const majorValue = numberFormatter.format(normalizeMajorValue(value)); - if (majorValue !== inputValue) { - setInputValue(majorValue); - } + setInputValue((prev) => (prev !== majorValue ? majorValue : prev)); } - }, [value, isFocused, locale, numberFormatter, inputValue]); + }, [value, isFocused, locale, numberFormatter]);This eliminates the dependency while preserving the same behavior.
examples/with-nextjs-and-clerk-auth/e2e/utils/vendor-helpers.ts (1)
71-74: Improve error recovery in catch block.The catch block at lines 71-74 logs the error but doesn't provide any cleanup or context about which step failed. For debugging flaky tests, it would be helpful to capture the current page state.
} catch (error) { - console.error('[createVendorIfNeeded] Error:', error); + console.error('[createVendorIfNeeded] Error:', error); + console.error('[createVendorIfNeeded] Current URL:', page.url()); + // Optionally capture screenshot for debugging + try { + await page.screenshot({ path: `vendor-creation-error-${Date.now()}.png` }); + } catch (screenshotError) { + console.error('[createVendorIfNeeded] Failed to capture screenshot:', screenshotError); + } return null; }packages/sdk-react/src/components/payables/PurchaseOrders/sections/PurchaseOrderEntitySection.tsx (1)
28-52: Consider removing redundant Form wrapperThe component uses
useFormContextto access form state (line 21-22), which means it expects to be rendered within an existing form context. Wrapping the content with<Form {...form}>at line 30 may be redundant and could cause nested form context issues.If the parent component already provides the form context, remove the Form wrapper:
return ( <section className="mtw:space-y-6"> - <Form {...form}> <FormField control={control} name="message" render={({ field }) => ( <FormItem> <FormLabel className="mtw:text-sm mtw:font-medium mtw:text-foreground"> {t(i18n)`Message`} </FormLabel> <FormControl> <Textarea {...field} disabled={disabled} className={cn('mtw:min-h-[120px]', disabled && 'mtw:opacity-70')} /> </FormControl> <FormMessage /> </FormItem> )} /> - </Form> </section> );packages/sdk-react/src/components/payables/PurchaseOrders/ExistingPurchaseOrderDetails/components/EditPurchaseOrderDetails.tsx (1)
12-18: Avoid double-wrapping withMoniteScopedProviders.
CreatePurchaseOrderalready wraps its content inMoniteScopedProviders, so this extra wrapper nests providers unnecessarily and spins up duplicate contexts. Please renderEditPurchaseOrderDetailsContentdirectly and drop the outer provider.packages/sdk-react/src/components/shared/ItemsSection/ItemsSection.tsx (1)
19-34: Replace MUI icon with lucide-react (minimal change).Per guidelines, prefer lucide-react for icons. Replace AddIcon with Plus icon.
As per coding guidelines
-import AddIcon from '@mui/icons-material/Add'; +import { Plus } from 'lucide-react';At usage (line 696):
- startIcon={<AddIcon />} + startIcon={<Plus size={16} />}Also add a TODO for tracking full MUI migration:
+// TODO: Migrate from MUI to shadcn/ui + Tailwind (Button, Typography, Table, TextField, Collapse, etc.)packages/sdk-react/src/components/payables/PurchaseOrders/components/FormErrorDisplay.tsx (2)
8-20: Suggest importing ErrorDisplayProps from the shared component.The
ErrorDisplayPropstype here duplicates the type already exported by@/ui/FormErrorDisplay. While the explicit field names document the expected fields, they're redundant with the index signature and create maintenance overhead if the shared type evolves.Consider simplifying to:
+import { FormErrorDisplay as SharedFormErrorDisplay, type ErrorDisplayProps } from '@/ui/FormErrorDisplay'; -import { FormErrorDisplay as SharedFormErrorDisplay } from '@/ui/FormErrorDisplay'; - -export type ErrorDisplayProps = { - generalError?: string | null; - fieldErrors: { - name?: string | null; - quantity?: string | null; - price?: string | null; - vat_rate_id?: string | null; - vat_rate_value?: string | null; - tax_rate_value?: string | null; - unit?: string | null; - [key: string]: string | null | undefined; - }; -};If the explicit field names serve as documentation, consider adding a JSDoc comment to the import or hook instead.
22-31: Consider whether the wrapper component adds value.This component is a pure passthrough to
SharedFormErrorDisplay. If there's no plan to customize behavior for Purchase Orders, consumers could import the shared component directly, reducing indirection.If the wrapper serves to:
- Establish a consistent local import path
- Enable future Purchase Order-specific customization
- Maintain symmetry with the
useFormErrorshookThen consider adding a JSDoc comment explaining the intent:
+/** + * FormErrorDisplay wrapper for Purchase Orders. + * Provides a consistent import path and enables future PO-specific error display customization. + */ export const FormErrorDisplay = memo(Otherwise, consumers could import directly from
@/ui/FormErrorDisplay.
| test.skip('should prevent saving until required fields are filled', async ({ | ||
| page, | ||
| }) => { | ||
| const testInfo = test.info(); | ||
| await openCreatePurchaseOrder(page); | ||
|
|
||
| const saveButton = page.getByRole('button', { | ||
| name: /Save and continue|Save/i, | ||
| }); | ||
| await expect(saveButton).toBeEnabled(); | ||
|
|
||
| await saveButton.click(); | ||
|
|
||
| await expect( | ||
| page.getByText(/Please check the form for errors/i) | ||
| ).toBeVisible(); | ||
|
|
||
| await createAndSaveDraft(page, testInfo); | ||
| }); |
There was a problem hiding this comment.
Fix the validation test logic before unskipping.
The test has several logical inconsistencies:
- Line 30: Expects the Save button to be enabled initially, which contradicts the test's stated purpose of verifying that saving is prevented until required fields are filled.
- Lines 32-36: Clicking an enabled Save button and then expecting validation errors doesn't align with "prevent saving until required fields are filled"—if the button should be disabled, it shouldn't be clickable.
- Line 38: Calling
createAndSaveDraftafter triggering validation errors is unclear—does the test intend to verify error recovery or just error display?
Recommended fix:
If the intent is to verify that the Save button becomes enabled only after required fields are filled:
- const saveButton = page.getByRole('button', {
- name: /Save and continue|Save/i,
- });
- await expect(saveButton).toBeEnabled();
-
- await saveButton.click();
-
- await expect(
- page.getByText(/Please check the form for errors/i)
- ).toBeVisible();
-
- await createAndSaveDraft(page, testInfo);
+ // Verify Save button is disabled initially
+ const saveButton = page.getByRole('button', {
+ name: /Save and continue|Save/i,
+ });
+ await expect(saveButton).toBeDisabled();
+
+ // Fill required fields and verify button becomes enabled
+ await createAndSaveDraft(page, testInfo);
+ await expect(saveButton).toBeEnabled();If the intent is to verify that clicking Save without required fields shows validation errors:
const saveButton = page.getByRole('button', {
name: /Save and continue|Save/i,
});
- await expect(saveButton).toBeEnabled();
+ // Attempt to save without filling required fields
await saveButton.click();
+ // Verify validation error message appears
await expect(
page.getByText(/Please check the form for errors/i)
).toBeVisible();
+ // Now fill required fields and save successfully
await createAndSaveDraft(page, testInfo);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.skip('should prevent saving until required fields are filled', async ({ | |
| page, | |
| }) => { | |
| const testInfo = test.info(); | |
| await openCreatePurchaseOrder(page); | |
| const saveButton = page.getByRole('button', { | |
| name: /Save and continue|Save/i, | |
| }); | |
| await expect(saveButton).toBeEnabled(); | |
| await saveButton.click(); | |
| await expect( | |
| page.getByText(/Please check the form for errors/i) | |
| ).toBeVisible(); | |
| await createAndSaveDraft(page, testInfo); | |
| }); | |
| test.skip('should prevent saving until required fields are filled', async ({ | |
| page, | |
| }) => { | |
| const testInfo = test.info(); | |
| await openCreatePurchaseOrder(page); | |
| // Verify Save button is disabled initially | |
| const saveButton = page.getByRole('button', { | |
| name: /Save and continue|Save/i, | |
| }); | |
| await expect(saveButton).toBeDisabled(); | |
| // Fill required fields and verify button becomes enabled | |
| await createAndSaveDraft(page, testInfo); | |
| await expect(saveButton).toBeEnabled(); | |
| }); |
🤖 Prompt for AI Agents
In
examples/with-nextjs-and-clerk-auth/e2e/tests/purchase-orders.validation.spec.ts
around lines 21–39, the test logic contradicts its intent; choose one of two
clear fixes: Option A (check disabled-until-filled): assert the Save button
isDisabled initially, attempt click should be skipped or assert not clickable,
fill required fields, assert the Save button becomes enabled, click it and
assert success or draft saved; Option B (check validation-on-click): assert the
Save button is enabled initially, click it without filling fields, assert
validation error text is visible and the form is not submitted, then optionally
fill fields and assert error clears and save succeeds. Implement only the chosen
flow, remove the irrelevant steps (e.g., do not click when expecting disabled,
do not call createAndSaveDraft unless testing recovery), and make assertions
reflect the selected intent.
| await vendorCombo.click(); | ||
| await page.waitForTimeout(500); |
There was a problem hiding this comment.
Replace fixed timeout with explicit wait for listbox.
The 500ms fixed timeout at line 13 is brittle and can cause flakiness. The combobox may open faster or slower depending on system load.
Apply this diff:
await vendorCombo.click();
- await page.waitForTimeout(500);
+
+ // Wait for listbox to open
+ const listboxId = await vendorCombo.getAttribute('aria-controls');
+ if (listboxId) {
+ await page.locator(`#${listboxId}`).waitFor({ state: 'visible', timeout: 5000 });
+ } else {
+ await page.locator('[role="listbox"]').first().waitFor({ state: 'visible', timeout: 5000 });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await vendorCombo.click(); | |
| await page.waitForTimeout(500); | |
| await vendorCombo.click(); | |
| // Wait for listbox to open | |
| const listboxId = await vendorCombo.getAttribute('aria-controls'); | |
| if (listboxId) { | |
| await page.locator(`#${listboxId}`).waitFor({ state: 'visible', timeout: 5000 }); | |
| } else { | |
| await page.locator('[role="listbox"]').first().waitFor({ state: 'visible', timeout: 5000 }); | |
| } |
🤖 Prompt for AI Agents
In examples/with-nextjs-and-clerk-auth/e2e/utils/vendor-helpers.ts around lines
12 to 13, replace the brittle fixed 500ms timeout after clicking the combobox
with an explicit wait for the listbox to appear; after await vendorCombo.click()
wait for the combobox's listbox element to be visible using a role or
selector-based wait (e.g. wait for role="listbox" or
waitForSelector('[role="listbox"]', { state: 'visible' }) or the locator's
waitFor) before continuing so the test proceeds only when the options are
present.
| await expect(vendorCombo).not.toHaveAttribute('aria-expanded', 'true', { | ||
| timeout: 5000, | ||
| }); |
There was a problem hiding this comment.
Verify combobox collapse more reliably.
Lines 27-29 check that aria-expanded is not 'true', but this assertion may be flaky if the attribute is removed entirely or set to 'false'. Additionally, the combobox might not update aria-expanded synchronously after option selection.
Apply this diff for more robust verification:
await existingOptions.first().click();
- await expect(vendorCombo).not.toHaveAttribute('aria-expanded', 'true', {
- timeout: 5000,
- });
+
+ // Wait for listbox to close after selection
+ const listboxId = await vendorCombo.getAttribute('aria-controls');
+ if (listboxId) {
+ await expect(page.locator(`#${listboxId}`)).toBeHidden({ timeout: 5000 });
+ } else {
+ await page.locator('[role="listbox"]').first().waitFor({ state: 'hidden', timeout: 5000 }).catch(() => {});
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await expect(vendorCombo).not.toHaveAttribute('aria-expanded', 'true', { | |
| timeout: 5000, | |
| }); | |
| await existingOptions.first().click(); | |
| // Wait for listbox to close after selection | |
| const listboxId = await vendorCombo.getAttribute('aria-controls'); | |
| if (listboxId) { | |
| await expect(page.locator(`#${listboxId}`)).toBeHidden({ timeout: 5000 }); | |
| } else { | |
| await page | |
| .locator('[role="listbox"]') | |
| .first() | |
| .waitFor({ state: 'hidden', timeout: 5000 }) | |
| .catch(() => {}); | |
| } |
🤖 Prompt for AI Agents
In examples/with-nextjs-and-clerk-auth/e2e/utils/vendor-helpers.ts around lines
27-29, the current assertion only checks that aria-expanded is not the string
'true', which can be flaky if the attribute is removed or updated
asynchronously; replace it with a wait/poll that reads the element's
aria-expanded attribute and asserts it is not 'true' (i.e., accept either
missing attribute or 'false'), increasing the timeout if needed so the
expectation retries until the combobox collapses.
| console.log( | ||
| '[createVendorIfNeeded] Create option not found, typing vendor name' | ||
| ); | ||
| await vendorCombo.fill('Test Vendor Inc.'); | ||
| await page.keyboard.press('Enter'); | ||
|
|
||
| return 'Test Vendor Inc.'; |
There was a problem hiding this comment.
Verify fallback vendor creation.
Lines 67-68 type a vendor name and press Enter as a fallback, but there's no verification that this action succeeded. The function returns 'Test Vendor Inc.' unconditionally, which could be incorrect if the combobox doesn't accept typed input or requires a different interaction.
Apply this diff to verify the fallback succeeds:
console.log(
'[createVendorIfNeeded] Create option not found, typing vendor name'
);
await vendorCombo.fill('Test Vendor Inc.');
await page.keyboard.press('Enter');
+
+ // Verify the vendor was set
+ await page.waitForTimeout(500);
+ const vendorText = await vendorCombo.textContent();
+ if (!vendorText || vendorText.trim() === '') {
+ console.error('[createVendorIfNeeded] Failed to set vendor via typing');
+ return null;
+ }
- return 'Test Vendor Inc.';
+ return vendorText;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log( | |
| '[createVendorIfNeeded] Create option not found, typing vendor name' | |
| ); | |
| await vendorCombo.fill('Test Vendor Inc.'); | |
| await page.keyboard.press('Enter'); | |
| return 'Test Vendor Inc.'; | |
| console.log( | |
| '[createVendorIfNeeded] Create option not found, typing vendor name' | |
| ); | |
| await vendorCombo.fill('Test Vendor Inc.'); | |
| await page.keyboard.press('Enter'); | |
| // Verify the vendor was set | |
| await page.waitForTimeout(500); | |
| const vendorText = await vendorCombo.textContent(); | |
| if (!vendorText || vendorText.trim() === '') { | |
| console.error('[createVendorIfNeeded] Failed to set vendor via typing'); | |
| return null; | |
| } | |
| return vendorText; |
| async function fillVendorForm(page: Page): Promise<void> { | ||
| const orgRadio = page.getByRole('radio', { name: /Organization|Company/i }); | ||
| if (await orgRadio.isVisible().catch(() => false)) { | ||
| await orgRadio.check(); | ||
| } | ||
|
|
||
| const nameField = page | ||
| .getByLabel(/Company name|Organization name|Name/i) | ||
| .or(page.getByPlaceholder(/Company name|Organization name/i)) | ||
| .first(); | ||
|
|
||
| if (await nameField.isVisible()) { | ||
| await nameField.fill(`Test Vendor ${Date.now()}`); | ||
| } | ||
|
|
||
| const emailField = page | ||
| .getByLabel(/Email/i) | ||
| .or(page.getByPlaceholder(/email/i)) | ||
| .first(); | ||
|
|
||
| if (await emailField.isVisible()) { | ||
| await emailField.fill(`vendor${Date.now()}@test.com`); | ||
| } | ||
|
|
||
| const phoneField = page | ||
| .getByLabel(/Phone/i) | ||
| .or(page.getByPlaceholder(/phone/i)) | ||
| .first(); | ||
|
|
||
| if (await phoneField.isVisible()) { | ||
| await phoneField.fill('+1234567890'); | ||
| } | ||
|
|
||
| const addressLine1 = page | ||
| .getByLabel(/Address|Street/i) | ||
| .or(page.getByPlaceholder(/Address|Street/i)) | ||
| .first(); | ||
|
|
||
| if (await addressLine1.isVisible()) { | ||
| await addressLine1.fill('123 Test Street'); | ||
| } | ||
|
|
||
| const cityField = page | ||
| .getByLabel(/City/i) | ||
| .or(page.getByPlaceholder(/City/i)) | ||
| .first(); | ||
|
|
||
| if (await cityField.isVisible()) { | ||
| await cityField.fill('Test City'); | ||
| } | ||
|
|
||
| const postalCodeField = page | ||
| .getByLabel(/Postal code|Zip/i) | ||
| .or(page.getByPlaceholder(/Postal code|Zip/i)) | ||
| .first(); | ||
|
|
||
| if (await postalCodeField.isVisible()) { | ||
| await postalCodeField.fill('12345'); | ||
| } | ||
|
|
||
| const countryField = page | ||
| .getByLabel(/Country/i) | ||
| .or(page.getByRole('combobox', { name: /Country/i })) | ||
| .first(); | ||
|
|
||
| if (await countryField.isVisible()) { | ||
| await countryField.click(); | ||
|
|
||
| const usOption = page.getByRole('option', { name: /United States|USA/i }); | ||
|
|
||
| if (await usOption.isVisible().catch(() => false)) { | ||
| await usOption.click(); | ||
| } else { | ||
| const firstCountry = page.getByRole('option').first(); | ||
|
|
||
| if (await firstCountry.isVisible()) { | ||
| await firstCountry.click(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| console.log('[fillVendorForm] Vendor form filled with test data'); | ||
| } |
There was a problem hiding this comment.
Ensure dialog is ready before filling vendor form.
The fillVendorForm function fills multiple fields conditionally, but doesn't verify that the dialog is fully loaded and interactive before starting. This can cause flakiness if fields are not yet visible or if the dialog is still animating.
Apply this diff to add an explicit dialog ready check:
async function fillVendorForm(page: Page): Promise<void> {
+ // Wait for dialog to be fully ready
+ await page.waitForSelector('[role="dialog"]', { state: 'visible', timeout: 5000 });
+ await page.waitForTimeout(300); // Allow dialog animations to complete
+
const orgRadio = page.getByRole('radio', { name: /Organization|Company/i });
if (await orgRadio.isVisible().catch(() => false)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function fillVendorForm(page: Page): Promise<void> { | |
| const orgRadio = page.getByRole('radio', { name: /Organization|Company/i }); | |
| if (await orgRadio.isVisible().catch(() => false)) { | |
| await orgRadio.check(); | |
| } | |
| const nameField = page | |
| .getByLabel(/Company name|Organization name|Name/i) | |
| .or(page.getByPlaceholder(/Company name|Organization name/i)) | |
| .first(); | |
| if (await nameField.isVisible()) { | |
| await nameField.fill(`Test Vendor ${Date.now()}`); | |
| } | |
| const emailField = page | |
| .getByLabel(/Email/i) | |
| .or(page.getByPlaceholder(/email/i)) | |
| .first(); | |
| if (await emailField.isVisible()) { | |
| await emailField.fill(`vendor${Date.now()}@test.com`); | |
| } | |
| const phoneField = page | |
| .getByLabel(/Phone/i) | |
| .or(page.getByPlaceholder(/phone/i)) | |
| .first(); | |
| if (await phoneField.isVisible()) { | |
| await phoneField.fill('+1234567890'); | |
| } | |
| const addressLine1 = page | |
| .getByLabel(/Address|Street/i) | |
| .or(page.getByPlaceholder(/Address|Street/i)) | |
| .first(); | |
| if (await addressLine1.isVisible()) { | |
| await addressLine1.fill('123 Test Street'); | |
| } | |
| const cityField = page | |
| .getByLabel(/City/i) | |
| .or(page.getByPlaceholder(/City/i)) | |
| .first(); | |
| if (await cityField.isVisible()) { | |
| await cityField.fill('Test City'); | |
| } | |
| const postalCodeField = page | |
| .getByLabel(/Postal code|Zip/i) | |
| .or(page.getByPlaceholder(/Postal code|Zip/i)) | |
| .first(); | |
| if (await postalCodeField.isVisible()) { | |
| await postalCodeField.fill('12345'); | |
| } | |
| const countryField = page | |
| .getByLabel(/Country/i) | |
| .or(page.getByRole('combobox', { name: /Country/i })) | |
| .first(); | |
| if (await countryField.isVisible()) { | |
| await countryField.click(); | |
| const usOption = page.getByRole('option', { name: /United States|USA/i }); | |
| if (await usOption.isVisible().catch(() => false)) { | |
| await usOption.click(); | |
| } else { | |
| const firstCountry = page.getByRole('option').first(); | |
| if (await firstCountry.isVisible()) { | |
| await firstCountry.click(); | |
| } | |
| } | |
| } | |
| console.log('[fillVendorForm] Vendor form filled with test data'); | |
| } | |
| async function fillVendorForm(page: Page): Promise<void> { | |
| // Wait for dialog to be fully ready | |
| await page.waitForSelector('[role="dialog"]', { state: 'visible', timeout: 5000 }); | |
| await page.waitForTimeout(300); // Allow dialog animations to complete | |
| const orgRadio = page.getByRole('radio', { name: /Organization|Company/i }); | |
| if (await orgRadio.isVisible().catch(() => false)) { | |
| await orgRadio.check(); | |
| } | |
| const nameField = page | |
| .getByLabel(/Company name|Organization name|Name/i) | |
| .or(page.getByPlaceholder(/Company name|Organization name/i)) | |
| .first(); | |
| if (await nameField.isVisible()) { | |
| await nameField.fill(`Test Vendor ${Date.now()}`); | |
| } | |
| const emailField = page | |
| .getByLabel(/Email/i) | |
| .or(page.getByPlaceholder(/email/i)) | |
| .first(); | |
| if (await emailField.isVisible()) { | |
| await emailField.fill(`vendor${Date.now()}@test.com`); | |
| } | |
| const phoneField = page | |
| .getByLabel(/Phone/i) | |
| .or(page.getByPlaceholder(/phone/i)) | |
| .first(); | |
| if (await phoneField.isVisible()) { | |
| await phoneField.fill('+1234567890'); | |
| } | |
| const addressLine1 = page | |
| .getByLabel(/Address|Street/i) | |
| .or(page.getByPlaceholder(/Address|Street/i)) | |
| .first(); | |
| if (await addressLine1.isVisible()) { | |
| await addressLine1.fill('123 Test Street'); | |
| } | |
| const cityField = page | |
| .getByLabel(/City/i) | |
| .or(page.getByPlaceholder(/City/i)) | |
| .first(); | |
| if (await cityField.isVisible()) { | |
| await cityField.fill('Test City'); | |
| } | |
| const postalCodeField = page | |
| .getByLabel(/Postal code|Zip/i) | |
| .or(page.getByPlaceholder(/Postal code|Zip/i)) | |
| .first(); | |
| if (await postalCodeField.isVisible()) { | |
| await postalCodeField.fill('12345'); | |
| } | |
| const countryField = page | |
| .getByLabel(/Country/i) | |
| .or(page.getByRole('combobox', { name: /Country/i })) | |
| .first(); | |
| if (await countryField.isVisible()) { | |
| await countryField.click(); | |
| const usOption = page.getByRole('option', { name: /United States|USA/i }); | |
| if (await usOption.isVisible().catch(() => false)) { | |
| await usOption.click(); | |
| } else { | |
| const firstCountry = page.getByRole('option').first(); | |
| if (await firstCountry.isVisible()) { | |
| await firstCountry.click(); | |
| } | |
| } | |
| } | |
| console.log('[fillVendorForm] Vendor form filled with test data'); | |
| } |
🤖 Prompt for AI Agents
In examples/with-nextjs-and-clerk-auth/e2e/utils/vendor-helpers.ts around lines
77 to 159, the fillVendorForm function should wait for the vendor dialog to be
fully loaded and interactive before trying to locate and fill fields; add an
explicit wait at the start of the function to wait for the dialog
(getByRole('dialog') or a unique dialog selector) to be visible/attached and
interactive (use Playwright's waitFor / locator.waitFor with state: 'visible'
and optionally check enabled/visible on a known form field) so subsequent
locator checks don't race with animations or delayed rendering.
| format: 'commonjs', | ||
| sourcemap: true, | ||
| interop: 'compat', | ||
| inlineDynamicImports: true, |
There was a problem hiding this comment.
Remove inlineDynamicImports as it defeats lazy-loading architecture.
Setting inlineDynamicImports: true bundles all dynamic imports inline, which directly contradicts the PR's lazy-loading architecture for tab-based navigation. This configuration:
- Negates code-splitting benefits for consumers
- Defeats the lazy-loaded components mentioned in the PR summary
- Increases bundle size by including all dynamically imported code upfront
- Is generally not recommended for library packages
Based on the past review thread, you indicated this was a debugging artifact that should be removed. Please remove both instances of inlineDynamicImports: true to restore proper code-splitting behavior.
Apply this diff to remove the problematic configuration:
packageJson.main
? ({
file: packageJson.main,
format: 'commonjs',
sourcemap: true,
interop: 'compat',
- inlineDynamicImports: true,
})
: null,
packageJson.module
? ({
file: packageJson.module,
format: 'esm',
sourcemap: true,
- inlineDynamicImports: true,
})
: null,Also applies to: 35-35
🤖 Prompt for AI Agents
In packages/sdk-react/config/rollup.config.mjs around lines 27 and 35, remove
the inlineDynamicImports: true entries because they force bundling of dynamic
imports and break the intended lazy-loading/code-splitting; edit the Rollup
config to delete those properties (both occurrences) so Rollup can emit separate
chunks for dynamic imports and preserve the tab-based lazy-loading architecture,
then run a build to verify chunks are generated as expected.
| const columns = useMemo(() => { | ||
| const baseColumns = config.columns.map((col) => ({ | ||
| field: col.field, | ||
| headerName: col.headerName, | ||
| width: col.width || 120, | ||
| sortable: col.sortable ?? true, | ||
| ...(col.type && { type: col.type as GridColDef['type'] }), | ||
| ...(col.cellClassName && { cellClassName: col.cellClassName }), | ||
| ...(col.headerAlign && { headerAlign: col.headerAlign }), | ||
| ...(col.align && { align: col.align }), | ||
| ...(col.renderCell && { renderCell: col.renderCell }), | ||
| ...(col.valueFormatter && { | ||
| valueFormatter: (_value: unknown, row: T) => | ||
| col.valueFormatter?.(row[col.field as keyof T], row), | ||
| }), | ||
| ...(col.valueGetter && { | ||
| valueGetter: (_value: unknown, row: T) => col.valueGetter?.(row), | ||
| }), |
There was a problem hiding this comment.
Fix DataGrid param bindings for valueFormatter/valueGetter.
DataGrid calls these callbacks with a single params object. Because we rewrap them as (value, row) => …, row is always undefined, so row[col.field …] throws the first time the grid renders. Please bind to the params object instead.
@@
-import {
- DataGrid,
- GridSortDirection,
- GridSortModel,
- GridRenderCellParams,
- GridColDef,
- GridRowParams,
- GridRowSelectionModel,
- GridCallbackDetails,
-} from '@mui/x-data-grid';
+import {
+ DataGrid,
+ GridSortDirection,
+ GridSortModel,
+ GridRenderCellParams,
+ GridColDef,
+ GridRowParams,
+ GridRowSelectionModel,
+ GridCallbackDetails,
+ GridValueFormatterParams,
+ GridValueGetterParams,
+} from '@mui/x-data-grid';
@@
- ...(col.valueFormatter && {
- valueFormatter: (_value: unknown, row: T) =>
- col.valueFormatter?.(row[col.field as keyof T], row),
- }),
- ...(col.valueGetter && {
- valueGetter: (_value: unknown, row: T) => col.valueGetter?.(row),
- }),
+ ...(col.valueFormatter && {
+ valueFormatter: (params: GridValueFormatterParams<T>) =>
+ col.valueFormatter?.(params.value, params.row as T),
+ }),
+ ...(col.valueGetter && {
+ valueGetter: (params: GridValueGetterParams<T>) =>
+ col.valueGetter?.(params.row as T),
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const columns = useMemo(() => { | |
| const baseColumns = config.columns.map((col) => ({ | |
| field: col.field, | |
| headerName: col.headerName, | |
| width: col.width || 120, | |
| sortable: col.sortable ?? true, | |
| ...(col.type && { type: col.type as GridColDef['type'] }), | |
| ...(col.cellClassName && { cellClassName: col.cellClassName }), | |
| ...(col.headerAlign && { headerAlign: col.headerAlign }), | |
| ...(col.align && { align: col.align }), | |
| ...(col.renderCell && { renderCell: col.renderCell }), | |
| ...(col.valueFormatter && { | |
| valueFormatter: (_value: unknown, row: T) => | |
| col.valueFormatter?.(row[col.field as keyof T], row), | |
| }), | |
| ...(col.valueGetter && { | |
| valueGetter: (_value: unknown, row: T) => col.valueGetter?.(row), | |
| }), | |
| // at the top of the file | |
| import { | |
| DataGrid, | |
| GridSortDirection, | |
| GridSortModel, | |
| GridRenderCellParams, | |
| GridColDef, | |
| GridRowParams, | |
| GridRowSelectionModel, | |
| GridCallbackDetails, | |
| GridValueFormatterParams, | |
| GridValueGetterParams, | |
| } from '@mui/x-data-grid'; | |
| … | |
| const columns = useMemo(() => { | |
| const baseColumns = config.columns.map((col) => ({ | |
| field: col.field, | |
| headerName: col.headerName, | |
| width: col.width || 120, | |
| sortable: col.sortable ?? true, | |
| ...(col.type && { type: col.type as GridColDef['type'] }), | |
| ...(col.cellClassName && { cellClassName: col.cellClassName }), | |
| ...(col.headerAlign && { headerAlign: col.headerAlign }), | |
| ...(col.align && { align: col.align }), | |
| ...(col.renderCell && { renderCell: col.renderCell }), | |
| ...(col.valueFormatter && { | |
| valueFormatter: (params: GridValueFormatterParams<T>) => | |
| col.valueFormatter?.(params.value, params.row as T), | |
| }), | |
| ...(col.valueGetter && { | |
| valueGetter: (params: GridValueGetterParams<T>) => | |
| col.valueGetter?.(params.row as T), | |
| }), | |
| // … | |
| })); | |
| // … | |
| }, [config.columns, selectedRows]); |
🤖 Prompt for AI Agents
In packages/sdk-react/src/components/payables/shared/ConfigurableDataTable.tsx
around lines 156 to 173, the wrappers for valueFormatter and valueGetter are
using (value, row) signatures which causes row to be undefined because DataGrid
passes a single params object; change the wrappers to accept the params object
and call the original handlers with params (e.g. for valueFormatter use (params)
=> col.valueFormatter?.(params.value, params.row) or pass the whole params if
the original expects it, and for valueGetter use (params) =>
col.valueGetter?.(params) or col.valueGetter?.(params.row) consistent with the
original function signature), ensuring you update types accordingly so the
DataGrid params object is forwarded instead of separate value/row args.
| const normalizeMajorValue = (val?: number): number => | ||
| typeof val === 'number' && !Number.isNaN(val) ? val : 0; |
There was a problem hiding this comment.
Handle Infinity values in normalization.
The current check typeof val === 'number' && !Number.isNaN(val) allows Infinity and -Infinity to pass through, which can produce "∞" or "-∞" when formatted. For a price field, infinite values are invalid.
Apply this diff to normalize infinite values to 0:
- const normalizeMajorValue = (val?: number): number =>
- typeof val === 'number' && !Number.isNaN(val) ? val : 0;
+ const normalizeMajorValue = (val?: number): number =>
+ typeof val === 'number' && !Number.isNaN(val) && Number.isFinite(val) ? val : 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const normalizeMajorValue = (val?: number): number => | |
| typeof val === 'number' && !Number.isNaN(val) ? val : 0; | |
| const normalizeMajorValue = (val?: number): number => | |
| typeof val === 'number' && !Number.isNaN(val) && Number.isFinite(val) ? val : 0; |
🤖 Prompt for AI Agents
In
packages/sdk-react/src/components/receivables/InvoiceDetails/CreateReceivable/sections/components/Items/PriceField.tsx
around lines 44 to 45, the normalizeMajorValue function currently treats
Infinity and -Infinity as valid numbers; update the validation to exclude
infinite values by using a finite check (e.g., Number.isFinite(val) or global
isFinite) so that non-finite numbers return 0, keeping the function signature
and behavior otherwise unchanged.
| import { t } from '@lingui/macro'; | ||
| import { useLingui } from '@lingui/react'; | ||
| import { TextField } from '@mui/material'; | ||
| import { Controller, Control, FieldValues, Path } from 'react-hook-form'; | ||
|
|
||
| interface ManualItemSelectorProps< | ||
| TFieldValues extends FieldValues = FieldValues, | ||
| > { | ||
| control: Control<TFieldValues>; | ||
| index: number; | ||
| fieldName: Path<TFieldValues>; | ||
| placeholder?: string; | ||
| error?: boolean; | ||
| disabled?: boolean; | ||
| onBlur?: () => void; | ||
| onChange?: (value: string) => void; | ||
| } | ||
|
|
||
| export const ManualItemSelector = < | ||
| TFieldValues extends FieldValues = FieldValues, | ||
| >({ | ||
| control, | ||
| fieldName, | ||
| placeholder, | ||
| error, | ||
| disabled, | ||
| onBlur, | ||
| onChange, | ||
| }: ManualItemSelectorProps<TFieldValues>) => { | ||
| const { i18n } = useLingui(); | ||
|
|
||
| return ( | ||
| <Controller | ||
| name={fieldName} | ||
| control={control} | ||
| render={({ field, fieldState }) => ( | ||
| <TextField | ||
| {...field} | ||
| placeholder={placeholder || t(i18n)`Line item`} | ||
| fullWidth | ||
| size="small" | ||
| error={error || !!fieldState.error} | ||
| helperText={fieldState.error?.message} | ||
| disabled={disabled} | ||
| onChange={(e) => { | ||
| field.onChange(e); | ||
| onChange?.(e.target.value); | ||
| }} | ||
| onBlur={(_e) => { | ||
| field.onBlur(); | ||
| onBlur?.(); | ||
| }} | ||
| className="mtw:min-w-[200px]" | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use shadcn/ui form controls instead of MUI.
New components in packages/sdk-react/**/*.tsx must use the shadcn/ui form primitives plus Tailwind classes, but this still imports MUI’s TextField. Please migrate to the shadcn FormField/Input pattern so the component aligns with the shared form system.
-import { TextField } from '@mui/material';
-import { Controller, Control, FieldValues, Path } from 'react-hook-form';
+import {
+ FormControl,
+ FormField,
+ FormItem,
+ FormMessage,
+} from '@/ui/components/form';
+import { Input } from '@/ui/components/input';
+import { Control, FieldValues, Path } from 'react-hook-form';
…
- <Controller
- name={fieldName}
- control={control}
- render={({ field, fieldState }) => (
- <TextField
- {...field}
- placeholder={placeholder || t(i18n)`Line item`}
- fullWidth
- size="small"
- error={error || !!fieldState.error}
- helperText={fieldState.error?.message}
- disabled={disabled}
- onChange={(e) => {
- field.onChange(e);
- onChange?.(e.target.value);
- }}
- onBlur={(_e) => {
- field.onBlur();
- onBlur?.();
- }}
- className="mtw:min-w-[200px]"
- />
- )}
- />
+ <FormField
+ name={fieldName}
+ control={control}
+ render={({ field, fieldState }) => (
+ <FormItem>
+ <FormControl>
+ <Input
+ {...field}
+ placeholder={placeholder || t(i18n)`Line item`}
+ disabled={disabled}
+ onChange={(event) => {
+ field.onChange(event.target.value);
+ onChange?.(event.target.value);
+ }}
+ onBlur={(event) => {
+ field.onBlur();
+ onBlur?.();
+ }}
+ className="mtw:w-full mtw:min-w-[200px]"
+ />
+ </FormControl>
+ <FormMessage>
+ {error ? fieldState.error?.message ?? t(i18n)`Invalid value` : null}
+ </FormMessage>
+ </FormItem>
+ )}
+ />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { t } from '@lingui/macro'; | |
| import { useLingui } from '@lingui/react'; | |
| import { TextField } from '@mui/material'; | |
| import { Controller, Control, FieldValues, Path } from 'react-hook-form'; | |
| interface ManualItemSelectorProps< | |
| TFieldValues extends FieldValues = FieldValues, | |
| > { | |
| control: Control<TFieldValues>; | |
| index: number; | |
| fieldName: Path<TFieldValues>; | |
| placeholder?: string; | |
| error?: boolean; | |
| disabled?: boolean; | |
| onBlur?: () => void; | |
| onChange?: (value: string) => void; | |
| } | |
| export const ManualItemSelector = < | |
| TFieldValues extends FieldValues = FieldValues, | |
| >({ | |
| control, | |
| fieldName, | |
| placeholder, | |
| error, | |
| disabled, | |
| onBlur, | |
| onChange, | |
| }: ManualItemSelectorProps<TFieldValues>) => { | |
| const { i18n } = useLingui(); | |
| return ( | |
| <Controller | |
| name={fieldName} | |
| control={control} | |
| render={({ field, fieldState }) => ( | |
| <TextField | |
| {...field} | |
| placeholder={placeholder || t(i18n)`Line item`} | |
| fullWidth | |
| size="small" | |
| error={error || !!fieldState.error} | |
| helperText={fieldState.error?.message} | |
| disabled={disabled} | |
| onChange={(e) => { | |
| field.onChange(e); | |
| onChange?.(e.target.value); | |
| }} | |
| onBlur={(_e) => { | |
| field.onBlur(); | |
| onBlur?.(); | |
| }} | |
| className="mtw:min-w-[200px]" | |
| /> | |
| )} | |
| import { t } from '@lingui/macro'; | |
| import { useLingui } from '@lingui/react'; | |
| import { | |
| FormControl, | |
| FormField, | |
| FormItem, | |
| FormMessage, | |
| } from '@/ui/components/form'; | |
| import { Input } from '@/ui/components/input'; | |
| import { Control, FieldValues, Path } from 'react-hook-form'; | |
| interface ManualItemSelectorProps< | |
| TFieldValues extends FieldValues = FieldValues, | |
| > { | |
| control: Control<TFieldValues>; | |
| index: number; | |
| fieldName: Path<TFieldValues>; | |
| placeholder?: string; | |
| error?: boolean; | |
| disabled?: boolean; | |
| onBlur?: () => void; | |
| onChange?: (value: string) => void; | |
| } | |
| export const ManualItemSelector = < | |
| TFieldValues extends FieldValues = FieldValues, | |
| >({ | |
| control, | |
| fieldName, | |
| placeholder, | |
| error, | |
| disabled, | |
| onBlur, | |
| onChange, | |
| }: ManualItemSelectorProps<TFieldValues>) => { | |
| const { i18n } = useLingui(); | |
| return ( | |
| <FormField | |
| name={fieldName} | |
| control={control} | |
| render={({ field, fieldState }) => ( | |
| <FormItem> | |
| <FormControl> | |
| <Input | |
| {...field} | |
| placeholder={placeholder || t(i18n)`Line item`} | |
| disabled={disabled} | |
| onChange={(event) => { | |
| field.onChange(event.target.value); | |
| onChange?.(event.target.value); | |
| }} | |
| onBlur={(event) => { | |
| field.onBlur(); | |
| onBlur?.(); | |
| }} | |
| className="mtw:w-full mtw:min-w-[200px]" | |
| /> | |
| </FormControl> | |
| <FormMessage> | |
| {error ? fieldState.error?.message ?? t(i18n)`Invalid value` : null} | |
| </FormMessage> | |
| </FormItem> | |
| )} | |
| /> | |
| ); | |
| }; |
2 participants
Purchase Orders Management
Complete Purchase Order Lifecycle
Purchase Order Form Features
valid_for_days(API field)Tab-based Navigation
🏗️ Shared Component Architecture
ItemsSection Component
ItemsSectionConfigConfigurableDataTable
FormErrorDisplay
ui/FormErrorDisplayfor cross-feature reuse🎨 UI Components
tab-bar.tsx
🛠️ Technical Improvements
Currency & Price Handling
actualCurrency → product.price.currency → item.currencyuseEffectfor async-loaded settingsDate Handling (Purchase Orders)
setUTCHours(0, 0, 0, 0)differenceInCalendarDaysinstead ofdifferenceInDayscreated_at/issued_at+valid_for_daysuseMemodependencies in Overview componentValidation with Zod
counterpart_id), line items, entity, currencyEmail Functionality
Email Composition Modal
API & Schema Updates
OpenAPI Schema
/payable_purchase_orders/payable_purchase_orders/{id}/send🧪 Testing
E2E Test Coverage
purchase-orders.spec.tspurchase-orders.validation.spec.tsUnit Tests
PurchaseOrders.test.tsxPurchaseOrderForm.test.tsxpurchase-order-currency.test.tsx- Currency conversion and display logiccalculations-normalization.test.ts- Date calculations with UTC normalization and DST handlingJest Configuration
jest-css-transform.jsto handle@importstatements in CSS files🔄 Refactoring
Code Organization
ItemsSection/- Reusable line items componentui/FormErrorDisplay/- Error display componentutils/vatUtils.ts- VAT calculation utilitiesutils/calculations.ts- Purchase order calculations with DST handlingutils/currencies.ts- Currency formatting and conversion🧪 Manual QA Checklist
Summary by CodeRabbit
New Features
Tests
Chores