Improve stream buffering and reconnection #3
Conversation
📝 WalkthroughWalkthroughAdds monotonic stream sequencing and client acknowledgment support across the Claude streaming stack: messages carry streamSeq, buffers use seq-based cursors, clients can ack lastSeenSeq (including ack-only requests), and per-tab last-seen sequence state and ack batching are tracked in the UI/store. Changes
sequenceDiagram
participant Client
participant StreamRoute as Stream Route
participant StreamBuf as Stream Buffer
participant Store as Streaming Store
participant Reconnect as Reconnect Route
Client->>StreamRoute: send stream event (requestId, payload, streamSeq)
StreamRoute->>StreamBuf: appendToStreamBuffer(requestId, payload, streamSeq)
StreamBuf->>StreamBuf: store {seq: streamSeq, line: payload}
StreamBuf-->>StreamRoute: ack append
StreamRoute-->>Client: emit BridgeMessage (messageId, streamSeq, payload)
Client->>Store: recordStreamSeq(tabId, streamSeq)
Store-->>Client: updated lastSeenStreamSeq
Client->>Reconnect: POST /reconnect (lastSeenSeq=N, ackOnly=false)
Reconnect->>StreamBuf: getUnreadMessages(requestId, userId, afterSeq=N)
StreamBuf-->>Reconnect: unread messages where seq > N + lastReadSeq
Reconnect-->>Client: return unread messages + lastReadSeq
Client->>Reconnect: POST /reconnect (lastSeenSeq=M, ackOnly=true)
Reconnect->>StreamBuf: ackStreamCursor(requestId, userId, lastSeenSeq=M)
StreamBuf-->>Reconnect: persisted lastReadSeq=M
Reconnect-->>Client: ACK confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (1)
apps/web/app/api/claude/stream/reconnect/route.ts (1)
106-116: Return ack status based on the actual cursor updateThe ack-only path always returns
acknowledged: true, even iflastSeenSeqis missing or the Redis ack fails. Consider reflecting the real ack result so callers can detect a no-op.Suggested adjustment
- if (ackOnly) { - if (typeof lastSeenSeq === "number") { - await ackStreamCursor(requestId, user.id, lastSeenSeq) - } - return NextResponse.json({ - ok: true, - hasStream: true, - acknowledged: true, - }) - } + if (ackOnly) { + let acknowledged = false + if (typeof lastSeenSeq === "number") { + acknowledged = !!(await ackStreamCursor(requestId, user.id, lastSeenSeq)) + } + return NextResponse.json({ + ok: true, + hasStream: true, + acknowledged, + }) + }
Refactored ACK tracking from global refs to per-tab state map. This fixes a bug where multiple concurrent streams could corrupt each other's cursor position during reconnection. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix binary name mismatch: Makefile now outputs `shell-server-go` to match module name (was causing systemd EXEC failures) - Remove shell.terminal.goalive.nl and sk.goalive.nl from all configs - Add shell-server-go.service to git-tracked ops/systemd/ - Update all documentation to reflect single shell domain (go.goalive.nl) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/shell-server-go/Makefile (1)
36-39:⚠️ Potential issue | 🟡 MinorThe
cleantarget doesn't remove the built binary.The binary is now built as
./shell-server-goin the current directory, butcleanonly removesbin/. The actual build artifact won't be cleaned up.🧹 Proposed fix
# Clean build artifacts clean: rm -rf bin/ + rm -f shell-server-go rm -rf client/node_modules rm -f .sessions.json .rate-limit-state.jsonapps/web/app/chat/hooks/useChatMessaging.ts (1)
318-319:⚠️ Potential issue | 🟡 MinorUse
targetTabIdfor validation instead ofexpectedTabIdto prevent message loss during tab switches.
expectedTabIdis captured from the currentactiveTab?.idat line 319, but if the user switches tabs while streaming is in progress,expectedTabIdbecomes stale and no longer reflects the tab for which the request was initiated. This causes the validation at lines 475-480 to reject valid server responses with a mismatched tabId, even though those messages are intended for the correcttargetTabId(the stable parameter).Replace the validation with a check against
targetTabIdinstead:- if (expectedTabId && responseTabId && responseTabId !== expectedTabId) { + if (responseTabId && responseTabId !== targetTabId) {This ensures that messages are validated against the actual tab the request was sent for, not the currently active tab.
🤖 Fix all issues with AI agents
In `@docs/operations/CADDY_SHELL_ISOLATION.md`:
- Around line 3-5: The "**Updated**:" metadata line in the document still shows
2025-12-07; please edit the "**Updated**:" field in CADDY_SHELL_ISOLATION.md to
reflect today's date (or the current revision date) so change tracking is
accurate, ensuring you only modify the "**Updated**:" line and keep the rest of
the header and Services list intact.
In `@ops/systemd/shell-server-go.service`:
- Line 25: SyslogIdentifier is set to "shell-server" but the service unit is
named "shell-server-go", causing inconsistent log filtering; update the
SyslogIdentifier entry to match the service name by changing SyslogIdentifier to
"shell-server-go" (or alternatively rename the unit to match SyslogIdentifier)
so that the SyslogIdentifier and the unit name (referenced as SyslogIdentifier
and the unit name "shell-server-go") are consistent for journalctl filtering.
In `@templates/site-template/scripts/generate-config.js`:
- Around line 52-54: Remove the invalid "X-Frame-Options": "ALLOWALL" entry from
the headers object in generate-config.js (and the duplicate at the other
location) — either delete that header key entirely or replace it by setting an
appropriate Content-Security-Policy using the frame-ancestors directive; update
the headers object(s) where "X-Frame-Options" appears so there is no "ALLOWALL"
value left.
- Around line 46-47: Replace the brittle manual domain-splitting used to build
allowedHosts with tldts' getDomain/getPublicSuffix to compute the registrable
domain (eTLD+1): install tldts in templates/site-template/package.json, import
the appropriate function in templates/site-template/scripts/generate-config.js,
and replace the expression "${domain.split('.').slice(-2).join('.')}" with the
registrable domain computed by tldts so allowedHosts becomes ["${domain}",
".${registrableDomain}"]; apply the same replacement in the other three
locations that build allowedHosts across the template set.
🧹 Nitpick comments (5)
apps/web/lib/build-info.d.ts (1)
1-8: Prefer an interface for the build-info shape.Inline object types are fine, but your TS guidelines prefer interfaces for object shapes. Consider introducing a
BuildInfointerface and using it forvalue.As per coding guidelines, "Prefer interface for defining object shapes in TypeScript."♻️ Suggested refactor
declare module "@/lib/build-info.json" { - const value: { - commit: string - branch: string - buildTime: string - } + interface BuildInfo { + commit: string + branch: string + buildTime: string + } + const value: BuildInfo export default value }apps/web/e2e-tests/templates-health.spec.ts (1)
79-89: Minor: Variable shadowing could reduce readability.The variable
responseon line 81 shadows the outerresponsefrom line 51 (the API fetch). While this works due to block scoping, consider renaming tonavResponseorpageResponsefor clarity.♻️ Suggested rename
// Final fallback: use real browser navigation (most accurate) if (!ok) { - const response = await page.goto(templateUrl, { waitUntil: "domcontentloaded", timeout: 10000 }) - if (response) { - status = response.status() + const navResponse = await page.goto(templateUrl, { waitUntil: "domcontentloaded", timeout: 10000 }) + if (navResponse) { + status = navResponse.status() ok = status >= 200 && status < 400 } else {apps/web/e2e-tests/global-teardown.ts (1)
76-87: Route the new deletion through the Repository layer.This adds another direct Supabase query; per codebase conventions, data access in TS should go through a Repository abstraction for consistency and testability. As per coding guidelines Use the Repository pattern for data access in TypeScript files.
apps/web/app/chat/hooks/useChatMessaging.ts (2)
83-91: Consider cleanup for stale tab entries inackStateByTabRef.The
ackStateByTabRefmap grows unbounded as tabs are opened. Old tab entries persist even after tabs are closed, causing a slow memory leak. Consider:
- Deleting the entry in the
finallyblock after stream completion- Adding periodic cleanup of entries for tabs that no longer exist
Suggested cleanup in finally block
void flushAck(targetTabId) const ackStateCleanup = getAckState(targetTabId) if (ackStateCleanup.ackTimeout) { clearTimeout(ackStateCleanup.ackTimeout) ackStateCleanup.ackTimeout = null } + // Clean up per-tab ACK state after stream ends + ackStateByTabRef.current.delete(targetTabId) abortControllerRef.current = null
267-279: Consider adding a timeout to the ACK fetch request.The ACK request has no timeout. If the server hangs,
ackInFlightremainstrueuntil the request eventually fails or completes, blocking subsequent ACKs. Since this is best-effort, a short timeout (e.g., 5-10 seconds) would improve resilience.Suggested fix with AbortSignal timeout
ackState.ackInFlight = true try { + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), 5000) await fetch("/api/claude/stream/reconnect", { method: "POST", headers: { "Content-Type": "application/json" }, credentials: "include", + signal: controller.signal, body: JSON.stringify({ tabGroupId, tabId: targetTabId, workspace, ackOnly: true, lastSeenSeq: seq, }), }) + clearTimeout(timeoutId) ackState.lastAckedSeq = Math.max(ackState.lastAckedSeq, seq)
| **Created**: 2025-12-07 | ||
| **Updated**: 2025-12-07 (nginx SNI router fix) | ||
| **Services**: `nginx`, `caddy`, `caddy-shell`, `shell.terminal.goalive.nl`, `go.goalive.nl`, `sk.goalive.nl` | ||
| **Services**: `nginx`, `caddy`, `caddy-shell`, `go.goalive.nl` |
There was a problem hiding this comment.
Update the "Updated" date to reflect this revision.
The documentation has been substantially modified (domain consolidation, port changes), but the "Updated" field still shows 2025-12-07. Consider updating to the current date for accurate change tracking.
📝 Suggested fix
**Created**: 2025-12-07
-**Updated**: 2025-12-07 (nginx SNI router fix)
+**Updated**: 2026-02-04 (consolidated to single shell domain go.goalive.nl)
**Services**: `nginx`, `caddy`, `caddy-shell`, `go.goalive.nl`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Created**: 2025-12-07 | |
| **Updated**: 2025-12-07 (nginx SNI router fix) | |
| **Services**: `nginx`, `caddy`, `caddy-shell`, `shell.terminal.goalive.nl`, `go.goalive.nl`, `sk.goalive.nl` | |
| **Services**: `nginx`, `caddy`, `caddy-shell`, `go.goalive.nl` | |
| **Created**: 2025-12-07 | |
| **Updated**: 2026-02-04 (consolidated to single shell domain go.goalive.nl) | |
| **Services**: `nginx`, `caddy`, `caddy-shell`, `go.goalive.nl` |
🤖 Prompt for AI Agents
In `@docs/operations/CADDY_SHELL_ISOLATION.md` around lines 3 - 5, The
"**Updated**:" metadata line in the document still shows 2025-12-07; please edit
the "**Updated**:" field in CADDY_SHELL_ISOLATION.md to reflect today's date (or
the current revision date) so change tracking is accurate, ensuring you only
modify the "**Updated**:" line and keep the rest of the header and Services list
intact.
| # Logging | ||
| StandardOutput=journal | ||
| StandardError=journal | ||
| SyslogIdentifier=shell-server |
There was a problem hiding this comment.
SyslogIdentifier doesn't match service name.
SyslogIdentifier=shell-server while the service is named shell-server-go. This inconsistency can cause confusion when filtering logs (e.g., journalctl -u shell-server-go vs journalctl -t shell-server).
📝 Proposed fix
-SyslogIdentifier=shell-server
+SyslogIdentifier=shell-server-go📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SyslogIdentifier=shell-server | |
| SyslogIdentifier=shell-server-go |
🤖 Prompt for AI Agents
In `@ops/systemd/shell-server-go.service` at line 25, SyslogIdentifier is set to
"shell-server" but the service unit is named "shell-server-go", causing
inconsistent log filtering; update the SyslogIdentifier entry to match the
service name by changing SyslogIdentifier to "shell-server-go" (or alternatively
rename the unit to match SyslogIdentifier) so that the SyslogIdentifier and the
unit name (referenced as SyslogIdentifier and the unit name "shell-server-go")
are consistent for journalctl filtering.
| allowedHosts: ["${domain}", ".${domain.split('.').slice(-2).join('.')}"], | ||
| hmr: { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and content
wc -l templates/site-template/scripts/generate-config.jsRepository: eenlars/alive
Length of output: 111
🏁 Script executed:
# Read the entire file to understand context
cat -n templates/site-template/scripts/generate-config.jsRepository: eenlars/alive
Length of output: 6636
🏁 Script executed:
# Check if tldts is available in the project dependencies
find . -name "package.json" -type f | head -20 | xargs grep -l "tldts" 2>/dev/null || echo "tldts not found in package.json files"Repository: eenlars/alive
Length of output: 94
🏁 Script executed:
# Check the structure of the templates/site-template directory
ls -la templates/site-template/Repository: eenlars/alive
Length of output: 544
🏁 Script executed:
# Check if there's a package.json in the site-template directory
cat templates/site-template/package.json 2>/dev/null || echo "No package.json in site-template"Repository: eenlars/alive
Length of output: 551
🏁 Script executed:
# Verify tldts is a legitimate and stable package
web_search: tldts npm packageRepository: eenlars/alive
Length of output: 108
🏁 Script executed:
# Double-check the exact behavior of the problematic slice logic
python3 <<'EOF'
# Test the current problematic logic
test_domains = [
"example.com",
"example.co.uk",
"example.com.au",
"example.blogspot.com",
"sub.example.co.uk"
]
for domain in test_domains:
parts = domain.split('.')
last_two = '.'.join(parts[-2:])
result = f".{last_two}"
print(f"{domain:30} → allowedHosts: [\"{domain}\", \"{result}\"]")
print("\n✗ Problem: .co.uk, .com.au, .blogspot.com are PUBLIC SUFFIXES")
print(" This allows ANY domain under those suffixes, defeating the host allowlist!")
EOFRepository: eenlars/alive
Length of output: 595
🌐 Web query:
tldts npm package public suffix parser
💡 Result:
tldts is an npm package that parses URLs/hostnames using the Public Suffix List (PSL) to extract pieces like hostname, publicSuffix (effective TLD), domain, subdomain, and flags like isIcann / isPrivate / isIp. It’s designed to be very fast and ships with TypeScript types and multiple bundle formats. [1][2]
Install
- Full PSL (ICANN + private):
npm i tldts[1] - ICANN-only rules:
npm i tldts-icann[3]
Use (public suffix / domain parsing)
import { parse } from "tldts";
parse("http://www.writethedocs.org/conf/eu/2017/");
// -> { hostname: "...", publicSuffix: "org", domain: "writethedocs.org", subdomain: "www", ... }[1]
If you only need a simpler PSL domain split (tld/sld/subdomain) there’s also psl, but it’s less URL-focused than tldts. [4]
Sources: [1] tldts on npm, [2] remusao/tldts GitHub, [3] tldts-icann on npm, [4] psl package docs.
Fix overly broad allowedHosts derivation that allows public suffixes.
The current logic using .split('.').slice(-2).join('.') produces public suffixes for multi-part TLDs (e.g., .co.uk, .com.au, .blogspot.com), which effectively disables host validation for any domain under those suffixes. Use tldts to compute the registrable domain (eTLD+1) instead.
Add tldts to templates/site-template/package.json and apply this fix across all four locations:
Proposed fix
+const { parse } = require("tldts")
+
const safeName = domain.replace(/\./g, "-")
const packageName = domain.replace(/\./g, "_")
+const registrableDomain = parse(domain).domain ?? domain
+const allowedHosts = [domain, `.${registrableDomain}`]
// Generate vite.config.ts
const viteConfig = `import path from "node:path";
@@
server: {
host: "::",
port: ${port},
- allowedHosts: ["${domain}", ".${domain.split('.').slice(-2).join('.')}"],
+ allowedHosts: ${JSON.stringify(allowedHosts)},
@@
preview: {
host: "::",
port: ${port},
- allowedHosts: ["${domain}", ".${domain.split('.').slice(-2).join('.')}"],
+ allowedHosts: ${JSON.stringify(allowedHosts)},
@@
server: {
@@
- allowedHosts: ["${domain}", ".${domain.split('.').slice(-2).join('.')}"],
+ allowedHosts: ${JSON.stringify(allowedHosts)},
@@
preview: {
@@
- allowedHosts: ["${domain}", ".${domain.split('.').slice(-2).join('.')}"],
+ allowedHosts: ${JSON.stringify(allowedHosts)},🤖 Prompt for AI Agents
In `@templates/site-template/scripts/generate-config.js` around lines 46 - 47,
Replace the brittle manual domain-splitting used to build allowedHosts with
tldts' getDomain/getPublicSuffix to compute the registrable domain (eTLD+1):
install tldts in templates/site-template/package.json, import the appropriate
function in templates/site-template/scripts/generate-config.js, and replace the
expression "${domain.split('.').slice(-2).join('.')}" with the registrable
domain computed by tldts so allowedHosts becomes ["${domain}",
".${registrableDomain}"]; apply the same replacement in the other three
locations that build allowedHosts across the template set.
| headers: { | ||
| "X-Frame-Options": "ALLOWALL", | ||
| }, |
There was a problem hiding this comment.
X-Frame-Options: ALLOWALL is invalid and ignored.
If you intend to allow framing, omit the header entirely, or use CSP frame-ancestors for explicit control.
✅ Simple fix (remove invalid header)
- headers: {
- "X-Frame-Options": "ALLOWALL",
- },
@@
- headers: {
- "X-Frame-Options": "ALLOWALL",
- },Also applies to: 60-62
🤖 Prompt for AI Agents
In `@templates/site-template/scripts/generate-config.js` around lines 52 - 54,
Remove the invalid "X-Frame-Options": "ALLOWALL" entry from the headers object
in generate-config.js (and the duplicate at the other location) — either delete
that header key entirely or replace it by setting an appropriate
Content-Security-Policy using the frame-ancestors directive; update the headers
object(s) where "X-Frame-Options" appears so there is no "ALLOWALL" value left.
1 participant
Summary
lastReadSeq)Test plan
bun run test🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes