clean up fyle_category #3929
clean up fyle_category #3929
Conversation
WalkthroughMigrates from fyle_category to category.system_category across models, services, and pages. Removes dependent custom-field population and several type-filtering/pagination utilities. Adjusts payloads to use cost_center_id only. Updates delete dialog callbacks to return Observable. Converts ReviewFooter input to signals and updates template bindings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant AE as AddEdit* Pages
participant TS as TransactionService
participant PES as Spender ExpensesService (v1)
participant PS as PolicyService
U->>AE: Create/Edit expense
AE->>AE: Build payload (cost_center_id, category.system_category)
AE->>TS: Transform/submit expense
TS->>TS: transformExpense (no fyle_category)
TS->>PES: transformTo(...)
PES->>PES: Use category.system_category for travel-class checks
PES-->>TS: Transformed payload
TS->>PS: transformTo(...) for policy checks
PS->>PS: Use category.system_category branches
PS-->>TS: Policy result
TS-->>AE: Response
AE-->>U: UI updates (delete dialogs return Observable<void>)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
✨ 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 title must start with "fix:", "feat:", "chore:", "refactor:", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR title must start with "fix:", "feat:", "chore:", "refactor:", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR title must start with "fix:", "feat:", "chore:", "refactor:", or "test:" (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 (6)
src/app/shared/components/review-footer/review-footer.component.ts (2)
29-31: Be explicit with output types, machaTighten typings to avoid implicit any.
- readonly saveAndGoToPrev = output(); + readonly saveAndGoToPrev = output<void>(); - readonly saveAndGoToNext = output(); + readonly saveAndGoToNext = output<void>();
13-28: Clean up migration TODO noise before curtain callThese auto-migration notes served their purpose; trim them before merge.
src/app/core/services/split-expense.service.ts (1)
199-206: Handle both 'flight' and 'airlines' and guardsystem_categoryCurrently only 'airlines' is checked, and
toLowerCase()can NPE. Align with Policy/Expenses services and normalize safely.Apply:
- if (transaction.category?.system_category.toLowerCase() === 'airlines') { + const sysCat = transaction.category?.system_category?.toLowerCase(); + if (sysCat === 'flight' || sysCat === 'airlines') { if (transaction.flight_journey_travel_class) { platformSplitObject.travel_classes.push(transaction.flight_journey_travel_class); } if (transaction.flight_return_travel_class) { platformSplitObject.travel_classes.push(transaction.flight_return_travel_class); } }src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts (2)
2406-2414: Don’t setcategory: null; useundefinedor omit the fieldSetting
category: nullclashes with the stricterPublicPolicyExpenseand triggers downstreamsystem_categoryreads. Switch toundefined(or drop the key).- category: null, + // omit or explicitly undefined to align with optional type + category: undefined,Bonus: with the model fix (making
categoryoptional), this stays type-safe everywhere.
958-964: Guard orgSettings.mileage or crash, ha!
orgSettings.mileage is optional; accessing.unitdirectly will explode when undefined. Use optional chaining and default to “KM” like a true superstar.- const distanceUnit = orgSettings.mileage.unit; + const distanceUnit = orgSettings?.mileage?.unit === 'MILES' ? 'MILES' : 'KM';src/app/fyle/add-edit-per-diem/add-edit-per-diem.page.ts (1)
2293-2299: Return void explicitly from deleteMethod to avoid type driftHarden the contract even if services return a payload today.
- deleteMethod: (): Observable<void> => { - if (removePerDiemFromReport) { - return this.platformReportService.ejectExpenses(reportId, id); - } - return this.expensesService.deleteExpenses([id]); - }, + deleteMethod: (): Observable<void> => { + if (removePerDiemFromReport) { + return this.platformReportService.ejectExpenses(reportId, id).pipe(map(() => void 0)); + } + return this.expensesService.deleteExpenses([id]).pipe(map(() => void 0)); + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
src/app/core/models/public-policy-expense.model.ts(2 hunks)src/app/core/models/v1/transaction.model.ts(0 hunks)src/app/core/services/custom-inputs.service.spec.ts(0 hunks)src/app/core/services/custom-inputs.service.ts(0 hunks)src/app/core/services/platform/v1/spender/expenses.service.ts(1 hunks)src/app/core/services/policy.service.ts(1 hunks)src/app/core/services/split-expense.service.ts(2 hunks)src/app/core/services/transaction.service.spec.ts(0 hunks)src/app/core/services/transaction.service.ts(0 hunks)src/app/core/services/transactions-outbox.service.ts(0 hunks)src/app/fyle/add-edit-expense/add-edit-expense.page.ts(12 hunks)src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts(4 hunks)src/app/fyle/add-edit-per-diem/add-edit-per-diem.page.ts(2 hunks)src/app/fyle/my-create-report/my-create-report.page.ts(0 hunks)src/app/shared/components/review-footer/review-footer.component.html(2 hunks)src/app/shared/components/review-footer/review-footer.component.ts(2 hunks)
💤 Files with no reviewable changes (7)
- src/app/core/services/custom-inputs.service.spec.ts
- src/app/core/models/v1/transaction.model.ts
- src/app/core/services/transaction.service.spec.ts
- src/app/core/services/transactions-outbox.service.ts
- src/app/core/services/custom-inputs.service.ts
- src/app/fyle/my-create-report/my-create-report.page.ts
- src/app/core/services/transaction.service.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/policy.service.tssrc/app/core/services/split-expense.service.tssrc/app/core/services/platform/v1/spender/expenses.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/review-footer/review-footer.component.htmlsrc/app/shared/components/review-footer/review-footer.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/review-footer/review-footer.component.htmlsrc/app/shared/components/review-footer/review-footer.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/review-footer/review-footer.component.html
🧠 Learnings (2)
📚 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/add-edit-per-diem/add-edit-per-diem.page.tssrc/app/fyle/add-edit-mileage/add-edit-mileage.page.tssrc/app/fyle/add-edit-expense/add-edit-expense.page.ts
📚 Learning: 2025-06-17T05:15:26.587Z
Learnt from: Dimple16
PR: fylein/fyle-mobile-app#3714
File: src/app/core/services/mileage.service.ts:23-25
Timestamp: 2025-06-17T05:15:26.587Z
Learning: In the Fyle mobile app, the migration from OrgUserSettingsService to PlatformEmployeeSettingsService introduced a potential breaking change where mileage_settings could be null or undefined. Test cases in add-edit-mileage-2.spec.ts and add-edit-mileage-5.spec.ts explicitly test scenarios where getOrgUserMileageSettings returns null or undefined, indicating this is a real-world scenario that needs defensive handling.
Applied to files:
src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts
🧬 Code graph analysis (4)
src/app/core/models/public-policy-expense.model.ts (1)
src/app/core/models/platform/platform-category.model.ts (1)
PlatformCategory(1-13)
src/app/core/services/split-expense.service.ts (2)
src/app/core/models/v1/transaction.model.ts (1)
Transaction(7-140)src/app/core/models/platform/v1/split-payload.model.ts (1)
SplitPayload(5-30)
src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts (4)
src/app/core/models/v1/transaction.model.ts (1)
Transaction(7-140)src/app/fyle/view-expense/view-expense.page.ts (1)
isNumber(208-210)src/app/fyle/view-mileage/view-mileage.page.ts (1)
isNumber(180-182)src/app/fyle/view-per-diem/view-per-diem.page.ts (1)
isNumber(135-137)
src/app/fyle/add-edit-expense/add-edit-expense.page.ts (4)
src/app/core/models/unflattened-transaction.model.ts (1)
UnflattenedTransaction(7-27)src/app/fyle/view-expense/view-expense.page.ts (1)
isNumber(208-210)src/app/fyle/view-mileage/view-mileage.page.ts (1)
isNumber(180-182)src/app/fyle/view-per-diem/view-per-diem.page.ts (1)
isNumber(135-137)
⏰ 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.ts (1)
19-19: ReviewList input change safe – all callers pass string[]
All three parent pages (add-edit-per-diem,add-edit-mileage,add-edit-expense) already declare and passreviewList: string[], so no API break needs addressing.src/app/shared/components/review-footer/review-footer.component.html (2)
11-11: LGTM — formatting onlyNo functional change here. Rock solid.
21-21: LGTM — formatting onlyAll good, boss.
src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts (2)
2446-2448: LGTM: cleaner policy check helperThe new
getIsPolicyExpensereads well and removes casting noise.
2914-2927: LGTM: dialog delete method now returnsObservable<void>Matches platform services, simplifies consumers. Rock-solid.
src/app/fyle/add-edit-per-diem/add-edit-per-diem.page.ts (1)
2281-2281: Align deleteMethod contract across component and callersYou’ve switched to Observable. Make sure FyDeleteDialogComponent’s input typing expects void and no downstream relies on a non-void payload. Style, but verify.
src/app/fyle/add-edit-expense/add-edit-expense.page.ts (2)
1574-1581: Good call skipping 'Unspecified' via system_categoryThis avoids fetching invalid categories. Rock solid.
3055-3057: Policy check helper looks rightUsing policy_amount < 0.0001 matches the rest of the codepath. Carry on.
| billable: boolean; | ||
| bus_travel_class: string; | ||
| category: string; | ||
| category: Pick<PlatformCategory, 'code' | 'id' | 'display_name' | 'name' | 'sub_category' | 'system_category'>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make category optional to match real payloads and avoid downstream crashes
PublicPolicyExpense now requires category, but several call-sites build tx without it (and even set category: null in add-edit-mileage). This mismatches usage and amplifies NPE risk when services read category.system_category.
Apply:
- category: Pick<PlatformCategory, 'code' | 'id' | 'display_name' | 'name' | 'sub_category' | 'system_category'>;
+ category?: Pick<PlatformCategory, 'code' | 'id' | 'display_name' | 'name' | 'sub_category' | 'system_category'>;Superstar tip: optional here, swagger there—peace everywhere.
📝 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.
| category: Pick<PlatformCategory, 'code' | 'id' | 'display_name' | 'name' | 'sub_category' | 'system_category'>; | |
| // ... other fields ... | |
| // Make `category` optional to reflect real payloads and prevent NPEs downstream | |
| category?: Pick< | |
| PlatformCategory, | |
| 'code' | 'id' | 'display_name' | 'name' | 'sub_category' | 'system_category' | |
| >; | |
| // ... remaining fields ... |
🤖 Prompt for AI Agents
In src/app/core/models/public-policy-expense.model.ts around line 12, the
PublicPolicyExpense type currently requires category which contradicts runtime
payloads and causes NPEs; change the property to be optional (and allow null) —
e.g. make it category?: Pick<PlatformCategory, 'code' | 'id' | 'display_name' |
'name' | 'sub_category' | 'system_category'> | null — so callers that omit or
set null won’t break downstream code that reads category fields.
| if ( | ||
| transaction.fyle_category?.toLowerCase() === 'flight' || | ||
| transaction.fyle_category?.toLowerCase() === 'airlines' | ||
| transaction.category?.system_category.toLowerCase() === 'flight' || | ||
| transaction.category?.system_category.toLowerCase() === 'airlines' | ||
| ) { | ||
| if (transaction.flight_journey_travel_class) { | ||
| expense.travel_classes.push(transaction.flight_journey_travel_class); | ||
| } | ||
| if (transaction.flight_return_travel_class) { | ||
| expense.travel_classes.push(transaction.flight_return_travel_class); | ||
| } | ||
| } else if (transaction.fyle_category?.toLowerCase() === 'bus' && transaction.bus_travel_class) { | ||
| } else if (transaction.category?.system_category.toLowerCase() === 'bus' && transaction.bus_travel_class) { | ||
| expense.travel_classes.push(transaction.bus_travel_class); | ||
| } else if (transaction.fyle_category?.toLowerCase() === 'train' && transaction.train_travel_class) { | ||
| } else if (transaction.category?.system_category.toLowerCase() === 'train' && transaction.train_travel_class) { | ||
| expense.travel_classes.push(transaction.train_travel_class); | ||
| } |
There was a problem hiding this comment.
Normalize system_category once; avoid toLowerCase() on undefined
Same grenade here—defuse it with a single safe read.
- if (
- transaction.category?.system_category.toLowerCase() === 'flight' ||
- transaction.category?.system_category.toLowerCase() === 'airlines'
- ) {
+ const sysCat = transaction.category?.system_category?.toLowerCase();
+ if (sysCat === 'flight' || sysCat === 'airlines') {
...
- } else if (transaction.category?.system_category.toLowerCase() === 'bus' && transaction.bus_travel_class) {
+ } else if (sysCat === 'bus' && transaction.bus_travel_class) {
...
- } else if (transaction.category?.system_category.toLowerCase() === 'train' && transaction.train_travel_class) {
+ } else if (sysCat === 'train' && transaction.train_travel_class) {
...
}📝 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 ( | |
| transaction.fyle_category?.toLowerCase() === 'flight' || | |
| transaction.fyle_category?.toLowerCase() === 'airlines' | |
| transaction.category?.system_category.toLowerCase() === 'flight' || | |
| transaction.category?.system_category.toLowerCase() === 'airlines' | |
| ) { | |
| if (transaction.flight_journey_travel_class) { | |
| expense.travel_classes.push(transaction.flight_journey_travel_class); | |
| } | |
| if (transaction.flight_return_travel_class) { | |
| expense.travel_classes.push(transaction.flight_return_travel_class); | |
| } | |
| } else if (transaction.fyle_category?.toLowerCase() === 'bus' && transaction.bus_travel_class) { | |
| } else if (transaction.category?.system_category.toLowerCase() === 'bus' && transaction.bus_travel_class) { | |
| expense.travel_classes.push(transaction.bus_travel_class); | |
| } else if (transaction.fyle_category?.toLowerCase() === 'train' && transaction.train_travel_class) { | |
| } else if (transaction.category?.system_category.toLowerCase() === 'train' && transaction.train_travel_class) { | |
| expense.travel_classes.push(transaction.train_travel_class); | |
| } | |
| // Normalize system_category once and safely handle undefined | |
| const sysCat = transaction.category?.system_category?.toLowerCase(); | |
| if (sysCat === 'flight' || sysCat === 'airlines') { | |
| if (transaction.flight_journey_travel_class) { | |
| expense.travel_classes.push(transaction.flight_journey_travel_class); | |
| } | |
| if (transaction.flight_return_travel_class) { | |
| expense.travel_classes.push(transaction.flight_return_travel_class); | |
| } | |
| } else if (sysCat === 'bus' && transaction.bus_travel_class) { | |
| expense.travel_classes.push(transaction.bus_travel_class); | |
| } else if (sysCat === 'train' && transaction.train_travel_class) { | |
| expense.travel_classes.push(transaction.train_travel_class); | |
| } |
🤖 Prompt for AI Agents
In src/app/core/services/platform/v1/spender/expenses.service.ts around lines
356 to 370, the code repeatedly calls
transaction.category?.system_category.toLowerCase() which can throw when
system_category is undefined; normalize it once by creating a safe lowercased
variable like const sysCat =
transaction.category?.system_category?.toLowerCase() || '' and then use sysCat
=== 'flight' etc. Update the conditional branches to reference sysCat for
comparisons and keep existing travel class push logic unchanged.
| if ( | ||
| transaction.fyle_category?.toLowerCase() === 'flight' || | ||
| transaction.fyle_category?.toLowerCase() === 'airlines' | ||
| transaction.category?.system_category.toLowerCase() === 'flight' || | ||
| transaction.category?.system_category.toLowerCase() === 'airlines' | ||
| ) { | ||
| if (transaction.flight_journey_travel_class) { | ||
| platformPolicyExpense.travel_classes.push(transaction.flight_journey_travel_class); | ||
| } | ||
| if (transaction.flight_return_travel_class) { | ||
| platformPolicyExpense.travel_classes.push(transaction.flight_return_travel_class); | ||
| } | ||
| } else if (transaction.fyle_category?.toLowerCase() === 'bus' && transaction.bus_travel_class) { | ||
| } else if (transaction.category?.system_category.toLowerCase() === 'bus' && transaction.bus_travel_class) { | ||
| platformPolicyExpense.travel_classes.push(transaction.bus_travel_class); | ||
| } else if (transaction.fyle_category?.toLowerCase() === 'train' && transaction.train_travel_class) { | ||
| } else if (transaction.category?.system_category.toLowerCase() === 'train' && transaction.train_travel_class) { | ||
| platformPolicyExpense.travel_classes.push(transaction.train_travel_class); |
There was a problem hiding this comment.
Guard system_category before toLowerCase(); otherwise one undefined and kaboom
If category exists but system_category is missing, toLowerCase() will throw. Normalize once and branch on a safe value.
Apply:
- if (
- transaction.category?.system_category.toLowerCase() === 'flight' ||
- transaction.category?.system_category.toLowerCase() === 'airlines'
- ) {
+ const sysCat = transaction.category?.system_category?.toLowerCase();
+ if (sysCat === 'flight' || sysCat === 'airlines') {
...
- } else if (transaction.category?.system_category.toLowerCase() === 'bus' && transaction.bus_travel_class) {
+ } else if (sysCat === 'bus' && transaction.bus_travel_class) {
...
- } else if (transaction.category?.system_category.toLowerCase() === 'train' && transaction.train_travel_class) {
+ } else if (sysCat === 'train' && transaction.train_travel_class) {
...
}Now the logic punches without missing.
📝 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 ( | |
| transaction.fyle_category?.toLowerCase() === 'flight' || | |
| transaction.fyle_category?.toLowerCase() === 'airlines' | |
| transaction.category?.system_category.toLowerCase() === 'flight' || | |
| transaction.category?.system_category.toLowerCase() === 'airlines' | |
| ) { | |
| if (transaction.flight_journey_travel_class) { | |
| platformPolicyExpense.travel_classes.push(transaction.flight_journey_travel_class); | |
| } | |
| if (transaction.flight_return_travel_class) { | |
| platformPolicyExpense.travel_classes.push(transaction.flight_return_travel_class); | |
| } | |
| } else if (transaction.fyle_category?.toLowerCase() === 'bus' && transaction.bus_travel_class) { | |
| } else if (transaction.category?.system_category.toLowerCase() === 'bus' && transaction.bus_travel_class) { | |
| platformPolicyExpense.travel_classes.push(transaction.bus_travel_class); | |
| } else if (transaction.fyle_category?.toLowerCase() === 'train' && transaction.train_travel_class) { | |
| } else if (transaction.category?.system_category.toLowerCase() === 'train' && transaction.train_travel_class) { | |
| platformPolicyExpense.travel_classes.push(transaction.train_travel_class); | |
| const sysCat = transaction.category?.system_category?.toLowerCase(); | |
| if (sysCat === 'flight' || sysCat === 'airlines') { | |
| if (transaction.flight_journey_travel_class) { | |
| platformPolicyExpense.travel_classes.push(transaction.flight_journey_travel_class); | |
| } | |
| if (transaction.flight_return_travel_class) { | |
| platformPolicyExpense.travel_classes.push(transaction.flight_return_travel_class); | |
| } | |
| } else if (sysCat === 'bus' && transaction.bus_travel_class) { | |
| platformPolicyExpense.travel_classes.push(transaction.bus_travel_class); | |
| } else if (sysCat === 'train' && transaction.train_travel_class) { | |
| platformPolicyExpense.travel_classes.push(transaction.train_travel_class); | |
| } |
🤖 Prompt for AI Agents
In src/app/core/services/policy.service.ts around lines 69 to 82, the code calls
transaction.category?.system_category.toLowerCase() which will throw if
system_category is undefined; normalize once by creating a safe local variable
(e.g. const sys = transaction.category?.system_category?.toLowerCase() ?? '')
and then branch on sys for 'flight', 'airlines', 'bus', 'train'; keep the
existing checks that travel class properties exist before pushing them.
| if (transaction.category?.system_category.toLowerCase() === 'bus' && transaction.bus_travel_class) { | ||
| platformSplitObject.travel_classes.push(transaction.bus_travel_class); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same safety net for Bus
Mirror the safe normalization.
- if (transaction.category?.system_category.toLowerCase() === 'bus' && transaction.bus_travel_class) {
+ const sysCat = transaction.category?.system_category?.toLowerCase();
+ if (sysCat === 'bus' && transaction.bus_travel_class) {
platformSplitObject.travel_classes.push(transaction.bus_travel_class);
}📝 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 (transaction.category?.system_category.toLowerCase() === 'bus' && transaction.bus_travel_class) { | |
| platformSplitObject.travel_classes.push(transaction.bus_travel_class); | |
| } | |
| } | |
| const sysCat = transaction.category?.system_category?.toLowerCase(); | |
| if (sysCat === 'bus' && transaction.bus_travel_class) { | |
| platformSplitObject.travel_classes.push(transaction.bus_travel_class); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/app/core/services/split-expense.service.ts around lines 210 to 213, the
bus branch currently pushes transaction.bus_travel_class directly; mirror the
safe normalization used elsewhere by first checking that bus_travel_class is
defined/truthy, normalizing it (trim and lowercase) and only then pushing the
normalized value into platformSplitObject.travel_classes to avoid null/undefined
entries and inconsistent casing.
| if (transaction.category?.system_category.toLowerCase() === 'train' && transaction.train_travel_class) { | ||
| platformSplitObject.travel_classes.push(transaction.train_travel_class); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same safety net for Train
Mirror the safe normalization.
- if (transaction.category?.system_category.toLowerCase() === 'train' && transaction.train_travel_class) {
+ const sysCat = transaction.category?.system_category?.toLowerCase();
+ if (sysCat === 'train' && transaction.train_travel_class) {
platformSplitObject.travel_classes.push(transaction.train_travel_class);
}📝 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 (transaction.category?.system_category.toLowerCase() === 'train' && transaction.train_travel_class) { | |
| platformSplitObject.travel_classes.push(transaction.train_travel_class); | |
| } | |
| } | |
| const sysCat = transaction.category?.system_category?.toLowerCase(); | |
| if (sysCat === 'train' && transaction.train_travel_class) { | |
| platformSplitObject.travel_classes.push(transaction.train_travel_class); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/app/core/services/split-expense.service.ts around lines 216 to 219, the
code calls toLowerCase() on transaction.category.system_category without the
same safety checks used elsewhere and pushes train_travel_class without
normalization; change the condition to use optional chaining when lowercasing:
transaction.category?.system_category?.toLowerCase() === 'train', and when
pushing, guard and normalize the value (e.g. if (transaction.train_travel_class)
platformSplitObject.travel_classes.push(transaction.train_travel_class?.toLowerCase().trim()));
this mirrors the safe normalization pattern used for other categories.
| deleteMethod: () => Observable<void>; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unify deleteMethod to return void explicitly
Mirror the per-diem fix to keep the dialog contract tight.
- deleteMethod: () => Observable<void>;
+ deleteMethod: () => Observable<void>;
@@
- deleteMethod: (): Observable<void> => {
- if (removeExpenseFromReport) {
- return this.platformReportService.ejectExpenses(reportId, this.activatedRoute.snapshot.params.id as string);
- }
- return this.expensesService.deleteExpenses([this.activatedRoute.snapshot.params.id as string]);
- },
+ deleteMethod: (): Observable<void> => {
+ if (removeExpenseFromReport) {
+ return this.platformReportService
+ .ejectExpenses(reportId, this.activatedRoute.snapshot.params.id as string)
+ .pipe(map(() => void 0));
+ }
+ return this.expensesService
+ .deleteExpenses([this.activatedRoute.snapshot.params.id as string])
+ .pipe(map(() => void 0));
+ },Ensure FyDeleteDialogComponent’s handler doesn’t rely on a response body.
Also applies to: 5092-5099
🏁 Script executed:
#!/bin/bash
# Locate definition of ejectExpenses in the codebase
rg -nP --type ts '\bejectExpenses\b' -C3
# Locate definition of deleteExpenses in the codebase
rg -nP --type ts '\bdeleteExpenses\b' -C3
# Locate the FyDeleteDialogComponent to inspect how deleteMethod’s return value is used
rg -nP --type ts 'class\s+FyDeleteDialogComponent' -C3
# Find all references to deleteMethod to see its invocation context
rg -nP --type ts 'deleteMethod' -C3Length of output: 98151
Enforce void return in deleteMethod
Pipe both platformReportService.ejectExpenses(...) and expensesService.deleteExpenses(...) through .pipe(map(() => void 0)) in the deleteMethod of getDeleteDialogProps (src/app/fyle/add-edit-expense/add-edit-expense.page.ts) so it consistently returns Observable<void>.
🤖 Prompt for AI Agents
In src/app/fyle/add-edit-expense/add-edit-expense.page.ts around lines
5080-5081, the deleteMethod in getDeleteDialogProps must return
Observable<void>; update both branches that call
platformReportService.ejectExpenses(...) and expensesService.deleteExpenses(...)
to chain .pipe(map(() => void 0)) so the observable emits void; ensure
rxjs/operators map is imported if not already.
| <ion-toolbar mode="md" class="review-footer--footer-toolbar__shadow review-footer--footer-toolbar__spacing"> | ||
| <div class="fy-footer-cta-container"> | ||
| @if (activeIndex !== 0 && reviewList?.length > 0) { | ||
| @if (activeIndex !== 0 && reviewList()?.length > 0) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stronger prev-button guard
Use numeric check and keep calls consistent with the TS default.
- @if (activeIndex !== 0 && reviewList()?.length > 0) {
+ @if (activeIndex > 0 && reviewList().length > 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.
| @if (activeIndex !== 0 && reviewList()?.length > 0) { | |
| @if (activeIndex > 0 && reviewList().length > 0) { |
🤖 Prompt for AI Agents
In src/app/shared/components/review-footer/review-footer.component.html around
line 4, the prev-button guard uses a strict non-zero check and optional chaining
length check that can be inconsistent with TS defaults; change it to a numeric
check and a null-safe length evaluation (e.g. check activeIndex > 0 and use
(reviewList()?.length ?? 0) > 0) so the guard always treats undefined/null as 0
and keeps call style consistent with TypeScript.
| (reviewList().length > 0 && !(activeIndex === reviewList().length - 1) | ||
| ? 'reviewFooter.saveAndNext' | ||
| : 'reviewFooter.save' | ||
| ) | transloco |
There was a problem hiding this comment.
Guard that length check like a superstar
Unconditional reviewList().length will crash when undefined. The TS fix (default to []) solves it; otherwise make the template null-safe.
Option A (preferred, with TS default): no change needed here.
Option B (template-only safeguard):
- (reviewList().length > 0 && !(activeIndex === reviewList().length - 1)
+ (((reviewList()?.length ?? 0) > 0) && !(activeIndex === ((reviewList()?.length ?? 0) - 1))📝 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.
| (reviewList().length > 0 && !(activeIndex === reviewList().length - 1) | |
| ? 'reviewFooter.saveAndNext' | |
| : 'reviewFooter.save' | |
| ) | transloco | |
| (((reviewList()?.length ?? 0) > 0) && !(activeIndex === ((reviewList()?.length ?? 0) - 1)) | |
| ? 'reviewFooter.saveAndNext' | |
| : 'reviewFooter.save' | |
| ) | transloco |
🤖 Prompt for AI Agents
In src/app/shared/components/review-footer/review-footer.component.html around
lines 23 to 26, the template calls reviewList().length unguarded which will
throw if reviewList() is undefined; either set a default value of [] for
reviewList in the component TypeScript (preferred) so reviewList() always
returns an array, or make the template null-safe by replacing the expression
with a guarded form that treats undefined as empty (e.g. use the safe navigation
and nullish fallback so length is 0 when reviewList() is undefined) and adjust
the comparison to use that safe length.
| @@ -1,5 +1,4 @@ | |||
| import { Component, Input, OnInit, inject, output } from '@angular/core'; | |||
| import { Expense } from 'src/app/core/models/expense.model'; | |||
| import { Component, Input, OnInit, inject, input, output } from '@angular/core'; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style punch: drop OnInit if unused
Empty lifecycle = dead weight. Remove import, implements, and method.
-import { Component, Input, OnInit, inject, input, output } from '@angular/core';
+import { Component, Input, inject, input, output } from '@angular/core';
@@
-export class ReviewFooterComponent implements OnInit {
+export class ReviewFooterComponent {
@@
- ngOnInit() {}
+Also applies to: 10-10, 33-33
🤖 Prompt for AI Agents
In src/app/shared/components/review-footer/review-footer.component.ts around
lines 1, 10, and 33, remove the unused OnInit lifecycle usage: delete OnInit
from the import list, remove any implements OnInit on the class, and delete the
empty ngOnInit() method; also clean up incorrect duplicate/incorrect imports
('input' and 'output') so only Angular's capitalized Input and Output remain in
the import statement. Ensure imports and class signature reflect only the
actually used symbols.
| // Your application code writes to the input. This prevents migration. | ||
| @Input() reviewList: Array<Expense>; | ||
| readonly reviewList = input<Array<string>>(); | ||
|
|
There was a problem hiding this comment.
Default the signal to avoid runtime ka-boom
If reviewList isn’t bound immediately, reviewList().length in the template can explode. Give it a safe default.
Apply:
- readonly reviewList = input<Array<string>>();
+ readonly reviewList = input<readonly string[]>([]);📝 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.
| readonly reviewList = input<Array<string>>(); | |
| readonly reviewList = input<readonly string[]>([]); |
🤖 Prompt for AI Agents
In src/app/shared/components/review-footer/review-footer.component.ts around
line 19, the reviewList signal is created without a default which can cause
reviewList().length in the template to throw if the input isn’t bound
immediately; fix by giving the input a safe default (an empty array) when
creating the signal (e.g. pass an empty-array default to input) so template
reads are safe.
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