fix: fy-number disable state #4002
Conversation
WalkthroughReconciliation check in the add-edit-expense page is now gated by a LaunchDarkly feature flag and executed asynchronously; the fy-number component now uses an Angular effect (registered after fc creation in ngOnInit) to synchronize its internal form control enabled/disabled state with the Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI / Component
participant LD as LaunchDarkly
participant Logic as AddEditExpense Logic
Note over UI,Logic: New flow when saving or loading expense
UI->>Logic: request reconciliation check (async)
Logic->>LD: fetch variation "reconciliation_beta"
LD-->>Logic: variation (true / false)
alt variation == true
Logic->>Logic: apply reconciliation handling (disable dateOfSpend, currencyObj)
else variation == false
Logic-->>Logic: skip reconciliation handling
end
Logic-->>UI: continue flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.component.{ts,html}📄 CodeRabbit inference engine (.cursor/rules/component-i18n-key-naming.mdc)
Files:
src/**/*.component.{ts,html}📄 CodeRabbit inference engine (.cursor/rules/component-i18n-key-naming.mdc)
Files:
🧠 Learnings (6)📓 Common learnings📚 Learning: 2024-10-23T11:45:16.974ZApplied to files:
📚 Learning: 2024-11-18T06:55:17.972ZApplied to files:
📚 Learning: 2024-11-04T13:10:36.868ZApplied to files:
📚 Learning: 2025-06-17T05:15:26.587ZApplied to files:
📚 Learning: 2024-11-13T06:25:08.957ZApplied to files:
⏰ 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). (4)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/app/fyle/add-edit-expense/add-edit-expense.page.ts(1 hunks)src/app/shared/components/fy-number/fy-number.component.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/fy-number/fy-number.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/fy-number/fy-number.component.ts
🧠 Learnings (3)
📚 Learning: 2024-11-18T06:55:17.972Z
Learnt from: sumrender
Repo: fylein/fyle-mobile-app PR: 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-expense/add-edit-expense.page.ts
📚 Learning: 2024-10-23T11:45:16.974Z
Learnt from: bistaastha
Repo: fylein/fyle-mobile-app PR: 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/add-edit-expense/add-edit-expense.page.ts
📚 Learning: 2024-11-04T13:10:36.868Z
Learnt from: arjunaj5
Repo: fylein/fyle-mobile-app PR: 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/add-edit-expense/add-edit-expense.page.ts
⏰ 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). (4)
- GitHub Check: build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
| const isReconciliationEnaled = this.launchDarklyService.getVariation('reconciliation_beta', false); | ||
| if (isReconciliationEnaled) { | ||
| this.isReconciledExpense.set(expense.is_reconciled); | ||
| if (this.isReconciledExpense()) { | ||
| this.fg.controls.dateOfSpend.disable(); | ||
| this.fg.controls.currencyObj.disable(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Thalaivaa! This feature flag logic has a serious problem, da!
Style la irukku, but three critical issues I'm spotting:
-
Typo in variable name:
isReconciliationEnaledshould beisReconciliationEnabled- even Superstar doesn't make spelling mistakes, machaan! -
Incorrect Observable usage:
getVariationreturns an Observable but you're using it synchronously. This is like trying to catch a bullet with your bare hands - only Rajini can do that! Based on the LaunchDarkly service pattern in this codebase, you need to subscribe to it. -
One-way street logic: Once you disable these controls, there's no logic to re-enable them if the expense becomes unreconciled or the feature flag changes. Controls can get permanently locked, boss!
Apply this diff to fix the synchronous Observable usage and typo:
- const isReconciliationEnaled = this.launchDarklyService.getVariation('reconciliation_beta', false);
- if (isReconciliationEnaled) {
- this.isReconciledExpense.set(expense.is_reconciled);
- if (this.isReconciledExpense()) {
- this.fg.controls.dateOfSpend.disable();
- this.fg.controls.currencyObj.disable();
- }
- }
+ this.launchDarklyService.getVariation('reconciliation_beta', false).subscribe((isReconciliationEnabled) => {
+ if (isReconciliationEnabled) {
+ this.isReconciledExpense.set(expense.is_reconciled);
+ if (this.isReconciledExpense()) {
+ this.fg.controls.dateOfSpend?.disable();
+ this.fg.controls.currencyObj?.disable();
+ }
+ }
+ });Also consider: You might want to add logic to re-enable these controls when the expense is not reconciled or when the feature flag is disabled. Right now it's a one-way ticket to Disabled Town!
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/app/fyle/add-edit-expense/add-edit-expense.page.ts around lines 2871 to
2878, rename the mistyped variable isReconciliationEnaled to
isReconciliationEnabled, change the synchronous call to
this.launchDarklyService.getVariation(...) to subscribe to the returned
Observable (or pipe takeUntil) and handle the emitted boolean; inside the
subscription set this.isReconciledExpense accordingly and then enable or disable
fg.controls.dateOfSpend and fg.controls.currencyObj based on the current
expense.is_reconciled value (disable when reconciled true, enable when false),
and ensure you react to future changes by either subscribing to
expense/is_reconciled changes or re-evaluating on feature-flag updates; also
store/teardown the subscription properly (e.g., add to a Subscription or use
takeUntil and clean up in ngOnDestroy) so controls aren’t permanently locked or
leaked.
| constructor() { | ||
| effect(() => { | ||
| const isDisabled = this.disabled(); | ||
| if (isDisabled !== undefined && this.fc) { | ||
| this.isDisabled = isDisabled; | ||
| if (isDisabled) { | ||
| this.fc.disable(); | ||
| } else if (this.fc.disabled) { | ||
| this.fc.enable(); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Machaan, this effect has a timing problem that even my style can't fix!
The constructor effect you've added has a critical race condition, boss:
-
Lifecycle timing issue: The constructor runs before
ngOnInit, butthis.fcis only initialized inngOnInit(line 156/159). When the effect first executes,this.fcisundefined, so the initialdisabled()value won't be applied. The logic only works for subsequent changes todisabled()- not the first time! -
Duplicate logic: You already have
setDisabledState()method (lines 120-127) that does exactly the same thing. Now you have two mechanisms fighting for control of the disabled state - this is like having two heroes in the same movie, unnecessary confusion! -
Incomplete synchronization: The
disabled()signal input and thesetDisabledState()method are separate paths. If the parent component callssetDisabledState(), the signal won't update. If the signal updates, great, but the initial value is missed.
Recommended fix: Move this logic to ngOnInit after fc is created, or use ngAfterViewInit. Here's the style way to do it:
constructor() {
- effect(() => {
- const isDisabled = this.disabled();
- if (isDisabled !== undefined && this.fc) {
- this.isDisabled = isDisabled;
- if (isDisabled) {
- this.fc.disable();
- } else if (this.fc.disabled) {
- this.fc.enable();
- }
- }
- });
}
ngOnInit(): void {
this.isIos = this.platform.is('ios');
// ... existing code ...
if (!this.isDistance) {
this.fc = new UntypedFormControl(null, Validators.pattern(/^-?(?:\d*\.\d+|\d+\.?)$/));
} else {
this.fc = new UntypedFormControl(null, Validators.pattern(/^\d*(\.\d+)?$/));
}
+
+ // Apply initial disabled state from signal input
+ const initialDisabledState = this.disabled();
+ if (initialDisabledState !== undefined) {
+ this.setDisabledState(initialDisabledState);
+ }
+
+ // Subscribe to changes in disabled signal
+ effect(() => {
+ const isDisabled = this.disabled();
+ if (isDisabled !== undefined) {
+ this.setDisabledState(isDisabled);
+ }
+ }, { injector: this.injector });
this.fc.valueChanges.subscribe((value) => {
// ... existing code ...
});
}Even better approach - just update the initial value in ngOnInit and rely on the existing setDisabledState method for everything else!
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/app/shared/components/fy-number/fy-number.component.ts around lines 39 to
51, the constructor effect runs before fc is initialized (ngOnInit), causing the
initial disabled() value to be missed and duplicating logic already present in
setDisabledState; remove the effect from the constructor, and instead apply the
initial disabled state after fc is created in ngOnInit (or ngAfterViewInit) and
rely on the existing setDisabledState method for subsequent updates so there is
a single authoritative path to enable/disable the control; ensure the signal and
setDisabledState remain synchronized (update the signal inside setDisabledState
or vice versa) and delete the duplicate constructor logic.
🔁 Code Duplication Report - Angular
📊 No significant change in code duplication. |
3 participants
Clickup
app.clickup.com
Code Coverage
Please add code coverage here
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit