fix: Demo polish follow-up - positions, parties, audit, ledger#1432
Merged
fix: Demo polish follow-up - positions, parties, audit, ledger#1432
Conversation
- Positions detail: fix BigInt crash by extracting units from google.type.Money object instead of passing object to BigInt() - Parties list: strip PARTY_STATUS_/PARTY_TYPE_ prefixes before passing to StatusBadge for correct color mapping - Audit log page: replace raw fetch with Connect-RPC client, add protobuf Struct-to-object conversion for diff viewer - Account detail: separate not-found vs server-error states - Ledger: load postings in RetrieveFinancialBookingLog backend endpoint (postings were intentionally not loaded in list queries) - Manifests: improve error message when ManifestHistoryService is not deployed
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSeparates account 404 vs generic errors, adds logging for non-NotFound account fetch failures, refactors audit pages/tests to use an API client with protobuf-to-plain mapping (adds UNKNOWN op), adds party formatting helpers, introduces currency-aware minor-unit conversion for positions, tweaks manifest error UI, and enriches booking-log responses with postings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as AuditPage (UI)
participant Client as ApiClient.audit
participant Server as Audit Service
UI->>Client: listAuditEntries(params: filters, pageToken)
Client->>Server: RPC/HTTP request (protobuf params)
Server-->>Client: response (entries with Struct/Value, op codes, nextPageToken)
Client-->>UI: returns entries
UI->>UI: normalize Struct/Value -> plain objects\nmap op codes -> AuditOperation (incl. UNKNOWN)\nrender table, badges, detail, handle retry/filter debounce
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
frontend/src/features/accounts/pages/account-detail.test.tsx (1)
61-72: Consider restoring explicit not-found-path coverage.This now validates the generic error branch only. Add a dedicated test for the
account-not-foundrendering path so both branches stay protected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/accounts/pages/account-detail.test.tsx` around lines 61 - 72, Add a dedicated test that covers the explicit "account-not-found" rendering path: create a new it block (e.g., it('shows account-not-found state for missing account', ...)) that uses server.use with http.post('*/meridian.current_account.v1.CurrentAccountService/RetrieveCurrentAccount', () => HttpResponse.json({ code: 'account-not-found' }, { status: 404 })) (or whatever error shape the app inspects), call renderDetailPage('nonexistent-id'), and assert inside waitFor that screen.getByTestId('account-not-found') is in the document; use the same helpers (server, http, HttpResponse, renderDetailPage, screen, waitFor) so both the generic error test and this specific not-found test are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/features/audit/pages/index.tsx`:
- Around line 41-45: The toOperationName function currently coerces unknown
values to 'UPDATE'; change it to return an explicit 'UNKNOWN' AuditOperation
instead: update the AuditOperation enum/type to include 'UNKNOWN' (and add it to
VALID_OPERATIONS if appropriate), change the numeric fallback in toOperationName
to use OPERATION_NAMES[op] ?? 'UNKNOWN', and ensure any UI styling or mapping
that depends on operation names treats 'UNKNOWN' with neutral styling; update
any references to OPERATION_NAMES or VALID_OPERATIONS to account for the new
'UNKNOWN' value.
- Around line 261-266: The RPC call to clients.audit.listAuditEntries is missing
the operation and changedBy filters from params; update the
ListAuditEntriesRequest construction in the component (where tableName and
recordId are set) to also pass params.filters?.operation and
params.filters?.changedBy (mapping to the request's operation and
changed_by/changedBy fields as expected by the client) so the backend receives
those filters when calling clients.audit.listAuditEntries.
- Around line 51-60: The current valueToPlain does not unwrap protoc-gen-es
oneof wrappers, so update the valueToPlain function to first detect the oneof
pattern (i.e., v.kind as an object with 'case' and 'value') and extract
fieldName and fieldValue, handling
stringValue/numberValue/boolValue/nullValue/structValue/listValue (use
structToObject for structValue and map valueToPlain over listValue.values), then
fall back to the existing flat-key checks
('stringValue'/'numberValue'/'boolValue'/'nullValue'/'structValue'/'listValue')
for fixtures; ensure valueToPlain returns primitives/null/objects/arrays
unwrapped in both cases.
In `@frontend/src/features/manifests/pages/manifest-current-view.tsx`:
- Around line 31-33: The component render that currently shows "Manifest history
is not available" and "The ManifestHistoryService is not yet deployed for this
environment." (inside ManifestCurrentView / the JSX branch that handles query
failures) is over-assertive; change it to a neutral, non-causal message and
surface the actual error when present. Update the error branch that handles the
failed manifest history query (the logic that routes all failures to this UI
state) to: 1) display a generic message like "Unable to load manifest history"
instead of asserting the service is not deployed, 2) show the underlying
error.message/details when the query error object is available, and 3) only show
a specific "service not deployed" suggestion if you can detect that precise
condition from the error payload. Use the existing symbols
ManifestHistoryService and the component/handler that maps query errors to this
JSX branch so you locate and update the rendering and error-handling logic.
In `@frontend/src/features/positions/pages/detail.tsx`:
- Around line 44-47: Wrap the BigInt conversions that assign amt from external
data in try-catch blocks to avoid runtime throws: surround the string-to-BigInt
and object.units-to-BigInt conversions (the branches checking typeof money ===
'string' and typeof money === 'object' && 'units' in money) with try/catch, and
on error set a safe fallback (e.g., amt = 0n or undefined), optionally log the
invalid payload (use existing logger or console.warn) and continue without
throwing; ensure you still handle both the money string and money.units cases so
malformed or decimal inputs do not crash the component.
---
Nitpick comments:
In `@frontend/src/features/accounts/pages/account-detail.test.tsx`:
- Around line 61-72: Add a dedicated test that covers the explicit
"account-not-found" rendering path: create a new it block (e.g., it('shows
account-not-found state for missing account', ...)) that uses server.use with
http.post('*/meridian.current_account.v1.CurrentAccountService/RetrieveCurrentAccount',
() => HttpResponse.json({ code: 'account-not-found' }, { status: 404 })) (or
whatever error shape the app inspects), call renderDetailPage('nonexistent-id'),
and assert inside waitFor that screen.getByTestId('account-not-found') is in the
document; use the same helpers (server, http, HttpResponse, renderDetailPage,
screen, waitFor) so both the generic error test and this specific not-found test
are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db16453f-5c0c-45d7-bf2c-14ac6fed78ec
📒 Files selected for processing (9)
frontend/src/features/accounts/hooks/use-accounts.tsfrontend/src/features/accounts/pages/[accountId].tsxfrontend/src/features/accounts/pages/account-detail.test.tsxfrontend/src/features/audit/pages/index.test.tsxfrontend/src/features/audit/pages/index.tsxfrontend/src/features/manifests/pages/manifest-current-view.tsxfrontend/src/features/parties/pages/index.tsxfrontend/src/features/positions/pages/detail.tsxservices/financial-accounting/service/grpc_booking_endpoints.go
Claude Code ReviewCommit: SummarySolid demo polish PR. The migration from raw Two non-blocking items: (1) the audit test file lost significant coverage that should be restored in a follow-up, and (2) the Go backend change has no unit test for the new posting-loading path. Risk Assessment
Findings
Bot Review NotesAll CodeRabbit threads are resolved. Key concerns that were raised and addressed across commits:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ion, account not-found UX - Audit: default unknown operations to UNKNOWN instead of UPDATE - Audit: wire operation and changedBy filters to RPC call - Audit: simplify struct helper since connect-es deserializes Struct to JsonObject - Positions: convert google.type.Money units+nanos to minor units for MoneyDisplay - Positions: add try-catch around BigInt conversions for robustness - Manifest: use generic error message instead of asserting service not deployed - Account detail: add internal account link suggestion on not-found page
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/positions/pages/detail.tsx (1)
143-152:⚠️ Potential issue | 🟠 MajorApply the same money conversion logic from BalanceView to MeasurementHistory to prevent crash with google.type.Money objects.
BalanceViewcorrectly convertsentry.amount.amountfromgoogle.type.Moneyobjects (withunitsandnanosfields) to minor units as bigint (lines 42-66). However,MeasurementHistorypassesentry.amount.amountdirectly toMoneyDisplaywithout conversion. Since both components receive the samelog.transactionLogEntriesarray, if entries contain google.type.Money objects, the history table will crash with "Cannot convert [object Object] to a BigInt" whenformatMoneyattempts the conversion.Extract the conversion logic into a shared helper function and reuse it in both places.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/positions/pages/detail.tsx` around lines 143 - 152, BalanceView converts google.type.Money objects (units+nanos) into minor-unit bigint before rendering, but MeasurementHistory passes entry.amount.amount raw into MoneyDisplay causing BigInt conversion crashes; extract that conversion logic from BalanceView into a shared helper (e.g. toMinorUnits or parseGoogleMoney) and use it in both BalanceView and MeasurementHistory when building values from log.transactionLogEntries so MoneyDisplay always receives a bigint amount compatible with formatMoney; update places that reference entry.amount.amount (in MeasurementHistory and BalanceView) to call the new helper and keep currency/direction handling the same.
♻️ Duplicate comments (1)
frontend/src/features/manifests/pages/manifest-current-view.tsx (1)
31-33:⚠️ Potential issue | 🟡 MinorStill masking real failure causes in the error state.
Line 33 still hints at a specific cause for all failures, and Line 85 still drops the actual query error details. This can mislead users on auth/network/5xx issues. Prefer neutral copy and show the concrete error message when available.
Suggested fix
-function ErrorState({ onRetry }: { onRetry: () => void }) { +function ErrorState({ onRetry, error }: { onRetry: () => void; error?: unknown }) { + const errorMessage = error instanceof Error ? error.message : undefined return ( <div className="flex flex-col items-center gap-3 py-8 text-muted-foreground"> - <span className="text-sm font-medium">Failed to load manifest</span> - <span className="text-xs">The manifest history service may not be available for this environment.</span> + <span className="text-sm font-medium">Unable to load manifest history</span> + <span className="text-xs">Please retry. If the issue persists, contact your administrator.</span> + {errorMessage ? <span className="text-xs break-all">{errorMessage}</span> : null} <Button variant="outline" size="sm" onClick={onRetry}> Retry </Button> </div> ) } @@ - if (error) return <ErrorState onRetry={() => void refetch()} /> + if (error) return <ErrorState onRetry={() => void refetch()} error={error} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/manifests/pages/manifest-current-view.tsx` around lines 31 - 33, Change the hardcoded, environment-specific error copy in the component that renders "Failed to load manifest" to neutral language (e.g., "Unable to load manifest") and, in the error rendering branch that currently swallows the query error, display the concrete error message when present by reading the query error object (e.g., error.message or the query's error property) and rendering it below the neutral copy; ensure you update the JSX in manifest-current-view.tsx's error state and use a conditional to avoid showing undefined messages.
🧹 Nitpick comments (3)
frontend/src/features/accounts/pages/[accountId].tsx (1)
362-378: Appropriate separation of not-found vs error states.The logic correctly distinguishes between a 404 (
account === null) and server/network errors (isError || account === undefined). The defensiveaccount === undefinedcheck handles edge cases where react-query might be in an unexpected state.Consider adding a retry button for transient failures:
♻️ Optional: Add retry functionality
- const { data: account, isLoading, isError } = useAccountDetail(accountId) + const { data: account, isLoading, isError, refetch } = useAccountDetail(accountId)Then in the error UI:
<p className="mt-2 text-sm text-muted-foreground"> There was a problem loading this account. Please try again. </p> + <Button variant="outline" className="mt-4" onClick={() => refetch()}> + Retry + </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/accounts/pages/`[accountId].tsx around lines 362 - 378, Add a retry control to the transient error UI by using the refetch function from the account query: inside the error return block (the branch guarded by isError || account === undefined) render a Retry button that calls refetch when clicked and shows a loading/disabled state when the query is fetching (using the query's isFetching or isLoading flag); update the JSX near Breadcrumbs and the "Failed to load account" message to include this button and ensure AccountNotFound handling (account === null) remains unchanged.frontend/src/features/audit/pages/index.test.tsx (2)
211-222: Strengthen the error-path test with retry behavior.After asserting the Retry button is shown, click it and assert
mockListAuditEntriesis called again. This validates recovery behavior, not just error rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/audit/pages/index.test.tsx` around lines 211 - 222, The test currently only asserts the Retry button appears when mockListAuditEntries rejects; extend it to assert retry behavior by simulating a user click on the Retry button and verifying mockListAuditEntries is invoked again. After obtaining retryButton (via screen.findByRole), call the appropriate click helper (e.g., userEvent.click or fireEvent.click) and add an assertion such as expect(mockListAuditEntries).toHaveBeenCalledTimes(<expected>) or toHaveBeenCalled() to confirm a new API call; ensure userEvent is imported if used and reference mockListAuditEntries and AuditLogPage to locate the test.
111-163: Add a regression test for filter forwarding.Current tests validate rendered output, but they don’t lock that
operationandchangedByare actually sent tolistAuditEntriesafter filter interaction. A focused assertion on the mock call payload would guard this PR fix from regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/audit/pages/index.test.tsx` around lines 111 - 163, Add a regression test that simulates the user interacting with the filters and then asserts the mocked API was called with the filter values: render <AuditLogPage /> inside Wrapper, set the operation filter and changedBy input via the same UI interactions used elsewhere in tests, trigger the search/apply action, and then assert mockListAuditEntries (or listAuditEntries mock) was called with an argument containing operation and changedBy (e.g., expect(mockListAuditEntries).toHaveBeenCalledWith(expect.objectContaining({ operation: 'UPDATE', changedBy: 'user@example.com' }))). Ensure you reference the existing mockListAuditEntries mock and the AuditLogPage component when adding the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/features/positions/pages/detail.tsx`:
- Around line 143-152: BalanceView converts google.type.Money objects
(units+nanos) into minor-unit bigint before rendering, but MeasurementHistory
passes entry.amount.amount raw into MoneyDisplay causing BigInt conversion
crashes; extract that conversion logic from BalanceView into a shared helper
(e.g. toMinorUnits or parseGoogleMoney) and use it in both BalanceView and
MeasurementHistory when building values from log.transactionLogEntries so
MoneyDisplay always receives a bigint amount compatible with formatMoney; update
places that reference entry.amount.amount (in MeasurementHistory and
BalanceView) to call the new helper and keep currency/direction handling the
same.
---
Duplicate comments:
In `@frontend/src/features/manifests/pages/manifest-current-view.tsx`:
- Around line 31-33: Change the hardcoded, environment-specific error copy in
the component that renders "Failed to load manifest" to neutral language (e.g.,
"Unable to load manifest") and, in the error rendering branch that currently
swallows the query error, display the concrete error message when present by
reading the query error object (e.g., error.message or the query's error
property) and rendering it below the neutral copy; ensure you update the JSX in
manifest-current-view.tsx's error state and use a conditional to avoid showing
undefined messages.
---
Nitpick comments:
In `@frontend/src/features/accounts/pages/`[accountId].tsx:
- Around line 362-378: Add a retry control to the transient error UI by using
the refetch function from the account query: inside the error return block (the
branch guarded by isError || account === undefined) render a Retry button that
calls refetch when clicked and shows a loading/disabled state when the query is
fetching (using the query's isFetching or isLoading flag); update the JSX near
Breadcrumbs and the "Failed to load account" message to include this button and
ensure AccountNotFound handling (account === null) remains unchanged.
In `@frontend/src/features/audit/pages/index.test.tsx`:
- Around line 211-222: The test currently only asserts the Retry button appears
when mockListAuditEntries rejects; extend it to assert retry behavior by
simulating a user click on the Retry button and verifying mockListAuditEntries
is invoked again. After obtaining retryButton (via screen.findByRole), call the
appropriate click helper (e.g., userEvent.click or fireEvent.click) and add an
assertion such as expect(mockListAuditEntries).toHaveBeenCalledTimes(<expected>)
or toHaveBeenCalled() to confirm a new API call; ensure userEvent is imported if
used and reference mockListAuditEntries and AuditLogPage to locate the test.
- Around line 111-163: Add a regression test that simulates the user interacting
with the filters and then asserts the mocked API was called with the filter
values: render <AuditLogPage /> inside Wrapper, set the operation filter and
changedBy input via the same UI interactions used elsewhere in tests, trigger
the search/apply action, and then assert mockListAuditEntries (or
listAuditEntries mock) was called with an argument containing operation and
changedBy (e.g.,
expect(mockListAuditEntries).toHaveBeenCalledWith(expect.objectContaining({
operation: 'UPDATE', changedBy: 'user@example.com' }))). Ensure you reference
the existing mockListAuditEntries mock and the AuditLogPage component when
adding the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9455c4f0-195b-4381-81b7-dde7ae4d0bee
📒 Files selected for processing (5)
frontend/src/features/accounts/pages/[accountId].tsxfrontend/src/features/audit/pages/index.test.tsxfrontend/src/features/audit/pages/index.tsxfrontend/src/features/manifests/pages/manifest-current-view.tsxfrontend/src/features/positions/pages/detail.tsx
…verage - Extract toMinorUnits helper for google.type.Money → minor unit conversion - Use toMinorUnits in both BalanceView and MeasurementHistory (fixes crash) - Manifest error state: show actual error message, use neutral copy - Account detail error state: add Retry button with loading state - Audit tests: verify retry click calls API again, add filter wiring test
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
frontend/src/features/manifests/pages/manifest-current-view.tsx (1)
31-35: Consider announcing this error state to assistive technologies.Adding
role="alert"(oraria-live="polite") would improve error discoverability for screen-reader users.♿ Suggested accessibility tweak
- <div className="flex flex-col items-center gap-3 py-8 text-muted-foreground"> + <div role="alert" aria-live="polite" className="flex flex-col items-center gap-3 py-8 text-muted-foreground">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/manifests/pages/manifest-current-view.tsx` around lines 31 - 35, The error message block in manifest-current-view.tsx isn't announced to assistive tech; add an accessibility attribute (e.g., role="alert" or aria-live="polite") to the error container element (the <div className="flex flex-col items-center gap-3 py-8 text-muted-foreground"> or the error <span> that renders error.message) so screen readers will announce the "Unable to load manifest" state and the error.message text; ensure the attribute is only present when error exists to avoid noisy announcements.frontend/src/features/audit/pages/index.test.tsx (4)
29-36: Minor: QueryClient instance created per render.A new
QueryClientis created inside theWrapperfunction body, which means each render creates a fresh instance. While this provides test isolation (which is good), it could cause issues if a test triggers re-renders, as the query cache would be lost.Consider extracting QueryClient creation to avoid this edge case:
💡 Alternative pattern if re-render issues arise
function Wrapper({ children }: { children: React.ReactNode }) { - const qc = makeQueryClient() + const [qc] = React.useState(() => makeQueryClient()) return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/audit/pages/index.test.tsx` around lines 29 - 36, The Wrapper component currently creates a new QueryClient on every render (makeQueryClient() inside Wrapper), which can drop the cache on re-renders; move QueryClient creation out of the render path—either create a single instance once and reuse it for tests or memoize it (e.g., using useRef or useMemo) so Wrapper still returns <QueryClientProvider client={qc}> and <TooltipProvider>{children}</TooltipProvider> but qc is stable across renders; update references in the test to use the stable qc instead of calling makeQueryClient() on each render.
210-230: Error handling test could verify error state display.The test verifies the retry mechanism works (call count increases), but doesn't explicitly assert that an error message is displayed to the user before clicking retry. Adding an assertion for the error state would make the test more comprehensive.
💡 Optional: Add error state assertion
const retryButton = await screen.findByRole('button', { name: /Retry/i }) expect(retryButton).toBeInTheDocument() + // Optionally verify error message is visible + expect(screen.getByText(/error|failed/i)).toBeInTheDocument() // Clear and set up success response for retry🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/audit/pages/index.test.tsx` around lines 210 - 230, The test currently mocks mockListAuditEntries to reject and only asserts the retry button call count; add an assertion that the error state is rendered before retrying by checking for the error UI produced by AuditLogPage—e.g., await screen.findByRole('alert') or await screen.findByText(/network error|failed to load/i) to confirm an error message is visible (then continue to clear and mockResolvedValue and click retry to assert mockListAuditEntries was called twice). Use the existing symbols mockListAuditEntries, AuditLogPage, retryButton, and screen.findByRole/findByText to locate the relevant checks.
180-181: Non-null assertions on DOM queries could cause confusing test failures.Using
closest('tr')!without a guard could produce a cryptic runtime error if the DOM structure changes. Consider adding an explicit assertion before the click.💡 Suggestion for clearer test failure messages
- const row = screen.getAllByText('acc-123')[0].closest('tr')! - await user.click(row) + const row = screen.getAllByText('acc-123')[0].closest('tr') + expect(row).not.toBeNull() + await user.click(row!)Also applies to: 200-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/audit/pages/index.test.tsx` around lines 180 - 181, The test uses a non-null assertion on the DOM query (const row = screen.getAllByText('acc-123')[0].closest('tr')!) which can yield cryptic failures; change this to explicitly assert the element exists before interacting with it (e.g., get the element via screen.getAllByText('acc-123')[0].closest('tr'), add an assertion like expect(row).toBeTruthy() or guard with if (!row) throw new Error(...), then await user.click(row)); apply the same explicit existence assertion for the other occurrence around the 'acc-123' click at the later lines (the second closest('tr') use).
127-148: Consider test resilience for badge styling assertions.Testing specific CSS classes like
bg-green-100,bg-blue-100,bg-red-100creates coupling to implementation details. If the design system or styling approach changes, these tests will break even if the visual behavior is correct.This is acceptable for now as a regression guard, but consider using visual regression testing or accessible attributes if styling tests become maintenance-heavy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/audit/pages/index.test.tsx` around lines 127 - 148, The test for badge styling is brittle because it asserts on specific CSS classes; update the test in the AuditLogPage spec to assert on stable, semantic attributes instead: have the test use mockListAuditEntries and render AuditLogPage as before, then locate the badges by their visible text (e.g., "INSERT", "UPDATE", "DELETE") and assert on an accessible attribute or stable identifier (aria-label, role, or data-testid) or on a semantic behavior (presence/display) rather than class names; if the component (AuditLogPage or the badge subcomponent) doesn't expose such attributes yet, add a stable attribute (e.g., data-testid or aria-label like "operation-badge-INSERT") to the badge component and update the test to assert that those attributes exist and the correct text is shown.frontend/src/features/positions/pages/detail.tsx (1)
14-17:CURRENCY_PRECISIONscope is narrower than the ISO 4217 comment implies.Line 14 says “ISO 4217”, but the map currently covers only a subset. Any other non-2-decimal currency will silently fall back to
2and be mis-scaled. Please either make this map exhaustive for supported currencies or tighten the comment to reflect the actual supported subset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/positions/pages/detail.tsx` around lines 14 - 17, The CURRENCY_PRECISION map is labeled "ISO 4217" but only contains a small subset, risking silent fallback to 2 decimals; update the CURRENCY_PRECISION constant so it accurately reflects intent: either populate it exhaustively with all supported non-2-decimal ISO 4217 currencies used by the app (ensuring entries for currencies like CLP, MGA, etc., as needed) or change the comment to state this is a curated subset of non-2-decimal currencies supported by the frontend and document the fallback behavior; locate the CURRENCY_PRECISION declaration in detail.tsx and modify the comment and/or entries accordingly so the scope is unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/features/positions/pages/detail.tsx`:
- Around line 67-68: The conversion uses a shared variable "currency" for all
entries causing wrong precision; update the call to toMinorUnits to use each
entry's currency (e.g., use entry.amount?.currency or entry.currency) when
converting entry.amount?.amount, falling back to the existing "currency" only if
the entry's currency is missing, and keep filtering out null results (amt ===
null) as before so summation uses correctly computed minor units per-entry.
---
Nitpick comments:
In `@frontend/src/features/audit/pages/index.test.tsx`:
- Around line 29-36: The Wrapper component currently creates a new QueryClient
on every render (makeQueryClient() inside Wrapper), which can drop the cache on
re-renders; move QueryClient creation out of the render path—either create a
single instance once and reuse it for tests or memoize it (e.g., using useRef or
useMemo) so Wrapper still returns <QueryClientProvider client={qc}> and
<TooltipProvider>{children}</TooltipProvider> but qc is stable across renders;
update references in the test to use the stable qc instead of calling
makeQueryClient() on each render.
- Around line 210-230: The test currently mocks mockListAuditEntries to reject
and only asserts the retry button call count; add an assertion that the error
state is rendered before retrying by checking for the error UI produced by
AuditLogPage—e.g., await screen.findByRole('alert') or await
screen.findByText(/network error|failed to load/i) to confirm an error message
is visible (then continue to clear and mockResolvedValue and click retry to
assert mockListAuditEntries was called twice). Use the existing symbols
mockListAuditEntries, AuditLogPage, retryButton, and
screen.findByRole/findByText to locate the relevant checks.
- Around line 180-181: The test uses a non-null assertion on the DOM query
(const row = screen.getAllByText('acc-123')[0].closest('tr')!) which can yield
cryptic failures; change this to explicitly assert the element exists before
interacting with it (e.g., get the element via
screen.getAllByText('acc-123')[0].closest('tr'), add an assertion like
expect(row).toBeTruthy() or guard with if (!row) throw new Error(...), then
await user.click(row)); apply the same explicit existence assertion for the
other occurrence around the 'acc-123' click at the later lines (the second
closest('tr') use).
- Around line 127-148: The test for badge styling is brittle because it asserts
on specific CSS classes; update the test in the AuditLogPage spec to assert on
stable, semantic attributes instead: have the test use mockListAuditEntries and
render AuditLogPage as before, then locate the badges by their visible text
(e.g., "INSERT", "UPDATE", "DELETE") and assert on an accessible attribute or
stable identifier (aria-label, role, or data-testid) or on a semantic behavior
(presence/display) rather than class names; if the component (AuditLogPage or
the badge subcomponent) doesn't expose such attributes yet, add a stable
attribute (e.g., data-testid or aria-label like "operation-badge-INSERT") to the
badge component and update the test to assert that those attributes exist and
the correct text is shown.
In `@frontend/src/features/manifests/pages/manifest-current-view.tsx`:
- Around line 31-35: The error message block in manifest-current-view.tsx isn't
announced to assistive tech; add an accessibility attribute (e.g., role="alert"
or aria-live="polite") to the error container element (the <div className="flex
flex-col items-center gap-3 py-8 text-muted-foreground"> or the error <span>
that renders error.message) so screen readers will announce the "Unable to load
manifest" state and the error.message text; ensure the attribute is only present
when error exists to avoid noisy announcements.
In `@frontend/src/features/positions/pages/detail.tsx`:
- Around line 14-17: The CURRENCY_PRECISION map is labeled "ISO 4217" but only
contains a small subset, risking silent fallback to 2 decimals; update the
CURRENCY_PRECISION constant so it accurately reflects intent: either populate it
exhaustively with all supported non-2-decimal ISO 4217 currencies used by the
app (ensuring entries for currencies like CLP, MGA, etc., as needed) or change
the comment to state this is a curated subset of non-2-decimal currencies
supported by the frontend and document the fallback behavior; locate the
CURRENCY_PRECISION declaration in detail.tsx and modify the comment and/or
entries accordingly so the scope is unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b78bcaa-1870-4961-b514-b17d44f3971c
📒 Files selected for processing (4)
frontend/src/features/accounts/pages/[accountId].tsxfrontend/src/features/audit/pages/index.test.tsxfrontend/src/features/manifests/pages/manifest-current-view.tsxfrontend/src/features/positions/pages/detail.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/features/positions/pages/detail.tsx`:
- Around line 149-150: toMinorUnits can return null but the component currently
passes its result straight into MoneyDisplay; modify the render in the detail
page to first call toMinorUnits(entry.amount.amount, entry.amount.currency),
store the result in a local variable, and if that variable is null render '—'
instead of MoneyDisplay (matching BalanceView behavior), otherwise pass the
non-null value and entry.amount.currency into MoneyDisplay. Ensure you update
the code paths around MoneyDisplay and the entry.amount references so the
null-guarded value is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c303cef-243f-458c-b5cc-4fb0490ff495
📒 Files selected for processing (1)
frontend/src/features/positions/pages/detail.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/features/positions/pages/detail.tsx`:
- Around line 35-36: In toMinorUnits, remove the unnecessary Number.isInteger
guard and replace the Number-based Math.round logic with BigInt "round half away
from zero" arithmetic: compute nanosBig = BigInt(nanos), divisor = BigInt(10 **
(9 - precision)), then q = nanosBig / divisor and r = nanosBig % divisor; if r
!= 0 and (abs(r) * 2 >= divisor) then adjust q by sign (q += 1 if nanosBig > 0,
else q -= 1); finally return units * factor + q; this ensures correct asymmetric
rounding for negative nanos.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1beecee-88bb-42d2-ad5c-e6467fc11872
📒 Files selected for processing (1)
frontend/src/features/positions/pages/detail.tsx
Replace Math.round with BigInt half-away-from-zero rounding to handle negative nanos correctly per google.type.Money spec.
Accept our prop-based AccountNotFound with EntityLink for internal account cross-linking over develop's useParams+Link approach.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #1430. Fixes remaining issues found during demo walkthrough on demo.meridianhub.cloud.
unitsfromgoogle.type.MoneyobjectPARTY_STATUS_/PARTY_TYPE_prefixes for correct StatusBadge color mapping (was showing raw enum text)fetch()with Connect-RPCclients.audit.listAuditEntries(), add protobuf Struct-to-object conversionRetrieveFinancialBookingLogviaGetPostingsByBookingLogID- postings were intentionally left empty in list queries to avoid N+1Known issues (out of scope)
hydrateAccountWithBalancemay fail when Position Keeping returns errorsTest plan