Repository navigation
Add Safari DevTools diagnostic logging - #1161
lawrencecchen wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds DEBUG-only diagnostic logging across browser portal and panel codepaths, introduces a portalEntryMissing computed flag used during portal binding decisions, and guards repeated DevTools/console shortcut repeats; no public API/signature changes or functional behavior changes beyond diagnostics and early-return on repeat key events. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR adds comprehensive Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as BrowserPanelView
participant BP as BrowserPanel
participant WKI as WKWebView Inspector
participant PR as BrowserWindowPortalRegistry
UI->>BP: toggleDeveloperTools()
BP-->>BP: log toggle.begin [diagnosticsSummary]
BP->>WKI: cmuxInspectorObject()
alt inspector nil
BP-->>BP: log toggle.abort reason=no_inspector
else inspector found
BP->>WKI: responds(to: show/close)?
alt selector missing
BP-->>BP: log toggle.abort reason=missing_selector
else selector available
BP->>WKI: cmuxCallVoid(show/close)
BP-->>BP: log toggle.end [targetVisible, selector]
BP-->>BP: async tick: log toggle.tick
end
end
UI->>BP: showDeveloperTools()
BP->>WKI: cmuxInspectorObject()
alt no inspector
BP-->>BP: log show.abort reason=no_inspector
else
BP->>WKI: cmuxCallVoid(show)
BP->>WKI: isVisible?
alt visible after show
BP->>BP: cancelDeveloperToolsRestoreRetry()
BP-->>BP: log retry.cancel
else not visible
BP->>BP: scheduleDeveloperToolsRestoreRetry()
BP-->>BP: log retry.schedule [attempt, delay]
note over BP: after delay…
BP-->>BP: log retry.fire [attempt]
BP->>BP: restoreDeveloperToolsAfterAttachIfNeeded()
end
BP-->>BP: log show.end [visibleBefore, visibleAfter]
end
UI->>BP: updateViewState (portal)
BP->>PR: isWebView(boundTo: anchorView) [portalEntryMissing check]
note over BP,PR: ⚠ captured before installPortalAnchorView
UI->>PR: bind / updateEntryVisibility / hide
alt missing_window_mapping
PR-->>PR: log portal.*.skip reason=missing_window_mapping
else portal found
PR->>PR: update entry
PR-->>PR: log portal.visibility / portal.hide / portal.refresh
end
BP-->>BP: log portal.update [full diagnosticsSummary + entryMissing]
Last reviewed commit: 7977484 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let activeSearchOverlay = coordinator.desiredPortalVisibleInUI ? searchOverlay : nil | ||
| let portalAnchorView = panel.portalAnchorView | ||
| let portalHideReason = !isCurrentPaneOwner ? "lostPaneOwnership" : "hidden" | ||
| let portalEntryMissing = !BrowserWindowPortalRegistry.isWebView(webView, boundTo: portalAnchorView) |
There was a problem hiding this comment.
portalEntryMissing captured before installPortalAnchorView changes anchor window
portalEntryMissing is now computed at this line, before installPortalAnchorView(portalAnchorView, in: host) runs at line ~4370. BrowserWindowPortalRegistry.isWebView returns false immediately when anchorView.window == nil, so on any first-attach (or re-attach after a detach) where portalAnchorView is not yet in a window, portalEntryMissing will be true — not because no portal entry exists in the registry, but simply because the window lookup short-circuits. The old code computed this variable inside the if host.window != nil, portalHostAccepted block, after installPortalAnchorView, which correctly queried "does the registry have a binding for this anchor's current window?"
The functional impact on shouldBindNow is benign (a stale true just makes bind run, which is correct), but the entryMissing=1 field in the portal.update diagnostic log now conflates "anchor had no window yet" with "binding was genuinely absent", reducing the signal quality of the log you're adding this PR to improve. Consider moving it back inside the if host.window != nil, portalHostAccepted block at line ~4441, or at minimum computing it after installPortalAnchorView so the window lookup is valid.
| private func logDeveloperToolsDebug(event: String, details: String? = nil) { | ||
| var line = "browser.devtools event=\(event) panel=\(id.uuidString.prefix(5)) \(debugDeveloperToolsDiagnosticsSummary())" | ||
| if let details, !details.isEmpty { | ||
| line += " \(details)" | ||
| } | ||
| dlog(line) | ||
| } |
There was a problem hiding this comment.
cmuxInspectorObject() called twice per log event via debugDeveloperToolsDiagnosticsSummary
debugDeveloperToolsDiagnosticsSummary() composes four sub-summaries. Two of them independently invoke cmuxInspectorObject():
debugDeveloperToolsStateSummary()(line 3808):let inspector = webView.cmuxInspectorObject() == nil ? 0 : 1debugDeveloperToolsInspectorSummary()(line 3761):let inspector = webView.cmuxInspectorObject()
Since logDeveloperToolsDebug calls debugDeveloperToolsDiagnosticsSummary() unconditionally on every event — including the high-frequency retry loop and every portal.update tick — this means every single log line incurs two private-API calls to obtain the inspector object. In addition, debugDeveloperToolsGeometrySummary() performs a full subview-tree walk (debugInspectorSubviewCount) on each call.
This is #if DEBUG-only so production is unaffected, but repro sessions can generate many events in quick succession (retries, view-state updates), and the redundant work makes the debug build noticeably heavier. Caching the inspector result inside debugDeveloperToolsDiagnosticsSummary and sharing it across the four sub-summaries would eliminate the duplication and the double lookup.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 7692-7697: The logs for repeat-suppressed shortcuts are
incorrectly marking the action as handled; update the calls to
logDeveloperToolsShortcutSnapshot (the branches checking event.isARepeat) to
record didHandle: false (or remove the handled flag) so repeats are logged as
ignored rather than "handled"; make the same change for both occurrences (the
toggle.repeatIgnored branch and the other repeat-suppression branch around the
second occurrence) so diagnostic traces correctly reflect that the action did
not run.
| if event.isARepeat { | ||
| #if DEBUG | ||
| logDeveloperToolsShortcutSnapshot(phase: "toggle.repeatIgnored", event: event, didHandle: true) | ||
| #endif | ||
| return true | ||
| } |
There was a problem hiding this comment.
Don't log ignored repeats as handled.
These branches intentionally skip the action, but the new trace records handled=1. In the surrounding post/tick logs, handled means the browser action actually ran, so this will make repeat-suppression look like a successful inspector/console open and muddy the diagnostics.
🔧 Proposed fix
if event.isARepeat {
`#if` DEBUG
- logDeveloperToolsShortcutSnapshot(phase: "toggle.repeatIgnored", event: event, didHandle: true)
+ logDeveloperToolsShortcutSnapshot(phase: "toggle.repeatIgnored", event: event)
`#endif`
return true
}
@@
if event.isARepeat {
`#if` DEBUG
- logDeveloperToolsShortcutSnapshot(phase: "console.repeatIgnored", event: event, didHandle: true)
+ logDeveloperToolsShortcutSnapshot(phase: "console.repeatIgnored", event: event)
`#endif`
return true
}Also applies to: 7713-7718
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 7692 - 7697, The logs for
repeat-suppressed shortcuts are incorrectly marking the action as handled;
update the calls to logDeveloperToolsShortcutSnapshot (the branches checking
event.isARepeat) to record didHandle: false (or remove the handled flag) so
repeats are logged as ignored rather than "handled"; make the same change for
both occurrences (the toggle.repeatIgnored branch and the other
repeat-suppression branch around the second occurrence) so diagnostic traces
correctly reflect that the action did not run.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
git diff --check./scripts/reload.sh --tag safari-devtools-logsfailed in this worktree because the current machine is missing the Metal toolchain component needed to buildGhosttyKit.xcframework; after reusing the existing framework from the base checkout, taggedxcodebuildprogressed through the modified files and produced the app bundle, but LaunchServices failed to open the tagged app with error-54Task
Summary by cubic
Adds detailed, DEBUG-only diagnostics for Safari DevTools and the portal lifecycle to trace why the inspector fails to show or attach. Also ignores repeated DevTools shortcuts to prevent accidental rapid toggles.
#if DEBUG; shortcut repeat handling applies in all builds.Written for commit 6dbf582. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Chores