feat(events): live domain event stream log panel#2653
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an authenticated SSE backend endpoint at /events/domain and a new Event Log settings panel that streams, parses, filters, auto-scrolls, displays, and exports live domain events. ChangesEvent Log Dashboard Feature
Sequence Diagram(s)sequenceDiagram
participant SettingsUI
participant EventLogPanel
participant EventAPI
participant CoreEventBus
SettingsUI->>EventLogPanel: open panel / mount
EventLogPanel->>EventAPI: GET /events/domain (Bearer token)
EventAPI->>EventLogPanel: SSE stream (data: JSON lines, keep-alive)
loop on each DomainEvent
CoreEventBus->>EventAPI: DomainEvent
EventAPI->>EventLogPanel: data: {domain,event,timestamp}
EventLogPanel->>SettingsUI: render prepend/append entry, update filters/controls
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/src/components/settings/panels/EventLogPanel.tsx (1)
47-50: ⚡ Quick winRemove the unused
EventSourcepath to simplify ownership/cleanup.The panel streams via
fetch, so creating/closing/storingEventSourceis dead code and makes lifecycle behavior harder to follow.Proposed cleanup
- const eventSourceRef = useRef<EventSource | null>(null); @@ - const es = new EventSource(url); - - // EventSource doesn't support custom headers, so we use fetch-based SSE - es.close(); + // EventSource doesn't support custom headers, so we use fetch-based SSE @@ - eventSourceRef.current = es; return () => { controller.abort(); }; @@ return () => { disconnect?.(); - eventSourceRef.current?.close(); };Also applies to: 101-101, 110-110
🤖 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 `@app/src/components/settings/panels/EventLogPanel.tsx` around lines 47 - 50, Remove the dead EventSource path: delete the instantiation and immediate close of EventSource (the variable `es` created via `new EventSource(url)`) and any other unused `EventSource` usage in EventLogPanel (including the occurrences around lines referenced at 101 and 110) so the component only uses the fetch-based SSE implementation; ensure no leftover references to `es` remain and that ownership/cleanup logic is consolidated into the fetch/SSE cleanup handlers in the EventLogPanel component.
🤖 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 `@app/src/components/settings/panels/EventLogPanel.tsx`:
- Around line 13-25: The component renders hard-coded UI text (badges and
status/button/count labels) — update it to use the i18n hook and keys: replace
visible literals like "Live", "Disconnected", "events", "Jump to latest" and
every badge label in EVENT_TYPE_COLORS with translation keys consumed via useT()
(e.g., change EVENT_TYPE_COLORS.{...}.label to a key string like
"eventLog.badge.tool" and when rendering call t('eventLog.badge.tool')). Add
corresponding entries to the English i18n file (app/src/lib/i18n/en.ts) for each
new key, update the JSX props (label/aria-label/button text/count suffix) to
call t(...) instead of hard-coded text, and ensure any tests or snapshots that
assert on these strings are updated accordingly.
- Line 3: The import for useT in EventLogPanel is using the wrong relative path
and causes a build error; update the import statement that currently references
'../../lib/i18n/I18nContext' to the correct relative path
'../../../lib/i18n/I18nContext' so the useT symbol is resolved (modify the
import line near the top of EventLogPanel.tsx to import useT from
'../../../lib/i18n/I18nContext').
- Around line 41-112: connect() creates an AbortController and starts the fetch
but the useEffect that calls connect() ignores its cleanup; update connect (the
function in EventLogPanel.tsx) to return a cleanup function that aborts the
controller and closes the EventSource/reader, then call that cleanup from the
useEffect's return so the streaming read loop is aborted on unmount (i.e., have
useEffect capture the cleanup = connect() and return () => { cleanup(); } or
store the controller in a ref and abort it in useEffect cleanup); ensure you
still set/clear eventSourceRef.current and call controller.abort() to prevent
state updates after unmount.
In `@app/src/lib/i18n/en.ts`:
- Around line 2960-2967: The new i18n keys under settings.developerMenu.eventLog
(for example 'settings.developerMenu.eventLog.title', '.desc', '.allTypes',
'.filterAgent', '.download', '.waiting', '.notConnected') were added only to
app/src/lib/i18n/en.ts; add these same keys (with English values) into each i18n
chunk file app/src/lib/i18n/chunks/en-1.ts ... en-5.ts and then add the
corresponding keys to each non-English locale chunk file (keeping existing
translations or adding placeholder translations) so all locale chunks include
the settings.developerMenu.eventLog.* entries to satisfy the i18n coverage gate.
---
Nitpick comments:
In `@app/src/components/settings/panels/EventLogPanel.tsx`:
- Around line 47-50: Remove the dead EventSource path: delete the instantiation
and immediate close of EventSource (the variable `es` created via `new
EventSource(url)`) and any other unused `EventSource` usage in EventLogPanel
(including the occurrences around lines referenced at 101 and 110) so the
component only uses the fetch-based SSE implementation; ensure no leftover
references to `es` remain and that ownership/cleanup logic is consolidated into
the fetch/SSE cleanup handlers in the EventLogPanel component.
🪄 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
Run ID: ed93a8f5-d588-4eb9-afe5-a98150822c38
📒 Files selected for processing (5)
app/src/components/settings/panels/DeveloperOptionsPanel.tsxapp/src/components/settings/panels/EventLogPanel.tsxapp/src/lib/i18n/en.tsapp/src/pages/Settings.tsxsrc/core/jsonrpc.rs
| 'settings.developerMenu.eventLog.title': 'Event Log', | ||
| 'settings.developerMenu.eventLog.desc': | ||
| 'Live colour-coded stream of all agent, tool, and system events', | ||
| 'settings.developerMenu.eventLog.allTypes': 'All types', | ||
| 'settings.developerMenu.eventLog.filterAgent': 'Filter by agent...', | ||
| 'settings.developerMenu.eventLog.download': 'Download', | ||
| 'settings.developerMenu.eventLog.waiting': 'Waiting for events...', | ||
| 'settings.developerMenu.eventLog.notConnected': 'Not connected to core', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify newly added event log keys exist across required locale files/chunks.
keys=(
"settings.developerMenu.eventLog.title"
"settings.developerMenu.eventLog.desc"
"settings.developerMenu.eventLog.allTypes"
"settings.developerMenu.eventLog.filterAgent"
"settings.developerMenu.eventLog.download"
"settings.developerMenu.eventLog.waiting"
"settings.developerMenu.eventLog.notConnected"
)
echo "== Checking English chunk files =="
fd -a '^en-[1-5]\.ts$' app/src/lib/i18n | sort
echo "== Checking all locale files/chunks for key presence =="
for k in "${keys[@]}"; do
echo "--- $k ---"
rg -n --fixed-strings "'$k'" app/src/lib/i18n || true
doneRepository: tinyhumansai/openhuman
Length of output: 1381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
keys=(
"settings.developerMenu.eventLog.title"
"settings.developerMenu.eventLog.desc"
"settings.developerMenu.eventLog.allTypes"
"settings.developerMenu.eventLog.filterAgent"
"settings.developerMenu.eventLog.download"
"settings.developerMenu.eventLog.waiting"
"settings.developerMenu.eventLog.notConnected"
)
echo "== Search for eventLog in chunks =="
rg -n "settings\.developerMenu\.eventLog" app/src/lib/i18n/chunks || true
echo "== Search for each key (no quote dependency) in chunks =="
for k in "${keys[@]}"; do
echo "--- $k ---"
rg -n --fixed-strings "$k" app/src/lib/i18n/chunks || true
doneRepository: tinyhumansai/openhuman
Length of output: 502
Propagate settings.developerMenu.eventLog.* keys to all i18n chunk files
The new settings.developerMenu.eventLog keys exist only in app/src/lib/i18n/en.ts (lines 2960-2967) and are missing from app/src/lib/i18n/chunks/en-{1..5}.ts and the corresponding non-English locale chunk files. This will break the i18n coverage gate.
🤖 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 `@app/src/lib/i18n/en.ts` around lines 2960 - 2967, The new i18n keys under
settings.developerMenu.eventLog (for example
'settings.developerMenu.eventLog.title', '.desc', '.allTypes', '.filterAgent',
'.download', '.waiting', '.notConnected') were added only to
app/src/lib/i18n/en.ts; add these same keys (with English values) into each i18n
chunk file app/src/lib/i18n/chunks/en-1.ts ... en-5.ts and then add the
corresponding keys to each non-English locale chunk file (keeping existing
translations or adding placeholder translations) so all locale chunks include
the settings.developerMenu.eventLog.* entries to satisfy the i18n coverage gate.
Sathvik-1007
force-pushed
the
feat/1849-live-event-stream-log
branch
from
8b04c54 to
5505f38
Compare
coderabbitai
Bot
added
the
rust-core
Sathvik-1007
force-pushed
the
feat/1849-live-event-stream-log
branch
from
5505f38 to
5b81259
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@app/src/components/settings/panels/EventLogPanel.tsx`:
- Around line 30-31: The hard-coded MAX_ENTRIES and fixed newest-on-top behavior
in EventLogPanel should be driven by the app config keys
dashboard.event_stream.max_entries and dashboard.event_stream.new_entries:
update EventLogPanel to read those config values (or accept them as props)
instead of using the MAX_ENTRIES constant, use the configured max_entries to cap
the list length, and conditionally render/insert new events at the top when
new_entries is true (and at the bottom when false); replace references to
MAX_ENTRIES and the existing insertion/ordering logic (including the code around
the current newest-on-top handling at the area noted around lines 92-95) to
respect these config-driven settings.
- Around line 1-249: Prettier is failing for EventLogPanel.tsx; run the
project's Prettier formatter (or npm/yarn script such as "npm run format" /
"yarn format") and apply formatting to this file so it matches CI rules; ensure
you preserve exported symbol EventLogPanel and do not change logic in functions
like connect, exportLog, or constants DOMAIN_BADGE_KEYS and MAX_ENTRIES—only
reformat the file to satisfy prettier --check.
- Around line 71-104: The read loop for the SSE stream exits on EOF but never
clears the live flag, so call setIsLive(false) when the stream ends; inside the
async reader loop (the block using reader.read(), decoder, buffer and
idRef.current) setIsLive(false) right before/after breaking when done is true
(or move the setIsLive(false) into a finally/after-loop cleanup) so the
component's isLive state is set to disconnected when the stream completes or is
closed.
In `@app/src/pages/Settings.tsx`:
- Line 43: Run Prettier to format the Settings.tsx file to satisfy CI: run the
project's Prettier command (e.g., prettier --write) on the file and fix
formatting issues around the import for EventLogPanel and other locations noted
(approx. the Settings component top imports and the block around line 449).
Ensure the file compiles after formatting and commit the formatted file so
prettier --check passes in CI.
🪄 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
Run ID: 97e5266e-bf94-474e-ab65-5735929bbfeb
📒 Files selected for processing (18)
app/src/components/settings/panels/DeveloperOptionsPanel.tsxapp/src/components/settings/panels/EventLogPanel.tsxapp/src/lib/i18n/chunks/ar-5.tsapp/src/lib/i18n/chunks/bn-5.tsapp/src/lib/i18n/chunks/de-5.tsapp/src/lib/i18n/chunks/en-5.tsapp/src/lib/i18n/chunks/es-5.tsapp/src/lib/i18n/chunks/fr-5.tsapp/src/lib/i18n/chunks/hi-5.tsapp/src/lib/i18n/chunks/id-5.tsapp/src/lib/i18n/chunks/it-5.tsapp/src/lib/i18n/chunks/ko-5.tsapp/src/lib/i18n/chunks/pt-5.tsapp/src/lib/i18n/chunks/ru-5.tsapp/src/lib/i18n/chunks/zh-CN-5.tsapp/src/lib/i18n/en.tsapp/src/pages/Settings.tsxsrc/core/jsonrpc.rs
✅ Files skipped from review due to trivial changes (5)
- app/src/lib/i18n/chunks/de-5.ts
- app/src/lib/i18n/chunks/ar-5.ts
- app/src/lib/i18n/chunks/id-5.ts
- app/src/lib/i18n/chunks/pt-5.ts
- app/src/lib/i18n/chunks/zh-CN-5.ts
Sathvik-1007
force-pushed
the
feat/1849-live-event-stream-log
branch
from
5b81259 to
d5aedab
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/jsonrpc.rs (1)
1077-1083: ⚡ Quick winHarden event-name scrubbing for tuple-style debug output.
The current split logic misses
Variant(...)shapes, so payload text can leak intoevent_nameif tuple variants are introduced.🔧 Suggested tweak
- let event_name = variant - .split_once('{') - .or_else(|| variant.split_once(' ')) + let event_name = variant + .split_once('{') + .or_else(|| variant.split_once('(')) + .or_else(|| variant.split_once(' ')) .map(|(name, _)| name.trim()) .unwrap_or(&variant);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/jsonrpc.rs` around lines 1077 - 1083, The event-name extraction for the Debug-formatted `variant` (assigned to `variant` and used to create `event_name`) currently only splits on '{' and ' ' and therefore can leak payload text for tuple-style variants like `Variant(...)`; update the scrub logic to also split on '(' (i.e., find the first occurrence of any of '{', ' ', '('), then take the left part and trim it, so tuple payloads are stripped the same as struct-style payloads; change the code that computes `event_name` (the block that calls `split_once` on `variant`) to try these separators in a robust order and fall back to the whole `variant` if none match.
🤖 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 `@app/src/components/settings/panels/EventLogPanel.tsx`:
- Around line 125-128: The current filteredEntries logic uses filterText against
e.event but the UI intends an agent-name filter; update the predicate to check
the entry's agent name field instead of e.event — e.g. replace the condition `if
(filterText && !e.event.toLowerCase().includes(filterText.toLowerCase()))` with
a check against the agent property (for example `const agentName = e.agent ||
e.agentName || e.actor` and `if (filterText &&
!agentName?.toLowerCase().includes(filterText.toLowerCase())) return false;`) so
the filterText matches agent names case-insensitively while keeping the existing
filterType check and final return.
- Around line 105-111: The cleanup in EventLogPanel's useEffect currently calls
setIsLive(false) which triggers the react-hooks/set-state-in-effect warning;
remove setIsLive(false) from the cleanup and instead ensure isLive is cleared
inside the connection lifecycle: update the connect() implementation to
setIsLive(false) on abort/error/finally and also make abort() set isLive false
(e.g., when controllerRef.current?.abort() is invoked handle the abort signal or
promise rejection to call setIsLive(false)), or if you intentionally must change
state in cleanup add a single-line eslint-disable comment with a clear
justification referencing why setIsLive must run on unmount and target
react-hooks/set-state-in-effect.
---
Nitpick comments:
In `@src/core/jsonrpc.rs`:
- Around line 1077-1083: The event-name extraction for the Debug-formatted
`variant` (assigned to `variant` and used to create `event_name`) currently only
splits on '{' and ' ' and therefore can leak payload text for tuple-style
variants like `Variant(...)`; update the scrub logic to also split on '(' (i.e.,
find the first occurrence of any of '{', ' ', '('), then take the left part and
trim it, so tuple payloads are stripped the same as struct-style payloads;
change the code that computes `event_name` (the block that calls `split_once` on
`variant`) to try these separators in a robust order and fall back to the whole
`variant` if none match.
🪄 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
Run ID: fabe830f-8085-4beb-972e-64c5ba074386
📒 Files selected for processing (18)
app/src/components/settings/panels/DeveloperOptionsPanel.tsxapp/src/components/settings/panels/EventLogPanel.tsxapp/src/lib/i18n/chunks/ar-5.tsapp/src/lib/i18n/chunks/bn-5.tsapp/src/lib/i18n/chunks/de-5.tsapp/src/lib/i18n/chunks/en-5.tsapp/src/lib/i18n/chunks/es-5.tsapp/src/lib/i18n/chunks/fr-5.tsapp/src/lib/i18n/chunks/hi-5.tsapp/src/lib/i18n/chunks/id-5.tsapp/src/lib/i18n/chunks/it-5.tsapp/src/lib/i18n/chunks/ko-5.tsapp/src/lib/i18n/chunks/pt-5.tsapp/src/lib/i18n/chunks/ru-5.tsapp/src/lib/i18n/chunks/zh-CN-5.tsapp/src/lib/i18n/en.tsapp/src/pages/Settings.tsxsrc/core/jsonrpc.rs
✅ Files skipped from review due to trivial changes (8)
- app/src/lib/i18n/chunks/bn-5.ts
- app/src/lib/i18n/chunks/it-5.ts
- app/src/lib/i18n/chunks/es-5.ts
- app/src/lib/i18n/chunks/pt-5.ts
- app/src/lib/i18n/chunks/ru-5.ts
- app/src/lib/i18n/chunks/de-5.ts
- app/src/lib/i18n/chunks/en-5.ts
- app/src/lib/i18n/en.ts
Sathvik-1007
force-pushed
the
feat/1849-live-event-stream-log
branch
from
81f5e29 to
122be9e
Compare
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — PR #2653
Solid foundation. The SSE plumbing in Rust is clean — auth check, proper error responses, keep-alive. The React panel has good structure: ring buffer, scroll handling, proper cleanup, i18n keys across all locales, and decent test coverage. Nice work.
But there are meaningful gaps against the acceptance criteria in #1849 that need to be closed before this ships.
Summary
| File | Area | Changes |
|---|---|---|
src/core/jsonrpc.rs |
Rust core | New /events/domain SSE endpoint with bearer auth |
EventLogPanel.tsx |
Frontend | New panel: SSE consumer, filters, export, auto-scroll |
EventLogPanel.test.tsx |
Tests | 10 test cases covering rendering, filters, export, scroll |
DeveloperOptionsPanel.tsx |
Frontend | Menu entry for event log |
Settings.tsx |
Frontend | Route for event-log panel |
*-5.ts (13 files) |
i18n | Translation keys for all locales |
Issues found (not covered by CodeRabbit)
3 major, 1 minor — see inline comments.
CodeRabbit already flagged: config-driven max_entries/new_entries order, agent filter logic mismatch. Not repeated here.
| <span className="text-[10px] text-stone-400 dark:text-neutral-500 font-mono shrink-0 pt-0.5"> | ||
| {entry.timestamp} | ||
| </span> | ||
| <span className="rounded-full bg-stone-200 dark:bg-neutral-800 px-2 py-0.5 text-[10px] text-stone-600 dark:text-neutral-300 shrink-0"> |
There was a problem hiding this comment.
[major] Every badge renders the same bg-stone-200 gray regardless of domain. Issue #1849 explicitly requires colour-coded badges (blue for TOOL, green for OK, amber for WARN, red for ERR).
You already have DOMAIN_BADGE_KEYS mapping domains to i18n keys. Add a parallel colour map and apply it here:
const DOMAIN_BADGE_COLORS: Record<string, { bg: string; text: string }> = {
tool: { bg: 'bg-blue-500/20', text: 'text-blue-400' },
agent: { bg: 'bg-green-500/20', text: 'text-green-400' },
system: { bg: 'bg-slate-500/20', text: 'text-slate-400' },
memory: { bg: 'bg-purple-500/20', text: 'text-purple-400' },
approval: { bg: 'bg-amber-500/20', text: 'text-amber-400' },
// ...
};Then use DOMAIN_BADGE_COLORS[entry.domain]?.bg etc. instead of the hardcoded stone classes.
| .or_else(|| variant.split_once(' ')) | ||
| .map(|(name, _)| name.trim()) | ||
| .unwrap_or(&variant); | ||
| let data = json!({ |
There was a problem hiding this comment.
[major] Issue #1849 requires "each entry shows timestamp, agent name, message, and a colour-coded type badge." The emitted JSON only has domain, event, and timestamp — no agent identifier.
The DomainEvent payload likely carries an agent name. Extract it before stripping the rest:
let agent = event.agent_name().unwrap_or_default();
let data = json!({
"domain": domain,
"event": event_name,
"agent": agent,
"timestamp": chrono::Utc::now().format("%H:%M:%S").to_string(),
});This also unblocks the frontend agent filter that CodeRabbit already flagged — right now there's no agent field to filter on.
| } | ||
| }, []); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
[major] No reconnect logic. When the SSE stream drops (server restart, network blip), the panel shows "Disconnected" and stays dead. For a live monitoring tool, auto-reconnect is table-stakes.
Add a retry with backoff in the catch/finally of connect():
} catch {
setIsLive(false);
} finally {
controllerRef.current = null;
// Auto-reconnect after 3s unless component unmounted
reconnectTimer.current = setTimeout(() => void connect(), 3000);
}Clear reconnectTimer in the useEffect cleanup alongside the abort.
| Err(_) => return None, | ||
| }; | ||
| let domain = event.domain().to_string(); | ||
| let variant = format!("{:?}", event); |
There was a problem hiding this comment.
[minor] format!("{:?}", event) + split_once to extract the variant name is fragile — it's coupled to the Debug impl and could silently break if the format changes.
Consider adding a variant_name(&self) -> &'static str method on DomainEvent (or using a match). Gives you a stable, clean event name and removes any risk of accidentally leaking payload data through Debug formatting.
SSE endpoint GET /events/domain streams DomainEvent bus. React panel at Settings > Developer > Event Log with colour-coded badges, type/agent filtering, auto-scroll, and ndjson export. Closes tinyhumansai#1849
Sathvik-1007
force-pushed
the
feat/1849-live-event-stream-log
branch
from
122be9e to
30d152b
Compare
The auto-scroll and pause-detection paths hard-coded `scrollTop = 0` and `scrollTop < 10`, so when `dashboard.event_stream.new_entries` is set to `bottom` (newest-at-bottom mode), the feed never followed new events and "Jump to latest" jumped to the top instead. Anchor on `scrollHeight` for the bottom branch in both effect and handler.
`agent_hint()` returned the tool name for `ToolExecutionStarted` but fell through to `None` for `ToolExecutionCompleted`, so the SSE feed showed an empty agent column on completion events. Both variants carry `tool_name` — surface it consistently.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
Bot
added
the
agent
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/components/settings/panels/__tests__/EventLogPanel.test.tsx (1)
160-177: ⚡ Quick winQuery by role or testid instead of Tailwind class.
Line 168 queries by the Tailwind class
.max-h-\[60vh\], which tests implementation details and is fragile if styling changes. Line 169 manually setsscrollTopviaObject.defineProperty, which is also implementation-focused.As per coding guidelines, prefer testing behavior over implementation details. Consider adding a
data-testidto the scrollable container inEventLogPanel.tsxor querying by semantic role, then use Testing Library's user event simulation.♻️ Suggested approach
In
EventLogPanel.tsx, add a testid:-<div className="max-h-[60vh] overflow-y-auto" ref={containerRef} onScroll={handleScroll}> +<div className="max-h-[60vh] overflow-y-auto" ref={containerRef} onScroll={handleScroll} data-testid="event-log-scroll-container">In the test:
- const scrollDiv = container.querySelector('.max-h-\\[60vh\\]')!; + const scrollDiv = screen.getByTestId('event-log-scroll-container'); Object.defineProperty(scrollDiv, 'scrollTop', { value: 100, writable: true }); fireEvent.scroll(scrollDiv);Alternatively, consider testing the scroll behavior more holistically by verifying that the "Jump to latest" button appears when the user would naturally scroll away (if you can simulate that behavior without directly manipulating
scrollTop).🤖 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 `@app/src/components/settings/panels/__tests__/EventLogPanel.test.tsx` around lines 160 - 177, Replace the fragile Tailwind class query and manual Object.defineProperty scrollTop manipulation by adding a stable data-testid on the EventLogPanel's scrollable container (e.g., data-testid="event-log-scroll") in EventLogPanel.tsx, then update the test in EventLogPanel.test.tsx to select the container via getByTestId('event-log-scroll') (or by role if appropriate) and simulate scrolling with Testing Library events (e.g., fireEvent.scroll(scrollEl, { target: { scrollTop: 100 } }) or use userEvent), then assert the "Jump to latest" button appears and click it; this removes dependence on implementation CSS and direct property stubbing while keeping the same assertions around EventLogPanel behavior.
🤖 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 `@app/src/components/settings/panels/__tests__/EventLogPanel.test.tsx`:
- Around line 82-98: In the EventLogPanel.test.tsx test for "filters events by
domain type", replace the manual Event construction/dispatch on the select
element with Testing Library's fireEvent.change (target.value = 'tool') against
the select returned from container.querySelector('select'); after firing the
change, assert the filtering behavior by checking that "ToolA" remains visible
(e.g., getByText/finder) and "AgentB" is not present (e.g., queryByText returns
null), using waitFor if needed to await UI updates; keep the existing
mockFetchSSE and renderWithProviders setup and update only the event dispatch
and assertions.
In `@app/src/components/settings/panels/EventLogPanel.tsx`:
- Line 204: The filename for export assigned to a.download uses new
Date().toISOString().slice(0, 19) which contains ':' characters invalid on
Windows; sanitize the timestamp by replacing colons (and any other problematic
characters) with safe characters (e.g., '-' or '_') before assigning to
a.download so the generated name like event-log-<sanitized-timestamp>.ndjson is
cross-platform safe; update the code around the a.download assignment (the line
that builds the filename) to perform the replacement.
---
Nitpick comments:
In `@app/src/components/settings/panels/__tests__/EventLogPanel.test.tsx`:
- Around line 160-177: Replace the fragile Tailwind class query and manual
Object.defineProperty scrollTop manipulation by adding a stable data-testid on
the EventLogPanel's scrollable container (e.g., data-testid="event-log-scroll")
in EventLogPanel.tsx, then update the test in EventLogPanel.test.tsx to select
the container via getByTestId('event-log-scroll') (or by role if appropriate)
and simulate scrolling with Testing Library events (e.g.,
fireEvent.scroll(scrollEl, { target: { scrollTop: 100 } }) or use userEvent),
then assert the "Jump to latest" button appears and click it; this removes
dependence on implementation CSS and direct property stubbing while keeping the
same assertions around EventLogPanel behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc7b4732-ef4c-4b76-a740-13c5c3848b27
📒 Files selected for processing (18)
app/src/components/settings/panels/DeveloperOptionsPanel.tsxapp/src/components/settings/panels/EventLogPanel.tsxapp/src/components/settings/panels/__tests__/EventLogPanel.test.tsxapp/src/lib/i18n/chunks/ar-5.tsapp/src/lib/i18n/chunks/bn-5.tsapp/src/lib/i18n/chunks/de-5.tsapp/src/lib/i18n/chunks/en-5.tsapp/src/lib/i18n/chunks/es-5.tsapp/src/lib/i18n/chunks/fr-5.tsapp/src/lib/i18n/chunks/hi-5.tsapp/src/lib/i18n/chunks/id-5.tsapp/src/lib/i18n/chunks/it-5.tsapp/src/lib/i18n/chunks/ko-5.tsapp/src/lib/i18n/chunks/pt-5.tsapp/src/lib/i18n/chunks/ru-5.tsapp/src/lib/i18n/chunks/zh-CN-5.tsapp/src/lib/i18n/en.tsapp/src/pages/Settings.tsx
💤 Files with no reviewable changes (1)
- app/src/pages/Settings.tsx
✅ Files skipped from review due to trivial changes (8)
- app/src/lib/i18n/chunks/bn-5.ts
- app/src/lib/i18n/chunks/fr-5.ts
- app/src/lib/i18n/chunks/zh-CN-5.ts
- app/src/lib/i18n/chunks/it-5.ts
- app/src/lib/i18n/chunks/id-5.ts
- app/src/lib/i18n/chunks/ko-5.ts
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/chunks/hi-5.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/lib/i18n/chunks/de-5.ts
- app/src/lib/i18n/chunks/pt-5.ts
- app/src/lib/i18n/chunks/es-5.ts
- app/src/lib/i18n/chunks/ru-5.ts
# Conflicts: # app/src/pages/Settings.tsx # src/openhuman/config/schema/dashboard.rs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/openhuman/config/schema/dashboard.rs (1)
35-37: ⚡ Quick winConstrain
new_entriesto an enum instead of free-formString.
new_entriescurrently accepts arbitrary text, so invalid config values can silently flow to clients. Model it as a serde enum (Top | Bottom) to enforce the contract at parse time.Proposed refactor
+#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, Default)] +#[serde(rename_all = "lowercase")] +pub enum NewEntriesOrder { + #[default] + Top, + Bottom, +} + #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] #[serde(default)] pub struct EventStreamConfig { @@ - /// Where new entries appear: "top" (newest first) or "bottom" (oldest first). - #[serde(default = "default_new_entries")] - pub new_entries: String, + /// Where new entries appear: "top" (newest first) or "bottom" (oldest first). + #[serde(default)] + pub new_entries: NewEntriesOrder, } @@ -fn default_new_entries() -> String { - "top".to_string() -} @@ Self { enabled: true, max_entries: 200, - new_entries: "top".to_string(), + new_entries: NewEntriesOrder::Top, } } }Also applies to: 46-47, 50-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/config/schema/dashboard.rs` around lines 35 - 37, Replace the free-form String type for the new_entries field with a serde-backed enum to enforce valid values: define an enum NewEntries { Top, Bottom } with #[derive(Deserialize, Serialize, Debug, Clone, PartialEq, Eq)] and #[serde(rename_all = "lowercase")] (or explicit renames), implement or provide a default function default_new_entries() -> NewEntries that returns NewEntries::Top, then change the struct field signature from pub new_entries: String to pub new_entries: NewEntries and keep #[serde(default = "default_new_entries")]; update any code that reads or compares new_entries to use the enum variants instead of string literals, and apply the same enum+default pattern to the other similar fields referenced (lines 46-47, 50-56).
🤖 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 `@src/openhuman/config/schema/dashboard.rs`:
- Around line 35-37: Replace the free-form String type for the new_entries field
with a serde-backed enum to enforce valid values: define an enum NewEntries {
Top, Bottom } with #[derive(Deserialize, Serialize, Debug, Clone, PartialEq,
Eq)] and #[serde(rename_all = "lowercase")] (or explicit renames), implement or
provide a default function default_new_entries() -> NewEntries that returns
NewEntries::Top, then change the struct field signature from pub new_entries:
String to pub new_entries: NewEntries and keep #[serde(default =
"default_new_entries")]; update any code that reads or compares new_entries to
use the enum variants instead of string literals, and apply the same
enum+default pattern to the other similar fields referenced (lines 46-47,
50-56).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78a3f0cb-aa96-4c63-8325-e561267ff5d6
📒 Files selected for processing (19)
app/src/components/settings/panels/DeveloperOptionsPanel.tsxapp/src/lib/i18n/chunks/ar-5.tsapp/src/lib/i18n/chunks/bn-5.tsapp/src/lib/i18n/chunks/de-5.tsapp/src/lib/i18n/chunks/en-5.tsapp/src/lib/i18n/chunks/es-5.tsapp/src/lib/i18n/chunks/fr-5.tsapp/src/lib/i18n/chunks/hi-5.tsapp/src/lib/i18n/chunks/id-5.tsapp/src/lib/i18n/chunks/it-5.tsapp/src/lib/i18n/chunks/ko-5.tsapp/src/lib/i18n/chunks/pt-5.tsapp/src/lib/i18n/chunks/ru-5.tsapp/src/lib/i18n/chunks/zh-CN-5.tsapp/src/lib/i18n/en.tsapp/src/pages/Settings.tsxsrc/openhuman/config/schema/dashboard.rssrc/openhuman/config/schema/mod.rssrc/openhuman/config/schema/types.rs
✅ Files skipped from review due to trivial changes (8)
- app/src/lib/i18n/chunks/zh-CN-5.ts
- app/src/lib/i18n/chunks/pt-5.ts
- app/src/lib/i18n/chunks/ru-5.ts
- app/src/lib/i18n/chunks/bn-5.ts
- app/src/lib/i18n/chunks/ar-5.ts
- src/openhuman/config/schema/types.rs
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/chunks/hi-5.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- app/src/components/settings/panels/DeveloperOptionsPanel.tsx
- app/src/lib/i18n/chunks/es-5.ts
- app/src/lib/i18n/chunks/it-5.ts
- app/src/lib/i18n/chunks/fr-5.ts
- app/src/lib/i18n/chunks/ko-5.ts
- app/src/lib/i18n/chunks/de-5.ts
- app/src/lib/i18n/chunks/id-5.ts
- app/src/pages/Settings.tsx
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Re-approving after conflict-resolution merge f5720411 — kept #2653's full EventStreamConfig (enabled/max_entries/new_entries) + added #2823's ModelHealthConfig to DashboardConfig; preserved both /event-log + /model-health routes in Settings.tsx; deduped accidental double mod dashboard + pub dashboard field/Default that the auto-merge produced. cargo check + cargo fmt clean. CR re-reviewed → COMMENTED. LGTM.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Continuation Review — All Prior Issues Resolved
Prior REQUEST_CHANGES feedback has been addressed:
| Issue | Status | Fix |
|---|---|---|
| Colour-coded badges | ✓ Fixed | DOMAIN_BADGE_COLORS with per-domain bg/text classes, applied to rendered badges |
| Agent name in SSE data | ✓ Fixed | agent_hint() method now includes ToolExecutionCompleted, data sent in SSE JSON |
| Auto-reconnect on disconnect | ✓ Fixed | 3s reconnect loop in finally block, properly guards against re-connect after unmount |
| Fragile variant extraction | ✓ Fixed | Proper match-based variant_name() method replaces Debug + split_once parsing |
Configuration-driven settings also now in place: max_entries and new_entries order (top/bottom) are read from dashboard.event_stream config and applied via refs, respecting server settings sent in first SSE event.
Remaining minor findings (CodeRabbit):
- Filename sanitization: Export uses
toISOString().slice(0, 19)which includes colons invalid on Windows — should replace with hyphens or underscores for cross-platform safety - Test completeness: Filter test dispatches change event but does not assert that AgentB is hidden after filtering — add waitFor + queryByText check
Code is solid. Tests cover happy path + disconnect states. Coverage gate passing. Ready for approval once these two are addressed.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
All prior major issues resolved — color badges properly implemented with per-domain Tailwind classes, agent name extraction via agent_hint() in SSE stream, auto-reconnect with 3s backoff, and config-driven entry limits working correctly. CI green, sanil-23 approved.
Two minor items remain unfixed but are non-blocking:
- Filename export timestamp still has colons (invalid on Windows filenames) — suggest:
.replace(/:/g, '-') - Filter test doesn't assert that filtering actually worked (test dispatches change event but never checks AgentB is hidden)
Code is clean and ready to go. Ship it.
graycyrus
dismissed
their stale review
Auto-corrected: policy violation — approval should be COMMENT, not APPROVED. Clean PRs require manual approval gate.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
All prior major issues resolved — color badges properly implemented with per-domain Tailwind classes, agent name extraction via agent_hint() in SSE stream, auto-reconnect with 3s backoff, and config-driven entry limits working correctly. CI green, sanil-23 approved.
Two minor items remain unfixed but are non-blocking:
- Filename export timestamp still has colons (invalid on Windows filenames) — suggest:
.replace(/:/g, '-') - Filter test doesn't assert that filtering actually worked (test dispatches change event but never checks AgentB is hidden)
Code is clean and ready to go.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Addresses graycyrus's two non-blocking review items (PR tinyhumansai#2653): 1. **Filename sanitization** — `toISOString().slice(0, 19)` produced colons, invalid in Windows filenames. Now `.replace(/:/g, '-')` so exports are cross-platform safe. 2. **Filter test completeness** — the domain-filter test dispatched a change event but never asserted the filter took effect. Now uses `fireEvent.change` + `waitFor` checking AgentB is hidden while ToolA remains visible. Co-Authored-By: Claude <noreply@anthropic.com>
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Re-approving after the two follow-up fixes (678957fc): filename sanitization (:/g, '-' on the export timestamp) + filter test now asserts AgentB is hidden + ToolA visible after dispatch. Addresses graycyrus's two non-blocking review items. CR re-reviewed → COMMENTED on prior head; will re-run on this push.
|
@graycyrus addressed your two non-blocking items in
Re-requested your review. Sole remaining CI red is the inherited |
4 participants
Summary
Implements #1849 — live typed event stream log in the dashboard.
Add a live typed event stream log to the OpenHuman dashboard that displays every agent event in real time as a scrolling, colour-coded feed. Each entry shows a timestamp, agent name, event type, and message - giving operators an instant, continuous view of what every agent in the swarm is doing without tailing raw log files.
Changes
Backend (Rust):
GET /events/domain— authenticated SSE endpoint streaming allDomainEventbus eventsDomainEvent::variant_name()— stable variant name extraction (no Debug format coupling)DomainEvent::agent_hint()— best-effort agent/session identifier per eventDashboardConfig+EventStreamConfig— config schema fordashboard.event_stream.{enabled, max_entries, new_entries}event: configmessage so frontend applies server settingsenabled: false(returns 404)Frontend (React):
EventLogPanel.tsx— Settings > Developer Options panelagentfieldmax_entriesandnew_entriesorder from serveri18n: 22 keys across
en.ts,en-5.ts, and all 12 locale chunk files.Acceptance criteria
max_entriesis reachedmax_entriesandnew_entriesorder are read fromconfig.yaml.github/workflows/coverage.yml)Configuration
Closes #1849
Summary by CodeRabbit
New Features
Internationalization
Tests