You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Adds a new ESLint rule ddg-local/no-blocking-init-request that prevents features from blocking their init() method on a request/response round trip to the client.
The rule rejects patterns like:
await this.request(...) in init()
await this.messaging.request(...) in init()
Returning a request promise from init()
This is important because init() is awaited by the shared feature init chain, so blocking on a client round trip delays all features' initialization and leaves the feature's ready state unresolved forever on platforms with no handler for the message.
Changes
New rule: scripts/eslint-rules/no-blocking-init-request.js — detects blocking requests in feature init() methods by walking the AST without descending into nested functions (so callbacks registered by init() don't trigger false positives)
Rule tests: scripts/eslint-rules/no-blocking-init-request.test.js — comprehensive test cases covering valid patterns (config gating, fire-and-forget, listeners) and invalid patterns (awaited requests, returned promises, wrapped in try/catch, etc.)
Rule registry: scripts/eslint-rules/index.js — exports the rule as part of the ddg-local plugin
ESLint config: Updated eslint.config.js to register the plugin and enable the rule for injected/src/**/*.js
Documentation:
scripts/eslint-rules/README.md — explains the rule, its rationale, and how to add new rules
injected/docs/features-guide.md — added "Red flags in init" section with detailed guidance on alternatives (remote config, userPreferences, subscribe, fire-and-forget)
injected/docs/coding-guidelines.md — added section on never blocking init() on a request
AGENTS.md and injected/AGENTS.md — added messaging constraints note
Existing code: Added eslint-disable comment to injected/src/features/click-to-load.js for a pre-existing case that cannot be refactored
CI: Added npm run test-eslint-rules to the test workflow
Package.json: Added test-eslint-rules script
Testing Steps
Run npm run test-eslint-rules to verify all rule test cases pass
Run npm run lint to verify the rule is enforced on injected features
Verify that click-to-load.js passes linting with its documented exception
Checklist
I have tested this change locally
I have added automated tests that cover this change
Low Risk
Changes are lint enforcement, docs, and CI workflow resilience; runtime injected behavior is unchanged aside from documented exceptions in two legacy features.
Overview
Adds a local ESLint ruleddg-local/no-blocking-init-request that flags injected features whose load() or init()block on a client messaging round trip (await this.request(...), this.messaging.request, or calls on *Messages wrappers built from this.messaging). The rule is wired through scripts/eslint-rules/, enabled for injected/src/**/*.js, and covered by RuleTester cases plus npm run test-eslint-rules in CI.
Documentation expands the feature lifecycle guidance: red flags in load / init, alternatives (remote config, userPreferences, subscribe, void this.setupFromClient()), and pointers from root and injected/AGENTS.md.
Pre-existing violations in click-to-load.js and duck-player-native.js are left in place with documented eslint-disable comments. The review-check workflow adds API retries and wraps commit status updates so transient GitHub API failures do not mask review validation results.
Reviewed by Cursor Bugbot for commit 1b726f1. Bugbot is set up for automated code reviews on this repo. Configure here.
A feature that gates `init()` on `await this.request(...)` blocks the
shared init chain on a client round trip, and never resolves on a
platform with no handler for the message. Enablement gating like this
belongs in Privacy Remote Configuration or `userPreferences`, both of
which arrive with the injected args.
- Add the `ddg-local` local ESLint plugin with a
`no-blocking-init-request` rule over `injected/src/**/*.js`. It reports
awaited/returned `this.request()` and `this.messaging.request()` calls
in `init()`'s own scope, leaving requests made from callbacks alone.
- Cover the rule with RuleTester cases, run via `npm run test-eslint-rules`
in the existing lint CI job.
- Document the anti-pattern and its alternatives as a red flag in the
features guide, coding guidelines, and both AGENTS.md files.
- Annotate the one pre-existing case in `click-to-load` with a scoped
eslint-disable explaining why it stays.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LKvRn94NRGRzk3xL5iZvN4
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Web Compatibility Assessment
File
Severity
Finding
scripts/eslint-rules/no-blocking-init-request.js
info
Static-analysis-only rule; no runtime injected behavior changes. Correctly scopes checks to init()'s own scope (callbacks/listeners excluded), matching the init-chain timing model in content-scope-features.js.
Pre-existing blocking await this.messaging.request('getClickToLoadState') in init() remains, now documented with an eslint-disable. This still delays the shared init chain and can hang on platforms without a handler; acceptable only because every CTL-bundling platform implements the handler.
The rule permits init() { this.setup(); } + async setup() { await this.request(...); }, which still blocks callInit() if setup() is not awaited but fires an un-awaited async function — actually wait, if setup is async and not awaited, init returns immediately. The test case shows init doesn't await setup, so init returns immediately while setup runs in background. That's actually fine for non-blocking. Good.
injected/src/features/duck-player-native.js (pre-existing, not in diff)
warning
async init() awaits messages.initialSetup() (wraps this.messaging.request). The rule does not flag this because the callee is a local helper, not this.request / this.messaging.request. Existing init-chain delay remains unenforced.
Positive: documents that blocking init() on client round-trips delays all features and the queued update() drain — aligns with Timing/Race Conditions guidance.
No API overrides, DOM patches, prototype changes, or wrapper-utility modifications in this diff.
Security Assessment
File
Severity
Finding
Entire diff
info
No changes to captured-globals.js, messaging transports, message-bridge trust boundaries, origin validation, or postMessage usage.
Docs / rule messaging
info
Reinforces gating enablement via remote config (getFeatureSettingEnabled) and userPreferences rather than per-page request() round-trips — improves Configuration Trust posture for future features.
eslint.config.js
info
Rule scoped to injected/src/**/*.js only; no effect on special-pages or build output.
No security vulnerabilities introduced.
Risk Level
Low Risk — tooling, documentation, and CI only; the sole production-code touch is a comment + eslint-disable on a pre-existing click-to-load exception.
Recommendations
(warning) Extend the rule (or add a follow-up) to catch messaging-wrapper indirection such as await messages.initialSetup() when messages is constructed from this.messaging in init() — duck-player-native.js is a live example the rule misses today.
(info) Consider flagging computed-member calls (this['request'](...)) for parity, though unlikely in practice.
(info) When refactoring click-to-load / duck-player-native, prefer the documented fire-and-forget pattern (void helper() or .then()/.catch()) so callInit() resolves immediately.
(info) Rule test coverage is solid (await, return, expression-bodied arrow, Promise.all, .catch() chain, this.messaging.request). Keep adding cases if the rule is extended.
`load()` fails differently from `init()`: `callLoad()` never awaits it, so
an `await` doesn't stall the load loop, it splits the method in two. The
hooks `load()` exists to install early land in a later task, by which
point the page may already have used the API being wrapped, and the
rejection is unhandled because nobody holds the promise. `load()` also
runs before remote-config exceptions apply, so the message goes out on
sites the feature never inits on.
- Report `load()` under its own `blockingLoad` message explaining that
failure mode, rather than reusing the init wording.
- Cover load in the rule tests. No existing `load()` is async, so nothing
needed an allowlist.
- Document it as "Red flags in `load`" under the load lifecycle section,
leaving the existing init anchor intact.
- Correct the recommended escape hatch: the previous `.then()` example
would have tripped the repo's own `promise/prefer-await-to-then`. Use
`void this.someAsyncSetup()` with the await in a separate method, which
is the established idiom here and lints clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LKvRn94NRGRzk3xL5iZvN4
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Injected PR Evaluation: Web Compatibility & Security
Re-assessed on synchronize (3ad8e5914 — extends rule to load()). Delta vs open review (f7c9e03): load() coverage, docs, and tests; runtime behavior unchanged except click-to-load comment/eslint-disable documentation.
Web Compatibility Assessment
File
Lines
Severity
Finding
injected/docs/features-guide.md
59–78
info
Positive: New "Red flags in load" section correctly documents the callLoad() non-await race — wrappers installed after await can land too late for early API reads (Timing & Race Conditions §4).
scripts/eslint-rules/no-blocking-init-request.js
127–148
info
Rule now flags load() with distinct blockingLoad message; aligns lint enforcement with the documented lifecycle hazard.
injected/src/features/click-to-load.js
1902–1907
info
Pre-existing blocking init() on getClickToLoadState; now documented with eslint-disable and explicit "do not copy" guidance. Not introduced by this PR.
injected/src/features/duck-player-native.js
64–65
warning
Rule gap (unchanged):await messages.initialSetup() blocks init() on a client round trip (messages.js wraps this.messaging.request('initialSetup')) but is not flagged — callee root is local messages, not this, and initialSetup is not in default methodNames.
injected/src/features/duckplayer/overlays.js
27
warning
Same gap: await messages.initialSetup() in overlay init path; not in load()/init() class methods so outside rule scope entirely.
Security Assessment
File
Lines
Severity
Finding
—
—
—
No security findings. Tooling/docs/CI only; no changes to messaging transports, message bridge, captured globals, or runtime security checks.
injected/src/features/click-to-load.js
1907
info
Pre-existing init() blocking on client state fetch delays shared init chain; operational concern, not a new exploit vector.
Risk Level
Low Risk — ESLint rule, documentation, and CI only; no new API overrides, messaging changes, or injected runtime behavior beyond documenting a pre-existing click-to-load exception.
Recommendations
warning — Close the initialSetup gap: Either add initialSetup to default methodNames in eslint.config.js, or broaden isClientRequestCall to flag any await on a method that transitively calls messaging.request (at minimum, also match local variables assigned from new *Messages(this.messaging, …)). duck-player-native.js line 65 is the concrete missed case.
info — Follow-up PR for legacy violations: Track click-to-load and duck-player-native blocking-init patterns separately; this PR correctly documents CTL as a one-off exception.
info — load() tests are solid: New blockingLoad cases in no-blocking-init-request.test.js cover await, return, and dual-phase violations — good guard against early-wrapper race regressions.
Two issues raised on the PR:
- `return await this.request(…)` was reached twice, once via the
ReturnStatement and once via the AwaitExpression, producing two
identical diagnostics at one location. Report each call once.
- A round trip routed through a feature's `*Messages` class was invisible
to the rule, because the callee root is a local variable rather than
`this`. Resolve that variable through scope and treat any awaited method
on an object built from `this.messaging` as a client round trip. This
matches on the initialiser, not on names, so an unrelated
`await someClient.request(…)` is still fine.
Across injected/src and messaging this flags exactly one further real
case, `duck-player-native`, which now carries a documented disable
alongside the click-to-load one. The other duck-player path suggested in
review, `initOverlays` in duck-player.js, is called with `void` and does
not block init, so it is correctly left alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LKvRn94NRGRzk3xL5iZvN4
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Web Compatibility & Security Assessment (synchronize → 345ae70b5)
Re-assessed after the messaging-wrapper detection fix. No runtime injected behavior changes beyond documenting pre-existing eslint-disable exceptions.
Web Compatibility Assessment
File
Lines
Severity
Finding
injected/src/features/click-to-load.js
1906–1907
warning
Pre-existing: await this.messaging.request('getClickToLoadState') in init() blocks the shared init chain (timing/race — §4 load() vs init() ordering). Documented exception; do not copy into new features.
injected/src/features/duck-player-native.js
69–70
warning
Pre-existing: await messages.initialSetup() blocks init() on a client round trip. Same init-chain delay pattern. Now correctly flagged by the rule; eslint-disable documents the exception.
injected/src/features/duck-player.js
106
info
Good reference pattern: void initOverlays(...) keeps init() synchronous while initOverlays awaits client state off the init path.
scripts/eslint-rules/no-blocking-init-request.js
127–145
info
load() coverage (blockingLoad) closes the early-wrapper race where callLoad() does not await. Positive for API-surface fidelity on future features.
scripts/eslint-rules/no-blocking-init-request.js
132
warning
Residual gap: isMessagingWrapperCall only traces Identifier roots, so await this.messages.initialSetup() (getter-backed) still bypasses default config. Local const messages = … wrappers are now caught.
No API overrides, prototype patches, DOM mutations, or platform entry-point changes in this delta.
Security Assessment
File
Lines
Severity
Finding
—
—
info
No changes to captured-globals.js, message-bridge trust boundaries, origin validation, or messaging transports. Security impact is indirect: docs + lint steer features toward remote-config gating instead of client round trips.
The `integration` job builds the docs with `treatWarningsAsErrors`, and
typedoc processes `injected/docs/*.md`, so two things in the new docs
broke it:
- A link to the `scripts/eslint-rules/` directory. typedoc treats
relative links as media to copy and warns on a directory; point at the
README file instead.
- Anchors into the "Red flags in ..." headings. typedoc's slugifier drops
inline code from a heading, so `` Red flags in `load` `` produced
`#red-flags-in` rather than GitHub's `#red-flags-in-load`, and the two
headings collided. Drop the backticks so both slugifiers agree; the
existing anchors in the docs, the rule README and the two
eslint-disable comments keep resolving.
`npm run docs` now completes with no warnings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LKvRn94NRGRzk3xL5iZvN4
Follow-up from review: the wrapper detection only resolved locals, so a
wrapper reached through `this` - `get messages() { return new
FeatureMessages(this.messaging) }`, or the equivalent field - still
slipped past under the default options. Resolve that case through the
surrounding class body, the same way locals go through scope, so it is
caught structurally rather than by adding method names to the defaults.
The class body comes from the lifecycle definition's own `parent`, which
ESLint has already set; descendants are not yet traversed at that point,
so their `parent` cannot be used.
Nothing new is flagged across injected, messaging and special-pages.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LKvRn94NRGRzk3xL5iZvN4
Today's GitHub instability took this job down in a way that hid its own
diagnosis: `findAuthorizedApproval` failed, and the catch handler's
`setReviewStatus` call then got a 503 from the statuses endpoint, so the
job died with "Unhandled error: HttpError" instead of the message it had
prepared. The status POST is best-effort reporting, so a failure there
now warns rather than throwing over the top of the real error.
Also enable octokit retries on this step (previously `retries: 0`), so a
one-off 5xx no longer fails the gate. The retry-exempt 4xx list is
unchanged, so genuine auth and validation errors still fail fast.
This does not change when the gate passes or fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LKvRn94NRGRzk3xL5iZvN4
The reason will be displayed to describe this comment to others. Learn more.
Injected PR Evaluation: Web Compatibility & Security
Re-assessed on synchronize (1b726f125, merge of main + messaging-wrapper class-member detection + review-check hardening).
Web Compatibility Assessment
File
Lines
Severity
Finding
injected/src/features/click-to-load.js
~1902–1907
warning
Pre-existing: await this.messaging.request('getClickToLoadState') in init() blocks the shared init chain until native responds. Documented eslint-disable added; behavior unchanged. Delays all feature init and queued update() drain (Timing §4).
injected/src/features/duck-player-native.js
~65–70
warning
Pre-existing: await messages.initialSetup() blocks init on a messaging-wrapper round trip. Now correctly caught by the ESLint rule's isMessagingWrapperCall path; exempted with rationale.
scripts/eslint-rules/no-blocking-init-request.js
entire rule
info
Positive: prevents new features from gating load()/init() on client round trips — the primary cause of late API wrapping and init-chain stalls. Covers this.request(), this.messaging.request(), local *Messages wrappers, and class-member getters/fields.
.then() chains in init() are allowed because they don't block the lifecycle method (init returns immediately). promise/prefer-await-to-then discourages the pattern separately; functionally non-blocking.
Fire-and-forget this.setup() (async helper, no void) is allowed — same non-blocking semantics as the documented void this.setupFromClient() idiom.
injected/docs/features-guide.md
new sections
info
Documentation-only; accurately describes load() vs init() timing constraints. No runtime impact.
No API overrides, prototype patches, DOM manipulation changes, or platform entry-point modifications in this PR.
Security Assessment
File
Lines
Severity
Finding
injected/src/features/click-to-load.js
~1907
info
Pre-existing blocking request() in init: on platforms without a handler, init chain hangs indefinitely (Config Trust / Timing). Not introduced by this PR.
injected/docs/coding-guidelines.md
new section
info
Reinforces nativeData must never appear in outgoing messages — positive security documentation.
scripts/eslint-rules/*
—
info
Dev-time lint only; no production bundle impact. No messaging transport, bridge, or origin-validation changes.
.github/workflows/review-check.yml
~37–57
info
CI resilience (API retries, try/catch around status reporting). No injected-runtime security surface.
No changes to captured-globals.js, message-bridge trust boundaries, postMessage usage, or config-gating logic.
Risk Level
Low Risk — This PR adds an ESLint rule, tests, and documentation to prevent a known lifecycle anti-pattern; runtime changes are limited to eslint-disable comments on two pre-existing exceptions and CI workflow hardening.
Recommendations
(info) The ESLint rule and its isMessagingWrapperCall / isMessagingMember detection (commit 4dc446516) correctly closes the gap where await this.messages.initialSetup() would bypass a methodNames: ['request']-only check. Good coverage.
(warning, follow-up) Consider refactoring click-to-load and duck-player-native to the void this.setupFromClient() pattern when feasible, so the documented exceptions can be removed.
(info)npm run test-eslint-rules wired into CI (tests.yml) — ensures rule regressions are caught. Verified passing locally (30/30).
(info) Incidental merge from main brought in text-selection (#2953); out of scope for this PR's authored changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
semver-patchBug fix / internal — no release needed
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Description
Adds a new ESLint rule
ddg-local/no-blocking-init-requestthat prevents features from blocking theirinit()method on a request/response round trip to the client.The rule rejects patterns like:
await this.request(...)ininit()await this.messaging.request(...)ininit()init()This is important because
init()is awaited by the shared feature init chain, so blocking on a client round trip delays all features' initialization and leaves the feature'sreadystate unresolved forever on platforms with no handler for the message.Changes
scripts/eslint-rules/no-blocking-init-request.js— detects blocking requests in featureinit()methods by walking the AST without descending into nested functions (so callbacks registered byinit()don't trigger false positives)scripts/eslint-rules/no-blocking-init-request.test.js— comprehensive test cases covering valid patterns (config gating, fire-and-forget, listeners) and invalid patterns (awaited requests, returned promises, wrapped in try/catch, etc.)scripts/eslint-rules/index.js— exports the rule as part of theddg-localplugineslint.config.jsto register the plugin and enable the rule forinjected/src/**/*.jsscripts/eslint-rules/README.md— explains the rule, its rationale, and how to add new rulesinjected/docs/features-guide.md— added "Red flags ininit" section with detailed guidance on alternatives (remote config, userPreferences, subscribe, fire-and-forget)injected/docs/coding-guidelines.md— added section on never blockinginit()on a requestAGENTS.mdandinjected/AGENTS.md— added messaging constraints noteeslint-disablecomment toinjected/src/features/click-to-load.jsfor a pre-existing case that cannot be refactorednpm run test-eslint-rulesto the test workflowtest-eslint-rulesscriptTesting Steps
npm run test-eslint-rulesto verify all rule test cases passnpm run lintto verify the rule is enforced on injected featuresclick-to-load.jspasses linting with its documented exceptionChecklist
https://claude.ai/code/session_01LKvRn94NRGRzk3xL5iZvN4
Note
Low Risk
Changes are lint enforcement, docs, and CI workflow resilience; runtime injected behavior is unchanged aside from documented exceptions in two legacy features.
Overview
Adds a local ESLint rule
ddg-local/no-blocking-init-requestthat flags injected features whoseload()orinit()block on a client messaging round trip (await this.request(...),this.messaging.request, or calls on*Messageswrappers built fromthis.messaging). The rule is wired throughscripts/eslint-rules/, enabled forinjected/src/**/*.js, and covered by RuleTester cases plusnpm run test-eslint-rulesin CI.Documentation expands the feature lifecycle guidance: red flags in
load/init, alternatives (remote config,userPreferences,subscribe,void this.setupFromClient()), and pointers from root andinjected/AGENTS.md.Pre-existing violations in
click-to-load.jsandduck-player-native.jsare left in place with documentedeslint-disablecomments. The review-check workflow adds API retries and wraps commit status updates so transient GitHub API failures do not mask review validation results.Reviewed by Cursor Bugbot for commit 1b726f1. Bugbot is set up for automated code reviews on this repo. Configure here.