Skip to content

fix(drag): stop blur listener leak on sidebar and shell drag gestures - #5923

Merged
M3gA-Mind merged 2 commits into
tinyhumansai:mainfrom
YellowSnnowmann:fix/blur-listener-leak-5898
Sep 1, 2026
Merged

fix(drag): stop blur listener leak on sidebar and shell drag gestures#5923
M3gA-Mind merged 2 commits into
tinyhumansai:mainfrom
YellowSnnowmann:fix/blur-listener-leak-5898

Conversation

@YellowSnnowmann

@YellowSnnowmann YellowSnnowmann commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Two removeEventListener('blur-sm', ...) calls were corrupted by a Tailwind codemod that renamed the CSS class blurblur-sm; the event name strings were incorrectly renamed along with the class names.
  • The paired addEventListener('blur', ...) still used the correct name, so the window blur listener was registered but never removed — leaking on every drag that ended via window blur.
  • Fix: restore both event name strings to 'blur' in Sidebar.tsx and RootShellLayout.tsx.

Problem

  • Every drag gesture on the sidebar or shell resize rail that ends via window losing focus (e.g. cmd-tab, window focus change) leaves a dangling blur listener on window.
  • Over a session with many such gestures, these accumulate and fire on every subsequent window blur event — re-triggering stop/detach logic on stale drag state.

Solution

  • Corrected the event name in window.removeEventListener from 'blur-sm' back to 'blur' in the two affected detach functions.
  • No logic changes; purely a string literal fix.

Submission Checklist

  • Tests added or updated — N/A: event-listener string literals are not exercisable in jsdom; the fix is verified by code inspection against the symmetric addEventListener('blur', ...) call.
  • Diff coverage ≥ 80% — N/A: no new code paths; correcting a string literal.
  • Coverage matrix updated — N/A: behaviour-only correction, no new or removed feature rows.
  • All affected feature IDs from the matrix are listed — N/A: no matrix row covers drag-blur listener cleanup.
  • No new external network dependencies introduced — N/A: no dependencies added.
  • Manual smoke checklist updated — N/A: drag-blur is not a release-cut surface.
  • Linked issue closed via Closes #NNNCloses Every sidebar drag leaks a window 'blur' listener ('blur' added, 'blur-sm' removed) #5898 in the Related section below.

Impact

  • Desktop only (drag gestures are desktop UI).
  • No performance or security implications; fixes a listener accumulation / memory leak.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: fix/blur-listener-leak-5898
  • Commit SHA: 4903e9b

Validation Run

  • pnpm --filter openhuman-app format:check
  • pnpm typecheck
  • Focused tests: N/A (string literal fix — no exercisable paths in jsdom)
  • Rust fmt/check (if changed): N/A
  • Tauri fmt/check (if changed): N/A

Validation Blocked

  • command: N/A
  • error: N/A
  • impact: N/A

Behavior Changes

  • Intended behavior change: blur listener is now correctly removed after every drag ends.
  • User-visible effect: eliminates listener accumulation over long sessions; no visible UX change.

Parity Contract

  • Legacy behavior preserved: drag start/stop/cancel paths unchanged.
  • Guard/fallback/dispatch parity checks: symmetric addEventListener/removeEventListener pair now uses matching event name.

Duplicate / Superseded PR Handling

  • Duplicate PR(s): N/A
  • Canonical PR: this PR
  • Resolution: N/A

Summary by CodeRabbit

  • Bug Fixes
    • Improved sidebar dragging cleanup when the browser window loses focus.
    • Prevented stale event handling that could affect subsequent sidebar interactions.

A Tailwind codemod renamed the CSS class blur → blur-sm, but also
corrupted two event-name string literals that happened to contain
the word "blur". The result: removeEventListener('blur-sm', …) never
matched the paired addEventListener('blur', …), so every sidebar or
shell drag that ended via a window blur (e.g. alt-tab) leaked the
pointermove + pointerup + pointercancel listeners for the lifetime of
the session.

