WEB-547 feat(proxy): forward fineract provider requests to sandbox for local dev #2958
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Note
|
| Cohort / File(s) | Summary |
|---|---|
Proxy config (dev) proxy.conf.js |
Context changed to ['/fineract-provider']; target → https://sandbox.mifos.community; pathRewrite updated to strip prefix; changeOrigin: true, secure: true, logLevel: 'debug'; added onProxyReq logging and onError handler (returns 502); added setupForProxy(proxyConfig) and exported it. |
Proxy config (local) proxy.localhost.conf.js |
New file exporting rule proxying /fineract-provider → http://localhost:8443; pathRewrite strips prefix; changeOrigin: true, secure: false, logLevel: 'debug'; includes onProxyReq logging and onError → 502. |
Proxy docs docs/backend-proxy.md |
Rewritten to document forwarding /fineract-provider/... → sandbox root, pathRewrite rationale, curl examples and sample outputs, 502 error behavior, corporate-proxy behavior (HttpsProxyAgent via setupForProxy()), and SSL guidance. |
README & locales README.md, .../locales/* |
Added Proxy Configuration sections and sandbox/local usage instructions; updated Quick Links and backend wording; duplicated proxy messaging across README sections; renamed Lithuanian locale li-LI.json → lt-LT.json. |
Sequence Diagram(s)
sequenceDiagram
participant Client as Client
participant DevServer as Dev Server / Proxy
participant CorpProxy as Corporate Proxy (optional)
participant Sandbox as Fineract Sandbox
rect rgb(240,248,255)
Note over Client,Sandbox: Request flow for /fineract-provider/...
Client->>DevServer: GET /fineract-provider/1.0/...
DevServer->>DevServer: pathRewrite -> log (onProxyReq)
alt HTTP_PROXY/http_proxy set
DevServer->>CorpProxy: Forward via HttpsProxyAgent
CorpProxy->>Sandbox: Forward request
Sandbox-->>CorpProxy: Response
CorpProxy-->>DevServer: Response
else no corporate proxy
DevServer->>Sandbox: Forward request directly
Sandbox-->>DevServer: Response
end
DevServer-->>Client: Proxy response
end
rect rgb(255,240,240)
Note over DevServer,Sandbox: Error handling
Sandbox--xDevServer: Error / Timeout
DevServer->>DevServer: onError — log and prepare 502
DevServer-->>Client: HTTP 502 + client-visible message
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the main change: adding a proxy to forward fineract-provider requests to the sandbox for local development. It is concise, specific, and clearly related to the primary objective of the changeset. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
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
This PR adds a proxy configuration to forward /fineract-provider requests to the Mifos sandbox environment for local development, replacing the previous Chuck Norris API example configuration. This enables developers to work against a remote Fineract instance while avoiding CORS issues and maintaining corporate proxy agent support.
Key changes:
- Replaced example API proxy configuration with production-oriented Fineract provider proxy
- Updated security settings and added debug logging for proxy requests
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
proxy.conf.js
Outdated
| secure: true, | ||
| logLevel: 'debug', | ||
| onProxyReq: function(proxyReq, req, res) { | ||
| console.log('[Proxy] Proxying:', req.method, req.url, '->', 'https://sandbox.mifos.community' + req.url); |
There was a problem hiding this comment.
The target URL is hardcoded in the log statement instead of using the target variable from the configuration. This creates a maintenance issue where updating the target on line 14 won't automatically update the logging output. Consider using a variable reference to ensure consistency.
| console.log('[Proxy] Proxying:', req.method, req.url, '->', 'https://sandbox.mifos.community' + req.url); | |
| console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + req.url); |
proxy.conf.js
Outdated
| secure: true, | ||
| logLevel: 'debug', | ||
| onProxyReq: function(proxyReq, req, res) { | ||
| console.log('[Proxy] Proxying:', req.method, req.url, '->', 'https://sandbox.mifos.community' + req.url); |
There was a problem hiding this comment.
The proxy configuration lacks error handling. Consider adding an onError callback to handle and log proxy errors gracefully. Without this, proxy failures may go unnoticed or cause unclear errors during development.
| console.log('[Proxy] Proxying:', req.method, req.url, '->', 'https://sandbox.mifos.community' + req.url); | |
| console.log('[Proxy] Proxying:', req.method, req.url, '->', 'https://sandbox.mifos.community' + req.url); | |
| }, | |
| onError: function(err, req, res) { | |
| console.error('[Proxy] Error while proxying request:', req && req.method, req && req.url, '->', 'https://sandbox.mifos.community', '-', err && err.message); | |
| if (res && !res.headersSent) { | |
| res.writeHead(502, { 'Content-Type': 'text/plain' }); | |
| res.end('Proxy error: ' + (err && err.message ? err.message : 'Unknown error')); | |
| } |
There was a problem hiding this comment.
Add an entry in the README about this change. Just another important point is that now it is exposed the API of the backend in the URL https://apis.mifos.community , these are the variables/values used in the Sandbox Web App
- FINERACT_API_URLS=https://apis.mifos.community
- FINERACT_API_URL=https://apis.mifos.community
- FINERACT_API_PROVIDER=/1.0/core/api
- FINERACT_API_ACTUATOR=/1.0/core
- FINERACT_API_VERSION=/v1
- FINERACT_PLATFORM_TENANT_IDENTIFIER=default
- MIFOS_DEFAULT_LANGUAGE=en-US
- MIFOS_SUPPORTED_LANGUAGES=cs-CS,de-DE,en-US,es-MX,fr-FR,it-IT,ko-KO,li-LI,lv-LV,ne-NE,pt-PT,sw-SW
- MIFOS_PRELOAD_CLIENTS=true
- MIFOS_DEFAULT_CHAR_DELIMITER=,
I think that you can keep another proxy conf in order to use the localhost
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proxy.conf.js (1)
3-3: Upgrade https-proxy-agent to patch CVE-2018-3739.Version 7.0.2 is affected by CVE-2018-3739, which allows denial-of-service and possible uninitialized-memory exposure via unsanitized proxy authentication data passed to Buffer. Upgrade to the latest patched version of https-proxy-agent.
🧹 Nitpick comments (1)
proxy.conf.js (1)
18-20: Consider using the config value instead of hardcoding the target URL.The logging hook hardcodes
'https://sandbox.mifos.community'instead of referencing the actual target from the configuration. If the target changes, the logs will be misleading.🔎 Suggested refactor
- onProxyReq: function(proxyReq, req, res) { - console.log('[Proxy] Proxying:', req.method, req.url, '->', 'https://sandbox.mifos.community' + req.url); + onProxyReq: function(proxyReq, req, res) { + console.log('[Proxy] Proxying:', req.method, req.url, '-> https://sandbox.mifos.community' + req.url); }Or better yet, reference the target from the parent scope:
+const PROXY_TARGET = 'https://sandbox.mifos.community'; const proxyConfig = [ { context: ['/fineract-provider'], - target: 'https://sandbox.mifos.community', + target: PROXY_TARGET, changeOrigin: true, secure: true, logLevel: 'debug', onProxyReq: function(proxyReq, req, res) { - console.log('[Proxy] Proxying:', req.method, req.url, '->', 'https://sandbox.mifos.community' + req.url); + console.log('[Proxy] Proxying:', req.method, req.url, '->', PROXY_TARGET + req.url); } } ];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy.conf.js
🔇 Additional comments (2)
proxy.conf.js (2)
13-14: LGTM! Verify the target endpoint path structure.The proxy configuration correctly forwards
/fineract-providerrequests to the Mifos sandbox. Since nopathRewriteis specified, the path is preserved in the forwarded request (e.g.,/fineract-provider/api/v1/offices→https://sandbox.mifos.community/fineract-provider/api/v1/offices).Please verify that the Mifos sandbox expects requests at the
/fineract-providerpath. If the sandbox API is directly at the root, you may need to add:context: ['/fineract-provider'], target: 'https://sandbox.mifos.community', + pathRewrite: { '^/fineract-provider': '' }, changeOrigin: true,
16-17: LGTM! Appropriate settings for secure proxy with debugging.Setting
secure: trueis correct for the HTTPS target, andlogLevel: 'debug'will help troubleshoot proxy issues during local development.
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Kindly review my comments
proxy.conf.js
Outdated
| secure: true, | ||
| logLevel: 'debug', | ||
| onProxyReq: function(proxyReq, req, res) { | ||
| console.log('[Proxy] Proxying:', req.method, req.url, '->', 'https://sandbox.mifos.community' + req.url); |
There was a problem hiding this comment.
Add an entry in the README about this change. Just another important point is that now it is exposed the API of the backend in the URL https://apis.mifos.community , these are the variables/values used in the Sandbox Web App
- FINERACT_API_URLS=https://apis.mifos.community
- FINERACT_API_URL=https://apis.mifos.community
- FINERACT_API_PROVIDER=/1.0/core/api
- FINERACT_API_ACTUATOR=/1.0/core
- FINERACT_API_VERSION=/v1
- FINERACT_PLATFORM_TENANT_IDENTIFIER=default
- MIFOS_DEFAULT_LANGUAGE=en-US
- MIFOS_SUPPORTED_LANGUAGES=cs-CS,de-DE,en-US,es-MX,fr-FR,it-IT,ko-KO,li-LI,lv-LV,ne-NE,pt-PT,sw-SW
- MIFOS_PRELOAD_CLIENTS=true
- MIFOS_DEFAULT_CHAR_DELIMITER=,
I think that you can keep another proxy conf in order to use the localhost
|
Use this PR https://mifosforge.jira.com/browse/WEB-547 in your commit and PR, squash and commit your changes and for any new future PR please open a ticket or you can ask in the web-app channel for any assistance https://mifosforge.jira.com/jira/software/c/projects/WEB/boards/62 . Thank you for your efforts and contributions. |
priyans11
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
proxy.conf.js (2)
29-46: Fix hardcoded target URL in commented configuration.The commented localhost configuration has the same hardcoding issue as the main config (line 40). Even though it's commented out, it should use
this.targetfor consistency.🔎 Proposed fix
// onError: function(err, req, res) { - // console.error('[Proxy] Error while proxying request:', req && req.method, req && req.url, '->', 'http://localhost:8443', '-', err && err.message); + // console.error('[Proxy] Error while proxying request:', req && req.method, req && req.url, '->', this.target, '-', err && err.message); // if (res && !res.headersSent) { // res.writeHead(502, { 'Content-Type': 'text/plain' }); // res.end('Proxy error: ' + (err && err.message ? err.message : 'Unknown error'));
67-67: Clarify the warning message.The warning message is misleading. It states "API requests will not be proxied," but this only refers to the absence of a corporate proxy agent (for environments requiring HTTP_PROXY). The API proxy configuration itself will still function and forward requests to the configured target.
🔎 Proposed clarification
} else { - console.warn('No proxy server configured. API requests will not be proxied.'); + console.warn('No corporate proxy server configured. Direct connections will be used.'); }
|
I suggest to create a new filea instead of commenting the section like a new file called proxy.localhost.conf.sample |
|
@IOhacker README updates are in progress and can be handled separately if required. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proxy.conf.js (2)
19-19: Add defensive null checks forthis.target.While
this.targetshould be available in the callback context, adding defensive checks would make the code more robust and prevent potentialundefinedstring concatenation if the binding fails.🔎 Proposed refinement
onProxyReq: function(proxyReq, req, res) { - console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + req.url); + console.log('[Proxy] Proxying:', req.method, req.url, '->', (this && this.target ? this.target : 'unknown') + req.url); }, onError: function(err, req, res) { - console.error('[Proxy] Error while proxying request:', req && req.method, req && req.url, '->', this.target, '-', err && err.message); + console.error('[Proxy] Error while proxying request:', req && req.method, req && req.url, '->', (this && this.target ? this.target : 'unknown'), '-', err && err.message); if (res && !res.headersSent) {Based on the previous review comments suggesting a safe fallback.
Also applies to: 22-22
29-46: Consider extracting the localhost configuration to a sample file.The commented localhost configuration duplicates the handler logic from the main config. As IOhacker suggested earlier, creating a separate
proxy.localhost.conf.samplefile would better follow DRY principles and make it easier to maintain both configurations independently.Based on the previous review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy.conf.js
🔇 Additional comments (3)
proxy.conf.js (3)
14-14: Verify the correct target URL.IOhacker's previous comment mentioned that the API is now exposed at
https://apis.mifos.community, but this configuration useshttps://sandbox.mifos.community. Please confirm which endpoint is correct for the proxy target.Based on the previous review comments.
12-28: Reminder: Complete the README updates.As mentioned in your previous comment and requested by IOhacker, please ensure the README is updated to document this proxy configuration change. The documentation should explain the proxy setup and the environment variables mentioned by IOhacker (FINERACT_API_URLS, FINERACT_API_PROVIDER, etc.).
Based on the previous review comments.
21-27: Excellent error handling implementation.The error handler correctly:
- Uses
this.targetconsistently with the request logger (addressing previous feedback)- Checks if headers were already sent before responding
- Returns an appropriate 502 status code
- Provides informative error messages for debugging
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
147-160: Clarify where sandbox environment variables should be configured.The environment variables are listed but the documentation doesn't explicitly state whether these should be set in
.env,environment.ts, Docker compose variables, or elsewhere. Add guidance on how to apply these variables in the development workflow.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md
🔇 Additional comments (1)
README.md (1)
135-179: No action required—referenced files exist and are correct.Both
proxy.conf.jsand./docs/backend-proxy.mdexist in the repository and are correctly referenced. The proxy configuration comment inproxy.conf.js(line 29) matches the README documentation exactly. The relationship between the proxy target (sandbox.mifos.community) and API endpoint (apis.mifos.community) is already clarified in the README with the note "(exposed in the sandbox)".Likely an incorrect or invalid review comment.
|
@priyans11 please squash and commit |
|
@IOhacker |
|
@IOhacker Kindly have another look when you get a chance. Thank you for your time and guidance. |
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Kindly, reply my comment
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
proxy.conf.js (1)
37-54: Consider extracting the localhost configuration to a separate file.IOhacker previously suggested creating a separate file (e.g.,
proxy.localhost.conf.sampleorproxy.localhost.conf.js) instead of keeping the alternative configuration commented inline. This would:
- Make it easier to switch between configurations (copy/rename file)
- Avoid maintaining duplicate code in comments
- Provide a clearer separation of different proxy setups
The current inline approach works fine, so this is optional.
🔎 Suggested approach
Create a new file
proxy.localhost.conf.jswith:'use strict'; const { HttpsProxyAgent } = require('https-proxy-agent'); const proxyConfig = [ { context: ['/fineract-provider'], target: 'http://localhost:8443', changeOrigin: true, secure: false, logLevel: 'debug', onProxyReq: function(proxyReq, req, res) { console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + req.url); }, onError: function(err, req, res) { console.error('[Proxy] Error while proxying request:', req && req.method, req && req.url, '->', this.target, '-', err && err.message); if (res && !res.headersSent) { res.writeHead(502, { 'Content-Type': 'text/plain' }); res.end('Proxy error: ' + (err && err.message ? err.message : 'Unknown error')); } } } ]; function setupForProxy(proxyConfig) { // ... same implementation } module.exports = setupForProxy(proxyConfig);Then document in README how to switch configurations (e.g., renaming files or updating angular.json).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdproxy.conf.js
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🔇 Additional comments (3)
proxy.conf.js (3)
18-35: LGTM! Logging and error handling are well implemented.The proxy logging and error handling have been properly addressed based on previous review feedback:
- Line 19 and 27 correctly use
this.targetinstead of hardcoded URLs- Comprehensive error handling with appropriate 502 status codes
- Defensive checks for headers and error messages
60-79: LGTM! Corporate proxy setup is well implemented.The
setupForProxyfunction correctly handles corporate proxy scenarios:
- Normalizes single or array configurations
- Reads standard
HTTP_PROXYenvironment variable- Applies the proxy agent to all configurations
- Provides helpful console feedback
The
https-proxy-agentpackage (v7.0.2) is properly listed in devDependencies and correctly imported at the top of the file.
13-14: Verify or correct the API proxy path configuration—documentation shows a potential domain/path mismatch.The proxy is configured to target
https://sandbox.mifos.communitywith context/fineract-providerand no pathRewrite. However, the README.md documentation indicates the actual Fineract API is exposed athttps://apis.mifos.community(a different domain) with path/1.0/core/api.Without pathRewrite, requests to
/fineract-provider/*will be forwarded as-is tohttps://sandbox.mifos.community/fineract-provider/*. If the sandbox backend expects a different path structure (e.g.,/1.0/core/api/*), this configuration will fail.Either:
- Confirm that
sandbox.mifos.communityaccepts and routes/fineract-providerrequests correctly, or- Update the configuration to use the correct target domain and add pathRewrite rules as documented
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proxy.conf.js (2)
19-21: Logged URL doesn't reflect the rewritten path.The log shows
this.target + req.urlwhich includes the/fineract-providerprefix, butpathRewritestrips this prefix before forwarding. For example, a request to/fineract-provider/api/v1/clientslogs as going tohttps://sandbox.mifos.community/fineract-provider/api/v1/clients, but it actually forwards tohttps://sandbox.mifos.community/api/v1/clients.Consider logging the rewritten path to avoid confusion when debugging:
🔎 Proposed fix
onProxyReq: function (proxyReq, req, res) { - console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + req.url); + const rewrittenPath = req.url.replace(/^\/fineract-provider/, ''); + console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + rewrittenPath); },
37-41: Add a commented comma to ease switching between configurations.If a developer uncomments the localhost block, they'll encounter a syntax error because there's no comma after the closing brace on line 37. Consider adding a commented comma to make the switch easier:
🔎 Proposed fix
} - } + }, // Uncomment this configuration to proxy to a local Fineract instance instead // {Alternatively, per the reviewer's earlier suggestion, consider creating a separate
proxy.localhost.conf.jssample file instead of inline comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy.conf.js
🔇 Additional comments (2)
proxy.conf.js (2)
22-36: Error handling looks solid.Good implementation with defensive checks (
res && !res.headersSent) and proper use ofthis.targetfor dynamic logging. The 502 response with a descriptive error message provides clear feedback during development.
13-17: Configuration looks good overall.The proxy setup correctly:
- Routes
/fineract-providerrequests to the sandbox backend- Strips the path prefix via
pathRewriteto avoid 404s (addresses the earlier review concern)- Uses
secure: truefor SSL validation- Enables debug logging for troubleshooting
priyans11
force-pushed
the
proxy_rule
branch
3 times, most recently
from
01f775d to
9753bd2
Compare
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
@priyans11 why the breadcrumb and entity notes tab have been modified in this ticket?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
proxy.conf.js (1)
39-57: Consider extracting the localhost config to a separate sample file.Per the earlier review feedback from IOhacker, it was suggested to create a separate
proxy.localhost.conf.samplefile instead of keeping the alternative configuration commented inline. This would make it easier for developers to switch between configurations and keep the main file cleaner.Also note: Line 48 uses
req.url(unprefixed) while the active configuration on line 21 uses the manually calculatedrewrittenPathfor consistency. If this block is ever uncommented, it would log different path formats.
🧹 Nitpick comments (1)
src/app/core/shell/breadcrumb/breadcrumb.component.ts (1)
100-101: Consider using stricter types instead ofany.Per Angular best practices and the coding guidelines for strict type safety, consider typing these variables more explicitly:
- let breadcrumbLabel: any; - let url: any; + let breadcrumbLabel: string | false = false; + let url: string | undefined;This would align with the
Breadcrumbinterface which expectslabel: stringandurl: string.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proxy.conf.jssrc/app/core/shell/breadcrumb/breadcrumb.component.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/core/shell/breadcrumb/breadcrumb.component.ts
🧬 Code graph analysis (1)
src/app/core/shell/breadcrumb/breadcrumb.component.ts (1)
src/app/core/shell/breadcrumb/breadcrumb.model.ts (1)
Breadcrumb(4-7)
🔇 Additional comments (5)
proxy.conf.js (1)
13-37: LGTM! Proxy configuration looks well-structured.The configuration correctly:
- Uses
pathRewriteto strip/fineract-providerprefix- References
this.targetdynamically in bothonProxyReqandonErrorhandlers- Returns proper 502 status with informative error messages
- Enables debug logging for development visibility
src/app/core/shell/breadcrumb/breadcrumb.component.ts (4)
58-64: Good implementation of OnDestroy and subscription cleanup.The
destroy$Subject pattern withtakeUntilis the recommended approach for managing Observable subscriptions in Angular components. This prevents memory leaks when the component is destroyed.
90-93: Clean observable merge pattern for language-aware breadcrumbs.The use of
merge()to combineNavigationEndevents withonLangChangeis a clean approach to ensure breadcrumbs regenerate when the language changes. ThetakeUntil(this.destroy$)properly manages the subscription lifecycle.
272-289: Translation fallback logic is sound.The enhanced
getTranslatemethod correctly:
- Checks
labels.text.*first for static breadcrumb labels- Falls back to
labels.menus.*for action names- Returns the original text if no translation is found
This ensures breadcrumbs remain functional even when translations are missing.
294-297: Proper cleanup in ngOnDestroy.Calling both
next()andcomplete()on thedestroy$Subject ensures all subscriptions usingtakeUntil(this.destroy$)are properly terminated and resources are released.
priyans11
force-pushed
the
proxy_rule
branch
3 times, most recently
from
449f1a0 to
73ceb3a
Compare
@IOhacker |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
proxy.conf.js
Outdated
| // onProxyReq: function(proxyReq, req, res) { | ||
| // console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + req.url); |
There was a problem hiding this comment.
The logging in the commented localhost configuration is inconsistent with the active sandbox configuration. The active configuration on line 21 computes a rewrittenPath to show the actual proxied path, but this localhost version logs req.url directly without applying the path rewrite. For consistency and accuracy in debugging, this should also compute and log the rewritten path similar to the active configuration.
| // onProxyReq: function(proxyReq, req, res) { | |
| // console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + req.url); | |
| // onProxyReq: function (proxyReq, req, res) { | |
| // const rewrittenPath = (req.url || '').replace(/^\/fineract-provider/, ''); | |
| // console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + rewrittenPath); |
README.md
Outdated
| - **Corporate Proxy Support:** Maintains support for corporate proxy agents via `HTTP_PROXY` environment variable | ||
| - **Debug Logging:** All proxy requests are logged for troubleshooting | ||
|
|
||
| For more details on proxy configuration, see the [backend proxy documentation](./docs/backend-proxy.md). |
There was a problem hiding this comment.
The referenced backend-proxy.md documentation file still contains outdated examples showing the old Chuck Norris API proxy configuration. Since this PR updates the proxy to use Fineract endpoints, the documentation should be updated to reflect the current proxy configuration to avoid confusion for developers.
| For more details on proxy configuration, see the [backend proxy documentation](./docs/backend-proxy.md). | |
| The proxy is configured to work with Fineract endpoints as described in this section. |
README.md
Outdated
|
|
||
| - **Target:** `https://sandbox.mifos.community` | ||
| - **API Endpoint:** `https://apis.mifos.community` (exposed in the sandbox) | ||
| - **System Reset:** Every 6 hours |
There was a problem hiding this comment.
The documentation states that the system resets every 6 hours, but it doesn't clarify what "system reset" means in this context. Does this mean the sandbox data is wiped, authentication tokens are invalidated, or the service restarts? Adding clarity here would help developers understand what to expect during development.
README.md
Outdated
| FINERACT_API_VERSION=/v1 | ||
| FINERACT_PLATFORM_TENANT_IDENTIFIER=default | ||
| MIFOS_DEFAULT_LANGUAGE=en-US | ||
| MIFOS_SUPPORTED_LANGUAGES=cs-CS,de-DE,en-US,es-MX,fr-FR,it-IT,ko-KO,li-LI,lv-LV,ne-NE,pt-PT,sw-SW |
There was a problem hiding this comment.
Several language locale codes appear to be incorrect based on ISO 639-1 and ISO 3166-1 standards. Specifically: cs-CS should be cs-CZ (Czech Republic), ko-KO should be ko-KR (South Korea), and ne-NE should be ne-NP (Nepal). Additionally, li-LI is unclear and should be verified - it might be lt-LT for Lithuanian. These should match the actual locale codes used by the Mifos sandbox environment.
| MIFOS_SUPPORTED_LANGUAGES=cs-CS,de-DE,en-US,es-MX,fr-FR,it-IT,ko-KO,li-LI,lv-LV,ne-NE,pt-PT,sw-SW | |
| MIFOS_SUPPORTED_LANGUAGES=cs-CZ,de-DE,en-US,es-MX,fr-FR,it-IT,ko-KR,lt-LT,lv-LV,ne-NP,pt-PT,sw-SW |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
proxy.conf.js (2)
39-58: Localhost configuration provided inline.The commented localhost configuration is consistent and mirrors the sandbox setup. However, a past review suggested creating a separate sample file (e.g.,
proxy.localhost.conf.sample) instead of inline comments for better discoverability and maintainability.Do you want to extract this into a separate sample file as previously suggested, or keep it inline for simplicity?
69-80: Clarify the proxy warning message.The warning "API requests will not be proxied" is misleading. The development proxy will still forward
/fineract-providerrequests to the sandbox—only the corporate proxy agent (viaHTTP_PROXY) won't be used. Consider clarifying:🔎 Proposed fix
} else { - console.warn('No proxy server configured. API requests will not be proxied.'); + console.log('No corporate proxy (HTTP_PROXY) configured. Using direct connection to proxy target.'); }README.md (2)
147-160: Consider clarifying the scope of environment variables.The listed environment variables (e.g.,
FINERACT_API_URLS,FINERACT_API_PROVIDER) appear to be sandbox deployment configurations rather than local proxy settings. Theproxy.conf.jsdoesn't consume these variables directly. Consider adding a note that these are the sandbox backend's configuration for context, to avoid confusion with local development proxy settings.
34-39: Consider wrapping bare URLs for better formatting.Markdownlint flags the bare URLs in the backend options list. While functional, wrapping them in angle brackets (
<https://...>) or making them proper markdown links would improve readability and make them clickable in rendered documentation.🔎 Proposed fix
- **Option A: Use existing remote server** - - Use the sandbox (MariaDB) at https://sandbox.mifos.community — sandbox data is reset every 6 hours; test data and transient state may be cleared. - - Use the demo (MariaDB) at https://demo.mifos.community - - Use the demo (Keycloak) at https://oauth.mifos.community - - Use the demo (2FA) at https://2fa.mifos.community - - Use the demo (Oidc) at https://oidc.mifos.community - - Use the demo (Postgres) at https://elephant.mifos.community + - Use the sandbox (MariaDB) at <https://sandbox.mifos.community> — sandbox data is reset every 6 hours; test data and transient state may be cleared. + - Use the demo (MariaDB) at <https://demo.mifos.community> + - Use the demo (Keycloak) at <https://oauth.mifos.community> + - Use the demo (2FA) at <https://2fa.mifos.community> + - Use the demo (Oidc) at <https://oidc.mifos.community> + - Use the demo (Postgres) at <https://elephant.mifos.community>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdproxy.conf.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
34-34: Bare URL used
(MD034, no-bare-urls)
35-35: Bare URL used
(MD034, no-bare-urls)
36-36: Bare URL used
(MD034, no-bare-urls)
37-37: Bare URL used
(MD034, no-bare-urls)
38-38: Bare URL used
(MD034, no-bare-urls)
39-39: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (4)
proxy.conf.js (2)
13-22: Proxy configuration looks solid!The configuration correctly forwards
/fineract-providerrequests to the sandbox, strips the path prefix, and logs rewritten paths usingthis.targetfor consistency. The debug logging will be helpful during development.
23-37: Error handling is well-implemented!The error handler properly uses
this.targetfor consistent logging, checksres.headersSentto prevent double-writes, and returns an appropriate 502 status code with a descriptive message.README.md (2)
15-15: Clear documentation of sandbox limitations!The updated notices about 6-hour data resets and clearing of test data/transient state provide helpful transparency for developers working with the sandbox environment.
Also applies to: 34-40
157-157: Language codes corrected to ISO standards!The locale codes have been properly updated to match ISO 639-1 and ISO 3166-1 standards (cs-CZ, ko-KR, lt-LT, ne-NP). This addresses the previously flagged inconsistencies.
Also applies to: 202-219
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/backend-proxy.md
🔇 Additional comments (3)
docs/backend-proxy.md (3)
19-34: Clear and well-documented proxy configuration.The sandbox proxy configuration is clearly explained with appropriate settings (secure: true, logLevel: 'debug') and useful request logging via onProxyReq. The code block is properly formatted.
36-39: Excellent explanation of pathRewrite behavior.The example clearly demonstrates how
/fineract-provider/1.0/...is rewritten to the sandbox root endpoint, making it easy for developers to understand the path transformation logic.
47-53: Documentation accurately reflects the actual implementation.The proxy.conf.js implementation confirms all documented details: the
setupForProxy(proxyConfig)function exists, correctly checks bothhttp_proxyandHTTP_PROXYenvironment variables, attachesHttpsProxyAgentto proxy entries when detected, and logs the proxy server withconsole.log(). The documentation is aligned with the implementation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
README.md
Outdated
| FINERACT_API_URLS=https://apis.mifos.community | ||
| FINERACT_API_URL=https://apis.mifos.community | ||
| FINERACT_API_PROVIDER=/1.0/core/api | ||
| FINERACT_API_ACTUATOR=/1.0/core |
There was a problem hiding this comment.
The FINERACT_API_ACTUATOR value shown here (/1.0/core) is inconsistent with the default value documented elsewhere in the README (/fineract-provider) and in the environment files. This could cause confusion for developers trying to configure their sandbox environment. Please verify which value is correct for the sandbox environment.
| FINERACT_API_ACTUATOR=/1.0/core | |
| FINERACT_API_ACTUATOR=/fineract-provider |
README.md
Outdated
| ```bash | ||
| FINERACT_API_URLS=https://apis.mifos.community | ||
| FINERACT_API_URL=https://apis.mifos.community | ||
| FINERACT_API_PROVIDER=/1.0/core/api |
There was a problem hiding this comment.
The FINERACT_API_PROVIDER value shown here (/1.0/core/api) is inconsistent with the default value documented elsewhere in the README (/fineract-provider/api) and in the environment files. This could cause confusion for developers trying to configure their sandbox environment. Please verify which value is correct for the sandbox environment.
| FINERACT_API_PROVIDER=/1.0/core/api | |
| FINERACT_API_PROVIDER=/fineract-provider/api |
docs/backend-proxy.md
Outdated
| The repository configures a helper function in `proxy.conf.js` named `setupForProxy(proxyConfig)` which will attach an | ||
| `HttpsProxyAgent` to proxy entries when the `HTTP_PROXY` (or `http_proxy`) environment variable is present. This lets | ||
| the dev server forward requests through a corporate proxy when required. See `proxy.conf.js` for the exact behaviour | ||
| and a short console log that reports the proxy server when detected. |
There was a problem hiding this comment.
The documentation mentions a function named setupForProxy(proxyConfig), but the actual function in proxy.conf.js is named setupForProxy (which is correct). However, the documentation doesn't mention the console.warn message that appears when HTTP_PROXY is not set, which states "No proxy server configured. API requests will not be proxied." This message is misleading because the dev server proxy will still work - only the corporate proxy forwarding is disabled. Consider clarifying this behavior in the documentation.
| The repository configures a helper function in `proxy.conf.js` named `setupForProxy(proxyConfig)` which will attach an | |
| `HttpsProxyAgent` to proxy entries when the `HTTP_PROXY` (or `http_proxy`) environment variable is present. This lets | |
| the dev server forward requests through a corporate proxy when required. See `proxy.conf.js` for the exact behaviour | |
| and a short console log that reports the proxy server when detected. | |
| The repository configures a helper function in `proxy.conf.js` named `setupForProxy` (which takes `proxyConfig` as an | |
| argument) that will attach an `HttpsProxyAgent` to proxy entries when the `HTTP_PROXY` (or `http_proxy`) environment | |
| variable is present. This lets the dev server forward requests through a corporate proxy when required. When a | |
| corporate proxy is detected, `setupForProxy` logs the proxy server address; when no `HTTP_PROXY`/`http_proxy` is set, | |
| it logs a warning such as "No proxy server configured. API requests will not be proxied." This warning only means that | |
| forwarding via a corporate proxy is disabled — the dev server proxy configuration described above still works normally. |
README.md
Outdated
| | Czech | cs | cs-CZ.json | | ||
| | German | de | de-DE.json | | ||
| | English | en | en-US.json | | ||
| | Spanish | es | es-MX.json | | ||
| | French | fr | fr-FR.json | | ||
| | Italian | it | it-IT.json | | ||
| | Korean | ko | ko-KO.json | | ||
| | Lithuanian | li | li-LI.json | | ||
| | Korean | ko | ko-KR.json | | ||
| | Lithuanian | lt | lt-LT.json | | ||
| | Latvian | lv | lv-LV.json | | ||
| | Nepali | ne | ne-NE.json | | ||
| | Nepali | ne | ne-NP.json | |
There was a problem hiding this comment.
The language codes in this table are incorrect for Czech, Korean, and Nepali. The codes should be cs-CS, ko-KO, and ne-NE to match the actual translation files in src/assets/translations/ and the environment configuration files.
The file names shown are also incorrect:
- cs-CZ.json should be cs-CS.json
- ko-KR.json should be ko-KO.json
- ne-NP.json should be ne-NE.json
README.md
Outdated
| | ------------------------- | --------------------------- | ----------------------------------------------------------------------- | | ||
| | MIFOS_DEFAULT_LANGUAGE | Default language | en-US | | ||
| | MIFOS_SUPPORTED_LANGUAGES | List of supported languages | cs-CS,de-DE,en-US,es-MX,fr-FR,it-IT,ko-KO,lt-LT,lv-LV,ne-NE,pt-PT,sw-SW | | ||
| | MIFOS_SUPPORTED_LANGUAGES | List of supported languages | cs-CZ,de-DE,en-US,es-MX,fr-FR,it-IT,ko-KR,lt-LT,lv-LV,ne-NP,pt-PT,sw-SW | |
There was a problem hiding this comment.
The supported languages list contains incorrect language codes. Based on the actual translation files and environment configuration, the codes should be cs-CS, ko-KO, and ne-NE (not cs-CZ, ko-KR, and ne-NP).
The corrected value should be:
cs-CS,de-DE,en-US,es-MX,fr-FR,it-IT,ko-KO,lt-LT,lv-LV,ne-NE,pt-PT,sw-SW
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mddocs/backend-proxy.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/backend-proxy.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
34-34: Bare URL used
(MD034, no-bare-urls)
35-35: Bare URL used
(MD034, no-bare-urls)
36-36: Bare URL used
(MD034, no-bare-urls)
37-37: Bare URL used
(MD034, no-bare-urls)
38-38: Bare URL used
(MD034, no-bare-urls)
39-39: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
README.md (3)
179-179: Verify backend-proxy.md documentation is current.Line 179 references backend-proxy.md documentation. A past review flagged that backend-proxy.md contained outdated proxy configuration examples (old Chuck Norris API proxy). Please verify that the
docs/backend-proxy.mdfile has been updated to reflect the current Fineract proxy configuration described in this README section, or clarify the reference.
157-158: Language codes are now consistent and accurate.The language locale codes throughout the README have been corrected and are now consistent:
- MIFOS_SUPPORTED_LANGUAGES (line 157–158) uses correct codes
- Configuration table (line 202) matches
- Language reference table (lines 204–219) aligns with actual translation files
The Lithuanian entry has been properly updated from
li-LItolt-LT(line 215), and all codes (cs-CS, ko-KO, ne-NE, lt-LT) now match the actual translation files in the codebase. Past review feedback has been successfully addressed.Also applies to: 202-202, 204-219
15-15: Sandbox reset clarification is improved.The documentation now clearly specifies that "test data and transient state" reset every 6 hours, addressing the past review comment about clarifying what "system reset" means. This messaging is consistent across lines 15, 34, and 145–146.
Also applies to: 34-34, 145-146
priyans11
force-pushed
the
proxy_rule
branch
2 times, most recently
from
cfe14f5 to
9117cc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
34-39: Wrap bare URLs in markdown link syntax to fix markdown lint violations (MD034).These bare URLs violate the
no-bare-urlsrule. Wrap each URL in markdown link syntax for consistency with markdown standards.🔎 Proposed fix
- - Use the sandbox (MariaDB) at https://sandbox.mifos.community — sandbox data is reset every 6 hours; test data and transient state may be cleared. - - Use the demo (MariaDB) at https://demo.mifos.community - - Use the demo (Keycloak) at https://oauth.mifos.community - - Use the demo (2FA) at https://2fa.mifos.community - - Use the demo (Oidc) at https://oidc.mifos.community - - Use the demo (Postgres) at https://elephant.mifos.community + - Use the [sandbox (MariaDB)](https://sandbox.mifos.community) — sandbox data is reset every 6 hours; test data and transient state may be cleared. + - Use the [demo (MariaDB)](https://demo.mifos.community) + - Use the [demo (Keycloak)](https://oauth.mifos.community) + - Use the [demo (2FA)](https://2fa.mifos.community) + - Use the [demo (Oidc)](https://oidc.mifos.community) + - Use the [demo (Postgres)](https://elephant.mifos.community)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mddocs/backend-proxy.mdproxy.conf.js
🚧 Files skipped from review as they are similar to previous changes (1)
- proxy.conf.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
34-34: Bare URL used
(MD034, no-bare-urls)
35-35: Bare URL used
(MD034, no-bare-urls)
36-36: Bare URL used
(MD034, no-bare-urls)
37-37: Bare URL used
(MD034, no-bare-urls)
38-38: Bare URL used
(MD034, no-bare-urls)
39-39: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (2)
README.md (2)
135-180: Proxy Configuration section is comprehensive and well-structured.The new section clearly documents the sandbox proxy setup, environment variables, localhost proxy switching instructions, and testing procedures. The inclusion of both successful and error proxy scenarios with curl examples and expected outputs is helpful for developers.
157-157: Language codes are accurate and consistent with actual translation files.The current codes in MIFOS_SUPPORTED_LANGUAGES (line 157) and the language table (lines 242, 248, 254, 255, 257) are correct and match the actual translation files in the repository. The codes
cs-CS,ko-KO, andne-NEare not errors—they correspond directly tocs-CS.json,ko-KO.json, andne-NE.jsonrespectively insrc/assets/translations/. All language codes are internally consistent across the README and the codebase.Likely an incorrect or invalid review comment.
priyans11
force-pushed
the
proxy_rule
branch
2 times, most recently
from
9308fd8 to
9e40362
Compare
@IOhacker I’m sorry, I missed this comment earlier. Could you please clarify whether you’re suggesting creating a separate proxy sample JavaScript file specifically for localhost? |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
proxy.conf.js (2)
64-83: Clarify the warning message to avoid developer confusion.The warning on line 79 states "API requests will not be proxied," which is misleading. When
HTTP_PROXYis not set, only corporate proxy forwarding is disabled—the Angular dev server proxy (forwarding/fineract-providerto the sandbox) continues to work normally. This warning may confuse developers who see it but observe the proxy functioning correctly.Based on past review feedback, consider updating the message to be more specific:
🔎 Proposed fix
} else { - console.warn('No proxy server configured. API requests will not be proxied.'); + console.warn('No corporate proxy (HTTP_PROXY) configured. Dev server proxy will still forward /fineract-provider requests.'); }Alternatively, you could make it informational rather than a warning:
} else { - console.warn('No proxy server configured. API requests will not be proxied.'); + console.log('No corporate proxy configured. Using direct connection for dev server proxy.'); }
3-3: Update https-proxy-agent from 7.0.2 to 7.0.6 (latest stable version).Version 7.0.2 has no known security vulnerabilities, but 7.0.6 is the current latest release and includes additional patches and maintenance updates.
🧹 Nitpick comments (1)
proxy.conf.js (1)
39-58: Consider creating a separate sample file for the localhost configuration.As suggested in previous feedback, instead of keeping this as a commented block, create a separate file
proxy.localhost.conf.sample(orproxy.conf.local.example.js) that developers can copy and rename when they need to proxy to a local Fineract instance. This approach:
- Keeps the main configuration file clean and focused
- Provides a clear, documented example for local development
- Makes it easier for developers to switch between configurations
🔎 Suggested approach
- Create a new file
proxy.conf.local.example.jswith the localhost configuration:'use strict'; const { HttpsProxyAgent } = require('https-proxy-agent'); const proxyConfig = [ { context: ['/fineract-provider'], target: 'http://localhost:8443', pathRewrite: { '^/fineract-provider': '' }, changeOrigin: true, secure: false, logLevel: 'debug', onProxyReq: function(proxyReq, req, res) { const rewrittenPath = (req.url || '').replace(/^\/fineract-provider/, ''); console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + rewrittenPath); }, onError: function(err, req, res) { console.error('[Proxy] Error while proxying request:', req && req.method, req && req.url, '->', this.target, '-', err && err.message); if (res && !res.headersSent) { res.writeHead(502, { 'Content-Type': 'text/plain' }); res.end('Proxy error: ' + (err && err.message ? err.message : 'Unknown error')); } } } ]; function setupForProxy(proxyConfig) { // ... same as in proxy.conf.js } module.exports = setupForProxy(proxyConfig);
Remove the commented block from
proxy.conf.js(lines 39-58)Add a note in the main file or README explaining: "To proxy to a local Fineract instance, copy
proxy.conf.local.example.jstoproxy.conf.js"This addresses the reviewer's feedback and provides a cleaner solution for developers who need localhost proxying.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mddocs/backend-proxy.mdproxy.conf.js
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🔇 Additional comments (4)
proxy.conf.js (1)
11-38: LGTM! Proxy configuration is well-structured.The proxy configuration correctly:
- Uses array context for clarity
- Strips the
/fineract-providerprefix viapathRewrite- Logs rewritten paths with dynamic
this.targetreference- Handles errors gracefully with 502 responses
All previously identified issues (hardcoded URLs, missing error handling) have been addressed.
docs/backend-proxy.md (3)
18-42: LGTM! Example configuration accurately reflects the implementation.The documented proxy configuration example correctly mirrors the actual implementation in
proxy.conf.js, including the array context, sandbox target, pathRewrite rule, and both lifecycle handlers.
44-68: LGTM! Error handling is well-documented with clear examples.The documentation thoroughly explains the proxy error handling behavior, including:
- Detailed description of what the
onErrorhandler does- Server-side logging example
- Client-side 502 response example
This addresses the previously identified gap in error handling documentation.
76-89: Corporate proxy documentation is accurate and helpful.The documentation clearly explains:
- The
setupForProxyfunction's purpose and behavior- The distinction between corporate proxy forwarding and dev server proxy functionality
- That the warning message refers only to corporate proxy, not the dev server proxy
This addresses potential developer confusion about the warning message. If the warning message in
proxy.conf.js(line 79) is updated per my earlier suggestion, ensure this documentation remains aligned with the actual log message text.
|
@priyans11 my suggestion is to add a new file, with another name and put the configuration there. No need to comment the code in the same file. In the Readme just add the instruction for explaining how to use the file for the localhost. Please trust yourself and don't use the AI (I think that you are doing a lot of changes suggested by the AI) |
…host sample and README MD034 fixes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proxy.localhost.conf.js (1)
18-19: Consider extracting the pathRewrite logic to reduce duplication.The
rewrittenPathcomputation on line 18 is identical to the one inproxy.conf.jsline 20. While this duplication is minor and the files serve different purposes (localhost vs. sandbox), extracting this into a shared helper function would improve maintainability if the rewrite logic ever needs to change.🔎 Optional refactor to eliminate duplication
Create a shared helper at the top of each file:
function getRewrittenPath(url) { return (url || '').replace(/^\/fineract-provider/, ''); }Then use it in
onProxyReq:onProxyReq: function (proxyReq, req, res) { - const rewrittenPath = (req.url || '').replace(/^\/fineract-provider/, ''); + const rewrittenPath = getRewrittenPath(req.url); console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + rewrittenPath); },However, since these are separate configuration files that developers will use independently, the current duplication is acceptable.
proxy.conf.js (1)
50-61: Clarify the warning message scope.The warning on line 60 states "API requests will not be proxied," but this is misleading. The proxy configuration will still forward
/fineract-providerrequests to the sandbox—theHTTP_PROXYenvironment variable is only needed for corporate proxy scenarios where an intermediate proxy server sits between your development machine and the internet.The current message might confuse developers who see it during normal operation (when no corporate proxy is needed).
🔎 Suggested clarification
} else { - console.warn('No proxy server configured. API requests will not be proxied.'); + console.log('No corporate proxy detected. Direct connection to target will be used.'); }Or make it more explicit:
} else { - console.warn('No proxy server configured. API requests will not be proxied.'); + console.log('HTTP_PROXY not set. Direct connection to the configured target (sandbox/localhost) will be used.'); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdproxy.conf.jsproxy.localhost.conf.js
🧰 Additional context used
🧬 Code graph analysis (2)
proxy.localhost.conf.js (1)
proxy.conf.js (1)
rewrittenPath(20-20)
proxy.conf.js (1)
proxy.localhost.conf.js (1)
rewrittenPath(18-18)
🔇 Additional comments (10)
proxy.localhost.conf.js (1)
1-37: LGTM! Configuration is correct for local development.The localhost proxy configuration is well-structured with appropriate settings for development against a local Fineract instance:
secure: falseis correct for localhostpathRewriteproperly strips the/fineract-providerprefix- Error handling returns HTTP 502 with descriptive messages
- Logging provides clear debugging output
proxy.conf.js (4)
3-10: LGTM! Proper setup for corporate proxy support.The import of
HttpsProxyAgentand updated comment block correctly document the proxy configuration's purpose.
13-18: LGTM! Correct sandbox configuration.The proxy configuration properly targets the Fineract sandbox with:
- Context array for
/fineract-providerroutes- Secure connection (
secure: true) for HTTPS target- Path rewriting to strip the prefix
- Debug logging enabled
19-22: LGTM! Request logging provides useful debugging information.The
onProxyReqhook correctly logs the proxied request with the rewritten path, making it easy to troubleshoot proxy behavior during development.
23-37: LGTM! Robust error handling with appropriate HTTP status.The
onErrorhandler properly:
- Logs detailed error information including the target
- Returns HTTP 502 (Bad Gateway) when the backend is unreachable
- Checks
res.headersSentto avoid double-send errorsREADME.md (5)
135-160: LGTM! Clear and comprehensive proxy documentation.The proxy configuration section provides:
- Clear explanation of the default sandbox setup
- Correct target URL and API endpoint information
- Important warning about 6-hour data resets
- Complete environment variable reference
162-180: LGTM! Helpful guidance for local development.The localhost proxy instructions are clear and correctly explain:
- How to use the separate
proxy.localhost.conf.jsfile- That the local Fineract instance should run on port 8443
- That
setupForProxy/HttpsProxyAgentare only for corporate proxy scenariosThe note about corporate proxies on lines 177-179 appropriately clarifies when the
HTTP_PROXYenvironment variable is needed.
181-228: LGTM! Excellent testing examples and error scenarios.The proxy features and testing section provides:
- Clear list of proxy benefits
- Practical curl examples for verification
- Expected console output for successful requests
- Documented error behavior with HTTP 502 responses
The examples will help developers verify their proxy setup is working correctly.
264-264: Lithuanian locale code updated correctly.The change from
li-LItolt-LTfor Lithuanian is correct. The ISO 639-1 code for Lithuanian islt, notli.
152-153: The documented locale codes are correct—no changes needed.The locale codes
ko-KO,ne-NE, andcs-CSin the README documentation match the actual translation files insrc/assets/translations/. These are the correct ISO locale codes for Korean (South Korea), Nepali (Nepal), and Czech (Czechia) respectively, and no correction toko-KR,ne-NP, orcs-CZis required.The
FINERACT_API_PROVIDERandFINERACT_API_ACTUATORvalues are consistent across the README, environment configuration files, and docker-compose setup.Note: The README line 157 is missing
es-CLfrom the supported languages list, though the translation file exists and is included in docker-compose.yml. Consider adding it for consistency if Spanish (Chile) support is intended.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proxy.localhost.conf.js (1)
17-20: Consider removing redundant path rewrite logic.Line 18 manually replicates the
pathRewriteconfiguration from line 13. Sincehttp-proxy-middlewareautomatically appliespathRewritebefore callingonProxyReq, manually recalculatingrewrittenPathis unnecessary and violates the DRY principle. Thereq.urlpassed toonProxyReqalready reflects the rewritten path.🔎 Simplified logging without redundant rewrite
onProxyReq: function (proxyReq, req, res) { - const rewrittenPath = (req.url || '').replace(/^\/fineract-provider/, ''); - console.log('[Proxy] Proxying:', req.method, req.url, '->', this.target + rewrittenPath); + console.log('[Proxy] Proxying:', req.method, 'to', this.target + req.url); },docs/backend-proxy.md (1)
27-31: Consider documenting the redundant path rewrite.Line 29 manually replicates the
pathRewritetransformation thathttp-proxy-middlewareautomatically applies before callingonProxyReq. While this may reflect the current implementation inproxy.conf.js, the documentation could note thatreq.urlalready contains the rewritten path, making the manual replacement unnecessary.Alternative simplified approach
If this example is updated (either here or in the actual
proxy.conf.js), consider:onProxyReq: function (proxyReq, req, res) { // req.url already reflects pathRewrite transformation console.log('[Proxy] Proxying:', req.method, 'to', this.target + req.url); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mddocs/backend-proxy.mdproxy.conf.jsproxy.localhost.conf.js
🚧 Files skipped from review as they are similar to previous changes (1)
- proxy.conf.js
🧰 Additional context used
🧬 Code graph analysis (1)
proxy.localhost.conf.js (1)
proxy.conf.js (1)
rewrittenPath(20-20)
🔇 Additional comments (5)
proxy.localhost.conf.js (1)
21-35: LGTM!The error handling is well-implemented with defensive checks for
resand!res.headersSent, ensuring graceful degradation when proxy failures occur.README.md (2)
162-180: LGTM!Clear, actionable instructions for local Fineract development. The note about
HttpsProxyAgent/setupForProxy(lines 178-179) appropriately clarifies when corporate proxy logic is needed versus direct localhost access.
190-228: Excellent testing documentation.The curl examples with expected outputs (both success and failure scenarios) provide practical, copy-paste-ready commands for developers to verify proxy behavior. The detailed console output examples are particularly helpful for troubleshooting.
docs/backend-proxy.md (2)
48-68: LGTM!Comprehensive error handling documentation with concrete examples of both server-side logging and client-side responses. This addresses the previous review feedback requesting
onErrordocumentation.
77-86: LGTM!The documentation clearly distinguishes between corporate proxy forwarding (controlled by
HTTP_PROXY) and the Angular dev server proxy (always active). Lines 83-84 effectively clarify that the warning message scope is limited to corporate proxy forwarding, addressing potential developer confusion.
@IOhacker thanks for the clarification. I ve made the changes as suggested by separating the configurations into a dedicated file and updating the README accordingly. |
2 participants
Summary
This PR adds a development proxy that forwards
/fineract-providerrequests to the Mifos sandbox and documents how it works. It enables local frontend development against the remote Fineract sandbox or a local Fineract instance, improves request logging, and makes error handling more robust. It also includes a ready-to-use localhost proxy sample and corporate-proxy support.What Changed
Proxy (
proxy.conf.js)/fineract-provider→https://sandbox.mifos.community.pathRewriteso requests map to the sandbox root.onError:setupForProxyto support corporate HTTP(S) proxies viaHTTPS_PROXY/HTTP_PROXY(usesHttpsProxyAgentwhen set).http://localhost:8443behavior.Localhost proxy (
proxy.localhost.conf.js) — newhttp://localhost:8443for local Fineract development.pathRewrite, andonErrorbehavior as sandbox config.ng serve --proxy-config proxy.localhost.conf.js
Documentation
README.mdcurlexamples:setupForProxy/HttpsProxyAgentis needed (corporate proxy only).backend-proxy.mdonErrorbehavior with example server logs and client 502 responses.setupForProxyandHTTP_PROXYbehavior.Files Changed
proxy.conf.js(updated)proxy.localhost.conf.js(added)README.md(updated)backend-proxy.md(updated)Testing / How to Verify
Start frontend using the localhost proxy
Successful proxied request
Simulated failure
Corporate proxy
HTTP_PROXY/HTTPS_PROXYenvironment variable and verify outbound proxying uses the upstream proxy(logs the Using proxy server: message).
Notes
proxy.localhost.conf.jsfor local dev onlocalhost:8443;setupForProxyis only required for corporate upstream proxies.Jira