feat(condo): DOMA-13196 upgrade billing receipts table#7638
Open
dkoviazin wants to merge 15 commits into
Open
Conversation
dkoviazin
added
the
🔬 WIP
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:
📝 WalkthroughWalkthroughReceiptsTable and supporting hooks were refactored to use the ChangesReceipts Table Refactoring
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx`:
- Around line 26-27: The import order is incorrect: place internal imports
before other same-group imports and alphabetize them; move the ServicesModal
import so it appears above the BillingReceiptForOrganizationGQL import in
ReceiptsTable.tsx (i.e., ensure ServicesModal is imported before
BillingReceiptForOrganizationGQL) and adjust surrounding newlines so imports
follow the project's groups (builtin → external → `@open-condo` → internal →
sibling → parent) and are alphabetized case-insensitively within their group.
- Around line 216-223: The bulk-delete loop using updateReceipt aborts on the
first rejected mutation and prevents the UI reset/refetch, causing a mixed
state; change the logic to run all mutations concurrently with
Promise.allSettled over selectedRows calling updateReceipt({ deletedAt }, { id
}), collect successes and failures, ensure selection reset
(tableRef.current?.api?.resetRowSelection()), setSelectedRowsCount(0), reset
pagination (tableRef.current?.api?.setPagination({ startRow: 0, endRow:
DEFAULT_PAGE_SIZE })), and always call tableRef.current?.api?.refetchData() even
if some mutations failed; surface or log failed IDs/errors so callers can show
an error message but do not skip the UI cleanup when there are partial failures
(refer to updateReceipt, selectedRows, tableRef, setSelectedRowsCount,
DEFAULT_PAGE_SIZE).
- Around line 100-116: Remove persisting row selection to URL: stop serializing
rowSelectionState into router.query by deleting the nextSelectedRows computation
and the branch that sets/deletes nextQuery.selectedRows; keep rowSelectionState
as ephemeral UI state only (do not add selectedRows to nextQuery), and ensure
only filters, sort and offset are written to router.query (references:
nextSelectedRows, rowSelectionState, nextQuery, ReceiptsTable.tsx).
- Around line 142-152: The onPeriodChange handler sets local state via setPeriod
but when value is cleared it incorrectly restores nextFilterState.period to
reportPeriod so the table remains filtered; update onPeriodChange (and its use
of tableRef?.api?.setFilterState) so that when value is null you set
nextFilterState.period = undefined (i.e., remove the effective period filter)
instead of assigning reportPeriod, ensuring the table filter matches the cleared
picker UI while leaving the reportPeriod logic unchanged when a value is
selected.
In `@apps/condo/domains/billing/hooks/useReceiptTableFilters.tsx`:
- Around line 26-30: The current construction of the where object lets
caller-provided query properties override the guard because ...query is spread
last; to fix, ensure the context filter (context: { id_in: contextIds }) is
authoritative by spreading query first and then explicitly setting context and
the address_contains_i based on searchText (i.e., build where as { ...query,
context: { id_in: contextIds }, ...( !isEmpty(searchText) ? {
address_contains_i: searchText } : {} ) }), so neither query.context nor
query.address_contains_i can override the enforced billing context or the
searchText-derived address filter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 801e6c16-91a1-463b-9beb-8b6ca5f9796e
📒 Files selected for processing (3)
apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsxapps/condo/domains/billing/hooks/useReceiptTableColumns.tsapps/condo/domains/billing/hooks/useReceiptTableFilters.tsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8989f7f045
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx (1)
277-287: 💤 Low valueAdd type annotation for the event parameter.
The
eventparameter has an implicitanytype. Consider typing it based on the bridge event shape to improve type safety.💡 Suggested fix
useEffect(() => { - const handleRedirect = async (event) => { + const handleRedirect = async (event: { type?: string }) => { if (get(event, 'type') === 'condo-bridge') { await tableRef.current?.api?.refetchData() } }Or use the proper event type from
@open-condo/bridgeif one is exported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx` around lines 277 - 287, The handler handleRedirect currently declares event with implicit any; add a proper type for its parameter (e.g., the exported BridgeEvent type from `@open-condo/bridge` or a local interface matching { type: string; [key: string]: any }) and update the signature of handleRedirect to use that type so bridge.subscribe(handleRedirect) and bridge.unsubscribe(handleRedirect) are type-safe; ensure imports include the chosen type and that tableRef.current?.api?.refetchData usage remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/condo/domains/billing/hooks/useReceiptTableColumns.ts`:
- Around line 38-46: Rename the file containing JSX to use the .tsx extension so
TypeScript can parse JSX: change useReceiptTableColumns.ts to
useReceiptTableColumns.tsx; locate the renderTextCellWithoutTooltip function
(which returns <Typography.Paragraph> and uses ELLIPSIS_CONFIG/ELLIPSIS_STYLES
and getHighlightedContents) to confirm the JSX-containing module is renamed, and
update any import sites that explicitly reference the .ts filename so consumers
import the updated .tsx module.
---
Nitpick comments:
In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx`:
- Around line 277-287: The handler handleRedirect currently declares event with
implicit any; add a proper type for its parameter (e.g., the exported
BridgeEvent type from `@open-condo/bridge` or a local interface matching { type:
string; [key: string]: any }) and update the signature of handleRedirect to use
that type so bridge.subscribe(handleRedirect) and
bridge.unsubscribe(handleRedirect) are type-safe; ensure imports include the
chosen type and that tableRef.current?.api?.refetchData usage remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5428309f-4765-4ad4-8aee-826c5785da08
📒 Files selected for processing (3)
apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsxapps/condo/domains/billing/components/BillingPageContent/ServicesModal.tsxapps/condo/domains/billing/hooks/useReceiptTableColumns.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx (2)
204-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winA transient load failure becomes unrecoverable here.
After the catch sets
loadingErrortotrue, the component returns the empty state and unmounts the table. Nothing in this render path resets that flag, so the user cannot trigger another fetch without a full page reload.Also applies to: 307-315
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx` around lines 204 - 209, The catch handler currently sets loadingError to true and returns an empty result, which leaves the table unmounted with no way to retry; update the data-fetch path (the function that catches errors and returns { rowData, rowCount } — reference setLoadingError and the fetch/load function around the current catch and the similar block at 307-315) so that you clear or reset loadingError before each new fetch attempt (call setLoadingError(false) at the start of the loader or retry logic) and set setLoadingError(false) on successful responses; alternatively expose a retry path (e.g., set a retry state or provide a retry button that clears loadingError) so transient failures do not become unrecoverable.
63-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the bulk-delete UI aligned with the new access rules.
The server-side access change still allows admins to soft-delete receipts, but
canManageReceiptsnow only checkslink.role.canImportBillingReceipts. That hides row selection and theActionBarfor admins who are still authorized by the mutation layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx` around lines 63 - 73, canManageReceipts only checks link.role.canImportBillingReceipts so admins who can soft-delete on the server still get hidden UI; update the canManageReceipts assignment in ReceiptsTable (where useOrganization and useBillingAndAcquiringContexts are used) to include the deletion/soft-delete permission as well (e.g. check link.role.canDeleteBillingReceipts or link.role.canSoftDeleteBillingReceipts in addition to link.role.canImportBillingReceipts) so row selection and the ActionBar are shown for users allowed to soft-delete by the mutation layer.
♻️ Duplicate comments (3)
apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx (3)
213-226:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle bulk-delete failures without leaving the UI half-reset.
The first rejected
updateReceiptaborts the loop, so some receipts may already be soft-deleted while selection reset, pagination reset, and refetch never run. Run the mutations withPromise.allSettledand always perform UI cleanup/refetch afterward.Suggested fix
- for (const id of selectedRows) { - await updateReceipt({ deletedAt }, { id }) - } - - tableRef.current?.api?.resetRowSelection() - setSelectedRowsCount(0) - tableRef.current?.api?.setPagination({ startRow: 0, endRow: DEFAULT_PAGE_SIZE }) - await tableRef.current?.api?.refetchData() + const results = await Promise.allSettled( + selectedRows.map((id) => updateReceipt({ deletedAt }, { id })), + ) + + tableRef.current?.api?.resetRowSelection() + setSelectedRowsCount(0) + tableRef.current?.api?.setPagination({ startRow: 0, endRow: DEFAULT_PAGE_SIZE }) + await tableRef.current?.api?.refetchData() + + const failedIds = selectedRows.filter((_, index) => results[index].status === 'rejected') + if (failedIds.length > 0) { + // surface/log failed ids so the user is not left guessing + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx` around lines 213 - 226, The softDeleteSelectedReceipts handler currently awaits each updateReceipt in a for-loop so a rejected mutation aborts the loop and prevents the UI cleanup (tableRef.api.resetRowSelection, setSelectedRowsCount, setPagination, refetchData) from running; change softDeleteSelectedReceipts to collect promises for updateReceipt for each selectedRows ID, run them with Promise.allSettled, and then always perform the cleanup/refetch (tableRef.current?.api.resetRowSelection(), setSelectedRowsCount(0), tableRef.current?.api.setPagination({ startRow: 0, endRow: DEFAULT_PAGE_SIZE }), await tableRef.current?.api.refetchData()) in a finally block (or after allSettled) so partial failures don’t leave the UI half-reset while still surfacing or logging per-id failures from the allSettled results.
143-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClearing the month picker should also clear the effective filter.
When
valueis cleared, the control shows no month selected, but the handler restoresnextFilterState.periodtoreportPeriod. The table stays filtered while the UI says otherwise.Suggested fix
- setPeriod(value) - const currentFilterState = tableRef.current?.api?.getFilterState() || {} const nextFilterState = { ...currentFilterState } - if (value && dateString) { - nextFilterState.period = value.startOf('month').format('YYYY-MM-01') - } else { - nextFilterState.period = reportPeriod || undefined - } + const nextPeriod = value && dateString ? value.startOf('month') : null + setPeriod(nextPeriod) + nextFilterState.period = nextPeriod?.format('YYYY-MM-01') tableRef.current?.api?.setFilterState(nextFilterState)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx` around lines 143 - 153, The onPeriodChange handler restores reportPeriod when value is cleared which leaves the table filtered while the month picker shows empty; update onPeriodChange (the function that calls setPeriod and uses tableRef.current?.api?.setFilterState) so that when value is null you set nextFilterState.period = undefined (clearing the effective filter) instead of assigning reportPeriod, ensuring the UI and table filter state stay in sync.
88-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not persist selected row ids in the URL.
selectedRowsis ephemeral UI state. Serializing it intorouter.querymakes refresh/share/back-navigation brittle and can hit common URL length limits on bulk selection.Suggested fix
- const { startRow, filterState, sortState, rowSelectionState, globalFilter } = params + const { startRow, filterState, sortState, globalFilter } = params @@ - const nextSelectedRows = rowSelectionState?.length > 0 ? JSON.stringify(rowSelectionState) : undefined - if (nextOffset) nextQuery.offset = nextOffset else delete nextQuery.offset @@ if (nextSortValue) nextQuery.sort = nextSortValue else delete nextQuery.sort - - if (nextSelectedRows) nextQuery.selectedRows = nextSelectedRows - else delete nextQuery.selectedRows🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx` around lines 88 - 117, The code is persisting ephemeral UI state (rowSelectionState) into the URL via nextSelectedRows/nextQuery.selectedRows; remove that behavior by deleting the creation of nextSelectedRows and the conditional that sets nextQuery.selectedRows, and instead ensure nextQuery.selectedRows is removed/unset (delete nextQuery.selectedRows) so selected row ids are never serialized into router.query; update any references to rowSelectionState/nextSelectedRows in ReceiptsTable (e.g., nextSelectedRows, rowSelectionState) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/condo/domains/billing/access/BillingReceipt.js`:
- Around line 70-75: The current isSoftDeleteInput function incorrectly requires
exactly three keys; change it to check that the payload contains a deletedAt
field and that every key present (from data or data.data) is included in
SOFT_DELETE_ALLOWED_UPDATE_FIELDS instead of enforcing keys.length === 3, so
isSoftDeleteInput allows `{ deletedAt }` and also validates any extra keys are
whitelisted.
- Around line 101-109: The current permission check uses
find('BillingIntegrationOrganizationContext', ...) and only rejects when no
contexts are returned, allowing batches to pass if some contextIds are missing;
change the logic in the function that uses find (the block that populates
contexts from BillingIntegrationOrganizationContext with contextIds) to validate
that every requested context was resolved by comparing contexts.length to
contextIds.length and return false (reject) if they differ (i.e., any context is
missing or soft-deleted) before calling checkPermissionsInEmployedOrganizations;
keep the rest of the flow (building organizationIds and calling
checkPermissionsInEmployedOrganizations) unchanged.
In `@apps/condo/domains/billing/schema/BillingReceipt.test.js`:
- Around line 382-391: The test incorrectly treats deletedReceipt as a single
receipt even though updateTestBillingReceipts returns an array; change the
assertion to inspect the array elements (or destructure the returned array into
e.g. [deletedReceipt1, deletedReceipt2]) from the call to
updateTestBillingReceipts and assert each element's deletedAt is set, and remove
the debug console.error; reference createTestBillingReceipt,
updateTestBillingReceipts and variables receiptToDelete / oneMoreReceiptToDelete
to locate the fix.
---
Outside diff comments:
In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx`:
- Around line 204-209: The catch handler currently sets loadingError to true and
returns an empty result, which leaves the table unmounted with no way to retry;
update the data-fetch path (the function that catches errors and returns {
rowData, rowCount } — reference setLoadingError and the fetch/load function
around the current catch and the similar block at 307-315) so that you clear or
reset loadingError before each new fetch attempt (call setLoadingError(false) at
the start of the loader or retry logic) and set setLoadingError(false) on
successful responses; alternatively expose a retry path (e.g., set a retry state
or provide a retry button that clears loadingError) so transient failures do not
become unrecoverable.
- Around line 63-73: canManageReceipts only checks
link.role.canImportBillingReceipts so admins who can soft-delete on the server
still get hidden UI; update the canManageReceipts assignment in ReceiptsTable
(where useOrganization and useBillingAndAcquiringContexts are used) to include
the deletion/soft-delete permission as well (e.g. check
link.role.canDeleteBillingReceipts or link.role.canSoftDeleteBillingReceipts in
addition to link.role.canImportBillingReceipts) so row selection and the
ActionBar are shown for users allowed to soft-delete by the mutation layer.
---
Duplicate comments:
In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx`:
- Around line 213-226: The softDeleteSelectedReceipts handler currently awaits
each updateReceipt in a for-loop so a rejected mutation aborts the loop and
prevents the UI cleanup (tableRef.api.resetRowSelection, setSelectedRowsCount,
setPagination, refetchData) from running; change softDeleteSelectedReceipts to
collect promises for updateReceipt for each selectedRows ID, run them with
Promise.allSettled, and then always perform the cleanup/refetch
(tableRef.current?.api.resetRowSelection(), setSelectedRowsCount(0),
tableRef.current?.api.setPagination({ startRow: 0, endRow: DEFAULT_PAGE_SIZE }),
await tableRef.current?.api.refetchData()) in a finally block (or after
allSettled) so partial failures don’t leave the UI half-reset while still
surfacing or logging per-id failures from the allSettled results.
- Around line 143-153: The onPeriodChange handler restores reportPeriod when
value is cleared which leaves the table filtered while the month picker shows
empty; update onPeriodChange (the function that calls setPeriod and uses
tableRef.current?.api?.setFilterState) so that when value is null you set
nextFilterState.period = undefined (clearing the effective filter) instead of
assigning reportPeriod, ensuring the UI and table filter state stay in sync.
- Around line 88-117: The code is persisting ephemeral UI state
(rowSelectionState) into the URL via nextSelectedRows/nextQuery.selectedRows;
remove that behavior by deleting the creation of nextSelectedRows and the
conditional that sets nextQuery.selectedRows, and instead ensure
nextQuery.selectedRows is removed/unset (delete nextQuery.selectedRows) so
selected row ids are never serialized into router.query; update any references
to rowSelectionState/nextSelectedRows in ReceiptsTable (e.g., nextSelectedRows,
rowSelectionState) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61ecf15f-c8bc-4996-a08e-49bd3b07ff1a
📒 Files selected for processing (6)
apps/condo/domains/billing/access/BillingReceipt.jsapps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsxapps/condo/domains/billing/gql.jsapps/condo/domains/billing/hooks/useReceiptTableColumns.tsapps/condo/domains/billing/hooks/useReceiptTableFilters.tsxapps/condo/domains/billing/schema/BillingReceipt.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx (1)
214-227:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the bulk delete resilient to partial failures.
If one
updateReceiptcall rejects, this loop exits immediately and skips row-selection reset, pagination reset, and refetch, even though some earlier receipts may already be soft-deleted.Suggested fix
const softDeleteSelectedReceipts = useCallback(async () => { if (!selectedRowIds.length) return const deletedAt = new Date().toISOString() - for (const id of selectedRowIds) { - await updateReceipt({ deletedAt }, { id }) - } - - tableRef.current?.api?.resetRowSelection() - setSelectedRowsCount(0) - setSelectedRowIds([]) - tableRef.current?.api?.setPagination({ startRow: 0, endRow: DEFAULT_PAGE_SIZE }) - await tableRef.current?.api?.refetchData() + const results = await Promise.allSettled( + selectedRowIds.map((id) => updateReceipt({ deletedAt }, { id })), + ) + + tableRef.current?.api?.resetRowSelection() + setSelectedRowsCount(0) + setSelectedRowIds([]) + tableRef.current?.api?.setPagination({ startRow: 0, endRow: DEFAULT_PAGE_SIZE }) + await tableRef.current?.api?.refetchData() + + const failedIds = results.flatMap((result, index) => ( + result.status === 'rejected' ? [selectedRowIds[index]] : [] + )) + if (failedIds.length) { + throw new Error(`Failed to delete receipts: ${failedIds.join(', ')}`) + } }, [selectedRowIds, updateReceipt])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx` around lines 214 - 227, The bulk-delete loop using selectedRowIds and updateReceipt is brittle: a rejection aborts the loop and skips cleanup. Change it to perform updates resiliently (e.g., use Promise.allSettled on selectedRowIds.map(id => updateReceipt(...)) or wrap each await updateReceipt call in a try/catch) so failures don’t stop processing other ids; collect/log failed ids/errors, but always run the cleanup steps (tableRef.current?.api?.resetRowSelection(), setSelectedRowsCount(0), setSelectedRowIds([]), tableRef.current?.api?.setPagination(...), await tableRef.current?.api?.refetchData()) in a finally block so selection, pagination and refetch happen regardless of partial failures.
🧹 Nitpick comments (2)
apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx (1)
1-24: ⚡ Quick winMove
@app/condo/schemainto the internal import group.Line 1 should come after the external and
@open-condo/*imports; keeping it at the top still violates the repo import-order rule and is likely to trip lint.As per coding guidelines, "Enforce import order groups: builtin → external → `@open-condo` → internal → sibling → parent with newlines between groups" and "Alphabetize imports case-insensitively within each import group".Suggested fix
-import { BillingReceipt as BillingReceiptType, BillingReceiptWhereInput, SortBillingReceiptsBy, TourStepTypeType } from '`@app/condo/schema`' import { Col, Row, Space, Typography, type RowProps } from 'antd' import dayjs, { Dayjs } from 'dayjs' import get from 'lodash/get' import isEqual from 'lodash/isEqual' import getConfig from 'next/config' import { useRouter } from 'next/router' import React, { CSSProperties, useCallback, useEffect, useMemo, useRef, useState } from 'react' import bridge from '`@open-condo/bridge`' import { useApolloClient } from '`@open-condo/next/apollo`' import { useIntl } from '`@open-condo/next/intl`' import { useOrganization } from '`@open-condo/next/organization`' import { ActionBar, ActionBarProps, Button, FullTableState, GetTableData, Input, RowSelectionState, Table, TableRef, } from '`@open-condo/ui`' +import { BillingReceipt as BillingReceiptType, BillingReceiptWhereInput, SortBillingReceiptsBy, TourStepTypeType } from '`@app/condo/schema`' import { ServicesModal } from '`@condo/domains/billing/components/BillingPageContent/ServicesModal`'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx` around lines 1 - 24, The import from '`@app/condo/schema`' (e.g., BillingReceipt, BillingReceiptWhereInput, SortBillingReceiptsBy, TourStepTypeType) is currently in the wrong group; move that whole import statement into the internal import group so it appears after external and `@open-condo/`* imports in ReceiptsTable.tsx, and ensure imports are alphabetized case-insensitively within each group (adjust surrounding imports such as bridge, useApolloClient, useIntl, useOrganization, `@open-condo/ui` imports and the moved '`@app/condo/schema`' line to maintain the repo's import-order rule).apps/condo/domains/billing/schema/BillingReceipt.test.js (1)
385-392: ⚡ Quick winAdd a mixed-organization batch denial case next to this success path.
This covers the happy path, but the new access helper has batch-specific validation that rejects batches spanning unauthorized contexts. A regression test with one allowed receipt and one foreign receipt would pin the exact branch this PR added.
Suggested test shape
+ test('Employee with canImportBillingReceipts cannot delete receipts from different organization contexts', async () => { + const [receiptInOwnOrg] = await createTestBillingReceipt(admin, context, property, account) + const [receiptInAnotherOrg] = await createTestBillingReceipt(admin, anotherContext, anotherProperty, anotherAccount) + + await expectToThrowAccessDeniedErrorToObjects(async () => { + await updateTestBillingReceipts(billingReceiptsImporter, [ + { id: receiptInOwnOrg.id, data: { deletedAt: new Date().toISOString() } }, + { id: receiptInAnotherOrg.id, data: { deletedAt: new Date().toISOString() } }, + ]) + }) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/billing/schema/BillingReceipt.test.js` around lines 385 - 392, Add a negative test next to the current success path that verifies batch rejection when receipts span organizations: call updateTestBillingReceipts with billingReceiptsImporter using one id that belongs to the test org (e.g., receiptToDelete.id) and one id that belongs to a different organization (create or reuse a foreign receipt fixture like foreignReceipt.id), then assert the call fails (throws or returns an error) and that deletedReceipts was not updated; target the batch validation branch introduced by the access helper by referencing updateTestBillingReceipts, billingReceiptsImporter, receiptToDelete and a foreign receipt identifier to pin the mixed-organization denial behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/condo/domains/billing/access/BillingReceipt.js`:
- Around line 70-75: The isSoftDeleteInput predicate currently treats
{deletedAt: null} as a soft-delete payload; update isSoftDeleteInput to only
return true when the payload contains a deletedAt field with a non-null value
(e.g., deletedAt !== null/undefined or typeof deletedAt === 'string' /
instanceof Date as appropriate) in addition to the existing checks against
SOFT_DELETE_ALLOWED_UPDATE_FIELDS so that STAFF with canImportBillingReceipts
cannot use this branch to restore receipts; keep the existing target extraction
(const target = data?.data || data) and overall object/key checks but add the
explicit non-null deletedAt value check before returning true.
---
Duplicate comments:
In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx`:
- Around line 214-227: The bulk-delete loop using selectedRowIds and
updateReceipt is brittle: a rejection aborts the loop and skips cleanup. Change
it to perform updates resiliently (e.g., use Promise.allSettled on
selectedRowIds.map(id => updateReceipt(...)) or wrap each await updateReceipt
call in a try/catch) so failures don’t stop processing other ids; collect/log
failed ids/errors, but always run the cleanup steps
(tableRef.current?.api?.resetRowSelection(), setSelectedRowsCount(0),
setSelectedRowIds([]), tableRef.current?.api?.setPagination(...), await
tableRef.current?.api?.refetchData()) in a finally block so selection,
pagination and refetch happen regardless of partial failures.
---
Nitpick comments:
In `@apps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsx`:
- Around line 1-24: The import from '`@app/condo/schema`' (e.g., BillingReceipt,
BillingReceiptWhereInput, SortBillingReceiptsBy, TourStepTypeType) is currently
in the wrong group; move that whole import statement into the internal import
group so it appears after external and `@open-condo/`* imports in
ReceiptsTable.tsx, and ensure imports are alphabetized case-insensitively within
each group (adjust surrounding imports such as bridge, useApolloClient, useIntl,
useOrganization, `@open-condo/ui` imports and the moved '`@app/condo/schema`' line
to maintain the repo's import-order rule).
In `@apps/condo/domains/billing/schema/BillingReceipt.test.js`:
- Around line 385-392: Add a negative test next to the current success path that
verifies batch rejection when receipts span organizations: call
updateTestBillingReceipts with billingReceiptsImporter using one id that belongs
to the test org (e.g., receiptToDelete.id) and one id that belongs to a
different organization (create or reuse a foreign receipt fixture like
foreignReceipt.id), then assert the call fails (throws or returns an error) and
that deletedReceipts was not updated; target the batch validation branch
introduced by the access helper by referencing updateTestBillingReceipts,
billingReceiptsImporter, receiptToDelete and a foreign receipt identifier to pin
the mixed-organization denial behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bcf48e2-4f01-4b7f-8617-1dd2fd2420f4
📒 Files selected for processing (4)
apps/condo/domains/billing/access/BillingReceipt.jsapps/condo/domains/billing/components/BillingPageContent/ReceiptsTable.tsxapps/condo/domains/billing/hooks/useReceiptTableFilters.tsxapps/condo/domains/billing/schema/BillingReceipt.test.js
dkoviazin
force-pushed
the
feat/condo/DOMA-13196/upgrade_billing_receipt_table
branch
from
c100926 to
680a0e1
Compare
dkoviazin
added
✋🙂 Review please
abshnko
reviewed
May 22, 2026
abshnko
reviewed
May 22, 2026
abshnko
reviewed
May 22, 2026
abshnko
reviewed
May 22, 2026
abshnko
reviewed
May 22, 2026
vovaaxeapolla
approved these changes
May 22, 2026
AleX83Xpert
approved these changes
May 22, 2026
dkoviazin
force-pushed
the
feat/condo/DOMA-13196/upgrade_billing_receipt_table
branch
from
f7990f3 to
eb127b3
Compare
dkoviazin
force-pushed
the
feat/condo/DOMA-13196/upgrade_billing_receipt_table
branch
from
eb127b3 to
0f9186e
Compare
|
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
4 participants
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.
BEFORE:
AFTER:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests