Conversation
Drop the `location /governance` and `location /masternodes` blocks: the backend doesn't expose those paths (they're client-side React routes), so proxying them to :3001 would 404 the SPA pages users navigate to. Fix `/mnstats` casing to `/mnStats` to match the backend route + SPA call. Add `/govlist` (the SPA polls it via `client.post('/govlist', [])`). Switch `/vault` from prefix to exact match so client-side routes like `/vault/import` still fall through to the SPA.
Made-with: Cursor
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a566c4efa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+64
to
+68
| # falls through to the SPA below. If you add a new public backend | ||
| # route that the SPA polls, add it here too. | ||
| location = /mnStats { proxy_pass http://127.0.0.1:3001; include /etc/nginx/snippets/sysnode-proxy.conf; } | ||
| location = /mnCount { proxy_pass http://127.0.0.1:3001; include /etc/nginx/snippets/sysnode-proxy.conf; } | ||
| location = /govlist { proxy_pass http://127.0.0.1:3001; include /etc/nginx/snippets/sysnode-proxy.conf; } |
There was a problem hiding this comment.
This block now proxies only /mnStats, /mnCount, and /govlist, but the same file states the backend’s public routes also include /mnSearch and /mnList (and server.js mounts both routers). In a same-origin deployment based on this example, requests to /mnSearch and /mnList will fall through to the SPA catch-all instead of reaching port 3001, returning the wrong content type/body for API callers.
Useful? React with 👍 / 👎.
| # Public anonymous data routes. Each is matched exactly so /<random> | ||
| # falls through to the SPA below. If you add a new public backend | ||
| # route that the SPA polls, add it here too. | ||
| location = /mnStats { proxy_pass http://127.0.0.1:3001; include /etc/nginx/snippets/sysnode-proxy.conf; } |
There was a problem hiding this comment.
Changing the proxy entry to exact location = /mnStats drops support for the previously proxied lowercase /mnstats path at the nginx layer. Existing lowercase callers (including operators following current README smoke-test examples) will now hit the frontend catch-all and receive HTML/non-API responses instead of backend JSON in this same-origin layout.
Useful? React with 👍 / 👎.
Three related issues raised on the PR #23 review and traced down to real code-level bugs while resolving them: 1. Stale-reference bug in /mnsearch (was /mnSearch): routes/mnSearch.js destructured `masternodesArr` at require time: const { masternodesArr } = require("../data/dataStore"); `services/masternodeTracker.js` REASSIGNS `dataStore.masternodesArr = []` every 10 seconds and then pushes nodes into the fresh array, so the captured reference forever pointed at the original empty `[]` from data/dataStore.js. /mnsearch always returned `{ returnArr: [], mnNumb: 0 }` in production. Fix: require the dataStore module and read `dataStore.masternodesArr` on every request, mirroring the `masternodesProvider` arrow function in server.js (which already handled this correctly for /gov/mns/lookup). Defensive: tolerate a non-array value (tracker mid-write / RPC failure). 2. Duplicate route registration: `POST /mnSearch` was registered in BOTH routes/mnSearch.js and routes/masternodes.js. The latter was effectively unreachable (and carried the same stale-reference bug). Delete routes/masternodes.js and its mount in server.js. 3. Error-message leakage in /mnlist (was /mnList): On RPC failure the route returned `{ error: err.message }` to the client, leaking internal hostnames/ports (e.g. "ECONNREFUSED 127.0.0.1:8370"). Mirror routes/governance.js: log full detail server-side via lib/securityLog (`mnList.rpc_failed`), return an opaque `{ error: 'internal' }` 500 to the client. 4. Standardize public route casing on lowercase: The historical canonical URL is lowercase (syshub still calls `https://syscoin.dev/mnstats` lowercase, and `/govlist` is already lowercase). The camelCase appeared only because the route files registered `router.get("/mnStats", …)`; Express's default case-insensitive routing hid the drift. Rename URL paths to lowercase across routes/{mnStats,mnList,mnSearch,mnCount}.js. JS file names kept as-is (internal). All existing camelCase callers continue to work at the route layer thanks to Express's case-insensitive default. nginx example: replace four `location =` exact-match blocks with a single case-INsensitive regex `~* ^/(mnstats|mncount|mnlist| mnsearch|govlist)$` so legacy bookmarks and external integrations using camelCase URLs keep working through the same-origin proxy — this mirrors Express's own forgiveness at the edge. The anchored `^/...$` keeps SPA paths like `/mnsearch/results` falling through to the React build. README "Public, unauthenticated routes" section now lists the lowercase canonical names and explains the case-insensitive compatibility for legacy callers. Tests: - New tests/mnSearch.routes.test.js pins the live-dataStore-read contract (including a tracker-style array reassignment, plus filtering/pagination regressions) and the defensive non-array path. - New tests/mnList.routes.test.js pins the 200-pass-through and the 500-with-opaque-body-and-server-side-log contract. - Existing mnCount + server.smoke tests updated to lowercase URLs. 950 jest tests pass. Made-with: Cursor
sidhujag
changed the title
4 tasks
…actor
The previous commit's sed substitution went too far — it lowercased
not only the URL string in the request paths but also the JS module
specifiers in require() calls:
require('../routes/mnSearch') → require('../routes/mnsearch')
require('../routes/mnList') → require('../routes/mnlist')
require('./mnCount') → require('./mncount')
The route file *names* were intentionally kept camelCase (they're
internal module identifiers, not URL paths). On a case-insensitive
filesystem (macOS APFS default) the broken require() paths still
resolve to the correct file so the local jest run was green; on the
case-sensitive Linux filesystem used by CI the resolver fails with:
Cannot find module '../routes/mnsearch' from 'tests/mnSearch.routes.test.js'
Restore the camelCase module specifiers. URL strings inside the
request bodies (e.g. `request(app).post('/mnsearch')`) stay
lowercase — those are the canonical we just standardized on.
Made-with: Cursor
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
sidhujag
pushed a commit
to syscoin/sysnode-info
that referenced
this pull request
Apr 26, 2026
Companion to syscoin/sysnode-backend#23. The historical canonical URL for the public sysnode-backend endpoints is lowercase: syshub still calls `https://syscoin.dev/mnstats` lowercase, and `/govlist` was already lowercase. The camelCase only appeared because the backend route files registered `router.get("/mnStats", …)`; Express's default case-insensitive routing hid the drift across both ends. Pin the SPA on the lowercase canonical so future readers and external integrators see one consistent shape: - src/lib/api.js: client.get('/mnstats'), client.get('/mncount'). - README data-source layout block updated. - Comments in NewProposal.{js,test.js} and governanceWindow.{js,test.js} refreshed to use the lowercase path names. (Behaviourally these are documentation refs; the request URL change is in api.js.) Backwards compat: the backend's case-insensitive default routing AND the case-insensitive `~*` regex match in the bundled nginx example (syscoin/sysnode-backend#23) mean any in-flight build still using camelCase keeps working during a rolling deploy. New code paths land on lowercase from the start. 880 jest tests pass. Made-with: Cursor
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
1 participant
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.
Summary
Three real code bugs surfaced while resolving the round-1 review on this PR (the original commit just aligned the bundled nginx example with the actual same-origin route layout). They've all been fixed here, plus a small consistency cleanup so we don't keep arguing about casing in future reviews.
Code fixes
/mnsearch:routes/mnSearch.jsdestructuredmasternodesArrat require time, butservices/masternodeTracker.jsreassignsdataStore.masternodesArr = []every 10 seconds and pushes into the fresh array. The captured reference forever pointed at the original empty[], so/mnsearchalways returned{ returnArr: [], mnNumb: 0 }in production. Fix: readdataStore.masternodesArron every request (matches themasternodesProviderarrow function already used byserver.jsfor the same reason).POST /mnSearchwas registered in bothroutes/mnSearch.jsandroutes/masternodes.js. The latter was effectively unreachable AND carried the same stale-ref bug. Deleted./mnlist: route returned{ error: err.message }on RPC failure, leaking internal hostnames/ports (e.g.ECONNREFUSED 127.0.0.1:8370). Now mirrorsroutes/governance.js: full detail logged server-side vialib/securityLog(mnList.rpc_failed), opaque{ error: 'internal' }500 to the client.Casing standardization
The historical canonical URL is lowercase —
syshubstill callshttps://syscoin.dev/mnstatslowercase and/govlistwas already lowercase. The camelCase only appeared because the route files registeredrouter.get("/mnStats", …); Express's default case-insensitive routing hid the drift.routes/{mnStats,mnList,mnSearch,mnCount}.js. JS file names kept as-is (internal namespace).deploy/nginx/sysnode.conf.examplenow uses a single case-insensitive regex match (~* ^/(mnstats|mncount|mnlist|mnsearch|govlist)$) so legacy bookmarks and external integrations using camelCase URLs keep working through the same-origin proxy — mirrors Express's forgiveness at the edge. The anchored^/...$keeps SPA paths like/mnsearch/resultsfalling through to the React build.Tests
tests/mnSearch.routes.test.jspins the live-dataStore-read contract (incl. a tracker-style reassignment, filtering, IP/payee match, pagination, and non-array defensiveness).tests/mnList.routes.test.jspins the 200 pass-through and the 500-with-opaque-body-and-server-side-log contract.950 jest tests pass.
Test plan
npm test(950/950)curl http://127.0.0.1:3001/mnstatsandcurl http://127.0.0.1:3001/mnStatsboth return JSON post-deployhttps://sysnode.dev11.top/mnstatsandhttps://sysnode.dev11.top/mnStatsboth proxy to backend after live nginx is updated to mirror the example