Repository navigation
Fix shared WebView task manager attribution - #4047
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds resource attribution for shared webview surfaces by introducing a method to divide CPU and memory metrics across multiple occurrences, integrating it into webview annotation, and validating the behavior with unit and integration tests. ChangesAttributed Payload for Shared Webview Resources
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/TaskManagerResourcesTests.swift`:
- Around line 10-25: Add unit tests for CmuxTopResourceSummary.attributedPayload
to cover edge cases of the occurrenceCount (sharedAcross) parameter: create
tests that call attributedPayload(sharedAcross: 1) to assert the payload is
unchanged (cpuPercent, residentBytes, virtualBytes, processCount, pids),
attributedPayload(sharedAcross: 0) to assert no divide-by-zero/crash and values
remain unchanged (or documented behavior), and attributedPayload(sharedAcross:
-1) to ensure negative inputs are handled consistently; place these new test
methods in TaskManagerResourcesTests.swift (e.g.,
testAttributedPayloadReturnsUnmodifiedPayloadWhenOccurrenceCountIsOne,
testAttributedPayloadHandlesZeroOccurrenceCount,
testAttributedPayloadHandlesNegativeOccurrenceCount) and assert the same numeric
expectations used in the existing test.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1b60175a-0141-4a10-b9cb-c32f6e4d894e
📒 Files selected for processing (4)
Sources/CmuxTopSnapshot.swiftSources/TerminalControllerTopSupport.swiftcmuxTests/CmuxTopSnapshotScopeTests.swiftcmuxTests/TaskManagerResourcesTests.swift
Greptile SummaryThis PR fixes task manager resource over-reporting when multiple WebView rows share the same WebContent PID. Previously each row showed the full process totals; now each row shows its proportional (1/N) share of CPU, resident memory, and virtual memory via the new
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to WebView resource attribution math with no effect on auth, persistence, or process control. The prorating logic is straightforward integer/float division applied only to display metrics; the unit tests explicitly cover the prorating calculation, edge cases for counts ≤ 1, and an end-to-end shared-PID fixture. Parent-level resource totals are unaffected because they use Set-deduplicated summation. The only open question is whether No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant TC as TerminalController
participant AWV as v2AnnotateTopWebView
participant SNAP as CmuxTopProcessSnapshot
participant SUM as CmuxTopResourceSummary
TC->>AWV: "v2AnnotateTopWebView(&webview, …, browserPIDOccurrences)"
AWV->>SNAP: expandedPIDs(rootPIDs: [pid])
SNAP-->>AWV: "pids (Set<Int>)"
AWV->>SNAP: summary(for: pids, rootPIDs: rootPIDs)
SNAP-->>AWV: CmuxTopResourceSummary (full totals)
AWV->>SUM: attributedPayload(sharedAcross: sharedProcessCount)
Note over SUM: cpuPercent /= N<br/>residentBytes /= N<br/>virtualBytes /= N<br/>pids, processCount unchanged
SUM-->>AWV: attributed [String: Any] payload
AWV-->>TC: "pids (Set<Int>, for parent rollup)"
Note over TC: Parent surface/pane/workspace/window<br/>use Set-deduplicated summaryPayload()<br/>so shared PIDs count only once
Reviews (2): Last reviewed commit: "Test shared WebView attribution edge cas..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func attributedPayload(sharedAcross occurrenceCount: Int) -> [String: Any] { | ||
| guard occurrenceCount > 1 else { return payload() } | ||
| var attributed = self | ||
| attributed.cpuPercent /= Double(occurrenceCount) | ||
| attributed.residentBytes = attributed.residentBytes / Int64(occurrenceCount) | ||
| attributed.virtualBytes = attributed.virtualBytes / Int64(occurrenceCount) | ||
| return attributed.payload() | ||
| } |
There was a problem hiding this comment.
missingPIDs is not attributed and its behaviour is untested
attributedPayload divides CPU/resident/virtual but passes missingPIDs through unchanged. If a shared WebContent PID is absent from the process snapshot, every WebView row referencing it will contain that PID in missing_pids. A consumer that reacts to missing_pids (e.g. "process gone – show reconnect banner") will fire N times for the same dead process. The unit test seeds an empty missingPIDs, so this path has no coverage today. Is duplicating missingPIDs across all shared WebView rows intentional, or should only one row carry the list while the others see []?
There was a problem hiding this comment.
Leaving missing_pids unchanged is intentional because it is row-scoped diagnostic metadata, not an attributable resource measurement. A consumer that aggregates missing PIDs should de-dupe by PID across rows.
— Claude Code
Addressed in commit 1e58b18; CodeRabbit resolved the inline thread after re-review.
This branch was successfully deployed
1 participant
Fixes task manager resource attribution for WebView rows that share the same WebContent PID. Each shared WebView row now shows its attributed share of CPU, resident memory, and virtual memory while keeping the PID list intact for process actions.
Verification:
./scripts/reload.sh --tag wvpercTaskManagerResourcesTests/testAttributedPayloadProratesSharedResourceMeasurements,CmuxTopSnapshotScopeTests/testSharedWebViewResourceRowsAreAttributedAcrossOccurrences./scripts/reload.sh --tag wvpc1Note
Medium Risk
Adjusts task manager resource attribution for WebView rows sharing a PID; risk is moderate because it changes how CPU/memory numbers are computed and displayed but is scoped and covered by new unit tests.
Overview
Fixes task manager resource attribution when multiple WebView rows share the same WebContent PID by prorating CPU, resident memory, and virtual memory per occurrence while keeping the underlying PID lists intact for actions.
Adds
CmuxTopResourceSummary.attributedPayload(sharedAcross:)and updatesv2AnnotateTopWebViewto use it (and to clampshared_process_countto at least 1). Adds targeted unit tests validating both the prorating logic and end-to-end window/WebView annotation behavior for shared PIDs.Reviewed by Cursor Bugbot for commit 1e58b18. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fix resource attribution in the task manager for shared WebView rows. Each WebView now shows its proportional CPU, resident, and virtual memory when multiple rows share the same WebContent PID, while keeping PID lists usable for process actions; edge cases (<=1 occurrence) return unmodified totals.
attributedPayload(sharedAcross:)(no-op for counts <= 1).TerminalController.Written for commit 1e58b18. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Tests