Fix: correct the event name in removeEventListener back to 'blur' in
both Sidebar.tsx (detach) and RootShellLayout.tsx (detach).

Closes tinyhumansai#5898
@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: bb382cac-7c25-4b78-9359-061f17833d62

📥 Commits

Reviewing files that changed from the base of the PR and between 4903e9b and 49288d5.

📒 Files selected for processing (1)
  • app/src/components/layout/shell/RootShellLayout.test.tsx

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


📝 Walkthrough

Walkthrough

The changes correct the event name used to remove window-level blur listeners after sidebar drag operations. A regression test verifies that focus loss stops later drag updates.

Changes

Sidebar drag cleanup

Layer / File(s) Summary
Correct blur listener removal
app/src/components/layout/shell/RootShellLayout.tsx, app/src/components/ui/Sidebar.tsx, app/src/components/layout/shell/RootShellLayout.test.tsx
Both cleanup functions now remove the blur event listener instead of blur-sm. The test verifies that sidebar width remains unchanged after window focus loss.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 49288

The PR restores matching blur listener registration and cleanup for sidebar and shell drag gestures, preventing listener accumulation without changing drag behavior. No actionable merge-blocking risk remains.

Suggested reviewers: senamakel

Poem

A rabbit watched the sidebar glide
The blur hook now comes off its ride
No dangling ears remain
Each drag cleans up its chain
And hops away with pride

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the fix to the blur listener leak during sidebar and shell drag gestures.
Linked Issues check ✅ Passed The pull request corrects both removeEventListener calls from 'blur-sm' to 'blur' in the locations required by issue #5898. The regression test verifies cleanup after window blur.
Out of Scope Changes check ✅ Passed The changes are limited to the two affected listener removals and an in-scope regression test. No unrelated code changes are present.
  • Fix all pre-merge checks with AI

Warning

Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice.


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

@YellowSnnowmann
YellowSnnowmann marked this pull request as ready for review September 1, 2026 08:55
@YellowSnnowmann
YellowSnnowmann requested a review from a team September 1, 2026 08:55
@YellowSnnowmann

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

coderabbitai[bot]
coderabbitai Bot previously approved these changes Sep 1, 2026
@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4903e9be74

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread app/src/components/layout/shell/RootShellLayout.tsx
@M3gA-Mind

Copy link
Copy Markdown
Collaborator

Review

Correct and minimal — exactly the two sites, nothing else touched. 'blur-sm'
was a Tailwind class rename that reached an event-name string, so detach()
never removed the listener it added. Both Sidebar.tsx and RootShellLayout.tsx
had the identical pairing.

I checked whether this was a wider class before filing #5898: grepping every
Tailwind-renamed token in addEventListener/removeEventListener position
across app/src returns only these two. Nothing else was affected, so this
PR closes the class rather than a sample of it.

One coordination point before this merges

#5887 contains a test.fail()-marked spec for exactly this bug, in
app/test/playwright/specs/app-shell-listener-hygiene.spec.ts:

