fix: q3_usability clone-settings page alignment fixes for qbo & xero #1379
Conversation
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
WalkthroughTemplates for QBO and Xero clone settings were structurally refactored with Tailwind-styled containers. Shared components adjusted padding/structure, simplified dropdown item layout, and removed a localization key. New SCSS utility class added for consistent container styling. No form control names, bindings, or conditional logic changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/shared/components/configuration/configuration-import-field/configuration-import-field.component.html (2)
31-31: DuplicateisDestinationFixedImportkeys in[ngClass]– simplify for readabilityBoth key–value pairs check the same condition. Collapse them to avoid redundant evaluations and keep the template tidy:
-[ngClass]="{'tw-mt-24-px tw-pb-24-px': isDestinationFixedImport, 'tw-rounded-lg tw-border-border-tertiary tw-border': isDestinationFixedImport}" +[ngClass]="{'tw-mt-24-px tw-pb-24-px tw-rounded-lg tw-border-border-tertiary tw-border': isDestinationFixedImport}"
34-35: Conditional padding class is hard to scan
[ngClass]="!isCloneSettingView ? 'tw-p-24-px' : 'tw-px-24-px'"mixes the ternary with static classes inclass="".
Readability improves when the conditional part is isolated in an object / array:<div class="tw-flex tw-justify-between" [ngClass]="isCloneSettingView ? 'tw-px-24-px' : 'tw-p-24-px'">Optional refactor, no functional impact.
src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.html (1)
464-472: Duplicate[isFieldMandatory]attribute – drop the extra oneAngular keeps the last duplicate, but the repetition is noise and can confuse maintainers.
- [isFieldMandatory]="true" [options]="scheduleIntervalHours" [form]="advancedSettingForm" - [isFieldMandatory]="true" + [isFieldMandatory]="true"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.html(1 hunks)src/app/shared/components/configuration/configuration-import-field/configuration-import-field.component.html(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-10-08T15:51:28.972Z
Learning: The user ashwin1111 has confirmed the suggestion to replace the hardcoded reference to 'Sage Intacct' with a dynamic reference to the target system using the variable `targetSystemName` in the subLabel string within the `business-central-export-settings.component.html` file.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-06-10T19:13:15.470Z
Learning: The user ashwin1111 has confirmed the suggestion to replace the hardcoded reference to 'Sage Intacct' with a dynamic reference to the target system using the variable `targetSystemName` in the subLabel string within the `business-central-export-settings.component.html` file.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-10-08T15:51:28.972Z
Learning: The user ashwin1111 has confirmed the replacement of the 'intacct' keyword with the variable `targetSystemName` for dynamic system naming in the `business-central-export-settings.component.html` file.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-06-10T19:13:15.470Z
Learning: The user ashwin1111 has confirmed the replacement of the 'intacct' keyword with the variable `targetSystemName` for dynamic system naming in the `business-central-export-settings.component.html` file.
src/app/shared/components/configuration/configuration-import-field/configuration-import-field.component.html (9)
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#298
File: src/app/shared/components/export-log/skipped-export-log-table/skipped-export-log-table.component.html:21-22
Timestamp: 2024-10-08T15:51:28.972Z
Learning: The user ashwin1111 has requested the removal of a condition that was always evaluating to false (`*ngIf="false"`), which was making an `h5` element never visible. This indicates a preference for removing redundant or placeholder code that is not currently in use.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#298
File: src/app/shared/components/export-log/skipped-export-log-table/skipped-export-log-table.component.html:21-22
Timestamp: 2024-06-10T19:13:15.470Z
Learning: The user ashwin1111 has requested the removal of a condition that was always evaluating to false (`*ngIf="false"`), which was making an `h5` element never visible. This indicates a preference for removing redundant or placeholder code that is not currently in use.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#353
File: src/app/core/services/business-central/business-central-configuration/business-central-advanced-settings.service.ts:0-0
Timestamp: 2024-06-10T19:13:15.470Z
Learning: The user ashwin1111 has confirmed the correction of a typo in the method name from `getExpenseFilelds` to `getExpenseFields` in the `BusinessCentralAdvancedSettingsService` class.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#353
File: src/app/core/services/business-central/business-central-configuration/business-central-advanced-settings.service.ts:0-0
Timestamp: 2024-10-08T15:51:28.972Z
Learning: The user ashwin1111 has confirmed the correction of a typo in the method name from `getExpenseFilelds` to `getExpenseFields` in the `BusinessCentralAdvancedSettingsService` class.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-06-10T19:13:15.470Z
Learning: The user requested the removal of the "CRE" acronym from the subLabel string in the business-central-export-settings.component.html file, indicating a preference for not including this acronym in the user interface text.
Learnt from: anishfyle
PR: fylein/fyle-integrations-app#367
File: src/app/core/models/sage300/db/sage300-accounting-export.model.ts:10-14
Timestamp: 2024-10-08T15:51:28.972Z
Learning: The user agreed with the suggestion to remove an unused import statement for `BusinessCentralAccountingExport` after the type was no longer used in the `Sage300AccountingExportResponse` interface.
Learnt from: anishfyle
PR: fylein/fyle-integrations-app#367
File: src/app/core/models/sage300/db/sage300-accounting-export.model.ts:10-14
Timestamp: 2024-06-10T19:13:15.470Z
Learning: The user agreed with the suggestion to remove an unused import statement for `BusinessCentralAccountingExport` after the type was no longer used in the `Sage300AccountingExportResponse` interface.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-10-08T15:51:28.972Z
Learning: The user ashwin1111 requested the removal of the "CRE" acronym from the subLabel string in the business-central-export-settings.component.html file, indicating a preference for not including this acronym in the user interface text.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-06-10T19:13:15.470Z
Learning: The user ashwin1111 requested the removal of the "CRE" acronym from the subLabel string in the business-central-export-settings.component.html file, indicating a preference for not including this acronym in the user interface text.
src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.html (2)
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-10-08T15:51:28.972Z
Learning: The user ashwin1111 has confirmed the suggestion to replace the hardcoded reference to 'Sage Intacct' with a dynamic reference to the target system using the variable `targetSystemName` in the subLabel string within the `business-central-export-settings.component.html` file.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-06-10T19:13:15.470Z
Learning: The user ashwin1111 has confirmed the suggestion to replace the hardcoded reference to 'Sage Intacct' with a dynamic reference to the target system using the variable `targetSystemName` in the subLabel string within the `business-central-export-settings.component.html` file.
⏰ 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). (3)
- GitHub Check: Cursor BugBot
- GitHub Check: lint
- GitHub Check: unit-test
🔇 Additional comments (2)
src/app/shared/components/configuration/configuration-import-field/configuration-import-field.component.html (1)
44-46: Sub-label rendering looks goodThe translation key, helpers and branding style hook up correctly; nothing to flag.
src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.html (1)
476-478: Disabled preview unit is not translated reactivelyUsing
disabled+[value]will freeze the value at render time.
If the user changes the frequency again, the singular/plural text will not update.Prefer
readonlywithngModel/formControlbinding or recompute the label elsewhere. Example:<input class="..." readonly [value]="advancedSettingForm.get('exportScheduleFrequency')?.value > 1 ? ('qboCloneSettings.hours' | transloco) : ('qboCloneSettings.hour' | transloco)">
Comment on lines
+51
to
+57
| {{ 'qboCloneSettings.exportReimbursableExpensesLabel' | transloco }} | ||
| <app-svg-icon [width]="'16px'" [height]="'16px'" [svgSource]="'info-circle-fill'" [styleClasses]="'tw-text-placeholder tw-ml-8-px'" [tooltipText]="'qboCloneSettings.exportReimbursableExpensesTooltip' | transloco: { brandName: brandingConfig.brandName }" [tooltipPosition]="'top'"></app-svg-icon> | ||
| </div> | ||
| <div> | ||
| <p-inputSwitch formControlName="reimbursableExpense"></p-inputSwitch> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
p-inputSwitch lacks an accessible label
The toggle controlling “Export Reimbursable Expenses” is rendered without an aria-label (or similar) attribute.
Screen-reader users will hear “switch” with no context.
Add an explicit label:
<p-inputSwitch
ariaLabel="Export reimbursable expenses"
formControlName="reimbursableExpense"></p-inputSwitch>Apply the same fix to the other toggles throughout the template.
🤖 Prompt for AI Agents
In
src/app/integrations/qbo/qbo-onboarding/qbo-clone-settings/qbo-clone-settings.component.html
around lines 51 to 57, the p-inputSwitch component controlling "Export
Reimbursable Expenses" lacks an accessible label, causing screen readers to
announce it without context. Add an ariaLabel attribute with a descriptive
string like "Export reimbursable expenses" to the p-inputSwitch element. Repeat
this fix for all other toggle switches in the template to ensure accessibility.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.html (2)
325-333: DuplicateisFieldMandatoryattribute – remove the redundancy
isFieldMandatoryis set twice on the same element. The second declaration silently overrides the first and adds noise to the template.- [isFieldMandatory]="true" <!-- line 329 – remove -->Removing the duplicate improves readability with zero functional impact.
11-102: Heavy markup duplication – consider extracting reusable wrappersMultiple sections repeat the same wrapper structure:
<div class="tw-px-24-px ..."> <div class="tw-rounded-12-px tw-border-border-tertiary tw-border tw-bg-white"> <div class="tw-p-24-px"> <!-- field -->Abstracting this into a small presentational component (e.g.
<clone-setting-card>) or a structural directive would:• Eliminate >150 duplicated lines
• Centralise styling tweaks (single-point change)
• Improve template readabilityNot urgent but worthwhile for long-term maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/core/services/xero/xero-configuration/xero-onboarding.service.ts(1 hunks)src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.html(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/core/services/xero/xero-configuration/xero-onboarding.service.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-10-08T15:51:28.972Z
Learning: The user ashwin1111 has confirmed the suggestion to replace the hardcoded reference to 'Sage Intacct' with a dynamic reference to the target system using the variable `targetSystemName` in the subLabel string within the `business-central-export-settings.component.html` file.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-06-10T19:13:15.470Z
Learning: The user ashwin1111 has confirmed the suggestion to replace the hardcoded reference to 'Sage Intacct' with a dynamic reference to the target system using the variable `targetSystemName` in the subLabel string within the `business-central-export-settings.component.html` file.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-06-10T19:13:15.470Z
Learning: The user ashwin1111 has confirmed the replacement of the 'intacct' keyword with the variable `targetSystemName` for dynamic system naming in the `business-central-export-settings.component.html` file.
Learnt from: ashwin1111
PR: fylein/fyle-integrations-app#343
File: src/app/integrations/business-central/business-central-shared/business-central-export-settings/business-central-export-settings.component.html:0-0
Timestamp: 2024-10-08T15:51:28.972Z
Learning: The user ashwin1111 has confirmed the replacement of the 'intacct' keyword with the variable `targetSystemName` for dynamic system naming in the `business-central-export-settings.component.html` file.
⏰ 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). (2)
- GitHub Check: Cursor BugBot
- GitHub Check: unit-test
🔇 Additional comments (1)
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.html (1)
198-209: Enum binding is already exposed in the componentThe
xero-clone-settings.component.tsclass declares:
- Line 139:
XeroCorporateCreditCardExpensesObject = XeroCorporateCreditCardExpensesObject;This makes the enum accessible in the template, so the comparison will work at runtime. No changes are needed.
Comment on lines
+134
to
+144
| <div class="clone-setting--dependent-field" *ngIf="exportSettingForm.controls.creditCardExportType.value"> | ||
| <app-clone-setting-field | ||
| [label]="'xeroCloneSettings.postToBankAccountLabel' | transloco: { exportType: (exportSettingForm.controls.reimbursableExportType.value | snakeCaseToSpaceCase | lowercase) }" | ||
| [options]="bankAccounts" | ||
| [placeholder]="'xeroCloneSettings.selectBankAccountPlaceholder' | transloco" | ||
| [form]="exportSettingForm" | ||
| [isFieldMandatory]="true" | ||
| [formControllerName]="'bankAccount'" | ||
| [dropdownDisplayKey]="'name'" | ||
| [tooltipText]="'xeroCloneSettings.postToBankAccountTooltip' | transloco"> | ||
| </app-clone-setting-field> |
There was a problem hiding this comment.
Incorrect exportType interpolation – references the wrong form control
The label for the Post to Bank Account field is interpolated with exportSettingForm.controls.reimbursableExportType.value, but this block is rendered for credit-card exports (guarded by creditCardExportType).
If reimbursable settings are disabled, the expression resolves to undefined, yielding a broken/empty translation.
- [label]="'xeroCloneSettings.postToBankAccountLabel' | transloco: { exportType: (exportSettingForm.controls.reimbursableExportType.value | snakeCaseToSpaceCase | lowercase) }"
+ [label]="'xeroCloneSettings.postToBankAccountLabel' | transloco: { exportType: (exportSettingForm.controls.creditCardExportType.value | snakeCaseToSpaceCase | lowercase) }"Fixing this prevents user-visible i18n glitches and keeps the label contextually correct.
📝 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
| <div class="clone-setting--dependent-field" *ngIf="exportSettingForm.controls.creditCardExportType.value"> | |
| <app-clone-setting-field | |
| [label]="'xeroCloneSettings.postToBankAccountLabel' | transloco: { exportType: (exportSettingForm.controls.reimbursableExportType.value | snakeCaseToSpaceCase | lowercase) }" | |
| [options]="bankAccounts" | |
| [placeholder]="'xeroCloneSettings.selectBankAccountPlaceholder' | transloco" | |
| [form]="exportSettingForm" | |
| [isFieldMandatory]="true" | |
| [formControllerName]="'bankAccount'" | |
| [dropdownDisplayKey]="'name'" | |
| [tooltipText]="'xeroCloneSettings.postToBankAccountTooltip' | transloco"> | |
| </app-clone-setting-field> | |
| <div class="clone-setting--dependent-field" *ngIf="exportSettingForm.controls.creditCardExportType.value"> | |
| <app-clone-setting-field | |
| [label]="'xeroCloneSettings.postToBankAccountLabel' | transloco: { exportType: (exportSettingForm.controls.creditCardExportType.value | snakeCaseToSpaceCase | lowercase) }" | |
| [options]="bankAccounts" | |
| [placeholder]="'xeroCloneSettings.selectBankAccountPlaceholder' | transloco" | |
| [form]="exportSettingForm" | |
| [isFieldMandatory]="true" | |
| [formControllerName]="'bankAccount'" | |
| [dropdownDisplayKey]="'name'" | |
| [tooltipText]="'xeroCloneSettings.postToBankAccountTooltip' | transloco"> | |
| </app-clone-setting-field> | |
| </div> |
🤖 Prompt for AI Agents
In
src/app/integrations/xero/xero-onboarding/xero-clone-settings/xero-clone-settings.component.html
around lines 134 to 144, the label interpolation for the Post to Bank Account
field incorrectly uses reimbursableExportType instead of creditCardExportType,
causing incorrect or empty translations when reimbursable settings are disabled.
Update the label binding to use
exportSettingForm.controls.creditCardExportType.value for the exportType
interpolation to ensure the label matches the displayed export type and
translations render correctly.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
JustARatherRidiculouslyLongUsername
approved these changes
Jul 18, 2025
src/app/core/services/xero/xero-configuration/xero-onboarding.service.ts
Show resolved
Hide resolved
ashwin1111
approved these changes
Jul 18, 2025
|
There was a problem hiding this comment.
Bug: Help Text Visibility Issue in Clone Settings
The h5 element's help text is now always visible due to the removal of the *ngIf="!isCloneSettingView" condition. This unintended change causes the text to display in clone setting views where it should be hidden, breaking the intended UI. Additionally, a hardcoded phrase " will be an available field when creating an expense." was removed from this help text.
src/app/shared/components/configuration/configuration-import-field/configuration-import-field.component.html#L43-L46
Was this report helpful? Give feedback by reacting with 👍 or 👎
🔁 Code Duplication Report - Angular
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
q3_usability clone-settings page alignment fixes for qbo & xero
route qbo - integrations/qbo/onboarding/clone_settings
route xero - integrations/xero/onboarding/clone_settings
sample images for fixes
Clickup
https://app.clickup.com/
Summary by CodeRabbit
Refactor
Style