WEB-887: Add Polar Area Chart support to report visualization.#3426
Open
Omar-Nabil2 wants to merge 1 commit intoopenMF:devfrom
Open
WEB-887: Add Polar Area Chart support to report visualization.#3426Omar-Nabil2 wants to merge 1 commit intoopenMF:devfrom
Omar-Nabil2 wants to merge 1 commit intoopenMF:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Chart Template & Styles src/app/reports/run-report/chart/chart.component.html, src/app/reports/run-report/chart/chart.component.scss |
Replaced per-toggle click handlers with a mat-button-toggle-group bound to selectedChartType and (change)="selectChart(...)"; added Polar option and ARIA translation; added outer wrapper classes and chart-output-container with fixed height and forced canvas sizing. |
Chart Component Logic src/app/reports/run-report/chart/chart.component.ts |
Component now implements OnInit/OnDestroy, adds debounced window resize handling and teardown, introduces selectedChartType, selectChart()/refreshChartIfSameType(), setPolarAreaChart(), and optional chartColors params for setters; switches color generation to theme-derived rgba(...); makes charts responsive and ensures Chart.js instance cleanup. |
Report Template Padding src/app/reports/run-report/run-report.component.html |
Added p-20 padding class to the collapsed output container. |
Localization (multiple locales) src/assets/translations/... cs-CS.json, de-DE.json, en-US.json, es-CL.json, es-MX.json, fr-FR.json, it-IT.json, ko-KO.json, lt-LT.json, lv-LV.json, ne-NE.json, pt-PT.json, sw-SW.json |
Added two buttons translation keys in each locale: "Polar Area Chart" and "Select Chart Type". |
Sequence Diagram(s)
sequenceDiagram
participant User as User (UI)
participant ChartComp as ChartComponent
participant ChartJS as Chart.js
participant Window as Window
User->>ChartComp: change selection (Bar / Pie / Polar)
ChartComp->>ChartComp: selectChart(type) / compute theme RGBA colors
ChartComp->>ChartJS: create/update chart with data & colors
ChartJS-->>ChartComp: render complete
Note right of Window: on window resize
Window->>ChartComp: resize event (debounced)
ChartComp->>ChartJS: update/resize chart
User->>ChartComp: component destroyed
ChartComp->>ChartJS: destroy chart instance
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
- IOhacker
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and accurately summarizes the main change: adding Polar Area Chart support to report visualization. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Pull request overview
Adds a new Chart.js Polar Area visualization option to the run-report chart UI, alongside existing Pie and Bar charts, and wires in supporting layout + i18n label updates.
Changes:
- Add Polar Area chart rendering and a chart-type selection handler in
ChartComponent. - Update chart UI controls/layout and add a fixed-size chart container style for responsive rendering.
- Add a new
"Polar Area Chart"translation key across multiple locale JSON files.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/reports/run-report/chart/chart.component.ts | Adds Polar Area chart support, chart selection state, and window resize handling |
| src/app/reports/run-report/chart/chart.component.html | Adds Polar Area toggle option and binds selection to component state |
| src/app/reports/run-report/chart/chart.component.scss | Introduces a sized container to support responsive canvas rendering |
| src/app/reports/run-report/run-report.component.html | Minor spacing adjustment for output header container |
| src/assets/translations/en-US.json | Adds "Polar Area Chart" label |
| src/assets/translations/de-DE.json | Adds "Polar Area Chart" label |
| src/assets/translations/fr-FR.json | Adds "Polar Area Chart" label |
| src/assets/translations/it-IT.json | Adds "Polar Area Chart" label |
| src/assets/translations/es-MX.json | Adds "Polar Area Chart" label |
| src/assets/translations/es-CL.json | Adds "Polar Area Chart" label |
| src/assets/translations/pt-PT.json | Adds "Polar Area Chart" label |
| src/assets/translations/cs-CS.json | Adds "Polar Area Chart" label |
| src/assets/translations/lt-LT.json | Adds "Polar Area Chart" label |
| src/assets/translations/lv-LV.json | Adds "Polar Area Chart" label |
| src/assets/translations/ko-KO.json | Adds "Polar Area Chart" label |
| src/assets/translations/ne-NE.json | Adds "Polar Area Chart" label |
| src/assets/translations/sw-SW.json | Adds "Polar Area Chart" label |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+11
to
+21
| <mat-button-toggle-group [value]="selectedChartType" aria-label="Select Chart Type"> | ||
| <mat-button-toggle value="Bar" (click)="selectChart('Bar')">{{ | ||
| 'labels.buttons.Bar Chart' | translate | ||
| }}</mat-button-toggle> | ||
| <mat-button-toggle value="Pie" (click)="selectChart('Pie')">{{ | ||
| 'labels.buttons.Pie Chart' | translate | ||
| }}</mat-button-toggle> | ||
| <mat-button-toggle value="Polar" (click)="selectChart('Polar')">{{ | ||
| 'labels.buttons.Polar Area Chart' | translate | ||
| }}</mat-button-toggle> | ||
| </mat-button-toggle-group> |
There was a problem hiding this comment.
The chart-type toggles are always clickable, but inputData is only set asynchronously. If a user clicks a toggle before getRunReportData() completes, selectChart() will pass undefined into the chart setters and cause runtime errors when reading inputData.keys/values. Disable/hide the toggle group until inputData is available (or guard inside selectChart()/setters).
src/assets/translations/pt-PT.json
Outdated
| "Payments": "Pagamentos", | ||
| "Permissions": "Permissões", | ||
| "Pie Chart": "Gráfico de pizza", | ||
| "Polar Area Chart": "Polar Area Chart", |
There was a problem hiding this comment.
For this non-English locale file, the new key is left in English ("Polar Area Chart": "Polar Area Chart") while nearby entries like "Pie Chart" are translated. The same appears to be true across other locale JSONs updated in this PR; please provide proper translations (or apply the project’s established fallback strategy consistently) so the UI is localized as expected.
Suggested change
| "Polar Area Chart": "Polar Area Chart", | |
| "Polar Area Chart": "Gráfico de área polar", |
Comment on lines
+62
to
+75
| ngOnInit() { | ||
| this.resizeListener = window.addEventListener('resize', () => { | ||
| this.resizeChart(); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Clean up on component destroy. | ||
| */ | ||
| ngOnDestroy() { | ||
| if (this.resizeListener) { | ||
| window.removeEventListener('resize', this.resizeListener); | ||
| } | ||
| if (this.chart) { |
There was a problem hiding this comment.
window.addEventListener() returns void, so assigning its return value to resizeListener means removeEventListener() in ngOnDestroy() will never remove the handler (leaking the listener and continuing to call resizeChart() after destroy). Store the actual callback function (or use an AbortController/RxJS fromEvent with teardown) and consider debouncing/throttling resize events to avoid queuing many setTimeout() calls during continuous resize.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (2)
src/app/reports/run-report/chart/chart.component.scss (2)
12-12: Use grid-aligned tokenized sizing instead of hardcoded500px.
500pxbreaks the 8px grid and hardcodes layout. Prefer a theme/SCSS variable or an 8px-aligned value.As per coding guidelines "Stick to the 8px grid system for visual design and spacing" and "Leverage SCSS variables defined in
src/main.scssandsrc/theme/mifosx-theme.scssrather than generating custom classes and explicit pixel values".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/chart/chart.component.scss` at line 12, Replace the hardcoded height: 500px in chart.component.scss with an 8px-grid-aligned token or SCSS theme variable (do not keep explicit pixel value); update the height declaration in chart.component.scss (the rule that currently sets height: 500px) to use an existing spacing/size variable from src/main.scss or src/theme/mifosx-theme.scss (or add a new variable like $chart-height set to an 8px-multiple such as 504px) so the component uses the theme token instead of a hardcoded value.
15-16: Remove!importantfrom canvas sizing to prevent conflicts with Chart.js responsive rendering.The canvas element is already wrapped in a relatively positioned parent container (500px height) and all charts are configured with
responsive: trueandmaintainAspectRatio: false. The!importantdeclarations on canvas width/height override Chart.js's own sizing logic, which can cause resize inconsistencies and visual issues. Let Chart.js manage canvas dimensions through the parent container and its responsive configuration.Current problematic code
.chart-output-container { position: relative; width: 100%; height: 500px; canvas { width: 100% !important; height: 100% !important; } }Remove the
!importantdeclarations from the canvas styles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/chart/chart.component.scss` around lines 15 - 16, The canvas CSS in .chart-output-container is forcing sizes with width: 100% !important and height: 100% !important which overrides Chart.js responsive sizing; remove the !important declarations from the canvas rules in chart.component.scss so the canvas can be sized by its relatively positioned parent (.chart-output-container) and Chart.js (responsive: true, maintainAspectRatio: false) can manage dimensions properly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/reports/run-report/chart/chart.component.html`:
- Line 11: The ARIA label on the mat-button-toggle-group is hardcoded ("Select
Chart Type") and must be localized; replace the literal aria-label with a
translated key (use the same i18n approach as the visible button labels) by
wiring the translation (e.g., via the translate pipe or TranslateService) for
the mat-button-toggle-group in chart.component.html/component class and add a
new translation key (e.g., REPORTS.CHART.SELECT_TYPE) to your i18n files, then
run npm run translations:extract to update extraction; target the
mat-button-toggle-group and selectedChartType usage so the assistive label is
localized alongside the buttons.
- Around line 11-21: Disable the toggle group until data is ready and drive
selection from the group-level change event: when getRunReportData() finishes
populating inputData set a boolean (e.g., dataReady or rely on inputData !=
null) and bind it to the mat-button-toggle-group [disabled] so toggles cannot be
used before data arrives; remove per-toggle (click) handlers and instead handle
selection via the mat-button-toggle-group (change) event, calling
selectChart($event.value) so keyboard and mouse both work; update the aria-label
to use the translate pipe aria-label="{{ 'labels.buttons.Select Chart Type' |
translate }}" and add the 'labels.buttons.Select Chart Type' entry to
translations (then run translations:extract); also add a defensive check in
selectChart() to no-op if inputData is not yet set.
In `@src/app/reports/run-report/chart/chart.component.ts`:
- Around line 57-74: The resize listener is never removed because ngOnInit
assigns the undefined return of window.addEventListener to resizeListener;
instead, capture and store the actual callback function reference and use that
same reference in ngOnDestroy. Concretely, create a function reference (e.g.,
this.resizeListener = () => this.resizeChart()) in ngOnInit and pass that to
window.addEventListener('resize', this.resizeListener), then call
window.removeEventListener('resize', this.resizeListener) in ngOnDestroy so the
listener is properly unregistered.
In `@src/assets/translations/cs-CS.json`:
- Line 527: The string value for the "Polar Area Chart" label in the cs-CS
translation file is still in English; update the value for the "Polar Area
Chart" key in src/assets/translations/cs-CS.json to a proper Czech translation
(e.g., "Polární plošný graf"), preserving the JSON key, quoting, and trailing
comma format to match adjacent entries.
In `@src/assets/translations/de-DE.json`:
- Line 528: The German translation file contains the key "Polar Area Chart"
whose value is still in English; update the value in
src/assets/translations/de-DE.json so the value is the German translation (e.g.,
"Polardiagramm" or "Polardiagramm (Fläche)") to keep chart option labels
consistently localized; locate the "Polar Area Chart" key and replace the
English string with the chosen German string.
In `@src/assets/translations/es-CL.json`:
- Line 528: The JSON entry with key "Polar Area Chart" has its value still in
English; update the value for the es-CL locale to the proper Spanish translation
(e.g., "Gráfico de Área Polar" or another approved Spanish (Chile) phrasing) so
the key "Polar Area Chart" maps to the localized string in
src/assets/translations/es-CL.json.
In `@src/assets/translations/es-MX.json`:
- Line 528: The JSON translation for the key "Polar Area Chart" in the es-MX
locale is still in English; update the value for the "Polar Area Chart" key in
src/assets/translations/es-MX.json to a Spanish translation such as "Gráfico de
área polar" so the chart toggle appears consistently localized with the other
chart labels.
In `@src/assets/translations/fr-FR.json`:
- Line 528: Translate the untranslated label with key "Polar Area Chart" into
French by replacing the English value with a proper French string (e.g.,
"Diagramme polaire"); update the value for the "Polar Area Chart" key in the
fr-FR translations so chart selection appears fully localized.
In `@src/assets/translations/it-IT.json`:
- Line 528: The Italian locale file contains the key "Polar Area Chart" with an
English value; update the value in src/assets/translations/it-IT.json for the
"Polar Area Chart" key to the correct Italian translation (e.g., "Grafico ad
area polare" or another approved Italian phrase) so the UI label is localized;
ensure you only change the value string for the existing key and preserve JSON
formatting.
In `@src/assets/translations/ko-KO.json`:
- Line 528: The ko-KO localization bundle contains an untranslated entry for the
chart label "Polar Area Chart"; update the value for the "Polar Area Chart" key
in src/assets/translations/ko-KO.json to the appropriate Korean translation
(e.g., "폴라 영역 차트" or "극좌표 영역 차트") so the chart selector displays fully localized
text for Korean users.
In `@src/assets/translations/lt-LT.json`:
- Line 528: The lt-LT translation for the button key "Polar Area Chart" is still
in English; update the value in src/assets/translations/lt-LT.json for the key
"Polar Area Chart" to a Lithuanian translation (e.g., "Poliarinė srities
diagrama") so the button appears localized in the Lithuanian UI.
In `@src/assets/translations/lv-LV.json`:
- Line 528: The Latvian translation for the key "Polar Area Chart" is still in
English; update the value for the "Polar Area Chart" key in the lv-LV JSON to
the proper Latvian translation (e.g., "Polāra laukuma diagramma") so the locale
no longer shows mixed-language UI.
In `@src/assets/translations/ne-NE.json`:
- Line 528: Replace the English value for the JSON key "Polar Area Chart" with
its Nepali translation; locate the "Polar Area Chart" entry and change the value
string to a Nepali label (for example "ध्रुवीय क्षेत्र चार्ट"), keeping the
surrounding JSON syntax (quotes and comma) intact and then run the
localization/lint checks to ensure valid JSON.
In `@src/assets/translations/pt-PT.json`:
- Line 528: The localization entry for the key "Polar Area Chart" is still in
English; update the pt-PT value to the Portuguese translation (e.g., "Gráfico de
Área Polar") so the key "Polar Area Chart" matches the rest of the localized
chart labels in pt-PT.
In `@src/assets/translations/sw-SW.json`:
- Line 528: The translation entry with key "Polar Area Chart" in the sw-SW
locale is still English; update its value to the Swahili localized label (e.g.,
"Chati ya Eneo la Polar" or your project's preferred glossary term) while
keeping the key name "Polar Area Chart" unchanged so the i18n lookup works
correctly.
---
Nitpick comments:
In `@src/app/reports/run-report/chart/chart.component.scss`:
- Line 12: Replace the hardcoded height: 500px in chart.component.scss with an
8px-grid-aligned token or SCSS theme variable (do not keep explicit pixel
value); update the height declaration in chart.component.scss (the rule that
currently sets height: 500px) to use an existing spacing/size variable from
src/main.scss or src/theme/mifosx-theme.scss (or add a new variable like
$chart-height set to an 8px-multiple such as 504px) so the component uses the
theme token instead of a hardcoded value.
- Around line 15-16: The canvas CSS in .chart-output-container is forcing sizes
with width: 100% !important and height: 100% !important which overrides Chart.js
responsive sizing; remove the !important declarations from the canvas rules in
chart.component.scss so the canvas can be sized by its relatively positioned
parent (.chart-output-container) and Chart.js (responsive: true,
maintainAspectRatio: false) can manage dimensions properly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d48e6dd2-565f-4c02-a926-ef5e8996a5b5
📒 Files selected for processing (17)
src/app/reports/run-report/chart/chart.component.htmlsrc/app/reports/run-report/chart/chart.component.scsssrc/app/reports/run-report/chart/chart.component.tssrc/app/reports/run-report/run-report.component.htmlsrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
IOhacker
requested changes
Mar 21, 2026
Omar-Nabil2
force-pushed
the
feature/responsive-chart-sizing
branch
from
32c8c41 to
c4c4d11
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/reports/run-report/chart/chart.component.ts (1)
53-55: Narrow chart types to a closed set.
selectedChartTypeand both handler parameters are plainstring, so a typo from the template can put the component into an invalid state and silently fall through the switch. A localChartType = 'Pie' | 'Bar' | 'Polar'(or enum) makes this branch exhaustive and lets Angular/TypeScript catch mismatches earlier.♻️ Suggested refactor
+type ChartType = 'Bar' | 'Pie' | 'Polar'; + export class ChartComponent implements OnChanges, OnInit, OnDestroy { @@ - selectedChartType: string = 'Pie'; + selectedChartType: ChartType = 'Pie'; @@ - selectChart(chartType: string) { + selectChart(chartType: ChartType) { @@ - refreshChartIfSameType(chartType: string) { + refreshChartIfSameType(chartType: ChartType) {As per coding guidelines,
src/app/**/*.ts: "Use TypeScript for all application code with strict typing conventions."Also applies to: 109-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/chart/chart.component.ts` around lines 53 - 55, Introduce a closed ChartType union or enum (e.g., ChartType = 'Pie' | 'Bar' | 'Polar') and update the selectedChartType declaration to use ChartType instead of string; change any handler parameter types referenced in this file (the chart-type change handler(s) around lines 109-135) to accept ChartType and update the switch/case on selectedChartType to be exhaustive (use a never/default case to surface missing variants). Ensure all template bindings and any assignments to selectedChartType are cast/typed as ChartType so TypeScript can catch mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/reports/run-report/chart/chart.component.ts`:
- Around line 68-73: The component enqueues timers via setTimeout when handling
resize and chart initialization which aren’t tracked or cleared; add properties
(e.g., this.resizeTimeoutId and this.initTimeoutId) to store the IDs returned
from setTimeout in the methods that currently call setTimeout (the
resizeListener callback and the code that calls setPieChart), clear any existing
resize timeout before setting a new one to debounce, and call clearTimeout on
both IDs (and null them) in ngOnDestroy() in addition to removing the resize
listener and destroying this.chart so no queued callbacks run against a
torn-down component or canvas.
---
Nitpick comments:
In `@src/app/reports/run-report/chart/chart.component.ts`:
- Around line 53-55: Introduce a closed ChartType union or enum (e.g., ChartType
= 'Pie' | 'Bar' | 'Polar') and update the selectedChartType declaration to use
ChartType instead of string; change any handler parameter types referenced in
this file (the chart-type change handler(s) around lines 109-135) to accept
ChartType and update the switch/case on selectedChartType to be exhaustive (use
a never/default case to surface missing variants). Ensure all template bindings
and any assignments to selectedChartType are cast/typed as ChartType so
TypeScript can catch mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96ec43a1-44a4-4da5-bfda-98c602d3e0d3
📒 Files selected for processing (17)
src/app/reports/run-report/chart/chart.component.htmlsrc/app/reports/run-report/chart/chart.component.scsssrc/app/reports/run-report/chart/chart.component.tssrc/app/reports/run-report/run-report.component.htmlsrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
✅ Files skipped from review due to trivial changes (10)
- src/app/reports/run-report/chart/chart.component.scss
- src/app/reports/run-report/run-report.component.html
- src/assets/translations/it-IT.json
- src/assets/translations/es-MX.json
- src/assets/translations/ne-NE.json
- src/assets/translations/cs-CS.json
- src/assets/translations/fr-FR.json
- src/assets/translations/pt-PT.json
- src/assets/translations/lt-LT.json
- src/assets/translations/de-DE.json
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/reports/run-report/chart/chart.component.html
- src/assets/translations/lv-LV.json
- src/assets/translations/en-US.json
- src/assets/translations/ko-KO.json
- src/assets/translations/es-CL.json
- src/assets/translations/sw-SW.json
Omar-Nabil2
force-pushed
the
feature/responsive-chart-sizing
branch
from
c4c4d11 to
38152f4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/reports/run-report/chart/chart.component.ts (1)
130-147: Consider narrowing thechartTypeparameter type.The
selectChartmethod acceptsstringbut only handles'Bar','Pie', and'Polar'. Using a union type would provide compile-time safety and make the valid options explicit.♻️ Optional type refinement
+type ChartType = 'Bar' | 'Pie' | 'Polar'; + /** Tracks the currently selected chart type */ -selectedChartType: string = 'Pie'; +selectedChartType: ChartType = 'Pie'; -selectChart(chartType: string) { +selectChart(chartType: ChartType) {This would also apply to
refreshChartIfSameType.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/chart/chart.component.ts` around lines 130 - 147, The selectChart method currently types chartType as string but only accepts 'Bar', 'Pie', and 'Polar'; change the signature of selectChart(chartType: string) to use a narrow union type (e.g., chartType: 'Bar' | 'Pie' | 'Polar' or a shared exported type alias ChartType) and update the matching switch to accept that type; also update refreshChartIfSameType to use the same union/type so callers get compile-time safety and the allowed values are explicit. Ensure all local references and callers of selectChart and refreshChartIfSameType are updated to the new type or cast appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/reports/run-report/chart/chart.component.ts`:
- Around line 130-147: The selectChart method currently types chartType as
string but only accepts 'Bar', 'Pie', and 'Polar'; change the signature of
selectChart(chartType: string) to use a narrow union type (e.g., chartType:
'Bar' | 'Pie' | 'Polar' or a shared exported type alias ChartType) and update
the matching switch to accept that type; also update refreshChartIfSameType to
use the same union/type so callers get compile-time safety and the allowed
values are explicit. Ensure all local references and callers of selectChart and
refreshChartIfSameType are updated to the new type or cast appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 144b33b6-729d-4140-b6c4-d1b964eff1b8
📒 Files selected for processing (17)
src/app/reports/run-report/chart/chart.component.htmlsrc/app/reports/run-report/chart/chart.component.scsssrc/app/reports/run-report/chart/chart.component.tssrc/app/reports/run-report/run-report.component.htmlsrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
✅ Files skipped from review due to trivial changes (14)
- src/app/reports/run-report/chart/chart.component.scss
- src/app/reports/run-report/run-report.component.html
- src/assets/translations/it-IT.json
- src/assets/translations/pt-PT.json
- src/assets/translations/ne-NE.json
- src/assets/translations/ko-KO.json
- src/assets/translations/de-DE.json
- src/assets/translations/fr-FR.json
- src/assets/translations/sw-SW.json
- src/assets/translations/lt-LT.json
- src/assets/translations/cs-CS.json
- src/assets/translations/lv-LV.json
- src/assets/translations/en-US.json
- src/assets/translations/es-MX.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/assets/translations/es-CL.json
Contributor
Author
|
Hi @IOhacker, I’ve reviewed and applied all the AI-suggested changes. Could you please take another look at my PR when you have a moment? |
alberto-art3ch
requested changes
Mar 22, 2026
Collaborator
alberto-art3ch
left a comment
alberto-art3ch
left a comment
There was a problem hiding this comment.
Please see my questions
| .subscribe((response: ChartData) => { | ||
| this.inputData = response; | ||
| this.setPieChart(this.inputData); | ||
| this.selectedChartType = 'Pie'; |
Collaborator
There was a problem hiding this comment.
Is this always a Pie Chart? or is this just the default?
Contributor
Author
There was a problem hiding this comment.
The API response does not provide a chart type field, so the component chooses Pie as the initial default for first render to keep behavior unchanged from the previous implementation. It is not always Pie: users can switch to Bar or Polar using the chart toggle after data loads. If we want backend-driven chart selection later, we can add a chartType attribute in the API contract and use that instead of a UI default.
{
"columnHeaders": [
{
"columnName": "office_name",
"columnType": "VARCHAR",
"columnDisplayType": "STRING",
"isColumnNullable": false,
"isColumnPrimaryKey": false,
"isColumnUnique": false,
"isColumnIndexed": false,
"columnValues": []
},
{
"columnName": "total_loans",
"columnType": "BIGINT",
"columnDisplayType": "INTEGER",
"isColumnNullable": false,
"isColumnPrimaryKey": false,
"isColumnUnique": false,
"isColumnIndexed": false,
"columnValues": []
},
{
"columnName": "total_principal",
"columnType": "DECIMAL",
"columnDisplayType": "DECIMAL",
"isColumnNullable": false,
"isColumnPrimaryKey": false,
"isColumnUnique": false,
"isColumnIndexed": false,
"columnValues": []
},
{
"columnName": "active_loans",
"columnType": "BIGINT",
"columnDisplayType": "INTEGER",
"isColumnNullable": false,
"isColumnPrimaryKey": false,
"isColumnUnique": false,
"isColumnIndexed": false,
"columnValues": []
}
],
"data": [
{
"row": [
"New Branch Name",
2,
28000,
2
]
},
{
"row": [
"West Sub-Branch",
1,
12000,
1
]
},
{
"row": [
"East Sub-Branch",
1,
4000,
1
]
}
]
}| @@ -158,6 +292,6 @@ export class ChartComponent implements OnChanges { | |||
| const r = Math.floor(Math.random() * 255); | |||
Collaborator
There was a problem hiding this comment.
Idealy I would like to suggest the use of theme colors as base
Contributor
Author
There was a problem hiding this comment.
Implemented your suggestion to use theme colors as the base for chart rendering.
I replaced fully random RGB generation with a theme-aware approach:
- colors now start from the active theme’s primary/accent palette
- a small variation is applied so slices/bars stay visually distinct
- dark mode uses dark-friendly fallback tones to preserve contrast
This keeps the charts aligned with the app’s theming system while preserving variety and readability.
Omar-Nabil2
force-pushed
the
feature/responsive-chart-sizing
branch
from
38152f4 to
ce0827b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/reports/run-report/chart/chart.component.ts`:
- Line 115: Remove the debug console.log(response) from ChartComponent (the
console.log(response) call that prints the response) so it does not pollute
production logs; if you need to retain diagnostics, replace it with an
appropriate logger/debug facility (e.g., this.logger.debug or a dev-only flag)
rather than using console.log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3248847-0be6-4c8b-92dc-980a18b7d9b2
📒 Files selected for processing (17)
src/app/reports/run-report/chart/chart.component.htmlsrc/app/reports/run-report/chart/chart.component.scsssrc/app/reports/run-report/chart/chart.component.tssrc/app/reports/run-report/run-report.component.htmlsrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
✅ Files skipped from review due to trivial changes (14)
- src/app/reports/run-report/chart/chart.component.scss
- src/app/reports/run-report/run-report.component.html
- src/assets/translations/ko-KO.json
- src/assets/translations/de-DE.json
- src/assets/translations/es-MX.json
- src/assets/translations/it-IT.json
- src/assets/translations/cs-CS.json
- src/assets/translations/fr-FR.json
- src/assets/translations/sw-SW.json
- src/assets/translations/en-US.json
- src/assets/translations/ne-NE.json
- src/assets/translations/es-CL.json
- src/assets/translations/lt-LT.json
- src/assets/translations/pt-PT.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/reports/run-report/chart/chart.component.html
- src/assets/translations/lv-LV.json
Omar-Nabil2
force-pushed
the
feature/responsive-chart-sizing
branch
from
ce0827b to
7a32c27
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/app/reports/run-report/chart/chart.component.ts (3)
49-52: Consider stricter types forchartanddataObject.Both properties use
any. While the learning suggests treating full API typing as a cross-cutting refactor, consider typingchartasChart | undefinedfromchart.jsand defining an interface fordataObjectin future iterations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/chart/chart.component.ts` around lines 49 - 52, Replace the loose any types with stricter ones: import and use the Chart type from chart.js and declare chart as Chart | undefined (replace the chart: any declaration), and replace the `@Input`() dataObject: any with a typed interface (e.g., export interface ReportData { /* fields */ } and use `@Input`() dataObject: ReportData). Keep the interface minimal for now with the fields the component actually reads and expand later.
132-149: Consider adding a default case or using a union type.The switch statement silently ignores unrecognized chart types. A default case with a warning or exhaustive typing would make debugging easier if new types are added incorrectly.
💡 Optional improvement
+type ChartType = 'Bar' | 'Pie' | 'Polar'; +selectedChartType: ChartType = 'Pie'; selectChart(chartType: string) { // ... switch (chartType) { case 'Bar': this.setBarChart(this.inputData, chartColors); break; case 'Pie': this.setPieChart(this.inputData, chartColors); break; case 'Polar': this.setPolarAreaChart(this.inputData, chartColors); break; + default: + console.warn(`Unknown chart type: ${chartType}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/chart/chart.component.ts` around lines 132 - 149, The switch in selectChart(chartType: string) silently ignores unknown chart types; update selectChart to handle unexpected values (e.g., add a default case that logs a warning via your logger or console.warn and resets selectedChartType or throws an Error) and/or tighten the signature by replacing chartType: string with a union type of allowed values ('Bar'|'Pie'|'Polar') so the compiler enforces exhaustiveness; reference the selectChart function and the selectedChartType property when adding the default handling or changing the parameter type.
333-348: Add input validation tohexToRgb.The function handles 3-char and 6-char hex, but doesn't validate the result. If
hexColoris malformed (e.g.,'#GG'or empty),parseInt(normalized, 16)returnsNaN, and bitwise operations onNaNyield0for all channels. Since inputs come from controlled sources (theme or hardcoded fallbacks), this is low risk, but defensive validation would prevent silent failures if the theme data is ever corrupted.🛡️ Optional defensive fix
private hexToRgb(hexColor: string): number[] { const hex = hexColor.replace('#', ''); + if (!/^[0-9A-Fa-f]{3}$|^[0-9A-Fa-f]{6}$/.test(hex)) { + return [128, 128, 128]; // Fallback gray for invalid input + } const normalized = hex.length === 3 ? hex .split('') .map((char) => char + char) .join('') : hex; const numeric = parseInt(normalized, 16); return [ (numeric >> 16) & 255, (numeric >> 8) & 255, numeric & 255 ]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reports/run-report/chart/chart.component.ts` around lines 333 - 348, The hexToRgb function currently assumes hexColor is valid and returns incorrect silent zeros when parseInt yields NaN; update hexToRgb to validate the input before bitwise conversion: ensure hexColor (after stripping '#') matches a 3- or 6-hex-digit regex (e.g., /^[0-9a-fA-F]{3}$|^[0-9a-fA-F]{6}$/), normalize 3-char to 6-char as already done, then call parseInt and verify the result is a valid number (not NaN) and that normalized.length === 6; if validation fails, either throw a descriptive Error or return a safe fallback RGB (e.g., [0,0,0]) and log the invalid input; reference the hexToRgb function to locate and modify the logic accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/reports/run-report/chart/chart.component.ts`:
- Around line 49-52: Replace the loose any types with stricter ones: import and
use the Chart type from chart.js and declare chart as Chart | undefined (replace
the chart: any declaration), and replace the `@Input`() dataObject: any with a
typed interface (e.g., export interface ReportData { /* fields */ } and use
`@Input`() dataObject: ReportData). Keep the interface minimal for now with the
fields the component actually reads and expand later.
- Around line 132-149: The switch in selectChart(chartType: string) silently
ignores unknown chart types; update selectChart to handle unexpected values
(e.g., add a default case that logs a warning via your logger or console.warn
and resets selectedChartType or throws an Error) and/or tighten the signature by
replacing chartType: string with a union type of allowed values
('Bar'|'Pie'|'Polar') so the compiler enforces exhaustiveness; reference the
selectChart function and the selectedChartType property when adding the default
handling or changing the parameter type.
- Around line 333-348: The hexToRgb function currently assumes hexColor is valid
and returns incorrect silent zeros when parseInt yields NaN; update hexToRgb to
validate the input before bitwise conversion: ensure hexColor (after stripping
'#') matches a 3- or 6-hex-digit regex (e.g.,
/^[0-9a-fA-F]{3}$|^[0-9a-fA-F]{6}$/), normalize 3-char to 6-char as already
done, then call parseInt and verify the result is a valid number (not NaN) and
that normalized.length === 6; if validation fails, either throw a descriptive
Error or return a safe fallback RGB (e.g., [0,0,0]) and log the invalid input;
reference the hexToRgb function to locate and modify the logic accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 254ed9f2-c336-44a5-b715-eefc107206be
📒 Files selected for processing (17)
src/app/reports/run-report/chart/chart.component.htmlsrc/app/reports/run-report/chart/chart.component.scsssrc/app/reports/run-report/chart/chart.component.tssrc/app/reports/run-report/run-report.component.htmlsrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
✅ Files skipped from review due to trivial changes (12)
- src/app/reports/run-report/run-report.component.html
- src/assets/translations/pt-PT.json
- src/assets/translations/ko-KO.json
- src/assets/translations/cs-CS.json
- src/app/reports/run-report/chart/chart.component.scss
- src/assets/translations/es-CL.json
- src/assets/translations/es-MX.json
- src/assets/translations/fr-FR.json
- src/assets/translations/it-IT.json
- src/assets/translations/lv-LV.json
- src/assets/translations/de-DE.json
- src/assets/translations/sw-SW.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/assets/translations/en-US.json
- src/assets/translations/ne-NE.json
- src/assets/translations/lt-LT.json
Contributor
Author
|
@alberto-art3ch I have applied your comments, Could you please check again ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds Polar Area Chart support to the report visualization component. Users can now select between Pie, Bar, and Polar Area charts for improved data visualization flexibility.
Changes
setPolarAreaChart()method with responsive Chart.js options; add window resize handling for all chart typesWhy?
Expanding chart type options provides users with more flexible ways to visualize report data. Polar Area charts are particularly useful for displaying multi-dimensional data and categorical distributions.
How to Test
Related Jira
WEB-887: Add Polar Area Chart support to report visualization
Related issues and discussion
#{Issue Number}
Screenshots, if any
before
before.mp4
after
screen_1774111698982.mp4
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Documentation