Remove dead duplicate webhook path, add ESLint, unhide 3 test files from tsc - #404
Merged
Merged
Conversation
Investigating the "unwired" dispatchMilestoneWebhook turned up something
different from what I reported last time: it is not missing wiring, it is
a second webhook system competing with one that already works.
The live path
-------------
src/indexer/webhook-delivery.ts -> deliverWebhooks() is imported by
src/indexer/poller.ts and runs after every poll. It filters by
contract_id, filters by event_types with the same "*" / JSON-array /
literal logic, retries with backoff, and logs. The public subscribe
endpoint (POST /api/webhooks/subscribe) feeds it via addSubscription.
The dead path
-------------
src/webhooks/{dispatcher,deliver,milestone-events}.ts reimplements the
same filtering with a milestone-shaped payload. Nothing outside that
directory imports any of it. Wiring it up would have double-delivered
every event in two different shapes -- a regression, not a fix. Removed.
That takes the milestone-events prototype-chain fix from #403 with it.
That fix was correct, but the module it fixed should not exist; the live
path never had the bug because it does not map event names to statuses.
Three exported functions that always threw
------------------------------------------
db.ts declared CREATE TABLE IF NOT EXISTS webhook_subscriptions twice
with different schemas. The per-contract one is declared first, so it
wins and the second never runs. It also declared interface
WebhookSubscription twice; TypeScript merges duplicate interfaces, so the
type described neither table.
addWebhookSubscription, removeWebhookSubscription and
getWebhookSubscriptions all query a `url` column that therefore does not
exist. Verified against a real database:
columns: id, contract_id, webhook_url, event_types, created_at
addSubscription -> OK
addWebhookSubscription -> THROWS: table has no column named url
getWebhookSubscriptions -> THROWS: no such column: url
removeWebhookSubscription -> THROWS: no such column: url
Nothing calls them. Removed, along with the dead table declaration and
the duplicate interface.
ESLint
------
The backend had no linter and no lint step. Added eslint 9 +
typescript-eslint with the type-aware parser, `npm run lint`, and a CI
step before the type check.
Calibrated so it is green today and useful tomorrow: rules that catch
defects tsc cannot are errors -- no-prototype-builtins (exactly the `in`
bug above, which type-checked fine), guard-for-in, no-floating-promises,
await-thenable, no-useless-catch, prefer-const, eqeqeq. Pre-existing debt
(no-unused-vars, no-unsafe-call, no-explicit-any) is warnings: 128 of
them, mostly Stellar SDK call sites in routes/jobs.ts that need a typing
pass rather than a rushed one. 0 errors, so the gate is real.
Fixed the 4 it found: 3 no-op `catch (e) { throw e }` wrappers and a
prefer-const.
Worth being precise about scope: this would not have stopped #401. That
branch failed `tsc` -- the corruption was already caught, and it was
merged anyway. A linter helps with what type-checking misses, not with
merging past a red build.
Also unhidden
-------------
tsconfig excluded three test suites from type checking:
ledger-range-tracker-improvements, indexer-metrics-collector-concurrency
and failover-recovery-backoff-retry. They run under jest but tsc never
saw them. Removed the exclusions -- they type-check with zero errors, so
the exclusion was hiding nothing but could have hidden anything. Same
class as the jest testPathIgnorePatterns removed in #402. Added
jest.setup.ts to include for the same reason.
lint 0 errors, tsc 0 errors, 1612 tests pass, build clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new ESLint config relies on import.meta.dirname (not broadly supported across Node versions) and the lockfile marks the chosen ESLint version as deprecated, both of which can destabilize the new CI lint gate.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes an unused/duplicate webhook delivery implementation (and its associated tests and broken DB helpers), adds ESLint (with a CI lint gate), and broadens TypeScript compilation coverage to include previously excluded test files (plus jest.setup.ts).
Changes:
- Deleted the unused
src/webhooks/*milestone webhook pipeline to avoid competing webhook delivery paths. - Removed a dead/incorrect
webhook_subscriptionstable definition and unused DB helpers that targeted a non-existenturlcolumn. - Added ESLint +
npm run lintand a CI lint step; updatedtsconfig.jsoninclude/exclude so test files andjest.setup.tsare type-checked.
File summaries
| File | Description |
|---|---|
| tsconfig.json | Includes jest.setup.ts and removes exclusions so more tests are type-checked. |
| eslint.config.mjs | Adds ESLint flat config + TypeScript-ESLint ruleset and test overrides. |
| .github/workflows/ci.yml | Adds npm run lint step before tsc in CI. |
| package.json | Adds lint script and ESLint + TypeScript-ESLint dev dependencies. |
| package-lock.json | Locks new ESLint / TypeScript-ESLint dependency graph. |
| src/indexer/db.ts | Removes duplicate webhook_subscriptions table and unused global-subscription helpers. |
| src/webhooks/milestone-events.ts | Deleted (unused milestone webhook event/type logic). |
| src/webhooks/dispatcher.ts | Deleted (unused dispatcher entrypoint). |
| src/webhooks/deliver.ts | Deleted (unused delivery implementation). |
| tests/rpc-poller-client.test.ts | Removes no-op catch { throw } wrappers. |
| tests/milestone-webhook-events.test.ts | Deleted (tests for removed milestone webhook module). |
| tests/database-writer-pool.test.ts | Minor prefer-const cleanup. |
Review details
- Files reviewed: 10/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1
to
+23
| import js from "@eslint/js"; | ||
| import tseslint from "typescript-eslint"; | ||
|
|
||
| export default tseslint.config( | ||
| { | ||
| ignores: [ | ||
| "dist/**", | ||
| "node_modules/**", | ||
| "coverage/**", | ||
| "eslint.config.mjs", | ||
| "jest.config.js", | ||
| "verify-ci.js", | ||
| ], | ||
| }, | ||
| js.configs.recommended, | ||
| ...tseslint.configs.recommended, | ||
| { | ||
| languageOptions: { | ||
| parserOptions: { | ||
| projectService: true, | ||
| tsconfigRootDir: import.meta.dirname, | ||
| }, | ||
| }, |
Comment on lines
+33
to
42
| "@eslint/js": "^9.39.5", | ||
| "@types/better-sqlite3": "^7.6.13", | ||
| "@types/cors": "^2.8.19", | ||
| "@types/express": "^5.0.6", | ||
| "@types/jest": "^30.0.0", | ||
| "@types/node": "^25.9.4", | ||
| "@types/supertest": "^7.2.0", | ||
| "cross-env": "^10.1.0", | ||
| "eslint": "^9.39.5", | ||
| "jest": "^30.4.2", |
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
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.
Investigating the "unwired"
dispatchMilestoneWebhookturned up something different from what I reported last time: it isn't missing wiring, it's a second webhook system competing with one that already works.The live path
src/indexer/webhook-delivery.ts→deliverWebhooks()is imported bysrc/indexer/poller.tsand runs after every poll. It filters bycontract_id, filters byevent_typeswith the same*/ JSON-array / literal logic, retries with backoff, and logs. The publicPOST /api/webhooks/subscribeendpoint feeds it viaaddSubscription.The dead path
src/webhooks/{dispatcher,deliver,milestone-events}.tsreimplements the same filtering with a milestone-shaped payload. Nothing outside that directory imports any of it. Wiring it up would have double-delivered every event in two different shapes — a regression, not a fix. Removed.That takes the prototype-chain fix from #403 with it. That fix was correct, but the module it fixed shouldn't exist; the live path never had the bug because it doesn't map event names to statuses.
Three exported functions that always threw
db.tsdeclaredCREATE TABLE IF NOT EXISTS webhook_subscriptionstwice with different schemas. The per-contract one is declared first, so it wins and the second never runs. It also declaredinterface WebhookSubscriptiontwice — TypeScript merges duplicate interfaces, so the type described neither table.addWebhookSubscription,removeWebhookSubscriptionandgetWebhookSubscriptionsall query aurlcolumn that therefore doesn't exist. Verified against a real database:Nothing calls them. Removed, with the dead table declaration and duplicate interface.
ESLint
The backend had no linter and no lint step. Added eslint 9 + typescript-eslint with the type-aware parser,
npm run lint, and a CI step before the type check.Calibrated to be green today and useful tomorrow. Errors — rules that catch what
tsccannot:no-prototype-builtins(exactly theinbug above, which type-checked fine),guard-for-in,no-floating-promises,await-thenable,no-useless-catch,prefer-const,eqeqeq. Warnings — pre-existing debt:no-unused-vars,no-unsafe-call,no-explicit-any, 128 of them, mostly Stellar SDK call sites inroutes/jobs.tsthat need a proper typing pass. 0 errors, so the gate is real.Fixed the 4 it found: three no-op
catch (e) { throw e }wrappers and aprefer-const.Worth being precise about scope: this would not have stopped #401. That branch failed
tsc— the corruption was already caught, and it was merged anyway. A linter helps with what type-checking misses, not with merging past a red build.Also unhidden
tsconfigexcluded three test suites from type checking —ledger-range-tracker-improvements,indexer-metrics-collector-concurrency,failover-recovery-backoff-retry. They run under jest buttscnever saw them. Removed the exclusions: they type-check with zero errors, so the exclusion was hiding nothing — but could have hidden anything. Same class as the jesttestPathIgnorePatternsremoved in #402. Addedjest.setup.tsfor the same reason.lint 0 errors, tsc 0 errors, 1612 tests pass, build clean.
🤖 Generated with Claude Code