perf(agents): reuse unchanged remote thread items across polls

[danielgap]

🔗 Linked Issue

Part of #1011 (status:approved) — first slice of the redesigned presence-poll delivery discussed on #1123. Deliberately does not close #1011: the elapsed-render-gate slice retargeted on top of this PR completes it.

🏷️ PR Type

• type:bug — Bug fix
• type:feature — New feature (perf → type:feature; label is maintainer-side, pull-only author)
• type:question — Question requiring tracked work
• type:docs — Documentation only
• type:refactor — Code refactoring
• type:chore — Maintenance/tooling
• type:breaking-change — Breaking change

📝 Summary

• refreshPresence() rebuilt every remote ThreadItem object on each 1-second poll, so the WeakMap<ThreadItem, string[]> render cache in threadLines missed for all remote items even when nothing render-relevant changed.
• Tool items are sanitized (args: {}) before a plain-JSON deep-equality check; when the new item equals the prior item at the same position, the prior object is reused so its cached rendering hits. Any delta keeps a fresh identity, so changed content can never be served stale (rendering depends only on item content and width).
• This is the base unit for the perf(agents): skip renders for unchanged presence polls #1123 redesign: once it lands, perf(agents): skip renders for unchanged presence polls #1123 is retargeted on top so its slice carries only the clock-derived elapsed render gate with its tests, and the CodeRabbit cache finding is answered by this landed unit.

📦 Size decision

Measured: 192 changed lines (191+/1−) against main — within the 400-line review budget.

📋 Changes

File	Change
lib/agents-view.ts	sameItemValue deep-equality helper; refreshPresence item mapping sanitizes first and reuses the prior ThreadItem when deeply equal at the same position
tests/agents-view-thread-identity.test.ts	new 3-test regression suite: unchanged items reuse identity across real polls; changed item takes a new identity while unchanged siblings reuse theirs; tool items compare post-sanitization

✅ Test Plan

