Skip to content

fix(desktop): bot faces stop repainting in a hidden Bots tab - #128219

Open
kshitijk4poor wants to merge 3 commits into
NousResearch:mainfrom
kshitijk4poor:fix/desktop-bots-hidden-face-clock
Open

kshitijk4poor wants to merge 3 commits into
NousResearch:mainfrom
kshitijk4poor:fix/desktop-bots-hidden-face-clock

Conversation

@kshitijk4poor

@kshitijk4poor kshitijk4poor commented Sep 29, 2026 •

Copy link
Copy Markdown

Outcome

After the Bots sidebar tab has been opened once, its bot faces stop repainting while the tab is hidden. Before this, every roster face kept animating at 15 fps for the rest of the session behind the Sessions tab.

Measured on isolated dev instances with four bot profiles, re-run on the final head after the rebase onto 7239625ae1. Main and this branch ran back-to-back, twice (A B A B). Each window comes after one visit to Bots and a return to Sessions, 3 windows per run:

rAF callbacks / 30 s layouts / 45 s DOM mutations / 45 s renderer CPU / 45 s GPU CPU / 45 s
main, run 1 1,800 / 1,800 / 1,800 594 / 594 / 597 47,520 / 47,520 / 47,760 3.29 / 3.27 / 3.30 s 0.58 / 0.58 / 0.57 s
this branch, run 1 36 / 36 / 36 0 / 0 / 0 0 / 0 / 0 1.18 / 1.16 / 1.15 s 0.37 / 0.39 / 0.37 s
main, run 2 1,800 / 1,800 / 1,800 591 / 593 / 593 47,280 / 47,440 / 47,440 3.30 / 3.26 / 3.35 s 0.58 / 0.59 / 0.60 s
this branch, run 2 35 / 32 / 36 0 / 0 / 0 0 / 0 / 0 1.34 / 1.15 / 1.53 s 0.40 / 0.42 / 0.43 s

The remaining ~36 rAF callbacks per 30 s come from the once-a-second recheck that notices a hidden face being shown again (see below). Each recheck is one frame and one document scan.

Before the Bots tab has been opened, both branches run 0 rAF callbacks. With the tab visible, both run 600 per 10 s, and revisiting the tab resumes at 600 per 10 s, so the faces animate exactly as before.

Load average was about 20–30 from other work. The counts don't depend on load; CPU compares fairly because the runs interleave.

Why

