Eliminate remaining runtime and maintenance debt - #108
Conversation
Server-side session shutdown did not prove that a same-process public Relay had observed its disconnect. Coordinate exact-URL observers at the WebSocket boundary, gate notification on owned transport shutdown, and make close finalization idempotent across natural, coordinated, manual, and late callback paths.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR consolidates command references, unifies diagnostic logger contracts, hardens relay and NIP client lifecycle cleanup, removes stale example surfaces, and adds public-behavior and compatibility tests across Node, Jest, and Bun. ChangesRuntime debt cleanup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
|
CI follow-up: the first hosted matrix exposed a Node 16-only test portability gap. Node 16 has no built-in Fresh verification:
The previous Node 20 job had already passed all 72 suites / 991 tests before the matrix was canceled by fail-fast from Node 16. The pushed fix has started a fresh hosted matrix. |
|
Second CI portability follow-up: Node 16 lacks Fresh verification:
Node 18's prior rerun completed all 72 suites / 991 tests before matrix fail-fast canceled the job. A fresh hosted matrix is starting from |
|
Final gate status:
PR head |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nip01/relay.ts (1)
243-264: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInvalidate the old attempt before emitting
Disconnect.
triggerEvent()is synchronous. If a pre-connect disconnect handler callsconnect(), Lines 259-262 subsequently clear and invalidate that new attempt. Complete the old attempt’s cleanup before invoking user callbacks.Proposed ordering fix
const wasConnected = this.connected; this.connected = false; this.clearBufferFlush(); - this.triggerEvent(RelayEvent.Disconnect, this.url); this.clearRelayDisconnectObserver(attemptId); this.clearRelayDisconnectFinalizer(attemptId); if (!wasConnected) { this.detachSocketHandlers(socket); this.connectionPromise = null; this.connectionAttempt++; reject(new Error("connection closed")); } + + this.triggerEvent(RelayEvent.Disconnect, this.url);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nip01/relay.ts` around lines 243 - 264, In finalizeClose, complete all old-attempt cleanup—including clearing connection state, detaching handlers, nulling connectionPromise, and incrementing connectionAttempt—before calling triggerEvent(RelayEvent.Disconnect, this.url). Preserve the existing guards and rejection behavior, while ensuring a synchronous handler that calls connect() is not affected by subsequent cleanup from the closed attempt.
🧹 Nitpick comments (1)
src/utils/websocket.ts (1)
14-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared relay URL normalizer for observer keys.
This local implementation creates a second relay canonicalization contract. Reuse the existing relay URL normalization utility so observer registration and notification remain consistent with the rest of the library.
As per coding guidelines, “Use the existing relay URL normalization utilities when working with relay URLs.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/websocket.ts` around lines 14 - 20, Replace the local normalizeWebSocketUrl implementation with the existing shared relay URL normalization utility, and update its callers to use that shared symbol for observer keys. Remove the duplicate try/catch canonicalization logic while preserving consistent normalization for registration and notification.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/nip02/nip02.test.ts`:
- Line 367: Remove the duplicate warnings constant near the nip02 test setup,
retaining only one declaration of the Array<{ message: string; value?: string }>
variable so the test compiles without changing its usage.
In `@tests/nip57/client.test.ts`:
- Around line 141-237: The setup in both tests should occur inside the
try/finally cleanup scope, before mutating globalThis.Bun or starting the relay,
so failures during setup are covered. Update the cleanup to conditionally
restore Bun and close/disconnect only initialized resources, including relay and
nostr; apply this to the empty-receipts test and “collects stored receipts
through every public receipt query.”
---
Outside diff comments:
In `@src/nip01/relay.ts`:
- Around line 243-264: In finalizeClose, complete all old-attempt
cleanup—including clearing connection state, detaching handlers, nulling
connectionPromise, and incrementing connectionAttempt—before calling
triggerEvent(RelayEvent.Disconnect, this.url). Preserve the existing guards and
rejection behavior, while ensuring a synchronous handler that calls connect() is
not affected by subsequent cleanup from the closed attempt.
---
Nitpick comments:
In `@src/utils/websocket.ts`:
- Around line 14-20: Replace the local normalizeWebSocketUrl implementation with
the existing shared relay URL normalization utility, and update its callers to
use that shared symbol for observer keys. Remove the duplicate try/catch
canonicalization logic while preserving consistent normalization for
registration and notification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b27be20-d36d-4805-a844-f95c04ff6fe1
📒 Files selected for processing (54)
CLAUDE.mdREADME.mddocs/adr/0002-unify-diagnostics-compatibly.mddocs/agents/runs/feature-cleanup-runtime-debt-ledger.mddocs/agents/runs/final-cleanup-review-session.mddocs/agents/runs/integrated-bun-compat-session.mddocs/agents/runs/issue-101-session.mddocs/agents/runs/issue-102-loaded-suite-followup-session.mddocs/agents/runs/issue-102-session.mddocs/agents/runs/issue-103-session.mddocs/agents/runs/issue-104-session.mddocs/agents/runs/issue-105-session.mddocs/agents/runs/issue-106-review-packet.mddocs/agents/runs/issue-106-session.mddocs/agents/runs/issue-107-session.mdexamples/EXAMPLE_STANDARDIZATION.mdexamples/README.mdexamples/nip19/README.mdexamples/nip44/nip44-demo.jspackage.jsonscripts/verify-commands.jssrc/entries/index.web.tssrc/index.tssrc/nip01/relay.tssrc/nip02/README.mdsrc/nip02/index.tssrc/nip47/README.mdsrc/nip47/client.tssrc/nip47/service.tssrc/nip47/types.tssrc/nip57/README.mdsrc/nip57/client.tssrc/nip57/diagnostics.tssrc/nip57/index.tssrc/nip57/utils.tssrc/nip86/index.tssrc/shims/nip04-stub.tssrc/utils/ephemeral-relay.tssrc/utils/logger.tssrc/utils/websocket.tstests/README.mdtests/TEST_STANDARDIZATION.mdtests/entries/web-exports.test.tstests/nip01/relay/websocket-implementation.test.tstests/nip02/nip02.test.tstests/nip57/client.test.tstests/nip86/nip86.test.tstests/scripts/verify-commands.test.tstests/security/security-limits-public.test.tstests/types/logger-compatibility.tstests/utils/ephemeral-relay-close-ordering.test.tstests/utils/ephemeral-relay-lifecycle.test.tstests/utils/security-validator.test.tstests/utils/websocket.test.ts
💤 Files with no reviewable changes (2)
- src/shims/nip04-stub.ts
- examples/nip44/nip44-demo.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Three-pass CodeRabbit review complete:
Final CI is green on Node 16, Node 18, Node 20, and Bun. Focused Jest/Bun NIP-57 tests, lint, TypeScript, and diff checks also passed locally. Grok orchestration was preflighted as requested, but the required |
1 participant
Summary
Completes cleanup items 1–7 from #100:
Implementation tickets: #101, #102, #103, #104, #105, #106, #107.
Design and run artifacts
docs/adr/0002-unify-diagnostics-compatibly.mddocs/agents/runs/feature-cleanup-runtime-debt-ledger.mddocs/agents/runs/integrated-bun-compat-session.mddocs/agents/runs/final-cleanup-review-session.mdVerification
npm ciexample:basicandexample:messaginggroupssecurity-validator.ts: 86.12% statements, 80.00% branches, 84.37% functions, 85.99% linesgit diff --checkReview
Risks
npm ciretains the existing audit baseline of 13 dependency vulnerabilities (2 low, 3 moderate, 8 high). Dependency locks were intentionally not changed and no potentially breaking audit fix was applied.Closes #100
Summary by CodeRabbit
New Features
ConsoleLogger/DiagnosticLoggerexports across platforms.WarningLogger(kept as a deprecated alias for compatibility).Bug Fixes
Documentation
Tests