Open
Conversation
amit429
requested review from
Chethan-Fyle,
arjunaj5 and
mvaishnavi
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughVaazhga! A complete in-app rating feature has arrived, my friend. New dependency added, AppRatingService orchestrates eligibility checks and native review triggers, UI components enhanced for icon display, localization strings added, and post-save prompt integration in AddEditExpensePage. Vera level implementation! Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AddEditExpensePage
participant AppRatingService
participant LaunchDarklyService
participant FeatureConfigService
participant PopoverController
participant AppReview
User->>AddEditExpensePage: Save Expense
AddEditExpensePage->>AppRatingService: attemptRatingPrompt()
AppRatingService->>LaunchDarklyService: Check IN_APP_RATING flag
AppRatingService->>AppRatingService: checkEligibility()
AppRatingService->>FeatureConfigService: getPromptHistory()
AppRatingService->>AppRatingService: Validate cooldowns & thresholds
alt Eligible for Rating
AppRatingService->>PopoverController: showPrePromptPopover()
PopoverController->>User: Display rating prompt
alt User accepts rating
User->>PopoverController: Select "Leave a Rating"
PopoverController->>AppRatingService: recordInteraction(nativePrompts)
AppRatingService->>AppReview: triggerNativeReview()
AppReview->>User: Open native app review
else User dismisses
User->>PopoverController: Select "Not Now"
PopoverController->>AppRatingService: recordInteraction(dismissals)
AppRatingService->>FeatureConfigService: Update AppRatingHistory
end
else Not Eligible
AppRatingService->>AddEditExpensePage: Return (no prompt shown)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/in-app-rating-engineering-doc.md`:
- Around line 11-26: The fenced ASCII diagram block that begins with
"┌─────────────────────────────────────────────────────┐" (and the similar
diagram/example fences later in the doc) lacks a language tag and triggers
markdownlint MD040; update each triple-backtick fence for those diagrams to use
the "text" language tag (e.g., replace ``` with ```text) so the markdown linter
stops flagging them—search for fences containing the ASCII box characters (e.g.,
"AppRatingService.attemptRatingPrompt()" diagram) and the other similar
diagram/example fences and add the text tag to each.
In `@src/app/core/services/app-rating.service.ts`:
- Around line 134-145: The getPromptHistory function currently swallows all
FeatureConfig read errors by returning a default empty AppRatingHistory in the
catchError, which causes cooldown checks to pass; remove the catchError that
returns of(defaultHistory) so errors from
featureConfigService.getConfiguration(...) propagate (or rethrow them) and allow
attemptRatingPrompt() to skip prompting on backend/cache failures; keep the
existing map((config) => config?.value || defaultHistory) behavior so only a
missing config value yields the empty history, but any read error from
getConfiguration will now bubble up.
- Around line 209-229: recordInteraction currently performs a read-modify-write
directly and can lose updates when invoked concurrently; change it to enqueue
intents and process them serially: add a private Subject (e.g.,
interactionQueue: Subject<'nativePrompts'|'dismissals'>) and, in the service
initialization, subscribe to interactionQueue.pipe(concatMap(type =>
this.getPromptHistory().pipe(take(1), map(history => { build updatedHistory },
concatMap(updatedHistory =>
this.featureConfigService.saveConfiguration<AppRatingHistory>({ feature:
this.FEATURE_KEY, key: this.CONFIG_KEY, value: updatedHistory, is_shared: false
})), catchError(() => of(null)))))) so all writes are serialized; then make
recordInteraction just call interactionQueue.next(type) instead of directly
calling getPromptHistory()/saveConfiguration().
- Around line 66-70: In checkEligibility() in AppRatingService, remove the
temporary return of(true) and restore the original eligibility pipeline
(re-enable the LaunchDarkly flag check, network/delegator reachability checks,
expense-count threshold check, and both cooldown checks) so the method evaluates
all conditions instead of always returning true; if any required component or
remote check is missing during restoration, make the fallback "fail closed"
(return Observable.of(false) or an error-to-false mapping) so the feature
remains disabled until the full pipeline is healthy.
In `@src/app/fyle/add-edit-expense/add-edit-expense.page.ts`:
- Around line 4008-4012: The post-save prompts are being called too early inside
checkIfReceiptIsMissingAndMandatory()'s finalize block; remove
triggerNpsSurvey() and triggerAppRating() from that finalize and keep only
hideSaveExpenseLoader() there, then invoke triggerNpsSurvey() and
triggerAppRating() from the successful completion path(s) of addExpense() and
editExpense() (i.e., after the expense save resolves/succeeds) so they only run
when the expense was actually saved; ensure calls remain in the success branch
and not in error/finalize, referencing checkIfReceiptIsMissingAndMandatory(),
addExpense(), editExpense(), hideSaveExpenseLoader(), triggerNpsSurvey(), and
triggerAppRating().
- Around line 4538-4542: The finalize() block currently calls
hideSaveExpenseLoader(), triggerNpsSurvey(), and triggerAppRating()
unconditionally so users see engagement prompts even on failed saves and may get
both prompts; move hideSaveExpenseLoader() to finalize() only, and call
triggerNpsSurvey() or triggerAppRating() only on a successful save path (e.g.,
inside the observable's success/next handler or a tap that checks a success
flag), using the save response to decide eligibility and enforce a single prompt
(pick one by priority or mutually-exclusive checks) before invoking
triggerNpsSurvey() or triggerAppRating() for functions/methods in this file like
hideSaveExpenseLoader(), triggerNpsSurvey(), triggerAppRating().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 034fb617-37ae-4a08-8890-66ed481b1f6b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
docs/in-app-rating-engineering-doc.mdpackage.jsonsrc/app/core/mock-data/ld-all-flags.data.tssrc/app/core/models/app-rating-history.model.tssrc/app/core/services/app-rating.service.tssrc/app/fyle/add-edit-expense/add-edit-expense.page.tssrc/assets/i18n/en.json
Comment on lines
+209
to
+229
| private recordInteraction(type: 'nativePrompts' | 'dismissals'): void { | ||
| this.getPromptHistory() | ||
| .pipe( | ||
| take(1), | ||
| switchMap((history) => { | ||
| const updatedHistory: AppRatingHistory = { | ||
| nativePrompts: [...(history?.nativePrompts || [])], | ||
| dismissals: [...(history?.dismissals || [])], | ||
| }; | ||
| updatedHistory[type].push(new Date().toISOString()); | ||
|
|
||
| return this.featureConfigService.saveConfiguration<AppRatingHistory>({ | ||
| feature: this.FEATURE_KEY, | ||
| key: this.CONFIG_KEY, | ||
| value: updatedHistory, | ||
| is_shared: false, | ||
| }); | ||
| }), | ||
| catchError(() => of(null)), | ||
| ) | ||
| .subscribe(); |
There was a problem hiding this comment.
Boss, this history update can lose one of the taps.
recordInteraction() does a read-modify-write with no serialization. If two prompt actions land close together, both calls can read the same old history and then overwrite each other with separate saveConfiguration() calls. Queue these writes with a single-flight/concatMap path, or move the merge server-side.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/core/services/app-rating.service.ts` around lines 209 - 229,
recordInteraction currently performs a read-modify-write directly and can lose
updates when invoked concurrently; change it to enqueue intents and process them
serially: add a private Subject (e.g., interactionQueue:
Subject<'nativePrompts'|'dismissals'>) and, in the service initialization,
subscribe to interactionQueue.pipe(concatMap(type =>
this.getPromptHistory().pipe(take(1), map(history => { build updatedHistory },
concatMap(updatedHistory =>
this.featureConfigService.saveConfiguration<AppRatingHistory>({ feature:
this.FEATURE_KEY, key: this.CONFIG_KEY, value: updatedHistory, is_shared: false
})), catchError(() => of(null)))))) so all writes are serialized; then make
recordInteraction just call interactionQueue.next(type) instead of directly
calling getPromptHistory()/saveConfiguration().
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/app/core/services/app-rating.service.ts (2)
211-231:⚠️ Potential issue | 🟠 MajorSerialize prompt-history updates.
saveConfiguration()overwrites the full config value, and this read-modify-write path is not serialized. If two prompt actions land close together, one timestamp can wipe out the other. Queue these writes through a singleconcatMappipeline, or move the merge server-side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/services/app-rating.service.ts` around lines 211 - 231, The read-modify-write in recordInteraction is racy because featureConfigService.saveConfiguration overwrites the whole config; replace the current take(1)+switchMap flow with a serialized write pipeline so concurrent calls are queued (e.g., maintain a Subject or Observable queue and use concatMap) that reads current history via getPromptHistory(), merges the new timestamp into the appropriate array (nativePrompts or dismissals), then calls featureConfigService.saveConfiguration; ensure the pipeline uses concatMap (or an internal single-stream Subject) to serialize writes and keep catchError to avoid breaking the queue.
71-75:⚠️ Potential issue | 🔴 CriticalRestore the real eligibility gate before release.
Line 74 still short-circuits the whole flow with
of(true), so every post-save trigger path bypasses the LaunchDarkly flag, connectivity/delegator checks, expense threshold, and both cooldowns. Superstar move: fail closed until the full pipeline is back.🛡️ Minimal safe fix
- return of(true); + return of(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/services/app-rating.service.ts` around lines 71 - 75, The checkEligibility() function currently short-circuits by returning of(true); remove that shortcut and restore the full eligibility pipeline: evaluate the LaunchDarkly feature flag (launchDarklyFlag), verify connectivity/delegator checks (isConnected(), isDelegatorEligible()), enforce the expense threshold (expenseAmount >= expenseAmountThreshold), and apply both user and org cooldown gates (userCooldown/organizationCooldown or lastPromptTimestamp checks), combining results into a single Observable<boolean> that fails closed (returns false) on errors or any failing check; ensure you keep the function name checkEligibility and observable semantics intact.src/app/fyle/add-edit-expense/add-edit-expense.page.ts (2)
4844-4850:⚠️ Potential issue | 🟠 MajorDon’t mark add flows as saved on placeholder emissions.
In
addExpense(), Line 4824 emitsof(addEntryAndSync(...)), and Lines 4832-4836 fireaddEntry(...).then(noop)before emittingnull. Thistap()therefore flipslastSaveSucceededbefore the outbox write settles, andtriggerPostSavePrompts()can run even when queueing fails. Returnfrom(...)for those promise branches and set the flag only after the promise resolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/fyle/add-edit-expense/add-edit-expense.page.ts` around lines 4844 - 4850, The tap() currently sets lastSaveSucceeded before the actual outbox write completes because addExpense() emits placeholder observables (of(addEntryAndSync(...)) and a null after calling addEntry(...).then(noop)); change those branches to return from(promise) (use from(addEntryAndSync(...)) and from(addEntry(...)) instead of of(...) and emitting null) so the Observable only completes when the promises resolve, and ensure the tap that sets this.lastSaveSucceeded runs after the promise-based from(...) resolves; keep triggerPostSavePrompts() and hideSaveExpenseLoader() in finalize() but only set lastSaveSucceeded after the successful resolution of addEntryAndSync/addEntry promise.
4551-4554:⚠️ Potential issue | 🟠 MajorDon’t launch the prompt flow from
finalize()in edit saves.Lines 4151, 4180, 4211, and 4239 navigate in the subscriber
nexthandlers. This teardown runs after that, so the rating popover or NPS survey can attach while the page is already leaving. MovetriggerPostSavePrompts()into the successful save path, not teardown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/fyle/add-edit-expense/add-edit-expense.page.ts` around lines 4551 - 4554, The teardown in the RxJS finalize() currently calls triggerPostSavePrompts() which runs after subscribers' next handlers (and navigation) and can attach prompts while the page is leaving; remove triggerPostSavePrompts() from the finalize() block (keep hideSaveExpenseLoader() there) and instead invoke triggerPostSavePrompts() explicitly in the successful save path inside the subscriber next/then handler after the save completes and any navigation logic (the same place where the existing navigation in the save success handlers occurs), so prompts are only launched on successful saves while the page is still active.
🤖 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-expense/add-edit-expense.page.ts`:
- Around line 3959-3972: The current subscription to
appRatingService.checkEligibility() can abort on error and skip both rating and
NPS flows; wrap the eligibility observable with a catchError that maps errors to
of(false) before take(1)/subscribe so failures are treated as “not eligible”
(same behavior as AppRatingService.attemptRatingPrompt()). In practice, update
the chain around appRatingService.checkEligibility() to pipe(..., catchError(()
=> of(false)), take(1)) and then keep the existing subscribe logic that calls
appRatingService.showRatingPrompt() or falls back to
launchDarklyService.getVariation('nps_survey', false) and
refinerService.startSurvey({ actionName: 'Save Expense' }).
---
Duplicate comments:
In `@src/app/core/services/app-rating.service.ts`:
- Around line 211-231: The read-modify-write in recordInteraction is racy
because featureConfigService.saveConfiguration overwrites the whole config;
replace the current take(1)+switchMap flow with a serialized write pipeline so
concurrent calls are queued (e.g., maintain a Subject or Observable queue and
use concatMap) that reads current history via getPromptHistory(), merges the new
timestamp into the appropriate array (nativePrompts or dismissals), then calls
featureConfigService.saveConfiguration; ensure the pipeline uses concatMap (or
an internal single-stream Subject) to serialize writes and keep catchError to
avoid breaking the queue.
- Around line 71-75: The checkEligibility() function currently short-circuits by
returning of(true); remove that shortcut and restore the full eligibility
pipeline: evaluate the LaunchDarkly feature flag (launchDarklyFlag), verify
connectivity/delegator checks (isConnected(), isDelegatorEligible()), enforce
the expense threshold (expenseAmount >= expenseAmountThreshold), and apply both
user and org cooldown gates (userCooldown/organizationCooldown or
lastPromptTimestamp checks), combining results into a single Observable<boolean>
that fails closed (returns false) on errors or any failing check; ensure you
keep the function name checkEligibility and observable semantics intact.
In `@src/app/fyle/add-edit-expense/add-edit-expense.page.ts`:
- Around line 4844-4850: The tap() currently sets lastSaveSucceeded before the
actual outbox write completes because addExpense() emits placeholder observables
(of(addEntryAndSync(...)) and a null after calling addEntry(...).then(noop));
change those branches to return from(promise) (use from(addEntryAndSync(...))
and from(addEntry(...)) instead of of(...) and emitting null) so the Observable
only completes when the promises resolve, and ensure the tap that sets
this.lastSaveSucceeded runs after the promise-based from(...) resolves; keep
triggerPostSavePrompts() and hideSaveExpenseLoader() in finalize() but only set
lastSaveSucceeded after the successful resolution of addEntryAndSync/addEntry
promise.
- Around line 4551-4554: The teardown in the RxJS finalize() currently calls
triggerPostSavePrompts() which runs after subscribers' next handlers (and
navigation) and can attach prompts while the page is leaving; remove
triggerPostSavePrompts() from the finalize() block (keep hideSaveExpenseLoader()
there) and instead invoke triggerPostSavePrompts() explicitly in the successful
save path inside the subscriber next/then handler after the save completes and
any navigation logic (the same place where the existing navigation in the save
success handlers occurs), so prompts are only launched on successful saves while
the page is still active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81671e9a-5bbd-4eaf-b9c1-4e1253474d39
📒 Files selected for processing (2)
src/app/core/services/app-rating.service.tssrc/app/fyle/add-edit-expense/add-edit-expense.page.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/fyle/add-edit-expense/add-edit-expense.page.ts (1)
5440-5554:⚠️ Potential issue | 🟡 MinorDei, this personal card save flow is missing the rating dialogue!
The
saveAndMatchWithPersonalCardTxn()method creates expenses but doesn't trigger the post-save prompts. While the other save flows (addExpense,editExpense) now have the rating/NPS orchestration, this path alone stays silent, machan.If personal card matching should also prompt for app rating or NPS survey, the same
lastSaveSucceeded+triggerPostSavePrompts()pattern needs to be applied here in the finalize block. If the exclusion is intentional (perhaps personal card flows shouldn't interrupt with prompts), that's a different matter—but there's no documentation confirming this. Best to align the approaches or document the reason, sir.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/fyle/add-edit-expense/add-edit-expense.page.ts` around lines 5440 - 5554, The saveAndMatchWithPersonalCardTxn() flow doesn't trigger post-save prompts (rating/NPS); update the success/finalize path to mirror addExpense/editExpense by setting lastSaveSucceeded = true when the upsert+match/upload completes successfully and then calling triggerPostSavePrompts() so the rating dialogue runs; specifically, in saveAndMatchWithPersonalCardTxn() reference the finalize/subscription success branch (after transactionService.upsert and personalCardsService.matchExpense/uploadAttachments) and ensure lastSaveSucceeded and triggerPostSavePrompts() are invoked on success (not only on error), similar to the other save flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/app/fyle/add-edit-expense/add-edit-expense.page.ts`:
- Around line 5440-5554: The saveAndMatchWithPersonalCardTxn() flow doesn't
trigger post-save prompts (rating/NPS); update the success/finalize path to
mirror addExpense/editExpense by setting lastSaveSucceeded = true when the
upsert+match/upload completes successfully and then calling
triggerPostSavePrompts() so the rating dialogue runs; specifically, in
saveAndMatchWithPersonalCardTxn() reference the finalize/subscription success
branch (after transactionService.upsert and
personalCardsService.matchExpense/uploadAttachments) and ensure
lastSaveSucceeded and triggerPostSavePrompts() are invoked on success (not only
on error), similar to the other save flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff3c22fb-b810-47c7-abb8-c7bc0d5f057d
📒 Files selected for processing (1)
src/app/fyle/add-edit-expense/add-edit-expense.page.ts
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
amit429
force-pushed
the
FYLE-in-app-rating
branch
from
65925e4 to
343e0fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/fyle/view-team-report/view-team-report.page.ts (1)
628-639:⚠️ Potential issue | 🟠 MajorBoss, move the redirect into the
sendBack()success path.Line 639 runs immediately after subscribing, so this page exits even when
sendBack()fails. That can make a failed mutation look successful to the user.⚡ Proposed fix
if (data && data.comment) { this.approverReportsService .sendBack(this.activatedRoute.snapshot.params.id as string, data.comment) .subscribe(() => { const message = 'Report Sent Back successfully'; this.matSnackBar.openFromComponent(ToastMessageComponent, { ...this.snackbarProperties.setSnackbarProperties('success', { message }), panelClass: ['msb-success-with-camera-icon'], }); this.trackingService.showToastMessage({ ToastContent: message }); + this.router.navigate(['/', 'enterprise', 'team_reports'], { replaceUrl: true }); }); - this.router.navigate(['/', 'enterprise', 'team_reports'], { replaceUrl: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/fyle/view-team-report/view-team-report.page.ts` around lines 628 - 639, The router.navigate call is executed immediately after subscribing, so the page redirects regardless of sendBack() success; move the navigation into the approverReportsService.sendBack(...).subscribe success callback (the existing arrow function that opens the snackbar and calls trackingService.showToastMessage) so navigation happens only on success, and add an error handler to the subscribe (or a second callback) to avoid redirect on failure and to surface an error to the user; update references: approverReportsService.sendBack, activatedRoute.snapshot.params.id, the subscribe success callback, and router.navigate.src/app/fyle/add-edit-expense/add-edit-expense-1.spec.ts (1)
265-274: 🧹 Nitpick | 🔵 TrivialAdd coverage for the new post-save prompt gating, boss.
This assertion update is fine, but the dangerous logic added in this PR is the rating-vs-NPS orchestration. Please cover at least: rating winning over NPS, no prompt on failed save, and the personal-card save path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/fyle/add-edit-expense/add-edit-expense-1.spec.ts` around lines 265 - 274, Add unit tests in add-edit-expense-1.spec.ts to cover the new post-save prompt gating: (1) add a test that when both rating and NPS eligibility are true the rating prompt is chosen — stub the service/selector that returns prompt type (or spy on the method used by component to decide prompt) and assert the component triggers the rating flow after save; (2) add a test that a failed save does not open any post-save prompt — simulate save failure (spy on save/saveExpense to return rejected observable/promise) and assert no prompt/navigation occurs; (3) add a test for the personal-card save path — set up the component as a personal card save, call the save method (or component.goBack when appropriate), and assert the router.navigate/other side-effects match the personal-card flow. Use existing symbols like component.saveExpense, component.goBack, component.isRedirectedFromReport and the same router.navigate spy to locate and verify behavior.
♻️ Duplicate comments (2)
src/app/core/services/app-rating.service.ts (2)
71-75:⚠️ Potential issue | 🔴 CriticalBoss, this ships the rating prompt permanently ON.
Line 74 bypasses the LD flag, connectivity, delegator, account-age, cooldown, and minimum-expense checks, so every trigger path becomes eligible in production.
🛡️ Safe minimum fix
- return of(true); + return of(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/services/app-rating.service.ts` around lines 71 - 75, The checkEligibility method currently bypasses all logic by returning of(true); revert this to re-enable the original eligibility pipeline: remove the hardcoded return and restore the combined Observable that checks the LaunchDarkly flag (feature flag), connectivity status, delegator status, account age, cooldown timer, and minimum-expense rules used before the change (the checkEligibility function in app-rating.service.ts). Ensure the method returns the composed Observable<boolean> that ANDs those checks (using existing helper methods/observables in this service) so production respects LD flag, connectivity, delegator, account-age, cooldown, and minimum-expense checks instead of always true.
211-231:⚠️ Potential issue | 🟠 MajorBoss, this history write can still drop one interaction.
recordInteraction()is still a read-modify-write with no serialization. Two prompt actions close together can read the same old history and overwrite one timestamp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/services/app-rating.service.ts` around lines 211 - 231, recordInteraction currently does a concurrent read-modify-write (getPromptHistory -> saveConfiguration) which can lose updates; instead serialize all interaction writes: create an internal interaction queue (e.g., a Subject or BehaviorSubject) and have a single processor pipeline that consumes that queue with concatMap, where for each queued type you call getPromptHistory(), build updatedHistory, then call featureConfigService.saveConfiguration with FEATURE_KEY and CONFIG_KEY; this guarantees sequential writes and prevents races (alternatively implement a CAS/retry using a versioned saveConfiguration if available), and update recordInteraction to push the interaction type into that queue rather than performing the read/save inline.
🤖 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/core/services/app-rating.service.ts`:
- Around line 191-194: The code records a cooldown via
recordInteraction('nativePrompts') even when triggerNativeReview() fails (the
plugin's requestReview may throw or not open a sheet), so change the flow in the
handlers that call triggerNativeReview() (lines around the current block and the
similar block at 201-208) to only call recordInteraction('nativePrompts') when
triggerNativeReview() actually succeeded/indicated the native sheet opened: have
triggerNativeReview() return a boolean or resolved value that signifies success
(or propagate the thrown error), await it, and conditionally call
recordInteraction('nativePrompts') only on a truthy success result; also ensure
exceptions from requestReview() are not swallowed so the conditional can act on
failures.
In `@src/app/fyle/add-edit-expense/add-edit-expense.page.ts`:
- Around line 4847-4853: The code sets lastSaveSucceeded = true in a tap() that
runs even when addExpense() emits a synchronous of(null) while addEntry(...) is
still pending, so a failed outbox write can leave lastSaveSucceeded true and
still trigger triggerPostSavePrompts(); fix by moving the success flag update to
run only after addEntry(...) actually resolves: modify addExpense()/the
observable chain so the emission waits for addEntry(...) to complete (or attach
a .then() to addEntry(...) and set this.lastSaveSucceeded = true inside that
fulfilled callback), and remove the premature tap() assignment; ensure
triggerPostSavePrompts() is still called only after the true save path (i.e.,
after addEntry resolves) and keep hideSaveExpenseLoader() in finalize().
- Around line 3953-3975: The personal-card save path
saveAndMatchWithPersonalCardTxn currently doesn't trigger post-save prompts;
modify its success branch to set this.lastSaveSucceeded = true and call
this.triggerPostSavePrompts() (same behavior as the other save success path).
Locate saveAndMatchWithPersonalCardTxn and add these two lines in the successful
completion/subscribe/then handler after the expense is persisted so users get
the appRatingService / refiner NPS flow.
In `@src/app/fyle/my-view-report/my-view-report.page.spec.ts`:
- Line 222: Add a spec that intercepts the callback passed into
Platform.registerBackButtonAction inside the ionViewWillEnter() flow and asserts
navigation behavior: spy on the mock NavController created with
jasmine.createSpyObj('NavController', ['back', 'push']) and capture the handler
argument passed to registerBackButtonAction (from the Platform mock), then
invoke that handler and verify navController.back() is called (or the fallback
push behavior is invoked when back() is not available); reference
ionViewWillEnter(), registerBackButtonAction(), and the NavController spy to
locate and implement this assertion.
In `@src/app/fyle/my-view-report/my-view-report.page.ts`:
- Around line 541-545: The hardware back-button handler registered via
platformHandlerService.registerBackButtonAction (hardwareBackButtonAction,
BackButtonActionPriority.MEDIUM) currently always calls
this.navController.back(), which can exit the app if there's no in-app history;
update the handler to check the existing navigateBack context (the navigateBack
value captured earlier on line ~411) and call that when available, otherwise
route to the safe fallback route (navigate to 'my_reports') instead of blindly
calling navController.back(); ensure you reference hardwareBackButtonAction,
registerBackButtonAction, BackButtonActionPriority.MEDIUM, navigateBack and
'my_reports' in the change.
In `@src/app/fyle/split-expense/split-expense.page.ts`:
- Line 1140: The optional chaining on
this.hardwareBackButtonAction?.unsubscribe() is inconsistent with other pages
that call this.hardwareBackButtonAction.unsubscribe() directly; update the code
in ionViewWillLeave to call this.hardwareBackButtonAction.unsubscribe() without
the ?. to match the pattern used in add-edit-per-diem.page.ts, relying on
ionViewWillEnter having created the subscription; ensure the symbol
this.hardwareBackButtonAction and the lifecycle hooks
ionViewWillEnter/ionViewWillLeave remain unchanged.
In `@src/app/fyle/view-team-report/view-team-report.page.spec.ts`:
- Around line 233-237: The tests register a hardware back-button handler via
Platform.backButton.subscribeWithPriority but never capture or invoke the
registered callback, so add behavior tests that grab the callback passed into
Platform.backButton.subscribeWithPriority (mock Platform.backButton as an object
with subscribeWithPriority spy that returns a Subscription mock and captures its
handler), then call that captured handler and assert NavController.back() was
invoked; update the spec(s) around the setup for ViewTeamReportPage to (1)
provide a Platform mock whose backButton.subscribeWithPriority stores the passed
callback, (2) after component initialization invoke that stored callback, and
(3) expect(navController.back).toHaveBeenCalled() to validate the wiring.
---
Outside diff comments:
In `@src/app/fyle/add-edit-expense/add-edit-expense-1.spec.ts`:
- Around line 265-274: Add unit tests in add-edit-expense-1.spec.ts to cover the
new post-save prompt gating: (1) add a test that when both rating and NPS
eligibility are true the rating prompt is chosen — stub the service/selector
that returns prompt type (or spy on the method used by component to decide
prompt) and assert the component triggers the rating flow after save; (2) add a
test that a failed save does not open any post-save prompt — simulate save
failure (spy on save/saveExpense to return rejected observable/promise) and
assert no prompt/navigation occurs; (3) add a test for the personal-card save
path — set up the component as a personal card save, call the save method (or
component.goBack when appropriate), and assert the router.navigate/other
side-effects match the personal-card flow. Use existing symbols like
component.saveExpense, component.goBack, component.isRedirectedFromReport and
the same router.navigate spy to locate and verify behavior.
In `@src/app/fyle/view-team-report/view-team-report.page.ts`:
- Around line 628-639: The router.navigate call is executed immediately after
subscribing, so the page redirects regardless of sendBack() success; move the
navigation into the approverReportsService.sendBack(...).subscribe success
callback (the existing arrow function that opens the snackbar and calls
trackingService.showToastMessage) so navigation happens only on success, and add
an error handler to the subscribe (or a second callback) to avoid redirect on
failure and to surface an error to the user; update references:
approverReportsService.sendBack, activatedRoute.snapshot.params.id, the
subscribe success callback, and router.navigate.
---
Duplicate comments:
In `@src/app/core/services/app-rating.service.ts`:
- Around line 71-75: The checkEligibility method currently bypasses all logic by
returning of(true); revert this to re-enable the original eligibility pipeline:
remove the hardcoded return and restore the combined Observable that checks the
LaunchDarkly flag (feature flag), connectivity status, delegator status, account
age, cooldown timer, and minimum-expense rules used before the change (the
checkEligibility function in app-rating.service.ts). Ensure the method returns
the composed Observable<boolean> that ANDs those checks (using existing helper
methods/observables in this service) so production respects LD flag,
connectivity, delegator, account-age, cooldown, and minimum-expense checks
instead of always true.
- Around line 211-231: recordInteraction currently does a concurrent
read-modify-write (getPromptHistory -> saveConfiguration) which can lose
updates; instead serialize all interaction writes: create an internal
interaction queue (e.g., a Subject or BehaviorSubject) and have a single
processor pipeline that consumes that queue with concatMap, where for each
queued type you call getPromptHistory(), build updatedHistory, then call
featureConfigService.saveConfiguration with FEATURE_KEY and CONFIG_KEY; this
guarantees sequential writes and prevents races (alternatively implement a
CAS/retry using a versioned saveConfiguration if available), and update
recordInteraction to push the interaction type into that queue rather than
performing the read/save inline.
🪄 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: dcaa93ee-6fb4-4105-a806-34218ea97408
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
src/app/core/services/app-rating.service.tssrc/app/fyle/add-edit-expense/add-edit-expense-1.spec.tssrc/app/fyle/add-edit-expense/add-edit-expense.page.tssrc/app/fyle/add-edit-mileage/add-edit-mileage-1.spec.tssrc/app/fyle/add-edit-mileage/add-edit-mileage.page.tssrc/app/fyle/add-edit-per-diem/add-edit-per-diem-1.page.spec.tssrc/app/fyle/add-edit-per-diem/add-edit-per-diem.page.tssrc/app/fyle/dashboard/dashboard.page.spec.tssrc/app/fyle/dashboard/dashboard.page.tssrc/app/fyle/my-view-report/my-view-report.page.spec.tssrc/app/fyle/my-view-report/my-view-report.page.tssrc/app/fyle/split-expense/split-expense.page.spec.tssrc/app/fyle/split-expense/split-expense.page.tssrc/app/fyle/view-team-report/view-team-report.page.spec.tssrc/app/fyle/view-team-report/view-team-report.page.tssrc/app/shared/components/capture-receipt/capture-receipt.component.spec.tssrc/app/shared/components/capture-receipt/capture-receipt.component.ts
Comment on lines
+191
to
+194
| if (data?.action === 'rate') { | ||
| this.trackingService.eventTrack('In App Rating Accepted', {}); | ||
| await this.triggerNativeReview(); | ||
| this.recordInteraction('nativePrompts'); |
There was a problem hiding this comment.
Boss, don’t start the 180-day cooldown when native review never opened.
triggerNativeReview() swallows plugin failures, but Line 194 still records nativePrompts. If requestReview() throws, the user never sees the native sheet and still gets cooled down for six months.
⚡ Proposed fix
- await this.triggerNativeReview();
- this.recordInteraction('nativePrompts');
+ const nativeReviewTriggered = await this.triggerNativeReview();
+ if (nativeReviewTriggered) {
+ this.recordInteraction('nativePrompts');
+ }
} else {
this.trackingService.eventTrack('In App Rating Dismissed', {});
this.recordInteraction('dismissals');
}
}
- private async triggerNativeReview(): Promise<void> {
+ private async triggerNativeReview(): Promise<boolean> {
try {
await AppReview.requestReview();
this.trackingService.eventTrack('In App Rating Native Triggered', {});
+ return true;
} catch (error: unknown) {
const message = error instanceof Error ? error.message : 'Unknown error';
this.trackingService.eventTrack('In App Rating Native Triggered Failed', { error: message });
+ return false;
}
}Also applies to: 201-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/core/services/app-rating.service.ts` around lines 191 - 194, The code
records a cooldown via recordInteraction('nativePrompts') even when
triggerNativeReview() fails (the plugin's requestReview may throw or not open a
sheet), so change the flow in the handlers that call triggerNativeReview()
(lines around the current block and the similar block at 201-208) to only call
recordInteraction('nativePrompts') when triggerNativeReview() actually
succeeded/indicated the native sheet opened: have triggerNativeReview() return a
boolean or resolved value that signifies success (or propagate the thrown
error), await it, and conditionally call recordInteraction('nativePrompts') only
on a truthy success result; also ensure exceptions from requestReview() are not
swallowed so the conditional can act on failures.
Comment on lines
+3953
to
+3975
| triggerPostSavePrompts(): void { | ||
| if (!this.lastSaveSucceeded) { | ||
| return; | ||
| } | ||
| this.lastSaveSucceeded = false; | ||
|
|
||
| this.appRatingService | ||
| .checkEligibility() | ||
| .pipe( | ||
| take(1), | ||
| catchError(() => of(false)), | ||
| ) | ||
| .subscribe((isRatingEligible) => { | ||
| if (isRatingEligible) { | ||
| this.appRatingService.showRatingPrompt(); | ||
| } else { | ||
| this.launchDarklyService.getVariation('nps_survey', false).subscribe((showNpsSurvey) => { | ||
| if (showNpsSurvey) { | ||
| this.refinerService.startSurvey({ actionName: 'Save Expense' }); | ||
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Wire this prompt flow into the personal-card save path too.
Boss, saveAndMatchWithPersonalCardTxn() is also a successful expense-save route, but it never sets lastSaveSucceeded or calls triggerPostSavePrompts(). Users creating an expense from a personal-card match will miss both the rating prompt and the NPS fallback.
🛠️ Possible fix
)
.subscribe(() => {
+ this.lastSaveSucceeded = true;
+ this.triggerPostSavePrompts();
this.showSnackBarToast({ message: 'Expense created successfully.' }, 'success', ['msb-success']);
this.router.navigate(['/', 'enterprise', 'personal_cards'], { queryParams: { refresh: true } });
this.trackingService.newExpenseCreatedFromPersonalCard();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/add-edit-expense/add-edit-expense.page.ts` around lines 3953 -
3975, The personal-card save path saveAndMatchWithPersonalCardTxn currently
doesn't trigger post-save prompts; modify its success branch to set
this.lastSaveSucceeded = true and call this.triggerPostSavePrompts() (same
behavior as the other save success path). Locate saveAndMatchWithPersonalCardTxn
and add these two lines in the successful completion/subscribe/then handler
after the expense is persisted so users get the appRatingService / refiner NPS
flow.
Comment on lines
+4847
to
4853
| tap(() => { | ||
| this.lastSaveSucceeded = true; | ||
| }), | ||
| finalize(() => { | ||
| this.hideSaveExpenseLoader(); | ||
| this.triggerNpsSurvey(); | ||
| this.triggerPostSavePrompts(); | ||
| }), |
There was a problem hiding this comment.
Don’t mark the save as successful before addEntry() actually resolves.
Thalaiva, this tap() runs even on the branch at Line 4835-Line 4839 where addExpense() fires addEntry(...).then(noop) and immediately emits of(null). If that outbox write rejects, lastSaveSucceeded is already true and the rating/NPS flow still runs on a failed save.
🧰 Safer change
- this.transactionOutboxService
- .addEntry(etxn.tx, etxn.dataUrls as { url: string; type: string }[], comments)
- .then(noop);
-
- return of(null);
+ return from(
+ this.transactionOutboxService.addEntry(
+ etxn.tx,
+ etxn.dataUrls as { url: string; type: string }[],
+ comments,
+ ),
+ ).pipe(map(() => null));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/add-edit-expense/add-edit-expense.page.ts` around lines 4847 -
4853, The code sets lastSaveSucceeded = true in a tap() that runs even when
addExpense() emits a synchronous of(null) while addEntry(...) is still pending,
so a failed outbox write can leave lastSaveSucceeded true and still trigger
triggerPostSavePrompts(); fix by moving the success flag update to run only
after addEntry(...) actually resolves: modify addExpense()/the observable chain
so the emission waits for addEntry(...) to complete (or attach a .then() to
addEntry(...) and set this.lastSaveSucceeded = true inside that fulfilled
callback), and remove the premature tap() assignment; ensure
triggerPostSavePrompts() is still called only after the true save path (i.e.,
after addEntry resolves) and keep hideSaveExpenseLoader() in finalize().
| useValue: launchDarklyServiceSpy, | ||
| }, | ||
| { provide: NavController, useValue: { push: NavController.prototype.back } }, | ||
| { provide: NavController, useValue: jasmine.createSpyObj('NavController', ['back', 'push']) }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test the new hardware-back path too, boss.
This mock only fixes DI. The changed callback in ionViewWillEnter() still has no assertion, so a back-navigation regression will pass the suite quietly. Please add one spec that captures the handler from registerBackButtonAction() and verifies the expected NavController/fallback behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/my-view-report/my-view-report.page.spec.ts` at line 222, Add a
spec that intercepts the callback passed into Platform.registerBackButtonAction
inside the ionViewWillEnter() flow and asserts navigation behavior: spy on the
mock NavController created with jasmine.createSpyObj('NavController', ['back',
'push']) and capture the handler argument passed to registerBackButtonAction
(from the Platform mock), then invoke that handler and verify
navController.back() is called (or the fallback push behavior is invoked when
back() is not available); reference ionViewWillEnter(),
registerBackButtonAction(), and the NavController spy to locate and implement
this assertion.
Comment on lines
541
to
545
| this.hardwareBackButtonAction = this.platformHandlerService.registerBackButtonAction( | ||
| BackButtonActionPriority.MEDIUM, | ||
| () => { | ||
| this.router.navigate(['/', 'enterprise', 'my_reports']); | ||
| this.navController.back(); | ||
| }, |
There was a problem hiding this comment.
Back button needs a fallback, boss.
This now always pops history. When the page is opened from a push/deep-link path or any flow that already used replaceUrl, there may be no prior in-app entry, so Android back can exit instead of taking the user to a safe screen. Line 411 already captures navigateBack; use that context here, or route to my_reports as a fallback.
⚙️ Suggested fix
this.hardwareBackButtonAction = this.platformHandlerService.registerBackButtonAction(
BackButtonActionPriority.MEDIUM,
() => {
- this.navController.back();
+ if (this.navigateBack) {
+ this.navController.back();
+ } else {
+ this.router.navigate(['/', 'enterprise', 'my_reports'], { replaceUrl: true });
+ }
},
);📝 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.
Suggested change
| this.hardwareBackButtonAction = this.platformHandlerService.registerBackButtonAction( | |
| BackButtonActionPriority.MEDIUM, | |
| () => { | |
| this.router.navigate(['/', 'enterprise', 'my_reports']); | |
| this.navController.back(); | |
| }, | |
| this.hardwareBackButtonAction = this.platformHandlerService.registerBackButtonAction( | |
| BackButtonActionPriority.MEDIUM, | |
| () => { | |
| if (this.navigateBack) { | |
| this.navController.back(); | |
| } else { | |
| this.router.navigate(['/', 'enterprise', 'my_reports'], { replaceUrl: true }); | |
| } | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/my-view-report/my-view-report.page.ts` around lines 541 - 545,
The hardware back-button handler registered via
platformHandlerService.registerBackButtonAction (hardwareBackButtonAction,
BackButtonActionPriority.MEDIUM) currently always calls
this.navController.back(), which can exit the app if there's no in-app history;
update the handler to check the existing navigateBack context (the navigateBack
value captured earlier on line ~411) and call that when available, otherwise
route to the safe fallback route (navigate to 'my_reports') instead of blindly
calling navController.back(); ensure you reference hardwareBackButtonAction,
registerBackButtonAction, BackButtonActionPriority.MEDIUM, navigateBack and
'my_reports' in the change.
| } | ||
|
|
||
| ionViewWillLeave(): void { | ||
| this.hardwareBackButtonAction?.unsubscribe(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional chaining on unsubscribe - safe but slightly inconsistent.
The optional chaining ?.unsubscribe() is defensive coding. However, other pages like add-edit-per-diem.page.ts call this.hardwareBackButtonAction.unsubscribe() directly without optional chaining. Since ionViewWillEnter always runs before ionViewWillLeave, the subscription will exist. Consider aligning with the existing pattern for consistency, but this is not a bug - just a style nit!
♻️ Suggested alignment with existing pattern
ionViewWillLeave(): void {
- this.hardwareBackButtonAction?.unsubscribe();
+ this.hardwareBackButtonAction.unsubscribe();
if (this.splitExpenseData) {
this.splitExpenseData.unsubscribe();
}
}📝 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.
Suggested change
| this.hardwareBackButtonAction?.unsubscribe(); | |
| ionViewWillLeave(): void { | |
| this.hardwareBackButtonAction.unsubscribe(); | |
| if (this.splitExpenseData) { | |
| this.splitExpenseData.unsubscribe(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/split-expense/split-expense.page.ts` at line 1140, The optional
chaining on this.hardwareBackButtonAction?.unsubscribe() is inconsistent with
other pages that call this.hardwareBackButtonAction.unsubscribe() directly;
update the code in ionViewWillLeave to call
this.hardwareBackButtonAction.unsubscribe() without the ?. to match the pattern
used in add-edit-per-diem.page.ts, relying on ionViewWillEnter having created
the subscription; ensure the symbol this.hardwareBackButtonAction and the
lifecycle hooks ionViewWillEnter/ionViewWillLeave remain unchanged.
Comment on lines
+233
to
+237
| { | ||
| provide: NavController, | ||
| useValue: jasmine.createSpyObj('NavController', ['back']), | ||
| }, | ||
| Platform, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Boss, the new hardware-back path still has no behavior test.
These specs verify unsubscribe() on leave, but they never capture the callback registered via subscribeWithPriority() and assert that it calls navController.back(). A broken back-button wiring will still pass this suite.
Also applies to: 270-271, 381-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/fyle/view-team-report/view-team-report.page.spec.ts` around lines 233
- 237, The tests register a hardware back-button handler via
Platform.backButton.subscribeWithPriority but never capture or invoke the
registered callback, so add behavior tests that grab the callback passed into
Platform.backButton.subscribeWithPriority (mock Platform.backButton as an object
with subscribeWithPriority spy that returns a Subscription mock and captures its
handler), then call that captured handler and assert NavController.back() was
invoked; update the spec(s) around the setup for ViewTeamReportPage to (1)
provide a Platform mock whose backButton.subscribeWithPriority stores the passed
callback, (2) after component initialization invoke that stored callback, and
(3) expect(navController.back).toHaveBeenCalled() to validate the wiring.
|
🔁 Code Duplication Report - Angular
🎉 This PR reduces duplicated code by 0.02%! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clickup
https://app.clickup.com
Code Coverage
Please add code coverage here
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit
Release Notes
New Features
Chores