fix: split mileage page observables to two parts to improve loading time#4140
fix: split mileage page observables to two parts to improve loading time#4140
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughIyar, the mileage add-edit flow is split into two sequential fetch phases: Phase 1 loads critical data to render the form quickly; Phase 2 loads secondary data (projects, reports, recent values, commute details) and patches dependent fields afterwards. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/fyle/add-edit-mileage/add-edit-mileage.page.setup.spec.ts`:
- Line 70: The test suite is using fdescribe('AddEditMileagePage', ...) which
forces only this spec to run; change fdescribe to describe in the
AddEditMileagePage spec (replace fdescribe('AddEditMileagePage', ...) with
describe('AddEditMileagePage', ...)) so the full test suite runs again and
re-run tests to confirm no other focused specs remain.
In `@src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts`:
- Around line 2087-2095: The final fg.patchValue is overwriting the freshly
autofilled costCenter with the stale/older costCenter variable; change the patch
logic in add-edit-mileage.page.ts so it uses the computed autoFillCostCenter
(and this.presetCostCenterId) when present instead of the older costCenter local
variable—mirror the effectiveProject pattern: derive an effectiveCostCenter
(prefer autoFillCostCenter.value, fall back to existing fg.value.costCenter) and
patch that into fg and presetCostCenterId so the UI and dependent fields aren't
reset to null.
- Around line 2004-2017: The forkJoin combining observables (forkJoin({ etxn:
this.etxn$, project: selectedProject$, report: selectedReport$, costCenter:
selectedCostCenter$, recentValue: this.recentlyUsedValues$, recentProjects:
this.recentlyUsedProjects$, recentCostCenters: this.recentlyUsedCostCenters$,
commuteDeductionDetails: commuteDeductionDetails$, employeeSettings:
employeeSettings$, orgSettings: orgSettings$ })) is brittle because any source
error will fail the whole join; update the non-critical sources (all except
this.etxn$) by piping them through catchError and returning safe fallbacks
(e.g., [] for lists, null for optional objects) before passing into forkJoin so
secondary hydration survives individual failures while keeping etxn as the
required source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dde3a6dc-f2c4-44bc-b4b5-533d417b2a43
📒 Files selected for processing (2)
src/app/fyle/add-edit-mileage/add-edit-mileage.page.setup.spec.tssrc/app/fyle/add-edit-mileage/add-edit-mileage.page.ts
src/app/fyle/add-edit-mileage/add-edit-mileage.page.setup.spec.ts
Outdated
Show resolved
Hide resolved
| forkJoin({ | ||
| etxn: this.etxn$, | ||
| project: selectedProject$, | ||
| report: selectedReport$, | ||
| costCenter: selectedCostCenter$, | ||
| recentValue: this.recentlyUsedValues$, | ||
| recentProjects: this.recentlyUsedProjects$, | ||
| recentCostCenters: this.recentlyUsedCostCenters$, | ||
| commuteDeductionDetails: commuteDeductionDetails$, | ||
| employeeSettings: employeeSettings$, | ||
| orgSettings: orgSettings$, | ||
| }) | ||
| .pipe(take(1)) | ||
| .subscribe( |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In RxJS, does forkJoin fail the whole combined observable if any inner observable errors before completion?
💡 Result:
Yes. forkJoin errors the combined observable if any inner/source observable errors, and it immediately unsubscribes from the other inner observables (so you won’t get a final combined value). [1]
If you want the join to still succeed when one source fails, you must handle errors inside each inner observable (e.g., with catchError returning a fallback value) before passing them to forkJoin. [1]
Sources:
[1] RxJS source/docs comment for forkJoin (ReactiveX/rxjs) (raw.githubusercontent.com)
Citations:
🏁 Script executed:
cd src/app/fyle/add-edit-mileage && wc -l add-edit-mileage.page.tsRepository: fylein/fyle-mobile-app
Length of output: 95
🏁 Script executed:
sed -n '1980,2050p' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts | cat -nRepository: fylein/fyle-mobile-app
Length of output: 2986
🏁 Script executed:
# Search for error handling patterns in this file
rg 'catchError|try|catch' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts -B 2 -A 2 | head -100Repository: fylein/fyle-mobile-app
Length of output: 888
🏁 Script executed:
# Check how the forkJoin result is consumed
sed -n '2015,2100p' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts | cat -nRepository: fylein/fyle-mobile-app
Length of output: 4054
🏁 Script executed:
# Find where selectedProject$ and other observables are created/defined
rg 'selectedProject\$|selectedReport\$|selectedCostCenter\$' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts -B 3 -A 3Repository: fylein/fyle-mobile-app
Length of output: 2200
🏁 Script executed:
# Search for recentlyUsedValues$, recentlyUsedProjects$, recentlyUsedCostCenters$
rg 'recentlyUsedValues\$|recentlyUsedProjects\$|recentlyUsedCostCenters\$' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts -B 3 -A 3Repository: fylein/fyle-mobile-app
Length of output: 2615
🏁 Script executed:
# Search for commuteDeductionDetails$, employeeSettings$, orgSettings$ creation
rg 'commuteDeductionDetails\$|employeeSettings\$|orgSettings\$' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts -B 3 -A 3 | head -80Repository: fylein/fyle-mobile-app
Length of output: 2742
🏁 Script executed:
# Check the full scope of the ngOnInit or initialization method to see observable creation
rg 'ngOnInit|this\.etxn\$ =' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts -A 30 | head -150Repository: fylein/fyle-mobile-app
Length of output: 3792
🏁 Script executed:
# Check if Phase 2 forkJoin has error handling in the subscribe call
sed -n '2000,2100p' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts | cat -nRepository: fylein/fyle-mobile-app
Length of output: 4763
🏁 Script executed:
# Check the complete subscribe signature - look for error handler
sed -n '2000,2150p' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts | grep -A 100 'forkJoin' | head -150Repository: fylein/fyle-mobile-app
Length of output: 4103
🏁 Script executed:
# Find where the subscribe call ends to see if there's an error handler
sed -n '2017,2200p' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts | tail -80Repository: fylein/fyle-mobile-app
Length of output: 2410
🏁 Script executed:
# Find the exact subscribe pattern - look for closing of subscribe with potential error handler
sed -n '2017,2130p' src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts | tail -50Repository: fylein/fyle-mobile-app
Length of output: 2122
Phase 2 forkJoin is a thunderbolt waiting to strike—one slow or erroring observable pulls down the entire secondary hydration.
RxJS forkJoin fails the combined observable if ANY source errors before completion. Your Phase 2 has no error handler, and you're joining 10 sources (three of them are nested forkJoin calls themselves). When one breaks—network timeout, service unavailable, whatever the heavens decree—the entire chain collapses. Projects, reports, cost centers, recent values: all vanish. The form appears but remains a skeleton.
For non-critical data like dropdowns and autofills, this all-or-nothing behavior is a weakness. Wrap the non-critical observables with catchError, returning fallback values (empty arrays for lists, null for objects), so the secondary data hydrates gracefully even when individual sources fail. Let etxn stand firm—it's the foundation. But the rest? They can fail alone without toppling the temple.
🛠️ Suggested resilience pattern
+ const safe = <T>(obs$: Observable<T>, fallback: T): Observable<T> =>
+ obs$.pipe(catchError(() => of(fallback)));
+
forkJoin({
- etxn: this.etxn$,
- project: selectedProject$,
- report: selectedReport$,
- costCenter: selectedCostCenter$,
- recentValue: this.recentlyUsedValues$,
- recentProjects: this.recentlyUsedProjects$,
- recentCostCenters: this.recentlyUsedCostCenters$,
- commuteDeductionDetails: commuteDeductionDetails$,
- employeeSettings: employeeSettings$,
- orgSettings: orgSettings$,
+ etxn: safe(this.etxn$, null),
+ project: safe(selectedProject$, null),
+ report: safe(selectedReport$, null),
+ costCenter: safe(selectedCostCenter$, null),
+ recentValue: safe(this.recentlyUsedValues$, null),
+ recentProjects: safe(this.recentlyUsedProjects$, []),
+ recentCostCenters: safe(this.recentlyUsedCostCenters$, []),
+ commuteDeductionDetails: safe(commuteDeductionDetails$, null),
+ employeeSettings: safe(employeeSettings$, null),
+ orgSettings: safe(orgSettings$, null),
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts` around lines 2004 -
2017, The forkJoin combining observables (forkJoin({ etxn: this.etxn$, project:
selectedProject$, report: selectedReport$, costCenter: selectedCostCenter$,
recentValue: this.recentlyUsedValues$, recentProjects:
this.recentlyUsedProjects$, recentCostCenters: this.recentlyUsedCostCenters$,
commuteDeductionDetails: commuteDeductionDetails$, employeeSettings:
employeeSettings$, orgSettings: orgSettings$ })) is brittle because any source
error will fail the whole join; update the non-critical sources (all except
this.etxn$) by piping them through catchError and returning safe fallbacks
(e.g., [] for lists, null for optional objects) before passing into forkJoin so
secondary hydration survives individual failures while keeping etxn as the
required source.
| if ( | ||
| doRecentCostCenterIdsExist && | ||
| (!etxn.tx.id || (etxn.tx.id && etxn.tx.state === 'DRAFT' && !etxn.tx.cost_center_id)) | ||
| ) { | ||
| const autoFillCostCenter = recentCostCenters && recentCostCenters.length > 0 && recentCostCenters[0]; | ||
|
|
||
| if (autoFillCostCenter) { | ||
| costCenter = autoFillCostCenter.value; | ||
| this.presetCostCenterId = autoFillCostCenter.value.id; | ||
| this.fg.patchValue({ costCenter }); | ||
| this.fg.patchValue({ costCenter: autoFillCostCenter.value }); | ||
| } |
There was a problem hiding this comment.
Line 2108 can undo cost-center autofill done at Line 2094.
You compute and patch autofilled costCenter, then final patch uses the older costCenter variable and can reset it (often to null). This can desync UI state and dependent fields.
🛠️ Suggested fix (mirror `effectiveProject` pattern)
+ let effectiveCostCenter = costCenter;
if (
doRecentCostCenterIdsExist &&
(!etxn.tx.id || (etxn.tx.id && etxn.tx.state === 'DRAFT' && !etxn.tx.cost_center_id))
) {
const autoFillCostCenter = recentCostCenters && recentCostCenters.length > 0 && recentCostCenters[0];
if (autoFillCostCenter) {
this.presetCostCenterId = autoFillCostCenter.value.id;
- this.fg.patchValue({ costCenter: autoFillCostCenter.value });
+ effectiveCostCenter = autoFillCostCenter.value;
+ this.fg.patchValue({ costCenter: effectiveCostCenter });
}
}
- this.fg.patchValue({ report, costCenter, project: effectiveProject ?? project }, { emitEvent: false });
+ this.fg.patchValue(
+ { report, costCenter: effectiveCostCenter ?? costCenter, project: effectiveProject ?? project },
+ { emitEvent: false },
+ );Also applies to: 2108-2108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/add-edit-mileage/add-edit-mileage.page.ts` around lines 2087 -
2095, The final fg.patchValue is overwriting the freshly autofilled costCenter
with the stale/older costCenter variable; change the patch logic in
add-edit-mileage.page.ts so it uses the computed autoFillCostCenter (and
this.presetCostCenterId) when present instead of the older costCenter local
variable—mirror the effectiveProject pattern: derive an effectiveCostCenter
(prefer autoFillCostCenter.value, fall back to existing fg.value.costCenter) and
patch that into fg and presetCostCenterId so the UI and dependent fields aren't
reset to null.
🔁 Code Duplication Report - Angular
📊 No significant change in code duplication. |
2 participants
Clickup
https://app.clickup.com
Code Coverage
Please add code coverage here
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit