fix: Qbd direct dashboard error section fix and video update #1135
Conversation
WalkthroughThis pull request involves updates across multiple components, focusing on modifying YouTube video links for QuickBooks Desktop (QBD) integration and refining the logic in the dashboard error section. The changes include updating demo video URLs in branding configurations and HTML components, streamlining import statements, and enhancing the conditional logic within the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
src/app/shared/components/helper/mapping/generic-mapping-table/generic-mapping-table.component.html (2)
32-34: LGTM! Consider extracting the hardcoded width.The text overflow handling implementation is solid, using proper CSS properties for truncation with tooltips.
Consider extracting the hardcoded width '16rem' to a CSS variable or component property for better maintainability:
-[ngStyle]="{'white-space': 'nowrap', 'overflow': 'hidden', 'text-overflow': 'ellipsis', 'max-width': '16rem'}" +[ngStyle]="{'white-space': 'nowrap', 'overflow': 'hidden', 'text-overflow': 'ellipsis', 'max-width': dropdownMaxWidth}"
34-34: Consider optimizing tooltip performance.The
isOverflowingfunction is called for every item, which might be unnecessary for short text.Consider adding a length check before calling isOverflowing:
-[pTooltip]="isOverflowing(textElement, value)" +[pTooltip]="value?.value?.length > 20 ? isOverflowing(textElement, value) : null"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/branding/fyle-branding-config.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.html(1 hunks)src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts(2 hunks)src/app/shared/components/helper/mapping/generic-mapping-table/generic-mapping-table.component.html(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.html
- src/app/branding/fyle-branding-config.ts
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.html
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts
[error] 5-5: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 178-178: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 180-180: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (3)
src/app/shared/components/helper/mapping/generic-mapping-table/generic-mapping-table.component.html (1)
32-34: Verify alignment with PR objectives.
The changes focus on UI improvements (text overflow and tooltips) in a shared component, but the PR objectives mention fixing the QBD direct dashboard error section. Could you clarify how these changes relate to the error section fix?
Let's check for related error handling code:
✅ Verification successful
Changes are related to QBD direct dashboard error section
The modifications to the generic mapping table component are indeed connected to the QBD direct dashboard error section. The component is used within the dashboard's error section to display and resolve mapping errors, as evidenced by:
- The component is used in
dashboard-error-section.component.htmlwith a dialog for resolving mapping errors - The code shows specific handling for QBD Direct in the component:
this.uiExposedAppName = this.appName === AppName.QBD_DIRECT ? AppName.QBD : this.appName;
- The text overflow and tooltip improvements directly enhance the error resolution experience by making long mapping values more readable in the error section.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for QBD dashboard error related files and components
rg -l "qbd.*dashboard.*error" --type ts --type html
# Search for references to this generic mapping table in QBD components
rg -l "generic-mapping-table" --type ts --type html | grep -i "qbd"
Length of output: 119
Script:
#!/bin/bash
# Let's try a broader search for QBD related files
rg -l "qbd" --type ts --type html
# Search for error-related code in QBD files
rg "error" --type ts --type html | grep -i "qbd"
# Search for any usage of generic-mapping-table component
rg -l "generic-mapping-table" --type ts --type html
# Look for the component's TypeScript file to understand its usage
fd "generic-mapping-table.component.ts$"
Length of output: 31473
Script:
#!/bin/bash
# Let's check if this generic mapping table is used in QBD direct dashboard
rg -B 5 -A 5 "generic-mapping-table" src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.html
# Check the dashboard error section component to see how it uses the generic mapping table
rg -B 5 -A 5 "generic-mapping-table" src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.html
# Look at the component's implementation to understand its purpose
cat src/app/shared/components/helper/mapping/generic-mapping-table/generic-mapping-table.component.ts
Length of output: 11454
src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts (2)
4-4: LGTM: Clean import statements
Good cleanup of unused imports and simplification of import statements.
Also applies to: 6-6
177-181: Verify error handling robustness
Please ensure proper handling of edge cases:
- When
importCodeFieldsis undefined - When
destinationFieldis null/undefined - When
appNameis not in the expected list
Let's verify the error handling:
🧰 Tools
🪛 Biome (1.9.4)
[error] 178-178: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 180-180: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
| if ([AppName.QBD_DIRECT, AppName.QBO].includes(this.appName)) { | ||
| this.isMultiLineOption = this.destinationField === FyleField.CATEGORY && brandingConfig.brandId !== 'co' ? true : false; | ||
| } else { | ||
| this.isMultiLineOption = this.importCodeFields?.includes(this.destinationField) && brandingConfig.brandId !== 'co' ? true : false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify conditional logic and improve maintainability
The current implementation has several areas for improvement:
- The ternary expressions can be simplified
- The repeated condition
brandingConfig.brandId !== 'co'should be extracted - The special handling for different apps needs documentation
Consider applying these improvements:
+ // Extract common condition
+ private isCoBrand(): boolean {
+ return brandingConfig.brandId === 'co';
+ }
showMappingResolve(errorType: AccountingErrorType, groupedError: Error[], sourceField: ExportErrorSourceType) {
// ... existing code ...
+ // Special handling for QBD and QBO apps for category mapping
if ([AppName.QBD_DIRECT, AppName.QBO].includes(this.appName)) {
- this.isMultiLineOption = this.destinationField === FyleField.CATEGORY && brandingConfig.brandId !== 'co' ? true : false;
+ this.isMultiLineOption = this.destinationField === FyleField.CATEGORY && !this.isCoBrand();
} else {
- this.isMultiLineOption = this.importCodeFields?.includes(this.destinationField) && brandingConfig.brandId !== 'co' ? true : false;
+ this.isMultiLineOption = this.importCodeFields?.includes(this.destinationField) && !this.isCoBrand();
}Also, please add documentation explaining why QBD_DIRECT and QBO apps have special handling for category mapping.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 178-178: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 180-180: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
DhaaraniCIT
added
deploy
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts
[error] 5-5: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 178-178: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 180-180: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (1)
src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.ts (1)
4-4: LGTM! Clean import statements
Good cleanup of unused imports, making the code more maintainable.
Also applies to: 6-6
| if ([AppName.QBD_DIRECT, AppName.QBO].includes(this.appName)) { | ||
| this.isMultiLineOption = this.sourceField === ExportErrorSourceType.CATEGORY && brandingConfig.brandId !== 'co' ? true : false; | ||
| } else { | ||
| this.isMultiLineOption = this.importCodeFields?.includes(this.destinationField) && brandingConfig.brandId !== 'co' ? true : false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify conditional logic and improve code clarity
The current implementation has several areas for improvement:
- Unnecessary ternary operators with boolean literals
- Repeated brandId check
- Missing documentation for special QBD/QBO handling
Apply this diff to improve the code:
+ // Special handling for QBD and QBO apps where category mapping always enables multi-line option
+ // for non-co brands
if ([AppName.QBD_DIRECT, AppName.QBO].includes(this.appName)) {
- this.isMultiLineOption = this.sourceField === ExportErrorSourceType.CATEGORY && brandingConfig.brandId !== 'co' ? true : false;
+ this.isMultiLineOption = this.sourceField === ExportErrorSourceType.CATEGORY && brandingConfig.brandId !== 'co';
} else {
- this.isMultiLineOption = this.importCodeFields?.includes(this.destinationField) && brandingConfig.brandId !== 'co' ? true : false;
+ this.isMultiLineOption = this.importCodeFields?.includes(this.destinationField) && brandingConfig.brandId !== 'co';
}Consider extracting the brandId check into a private method for better reusability:
private isNonCoBrand(): boolean {
return brandingConfig.brandId !== 'co';
}🧰 Tools
🪛 Biome (1.9.4)
[error] 178-178: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 180-180: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
* fix: Qbd direct dashboard error section fix and video update * fix: Qbd direct dashboard error section fix and video update
Description
fix: Qbd direct dashboard error section fix and video update
Clickup
https://app.clickup.com/
Summary by CodeRabbit
New Features
Bug Fixes
Chores