test(ownts): runtime confirmation that the two OSS true positives really leak#207
Conversation
Adds frontend/ownts/oss-verify — an independent, executable check that the two
true positives in docs/notes/ownts-oss-benchmark.md are real DOM listener leaks,
not just findings the analyzer agrees with itself on.
- react-scroll-to-bottom@4.2.0 Composer.js:574 — addEventListener({capture:true})
torn down by removeEventListener with the default (false) capture; the flag is
part of the removal key, so the listener is never removed.
- @reactuses/core@6.4.0 index.mjs — onPressed = t => () => {…}, so onPressed('mouse')
is a fresh function at add and again at remove; identities differ, so the
drag/touch listeners are never removed.
Each shape is run in a real DOM (jsdom): register -> run the effect's cleanup ->
dispatch; the handler still firing proves the leak. Paired correct-cleanup controls
must go silent, so the leak assertions are non-vacuous. Cross-checked in real
Chromium (identical result, recorded in the README).
Reduced cases are authored here with pkg@version:line + upstream sha256 provenance;
no third-party source is vendored (repo policy). Wired into CI as ownts-oss-verify.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JsBjTfMzCm15fZ616nM1cz
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a jsdom runtime verification package for two OwnTS OSS listener leaks, matching cleanup controls, documentation, dependency setup, and a GitHub Actions job that runs the tests. ChangesOwnTS OSS runtime verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c59e84500
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| element.addEventListener("dragstart", onPressed("mouse")); | ||
| // effect cleanup, as published — a *different* freshly-returned fn | ||
| element.removeEventListener("dragstart", onPressed("mouse")); |
There was a problem hiding this comment.
When this job is used as the runtime proof for @reactuses/core@6.4.0, these lines test a fresh-function identity mismatch (onPressed("mouse") at add/remove), but the cited package's published dist/index.mjs uses useEventListener/useDraggable with stable listener functions and leaks because cleanup drops the capture/options argument. Because the reduced case is not the package shape named in the README, CI can pass while failing to confirm (or catch regressions in) the actual OSS true positive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks — checked this against the published tarball (npm pack @reactuses/core@6.4.0, dist/index.mjs). The cited hook is useMousePressed, and the listeners are not stable functions:
const onPressed = useCallback((srcType) => () => { setPressed(true); setSourceType(srcType); }, []);
...
element.addEventListener('dragstart', onPressed('mouse'), listenerOptions$2); // :2830
...
element.removeEventListener('dragstart', onPressed('mouse')); // :2846onPressed('mouse') is a curried call that returns a fresh function at add and again at remove, so the identities differ — that's the leak, matching the reduced case (and the benchmark note). The dropped argument is listenerOptions$2 = { passive: true } (:2809), which is not the cause: a listener's removal key is (type, callback, capture) — passive isn't part of it, and no capture is set (false on both sides).
That said, your fidelity point was fair: the reduced case had omitted the option. Fixed in 52cb593 — the case-2 leak test now adds with { passive: true } and removes without it (the package's exact shape), and a new control adds with the option and removes without it yet still goes clean, isolating the option-drop as a red herring. Still 5/5.
Generated by Claude Code
…te option-drop
Codex review flagged that the reduced case for @reactuses/core@6.4.0 might not be
the published shape. The published hook (useMousePressed) adds the drag/touch
listeners with listenerOptions$2 = { passive: true } and removes them without it.
The leak, however, is the fresh onPressed('mouse') identity (a curried
useCallback), not the dropped option: `passive` is not part of a listener's
removal key and no capture is set.
- add now passes { passive: true } (as published); remove omits it — exact shape.
- the case-2 control adds WITH the option and removes WITHOUT it and still goes
clean, proving the option-drop is a red herring and identity is the sole cause.
- README/provenance updated (hook name, listenerOptions$2 line, the note).
Still 5/5 pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JsBjTfMzCm15fZ616nM1cz
2 participants
Что и зачем
Добавляет
frontend/ownts/oss-verify— независимое исполняемое подтверждение, что две true-positive находки изdocs/notes/ownts-oss-benchmark.md(react-scroll-to-bottom@4.2.0capture-mismatch и@reactuses/core@6.4.0fresh-fn-identity) — реальные утечки слушателей в DOM, а не то, с чем анализатор просто соглашается сам с собой. Формы прогоняются в настоящем DOM (jsdom): регистрируем слушатель → выполняем cleanup эффекта → диспатчим событие; срабатывание хендлера после cleanup = утечка.Тип изменения
Как проверено
python tests/run_tests.pyruff check .иmypycd frontend/ownts/oss-verify && npm ci && npm test→# tests 5 # pass 5 # fail 0(2 leak-доказательства + 2 корректных-cleanup контроля + 1 проверка root-cause идентичности функции). Каждый leak-кейс парный с контролем, который обязан остаться «тихим», — так ассерты неложные. Кросс-проверено в живом Chromium (результат совпадает, зафиксирован в README). Новый CI-джоб —ownts-oss-verify.Связанные issue
Refs
docs/notes/ownts-oss-benchmark.md(«First confirmed TRUE positive»). Отдельного issue нет.Чеклист
frontend/ownts/oss-verify/README.md(провенанс: pkg@version:line + upstream sha256; таблица jsdom↔Chromium; политика «чужой код не вендорим»)feat:,fix:,docs:…)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests