refactor: remove unwanted Expense model that has tx_ properties #3931
Conversation
WalkthroughThe codebase migrates from the legacy Expense shape to v1 Transaction: deletes the Expense interface and a dependent-field mock, updates models, services, components, pages, templates, and tests to use Partial, adds Transaction.source_account, and removes legacy payment-mode helpers and related tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant MyExpenses as MyExpensesPage
participant Outbox as TransactionsOutboxService
participant TS as TransactionService
participant Card as ExpensesCardComponent
User->>MyExpenses: Open My Expenses
MyExpenses->>Outbox: getPendingExpenses()
Outbox-->>MyExpenses: Partial<Transaction>[]
MyExpenses->>Card: Render cards (Partial<Transaction>)
User->>Card: Select / long-press
Card-->>MyExpenses: cardClickedForSelection(expense: Partial<Transaction>)
MyExpenses->>MyExpenses: Toggle selection (by id / deep compare)
User->>MyExpenses: Delete selected
MyExpenses->>TS: getExpenseDeletionMessage(selected)
TS-->>MyExpenses: message
MyExpenses->>Outbox: deleteBulkOfflineExpenses(pendingExpenses, selected)
Outbox->>Outbox: remove by index
Outbox-->>MyExpenses: updated pendingExpenses
MyExpenses-->>User: updated UI
sequenceDiagram
autonumber
participant TS as TransactionService
participant TX as Transaction (Partial)
note over TS,TX: Field access refactor from tx_* → Transaction fields
TX->>TS: getIsDraft(tx)
TS->>TS: return tx.state === 'DRAFT'
TS-->>TX: boolean
TX->>TS: getVendorDetails(tx)
TS->>TS: derive vendor from tx.vendor / tx.vendorDetails
TS-->>TX: string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
|
PR description must contain a link to a ClickUp (case-insensitive) |
| @if (!(filters?.state || filters?.date || simpleSearchText) && !isLoading) { | ||
| <div> | ||
| @for (pendingTransaction of pendingTransactions; track pendingTransaction.id; let i = $index) { | ||
| @for (pendingTransaction of pendingTransactions; track pendingTransaction; let i = $index) { |
There was a problem hiding this comment.
id does not exist for pending expenses
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
src/app/fyle/my-expenses/my-expenses.page.html (1)
224-237: Track by a stable key, not object identity.Tracking the whole object will re-render on every refresh since outbox items are re-materialized. Track by index for offline list. One shot, perfect shot.
-@for (pendingTransaction of pendingTransactions; track pendingTransaction; let i = $index) { +@for (pendingTransaction of pendingTransactions; track $index; let i = $index) {src/app/core/services/transactions-outbox.service.ts (1)
198-205: Beware negative indexes in deleteBulkOfflineExpenses
Filter out-1values before deletion or you will blast the wrong queue item, mind it!- deleteBulkOfflineExpenses(pendingTransactions: Partial<Transaction>[], deleteExpenses: Partial<Transaction>[]): void { - const indexes = deleteExpenses.map((offlineExpense) => indexOf(pendingTransactions, offlineExpense)); + deleteBulkOfflineExpenses(pendingTransactions: Partial<Transaction>[], deleteExpenses: Partial<Transaction>[]): void { + const indexes = deleteExpenses + .map((offlineExpense) => indexOf(pendingTransactions, offlineExpense)) + .filter((i) => i > -1);Also update the unit test in
transactions-outbox.service.spec.tsto remove the expectation fordeleteOfflineExpense(-1)and adjust the call count accordingly.src/app/fyle/my-expenses/my-expenses.page.ts (2)
902-919: Call sync once, not N times.
from(this.pendingTransactions).pipe(switchMap(() => sync()))starts multiple syncs and cancels mid-flight; unnecessary and brittle. Keep it simple, keep it stylish.syncOutboxExpenses(): void { - this.pendingTransactions = this.transactionOutboxService.getPendingTransactions(); - if (this.pendingTransactions.length > 0) { - this.syncing = true; - from(this.pendingTransactions) - .pipe( - switchMap(() => from(this.transactionOutboxService.sync())), - finalize(() => { - this.syncing = false; - const pendingTransactions = this.transactionOutboxService.getPendingTransactions(); - if (pendingTransactions.length === 0) { - this.doRefresh(); - this.pendingTransactions = []; - } - }), - ) - .subscribe(noop); - } + this.pendingTransactions = this.transactionOutboxService.getPendingTransactions(); + if (this.pendingTransactions.length > 0) { + this.syncing = true; + from(this.transactionOutboxService.sync()) + .pipe( + finalize(() => { + this.syncing = false; + const pendingTransactions = this.transactionOutboxService.getPendingTransactions(); + if (pendingTransactions.length === 0) { + this.doRefresh(); + this.pendingTransactions = []; + } + }), + ) + .subscribe(noop); + } }
1639-1651: Bulk delete flow OK, but return type is misleading.Offline path returns
of(null)though the method isObservable<void>. Minor, but tighten it.- deleteSelectedExpenses(offlineExpenses: Partial<Transaction>[]): Observable<void> { + deleteSelectedExpenses(offlineExpenses: Partial<Transaction>[]): Observable<void> { if (offlineExpenses?.length > 0) { this.transactionOutboxService.deleteBulkOfflineExpenses(this.pendingTransactions, offlineExpenses); - return of(null); + return of(void 0); } else { ... } }src/app/shared/components/expenses-card/expenses-card.component.html (2)
47-49: Typo in isSycing$ — rename to isSyncing$ for sanity. Style-aa irukku, clarity-um venum.Minor, but improves readability and greppability. Update TS and template together.
Also applies to: 108-110
106-106: CSS class typo: deatils → details.Tiny nit; fix if CSS allows.
src/app/shared/components/expenses-card/expenses-card.component.ts (2)
184-191: Update getter to read InputSignal. Semma smooth.get isSelected(): boolean { - if (this.selectedElements) { - if (this.expense.id) { - return this.selectedElements.some((txn) => this.expense.id === txn.id); + const selected = this.selectedElements(); + if (selected) { + if (this.expense?.id) { + return selected.some((txn) => this.expense.id === txn.id); } else { - return this.selectedElements.some((txn) => isEqual(this.expense, txn)); + return selected.some((txn) => isEqual(this.expense, txn)); } } return false; }
201-215: Receipt indicator logic: empty array treated as truthy — fix check, thalaiva.[] is truthy; current code marks receipt present even when none.
- } else { - if (!this.expense.file_ids) { + } else { + if (!this.expense.file_ids || this.expense.file_ids.length === 0) { this.receiptIcon = 'assets/svg/list-plus.svg'; if (this.isFromPotentialDuplicates || this.isFromViewReports) { this.receiptIcon = 'assets/svg/list.svg'; } } else { this.isReceiptPresent = true; } }src/app/core/services/transaction.service.ts (1)
293-310: Per diem vendor string can crash when num_days is undefined — guard it. Nallaa handle pannunga.- } else if (systemCategory === 'per diem') { - vendorDisplayName = expense.num_days.toString(); - if (expense.num_days > 1) { + } else if (systemCategory === 'per diem') { + const days = expense.num_days ?? 0; + vendorDisplayName = days.toString(); + if (days > 1) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/app/core/mock-data/dependent-field-expenses.data.ts(0 hunks)src/app/core/mock-data/expense.data.ts(0 hunks)src/app/core/models/expense.model.ts(0 hunks)src/app/core/models/v1/transaction.model.ts(2 hunks)src/app/core/services/custom-inputs.service.spec.ts(0 hunks)src/app/core/services/transaction.service.spec.ts(0 hunks)src/app/core/services/transaction.service.ts(6 hunks)src/app/core/services/transactions-outbox.service.ts(1 hunks)src/app/fyle/my-expenses/my-expenses.page.html(1 hunks)src/app/fyle/my-expenses/my-expenses.page.ts(7 hunks)src/app/shared/components/expenses-card/expenses-card.component.html(8 hunks)src/app/shared/components/expenses-card/expenses-card.component.ts(16 hunks)src/app/shared/components/review-footer/review-footer.component.spec.ts(1 hunks)
💤 Files with no reviewable changes (5)
- src/app/core/services/custom-inputs.service.spec.ts
- src/app/core/services/transaction.service.spec.ts
- src/app/core/mock-data/expense.data.ts
- src/app/core/models/expense.model.ts
- src/app/core/mock-data/dependent-field-expenses.data.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{service,pipe,directive}.ts
📄 CodeRabbit inference engine (.cursor/rules/atomics-i18n-key-pattern.mdc)
**/*.{service,pipe,directive}.ts: i18n keys for services, pipes, and directives must use the schema: services.., pipes.., directives..
Convert kebab-case file names to camelCase and remove the suffix (Service, Pipe, Directive) when generating i18n keys for services, pipes, and directives
Use only the three roots: services, pipes, directives for i18n key grouping
Use meaningful, semantic, and context-aware key names for i18n keys; avoid generic names like message1, text1, error1
Files:
src/app/core/services/transactions-outbox.service.tssrc/app/core/services/transaction.service.ts
**/*.component.{ts,html}
📄 CodeRabbit inference engine (.cursor/rules/component-i18n-key-naming.mdc)
**/*.component.{ts,html}: Top-level object in i18n translation files must be the component or feature folder name, derived from the file name by converting kebab-case to camelCase and removing the first matching prefix from: 'feature', 'ui', 'component'.
Keys inside the top-level object in i18n translation files must be free-form, not prefixed, and there must be no nested objects beyond the first level.
Use meaningful, semantic, and context-aware key names for i18n keys; avoid generic names like 'label1', 'text1', or 'message1'.
Do not translate strings that are already translation keys, tracking/analytics strings, strings with only special characters, comments, non-user-visible text, or variable bindings and expressions.
One object per component or feature in i18n translation files; avoid deep nesting and avoid prefixes like 'ui*' or 'feature*' in object names.
Files:
src/app/shared/components/expenses-card/expenses-card.component.htmlsrc/app/shared/components/expenses-card/expenses-card.component.ts
src/**/*.component.{ts,html}
📄 CodeRabbit inference engine (.cursor/rules/component-i18n-key-naming.mdc)
Translation keys for Angular components and templates located in 'src/**' must be placed in 'src/assets/i18n/{lang}.json' for every supported language.
Files:
src/app/shared/components/expenses-card/expenses-card.component.htmlsrc/app/shared/components/expenses-card/expenses-card.component.ts
**/*.component.html
📄 CodeRabbit inference engine (.cursor/rules/component-i18n-key-naming.mdc)
Prefer property binding for user-facing attributes in Angular templates when using translation keys.
Files:
src/app/shared/components/expenses-card/expenses-card.component.html
🧠 Learnings (4)
📚 Learning: 2024-11-18T06:55:17.972Z
Learnt from: sumrender
PR: fylein/fyle-mobile-app#3265
File: src/app/fyle/personal-cards-matched-expenses/expense-preview/expense-preview.component.spec.ts:35-37
Timestamp: 2024-11-18T06:55:17.972Z
Learning: In `src/app/fyle/add-edit-expense/add-edit-expense.page.ts`, the `unmatchExpense` method is used for corporate card expenses, not for personal card expenses.
Applied to files:
src/app/fyle/my-expenses/my-expenses.page.htmlsrc/app/shared/components/expenses-card/expenses-card.component.htmlsrc/app/fyle/my-expenses/my-expenses.page.tssrc/app/shared/components/expenses-card/expenses-card.component.tssrc/app/core/services/transaction.service.ts
📚 Learning: 2024-10-23T11:45:16.974Z
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:293-354
Timestamp: 2024-10-23T11:45:16.974Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', the flags 'isVisaRTFEnabled$', 'isMastercardRTFEnabled$', and 'isYodleeEnabled$' do not have rapidly changing values, so tests for rapid changes in these flags are unnecessary.
Applied to files:
src/app/shared/components/review-footer/review-footer.component.spec.ts
📚 Learning: 2024-11-04T13:10:36.868Z
Learnt from: arjunaj5
PR: fylein/fyle-mobile-app#3242
File: src/app/fyle/my-expenses/my-expenses.page.spec.ts:967-968
Timestamp: 2024-11-04T13:10:36.868Z
Learning: In `src/app/fyle/my-expenses/my-expenses.page.spec.ts`, when testing private methods like `filterDEIncompleteExpenses`, it's acceptable to use `//ts-ignore` to spy on them in unit tests.
Applied to files:
src/app/shared/components/review-footer/review-footer.component.spec.ts
📚 Learning: 2024-10-23T08:40:59.542Z
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:200-200
Timestamp: 2024-10-23T08:40:59.542Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', moving 'component.isAddCorporateCardVisible$ = of(true);' to the top-level beforeEach block causes tests to fail, so mock values need to be set in individual tests.
Applied to files:
src/app/shared/components/review-footer/review-footer.component.spec.ts
🧬 Code graph analysis (5)
src/app/core/services/transactions-outbox.service.ts (1)
src/app/core/models/v1/transaction.model.ts (1)
Transaction(8-142)
src/app/core/models/v1/transaction.model.ts (1)
src/app/core/models/platform/v1/account.model.ts (1)
Account(1-12)
src/app/fyle/my-expenses/my-expenses.page.ts (1)
src/app/core/models/v1/transaction.model.ts (1)
Transaction(8-142)
src/app/shared/components/expenses-card/expenses-card.component.ts (1)
src/app/core/models/v1/transaction.model.ts (1)
Transaction(8-142)
src/app/core/services/transaction.service.ts (1)
src/app/core/models/v1/transaction.model.ts (1)
Transaction(8-142)
🪛 GitHub Check: Run linters
src/app/shared/components/expenses-card/expenses-card.component.ts
[failure] 76-76:
Use InputSignals (e.g. via input()) for Component input properties rather than the legacy @Input() decorator
[failure] 92-92:
Use InputSignals (e.g. via input()) for Component input properties rather than the legacy @Input() decorator
[failure] 151-151:
Expected blank line between class members
[failure] 150-150:
Expected blank line between class members
🪛 GitHub Actions: Lint
src/app/shared/components/expenses-card/expenses-card.component.ts
[error] 76-76: lint:check failed: ESLint: Use InputSignals (e.g. via input()) for Component input properties rather than the legacy @Input() decorator. (@angular-eslint/prefer-signals)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
src/app/shared/components/review-footer/review-footer.component.spec.ts (2)
3-3: Import cleanup LGTM.Removing IonButton import is correct since the spec doesn’t reference it.
108-119: Replace stale mock with apiExpenseRes in review-footer spec
Replace deletedexpensesWithDependentFieldswithapiExpenseResand setactiveIndextoapiExpenseRes.length - 1. Tests will rock like a Rajini punch!- reviewFooterComponent.activeIndex = 1; - reviewFooterComponent.reviewList = expensesWithDependentFields; + reviewFooterComponent.reviewList = apiExpenseRes; + reviewFooterComponent.activeIndex = apiExpenseRes.length - 1;src/app/core/models/v1/transaction.model.ts (1)
4-4: LGTM: source_account added with correct type.Import path is right;
Pick<Account, 'id' | 'type'>is precise. Superstar move.Also applies to: 71-71
src/app/fyle/my-expenses/my-expenses.page.ts (1)
134-134: LGTM: type migration to Partial.Consistent with the model refactor. Rock solid.
Also applies to: 140-140, 195-195
src/app/shared/components/expenses-card/expenses-card.component.ts (1)
76-76: Verify mutation ofexpenseandselectedElementsmigration
expense: Partial<Transaction>is mutated (e.g. updatingfile_ids), so either keep the@Input()withor refactor to an immutable pattern before adopting signals.// eslint-disable-next-line @angular-eslint/prefer-signals @Input() expense: Partial<Transaction>;selectedElements: Partial<Transaction>[]is read-only — safe to convert to anInputSignal, for example:readonly selectedElements = input<Partial<Transaction>[]>([]);Confirm in your component that
expenseis indeed mutated and adjust accordingly, ha!src/app/core/services/transaction.service.ts (3)
322-333: State helpers OK — but consider strict equality with nullish coalescing for clarity. Rock solid already.
589-596: source_account passthrough LGTM.Mapping aligns with Transaction model usage in the component.
636-662: Confirm backend contract for tx_ filters*
Frontend still uses tx_* query params (tx_report_id, tx_state, tx_policy_amount via FilterQueryParams and GetExpensesQueryParams). Ensure backend API still accepts these after the refactor—no surprises, superstar!
| isMergeAllowed(expenses: Partial<Transaction>[]): boolean { | ||
| if (expenses.length === 2) { | ||
| const areSomeMileageOrPerDiemExpenses = expenses.some( | ||
| (expense) => expense.tx_fyle_category === 'Mileage' || expense.tx_fyle_category === 'Per Diem', | ||
| (expense) => expense.category?.system_category === 'Mileage' || expense.category?.system_category === 'Per Diem', | ||
| ); | ||
| const areAllExpensesSubmitted = expenses.every((expense) => | ||
| ['APPROVER_PENDING', 'APPROVED', 'PAYMENT_PENDING', 'PAYMENT_PROCESSING', 'PAID'].includes(expense.tx_state), | ||
| ['APPROVER_PENDING', 'APPROVED', 'PAYMENT_PENDING', 'PAYMENT_PROCESSING', 'PAID'].includes(expense.state), | ||
| ); | ||
| const areAllCCCMatchedExpenses = expenses.every((expense) => expense.tx_corporate_credit_card_expense_group_id); | ||
| const areAllCCCMatchedExpenses = expenses.every((expense) => expense.corporate_credit_card_expense_group_id); | ||
| return !areSomeMileageOrPerDiemExpenses && !areAllExpensesSubmitted && !areAllCCCMatchedExpenses; | ||
| } else { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Merge rules look tight. One small ask: guard state undefined for includes().
Minor, optional.
- const areAllExpensesSubmitted = expenses.every((expense) =>
- ['APPROVER_PENDING', 'APPROVED', 'PAYMENT_PENDING', 'PAYMENT_PROCESSING', 'PAID'].includes(expense.state),
- );
+ const submittedStates = ['APPROVER_PENDING', 'APPROVED', 'PAYMENT_PENDING', 'PAYMENT_PROCESSING', 'PAID'] as const;
+ const areAllExpensesSubmitted = expenses.every((expense) => !!expense.state && submittedStates.includes(expense.state));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isMergeAllowed(expenses: Partial<Transaction>[]): boolean { | |
| if (expenses.length === 2) { | |
| const areSomeMileageOrPerDiemExpenses = expenses.some( | |
| (expense) => expense.tx_fyle_category === 'Mileage' || expense.tx_fyle_category === 'Per Diem', | |
| (expense) => expense.category?.system_category === 'Mileage' || expense.category?.system_category === 'Per Diem', | |
| ); | |
| const areAllExpensesSubmitted = expenses.every((expense) => | |
| ['APPROVER_PENDING', 'APPROVED', 'PAYMENT_PENDING', 'PAYMENT_PROCESSING', 'PAID'].includes(expense.tx_state), | |
| ['APPROVER_PENDING', 'APPROVED', 'PAYMENT_PENDING', 'PAYMENT_PROCESSING', 'PAID'].includes(expense.state), | |
| ); | |
| const areAllCCCMatchedExpenses = expenses.every((expense) => expense.tx_corporate_credit_card_expense_group_id); | |
| const areAllCCCMatchedExpenses = expenses.every((expense) => expense.corporate_credit_card_expense_group_id); | |
| return !areSomeMileageOrPerDiemExpenses && !areAllExpensesSubmitted && !areAllCCCMatchedExpenses; | |
| } else { | |
| isMergeAllowed(expenses: Partial<Transaction>[]): boolean { | |
| if (expenses.length === 2) { | |
| const areSomeMileageOrPerDiemExpenses = expenses.some( | |
| (expense) => | |
| expense.category?.system_category === 'Mileage' || | |
| expense.category?.system_category === 'Per Diem', | |
| ); | |
| const submittedStates = [ | |
| 'APPROVER_PENDING', | |
| 'APPROVED', | |
| 'PAYMENT_PENDING', | |
| 'PAYMENT_PROCESSING', | |
| 'PAID', | |
| ] as const; | |
| const areAllExpensesSubmitted = expenses.every( | |
| (expense) => !!expense.state && submittedStates.includes(expense.state), | |
| ); | |
| const areAllCCCMatchedExpenses = expenses.every( | |
| (expense) => expense.corporate_credit_card_expense_group_id, | |
| ); | |
| return ( | |
| !areSomeMileageOrPerDiemExpenses && | |
| !areAllExpensesSubmitted && | |
| !areAllCCCMatchedExpenses | |
| ); | |
| } else { | |
| // ... |
🤖 Prompt for AI Agents
In src/app/core/services/transaction.service.ts around lines 413 to 423, the
expenses.every(...) call uses includes(expense.state) without guarding for
undefined state; update that predicate to handle missing or undefined state
(e.g., check expense.state != null && ['APPROVER_PENDING',
...].includes(expense.state) or use ['...'].includes(expense.state ?? '') ) so
includes always receives a string and avoids runtime errors; make this one-line
change and keep the rest of the logic intact.
| setOutboxExpenseStatsOnSelect(): void { | ||
| this.allExpensesStats$ = of({ | ||
| count: this.selectedOutboxExpenses.length, | ||
| amount: this.selectedOutboxExpenses.reduce((acc, txnObj) => acc + txnObj.tx_amount, 0), | ||
| amount: this.selectedOutboxExpenses.reduce((acc, exp) => acc + exp.amount, 0), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Null-safe sum for optional amount.
amount is optional; 0 + undefined => NaN. Add a fallback, keep the swagger.
- this.allExpensesStats$ = of({
- count: this.selectedOutboxExpenses.length,
- amount: this.selectedOutboxExpenses.reduce((acc, exp) => acc + exp.amount, 0),
- });
+ this.allExpensesStats$ = of({
+ count: this.selectedOutboxExpenses.length,
+ amount: this.selectedOutboxExpenses.reduce((acc, exp) => acc + (exp.amount ?? 0), 0),
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setOutboxExpenseStatsOnSelect(): void { | |
| this.allExpensesStats$ = of({ | |
| count: this.selectedOutboxExpenses.length, | |
| amount: this.selectedOutboxExpenses.reduce((acc, txnObj) => acc + txnObj.tx_amount, 0), | |
| amount: this.selectedOutboxExpenses.reduce((acc, exp) => acc + exp.amount, 0), | |
| }); | |
| } | |
| setOutboxExpenseStatsOnSelect(): void { | |
| this.allExpensesStats$ = of({ | |
| count: this.selectedOutboxExpenses.length, | |
| amount: this.selectedOutboxExpenses.reduce( | |
| (acc, exp) => acc + (exp.amount ?? 0), | |
| 0 | |
| ), | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/app/fyle/my-expenses/my-expenses.page.ts around lines 1097 to 1102, the
reducer sums optional expense.amount values which can be undefined and yield
NaN; update the reduce call to treat missing amounts as 0 (for example use acc +
(exp.amount ?? 0)) while keeping the existing swagger typing and initial
accumulator as 0 so the sum remains null-safe.
| let isSelectedElementsIncludesExpense = false; | ||
| if (expense.tx_id) { | ||
| isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((txn) => expense.tx_id === txn.tx_id); | ||
| if (expense.id) { | ||
| isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((exp) => expense.id === exp.id); | ||
| } else { | ||
| isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((txn) => isEqual(txn, expense)); | ||
| isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((exp) => isEqual(exp, expense)); | ||
| } | ||
|
|
||
| if (isSelectedElementsIncludesExpense) { | ||
| if (expense.tx_id) { | ||
| this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((txn) => txn.tx_id !== expense.tx_id); | ||
| if (expense.id) { | ||
| this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((exp) => exp.id !== expense.id); | ||
| } else { | ||
| this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((txn) => !isEqual(txn, expense)); | ||
| this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((exp) => !isEqual(exp, expense)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Selection by deep-equal is O(n²) and brittle; prefer a stable local id.
Offline txns lack id, so isEqual works but is heavy and fragile if objects rehydrate. Add a stable local_id to outbox entries and use it here. Mass save, mass style.
In TransactionsOutboxService.addEntry (separate file), set a local id:
- addEntry(
- transaction: Partial<Transaction>,
+ addEntry(
+ transaction: Partial<Transaction>,
dataUrls: { url: string; type: string }[],
comments?: string[],
): Promise<void> {
- this.queue.push({
+ const localId = (crypto as any)?.randomUUID?.() ?? Date.now().toString();
+ this.queue.push({
- transaction,
+ transaction: { ...transaction, local_id: localId } as any,
dataUrls,
comments,
});
return this.saveQueue();
}Then here:
- if (expense.id) {
+ if (expense.id ?? (expense as any).local_id) {
- isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((exp) => expense.id === exp.id);
+ const key = expense.id ?? (expense as any).local_id;
+ isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some(
+ (exp) => (exp.id ?? (exp as any).local_id) === key,
+ );
} else {
isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((exp) => isEqual(exp, expense));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let isSelectedElementsIncludesExpense = false; | |
| if (expense.tx_id) { | |
| isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((txn) => expense.tx_id === txn.tx_id); | |
| if (expense.id) { | |
| isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((exp) => expense.id === exp.id); | |
| } else { | |
| isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((txn) => isEqual(txn, expense)); | |
| isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((exp) => isEqual(exp, expense)); | |
| } | |
| if (isSelectedElementsIncludesExpense) { | |
| if (expense.tx_id) { | |
| this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((txn) => txn.tx_id !== expense.tx_id); | |
| if (expense.id) { | |
| this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((exp) => exp.id !== expense.id); | |
| } else { | |
| this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((txn) => !isEqual(txn, expense)); | |
| this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((exp) => !isEqual(exp, expense)); | |
| } | |
| let isSelectedElementsIncludesExpense = false; | |
| - if (expense.id) { | |
| if (expense.id ?? (expense as any).local_id) { | |
| const key = expense.id ?? (expense as any).local_id; | |
| isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some( | |
| (exp) => (exp.id ?? (exp as any).local_id) === key, | |
| ); | |
| } else { | |
| isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((exp) => isEqual(exp, expense)); | |
| } | |
| if (isSelectedElementsIncludesExpense) { | |
| if (expense.id) { | |
| this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((exp) => exp.id !== expense.id); | |
| } else { | |
| this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((exp) => !isEqual(exp, expense)); | |
| } |
🤖 Prompt for AI Agents
In src/app/fyle/my-expenses/my-expenses.page.ts around lines 1105 to 1117,
replace the deep-equal based selection logic with stable local_id comparison:
check expense.local_id (fall back to id if needed) against
selectedOutboxExpenses' local_id (or id) when testing inclusion, and filter by
that same local_id when removing; assume TransactionsOutboxService.addEntry now
sets a local_id on outbox entries so use local_id for offline entries to avoid
O(n²) isEqual calls and brittle rehydration issues.
| @if (expense.id && data.isConnected) { | ||
| {{ expense.spent_at || expense.created_at | dateFormat }} | ||
| } @else { |
There was a problem hiding this comment.
Date pipe precedence bug — wrap with parentheses, boss.
Without parentheses, dateFormat applies only to created_at. You want it on the coalesced value.
- {{ expense.spent_at || expense.created_at | dateFormat }}
+ {{ (expense.spent_at || expense.created_at) | dateFormat }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @if (expense.id && data.isConnected) { | |
| {{ expense.spent_at || expense.created_at | dateFormat }} | |
| } @else { | |
| @if (expense.id && data.isConnected) { | |
| {{ (expense.spent_at || expense.created_at) | dateFormat }} | |
| } @else { |
🤖 Prompt for AI Agents
In src/app/shared/components/expenses-card/expenses-card.component.html around
lines 4 to 6, the dateFormat pipe is currently only applied to created_at due to
operator precedence; wrap the coalesced expression in parentheses so the pipe
receives the entire value (expense.spent_at || expense.created_at) and not just
created_at, ensuring the selected date is formatted correctly.
| expense.dataUrls?.length > 0 && | ||
| imageTransperencyOverlay + 'url(' + '../../../../assets/images/pdf-receipt-placeholder.png' + ')', | ||
| }" | ||
| > |
There was a problem hiding this comment.
Using non-existent expense.dataUrls — switch to file_ids or receipt flags. Kaboom avoided.
Transaction model doesn’t expose dataUrls. These checks will always be falsy and break UI.
- expense.dataUrls?.length > 0 &&
+ (expense.file_ids?.length ?? 0) > 0 &&- expense.dataUrls?.length > 0 &&
+ (expense.file_ids?.length ?? 0) > 0 &&- @if (!expense.dataUrls?.length) {
+ @if (!(expense.file_ids?.length)) {Also applies to: 65-69, 69-71
| that.isScanInProgress = !that.isScanCompleted && !this.expense.extracted_data; | ||
| } else { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
this vs that inconsistency — stick to this. Clean like a vintage entry.
- that.isScanInProgress = !that.isScanCompleted && !this.expense.extracted_data;
+ that.isScanInProgress = !that.isScanCompleted && !that.expense.extracted_data;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| that.isScanInProgress = !that.isScanCompleted && !this.expense.extracted_data; | |
| } else { | |
| that.isScanInProgress = !that.isScanCompleted && !that.expense.extracted_data; | |
| } else { |
🤖 Prompt for AI Agents
In src/app/shared/components/expenses-card/expenses-card.component.ts around
lines 251-252, there's an inconsistent use of the legacy alias "that" alongside
"this"; change the code to use "this.isScanInProgress" instead of
"that.isScanInProgress" and remove the "that" alias; if the surrounding callback
is a regular function causing "this" to be lost, convert the function to an
arrow function (or explicitly bind the correct context) so "this" refers to the
component instance, then update any other occurrences of "that" in the same
scope and run lint/tests to confirm no regressions.
| canShowPaymentModeIcon(): void { | ||
| this.showPaymentModeIcon = | ||
| this.expense.source_account_type === AccountType.PERSONAL && !this.expense.tx_skip_reimbursement; | ||
| this.expense.source_account?.type === AccountType.PERSONAL_CASH_ACCOUNT && !this.expense.skip_reimbursement; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
AccountType source mismatch — verify enum and values; might be wrong constant. Mass check pannunga.
Component imports AccountType from platform/v1/account.model, service uses ../enums/account-type.enum. Also constants PERSONAL_CASH_ACCOUNT and PERSONAL_CORPORATE_CREDIT_CARD_ACCOUNT need confirmation.
If mismatch, import AccountType from the single canonical source and use the correct members.
Also applies to: 320-333
🏁 Script executed:
#!/bin/bash
# Verify AccountType definitions and usages
rg -nP --type=ts -C2 "export enum AccountType|type\s*:\s*['\"]?(PERSONAL|PERSONAL_CASH_ACCOUNT|PERSONAL_CORPORATE_CREDIT_CARD_ACCOUNT)['\"]?" src/app | sed -n '1,200p'
rg -nP --type=ts -C2 "\bAccountType\b" src/app/core | sed -n '1,200p'Length of output: 28231
Unify AccountType import and correct enum members
- In
expenses-card.component.ts, importAccountTypefromsrc/app/core/enums/account-type.enum.ts(notplatform/v1/account.model.ts). - Replace
AccountType.PERSONAL_CASH_ACCOUNTwithAccountType.PERSONALandAccountType.PERSONAL_CORPORATE_CREDIT_CARD_ACCOUNTwithAccountType.CCC(or the matching keys in the unified enum). - Remove the duplicate
AccountTypedefinition inplatform/v1/account.model.tsand ensure all modules reference the single canonical enum.
🤖 Prompt for AI Agents
In src/app/shared/components/expenses-card/expenses-card.component.ts around
lines 260 to 263, the component currently uses a non-canonical AccountType and
incorrect enum members; update the import to pull AccountType from
src/app/core/enums/account-type.enum.ts instead of platform/v1/account.model.ts,
replace AccountType.PERSONAL_CASH_ACCOUNT with AccountType.PERSONAL (and any
AccountType.PERSONAL_CORPORATE_CREDIT_CARD_ACCOUNT usages with AccountType.CCC
or the matching key in the core enum), and then remove the duplicate AccountType
definition from platform/v1/account.model.ts and update any other modules still
importing the duplicate to reference the single canonical enum.
| this.isMileageExpense = this.expense.category?.name?.toLowerCase() === 'mileage'; | ||
| this.isPerDiem = this.expense.category?.name?.toLowerCase() === 'per diem'; | ||
|
|
||
| this.category = this.expense.tx_org_category?.toLowerCase(); | ||
| this.expense.isDraft = this.transactionService.getIsDraft(this.expense); | ||
| this.expense.isPolicyViolated = this.expense.tx_policy_flag; | ||
| this.expense.isCriticalPolicyViolated = this.transactionService.getIsCriticalPolicyViolated(this.expense); | ||
| this.expense.vendorDetails = this.transactionService.getVendorDetails(this.expense); | ||
| this.category = this.expense.category?.name?.toLowerCase(); | ||
| this.isDraft = this.transactionService.getIsDraft(this.expense); |
There was a problem hiding this comment.
Mileage/Per Diem detection should use system_category, not name. Get the category right, superstar.
Name is user-defined; system_category is canonical.
- this.isMileageExpense = this.expense.category?.name?.toLowerCase() === 'mileage';
- this.isPerDiem = this.expense.category?.name?.toLowerCase() === 'per diem';
- this.category = this.expense.category?.name?.toLowerCase();
+ this.isMileageExpense = this.expense.category?.system_category === 'Mileage';
+ this.isPerDiem = this.expense.category?.system_category === 'Per Diem';
+ this.category = this.expense.category?.system_category?.toLowerCase();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.isMileageExpense = this.expense.category?.name?.toLowerCase() === 'mileage'; | |
| this.isPerDiem = this.expense.category?.name?.toLowerCase() === 'per diem'; | |
| this.category = this.expense.tx_org_category?.toLowerCase(); | |
| this.expense.isDraft = this.transactionService.getIsDraft(this.expense); | |
| this.expense.isPolicyViolated = this.expense.tx_policy_flag; | |
| this.expense.isCriticalPolicyViolated = this.transactionService.getIsCriticalPolicyViolated(this.expense); | |
| this.expense.vendorDetails = this.transactionService.getVendorDetails(this.expense); | |
| this.category = this.expense.category?.name?.toLowerCase(); | |
| this.isDraft = this.transactionService.getIsDraft(this.expense); | |
| this.isMileageExpense = this.expense.category?.system_category === 'Mileage'; | |
| this.isPerDiem = this.expense.category?.system_category === 'Per Diem'; | |
| this.category = this.expense.category?.system_category?.toLowerCase(); | |
| this.isDraft = this.transactionService.getIsDraft(this.expense); |
🤖 Prompt for AI Agents
In src/app/shared/components/expenses-card/expenses-card.component.ts around
lines 273 to 277, the code currently detects mileage/per diem by inspecting
expense.category.name (user-editable); change it to use the canonical
expense.category.system_category instead: set this.isMileageExpense =
this.expense.category?.system_category?.toLowerCase() === 'mileage' and
this.isPerDiem = this.expense.category?.system_category?.toLowerCase() === 'per
diem', and set this.category =
this.expense.category?.system_category?.toLowerCase() (falling back to undefined
if absent) while leaving the isDraft assignment unchanged.
| canAddAttachment(): boolean { | ||
| return ( | ||
| !this.isFromViewReports && | ||
| !(this.isMileageExpense || this.isPerDiem || this.expense.tx_file_ids || this.isFromPotentialDuplicates) && | ||
| !(this.isMileageExpense || this.isPerDiem || this.expense.file_ids || this.isFromPotentialDuplicates) && | ||
| !this.isSelectionModeEnabled | ||
| ); | ||
| } |
There was a problem hiding this comment.
Attachment add check should test length; array truthiness bites. Padam super after this.
- !(this.isMileageExpense || this.isPerDiem || this.expense.file_ids || this.isFromPotentialDuplicates) &&
+ !(this.isMileageExpense ||
+ this.isPerDiem ||
+ (this.expense.file_ids?.length ?? 0) > 0 ||
+ this.isFromPotentialDuplicates) &&📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| canAddAttachment(): boolean { | |
| return ( | |
| !this.isFromViewReports && | |
| !(this.isMileageExpense || this.isPerDiem || this.expense.tx_file_ids || this.isFromPotentialDuplicates) && | |
| !(this.isMileageExpense || this.isPerDiem || this.expense.file_ids || this.isFromPotentialDuplicates) && | |
| !this.isSelectionModeEnabled | |
| ); | |
| } | |
| canAddAttachment(): boolean { | |
| return ( | |
| !this.isFromViewReports && | |
| !(this.isMileageExpense || | |
| this.isPerDiem || | |
| (this.expense.file_ids?.length ?? 0) > 0 || | |
| this.isFromPotentialDuplicates) && | |
| !this.isSelectionModeEnabled | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/app/shared/components/expenses-card/expenses-card.component.ts around
lines 348 to 354, the canAddAttachment() check treats expense.file_ids as a
boolean which is incorrect for arrays; update the condition to test the array
length (e.g. use !this.expense.file_ids?.length or
!(Array.isArray(this.expense.file_ids) && this.expense.file_ids.length > 0)) so
existing logic still prevents adding attachments when file_ids is a non-empty
array, while preserving the other flags.
| matchReceiptWithEtxn(fileObj: FileObject): void { | ||
| this.expense.tx_file_ids = []; | ||
| this.expense.tx_file_ids.push(fileObj.id); | ||
| fileObj.transaction_id = this.expense.tx_id; | ||
| this.expense.file_ids = []; | ||
| this.expense.file_ids.push(fileObj.id); | ||
| fileObj.transaction_id = this.expense.id; | ||
| } |
There was a problem hiding this comment.
Don’t nuke existing file_ids — push safely. Semma careful.
- this.expense.file_ids = [];
- this.expense.file_ids.push(fileObj.id);
+ this.expense.file_ids = [...(this.expense.file_ids ?? []), fileObj.id];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| matchReceiptWithEtxn(fileObj: FileObject): void { | |
| this.expense.tx_file_ids = []; | |
| this.expense.tx_file_ids.push(fileObj.id); | |
| fileObj.transaction_id = this.expense.tx_id; | |
| this.expense.file_ids = []; | |
| this.expense.file_ids.push(fileObj.id); | |
| fileObj.transaction_id = this.expense.id; | |
| } | |
| matchReceiptWithEtxn(fileObj: FileObject): void { | |
| - this.expense.file_ids = []; | |
| this.expense.file_ids = [...(this.expense.file_ids ?? []), fileObj.id]; | |
| fileObj.transaction_id = this.expense.id; | |
| } |
🤖 Prompt for AI Agents
In src/app/shared/components/expenses-card/expenses-card.component.ts around
lines 442 to 446, the current code clears expense.file_ids before adding the new
file and thereby nukes existing attachments; change it to safely append the new
file id: ensure expense and expense.file_ids exist (initialize to an array if
missing), push the new fileObj.id only if it’s not already present to avoid
duplicates, and then set fileObj.transaction_id = this.expense.id; also handle
the case where this.expense may be null/undefined by returning early or throwing
as appropriate.
|
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
src/app/core/services/transactions-outbox.service.ts (2)
192-196: Guard against negative index; avoid deleting the last entry by mistake.If index is -1, splice(-1, 1) nukes the last item. Not today, boss.
- deleteOfflineExpense(index: number): null { - this.queue.splice(index, 1); - this.saveQueue(); - return null; - } + deleteOfflineExpense(index: number): null { + if (index < 0 || index >= this.queue.length) { + return null; + } + this.queue.splice(index, 1); + this.saveQueue(); + return null; + }
198-205: indexOf on objects will miss; compute indexes by id/local_id and ignore -1.Using indexOf on cloned objects returns -1; sorting and deleting -1 will pop the last entry. Kaboom. Match by id or local_id; filter out misses.
- deleteBulkOfflineExpenses(pendingTransactions: Partial<Transaction>[], deleteExpenses: Partial<Transaction>[]): void { - const indexes = deleteExpenses.map((offlineExpense) => indexOf(pendingTransactions, offlineExpense)); - // We need to delete last element of this list first - indexes.sort((a, b) => b - a); - indexes.forEach((index) => { - this.deleteOfflineExpense(index); - }); - } + deleteBulkOfflineExpenses(pending: Partial<Transaction>[], toDelete: Partial<Transaction>[]): void { + const keyOf = (t: Partial<Transaction>) => (t.id as string) ?? ((t as any).local_id as string) ?? null; + const indexes = toDelete + .map((item) => { + const k = keyOf(item); + if (k) { + return pending.findIndex((p) => keyOf(p) === k); + } + // Best-effort fallback for truly keyless items + return pending.findIndex( + (p) => + p.amount === item.amount && + p.currency === item.currency && + (p.spent_at as any)?.toString?.() === (item.spent_at as any)?.toString?.() + ); + }) + .filter((i) => i >= 0) + .sort((a, b) => b - a); + indexes.forEach((i) => this.deleteOfflineExpense(i)); + }Also remove the now-unused lodash indexOf import.
src/app/core/services/transactions-outbox.service.spec.ts (1)
233-240: Fix brittle expectation: don’t expect deletion with -1.With robust key-based matching, -1 should never be used. Update the test to derive deleteExpenses from getPendingExpenses() and assert only valid indexes are deleted.
- it('deleteBulkOfflineExpenses(): should delete bulk offline expenses', () => { - spyOn(transactionsOutboxService, 'deleteOfflineExpense').and.returnValues(null); - const pendingTransactions = [expenseList2[0]]; - const deleteExpenses = expenseList2; - transactionsOutboxService.deleteBulkOfflineExpenses(pendingTransactions, deleteExpenses); - expect(transactionsOutboxService.deleteOfflineExpense).toHaveBeenCalledTimes(2); - expect(transactionsOutboxService.deleteOfflineExpense).toHaveBeenCalledWith(0); - expect(transactionsOutboxService.deleteOfflineExpense).toHaveBeenCalledWith(-1); - }); + it('deleteBulkOfflineExpenses(): should delete only matching offline expenses by id/local_id', () => { + spyOn(transactionsOutboxService, 'deleteOfflineExpense').and.returnValues(null); + // Arrange pending based on the service’s shape + transactionsOutboxService.queue = cloneDeep(outboxQueueData1); + const pending = transactionsOutboxService.getPendingExpenses(); + const deleteExpenses = [pending[0]]; // one valid match + + // Act + transactionsOutboxService.deleteBulkOfflineExpenses(pending, deleteExpenses); + + // Assert + expect(transactionsOutboxService.deleteOfflineExpense).toHaveBeenCalledTimes(1); + expect(transactionsOutboxService.deleteOfflineExpense).toHaveBeenCalledWith(0); + });src/app/fyle/my-expenses/my-expenses.page.html (1)
222-235: Use Transaction fields (spent_at/created_at), not legacy tx_ fields.Thalaivar tip: pendingExpenses are Transactions; tx_txn_dt/tx_created_at will be undefined.
- @for (pendingExpense of pendingExpenses; track pendingExpense; let i = $index) { + @for (pendingExpense of pendingExpenses; track pendingExpense; let i = $index) { <app-expense-card [expense]="pendingExpense" [isFirstOfflineExpense]="i === 0" - [previousExpenseTxnDate]="pendingExpenses[i - 1]?.tx_txn_dt" - [previousExpenseCreatedAt]="pendingExpenses[i - 1]?.tx_created_at" + [previousExpenseTxnDate]="pendingExpenses[i - 1]?.spent_at" + [previousExpenseCreatedAt]="pendingExpenses[i - 1]?.created_at" [isSelectionModeEnabled]="selectionMode" [selectedElements]="selectedOutboxExpenses" [isOutboxExpense]="true" (setMultiselectMode)="switchOutboxSelectionMode($event)" (cardClickedForSelection)="selectOutboxExpense($event)" > </app-expense-card> }src/app/fyle/my-expenses/my-expenses.page.ts (3)
903-921: Simplify sync flow; calling sync once is enough.from(this.pendingExpenses).pipe(switchMap(sync)) cancels repeatedly and is confusing. Call sync() directly; then refresh if queue is empty.
- syncOutboxExpenses(): void { - this.pendingExpenses = this.transactionOutboxService.getPendingExpenses(); - if (this.pendingExpenses.length > 0) { - this.syncing = true; - from(this.pendingExpenses) - .pipe( - switchMap(() => from(this.transactionOutboxService.sync())), - finalize(() => { - this.syncing = false; - const pendingExpenses = this.transactionOutboxService.getPendingExpenses(); - if (pendingExpenses.length === 0) { - this.doRefresh(); - this.pendingExpenses = []; - } - }), - ) - .subscribe(noop); - } - } + syncOutboxExpenses(): void { + this.pendingExpenses = this.transactionOutboxService.getPendingExpenses(); + if (this.pendingExpenses.length === 0) return; + this.syncing = true; + from(this.transactionOutboxService.sync()) + .pipe( + finalize(() => { + this.syncing = false; + this.pendingExpenses = this.transactionOutboxService.getPendingExpenses(); + if (this.pendingExpenses.length === 0) this.doRefresh(); + }), + ) + .subscribe(noop); + }
1105-1139: Selection by deep-equal is brittle; compare by id/local_id.Offline entries often lack id; add and use local_id from the outbox for stable comparisons. Faster, safer, stylish.
- selectOutboxExpense(expense: Partial<Transaction>): void { - let isSelectedElementsIncludesExpense = false; - if (expense.id) { - isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((exp) => expense.id === exp.id); - } else { - isSelectedElementsIncludesExpense = this.selectedOutboxExpenses.some((exp) => isEqual(exp, expense)); - } + selectOutboxExpense(expense: Partial<Transaction>): void { + const keyOf = (e: Partial<Transaction>) => (e.id as string) ?? ((e as any).local_id as string) ?? null; + const key = keyOf(expense); + const includes = key + ? this.selectedOutboxExpenses.some((e) => keyOf(e) === key) + : this.selectedOutboxExpenses.some((e) => isEqual(e, expense)); - if (isSelectedElementsIncludesExpense) { - if (expense.id) { - this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((exp) => exp.id !== expense.id); - } else { - this.selectedOutboxExpenses = this.selectedOutboxExpenses.filter((exp) => !isEqual(exp, expense)); - } + if (includes) { + this.selectedOutboxExpenses = key + ? this.selectedOutboxExpenses.filter((e) => keyOf(e) !== key) + : this.selectedOutboxExpenses.filter((e) => !isEqual(e, expense)); } else { this.selectedOutboxExpenses.push(expense); }
1744-1753: onSelectAll pending path is correct. Consider copying, not aliasing.Assigning array by reference couples UI to service state. Clone for safety.
- this.selectedOutboxExpenses = this.pendingExpenses; + this.selectedOutboxExpenses = cloneDeep(this.pendingExpenses);src/app/fyle/my-expenses/my-expenses.page.spec.ts (4)
1279-1291: Drop the formatTransactions unit test.Component no longer exposes formatTransactions. Remove this block to appease the test gods.
- it('formatTransactions(): should format transactions correctly', () => { - const unformattedTransactions = unformattedTxnData; - const formattedTransactions = component.formatTransactions(unformattedTransactions); - - expect(formattedTransactions.length).toBe(unformattedTransactions.length); - expect(formattedTransactions).toEqual(expectedFormattedTransaction); - });
2956-2965: Fix property rename in tests: pendingTransactions → pendingExpenses.This is the pipeline error. Change the assignment and expectations accordingly.
- component.pendingTransactions = []; + component.pendingExpenses = [];
3171-3181: Select-all path: adjust to pendingExpenses and outbox list.Match the component’s new names and behavior.
- it('should select all pending transactions and update stats', () => { - component.pendingTransactions = expenseList4; + it('should select all pending expenses and update stats', () => { + component.pendingExpenses = expenseList4 as any; transactionService.getReportableExpenses.and.returnValue(expenseList4); spyOn(component, 'setOutboxExpenseStatsOnSelect'); component.onSelectAll(true); - expect(transactionService.getReportableExpenses).toHaveBeenCalledOnceWith(expenseList4); + expect(transactionService.getReportableExpenses).toHaveBeenCalledOnceWith(expenseList4 as any); expect(component.setOutboxExpenseStatsOnSelect).toHaveBeenCalledTimes(1); expect(component.isReportableExpensesSelected).toBeTrue(); });
523-531: Remove orphaned spy on formatTransactions in ionViewWillEnter() suite.This spy breaks compilation since the method is gone.
- spyOn(component, 'formatTransactions').and.returnValue(apiExpenseRes);
♻️ Duplicate comments (1)
src/app/fyle/my-expenses/my-expenses.page.ts (1)
1099-1102: Null-safe sum for optional amount. Superstar move.amount is optional; guard with ?? 0 to avoid NaN.
- amount: this.selectedOutboxExpenses.reduce((acc, exp) => acc + exp.amount, 0), + amount: this.selectedOutboxExpenses.reduce((acc, exp) => acc + (exp.amount ?? 0), 0),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/app/core/services/transactions-outbox.service.spec.ts(1 hunks)src/app/core/services/transactions-outbox.service.ts(2 hunks)src/app/fyle/my-expenses/my-expenses.page.html(5 hunks)src/app/fyle/my-expenses/my-expenses.page.spec.ts(2 hunks)src/app/fyle/my-expenses/my-expenses.page.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{service,pipe,directive}.ts
📄 CodeRabbit inference engine (.cursor/rules/atomics-i18n-key-pattern.mdc)
**/*.{service,pipe,directive}.ts: i18n keys for services, pipes, and directives must use the schema: services.., pipes.., directives..
Convert kebab-case file names to camelCase and remove the suffix (Service, Pipe, Directive) when generating i18n keys for services, pipes, and directives
Use only the three roots: services, pipes, directives for i18n key grouping
Use meaningful, semantic, and context-aware key names for i18n keys; avoid generic names like message1, text1, error1
Files:
src/app/core/services/transactions-outbox.service.ts
🧠 Learnings (4)
📚 Learning: 2024-11-18T06:55:17.972Z
Learnt from: sumrender
PR: fylein/fyle-mobile-app#3265
File: src/app/fyle/personal-cards-matched-expenses/expense-preview/expense-preview.component.spec.ts:35-37
Timestamp: 2024-11-18T06:55:17.972Z
Learning: In `src/app/fyle/add-edit-expense/add-edit-expense.page.ts`, the `unmatchExpense` method is used for corporate card expenses, not for personal card expenses.
Applied to files:
src/app/fyle/my-expenses/my-expenses.page.htmlsrc/app/fyle/my-expenses/my-expenses.page.spec.tssrc/app/fyle/my-expenses/my-expenses.page.ts
📚 Learning: 2024-11-04T13:10:36.868Z
Learnt from: arjunaj5
PR: fylein/fyle-mobile-app#3242
File: src/app/fyle/my-expenses/my-expenses.page.spec.ts:967-968
Timestamp: 2024-11-04T13:10:36.868Z
Learning: In `src/app/fyle/my-expenses/my-expenses.page.spec.ts`, when testing private methods like `filterDEIncompleteExpenses`, it's acceptable to use `//ts-ignore` to spy on them in unit tests.
Applied to files:
src/app/fyle/my-expenses/my-expenses.page.spec.ts
📚 Learning: 2024-10-23T11:45:16.974Z
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:293-354
Timestamp: 2024-10-23T11:45:16.974Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', the flags 'isVisaRTFEnabled$', 'isMastercardRTFEnabled$', and 'isYodleeEnabled$' do not have rapidly changing values, so tests for rapid changes in these flags are unnecessary.
Applied to files:
src/app/fyle/my-expenses/my-expenses.page.spec.ts
📚 Learning: 2024-10-23T08:40:59.542Z
Learnt from: bistaastha
PR: fylein/fyle-mobile-app#3240
File: src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts:200-200
Timestamp: 2024-10-23T08:40:59.542Z
Learning: In 'src/app/fyle/manage-corporate-cards/manage-corporate-cards.page.spec.ts', moving 'component.isAddCorporateCardVisible$ = of(true);' to the top-level beforeEach block causes tests to fail, so mock values need to be set in individual tests.
Applied to files:
src/app/fyle/my-expenses/my-expenses.page.spec.ts
🧬 Code graph analysis (3)
src/app/core/services/transactions-outbox.service.spec.ts (1)
src/app/core/mock-data/outbox-queue.data.ts (1)
outboxQueueData1(6-22)
src/app/core/services/transactions-outbox.service.ts (2)
src/app/core/models/v1/transaction.model.ts (1)
Transaction(8-142)src/app/core/services/platform/v1/spender/expenses.service.ts (1)
deleteExpenses(392-400)
src/app/fyle/my-expenses/my-expenses.page.ts (1)
src/app/core/models/v1/transaction.model.ts (1)
Transaction(8-142)
🪛 GitHub Actions: Unit Tests
src/app/fyle/my-expenses/my-expenses.page.spec.ts
[error] 527-527: Argument of type '"formatTransactions"' is not assignable to parameter of type 'keyof MyExpensesPage'.
[error] 2956-2956: Property 'pendingTransactions' does not exist on type 'MyExpensesPage'.
🔇 Additional comments (5)
src/app/fyle/my-expenses/my-expenses.page.ts (2)
134-142: Rename to pendingExpenses and selectedOutboxExpenses looks good.Clean, consistent with the service refactor.
1640-1652: Return type is Observable, but the offline branch is side-effect only.LGTM for now since caller awaits completion via popover flow. Just ensure callers subscribe.
Would you like me to scan call sites to ensure subscribe() is present where needed?
src/app/fyle/my-expenses/my-expenses.page.spec.ts (2)
210-214: Spy should match service rename. Good.getPendingExpenses is the right method post-refactor.
232-240: Ignore outdated bulk-delete assertion comment
My friend, the tests never inspect index values or “–1” deletes—there’s no such logic here. The suggestion about ignoring non-matching items in index assertions does not apply.Likely an incorrect or invalid review comment.
src/app/core/services/transactions-outbox.service.spec.ts (1)
213-221: New test name matches method rename. Solid.Proceed.
| getPendingExpenses(): Partial<Transaction>[] { | ||
| return this.queue.map((entry) => ({ ...entry.transaction, dataUrls: entry.dataUrls })); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename looks fine, but add a stable local_id for offline items.
Super move da! getPendingExpenses() aligns the API with Transactions. To make downstream selection/deletion rock-solid, include a stable local_id on each outbox transaction (set in addEntry) so callers can reliably identify items even without id.
Apply this in addEntry:
addEntry(
transaction: Partial<Transaction>,
dataUrls: { url: string; type: string }[],
comments?: string[],
): Promise<void> {
- this.queue.push({
- transaction,
+ const localId = (crypto as any)?.randomUUID?.() ?? Date.now().toString();
+ this.queue.push({
+ transaction: { ...(transaction as any), local_id: localId } as any,
dataUrls,
comments,
});
return this.saveQueue();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getPendingExpenses(): Partial<Transaction>[] { | |
| return this.queue.map((entry) => ({ ...entry.transaction, dataUrls: entry.dataUrls })); | |
| } | |
| addEntry( | |
| transaction: Partial<Transaction>, | |
| dataUrls: { url: string; type: string }[], | |
| comments?: string[], | |
| ): Promise<void> { | |
| const localId = (crypto as any)?.randomUUID?.() ?? Date.now().toString(); | |
| this.queue.push({ | |
| transaction: { ...(transaction as any), local_id: localId } as any, | |
| dataUrls, | |
| comments, | |
| }); | |
| return this.saveQueue(); | |
| } |
🤖 Prompt for AI Agents
In src/app/core/services/transactions-outbox.service.ts around lines 188-190,
getPendingExpenses currently maps entries to transactions but doesn't include a
stable local_id for offline items; modify addEntry to generate and store a
stable local_id (e.g. a UUID or monotonic counter) on the outbox entry when
created and ensure that getPendingExpenses returns that local_id on each
returned transaction object (include it in the mapped object alongside
transaction and dataUrls) so downstream selection/deletion can reliably identify
items without a server id.
| transactionOutboxService.getPendingExpenses.and.returnValues(mockPendingTransactions, []); | ||
| transactionOutboxService.sync.and.resolveTo(undefined); | ||
| spyOn(component, 'doRefresh'); | ||
|
|
||
| component.syncOutboxExpenses(); | ||
| tick(100); | ||
|
|
||
| expect(component.pendingTransactions).toEqual([]); | ||
| expect(component.pendingExpenses).toEqual([]); | ||
| expect(component.formatTransactions).toHaveBeenCalledTimes(2); | ||
| expect(transactionOutboxService.getPendingTransactions).toHaveBeenCalledTimes(2); | ||
| expect(transactionOutboxService.getPendingExpenses).toHaveBeenCalledTimes(2); | ||
| expect(transactionOutboxService.sync).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove formatTransactions spying and assertions; method no longer exists.
Why fear when Rajini is here? Trim the test to assert syncing + pendingExpenses + service calls only.
- spyOn(component, 'formatTransactions').and.returnValues(mockFormattedTransactions, []);
transactionOutboxService.getPendingExpenses.and.returnValues(mockPendingTransactions, []);
transactionOutboxService.sync.and.resolveTo(undefined);
spyOn(component, 'doRefresh');
component.syncOutboxExpenses();
tick(100);
- expect(component.pendingExpenses).toEqual([]);
- expect(component.formatTransactions).toHaveBeenCalledTimes(2);
+ expect(component.pendingExpenses).toEqual([]);
expect(transactionOutboxService.getPendingExpenses).toHaveBeenCalledTimes(2);
expect(transactionOutboxService.sync).toHaveBeenCalledTimes(1);
expect(component.syncing).toBeFalse();
expect(component.doRefresh).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
In src/app/fyle/my-expenses/my-expenses.page.spec.ts around lines 1759 to 1769,
remove any spying and assertions related to the now-removed formatTransactions
method and simplify the test to only assert that sync was invoked,
pendingExpenses ends up empty, and the transactionOutboxService methods were
called the expected number of times; specifically delete spyOn/expect lines for
formatTransactions, keep the getPendingExpenses and sync stubs, preserve the
pendingExpenses assertion and the service call count assertions, and ensure
tick(100) and any refresh spy are adjusted/removed only if they are tied to
formatTransactions so the test only verifies syncing, pendingExpenses state, and
service interactions.
2 participants
Clickup
Please add link here
Code Coverage
Please add code coverage here
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit
New Features
Refactor
Bug Fixes