feat: add WebSocket reconnection with exponential backoff (#57)#115
Open
harika880 wants to merge 1 commit into
Open
feat: add WebSocket reconnection with exponential backoff (#57)#115harika880 wants to merge 1 commit into
harika880 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughImplements WebSocket reconnection with exponential backoff for the frontend live event feed. Adds a custom React hook managing socket lifecycle, state transitions, and reconnection scheduling; provides Jest test coverage for backoff logic and state machine; integrates hook into the dashboard page with status badges showing ChangesWebSocket Reconnection and Live Feed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🧪 PR Test Results
Python 3.12 · commit 7ab41b4 |
Author
|
"The ruff lint failure is unrelated to this PR — it's a pre-existing issue in the Python backend files, not in the frontend WebSocket changes." |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
agentwatch/frontend/hooks/useWebSocket.ts (2)
101-105: 💤 Low valueConsider bounded message retention for long-lived connections.
The
messagesarray grows unboundedly as events arrive. For long-running sessions with high message volume, this could lead to memory pressure. Consider:
- Adding an optional
maxMessagesoption to cap the array size- Letting consumers clear messages via a returned
clearMessagescallbackThis is low priority if sessions are short or consumers handle cleanup externally.
🤖 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 `@agentwatch/frontend/hooks/useWebSocket.ts` around lines 101 - 105, The messages state in useWebSocket (ws.onmessage handler using setMessages) grows unbounded; add an optional maxMessages parameter to the useWebSocket hook and enforce it in the setMessages updater (e.g., when appending new { raw, receivedAt } drop oldest entries to keep length <= maxMessages), and expose a clearMessages callback from the hook that resets messages to an empty array so consumers can clear history; keep existing onMessageRef behavior unchanged and make maxMessages optional with sensible default (e.g., undefined meaning unlimited).
134-145: 💤 Low valueState not reset when URL changes.
When
urlchanges, the effect reconnects to the new endpoint, butmessages,status,elapsedSecs, andattemptCountretain their previous values. Messages from the old URL will persist in the array. For this PR's single-endpoint use case this is likely fine, but for broader hook reusability consider resetting state when the URL changes.Proposed fix to reset state on URL change
useEffect(() => { stoppedRef.current = false; + setMessages([]); + setStatus('connecting'); + setElapsedSecs(0); + setAttemptCount(0); + attemptRef.current = 0; connect(); return () => {🤖 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 `@agentwatch/frontend/hooks/useWebSocket.ts` around lines 134 - 145, When the url changes the hook reconnects but doesn't clear prior connection state; update the effect that runs on [url] (the useEffect that calls connect and manages stoppedRef/wsRef) to reset the hook state before reconnecting by calling setMessages([]), setStatus(/* initial status value used in the hook */), setElapsedSecs(0), and setAttemptCount(0), and also clear any elapsedTimerRef and retryTimerRef; keep stoppedRef.current assignments and existing wsRef.current?.close() behavior, then call connect() so the new URL starts with a clean state.agentwatch/frontend/hooks/__tests__/useWebSocket.test.ts (1)
47-48: 💤 Low valueConsider type-safe WebSocket mock assignment.
The
@ts-ignoredirective suppresses type checking when assigningMockWebSockettoglobal.WebSocket. While common in tests, this bypasses verification that the mock fully implements the WebSocket interface.♻️ Type-safe alternative
- // `@ts-ignore` - global.WebSocket = MockWebSocket; + global.WebSocket = MockWebSocket as unknown as typeof WebSocket;This maintains the assertion while making the type cast explicit.
🤖 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 `@agentwatch/frontend/hooks/__tests__/useWebSocket.test.ts` around lines 47 - 48, Replace the // `@ts-ignore` assignment by doing an explicit, type-safe cast when assigning MockWebSocket to global.WebSocket: ensure you cast global to a type that permits overwriting WebSocket (e.g., cast global to unknown/any or to an interface with WebSocket) and then assign MockWebSocket, referencing the MockWebSocket symbol and the global.WebSocket target so TypeScript no longer needs to be suppressed.agentwatch/frontend/jest.config.json (1)
1-1: 💤 Low valueRemove trailing whitespace.
Line 1 has trailing whitespace after the closing brace.
✨ Proposed fix
-{"preset":"ts-jest","testEnvironment":"jsdom"} +{"preset":"ts-jest","testEnvironment":"jsdom"}🤖 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 `@agentwatch/frontend/jest.config.json` at line 1, The JSON line {"preset":"ts-jest","testEnvironment":"jsdom"} in the Jest config contains trailing whitespace after the closing brace; edit the jest config so the line ends immediately after the brace (no extra spaces or tabs), ensuring the file contains {"preset":"ts-jest","testEnvironment":"jsdom"} with no trailing whitespace.
🤖 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.
Nitpick comments:
In `@agentwatch/frontend/hooks/__tests__/useWebSocket.test.ts`:
- Around line 47-48: Replace the // `@ts-ignore` assignment by doing an explicit,
type-safe cast when assigning MockWebSocket to global.WebSocket: ensure you cast
global to a type that permits overwriting WebSocket (e.g., cast global to
unknown/any or to an interface with WebSocket) and then assign MockWebSocket,
referencing the MockWebSocket symbol and the global.WebSocket target so
TypeScript no longer needs to be suppressed.
In `@agentwatch/frontend/hooks/useWebSocket.ts`:
- Around line 101-105: The messages state in useWebSocket (ws.onmessage handler
using setMessages) grows unbounded; add an optional maxMessages parameter to the
useWebSocket hook and enforce it in the setMessages updater (e.g., when
appending new { raw, receivedAt } drop oldest entries to keep length <=
maxMessages), and expose a clearMessages callback from the hook that resets
messages to an empty array so consumers can clear history; keep existing
onMessageRef behavior unchanged and make maxMessages optional with sensible
default (e.g., undefined meaning unlimited).
- Around line 134-145: When the url changes the hook reconnects but doesn't
clear prior connection state; update the effect that runs on [url] (the
useEffect that calls connect and manages stoppedRef/wsRef) to reset the hook
state before reconnecting by calling setMessages([]), setStatus(/* initial
status value used in the hook */), setElapsedSecs(0), and setAttemptCount(0),
and also clear any elapsedTimerRef and retryTimerRef; keep stoppedRef.current
assignments and existing wsRef.current?.close() behavior, then call connect() so
the new URL starts with a clean state.
In `@agentwatch/frontend/jest.config.json`:
- Line 1: The JSON line {"preset":"ts-jest","testEnvironment":"jsdom"} in the
Jest config contains trailing whitespace after the closing brace; edit the jest
config so the line ends immediately after the brace (no extra spaces or tabs),
ensuring the file contains {"preset":"ts-jest","testEnvironment":"jsdom"} with
no trailing whitespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f6e4f03-bbf8-4af2-af22-8cbe8125dc94
⛔ Files ignored due to path filters (297)
agentwatch/frontend/node_modules/.bin/baseline-browser-mappingis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/baseline-browser-mapping.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/baseline-browser-mapping.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/browserslistis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/browserslist.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/browserslist.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/esparseis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/esparse.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/esparse.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/esvalidateis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/esvalidate.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/esvalidate.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/globis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/glob.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/glob.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/handlebarsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/handlebars.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/handlebars.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/import-local-fixtureis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/import-local-fixture.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/import-local-fixture.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/jestis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/jest.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/jest.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/js-yamlis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/js-yaml.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/js-yaml.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/jsescis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/jsesc.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/jsesc.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/json5is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/json5.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/json5.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/lz-stringis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/lz-string.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/lz-string.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/napi-postinstallis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/napi-postinstall.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/napi-postinstall.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/node-whichis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/node-which.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/node-which.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/parseris excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/parser.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/parser.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/semveris excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/semver.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/semver.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/tldtsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/tldts.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/tldts.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/ts-jestis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/ts-jest.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/ts-jest.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/tscis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/tsc.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/tsc.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/tsserveris excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/tsserver.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/tsserver.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/uglifyjsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/uglifyjs.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/uglifyjs.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/update-browserslist-dbis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/update-browserslist-db.cmdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/.bin/update-browserslist-db.ps1is excluded by!**/node_modules/**agentwatch/frontend/node_modules/.package-lock.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/LICENSEis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/README.mdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/node_modules/lru-cache/LICENSEis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/node_modules/lru-cache/README.mdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/node_modules/lru-cache/package.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/package.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/index.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/cache.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/color.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/common.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/constant.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/convert.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/css-calc.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/css-gradient.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/css-var.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/relative-color.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/resolve.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/typedef.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@asamuzakjp/css-color/src/js/util.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/code-frame/LICENSEis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/code-frame/README.mdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/code-frame/lib/index.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/code-frame/lib/index.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/code-frame/package.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/LICENSEis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/README.mdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/corejs2-built-ins.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/corejs3-shipped-proposals.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/data/corejs2-built-ins.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/data/corejs3-shipped-proposals.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/data/native-modules.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/data/overlapping-plugins.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/data/plugin-bugfixes.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/data/plugins.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/native-modules.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/overlapping-plugins.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/package.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/plugin-bugfixes.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/compat-data/plugins.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/LICENSEis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/README.mdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/cache-contexts.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/cache-contexts.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/caching.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/caching.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/config-chain.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/config-chain.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/config-descriptors.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/config-descriptors.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/files/configuration.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/files/configuration.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/files/import.cjsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/files/import.cjs.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/files/index-browser.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/files/index-browser.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/files/index.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/files/index.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/files/module-types.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/files/module-types.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/files/package.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/files/package.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/files/plugins.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/files/plugins.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/files/types.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/files/types.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/files/utils.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/files/utils.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/full.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/full.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/helpers/config-api.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/helpers/config-api.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/helpers/deep-array.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/helpers/deep-array.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/helpers/environment.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/helpers/environment.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/index.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/index.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/item.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/item.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/partial.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/partial.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/pattern-to-regex.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/pattern-to-regex.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/plugin.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/plugin.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/printer.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/printer.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/resolve-targets-browser.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/resolve-targets-browser.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/resolve-targets.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/resolve-targets.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/util.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/util.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/validation/option-assertions.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/validation/option-assertions.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/validation/options.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/validation/options.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/validation/plugins.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/validation/plugins.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/config/validation/removed.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/config/validation/removed.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/errors/config-error.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/errors/config-error.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/errors/rewrite-stack-trace.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/errors/rewrite-stack-trace.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/gensync-utils/async.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/gensync-utils/async.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/gensync-utils/fs.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/gensync-utils/fs.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/gensync-utils/functional.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/gensync-utils/functional.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/index.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/index.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/parse.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/parse.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/parser/index.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/parser/index.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/parser/util/missing-plugin-helper.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/parser/util/missing-plugin-helper.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/tools/build-external-helpers.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/tools/build-external-helpers.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transform-ast.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transform-ast.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transform-file-browser.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transform-file-browser.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transform-file.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transform-file.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transform.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transform.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/block-hoist-plugin.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/block-hoist-plugin.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/file/babel-7-helpers.cjsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/file/babel-7-helpers.cjs.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/file/file.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/file/file.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/file/generate.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/file/generate.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/file/merge-map.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/file/merge-map.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/index.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/index.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/normalize-file.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/normalize-file.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/normalize-opts.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/normalize-opts.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/plugin-pass.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/plugin-pass.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/read-input-source-map-file-browser.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/read-input-source-map-file-browser.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/read-input-source-map-file.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/read-input-source-map-file.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/transformation/util/clone-deep.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/transformation/util/clone-deep.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/lib/vendor/import-meta-resolve.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/lib/vendor/import-meta-resolve.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/core/package.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/src/config/files/index-browser.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/src/config/files/index.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/src/config/resolve-targets-browser.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/src/config/resolve-targets.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/src/transform-file-browser.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/src/transform-file.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/src/transformation/read-input-source-map-file-browser.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/core/src/transformation/read-input-source-map-file.tsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/LICENSEis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/README.mdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/buffer.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/buffer.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/base.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/base.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/classes.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/classes.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/deprecated.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/deprecated.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/expressions.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/expressions.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/flow.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/flow.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/index.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/index.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/jsx.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/jsx.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/methods.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/methods.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/modules.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/modules.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/statements.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/statements.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/template-literals.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/template-literals.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/types.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/types.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/generators/typescript.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/generators/typescript.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/index.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/index.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/node/index.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/node/index.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/node/parentheses.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/node/parentheses.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/nodes.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/nodes.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/printer.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/printer.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/source-map.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/source-map.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/lib/token-map.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/generator/lib/token-map.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/generator/package.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-compilation-targets/LICENSEis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-compilation-targets/README.mdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/debug.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/debug.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/filter-items.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/filter-items.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/index.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/index.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/options.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/options.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/pretty.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/pretty.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/targets.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/targets.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/utils.jsis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-compilation-targets/lib/utils.js.mapis excluded by!**/node_modules/**,!**/*.mapagentwatch/frontend/node_modules/@babel/helper-compilation-targets/package.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-globals/LICENSEis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-globals/README.mdis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-globals/data/browser-upper.jsonis excluded by!**/node_modules/**agentwatch/frontend/node_modules/@babel/helper-globals/data/builtin-lower.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (3)
agentwatch/frontend/hooks/__tests__/useWebSocket.test.tsagentwatch/frontend/hooks/useWebSocket.tsagentwatch/frontend/jest.config.json
Owner
|
@harika880 Thanks for the contribution. Before I review the implementation, the PR appears to contain over 1.2 million lines across 3000+ files, which is far larger than expected for a WebSocket reconnection feature. Could you check whether build artifacts, node_modules, .next, coverage files, or other generated files were accidentally committed? Please clean up the branch so the PR only contains the files related to the WebSocket reconnection implementation. Once that's done I'll review the actual changes. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@agentwatch/api/server.py`:
- Around line 51-70: The _Limiter class currently uses an unbounded defaultdict
_buckets in check which will leak memory; replace _buckets with a bounded TTL
cache or add pruning logic: either use cachetools.TTLCache(maxsize=<reasonable>,
ttl=60) (import cachetools and initialize _buckets = TTLCache(...)) or, if you
prefer a manual approach inside _Limiter.check, iterate and delete keys whose
stored "start" is older than the window before accessing the caller IP; keep the
existing semantics of check (update b["count"], b["start"], set
request.state.rl_limit and rl_remaining, and raise the same HTTPException) and
ensure the cache/prune is used wherever _buckets is referenced.
- Line 399: The _limiter.check call is dereferencing request.client.host which
can be None; change both occurrences where _limiter.check(...
request.client.host+':r', RATE_READ, request) is used to safely compute a
client_host string first (e.g. client_host = (request.client.host if
request.client else request.headers.get('x-forwarded-for') or 'unknown')) and
then call _limiter.check(client_host + ':r', RATE_READ, request); apply this
same change for the GET handler and the POST /api/v1/sessions handler (the two
places calling _limiter.check) so you no longer assume request.client is
non-null.
- Around line 439-442: The ingest_event handler accepts Request but never
enforces rate limiting; call the limiter check at the start of ingest_event by
invoking _limiter.check(request, RATE_WRITE) (or the project's equivalent
RATE_WRITE constant) before publishing the event and handle a rate-limit failure
by raising an HTTPException(429) or returning the same error path your other
endpoints use; update the ingest_event function (and import
RATE_WRITE/HTTPException if needed) so the check runs prior to
get_event_bus().publish(event).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 008ede00-9e2c-443d-8953-62362bcb5cbf
📒 Files selected for processing (2)
.gitignoreagentwatch/api/server.py
Comment on lines
+51
to
+70
| class _Limiter: | ||
| def __init__(self): | ||
| self._buckets = defaultdict(lambda: {"count": 0, "start": 0.0}) | ||
| def check(self, ip, limit, request): | ||
| now = time.time() | ||
| b = self._buckets[ip] | ||
| if now - b["start"] > 60: | ||
| b["count"] = 0 | ||
| b["start"] = now | ||
| b["count"] += 1 | ||
| request.state.rl_limit = limit | ||
| request.state.rl_remaining = max(0, limit - b["count"]) | ||
| if b["count"] > limit: | ||
| raise HTTPException( | ||
| status_code=429, | ||
| detail="rate_limit_exceeded", | ||
| headers={"X-RateLimit-Limit": str(limit), | ||
| "X-RateLimit-Remaining": "0", | ||
| "Retry-After": "60"}, | ||
| ) |
There was a problem hiding this comment.
Unbounded memory growth in rate limiter.
_buckets is never pruned. Each unique IP+suffix key persists indefinitely. In production with many clients, this will leak memory over time.
Consider adding periodic cleanup of stale entries (e.g., entries older than the window) or using a bounded cache like cachetools.TTLCache.
🤖 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 `@agentwatch/api/server.py` around lines 51 - 70, The _Limiter class currently
uses an unbounded defaultdict _buckets in check which will leak memory; replace
_buckets with a bounded TTL cache or add pruning logic: either use
cachetools.TTLCache(maxsize=<reasonable>, ttl=60) (import cachetools and
initialize _buckets = TTLCache(...)) or, if you prefer a manual approach inside
_Limiter.check, iterate and delete keys whose stored "start" is older than the
window before accessing the caller IP; keep the existing semantics of check
(update b["count"], b["start"], set request.state.rl_limit and rl_remaining, and
raise the same HTTPException) and ensure the cache/prune is used wherever
_buckets is referenced.
| status: str | None = Query(default=None), | ||
| since_hours: int | None = Query(default=None), | ||
| ) -> SessionListResponse: | ||
| _limiter.check(request.client.host+':r', RATE_READ, request) |
There was a problem hiding this comment.
request.client can be None, causing AttributeError.
When behind certain reverse proxies or in test environments, request.client may be None. Accessing .host directly will crash the server.
Proposed fix
- _limiter.check(request.client.host+':r', RATE_READ, request)
+ client_ip = request.client.host if request.client else "unknown"
+ _limiter.check(client_ip + ':r', RATE_READ, request)Apply the same fix at line 413 for POST /api/v1/sessions.
📝 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.
Suggested change
| _limiter.check(request.client.host+':r', RATE_READ, request) | |
| client_ip = request.client.host if request.client else "unknown" | |
| _limiter.check(client_ip + ':r', RATE_READ, request) |
🤖 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 `@agentwatch/api/server.py` at line 399, The _limiter.check call is
dereferencing request.client.host which can be None; change both occurrences
where _limiter.check(... request.client.host+':r', RATE_READ, request) is used
to safely compute a client_host string first (e.g. client_host =
(request.client.host if request.client else
request.headers.get('x-forwarded-for') or 'unknown')) and then call
_limiter.check(client_host + ':r', RATE_READ, request); apply this same change
for the GET handler and the POST /api/v1/sessions handler (the two places
calling _limiter.check) so you no longer assume request.client is non-null.
Comment on lines
439
to
442
| @app.post("/api/v1/events") | ||
| async def ingest_event(event: AgentEvent) -> dict[str, Any]: | ||
| async def ingest_event(request: Request, event: AgentEvent) -> dict[str, Any]: | ||
| await get_event_bus().publish(event) | ||
| return {"status": "accepted", "event_id": event.event_id} |
There was a problem hiding this comment.
POST /api/v1/events accepts Request but doesn't enforce rate limiting.
The handler signature was updated to accept Request, but _limiter.check() is not called. This write endpoint seems like a candidate for RATE_WRITE limiting. Was the check intended here?
,
Proposed fix if rate limiting is intended
`@app.post`("/api/v1/events")
async def ingest_event(request: Request, event: AgentEvent) -> dict[str, Any]:
+ client_ip = request.client.host if request.client else "unknown"
+ _limiter.check(client_ip + ':w', RATE_WRITE, request)
await get_event_bus().publish(event)
return {"status": "accepted", "event_id": event.event_id}🤖 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 `@agentwatch/api/server.py` around lines 439 - 442, The ingest_event handler
accepts Request but never enforces rate limiting; call the limiter check at the
start of ingest_event by invoking _limiter.check(request, RATE_WRITE) (or the
project's equivalent RATE_WRITE constant) before publishing the event and handle
a rate-limit failure by raising an HTTPException(429) or returning the same
error path your other endpoints use; update the ingest_event function (and
import RATE_WRITE/HTTPException if needed) so the check runs prior to
get_event_bus().publish(event).
sreerevanth
requested changes
Jun 1, 2026
Owner
sreerevanth
left a comment
sreerevanth
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The WebSocket reconnection work looks useful, but this PR currently includes unrelated backend rate-limiting changes and several development/debug scripts.
Before merge, please:
Resolve the merge conflicts
Address the rate-limiter review comments
Remove unrelated debug/fix scripts from the PR
Keep the PR focused on the WebSocket reconnection feature
harika880
force-pushed
the
feat/ws-reconnection-57
branch
from
3730e9d to
967641b
Compare
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)
agentwatch/api/server.py (1)
432-443:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
/healthstill bypasses the new auth boundary.This endpoint remains public while returning DB connectivity plus trace, event-bus, safety, and cost stats. That exposes the same class of operational detail the new protected
/api/v1/system/statusendpoint is meant to gate. If/healthmust stay probe-friendly, keep it to a minimal liveness payload and move the detailed fields behind an authenticated route.🤖 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 `@agentwatch/api/server.py` around lines 432 - 443, The /health endpoint (function health) currently exposes operational details (_db_session_factory connectivity, _collector.get_stats(), get_event_bus().stats(), _safety_engine.stats(), _cost_tracker.stats()) without authentication; reduce /health to a minimal liveness payload (e.g., "status" and "timestamp" only) and remove those detailed fields, then surface the detailed diagnostics behind the authenticated system-status route (e.g., reuse or call the authenticated /api/v1/system/status handler) or create a new auth-protected endpoint that returns the DB, traces, event bus, safety, and cost stats.
🤖 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 `@agentwatch/api/server.py`:
- Around line 174-176: The production flag logic is too strict: _IS_PROD only
matches the exact string "production" so values like "prod" or whitespace-padded
values bypass fail-closed auth; update the environment normalization and check
used by the startup guard and request-time misconfiguration check by computing
_ENV = (os.getenv("AGENTWATCH_ENV") or os.getenv("ENVIRONMENT") or
"development").strip().lower() and then set _IS_PROD using a broader test such
as _ENV.startswith("prod") (or membership in {"production","prod"}) so
trimmed/shortened variants are treated as production for the fail-closed auth
paths referenced by _IS_PROD.
- Around line 402-416: The current CORS setup constructs _cors_origins and
silently defaults to ["*"] when CORS_ALLOWED_ORIGINS is unset; change this to
fail closed by (a) defaulting _cors_origins to an empty list ([]) instead of
["*"] and setting _cors_credentials accordingly, and (b) validate at startup and
raise an explicit error if CORS_ALLOWED_ORIGINS is empty while running in
production (or when a PRODUCTION/ENV flag indicates non-dev), so the
app.add_middleware(CORSMiddleware, allow_origins=_cors_origins,
allow_credentials=_cors_credentials, ...) will never be initialized with a
wildcard in prod; update the logic around _raw_cors_origins, _cors_origins, and
_cors_credentials to implement these checks and raise a RuntimeError (or
similar) when the env is unsafe.
---
Outside diff comments:
In `@agentwatch/api/server.py`:
- Around line 432-443: The /health endpoint (function health) currently exposes
operational details (_db_session_factory connectivity, _collector.get_stats(),
get_event_bus().stats(), _safety_engine.stats(), _cost_tracker.stats()) without
authentication; reduce /health to a minimal liveness payload (e.g., "status" and
"timestamp" only) and remove those detailed fields, then surface the detailed
diagnostics behind the authenticated system-status route (e.g., reuse or call
the authenticated /api/v1/system/status handler) or create a new auth-protected
endpoint that returns the DB, traces, event bus, safety, and cost stats.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 322ff03a-9bf3-41d9-ba9b-780cebb904a6
📒 Files selected for processing (1)
agentwatch/api/server.py
Comment on lines
+174
to
+176
| # Environment detection for fail-closed logic | ||
| _ENV = os.getenv("AGENTWATCH_ENV") or os.getenv("ENVIRONMENT") or "development" | ||
| _IS_PROD = _ENV.lower() == "production" |
There was a problem hiding this comment.
Broaden the production check before using it for fail-closed auth.
_IS_PROD only flips for the exact string "production". A deployment using values like prod or whitespace-padded env values will skip both the startup guard and the request-time misconfiguration check, leaving the API effectively open when AGENTWATCH_API_KEY is unset.
Suggested fix
-_ENV = os.getenv("AGENTWATCH_ENV") or os.getenv("ENVIRONMENT") or "development"
-_IS_PROD = _ENV.lower() == "production"
+_ENV = (os.getenv("AGENTWATCH_ENV") or os.getenv("ENVIRONMENT") or "development").strip()
+_IS_PROD = _ENV.lower() in {"prod", "production"}🤖 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 `@agentwatch/api/server.py` around lines 174 - 176, The production flag logic
is too strict: _IS_PROD only matches the exact string "production" so values
like "prod" or whitespace-padded values bypass fail-closed auth; update the
environment normalization and check used by the startup guard and request-time
misconfiguration check by computing _ENV = (os.getenv("AGENTWATCH_ENV") or
os.getenv("ENVIRONMENT") or "development").strip().lower() and then set _IS_PROD
using a broader test such as _ENV.startswith("prod") (or membership in
{"production","prod"}) so trimmed/shortened variants are treated as production
for the fail-closed auth paths referenced by _IS_PROD.
Comment on lines
+402
to
416
| _raw_cors_origins = os.getenv("CORS_ALLOWED_ORIGINS", "") | ||
| _cors_origins: list[str] = ( | ||
| [o.strip() for o in _raw_cors_origins.split(",") if o.strip()] | ||
| if _raw_cors_origins.strip() | ||
| else ["*"] | ||
| ) | ||
| _cors_credentials = _cors_origins != ["*"] | ||
|
|
||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=["*"], | ||
| allow_credentials=True, | ||
| allow_origins=_cors_origins, | ||
| allow_credentials=_cors_credentials, | ||
| allow_methods=["*"], | ||
| allow_headers=["*"], | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
FastAPI 0.115 CORSMiddleware wildcard allow_origins custom headers X-Api-Key allow_credentials false browser preflight behavior
💡 Result:
When you configure CORSMiddleware in FastAPI with allow_origins=[""] and allow_credentials=False, the browser's CORS preflight behavior for requests containing custom headers (like X-Api-Key) is governed by the CORS specification and how FastAPI handles the preflight OPTIONS request [1][2]. 1. Preflight Trigger: When a browser detects a cross-origin request that includes a custom header such as X-Api-Key, it is classified as a non-simple request [1][3]. The browser automatically sends a preflight OPTIONS request to the server before the actual request to verify that the server allows the specific method and headers [1][4]. 2. Middleware Response: FastAPI's CORSMiddleware intercepts this OPTIONS request [1][2]. Because allow_origins=[""] is set and allow_credentials=False, the middleware is configured to permit all origins and does not require credentialed support [1][5]. 3. Handling X-Api-Key: - For the preflight to succeed, the middleware must be configured to allow the custom header. By default, CORSMiddleware does not automatically allow every custom header you might send [1]. - You must explicitly add 'x-api-key' (or whatever your header name is) to the allow_headers list in your CORSMiddleware configuration [1][6]. - If you use allow_headers=[""], the wildcard will effectively authorize any custom header, including X-Api-Key, in the preflight response [1][2]. 4. Browser Behavior: - If CORSMiddleware is configured with allow_headers=[""] or explicitly includes 'x-api-key', it will respond to the OPTIONS request with Access-Control-Allow-Headers: x-api-key (or equivalent) [7][1]. - The browser, seeing this confirmation, will proceed to send the actual request [4]. - Because allow_credentials is set to False, the browser will not send cookies or HTTP authentication headers, but it will send the custom X-Api-Key header as it is now explicitly permitted by the preflight response [1][6]. In summary, setting allow_credentials=False allows you to use a wildcard () for allowed_origins [1][2]. To ensure preflight succeeds for requests with custom headers like X-Api-Key, you must ensure that either the header is explicitly listed in allow_headers or that allow_headers=[""] is used [1][6].
Citations:
- 1: https://fastapi.tiangolo.com/tutorial/cors/
- 2: https://github.com/fastapi/fastapi/blob/master/docs/en/docs/tutorial/cors.md
- 3: https://dev.to/armorbreak/http-headers-every-developer-should-know-2026-pj6
- 4: Proper support for OPTIONS method for CORS pre-flight requests TykTechnologies/tyk#1191
- 5: https://fastapi.tiangolo.com/tutorial/cors/?h=+cors
- 6: https://www.learnwithparam.com/blog/fastapi-cors-production-agentic-apis
- 7: https://fastapi.tiangolo.com/reference/middleware/
Fail closed for CORS when CORS_ALLOWED_ORIGINS is unset in production
In agentwatch/api/server.py (lines 402-416), _cors_origins falls back to allow_origins=["*"] whenever CORS_ALLOWED_ORIGINS is empty/unset, with allow_headers=["*"] still permitting preflighted custom headers from browsers. This silently widens cross-origin access and undercuts the intended hardening. Require an explicit allowlist in production (e.g., fail at startup or default to allow_origins=[]).
🤖 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 `@agentwatch/api/server.py` around lines 402 - 416, The current CORS setup
constructs _cors_origins and silently defaults to ["*"] when
CORS_ALLOWED_ORIGINS is unset; change this to fail closed by (a) defaulting
_cors_origins to an empty list ([]) instead of ["*"] and setting
_cors_credentials accordingly, and (b) validate at startup and raise an explicit
error if CORS_ALLOWED_ORIGINS is empty while running in production (or when a
PRODUCTION/ENV flag indicates non-dev), so the
app.add_middleware(CORSMiddleware, allow_origins=_cors_origins,
allow_credentials=_cors_credentials, ...) will never be initialized with a
wildcard in prod; update the logic around _raw_cors_origins, _cors_origins, and
_cors_credentials to implement these checks and raise a RuntimeError (or
similar) when the env is unsafe.
- Reconnect Live Feed with 1s/2s/4s backoff capped at 30s - Show Reconnecting badge with elapsed time; failed state after max attempts - Preserve event list across reconnects; configurable NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS - Add Jest tests for backoff and status transitions - Ignore node_modules globally in git Co-authored-by: Cursor <cursoragent@cursor.com>
harika880
force-pushed
the
feat/ws-reconnection-57
branch
from
967641b to
d49f0e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/__tests__/wsReconnect.test.ts (1)
42-50: ⚡ Quick winAdd coverage for invalid env values fallback.
parseMaxReconnectAttempts()has validation branches for invalid/non-positive values, but this suite currently doesn’t assert them ('0','-1','abc'→8).Suggested test additions
describe('parseMaxReconnectAttempts', () => { @@ it('reads env override', () => { process.env.NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS = '3' expect(parseMaxReconnectAttempts()).toBe(3) }) + + it('falls back to 8 for invalid values', () => { + process.env.NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS = '0' + expect(parseMaxReconnectAttempts()).toBe(8) + + process.env.NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS = '-1' + expect(parseMaxReconnectAttempts()).toBe(8) + + process.env.NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS = 'abc' + expect(parseMaxReconnectAttempts()).toBe(8) + }) })🤖 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 `@frontend/__tests__/wsReconnect.test.ts` around lines 42 - 50, Add tests in wsReconnect.test.ts covering invalid NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS values so parseMaxReconnectAttempts() falls back to the default (8). Specifically: for values '0', '-1', and 'abc' set process.env.NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS to each, call parseMaxReconnectAttempts(), and assert it returns 8; also ensure you cleanup or delete the env between cases (similar to existing tests) so each case is isolated. Use the existing test names/patterns around parseMaxReconnectAttempts() to locate where to add these assertions.
🤖 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 `@frontend/__tests__/wsReconnect.test.ts`:
- Around line 32-33: The test is setting and restoring the wrong environment
variable (NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS) so it doesn't match the runtime
contract (NEXT_PUBLIC_WS_MAX_ATTEMPTS); update all references in the test (the
assignments that save original, set, and restore process.env entries) to use
NEXT_PUBLIC_WS_MAX_ATTEMPTS instead of NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS so
the test reflects the PR objective and will pick up the correct runtime config.
In `@frontend/lib/useLiveEventSocket.ts`:
- Around line 51-87: connect() is clearing timers every attempt which stops
elapsedTimer and because disconnectedAt is only set once it prevents
startElapsedTicker from restarting; modify the onclose/connect flow so the
elapsed ticker is restarted on each reconnect attempt: after clearTimers() in
connect() (or at start of onclose before setting disconnectedAt) ensure
startElapsedTicker() is called whenever status will become 'reconnecting' (use
nextLiveFeedStatus result) or when reconnectTimer is scheduled, and ensure
disconnectedAt is set to Date.now() each time you start the ticker (and reset to
null in onopen as already present) so reconnectElapsedSec continues updating
across multiple attempts; update handlers referring to connect, clearTimers,
onclose, disconnectedAt, startElapsedTicker, and onopen accordingly.
---
Nitpick comments:
In `@frontend/__tests__/wsReconnect.test.ts`:
- Around line 42-50: Add tests in wsReconnect.test.ts covering invalid
NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS values so parseMaxReconnectAttempts()
falls back to the default (8). Specifically: for values '0', '-1', and 'abc' set
process.env.NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS to each, call
parseMaxReconnectAttempts(), and assert it returns 8; also ensure you cleanup or
delete the env between cases (similar to existing tests) so each case is
isolated. Use the existing test names/patterns around
parseMaxReconnectAttempts() to locate where to add these assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b203d906-1973-4ef8-8992-c283b7d23f5b
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.gitignorefrontend/__tests__/wsReconnect.test.tsfrontend/jest.config.jsfrontend/lib/useLiveEventSocket.tsfrontend/lib/wsReconnect.tsfrontend/package.jsonfrontend/pages/index.tsx
💤 Files with no reviewable changes (2)
- frontend/package.json
- frontend/pages/index.tsx
✅ Files skipped from review due to trivial changes (2)
- frontend/jest.config.js
- .gitignore
Comment on lines
+32
to
+33
| const original = process.env.NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS | ||
|
|
There was a problem hiding this comment.
Env var contract in tests does not match the PR objective.
These tests lock in NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS, but the objective specifies NEXT_PUBLIC_WS_MAX_ATTEMPTS. If deployment follows the documented objective, runtime config won’t apply.
Also applies to: 43-49
🤖 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 `@frontend/__tests__/wsReconnect.test.ts` around lines 32 - 33, The test is
setting and restoring the wrong environment variable
(NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS) so it doesn't match the runtime contract
(NEXT_PUBLIC_WS_MAX_ATTEMPTS); update all references in the test (the
assignments that save original, set, and restore process.env entries) to use
NEXT_PUBLIC_WS_MAX_ATTEMPTS instead of NEXT_PUBLIC_WS_MAX_RECONNECT_ATTEMPTS so
the test reflects the PR objective and will pick up the correct runtime config.
Comment on lines
+51
to
+87
| const connect = () => { | ||
| if (cancelled) return | ||
| clearTimers() | ||
| socket?.close() | ||
| socket = createEventSocket((event) => { | ||
| onEventRef.current(event) | ||
| refreshRef.current() | ||
| }) | ||
|
|
||
| socket.onopen = () => { | ||
| attempt = 0 | ||
| disconnectedAt = null | ||
| setReconnectElapsedSec(0) | ||
| setStatus('streaming') | ||
| } | ||
|
|
||
| socket.onclose = () => { | ||
| if (cancelled) return | ||
| setStatus((prev) => { | ||
| const next = nextLiveFeedStatus(prev, 'close') | ||
| if (next === 'reconnecting' && disconnectedAt === null) { | ||
| disconnectedAt = Date.now() | ||
| startElapsedTicker() | ||
| } | ||
| return next | ||
| }) | ||
|
|
||
| attempt += 1 | ||
| if (attempt > maxAttempts) { | ||
| clearTimers() | ||
| setStatus('failed') | ||
| return | ||
| } | ||
|
|
||
| const delay = wsBackoffDelayMs(attempt - 1) | ||
| reconnectTimer = setTimeout(connect, delay) | ||
| } |
There was a problem hiding this comment.
Elapsed-time ticker freezes after the first reconnect attempt.
connect() calls clearTimers() on every attempt, which clears elapsedTimer. The ticker is only (re)started in onclose under the guard disconnectedAt === null (Line 71). After the first close, disconnectedAt becomes non-null and stays so until a successful onopen, so the ticker is never restarted on subsequent reconnect attempts. As a result reconnectElapsedSec stops updating after roughly the first second, breaking the "Reconnecting… (elapsed)" requirement during longer outages.
🐛 Proposed fix — restart the ticker on each reconnect attempt
const connect = () => {
if (cancelled) return
clearTimers()
+ if (disconnectedAt !== null) startElapsedTicker()
socket?.close()🤖 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 `@frontend/lib/useLiveEventSocket.ts` around lines 51 - 87, connect() is
clearing timers every attempt which stops elapsedTimer and because
disconnectedAt is only set once it prevents startElapsedTicker from restarting;
modify the onclose/connect flow so the elapsed ticker is restarted on each
reconnect attempt: after clearTimers() in connect() (or at start of onclose
before setting disconnectedAt) ensure startElapsedTicker() is called whenever
status will become 'reconnecting' (use nextLiveFeedStatus result) or when
reconnectTimer is scheduled, and ensure disconnectedAt is set to Date.now() each
time you start the ticker (and reset to null in onopen as already present) so
reconnectElapsedSec continues updating across multiple attempts; update handlers
referring to connect, clearTimers, onclose, disconnectedAt, startElapsedTicker,
and onopen accordingly.
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
2 participants
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.
Closes #57
Changes
useWebSockethook with exponential backoff reconnection (1s → 2s → 4s, max 30s)Summary by CodeRabbit
New Features
Tests
Chores