feat: add log filtering and streaming endpoints - #41
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughLoads .env, computes and validates runtime keys, adds key-test endpoints, repo existence and PowerShell script-run endpoints with JSONL structured logging, adds log-file listing, replaces log reading with JSONL parsing and filtering, adds SSE log stream, and gates callGemini on KEYS.GEMINI. ChangesServer operational APIs
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server
participant PowerShell
participant ServerLogs
Client->>API_Server: POST /api/run-script/:name
API_Server->>PowerShell: spawn pwsh/powershell (script path, cwd=APPS_DIR)
PowerShell-->>API_Server: stdout / stderr chunks
API_Server->>ServerLogs: write JSONL entries (running, output, success/error)
PowerShell-->>API_Server: exit code / error
API_Server-->>Client: JSON { exit, code, error? }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
unified-app/server.js (1)
75-75: ⚡ Quick winAvoid duplicate
GET /api/logsroute declarations.There is another
/api/logshandler at Line 152-165. This earlier handler already sends a response, so the later one is shadowed and can drift from the actual API contract.🤖 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 `@unified-app/server.js` at line 75, There are two declarations of the same Express route app.get('/api/logs') which causes the later handler to be shadowed; remove the duplicate and consolidate any needed behavior into the single handler (keep the original implementation that sends the response and merge any additional logic from the later handler into it). Locate both app.get('/api/logs') handlers in server.js, reconcile their implementations so only one route remains, and ensure the final handler uses the existing req/res flow and preserves any middleware or error handling that both versions relied on.
🤖 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 `@unified-app/server.js`:
- Around line 122-124: The current SSE handler only sets an interval named
interval that writes ping events via res.write and never watches or polls the
log file for new appended data, so live logs never stream after init; modify the
/api/logs/stream handler to attach a file watcher or a polling loop (using
fs.watch or periodic fs.stat + read with a tracked offset) that reads only
appended bytes from the target log file and calls res.write for each new log
line/event, ensure you reuse the existing res.write pattern (instead of only
sending pings), handle client disconnect by clearing interval/watcher/stream,
and keep variable names like interval and res intact so the change is localized
to the SSE handler logic.
- Around line 70-71: The /api/log-files route currently returns CONFIG.logs with
exists hardcoded to true; update the handler (the app.get('/api/log-files', ...)
route) to compute exists by checking the filesystem for each log path from
CONFIG.logs (use fs.existsSync or asynchronous fs.promises.stat with try/catch)
and return { name: l.name, exists: <result> } for each entry; ensure you
reference the log file path property on CONFIG.logs objects and handle missing
files without throwing (treat as exists: false).
- Around line 86-90: The handler currently reads limit from req.query and
directly does parseInt(limit) then entries.slice, which yields surprising
results for invalid/negative values; update the code around req.query/limit to
validate limit before slicing: if limit is undefined, leave entries unchanged;
otherwise parse with parseInt(limit, 10), ensure the parsed value is a
non-negative integer (use Number.isInteger and >= 0 or isNaN check) and if
invalid respond with a 400 client error (bad request) describing the problem;
only call entries.slice(0, n) when the validated n is a safe non-negative
integer.
- Line 106: The SSE response is using the wrong MIME type; update the header set
on the response (the res.setHeader call that currently sets 'Content-Type' to
'text/eventstream') to use the standard 'text/event-stream' value so EventSource
clients accept the stream; locate the res.setHeader invocation in server.js that
sets 'Content-Type' and change the value to 'text/event-stream'.
---
Nitpick comments:
In `@unified-app/server.js`:
- Line 75: There are two declarations of the same Express route
app.get('/api/logs') which causes the later handler to be shadowed; remove the
duplicate and consolidate any needed behavior into the single handler (keep the
original implementation that sends the response and merge any additional logic
from the later handler into it). Locate both app.get('/api/logs') handlers in
server.js, reconcile their implementations so only one route remains, and ensure
the final handler uses the existing req/res flow and preserves any middleware or
error handling that both versions relied on.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d1942f37-1bee-4cbd-a333-7803b24edf4a
📒 Files selected for processing (1)
unified-app/server.js
| app.get('/api/log-files', (req, res) => { | ||
| res.json(CONFIG.logs.map(l => ({ name: l.name, exists: true }))); |
There was a problem hiding this comment.
Compute exists from the filesystem instead of hardcoding true.
/api/log-files currently reports all log files as present, which makes the existence flag inaccurate.
Proposed fix
app.get('/api/log-files', (req, res) => {
- res.json(CONFIG.logs.map(l => ({ name: l.name, exists: true })));
+ res.json(
+ CONFIG.logs.map(l => ({
+ name: l.name,
+ exists: existsSync(join(__dirname, l.path))
+ }))
+ );
});🤖 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 `@unified-app/server.js` around lines 70 - 71, The /api/log-files route
currently returns CONFIG.logs with exists hardcoded to true; update the handler
(the app.get('/api/log-files', ...) route) to compute exists by checking the
filesystem for each log path from CONFIG.logs (use fs.existsSync or asynchronous
fs.promises.stat with try/catch) and return { name: l.name, exists: <result> }
for each entry; ensure you reference the log file path property on CONFIG.logs
objects and handle missing files without throwing (treat as exists: false).
| const { step, status, limit } = req.query; | ||
| let entries = allEntries; | ||
| if (step) entries = entries.filter(e => e.step === step); | ||
| if (status) entries = entries.filter(e => e.status === status); | ||
| if (limit) entries = entries.slice(0, parseInt(limit)); |
There was a problem hiding this comment.
Validate limit before slicing entries.
Invalid or negative values (for example, "abc" or -1) currently produce surprising results instead of a clear client error.
Proposed fix
const { step, status, limit } = req.query;
let entries = allEntries;
if (step) entries = entries.filter(e => e.step === step);
if (status) entries = entries.filter(e => e.status === status);
- if (limit) entries = entries.slice(0, parseInt(limit));
+ if (limit !== undefined) {
+ const parsedLimit = Number(limit);
+ if (!Number.isInteger(parsedLimit) || parsedLimit < 1) {
+ return res.status(400).json({ error: 'limit must be a positive integer' });
+ }
+ entries = entries.slice(0, parsedLimit);
+ }🤖 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 `@unified-app/server.js` around lines 86 - 90, The handler currently reads
limit from req.query and directly does parseInt(limit) then entries.slice, which
yields surprising results for invalid/negative values; update the code around
req.query/limit to validate limit before slicing: if limit is undefined, leave
entries unchanged; otherwise parse with parseInt(limit, 10), ensure the parsed
value is a non-negative integer (use Number.isInteger and >= 0 or isNaN check)
and if invalid respond with a 400 client error (bad request) describing the
problem; only call entries.slice(0, n) when the validated n is a safe
non-negative integer.
|
|
||
| // Stream logs (Server-Sent Events) | ||
| app.get('/api/logs/stream', (req, res) => { | ||
| res.setHeader('Content-Type', 'text/eventstream'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the required HTTP Content-Type header for Server-Sent Events (EventSource) responses?
💡 Result:
The required HTTP Content-Type header for Server-Sent Events (SSE) responses is text/event-stream [1][2][3]. According to the HTML Living Standard and the W3C specification for the EventSource API, servers must use this specific MIME type to correctly identify the response as an event stream [1][3][4]. If a server responds with a different Content-Type, or fails to provide one, the browser's EventSource interface will fail the connection [3][4]. Additionally, the event stream must be encoded as UTF-8 [1][3][4]. While the specification allows for an optional charset parameter (e.g., text/event-stream; charset=utf-8), it is generally unnecessary as UTF-8 is the required default [3].
Citations:
- 1: https://html.spec.whatwg.org/dev/server-sent-events.html
- 2: https://developer.mozilla.org/en-US/docs/Server-sent_events/Using_server-sent_events
- 3: https://www.w3.org/TR/2015/REC-eventsource-20150203
- 4: https://html.spec.whatwg.org/multipage/server-sent-events.html
🏁 Script executed:
# First, find the server.js file and verify the content at line 106
find . -name "server.js" -type f | head -20Repository: atomeam/atomarcade-bridge
Length of output: 93
🏁 Script executed:
# Search for instances of Content-Type headers related to event streams
rg "text/event" -A 1 -B 1Repository: atomeam/atomarcade-bridge
Length of output: 278
🏁 Script executed:
# Specifically search for the incorrect and correct SSE header variants
rg "text/eventstream|text/event-stream" -nRepository: atomeam/atomarcade-bridge
Length of output: 148
🏁 Script executed:
# Get more context around both occurrences
sed -n '104,110p' unified-app/server.js
sed -n '156,162p' unified-app/server.jsRepository: atomeam/atomarcade-bridge
Length of output: 423
🏁 Script executed:
# Check if there are any other instances we might have missed
rg "eventstream" -nRepository: atomeam/atomarcade-bridge
Length of output: 148
🏁 Script executed:
# Also check the exact line numbers and their context for line 158
sed -n '156,162p' unified-app/server.jsRepository: atomeam/atomarcade-bridge
Length of output: 304
🏁 Script executed:
# Count total occurrences and show them with line numbers
wc -l unified-app/server.js
rg "text/eventstream" -n --no-headingRepository: atomeam/atomarcade-bridge
Length of output: 174
🏁 Script executed:
# Show the exact line 106 to verify the review comment's accuracy
sed -n '106p' unified-app/server.jsRepository: atomeam/atomarcade-bridge
Length of output: 83
🏁 Script executed:
# Let me get a broader view around where the incorrect headers appear
sed -n '150,165p' unified-app/server.jsRepository: atomeam/atomarcade-bridge
Length of output: 563
🏁 Script executed:
# Double-check if there are any other SSE-related endpoints with this issue
rg "get\(|post\(" -A 5 | rg -B 5 "eventstream"Repository: atomeam/atomarcade-bridge
Length of output: 422
Fix SSE Content-Type header to the standard text/event-stream at line 158.
The header text/eventstream is incorrect and violates the EventSource specification. Per W3C standards and the HTML Living Standard, the correct MIME type for Server-Sent Events responses is text/event-stream. Using the wrong header causes EventSource clients to reject the stream and fail the connection.
Proposed fix
- res.setHeader('Content-Type', 'text/eventstream');
+ res.setHeader('Content-Type', 'text/event-stream');📝 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.
| res.setHeader('Content-Type', 'text/eventstream'); | |
| res.setHeader('Content-Type', 'text/event-stream'); |
🤖 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 `@unified-app/server.js` at line 106, The SSE response is using the wrong MIME
type; update the header set on the response (the res.setHeader call that
currently sets 'Content-Type' to 'text/eventstream') to use the standard
'text/event-stream' value so EventSource clients accept the stream; locate the
res.setHeader invocation in server.js that sets 'Content-Type' and change the
value to 'text/event-stream'.
| const interval = setInterval(() => { | ||
| res.write(`data: ${JSON.stringify({ type: 'ping', ts: new Date().toISOString() })}\n\n`); | ||
| }, 15000); |
There was a problem hiding this comment.
/api/logs/stream is heartbeat-only after init and does not stream new logs.
After the initial payload, the interval only sends ping; no file watch/poll logic emits appended log entries, so this does not satisfy live log streaming behavior.
🤖 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 `@unified-app/server.js` around lines 122 - 124, The current SSE handler only
sets an interval named interval that writes ping events via res.write and never
watches or polls the log file for new appended data, so live logs never stream
after init; modify the /api/logs/stream handler to attach a file watcher or a
polling loop (using fs.watch or periodic fs.stat + read with a tracked offset)
that reads only appended bytes from the target log file and calls res.write for
each new log line/event, ensure you reuse the existing res.write pattern
(instead of only sending pings), handle client disconnect by clearing
interval/watcher/stream, and keep variable names like interval and res intact so
the change is localized to the SSE handler logic.
1 participant
Summary
?step=observer&status=running&limit=10GET /api/logs/stream/api/log-filesfor configEndpoints
GET /api/logs— Read logs with filtersGET /api/logs?status=running— Filter by statusGET /api/logs?step=evaluator— Filter by stepGET /api/logs?limit=5— Limit resultsGET /api/logs/stream— SSE live streamGET /api/log-files— Config list@atomeam can click here to continue refining the PR
Summary by CodeRabbit
New Features
Bug Fixes
Chores