feat: per-test device log capture with interactive UI#69
Merged
arnoldlaishram merged 12 commits intomainfrom Apr 8, 2026
Merged
feat: per-test device log capture with interactive UI#69arnoldlaishram merged 12 commits intomainfrom
arnoldlaishram merged 12 commits intomainfrom
Conversation
Capture device logs (Android logcat, iOS log stream) for each test, mirroring the existing recording pipeline. Logs are streamed to disk, redacted for secrets, and saved as report artifacts. The report UI is redesigned with a Jam.dev-inspired layout: video player as the hero on the left, tabbed panel on the right with Actions and Device Logs tabs. Each log line is clickable to seek the video to the corresponding timestamp, with bidirectional sync (seekbar scrub highlights nearest log line). Pre-recording log lines are filtered out to maintain timing parity. - Bump RunManifest.schemaVersion from 2 to 2|3 - Add LogCaptureManager/Provider mirroring RecordingManager/Provider - Add AndroidLogcatProvider (adb logcat -v threadtime) - Add IOSLogProvider (xcrun simctl spawn log stream --style compact) - Add deviceLog fields to TestResult, DeviceAgent interface - Integrate log capture in sessionRunner and testRunner - Redact secrets in reportWriter using existing redactResolvedValue - Restructure report layout: video-panel (left) + tabs-panel (right) - Add interactive clickable log lines with timestamp parsing - Add reverse sync: video seekbar highlights nearest log line Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add driver process death detection and automatic restart for iOS simulator when XCUITest runner gets suspended/killed during app launches - Add gRPC retry logic with linear backoff for transient connection errors - Fix empty iOS device logs by resolving CFBundleExecutable name from bundle ID (e.g. org.wikimedia.wikipedia → Wikipedia) for log stream process predicate filtering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add search input for real-time log filtering (case-insensitive) - Add level filter chips (All / Errors / Warnings) with color coding - Style scrollbar for log container (webkit) - Color-code log lines by level (red border for errors, amber for warnings) - Add Cmd+F keyboard shortcut to focus search when logs tab is active - Filter noisy Apple subsystems (xctest, UIKit, BackBoard) from iOS logs - Applied to both CLI reportTemplate and report-web renderers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds per-test device log capture (Android/iOS): provider interfaces and implementations, LogCaptureManager, Device integration and gRPC injection; CLI start/stop/abort wiring and reportWriter artifact copy + redact; manifest schema bump to v3; report-web/server-side tailing and UI for device logs. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Device as Device
participant Manager as LogCaptureManager
participant Provider as LogCaptureProvider
participant OS as OS (adb/xcrun)
participant Writer as ReportWriter
participant Web as ReportWeb
Runner->>Device: startLogCapture(runId, testId)
Device->>Manager: startLogCapture(params)
Manager->>Provider: checkAvailability()
Provider->>OS: which adb / which xcrun
OS-->>Provider: available
Manager->>Provider: startLogCapture(deviceId, outputPath)
Provider->>OS: spawn log stream
OS->>Provider: stdout -> file
Provider-->>Manager: {process,response}
Manager-->>Device: success
Runner->>Device: stopLogCapture(runId, testId)
Device->>Manager: stopLogCapture(runId, testId)
Manager->>Provider: stopLogCapture(process)
Provider->>OS: send SIGINT, wait exit
Provider->>Manager: success + timestamps
Manager-->>Device: {filePath, timestamps}
Writer->>Manager: read captured file (tmp)
Writer->>Writer: redactResolvedValue(bindings)
Writer->>Writer: write device.log into report/tests/<id> (if retained)
Web->>Writer: load manifest (schema 2|3)
Web->>Writer: readDeviceLogTail -> deviceLogTailText
Web-->>User: render device-log details + download
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/device-node/src/device/ios/IOSSimulator.ts (1)
65-115:⚠️ Potential issue | 🟡 MinorWrap
back()in the same recovery path.Lines 66-115 move the other gRPC-backed gestures behind
_withDriverRecovery, butback()in this block still callsCommonDriverActions.back()directly. After the XCUITest runner dies, a back action will bypass the new restart logic and fail while the neighboring actions recover.Suggested adjustment
async back(_action: BackAction): Promise<DeviceNodeResponse> { - return await this._commonDriverActions.back(); + return this._withDriverRecovery('back', () => this._commonDriverActions.back()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/device-node/src/device/ios/IOSSimulator.ts` around lines 65 - 115, The back() method currently calls this._commonDriverActions.back() directly and bypasses the new recovery logic; change it to return this._withDriverRecovery('back', () => this._commonDriverActions.back()) so the back action is executed through the same _withDriverRecovery path (preserve the Promise<DeviceNodeResponse> return and use the same string key 'back' to match other calls).packages/report-web/src/renderers.ts (1)
1686-1750:⚠️ Potential issue | 🟠 MajorThe report-web player still doesn't move the active log line during playback.
highlightNearestLogLine()is only invoked from the manual seekbar path, so thetimeupdate/playcallbacks and the step-selection seek never refresh the active row. That leaves the Device Logs tab out of sync as soon as the user presses play or clicks an action.Suggested fix
const syncControls = () => { syncRecordingShell(container, video); updateRecordingControls(container, video); + highlightNearestLogLine(container, video.currentTime); }; @@ syncRecordingShell(container, video); updateRecordingControls(container, video); + highlightNearestLogLine(container, clampedSeconds); };Also applies to: 1814-1828
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/report-web/src/renderers.ts` around lines 1686 - 1750, syncControls currently updates UI but doesn't refresh the active log line, so playback/time updates and step-selection seeks leave Device Logs out of sync; modify syncControls (or the event handlers it calls) to call highlightNearestLogLine(container, video.currentTime) after syncing controls, and also ensure any call sites that call selectNearestStepForTime(testId, nextTime) (e.g., applySeek) also call highlightNearestLogLine(container, nextTime) so the log row is updated during timeupdate/play and step-seeking paths.packages/cli/src/reportTemplate.ts (1)
1427-1489:⚠️ Potential issue | 🟠 MajorThe active log row only updates on manual scrub.
highlightNearestLogLine()is called from the seekbar handler on Line 1447, but thetimeupdate/playlisteners on Lines 1475-1482 only runsyncControls(), and the seek insidesyncRecording()on Lines 1555-1569 also omits it. Pressing play or selecting an action therefore leaves the log pane stale instead of following the video.Also applies to: 1555-1569
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/reportTemplate.ts` around lines 1427 - 1489, The active log row isn't updated during playback because highlightNearestLogLine(...) is only called from applySeek; update syncControls() to call highlightNearestLogLine(container, video.currentTime) (using the same testId lookup as applySeek: container.getAttribute('data-step-detail')) so all events that trigger syncControls (loadedmetadata, timeupdate, play, pause, ratechange, etc.) update the log; also add the same highlightNearestLogLine call into the syncRecording/syncRecordingShell flow (the function referenced as syncRecordingShell and the syncRecording implementation around lines 1555-1569) wherever the video time is advanced programmatically so the log pane follows seeks done by those functions as well.
🧹 Nitpick comments (5)
packages/device-node/src/device/LogInfo.ts (1)
26-28: MakemarkAsEnded()idempotent.Line 27 overwrites
endTimeon every call. If two teardown paths touch the sameLogInfo,completedAtcan drift later than the real stop time. Preserve the first terminal timestamp instead.Small hardening diff
markAsEnded(): void { - this.endTime = new Date(); + this.endTime ??= new Date(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/device-node/src/device/LogInfo.ts` around lines 26 - 28, markAsEnded currently overwrites the terminal timestamp each call; make it idempotent by only setting the first terminal time — in LogInfo.markAsEnded, check the terminal field (endTime / completedAt) and if it's already set (non-null/undefined) return without changing it, otherwise assign new Date(); this preserves the original stop time if multiple teardown paths call markAsEnded.packages/cli/src/testRunner.ts (1)
295-304: Small maintainability win: computerunIdonce per test iteration.Line 301 repeats the same
path.basename(runDir)used on Line 295. Hoisting it improves readability and avoids drift.♻️ Proposed refactor
+ const runId = path.basename(runDir); const goalResult = await testRunnerDependencies.executeTestOnSession(goalSession, { goal, @@ recording: { - runId: path.basename(runDir), + runId, testId: test.testId!, outputFilePath: recordingOutputPath, keepPartialOnFailure: true, }, deviceLog: { - runId: path.basename(runDir), + runId, testId: test.testId!, keepPartialOnFailure: true, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/testRunner.ts` around lines 295 - 304, The code repeats path.basename(runDir) for runId in both the recording and deviceLog objects; to fix, compute const runId = path.basename(runDir) once at the start of the test iteration (near where runDir is in scope) and replace both occurrences with runId so recording.output and deviceLog use the same hoisted variable (update references around the object literals that currently set runId: path.basename(runDir)).packages/report-web/src/artifacts.ts (1)
290-299: Minor: Function name suggests tail reading but reads entire file.The
readDeviceLogTailfunction reads the complete device log file. If tail truncation is intended (e.g., last N lines for performance), this should be implemented. Otherwise, consider renaming toreadDeviceLogTextfor clarity.Also, lines 295-298 can be simplified since
readRunArtifactTextalready returnsundefinedon empty/missing content:♻️ Simplified implementation
const readDeviceLogTail = async (deviceLogPath: string | undefined): Promise<string | undefined> => { if (!deviceLogPath) { return undefined; } - const content = await readRunArtifactText(context, runId, deviceLogPath); - if (!content) { - return undefined; - } - return content; + return await readRunArtifactText(context, runId, deviceLogPath); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/report-web/src/artifacts.ts` around lines 290 - 299, The function readDeviceLogTail misleadingly reads the entire log; either implement true tail behavior (e.g., call readRunArtifactText(context, runId, deviceLogPath) then truncate to the last N lines/bytes before returning) or rename it to readDeviceLogText and update its callers; in either case simplify the body to directly return the result of readRunArtifactText (remove the redundant content checks) and, if you implement truncation, document/parameterize the N and ensure readRunArtifactText is still called to fetch the source content.packages/cli/src/reportServer.ts (1)
305-311: Same observation asartifacts.ts: function reads entire file despite "Tail" name.For consistency with
artifacts.ts, consider the same simplification:♻️ Simplified implementation
const readDeviceLogTail = async (deviceLogPath: string): Promise<string | undefined> => { - const content = await readRunArtifactText(artifactsDir, runId, deviceLogPath); - if (!content) { - return undefined; - } - return content; + return await readRunArtifactText(artifactsDir, runId, deviceLogPath); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/reportServer.ts` around lines 305 - 311, The readDeviceLogTail function currently reads the entire artifact then checks for empty content, which is redundant and mismatches the "Tail" name; simplify it by returning the Promise from readRunArtifactText directly (i.e., replace the body of readDeviceLogTail with a single return readRunArtifactText(artifactsDir, runId, deviceLogPath)) so the function signature and behavior remain string|undefined and match the simplified pattern used in artifacts.ts.fab/changes/260405-g0d1-capture-device-logs/spec.md (1)
316-323: Optional wording polish: hyphenate compound adjective in scenario titles.Use “device-log” when used adjectivally in headings (e.g., “device-log report section”) for consistency with style linting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fab/changes/260405-g0d1-capture-device-logs/spec.md` around lines 316 - 323, Update the scenario headings to hyphenate the compound adjective “device-log” for consistency and style linting: change the titles "Scenario: Test with device log in report" and "Scenario: Test without device log in report" to use "device-log" (e.g., "Scenario: Test with device-log in report") and ensure any adjectival uses in this spec (headings or section labels) follow the same "device-log" form to match the CSS class/device identifier `device-log`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/memory/report-web/renderers.md`:
- Around line 13-24: Update the docs/memory/report-web/renderers.md content that
describes the device log output to match the current renderer behavior in
packages/report-web/src/renderers.ts and packages/cli/src/reportTemplate.ts:
replace the old inline <details class="device-log"> description with a
description of the tabbed "Actions" / "Device Logs" workspace (Device Logs tab
contains the tail of the log (~last ~200 lines, HTML-escaped) inside a pre,
includes search/filter controls and a "Download full log" link), and note its
position after the recording video section; make the same replacement for the
mirrored text at lines 37-39 so the doc reflects the new DOM shape.
In `@fab/changes/260405-g0d1-capture-device-logs/.status.yaml`:
- Around line 16-20: Update the checklist.completed value in .status.yaml to
reflect the true number of completed items (set checklist.completed to 34) so it
matches the actual checklist in
fab/changes/260405-g0d1-capture-device-logs/checklist.md; ensure the
checklist.total remains 34 and verify checklist.generated/path entries are
unchanged so any automation reading checklist.completed sees the change as
complete.
In `@fab/changes/260405-g0d1-capture-device-logs/intake.md`:
- Line 15: Update the simctl example so it includes the simulator target (use
the same "<udid>" or "booted" form used elsewhere in the doc) — replace the
incomplete `xcrun simctl spawn log stream` example with one that specifies the
simulator (e.g., `xcrun simctl spawn <udid|booted> log stream`) so the command
is copy-pastable and runnable; locate the example string "xcrun simctl spawn log
stream" in the intake.md content and add the simulator target accordingly.
In `@fab/changes/260405-g0d1-capture-device-logs/spec.md`:
- Around line 307-345: The spec and scenarios currently require an inline
<details> device-log block rendered by reportTemplate.ts (including server-side
tailing, splitting on '\n', taking last 200 lines, HTML-escaping via escapeHtml,
and linking deviceLogRoute) but the PR implements a two-panel tabbed interactive
viewer with synchronized video-log behavior; update the spec and scenarios to
reflect the delivered UX by replacing the inline-<details> requirements with
descriptions of the tabbed viewer (mentioning the recording panel and the log
panel), the synchronization contract (click-to-seek and video time -> log
highlighting), and that reportTemplate.ts should render the tabbed UI and still
perform server-side tailing/escapeHtml for the log content and provide a
download link for the full deviceLogFile; ensure test scenarios cover
presence/absence of deviceLogFile, the server-side tail read behavior (last 200
lines), and the interactive synchronization behaviors.
- Around line 9-13: The "Non-goals" section in spec.md currently lists "Log
filtering or search in the report viewer", "Log level colorization or syntax
highlighting in the `<pre>` block", "User-configurable log format", and
"Streaming logs to the report in real-time" which contradicts the PR's shipped
scope; update the Non-goals paragraph in
fab/changes/260405-g0d1-capture-device-logs/spec.md by removing or revising any
bullet that contradicts the implemented features (e.g., remove the lines "Log
filtering or search in the report viewer", "Log level colorization or syntax
highlighting in the `<pre>` block", and any other items that are actually
implemented), and replace them with accurate exclusions or a short clarifying
sentence that maps each implemented capability (search/filter, colorization,
format options, streaming) to whether it is in-scope or out-of-scope so
acceptance criteria align with the shipped scope.
- Line 365: The Markdown table cell contains the unescaped pipe in the schema
version literal `2 | 3`, which breaks table parsing; update the cell to escape
the pipe (e.g., `2 \| 3`) or wrap the value in backticks (e.g., `2 | 3`) so the
row (the entry referencing RunManifest.ts and the literal `2`) renders correctly
and avoids MD056.
In `@packages/cli/src/reportTemplate.ts`:
- Around line 2211-2269: The Android timestamp parsing currently uses the
current year inside parseLogTimestamp which causes year drift; change
parseLogTimestamp to accept an optional base date/year (e.g.,
baseDateOrTimestamp?: string | Date) and, when parsing the MM-DD HH:MM:SS.mmm
format, derive the year from that base (or choose the nearest year to the base)
instead of new Date().getFullYear(); then update renderDeviceLogLines to pass
recordingStartedAt (or a Date created from it) into parseLogTimestamp for both
filtering and rendering so all comparisons and data-log-ts values use the
inferred year consistently.
In `@packages/cli/src/sessionRunner.ts`:
- Around line 471-515: The code clears activeLogCapture regardless of whether
stopLogCapture succeeded, which can leak a running capture when stopLogCapture
throws; modify the exception path so it mirrors the failure branch: in the catch
for stopLogCapture call session.device.abortLogCapture(activeLogCapture.runId,
activeLogCapture.keepPartialOnFailure) (handling errors with Logger.w), and only
set activeLogCapture = undefined after the abort attempt completes; ensure
references are to activeLogCapture, session.device.stopLogCapture, and
session.device.abortLogCapture so the cleanup always runs before clearing the
handle.
In `@packages/device-node/src/device/Device.ts`:
- Around line 280-284: The current logic falls back to request.appIdentifier
when this._runtime.resolveLogFilterIdentifier returns null, which restores an
incorrect iOS log filter; change the flow in the Device.ts logic around
logIdentifier so that after calling
this._runtime.resolveLogFilterIdentifier(logIdentifier) you do not reuse
request.appIdentifier if resolved is falsy — instead emit a warning (via the
existing logger) and set logIdentifier to undefined/null (or throw to fail
start) so the iOS log-stream predicate is dropped; update references to
resolveLogFilterIdentifier, logIdentifier and request.appIdentifier accordingly
to locate and change the behavior.
In `@packages/device-node/src/device/LogCaptureManager.ts`:
- Around line 85-118: The code currently creates temp files and records
LogInfo/map state before validating the provider; call and await
provider.checkAvailability() immediately after retrieving provider (right after
const provider = this._providers.get(params.platform)) and if it returns false
or throws return a DeviceNodeResponse with success:false and a clear message,
before creating logDir, sanitized filenames, LogInfo, or updating
this._logInfoMap / this._deviceToLogKeysMap; also move the creation/setting of
LogInfo and the device->keys map to after provider.startLogCapture has
successfully completed so state is only recorded for truly started captures, and
catch/propagate provider.startLogCapture errors into a failure
DeviceNodeResponse.
In `@packages/device-node/src/grpc/GrpcDriverClient.ts`:
- Around line 332-359: The _unaryCall wrapper currently defaults to retrying
(maxRetries = 2) which can cause duplicate execution of mutating RPCs; change
the default to no retries by setting maxRetries to 0 in the _unaryCall
implementation and update its doc comment to state callers must opt-in for
retries (e.g., getDeviceScale should pass maxRetries: 2); keep the existing
linear backoff logic and error handling unchanged, and audit callers of
_unaryCall (methods like tap, enterText, swipe, launchApp, killApp) to ensure
they do not pass a retry value unless the operation is idempotent or explicitly
safe to retry.
In `@packages/device-node/src/grpc/setup/IOSSimulatorSetup.ts`:
- Around line 149-197: The restart logic in restartDriver currently closes
grpcClient and starts a new driver process but does not replace the closed
transport used by CommonDriverActions (wired in GrpcDriverSetup and exercised by
IOSSimulator._withDriverRecovery), so subsequent retries still use the dead
client; fix by creating a fresh gRPC client after start (the same kind created
originally), wait until _connectWithPolling succeeds against the new client,
then swap/replace the client instance that GrpcDriverSetup/CommonDriverActions
uses (so callers use the refreshed transport) before calling
waitForDriverCaptureReadiness/_updateIOSAppIds and before returning from
restartDriver; locate restartDriver, grpcClient, _connectWithPolling,
_trackIOSDriverProcess, _updateIOSAppIds, and ensure the replacement happens
atomically so _withDriverRecovery observes the new client.
In `@packages/report-web/src/renderers.ts`:
- Around line 2603-2661: The Android MM-DD parser in parseLogTimestamp currently
uses the viewer clock year (new Date().getFullYear()), which breaks logs viewed
later; change parseLogTimestamp to accept an optional base timestamp or
recordingStartedAt parameter and, when androidMatch is found, derive the year
from new Date(recordingStartedAt).getFullYear() if provided (fall back to
current year only if recordingStartedAt is undefined). Update
renderDeviceLogLines to pass recordingStartedAt into parseLogTimestamp (both in
the filter and map calls) so all timestamp comparisons and data-log-ts
attributes use the run's year context.
---
Outside diff comments:
In `@packages/cli/src/reportTemplate.ts`:
- Around line 1427-1489: The active log row isn't updated during playback
because highlightNearestLogLine(...) is only called from applySeek; update
syncControls() to call highlightNearestLogLine(container, video.currentTime)
(using the same testId lookup as applySeek:
container.getAttribute('data-step-detail')) so all events that trigger
syncControls (loadedmetadata, timeupdate, play, pause, ratechange, etc.) update
the log; also add the same highlightNearestLogLine call into the
syncRecording/syncRecordingShell flow (the function referenced as
syncRecordingShell and the syncRecording implementation around lines 1555-1569)
wherever the video time is advanced programmatically so the log pane follows
seeks done by those functions as well.
In `@packages/device-node/src/device/ios/IOSSimulator.ts`:
- Around line 65-115: The back() method currently calls
this._commonDriverActions.back() directly and bypasses the new recovery logic;
change it to return this._withDriverRecovery('back', () =>
this._commonDriverActions.back()) so the back action is executed through the
same _withDriverRecovery path (preserve the Promise<DeviceNodeResponse> return
and use the same string key 'back' to match other calls).
In `@packages/report-web/src/renderers.ts`:
- Around line 1686-1750: syncControls currently updates UI but doesn't refresh
the active log line, so playback/time updates and step-selection seeks leave
Device Logs out of sync; modify syncControls (or the event handlers it calls) to
call highlightNearestLogLine(container, video.currentTime) after syncing
controls, and also ensure any call sites that call
selectNearestStepForTime(testId, nextTime) (e.g., applySeek) also call
highlightNearestLogLine(container, nextTime) so the log row is updated during
timeupdate/play and step-seeking paths.
---
Nitpick comments:
In `@fab/changes/260405-g0d1-capture-device-logs/spec.md`:
- Around line 316-323: Update the scenario headings to hyphenate the compound
adjective “device-log” for consistency and style linting: change the titles
"Scenario: Test with device log in report" and "Scenario: Test without device
log in report" to use "device-log" (e.g., "Scenario: Test with device-log in
report") and ensure any adjectival uses in this spec (headings or section
labels) follow the same "device-log" form to match the CSS class/device
identifier `device-log`.
In `@packages/cli/src/reportServer.ts`:
- Around line 305-311: The readDeviceLogTail function currently reads the entire
artifact then checks for empty content, which is redundant and mismatches the
"Tail" name; simplify it by returning the Promise from readRunArtifactText
directly (i.e., replace the body of readDeviceLogTail with a single return
readRunArtifactText(artifactsDir, runId, deviceLogPath)) so the function
signature and behavior remain string|undefined and match the simplified pattern
used in artifacts.ts.
In `@packages/cli/src/testRunner.ts`:
- Around line 295-304: The code repeats path.basename(runDir) for runId in both
the recording and deviceLog objects; to fix, compute const runId =
path.basename(runDir) once at the start of the test iteration (near where runDir
is in scope) and replace both occurrences with runId so recording.output and
deviceLog use the same hoisted variable (update references around the object
literals that currently set runId: path.basename(runDir)).
In `@packages/device-node/src/device/LogInfo.ts`:
- Around line 26-28: markAsEnded currently overwrites the terminal timestamp
each call; make it idempotent by only setting the first terminal time — in
LogInfo.markAsEnded, check the terminal field (endTime / completedAt) and if
it's already set (non-null/undefined) return without changing it, otherwise
assign new Date(); this preserves the original stop time if multiple teardown
paths call markAsEnded.
In `@packages/report-web/src/artifacts.ts`:
- Around line 290-299: The function readDeviceLogTail misleadingly reads the
entire log; either implement true tail behavior (e.g., call
readRunArtifactText(context, runId, deviceLogPath) then truncate to the last N
lines/bytes before returning) or rename it to readDeviceLogText and update its
callers; in either case simplify the body to directly return the result of
readRunArtifactText (remove the redundant content checks) and, if you implement
truncation, document/parameterize the N and ensure readRunArtifactText is still
called to fetch the source content.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48a76439-2752-4d2f-8a1b-61827d135738
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
docs/memory/cli/index.mddocs/memory/cli/report-writer.mddocs/memory/device-node/index.mddocs/memory/device-node/log-capture.mddocs/memory/index.mddocs/memory/report-web/index.mddocs/memory/report-web/renderers.mdfab/changes/260405-g0d1-capture-device-logs/.history.jsonlfab/changes/260405-g0d1-capture-device-logs/.status.yamlfab/changes/260405-g0d1-capture-device-logs/checklist.mdfab/changes/260405-g0d1-capture-device-logs/intake.mdfab/changes/260405-g0d1-capture-device-logs/spec.mdfab/changes/260405-g0d1-capture-device-logs/tasks.mdpackages/cli/src/reportServer.tspackages/cli/src/reportTemplate.tspackages/cli/src/reportWriter.tspackages/cli/src/runIndex.tspackages/cli/src/sessionRunner.tspackages/cli/src/testRunner.tspackages/common/src/index.tspackages/common/src/interfaces/DeviceAgent.tspackages/common/src/models/DeviceLog.tspackages/common/src/models/RunManifest.tspackages/common/src/models/TestResult.tspackages/device-node/src/device/AndroidLogcatProvider.tspackages/device-node/src/device/Device.tspackages/device-node/src/device/IOSLogProvider.tspackages/device-node/src/device/LogCaptureManager.tspackages/device-node/src/device/LogCaptureProvider.tspackages/device-node/src/device/LogInfo.tspackages/device-node/src/device/ios/IOSSimulator.test.tspackages/device-node/src/device/ios/IOSSimulator.tspackages/device-node/src/device/shared/DeviceRuntime.tspackages/device-node/src/grpc/GrpcDriverClient.tspackages/device-node/src/grpc/GrpcDriverSetup.tspackages/device-node/src/grpc/setup/IOSSimulatorSetup.tspackages/device-node/src/index.tspackages/device-node/src/infra/ios/SimctlClient.tspackages/goal-executor/src/TestExecutor.tspackages/report-web/src/artifacts.tspackages/report-web/src/renderers.ts
Comment on lines
+13
to
+24
| When `deviceLogFile` is present on a test, the renderer outputs: | ||
|
|
||
| ```html | ||
| <details class="device-log"> | ||
| <summary>Device log (tail)</summary> | ||
| <pre>{last ~200 lines, HTML-escaped}</pre> | ||
| <a href="{deviceLogRoute}" download>Download full log</a> | ||
| </details> | ||
| ``` | ||
|
|
||
| This block is positioned after the recording video section. When `deviceLogFile` is undefined, nothing is rendered. | ||
|
|
There was a problem hiding this comment.
Refresh this memory doc to match the current renderer output.
Lines 13-24 and 37-39 still describe an inline <details class="device-log"> block, but packages/report-web/src/renderers.ts and packages/cli/src/reportTemplate.ts now render a tabbed Actions / Device Logs workspace with search/filter controls. Leaving the old structure here will point follow-up changes at the wrong DOM shape.
Also applies to: 37-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/memory/report-web/renderers.md` around lines 13 - 24, Update the
docs/memory/report-web/renderers.md content that describes the device log output
to match the current renderer behavior in packages/report-web/src/renderers.ts
and packages/cli/src/reportTemplate.ts: replace the old inline <details
class="device-log"> description with a description of the tabbed "Actions" /
"Device Logs" workspace (Device Logs tab contains the tail of the log (~last
~200 lines, HTML-escaped) inside a pre, includes search/filter controls and a
"Download full log" link), and note its position after the recording video
section; make the same replacement for the mirrored text at lines 37-39 so the
doc reflects the new DOM shape.
Comment on lines
+16
to
+20
| checklist: | ||
| generated: true | ||
| path: checklist.md | ||
| completed: 0 | ||
| total: 34 |
There was a problem hiding this comment.
Synchronize checklist.completed with the actual checklist.
Line 19 still says completed: 0, but fab/changes/260405-g0d1-capture-device-logs/checklist.md already has all 34 checks marked off. Any automation that trusts .status.yaml will treat this change as incomplete.
Suggested fix
checklist:
generated: true
path: checklist.md
- completed: 0
+ completed: 34
total: 34📝 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
| checklist: | |
| generated: true | |
| path: checklist.md | |
| completed: 0 | |
| total: 34 | |
| checklist: | |
| generated: true | |
| path: checklist.md | |
| completed: 34 | |
| total: 34 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fab/changes/260405-g0d1-capture-device-logs/.status.yaml` around lines 16 -
20, Update the checklist.completed value in .status.yaml to reflect the true
number of completed items (set checklist.completed to 34) so it matches the
actual checklist in fab/changes/260405-g0d1-capture-device-logs/checklist.md;
ensure the checklist.total remains 34 and verify checklist.generated/path
entries are unchanged so any automation reading checklist.completed sees the
change as complete.
|
|
||
| ## Why | ||
|
|
||
| 1. **Problem**: When a test fails, users must reproduce locally and manually run `adb logcat` / `xcrun simctl spawn log stream` to see what the OS and app printed. This adds significant debugging time and requires device access. |
There was a problem hiding this comment.
Fix the simctl example so it is runnable.
Line 15 omits the simulator target, so xcrun simctl spawn log stream cannot be copy-pasted as written. Use the same <udid> or booted form referenced later in this doc.
Suggested edit
- 1. **Problem**: When a test fails, users must reproduce locally and manually run `adb logcat` / `xcrun simctl spawn log stream` to see what the OS and app printed. This adds significant debugging time and requires device access.
+ 1. **Problem**: When a test fails, users must reproduce locally and manually run `adb logcat` / `xcrun simctl spawn <udid> log stream` to see what the OS and app printed. This adds significant debugging time and requires device access.📝 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
| 1. **Problem**: When a test fails, users must reproduce locally and manually run `adb logcat` / `xcrun simctl spawn log stream` to see what the OS and app printed. This adds significant debugging time and requires device access. | |
| 1. **Problem**: When a test fails, users must reproduce locally and manually run `adb logcat` / `xcrun simctl spawn <udid> log stream` to see what the OS and app printed. This adds significant debugging time and requires device access. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fab/changes/260405-g0d1-capture-device-logs/intake.md` at line 15, Update the
simctl example so it includes the simulator target (use the same "<udid>" or
"booted" form used elsewhere in the doc) — replace the incomplete `xcrun simctl
spawn log stream` example with one that specifies the simulator (e.g., `xcrun
simctl spawn <udid|booted> log stream`) so the command is copy-pastable and
runnable; locate the example string "xcrun simctl spawn log stream" in the
intake.md content and add the simulator target accordingly.
Comment on lines
+9
to
+13
| - Log filtering or search in the report viewer — full-text search is out of scope | ||
| - Log level colorization or syntax highlighting in the `<pre>` block | ||
| - User-configurable log format (threadtime / compact are fixed per platform) | ||
| - Streaming logs to the report in real-time during test execution | ||
|
|
There was a problem hiding this comment.
Non-goals contradict the documented shipped scope.
This section excludes search/filter/colorization, but the PR objective explicitly includes those capabilities. Please align this spec section with the actual feature scope to avoid invalid acceptance criteria.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fab/changes/260405-g0d1-capture-device-logs/spec.md` around lines 9 - 13, The
"Non-goals" section in spec.md currently lists "Log filtering or search in the
report viewer", "Log level colorization or syntax highlighting in the `<pre>`
block", "User-configurable log format", and "Streaming logs to the report in
real-time" which contradicts the PR's shipped scope; update the Non-goals
paragraph in fab/changes/260405-g0d1-capture-device-logs/spec.md by removing or
revising any bullet that contradicts the implemented features (e.g., remove the
lines "Log filtering or search in the report viewer", "Log level colorization or
syntax highlighting in the `<pre>` block", and any other items that are actually
implemented), and replace them with accurate exclusions or a short clarifying
sentence that maps each implemented capability (search/filter, colorization,
format options, streaming) to whether it is in-scope or out-of-scope so
acceptance criteria align with the shipped scope.
Comment on lines
+307
to
+345
| The report SHALL render an inline collapsible `<details>` section for each test that has a device log: | ||
| ```html | ||
| <details class="device-log"> | ||
| <summary>Device log (tail)</summary> | ||
| <pre>{last ~200 lines, HTML-escaped}</pre> | ||
| <a href="{deviceLogRoute}" download>Download full log</a> | ||
| </details> | ||
| ``` | ||
|
|
||
| #### Scenario: Test with device log in report | ||
| - **GIVEN** a test result with `deviceLogFile` set | ||
| - **WHEN** the HTML report is rendered | ||
| - **THEN** a `<details class="device-log">` element appears | ||
| - **AND** the `<pre>` contains the last ~200 lines of the log, HTML-escaped | ||
| - **AND** a download link points to the full log file | ||
|
|
||
| #### Scenario: Test without device log in report | ||
| - **GIVEN** a test result with `deviceLogFile` undefined | ||
| - **WHEN** the HTML report is rendered | ||
| - **THEN** no device log section is rendered for that test | ||
|
|
||
| ### Requirement: Server-side tail read | ||
| The tail read SHALL happen server-side during HTML rendering: read the device log file, split on `\n`, take the last 200 lines, and HTML-escape the content. | ||
|
|
||
| #### Scenario: Log file with 500 lines | ||
| - **GIVEN** a device.log file with 500 lines | ||
| - **WHEN** the report HTML is generated | ||
| - **THEN** the `<pre>` block contains exactly the last 200 lines | ||
|
|
||
| ## CLI: Report Template | ||
|
|
||
| ### Requirement: Device log in report template | ||
| `reportTemplate.ts` SHALL render the same `<details>` block next to the `<video>` tag for the recording, using `escapeHtml` for the log content. | ||
|
|
||
| #### Scenario: CLI-generated report with device log | ||
| - **GIVEN** a test result with `deviceLogFile` | ||
| - **WHEN** `reportTemplate.ts` generates the HTML | ||
| - **THEN** the device log `<details>` block appears after the recording section | ||
|
|
There was a problem hiding this comment.
Report UX requirements are out of sync with the intended tabbed interactive viewer.
The spec mandates an inline <details> tail block near recording, while the PR objective describes a two-panel tabbed UI with video-log synchronization and click-to-seek. Please update the requirements/scenarios to match the delivered UX contract.
🧰 Tools
🪛 LanguageTool
[grammar] ~316-~316: Use a hyphen to join words.
Context: ...``` #### Scenario: Test with device log in report - GIVEN a test result with...
(QB_NEW_EN_HYPHEN)
[grammar] ~323-~323: Use a hyphen to join words.
Context: ... #### Scenario: Test without device log in report - GIVEN a test result with...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fab/changes/260405-g0d1-capture-device-logs/spec.md` around lines 307 - 345,
The spec and scenarios currently require an inline <details> device-log block
rendered by reportTemplate.ts (including server-side tailing, splitting on '\n',
taking last 200 lines, HTML-escaping via escapeHtml, and linking deviceLogRoute)
but the PR implements a two-panel tabbed interactive viewer with synchronized
video-log behavior; update the spec and scenarios to reflect the delivered UX by
replacing the inline-<details> requirements with descriptions of the tabbed
viewer (mentioning the recording panel and the log panel), the synchronization
contract (click-to-seek and video time -> log highlighting), and that
reportTemplate.ts should render the tabbed UI and still perform server-side
tailing/escapeHtml for the log content and provide a download link for the full
deviceLogFile; ensure test scenarios cover presence/absence of deviceLogFile,
the server-side tail read behavior (last 200 lines), and the interactive
synchronization behaviors.
Comment on lines
+85
to
+118
| const provider = this._providers.get(params.platform); | ||
| if (!provider) { | ||
| return new DeviceNodeResponse({ | ||
| success: false, | ||
| message: `Log capture is not configured for platform: ${params.platform}`, | ||
| }); | ||
| } | ||
|
|
||
| const logDir = path.join(os.tmpdir(), 'finalrun-logs'); | ||
| await fsp.mkdir(logDir, { recursive: true }); | ||
|
|
||
| const sanitizedRunId = this._sanitizeForFilename(params.runId); | ||
| const sanitizedTestId = this._sanitizeForFilename(params.testId); | ||
| const filePath = path.join(logDir, `${sanitizedRunId}_${sanitizedTestId}.${provider.fileExtension}`); | ||
|
|
||
| const logInfo = new LogInfo({ | ||
| deviceId: params.deviceId, | ||
| filePath, | ||
| runId: params.runId, | ||
| testId: params.testId, | ||
| platform: params.platform, | ||
| }); | ||
| this._logInfoMap.set(mapKey, logInfo); | ||
| this._deviceToLogKeysMap.set(params.deviceId, [ | ||
| ...(this._deviceToLogKeysMap.get(params.deviceId) ?? []), | ||
| mapKey, | ||
| ]); | ||
|
|
||
| try { | ||
| const providerResult = await provider.startLogCapture({ | ||
| deviceId: params.deviceId, | ||
| outputFilePath: filePath, | ||
| appIdentifier: params.appIdentifier, | ||
| }); |
There was a problem hiding this comment.
Check provider availability before allocating capture state.
LogCaptureProvider now exposes checkAvailability(), but Lines 93-111 still create temp-file state before any toolchain validation and Lines 113-138 can return a started capture without ever calling it. Missing adb/xcrun should fail fast here, not after the manager has already recorded the session as active.
Suggested fix
const provider = this._providers.get(params.platform);
if (!provider) {
return new DeviceNodeResponse({
success: false,
message: `Log capture is not configured for platform: ${params.platform}`,
});
}
+
+ const availability = await provider.checkAvailability();
+ if (!availability.success) {
+ return availability;
+ }
const logDir = path.join(os.tmpdir(), 'finalrun-logs');📝 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
| const provider = this._providers.get(params.platform); | |
| if (!provider) { | |
| return new DeviceNodeResponse({ | |
| success: false, | |
| message: `Log capture is not configured for platform: ${params.platform}`, | |
| }); | |
| } | |
| const logDir = path.join(os.tmpdir(), 'finalrun-logs'); | |
| await fsp.mkdir(logDir, { recursive: true }); | |
| const sanitizedRunId = this._sanitizeForFilename(params.runId); | |
| const sanitizedTestId = this._sanitizeForFilename(params.testId); | |
| const filePath = path.join(logDir, `${sanitizedRunId}_${sanitizedTestId}.${provider.fileExtension}`); | |
| const logInfo = new LogInfo({ | |
| deviceId: params.deviceId, | |
| filePath, | |
| runId: params.runId, | |
| testId: params.testId, | |
| platform: params.platform, | |
| }); | |
| this._logInfoMap.set(mapKey, logInfo); | |
| this._deviceToLogKeysMap.set(params.deviceId, [ | |
| ...(this._deviceToLogKeysMap.get(params.deviceId) ?? []), | |
| mapKey, | |
| ]); | |
| try { | |
| const providerResult = await provider.startLogCapture({ | |
| deviceId: params.deviceId, | |
| outputFilePath: filePath, | |
| appIdentifier: params.appIdentifier, | |
| }); | |
| const provider = this._providers.get(params.platform); | |
| if (!provider) { | |
| return new DeviceNodeResponse({ | |
| success: false, | |
| message: `Log capture is not configured for platform: ${params.platform}`, | |
| }); | |
| } | |
| const availability = await provider.checkAvailability(); | |
| if (!availability.success) { | |
| return availability; | |
| } | |
| const logDir = path.join(os.tmpdir(), 'finalrun-logs'); | |
| await fsp.mkdir(logDir, { recursive: true }); | |
| const sanitizedRunId = this._sanitizeForFilename(params.runId); | |
| const sanitizedTestId = this._sanitizeForFilename(params.testId); | |
| const filePath = path.join(logDir, `${sanitizedRunId}_${sanitizedTestId}.${provider.fileExtension}`); | |
| const logInfo = new LogInfo({ | |
| deviceId: params.deviceId, | |
| filePath, | |
| runId: params.runId, | |
| testId: params.testId, | |
| platform: params.platform, | |
| }); | |
| this._logInfoMap.set(mapKey, logInfo); | |
| this._deviceToLogKeysMap.set(params.deviceId, [ | |
| ...(this._deviceToLogKeysMap.get(params.deviceId) ?? []), | |
| mapKey, | |
| ]); | |
| try { | |
| const providerResult = await provider.startLogCapture({ | |
| deviceId: params.deviceId, | |
| outputFilePath: filePath, | |
| appIdentifier: params.appIdentifier, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/device-node/src/device/LogCaptureManager.ts` around lines 85 - 118,
The code currently creates temp files and records LogInfo/map state before
validating the provider; call and await provider.checkAvailability() immediately
after retrieving provider (right after const provider =
this._providers.get(params.platform)) and if it returns false or throws return a
DeviceNodeResponse with success:false and a clear message, before creating
logDir, sanitized filenames, LogInfo, or updating this._logInfoMap /
this._deviceToLogKeysMap; also move the creation/setting of LogInfo and the
device->keys map to after provider.startLogCapture has successfully completed so
state is only recorded for truly started captures, and catch/propagate
provider.startLogCapture errors into a failure DeviceNodeResponse.
Comment on lines
+149
to
+197
| // Restart callback — runs the full post-install setup sequence | ||
| // (same steps as initial setup: kill stale, terminate, start, connect, capture readiness, app IDs) | ||
| const restartDriver = async (): Promise<IOSDriverProcessHandle> => { | ||
| Logger.i(`IOSSimulatorSetup: Restarting driver for ${deviceId}...`); | ||
|
|
||
| await this._killStaleHostProcessesOnPortFn(DEFAULT_GRPC_PORT_START); | ||
|
|
||
| try { | ||
| await this._simctlClient.terminateApp(deviceId, IOS_DRIVER_RUNNER_BUNDLE_ID); | ||
| } catch { | ||
| // May already be dead | ||
| } | ||
|
|
||
| grpcClient.close(); | ||
|
|
||
| const newProcess = this._simctlClient.startDriver(deviceId, DEFAULT_GRPC_PORT_START); | ||
| const newStartupState = this._trackIOSDriverProcess(deviceId, newProcess); | ||
|
|
||
| const connected = await this._connectWithPolling( | ||
| grpcClient, | ||
| '127.0.0.1', | ||
| DEFAULT_GRPC_PORT_START, | ||
| { | ||
| getStartupFailureMessage: () => newStartupState.failureMessage, | ||
| getWaitStatusMessage: () => this._formatWaitStatus(newStartupState), | ||
| }, | ||
| ); | ||
| if (!connected) { | ||
| throw new Error('Failed to reconnect to iOS driver via gRPC after restart'); | ||
| } | ||
|
|
||
| const captureReady = await waitForDriverCaptureReadiness(grpcClient, { | ||
| timeoutMs: this._captureReadinessTimeoutMs, | ||
| delayMs: this._captureReadinessDelayMs, | ||
| }); | ||
| if (!captureReady.ready) { | ||
| throw new Error( | ||
| `iOS driver restarted but capture readiness failed: ${captureReady.message ?? 'unknown'}`, | ||
| ); | ||
| } | ||
|
|
||
| await this._updateIOSAppIds(deviceId, grpcClient, { throwOnFailure: true }); | ||
|
|
||
| newStartupState.setupComplete = true; | ||
| Logger.i(`IOSSimulatorSetup: Driver restarted successfully for ${deviceId}`); | ||
| return newProcess; | ||
| }; | ||
|
|
||
| return { deviceId, grpcPort: DEFAULT_GRPC_PORT_START, driverProcess, restartDriver }; |
There was a problem hiding this comment.
Restarting only the process is not enough here.
Line 162 closes grpcClient, but packages/device-node/src/grpc/GrpcDriverSetup.ts:115-139 wires that same client into CommonDriverActions. IOSSimulator._withDriverRecovery() then retries the failed operation through CommonDriverActions, so after the restart the second call is still using the closed client rather than a refreshed transport. The recovery callback needs to refresh the client used by CommonDriverActions, not just return a new process handle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/device-node/src/grpc/setup/IOSSimulatorSetup.ts` around lines 149 -
197, The restart logic in restartDriver currently closes grpcClient and starts a
new driver process but does not replace the closed transport used by
CommonDriverActions (wired in GrpcDriverSetup and exercised by
IOSSimulator._withDriverRecovery), so subsequent retries still use the dead
client; fix by creating a fresh gRPC client after start (the same kind created
originally), wait until _connectWithPolling succeeds against the new client,
then swap/replace the client instance that GrpcDriverSetup/CommonDriverActions
uses (so callers use the refreshed transport) before calling
waitForDriverCaptureReadiness/_updateIOSAppIds and before returning from
restartDriver; locate restartDriver, grpcClient, _connectWithPolling,
_trackIOSDriverProcess, _updateIOSAppIds, and ensure the replacement happens
atomically so _withDriverRecovery observes the new client.
droid-ash
changed the title
droid-ash
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/intake.md`:
- Around line 89-91: The current flow calls redactResolvedValue
(packages/common/src/repoPlaceholders.ts) and loads the entire log into memory
after the 50 MB warning; change writeTestRecord() so that _copyLogArtifact
(called after _copyRecordingArtifact) uses a streaming/chunked redaction path
instead of buffering the whole file: implement a streaming reader → incremental
redactResolvedValue-like transformer or, if streaming redaction isn’t feasible,
enforce a hard cap that truncates the input and records a truncation marker in
the three new testRecord fields so memory is bounded; update _copyLogArtifact to
detect large files and choose the streaming transformer or truncation fallback
and ensure testRecord contains the truncation/streaming-metadata.
- Line 9: Replace the phrase "iOS system log" with the tighter wording "iOS log"
in the intake description that currently reads "User requested device log
capture per test (Android logcat + iOS system log), mirroring the existing
recording pipeline."; update that sentence to read "Android logcat + iOS log" so
the wording is clearer and non-redundant.
In `@fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/spec.md`:
- Around line 7-13: Update the archived spec so it matches the implemented
behavior: remove or revise the "Non-Goals" entry that states "Log filtering or
search in the report viewer — full-text search is out of scope" and replace the
simple `<details>` tail block description (the section currently described
around the `<details>` block) with a clear UI spec that includes interactive
search/filtering (including Cmd+F behavior), clickable log entries that seek the
video, synchronized log highlighting alongside video playback, and a two-panel
layout; ensure you reference the "Non-Goals" heading and the `<details>` tail
block in your edits and explicitly enumerate the new interactive behaviors and
their expected user interactions.
- Line 365: The markdown table row at Line 365 contains the cell value `2 | 3`
which is being parsed as an extra column separator; fix it by escaping the pipe
or wrapping the union value in backticks (e.g., `2 \| 3` or `\`2 | 3\``) so the
table stays a single cell; also verify the text referencing RunManifest.ts:65
(literal `2`) remains correct after the change.
- Around line 49-51: The spec's LogCaptureProvider signature is missing the
optional appIdentifier parameter; update the declaration for startLogCapture in
LogCaptureProvider to include appIdentifier?: string in its params (so
startLogCapture(params: { deviceId: string; outputFilePath: string;
appIdentifier?: string }): Promise<{ process: ChildProcess; response:
DeviceNodeResponse }>), leaving stopLogCapture unchanged; reference the
LogCaptureProvider interface and startLogCapture/stopLogCapture method names
when making the change.
In `@fab/changes/archive/index.md`:
- Line 3: Update the release note text for entry
"260405-g0d1-capture-device-logs" to remove the redundant phrase "iOS system
log" and replace it with "iOS log" so the line reads "...Capture per-test device
logs (Android logcat + iOS log)..." — edit the string that contains
"260405-g0d1-capture-device-logs" in the archive/index.md to perform this
single-word change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 895647c0-38dc-4334-99ef-58a12ea4f1cf
📒 Files selected for processing (7)
fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/.history.jsonlfab/changes/archive/2026/04/260405-g0d1-capture-device-logs/.status.yamlfab/changes/archive/2026/04/260405-g0d1-capture-device-logs/checklist.mdfab/changes/archive/2026/04/260405-g0d1-capture-device-logs/intake.mdfab/changes/archive/2026/04/260405-g0d1-capture-device-logs/spec.mdfab/changes/archive/2026/04/260405-g0d1-capture-device-logs/tasks.mdfab/changes/archive/index.md
✅ Files skipped from review due to trivial changes (3)
- fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/.history.jsonl
- fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/.status.yaml
- fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/checklist.md
|
|
||
| ## Origin | ||
|
|
||
| > User requested device log capture per test (Android logcat + iOS system log), mirroring the existing recording pipeline. A detailed implementation plan was collaboratively developed covering architecture, file changes, and verification strategy. |
There was a problem hiding this comment.
Tighten wording: “iOS system log” → “iOS log”.
Line 9 is slightly redundant; “iOS log” is clearer and cleaner.
🧰 Tools
🪛 LanguageTool
[style] ~9-~9: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “iOS”.
Context: ... log capture per test (Android logcat + iOS system log), mirroring the existing recording ...
(ACRONYM_TAUTOLOGY)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/intake.md` at
line 9, Replace the phrase "iOS system log" with the tighter wording "iOS log"
in the intake description that currently reads "User requested device log
capture per test (Android logcat + iOS system log), mirroring the existing
recording pipeline."; update that sentence to read "Android logcat + iOS log" so
the wording is clearer and non-redundant.
Comment on lines
+89
to
+91
| 2. After copy: **read → redact → write** using `redactResolvedValue` from `packages/common/src/repoPlaceholders.ts` | ||
| 3. Size guard: warn if raw file > 50 MB (redaction holds whole file in memory) | ||
| - In `writeTestRecord()`: call `_copyLogArtifact` after `_copyRecordingArtifact`, add three new fields to `testRecord` |
There was a problem hiding this comment.
Avoid full-file in-memory redaction for large device logs.
Line 89–Line 90 describes loading the full log into memory even after crossing the 50 MB threshold. Warning-only handling can still cause memory pressure/OOM in CI on noisy runs. Please switch to chunked/streaming redaction (or enforce a hard cap + truncation/fail-safe) so artifact writing remains bounded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/intake.md` around
lines 89 - 91, The current flow calls redactResolvedValue
(packages/common/src/repoPlaceholders.ts) and loads the entire log into memory
after the 50 MB warning; change writeTestRecord() so that _copyLogArtifact
(called after _copyRecordingArtifact) uses a streaming/chunked redaction path
instead of buffering the whole file: implement a streaming reader → incremental
redactResolvedValue-like transformer or, if streaming redaction isn’t feasible,
enforce a hard cap that truncates the input and records a truncation marker in
the three new testRecord fields so memory is bounded; update _copyLogArtifact to
detect large files and choose the streaming transformer or truncation fallback
and ensure testRecord contains the truncation/streaming-metadata.
Comment on lines
+7
to
+13
| ## Non-Goals | ||
|
|
||
| - Log filtering or search in the report viewer — full-text search is out of scope | ||
| - Log level colorization or syntax highlighting in the `<pre>` block | ||
| - User-configurable log format (threadtime / compact are fixed per platform) | ||
| - Streaming logs to the report in real-time during test execution | ||
|
|
There was a problem hiding this comment.
Spec scope and UI requirements conflict with this PR’s stated behavior.
Line 9 says search/filter are out of scope, and Lines 307-345 define only a simple <details> tail block. That conflicts with the PR objective (interactive search/filter, Cmd+F, clickable log→video seek, and video-synced highlighting in a two-panel layout). Please align this archived spec to the implemented behavior so it remains a reliable source of truth.
Also applies to: 282-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/spec.md` around
lines 7 - 13, Update the archived spec so it matches the implemented behavior:
remove or revise the "Non-Goals" entry that states "Log filtering or search in
the report viewer — full-text search is out of scope" and replace the simple
`<details>` tail block description (the section currently described around the
`<details>` block) with a clear UI spec that includes interactive
search/filtering (including Cmd+F behavior), clickable log entries that seek the
video, synchronized log highlighting alongside video playback, and a two-panel
layout; ensure you reference the "Non-Goals" heading and the `<details>` tail
block in your edits and explicitly enumerate the new interactive behaviors and
their expected user interactions.
Comment on lines
+49
to
+51
| A `LogCaptureProvider` interface SHALL be defined in `packages/device-node/src/device/LogCaptureProvider.ts` mirroring `RecordingProvider`. It SHALL declare: | ||
| - `startLogCapture(params: { deviceId: string; outputFilePath: string }): Promise<{ process: ChildProcess; response: DeviceNodeResponse }>` | ||
| - `stopLogCapture(params: { process: ChildProcess; outputFilePath: string }): Promise<DeviceNodeResponse>` |
There was a problem hiding this comment.
LogCaptureProvider signature in spec is missing appIdentifier.
Line 50 omits appIdentifier?: string from startLogCapture(params), but the current interface includes it. This makes the contract in the spec inaccurate for iOS predicate/process-name targeting flows.
Proposed spec correction
-- `startLogCapture(params: { deviceId: string; outputFilePath: string }): Promise<{ process: ChildProcess; response: DeviceNodeResponse }>`
+- `startLogCapture(params: { deviceId: string; outputFilePath: string; appIdentifier?: string }): Promise<{ process: ChildProcess; response: DeviceNodeResponse }>`📝 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
| A `LogCaptureProvider` interface SHALL be defined in `packages/device-node/src/device/LogCaptureProvider.ts` mirroring `RecordingProvider`. It SHALL declare: | |
| - `startLogCapture(params: { deviceId: string; outputFilePath: string }): Promise<{ process: ChildProcess; response: DeviceNodeResponse }>` | |
| - `stopLogCapture(params: { process: ChildProcess; outputFilePath: string }): Promise<DeviceNodeResponse>` | |
| A `LogCaptureProvider` interface SHALL be defined in `packages/device-node/src/device/LogCaptureProvider.ts` mirroring `RecordingProvider`. It SHALL declare: | |
| - `startLogCapture(params: { deviceId: string; outputFilePath: string; appIdentifier?: string }): Promise<{ process: ChildProcess; response: DeviceNodeResponse }>` | |
| - `stopLogCapture(params: { process: ChildProcess; outputFilePath: string }): Promise<DeviceNodeResponse>` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/spec.md` around
lines 49 - 51, The spec's LogCaptureProvider signature is missing the optional
appIdentifier parameter; update the declaration for startLogCapture in
LogCaptureProvider to include appIdentifier?: string in its params (so
startLogCapture(params: { deviceId: string; outputFilePath: string;
appIdentifier?: string }): Promise<{ process: ChildProcess; response:
DeviceNodeResponse }>), leaving stopLogCapture unchanged; reference the
LogCaptureProvider interface and startLogCapture/stopLogCapture method names
when making the change.
| | 13 | Certain | Enable log capture unconditionally (no user flag) | Clarified from intake #13 — user confirmed | S:95 R:85 A:65 D:70 | | ||
| | 14 | Certain | LogCaptureManager uses same map key delimiter `###` as RecordingManager | Codebase pattern — RecordingManager.ts:50 uses `###` | S:95 R:90 A:95 D:95 | | ||
| | 15 | Certain | DeviceLogCaptureResult uses same timestamp format (ISO 8601) as recording | Codebase pattern — RecordingManager returns `toISOString()` | S:95 R:90 A:95 D:95 | | ||
| | 16 | Certain | Schema version type becomes union `2 | 3` for backward compat | Codebase — RunManifest.ts:65 currently uses literal `2` | S:90 R:70 A:90 D:90 | |
There was a problem hiding this comment.
Fix markdown table parsing at Line 365.
The cell text 2 | 3 introduces an extra column separator (MD056). Escape the pipe or wrap the value in backticks.
Proposed markdown fix
-| 16 | Certain | Schema version type becomes union `2 | 3` for backward compat | Codebase — RunManifest.ts:65 currently uses literal `2` | S:90 R:70 A:90 D:90 |
+| 16 | Certain | Schema version type becomes union `2 \| 3` for backward compat | Codebase — RunManifest.ts:65 currently uses literal `2` | S:90 R:70 A:90 D:90 |📝 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
| | 16 | Certain | Schema version type becomes union `2 | 3` for backward compat | Codebase — RunManifest.ts:65 currently uses literal `2` | S:90 R:70 A:90 D:90 | | |
| | 16 | Certain | Schema version type becomes union `2 \| 3` for backward compat | Codebase — RunManifest.ts:65 currently uses literal `2` | S:90 R:70 A:90 D:90 | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 365-365: Table column count
Expected: 5; Actual: 6; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fab/changes/archive/2026/04/260405-g0d1-capture-device-logs/spec.md` at line
365, The markdown table row at Line 365 contains the cell value `2 | 3` which is
being parsed as an extra column separator; fix it by escaping the pipe or
wrapping the union value in backticks (e.g., `2 \| 3` or `\`2 | 3\``) so the
table stays a single cell; also verify the text referencing RunManifest.ts:65
(literal `2`) remains correct after the change.
| @@ -0,0 +1,3 @@ | |||
| # Archive Index | |||
|
|
|||
| - **260405-g0d1-capture-device-logs** — Capture per-test device logs (Android logcat + iOS system log) with video-synced interactive UI, search/filter controls, iOS gRPC driver recovery, and iOS log predicate fix | |||
There was a problem hiding this comment.
Tighten wording: avoid acronym tautology on Line 3.
“iOS system log” is redundant; “iOS log” is clearer and cleaner in docs.
Suggested edit
-- **260405-g0d1-capture-device-logs** — Capture per-test device logs (Android logcat + iOS system log) with video-synced interactive UI, search/filter controls, iOS gRPC driver recovery, and iOS log predicate fix
+- **260405-g0d1-capture-device-logs** — Capture per-test device logs (Android logcat + iOS log) with video-synced interactive UI, search/filter controls, iOS gRPC driver recovery, and iOS log predicate fix📝 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
| - **260405-g0d1-capture-device-logs** — Capture per-test device logs (Android logcat + iOS system log) with video-synced interactive UI, search/filter controls, iOS gRPC driver recovery, and iOS log predicate fix | |
| - **260405-g0d1-capture-device-logs** — Capture per-test device logs (Android logcat + iOS log) with video-synced interactive UI, search/filter controls, iOS gRPC driver recovery, and iOS log predicate fix |
🧰 Tools
🪛 LanguageTool
[style] ~3-~3: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “iOS”.
Context: ... per-test device logs (Android logcat + iOS system log) with video-synced interactive UI, ...
(ACRONYM_TAUTOLOGY)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fab/changes/archive/index.md` at line 3, Update the release note text for
entry "260405-g0d1-capture-device-logs" to remove the redundant phrase "iOS
system log" and replace it with "iOS log" so the line reads "...Capture per-test
device logs (Android logcat + iOS log)..." — edit the string that contains
"260405-g0d1-capture-device-logs" in the archive/index.md to perform this
single-word change.
- Default maxRetries to 0 for mutating RPCs (tap, enterText, swipe) to prevent action duplication; read-only RPCs opt in with maxRetries: 2 - Keep maxRetries: 2 on launchApp — retries enable iOS driver recovery - Fix Android log year inference using recordingStartedAt instead of viewer clock year - Fix log capture cleanup ordering so finally block can abort on errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/device-node/src/grpc/GrpcDriverClient.ts (1)
88-99:⚠️ Potential issue | 🟡 MinorKeep
ping()single-attempt.
ping()is still documented as the caller-owned polling primitive at Line 35 and as a 3s check at Line 83. WithmaxRetries: 2, one failed poll can now stretch to roughly 10.5s, which slows startup and recovery loops that already retry outside this method.Suggested change
const response = await this._unaryCall('getDeviceScale', {}, { timeoutMs: 3000, errorLogLevel: 'debug', - maxRetries: 2, + maxRetries: 0, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/device-node/src/grpc/GrpcDriverClient.ts` around lines 88 - 99, The ping() method performs a caller-intended single-attempt health check but currently passes maxRetries: 2 to _unaryCall, causing long delays; update ping() to call _unaryCall('getDeviceScale', {}, { timeoutMs: 3000, errorLogLevel: 'debug' }) without maxRetries (remove maxRetries) so ping() is a single 3s attempt; keep the existing early-return when this._client is missing and preserve error handling around the await.
♻️ Duplicate comments (1)
packages/device-node/src/grpc/GrpcDriverClient.ts (1)
225-234:⚠️ Potential issue | 🟠 MajorGate
launchAppretries to idempotent launches only.This still retries every
launchAppcall from the shared client, butdrivers/ios/finalrun-ios-test/GrpcDriverServer.swift:321-369only makes the no-args / already-foreground path harmless to repeat. Requests with launch arguments can still hitapp.launch()again if the first attempt succeeded and only the reply was lost, which can reset app state mid-test. Keep the retry opt-in in the iOS recovery path, or disable it here for non-idempotent launch requests.Safer default in the shared client
- return this._unaryCall('launchApp', params, { timeoutMs: 60000, maxRetries: 2 }); // 60s timeout like Dart; retries enable driver recovery on iOS + return this._unaryCall('launchApp', params, { timeoutMs: 60000, maxRetries: 0 }); // keep retries opt-in at the iOS recovery call site🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/device-node/src/grpc/GrpcDriverClient.ts` around lines 225 - 234, The shared client currently always enables retries for launchApp which can re-run non-idempotent launches; change launchApp to disable retries by default by calling this._unaryCall('launchApp', params, { timeoutMs: 60000, maxRetries: 0 }) so only callers that explicitly opt into recovery (e.g., the iOS recovery path in drivers/ios/finalrun-ios-test/GrpcDriverServer.swift) can retry; ensure you keep the 60s timeout but set maxRetries to 0 in the launchApp method to prevent accidental duplicate launches when arguments, permissions, or uninstall flags are present.
🧹 Nitpick comments (1)
packages/cli/src/reportWriter.ts (1)
711-729: Missing early return when source equals target path.The
_copyRecordingArtifactmethod (lines 694-696) has an early return whensourcePath === targetPathto avoid unnecessary file operations. This method lacks that check.♻️ Proposed fix for consistency
const logRelative = path.posix.join('tests', testId, 'device.log'); const sourcePath = path.resolve(deviceLog.filePath); const targetPath = path.resolve(path.join(this._runDir, logRelative)); try { await fsp.access(sourcePath); } catch { Logger.w(`Device log file not found for report copy: ${deviceLog.filePath}`); return undefined; } + if (sourcePath === targetPath) { + return logRelative; + } + const stats = await fsp.stat(sourcePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/reportWriter.ts` around lines 711 - 729, The block in _copyRecordingArtifact (where logRelative, sourcePath, targetPath are computed) should short-circuit when sourcePath === targetPath to avoid redundant file access/copy; add an early return (return undefined) immediately after computing targetPath if the resolved paths are equal, matching the existing behavior in _copyRecordingArtifact elsewhere and preventing unnecessary fsp.access/fsp.copyFile operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/reportWriter.ts`:
- Around line 732-742: The try/catch around reading and redacting the copied log
file may still return logRelative on redaction failure, risking secret leakage;
modify the catch block in the function that uses fsp.readFile/fsp.writeFile and
redactResolvedValue so that when an error occurs you remove the copied file (use
fsp.unlink or equivalent) or return undefined instead of returning logRelative,
and ensure you still log the formatted error via
Logger.w(this._formatError(error)); this change should be applied where
logRelative is returned so callers do not include the unredacted file in
reports.
---
Outside diff comments:
In `@packages/device-node/src/grpc/GrpcDriverClient.ts`:
- Around line 88-99: The ping() method performs a caller-intended single-attempt
health check but currently passes maxRetries: 2 to _unaryCall, causing long
delays; update ping() to call _unaryCall('getDeviceScale', {}, { timeoutMs:
3000, errorLogLevel: 'debug' }) without maxRetries (remove maxRetries) so ping()
is a single 3s attempt; keep the existing early-return when this._client is
missing and preserve error handling around the await.
---
Duplicate comments:
In `@packages/device-node/src/grpc/GrpcDriverClient.ts`:
- Around line 225-234: The shared client currently always enables retries for
launchApp which can re-run non-idempotent launches; change launchApp to disable
retries by default by calling this._unaryCall('launchApp', params, { timeoutMs:
60000, maxRetries: 0 }) so only callers that explicitly opt into recovery (e.g.,
the iOS recovery path in drivers/ios/finalrun-ios-test/GrpcDriverServer.swift)
can retry; ensure you keep the 60s timeout but set maxRetries to 0 in the
launchApp method to prevent accidental duplicate launches when arguments,
permissions, or uninstall flags are present.
---
Nitpick comments:
In `@packages/cli/src/reportWriter.ts`:
- Around line 711-729: The block in _copyRecordingArtifact (where logRelative,
sourcePath, targetPath are computed) should short-circuit when sourcePath ===
targetPath to avoid redundant file access/copy; add an early return (return
undefined) immediately after computing targetPath if the resolved paths are
equal, matching the existing behavior in _copyRecordingArtifact elsewhere and
preventing unnecessary fsp.access/fsp.copyFile operations.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8af40b2e-a4ed-4146-a8ba-8ee8853323ce
📒 Files selected for processing (7)
packages/cli/src/reportTemplate.tspackages/cli/src/reportWriter.tspackages/cli/src/sessionRunner.tspackages/common/src/models/TestResult.tspackages/device-node/src/grpc/GrpcDriverClient.tspackages/goal-executor/src/TestExecutor.tspackages/report-web/src/renderers.ts
✅ Files skipped from review due to trivial changes (3)
- packages/common/src/models/TestResult.ts
- packages/goal-executor/src/TestExecutor.ts
- packages/cli/src/reportTemplate.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/src/sessionRunner.ts
- packages/report-web/src/renderers.ts
Error lines now have a thicker dark red border (3px) with a subtle red background tint, while warning lines remain with a lighter amber border only. Makes errors immediately distinguishable at a glance. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/reportTemplate.ts`:
- Around line 1095-1105: The Logs tab highlight isn't recomputed except via
applySeek; update the tab-switch and playback update paths to call
highlightNearestLogLine so the highlighted log row stays synced: when switchTab
(the function in the diff) activates a tab, detect if the activated tab is the
Logs tab and call highlightNearestLogLine; likewise ensure the timeupdate
handler (and any other playback advance handlers that currently only update
seekbar/applySeek) also invokes highlightNearestLogLine when the Logs tab is
active; reference the functions switchTab, highlightNearestLogLine, and
applySeek to locate where to add these calls.
- Around line 1906-1919: The inline device log currently renders the entire
deviceLogTailText (via renderDeviceLogLines) which can produce huge HTML; change
the implementation so only the last N lines or last N KB are inlined while
preserving the full download link. Either (A) truncate deviceLogTailText in
reportServer.ts where it’s read (see the loading logic around the existing
deviceLogTailText assignment) to a safe tail size (e.g., last 500 lines or 64KB)
and add a “truncated” marker, or (B) update renderDeviceLogLines in
packages/cli/src/reportTemplate.ts to accept a maxLines/maxBytes parameter and
return only the tail plus an explicit “…truncated, download full log” line;
ensure the device-log-download anchor still points to the full artifact and that
the device-log-inline element includes an attribute or visible note when
truncation occurred.
- Around line 1394-1406: The global keydown handler currently queries
document.querySelector('.tab-content.is-active[data-tab-content="logs"]') and
can focus a hidden .device-log-search elsewhere; change the lookup to scope
inside the currently visible detail panel instead of the whole document. Update
the handler (the anonymous function passed to
document.addEventListener('keydown')) to first locate the visible detail
container (the DOM element that represents the open test/detail panel) and then
call container.querySelector('.tab-content.is-active[data-tab-content="logs"]')
and container.querySelector('.device-log-search') so the search input is only
focused when it is inside the visible detail panel; keep the e.preventDefault(),
focus(), and select() behavior only when that scoped element is found.
In `@packages/report-web/src/renderers.ts`:
- Around line 1354-1364: The tab activation code (switchTab and the equivalent
block at 1693-1709) never triggers a recompute of the active log row because
highlightNearestLogLine() is only invoked from applySeek(); call
highlightNearestLogLine() at the end of switchTab (and the other tab-activation
block) so enabling tab switches to re-sync the highlighted log row, and also
invoke highlightNearestLogLine() from the playback advance/tick or play handler
(the function that advances time during normal playback) so normal playback
updates the highlighted row without requiring a seek.
- Around line 2197-2210: The device log text (test.deviceLogTailText) is being
embedded twice — rendered inline via renderDeviceLogLines and left in the
serialized bootstrap reportPayload (stripSnapshotYamlText doesn't remove it)
causing huge duplicate payloads; to fix, stop including the full log in the
bootstrap payload and only keep a small preview for the DOM: update the code
that prepares reportPayload to clear or omit test.deviceLogTailText before
serialization (so reportPayload no longer contains the full text) and change the
inline rendering to use a truncated preview field (or a new
deviceLogTailPreview) consumed by renderDeviceLogLines, ensuring download links
still reference the full deviceLogFile but the full log string is not kept in
reportPayload.
- Around line 1653-1665: The global keydown handler currently queries the whole
document for '.tab-content.is-active[data-tab-content="logs"]' which can match
hidden test panels; restrict the selector to only the visible detail panel (e.g.
query '.detail-panel.is-active .tab-content.is-active[data-tab-content="logs"]'
or first find the visible detail panel element and then
querySelector('.tab-content.is-active[data-tab-content="logs"]') within it) so
the Cmd/Ctrl+F override only focuses a .device-log-search inside the currently
shown detail panel; keep the same preventDefault/focus/select behavior on the
found input.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e1b858b-7cd0-46f6-9940-ed12d31b11c1
📒 Files selected for processing (2)
packages/cli/src/reportTemplate.tspackages/report-web/src/renderers.ts
Comment on lines
+2197
to
+2210
| ${test?.deviceLogFile | ||
| ? `<div class="tab-content" data-tab-content="logs"> | ||
| <div class="device-log-inline" data-recording-started="${escapeHtml(test.recordingStartedAt ?? '')}"> | ||
| <div class="device-log-toolbar"> | ||
| <input class="device-log-search" type="text" placeholder="Search logs..." /> | ||
| <span class="device-log-match-count"></span> | ||
| <div class="device-log-filters"> | ||
| <button class="log-filter-chip is-active" data-log-level="all" onclick="handleLogFilter(this)" type="button">All</button> | ||
| <button class="log-filter-chip" data-log-level="error" onclick="handleLogFilter(this)" type="button">Errors</button> | ||
| <button class="log-filter-chip" data-log-level="warn" onclick="handleLogFilter(this)" type="button">Warnings</button> | ||
| </div> | ||
| </div> | ||
| <div class="device-log-lines">${renderDeviceLogLines(test.deviceLogTailText ?? '', test.recordingStartedAt)}</div> | ||
| <a class="device-log-download" href="${escapeHtml(test.deviceLogFile)}" download>Download full log</a> |
There was a problem hiding this comment.
Don’t ship the full device log twice.
packages/report-web/src/artifacts.ts:290-299 loads the entire artifact into deviceLogTailText, this block renders it inline, and stripSnapshotYamlText() still leaves the same string in reportPayload. Large logs will therefore be embedded once in the DOM and again in the bootstrap script, which is an easy way to make the run page sluggish or unopenable. This still needs an upstream tail/size cap, but the duplicate bootstrap copy should be removed here too.
♻️ Suggested fix for the bootstrap payload
function stripSnapshotYamlText(manifest: ReportRunManifest): RunManifest {
return {
...manifest,
input: {
...manifest.input,
tests: manifest.input.tests.map(({ snapshotYamlText: _snapshotYamlText, ...test }) => test),
},
- tests: manifest.tests.map(({ snapshotYamlText: _snapshotYamlText, ...test }) => test),
+ tests: manifest.tests.map(
+ ({ snapshotYamlText: _snapshotYamlText, deviceLogTailText: _deviceLogTailText, ...test }) => test,
+ ),
};
}📝 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
| ${test?.deviceLogFile | |
| ? `<div class="tab-content" data-tab-content="logs"> | |
| <div class="device-log-inline" data-recording-started="${escapeHtml(test.recordingStartedAt ?? '')}"> | |
| <div class="device-log-toolbar"> | |
| <input class="device-log-search" type="text" placeholder="Search logs..." /> | |
| <span class="device-log-match-count"></span> | |
| <div class="device-log-filters"> | |
| <button class="log-filter-chip is-active" data-log-level="all" onclick="handleLogFilter(this)" type="button">All</button> | |
| <button class="log-filter-chip" data-log-level="error" onclick="handleLogFilter(this)" type="button">Errors</button> | |
| <button class="log-filter-chip" data-log-level="warn" onclick="handleLogFilter(this)" type="button">Warnings</button> | |
| </div> | |
| </div> | |
| <div class="device-log-lines">${renderDeviceLogLines(test.deviceLogTailText ?? '', test.recordingStartedAt)}</div> | |
| <a class="device-log-download" href="${escapeHtml(test.deviceLogFile)}" download>Download full log</a> | |
| function stripSnapshotYamlText(manifest: ReportRunManifest): RunManifest { | |
| return { | |
| ...manifest, | |
| input: { | |
| ...manifest.input, | |
| tests: manifest.input.tests.map(({ snapshotYamlText: _snapshotYamlText, ...test }) => test), | |
| }, | |
| tests: manifest.tests.map( | |
| ({ snapshotYamlText: _snapshotYamlText, deviceLogTailText: _deviceLogTailText, ...test }) => test, | |
| ), | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/report-web/src/renderers.ts` around lines 2197 - 2210, The device
log text (test.deviceLogTailText) is being embedded twice — rendered inline via
renderDeviceLogLines and left in the serialized bootstrap reportPayload
(stripSnapshotYamlText doesn't remove it) causing huge duplicate payloads; to
fix, stop including the full log in the bootstrap payload and only keep a small
preview for the DOM: update the code that prepares reportPayload to clear or
omit test.deviceLogTailText before serialization (so reportPayload no longer
contains the full text) and change the inline rendering to use a truncated
preview field (or a new deviceLogTailPreview) consumed by renderDeviceLogLines,
ensuring download links still reference the full deviceLogFile but the full log
string is not kept in reportPayload.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/report-web/src/renderers.ts (3)
2197-2214:⚠️ Potential issue | 🟠 MajorDevice log content is embedded twice, inflating page size.
The full
test.deviceLogTailTextis rendered inline in the DOM (line 2209 viarenderDeviceLogLines) and also serialized into the bootstrapreportPayloadJSON (line 322), sincestripSnapshotYamlText()(lines 2548-2556) only stripssnapshotYamlTextbut notdeviceLogTailText. For large log files, this doubles the payload and can make the page sluggish.♻️ Suggested fix for stripSnapshotYamlText
function stripSnapshotYamlText(manifest: ReportRunManifest): RunManifest { return { ...manifest, input: { ...manifest.input, tests: manifest.input.tests.map(({ snapshotYamlText: _snapshotYamlText, ...test }) => test), }, - tests: manifest.tests.map(({ snapshotYamlText: _snapshotYamlText, ...test }) => test), + tests: manifest.tests.map( + ({ snapshotYamlText: _snapshotYamlText, deviceLogTailText: _deviceLogTailText, ...test }) => test, + ), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/report-web/src/renderers.ts` around lines 2197 - 2214, The page is embedding large device logs twice: once inline via renderDeviceLogLines(test.deviceLogTailText, ...) and again inside the serialized reportPayload; update stripSnapshotYamlText to also remove/replace test.deviceLogTailText (just like snapshotYamlText) before building the bootstrap reportPayload so the full deviceLogTailText is not included in the JSON payload; locate stripSnapshotYamlText and the code that generates reportPayload and ensure it strips or clears test.deviceLogTailText for each test object (leave rendering unchanged so inline display still works).
1354-1364:⚠️ Potential issue | 🟠 MajorLog highlight not updated on tab switch or during playback.
When the user switches to the Device Logs tab, the highlighted log line is not recomputed to match the current video position. Similarly, during normal video playback, the
timeupdateevent triggerssyncControls()but never callshighlightNearestLogLine(), leaving the log viewer out of sync.Consider calling
highlightNearestLogLine()at the end ofswitchTab()(when switching to the logs tab) and from withinsyncControls()during playback to keep the log highlight in sync with the video position.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/report-web/src/renderers.ts` around lines 1354 - 1364, switchTab currently activates tab UI but doesn't update the log highlight; add a call to highlightNearestLogLine() at the end of switchTab when the activated tab is the Device Logs tab (detect via button.dataset.tab or panel contents) so the nearest log line is recomputed when the user switches to logs. Also update syncControls() so that on video timeupdate (playback) it calls highlightNearestLogLine() while the player is advancing (or whenever time changes) so the log viewer stays synchronized with the video position.
1653-1665:⚠️ Potential issue | 🟡 MinorScope the Cmd/Ctrl+F override to the visible detail panel.
The current selector
document.querySelector('.tab-content.is-active[data-tab-content="logs"]')searches the entire document and can match a logs tab in a hidden test panel. Once a user has opened Device Logs on one test, Cmd/Ctrl+F may focus an off-screen input instead of triggering browser find on the currently visible panel.♻️ Suggested fix
- var activeLogTab = document.querySelector('.tab-content.is-active[data-tab-content="logs"]'); + var activePanel = document.querySelector('.detail-shell.is-visible'); + var activeLogTab = activePanel + ? activePanel.querySelector('.tab-content.is-active[data-tab-content="logs"]') + : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/report-web/src/renderers.ts` around lines 1653 - 1665, The global keydown handler currently finds a logs tab anywhere in the document using document.querySelector('.tab-content.is-active[data-tab-content="logs"]') which can match hidden/off-screen panels; update the handler to restrict the search to the currently visible detail panel (the keydown listener block that contains the selector and the '.device-log-search' lookup). Specifically, locate the keydown listener and change the lookup to either query inside the active/visible detail container (e.g., find the visible detail panel element first and then call .querySelector('.tab-content.is-active[data-tab-content="logs"] .device-log-search')) or use a visibility check (e.g., filter candidates by element.offsetParent !== null or getClientRects().length > 0) before calling focus()/select() on '.device-log-search'. Ensure you only call e.preventDefault()/focus()/select() when the matched '.device-log-search' is in the visible panel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/report-web/src/renderers.ts`:
- Around line 2197-2214: The page is embedding large device logs twice: once
inline via renderDeviceLogLines(test.deviceLogTailText, ...) and again inside
the serialized reportPayload; update stripSnapshotYamlText to also
remove/replace test.deviceLogTailText (just like snapshotYamlText) before
building the bootstrap reportPayload so the full deviceLogTailText is not
included in the JSON payload; locate stripSnapshotYamlText and the code that
generates reportPayload and ensure it strips or clears test.deviceLogTailText
for each test object (leave rendering unchanged so inline display still works).
- Around line 1354-1364: switchTab currently activates tab UI but doesn't update
the log highlight; add a call to highlightNearestLogLine() at the end of
switchTab when the activated tab is the Device Logs tab (detect via
button.dataset.tab or panel contents) so the nearest log line is recomputed when
the user switches to logs. Also update syncControls() so that on video
timeupdate (playback) it calls highlightNearestLogLine() while the player is
advancing (or whenever time changes) so the log viewer stays synchronized with
the video position.
- Around line 1653-1665: The global keydown handler currently finds a logs tab
anywhere in the document using
document.querySelector('.tab-content.is-active[data-tab-content="logs"]') which
can match hidden/off-screen panels; update the handler to restrict the search to
the currently visible detail panel (the keydown listener block that contains the
selector and the '.device-log-search' lookup). Specifically, locate the keydown
listener and change the lookup to either query inside the active/visible detail
container (e.g., find the visible detail panel element first and then call
.querySelector('.tab-content.is-active[data-tab-content="logs"]
.device-log-search')) or use a visibility check (e.g., filter candidates by
element.offsetParent !== null or getClientRects().length > 0) before calling
focus()/select() on '.device-log-search'. Ensure you only call
e.preventDefault()/focus()/select() when the matched '.device-log-search' is in
the visible panel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f82805a-5886-43df-817d-ff785a06399f
📒 Files selected for processing (2)
packages/cli/src/reportTemplate.tspackages/report-web/src/renderers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/reportTemplate.ts
Show "Test file not found" instead of misleading "Test selector not found" when a YAML file doesn't exist, with "Did you mean?" suggestions using Levenshtein distance matching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eport viewer - Thread resolved adbPath into AndroidLogcatProvider instead of bare 'adb' - Warn and capture unfiltered when iOS log filter identifier can't be resolved - Run checkAvailability() before allocating state in LogCaptureManager - Delete unredacted log file on redaction failure to prevent secret leaks - Sync log highlight with video playback (throttled timeupdate) and tab switch - Scope Cmd+F to visible test panel only - Strip deviceLogTailText from report-web bootstrap payload to avoid double-embed - Cap inline device log to 500 lines with truncation notice Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/device-node/src/device/Device.ts (1)
280-289:⚠️ Potential issue | 🟠 MajorCatch filter-resolution errors and fall back to unfiltered capture.
This only handles a falsy resolver result. If
resolveLogFilterIdentifier()throws,startLogCapture()rejects,packages/cli/src/sessionRunner.tsnever setsactiveLogCapture, and the test finishes with no device log at all. Catch the resolution error here, warn, and continue withlogIdentifier = undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/device-node/src/device/Device.ts` around lines 280 - 289, The current block calls this._runtime.resolveLogFilterIdentifier and only handles a falsy return; update the logic in startLogCapture (the section that uses logIdentifier and this._runtime.resolveLogFilterIdentifier) to catch any exceptions from resolveLogFilterIdentifier, call Logger.w with a clear message including the original logIdentifier and error, and set logIdentifier = undefined so capture proceeds unfiltered; ensure the catch only affects resolution (do not rethrow) so startLogCapture can continue and packages/cli/src/sessionRunner.ts can still set activeLogCapture.packages/cli/src/reportTemplate.ts (1)
2232-2239:⚠️ Potential issue | 🟠 MajorHandle Android New Year rollover in
MM-DDtimestamps.A run that starts on December 31, 2026 and later emits
01-01 00:00:05.000 ...on January 1, 2027 still gets parsed as2026-01-01, so the line is treated as “before recording” and falls out of sync. For Android logcat lines, pick the closest candidate aroundrecordingStartedAt(year - 1,year,year + 1) instead of always using the reference year verbatim.Also applies to: 2273-2281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/reportTemplate.ts` around lines 2232 - 2239, parseLogTimestamp currently builds an Android MM-DD timestamp using the referenceDate's year verbatim, which misparses events across New Year; update parseLogTimestamp to try candidate years (referenceYear - 1, referenceYear, referenceYear + 1), construct a Date for each candidate (using the parsed month/day/time), and select the candidate whose timestamp is nearest to the referenceDate (or to now if no referenceDate) before returning its ISO string; apply the same nearest-year selection logic to the other Android-timestamp parsing block referenced around the other Android pattern (lines ~2273-2281) so both handlers handle New Year rollover correctly.packages/report-web/src/renderers.ts (1)
2624-2631:⚠️ Potential issue | 🟠 MajorMirror the New Year rollover fix in the report-web parser.
This parser still breaks on captures that cross New Year. A run that starts on December 31, 2026 and later logs
01-01 ...on January 1, 2027 is parsed into2026-01-01, so the line gets filtered out and stops syncing with playback. Use the nearest year aroundrecordingStartedAt, not just its calendar year.Also applies to: 2665-2673
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/report-web/src/renderers.ts` around lines 2624 - 2631, parseLogTimestamp currently picks the reference year directly which causes New Year rollovers to be assigned the wrong year; change it to try year candidates (referenceYear - 1, referenceYear, referenceYear + 1), construct a Date for each candidate (using parsed month/day/time), compare absolute difference to new Date(referenceDate) (or now if referenceDate is absent) and return the ISO string for the candidate with the smallest difference; apply the same nearest-year logic to the duplicate parser block referenced around the other occurrence (lines ~2665-2673) so cross-year logs are assigned the closest calendar year to recordingStartedAt.
🧹 Nitpick comments (1)
packages/cli/src/reportWriter.ts (1)
722-736: Avoid whole-file redaction for oversized device logs.The 50 MB branch only warns; this path still loads the entire artifact into memory as
rawand often creates a second full copy asredacted. A noisylogcatrun can make report writing the highest-memory step in the pipeline. Consider a hard cap / omit-with-warning path or streaming redaction here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/reportWriter.ts` around lines 722 - 736, The current logic still reads the entire file into memory even when stats.size > 50MB; change behavior to avoid whole-file redaction for oversized logs by skipping in-memory redaction for large files. Detect the large-file condition using stats.size (the existing check) and after fsp.copyFile simply log a warning and do not call fsp.readFile, redactResolvedValue, or fsp.writeFile for files exceeding the cap (or alternatively implement a streaming redaction path using fs.createReadStream/fs.createWriteStream if you prefer to preserve redaction for large files). Update the code paths around sourcePath/targetPath, the stats.size check, and the block that calls redactResolvedValue so that big files are copied but not loaded into memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/testSelection.ts`:
- Around line 73-79: The tests still assert the old error text "Test selector
not found:" but the code in testSelection.ts now throws "Test file not found:
${selector}", so update the failing assertions to match the new message; change
occurrences of assert.match(result.stderr, /Test selector not found:/) to
assert.match(result.stderr, /Test file not found:/) and change
assert.doesNotMatch(result.stderr, /Test selector not found/) to
assert.doesNotMatch(result.stderr, /Test file not found/) (or adjust the regexes
to accept both variants) so the assertions align with the error thrown by the
code in testSelection.ts (the block that throws new Error(`Test file not found:
${selector}${suggestion}`)).
In `@packages/device-node/src/device/AndroidLogcatProvider.ts`:
- Around line 118-139: In stopLogCapture(), don't treat process.kill('SIGINT')
returning false as an immediate failure; instead, if kill returned false check
params.process.exitCode (via the existing _waitForExit() helper) and continue
cleanup because the child may have already exited; also change the wait to
listen for the child process 'close' event (or adapt _waitForExit() to await
'close') so piped stdout is fully flushed before unpipe/closing the stream and
returning success, and only return a failure if the process truly fails with a
non-zero exit code after this close-based wait; update references:
stopLogCapture(), _waitForExit(), params.process, params.outputFilePath.
---
Duplicate comments:
In `@packages/cli/src/reportTemplate.ts`:
- Around line 2232-2239: parseLogTimestamp currently builds an Android MM-DD
timestamp using the referenceDate's year verbatim, which misparses events across
New Year; update parseLogTimestamp to try candidate years (referenceYear - 1,
referenceYear, referenceYear + 1), construct a Date for each candidate (using
the parsed month/day/time), and select the candidate whose timestamp is nearest
to the referenceDate (or to now if no referenceDate) before returning its ISO
string; apply the same nearest-year selection logic to the other
Android-timestamp parsing block referenced around the other Android pattern
(lines ~2273-2281) so both handlers handle New Year rollover correctly.
In `@packages/device-node/src/device/Device.ts`:
- Around line 280-289: The current block calls
this._runtime.resolveLogFilterIdentifier and only handles a falsy return; update
the logic in startLogCapture (the section that uses logIdentifier and
this._runtime.resolveLogFilterIdentifier) to catch any exceptions from
resolveLogFilterIdentifier, call Logger.w with a clear message including the
original logIdentifier and error, and set logIdentifier = undefined so capture
proceeds unfiltered; ensure the catch only affects resolution (do not rethrow)
so startLogCapture can continue and packages/cli/src/sessionRunner.ts can still
set activeLogCapture.
In `@packages/report-web/src/renderers.ts`:
- Around line 2624-2631: parseLogTimestamp currently picks the reference year
directly which causes New Year rollovers to be assigned the wrong year; change
it to try year candidates (referenceYear - 1, referenceYear, referenceYear + 1),
construct a Date for each candidate (using parsed month/day/time), compare
absolute difference to new Date(referenceDate) (or now if referenceDate is
absent) and return the ISO string for the candidate with the smallest
difference; apply the same nearest-year logic to the duplicate parser block
referenced around the other occurrence (lines ~2665-2673) so cross-year logs are
assigned the closest calendar year to recordingStartedAt.
---
Nitpick comments:
In `@packages/cli/src/reportWriter.ts`:
- Around line 722-736: The current logic still reads the entire file into memory
even when stats.size > 50MB; change behavior to avoid whole-file redaction for
oversized logs by skipping in-memory redaction for large files. Detect the
large-file condition using stats.size (the existing check) and after
fsp.copyFile simply log a warning and do not call fsp.readFile,
redactResolvedValue, or fsp.writeFile for files exceeding the cap (or
alternatively implement a streaming redaction path using
fs.createReadStream/fs.createWriteStream if you prefer to preserve redaction for
large files). Update the code paths around sourcePath/targetPath, the stats.size
check, and the block that calls redactResolvedValue so that big files are copied
but not loaded into memory.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b0b595b-667b-4a78-a1ea-3d2295e97847
📒 Files selected for processing (10)
packages/cli/src/reportServer.tspackages/cli/src/reportTemplate.tspackages/cli/src/reportWriter.tspackages/cli/src/testSelection.tspackages/device-node/src/device/AndroidLogcatProvider.tspackages/device-node/src/device/Device.tspackages/device-node/src/device/LogCaptureManager.tspackages/device-node/src/grpc/GrpcDriverSetup.tspackages/report-web/src/artifacts.tspackages/report-web/src/renderers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/src/reportServer.ts
- packages/report-web/src/artifacts.ts
- packages/device-node/src/device/LogCaptureManager.ts
Comment on lines
73
to
79
| if (!stats?.isFile()) { | ||
| throw new Error(`Test selector not found: ${resolvedPath}`); | ||
| const similar = findSimilarFiles(selector, allTestFiles, testsDir); | ||
| const suggestion = similar.length > 0 | ||
| ? `\n\nDid you mean?\n${similar.map(f => ` - ${f}`).join('\n')}` | ||
| : ''; | ||
| throw new Error(`Test file not found: ${selector}${suggestion}`); | ||
| } |
There was a problem hiding this comment.
Error message change breaks existing test assertions.
The error message was changed from "Test selector not found: ${resolvedPath}" to "Test file not found: ${selector}". Per the provided context, finalrun.test.ts:811 asserts assert.match(result.stderr, /Test selector not found:/); which will now fail.
Additionally, the test at finalrun.test.ts:508 uses assert.doesNotMatch(result.stderr, /Test selector not found/); which will now pass vacuously, defeating the intent of that check.
Update the test assertions to match the new error message:
- assert.match(result.stderr, /Test selector not found:/);
+ assert.match(result.stderr, /Test file not found:/);- assert.doesNotMatch(result.stderr, /Test selector not found/);
+ assert.doesNotMatch(result.stderr, /Test file not found/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/testSelection.ts` around lines 73 - 79, The tests still
assert the old error text "Test selector not found:" but the code in
testSelection.ts now throws "Test file not found: ${selector}", so update the
failing assertions to match the new message; change occurrences of
assert.match(result.stderr, /Test selector not found:/) to
assert.match(result.stderr, /Test file not found:/) and change
assert.doesNotMatch(result.stderr, /Test selector not found/) to
assert.doesNotMatch(result.stderr, /Test file not found/) (or adjust the regexes
to accept both variants) so the assertions align with the error thrown by the
code in testSelection.ts (the block that throws new Error(`Test file not found:
${selector}${suggestion}`)).
Comment on lines
+118
to
+139
| const killSent = params.process.kill('SIGINT'); | ||
| Logger.i(`AndroidLogcatProvider: Sent SIGINT to adb logcat process: ${killSent}`); | ||
|
|
||
| if (!killSent) { | ||
| Logger.e( | ||
| `AndroidLogcatProvider: Failed to deliver SIGINT for log capture file: ${params.outputFilePath}`, | ||
| ); | ||
| return new DeviceNodeResponse({ | ||
| success: false, | ||
| message: 'Failed to send SIGINT to adb logcat process.', | ||
| }); | ||
| } | ||
|
|
||
| const exitCode = await this._waitForExit(params.process); | ||
| Logger.i( | ||
| `AndroidLogcatProvider: adb logcat process exited with code ${exitCode} for file: ${params.outputFilePath}`, | ||
| ); | ||
|
|
||
| // Flush and close the write stream piped from stdout | ||
| if (params.process.stdout) { | ||
| params.process.stdout.unpipe(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/device-node/src/device/AndroidLogcatProvider.ts | head -200Repository: final-run/finalrun-agent
Length of output: 7654
🏁 Script executed:
grep -r "abortLogCapture" packages/cli/src/sessionRunner.ts -A 5 -B 5Repository: final-run/finalrun-agent
Length of output: 914
🏁 Script executed:
rg "abortLogCapture|keepPartialOnFailure" packages/ --type ts -B 2 -A 10Repository: final-run/finalrun-agent
Length of output: 17025
🏁 Script executed:
sed -n '130,160p' packages/device-node/src/device/LogCaptureManager.tsRepository: final-run/finalrun-agent
Length of output: 921
🏁 Script executed:
grep -n "async abortLogCapture" packages/device-node/src/device/LogCaptureManager.ts -A 30Repository: final-run/finalrun-agent
Length of output: 1149
🏁 Script executed:
grep -n "async stopLogCapture" packages/device-node/src/device/LogCaptureManager.ts -A 50 | head -80Repository: final-run/finalrun-agent
Length of output: 1587
🏁 Script executed:
grep -n "async stopLogCapture" packages/device-node/src/device/LogCaptureManager.ts -A 70 | tail -30Repository: final-run/finalrun-agent
Length of output: 1072
🏁 Script executed:
cat -n packages/device-node/src/device/LogCaptureManager.ts | sed -n '159,230p'Repository: final-run/finalrun-agent
Length of output: 2602
🏁 Script executed:
cat -n packages/device-node/src/device/LogCaptureManager.ts | sed -n '197,206p'Repository: final-run/finalrun-agent
Length of output: 373
🏁 Script executed:
web_search "Node.js ChildProcess kill() returns false when process already exited"Repository: final-run/finalrun-agent
Length of output: 118
Handle already-exited adb logcat processes gracefully instead of reporting failure.
If the child process has already exited before stopLogCapture() is called, kill('SIGINT') returns false and the method incorrectly reports failure. While _waitForExit() does check process.exitCode and returns early for already-exited processes, the failure at lines 121-128 prevents this path from being reached. Instead, treat a non-zero exitCode check as "already stopped" and proceed with cleanup. Additionally, consider waiting for the child 'close' event rather than just 'exit' to ensure piped stdout has fully flushed to disk before returning success, since 'exit' fires before streams finish draining.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/device-node/src/device/AndroidLogcatProvider.ts` around lines 118 -
139, In stopLogCapture(), don't treat process.kill('SIGINT') returning false as
an immediate failure; instead, if kill returned false check
params.process.exitCode (via the existing _waitForExit() helper) and continue
cleanup because the child may have already exited; also change the wait to
listen for the child process 'close' event (or adapt _waitForExit() to await
'close') so piped stdout is fully flushed before unpipe/closing the stream and
returning success, and only return a failure if the process truly fails with a
non-zero exit code after this close-based wait; update references:
stopLogCapture(), _waitForExit(), params.process, params.outputFilePath.
…cesses - Update finalrun.test.ts assertions to match new "Test file not found" message - Handle already-exited adb/xcrun processes gracefully in stopLogCapture instead of reporting false failure when process.exitCode is already set Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
arnoldlaishram
approved these changes
Apr 8, 2026
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.
Summary
Adds end-to-end device log capture for every test run, with an interactive report UI and critical iOS reliability fixes.
Device Log Capture Pipeline
LogCaptureManager/LogCaptureProviderabstraction mirroring the existing recording pipeline patternadb logcatwith PID-based filtering per app, ring buffer clearing before each testxcrun simctl spawn log streamwith process-name predicate filteringReportWriterinto test artifacts with timestamps (deviceLogFile,deviceLogStartedAt,deviceLogCompletedAt)RunManifest.schemaVersionbumped from 2 → 3Interactive Report UI (Device Logs Tab)
12 / 345 lines)Cmd+F/Ctrl+Ffocuses the log search when the logs tab is activecli/reportTemplate.ts(standalone server) andreport-web/renderers.ts(Next.js app)iOS gRPC Driver Recovery
exitCode/killed) — no error message parsing_withDriverRecovery()wrapper on all gRPC-calling methods inIOSSimulator_unaryCallretry with linear backoff (500ms, 1000ms) for transient connection errorsmaxRetries: 0to avoid double-retry with ScreenshotCapture's own retry loopiOS Log Filtering Fixes
CFBundleExecutablefromxcrun simctl listappsso log stream predicate uses actual process name (e.g.,Wikipedia) instead of bundle ID (org.wikimedia.wikipedia)com.apple.dt.xctest,com.apple.UIKit,com.apple.BackBoard) that accounted for ~55% of log volumeFiles Changed (43 files, +3156 / -172)
New files:
packages/device-node/src/device/LogCaptureManager.ts— manager with per-device session trackingpackages/device-node/src/device/LogCaptureProvider.ts— provider interfacepackages/device-node/src/device/AndroidLogcatProvider.ts— Android logcat implementationpackages/device-node/src/device/IOSLogProvider.ts— iOS log stream implementationpackages/device-node/src/device/LogInfo.ts— log info typepackages/common/src/models/DeviceLog.ts— shared log capture result typeKey modified files:
packages/device-node/src/device/Device.ts— log capture lifecycle + log filter identifier resolutionpackages/device-node/src/device/ios/IOSSimulator.ts— driver recovery +resolveLogFilterIdentifierpackages/device-node/src/grpc/GrpcDriverClient.ts— retry logicpackages/device-node/src/grpc/setup/IOSSimulatorSetup.ts— restart callbackpackages/device-node/src/infra/ios/SimctlClient.ts—CFBundleExecutableextractionpackages/cli/src/sessionRunner.ts— log capture orchestration in test lifecyclepackages/cli/src/reportWriter.ts— log artifact writingpackages/cli/src/reportTemplate.ts— report UI with search/filter/scrollbarpackages/report-web/src/renderers.ts— report UI (Next.js version)Test plan
device.logcontains actual app logs (not empty, not just XCTest noise)device.logcaptures logcat with PID filtering🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation