refactor(indexer): replace express healthcheck server with hono - #3555
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe indexer replaces Express with Hono. It updates dependencies, route handlers, error responses, server startup, and documentation while preserving endpoint payloads and status behavior. ChangesIndexer Hono migration
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/indexer/package.jsonESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/indexer/src/index.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3555 +/- ##
=======================================
Coverage 75.25% 75.25%
=======================================
Files 1163 1163
Lines 30165 30165
Branches 7502 7504 +2
=======================================
+ Hits 22700 22702 +2
+ Misses 6591 6589 -2
Partials 874 874
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean, mechanical swap of Express for Hono in the indexer's healthcheck server.
Extended reasoning...
Overview
This PR touches only apps/indexer/package.json, apps/indexer/src/index.ts, and the lockfile. It replaces the Express-based /status and /nodes healthcheck endpoints with an equivalent Hono app served via @hono/node-server, and drops the express/@types/express dependencies. The route logic itself is unchanged — res.send(...) becomes c.json(...), res.status(500).send(...) becomes c.text(..., 500), and app.listen becomes serve({ fetch: app.fetch, port }). This matches the framework already used elsewhere in the monorepo (apps/api, apps/provider-proxy, apps/tx-signer), so it's a consistency-motivated refactor rather than a behavior change.
Security risks
None. This is an internal healthcheck endpoint (status/nodes reporting), not user input handling, auth, or crypto. No new attack surface is introduced by the framework swap.
Level of scrutiny
Low. The diff is small, mechanical, and 1:1 in behavior — same two GET routes, same response bodies, same status codes, same port binding. It's a dependency swap plus direct API translation, not a design change requiring a human decision.
Other factors
The one finding from the bug-hunting pass is a documentation nit (CLAUDE.md/architecture.md still say "Express" for apps/indexer) — not a functional bug, and doesn't warrant blocking this PR.
There was a problem hiding this comment.
LGTM — straightforward Express-to-Hono swap for the indexer healthcheck server, consistent with the rest of the monorepo already standardizing on Hono.
What was reviewed: the /status and /nodes route handlers (response bodies and error paths preserved: res.send→c.json, res.status(500).send(...)→c.text(..., 500)), the app.listen→serve({ fetch: app.fetch, port }) startup swap, the package.json dependency swap (express/@types/express removed, hono/@hono/node-server added), and the doc updates in CLAUDE.md and doc/architecture.md, which now correctly say "Hono" instead of "Express" (this addresses the stale-docs nit from my previous run on this PR).
Extended reasoning...
Overview
This PR replaces the Express-based healthcheck server in apps/indexer with a Hono app served via @hono/node-server, dropping the express and @types/express dependencies. Only /status and /nodes GET routes are affected; all other indexer logic (scheduler, sync tasks, DB access) is untouched.
Security risks
None. This is an internal healthcheck endpoint with no auth, input parsing, or user-controlled data — the change is a like-for-like framework swap with identical response payloads and error-handling behavior (500 + "An error occurred" on exception, same JSON bodies on success).
Level of scrutiny
Low. This is a mechanical, well-scoped refactor (XS size label) that follows an established pattern already used in apps/api, apps/provider-proxy, apps/tx-signer, and apps/provider-inventory, all of which use Hono. The diff is small, self-contained, and the behavior-preservation is easy to verify by direct comparison of the before/after handlers.
Other factors
CI (Codecov) reports all tests passing with no coverage regression. The package-lock.json diff is a clean dependency swap with no unexpected transitive changes. The previously-flagged stale documentation (CLAUDE.md and doc/architecture.md still describing the indexer as Express-based) has already been corrected in this diff.
8dc5dc0 to
4907a8a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
LGTM. The stale docs I flagged previously (CLAUDE.md and doc/architecture.md still referencing Express) have been updated to reference Hono in this revision.
Extended reasoning...
Overview
This PR swaps the indexer's Express-based /status and /nodes healthcheck routes for Hono served via @hono/node-server, dropping express/@types/express. Changes are confined to apps/indexer/src/index.ts (route handlers + server startup), apps/indexer/package.json (dependency swap), package-lock.json, and two doc files (CLAUDE.md, doc/architecture.md).
Security risks
None. This is an internal healthcheck server (no auth, no user-facing data), and the route logic (fetch task/sync status, catch-wrap into a 500 text response) is preserved 1:1 from the Express version.
Level of scrutiny
Low. This is a well-scoped, mechanical framework substitution in a non-critical internal endpoint (indexer healthcheck), sized XS, with response payloads and error handling kept behaviorally identical to the original Express implementation.
Other factors
My earlier inline nit about stale Express references in CLAUDE.md/doc/architecture.md has since been fixed in this revision (both now say Hono). No tests exist for these healthcheck routes in either the before or after state, which is consistent with the endpoint's low-stakes internal-tooling nature. No bugs were found by the bug hunting system on this run.
|
it's safe to merge with this snyk error because indexer web server is not publicly available |
4907a8a to
f811a15
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
LGTM — straightforward, mechanical migration from Express to Hono for the indexer healthcheck server.
Extended reasoning...
Overview
This PR swaps the indexer's internal /status and /nodes healthcheck endpoints from Express to Hono + @hono/node-server, matching the framework already used by apps/api, apps/provider-proxy, apps/provider-inventory, and apps/tx-signer. The route handlers are a direct 1:1 translation (res.send(...) → c.json(...), res.status(500).send(...) → c.text(..., 500)), server startup switches from app.listen to serve({ fetch: app.fetch, port }), and express/@types/express are fully removed from package.json. Documentation (CLAUDE.md, doc/architecture.md) is updated in the same PR to reflect the new stack.
Security risks
None identified. This is an internal healthcheck server (confirmed by the author as not publicly exposed), and the change preserves existing response payloads, error handling, and Sentry reporting exactly.
Level of scrutiny
Low — this is a well-precedented, mechanical framework substitution with no behavior change, small diff surface (one source file plus deps/docs), and CI (tests, coverage) passing.
Other factors
A nit I raised in a prior run (stale Express references in CLAUDE.md/doc/architecture.md) has already been fixed in the current diff. No other findings from this run.
Swap the Express-based status/nodes healthcheck endpoints for a Hono app served via @hono/node-server, dropping the express and @types/express deps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the express->hono webserver swap: update the architecture docs and project description that still described the indexer as Express. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f811a15 to
f461015
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
LGTM — the prior doc staleness nit (CLAUDE.md / doc/architecture.md still saying Express) has been fixed in this revision, both now correctly say Hono.
Extended reasoning...
Overview
This PR swaps the indexer's internal healthcheck server (/status, /nodes) from Express to Hono + @hono/node-server, matching the framework already used by apps/api, apps/provider-proxy, and apps/provider-inventory. The route handlers, response payloads, Sentry error reporting, and 500-error text are preserved 1:1 — only the framework glue (req/res → c.json/c.text, app.listen → serve({ fetch: app.fetch, ... })) changed. Docs (CLAUDE.md, doc/architecture.md) and package.json/package-lock.json were updated consistently to drop express/@types/express and add hono/@hono/node-server.
Security risks
None. This is an internal healthcheck/status server; the author confirmed on the PR thread that it is not publicly exposed, which also addresses the earlier Snyk advisory noise. No auth, crypto, or data-access code is touched.
Level of scrutiny
Low. This is a mechanical, same-behavior framework swap in a small, non-public surface, consistent with the pattern already used elsewhere in the monorepo. CI (tests, coverage, lint) is green and the diff is easy to verify line-by-line against the old Express handlers.
Other factors
I left a nit on a previous revision about CLAUDE.md and doc/architecture.md still describing the indexer as Express-based; both have since been corrected to say Hono, so that's resolved. No bugs were found by the bug hunting system on this run, and nothing else stood out in apps/indexer/src/index.ts or the dependency changes.
2 participants
Why
Consistency. Closes CON-790
What
Swap the Express-based status/nodes healthcheck endpoints for a Hono app served via @hono/node-server, dropping the express and @types/express deps.
Summary by CodeRabbit
Refactor
Bug Fixes
/statusand/nodesendpoints continue returning the same information.Documentation