Visiting a sidebar tab keeps its pane mounted as a keep-alive layer hidden with visibility: hidden (#69750, 30f6fc81e2, 2026-07-22). The shared face clock decided what to paint from an IntersectionObserver alone. That gate came from ba2fb191c6 by @digitalbase, landed via @teknium1's #88543 on 2026-08-17. A visibility: hidden layer keeps its box, so the observer kept reporting every roster face as intersecting, and the clock never parked.

What changed

  • plugins/hermes-bots/avatar.tsx:
    • The clock paints only faces that intersect AND pass checkVisibility({ visibilityProperty: true }). It parks when none do.
    • Visibility is re-checked at the existing 1 Hz rescan and in observer callbacks, not on every painted frame. It only changes on a tab switch, and a per-frame check could force style recalcs.
    • This also removes the as () => boolean cast and its TODO: idle is now a plain boolean.
  • plugins/hermes-bots/plugin.tsx: showing the Bots pane wakes the clock. Before, it only woke because every face happened to re-render on the tab switch.
  • Faces also live in other keep-alive tabs: the empty bot chat, group chats and the routines pane. Revealing one of those changes neither intersection nor, under a memoised host, the face's render, so nothing would wake a parked clock. While faces intersect but are all hidden, the clock rechecks once a second instead of parking for good. That is one scan per second, against main's 15 fps repaint of every hidden face. (Found in review.)
  • avatar.face-clock.test.ts: one test. A face that intersects but sits in a hidden pane parks the clock, and revealing it with no re-render resumes painting within a second (fake timers). It fails on main, and it fails with the recheck removed.

Verification

  • npx vitest run src/plugins/hermes-bots: 93 files, 754 tests pass. On this loaded machine, bot-delete.test.ts and hidden-bots.test.ts overrun their 60 s cold-import beforeAll in a full-directory run. Main fails them the same way in the same run, and both pass with the budget raised; they take about 95 s on each branch.
  • The new test fails against main's avatar.tsx and passes with this change.
  • eslint and prettier are clean on the changed files.
  • Live A/B on isolated instances as above.

After the Bots sidebar tab is visited once, its pane stays mounted as a
keep-alive layer hidden with `visibility: hidden`. The shared face clock
only asked an IntersectionObserver which faces were visible, and a hidden
layer keeps its box, so every roster face still counted as intersecting
and was repainted at 15 fps for the rest of the session.

The clock now paints only faces that are intersecting AND pass
`checkVisibility({ visibilityProperty: true })`, and parks when none do.
Re-activating the tab re-renders its BotFaces, which already wakes the
clock. The shown set is computed at the start of each draw (and on
observer callbacks), so the check never forces a mid-frame style recalc.
Re-activating the pane only woke the parked clock because every face
happens to re-render on the tab switch; a memoised ancestor would leave
its face frozen. The pane-visibility listener now wakes the clock
directly. Visibility is also re-checked at the 1Hz rescan instead of on
every painted frame: it only changes on a tab switch.
…nder

Faces also live in chat tabs (the empty bot chat), group chats and the
routines pane, all inside keep-alive tab stacks. Revealing such a tab
changes neither the face's intersection nor, under a memoised host, its
render, so a clock parked on hidden faces would leave them frozen. While
faces intersect but are all hidden, recheck once a second instead of
parking for good: one document scan per second instead of main's 15 fps
repaint of every hidden face.
@kshitijk4poor
kshitijk4poor force-pushed the fix/desktop-bots-hidden-face-clock branch from 29d4d77 to d3ffe30 Compare September 30, 2026 23:09
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

The premise holds and the direction is right — I verified both. The inactive keep-alive tab is hidden with visibility: hidden in pane-body.tsx:40 and (via a Tailwind class) in keep-alive-panes.tsx:130, and pane-visibility.ts / tree-group.tsx:772 both document that visibility rather than display is deliberate so the layout box survives. A hidden layer keeping its box is exactly why the observer kept reporting every roster face as intersecting and the clock never parked.

Worth adding before merge: two of the three behaviours this change introduces have no test that goes red when they're removed.

Your claims, re-verified. The new test fails on main (expected false to be true at line 290) and passes on head; removing the hiddenRecheck block kills it too (expected 1 to be 2), a real kill, not an already-red test. Full-directory run: 1 failed | 749 passed (750), both failures pre-existing or environmental.

The gap that matters most. Mutating svg.checkVisibility({ visibilityProperty: true }) to svg.checkVisibility() leaves the suite fully green. The reason is structural: jsdom has no checkVisibility at all — I measured undefined for both hiding cases — so face.checkVisibility = () => shown replaces the whole decision and nothing observes the options object.

That option is load-bearing. Per MDN, bare checkVisibility() returns true under a visibility: hidden ancestor; it reports false only for a missing box or content-visibility: hidden. Drop the option and refreshShownFaces filters nothing, the clock keeps repainting hidden faces at 15 fps, and nothing fails. A two-line prototype stub closes this: Element.prototype.checkVisibility = vi.fn(() => true) in beforeEach, delete in afterEach, then assert the call arg.

Second gap. Deleting the new startFaceClock() in plugin.tsx keeps the whole hermes-bots suite green. avatar.face-clock.test.ts calls it directly and never goes through the plugin, so the most user-visible path — switch back to the Bots tab, clock wakes — isn't pinned. The recheck covers it today, so this is coverage rather than a bug; but the paths are independent, and if the recheck fails the plugin call is the only wake source left.

Not a defect report; the behaviour looks correct. I'd take a follow-up PR for the two assertions as a courtesy rather than a merge blocker.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants