Fix: Aligned Report Details Layout and Added BIRT Translation (WEB-821)#86
Open
vaishnavirrapolu-06 wants to merge 9 commits intoopenMF:devfrom
Open
Fix: Aligned Report Details Layout and Added BIRT Translation (WEB-821)#86vaishnavirrapolu-06 wants to merge 9 commits intoopenMF:devfrom
vaishnavirrapolu-06 wants to merge 9 commits intoopenMF:devfrom
Conversation
📝 WalkthroughWalkthroughThis PR updates package dependencies (particularly i18n-related packages), adds localization infrastructure for UI labels in products.json, and applies widespread JSX formatting and restructuring across multiple page components while maintaining existing functionality. The system/reports section includes i18n integration and UI styling updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pages/groups/groups-view/group-actions/TransferClients.tsx (1)
58-76:⚠️ Potential issue | 🟡 MinorAdd language dependency to useEffect to refresh fallback labels on language change.
Line 68 uses the
tfunction to create fallback labels, but the dependency array at line 76 only includes[id]. When the user changes language, the translated fallback labels won't update until the component reloads oridchanges.Suggested fix
- const { t } = useTranslation('groups') + const { t, i18n } = useTranslation('groups') ... - }, [id]) + }, [id, i18n.language])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/group-actions/TransferClients.tsx` around lines 58 - 76, The useEffect that builds fallback labels with t (in the async call using (groupsApi as any).retrieveAll17?.()) currently only depends on id, so translated fallback names won't update on language changes; update the dependency array of that useEffect to include the translation change (e.g., add t or i18n.language) so the effect re-runs and calls setDestOptions with new t values when language changes.src/pages/groups/groups-view/GroupsView.tsx (1)
86-197:⚠️ Potential issue | 🟡 MinorInclude translation function in dependency array to prevent stale menu labels after language change.
menuOptionscallst()multiple times but doesn't includetori18n.languagein its dependency array. When a user switches language, thetfunction reference updates, but the memoized options won't recompute, causing menu labels to remain in the previous language.Suggested fix
- }, [group, isActive, hasMembers, hasCalendar, hasStaff, inCenter]) + }, [ + group, + isActive, + hasMembers, + hasCalendar, + hasStaff, + inCenter, + t, + i18n.language, + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/GroupsView.tsx` around lines 86 - 197, menuOptions is memoized with useMemo but calls t() for translations and doesn't include t or i18n.language in its dependency array, causing labels to stay stale after language change; update the dependency array for the useMemo that defines menuOptions to include the translation function/reference (t) or the current language (i18n.language) so the menu recomputes on locale changes — locate the useMemo that returns menuOptions in GroupsView.tsx and add t (or i18n.language) to its dependency list along with the existing [group, isActive, hasMembers, hasCalendar, hasStaff, inCenter].src/pages/groups/groups-view/general-tab/GroupsGeneralTab.tsx (1)
385-389:⚠️ Potential issue | 🟡 MinorRedundant ternary — both branches return the same value.
The ternary expression returns
acc.accountNoin both branches, making the condition pointless. The column header switches between "Closed Date" and "Last Active" based onshowClosedSavingAccounts, so this cell should display different data for each state: the closed date for closed accounts, or the last active transaction date for active accounts.Use the same pattern as ClientsGeneralTab for savings accounts:
acc?.timeline?.closedOnDatefor closed accountsacc?.lastActiveTransactionDatefor active accounts (with fallback toacc?.timeline?.activatedOnDate)Import
formatDatefrom@/lib/date-utilsand apply it to handle array-type date fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/general-tab/GroupsGeneralTab.tsx` around lines 385 - 389, The TableCell currently uses a redundant ternary over showClosedSavingAccounts and always prints acc.accountNo; replace it so when showClosedSavingAccounts is true it displays the closed date (use acc?.timeline?.closedOnDate), otherwise display the last active transaction date with fallback to activation (use acc?.lastActiveTransactionDate ?? acc?.timeline?.activatedOnDate), and pass the value through formatDate from '@/lib/date-utils' to handle array/date formatting; update the import and the TableCell rendering in GroupsGeneralTab where acc is referenced.
🧹 Nitpick comments (1)
src/pages/system/manage-reports/ViewReports.tsx (1)
1-1: Avoid disabling ESLint for the entire file.A file-scoped
/* eslint-disable */hides unrelated regressions in this component. If one rule is noisy, suppress that rule as locally as possible instead of turning checks off across the whole view.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/system/manage-reports/ViewReports.tsx` at line 1, The file-wide "/* eslint-disable */" in ViewReports.tsx suppresses all linting and should be removed; instead, remove that top-level directive and, if a specific rule is noisy, add targeted inline disables (e.g., /* eslint-disable-next-line rule-name */) directly above the offending statement(s) or wrap only the minimal block that needs suppression (for example around JSX in the ViewReports component or a specific helper function), keeping the rest of the file linted normally.
🤖 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/locales/en-US/products.json`:
- Around line 18-43: Add the missing 14 "label" keys (all nested keys under
label: anchor.home, anchor.system, anchor.managereports, button.edit,
button.back, heading.report, heading.reporttype, heading.reportcategory,
heading.corereport, heading.userreport, boolean.yes, boolean.no, text.BIRT,
text.Pentaho) from en-US/products.json into the corresponding locales (es-ES,
fr-FR, it-IT) so those files contain identical key structure and translated or
placeholder values; ensure the JSON nesting and key names exactly match the
en-US entries to prevent fallback to English.
In
`@src/pages/clients/clients-view/family-members-tab/ClientsFamilyMembersAddTab.tsx`:
- Around line 74-77: The Label components in ClientsFamilyMembersAddTab.tsx
(e.g., the <Label> wrapping the first name, last name, relationship, DOB, etc.)
are only visual siblings and not bound to their inputs; update each Label to
associate with its corresponding control by either adding a matching htmlFor on
the Label and an id on the input/select, or by wrapping the input inside the
Label, and also set the control's required/aria-required attribute (or required
prop for custom input components) so the required state is exposed
programmatically for screen readers; apply this change for the instances around
the firstName, lastName, relationship, birthDate, and other listed fields.
In `@src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx`:
- Around line 163-167: The Label next to the search Input in
ManageGroupMembers.tsx isn’t associated with the input element, so add a
matching id on the Input and htmlFor on the Label (e.g., id="clientSearch" and
htmlFor="clientSearch") to wire them together; update the JSX for the Label
component and the Input component used in that file (ensure the Input accepts an
id prop and Label accepts htmlFor if they are custom components) so the visible
client label is programmatically linked to the autocomplete/search field.
In `@src/pages/system/manage-reports/ViewReports.tsx`:
- Around line 101-109: The badge for report flags currently treats undefined the
same as false, causing "No" to display before data loads; update the rendering
for report?.coreReport (and likewise for report?.useReport) to explicitly check
for true/false and render a neutral placeholder (e.g., "-" or "Unknown") when
the value is undefined (for example use strict equality checks like
report?.coreReport === true / === false or test typeof report?.coreReport ===
"boolean" before choosing Yes/No), and apply the same change to the other badge
rendering block that references report?.useReport so undefined is not shown as
No.
---
Outside diff comments:
In `@src/pages/groups/groups-view/general-tab/GroupsGeneralTab.tsx`:
- Around line 385-389: The TableCell currently uses a redundant ternary over
showClosedSavingAccounts and always prints acc.accountNo; replace it so when
showClosedSavingAccounts is true it displays the closed date (use
acc?.timeline?.closedOnDate), otherwise display the last active transaction date
with fallback to activation (use acc?.lastActiveTransactionDate ??
acc?.timeline?.activatedOnDate), and pass the value through formatDate from
'@/lib/date-utils' to handle array/date formatting; update the import and the
TableCell rendering in GroupsGeneralTab where acc is referenced.
In `@src/pages/groups/groups-view/group-actions/TransferClients.tsx`:
- Around line 58-76: The useEffect that builds fallback labels with t (in the
async call using (groupsApi as any).retrieveAll17?.()) currently only depends on
id, so translated fallback names won't update on language changes; update the
dependency array of that useEffect to include the translation change (e.g., add
t or i18n.language) so the effect re-runs and calls setDestOptions with new t
values when language changes.
In `@src/pages/groups/groups-view/GroupsView.tsx`:
- Around line 86-197: menuOptions is memoized with useMemo but calls t() for
translations and doesn't include t or i18n.language in its dependency array,
causing labels to stay stale after language change; update the dependency array
for the useMemo that defines menuOptions to include the translation
function/reference (t) or the current language (i18n.language) so the menu
recomputes on locale changes — locate the useMemo that returns menuOptions in
GroupsView.tsx and add t (or i18n.language) to its dependency list along with
the existing [group, isActive, hasMembers, hasCalendar, hasStaff, inCenter].
---
Nitpick comments:
In `@src/pages/system/manage-reports/ViewReports.tsx`:
- Line 1: The file-wide "/* eslint-disable */" in ViewReports.tsx suppresses all
linting and should be removed; instead, remove that top-level directive and, if
a specific rule is noisy, add targeted inline disables (e.g., /*
eslint-disable-next-line rule-name */) directly above the offending statement(s)
or wrap only the minimal block that needs suppression (for example around JSX in
the ViewReports component or a specific helper function), keeping the rest of
the file linted normally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 312d14ca-9a41-4aa7-96cd-a5fd0a493afa
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
package.jsonsrc/locales/en-US/products.jsonsrc/pages/centers/centers-view/general-tab/CentersGeneralTab.tsxsrc/pages/clients/Clients.tsxsrc/pages/clients/clients-view/ClientsView.tsxsrc/pages/clients/clients-view/Identities-tab/ClientsidentitiesTab.tsxsrc/pages/clients/clients-view/address-tab/ClientsAddressTab.tsxsrc/pages/clients/clients-view/charges/view-charge/ViewCharge.tsxsrc/pages/clients/clients-view/family-members-tab/ClientsFamilyMembersAddTab.tsxsrc/pages/clients/clients-view/family-members-tab/ClientsFamilyMembersTab.tsxsrc/pages/clients/clients-view/general-tab/ClientsGeneralTab.tsxsrc/pages/groups/groups-view/GroupsView.tsxsrc/pages/groups/groups-view/general-tab/GroupsGeneralTab.tsxsrc/pages/groups/groups-view/group-actions/AttachMeeting.tsxsrc/pages/groups/groups-view/group-actions/EditGroups.tsxsrc/pages/groups/groups-view/group-actions/ManageGroupMembers.tsxsrc/pages/groups/groups-view/group-actions/TransferClients.tsxsrc/pages/system/manage-reports/ViewReports.tsx
Comment on lines
+18
to
+43
| "label": { | ||
| "anchor": { | ||
| "home": "Home", | ||
| "system": "System", | ||
| "managereports": "Manage Reports" | ||
| }, | ||
| "button": { | ||
| "edit": "Edit", | ||
| "back": "Back" | ||
| }, | ||
| "heading": { | ||
| "report": "Report", | ||
| "reporttype": "Report Type", | ||
| "reportcategory": "Report Category", | ||
| "corereport": "Core Report", | ||
| "userreport": "User Report" | ||
| }, | ||
| "boolean": { | ||
| "yes": "Yes", | ||
| "no": "No" | ||
| }, | ||
| "text": { | ||
| "BIRT": "Mifos Reporting Plugin for Eclipse Birt", | ||
| "Pentaho": "Mifos Reporting Plugin for Pentaho" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import json
from pathlib import Path
required = [
('label', 'anchor', 'home'),
('label', 'anchor', 'system'),
('label', 'anchor', 'managereports'),
('label', 'button', 'edit'),
('label', 'button', 'back'),
('label', 'heading', 'report'),
('label', 'heading', 'reporttype'),
('label', 'heading', 'reportcategory'),
('label', 'heading', 'corereport'),
('label', 'heading', 'userreport'),
('label', 'boolean', 'yes'),
('label', 'boolean', 'no'),
('label', 'text', 'BIRT'),
('label', 'text', 'Pentaho'),
]
for path in sorted(Path('src/locales').glob('*/products.json')):
data = json.loads(path.read_text())
missing = []
for keys in required:
current = data
for key in keys:
if isinstance(current, dict) and key in current:
current = current[key]
else:
missing.append('.'.join(keys))
break
status = 'OK' if not missing else 'MISSING: ' + ', '.join(missing)
print(f'{path}: {status}')
PYRepository: openMF/mifos-x-web-app-react
Length of output: 1160
Add the new label keys to all shipped locale files.
The new report-detail keys are present only in en-US/products.json but missing from es-ES, fr-FR, and it-IT. The report detail view will silently fall back to English for users of these locales. Add all 14 new label.* keys to each of these three files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/locales/en-US/products.json` around lines 18 - 43, Add the missing 14
"label" keys (all nested keys under label: anchor.home, anchor.system,
anchor.managereports, button.edit, button.back, heading.report,
heading.reporttype, heading.reportcategory, heading.corereport,
heading.userreport, boolean.yes, boolean.no, text.BIRT, text.Pentaho) from
en-US/products.json into the corresponding locales (es-ES, fr-FR, it-IT) so
those files contain identical key structure and translated or placeholder
values; ensure the JSON nesting and key names exactly match the en-US entries to
prevent fallback to English.
Comment on lines
74
to
77
| <Label> | ||
| {t('familyMembers.labelFirstName')}<span className="text-red-500">*</span> | ||
| {t('familyMembers.labelFirstName')} | ||
| <span className="text-red-500">*</span> | ||
| </Label> |
There was a problem hiding this comment.
These required fields are still visually marked only.
Each touched <Label> here is still just a sibling of its control, so the inputs/selects do not get a programmatic label, and the red * is the only required cue. Please bind the labels to the underlying controls and expose the required state on the controls themselves.
♿ Suggested pattern
- <Label>
+ <Label htmlFor="first-name">
{t('familyMembers.labelFirstName')}
- <span className="text-red-500">*</span>
+ <span aria-hidden="true" className="text-red-500">*</span>
</Label>
<Input
+ id="first-name"
+ required
value={form.firstName}
onChange={e => set('firstName', e.target.value)}
/>
- <Label>
+ <Label htmlFor="relationship">
{t('familyMembers.labelRelationship')}
- <span className="text-red-500">*</span>
+ <span aria-hidden="true" className="text-red-500">*</span>
</Label>
<Select
value={form.relationship}
onValueChange={v => set('relationship', v)}
>
- <SelectTrigger className="rounded-none border-0 border-b border-zinc-300">
+ <SelectTrigger
+ id="relationship"
+ aria-required="true"
+ className="rounded-none border-0 border-b border-zinc-300"
+ >Also applies to: 93-96, 114-117, 136-139, 167-170, 246-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/clients/clients-view/family-members-tab/ClientsFamilyMembersAddTab.tsx`
around lines 74 - 77, The Label components in ClientsFamilyMembersAddTab.tsx
(e.g., the <Label> wrapping the first name, last name, relationship, DOB, etc.)
are only visual siblings and not bound to their inputs; update each Label to
associate with its corresponding control by either adding a matching htmlFor on
the Label and an id on the input/select, or by wrapping the input inside the
Label, and also set the control's required/aria-required attribute (or required
prop for custom input components) so the required state is exposed
programmatically for screen readers; apply this change for the instances around
the firstName, lastName, relationship, birthDate, and other listed fields.
Comment on lines
+163
to
167
| <Label className="mb-1 block"> | ||
| {t('manageMembers.labelClient')} | ||
| </Label> | ||
| <Input | ||
| placeholder={t('manageMembers.placeholderSearch')} |
There was a problem hiding this comment.
Wire this label to the autocomplete input.
The visible client label is still separate from the <Input>, so the search box has no programmatic name; the placeholder does not replace it. Add htmlFor/id here before shipping.
♿ Suggested fix
- <Label className="mb-1 block">
+ <Label htmlFor="client-search" className="mb-1 block">
{t('manageMembers.labelClient')}
</Label>
<Input
+ id="client-search"
placeholder={t('manageMembers.placeholderSearch')}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Label className="mb-1 block"> | |
| {t('manageMembers.labelClient')} | |
| </Label> | |
| <Input | |
| placeholder={t('manageMembers.placeholderSearch')} | |
| <Label htmlFor="client-search" className="mb-1 block"> | |
| {t('manageMembers.labelClient')} | |
| </Label> | |
| <Input | |
| id="client-search" | |
| placeholder={t('manageMembers.placeholderSearch')} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx` around
lines 163 - 167, The Label next to the search Input in ManageGroupMembers.tsx
isn’t associated with the input element, so add a matching id on the Input and
htmlFor on the Label (e.g., id="clientSearch" and htmlFor="clientSearch") to
wire them together; update the JSX for the Label component and the Input
component used in that file (ensure the Input accepts an id prop and Label
accepts htmlFor if they are custom components) so the visible client label is
programmatically linked to the autocomplete/search field.
Comment on lines
+101
to
+109
| <div | ||
| className={`px-2 py-0.5 rounded text-xs font-bold ${ | ||
| report?.coreReport | ||
| ? "bg-green-100 text-green-700" | ||
| : "bg-gray-100 text-gray-700" | ||
| }`} | ||
| > | ||
| {report?.coreReport ? t("label.boolean.yes") : t("label.boolean.no")} | ||
| </div> |
There was a problem hiding this comment.
Don't render unknown report flags as No.
Both badges currently treat undefined the same as false, so the page shows negative values before the fetch resolves and after a failed fetch. Render a neutral placeholder until those booleans are actually known.
💡 Suggested fix
- report?.coreReport
- ? "bg-green-100 text-green-700"
- : "bg-gray-100 text-gray-700"
+ report?.coreReport == null
+ ? "bg-zinc-100 text-zinc-500"
+ : report.coreReport
+ ? "bg-green-100 text-green-700"
+ : "bg-gray-100 text-gray-700"
...
- {report?.coreReport ? t("label.boolean.yes") : t("label.boolean.no")}
+ {report?.coreReport == null
+ ? "—"
+ : report.coreReport
+ ? t("label.boolean.yes")
+ : t("label.boolean.no")}Apply the same branch to report?.useReport.
Also applies to: 116-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/system/manage-reports/ViewReports.tsx` around lines 101 - 109, The
badge for report flags currently treats undefined the same as false, causing
"No" to display before data loads; update the rendering for report?.coreReport
(and likewise for report?.useReport) to explicitly check for true/false and
render a neutral placeholder (e.g., "-" or "Unknown") when the value is
undefined (for example use strict equality checks like report?.coreReport ===
true / === false or test typeof report?.coreReport === "boolean" before choosing
Yes/No), and apply the same change to the other badge rendering block that
references report?.useReport so undefined is not shown as No.
Contributor
|
@vaishnavirrapolu-06 the expected repository is https://github.com/openMF/web-app |
Contributor
|
this PR has missing translation files |
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
I have completed the UI and localization fixes for the Manage Reports detail view.
Changes Made:
ViewReports.tsxto use a consistent label width (w-48), ensuring all report details are perfectly aligned in a grid-like structure.en.jsonto display the full descriptive name: "Mifos Reporting Plugin for Eclipse Birt".GetReportsResponse.Verification:
I verified these changes using mock data to confirm the layout and translation logic work as intended (see attached screenshot). Note: Local environment routing currently defaults to an empty view, but the component code is fully tested.
Summary by CodeRabbit
Release Notes
New Features
UI Improvements
Chores