Wait longer for search result containers#1016
Conversation
📝 WalkthroughWalkthroughThe element-waiting utility now performs immediate lookups, cleans up observers and timers, supports timed-out resolution, and exposes a five-second wrapper. Brave and DuckDuckGo use the wrapper, with unit tests covering lookup, mutation, timeout, and cleanup behavior. ChangesSite adapter element waiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BraveOrDuckDuckGo
participant waitForSiteAdapterElement
participant DOM
BraveOrDuckDuckGo->>waitForSiteAdapterElement: wait for results container selector
waitForSiteAdapterElement->>DOM: query selector and observe mutations
DOM-->>waitForSiteAdapterElement: matching element or five-second timeout
waitForSiteAdapterElement-->>BraveOrDuckDuckGo: element or null
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
PR Summary by QodoFix site-adapter container wait by using a 5s timeout helper
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Incremental review note: Re-verified against HEAD Previous Review Summary (commit f982427)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f982427)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Reviewed by hy3:free · Input: 24.8K · Output: 3K · Cached: 232.4K |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the element selection logic by introducing a new helper function waitForSiteAdapterElement with a default 5-second timeout, replacing the direct usage of waitForElementToExistAndSelect in the Brave and DuckDuckGo site adapters. It also updates waitForElementToExistAndSelect to ensure pending timeouts are cleared and MutationObservers are disconnected correctly, and adds comprehensive unit tests. The review feedback suggests using explicit .mjs file extensions for imports in the Brave adapter to ensure ESM compatibility, and recommends using the built-in t.mock.method utility from node:test instead of directly mutating globalThis in the unit tests to prevent potential flakiness.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: f9824274a7
ℹ️ 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".
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/utils/wait-for-element-to-exist-and-select.test.mjs (1)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock
clearTimeoutalongsidesetTimeoutto ensure proper timer cleanup.Since the utility under test calls
clearTimeoutinternally, it is best practice to mock it alongsidesetTimeoutso that the mock timers are properly cleared from Node's mock timer registry. You can achieve this by simply omitting theapisargument, which defaults to mocking all timer functions (includingsetTimeoutandclearTimeout).Consider applying this change here and in the other tests (lines 83, 117, 135) that use
t.mock.timers.enable({ apis: ['setTimeout'] }).♻️ Proposed fix
- t.mock.timers.enable({ apis: ['setTimeout'] }) + t.mock.timers.enable()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/utils/wait-for-element-to-exist-and-select.test.mjs` around lines 54 - 55, Update the mock timer setup in the waitForElementToExistAndSelect tests, including the cases around lines 54, 83, 117, and 135, to enable mock timers without restricting the apis argument. This must mock both setTimeout and clearTimeout so the utility’s timer cleanup is tracked correctly.src/content-script/site-adapters/brave/index.mjs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnsure consistent import paths with explicit file extensions. These imports lack explicit
.mjsextensions, which can cause module resolution issues in native ESM environments and creates inconsistency across the adapters.
src/content-script/site-adapters/brave/index.mjs#L1-L1: Append/index.mjsto the../../../utilsimport path.src/content-script/site-adapters/duckduckgo/index.mjs#L2-L2: Append.mjsto the../indeximport path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/content-script/site-adapters/brave/index.mjs` at line 1, Update the import in src/content-script/site-adapters/brave/index.mjs at lines 1-1 to reference ../../../utils/index.mjs, and update the import in src/content-script/site-adapters/duckduckgo/index.mjs at lines 2-2 to append the .mjs extension to ../index; preserve the imported symbols and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/content-script/site-adapters/brave/index.mjs`:
- Line 1: Update the import in src/content-script/site-adapters/brave/index.mjs
at lines 1-1 to reference ../../../utils/index.mjs, and update the import in
src/content-script/site-adapters/duckduckgo/index.mjs at lines 2-2 to append the
.mjs extension to ../index; preserve the imported symbols and behavior.
In `@tests/unit/utils/wait-for-element-to-exist-and-select.test.mjs`:
- Around line 54-55: Update the mock timer setup in the
waitForElementToExistAndSelect tests, including the cases around lines 54, 83,
117, and 135, to enable mock timers without restricting the apis argument. This
must mock both setTimeout and clearTimeout so the utility’s timer cleanup is
tracked correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15407d6a-a7ea-4d41-86dd-5236fa8dbf47
📒 Files selected for processing (4)
src/content-script/site-adapters/brave/index.mjssrc/content-script/site-adapters/duckduckgo/index.mjssrc/utils/wait-for-element-to-exist-and-select.mjstests/unit/utils/wait-for-element-to-exist-and-select.test.mjs
Give Brave and DuckDuckGo result containers five seconds to appear instead of five milliseconds. Centralize that policy, clean up the pending timer after a match, and cover observable timing behavior.
f982427 to
b8e84b9
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes an unintended 5ms timeout in site-adapter container waits by introducing a centralized 5-second “site adapter wait” helper and updating Brave/DuckDuckGo adapters to use it. It also improves the underlying wait helper’s cleanup behavior and adds deterministic unit tests to prevent regressions.
Changes:
- Refactor
waitForElementToExistAndSelectto reuse the initially selected element, add a sharedfinish()path, and clear a pending timeout when a mutation match resolves. - Add
waitForSiteAdapterElement()that standardizes the site-adapter container wait to 5 seconds. - Update Brave and DuckDuckGo adapters to use the new helper (including Brave’s compound-selector fallback behavior) and add unit tests covering timing, fallback selectors, and timeout cleanup.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/unit/utils/wait-for-element-to-exist-and-select.test.mjs | Adds deterministic unit coverage for immediate resolution, mutation-based resolution, timeout behavior, timeout cleanup, and the 5-second site-adapter policy. |
| src/utils/wait-for-element-to-exist-and-select.mjs | Centralizes resolve/cleanup logic and introduces waitForSiteAdapterElement() with a fixed 5-second timeout. |
| src/content-script/site-adapters/duckduckgo/index.mjs | Switches DuckDuckGo initialization wait to the standardized 5-second site-adapter helper. |
| src/content-script/site-adapters/brave/index.mjs | Switches Brave initialization wait to the standardized helper and uses a compound selector to allow sidebar/results fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Code review by qodo was updated up to the latest commit b8e84b9 |
Summary
Why
The adapters passed
5directly to a helper that usessetTimeout, so their intended rendering wait expired after only 5 ms. This change narrows the fix to the initial container wait and does not change Bilibili's no-timeout behavior.Validation
npm test(46 test files)npm run test:coverageon Node.js 22 (36.26% lines)npm run lintnpm run buildgit diff --check