fix: viewer model dropdown, SSE cleanup, OpenRouter URL, zh-TW mode, alias fix, restart guard, FTS5 fallback, httpcore dep#2069
Conversation
…alias fix, restart guard, FTS5 fallback, httpcore dep - #1958: Map precise model IDs in viewer settings dropdown, preserve on save - #1959: Clean up dead SSE clients on disconnect/error, periodic 30s sweep - #1960: Make OpenRouter base URL configurable via CLAUDE_MEM_OPENROUTER_BASE_URL - #1961: Add Traditional Chinese (zh-TW) mode file with proper translations - #2054: Use version-agnostic alias path, always update existing aliases - #2053: Replace flat restart counter with time-windowed counter (60s window, cap 10, 5m decay) - #2048: Fall back to FTS5 full-text search when Chroma disabled for text queries - #2046: Add httpcore and httpx to chroma-mcp uvx dependencies Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code ReviewThis PR bundles 8 bug fixes. Overall the approach is solid, but there are a few issues worth addressing before merge. Overview
Issues🔴 Bug —
|
Greptile SummaryThis PR bundles eight bug fixes: precise model IDs in the viewer dropdown, SSE dead-client cleanup, configurable OpenRouter base URL, Traditional Chinese mode, version-agnostic CLI alias, time-windowed restart guard, FTS5 fallback when Chroma is disabled, and
Confidence Score: 4/5Safe to merge after fixing the custom-model ID overwrite in ContextSettingsModal One P1 defect in ContextSettingsModal would silently corrupt a user's custom model setting when they interact with the dropdown. All other findings are P2 style/cleanup items. Everything else in the PR looks correct. src/ui/viewer/components/ContextSettingsModal.tsx (custom option value bug) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Session Generator Exit] --> B{pendingCount > 0?}
B -- Yes --> C[recordRestartAndCheckAllowed]
C -- allowed --> D[Restart Generator]
C -- blocked --> E[terminateSession / abort]
B -- No --> F[resetRestartCounter]
F --> G[Remove Session]
H[SSEBroadcaster] --> I{Client connects}
I --> J[Add to sseClients Set]
J --> K[Listen: close / error → removeClient]
H --> L[setInterval 30s → cleanupDeadClients]
H --> M[broadcast → inline dead-client prune]
N[SearchManager text query] --> O{ChromaDB initialised?}
O -- Yes --> P[Semantic search]
O -- No --> Q[FTS5 fallback]
Q --> R{Results found?}
R -- Yes --> S[Return FTS5 results]
R -- No --> T[chromaFailed = true → show install hint]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private sseClients: Set<SSEClient> = new Set(); | ||
| private cleanupInterval: ReturnType<typeof setInterval> | null = null; | ||
|
|
||
| constructor() { | ||
| // Periodic cleanup of dead/disconnected SSE clients every 30 seconds | ||
| this.cleanupInterval = setInterval(() => { | ||
| this.cleanupDeadClients(); | ||
| }, 30_000); | ||
| } |
There was a problem hiding this comment.
dispose() is never called — interval leaks on shutdown
The setInterval started in the constructor runs every 30 s for the lifetime of the process. The new dispose() method clears it, but there is no call to dispose() anywhere in the diff (not in worker-service.ts, not in WorkerService's shutdown/cleanup path). The live setInterval will prevent Node/Bun from exiting cleanly during graceful shutdown.
Wire dispose() into the existing graceful-shutdown sequence, e.g.:
// in WorkerService shutdown
this.sseBroadcaster.dispose();| */ | ||
| export function resetRestartCounter(session: ActiveSession): void { | ||
| session.consecutiveRestarts = 0; | ||
| session.restartTimestamps = []; | ||
| } | ||
|
|
||
| /** | ||
| * Apply time decay: if enough time has passed since the last restart, | ||
| * clear the restart history. Call this periodically during successful processing. | ||
| */ | ||
| export function applyRestartDecay(session: ActiveSession): void { | ||
| if (!session.restartTimestamps || session.restartTimestamps.length === 0) return; | ||
|
|
||
| const now = Date.now(); | ||
| const mostRecentRestart = Math.max(...session.restartTimestamps); | ||
|
|
||
| if (now - mostRecentRestart > RESTART_DECAY_MS) { | ||
| logger.debug('SYSTEM', 'Restart counter decayed after sustained success', { | ||
| sessionId: session.sessionDbId, | ||
| previousRestarts: session.restartTimestamps.length, | ||
| decayMs: RESTART_DECAY_MS | ||
| }); | ||
| resetRestartCounter(session); | ||
| } | ||
| } |
There was a problem hiding this comment.
applyRestartDecay is defined but never called
applyRestartDecay is exported but nothing in this PR (or anywhere in the diff) calls it. The docstring says "call this periodically during successful processing," but no callsite wires it up. The 5-minute decay won't happen; timestamps age out only via the 60 s window filter. Consider either connecting it to the message-processing loop or removing it until it is needed.
…ssful processing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review — PR #2069This is a solid multi-fix PR. The individual changes are focused and well-scoped. A few issues worth addressing before merge, plus some minor suggestions. OverviewEight independent fixes bundled together: SSE leak cleanup, configurable OpenRouter URL, zh-TW language mode, version-agnostic alias, time-windowed restart guard, FTS5 fallback, httpcore dependency, and viewer model dropdown. Issues1. <option value="custom">custom: {model}</option>
2. if (restartsInWindow > MAX_WINDOWED_RESTARTS) { // blocks at 11, not 10The PR description says "cap 10" but 3. const updated = existingContent.replace(/function claude-mem \{[^\n]*\}\n?/, functionDef);
Minor Suggestions4.
5. const { recordRestartAndCheckAllowed, resetRestartCounter } = await import('./worker/RestartGuard.js');This pattern appears 4+ times in hot paths (session exit handlers). There's no meaningful code-splitting benefit here; these should be static imports at the top of each file. Dynamic imports inside callbacks also obscure the module graph. 6. The What Looks Good
SummaryThe SSE, FTS5 fallback, RestartGuard, and OpenRouter fixes are all clean. The three issues above — the selectable |
|
Closing to start fresh from main — will redo fixes isolated in Docker container. |
1 participant
Summary
Test plan
Closes #1958, closes #1959, closes #1960, closes #1961, closes #2054, closes #2053, closes #2048, closes #2046
🤖 Generated with Claude Code