fix(drag): stop blur listener leak on sidebar and shell drag gestures - #5923
Conversation
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe 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. ChangesSidebar drag cleanup
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to 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: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 review |
|
There was a problem hiding this comment.
💡 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".
ReviewCorrect and minimal — exactly the two sites, nothing else touched. I checked whether this was a wider class before filing #5898: grepping every One coordination point before this merges#5887 contains a
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.
There was a problem hiding this comment.
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
How this change flows2 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
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. |
|
Good flag on #5887. Noted: once this PR lands, the |
M3gA-Mind
left a comment
There was a problem hiding this comment.
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 callsdetach(), which removespointerup/
pointercanceland clearsdraggingRefeither 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.
Summary
removeEventListener('blur-sm', ...)calls were corrupted by a Tailwind codemod that renamed the CSS classblur→blur-sm; the event name strings were incorrectly renamed along with the class names.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.'blur'inSidebar.tsxandRootShellLayout.tsx.Problem
blurlistener onwindow.stop/detachlogic on stale drag state.Solution
window.removeEventListenerfrom'blur-sm'back to'blur'in the two affecteddetachfunctions.Submission Checklist
addEventListener('blur', ...)call.Closes #NNN— Closes Every sidebar drag leaks a window 'blur' listener ('blur' added, 'blur-sm' removed) #5898 in the Related section below.Impact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit