Staging - #109
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.
Eliminate remaining runtime and maintenance debt
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR consolidates command documentation and verification, introduces shared diagnostic logger contracts with compatibility aliases, hardens relay and NIP client lifecycle behavior, adds public security and compatibility tests, and removes stale example and shim surfaces. ChangesRuntime cleanup and contracts
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/utils/websocket.ts (1)
14-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the canonical relay URL normalizer.
This local implementation creates a second normalization contract, risking observer-key mismatches with other relay operations. Replace it with the existing utility.
As per coding guidelines, “Use the existing relay URL normalization utilities for relay operations.”
🤖 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 canonical relay URL normalization utility, and update its callers to use that shared symbol. Remove the duplicate try/catch normalization logic while preserving the current WebSocket URL normalization behavior.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 `@docs/agents/runs/feature-cleanup-runtime-debt-ledger.md`:
- Line 12: Update the PR metadata references in the document’s current-status
entries to identify PR `#109` as the active staging-to-main review. Explicitly
label PR `#108` as historical wherever it remains mentioned, including the
additional referenced line.
In `@src/nip57/client.ts`:
- Around line 147-148: Replace the receipt collection array in the subscription
flow with a Map<string, NostrEvent> keyed by event.id, updating entries as
receipts arrive and resolving the map values before calculating totals or event
statistics. Preserve activeSubscriptionIds behavior, and add a multi-relay test
confirming identical receipts are counted only once.
- Around line 144-151: Update collectZapReceipts to use subscribeDetailed() and
track EOSE completion for every returned relay subscription, settling only once
all subscribed relays have completed or the legacy timeout fires; preserve
events from relays whose EOSE arrives later, including synchronous EOSE during
subscription setup. Add a staggered two-relay regression test using the
ephemeral relay setup to verify both relays’ events are collected before
resolution.
In `@src/utils/ephemeral-relay.ts`:
- Around line 177-184: Update the start() method to cache its in-flight startup
operation in a _startPromise, making concurrent calls await and reuse that
promise before returning the relay. Clear _startPromise after the startup
promise settles, while preserving the existing _closePromise handling and
already-started checks.
- Around line 134-146: The constructor’s purge interval handling must disable
scheduling for zero, negative, NaN, and Infinity values. Update the options
normalization and _purge assignment in the constructor to validate purgeInterval
consistently for both numeric and NostrRelayOptions inputs, preserving only
finite positive intervals and treating invalid or zero values as disabled.
---
Nitpick comments:
In `@src/utils/websocket.ts`:
- Around line 14-20: Replace the local normalizeWebSocketUrl implementation with
the existing canonical relay URL normalization utility, and update its callers
to use that shared symbol. Remove the duplicate try/catch normalization logic
while preserving the current WebSocket URL normalization behavior.
🪄 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: c53f9d10-95b9-402e-8970-730729076f7d
📒 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
| - Feature branch: `feature/cleanup-runtime-debt` | ||
| - Human owner: plebdev | ||
| - Started: 2026-07-14 | ||
| - Current status: implementation, integrated verification, and local final review complete; non-draft staging PR #108 open for hosted review |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Refresh the current PR metadata.
The active review is PR #109 (staging → main), but these lines describe PR #108 as the current open review. Update the references or explicitly label PR #108 as historical.
Also applies to: 32-32
🤖 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 `@docs/agents/runs/feature-cleanup-runtime-debt-ledger.md` at line 12, Update
the PR metadata references in the document’s current-status entries to identify
PR `#109` as the active staging-to-main review. Explicitly label PR `#108` as
historical wherever it remains mentioned, including the additional referenced
line.
| /** Collect matching zap receipts until the first EOSE or the legacy timeout. */ | ||
| private collectZapReceipts(filter: Filter): Promise<NostrEvent[]> { | ||
| return new Promise((resolve, reject) => { | ||
| const events: NostrEvent[] = []; | ||
| const activeSubscriptionIds = new Set<string>(); | ||
| let isSettled = false; | ||
| let subscribeComplete = false; | ||
| let reachedEoseSynchronously = false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wait for EOSE from every subscribed relay.
Nostr.subscribe() forwards EOSE once per relay, but handleEose() settles on the first callback. A faster relay therefore closes every subscription before slower relays finish; synchronous EOSE can also suppress events from relays subscribed later in the same call. Track completed relays through subscribeDetailed() and settle only after every returned subscription reaches EOSE or the timeout fires. (github.com)
Add a staggered two-relay regression test. As per coding guidelines, require comprehensive tests for new features and use the ephemeral relay for relay-dependent tests.
Also applies to: 187-201, 229-236
🤖 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/nip57/client.ts` around lines 144 - 151, Update collectZapReceipts to use
subscribeDetailed() and track EOSE completion for every returned relay
subscription, settling only once all subscribed relays have completed or the
legacy timeout fires; preserve events from relays whose EOSE arrives later,
including synchronous EOSE during subscription setup. Add a staggered two-relay
regression test using the ephemeral relay setup to verify both relays’ events
are collected before resolution.
Sources: Coding guidelines, MCP tools
| const events: NostrEvent[] = []; | ||
| const activeSubscriptionIds = new Set<string>(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Deduplicate receipts by event ID.
The same signed receipt received from multiple relays is appended repeatedly, causing getTotalZapsReceived() and event statistics to double-count it. Collect into a Map<string, NostrEvent> keyed by event.id, then resolve its values. Nostr forwards events independently from each relay. (github.com)
Add a multi-relay duplicate-receipt test. As per coding guidelines, require comprehensive tests for new features.
Also applies to: 184-184, 198-200
🤖 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/nip57/client.ts` around lines 147 - 148, Replace the receipt collection
array in the subscription flow with a Map<string, NostrEvent> keyed by event.id,
updating entries as receipts arrive and resolving the map values before
calculating totals or event statistics. Preserve activeSubscriptionIds behavior,
and add a multi-relay test confirming identical receipts are counted only once.
Sources: Coding guidelines, MCP tools
| constructor( | ||
| port: number, | ||
| purgeIntervalOrOptions?: number | NostrRelayOptions, | ||
| ) { | ||
| const options = | ||
| typeof purgeIntervalOrOptions === "number" | ||
| ? { purgeInterval: purgeIntervalOrOptions } | ||
| : (purgeIntervalOrOptions ?? {}); | ||
|
|
||
| this._cache = []; | ||
| this._emitter = new EventEmitter(); | ||
| this._port = port; | ||
| this._purge = purge_ival ?? null; | ||
| this._purge = options.purgeInterval ?? null; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 '\bpurgeInterval\b|\b_purge\b' src tests README.mdRepository: AustinKelsay/snstr
Length of output: 3517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' src/utils/ephemeral-relay.ts
printf '\n---\n'
rg -n 'purgeInterval|setInterval|clearInterval|maybeUnref|normalize|validate|finite|positive' src/utils/ephemeral-relay.ts tests/utils/ephemeral-relay-lifecycle.test.tsRepository: AustinKelsay/snstr
Length of output: 10164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/utils/ephemeral-relay.ts')
text = p.read_text()
for term in ['purgeInterval', 'setInterval', 'clearInterval', 'maybeUnref', 'validate']:
print(f'== {term} ==')
for i, line in enumerate(text.splitlines(), 1):
if term in line:
print(f'{i}: {line}')
PYRepository: AustinKelsay/snstr
Length of output: 1902
🏁 Script executed:
node - <<'JS'
const cases = [0, -1, -100, NaN, Infinity, -Infinity, 0.01, 1];
for (const d of cases) {
const t = setInterval(() => {}, d);
// timer internals are not public API, but this is a read-only probe.
console.log(String(d).padEnd(10), '->', t._idleTimeout);
clearInterval(t);
}
JSRepository: AustinKelsay/snstr
Length of output: 641
🏁 Script executed:
node - <<'JS'
const cases = [0, -1, -100, NaN, Infinity, -Infinity, 0.01, 1];
for (const d of cases) {
const t = setInterval(() => {}, d);
console.log(JSON.stringify({ delay: d, idleTimeout: t._idleTimeout }));
clearInterval(t);
}
JSRepository: AustinKelsay/snstr
Length of output: 753
🌐 Web query:
Node.js setInterval delay NaN negative Infinity minimum 1 ms documentation
💡 Result:
In Node.js, the setInterval function handles invalid or out-of-range delay values by automatically coercing them to a default value of 1 millisecond [1][2]. Specifically, according to official Node.js documentation, when the delay argument is: * Less than 1 millisecond [1][3] * NaN (Not-a-Number) [1][2] * Larger than 2,147,483,647 milliseconds (the maximum 32-bit signed integer value) [1][4] The delay is set to 1 millisecond [1][5]. Additionally, any non-integer delay values provided are truncated to their integer part [1][3]. While this is the current behavior, Node.js has introduced documentation-only deprecation warnings to discourage the use of NaN or negative numbers as delay values, as these are considered invalid input [6]. It is recommended to always use positive integer values for timer delays to ensure predictable behavior [1][2].
Citations:
- 1: https://nodejs.org/api/timers.html
- 2: https://github.com/nodejs/node/blob/main/doc/api/timers.md
- 3: https://nodejs.org/api/timers.md
- 4: https://nodejs.org/dist/latest/docs/api/timers.html
- 5: https://nodejs.org/docs/latest-v26.x/api/timers.html
- 6: doc: deprecate NaN or negative number as delay to setTimeout and setInterval nodejs/node#53005
Validate purgeInterval before scheduling it
src/utils/ephemeral-relay.ts:136-146 — setInterval coerces 0, negative, NaN, and Infinity to a 1ms timer, so these inputs still create an extremely aggressive purge loop instead of disabling purge. Reject non-finite/non-positive values, or make 0 an explicit disabled sentinel for both constructor forms.
🤖 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/ephemeral-relay.ts` around lines 134 - 146, The constructor’s purge
interval handling must disable scheduling for zero, negative, NaN, and Infinity
values. Update the options normalization and _purge assignment in the
constructor to validate purgeInterval consistently for both numeric and
NostrRelayOptions inputs, preserving only finite positive intervals and treating
invalid or zero values as disabled.
| async start() { | ||
| if (this._closePromise) { | ||
| await this._closePromise; | ||
| } | ||
|
|
||
| if (this._wss || this._inMemoryServer) { | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cache and return the in-flight startup promise.
The first call assigns _wss before listening; a concurrent start() then returns success at Line 182 even though binding may still fail. Track _startPromise like _closePromise and clear it after settlement.
🤖 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/ephemeral-relay.ts` around lines 177 - 184, Update the start()
method to cache its in-flight startup operation in a _startPromise, making
concurrent calls await and reuse that promise before returning the relay. Clear
_startPromise after the startup promise settles, while preserving the existing
_closePromise handling and already-started checks.
Prepare v0.5.0 release
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation