Ankit | Test | View financial reports of multiple company together with currency filter #14363
Conversation
WalkthroughThis pull request introduces a comprehensive multi-currency reporting feature for the Giddh web application. It adds a new module and routing configuration for multi-currency reports, including trial balance, profit and loss, and balance sheet reports. The implementation includes new components, services, and store management for fetching and displaying financial data across multiple currencies. The feature is localized for English, Hindi, and Marathi languages and integrates with the existing application architecture. Changes
Sequence DiagramsequenceDiagram
participant User
participant MultiCurrencyReportsComponent
participant TlPlService
participant MultiCurrencyReportsComponentStore
User->>MultiCurrencyReportsComponent: Select Report Type
MultiCurrencyReportsComponent->>TlPlService: Request Report Data
TlPlService->>MultiCurrencyReportsComponentStore: Fetch Report
MultiCurrencyReportsComponentStore-->>MultiCurrencyReportsComponent: Return Report Data
MultiCurrencyReportsComponent->>User: Display Report
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 25
🔭 Outside diff range comments (4)
apps/web-giddh/src/app/financial-reports/components/trial-balance/trial-balance.component.ts (3)
Line range hint
26-27: Initialize boolean properties with default valuesThe
expandAllandsearchproperties should be initialized with default values to ensure predictable behavior.- public expandAll: boolean; - public search: string; + public expandAll: boolean = false; + public search: string = '';
Line range hint
47-55: Add null safety checks in selectedCompany setterThe setter should include null safety checks for nested properties to prevent potential runtime errors.
@Input() public set selectedCompany(value: CompanyResponse) { this._selectedCompany = value; - if (value && value.activeFinancialYear && !this.isDateSelected) { + if (value?.activeFinancialYear?.financialYearStarts && + value?.activeFinancialYear?.financialYearEnds && + !this.isDateSelected) { this.request = { refresh: false, from: value.activeFinancialYear.financialYearStarts, - to: this.selectedCompany.activeFinancialYear.financialYearEnds + to: value.activeFinancialYear.financialYearEnds }; } }
Line range hint
108-116: Enhance filterData method with type safety and error handlingThe method should include proper error handling and type checking for the request parameter.
public filterData(request: TrialBalanceRequest) { + if (!request) { + this.toaster.errorToast('Invalid request parameters'); + return; + } this.from = request.from; this.to = request.to; this.isDateSelected = request && request.selectedDateOption === '1'; if (this.isV2) { this.store.dispatch(this.tlPlActions.GetV2TrialBalance(cloneDeep(request))); } else { this.store.dispatch(this.tlPlActions.GetTrialBalance(cloneDeep(request))); } }apps/web-giddh/src/app/store/tl-pl/tl-pl.reducer.ts (1)
Line range hint
240-267: Consider using decimal.js for financial calculations.The current implementation uses standard JavaScript number operations which can lead to floating-point precision issues. For financial calculations, it's recommended to use a library like decimal.js.
+import { Decimal } from 'decimal.js'; export const prepareProfitLossData = (data) => { if (data && data.groupInfo && data.groupInfo.groupDetails && data.incomeStatment) { let plData: ProfitLossData = filterProfitLossData(data.groupInfo.groupDetails, data.incomeStatment); - plData.expenseTotal = calculateTotalExpense(plData.expArr); - plData.expenseTotalEnd = calculateTotalExpenseEnd(plData.expArr); - plData.incomeTotal = calculateTotalIncome(plData.incArr); - plData.incomeTotalEnd = calculateTotalIncomeEnd(plData.incArr); - plData.closingBalance = Math.abs(plData.incomeTotal - plData.expenseTotal); + plData.expenseTotal = new Decimal(calculateTotalExpense(plData.expArr)); + plData.expenseTotalEnd = new Decimal(calculateTotalExpenseEnd(plData.expArr)); + plData.incomeTotal = new Decimal(calculateTotalIncome(plData.incArr)); + plData.incomeTotalEnd = new Decimal(calculateTotalIncomeEnd(plData.incArr)); + plData.closingBalance = plData.incomeTotal.minus(plData.expenseTotal).abs();🧰 Tools
🪛 Biome (1.9.4)
[error] 241-241: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧹 Nitpick comments (60)
apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.html (2)
4-5: Avoid redundant directives/conditions for element visibility.
Currently, you use both [hidden]="!groupDetail.isVisible" (line 4) and *ngIf="groupDetail.groupName && (groupDetail.isVisible || groupDetail.isCreated)" (line 5) on the same element. This may lead to confusion or unintended outcomes. Consider combining or simplifying these conditions to maintain clarity and reduce duplication.
89-93: Assess dynamic component handling for large sets of accounts.
account-detail-modal-component is dynamically instantiated inside the popover. Rendering many of these simultaneously (e.g., multiple open popovers) can be resource-intensive. Consider implementing an approach that reuses a single modal instance or defers instantiation of the component to optimize performance.apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html (4)
7-11: Consider an accessibility enhancement for the "no data" image.While an alt text is already provided, you might want to explore further enhancements or additional ARIA attributes if this element represents important status information for screen readers.
14-63: Improve search toggling logic for better maintainability.The search toggle is handled inline here. Consider packaging the toggle and clickOutside logic into a dedicated reusable directive or capturing it in separate methods to keep the template cleaner.
141-289: Consider reducing repetition in the expenses portion.Sections for “revenue,” “operatingcost,” “otherexpenses,” etc., share a similar structure. Refactoring them into a reusable sub-component could improve readability and reduce template verbosity.
314-336: Centralize the tax rate configuration.Hardcoding 30% taxes can become limiting when needs change or vary by region. Consider storing the tax rate in a configuration for better flexibility and maintainability.
apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.ts (2)
84-100: Good approach for conditionally showing the modal vs. navigating.When loading details from the SearchService, the code checks the account’s parent group to determine if the modal should be displayed or if the user is taken directly to the ledger. This logic is clear and aids the user experience. However, consider adding fallback or error-handling logic (e.g., what if the response object is missing parentGroups).
102-104: Utilize the hideModal for consistent cleanup.The hideModal method sets modalUniqueName to null, ensuring a single entry point for resetting. If needed, consider hooking future reset logic or animations into this method to maintain consistency and single-responsibility.
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.ts (3)
13-31: Consider adding a proper type for incomeStatement.The
incomeStatementinput is currently typed asany. Consider creating an interface to properly type this property for better type safety and code maintainability.-@Input() public incomeStatement: any; +interface IncomeStatement { + // Add appropriate properties based on the actual structure +} +@Input() public incomeStatement: IncomeStatement;
36-49: Fix incorrect @memberof tag in JSDoc comment.The JSDoc comment references
ProfitLossReportGridComponentbut should referenceProfitLossReportGridRowComponent.-* @memberof ProfitLossReportGridComponent +* @memberof ProfitLossReportGridRowComponent
69-79: LGTM! Fix @memberof tag in JSDoc.The trackByFn implementation is correct and follows best practices for performance optimization. However, the JSDoc comment references
ProfitLossGridRowComponentbut should beProfitLossReportGridRowComponent.-* @memberof ProfitLossGridRowComponent +* @memberof ProfitLossReportGridRowComponentapps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.html (2)
1-6: Consider standardizing the translation file path naming convention.The translation file path 'trial-profit-balance/profit-loss' mixes different terms (trial/profit). Consider renaming it to maintain consistency, e.g., 'profit-loss-balance/profit-loss' or 'trial-balance/profit-loss'.
9-11: Consider consolidating similar filter event handlers.The component has multiple similar event handlers:
(filterValue)="searchData($event)"(onPropertyChanged)="filterData()"Consider consolidating these into a single, more comprehensive filter handler for better maintainability.
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.scss (5)
1-12: Consider removing!importantand using relative units.The date range picker styles could be improved:
- Remove
!importantflag by using more specific selectors- Consider using relative units (em/rem) for height and font-size for better accessibility
.daterange-picker-tb-pl-bs { - background-color: var(--color-white) !important; - height: 36px; - font-size: var(--font-size-15); + background-color: var(--color-white); + height: 2.25rem; + font-size: 0.9375rem; }
30-74: Apply DRY principles to icon styles and improve layout robustness.
- The input wrapper uses absolute positioning with hardcoded values which might break layout
- There's significant style duplication between expand-icon and download-data-icon
Consider extracting common icon styles to a shared class:
+.icon-base { + display: flex; + width: 36px; + height: 36px; + justify-content: center; + align-items: center; + background-color: var(--color-white); + font-size: var(--font-size-15); + color: var(--color-giddh-blue) !important; + border: 1px solid var(--color-secondary-input-bar-border); + border-radius: 5px; +} .expand-icon span { - display: flex; - width: 36px; - height: 36px; - justify-content: center; - align-items: center; - background-color: var(--color-white); - font-size: var(--font-size-16); - color: var(--color-giddh-blue) !important; - border: 1px solid var(--color-secondary-input-bar-border); - border-radius: 5px; + @extend .icon-base; } .download-data-icon a { - display: flex; - width: 36px; - height: 36px; - justify-content: center; - align-items: center; - background-color: var(--color-white); - font-size: var(--font-size-15); - color: var(--color-giddh-blue) !important; - border: 1px solid var(--color-secondary-input-bar-border); - border-radius: 5px; + @extend .icon-base; }
78-85: Improve dropdown triangle positioning.The triangle indicator uses absolute positioning with hardcoded values which might break with different content lengths or font sizes.
Consider using relative positioning or transform:
.glyphicon-triangle-bottom { z-index: 1; position: absolute; - left: 152px; - top: 14px; + right: 1rem; + top: 50%; + transform: translateY(-50%); font-size: 9px; color: var(--color-iron); }
113-123: Remove duplicate height declaration.The height property is declared twice with different values.
.icon-calender { position: absolute; z-index: 1; - height: 32px; width: 36px; display: inline-flex; justify-content: center; align-items: center; height: 36px !important; cursor: pointer; }
124-191: Optimize media queries and reduce repetition.
- Multiple
.download-data-icondeclarations across breakpoints- Repeated width/min-width declarations for
.datepicker-fieldConsider consolidating media queries and using a mobile-first approach:
+// Base styles +.datepicker-field { + width: 100%; + min-width: 100%; +} + +// Tablet and up +@media (min-width: 768px) { + .datepicker-field { + width: 220px; + min-width: 220px; + } +} + +// Desktop and up +@media (min-width: 1100px) { + .download-data-icon { + padding-right: 0; + } +}apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.html (1)
18-28: Consider optimizing multiple async pipe usageWhile the implementation is correct, using multiple async pipes on the same observable (
data$ | async) could lead to multiple subscriptions. Consider storing the result in a local variable using *ngIf as follows:- <div *ngIf="(data$ | async) && !(showLoader | async)"> - <trial-balance-report-grid - #tbGrid - [search]="search" - [from]="from" - [to]="to" - (searchChange)="searchChanged($event)" - [expandAll]="expandAll" - [data$]="data$ | async" - ></trial-balance-report-grid> + <div *ngIf="(data$ | async) as data"> + <div *ngIf="!(showLoader | async)"> + <trial-balance-report-grid + #tbGrid + [search]="search" + [from]="from" + [to]="to" + (searchChange)="searchChanged($event)" + [expandAll]="expandAll" + [data$]="data" + ></trial-balance-report-grid> + </div> + </div>apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.store.ts (2)
9-13: Consider using more specific types instead of any.
Defining your state with strict types encourages type-safety and helps reduce run-time errors. For example, if possible, replace the “any” type with more specific interfaces describing the structure of “reportDataList” and “filterRequestData”.
81-101: Provide clearer naming for creatMultiCurrencyReport.
The method name “creatMultiCurrencyReport” seems to have a minor typo. Consider renaming it to “createMultiCurrencyReport” for better readability and consistency.apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.html (1)
1-6: Avoid inline styling and repeated logic for groupDetail checks.
Inline styles like “[ngStyle]="{ 'padding-left': padding + 'px' }"” and repetitivegroupDetail.isVisible || groupDetail.isCreatedchecks may lead to harder maintenance. Consider centralizing styling in a CSS/SCSS file and using well-named methods or structural directives to encapsulate repeated conditions.apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.module.ts (1)
13-37: Remove or restructure large commented-out blocks.
Retaining commented imports for a long period can introduce confusion. If these imports are no longer needed, remove them, or if they are placeholders for future use, annotate with a clear comment specifying their upcoming usage.apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.ts (2)
80-89: Address optional chaining request to prevent potential undefined errors.
At line 82, where you check for “value && value.activeFinancialYear…”, you could leverage optional chaining to simplify this logic: “value?.activeFinancialYear?.financialYearStarts”. Use it carefully to avoid accidental swallowing of needed checks.🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
131-145: Reuse constants or enumerations instead of string literals.
Repeated references to 'isVisible', 'isCreated', etc., can benefit from storing these strings in a central constants file or an enum to avoid typos and foster easy refactoring.apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.ts (2)
157-159: Consider using optional chaining for increased safety
Here, you are checking both "this.searchInputEl" and its "nativeElement" member. You can simplify this by leveraging optional chaining.Example fix:
- if (this.searchInputEl && this.searchInputEl.nativeElement) { - this.searchInputEl.nativeElement.focus(); - } + this.searchInputEl?.nativeElement?.focus();🧰 Tools
🪛 Biome (1.9.4)
[error] 157-157: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
192-194: Avoid assignment within the while condition
While this pattern is often used for DOM traversal, it can be unclear to readers and flagged by linters. Consider refactoring it for clarity.Example fix:
-public childOf(child, parent): boolean { - while ((child = child.parentNode) && child !== parent) { - } - return !!child; +public childOf(child, parent): boolean { + let current = child?.parentNode; + while (current && current !== parent) { + current = current.parentNode; + } + return !!current; }🧰 Tools
🪛 Biome (1.9.4)
[error] 192-192: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.ts (4)
54-62: Use optional chaining to simplify checks
Here, you explicitly check the presence of "value" and "value.activeFinancialYear" and "value.financialYears" in a single condition. Consider using optional chaining for improved readability and safety.Example fix:
-if (value && value.activeFinancialYear && value.financialYears && !this.isDateSelected) { +if (value?.activeFinancialYear && value?.financialYears && !this.isDateSelected) {🧰 Tools
🪛 Biome (1.9.4)
[error] 54-54: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
98-103: Consider optional chaining for 'data.message'
When you check for "data && data.message", you can write this more clearly via optional chaining.Example fix:
-if (data && data.message) { +if (data?.message) {🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
104-111: Leverage optional chaining for 'data.liabilities'
This can be made more concise by using optional chaining before proceeding to set up child properties.Example fix:
-if (data && data.liabilities) { +if (data?.liabilities) {🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
112-119: Use optional chaining for 'data.assets'
Similar to above, you can shorten your null checks by using optional chaining.Example fix:
-if (data && data.assets) { +if (data?.assets) {🧰 Tools
🪛 Biome (1.9.4)
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts (2)
174-176: Consider using optional chaining for better readability
Use optional chaining when accessing chained members to handle null/undefined scenarios gracefully.Example fix:
-if (this.searchInputEl && this.searchInputEl.nativeElement) { - this.searchInputEl.nativeElement.focus(); -} +this.searchInputEl?.nativeElement?.focus();🧰 Tools
🪛 Biome (1.9.4)
[error] 174-174: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
219-221: Refrain from using assignments in the while loop condition
Assignments within conditions can make the code less readable and are often flagged by linters.Example fix:
-public childOf(c, p) { - while ((c = c.parentNode) && c !== p) { - } - return !!c; +public childOf(c, p) { + let currentNode = c?.parentNode; + while (currentNode && currentNode !== p) { + currentNode = currentNode.parentNode; + } + return !!currentNode; }🧰 Tools
🪛 Biome (1.9.4)
[error] 219-219: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.ts (2)
175-177: Use optional chaining for clarity and safety
Similarly to the other components, refactor to optional chaining for improved readability.Example fix:
-if (this.searchInputEl && this.searchInputEl.nativeElement) { - this.searchInputEl.nativeElement.focus(); -} +this.searchInputEl?.nativeElement?.focus();🧰 Tools
🪛 Biome (1.9.4)
[error] 175-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
209-211: Avoid assignment in the loop condition
This pattern can cause confusion and is generally warned against by linters. Refactor for clarity.Example fix:
-public childOf(c, p) { - while ((c = c.parentNode) && c !== p) { - } - return !!c; +public childOf(c, p) { + let currentNode = c?.parentNode; + while (currentNode && currentNode !== p) { + currentNode = currentNode.parentNode; + } + return !!currentNode; }🧰 Tools
🪛 Biome (1.9.4)
[error] 209-209: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html (2)
9-10: Ensure the image is accessibleYou have provided an alt attribute, which is good. Confirm that this text meaningfully describes the image for screen readers.
221-235: Consider lazy loading for aside menuWhen toggling the account aside menu, consider lazy loading or deferring any heavy computations until the menu is opened. This can further enhance user experience and reduce memory usage.
apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts (3)
67-77: Use optional chaining for clean codeInstead of multiple null checks (e.g. value && value.activeFinancialYear), consider using optional chaining (e.g. value?.activeFinancialYear) to improve readability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
93-99: Refine nested null checksSimilarly, for data && data.incomeStatment && data.incomeStatment.costOfGoodsSold, you can utilize “data?.incomeStatment?.costOfGoodsSold” for readability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 93-93: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 99-99: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
148-179: Adopt optional chaining in subsequent checksWherever you check data && data.expArr or data && data.incArr, you can apply the same optional chaining approach for consistency.
🧰 Tools
🪛 Biome (1.9.4)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 164-164: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts (1)
284-288: Use optional chaining in date label checksIf the property “name” might be undefined, consider “value?.name” in your condition to remove redundant checks. Similarly, for startDate / endDate references, you can adopt optional chaining.
🧰 Tools
🪛 Biome (1.9.4)
[error] 284-284: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 288-288: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.html (1)
9-15: Enhance tab group accessibility and performanceThe mat-tab-group implementation could be improved for accessibility and performance.
<mat-tab-group mat-stretch-tabs="false" mat-align-tabs="start" [selectedIndex]="selectedTabIndex" (selectedIndexChange)="tabChanged($event)" [disableRipple]="true" animationDuration="0ms" + role="tablist" + aria-label="Financial reports" + [preserveContent]="true" class="table-header-sticky" >The
preserveContentproperty will help maintain the component state when switching between tabs.apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.scss (2)
42-49: Remove duplicate CSS property declarationsThe
white-space: nowrapproperty is declared twice within the same selector. Also consider consolidating repeated width declarations.th { width: 100%; - white-space: nowrap !important; word-break: normal; width: 30%; max-width: 30%; white-space: nowrap; min-width: 150px; }
19-21: Consider accessibility implications of fixed heightFixed height on input elements may cause issues with different font sizes and accessibility settings. Consider using min-height or padding instead.
input { - height: 47px; + min-height: 47px; + padding: 8px 12px; }apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.ts (1)
75-77: Add null check in trackBy functionThe trackBy function should handle null/undefined items to prevent runtime errors.
public trackByFn(index, item: Account): string { - return item?.uniqueName; + return item?.uniqueName ?? `index_${index}`; }apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.html (1)
40-43: Simplify complex conditional logicThe *ngIf condition is hard to read and maintain. Consider extracting the logic to a component method.
+// In component class +public shouldShowVirtualScroll(): boolean { + const filteredAccounts = this.groupDetail?.accounts | accountsFilter: !this.groupDetail?.isOpen && this.search; + return (this.groupDetail?.isOpen || (this.search && !this.isExpandToggledDuringSearch)) && + filteredAccounts?.length >= this.minimumViewportLimit; +} -*ngIf=" - (groupDetail?.isOpen || (search && !isExpandToggledDuringSearch)) && - (groupDetail?.accounts | accountsFilter: !groupDetail?.isOpen && search)?.length >= minimumViewportLimit -" +*ngIf="shouldShowVirtualScroll()"apps/web-giddh/src/app/shared/header/pipe/financial-search.pipe.ts (2)
5-10: Consider adding pure pipe optimization hintsWhile the pipe is marked as pure, consider adding a comment explaining the performance implications and when to use pure vs impure pipes.
@Pipe({ name: 'financialSearch', pure: true, standalone: true + // This pipe is marked as pure for performance optimization. + // It will only execute when input references change. })
Line range hint
109-120: Extract parent group names to a constantThe hardcoded parent group names should be extracted to a constant or configuration for better maintainability.
+const PARENT_GROUP_NAMES = [ + 'operatingcost', + 'revenuefromoperations', + 'otherincome', + 'indirectexpenses' +] as const; public resetGroup(input: ChildGroup) { - let parentGroups = ['operatingcost', 'revenuefromoperations', 'otherincome', 'indirectexpenses']; if (input) { for (let grp of input.childGroups) { grp = this.resetGroup(grp); - grp.isVisible = parentGroups.includes(grp?.uniqueName); + grp.isVisible = PARENT_GROUP_NAMES.includes(grp?.uniqueName); grp.isIncludedInSearch = true; }apps/web-giddh/src/app/financial-reports/financial-reports.module.ts (2)
104-105: Consider organizing imports into feature modulesThe module has many imports. Consider organizing related functionality into feature modules for better maintainability.
For example:
- Create a SharedUIModule for UI-related imports (ModalModule, TabsModule, etc.)
- Create a FinancialComponentsModule for financial-specific components
This would make the module more maintainable and improve build optimization.
Line range hint
45-83: Consider splitting components into feature modulesThe module has many declarations. Consider splitting related components into feature modules based on functionality.
For example:
- Create a TrialBalanceModule
- Create a ProfitLossModule
- Create a BalanceSheetModule
This would improve code organization and enable lazy loading of features.
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.html (3)
10-16: Add form validation messages and ARIA labelsThe form lacks accessibility attributes and validation message displays. Consider enhancing user experience with proper validation feedback.
<form [formGroup]="filterForm" (submit)="onSubmit()" class="form-inline financial-select-year" name="fromToDateForm" + aria-label="Multi-currency report filters" novalidate>
35-50: Add required field indicator and validation stateThe company selection dropdown is missing visual indicators for required state and validation feedback.
<mat-form-field appearance="outline" class="custom-placeholder"> <mat-label>{{ localeData?.company_list }}</mat-label> <mat-select name="company" formControlName="shareCompanyList" multiple + [required]="true" + [aria-required]="true" placeholder="{{ commonLocaleData?.app_select }}"> + <mat-error *ngIf="filterForm.get('shareCompanyList')?.errors?.required"> + {{ commonLocaleData?.app_field_required }} + </mat-error>
111-133: Enhance datepicker modal accessibilityThe datepicker modal template should include proper ARIA attributes and keyboard navigation support.
-<ng-template #datepickerTemplate> +<ng-template #datepickerTemplate role="dialog" aria-label="Select date range"> <div class="datepicker-modal"> <div class="modal-body"> <app-datepicker-wrapper [inputStartDate]="selectedDateRange?.startDate" [inputEndDate]="selectedDateRange?.endDate" + [ariaLabel]="'Date range selector'" [alwaysShowCalendars]="true"apps/web-giddh/src/app/services/tl-pl.service.ts (1)
123-124: Use template literals for URL constructionThe current string concatenation with multiple optional chaining operators could be simplified.
- ?.replace(':companyUniqueName', encodeURIComponent(this.companyUniqueName)) - ?.replace(':reportType', ("balance-sheet"))).pipe( + ?.replace(/:companyUniqueName|:reportType/g, match => + match === ':companyUniqueName' ? encodeURIComponent(this.companyUniqueName) : 'balance-sheet' + )).pipe(apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.html (1)
37-45: Enhance accessibility for search input.The search input field should have:
- An associated label for screen readers
- ARIA attributes for better accessibility
<input type="search" #searchInputEl class="form-control" + aria-label="Search balance sheet entries" [placeholder]="commonLocaleData?.app_search" - aria-describedby="sizing-addon3" + aria-describedby="searchHelpText" id="w-398px" [formControl]="bsSearchControl" /> +<span id="searchHelpText" class="sr-only"> + {{ commonLocaleData?.app_search_help_text }} +</span>apps/web-giddh/src/app/multi-currency-reports/balence.json (2)
1-1: Fix the filename typo: "balence.json" should be "balance.json"The filename contains a spelling error which should be corrected for better maintainability and clarity.
6-24: Consider adding TypeScript interfaces for the data structureThe JSON structure has well-defined patterns that could benefit from TypeScript interfaces for better type safety and documentation.
Consider creating interfaces like:
interface BalanceAmount { amount: number; type: 'DEBIT' | 'CREDIT'; } interface Currency { code: string; symbol: string | null; } interface Account { creditTotal: number; debitTotal: number; closingBalance: BalanceAmount; openingBalance: BalanceAmount; currency: Currency; name: string; uniqueName: string; groupName: string; }apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.html (3)
19-30: Consider standardizing search-related property namesThe grid component uses
[search]as input but(searchChange)as output. Consider standardizing these names for better maintainability.- [search]="search" + [searchTerm]="search" - (searchChange)="searchChanged($event)" + (searchTermChange)="searchChanged($event)"Also, the template reference variable
#bsGridappears to be unused in this template. If it's not being used in the component's TypeScript file, consider removing it.
31-35: Enhance accessibility for no-data stateWhile the implementation is correct, consider adding ARIA attributes and roles for better accessibility.
- <div *ngIf="!(showLoader | async) && !data" class="d-flex align-items-center justify-content-center tb-pl-bs-data"> + <div *ngIf="!(showLoader | async) && !data" class="d-flex align-items-center justify-content-center tb-pl-bs-data" role="alert" aria-live="polite">
1-36: Well-structured component with good separation of concernsThe template demonstrates good architectural decisions:
- Clear separation of concerns with dedicated components for filtering and data display
- Proper state management for loading, data, and no-data scenarios
- Reusable components that can be shared across different financial reports
Consider adding error handling state for scenarios where data loading fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
apps/web-giddh/src/app/app.routes.ts(1 hunks)apps/web-giddh/src/app/financial-reports/components/filter/filter.component.ts(1 hunks)apps/web-giddh/src/app/financial-reports/components/trial-balance/trial-balance.component.ts(1 hunks)apps/web-giddh/src/app/financial-reports/financial-reports.module.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balence.json(1 hunks)apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.module.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.routing.module.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.store.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency.const.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/pipes/accounts-filter.pipe.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit.json(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial.json(1 hunks)apps/web-giddh/src/app/services/apiurls/tl-pl.api.ts(1 hunks)apps/web-giddh/src/app/services/tl-pl.service.ts(2 hunks)apps/web-giddh/src/app/shared/header/pipe/financial-search.pipe.ts(1 hunks)apps/web-giddh/src/app/store/tl-pl/tl-pl.reducer.ts(2 hunks)apps/web-giddh/src/assets/locale/sidebar-menu/en.json(1 hunks)apps/web-giddh/src/assets/locale/sidebar-menu/hi.json(1 hunks)apps/web-giddh/src/assets/locale/sidebar-menu/mr.json(1 hunks)apps/web-giddh/src/assets/locale/trial-profit-balance/en.json(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.scss
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.scss
- apps/web-giddh/src/app/financial-reports/components/filter/filter.component.ts
- apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss
- apps/web-giddh/src/app/multi-currency-reports/profit.json
- apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.scss
- apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.scss
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.scss
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.ts
[error] 229-229: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 245-245: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.ts
[error] 175-175: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 209-209: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.ts
[error] 157-157: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 192-192: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts
[error] 211-211: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 284-284: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 288-288: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.ts
[error] 54-54: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 98-98: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 104-104: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 112-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.ts
[error] 82-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts
[error] 174-174: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 219-219: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 93-93: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 99-99: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 164-164: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (42)
apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.html (2)
14-20: Ensure consistent formatting for monetary values.
The component with pipes like recType offers good flexibility. Verify that amount-field is not already returning the correct format, as double-formatting may appear inconsistent. Also, confirm that null or undefined values (e.g., groupDetail.forwardedBalance) are handled gracefully.
68-72: Confirm double-click handler is correctly defined.
You have (dblclick)="accountInfo(account, $event)" on line 71, but the method is not visible in this snippet. Ensure that accountInfo is properly implemented in the component’s .ts file and tested for edge cases (e.g., empty accounts, concurrency issues, etc.).
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html (5)
1-6: Looks good overall for initializing translations.
The usage of the appTranslate directive is consistent, and the event bindings for “localeData” and “commonLocaleData” provide a clean approach to dynamic locale configuration.
64-121: Validate handling of empty or null child groups.
In nested structures, always check if the child array is empty or null. Confirm that the “groupDetails” template gracefully handles these cases without throwing errors.
122-138: Gross profit section logic looks straightforward and clear.
No issues found. The usage of is appropriate, and the strong tags highlight the total nicely.
290-313: Income before taxes logic is correct.
Displaying a minus sign for DEBIT amounts ensures clarity. The approach for toggling the sign looks coherent.
343-353: Operator sign template is clean and modular.
No issues found. The dynamic condition for rendering the minus sign is logically correct and keeps the template DRY.
apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.ts (8)
1-25: Imports and organizational structure look fine.
All necessary Angular core modules and dependencies are correctly imported. Unused or commented-out imports are removed, preventing confusion.
26-31: Use of OnPush change detection strategy is well-chosen.
Using OnPush is beneficial for performance, especially in large, data-intensive components like multi-currency grids. This helps Angular efficiently detect changes only when the inputs are updated, rather than on every single event.
33-45: Input declarations are clear and well-defined.
Each input has a descriptive name and consistent type definitions (e.g., string, boolean). The use of typed inputs enhances maintainability and clarity. The JSDoc comments are also beneficial for code readability.
46-55: Good use of Output emitters and ReplaySubject for memory management.
The EventEmitter for opening the account modal is a straightforward pattern for parent-child communication in Angular, and ReplaySubject helps avoid memory leaks. This is a clean approach.
64-71: Minor improvement: re-check detectChanges usage in ngOnChanges.
Calling detectChanges can be expensive if done too frequently. While this may be necessary for OnPush components, verify calls to ensure they are needed and won't degrade performance with large data sets.
106-108: trackByFn is a solid performance optimization.
Using trackBy with a unique field such as uniqueName helps Angular’s change detection handle large lists more efficiently, especially in OnPush components.
110-118: Correct subscription cleanup.
ngOnDestroy properly emits and completes the ReplaySubject. This helps prevent memory leaks and ensures that all subscriptions bound to destroyed$ are unsubscribed. Good job.
120-132: DOM manipulation within popover: verify requirements for cross-browser compatibility.
The code removes the modal element from the popover container using Renderer2. Ensure that the final user experience (e.g., for Safari, IE11 if required) remains consistent. Also, test nested modals and multi-level popovers to ensure correct event flows.
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.ts (2)
1-12: LGTM! Good use of performance optimization.
The component setup demonstrates good practices:
- Using OnPush change detection strategy for better performance
- Clear and focused imports
- Attribute selector pattern suggesting proper usage context
33-34: LGTM! Clean dependency injection.
The constructor is properly focused on injecting the necessary dependency for change detection.
apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.html (3)
16-18: LGTM! Well-implemented loading state.
Good use of async pipe for subscription management and clean loading state implementation.
32-36: LGTM! Well-implemented no-data state.
Good implementation with proper:
- Conditional rendering
- Flexbox layout
- Localization
20-30: Verify searchChanged handler compatibility.
The searchChanged method is called from both the filter component and the grid component. Please verify that this handler can properly handle events from both sources, as they might have different event payloads or requirements.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The searchChanged handler implementation is consistent and safe
The searchChanged method in the profit-loss component follows a standard pattern used across the application, handling a simple string event from both the filter and grid components. The grid component properly declares @Output() searchChange = new EventEmitter<string>(), ensuring type safety and consistent event payload.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the searchChanged method implementation
ast-grep --pattern 'searchChanged($_) {
$$$
}'
Length of output: 50
Script:
#!/bin/bash
# Search for the searchChanged method using ripgrep for TypeScript files
rg -t ts "searchChanged" -A 5 -B 5
Length of output: 7959
Script:
#!/bin/bash
# Let's check the implementation of profit-loss-report-grid component to see how it emits searchChange events
rg -t ts "profit-loss-report-grid" -A 10 -B 10
Length of output: 11522
Script:
#!/bin/bash
# Let's check the implementation of searchChange event in the grid component
rg -t ts "searchChange.*EventEmitter" -A 5 apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts
Length of output: 471
Script:
#!/bin/bash
# Let's check how the search input is handled in the grid component
rg -t ts "searchControl" -A 10 apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts
Length of output: 153
apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.html (3)
15-17: LGTM! Clean implementation of loading state
The loading state implementation follows Angular best practices using the async pipe and proper component structure.
29-36: Verify CSS class naming convention
The CSS class 'tb-pl-bs-data' seems to use abbreviations. Consider using more descriptive class names for better maintainability.
1-6: Verify translation file path consistency
The translation file path 'trial-profit-balance/trial-balance' seems inconsistent with the component's location in 'multi-currency-reports'. This might cause translation loading issues.
✅ Verification successful
Translation file path is correctly configured
The file path 'trial-profit-balance/trial-balance' is consistent with the application's translation structure. This is evidenced by:
- Multiple components across the application using the same 'trial-profit-balance' base path
- The translation directive implementation showing proper handling of file paths
- Consistent usage pattern across both financial-reports and multi-currency-reports modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the translation file exists in the specified path
fd -t f "trial-balance.json" -p "assets/i18n/**/trial-profit-balance/"
# Also check for any similar translation files that might be the correct ones
fd -t f "trial-balance.json" -p "assets/i18n/**/multi-currency-reports/"
Length of output: 393
Script:
#!/bin/bash
# First, let's find the assets/i18n directory or any translation files
fd -t d "i18n"
# Then, let's search for any translation files related to trial balance
fd -t f "trial-balance"
# Also search for translation-related configuration or module setup
rg -l "TranslateModule|TranslateService" --type ts
Length of output: 1485
Script:
#!/bin/bash
# Let's check the appTranslate directive implementation to understand the expected file structure
rg -l "appTranslate" --type ts
# Also check for any existing translation file path patterns in the codebase
rg -l "trial-profit-balance" --type ts --type html
# And check the actual implementation of translation loading
ast-grep --pattern 'class $_ implements $_ {
$$$
file = $_;
$$$
}'
Length of output: 2321
Script:
#!/bin/bash
# Let's examine the translate directive implementation
cat apps/web-giddh/src/app/theme/translate/translate.directive.ts
# And check if other components in multi-currency-reports are using the same translation path
rg "trial-profit-balance" --type html -A 2 -B 2
Length of output: 16701
apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.store.ts (1)
48-71: Validate the error handling logic for getMultiCurrencyReport.
The effect correctly handles the success and error scenarios. However, consider also logging the error details, if available, to aid in debugging or further analytics.
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.html (1)
60-73: Review the cdk-virtual-scroll-viewport usage.
While this approach helps handle large datasets efficiently, confirm that the list always meets your minimum item size and buffer configuration. Verify it aligns with typical use-cases for multi-currency reports where data can be very large.
apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.module.ts (2)
1-12: Verify necessity of all imported modules.
This module imports a substantial number of libraries. Confirm that each library (e.g., TabsModule, PopoverModule) is essential for the multi-currency reports, or consider removing unnecessary imports for a leaner bundle.
72-100: Ensure consistent naming and references in declarations.
Components like “GridReportRowComponent” and “ProfitLossReportGridRowComponent” might cause confusion if they provide overlapping functionality. Verify that each declared component has a unique responsibility.
apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.ts (2)
1-9: Check for missing validations in input data.
You might want to validate the input data, such as ensuring the selected company is not null or undefined, before proceeding with store actions.
186-189: Ensure that searchData usage aligns with user expectations.
Calling “creatMultiCurrencyReport” inside the searchData method is somewhat unexpected. Verify that initiating a new report creation aligns with the typical user action for searching.
apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html (2)
83-114: Verify trackBy usage
Using "trackBy: trackByFn" can improve performance in *ngFor loops. Ensure that "trackByFn" is defined in the component to prevent unnecessary re-renders.
114-147: Leverage cdkVirtualScroll effectively
Your use of cdk-virtual-scroll-viewport is an excellent choice for performance when displaying large data sets. Validate that the item size is properly tuned for optimal rendering performance.
apps/web-giddh/src/app/multi-currency-reports/multi-currency.const.ts (1)
1-5: Enum usage is clear
The added enum provides a clean, self-documenting way to manage report types. No improvements required.
apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.routing.module.ts (1)
1-18: LGTM! Routing module is properly configured.
The routing module follows Angular best practices with proper authentication guards.
Let's verify the existence of the referenced component:
✅ Verification successful
Routing module configuration is valid with all dependencies present
The referenced MultiCurrencyReportsComponent exists at the expected location: apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.ts, confirming that the routing configuration is properly set up with all required dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the MultiCurrencyReportsComponent exists
# Expected: Should find the component file
fd -g "*multi-currency-reports.component.ts" -t f
Length of output: 133
apps/web-giddh/src/app/services/apiurls/tl-pl.api.ts (1)
10-10: Consider adding input validation for reportType parameter.
While the API endpoint follows the existing pattern, consider implementing validation for the reportType parameter to ensure only valid report types are processed.
Let's check how this endpoint is used in the service:
apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.html (1)
18-20: Consider lazy loading for tab content
The current implementation might cause unnecessary initial load of all report components. Consider implementing lazy loading for better performance.
Let's verify the impact:
Consider implementing lazy loading using Angular's built-in features or a custom lazy loading strategy for the report components to improve initial load time.
Also applies to: 25-27, 32-34
✅ Verification successful
Let me check how these components are currently loaded by examining the module configuration.
Let me check the routing configuration to see how this module is loaded.
Let me check how this module is loaded in the main app routing to verify if it's already lazy loaded.
Module is already lazy loaded, but tab content can be optimized further
The MultiCurrencyReportsModule is already properly lazy loaded in the main app routing:
{ path: 'multi-currency-report', loadChildren: () => import('./multi-currency-reports/multi-currency-reports.module').then(module => module.MultiCurrencyReportsModule) }However, within the lazy-loaded module, all tab components (TrialBalance, ProfitLoss, BalanceSheet) are eagerly loaded when the module loads. For better performance, these components could be split into separate sub-modules and lazy loaded when their respective tabs are activated.
- Consider splitting
TrialBalanceReportComponent,ProfitLossReportComponent, andBalanceSheetReportComponentinto separate feature modules - Use dynamic imports with
loadChildrenfor each tab route
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check bundle size impact of report components
rg -l "trial-balance-report|profit-loss-report|balance-sheet-report" | xargs wc -l
Length of output: 1890
Script:
#!/bin/bash
# Check module configuration and routing setup
cat apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.module.ts
Length of output: 7689
Script:
#!/bin/bash
# Check routing configuration
cat apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.routing.module.ts
Length of output: 631
Script:
#!/bin/bash
# Check main app routing
rg "MultiCurrencyReportsModule" -A 5 -B 5
Length of output: 2352
apps/web-giddh/src/app/app.routes.ts (1)
103-103: LGTM!
The new route for multi-currency reports follows the established routing pattern and correctly implements authorization guards.
apps/web-giddh/src/app/store/tl-pl/tl-pl.reducer.ts (1)
Line range hint 344-350: Add validation for negative values in balance sheet calculations.
The balance sheet calculations should handle and validate negative values appropriately, especially for assets and liabilities.
apps/web-giddh/src/assets/locale/sidebar-menu/en.json (1)
618-629: LGTM! Verify the icon asset exists.
The menu item structure follows the established pattern and includes all required fields. The queryParams are consistent with other version 2 voucher implementations.
✅ Verification successful
Icon asset verified and exists in the codebase
The icon-multi-currency class is properly defined in the icomoon font system at apps/web-giddh/src/assets/fonts/icomoon/icomoon.css with the unicode content \ea10. The icon is implemented using the icomoon icon font, which is correctly set up in the project with all necessary font files (.eot, .ttf, .woff).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the icon asset exists in the codebase
fd -t f "icon-multi-currency" "apps/web-giddh/src/assets"
Length of output: 55
Script:
#!/bin/bash
# Let's check for icon-related files and their contents to understand the icon system
fd -t f "icon" "apps/web-giddh/src/assets" -e css -e scss
# Also search for icon-multi-currency references in the codebase
rg "icon-multi-currency" --type css --type scss --type html
Length of output: 144
Script:
#!/bin/bash
# Let's try a broader search for icon files and the specific icon class
rg -t css -t html "icon-multi-currency"
# Also check for any icon font files or icon-related stylesheets
fd "icon" "apps/web-giddh/src"
# Look for font files that might contain icons
fd -e woff -e woff2 -e ttf -e eot . "apps/web-giddh/src"
Length of output: 4558
Script:
#!/bin/bash
# Let's check the content of the icomoon.css file to verify the icon definition
cat "apps/web-giddh/src/assets/fonts/icomoon/icomoon.css" | grep -A 2 "icon-multi-currency"
Length of output: 141
apps/web-giddh/src/assets/locale/sidebar-menu/mr.json (1)
618-629: LGTM! Translation and structure are consistent.
The Marathi translation and menu item structure are correctly implemented and consistent with the English version.
apps/web-giddh/src/assets/locale/sidebar-menu/hi.json (1)
618-629: LGTM! Translation and structure are consistent.
The Hindi translation and menu item structure are correctly implemented and consistent with the English version.
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.html (2)
1-6: LGTM: Translation setup follows best practices
The translation container is well-structured with proper locale data handling for both specific and common translations.
16-18: LGTM: Loading state properly implemented
The loading state implementation follows Angular best practices with proper use of the async pipe and a dedicated loader component.
| *ngIf=" | ||
| (groupDetail?.isOpen || (search && !isExpandToggledDuringSearch)) && | ||
| (groupDetail?.accounts | accountsFilter: !groupDetail?.isOpen && search)?.length >= minimumViewportLimit |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider storing pipe-filtered results to avoid repeated processing.
You repeatedly apply (groupDetail?.accounts | accountsFilter: !groupDetail?.isOpen && search)?.length in the *ngIf statement. If these lists become large, repeated pipe calls might slightly affect performance. Storing this result in a local variable could improve readability and speed.
| public entryClicked(acc) { | ||
| let url = location.href + '?returnUrl=ledger/' + acc?.uniqueName + '/' + this.from + '/' + this.to; | ||
| if (isElectron) { | ||
| let ipcRenderer = (window as any).require('electron').ipcRenderer; | ||
| url = location.origin + location.pathname + '#./pages/ledger/' + acc?.uniqueName + '/' + this.from + '/' + this.to; | ||
| ipcRenderer.send('open-url', url); | ||
| } else { | ||
| (window as any).open(url); | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify electron usage and ensure safe URL assembly.
The code checks for an Electron environment and then uses IPC for URL navigation. For security and maintainability, confirm that the constructed URL query parameters (e.g., from, to) are sanitized or encoded if needed. Keep an eye on potential XSS vectors when injecting values directly into URLs.
| /** | ||
| * Handles the click event on an account entry, opening the ledger page for that account. | ||
| * Opens the ledger page in a new window or in an Electron environment using IPC. | ||
| * | ||
| * @param {any} acc - The account that was clicked. | ||
| * @memberof ProfitLossReportGridComponent | ||
| */ | ||
| public entryClicked(acc) { | ||
| let url = location.href + '?returnUrl=ledger/' + acc?.uniqueName + '/' + this.from + '/' + this.to; | ||
| if (isElectron) { | ||
| let ipcRenderer = (window as any).require('electron').ipcRenderer; | ||
| url = location.origin + location.pathname + '#./pages/ledger/' + acc?.uniqueName + '/' + this.from + '/' + this.to; | ||
| console.log(ipcRenderer.send('open-url', url)); | ||
| } else { | ||
| (window as any).open(url); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log and improve error handling.
Several issues need attention:
- Production code shouldn't contain console.log statements
- IPC communication results should be properly handled
- The JSDoc comment has incorrect @memberof tag
-console.log(ipcRenderer.send('open-url', url));
+ipcRenderer.send('open-url', url);Also consider adding error handling:
try {
ipcRenderer.send('open-url', url);
} catch (error) {
// Handle or report error appropriately
}| <filter-multi-currency | ||
| #filter | ||
| (filterValue)="searchData($event)" | ||
| (onPropertyChanged)="filterData()" | ||
| (seachChange)="searchChanged($event)" | ||
| (lastSyncDate)="lastDate($event)" | ||
| (expandAllChange)="expandAllEvent()" | ||
| [(expandAll)]="expandAll" | ||
| ></filter-multi-currency> |
There was a problem hiding this comment.
Fix typo in event name 'seachChange'.
The event name contains a typo: 'seachChange' should be 'searchChange'.
- (seachChange)="searchChanged($event)"
+ (searchChange)="searchChanged($event)"📝 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.
| <filter-multi-currency | |
| #filter | |
| (filterValue)="searchData($event)" | |
| (onPropertyChanged)="filterData()" | |
| (seachChange)="searchChanged($event)" | |
| (lastSyncDate)="lastDate($event)" | |
| (expandAllChange)="expandAllEvent()" | |
| [(expandAll)]="expandAll" | |
| ></filter-multi-currency> | |
| <filter-multi-currency | |
| #filter | |
| (filterValue)="searchData($event)" | |
| (onPropertyChanged)="filterData()" | |
| (searchChange)="searchChanged($event)" | |
| (lastSyncDate)="lastDate($event)" | |
| (expandAllChange)="expandAllEvent()" | |
| [(expandAll)]="expandAll" | |
| ></filter-multi-currency> |
| ::ng-deep .custom-placeholder .mat-form-field-label { | ||
| color: red !important; | ||
| } | ||
|
|
||
| ::ng-deep .custom-placeholder.mat-focused .mat-form-field-label { | ||
| color: red !important; | ||
| } | ||
|
|
||
| ::ng-deep .custom-placeholder .mat-form-field-label.mat-error { | ||
| color: red !important; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Angular Material style overrides.
::ng-deepis deprecated and may break in future Angular versions. Consider using Angular Material's theming system instead.- There are redundant color declarations that can be consolidated.
-::ng-deep .custom-placeholder .mat-form-field-label {
- color: red !important;
-}
-
-::ng-deep .custom-placeholder.mat-focused .mat-form-field-label {
- color: red !important;
-}
-
-::ng-deep .custom-placeholder .mat-form-field-label.mat-error {
- color: red !important;
-}
+// In your theme file
+@use '@angular/material' as mat;
+
+$custom-theme: mat.define-theme((
+ color: (
+ primary: mat.define-palette($custom-palette),
+ accent: mat.define-palette($custom-accent-palette),
+ )
+));
+
+// Or use CSS custom properties
+.custom-placeholder {
+ --mdc-text-field-label-color: red;
+}Committable suggestion skipped: line range outside the PR's diff.
| "request": { | ||
| "reportCurrency": "INR", | ||
| "companiesList": [ | ||
| { | ||
| "name": "Ankit", | ||
| "uniqueName": "ankitin172647646628106m1q5", | ||
| "from": "14-12-2024", | ||
| "to": "14-12-2024" | ||
| }, | ||
| { | ||
| "name": "Ankit123", | ||
| "uniqueName": "ankit1in1726484360480050fed", | ||
| "from": "14-12-2024", | ||
| "to": "14-12-2024" | ||
| }, | ||
| { | ||
| "name": "AnshiUSAComp", | ||
| "uniqueName": "anshiuus17195562098160dxttu", | ||
| "from": "14-12-2024", | ||
| "to": "14-12-2024" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Add request data validation
The request structure has several potential issues:
- Company names can be null
- Date ranges are not validated
- No validation for the report currency
Consider adding the following validations:
- Required company names
- Valid date ranges (from date should be before to date)
- Supported currency codes list
| "reportCurrency": "LTL", | ||
| "companiesList": [ |
There was a problem hiding this comment.
Fix currency mismatch between request and response
The request specifies "LTL" as the report currency, but all account entries in the response use "INR". This inconsistency defeats the purpose of the multi-currency reporting feature.
Ensure that:
- The response currency matches the requested currency
- Proper currency conversion is applied to all monetary values
- The currency code in the response reflects the requested currency
Also applies to: 53-55
| } | ||
| ] | ||
| }, | ||
| "date": "14-12-2024", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve date handling for international support
The current date implementation has several limitations:
- Uses DD-MM-YYYY format which might not be suitable for all locales
- Lacks timezone information
- Dates appear to be hardcoded in the response
Consider:
- Using ISO 8601 date format (YYYY-MM-DD)
- Including timezone information
- Making dates dynamic based on the actual report generation time
Also applies to: 934-935, 940-941
| "name": null, | ||
| "uniqueName": "ankit1in1726484360480050fed", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement consistent null handling
The response contains several null values that should be handled more explicitly:
- Company names are null in the request
- Message field is null
- Currency symbols are null
Consider:
- Using empty strings instead of null for optional string fields
- Removing optional fields when they're not used
- Documenting which fields can be null and why
Also applies to: 938-939, 5-5
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.html
Outdated
Show resolved
Hide resolved
| (filterValue)="searchData($event)" | ||
| (onPropertyChanged)="filterData()" | ||
| (lastSyncDate)="lastDate($event)" | ||
| (seachChange)="searchChanged($event)" |
There was a problem hiding this comment.
| (seachChange)="searchChanged($event)" | |
| (searchChange)="searchChanged($event)" |
| (expandAllChange)="expandAllEvent()" | ||
| [(expandAll)]="expandAll" | ||
| ></filter-multi-currency> | ||
| <div *ngIf="showLoader | async"> |
There was a problem hiding this comment.
remove div and add condition directly in loader
| [lastSyncDate]="lastSyncDate" | ||
| ></balance-sheet-report-grid> | ||
| </div> | ||
| <div *ngIf="!(showLoader | async) && !data" class="d-flex align-items-center justify-content-center tb-pl-bs-data"> |
There was a problem hiding this comment.
| <div *ngIf="!(showLoader | async) && !data" class="d-flex align-items-center justify-content-center tb-pl-bs-data"> | |
| <div *ngIf="!(showLoader | async) && !data" class="text-center tb-pl-bs-data"> | |
| <h2>{{ localeData?.no_data_found }}</h2> | |
| </div> |
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.ts
Show resolved
Hide resolved
| /** Stores the selected company data */ | ||
| private _selectedCompany: CompanyResponse; | ||
|
|
||
| constructor(public tlPlActions: TBPlBsActions, private cd: ChangeDetectorRef, private toaster: ToasterService, private componentStore: MultiCurrencyReportsComponentStore) { |
There was a problem hiding this comment.
| constructor(public tlPlActions: TBPlBsActions, private cd: ChangeDetectorRef, private toaster: ToasterService, private componentStore: MultiCurrencyReportsComponentStore) { | |
| constructor(public tlPlActions: TBPlBsActions, private changeDetectionRef: ChangeDetectorRef, private toaster: ToasterService, private componentStore: MultiCurrencyReportsComponentStore) { |
| padding: 0 !important; | ||
| } | ||
| i.glyphicon-search { | ||
| position: relative; |
There was a problem hiding this comment.
use class - position-relative font-14 pd-r1 cursor-pointer d-flex align-items-center light-gray
| } | ||
| } | ||
| } | ||
| @media screen and (max-width: 767px) { |
There was a problem hiding this comment.
Delete scss code for less than 992px
| /** Padding for the report grid */ | ||
| @Input() public padding: string; | ||
| /** Day.js instance for date formatting */ | ||
| public dayjs = dayjs; |
There was a problem hiding this comment.
| public dayjs = dayjs; | |
| public dayjs: any = dayjs; |
| /** Stores the last synchronization date */ | ||
| @Input() public lastSyncDate: string = ''; | ||
| /** Emits an event when the search input changes */ | ||
| @Output() public searchChange = new EventEmitter<string>(); |
There was a problem hiding this comment.
| @Output() public searchChange = new EventEmitter<string>(); | |
| @Output() public searchChange: EventEmitter<string> = new EventEmitter(); |
| public isExpandToggledDuringSearch: boolean; | ||
|
|
||
| constructor(private cd: ChangeDetectorRef, private zone: NgZone) { | ||
|
|
| /** Reference to the search input element */ | ||
| @ViewChild('searchInputEl', { static: true }) public searchInputEl: ElementRef; | ||
| /** Form control for the search input */ | ||
| public bsSearchControl: UntypedFormControl = new UntypedFormControl(); |
There was a problem hiding this comment.
Do not use Untyped Form use Only FormControl
| /** Indicates if expand all was toggled during search */ | ||
| public isExpandToggledDuringSearch: boolean; | ||
|
|
||
| constructor(private cd: ChangeDetectorRef, private zone: NgZone) { |
There was a problem hiding this comment.
| constructor(private cd: ChangeDetectorRef, private zone: NgZone) { | |
| constructor(private changeDetectorRef: ChangeDetectorRef, private zone: NgZone) { |
| * @memberof BalanceSheetReportGridComponent | ||
| */ | ||
| public ngOnChanges(changes: SimpleChanges) { | ||
| if (changes.expandAll && !changes.expandAll.firstChange && changes.expandAll.currentValue !== changes.expandAll.previousValue) { |
There was a problem hiding this comment.
| if (changes.expandAll && !changes.expandAll.firstChange && changes.expandAll.currentValue !== changes.expandAll.previousValue) { | |
| if (changes && changes.expandAll && !changes.expandAll.firstChange && changes.expandAll.currentValue !== changes.expandAll.previousValue) { |
There was a problem hiding this comment.
if (changes?.expandAll && !changes.expandAll.firstChange && changes.expandAll.currentValue !== changes.expandAll.previousValue) {
| this.toggleVisibility(this.bsData.liabilities, changes.expandAll.currentValue); | ||
| // always make first level visible .... | ||
| if (this.bsData.liabilities) { | ||
| each(this.bsData.liabilities, (childGroup: any) => { |
| } | ||
| }); | ||
| } | ||
|
|
| this.companyUniqueName = this.generalService.companyUniqueName; | ||
| return this.http.get(this.config.apiUrl + TB_PL_BS_API.GET_MULTI_CURRENCY_REPORT | ||
| ?.replace(':companyUniqueName', encodeURIComponent(this.companyUniqueName)) | ||
| ?.replace(':reportType', (reportType)) ).pipe( |
There was a problem hiding this comment.
| ?.replace(':reportType', (reportType)) ).pipe( | |
| ?.replace(':reportType', reportType) ).pipe( |
| @@ -0,0 +1,946 @@ | |||
| { | |||
| @@ -0,0 +1,455 @@ | |||
| { | |||
| @@ -0,0 +1,607 @@ | |||
| { | |||
...ce-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss
Show resolved
Hide resolved
...ance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.ts
Show resolved
Hide resolved
| * @memberof BalanceSheetReportGridRowComponent | ||
| */ | ||
| public ngOnChanges(changes: SimpleChanges) { | ||
| if (changes.groupDetail && !changes.groupDetail.firstChange && changes.groupDetail.currentValue !== changes.groupDetail.previousValue) { |
There was a problem hiding this comment.
| if (changes.groupDetail && !changes.groupDetail.firstChange && changes.groupDetail.currentValue !== changes.groupDetail.previousValue) { | |
| if (changes && changes.groupDetail && !changes.groupDetail.firstChange && changes.groupDetail.currentValue !== changes.groupDetail.previousValue) { |
| if (changes.groupDetail && !changes.groupDetail.firstChange && changes.groupDetail.currentValue !== changes.groupDetail.previousValue) { | ||
| this.cd.detectChanges(); | ||
| } | ||
| if (changes.search && !changes.search.firstChange && changes.search.currentValue !== changes.search.previousValue) { |
There was a problem hiding this comment.
| if (changes.search && !changes.search.firstChange && changes.search.currentValue !== changes.search.previousValue) { | |
| if (changes && changes.search && !changes.search.firstChange && changes.search.currentValue !== changes.search.previousValue) { |
| if (isElectron) { | ||
| let ipcRenderer = (window as any).require('electron').ipcRenderer; | ||
| url = location.origin + location.pathname + '#./pages/ledger/' + acc?.uniqueName + '/' + this.from + '/' + this.to; | ||
| console.log(ipcRenderer.send('open-url', url)); |
There was a problem hiding this comment.
| console.log(ipcRenderer.send('open-url', url)); | |
| ipcRenderer.send('open-url', url); |
| > | ||
| <div class="clearfix"> | ||
| <div class="row"> | ||
| <div class="col-xl-9 col-lg-12 col-md-12"> |
There was a problem hiding this comment.
| <div class="col-xl-9 col-lg-12 col-md-12"> | |
| <div class="col-xl-9 col-12"> |
| * @memberof TrialBalanceGridComponent | ||
| */ | ||
| public openAccountModal(account: any): void { | ||
| this.accountDetails = account; |
| #filter | ||
| (filterValue)="searchData($event)" | ||
| (onPropertyChanged)="filterData()" | ||
| (seachChange)="searchChanged($event)" |
There was a problem hiding this comment.
| (seachChange)="searchChanged($event)" | |
| (searchChange)="searchChanged($event)" |
| *ngIf="!(showLoader | async) && !(data$ | async)" | ||
| class="d-flex align-items-center justify-content-center tb-pl-bs-data" | ||
| > | ||
| <div class="d-flex"> |
There was a problem hiding this comment.
renove this div no use
| /** Observable for controlling loader state */ | ||
| public showLoader: Observable<boolean>; | ||
| /** Observable for report data */ | ||
| public data$: Observable<any> = this.componentStore.reportDataList$; |
There was a problem hiding this comment.
| public data$: Observable<any> = this.componentStore.reportDataList$; | |
| public reportDataList $: Observable<any> = this.componentStore.reportDataList$; |
| public tlPlActions: TBPlBsActions, | ||
| private toaster: ToasterService, | ||
| private componentStore: MultiCurrencyReportsComponentStore) { | ||
| this.showLoader = this.componentStore.inProgressReport$; |
There was a problem hiding this comment.
why write here write this
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts (2)
296-300: Remove unnecessary setTimeout in expandAllEventThe 1ms timeout with change detection seems unnecessary and could potentially cause UI flickering. If change detection is needed, it can be triggered directly.
public expandAllEvent() { - setTimeout(() => { - this.cd.detectChanges(); - }, 1); + this.cd.detectChanges(); }
277-289: Improve documentation for findIndex method's negative index logicThe method's use of negative indices for non-zero positions needs better documentation to explain the business logic and purpose.
Add a more detailed comment explaining why non-zero indices are multiplied by -1:
/** * Finds the index of the active financial year from the list of financial years. * + * @description + * Returns 0 for the first financial year, and negative indices for subsequent years. + * This special indexing is used to [explain business purpose here]. * * @param {*} activeFY - The currently active financial year. * @param {*} financialYears - The list of available financial years. * @return {number} - The index of the active financial year (0 or negative). * @memberof ProfitLossReportComponent */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts(1 hunks)apps/web-giddh/src/app/store/tl-pl/tl-pl.reducer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web-giddh/src/app/store/tl-pl/tl-pl.reducer.ts
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 93-93: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 99-99: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 164-164: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts (1)
1-22: LGTM! Component setup follows Angular best practices
The component is well-structured with appropriate dependencies for financial reporting functionality.
| public searchData(event: any) { | ||
| this.componentStore.creatMultiCurrencyReport({ reportType: ReportType.BalanceSheet, payload: event }); | ||
| } |
There was a problem hiding this comment.
Fix incorrect report type in searchData method
The method is using ReportType.BalanceSheet when it should be using ReportType.ProfitLoss since this is a profit-loss report component.
- this.componentStore.creatMultiCurrencyReport({ reportType: ReportType.BalanceSheet, payload: event });
+ this.componentStore.creatMultiCurrencyReport({ reportType: ReportType.ProfitLoss, payload: event });📝 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.
| public searchData(event: any) { | |
| this.componentStore.creatMultiCurrencyReport({ reportType: ReportType.BalanceSheet, payload: event }); | |
| } | |
| public searchData(event: any) { | |
| this.componentStore.creatMultiCurrencyReport({ reportType: ReportType.ProfitLoss, payload: event }); | |
| } |
| } else { | ||
| cogs = null; | ||
| } | ||
|
|
||
| if (data && data.message) { | ||
| setTimeout(() => { | ||
| this.toaster.clearAllToaster(); | ||
| this.toaster.infoToast(data.message); | ||
| }, 100); | ||
| } | ||
|
|
||
| if (cogs) { | ||
| let cogsGrp: ChildGroup = new ChildGroup(); | ||
| cogsGrp.isCreated = true; | ||
| cogsGrp.isVisible = true; | ||
| cogsGrp.isIncludedInSearch = true; | ||
| cogsGrp.isOpen = false; | ||
| cogsGrp.level1 = false; | ||
| cogsGrp.uniqueName = 'cogs'; | ||
| cogsGrp.groupName = 'Less: Cost of Goods Sold'; | ||
| cogsGrp.closingBalance = { | ||
| amount: cogs.cogs, | ||
| type: 'DEBIT' | ||
| }; | ||
| cogsGrp.accounts = []; | ||
| cogsGrp.childGroups = []; | ||
|
|
||
| Object.keys(cogs)?.filter(f => ['openingInventory', 'closingInventory', 'purchasesStockAmount', 'manufacturingExpenses', 'debitNoteStockAmount'].includes(f)).forEach(f => { | ||
| let cg = new ChildGroup(); | ||
| cg.isCreated = false; | ||
| cg.isVisible = false; | ||
| cg.isIncludedInSearch = true; | ||
| cg.isOpen = false; | ||
| cg.uniqueName = f; | ||
| cg.groupName = (f) ? f?.replace(/([a-z0-9])([A-Z])/g, '$1 $2') : ""; | ||
| cg.category = f === 'income'; | ||
| cg.closingBalance = { | ||
| amount: cogs[f], | ||
| type: 'CREDIT' | ||
| }; | ||
| cg.accounts = []; | ||
| cg.childGroups = []; | ||
| if (['purchasesStockAmount', 'manufacturingExpenses'].includes(f)) { | ||
| cg.groupName = `+ ${cg.groupName}`; | ||
| } else if (['closingInventory', 'debitNoteStockAmount'].includes(f)) { | ||
| cg.groupName = `- ${cg.groupName}`; | ||
| } | ||
| cogsGrp.childGroups.push(cg); | ||
| }); | ||
|
|
||
| this.cogsData = cogsGrp; | ||
| } | ||
|
|
||
| if (data && data.expArr) { | ||
| this.InitData(data.expArr, "expenses"); | ||
| data.expArr.forEach(g => { | ||
| g.category = "expenses"; | ||
| g.isVisible = true; | ||
| g.isCreated = true; | ||
| g.isIncludedInSearch = true; | ||
| g.isOpen = true; | ||
| g.childGroups.forEach(c => { | ||
| c.category = "expenses"; | ||
| c.isVisible = true; | ||
| c.isCreated = true; | ||
| c.isIncludedInSearch = true; | ||
| }); | ||
| }); | ||
| } | ||
| if (data && data.incArr) { | ||
| this.InitData(data.incArr, "income"); | ||
| data.incArr.forEach(g => { | ||
| g.category = "income"; | ||
| g.isVisible = true; | ||
| g.isCreated = true; | ||
| g.isIncludedInSearch = true; | ||
| g.isOpen = true; | ||
| g.childGroups.forEach(c => { | ||
| c.category = "income"; | ||
| c.isVisible = true; | ||
| c.isCreated = true; | ||
| c.isIncludedInSearch = true; | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| if (data?.incomeStatement?.grossProfit?.type === "DEBIT" && data.incomeStatement.grossProfit.amount) { | ||
| data.incomeStatement.grossProfit.amount = "-" + data.incomeStatement.grossProfit.amount; | ||
| } | ||
|
|
||
| if (data?.incomeStatement?.operatingProfit?.type === "DEBIT" && data.incomeStatement.operatingProfit.amount) { | ||
| data.incomeStatement.operatingProfit.amount = "-" + data.incomeStatement.operatingProfit.amount; | ||
| } | ||
|
|
||
| this.data = data; | ||
| } else { | ||
| this.data = null; | ||
| } | ||
| this.cd.detectChanges(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve code maintainability with optional chaining and method extraction
- Use optional chaining to simplify null checks
- Extract complex data transformation logic into separate methods for better maintainability
- if (data && data.incomeStatement && data.incomeStatement.costOfGoodsSold) {
+ if (data?.incomeStatement?.costOfGoodsSold) {
- if (data && data.message) {
+ if (data?.message) {
- if (data && data.expArr) {
+ if (data?.expArr) {
- if (data && data.incArr) {
+ if (data?.incArr) {Consider extracting the COGS data transformation (lines 106-146) and income/expense array processing (lines 148-179) into separate methods for better readability and maintainability.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 93-93: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 99-99: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 164-164: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| * | ||
| * @memberof BalanceSheetReportGridComponent | ||
| */ | ||
| public ngOnInit() { |
There was a problem hiding this comment.
| public ngOnInit() { | |
| public ngOnInit(): void { |
|
|
||
| } | ||
|
|
||
| public ngOnInit() { |
There was a problem hiding this comment.
| public ngOnInit() { | |
| public ngOnInit(): void { |
also write js doc
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (2)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.html (1)
84-154: 🛠️ Refactor suggestionExtract repeated logic into helper components.
The template contains repeated logic for rendering minus signs and amount fields in both group and account sections. This should be extracted into reusable components for better maintainability.
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.html (1)
65-70: 🛠️ Refactor suggestionImprove accessibility for account entries
Similar to a past review comment, enhance accessibility for account entries.
-<div class="row" (dblclick)="entryClicked(account)"> +<div class="row" + (dblclick)="entryClicked(account)" + (click)="entryClicked(account)" + (keydown.enter)="entryClicked(account)" + role="button" + tabindex="0" + [attr.aria-label]="'View ledger for ' + account.name">
🧹 Nitpick comments (37)
apps/web-giddh/src/app/shared/helpers/defaultDateFormat.ts (1)
2-2: Add test coverage or usage references for this new date format.It’s good to see an additional date format constant. To maintain consistency and prevent confusion within the codebase, consider adding usage references or at least ensuring you have test coverage (e.g., unit or integration tests) that verifies this format where it is applied.
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.html (3)
20-28: Enhance accessibility for the date picker input.The date picker input needs accessibility improvements:
- Add
aria-labelfor screen readers- Add
role="button"since it's clickable- Add keyboard support (Enter/Space key handlers)
<div class="custom-datepicker" (click)="showGiddhDatepicker($event)"> <input matInput type="text" + role="button" + aria-label="Select date range" + (keydown.enter)="showGiddhDatepicker($event)" + (keydown.space)="showGiddhDatepicker($event)" name="selectedDateRange" [value]="selectedDateRangeUi ? selectedDateRangeUi : ''" class="giddh-datepicker date-range-picker" /> </div>
50-61: Move complex logic from template to component class.The currency dropdown's clear handler contains business logic that should be in the component class:
-<reactive-dropdown-field - [label]="localeData?.company_list" - [placeholder]="commonLocaleData?.app_select" - [labelValue]="getForm('selectCurrency')?.value" - [options]="currencyList" - formControlName="selectCurrency" - (onClear)="getForm('selectCurrency')?.patchValue(this.activeCompany?.baseCurrency)" - [required]="true" -> +<reactive-dropdown-field + [label]="localeData?.company_list" + [placeholder]="commonLocaleData?.app_select" + [labelValue]="getForm('selectCurrency')?.value" + [options]="currencyList" + formControlName="selectCurrency" + (onClear)="handleCurrencyClear()" + [required]="isSelectCurrencyRequired" +>Add to component class:
public isSelectCurrencyRequired = true; public handleCurrencyClear(): void { this.getForm('selectCurrency')?.patchValue(this.activeCompany?.baseCurrency); }
63-82: Improve anchor tags and reduce code duplication.The expand/collapse section can be improved:
-<div class="mr-r15 mr-t1" > - <a - class="cursor-pointer expand-icon" - (click)="emitExpand(true)" - data-placement="top" - [matTooltip]="commonLocaleData?.app_expand_all" - *ngIf="!expandAll" - > - <span class="icon-expandIcon"></span> - </a> - <a - class="cursor-pointer expand-icon" - (click)="emitExpand(false)" - data-placement="top" - [matTooltip]="commonLocaleData?.app_collapse_all" - *ngIf="expandAll" - > - <span class="icon-collapse"></span> - </a> +<div class="mr-r15 mr-t1"> + <button + type="button" + class="expand-icon" + (click)="emitExpand(!expandAll)" + [matTooltip]="expandAll ? commonLocaleData?.app_collapse_all : commonLocaleData?.app_expand_all" + > + <span [class]="expandAll ? 'icon-collapse' : 'icon-expandIcon'"></span> + </button> </div>Changes made:
- Replaced anchor tags with semantic button elements
- Reduced duplication by using conditional expressions
- Simplified click handler
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.html (2)
21-36: Simplify minus sign conditions using a method.The complex conditional logic for displaying minus signs could be moved to a component method for better maintainability and reusability.
Consider adding a method to the component:
shouldShowMinusSign(item: any): boolean { return ( (item.category === 'income' && item.closingBalance.type === 'DEBIT' && item.closingBalance.amount !== 0) || (item.category === 'expenses' && item.closingBalance.type === 'CREDIT' && item.closingBalance.amount !== 0) ); }Then simplify the template:
- <span - *ngIf=" - groupDetail.category === 'income' && - groupDetail.closingBalance.type === 'DEBIT' && - groupDetail.closingBalance.amount !== 0 - " - >-</span - > - <span - *ngIf=" - groupDetail.category === 'expenses' && - groupDetail.closingBalance.type === 'CREDIT' && - groupDetail.closingBalance.amount !== 0 - " - >-</span - > + <span *ngIf="shouldShowMinusSign(groupDetail)">-</span>
60-73: Optimize virtual scroll performance.The virtual scroll implementation looks good, but consider these performance optimizations:
- The
itemSizecould be made dynamic based on the actual content size- The complex condition with
accountsFilterpipe might cause unnecessary re-rendersConsider moving the filter logic to the component:
get filteredAccounts() { return this.groupDetail?.accounts?.filter(account => !this.groupDetail?.isOpen && this.search ? // your filter logic here : true ); }Then simplify the template:
- *ngIf=" - (groupDetail?.isOpen || (search && !isExpandToggledDuringSearch)) && - (groupDetail?.accounts | accountsFilter: !groupDetail?.isOpen && search)?.length >= minimumViewportLimit - " + *ngIf="(groupDetail?.isOpen || (search && !isExpandToggledDuringSearch)) && filteredAccounts.length >= minimumViewportLimit"apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.html (3)
20-20: Ensure consistent usage of the recType pipe across balance fields.The
recTypepipe is applied inconsistently. It's used onforwardedBalanceandclosingBalancebut not ondebitTotalandcreditTotal.Consider applying the pipe consistently across all balance fields if it's required for proper data representation.
Also applies to: 39-39
1-41: Consider adding ARIA attributes for better accessibility.The financial data grid lacks proper accessibility attributes which could make it difficult for screen readers to interpret the data correctly.
Consider adding:
role="grid"to the main containeraria-labelattributes for financial values- Proper heading structure for group names
-<div +<div role="grid" aria-label="Financial Report Grid" class="row tb-pl-bs-grid-row" [financial-accordion]="groupDetail" [hidden]="!groupDetail.isVisible" *ngIf="groupDetail.groupName && (groupDetail.isVisible || groupDetail.isCreated)">
43-45: Consider extracting complex conditions into component methods.The template contains complex conditions that could be moved to the component class for better maintainability and testability.
Consider creating a method in the component:
shouldShowVirtualScroll(): boolean { return (this.groupDetail?.isOpen || (this.search && !this.isExpandToggledDuringSearch)) && this.getFilteredAccountsLength() >= this.minimumViewportLimit; } getFilteredAccountsLength(): number { return (this.groupDetail?.accounts | accountsFilter: !this.groupDetail?.isOpen && this.search)?.length || 0; }Then simplify the template:
-*ngIf="(groupDetail?.isOpen || (search && !isExpandToggledDuringSearch)) && - (groupDetail?.accounts | accountsFilter: !groupDetail?.isOpen && search)?.length >= minimumViewportLimit" +*ngIf="shouldShowVirtualScroll()"apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.scss (4)
1-12: Improve base styles maintainability.Consider these improvements:
- Remove
!importantflag as it makes styles harder to maintain.- Extract fixed pixel values into CSS variables for better consistency.
.daterange-picker-tb-pl-bs { - background-color: var(--color-white) !important; + background-color: var(--color-white); text-decoration: none; text-align: left; border: 1px solid var(--color-iron); vertical-align: middle; - height: 36px; + height: var(--input-height, 36px); font-size: var(--font-size-15); } .tb-btn-apply { - margin-top: 5px; + margin-top: var(--spacing-xs, 5px); }
13-40: Improve input wrapper and expand icon styles.
- Remove
!importantflag from positioning.- Consider using CSS Grid or Flexbox instead of absolute positioning.
- Extract repeated values into CSS variables.
.input-wrapper:before { position: absolute; content: ""; display: block; - height: 36px; + height: var(--input-height, 36px); width: 1px; background-color: var(--color-iron); - left: 37px !important; + left: var(--input-wrapper-spacing, 37px); z-index: 100; top: 0; } .expand-icon { - height: 36px; + height: var(--input-height, 36px); display: flex; align-items: flex-end; }
41-71: Improve layout stability and responsiveness.The absolute positioning of
.glyphicon-triangle-bottommight cause layout issues. Consider:
- Using a more flexible layout approach.
- Making the company branch dropdown width responsive.
.glyphicon-triangle-bottom { - z-index: 1; - position: absolute; - left: 152px; - font-size: 9px; - color: var(--color-iron); - top: 14px; + position: relative; + margin-left: auto; + font-size: var(--font-size-xs, 9px); + color: var(--color-iron); } .company-branch-dropdown { - width: 178px; + width: 100%; + max-width: 178px; }
123-124: Remove extra empty lines.Keep only one empty line at the end of the file.
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.ts (4)
58-58: Consider using optional chaining
Instead ofif (data && data.liabilities), you can simplify checks by usingdata?.liabilities. This reduces verbosity and aligns with modern TypeScript features.🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
66-66: Use optional chaining for consistency
Similarly, consider changingif (data && data.assets)toif (data?.assets). This makes the code more concise and consistent with TypeScript best practices.🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
82-102: Efficient recursion
TheinitDatamethod uses recursion for nested child groups. Validate performance if datasets are large, and ensure no cyclic dependencies. Consider adding maximum depth checks if data structure might be untrusted.
161-165: Potentially removesetTimeout
If possible, removing or reducing the delay can improve responsiveness. Examine whetherdetectChanges()can be called without the setTimeout wrapper.apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.ts (3)
55-65: Confirm existence ofgroupDetails
When subscribing toreportDataList$, confirm thatresponse?.groupDetailsis not undefined. You could use optional chaining or an early return to avoid potential runtime errors ifgroupDetailsis missing.
74-88: Nested recursion ininitData
YourinitDatamethod recurses intochildGroup.childGroups. If data can be deeply nested, ensure there's no risk of stack overflow or performance bottlenecks.
174-180: Check search logic
Whensearchis reset (!this.search), you toggleexpandAllto false. Confirm that this behavior is the intended user experience, especially if a user might want to keep items expanded.apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts (2)
174-180: Optional chaining suggestion
Instead of checkingif (this.searchInputEl && this.searchInputEl.nativeElement), considerthis.searchInputEl?.nativeElement?.focus()to simplify the condition.🧰 Tools
🪛 Biome (1.9.4)
[error] 176-176: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
220-222: Avoid assignments inwhilecondition
Using(child = child.parentNode)within thewhileloop condition can be confusing and is flagged by static analysis tools. Consider refactoring to maintain clarity:- while ((child = child.parentNode) && child !== parent) { + child = child.parentNode; + while (child && child !== parent) { ... }🧰 Tools
🪛 Biome (1.9.4)
[error] 220-220: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts (2)
214-218: Split the assignment for clarity
Usingconst a = this.search = '';conflates multiple assignments, which can confuse readers and static-analysis tools. Consider:- const a = this.search = ''; + this.search = ''; + const a = this.search;🧰 Tools
🪛 Biome (1.9.4)
[error] 216-216: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
302-303: Optional chaining
For expressions likeif (value && value.name)orif (value && value.startDate)you can simplify with optional chaining. For example:if (value?.name) {...}.Also applies to: 307-308
🧰 Tools
🪛 Biome (1.9.4)
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.scss (2)
21-41: Consider reusability
The.tb-sectionstyling includes nested.search-containerand.show-clear-search. If multiple components share similar patterns, consider extracting and reusing base styles.
42-50: Responsive design
Using media queries atmax-width: 1024pxis a good starting point. Double-check smaller breakpoints if there's usage on tablets or mobile devices with narrower screens.apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.html (1)
16-31: Optimize conditional rendering structureThe current structure has nested div elements with similar conditions. This can be simplified using
*ngIfwithelse.Consider this more maintainable structure:
- <giddh-page-loader *ngIf="showLoader | async" [cssClass]="'mt-0 mb-0'"></giddh-page-loader> - <div *ngIf="!(showLoader | async) && data"> - <balance-sheet-report-grid - #bsGrid - [search]="search" - [from]="from" - [to]="to" - [expandAll]="expandAll" - [bsData]="data" - [lastSyncDate]="lastSyncDate" - ></balance-sheet-report-grid> - </div> - <div *ngIf="!(showLoader | async) && !data" class="text-center tb-pl-bs-data"> - <h2>{{ localeData?.no_data_found }}</h2> - </div> + <ng-container *ngIf="showLoader | async; else content"> + <giddh-page-loader [cssClass]="'mt-0 mb-0'"></giddh-page-loader> + </ng-container> + <ng-template #content> + <ng-container *ngIf="data; else noData"> + <balance-sheet-report-grid + #bsGrid + [search]="search" + [from]="from" + [to]="to" + [expandAll]="expandAll" + [bsData]="data" + [lastSyncDate]="lastSyncDate" + ></balance-sheet-report-grid> + </ng-container> + <ng-template #noData> + <div class="text-center tb-pl-bs-data"> + <h2>{{ localeData?.no_data_found }}</h2> + </div> + </ng-template> + </ng-template>apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.ts (1)
60-61: Fix incorrect class name in method documentationThe JSDoc comment references
FinancialReportsComponentinstead ofMultiCurrencyReportsComponent.Update the class name in the documentation:
- * @memberof FinancialReportsComponent + * @memberof MultiCurrencyReportsComponentapps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.html (3)
1-6: Enhance accessibility for group rowsAdd ARIA attributes and keyboard navigation to improve accessibility.
<div class="row pl-grid-row" [financial-accordion]="groupDetail" *ngIf="groupDetail.groupName && (groupDetail.isVisible || groupDetail.isCreated)" - [ngClass]="{ 'd-none': !groupDetail.isVisible }" + [ngClass]="{ 'd-none': !groupDetail.isVisible }" + role="button" + tabindex="0" + [attr.aria-expanded]="groupDetail.isOpen" + [attr.aria-label]="'Group ' + groupDetail.groupName" >
39-43: Simplify viewport visibility conditionThe current condition is complex and hard to maintain. Consider extracting it to a component property or method.
+// In component class +get shouldShowVirtualScroll(): boolean { + return (this.groupDetail?.isOpen || (this.search && !this.isExpandToggledDuringSearch)) && + (this.groupDetail?.accounts | accountsFilter: !this.groupDetail?.isOpen && this.search)?.length >= this.minimumViewportLimit; +} -<cdk-virtual-scroll-viewport - *ngIf=" - (groupDetail?.isOpen || (search && !isExpandToggledDuringSearch)) && - (groupDetail?.accounts | accountsFilter: !groupDetail?.isOpen && search)?.length >= minimumViewportLimit - " +<cdk-virtual-scroll-viewport + *ngIf="shouldShowVirtualScroll"
71-96: Reduce code duplication in amount displayThe amount display pattern is repeated multiple times. Consider extracting it into a reusable template.
+<!-- Add to template --> +<ng-template #amountDisplay let-balance> + <span> + <span class="d-inline-flex"> + <amount-field + [amount]="balance.amount" + [currencySymbol]="false" + [currencyCode]="false"> + </amount-field> + </span> + {{ balance | recType }} + </span> +</ng-template> -<!-- Replace repeated code with template usage --> -<div class="col-4 account text-ellipsis text-right"> - <span> - <span class="d-inline-flex"> - <amount-field - [amount]="account.closingBalance.amount" - [currencySymbol]="false" - [currencyCode]="false"> - </amount-field> - </span> - {{ account.closingBalance | recType }} - </span> -</div> +<div class="col-4 account text-ellipsis text-right"> + <ng-container *ngTemplateOutlet="amountDisplay; context: { $implicit: account.closingBalance }"> + </ng-container> +</div>apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.store.ts (1)
57-57: Improve readability of state updateBreak down the long line into multiple lines for better readability.
- return this.patchState({ reportDataList: res.body.response, filterRequestData: { request: res.body.request, lastFetchedAt: res.body.lastFetchedAt }, inProgressReport: false }); + return this.patchState({ + reportDataList: res.body.response, + filterRequestData: { + request: res.body.request, + lastFetchedAt: res.body.lastFetchedAt + }, + inProgressReport: false + });apps/web-giddh/src/app/services/tl-pl.service.ts (1)
123-124: Consider using constant for report typeThe hard-coded string "balance-sheet" should be moved to the
ReportTypeenum for consistency with the multi-currency report methods.- ?.replace(':reportType', ("balance-sheet"))).pipe( + ?.replace(':reportType', ReportType.BalanceSheet)).pipe(apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.ts (1)
153-159: Improve modal handling to prevent potential issuesThe current approach of manually moving the modal element in the DOM could lead to memory leaks and race conditions.
Consider using Angular's built-in portal system:
import { ComponentPortal, DomPortalOutlet } from '@angular/cdk/portal'; @Injectable() export class ModalService { private portalOutlet: DomPortalOutlet; constructor( @Inject(DOCUMENT) private document: Document, private applicationRef: ApplicationRef, private injector: Injector, private componentFactoryResolver: ComponentFactoryResolver ) { // Create a portal outlet at body level this.portalOutlet = new DomPortalOutlet( document.body, componentFactoryResolver, applicationRef, injector ); } attachModal(component: any) { const portal = new ComponentPortal(component); return this.portalOutlet.attach(portal); } }apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.ts (1)
83-121: Reduce complexity and duplication in ngOnChangesThe method contains duplicate logic for handling assets and liabilities.
Consider extracting the common logic:
+private updateGroupVisibility(groups: any[], isVisible: boolean): void { + if (!groups) return; + groups.forEach((childGroup: any) => { + if (childGroup.isIncludedInSearch) { + childGroup.isVisible = true; + childGroup.accounts.forEach((account: any) => { + if (account.isIncludedInSearch) { + account.isVisible = true; + } + }); + } + }); +} public ngOnChanges(changes: SimpleChanges): void { if (changes?.expandAll?.currentValue !== changes?.expandAll?.previousValue) { this.isExpandToggledDuringSearch = true; if (this.bsData) { this.zone.run(() => { this.toggleVisibility(this.bsData.assets, changes.expandAll.currentValue); this.toggleVisibility(this.bsData.liabilities, changes.expandAll.currentValue); - // Duplicate code removed + this.updateGroupVisibility(this.bsData.liabilities, true); + this.updateGroupVisibility(this.bsData.assets, true); this.changeDetectionRef.detectChanges(); }); } } }apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.ts (2)
106-106: Remove unnecessary commentThe comment "//ankit" serves no purpose and should be removed.
218-237: Optimize group visibility toggle implementationThe current implementation has potential performance and stability issues:
- Uses traditional for loops instead of more readable forEach
- Unlimited recursion depth could cause stack overflow
Consider these improvements:
+const MAX_RECURSION_DEPTH = 100; -public toggleGroupVisibility(group: Array<ChildGroup>, isVisible: boolean): void { +public toggleGroupVisibility(group: Array<ChildGroup>, isVisible: boolean, depth: number = 0): void { + if (depth > MAX_RECURSION_DEPTH) { + console.warn('Maximum recursion depth reached'); + return; + } - for (let groupIndex = 0; groupIndex < group?.length; groupIndex++) { - const currentGroup: ChildGroup = group[groupIndex]; + group?.forEach((currentGroup: ChildGroup) => { if (currentGroup.isIncludedInSearch) { currentGroup.isCreated = isVisible; currentGroup.isVisible = isVisible; currentGroup.isOpen = isVisible; - for (let accountIndex = 0; accountIndex < currentGroup.accounts?.length; accountIndex++) { - const currentAccount: Account = currentGroup.accounts[accountIndex]; + currentGroup.accounts?.forEach((currentAccount: Account) => { if (currentAccount.isIncludedInSearch) { currentAccount.isCreated = isVisible; currentAccount.isVisible = isVisible; } - } + }); if (currentGroup.childGroups?.length) { - this.toggleGroupVisibility(currentGroup.childGroups, isVisible); + this.toggleGroupVisibility(currentGroup.childGroups, isVisible, depth + 1); } } - } + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.module.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.routing.module.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.store.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/pipes/accounts-filter.pipe.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.ts(1 hunks)apps/web-giddh/src/app/services/tl-pl.service.ts(2 hunks)apps/web-giddh/src/app/shared/helpers/defaultDateFormat.ts(1 hunks)apps/web-giddh/src/app/store/tl-pl/tl-pl.reducer.ts(2 hunks)apps/web-giddh/src/assets/locale/trial-profit-balance/hi.json(1 hunks)apps/web-giddh/src/assets/locale/trial-profit-balance/mr.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.routing.module.ts
- apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.html
- apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.html
- apps/web-giddh/src/app/store/tl-pl/tl-pl.reducer.ts
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html
- apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.html
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.scss
- apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html
- apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.html
- apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.scss
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.ts
- apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.ts
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 190-190: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.ts
[error] 58-58: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 66-66: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts
[error] 216-216: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 307-307: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.ts
[error] 173-173: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 205-205: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts
[error] 176-176: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 220-220: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (21)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.html (3)
1-6: LGTM: Translation setup is properly configured.
The translation directive is correctly implemented with appropriate event bindings for locale data.
30-49: LGTM: Company selection dropdown is well implemented.
The Material select implementation includes all necessary features:
- Multiple selection support
- Form control binding
- Proper translations
- Appropriate placeholder handling
94-116: LGTM: Datepicker template is well configured.
The datepicker implementation includes all necessary features:
- Custom wrapper component
- Comprehensive configuration options
- Proper event handling for date selection
- Modal structure follows best practices
apps/web-giddh/src/assets/locale/trial-profit-balance/mr.json (1)
7-8: LGTM! Marathi translations are accurate.
The translations for "company_list" and "currency_list" are correctly implemented in Marathi.
Let's verify consistency across all language files:
✅ Verification successful
Translations are consistent across language files
The translations for "company_list" and "currency_list" are properly implemented across all three language files:
- English (en.json): "Company List" and "Currency List"
- Hindi (hi.json): "कंपनी सूची" and "मुद्रा सूची"
- Marathi (mr.json): "कंपनी यादी" and "चलन यादी"
Each translation accurately represents the meaning in its respective language while maintaining consistent terminology. The Marathi translations under review are correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these keys exist in all language files with proper translations
# Expected: Should find these keys in en.json and other language files
echo "Checking for company_list and currency_list keys across all language files:"
fd -e json . apps/web-giddh/src/assets/locale/trial-profit-balance/ -x jq -r 'select(.company_list != null and .currency_list != null) | "\(.company_list) | \(.currency_list)"'
Length of output: 415
Script:
#!/bin/bash
# Let's identify which language files contain these translations
fd -e json . apps/web-giddh/src/assets/locale/trial-profit-balance/ -x echo "File: {}" -x jq -r 'select(.company_list != null and .currency_list != null) | "\(.company_list) | \(.currency_list)"'
# Also check if there are any other language files that might be missing these keys
echo -e "\nChecking for files missing these keys:"
fd -e json . apps/web-giddh/src/assets/locale/trial-profit-balance/ -x echo "File: {}" -x jq -r 'if (.company_list == null or .currency_list == null) then "Missing one or both keys" else empty end'
Length of output: 10648
Script:
#!/bin/bash
# Let's check the actual content of a few key files to verify translations
echo "Checking English translations:"
jq -r '.company_list, .currency_list' apps/web-giddh/src/assets/locale/trial-profit-balance/en.json
echo -e "\nChecking Hindi translations:"
jq -r '.company_list, .currency_list' apps/web-giddh/src/assets/locale/trial-profit-balance/hi.json
echo -e "\nChecking Marathi translations:"
jq -r '.company_list, .currency_list' apps/web-giddh/src/assets/locale/trial-profit-balance/mr.json
Length of output: 596
apps/web-giddh/src/assets/locale/trial-profit-balance/hi.json (1)
7-8: LGTM! Hindi translations are accurate.
The translations for "company_list" and "currency_list" are correctly implemented in Hindi, using appropriate Hindi terminology ("सूची" for list).
apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.html (2)
43-45: Consider storing pipe-filtered results to avoid repeated processing.
You repeatedly apply the accountsFilter pipe in multiple places. For large lists, repeated pipe calls might affect performance.
14-19: Verify currency handling requirements.
All amount-field components have currencySymbol and currencyCode set to false. Given that this is a multi-currency report component, we should verify if this is the intended behavior.
Also applies to: 106-111, 116-117, 122-123, 128-133
✅ Verification successful
Based on the search results, I can see that the amount-field component is used extensively throughout the application with different currency display configurations:
-
In the multi-currency reports (grid-row, profit-loss, balance-sheet), all instances consistently have
currencySymbolandcurrencyCodeset tofalse, similar to our file under review. -
In other parts of the application like voucher and VAT report components, the currency display is controlled based on conditions:
- Some use
baseCurrencySymboland setcurrencyCodeto false - Some use dynamic currency symbols from account details
- Some toggle based on
isMulticurrencyAccountflag
- Some use
The consistent pattern in multi-currency report components suggests this is intentional design where currency information is handled elsewhere in these reports.
Currency display is correctly configured for multi-currency reports
The current implementation where currencySymbol and currencyCode are set to false in the multi-currency report components is the correct pattern, as:
- All multi-currency report components follow this same pattern consistently
- The currency information appears to be handled at a higher level or displayed separately for these reports
- This allows for consistent numerical display across different currencies in the report grid
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of amount-field to understand the currency display pattern
rg -A 5 "amount-field" --type html
# Search for currency-related configurations
rg -A 5 "currencySymbol|currencyCode" --type ts
Length of output: 111786
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.scss (2)
72-95: LGTM! Well-structured SCSS for linked entities.
The nested structure is clean, maintainable, and follows SCSS best practices. The use of CSS variables for colors and fonts ensures consistency.
1-124: Further optimize SCSS structure.
Following up on the previous review comment, consider these additional SCSS optimizations:
- Group related styles using SCSS maps.
- Create mixins for repeated patterns.
- Use SCSS variables for common values.
// Example optimization
$dimensions: (
input: 36px,
icon: 32px
);
@mixin flex-center {
display: flex;
justify-content: center;
align-items: center;
}
// Usage
.icon-calender {
width: map-get($dimensions, input);
height: map-get($dimensions, input);
@include flex-center;
}apps/web-giddh/src/app/multi-currency-reports/balance-sheet/balance-sheet-report.component.ts (2)
1-19: Imports and Declarations Look Good
These imports are well-organized and the usage of Angular and RxJS operators is appropriate.
54-80: Validate the subscription and data assumption
Within the constructor, you assume response is a valid object for balance sheet data. Consider checking if response is missing certain properties to prevent runtime issues.
🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 66-66: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/trial-balance/trial-balance-report.component.ts (2)
1-15: Imports and class definition
All required imports are present, and the TrialBalanceReportComponent implements the correct Angular lifecycle hooks.
108-109: Reassess filterData
Currently, filterData() calls getMultiCurrencyReport without verifying user input or date ranges. Ensure the request object is formed properly if additional parameters are needed.
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts (1)
1-30: Component setup
The overall structure with OnInit, OnChanges, and OnDestroy is good. Inputs and outputs are clearly defined, promoting reusability.
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts (1)
220-241: Validate dynamic data
When forming data.companiesList, watch out for invalid or empty company identifiers. Consider early validation or fallback for missing form values.
apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.scss (1)
1-5: Consistent styling
The .trial-balance input height setting ensures a uniform look and feel. Continue verifying consistent styling across the multi-currency modules for a unified user experience.
apps/web-giddh/src/app/multi-currency-reports/pipes/accounts-filter.pipe.ts (1)
1-36: Implementation looks good!
The pipe implementation is well-structured with:
- Proper null checks and type safety
- Efficient use of nullish coalescing operator
- Pure pipe declaration for better performance
- Clear and concise documentation
apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.ts (1)
74-77:
Improve type safety and error handling in tabChanged method
The method accepts any type for selectedTabIndex which could lead to runtime errors.
Apply these changes:
- public tabChanged(selectedTabIndex: any): void {
+ public tabChanged(selectedTabIndex: number): void {
+ if (!Number.isInteger(selectedTabIndex) || selectedTabIndex < 0 || selectedTabIndex > 2) {
+ console.error('Invalid tab index:', selectedTabIndex);
+ return;
+ }
this.selectedTabIndex = selectedTabIndex;
- this.generalService.updateActivatedRouteQueryParams({ val: selectedTabIndex === 0 ? ReportType.TrialBalance : selectedTabIndex === 1 ? ReportType.ProfitLoss : ReportType.BalanceSheet, tabIndex: selectedTabIndex });
+ const reportType = selectedTabIndex === 0 ? ReportType.TrialBalance :
+ selectedTabIndex === 1 ? ReportType.ProfitLoss : ReportType.BalanceSheet;
+ this.generalService.updateActivatedRouteQueryParams({
+ val: reportType,
+ tabIndex: selectedTabIndex
+ });Likely invalid or redundant comment.
apps/web-giddh/src/app/services/tl-pl.service.ts (2)
184-202: 🛠️ Refactor suggestion
Improve type safety in getMultiCurrencyReport method
The method uses any types which reduces type safety. Consider creating proper interfaces for the response data structure.
- public getMultiCurrencyReport(reportType: string): Observable<BaseResponse<any, any>> {
+ public getMultiCurrencyReport(reportType: ReportType): Observable<BaseResponse<MultiCurrencyReport, void>> {
this.companyUniqueName = this.generalService.companyUniqueName;
+ if (!reportType) {
+ throw new Error('Report type is required');
+ }
return this.http.get(this.config.apiUrl + TB_PL_BS_API.GET_MULTI_CURRENCY_REPORT
?.replace(':companyUniqueName', encodeURIComponent(this.companyUniqueName))
?.replace(':reportType', reportType)).pipe(
map((res) => {
- let data: BaseResponse<any, any> = res;
+ let data: BaseResponse<MultiCurrencyReport, void> = res;
data.request = '';
return data;
}),
- catchError((e) => this.errorHandler.HandleCatch<any, any>(e, '')));
+ catchError((e) => this.errorHandler.HandleCatch<MultiCurrencyReport, void>(e, { reportType })));Likely invalid or redundant comment.
205-224:
Fix typo and improve type safety in creatMultiCurrencyReport method
The method has a typo in its name and uses any types which reduces type safety.
- public creatMultiCurrencyReport(reportType: string, payload: any): Observable<BaseResponse<any, any>> {
+ public createMultiCurrencyReport(reportType: ReportType, payload: MultiCurrencyPayload): Observable<BaseResponse<MultiCurrencyReport, MultiCurrencyPayload>> {
this.companyUniqueName = this.generalService.companyUniqueName;
+ if (!reportType || !payload) {
+ throw new Error('Report type and payload are required');
+ }
return this.http.post(this.config.apiUrl + TB_PL_BS_API.GET_MULTI_CURRENCY_REPORT
?.replace(':companyUniqueName', encodeURIComponent(this.companyUniqueName))
?.replace(':reportType', reportType), payload).pipe(
map((res) => {
- let data: BaseResponse<any, any> = res;
+ let data: BaseResponse<MultiCurrencyReport, MultiCurrencyPayload> = res;
data.request = '';
return data;
}),
- catchError((e) => this.errorHandler.HandleCatch<any, any>(e, '')));
+ catchError((e) => this.errorHandler.HandleCatch<MultiCurrencyReport, MultiCurrencyPayload>(e, { reportType, payload })));Let's verify the usage of this method name across the codebase:
apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.ts (1)
78-85:
Secure the URL construction in entryClicked method
The URL construction using template literals with raw parameters could be vulnerable to XSS attacks.
Apply this fix to encode URL parameters:
-let url = location.href + '?returnUrl=ledger/' + account?.uniqueName + '/' + this.from + '/' + this.to;
+let url = location.href + '?returnUrl=ledger/' +
+ encodeURIComponent(account?.uniqueName) + '/' +
+ encodeURIComponent(this.from) + '/' +
+ encodeURIComponent(this.to);Likely invalid or redundant comment.
| <ng-template #templateSection let-account> | ||
| <section | ||
| class="pl-grid-row account" | ||
| [ngClass]="{ 'd-none': !account.isVisible }" | ||
| (dblclick)="entryClicked(account)" | ||
| > | ||
| <div class="row"> | ||
| <div | ||
| class="col-8" | ||
| [ngStyle]="{ 'padding-left': padding + 20 + 'px' }" | ||
| [innerHTML]="account.name | lowercase | highlight: search" | ||
| ></div> | ||
| <div class="col-4 bd-rl text-left pull-right"> | ||
| <div class="row d-flex"> | ||
| <span class="col-sm-6 col-12 text-right"> | ||
| <span | ||
| *ngIf=" | ||
| account.category === 'income' && | ||
| account.closingBalance.type === 'DEBIT' && | ||
| account.closingBalance.amount !== 0 | ||
| " | ||
| >-</span | ||
| > | ||
| <span | ||
| *ngIf=" | ||
| account.category === 'expenses' && | ||
| account.closingBalance.type === 'CREDIT' && | ||
| account.closingBalance.amount !== 0 | ||
| " | ||
| >-</span | ||
| > | ||
| <span class="d-inline-flex"> | ||
| <amount-field | ||
| [amount]="account.closingBalance.amount" | ||
| [currencySymbol]="false" | ||
| [currencyCode]="false" | ||
| > | ||
| </amount-field> | ||
| </span> | ||
| </span> | ||
| <span class="col-sm-6 col-12 invisible"> | ||
| <span | ||
| *ngIf=" | ||
| account.category === 'income' && | ||
| account.closingBalance.type === 'DEBIT' && | ||
| account.closingBalance.amount !== 0 | ||
| " | ||
| >-</span | ||
| > | ||
| <span | ||
| *ngIf=" | ||
| account.category === 'expenses' && | ||
| account.closingBalance.type === 'CREDIT' && | ||
| account.closingBalance.amount !== 0 | ||
| " | ||
| >-</span | ||
| > | ||
| <span class="d-inline-flex"> | ||
| <amount-field | ||
| [amount]="account.closingBalance.amount" | ||
| [currencySymbol]="false" | ||
| [currencyCode]="false" | ||
| > | ||
| </amount-field> | ||
| </span> | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </section> | ||
| </ng-template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and reduce code duplication.
The account details section needs accessibility improvements and contains duplicated code from the group section:
- Add ARIA labels for financial data
- Extract common code into reusable components
Add accessibility attributes:
<section
class="pl-grid-row account"
[ngClass]="{ 'd-none': !account.isVisible }"
(dblclick)="entryClicked(account)"
+ role="row"
+ [attr.aria-label]="'Account: ' + account.name"
>Consider creating a reusable component for the amount display:
@Component({
selector: 'app-amount-display',
template: `
<span *ngIf="shouldShowMinusSign(item)">-</span>
<span class="d-inline-flex">
<amount-field
[amount]="item.closingBalance.amount"
[currencySymbol]="false"
[currencyCode]="false"
>
</amount-field>
</span>
`
})
export class AmountDisplayComponent {
@Input() item: any;
shouldShowMinusSign(item: any): boolean {
return (
(item.category === 'income' && item.closingBalance.type === 'DEBIT' && item.closingBalance.amount !== 0) ||
(item.category === 'expenses' && item.closingBalance.type === 'CREDIT' && item.closingBalance.amount !== 0)
);
}
}| <amount-field | ||
| [amount]="groupDetail.closingBalance.amount" | ||
| [currencySymbol]="false" | ||
| [currencyCode]="false" | ||
| > | ||
| </amount-field> | ||
| </span> | ||
| </span> | ||
| <span class="col-sm-6 col-12 invisible"> | ||
| <span class="d-inline-flex"> | ||
| <amount-field | ||
| [amount]="groupDetail.closingBalance.amount" | ||
| [currencySymbol]="false" | ||
| [currencyCode]="false" | ||
| > | ||
| </amount-field> | ||
| </span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate amount-field component.
The same amount-field component is duplicated within the same row, with one being invisible. This appears to be unnecessary and adds to the template complexity.
- <span class="col-sm-6 col-12 invisible">
- <span class="d-inline-flex">
- <amount-field
- [amount]="groupDetail.closingBalance.amount"
- [currencySymbol]="false"
- [currencyCode]="false"
- >
- </amount-field>
- </span>
- </span>📝 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.
| <amount-field | |
| [amount]="groupDetail.closingBalance.amount" | |
| [currencySymbol]="false" | |
| [currencyCode]="false" | |
| > | |
| </amount-field> | |
| </span> | |
| </span> | |
| <span class="col-sm-6 col-12 invisible"> | |
| <span class="d-inline-flex"> | |
| <amount-field | |
| [amount]="groupDetail.closingBalance.amount" | |
| [currencySymbol]="false" | |
| [currencyCode]="false" | |
| > | |
| </amount-field> | |
| </span> | |
| <amount-field | |
| [amount]="groupDetail.closingBalance.amount" | |
| [currencySymbol]="false" | |
| [currencyCode]="false" | |
| > | |
| </amount-field> | |
| </span> | |
| </span> |
| <section | ||
| class="account overflow-visible" | ||
| *ngIf="account.isVisible" | ||
| (dblclick)="accountInfo(account, $event)" | ||
| [exclude]="'.row-item'" | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for account interaction.
The account section relies on double-click for interaction, which might not be accessible to all users.
Consider adding keyboard interaction:
<section
class="account overflow-visible"
*ngIf="account.isVisible"
(dblclick)="accountInfo(account, $event)"
+ (keydown.enter)="accountInfo(account, $event)"
+ tabindex="0"
+ role="button"
+ [attr.aria-label]="'View details for account ' + account.name"
[exclude]="'.row-item'"
>📝 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.
| <section | |
| class="account overflow-visible" | |
| *ngIf="account.isVisible" | |
| (dblclick)="accountInfo(account, $event)" | |
| [exclude]="'.row-item'" | |
| > | |
| <section | |
| class="account overflow-visible" | |
| *ngIf="account.isVisible" | |
| (dblclick)="accountInfo(account, $event)" | |
| (keydown.enter)="accountInfo(account, $event)" | |
| tabindex="0" | |
| role="button" | |
| [attr.aria-label]="'View details for account ' + account.name" | |
| [exclude]="'.row-item'" | |
| > |
| .icon-calender { | ||
| position: absolute; | ||
| z-index: 1; | ||
| height: 32px; | ||
| width: 36px; | ||
| display: inline-flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| height: 36px !important; | ||
| cursor: pointer; | ||
| } | ||
| @media (max-width: 1099px) and (min-width: 1019px) { | ||
| .financial-select-year { | ||
| margin-bottom: 15px; | ||
| .company-branch-dropdown { | ||
| width: 158px; | ||
| } | ||
| .expand-collapse-icon { | ||
| position: absolute; | ||
| bottom: -36px; | ||
| right: 15px; | ||
| } | ||
| } | ||
| .download-data-icon { | ||
| padding-right: 60px !important; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix duplicate properties and improve media query.
- Remove duplicate height property in
.icon-calender. - Consider using standard breakpoint variables.
- Remove
!importantflag from padding.
.icon-calender {
position: absolute;
z-index: 1;
- height: 32px;
width: 36px;
display: inline-flex;
justify-content: center;
align-items: center;
height: 36px !important;
cursor: pointer;
}
-@media (max-width: 1099px) and (min-width: 1019px) {
+@media (max-width: var(--breakpoint-lg, 1099px)) and (min-width: var(--breakpoint-md, 1019px)) {
.financial-select-year {
margin-bottom: 15px;
.company-branch-dropdown {
width: 158px;
}
.expand-collapse-icon {
position: absolute;
bottom: -36px;
right: 15px;
}
}
.download-data-icon {
- padding-right: 60px !important;
+ padding-right: 60px;
}
}📝 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.
| .icon-calender { | |
| position: absolute; | |
| z-index: 1; | |
| height: 32px; | |
| width: 36px; | |
| display: inline-flex; | |
| justify-content: center; | |
| align-items: center; | |
| height: 36px !important; | |
| cursor: pointer; | |
| } | |
| @media (max-width: 1099px) and (min-width: 1019px) { | |
| .financial-select-year { | |
| margin-bottom: 15px; | |
| .company-branch-dropdown { | |
| width: 158px; | |
| } | |
| .expand-collapse-icon { | |
| position: absolute; | |
| bottom: -36px; | |
| right: 15px; | |
| } | |
| } | |
| .download-data-icon { | |
| padding-right: 60px !important; | |
| } | |
| } | |
| .icon-calender { | |
| position: absolute; | |
| z-index: 1; | |
| width: 36px; | |
| display: inline-flex; | |
| justify-content: center; | |
| align-items: center; | |
| height: 36px !important; | |
| cursor: pointer; | |
| } | |
| @media (max-width: var(--breakpoint-lg, 1099px)) and (min-width: var(--breakpoint-md, 1019px)) { | |
| .financial-select-year { | |
| margin-bottom: 15px; | |
| .company-branch-dropdown { | |
| width: 158px; | |
| } | |
| .expand-collapse-icon { | |
| position: absolute; | |
| bottom: -36px; | |
| right: 15px; | |
| } | |
| } | |
| .download-data-icon { | |
| padding-right: 60px; | |
| } | |
| } |
| (searchChange)="searchChanged($event)" | ||
| (expandAllChange)="expandAllEvent()" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate event binding
The searchChange event is bound twice - once in the filter component and once in the grid component. This could lead to duplicate event handling.
Keep only one event binding, preferably in the filter component:
<filter-multi-currency
#filter
(filterValue)="searchData($event)"
(onPropertyChanged)="filterData()"
(lastSyncDate)="lastDate($event)"
(searchChange)="searchChanged($event)"
(expandAllChange)="expandAllEvent()"
[(expandAll)]="expandAll"
></filter-multi-currency>
<div *ngIf="!(showLoader | async) && data">
<balance-sheet-report-grid
#bsGrid
[search]="search"
[from]="from"
[to]="to"
- (searchChange)="searchChanged($event)"
[expandAll]="expandAll"
[bsData]="data"
[lastSyncDate]="lastSyncDate"
></balance-sheet-report-grid>
</div>Also applies to: 23-23
| public expandAllEvent(): void { | ||
| setTimeout(() => { | ||
| this.changeDetectionRef.detectChanges(); | ||
| }, 1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid setTimeout for change detection
Using setTimeout for change detection is an anti-pattern. Consider using async pipe or proper state management.
- public expandAllEvent(): void {
- setTimeout(() => {
- this.changeDetectionRef.detectChanges();
- }, 1);
- }
+ public expandAllEvent(): void {
+ Promise.resolve().then(() => {
+ this.expandAll = !this.expandAll;
+ this.changeDetectionRef.markForCheck();
+ });
+ }Committable suggestion skipped: line range outside the PR's diff.
| this.componentStore.reportDataList$.pipe(takeUntil(this.destroyed$)).subscribe((response) => { | ||
| if (response) { | ||
| let data = prepareProfitLossData(cloneDeep(response)) as ProfitLossData; | ||
| let cogs; | ||
| if (data && data.incomeStatement && data.incomeStatement.costOfGoodsSold) { | ||
| cogs = cloneDeep(data.incomeStatement.costOfGoodsSold) as GetCogsResponse; | ||
| } else { | ||
| cogs = null; | ||
| } | ||
|
|
||
| if (cogs) { | ||
| let cogsGrp: ChildGroup = new ChildGroup(); | ||
| cogsGrp.isCreated = true; | ||
| cogsGrp.isVisible = true; | ||
| cogsGrp.isIncludedInSearch = true; | ||
| cogsGrp.isOpen = false; | ||
| cogsGrp.level1 = false; | ||
| cogsGrp.uniqueName = 'cogs'; | ||
| cogsGrp.groupName = 'Less: Cost of Goods Sold'; | ||
| cogsGrp.closingBalance = { | ||
| amount: cogs.cogs, | ||
| type: 'DEBIT' | ||
| }; | ||
| cogsGrp.accounts = []; | ||
| cogsGrp.childGroups = []; | ||
|
|
||
| Object.keys(cogs)?.filter(f => ['openingInventory', 'closingInventory', 'purchasesStockAmount', 'manufacturingExpenses', 'debitNoteStockAmount'].includes(f)).forEach(f => { | ||
| let childGroup = new ChildGroup(); | ||
| childGroup.isCreated = false; | ||
| childGroup.isVisible = false; | ||
| childGroup.isIncludedInSearch = true; | ||
| childGroup.isOpen = false; | ||
| childGroup.uniqueName = f; | ||
| childGroup.groupName = (f) ? f?.replace(/([a-z0-9])([A-Z])/g, '$1 $2') : ""; | ||
| childGroup.category = f === 'income'; | ||
| childGroup.closingBalance = { | ||
| amount: cogs[f], | ||
| type: 'CREDIT' | ||
| }; | ||
| childGroup.accounts = []; | ||
| childGroup.childGroups = []; | ||
| if (['purchasesStockAmount', 'manufacturingExpenses'].includes(f)) { | ||
| childGroup.groupName = `+ ${childGroup.groupName}`; | ||
| } else if (['closingInventory', 'debitNoteStockAmount'].includes(f)) { | ||
| childGroup.groupName = `- ${childGroup.groupName}`; | ||
| } | ||
| cogsGrp.childGroups.push(childGroup); | ||
| }); | ||
|
|
||
| this.cogsData = cogsGrp; | ||
| } | ||
|
|
||
| if (data && data.expArr) { | ||
| this.initData(data.expArr, "expenses"); | ||
| data.expArr.forEach(group => { | ||
| group.category = "expenses"; | ||
| group.isVisible = true; | ||
| group.isCreated = true; | ||
| group.isIncludedInSearch = true; | ||
| group.isOpen = true; | ||
| group.childGroups.forEach(childGroups => { | ||
| childGroups.category = "expenses"; | ||
| childGroups.isVisible = true; | ||
| childGroups.isCreated = true; | ||
| childGroups.isIncludedInSearch = true; | ||
| }); | ||
| }); | ||
| } | ||
| if (data && data.incArr) { | ||
| this.initData(data.incArr, "income"); | ||
| data.incArr.forEach(group => { | ||
| group.category = "income"; | ||
| group.isVisible = true; | ||
| group.isCreated = true; | ||
| group.isIncludedInSearch = true; | ||
| group.isOpen = true; | ||
| group.childGroups.forEach(childGroups => { | ||
| childGroups.category = "income"; | ||
| childGroups.isVisible = true; | ||
| childGroups.isCreated = true; | ||
| childGroups.isIncludedInSearch = true; | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| if (data?.incomeStatement?.grossProfit?.type === "DEBIT" && data.incomeStatement.grossProfit.amount) { | ||
| data.incomeStatement.grossProfit.amount = "-" + data.incomeStatement.grossProfit.amount; | ||
| } | ||
|
|
||
| if (data?.incomeStatement?.operatingProfit?.type === "DEBIT" && data.incomeStatement.operatingProfit.amount) { | ||
| data.incomeStatement.operatingProfit.amount = "-" + data.incomeStatement.operatingProfit.amount; | ||
| } | ||
|
|
||
| this.data = data; | ||
| } else { | ||
| this.data = null; | ||
| } | ||
| this.changeDetectionRef.detectChanges(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor complex data transformation logic in ngOnInit
The ngOnInit method is too complex and handles multiple responsibilities. Consider extracting the transformation logic into separate methods for better maintainability.
- Extract COGS data transformation (lines 60-106)
- Extract income/expense array processing (lines 108-139)
- Use optional chaining as suggested by static analysis
public ngOnInit(): void {
this.componentStore.reportDataList$.pipe(takeUntil(this.destroyed$)).subscribe((response) => {
if (response) {
let data = prepareProfitLossData(cloneDeep(response)) as ProfitLossData;
- let cogs;
- if (data && data.incomeStatement && data.incomeStatement.costOfGoodsSold) {
- cogs = cloneDeep(data.incomeStatement.costOfGoodsSold) as GetCogsResponse;
- } else {
- cogs = null;
- }
+ this.processCogsData(data);
+ this.processIncomeExpenseData(data);
+ this.processGrossOperatingProfit(data);
this.data = data;
} else {
this.data = null;
}
this.changeDetectionRef.detectChanges();
});
}
+private processCogsData(data: ProfitLossData): void {
+ const cogs = data?.incomeStatement?.costOfGoodsSold ?
+ cloneDeep(data.incomeStatement.costOfGoodsSold) as GetCogsResponse : null;
+ if (cogs) {
+ this.cogsData = this.createCogsGroup(cogs);
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| public accountInfo(account: any, event: Event): void { | ||
| this.searchService.loadDetails(account?.uniqueName).pipe(takeUntil(this.destroyed$)).subscribe(response => { | ||
| if (response?.body) { | ||
| this.accountDetails = response.body; | ||
| const parentGroups = response.body?.parentGroups?.join(', '); | ||
| const creditorsString = 'currentliabilities, sundrycreditors'; | ||
| const debtorsString = 'currentassets, sundrydebtors'; | ||
| if (parentGroups?.indexOf(creditorsString) > -1 || parentGroups?.indexOf(debtorsString) > -1) { | ||
| this.modalUniqueName = response.body?.uniqueName; | ||
| } else { | ||
| this.modalUniqueName = ''; | ||
| this.entryClicked(account); | ||
| } | ||
| this.changeDetectionRef.detectChanges(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and group checks
- The API call lacks error handling
- Hard-coded strings for group checks make maintenance difficult
Consider these improvements:
this.searchService.loadDetails(account?.uniqueName).pipe(takeUntil(this.destroyed$)).subscribe({
+ next: (response) => {
if (response?.body) {
this.accountDetails = response.body;
const parentGroups = response.body?.parentGroups?.join(', ');
- const creditorsString = 'currentliabilities, sundrycreditors';
- const debtorsString = 'currentassets, sundrydebtors';
+ const ACCOUNT_GROUPS = {
+ CREDITORS: 'currentliabilities, sundrycreditors',
+ DEBTORS: 'currentassets, sundrydebtors'
+ };
if (parentGroups?.indexOf(creditorsString) > -1 ||
parentGroups?.indexOf(debtorsString) > -1) {
this.modalUniqueName = response.body?.uniqueName;
} else {
this.modalUniqueName = '';
this.entryClicked(account);
}
this.changeDetectionRef.detectChanges();
}
+ },
+ error: (error) => {
+ console.error('Error loading account details:', error);
+ // Handle error appropriately
+ }
});Committable suggestion skipped: line range outside the PR's diff.
| public dayjs: any = dayjs; | ||
| /** Indicates if there is no data available */ | ||
| public noData: boolean; | ||
| /** Determines if the clear search button should be shown */ | ||
| public showClearSearch: boolean = false; | ||
| /** Form control for the search input */ | ||
| public bsSearchControl: FormControl = new FormControl(); | ||
| /** Stores the Giddh date format */ | ||
| public giddhDateFormat: string = GIDDH_DATE_FORMAT; | ||
| /** Stores local JSON data */ | ||
| public localeData: any = {}; | ||
| /** Stores common JSON data */ | ||
| public commonLocaleData: any = {}; | ||
| /** Indicates whether data should be hidden during search */ | ||
| public hideData: boolean; | ||
| /** Indicates if expand all was toggled during search */ | ||
| public isExpandToggledDuringSearch: boolean; | ||
| /** Observable to manage memory leaks */ | ||
| private destroyed$: ReplaySubject<boolean> = new ReplaySubject(1); | ||
|
|
||
| constructor(private changeDetectionRef: ChangeDetectorRef, private zone: NgZone) { | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety with proper type definitions
Several properties lack proper TypeScript types, which could lead to runtime errors.
Apply these type improvements:
-public dayjs: any = dayjs;
+public dayjs = dayjs;
-public localeData: any = {};
+interface LocaleData {
+ [key: string]: string;
+}
+public localeData: LocaleData = {};
-public commonLocaleData: any = {};
+public commonLocaleData: LocaleData = {};
-public bsSearchControl: FormControl = new FormControl();
+public bsSearchControl = new FormControl<string>('');Committable suggestion skipped: line range outside the PR's diff.
| public openAccountModal(account: any): void { | ||
| this.accountDetails = account; | ||
| this.activeGroupUniqueName = account?.parentGroups[account?.parentGroups?.length - 1]?.uniqueName; | ||
| this.createNewAccountDialogRef = this.dialog.open(this.createNew, { | ||
| width: 'var(--aside-pane-width)', | ||
| position: { | ||
| right: '0', | ||
| top: '0' | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve modal handling and type safety
The account modal implementation has several issues:
- Uses untyped
anyfor account parameter - Hard-coded CSS variable for width
- No error handling for dialog operations
Consider these improvements:
+interface AccountModalData {
+ account: Account;
+ activeGroupUniqueName: string;
+}
-public openAccountModal(account: any): void {
+public openAccountModal(account: Account): void {
this.accountDetails = account;
this.activeGroupUniqueName = account?.parentGroups[account?.parentGroups?.length - 1]?.uniqueName;
- this.createNewAccountDialogRef = this.dialog.open(this.createNew, {
+ this.createNewAccountDialogRef = this.dialog.open<typeof this.createNew, AccountModalData>(this.createNew, {
width: 'var(--aside-pane-width)',
position: {
right: '0',
top: '0'
- }
+ },
+ data: {
+ account: this.accountDetails,
+ activeGroupUniqueName: this.activeGroupUniqueName
+ }
+ });
+
+ this.createNewAccountDialogRef.afterClosed().subscribe({
+ next: (result) => {
+ // Handle dialog close
+ },
+ error: (error) => {
+ console.error('Error closing dialog:', error);
+ }
});
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.html (1)
68-73: 🛠️ Refactor suggestionEnhance accessibility for account interaction.
The account section relies solely on double-click for interaction, which may not be accessible to all users.
🧹 Nitpick comments (11)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss (2)
1-3: Consider adding responsive flex-wrap behaviorThe
nowrapproperty might cause horizontal scrolling on mobile devices. Consider allowing wrapping on smaller screens using a media query..pl-grid-row { - flex-wrap: nowrap; + flex-wrap: nowrap; + @media screen and (max-width: 768px) { + flex-wrap: wrap; + } }
4-9: Adjust width for better mobile responsivenessThe fixed 30% width might be too wide for mobile views. Consider adjusting the width for smaller screens.
.group { width: 30%; max-width: 30%; white-space: nowrap; min-width: 150px; + @media screen and (max-width: 768px) { + width: 100%; + max-width: 100%; + } }apps/web-giddh/src/app/multi-currency-reports/directives/financial-accordion.directive.ts (2)
9-10: Clarify or remove alias usage for Input property
Although the@Input('financial-accordion')alias clarifies the directive’s selector, consider either aligning it with a naming convention or removing the alias for consistency.
32-38: Consider renaming the forEach callback variable for clarity
Currently the variable is namedaccountin both accounts array loop and child groups array loop. Renaming the child group loop’s variable to something more descriptive (e.g.childGroup) will improve readability.this.data.childGroups.forEach((group: ChildGroup) => { - if (account.isIncludedInSearch) { - account.isVisible = !account.isVisible; - isChildVisible = account.isVisible; - account.isOpen = false; + if (group.isIncludedInSearch) { + group.isVisible = !group.isVisible; + isChildVisible = group.isVisible; + group.isOpen = false; } });apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.html (2)
77-86: Improve modal trigger implementation.The current implementation using an empty triggers attribute and manual control might lead to inconsistent behavior.
<div class="p-0 bd-r-right-none" [innerHTML]="account.name | lowercase | highlight: search" [popover]="accountDetailModalTemplate" #accountDetailPopup="bs-popover" - triggers="" + triggers="manual" container="body" containerClass="financial-report-account-detail-container" placement="bottom" + [adaptivePosition]="true" [isOpen]="modalUniqueName && modalUniqueName === account?.uniqueName" + (onShown)="handleModalShown()" + (onHidden)="handleModalHidden()" >
14-20: Consider extracting repeated amount-field pattern into a reusable component.The amount-field configuration is repeated multiple times throughout the template with identical settings.
Create a new component:
@Component({ selector: 'formatted-amount', template: ` <span class="d-inline-flex"> <amount-field [amount]="amount" [currencySymbol]="false" [currencyCode]="false"> </amount-field> </span> <ng-content></ng-content> ` }) export class FormattedAmountComponent { @Input() amount: number; }Then simplify the template usage:
- <span class="d-inline-flex"> - <amount-field - [amount]="groupDetail.forwardedBalance?.amount" - [currencySymbol]="false" - [currencyCode]="false"> - </amount-field> - </span> + <formatted-amount [amount]="groupDetail.forwardedBalance?.amount"> + </formatted-amount>Also applies to: 107-113, 117-119, 123-125, 129-135
apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html (3)
34-60: Enhance search accessibility and UX.The search implementation needs improvements:
- Add
aria-expandedattribute to the search toggle for accessibility.- The search icon is referenced in the click handler but not implemented.
- Consider adding a clear search button for better UX.
<span - [ngClass]="{ 'd-none': showClearSearch }" + [ngClass]="{ 'd-none': showClearSearch }" + aria-expanded="{{showClearSearch}}" class="d-flex align-items-center"> <span class="pd-r1"><strong>{{ localeData?.particular }}</strong></span> - <i class="position-relative font-14 pd-r1 cursor-pointer d-flex align-items-center light-gray" id="showSearch" (click)="toggleSearch()"></i> + <i class="icon-search position-relative font-14 pd-r1 cursor-pointer d-flex align-items-center light-gray" id="showSearch" (click)="toggleSearch()"></i> </span> <span [hidden]="!showClearSearch" class="show-clear-search"> <input matInput type="search" #searchInputEl class="form-control" [placeholder]="commonLocaleData?.app_search" aria-label="search" - [formControl]="accountSearchControl" + [formControl]="accountSearchControl"/> + <button + *ngIf="accountSearchControl.value" + class="clear-search-btn" + aria-label="Clear search" + (click)="accountSearchControl.reset()"> + <i class="icon-cross"></i> + </button> </span>
107-139: Optimize virtual scrolling performance.The current virtual scrolling implementation could be optimized:
- Fixed
itemSizeof 35px might not accommodate varying content heights.- Missing buffer size properties for smoother scrolling.
<cdk-virtual-scroll-viewport *ngIf="expandAll && !isSubGroup && !search" [itemSize]="35" + [minBufferPx]="500" + [maxBufferPx]="1000" class="trial-balance-viewport">Consider implementing a dynamic item size strategy if the content height varies significantly.
163-199: Add type safety to amount-field inputs.Consider adding type safety to the boolean inputs of amount-field component to prevent potential runtime errors.
<amount-field [amount]="data$?.forwardedBalance?.amount" - [currencySymbol]="false" - [currencyCode]="false" + [currencySymbol]="false as const" + [currencyCode]="false as const" > </amount-field>apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.html (2)
1-6: Add type safety to locale data bindingsConsider defining interfaces for the locale data structure to improve type safety and maintainability.
interface BalanceSheetLocaleData { particular: string; equity_liabilities: string; total_liabilities: string; ii_assets: string; total_assets: string; } interface CommonLocaleData { app_search: string; app_as_of: string; }
16-40: Enhance search accessibility and user experienceThe search implementation needs the following improvements:
- Add
aria-expandedattribute to indicate search visibility state- Add clear search button for better UX
- Add keyboard support for search toggle
<div - [ngClass]="{ 'd-none': showClearSearch }" + [ngClass]="{ 'd-none': showClearSearch }" + role="search" + [attr.aria-expanded]="showClearSearch" class="d-flex align-items-center"> <span class="mr-l15 mr-r05">{{ localeData?.particular }}</span> - <i class="icon-search" (click)="toggleSearch()"></i> + <i class="icon-search" + (click)="toggleSearch()" + (keydown.enter)="toggleSearch()" + tabindex="0" + role="button" + [attr.aria-label]="showClearSearch ? 'Hide search' : 'Show search'"> + </i> </div> <div [hidden]="!showClearSearch"> <input matInput type="search" #searchInputEl class="form-control" [placeholder]="commonLocaleData?.app_search" aria-label="search" - [formControl]="bsSearchControl" + [formControl]="bsSearchControl"> + <button + *ngIf="bsSearchControl.value" + class="clear-search" + (click)="bsSearchControl.reset()" + aria-label="Clear search"> + <i class="icon-cross"></i> + </button> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/directives/financial-accordion.directive.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.module.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html
- apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.html
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.html
- apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.module.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web-giddh/src/app/multi-currency-reports/directives/financial-accordion.directive.ts
[error] 13-15: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (8)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss (1)
18-20: LGTM! Good use of CSS variables for theming
The implementation correctly uses CSS variables for consistent theming across components.
apps/web-giddh/src/app/multi-currency-reports/directives/financial-accordion.directive.ts (1)
40-41: Verify visibility logic for parent group
This directive sets this.data.isVisible = true; unconditionally, making the parent always visible. If partial toggling is desired (i.e., parent should remain collapsed if all children are collapsed), revise this logic to match intended user behavior.
apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.html (1)
43-45: Consider storing pipe-filtered results to avoid repeated processing.
The complex condition with repeated pipe calls could affect performance with large datasets.
Consider storing the filtered accounts in a component property:
// In component class
filteredAccounts = this.groupDetail?.accounts.filter(account =>
!this.groupDetail?.isOpen && this.search ? this.accountsFilter(account) : true
);Then simplify the template:
- (groupDetail?.accounts | accountsFilter: !groupDetail?.isOpen && search)?.length >= minimumViewportLimit
+ filteredAccounts?.length >= minimumViewportLimitapps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html (3)
1-6: Verify translation file path exists.
The template uses translations from 'trial-profit-balance/trial-balance/grid'. Let's verify this path exists in the translation files.
83-125: Verify grid-report-row directive implementation.
The grid-report-row directive handles multiple inputs. Let's verify its implementation to ensure proper input handling and change detection.
✅ Verification successful
Let me search for the directive implementation with a different approach to verify the input handling.
Let me check the actual implementation of the component to verify the input handling.
Input handling and change detection are properly implemented
The grid-report-row component implementation shows:
- All required inputs are properly declared with
@Input()decorators - Change detection is optimized using
ChangeDetectionStrategy.OnPush - Input changes are properly handled in
ngOnChangeswith specific checks forgroupDetailandsearch - Memory management is handled correctly with proper cleanup in
ngOnDestroy - The component implements proper type safety and follows Angular best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check grid-report-row directive implementation
ast-grep --pattern 'export class $_ {
@Input() isExpandToggledDuringSearch: boolean;
@Input() groupDetail: any;
@Input() padding: number;
@Input() search: string;
@Input() from: Date;
@Input() to: Date;
@Input() expandAll: boolean;
$$$
}'
Length of output: 256
Script:
#!/bin/bash
# Search for grid-report-row directive implementation
fd "grid-report-row" --type f --exec rg -l "grid-report-row"
# Search for directive decorator and class definition
rg -A 10 "@Directive.*grid-report-row"
Length of output: 185
Script:
#!/bin/bash
# Let's examine the content of the grid-report-row component
cat ./apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.ts
Length of output: 6348
211-222: Prevent potential memory leaks in aside menu.
Ensure proper cleanup of subscriptions and dialog references in the component class:
- Implement unsubscribe for getUpdatedList event.
- Properly dispose of createNewAccountDialogRef.
✅ Verification successful
Memory management is properly implemented
After reviewing the component implementation, I can confirm that:
- The component properly implements
OnDestroyand uses aReplaySubject(destroyed$) to handle subscription cleanup. - All subscriptions (like
accountSearchControl.valueChanges) are properly managed usingtakeUntil(this.destroyed$). - The
createNewAccountDialogRefis only used in template for closing the dialog, which is a valid cleanup approach as Angular's Material Dialog service handles its own cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the component implements proper cleanup
ast-grep --pattern 'export class $_ implements OnDestroy {
$$$
ngOnDestroy() {
$$$
}
$$$
}'
Length of output: 105
Script:
#!/bin/bash
# Search for the component file and check its implementation
fd "trial-balance-report-grid.component.ts" --exec cat {}
Length of output: 10130
Script:
#!/bin/bash
# Check for dialog service usage and subscription patterns
rg -A 5 "createNewAccountDialogRef"
Length of output: 10169
Script:
#!/bin/bash
# Check for event subscriptions and cleanup
ast-grep --pattern 'export class TrialBalanceReportGridComponent {
$$$
}'
Length of output: 77
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.html (2)
56-78: Consider consolidating duplicate template code
The group details template is duplicated for both assets and liabilities sections. Consider extracting it into a shared template or component.
Also applies to: 126-147
95-101: Add null check and improve currency display options
The amount-field component needs improvements:
- Add null checks for amount values
- Consider making currency display configurable at a higher level
Also applies to: 107-113, 166-172, 178-184
...ce-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss
Show resolved
Hide resolved
| constructor() { | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary constructor
The constructor is empty and does not perform any initialization tasks. You can safely remove it to simplify and reduce clutter.
constructor() {
-
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-15: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
| <div | ||
| class="row tb-pl-bs-grid-row" | ||
| [financial-accordion]="groupDetail" | ||
| [hidden]="!groupDetail.isVisible" | ||
| *ngIf="groupDetail.groupName && (groupDetail.isVisible || groupDetail.isCreated)" | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for the financial accordion.
The financial-accordion directive lacks proper accessibility attributes for keyboard navigation and screen readers.
<div
class="row tb-pl-bs-grid-row"
[financial-accordion]="groupDetail"
+ role="region"
+ [attr.aria-expanded]="groupDetail.isVisible"
+ [attr.aria-label]="groupDetail.groupName + ' financial group'"
[hidden]="!groupDetail.isVisible"
*ngIf="groupDetail.groupName && (groupDetail.isVisible || groupDetail.isCreated)"
>📝 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.
| <div | |
| class="row tb-pl-bs-grid-row" | |
| [financial-accordion]="groupDetail" | |
| [hidden]="!groupDetail.isVisible" | |
| *ngIf="groupDetail.groupName && (groupDetail.isVisible || groupDetail.isCreated)" | |
| > | |
| <div | |
| class="row tb-pl-bs-grid-row" | |
| [financial-accordion]="groupDetail" | |
| role="region" | |
| [attr.aria-expanded]="groupDetail.isVisible" | |
| [attr.aria-label]="groupDetail.groupName + ' financial group'" | |
| [hidden]="!groupDetail.isVisible" | |
| *ngIf="groupDetail.groupName && (groupDetail.isVisible || groupDetail.isCreated)" | |
| > |
...reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.html
Show resolved
Hide resolved
| <th class="text-right font-16" width="33.33%"> | ||
| {{ commonLocaleData?.app_as_of }} | ||
| <span | ||
| *ngIf="lastSyncDate.length && (lastSyncDate !== 'Invalid Date')" |
There was a problem hiding this comment.
| *ngIf="lastSyncDate.length && (lastSyncDate !== 'Invalid Date')" | |
| *ngIf="lastSyncDate && dayjs(lastSyncDate)?.isValid()" |
| " | ||
| > | ||
| </ng-container> | ||
| <!--/Grid--> |
| /** Determines if the clear search button should be shown */ | ||
| public showClearSearch: boolean = false; | ||
| /** Form control for the search input */ | ||
| public bsSearchControl: FormControl = new FormControl(); |
There was a problem hiding this comment.
| public bsSearchControl: FormControl = new FormControl(); | |
| public bsSearchControl: FormControl = new FormControl<string>(''); |
| * @returns {boolean} True if the element is a child, false otherwise | ||
| * @memberof BalanceSheetReportGridComponent | ||
| */ | ||
| public childOf(child: any, parent: any): boolean { |
There was a problem hiding this comment.
use from general service
| <div | ||
| class="row pl-grid-row" | ||
| [financial-accordion]="groupDetail" | ||
| *ngIf="groupDetail.groupName && (groupDetail.isVisible || groupDetail.isCreated)" |
| this.activatedRoute.queryParams.pipe(takeUntil(this.destroyed$)).subscribe((val) => { | ||
| if (val.tabIndex) { | ||
| this.selectedTabIndex = Number(val.tabIndex); | ||
| this.tabChanged(this.selectedTabIndex) |
There was a problem hiding this comment.
| this.tabChanged(this.selectedTabIndex) | |
| this.tabChanged(this.selectedTabIndex); |
| class="table-header-sticky" > | ||
| <!-- Trial Balance Tab --> | ||
| <mat-tab label="{{ localeData?.tabs?.trial_balance }}"> | ||
| <div class="container-fluid pd-t1"> |
There was a problem hiding this comment.
| <div class="container-fluid pd-t1"> | |
| <div class="container-fluid pd-t1" *ngIf="selectedTabIndex === 0"> |
| @@ -0,0 +1,37 @@ | |||
| <div | |||
| class="relative" | |||
There was a problem hiding this comment.
| class="relative" |
| @@ -0,0 +1,37 @@ | |||
| <div | |||
There was a problem hiding this comment.
| <div | |
| <ng-container |
| const parentNode = this.document.querySelector('.financial-report-account-detail-container'); | ||
| /* Need to remove the element from the popover so that it could be attached to body as we show the account | ||
| modal within a popover which can't display the modal within it */ | ||
| this.renderer.addClass(modalInstance._element.nativeElement, 'm-0') |
There was a problem hiding this comment.
| this.renderer.addClass(modalInstance._element.nativeElement, 'm-0') | |
| this.renderer.addClass(modalInstance._element.nativeElement, 'm-0'); |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss (2)
1-6: Consider adding overflow handling for small screensThe width constraints are well-defined, but consider adding
overflow: hiddento prevent content from breaking the layout on very small screens..group { width: 30%; max-width: 30%; white-space: nowrap; min-width: 150px; + overflow: hidden; }
15-17: Consider using a more generic class nameThe class name
trial-balance-viewportseems too specific for a style that only sets a background color. Consider using a more generic name if this background style is meant to be reused across different components.-.trial-balance-viewport { +.themed-viewport { background-color: var(--color-common-quinary-background-for-all-components); }apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts (3)
88-109: Consider removing the extra setTimeout logic and direct data togglingYou are toggling
this.hideDatato true and then restoring it after a 10ms delay within asetTimeout. This pattern might cause unexpected race conditions if multiple subscriptions fire in quick succession. Consider directly applyingthis.hideData = false;afterthis.searchChange.emit(...)if no substantial asynchronous operation is required. Also,detectChanges()can be called immediately post-emission to ensure an updated view without resorting to a timer.
119-169: Reduce duplication by centralizing visibility toggling logicWithin
ngOnChanges, you manually toggleincArrandexpArrelements' visibility, then apply near-identical logic to the same arrays again. Similarly, there's repeated logic forthis.cogsData. Consider centralizing this logic to avoid code duplication—for instance, by calling a helper method to toggle array items and pass in any relevant parameters.
177-183: Leverage optional chaining for safer property accessCurrently, you check both
this.searchInputElandthis.searchInputEl.nativeElement. Replacing this check with optional chaining can increase readability and prevent possible undefined property errors, e.g.:-if (this.searchInputEl && this.searchInputEl.nativeElement) { +if (this.searchInputEl?.nativeElement) { setTimeout(() => { this.searchInputEl.nativeElement.focus(); }, 200); }🧰 Tools
🪛 Biome (1.9.4)
[error] 179-179: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.html (1)
1-36: Ripple effect considerationYou have set
[disableRipple]="true"on themat-tab-group. If the goal is to unify UX with a simpler interaction, this is valid. Otherwise, consider enabling the ripple to maintain the default Material design experience, which can provide visual feedback to users.apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.ts (1)
56-65: Replaceacc: anywith a stronger interfaceThe
entryClickedmethod acceptsacc: anyand usesacc?.uniqueName. Use a dedicated interface or type if possible to avoidany—it clarifies usage and prevents runtime errors. For instance:interface AccountInfo { uniqueName: string; /* other fields */ }Then:
- public entryClicked(acc: any): void { + public entryClicked(acc: AccountInfo): void {apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.ts (1)
60-69: Strengthen the account parameter type to avoidanyCurrently, the
accparameter is declared asany. Define an appropriate interface or use an existing model (e.g.,Account) that includesuniqueNameto ensure better compile-time checks and code clarity.apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.ts (2)
53-54: Improve type safety for locale data properties.The locale data properties use
anytype which reduces type safety.Apply this improvement:
-public localeData: any = {}; -public commonLocaleData: any = {}; +interface LocaleData { + [key: string]: string; +} +public localeData: LocaleData = {}; +public commonLocaleData: LocaleData = {};Also applies to: 63-66
134-150: Optimize change detection in search value subscription.The current implementation triggers change detection for every search value change, which could impact performance.
Apply this improvement:
this.bsSearchControl.valueChanges.pipe( - debounceTime(700), takeUntil(this.destroyed$)) + debounceTime(700), + takeUntil(this.destroyed$)) .subscribe((newValue) => { if (newValue) { this.searchInput = newValue; this.hideData = true; this.searchChange.emit(this.searchInput); this.isExpandToggledDuringSearch = false; if (newValue === '') { this.showClearSearch = false; } - setTimeout(() => { + this.zone.run(() => { this.hideData = false; this.changeDetectionRef.detectChanges(); - }, 10); + }); } });apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.html (1)
27-36: Enhance accessibility for search input.The search input lacks proper accessibility attributes.
Apply these improvements:
<input matInput type="search" #searchInputEl class="form-control" [placeholder]="commonLocaleData?.app_search" aria-label="search" + role="searchbox" + [attr.aria-expanded]="showClearSearch" + [attr.aria-controls]="'searchResults'" [formControl]="bsSearchControl" />apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts (3)
126-128: Improve change detection reference checkThe current check for destroyed change detector could be improved.
- if (!this.changeDetectionRef['destroyed']) { + if (!this.changeDetectionRef.destroyed) { this.changeDetectionRef.detectChanges(); }
109-163: Consider combining related subscriptionsThe multiple subscriptions in ngOnInit could be combined using RxJS operators for better maintainability.
ngOnInit(): void { // Combine related streams combineLatest([ this.componentStore.universalDate$.pipe(distinctUntilChanged()), this.componentStore.currencyList$, this.componentStore.companyList$, this.componentStore.filterRequestData$ ]).pipe( takeUntil(this.destroyed$) ).subscribe(([dateObj, currency, companies, filterRequestData]) => { // Handle each stream's data if (dateObj) { // Handle date changes } if (currency) { // Handle currency changes } // ... and so on }); // Keep search control separate as it needs debounceTime this.accountSearchControl.valueChanges.pipe( debounceTime(700), takeUntil(this.destroyed$) ).subscribe((newValue) => { // Handle search changes }); }
290-310: Improve null checks with optional chainingThe date selection callback could benefit from optional chaining for better readability.
- if (value && value.event === "cancel") { + if (value?.event === "cancel") { - if (value && value.name) { + if (value?.name) { - if (value && value.startDate && value.endDate) { + if (value?.startDate && value?.endDate) {🧰 Tools
🪛 Biome (1.9.4)
[error] 297-297: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.module.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.scss(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.scss
- apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html
- apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.html
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.html
- apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.scss
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.scss
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.scss
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/components/profit-loss-grid-row/profit-loss-report-grid-row.component.html
- apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.html
- apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.scss
- apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.component.ts
👮 Files not reviewed due to content moderation or server errors (3)
- apps/web-giddh/src/app/multi-currency-reports/grid-row/grid-report-row.component.ts
- apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.ts
- apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/balance-sheet-report-grid.component.ts
[error] 161-161: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts
[error] 211-211: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 297-297: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.ts
[error] 177-177: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/components/profit-loss-grid/profit-loss-report-grid.component.ts
[error] 179-179: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web-giddh/src/app/multi-currency-reports/profit-loss/profit-loss-report.component.ts
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 108-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
apps/web-giddh/src/app/multi-currency-reports/balance-sheet/components/balance-sheet-grid/components/balance-sheet-grid-row/balance-sheet-report-grid-row.component.scss (1)
7-14: Text truncation implementation looks good
The current implementation using white-space, overflow, and text-overflow properties is the correct approach for single-line truncation, as discussed in the previous review.
apps/web-giddh/src/app/multi-currency-reports/multi-currency-reports.module.ts (1)
86-86:
Move FinancialSearchPipe from imports to declarations
Pipes registered in the imports array may not function correctly. According to Angular best practices, pipes should be declared in the @NgModule declarations array:
imports: [
// ...
- FinancialSearchPipe,
// ...
],
declarations: [
// ...
+ FinancialSearchPipe,
// ...
],Likely invalid or redundant comment.
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts (3)
1-20: LGTM! Good use of OnPush change detection
The component setup is well-organized with appropriate imports and uses ChangeDetectionStrategy.OnPush, which is good for performance optimization.
211-213:
Remove assignment from expression
The assignment within the expression makes the code harder to understand and maintain.
- const a = this.search = '';
+ this.search = '';
+ this.searchChange.emit(this.search);Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 211-211: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
253-257: Verify the necessity of setTimeout
The setTimeout usage in emitExpand seems unnecessary unless there's a specific reason for the delay.
If there's no specific reason, consider removing the setTimeout:
public emitExpand(event: boolean): void {
- setTimeout(() => {
this.expandAllChange.emit(event);
- }, 10);
}| imports: [ | ||
| CommonModule, | ||
| FormsModule, | ||
| ReactiveFormsModule, | ||
| Daterangepicker, | ||
| MultiCurrencyReportsRoutingModule, | ||
| HighlightModule, | ||
| RecTypeModule, | ||
| ClickOutsideModule, | ||
| CurrencyModule, | ||
| AccountDetailModalModule, | ||
| ScrollingModule, | ||
| TranslateDirectiveModule, | ||
| HamburgerMenuModule, | ||
| GiddhPageLoaderModule, | ||
| AmountFieldComponentModule, | ||
| DatepickerWrapperModule, | ||
| PopoverModule.forRoot(), | ||
| AsideMenuAccountModule, | ||
| MatTooltipModule, | ||
| MatButtonModule, | ||
| MatFormFieldModule, | ||
| MatSelectModule, | ||
| FinancialSearchPipe, | ||
| MatInputModule, | ||
| TooltipModule, | ||
| MatTabsModule, | ||
| MatListModule, | ||
| FormFieldsModule | ||
| ], |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now verify the usage of imported modules in the multi-currency-reports module:
Most imported modules are actively used, but some can be removed
- Actively used modules:
- Material modules (MatTabsModule, MatFormFieldModule, MatSelectModule, MatTooltipModule, MatButtonModule) - Used in templates for tabs, form fields, select dropdowns, tooltips and buttons
- FormsModule/ReactiveFormsModule - Used for form handling in filter component
- AmountFieldComponentModule - Used extensively for displaying amounts
- DatepickerWrapperModule - Used in filter component for date selection
- GiddhPageLoaderModule - Used in all report components for loading states
- HighlightModule - Used for highlighting search results in templates
- RecTypeModule - Used for formatting record types in templates
- ClickOutsideModule - Used for handling outside clicks in search
- TranslateDirectiveModule - Used for translations via appTranslate directive
- HamburgerMenuModule - Used in main component template
- AsideMenuAccountModule - Used in trial balance grid
Modules that can be safely removed:
- ScrollingModule - No virtual scrolling usage found
- CurrencyModule - No currency component usage found
- PopoverModule - Only basic popover functionality used, can use MatTooltip instead
- MatInputModule - No mat-input usage found
- MatListModule - No mat-list usage found
- FormFieldsModule - No form-fields component usage found
- TooltipModule - Redundant since MatTooltipModule is used
🔗 Analysis chain
Verify usage of all imported modules to ensure minimal overhead
A large number of modules are imported here. Consider verifying whether each is actively used. Unused modules add to bundle size and complexity.
Here is a script to search for references to these modules within your codebase:
Let me analyze the search results to verify the usage of imported modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references of each module in the entire repo to confirm usage.
modules=("FormsModule" "ReactiveFormsModule" "Daterangepicker" "HighlightModule" "RecTypeModule" "ClickOutsideModule" "CurrencyModule" "AccountDetailModalModule" "ScrollingModule" "TranslateDirectiveModule" "HamburgerMenuModule" "GiddhPageLoaderModule" "AmountFieldComponentModule" "DatepickerWrapperModule" "PopoverModule" "AsideMenuAccountModule" "MatTooltipModule" "MatButtonModule" "MatFormFieldModule" "MatSelectModule" "FinancialSearchPipe" "MatInputModule" "TooltipModule" "MatTabsModule" "MatListModule" "FormFieldsModule")
for mod in "${modules[@]}"; do
echo "Searching for references to $mod ..."
rg "$mod"
done
Length of output: 71335
Script:
#!/bin/bash
# Search for actual usage of components/directives from these modules in the template files
rg -t html "mat-tooltip|mat-button|mat-form-field|mat-select|mat-input|mat-tab|mat-list" apps/web-giddh/src/app/multi-currency-reports/
# Search for usage of other components/directives
rg -t html "giddh-page-loader|amount-field|datepicker-wrapper|popover|aside-menu-account|financial-search|form-fields" apps/web-giddh/src/app/multi-currency-reports/
# Search for highlight and recType pipe usage
rg -t html "highlight:|recType:" apps/web-giddh/src/app/multi-currency-reports/
# Search for click outside directive usage
rg -t html "clickOutside" apps/web-giddh/src/app/multi-currency-reports/
# Search for currency component usage
rg -t html "<currency-" apps/web-giddh/src/app/multi-currency-reports/
# Search for account detail modal component usage
rg -t html "account-detail-modal" apps/web-giddh/src/app/multi-currency-reports/
# Search for translate directive usage
rg -t html "[translate]" apps/web-giddh/src/app/multi-currency-reports/
# Search for hamburger menu component usage
rg -t html "hamburger-menu" apps/web-giddh/src/app/multi-currency-reports/
Length of output: 95145
| <ng-template #groupDetails let-list let-padding="padding"> | ||
| <ng-container *ngFor="let grp of list"> | ||
| <section | ||
| *ngIf="grp.isVisible" | ||
| balance-sheet-report-grid-row | ||
| [isExpandToggledDuringSearch]="isExpandToggledDuringSearch" | ||
| [groupDetail]="grp" | ||
| [padding]="padding" | ||
| [search]="search" | ||
| [from]="from" | ||
| [to]="to" | ||
| [expandAll]="expandAll" | ||
| > | ||
| <ng-container | ||
| *ngTemplateOutlet=" | ||
| groupDetails; | ||
| context: { $implicit: grp.childGroups, padding: padding + 20 } | ||
| " | ||
| > | ||
| </ng-container> | ||
| </section> | ||
| </ng-container> | ||
| </ng-template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate template code.
The group details template is duplicated for assets and liabilities sections.
Extract into a shared template:
+<ng-template #sharedGroupDetails let-list let-padding="padding" let-section="section">
+ <ng-container *ngFor="let grp of list">
+ <section
+ *ngIf="grp.isVisible"
+ balance-sheet-report-grid-row
+ [isExpandToggledDuringSearch]="isExpandToggledDuringSearch"
+ [groupDetail]="grp"
+ [padding]="padding"
+ [search]="search"
+ [from]="from"
+ [to]="to"
+ [expandAll]="section === 'liabilities' ? expandAll : undefined"
+ >
+ <ng-container
+ *ngTemplateOutlet="
+ sharedGroupDetails;
+ context: {
+ $implicit: grp.childGroups,
+ padding: padding + 20,
+ section: section
+ }
+ "
+ >
+ </ng-container>
+ </section>
+ </ng-container>
+</ng-template>Then use it for both sections:
<ng-container
*ngTemplateOutlet="
sharedGroupDetails;
context: {
$implicit: bsData?.liabilities | financialSearch: search,
padding: 10,
section: 'liabilities'
}
"
>
</ng-container>Also applies to: 121-142
| export class FilterMultiCurrencyComponent implements OnInit, OnDestroy { | ||
| /** Reference to the modal used for creating tags */ | ||
| @ViewChild('createTagModal', { static: true }) public createTagModal: ModalDirective; | ||
| /** Template reference for the date picker directive */ | ||
| @ViewChild('datepickerTemplate') public datepickerTemplate: TemplateRef<any>; | ||
| /** A boolean indicating whether all elements are expanded */ | ||
| @Input() public expandAll: boolean; | ||
| /** Event emitter for sending the last synchronization date */ | ||
| @Output() public lastSyncDate: EventEmitter<string> = new EventEmitter(); | ||
| /** Event emitter for notifying property changes */ | ||
| @Output() public onPropertyChanged: EventEmitter<any> = new EventEmitter(); | ||
| /** Event emitter for sending the filter value */ | ||
| @Output() public filterValue: EventEmitter<any> = new EventEmitter(); | ||
| /** Event emitter for notifying search changes */ | ||
| @Output() public searchChange: EventEmitter<string> = new EventEmitter(); | ||
| /** Event emitter for toggling the expand/collapse state of all elements */ | ||
| @Output() public expandAllChange: EventEmitter<boolean> = new EventEmitter(); | ||
| /** The current date object representing today's date */ | ||
| public today: Date = new Date(); | ||
| /** The selected date option, initialized to '0' */ | ||
| public selectedDateOption: string = '0'; | ||
| /** The reactive form group for managing filter inputs */ | ||
| public filterForm: FormGroup; | ||
| /** The string used for searching items */ | ||
| public search: string = ''; | ||
| /** The list of financial options available for selection */ | ||
| public financialOptions: IOption[] = []; | ||
| /** The form control for managing account search input */ | ||
| public accountSearchControl: FormControl = new FormControl<string>(''); | ||
| /** The list of tags associated with the component */ | ||
| public tags: TagRequest[] = []; | ||
| /** The currently selected tag */ | ||
| public selectedTag: string; | ||
| /** A boolean indicating the current state of the universal date picker */ | ||
| public universalDateICurrent: boolean = false; | ||
| /** Stores the currently active company information */ | ||
| public activeCompany: any; | ||
| /** Reference to the modal for managing its state */ | ||
| public modalRef: BsModalRef; | ||
| /** The selected date range used in API requests */ | ||
| public selectedDateRange: any; | ||
| /** The selected date range displayed on the user interface */ | ||
| public selectedDateRangeUi: any; | ||
| /** Instance of the dayjs library for date manipulation */ | ||
| public dayjs: any = dayjs; | ||
| /** The selected "from" date in string format */ | ||
| public fromDate: string; | ||
| /** The selected "to" date in string format */ | ||
| public toDate: string; | ||
| /** The label for the selected date range */ | ||
| public selectedRangeLabel: any = ""; | ||
| /** The x and y position of the date field used to position the date picker */ | ||
| public dateFieldPosition: any = { x: 0, y: 0 }; | ||
| /** Stores the local JSON data for the component */ | ||
| public localeData: any = {}; | ||
| /** Stores the common JSON data for the application */ | ||
| public commonLocaleData: any = {}; | ||
| /** List of companies available for selection */ | ||
| public companyList: any; | ||
| /** List of currencies available for selection */ | ||
| public currencyList: any; | ||
| /** This will store available date ranges */ | ||
| public datePickerOption: any = GIDDH_DATE_RANGE_PICKER_RANGES; | ||
| /** ReplaySubject used to handle cleanup and prevent memory leaks */ | ||
| private destroyed$: ReplaySubject<boolean> = new ReplaySubject(1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type annotations to improve type safety
Several properties lack explicit type annotations which could lead to type-related issues:
activeCompany: anycompanyList: anycurrencyList: anyselectedDateRange: any
Consider adding proper type annotations:
- public activeCompany: any;
+ public activeCompany: { baseCurrency: string; [key: string]: any };
- public companyList: any;
+ public companyList: Array<{ name: string; uniqueName: string }>;
- public currencyList: any;
+ public currencyList: Array<IOption>;
- public selectedDateRange: any;
+ public selectedDateRange: { startDate: dayjs.Dayjs; endDate: dayjs.Dayjs };Committable suggestion skipped: line range outside the PR's diff.
| constructor(private formBuilder: FormBuilder, | ||
| private changeDetectionRef: ChangeDetectorRef, | ||
| private generalService: GeneralService, | ||
| private modalService: BsModalService, | ||
| private componentStore: MultiCurrencyReportsComponentStore | ||
| ) { | ||
| this.filterForm = this.formBuilder.group({ | ||
| from: [''], | ||
| to: [''], | ||
| shareCompanyList: [null], | ||
| selectCurrency: [null] | ||
| }); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use typed reactive forms for better type safety
The form initialization uses untyped controls. Consider using typed form controls:
+ interface FilterFormModel {
+ from: string;
+ to: string;
+ shareCompanyList: string[] | null;
+ selectCurrency: string | null;
+ }
- this.filterForm = this.formBuilder.group({
+ this.filterForm = this.formBuilder.group<FilterFormModel>({
from: [''],
to: [''],
shareCompanyList: [null],
selectCurrency: [null]
});Committable suggestion skipped: line range outside the PR's diff.
| * Initializes the component | ||
| * | ||
| * @returns {void} | ||
| * @returns {void}ß |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts (2)
21-86: 🛠️ Refactor suggestionAdd type annotations to improve type safety
Several properties lack explicit type annotations which could lead to type-related issues.
Apply these type annotations:
- public activeCompany: any; + public activeCompany: { baseCurrency: string; [key: string]: any }; - public companyList: any; + public companyList: Array<{ name: string; uniqueName: string }>; - public currencyList: any; + public currencyList: Array<IOption>; - public selectedDateRange: any; + public selectedDateRange: { startDate: dayjs.Dayjs; endDate: dayjs.Dayjs }; - public dateFieldPosition: any; + public dateFieldPosition: { x: number; y: number }; - public localeData: any; + public localeData: Record<string, unknown>; - public commonLocaleData: any; + public commonLocaleData: Record<string, unknown>;
87-100: 🛠️ Refactor suggestionUse typed reactive forms for better type safety
The form initialization uses untyped controls.
+ interface FilterFormModel { + from: string; + to: string; + shareCompanyList: string[] | null; + selectCurrency: string | null; + } - this.filterForm = this.formBuilder.group({ + this.filterForm = this.formBuilder.group<FilterFormModel>({ from: [''], to: [''], shareCompanyList: [null], selectCurrency: [null] });
🧹 Nitpick comments (1)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts (1)
253-257: Document the purpose of setTimeout delayThe use of a magic number (10ms) in setTimeout without explanation makes the code harder to maintain.
public emitExpand(event: boolean): void { + // Add a small delay to ensure the UI has updated before emitting the event setTimeout(() => { this.expandAllChange.emit(event); - }, 10); + }, 10); // 10ms delay for UI update }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html(1 hunks)apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.scss
- apps/web-giddh/src/app/multi-currency-reports/trial-balance/components/trial-balance-grid/trial-balance-report-grid.component.html
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts
[error] 211-211: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 297-297: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
apps/web-giddh/src/app/multi-currency-reports/filter/filter-multi-currency.component.ts (2)
1-20: LGTM! Good use of OnPush change detection
The component setup is well-organized with appropriate imports and change detection strategy.
209-213: 🛠️ Refactor suggestion
Avoid assignment in expressions
The assignment within the expression makes the code harder to understand.
public filterData(): void {
this.onPropertyChanged.emit();
- const a = this.search = '';
- this.searchChange.emit(a);
+ this.search = '';
+ this.searchChange.emit(this.search);
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 211-211: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
https://app.clickup.com/t/86cwm2pf0
Summary by CodeRabbit
I'll help you create concise release notes for this pull request. Based on the comprehensive changes, here's a summary:
New Features
Improvements
Technical Enhancements