test('blur listeners are balanced across a drag — KNOWN FAILING, W5 BUG-12', … {
  test.fail();
  expect(await countOf(page, 'blur')).toBe(before);

test.fail() means Playwright expects the test to fail. The moment this PR
lands, that test starts passing — and a passing test.fail() test is
reported as a failure. That is the marker doing its job (it is meant to convert
into a hard CI signal the day the bug is fixed), but it means whoever merges
second has to remove the marker in the same window.

No objection to the fix itself. Flagging the ordering only.

Proves that a `blur` event mid-drag cleanly detaches the `pointermove`
listener: after `blur`, moving the pointer must not change the sidebar
width.

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinysweeper found nothing blocking. Approving.

             $0.0114 · 81,182 in / 2,277 out · 8,028 cached (10%) · openrouter/openai/text-embedding-3-small, deepseek/deepseek-v4-flash, z-ai/glm-5.2 · 146 embedded
critique:    $0.0074 · 33,417 in / 1,954 out · 8,028 cached (24%) · deepseek/deepseek-v4-flash, z-ai/glm-5.2
security:    $0.0027 · 32,655 in / 160 out   · 0 cached (0%)      · deepseek/deepseek-v4-flash
tests:       $0.0009 · 11,133 in / 93 out    · 0 cached (0%)      · deepseek/deepseek-v4-flash
description: $0.0003 · 3,977 in  / 70 out    · 0 cached (0%)      · deepseek/deepseek-v4-flash

Comment thread app/src/components/layout/shell/RootShellLayout.test.tsx
@tinysweeper

tinysweeper Bot commented Sep 1, 2026

Copy link
Copy Markdown

How this change flows

2 changed behaviours across 9 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 41 further behaviours left out to keep the diagram readable.

flowchart LR
  n0["RootShellLayout<br/>changed"]:::changed
  n1["SidebarRailProps<br/>changed"]:::changed
  n2["commitWidth"]:::impacted
  n3["renderShell"]:::impacted
  n4["SidebarRail"]:::impacted
  n5["cn"]:::impacted
  n6["dispatch"]:::impacted
  n7["SidebarProvider"]:::impacted
  n0 -->|calls| n6
  n0 -->|uses| n6
  n2 -->|calls| n6
  n2 -->|uses| n6
  n3 -->|uses| n4
  n3 -->|uses| n7
  n4 -->|uses| n1
  n4 -->|calls| n5
  n7 -->|calls| n5
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Loading

Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge.

tinysweeper 0.1.0

@tinysweeper tinysweeper Bot added the priority: p2 Soon. Real but survivable — a rough edge, a gap, a thing that will bite later. label Sep 1, 2026
@YellowSnnowmann

Copy link
Copy Markdown
Collaborator Author

Good flag on #5887. Noted: once this PR lands, the test.fail()-marked spec in app/test/playwright/specs/app-shell-listener-hygiene.spec.ts will start passing, which Playwright reports as a failure (the marker doing its job). Whoever merges second needs to drop the test.fail() and the comment in the same pass — ideally as part of the merge or as an immediate follow-up.

@M3gA-Mind M3gA-Mind left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — approving the fix, with one note on the new test

CI green (16/16), zero unresolved threads, mergeable. The fix is right and
closes the whole class — I re-checked that no other Tailwind-renamed token
appears in addEventListener/removeEventListener position anywhere in
app/src; these two sites were all of it.

The added test does not guard the bug it was added for

detaches drag listeners when the window loses focus mid-drag is a good
behavioural test, but I do not think it fails without the fix. Tracing
handleRailPointerDown:

  • addEventListener('blur', stop) is attached either way;
  • on blur, stop() runs and calls detach(), which removes pointerup /
    pointercancel and clears draggingRef either way;
  • so the drag stops and the width stays at 300px with the bug present.

The only thing the bug changes is that the 'blur' listener itself is never
removed — a leak, not a behavioural difference. A test that asserts post-blur
width cannot see that. Catching it needs a listener count across a drag,
which is what app-shell-listener-hygiene.spec.ts in #5887 does.

Not a blocker: the fix is correct and the test is a reasonable behavioural
addition. Flagging it so nobody later reads it as the regression guard.

Merge-ordering, repeated from my earlier comment

#5887 marks this exact bug test.fail(). When this lands, that test starts
passing, and a passing test.fail() test is reported as a failure — the
marker has to come off in the same window. That is the marker working as
designed, not a problem with either PR.

@M3gA-Mind
M3gA-Mind merged commit 2b22f90 into tinyhumansai:main Sep 1, 2026
31 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p2 Soon. Real but survivable — a rough edge, a gap, a thing that will bite later.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Every sidebar drag leaks a window 'blur' listener ('blur' added, 'blur-sm' removed)

2 participants