MXWAR-66: Force add API services and dynamic report components#76
Open
DhroovSankla wants to merge 1 commit intoopenMF:devfrom
Open
MXWAR-66: Force add API services and dynamic report components#76DhroovSankla wants to merge 1 commit intoopenMF:devfrom
DhroovSankla 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:
📝 WalkthroughWalkthroughRemoved Vite and local env configuration files, added an Axios API client and a reports API wrapper, introduced a RunReports React page for executing reports, and updated routing to mount RunReports at /reports/run/:reportName. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Run as RunReports<br/>Component
participant ReportsAPI as reportsApi<br/>Service
participant Axios as AxiosClient
participant Fineract as Fineract API
User->>Run: Navigate to /reports/run/:reportName
activate Run
Run->>ReportsAPI: retrieveReportByName(reportName)
Run->>ReportsAPI: retrieveOffices()
activate ReportsAPI
ReportsAPI->>Axios: GET /reports/{reportName}
ReportsAPI->>Axios: GET /offices
activate Axios
Axios->>Axios: inject Authorization header (from sessionStorage)
Axios->>Fineract: send requests
Fineract-->>Axios: return metadata & offices
deactivate Axios
deactivate ReportsAPI
Run->>Run: render dynamic form with parameters & offices
deactivate Run
User->>Run: submit form (parameters)
activate Run
Run->>ReportsAPI: runReport(reportName, params)
activate ReportsAPI
ReportsAPI->>Axios: GET /runreports/{reportName}?...params&response=true
activate Axios
Axios->>Axios: inject Authorization header
Axios->>Fineract: execute report
Fineract-->>Axios: report results
deactivate Axios
deactivate ReportsAPI
Run->>User: display results or error
deactivate Run
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![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
🧹 Nitpick comments (2)
src/pages/reports/RunReports.tsx (2)
113-114: Useparam.parameterNameas key instead of index.Using array index as a React key can cause issues with component state if the list order changes.
param.parameterNameshould be unique and stable.- reportData.reportParameters.map((param: any, index: number) => ( - <div key={index} className="space-y-2"> + reportData.reportParameters.map((param: any) => ( + <div key={param.parameterName} className="space-y-2">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/reports/RunReports.tsx` around lines 113 - 114, Replace the unstable array index key in the report parameter rendering with a stable unique identifier: in the reportData.reportParameters.map(...) callback (the JSX that renders each <div key={index} ...>), use param.parameterName as the key instead of index so the key becomes stable across reorders and preserves component state.
51-90: Input elements lack proper label association for accessibility.The
<label>elements are not associated with their inputs viahtmlFor/idattributes. Screen readers won't properly announce the labels for these form controls.♿ Accessibility fix example
- <div key={index} className="space-y-2"> - <label className="text-sm font-semibold text-zinc-700 capitalize"> + <div key={param.parameterName} className="space-y-2"> + <label + htmlFor={param.parameterName} + className="text-sm font-semibold text-zinc-700 capitalize" + > {param.parameterName.replace('parameter', '')} </label> {renderParameterInput(param)}Then add
id={paramKey}to each input/select inrenderParameterInput.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/reports/RunReports.tsx` around lines 51 - 90, renderParameterInput currently builds inputs/selects but does not associate labels with inputs; add id={paramKey} to each input and select returned in renderParameterInput and ensure any corresponding <label> uses htmlFor={paramKey} (use the existing label variable derived from paramKey) so screen readers can correctly associate labels with the controls (update the input/select elements in the cases 'startDate', 'endDate', 'officeId', and default).
🤖 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/pages/reports/RunReports.tsx`:
- Around line 45-48: handleRunReport currently only logs formValues and never
invokes the API; replace the console.log with a call to
reportsApi.runReport(reportName, formValues) (or await reportsApi.runReport(...)
if handleRunReport is converted to async) and handle the promise result and
errors (e.g., show success/error UI or set loading state). Locate the
handleRunReport function and update it to call reportsApi.runReport using the
existing reportName and formValues identifiers, add try/catch (or .then/.catch)
to surface failures, and update any relevant state (loading, error, success)
after the call.
- Around line 22-28: Replace the client-side filtering of a full report list
with the dedicated API call: call reportsApi.retrieveReportByName(reportName)
instead of reportsApi.retrieveReportList(), keep the concurrent
retrieveOffices() call in the Promise.all, and use the returned value to
setReportData (replace reportRes.data.find(...) with the single report
response). Update the Promise.all array and the variables (reportRes ->
reportByNameRes or similar) so setReportData receives the retrieved report
directly.
- Around line 30-34: The catch block only logs errors to console and leaves the
UI blank; add a user-facing error state by introducing a new state variable
(e.g., const [error, setError] = useState<string | null>(null)), update the
catch in the fetch/report loader (where setLoading is used) to call setError
with a user-friendly message (and optionally the error.message), and clear it on
successful fetch; then update the component render to check error and display a
visible error UI (a message/banner with retry action) before rendering the form
so users see what went wrong and can retry.
- Around line 8-9: The RunReports component is currently unreachable because the
router renders the Reports component for the path /reports/run/:reportName;
either connect RunReports to the router or remove it. To fix, either (A) import
RunReports where routes are defined and replace the route that uses Reports for
path "/reports/run/:reportName" so it renders RunReports (ensure the import and
usage of useParams in RunReports remains correct), or (B) if RunReports is
obsolete, delete the RunReports component and remove any unused imports/exports
referencing it. Make sure to update tests/exports and run the app to confirm the
route now displays the intended component.
---
Nitpick comments:
In `@src/pages/reports/RunReports.tsx`:
- Around line 113-114: Replace the unstable array index key in the report
parameter rendering with a stable unique identifier: in the
reportData.reportParameters.map(...) callback (the JSX that renders each <div
key={index} ...>), use param.parameterName as the key instead of index so the
key becomes stable across reorders and preserves component state.
- Around line 51-90: renderParameterInput currently builds inputs/selects but
does not associate labels with inputs; add id={paramKey} to each input and
select returned in renderParameterInput and ensure any corresponding <label>
uses htmlFor={paramKey} (use the existing label variable derived from paramKey)
so screen readers can correctly associate labels with the controls (update the
input/select elements in the cases 'startDate', 'endDate', 'officeId', and
default).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d6eb880-3a78-46f8-a02a-42a6d4d6b595
📒 Files selected for processing (6)
.env.development.env.samplesrc/fineract-api/api.tssrc/fineract-api/reportsApi.tssrc/pages/reports/RunReports.tsxvite.config.ts
💤 Files with no reviewable changes (3)
- .env.development
- vite.config.ts
- .env.sample
gkbishnoi07
requested changes
Mar 7, 2026
Collaborator
gkbishnoi07
left a comment
gkbishnoi07
left a comment
There was a problem hiding this comment.
the Diff is too big to render. To view, src/fineract-api/api.ts
DhroovSankla
force-pushed
the
MXWAR-66-run-reports
branch
from
0cdb270 to
2d9e5cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/reportsApi.ts (1)
3-14: Keep a single canonicalreportsApiimplementation.This now duplicates
src/fineract-api/reportsApi.ts. Once query handling, auth wiring, or response typing changes in one copy, callers will get different behavior depending on which module they import. Please consolidate to one implementation and re-export it from the other path if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/reportsApi.ts` around lines 3 - 14, There are two duplicated implementations of the reportsApi object (symbols: reportsApi, retrieveReportList, retrieveReportByName, retrieveOffices, runReport); remove this duplicate implementation and instead re-export the single canonical reportsApi implementation from the other module so all imports resolve to the same object, preserving the exact exported API shape and typings (retrieveReportList, retrieveReportByName, retrieveOffices, runReport) and ensuring any auth/query wiring remains unchanged while updating callers to import from this module if necessary.
🤖 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/services/api.ts`:
- Around line 3-17: The API client currently hardcodes tenant and reads the
wrong storage key; update the axios instance/headers created in api
(axios.create) and the request interceptor (api.interceptors.request.use) to
read the auth token from localStorage.getItem('mifosToken') (not
sessionStorage['mifos_token']) and set Authorization accordingly, and replace
the hardcoded 'default' Fineract-Platform-TenantId header with the app’s tenant
value (mifosTenant) obtained from the same source/config used by the login flow
so requests use the correct tenant and auth.
---
Nitpick comments:
In `@src/services/reportsApi.ts`:
- Around line 3-14: There are two duplicated implementations of the reportsApi
object (symbols: reportsApi, retrieveReportList, retrieveReportByName,
retrieveOffices, runReport); remove this duplicate implementation and instead
re-export the single canonical reportsApi implementation from the other module
so all imports resolve to the same object, preserving the exact exported API
shape and typings (retrieveReportList, retrieveReportByName, retrieveOffices,
runReport) and ensuring any auth/query wiring remains unchanged while updating
callers to import from this module if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a966c476-f908-4238-9694-500184c8836c
📒 Files selected for processing (2)
src/services/api.tssrc/services/reportsApi.ts
src/services/api.ts
Outdated
Comment on lines
+3
to
+17
| const api = axios.create({ | ||
| baseURL: import.meta.env.VITE_FINERACT_API_URL || '/fineract-provider/api/v1', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Fineract-Platform-TenantId': 'default', | ||
| }, | ||
| }); | ||
|
|
||
| api.interceptors.request.use((config) => { | ||
| const token = sessionStorage.getItem('mifos_token'); | ||
| if (token && config.headers) { | ||
| config.headers.Authorization = `Basic ${token}`; | ||
| } | ||
| return config; | ||
| }); |
There was a problem hiding this comment.
Align this client with the app’s existing auth and tenant contract.
The current interceptor reads sessionStorage['mifos_token'] while the login flow writes localStorage['mifosToken'], and this client also hardcodes tenant default instead of following mifosTenant. As written, report requests can go out unauthenticated or hit the wrong tenant.
Suggested fix
const api = axios.create({
baseURL: import.meta.env.VITE_FINERACT_API_URL || '/fineract-provider/api/v1',
headers: {
'Content-Type': 'application/json',
- 'Fineract-Platform-TenantId': 'default',
+ 'Fineract-Platform-TenantId': localStorage.getItem('mifosTenant') || 'default',
},
});
api.interceptors.request.use((config) => {
- const token = sessionStorage.getItem('mifos_token');
+ const token = localStorage.getItem('mifosToken');
if (token && config.headers) {
config.headers.Authorization = `Basic ${token}`;
+ config.headers['Fineract-Platform-TenantId'] =
+ localStorage.getItem('mifosTenant') || 'default';
}
return config;
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/api.ts` around lines 3 - 17, The API client currently hardcodes
tenant and reads the wrong storage key; update the axios instance/headers
created in api (axios.create) and the request interceptor
(api.interceptors.request.use) to read the auth token from
localStorage.getItem('mifosToken') (not sessionStorage['mifos_token']) and set
Authorization accordingly, and replace the hardcoded 'default'
Fineract-Platform-TenantId header with the app’s tenant value (mifosTenant)
obtained from the same source/config used by the login flow so requests use the
correct tenant and auth.
98377d4 to
b126a9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/api.ts (1)
5-15:⚠️ Potential issue | 🔴 CriticalKeep auth and tenant lookup aligned with the rest of the app.
src/pages/login/loginSlice.tswrites credentials tolocalStorage['mifosToken'], andsrc/router/ProtectedRoutes.tsxreads that same key. This interceptor still readssessionStorage['mifos_token'], so report calls stay unauthenticated after login. The hardcodeddefaulttenant has the same problem for non-default deployments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api.ts` around lines 5 - 15, The request interceptor is using sessionStorage['mifos_token'] and a hardcoded tenant 'default', which doesn't match the app's storage keys; update api.interceptors.request.use to read the auth token from localStorage.getItem('mifosToken') (the same key written by loginSlice) and set Authorization when present, and replace the hardcoded 'default' tenant header with a value read from localStorage (e.g. localStorage.getItem('mifosTenant')) with a safe fallback like process.env.REACT_APP_FINERACT_TENANT || 'default' so tenant lookup aligns with the rest of the app.
🤖 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/services/api.ts`:
- Around line 3-4: Replace the direct import.meta.env read in the axios client
creation by calling the app's shared API URL resolver and using its result for
the baseURL: in the code that builds const api = axios.create({ baseURL: ... }),
import and call the shared resolver function exported by the app's env/http
helpers (the resolver commonly exported from env-config.ts or http-client.ts —
e.g., getFineractBaseUrl or resolveApiBaseUrl) and pass its returned URL into
baseURL instead of import.meta.env.VITE_FINERACT_API_URL ||
'/fineract-provider/api/v1'; remove the hardcoded fallback so this client uses
the same runtime window.__env overrides and assembly logic as the rest of the
app.
---
Duplicate comments:
In `@src/services/api.ts`:
- Around line 5-15: The request interceptor is using
sessionStorage['mifos_token'] and a hardcoded tenant 'default', which doesn't
match the app's storage keys; update api.interceptors.request.use to read the
auth token from localStorage.getItem('mifosToken') (the same key written by
loginSlice) and set Authorization when present, and replace the hardcoded
'default' tenant header with a value read from localStorage (e.g.
localStorage.getItem('mifosTenant')) with a safe fallback like
process.env.REACT_APP_FINERACT_TENANT || 'default' so tenant lookup aligns with
the rest of the app.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05dc5122-7937-4126-a2e3-885187b10a4a
📒 Files selected for processing (4)
src/pages/reports/RunReports.tsxsrc/router/AppRoutes.tsxsrc/services/api.tssrc/services/reportsApi.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/router/AppRoutes.tsx
- src/services/reportsApi.ts
- src/pages/reports/RunReports.tsx
DhroovSankla
force-pushed
the
MXWAR-66-run-reports
branch
from
2be498c to
edccb7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/pages/reports/RunReports.tsx (4)
35-38: Consider logging caught errors for debugging.Both
catchblocks set user-facing error messages but don't log the actual error, making debugging difficult.♻️ Proposed fix
- } catch { + } catch (err) { + console.error('Failed to load report data:', err) setError( 'Failed to load report parameters. Ensure you are connected to the demo server.' ) // and - } catch { + } catch (err) { + console.error('Report execution failed:', err) setError('Execution failed. Please check your parameters.') }Also applies to: 60-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/reports/RunReports.tsx` around lines 35 - 38, The catch blocks in RunReports.tsx currently swallow exceptions and only call setError with a user-facing message; update both catch blocks (the one around the report parameters fetch and the one around the other fetch at lines referenced) to accept an error parameter (e.g., catch (err)) and log the error details via console.error or a project logger before calling setError, referencing the setError call and the surrounding async function (e.g., the report-loading function in RunReports) so the thrown error is captured and recorded for debugging.
1-1: UnusedReactimport.With React 19 and the new JSX transform, the
Reactnamespace import is not required forReact.FC. You can remove the import or useimport type { FC } from 'react'if preferred.♻️ Proposed fix
-import React, { useEffect, useState } from 'react' +import { useEffect, useState, type FC } from 'react' // ... -const RunReports: React.FC = () => { +const RunReports: FC = () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/reports/RunReports.tsx` at line 1, Remove the unused default React import from the top of the file (the line importing React) since the new JSX transform makes it unnecessary; either delete that import entirely or replace it with a type-only import like "import type { FC } from 'react'" if you still reference the FC type (e.g., component declaration such as RunReports or RunReports: FC). Ensure there are no other references to the React namespace before committing.
50-63: Report execution succeeds but results are not displayed.The
handleRunReportfunction calls the API and logs the response, but the user has no visibility into the results. Consider displaying the report data, downloading it, or navigating to a results view.💡 Suggested approach
+ const [reportResult, setReportResult] = useState<unknown>(null) + const handleRunReport = async () => { try { if (reportName) { const response = await reportsApi.runReport( reportName, formValues as Record<string, unknown> ) - console.log('Report Data:', response.data) + setReportResult(response.data) + // Or trigger a download, navigate to results page, etc. } } catch { setError('Execution failed. Please check your parameters.') } }Then render
reportResultin the UI when available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/reports/RunReports.tsx` around lines 50 - 63, The handler currently logs API results but doesn't surface them to the user; update handleRunReport to save the returned payload (response.data) into component state (e.g., introduce reportResult and setReportResult via useState) after the successful reportsApi.runReport call, and then render reportResult in the component UI (or trigger a download/navigate to a results view) so users can see the report; ensure error handling still uses setError.
107-108: Unsafe type assertion onreportData.Casting
unknownto a specific shape bypasses type safety. Consider defining a proper interface for the report response and using type guards or schema validation.💡 Suggested approach
+interface ReportData { + reportParameters?: ReportParameter[] + // Add other expected fields +} - const [reportData, setReportData] = useState<unknown>(null) + const [reportData, setReportData] = useState<ReportData | null>(null) // Remove the casting on line 108 - const typedData = reportData as { reportParameters?: ReportParameter[] } + // Use reportData directly with proper typing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/reports/RunReports.tsx` around lines 107 - 108, The code unsafely asserts reportData to a shape via "const typedData = reportData as { reportParameters?: ReportParameter[] }"; instead define a proper interface (e.g., ReportResponse { reportParameters?: ReportParameter[]; ... }) and replace the assertion with a runtime type guard or validation function (e.g., isReportResponse(obj): obj != null && Array.isArray(obj.reportParameters) && reportParameters items validate as ReportParameter) and use that guard before mapping; update usages in RunReports.tsx to narrow reportData via the guard instead of a direct cast so TypeScript safety is preserved.src/pages/centers/centers-view/CentersView.tsx (1)
118-125: Consider usingAlertDialogfor consistent UX.The Delete action uses the native
confirm()dialog, while other parts of the codebase (e.g.,ViewUsers.tsx) use theAlertDialogcomponent for similar confirmations. Consider aligning with the established UI pattern for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/centers/centers-view/CentersView.tsx` around lines 118 - 125, Replace the native confirm() usage in the Delete menu item with the app's AlertDialog pattern used elsewhere: add state (e.g., isDeleteDialogOpen) and an AlertDialog component imported from the UI library, render an AlertDialog.Trigger in place of the current onClick so opening the dialog is controlled, and call the existing handleDelete() from the AlertDialog's confirm/accept handler (and close the dialog on cancel). Ensure the AlertDialog includes the same confirm text and accessibility attributes and remove the inline confirm() call inside the menu item's onClick..gitignore (1)
39-39: Consider adding a comment to explain whyvite.config.tsis ignored.The project intentionally removed
vite.config.tsin favor of using Vite's default configuration. Adding a brief comment like# vite.config.ts was removed in favor of default configwould help future developers understand this choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 39, Add a brief explanatory comment above the vite.config.ts entry in .gitignore stating why it is ignored (e.g., "vite.config.ts was removed in favor of Vite's default config") so future developers understand the intent; update the .gitignore entry for vite.config.ts to include that single-line comment immediately before or after the existing filename line.
🤖 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/pages/navigation/client-navigation/ClientNavigation.tsx`:
- Around line 62-64: The span is rendering the string result of the typeof
operator instead of the actual ID; update the expression in ClientNavigation.tsx
to render the actual client.externalId (e.g., use client.externalId ??
t('actions.na') or client.externalId || t('actions.na') depending on whether
empty strings should be considered valid) so the span shows the real externalId
value rather than the word "string"/"undefined".
In `@src/pages/reports/RunReports.tsx`:
- Around line 65-98: renderInput creates uncontrolled inputs/selects: bind each
element's value to the corresponding formValues entry so the UI reflects state
and can be pre-populated. Update renderInput (and the select for officeId) to
set value={formValues[name] ?? ''} (or equivalent safe accessor) and keep
onChange calling handleInputChange(name, e.target.value); reference renderInput,
handleInputChange, and formValues (and offices for the select) when making the
changes.
In `@src/pages/users/ViewUsers.tsx`:
- Around line 104-109: The Confirm button in the AlertDialog uses
AlertDialogAction but has its onClick commented out, leaving the delete
operation inert; restore and wire an actual handler (e.g., handleDelete) or
remove the button if deletion isn't supported. Implement or locate the delete
routine (handleDelete) in the ViewUsers component to call the delete
API/dispatch, update local state or refetch the user list, and close the dialog
(and handle errors/loading states), then re-enable the onClick on
AlertDialogAction to call handleDelete; ensure the handler is bound correctly
and any necessary props (user id) are passed into the function.
---
Nitpick comments:
In @.gitignore:
- Line 39: Add a brief explanatory comment above the vite.config.ts entry in
.gitignore stating why it is ignored (e.g., "vite.config.ts was removed in favor
of Vite's default config") so future developers understand the intent; update
the .gitignore entry for vite.config.ts to include that single-line comment
immediately before or after the existing filename line.
In `@src/pages/centers/centers-view/CentersView.tsx`:
- Around line 118-125: Replace the native confirm() usage in the Delete menu
item with the app's AlertDialog pattern used elsewhere: add state (e.g.,
isDeleteDialogOpen) and an AlertDialog component imported from the UI library,
render an AlertDialog.Trigger in place of the current onClick so opening the
dialog is controlled, and call the existing handleDelete() from the
AlertDialog's confirm/accept handler (and close the dialog on cancel). Ensure
the AlertDialog includes the same confirm text and accessibility attributes and
remove the inline confirm() call inside the menu item's onClick.
In `@src/pages/reports/RunReports.tsx`:
- Around line 35-38: The catch blocks in RunReports.tsx currently swallow
exceptions and only call setError with a user-facing message; update both catch
blocks (the one around the report parameters fetch and the one around the other
fetch at lines referenced) to accept an error parameter (e.g., catch (err)) and
log the error details via console.error or a project logger before calling
setError, referencing the setError call and the surrounding async function
(e.g., the report-loading function in RunReports) so the thrown error is
captured and recorded for debugging.
- Line 1: Remove the unused default React import from the top of the file (the
line importing React) since the new JSX transform makes it unnecessary; either
delete that import entirely or replace it with a type-only import like "import
type { FC } from 'react'" if you still reference the FC type (e.g., component
declaration such as RunReports or RunReports: FC). Ensure there are no other
references to the React namespace before committing.
- Around line 50-63: The handler currently logs API results but doesn't surface
them to the user; update handleRunReport to save the returned payload
(response.data) into component state (e.g., introduce reportResult and
setReportResult via useState) after the successful reportsApi.runReport call,
and then render reportResult in the component UI (or trigger a download/navigate
to a results view) so users can see the report; ensure error handling still uses
setError.
- Around line 107-108: The code unsafely asserts reportData to a shape via
"const typedData = reportData as { reportParameters?: ReportParameter[] }";
instead define a proper interface (e.g., ReportResponse { reportParameters?:
ReportParameter[]; ... }) and replace the assertion with a runtime type guard or
validation function (e.g., isReportResponse(obj): obj != null &&
Array.isArray(obj.reportParameters) && reportParameters items validate as
ReportParameter) and use that guard before mapping; update usages in
RunReports.tsx to narrow reportData via the guard instead of a direct cast so
TypeScript safety is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cef124e4-0112-4222-a22d-0033188f8bb9
📒 Files selected for processing (33)
.gitignoresrc/components/custom/language-switcher/LanguageSwitcher.tsxsrc/components/custom/navbar/MfNavbar.tsxsrc/components/custom/sidebar/AppSidebar.tsxsrc/lib/axios.tssrc/lib/env-config.tssrc/lib/http-client.tssrc/lib/i18n.tssrc/main.tsxsrc/pages/centers/Centers.tsxsrc/pages/centers/centers-view/CentersView.tsxsrc/pages/groups/Groups.tsxsrc/pages/home/dashboard/amount-collected-pie/AmountCollectedPie.tsxsrc/pages/home/dashboard/amount-disbursed-pie/AmountDisbursedPie.tsxsrc/pages/home/dashboard/client-trends-bar/ClientTrendsBar.tsxsrc/pages/login/Login.tsxsrc/pages/navigation/Navigation.tsxsrc/pages/navigation/center-navigation/CenterNavigation.tsxsrc/pages/navigation/client-navigation/ClientNavigation.tsxsrc/pages/navigation/group-navigation/GroupNavigation.tsxsrc/pages/navigation/office-navigation/OfficeNavigation.tsxsrc/pages/navigation/staff-navigation/StaffNavigation.tsxsrc/pages/notifications/Notifications.tsxsrc/pages/organization/holidays/Holidays.tsxsrc/pages/organization/holidays/ViewHolidays.tsxsrc/pages/organization/offices/Offices.tsxsrc/pages/profile/Profile.tsxsrc/pages/reports/Reports.tsxsrc/pages/reports/RunReports.tsxsrc/pages/settings/Settings.tsxsrc/pages/users/ViewUsers.tsxsrc/services/api.tssrc/services/reportsApi.ts
✅ Files skipped from review due to trivial changes (18)
- src/pages/navigation/Navigation.tsx
- src/pages/groups/Groups.tsx
- src/pages/profile/Profile.tsx
- src/pages/home/dashboard/client-trends-bar/ClientTrendsBar.tsx
- src/pages/settings/Settings.tsx
- src/pages/navigation/staff-navigation/StaffNavigation.tsx
- src/pages/organization/holidays/Holidays.tsx
- src/components/custom/navbar/MfNavbar.tsx
- src/pages/navigation/group-navigation/GroupNavigation.tsx
- src/pages/navigation/office-navigation/OfficeNavigation.tsx
- src/pages/centers/Centers.tsx
- src/pages/notifications/Notifications.tsx
- src/pages/home/dashboard/amount-disbursed-pie/AmountDisbursedPie.tsx
- src/components/custom/sidebar/AppSidebar.tsx
- src/pages/organization/offices/Offices.tsx
- src/pages/reports/Reports.tsx
- src/components/custom/language-switcher/LanguageSwitcher.tsx
- src/pages/organization/holidays/ViewHolidays.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/services/reportsApi.ts
- src/services/api.ts
Comment on lines
104
to
109
| <AlertDialogAction | ||
| className="bg-red-600 hover:bg-red-700 text-white cursor-pointer" | ||
| // onClick={handleDelete} | ||
| // onClick={handleDelete} | ||
| > | ||
| Confirm | ||
| </AlertDialogAction> |
There was a problem hiding this comment.
Delete confirmation button has no action.
The onClick handler is commented out, making the "Confirm" button non-functional. Users will see a delete confirmation dialog but clicking "Confirm" does nothing. Either implement handleDelete and wire it up, or remove the Delete button entirely if not yet supported.
🛠️ Suggested approach
+ const handleDelete = async () => {
+ try {
+ await usersApi.delete25(Number(id));
+ navigate('/appusers');
+ } catch (err) {
+ console.error('Failed to delete user', err);
+ }
+ };
// In the AlertDialogAction:
<AlertDialogAction
className="bg-red-600 hover:bg-red-700 text-white cursor-pointer"
- // onClick={handleDelete}
+ onClick={handleDelete}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/users/ViewUsers.tsx` around lines 104 - 109, The Confirm button in
the AlertDialog uses AlertDialogAction but has its onClick commented out,
leaving the delete operation inert; restore and wire an actual handler (e.g.,
handleDelete) or remove the button if deletion isn't supported. Implement or
locate the delete routine (handleDelete) in the ViewUsers component to call the
delete API/dispatch, update local state or refetch the user list, and close the
dialog (and handle errors/loading states), then re-enable the onClick on
AlertDialogAction to call handleDelete; ensure the handler is bound correctly
and any necessary props (user id) are passed into the function.
c25392a to
ec60129
Compare
Contributor
|
@DhroovSankla still there are pending changes to be solved |
DhroovSankla
force-pushed
the
MXWAR-66-run-reports
branch
from
ec60129 to
193f806
Compare
Contributor
Author
Hi @IOhacker , thanks for looking at this! I’ve updated the PR with the required license headers and fixed a naming mismatch in the description. I don't see any other inline comments on the 'Files changed' tab—could you point me toward the specific pending changes you'd like me to solve? I want to make sure this is 100% ready for the migration. |
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MXWAR-66 :
This PR implements the dynamic parameter form for the Run Reports feature. It enables the application to fetch report metadata and render appropriate input fields (date pickers, dropdowns, etc.) based on the report's requirements.
Key Changes:
API Enhancement: Added retrieveOffices and runReport methods to reportsApi.ts.
Dynamic Form Logic: Implemented renderParameterInput in RunReports.tsx to handle different Fineract parameter types.
Office Integration: Integrated real-time office data fetching to populate branch dropdowns.
State Management: Implemented state binding for all dynamic inputs to capture user data for report execution.
Testing:
Verified that navigating to /reports/run/[reportName] correctly fetches metadata.
Confirmed that the "Office" dropdown populates with live data from the /offices endpoint.
Logged the submission object to the console to ensure all parameters are captured correctly.
Summary by CodeRabbit
New Features
Chores