• node --experimental-strip-types --test tests/agents-view-thread-identity.test.ts — RED observed (3 identity-assertion failures) on pristine source, then 3/3 pass after the fix
• node --experimental-strip-types --test tests/agents-view.test.ts — 27/27 pass
• pnpm run typecheck — 188 recorded diagnostics, no regressions (10 file/code pairs improved)
• pnpm test — 3406 pass / 0 failed assertions / 41 skipped; unit stage exits 1 on 36 cancellations that reproduce identically on current main (unref'd AbortSignal.timeout fixture liveness: 26 in tests/inprocess-reviewer.test.ts first at :274, 10 in tests/rdd-status-line.test.ts first at :169) — environmental, not introduced here
• Independent read-only verification of commit 65466d52: PASS, no blockers (reuse compares sanitized-vs-sanitized both directions; growth/shrink/insertion-shift edges are conservative — worst case a cache miss, never staleness)

🧾 Review provenance

Native review approved for this exact candidate: lineage review-2bd0fed047a44611 (medium, review-reliability), closure approved and acknowledgement burned (sha256:a4e9657494d4b0f6e8d8f04127a5ef6043906b21a87630422a682f38e9e3faf6). Four advisory findings were explicitly informational and non-blocking; recorded as follow-up work, not corrections to this candidate:

• growth-branch test coverage (lib/agents-view.ts:220) — naturally fits the perf(agents): skip renders for unchanged presence polls #1123 test slice
• positional-misalignment note (:216-224)
• real-timer dependence in the new tests (tests/agents-view-thread-identity.test.ts:89-97)
• render-purity assumption (:223) — keep renderThreadItem dependent only on item content + width

🏷️ Labels (maintainer-side, pull-only author)

• type:feature (exactly one type)

✅ Contributor Checklist

• Linked an approved issue (perf: fullscreen mode still re-renders the full transcript on the 1 Hz agents-view presence poll #1011, part-of by design; closing keyword intentionally on the perf(agents): skip renders for unchanged presence polls #1123 slice)
• Added exactly one type:* label — maintainer-side (pull-only)
• Ran shellcheck on modified scripts — no shell scripts touched
• Skills load correctly — no skill assets changed
• Docs updated — no behavior-facing docs surface for this internal cache reuse
• Conventional commit format (perf(agents): ...)
• No Co-Authored-By trailers

Summary by CodeRabbit

• Bug Fixes
  • Unchanged remote thread items now remain consistent across presence refreshes, while items whose content changes are updated. This helps prevent unchanged parts of a thread from being needlessly disrupted as new presence data arrives, including when only some items in a thread have changed.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c45be466-8623-40b5-a9ba-5a109179be72

📥 Commits

Reviewing files that changed from the base of the PR and between b50b417 and 65466d5.

📒 Files selected for processing (2)

• lib/agents-view.ts
• tests/agents-view-thread-identity.test.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

Remote thread refreshes now reuse prior item objects when sanitized item values match. Regression tests check identity reuse across presence polls, changed-item identities, unchanged siblings, and sanitized tool arguments.

Changes

Remote Thread Item Identity

Layer / File(s)	Summary
Recursive item-value comparison lib/agents-view.ts	A new helper recursively compares values, including object keys and nested values.
Presence refresh identity reuse lib/agents-view.ts, tests/agents-view-thread-identity.test.ts	Presence refreshes reuse prior item objects when sanitized values match. Tests cover unchanged and changed items across polls, including tool items with sanitized empty args.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: alan-thegentleman

Merge Risk: ⚪ Minimal · up to 65466

No merge-blocking issue is identified for the presence-poll identity change; merge after normal checks.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	Issue [#1011] requires the 1 Hz presence poll to avoid a costly full layout pass or make that pass effectively cheap when UI state is unchanged. This PR correctly reuses unchanged sanitized `ThreadIte…	Complete the remaining [#1011] behavior for unchanged presence polls. Prevent the poll from driving the costly full render when no meaningful UI state changed, or provide an equivalent elapsed/render gate that satisfies the issue's expected…
Docstring Coverage	⚠️ Warning	Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: reusing unchanged remote thread items across presence polls.
Out of Scope Changes check	✅ Passed	The changes stay within the scope of [#1011]. The sameItemValue comparison, sanitized tool-item reuse, and identity regression tests directly support reuse of cached remote thread renderings. No unr…

Full details: Linked Issues check

Explanation

Issue [#1011] requires the 1 Hz presence poll to avoid a costly full layout pass or make that pass effectively cheap when UI state is unchanged. This PR correctly reuses unchanged sanitized ThreadItem objects and adds regression tests for item identity. However, refreshPresence() still calls requestRender() after every applied poll, and the PR does not add a render gate or establish that the remaining full layout pass is cheap enough. The implementation is one performance slice, not the complete directly linked issue objective.

Resolution

Complete the remaining [#1011] behavior for unchanged presence polls. Prevent the poll from driving the costly full render when no meaningful UI state changed, or provide an equivalent elapsed/render gate that satisfies the issue's expected behavior. Add an automated regression test for the no-change poll path.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[carlosmoradev]

Excellent work @danielgap! Really clean performance optimization.

Reusing prior ThreadItem references via sameItemValue directly solves the WeakMap cache invalidation issue without introducing staleness risks. Because thread logs are append-only, the positional comparison is safe, and the worst-case on drops is simply a cache miss rather than serving stale rendering. Sanitizing tool args before comparison is also a great defensive touch.

The regression suite in tests/agents-view-thread-identity.test.ts thoroughly verifies object identity retention across disk polls, and all CI checks are green. Looks ready for merge to me!

[danielgap]

Thanks Carlos! Really glad the sameItemValue approach held up.

Your read matches the intent exactly: thread logs are append-only, so positional comparison is safe, and the worst case on drops is a cache miss rather than stale rendering. Sanitizing tool args before comparison was the defensive gap I wanted closed for good.

Merging it now.

[danielgap]

Small correction from my side: I don't have merge permissions on this repo, so the merge button is maintainer-side.

@carlosmoradev when you get a chance, could you merge this one? Everything is green on my end.