feat(frontend): create data hooks for all feature modules#1423
Merged
feat(frontend): create data hooks for all feature modules#1423
Conversation
Add reusable query hooks following the established pattern from features/accounts/hooks/use-accounts.ts: - payments: usePaymentsTable, usePaymentDetail - ledger: useBookingLogsTable, useBookingLogDetail, useLedgerPostings - positions: usePositionLogsTable, usePositionLogDetail - parties: usePartiesTable, usePartyDetail, usePartyAssociations - reconciliation: useReconciliationRunsTable, useReconciliationRunDetail - sagas: useSagasTable, useSagaDetail, useActiveSaga - market-data: useDatasetsTable, useDatasetDetail, useDatasetObservations - internal-accounts: useInternalAccountsTable, useInternalAccountDetail Each hook uses tenant-scoped query keys, Connect-RPC clients via useApiClients, and useTenantSlug for tenant scoping.
Replace inline useQuery calls and raw fetch patterns in page components with the reusable hooks created in the previous commit. This removes duplicated data-fetching logic from page files and centralizes it in the hooks layer. Pages updated: - payments: list + detail - ledger: list + booking log detail - positions: list + detail - parties: list + detail + associations tab - reconciliation: list - sagas: list + detail - market-data: list + dataset detail - internal-accounts: list + detail
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:
📝 WalkthroughWalkthroughIntroduce tenant-scoped React Query hooks across multiple features, add barrel re-exports, refactor pages/components to consume the new hooks instead of inline API/tenant logic, and update tests to mock the new hooks and tenant context. Changes
Sequence Diagram(s)mermaid Page->>Hook: call (e.g. usePaymentsTable / useXDetail) Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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 |
Claude Code ReviewCommit: SummaryClean, well-executed extraction of inline Notable improvements beyond extraction:
This satisfies the requirement for task Risk Assessment
FindingsNo blocking issues found. The extraction is faithful to the original logic with the improvements noted above.
Bot Review NotesAll bot review threads are now resolved. My assessment of the previously flagged concerns:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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: 9
🧹 Nitpick comments (5)
frontend/src/features/internal-accounts/pages/index.tsx (1)
11-19: Consider exportingInternalAccountRowfrom the hooks module to avoid duplication.The
InternalAccountRowinterface is defined identically in bothuse-internal-accounts.ts(lines 8-16) and this page component. If the shape changes in the hook, this local definition could become stale.♻️ Suggested refactor
In
use-internal-accounts.ts, export the interface:-interface InternalAccountRow { +export interface InternalAccountRow {Then in
index.tsx:-import { useInternalAccountsTable } from '../hooks' +import { useInternalAccountsTable, type InternalAccountRow } from '../hooks' import { CreateInternalAccountDialog } from './create-internal-account-dialog' -interface InternalAccountRow { - accountId: string - accountCode: string - name: string - behaviorClass: string - accountStatus: number - instrumentCode: string - createdAt?: { seconds: bigint | number; nanos?: number } | null -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/internal-accounts/pages/index.tsx` around lines 11 - 19, The page defines an InternalAccountRow interface duplicate of the one in use-internal-accounts.ts; export the existing InternalAccountRow from use-internal-accounts.ts (add an export on the interface) and remove the local definition in frontend/src/features/internal-accounts/pages/index.tsx, then import { InternalAccountRow } from the hooks module and use that type for the page component to keep a single source of truth.frontend/src/features/internal-accounts/pages/[accountId].tsx (1)
178-190: Consider extractingInternalAccountTransactionsquery to a hook for consistency.The main account detail fetch now uses
useInternalAccountDetail, but the transactions query withinInternalAccountTransactionsstill uses inlineuseQuery. This is a minor inconsistency with the hook-based pattern, but acceptable if intentionally scoped out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/internal-accounts/pages/`[accountId].tsx around lines 178 - 190, Extract the inline useQuery in InternalAccountTransactions into a reusable hook (e.g. useInternalAccountTransactions) that accepts tenantSlug, accountId, and optional instrumentCode, calls clients.financialAccounting.listLedgerPostings with the same pagination and queryKey logic, and returns { data, isLoading, isError } so InternalAccountTransactions simply calls useInternalAccountTransactions(accountId, instrumentCode) and renders from the hook result; ensure the hook uses useApiClients and useTenantContext (or accepts tenantSlug) and preserves the enabled: !!accountId behavior and queryKey structure (tenantKeys.internalAccount(...), 'postings').frontend/src/features/parties/hooks/use-parties.ts (1)
38-64: MissingtenantSluginuseCallbackdependencies.The
queryFncallback depends onclients.partybut not ontenantSlug. WhiletenantSlugisn't directly used insidequeryFn, thequeryKeyreturned alongside it does depend ontenantSlug. IftenantSlugchanges, consumers using the stalequeryFnwith a newqueryKeycould lead to cache inconsistencies.This is likely fine since DataTable presumably re-renders when
tenantSlugchanges, but worth verifying the pattern is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/parties/hooks/use-parties.ts` around lines 38 - 64, The queryFn passed to useCallback should include tenantSlug in its dependency array to avoid a stale callback when tenantSlug changes; update the useCallback dependency list for queryFn to include tenantSlug (in addition to clients.party), since queryKey (from tenantKeys.parties(tenantSlug)) is returned alongside queryFn and must stay consistent with tenantSlug changes.frontend/src/features/reconciliation/hooks/use-reconciliation.ts (1)
6-7: Decouple hook types from page modulesLine 6 and Line 7 import data contracts from
../pages/*, which creates an inverted dependency (hooks depending on pages). Move these interfaces to a feature-leveltypes.ts(or API model module) and import from there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/reconciliation/hooks/use-reconciliation.ts` around lines 6 - 7, The hook currently imports ReconciliationRun and ReconciliationRunDetail from page modules, creating an inverted dependency; extract those interfaces into a feature-level types file (e.g., create a types.ts next to use-reconciliation.ts) and move the ReconciliationRun and ReconciliationRunDetail definitions there, then update use-reconciliation.ts to import the types from that new feature module instead of ../pages/*; ensure any other files that referenced the old page-local types are updated to import from the new types module to keep contracts consistent.frontend/src/features/positions/hooks/use-positions.ts (1)
7-7: Move shared types out of page modules.
hooks/use-positions.tsimportingFinancialPositionLogfrompages/indexcouples the data layer to the UI layer and makes reuse brittle. Please move these interfaces to a dedicated feature types module (or generated API types) and import from there.♻️ Suggested refactor
- import type { FinancialPositionLog } from '../pages/index' + import type { FinancialPositionLog } from '../types'- // frontend/src/features/positions/pages/index.tsx - export interface FinancialPositionLog { ... } + // frontend/src/features/positions/types.ts + export interface FinancialPositionLog { ... } + // frontend/src/features/positions/pages/index.tsx + import type { FinancialPositionLog } from '../types'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/positions/hooks/use-positions.ts` at line 7, The hook use-positions.ts currently imports the UI page type FinancialPositionLog from pages/index which couples data to presentation; create a new feature type module (e.g., frontend/src/features/positions/types.ts or move into your generated API types) that exports FinancialPositionLog (and any related interfaces), update use-positions.ts to import FinancialPositionLog from that new types module, and update pages/index to import the type from the same new module so both hook (usePositions) and page consume the shared type instead of pages/index.
🤖 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/ledger/hooks/use-ledger.ts`:
- Around line 63-70: The list hook maps response.financialBookingLogs into items
using instrumentCode: getInstrumentCode(log.baseInstrumentCode) while the detail
hook treats instrumentCode as currency—this causes a data-shape drift; update
the mapping and types so instrumentCode consistently represents the instrument
and add a new currencyCode field: extend the FinancialBookingLog type/interface
to include currencyCode, populate currencyCode from the appropriate source in
the same mapping where items is built (alongside id, status, etc.), keep
instrumentCode assigned via getInstrumentCode(log.baseInstrumentCode), and
update any detail hook/consumers to read currencyCode for currency displays and
totals instead of instrumentCode.
In `@frontend/src/features/ledger/pages/booking-log-detail.tsx`:
- Around line 157-159: The code currently falls back to a hardcoded 'GBP' when
data.instrumentCode is missing; change the fallback to derive the currency from
the posting data instead (e.g., use data?.postings[0]?.currency or another
canonical field) and pass that into computeTotals so totals use a
posting-derived currency rather than always 'GBP'; update the variable
assignment for currency near the postings/computeTotals usage (refer to
currency, postings, and computeTotals) to implement this safer fallback.
In `@frontend/src/features/market-data/hooks/use-market-data.ts`:
- Around line 31-32: statusFilter and categoryFilter currently use parseInt
which can yield NaN and that NaN is sent to listDataSets; change the parsing so
any invalid parse coerces to 0 (e.g. parseInt result stored to a temp variable
then set to 0 if Number.isNaN(value)) before building the RPC request; update
the logic around params.filters?.status and params.filters?.category so you pass
only numeric 0 or a valid integer to listDataSets.
In `@frontend/src/features/parties/pages/tabs/associations-tab.tsx`:
- Line 15: The component is calling usePartyAssociations(partyId) but only
destructures isLoading and always renders EmptyState; update the hook usage to
also destructure the returned data (e.g., const { data, isLoading } =
usePartyAssociations(partyId)) and change the render logic to: when isLoading
show a loader, when data exists and has items render the associations (map over
data or render an AssociationsList component), otherwise render EmptyState; if
always showing EmptyState was intentional, add a TODO comment next to
usePartyAssociations to document that placeholder behavior.
In `@frontend/src/features/payments/hooks/use-payments.ts`:
- Around line 60-67: The queryFn in use-payments.ts currently returns the
protobuf PaymentOrder via an unsafe double-cast to PaymentOrderDetail; replace
that cast with explicit mapping: take response.paymentOrder (PaymentOrder) and
construct a PaymentOrderDetail object by extracting amount.value and
currency_code from its MoneyAmount, converting the protobuf status enum to a
string, and populating sagaSteps and compensationSteps (either by fetching the
source that populates them or returning empty arrays/null with a comment if they
are provided elsewhere), so components like SagaTimeline in payment-detail.tsx
receive the expected fields; update the queryFn return to this constructed
PaymentOrderDetail rather than p as unknown as PaymentOrderDetail.
In `@frontend/src/features/positions/hooks/use-positions.ts`:
- Around line 23-27: Guard the parsed status filter before casting to
TransactionStatus: check that params.filters?.status (statusValue) is a valid
numeric value (e.g., Number(statusValue) yields a finite integer within the
TransactionStatus enum range) and only then cast; otherwise pass
TransactionStatus.UNSPECIFIED to
clients.positionKeeping.listFinancialPositionLogs. Update the logic in
use-positions.ts around the statusValue handling to validate/parsesafe the value
(reject NaN/Infinity/invalid enum numbers) and fallback to
TransactionStatus.UNSPECIFIED when invalid.
In `@frontend/src/features/reconciliation/hooks/use-reconciliation.ts`:
- Around line 59-65: The queryFn in use-reconciliation.ts currently bypasses
type safety with response as unknown as ReconciliationRunDetail; instead, remove
the unsafe double-cast and transform/validate the RPC payload returned by
clients.accountReconciliation.getReconciliationRun into a proper
ReconciliationRunDetail using the same normalization/mapping logic the table
hook uses (i.e., the mapper/normalizer that converts RPC payloads and applies
defaults and enum mapping), returning null on missing response and returning the
normalized ReconciliationRunDetail; ensure you reference runId and keep the
function signature intact.
In `@frontend/src/features/sagas/hooks/use-sagas.ts`:
- Around line 60-67: The queryFn in use-sagas.ts currently swallows all errors
from sagaRegistry.getActiveSaga and returns null; change it to only convert
not-found errors to null and rethrow all others: import ConnectError and Code
from '@connectrpc/connect', implement a helper like isNotFoundError(error) that
checks error instanceof ConnectError && error.code === Code.NotFound, call
getActiveSaga inside try/catch in queryFn, return null when
isNotFoundError(error) is true, otherwise rethrow the caught error so React
Query can surface it; reference sagaRegistry.getActiveSaga and the queryFn
wrapper when making this change.
In `@frontend/src/features/sagas/pages/detail.tsx`:
- Around line 134-139: The tenant-scoped saga detail cache (fetched by
useSagaDetail with key ['tenants', tenantSlug, 'sagas', definitionId]) is not
being invalidated after Activate/Deprecate; update the mutation success handlers
that currently only invalidate ['starlark-config', definitionId] to also
invalidate the tenant-scoped query key ['tenants', tenantSlug, 'sagas',
definitionId] (and the tenant sagas list ['tenants', tenantSlug, 'sagas'] if
present) so useSagaDetail and useActiveSaga see fresh data; locate the handlers
referenced in this file (the Activate/Deprecate mutation success callbacks) and
add the extra invalidateQueries calls using tenantSlug and definitionId.
---
Nitpick comments:
In `@frontend/src/features/internal-accounts/pages/`[accountId].tsx:
- Around line 178-190: Extract the inline useQuery in
InternalAccountTransactions into a reusable hook (e.g.
useInternalAccountTransactions) that accepts tenantSlug, accountId, and optional
instrumentCode, calls clients.financialAccounting.listLedgerPostings with the
same pagination and queryKey logic, and returns { data, isLoading, isError } so
InternalAccountTransactions simply calls
useInternalAccountTransactions(accountId, instrumentCode) and renders from the
hook result; ensure the hook uses useApiClients and useTenantContext (or accepts
tenantSlug) and preserves the enabled: !!accountId behavior and queryKey
structure (tenantKeys.internalAccount(...), 'postings').
In `@frontend/src/features/internal-accounts/pages/index.tsx`:
- Around line 11-19: The page defines an InternalAccountRow interface duplicate
of the one in use-internal-accounts.ts; export the existing InternalAccountRow
from use-internal-accounts.ts (add an export on the interface) and remove the
local definition in frontend/src/features/internal-accounts/pages/index.tsx,
then import { InternalAccountRow } from the hooks module and use that type for
the page component to keep a single source of truth.
In `@frontend/src/features/parties/hooks/use-parties.ts`:
- Around line 38-64: The queryFn passed to useCallback should include tenantSlug
in its dependency array to avoid a stale callback when tenantSlug changes;
update the useCallback dependency list for queryFn to include tenantSlug (in
addition to clients.party), since queryKey (from tenantKeys.parties(tenantSlug))
is returned alongside queryFn and must stay consistent with tenantSlug changes.
In `@frontend/src/features/positions/hooks/use-positions.ts`:
- Line 7: The hook use-positions.ts currently imports the UI page type
FinancialPositionLog from pages/index which couples data to presentation; create
a new feature type module (e.g., frontend/src/features/positions/types.ts or
move into your generated API types) that exports FinancialPositionLog (and any
related interfaces), update use-positions.ts to import FinancialPositionLog from
that new types module, and update pages/index to import the type from the same
new module so both hook (usePositions) and page consume the shared type instead
of pages/index.
In `@frontend/src/features/reconciliation/hooks/use-reconciliation.ts`:
- Around line 6-7: The hook currently imports ReconciliationRun and
ReconciliationRunDetail from page modules, creating an inverted dependency;
extract those interfaces into a feature-level types file (e.g., create a
types.ts next to use-reconciliation.ts) and move the ReconciliationRun and
ReconciliationRunDetail definitions there, then update use-reconciliation.ts to
import the types from that new feature module instead of ../pages/*; ensure any
other files that referenced the old page-local types are updated to import from
the new types module to keep contracts consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40243401-b407-46af-a32b-dae1c49c30e3
📒 Files selected for processing (33)
frontend/src/features/internal-accounts/hooks/index.tsfrontend/src/features/internal-accounts/hooks/use-internal-accounts.tsfrontend/src/features/internal-accounts/pages/[accountId].tsxfrontend/src/features/internal-accounts/pages/index.tsxfrontend/src/features/ledger/hooks/index.tsfrontend/src/features/ledger/hooks/use-ledger.tsfrontend/src/features/ledger/pages/booking-log-detail.tsxfrontend/src/features/ledger/pages/booking-log-header.tsxfrontend/src/features/ledger/pages/index.tsxfrontend/src/features/market-data/hooks/index.tsfrontend/src/features/market-data/hooks/use-market-data.tsfrontend/src/features/market-data/pages/[datasetCode].tsxfrontend/src/features/market-data/pages/index.tsxfrontend/src/features/parties/hooks/index.tsfrontend/src/features/parties/hooks/use-parties.tsfrontend/src/features/parties/pages/[partyId].tsxfrontend/src/features/parties/pages/index.tsxfrontend/src/features/parties/pages/tabs/associations-tab.tsxfrontend/src/features/payments/hooks/index.tsfrontend/src/features/payments/hooks/use-payments.tsfrontend/src/features/payments/pages/index.tsxfrontend/src/features/payments/pages/payment-detail.tsxfrontend/src/features/positions/hooks/index.tsfrontend/src/features/positions/hooks/use-positions.tsfrontend/src/features/positions/pages/detail.tsxfrontend/src/features/positions/pages/index.tsxfrontend/src/features/reconciliation/hooks/index.tsfrontend/src/features/reconciliation/hooks/use-reconciliation.tsfrontend/src/features/reconciliation/pages/index.tsxfrontend/src/features/sagas/hooks/index.tsfrontend/src/features/sagas/hooks/use-sagas.tsfrontend/src/features/sagas/pages/detail.tsxfrontend/src/features/sagas/pages/index.tsx
Update 10 test files to work with the refactored pages that now use data hooks instead of inline query logic: - Add @/hooks/use-tenant-context mock for hooks that need tenantSlug - Switch from useClients to useApiClients mocks where hooks changed - Mock ../hooks module for payments and reconciliation tests - Update booking-log-header test to use instrumentCode (was baseCurrency)
bjcoombs
dismissed
coderabbitai[bot]’s stale review
Stale review — test fixes pushed
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Remove unused useClients import and mockPaymentDetailQuery variable to fix lint errors.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/features/ledger/pages/booking-log-header.test.tsx (1)
40-42: Test name should be updated to reflect the field change.The test is named "renders the base currency" but the mock data now uses
instrumentCode. Consider updating the test description for consistency and maintainability.📝 Suggested fix
- it('renders the base currency', () => { + it('renders the instrument code', () => { render(<BookingLogHeader bookingLog={mockBookingLog} />) expect(screen.getByText('GBP')).toBeInTheDocument() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/ledger/pages/booking-log-header.test.tsx` around lines 40 - 42, Rename the test description to reflect the field change: update the it(...) string in booking-log-header.test.tsx for the test that renders BookingLogHeader with mockBookingLog (the test currently reads "renders the base currency") to something like "renders the instrument code" or "renders the instrument" so it matches the mockBookingLog's instrumentCode field and keeps the test name consistent with the assertion using screen.getByText('GBP').frontend/src/features/parties/pages/tabs/associations-tab.test.tsx (1)
68-72: Reduce duplicateduseApiClientsoverrides with a shared helper.Line 68 and the other override blocks repeat near-identical setup, which makes future edits error-prone. Consider reusing the default
mockPartyClientand only overridingretrieveAssociationsper scenario.♻️ Suggested refactor
+function mockApiClientWithRetrieve( + retrieveAssociationsImpl: () => Promise<unknown> +) { + vi.mocked(useApiClients).mockReturnValue({ + party: { + ...mockPartyClient, + retrieveAssociations: vi.fn(retrieveAssociationsImpl), + }, + } as ReturnType<typeof useApiClients>) +} // loading -vi.mocked(useApiClients).mockReturnValue({ - party: { - retrieveAssociations: vi.fn(() => new Promise(() => {})), - }, -} as ReturnType<typeof useApiClients>) +mockApiClientWithRetrieve(() => new Promise(() => {})) // empty state -vi.mocked(useApiClients).mockReturnValue({ - party: { - retrieveAssociations: vi.fn().mockResolvedValue({}), - }, -} as ReturnType<typeof useApiClients>) +mockApiClientWithRetrieve(() => Promise.resolve({}))Also applies to: 81-85, 95-99, 109-113, 123-127, 140-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/parties/pages/tabs/associations-tab.test.tsx` around lines 68 - 72, Create a shared default mock for the party API client and reuse it across tests instead of repeating vi.mocked(useApiClients).mockReturnValue blocks; specifically, add a reusable mockPartyClient object (with defaults for other methods) and in each test override only retrieveAssociations (e.g., vi.mocked(useApiClients).mockReturnValue({ party: { ...mockPartyClient, retrieveAssociations: /* scenario fn */ } } as ReturnType<typeof useApiClients>)); update tests that currently set retrieveAssociations at lines around the override blocks to use this pattern so each test only supplies the scenario-specific retrieveAssociations implementation.frontend/src/features/payments/pages/payments-list.test.tsx (1)
66-67: Optional DRY cleanup for repeated mock response setup.The repeated
mockQueryFn.mockResolvedValue({ items: ... })calls can be extracted into a tiny helper to reduce duplication.♻️ Proposed refactor
+const setPaymentsResponse = (items: unknown[] = []) => { + mockQueryFn.mockResolvedValue({ items }) +} ... - mockQueryFn.mockResolvedValue({ items: [] }) + setPaymentsResponse([]) ... - mockQueryFn.mockResolvedValue({ items: samplePayments }) + setPaymentsResponse(samplePayments)Also applies to: 78-79, 97-98, 112-113, 127-128, 144-145, 172-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/payments/pages/payments-list.test.tsx` around lines 66 - 67, Extract the repeated mockQueryFn.mockResolvedValue({ items: ... }) calls into a small helper (e.g., setMockItems or mockQueryResponse) and replace each direct mockQueryFn.mockResolvedValue invocation in payments-list.test.tsx with that helper; locate uses of mockQueryFn.mockResolvedValue in the test file and update them to call the helper with the desired items array so the setup is DRY and easier to change.
🤖 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/payments/pages/payment-detail.test.tsx`:
- Around line 9-10: Delete the unused mock variable definition
mockPaymentDetailQuery (the vi.fn() declaration) from the test file; remove the
const mockPaymentDetailQuery = vi.fn() line and ensure there are no remaining
references to that symbol in payment-detail.test.tsx so lint stops flagging it.
In `@frontend/src/features/reconciliation/pages/index.test.tsx`:
- Around line 8-16: The module mock factory captures mockQueryFn too early; make
mockQueryFn hoisted so it's initialized before vi.mock runs. Replace the
top-level const mockQueryFn = vi.fn() with a hoisted initializer using
vi.hoisted(() => vi.fn()), leaving the vi.mock(...) and its use of
useReconciliationRunsTable/queryFn unchanged so the factory sees the initialized
mockQueryFn.
---
Nitpick comments:
In `@frontend/src/features/ledger/pages/booking-log-header.test.tsx`:
- Around line 40-42: Rename the test description to reflect the field change:
update the it(...) string in booking-log-header.test.tsx for the test that
renders BookingLogHeader with mockBookingLog (the test currently reads "renders
the base currency") to something like "renders the instrument code" or "renders
the instrument" so it matches the mockBookingLog's instrumentCode field and
keeps the test name consistent with the assertion using screen.getByText('GBP').
In `@frontend/src/features/parties/pages/tabs/associations-tab.test.tsx`:
- Around line 68-72: Create a shared default mock for the party API client and
reuse it across tests instead of repeating
vi.mocked(useApiClients).mockReturnValue blocks; specifically, add a reusable
mockPartyClient object (with defaults for other methods) and in each test
override only retrieveAssociations (e.g.,
vi.mocked(useApiClients).mockReturnValue({ party: { ...mockPartyClient,
retrieveAssociations: /* scenario fn */ } } as ReturnType<typeof
useApiClients>)); update tests that currently set retrieveAssociations at lines
around the override blocks to use this pattern so each test only supplies the
scenario-specific retrieveAssociations implementation.
In `@frontend/src/features/payments/pages/payments-list.test.tsx`:
- Around line 66-67: Extract the repeated mockQueryFn.mockResolvedValue({ items:
... }) calls into a small helper (e.g., setMockItems or mockQueryResponse) and
replace each direct mockQueryFn.mockResolvedValue invocation in
payments-list.test.tsx with that helper; locate uses of
mockQueryFn.mockResolvedValue in the test file and update them to call the
helper with the desired items array so the setup is DRY and easier to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33814a60-9e29-4e67-91be-ebd2ebc2b860
📒 Files selected for processing (10)
frontend/src/features/ledger/pages/booking-log-header.test.tsxfrontend/src/features/parties/pages/[partyId].test.tsxfrontend/src/features/parties/pages/tabs/associations-tab.test.tsxfrontend/src/features/payments/pages/payment-detail.test.tsxfrontend/src/features/payments/pages/payments-list.test.tsxfrontend/src/features/positions/pages/detail.test.tsxfrontend/src/features/positions/pages/index.test.tsxfrontend/src/features/reconciliation/pages/index.test.tsxfrontend/src/features/sagas/pages/detail.test.tsxfrontend/src/features/sagas/pages/index.test.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/src/features/parties/pages/tabs/associations-tab.test.tsx (2)
9-13: RemoveuseClientsfrom this test mock to keep migration intent strict.This file now validates
useApiClients; retaininguseClientshere can let accidental fallback usage pass tests silently.🧹 Suggested cleanup
vi.mock('@/api/context', () => ({ - useClients: vi.fn(() => ({ - party: { - retrieveAssociations: mockRetrieveAssociations, - }, - })), useApiClients: vi.fn(() => ({ party: { retrieveAssociations: mockRetrieveAssociations, listParties: vi.fn().mockResolvedValue({ parties: [] }), registerAssociations: vi.fn(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/parties/pages/tabs/associations-tab.test.tsx` around lines 9 - 13, Remove the mocked useClients export from the test and replace references with the explicit useApiClients mock; specifically, delete the useClients: vi.fn(...) block (which currently returns party.retrieveAssociations: mockRetrieveAssociations) and ensure the test only mocks useApiClients to return the party.retrieveAssociations = mockRetrieveAssociations so the test validates useApiClients rather than allowing a fallback to useClients.
68-73: Avoid partialpartymocks with forced return-type casts.These overrides provide only
retrieveAssociationsand then cast toReturnType<typeof useApiClients>. That’s brittle ifAssociationsTabstarts reading otherpartymethods during render. ReusemockPartyClientand override only what each test needs.♻️ Example pattern
vi.mocked(useApiClients).mockReturnValue({ party: { - retrieveAssociations: vi.fn(() => new Promise(() => {})), + ...mockPartyClient, + retrieveAssociations: vi.fn(() => new Promise(() => {})), }, } as ReturnType<typeof useApiClients>)Also applies to: 81-86, 95-100, 109-114, 123-128, 140-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/parties/pages/tabs/associations-tab.test.tsx` around lines 68 - 73, The test currently stubs useApiClients with a partial party object (only retrieveAssociations) and casts to ReturnType<typeof useApiClients>, which is brittle; instead import/reuse the existing mockPartyClient and return a full party client, then override only retrieveAssociations for the specific test. Replace vi.mocked(useApiClients).mockReturnValue({ party: { retrieveAssociations: ... } } as ReturnType<typeof useApiClients>) with vi.mocked(useApiClients).mockReturnValue({ party: mockPartyClient({...override retrieveAssociations here...}) }) (or merge overrides onto mockPartyClient) so AssociationsTab and other consumers always see a complete party client; apply the same pattern to the other occurrences noted (lines ~81-86, 95-100, 109-114, 123-128, 140-143).frontend/src/features/payments/pages/payment-detail.test.tsx (1)
88-89: Extract repeated hook-state stubs into a helper.Line 88, Line 132, Line 189, Line 236, and Line 250 duplicate mock setup. A tiny helper would reduce drift and simplify future state additions.
♻️ Proposed cleanup
+const setPaymentDetailState = ( + state: Partial<ReturnType<typeof usePaymentDetail>> = {}, +) => { + mockUsePaymentDetail.mockReturnValue( + { data: sampleDetail, isLoading: false, isError: false, ...state } as ReturnType<typeof usePaymentDetail>, + ) +} + describe('PaymentDetailPage - structure', () => { beforeEach(() => { - mockUsePaymentDetail.mockReturnValue({ data: sampleDetail, isLoading: false, isError: false } as ReturnType<typeof usePaymentDetail>) + setPaymentDetailState() }) @@ describe('PaymentDetailPage - Overview tab', () => { beforeEach(() => { - mockUsePaymentDetail.mockReturnValue({ data: sampleDetail, isLoading: false, isError: false } as ReturnType<typeof usePaymentDetail>) + setPaymentDetailState() }) @@ describe('PaymentDetailPage - Saga Steps tab', () => { beforeEach(() => { - mockUsePaymentDetail.mockReturnValue({ data: sampleDetail, isLoading: false, isError: false } as ReturnType<typeof usePaymentDetail>) + setPaymentDetailState() }) @@ it('shows skeleton while loading', () => { - mockUsePaymentDetail.mockReturnValue({ data: undefined, isLoading: true, isError: false } as ReturnType<typeof usePaymentDetail>) + setPaymentDetailState({ data: undefined, isLoading: true, isError: false }) @@ it('shows error message on fetch failure', async () => { - mockUsePaymentDetail.mockReturnValue({ data: undefined, isLoading: false, isError: true } as ReturnType<typeof usePaymentDetail>) + setPaymentDetailState({ data: undefined, isLoading: false, isError: true })Also applies to: 132-133, 189-190, 236-237, 250-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/payments/pages/payment-detail.test.tsx` around lines 88 - 89, Repeated mock setups for usePaymentDetail are duplicated across tests; extract them into a single helper (e.g., stubPaymentDetail or setMockUsePaymentDetail) in the test file that accepts a partial state (data, isLoading, isError) and calls mockUsePaymentDetail.mockReturnValue(...) using ReturnType<typeof usePaymentDetail> and sampleDetail defaults; replace each direct mockUsePaymentDetail.mockReturnValue(...) occurrence (lines referencing mockUsePaymentDetail, usePaymentDetail, sampleDetail) with calls to this helper so all tests reuse the same centralized stub and can easily extend state later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/features/parties/pages/tabs/associations-tab.test.tsx`:
- Around line 9-13: Remove the mocked useClients export from the test and
replace references with the explicit useApiClients mock; specifically, delete
the useClients: vi.fn(...) block (which currently returns
party.retrieveAssociations: mockRetrieveAssociations) and ensure the test only
mocks useApiClients to return the party.retrieveAssociations =
mockRetrieveAssociations so the test validates useApiClients rather than
allowing a fallback to useClients.
- Around line 68-73: The test currently stubs useApiClients with a partial party
object (only retrieveAssociations) and casts to ReturnType<typeof
useApiClients>, which is brittle; instead import/reuse the existing
mockPartyClient and return a full party client, then override only
retrieveAssociations for the specific test. Replace
vi.mocked(useApiClients).mockReturnValue({ party: { retrieveAssociations: ... }
} as ReturnType<typeof useApiClients>) with
vi.mocked(useApiClients).mockReturnValue({ party: mockPartyClient({...override
retrieveAssociations here...}) }) (or merge overrides onto mockPartyClient) so
AssociationsTab and other consumers always see a complete party client; apply
the same pattern to the other occurrences noted (lines ~81-86, 95-100, 109-114,
123-128, 140-143).
In `@frontend/src/features/payments/pages/payment-detail.test.tsx`:
- Around line 88-89: Repeated mock setups for usePaymentDetail are duplicated
across tests; extract them into a single helper (e.g., stubPaymentDetail or
setMockUsePaymentDetail) in the test file that accepts a partial state (data,
isLoading, isError) and calls mockUsePaymentDetail.mockReturnValue(...) using
ReturnType<typeof usePaymentDetail> and sampleDetail defaults; replace each
direct mockUsePaymentDetail.mockReturnValue(...) occurrence (lines referencing
mockUsePaymentDetail, usePaymentDetail, sampleDetail) with calls to this helper
so all tests reuse the same centralized stub and can easily extend state later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9517ceb4-6bc8-4c22-96a1-ae523efda165
📒 Files selected for processing (2)
frontend/src/features/parties/pages/tabs/associations-tab.test.tsxfrontend/src/features/payments/pages/payment-detail.test.tsx
bjcoombs
dismissed
coderabbitai[bot]’s stale review
Suggestions noted — non-blocking improvements for follow-up
- Replace unsafe `as unknown as` casts with explicit field mapping in usePaymentDetail and useReconciliationRunDetail - Remove useCallback wrapper from usePartiesTable queryFn for consistency with other hooks - Add tenantSlug guard to enabled checks in usePositionLogDetail and useSagaDetail - Guard against NaN from parseInt in usePositionLogsTable and useDatasetsTable filter parsing - Make instrumentCode mapping consistent between booking log list and detail hooks (both use getInstrumentCode/baseInstrumentCode) - Remove unused getCurrencyName function from useBookingLogDetail - Derive currency from posting data instead of hardcoding 'GBP' fallback in BookingLogDetailPage - Remove error-swallowing try/catch in useActiveSaga - Add TODO for unused association data in AssociationsTab - Update test mock data to use baseInstrumentCode field
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/ledger/pages/booking-log-detail.test.tsx (1)
371-382:⚠️ Potential issue | 🟡 MinorNumeric-currency test does not validate currency behavior.
Line 371’s test name says numeric currency handling is verified, but it only asserts
log-001renders. This can pass even if currency mapping/display is broken.Suggested test hardening
it('handles numeric currency code (GBP=1)', async () => { - const logWithNumericCurrency = { ...mockBookingLog, baseInstrumentCode: 1 } + const logWithNumericCurrency = { + ...mockBookingLog, + postings: [ + { + ...mockBookingLog.postings[0], + postingAmount: { ...mockBookingLog.postings[0].postingAmount, currencyCode: 1 }, + }, + { + ...mockBookingLog.postings[1], + postingAmount: { ...mockBookingLog.postings[1].postingAmount, currencyCode: 1 }, + }, + ], + } setupMockClients({ result: { financialBookingLog: logWithNumericCurrency } }) renderWithApiClients(<BookingLogDetailPage />, { initialToken: createTenantUserToken('tenant-001'), queryClient: createTestQueryClient(), }) await waitFor(() => { - expect(screen.getAllByText('log-001').length).toBeGreaterThan(0) + expect(screen.getByTestId('debit-total')).toBeInTheDocument() }) + expect(screen.getByTestId('debit-total')).toHaveTextContent(/GBP/i) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/ledger/pages/booking-log-detail.test.tsx` around lines 371 - 382, The test "handles numeric currency code (GBP=1)" only asserts the ID renders but not that numeric baseInstrumentCode is mapped to GBP; update the test around the BookingLogDetailPage case that creates logWithNumericCurrency (using mockBookingLog and setupMockClients) to also assert the rendered currency display is correct (e.g., check for "GBP" or the expected symbol/format) after renderWithApiClients and waitFor; ensure you locate the test block that sets baseInstrumentCode: 1 and add an expectation that verifies the currency mapping/display rather than just asserting log-001 appears.
♻️ Duplicate comments (2)
frontend/src/features/positions/hooks/use-positions.ts (1)
23-27:⚠️ Potential issue | 🟡 MinorHarden
statusparsing beyond NaN checks.Line [27] currently rejects
NaNbut still accepts non-enum numeric values (e.g. decimals/Infinity/out-of-range) and forwards them asTransactionStatus. Validate finite integer + enum membership, otherwise fallback toUNSPECIFIED.Proposed fix
const statusValue = params.filters?.status + const parsedStatus = Number(statusValue) + const status = + Number.isFinite(parsedStatus) && + Number.isInteger(parsedStatus) && + Object.values(TransactionStatus).includes(parsedStatus as TransactionStatus) + ? (parsedStatus as TransactionStatus) + : TransactionStatus.UNSPECIFIED const response = await clients.positionKeeping.listFinancialPositionLogs({ pageToken: params.pageToken ?? '', accountId: params.filters?.accountId ?? '', - status: statusValue && !Number.isNaN(Number(statusValue)) ? (Number(statusValue) as TransactionStatus) : TransactionStatus.UNSPECIFIED, + status,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/positions/hooks/use-positions.ts` around lines 23 - 27, The status parsing in use-positions.ts currently only guards against NaN and can still pass decimals, Infinity, or out-of-range numbers to clients.positionKeeping.listFinancialPositionLogs; update the logic that computes status (the statusValue handling) to: convert to Number, ensure Number.isFinite(value) and Number.isInteger(value), then verify the integer is a valid TransactionStatus (e.g., check membership against Object.values(TransactionStatus) or a known set of enum values); if any check fails, set status to TransactionStatus.UNSPECIFIED before calling listFinancialPositionLogs.frontend/src/features/sagas/hooks/use-sagas.ts (1)
58-66:⚠️ Potential issue | 🟠 MajorAdd selective NotFound handling to return
nullinstead of surfacing as error state.The current implementation will surface
NotFounderrors fromgetActiveSagaas React Query error states, butNotFoundindicates "no active saga exists"—a valid state that should returnnull. This pattern is already established in similar hooks likeuse-accounts.tsanduse-internal-accounts.ts.🐛 Proposed fix
export function useActiveSaga(sagaName: string | undefined, enabled: boolean = true) { const { sagaRegistry } = useApiClients() return useQuery({ queryKey: ['starlark-config', 'active', sagaName], queryFn: async () => { if (!sagaName) return null - const response = await sagaRegistry.getActiveSaga({ name: sagaName }) - return response + try { + const response = await sagaRegistry.getActiveSaga({ name: sagaName }) + return response + } catch (err: unknown) { + if (ConnectError.from(err).code === Code.NotFound) return null + throw err + } }, enabled: !!sagaName && enabled, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/sagas/hooks/use-sagas.ts` around lines 58 - 66, Modify the queryFn in the use-sagas hook so that when calling sagaRegistry.getActiveSaga({ name: sagaName }) it catches errors and if the error indicates a NotFound condition returns null instead of letting React Query treat it as an error; if the error is anything else, rethrow it. Locate the async queryFn inside useQuery in use-sagas.ts and wrap the call to getActiveSaga with a try/catch that inspects the thrown error (matching the NotFound identifier used elsewhere in hooks like use-accounts.ts / use-internal-accounts.ts) to selectively return null for that case and propagate other errors.
🧹 Nitpick comments (9)
frontend/src/features/parties/hooks/use-parties.ts (1)
6-6: Decouple hook types from page modules.
use-parties.tscurrently importsPartyfrom../pages/index(Line 6). This creates a data-layer → page-layer dependency. Please movePartyto a feature-level shared type module (e.g.,features/parties/types) and import from there.♻️ Proposed refactor
- import type { Party } from '../pages/index' + import type { Party } from '../types/party'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/parties/hooks/use-parties.ts` at line 6, The hook file use-parties.ts imports the Party type from a page module which creates an improper dependency; extract or create a feature-level type module (e.g., features/parties/types) that exports the Party type, move the Party definition there, update use-parties.ts to import Party from the new types module, and change any other files that currently import Party from ../pages/index to import from the new feature types module so the data layer no longer depends on the page layer.frontend/src/features/positions/hooks/use-positions.ts (1)
7-7: Decouple hook types from page modules.Line [7] imports
FinancialPositionLogfrom../pages/index, which couples data hooks to UI pages. Move this type to a feature-leveltypesmodule (or use the generated API type directly) so hooks stay reusable and avoid page↔hook dependency drift.Proposed refactor
-import type { FinancialPositionLog } from '../pages/index' +import type { FinancialPositionLog } from '@/api/gen/meridian/positionkeeping/v1/position_keeping_pb'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/positions/hooks/use-positions.ts` at line 7, The hook imports the UI page type FinancialPositionLog, coupling the hook to a page; create a feature-level types module (e.g., export FinancialPositionLog from a new positions/types or use the generated API type) and update the hook (usePositions) to import FinancialPositionLog from that feature types module instead of the page; also update any consumers (the page that previously exported FinancialPositionLog) to import the type from the new feature types module so the page↔hook dependency is removed.frontend/src/features/ledger/hooks/use-ledger.ts (3)
60-60: Avoidas nevertype cast for status filter.The
as nevercast suppresses type checking rather than properly aligning the types. Consider defining a proper type for the status filter value or using the correct enum type from the API schema.💡 Suggested approach
- ...(statusFilter !== undefined && { status: statusFilter as never }), + ...(statusFilter !== undefined && { status: Number(statusFilter) }),Or, if the API expects a specific enum type, import and use that type for the conversion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/ledger/hooks/use-ledger.ts` at line 60, Replace the unsafe "as never" cast on the object spread expression "...(statusFilter !== undefined && { status: statusFilter as never })" by using the correct status type: import the API enum/type (e.g., LedgerStatus or the API's status enum) or declare a proper union type for statusFilter and cast to that type (e.g., status: statusFilter as LedgerStatus) so the object property has the correct type instead of never; update the use-ledger hook's statusFilter declaration or the spread to match the imported/declared status type.
73-73: Postings not mapped in list hook but fully mapped in detail hook.The list hook passes postings through with only a type assertion, while the detail hook (lines 107-126) maps each posting to the
LedgerPostingshape with normalized fields. This creates a data-shape inconsistency between list and detail views.If list consumers expect the same
LedgerPostingstructure, consider reusing the same mapping logic. If list postings are intentionally raw, document this distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/ledger/hooks/use-ledger.ts` at line 73, The list hook currently assigns postings with a raw type assertion at the postings property (postings: (log.postings ?? []) as FinancialBookingLog['postings']) causing a mismatch with the detail hook which maps each posting to the LedgerPosting shape; update the list hook to reuse the same mapping/normalization used in the detail view (the mapping that converts raw posting fields into LedgerPosting) so list items produce the same normalized LedgerPosting structure, or explicitly document the intended raw shape if you want them different; locate the postings assignment in the list branch of use-ledger.ts and apply the same per-posting transform used around the detail mapping (the LedgerPosting mapping block) to ensure consistent data shapes.
115-118: Consider extracting BigInt conversion to a reusable helper.This IIFE pattern for safe BigInt conversion is duplicated in
booking-log-detail.tsx(lines 46 and 79). Extracting it would reduce duplication and improve maintainability.💡 Suggested helper
function toBigInt(value: unknown): bigint { if (typeof value === 'bigint') return value if (typeof value === 'number' && Number.isSafeInteger(value)) return BigInt(value) return 0n }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/ledger/hooks/use-ledger.ts` around lines 115 - 118, The IIFE used to safely convert postingAmount?.units to a bigint is duplicated (in use-ledger.ts inside the units property and in booking-log-detail.tsx), so extract a reusable helper named toBigInt(value: unknown): bigint that returns the bigint, converts safe integers, or 0n otherwise; replace the anonymous IIFE calls with toBigInt(p.postingAmount?.units) in useLedger (units) and the two locations in booking-log-detail to remove duplication and improve maintainability, and add the helper to a shared place (e.g., a util/module alongside ledger hooks) and import it where needed.frontend/src/features/ledger/pages/booking-log-detail.tsx (3)
39-39: Remove unused_currencyparameter fromcomputeTotals.The
_currencyparameter is not used in the function body. Either remove it or document why it's reserved for future use.💡 Suggested fix
-function computeTotals(postings: LedgerPosting[], _currency: string): { debitTotal: bigint; creditTotal: bigint } { +function computeTotals(postings: LedgerPosting[]): { debitTotal: bigint; creditTotal: bigint } {Update the call site at line 160:
- const { debitTotal, creditTotal } = computeTotals(postings, currency) + const { debitTotal, creditTotal } = computeTotals(postings)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/ledger/pages/booking-log-detail.tsx` at line 39, Remove the unused `_currency` parameter from the computeTotals function signature and update all call sites that currently pass a currency argument to call computeTotals with only the postings array; specifically, change function computeTotals(postings: LedgerPosting[], _currency: string) to computeTotals(postings: LedgerPosting[]): { debitTotal: bigint; creditTotal: bigint } and update invocations (e.g., where BookingLogDetail invokes computeTotals) to stop supplying the currency argument so TypeScript types remain consistent.
26-37: DuplicategetDirectionNamefunction.This function is identical to the one in
use-ledger.ts(lines 24-35). Consider exporting it from the hooks module and reusing it here to avoid duplication.💡 Suggested fix
Export
getDirectionNamefromuse-ledger.ts:export function getDirectionName(direction: unknown): string { ... }Then in this file:
-import { useBookingLogDetail } from '../hooks' +import { useBookingLogDetail, getDirectionName } from '../hooks'And remove the local definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/ledger/pages/booking-log-detail.tsx` around lines 26 - 37, The getDirectionName function is duplicated; remove the local definition in booking-log-detail.tsx and instead export getDirectionName from the hooks module (use-ledger.ts) by adding an export for the existing function there, then import getDirectionName into booking-log-detail.tsx and use it. Ensure the exported symbol name is exactly getDirectionName so references in booking-log-detail.tsx resolve without changes.
158-159: Currency derivation priority may be semantically reversed.The current logic prioritizes
instrumentCode(mapped frombaseInstrumentCodein the hook) overpostingCurrency. IfinstrumentCoderepresents the financial instrument rather than currency, the fallback order should be reversed to prefer the actual posting currency.💡 Suggested fix
const postingCurrency = postings.find((p) => p.postingAmount?.currencyCode)?.postingAmount?.currencyCode - const currency = data?.instrumentCode || postingCurrency || '' + const currency = postingCurrency || data?.instrumentCode || ''Alternatively, if
instrumentCodetruly represents currency in this context, consider renaming it or adding a separatecurrencyCodefield to theFinancialBookingLogtype for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/ledger/pages/booking-log-detail.tsx` around lines 158 - 159, The currency selection currently uses data?.instrumentCode before the actual posting currency; change the fallback order so postingCurrency (derived from postings.find(p => p.postingAmount?.currencyCode)?.postingAmount?.currencyCode) is used first and data?.instrumentCode is the fallback when postingCurrency is absent for the computed currency variable; alternatively, if instrumentCode truly represents currency, update the FinancialBookingLog type or rename instrumentCode to currencyCode to make intent explicit (adjust all usages of currency, postingCurrency, and data?.instrumentCode accordingly).frontend/src/features/sagas/hooks/use-sagas.ts (1)
40-49: Unreachable fallback query key due toenabledconstraint.The queryKey has a fallback branch
['starlark-config', definitionId]for whentenantSlugis undefined (line 43), but theenabledcondition on line 48 requirestenantSlugto be truthy. This makes the fallback branch dead code—the query will never execute without a tenant slug.Either remove the fallback to simplify the logic, or relax the
enabledcheck if tenant-less queries are valid.🔧 Option: Simplify by removing unreachable fallback
return useQuery({ - queryKey: tenantSlug - ? [...tenantKeys.sagas(tenantSlug), definitionId] - : ['starlark-config', definitionId], + queryKey: [...tenantKeys.sagas(tenantSlug ?? ''), definitionId], queryFn: async () => { const response = await sagaRegistry.getSaga({ id: definitionId ?? '' }) return response.saga }, enabled: Boolean(tenantSlug && definitionId), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/sagas/hooks/use-sagas.ts` around lines 40 - 49, The current useQuery call in use-sagas.ts builds a fallback queryKey ['starlark-config', definitionId] when tenantSlug is falsy, but the enabled flag requires tenantSlug, making that branch unreachable; fix by removing the fallback and use tenant-dependent keys only (use [...tenantKeys.sagas(tenantSlug), definitionId] when tenantSlug is present) so queryKey and enabled are consistent, keeping sagaRegistry.getSaga and the enabled: Boolean(tenantSlug && definitionId) logic; alternatively, if tenant-less queries should be allowed, relax enabled to Boolean(definitionId) and keep the fallback key ['starlark-config', definitionId] so both branches can execute.
🤖 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/parties/hooks/use-parties.ts`:
- Around line 73-80: The query functions use non-null assertions (partyId!)
inside the async queryFn blocks (the anonymous queryFn for the retrieve call and
the other queryFn around lines 90-94), which can cause runtime undefined usage
if refetch is invoked; update each queryFn to perform an explicit runtime guard
(e.g., if (!partyId) throw new Error('partyId is required') or return undefined)
before calling clients.party.retrieveParty(...) (and the other clients.party.*
call) so the function never dereferences partyId without checking, while keeping
the existing enabled: Boolean(tenantSlug && partyId) guard.
---
Outside diff comments:
In `@frontend/src/features/ledger/pages/booking-log-detail.test.tsx`:
- Around line 371-382: The test "handles numeric currency code (GBP=1)" only
asserts the ID renders but not that numeric baseInstrumentCode is mapped to GBP;
update the test around the BookingLogDetailPage case that creates
logWithNumericCurrency (using mockBookingLog and setupMockClients) to also
assert the rendered currency display is correct (e.g., check for "GBP" or the
expected symbol/format) after renderWithApiClients and waitFor; ensure you
locate the test block that sets baseInstrumentCode: 1 and add an expectation
that verifies the currency mapping/display rather than just asserting log-001
appears.
---
Duplicate comments:
In `@frontend/src/features/positions/hooks/use-positions.ts`:
- Around line 23-27: The status parsing in use-positions.ts currently only
guards against NaN and can still pass decimals, Infinity, or out-of-range
numbers to clients.positionKeeping.listFinancialPositionLogs; update the logic
that computes status (the statusValue handling) to: convert to Number, ensure
Number.isFinite(value) and Number.isInteger(value), then verify the integer is a
valid TransactionStatus (e.g., check membership against
Object.values(TransactionStatus) or a known set of enum values); if any check
fails, set status to TransactionStatus.UNSPECIFIED before calling
listFinancialPositionLogs.
In `@frontend/src/features/sagas/hooks/use-sagas.ts`:
- Around line 58-66: Modify the queryFn in the use-sagas hook so that when
calling sagaRegistry.getActiveSaga({ name: sagaName }) it catches errors and if
the error indicates a NotFound condition returns null instead of letting React
Query treat it as an error; if the error is anything else, rethrow it. Locate
the async queryFn inside useQuery in use-sagas.ts and wrap the call to
getActiveSaga with a try/catch that inspects the thrown error (matching the
NotFound identifier used elsewhere in hooks like use-accounts.ts /
use-internal-accounts.ts) to selectively return null for that case and propagate
other errors.
---
Nitpick comments:
In `@frontend/src/features/ledger/hooks/use-ledger.ts`:
- Line 60: Replace the unsafe "as never" cast on the object spread expression
"...(statusFilter !== undefined && { status: statusFilter as never })" by using
the correct status type: import the API enum/type (e.g., LedgerStatus or the
API's status enum) or declare a proper union type for statusFilter and cast to
that type (e.g., status: statusFilter as LedgerStatus) so the object property
has the correct type instead of never; update the use-ledger hook's statusFilter
declaration or the spread to match the imported/declared status type.
- Line 73: The list hook currently assigns postings with a raw type assertion at
the postings property (postings: (log.postings ?? []) as
FinancialBookingLog['postings']) causing a mismatch with the detail hook which
maps each posting to the LedgerPosting shape; update the list hook to reuse the
same mapping/normalization used in the detail view (the mapping that converts
raw posting fields into LedgerPosting) so list items produce the same normalized
LedgerPosting structure, or explicitly document the intended raw shape if you
want them different; locate the postings assignment in the list branch of
use-ledger.ts and apply the same per-posting transform used around the detail
mapping (the LedgerPosting mapping block) to ensure consistent data shapes.
- Around line 115-118: The IIFE used to safely convert postingAmount?.units to a
bigint is duplicated (in use-ledger.ts inside the units property and in
booking-log-detail.tsx), so extract a reusable helper named toBigInt(value:
unknown): bigint that returns the bigint, converts safe integers, or 0n
otherwise; replace the anonymous IIFE calls with
toBigInt(p.postingAmount?.units) in useLedger (units) and the two locations in
booking-log-detail to remove duplication and improve maintainability, and add
the helper to a shared place (e.g., a util/module alongside ledger hooks) and
import it where needed.
In `@frontend/src/features/ledger/pages/booking-log-detail.tsx`:
- Line 39: Remove the unused `_currency` parameter from the computeTotals
function signature and update all call sites that currently pass a currency
argument to call computeTotals with only the postings array; specifically,
change function computeTotals(postings: LedgerPosting[], _currency: string) to
computeTotals(postings: LedgerPosting[]): { debitTotal: bigint; creditTotal:
bigint } and update invocations (e.g., where BookingLogDetail invokes
computeTotals) to stop supplying the currency argument so TypeScript types
remain consistent.
- Around line 26-37: The getDirectionName function is duplicated; remove the
local definition in booking-log-detail.tsx and instead export getDirectionName
from the hooks module (use-ledger.ts) by adding an export for the existing
function there, then import getDirectionName into booking-log-detail.tsx and use
it. Ensure the exported symbol name is exactly getDirectionName so references in
booking-log-detail.tsx resolve without changes.
- Around line 158-159: The currency selection currently uses
data?.instrumentCode before the actual posting currency; change the fallback
order so postingCurrency (derived from postings.find(p =>
p.postingAmount?.currencyCode)?.postingAmount?.currencyCode) is used first and
data?.instrumentCode is the fallback when postingCurrency is absent for the
computed currency variable; alternatively, if instrumentCode truly represents
currency, update the FinancialBookingLog type or rename instrumentCode to
currencyCode to make intent explicit (adjust all usages of currency,
postingCurrency, and data?.instrumentCode accordingly).
In `@frontend/src/features/parties/hooks/use-parties.ts`:
- Line 6: The hook file use-parties.ts imports the Party type from a page module
which creates an improper dependency; extract or create a feature-level type
module (e.g., features/parties/types) that exports the Party type, move the
Party definition there, update use-parties.ts to import Party from the new types
module, and change any other files that currently import Party from
../pages/index to import from the new feature types module so the data layer no
longer depends on the page layer.
In `@frontend/src/features/positions/hooks/use-positions.ts`:
- Line 7: The hook imports the UI page type FinancialPositionLog, coupling the
hook to a page; create a feature-level types module (e.g., export
FinancialPositionLog from a new positions/types or use the generated API type)
and update the hook (usePositions) to import FinancialPositionLog from that
feature types module instead of the page; also update any consumers (the page
that previously exported FinancialPositionLog) to import the type from the new
feature types module so the page↔hook dependency is removed.
In `@frontend/src/features/sagas/hooks/use-sagas.ts`:
- Around line 40-49: The current useQuery call in use-sagas.ts builds a fallback
queryKey ['starlark-config', definitionId] when tenantSlug is falsy, but the
enabled flag requires tenantSlug, making that branch unreachable; fix by
removing the fallback and use tenant-dependent keys only (use
[...tenantKeys.sagas(tenantSlug), definitionId] when tenantSlug is present) so
queryKey and enabled are consistent, keeping sagaRegistry.getSaga and the
enabled: Boolean(tenantSlug && definitionId) logic; alternatively, if
tenant-less queries should be allowed, relax enabled to Boolean(definitionId)
and keep the fallback key ['starlark-config', definitionId] so both branches can
execute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9ccba46-2a62-42a1-a990-e51427c918ed
📒 Files selected for processing (10)
frontend/src/features/ledger/hooks/use-ledger.tsfrontend/src/features/ledger/pages/booking-log-detail.test.tsxfrontend/src/features/ledger/pages/booking-log-detail.tsxfrontend/src/features/market-data/hooks/use-market-data.tsfrontend/src/features/parties/hooks/use-parties.tsfrontend/src/features/parties/pages/tabs/associations-tab.tsxfrontend/src/features/payments/hooks/use-payments.tsfrontend/src/features/positions/hooks/use-positions.tsfrontend/src/features/reconciliation/hooks/use-reconciliation.tsfrontend/src/features/sagas/hooks/use-sagas.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/features/payments/hooks/use-payments.ts
- frontend/src/features/market-data/hooks/use-market-data.ts
- frontend/src/features/parties/pages/tabs/associations-tab.tsx
- frontend/src/features/reconciliation/hooks/use-reconciliation.ts
- Fix cache invalidation in saga detail page to use tenant-scoped query keys matching useSagaDetail hook - Restore NotFound-specific error handling in useActiveSaga instead of swallowing all errors - Use vi.hoisted for mock variable in reconciliation test to ensure correct initialization order with vi.mock hoisting - Replace non-null assertions with nullish coalescing in party hooks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/features/sagas/pages/detail.tsx (1)
132-134: Consider renamingsagaQueryRootto reflect actual scope.This key includes
definitionId, so it represents a detail key, not a root namespace. A clearer name reduces future invalidation mistakes.Suggested rename
- const sagaQueryRoot = tenantSlug + const sagaDetailQueryKey = tenantSlug ? [...tenantKeys.sagas(tenantSlug), definitionId] : ['starlark-config', definitionId] ... - void qc.invalidateQueries({ queryKey: sagaQueryRoot }) + void qc.invalidateQueries({ queryKey: sagaDetailQueryKey }) ... - void qc.invalidateQueries({ queryKey: sagaQueryRoot }) + void qc.invalidateQueries({ queryKey: sagaDetailQueryKey })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/sagas/pages/detail.tsx` around lines 132 - 134, Rename the variable sagaQueryRoot to a name that reflects it includes the detail identifier (e.g., sagaDetailKey or sagaQueryKey) because it contains definitionId; update the declaration that builds the array (the expression using tenantSlug, tenantKeys.sagas(tenantSlug), and definitionId) and rename all subsequent usages of sagaQueryRoot in this module (and any imports/exports) to the new identifier to ensure clearer semantics and avoid future invalidation mistakes.
🤖 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/sagas/pages/detail.tsx`:
- Around line 140-145: The page shows a false "not found" because
useSagaDetail(definitionId) is disabled when tenantSlug is falsy; either enforce
tenant selection at the route or allow non-tenant fetches — I recommend enabling
the query in non-tenant mode: in use-sagas.ts remove the tenantSlug && part from
the enabled option so enabled becomes Boolean(definitionId) (ensuring
useSagaDetail can run when tenantSlug is null), and update detail.test.tsx to
add a test case where useTenantSlug returns null/undefined to cover
platform-admin non-tenant behavior; references: useSagaDetail, useActiveSaga,
tenantSlug, enabled flag in use-sagas.ts, and detail.test.tsx.
---
Nitpick comments:
In `@frontend/src/features/sagas/pages/detail.tsx`:
- Around line 132-134: Rename the variable sagaQueryRoot to a name that reflects
it includes the detail identifier (e.g., sagaDetailKey or sagaQueryKey) because
it contains definitionId; update the declaration that builds the array (the
expression using tenantSlug, tenantKeys.sagas(tenantSlug), and definitionId) and
rename all subsequent usages of sagaQueryRoot in this module (and any
imports/exports) to the new identifier to ensure clearer semantics and avoid
future invalidation mistakes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4aabd8f5-1632-4712-b62b-4186e8c95cba
📒 Files selected for processing (4)
frontend/src/features/parties/hooks/use-parties.tsfrontend/src/features/reconciliation/pages/index.test.tsxfrontend/src/features/sagas/hooks/use-sagas.tsfrontend/src/features/sagas/pages/detail.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/features/sagas/hooks/use-sagas.ts
- frontend/src/features/reconciliation/pages/index.test.tsx
- frontend/src/features/parties/hooks/use-parties.ts
Remove tenantSlug guard from useSagaDetail enabled check since the starlark-config route is accessible to platform admins without a selected tenant. The hook already has a fallback query key for this non-tenant case.
bjcoombs
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
All review issues addressed in subsequent commits
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
Extracts inline
useQuerycalls from all feature module page components into reusable data hooks, following the pattern established infeatures/accounts/hooks/use-accounts.ts(PR #1413).useAuthenticatedFetchpatterns (payments, reconciliation) to Connect-RPC clientsChanges
New hook files (16 files)
Each module gets a
hooks/directory with a hook file and barrel export:payments/hooks/use-payments.ts—usePaymentsTable(),usePaymentDetail()ledger/hooks/use-ledger.ts—useBookingLogsTable(),useBookingLogDetail(),useLedgerPostings()positions/hooks/use-positions.ts—usePositionLogsTable(),usePositionLogDetail()parties/hooks/use-parties.ts—usePartiesTable(),usePartyDetail(),usePartyAssociations()reconciliation/hooks/use-reconciliation.ts—useReconciliationRunsTable(),useReconciliationRunDetail()sagas/hooks/use-sagas.ts—useSagasTable(),useSagaDetail(),useActiveSaga()market-data/hooks/use-market-data.ts—useDatasetsTable(),useDatasetDetail(),useDatasetObservations()internal-accounts/hooks/use-internal-accounts.ts—useInternalAccountsTable(),useInternalAccountDetail()Modified page files (17 files)
All page components updated to import and use the new hooks, removing inline query logic, helper functions, and unused imports.
Patterns
All hooks follow the established accounts pattern:
{ queryKey, queryFn, tenantSlug }forDataTablecomponentuseQuery()result directly@/lib/query-keys.ts@/api/contextuseTenantSlug()for tenant scopingTask Master
034-frontend-alignment.10 — Create data hooks for all feature modules
Test plan
npx tsc --noEmit)