Create Unified Web Requests Module #51
Conversation
WalkthroughA new URL pattern-matching utility and an advanced web request handling abstraction are introduced, enhancing session and web request management. Content blocking now utilizes the improved session wrapper. Logging is refined to separate colored console output from decolored log file entries. Debugging areas are expanded with new categories. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant UnifiedWebRequest
participant WebRequest
participant Listener
Caller->>UnifiedWebRequest: onBeforeRequest(filter, listener, id)
UnifiedWebRequest->>WebRequest: Registers internal handler
WebRequest-->>UnifiedWebRequest: Emits event
UnifiedWebRequest->>Listener: Calls all matching listeners
Listener-->>UnifiedWebRequest: Returns (possibly async) result
UnifiedWebRequest-->>WebRequest: Aggregates and applies results
sequenceDiagram
participant ContentBlocker
participant Session
participant createBetterSession
participant Blocker
ContentBlocker->>createBetterSession: Wrap session with key
createBetterSession-->>ContentBlocker: Returns enhanced session
ContentBlocker->>Blocker: enableBlockingInSession(enhanced session)
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/main/browser/utility/match-pattern.ts (2)
59-69: Consider optimizing regex pattern creation for frequently used patterns.The path matching logic converts the path pattern to a regex string and creates a new RegExp object for each match operation. If this function is called frequently with the same patterns, consider caching the compiled regex patterns to improve performance.
// Add at the top of the file: +const pathPatternCache = new Map<string, RegExp>(); // Then modify the path regex creation: -const pathRegexStr = pathPattern.replace(/\*/g, ".*").replace(/\?/g, "\\?"); -const pathRegex = new RegExp(`^${pathRegexStr}$`); +// Get or create the path regex +let pathRegex = pathPatternCache.get(pathPattern); +if (!pathRegex) { + const pathRegexStr = pathPattern.replace(/\*/g, ".*").replace(/\?/g, "\\?"); + pathRegex = new RegExp(`^${pathRegexStr}$`); + pathPatternCache.set(pathPattern, pathRegex); +}
70-73: Appropriate error handling with debug logging.The function properly catches any errors that might occur during URL parsing or pattern matching and logs them to the debug output. Using
WEB_REQUESTSas the debug area here is fine, but consider usingMATCH_PATTERNfor consistency with the rest of the file.-debugPrint("WEB_REQUESTS", `Error matching URL pattern: ${error}`); +debugPrint("MATCH_PATTERN", `Error matching URL pattern: ${error}`);src/main/browser/utility/web-requests.ts (1)
658-667: Cache implemented with a strongMap→ memory leak risk
betterWebRequestCachekeys are plain strings that reference the ID + underlying objectId.
Once an enhanced instance is created there is no path for GC even if the originalWebRequestis long-gone.Switch to a two-level
WeakMapkeyed by the originalWebRequest(orSession) so the cache is eligible for collection automatically:-// Cache for betterWebRequest instances -const betterWebRequestCache = new Map<string, WebRequest>(); +// GC-friendly cache +const betterWebRequestCache = new WeakMap<WebRequest, Map<string, WebRequest>>();(You will need small adjustments when storing/retrieving but the change prevents unbounded growth during long-running app sessions.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
src/main/browser/utility/match-pattern.ts(1 hunks)src/main/browser/utility/web-requests.ts(1 hunks)src/main/modules/content-blocker.ts(4 hunks)src/main/modules/logs.ts(1 hunks)src/main/modules/output.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/modules/content-blocker.ts (1)
src/main/browser/utility/web-requests.ts (1)
createBetterSession(771-809)
src/main/browser/utility/match-pattern.ts (1)
src/main/modules/output.ts (1)
debugPrint(28-34)
🪛 Biome (1.9.4)
src/main/modules/logs.ts
[error] 42-42: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (12)
src/main/modules/output.ts (1)
20-23: Good addition of debug areas for the new web request features.The new debug areas clearly map to the new functionalities being added in this PR. I notice that
WEB_REQUESTSis set totrueby default, while others arefalse, which makes sense to provide visibility into the core web request operations while keeping the more detailed debug areas disabled by default.src/main/modules/logs.ts (2)
38-45: Good improvement to log file handling.This change properly separates the console output from log file content by creating a separate
decoloredChunkvariable. Now the console output retains the original ANSI color codes, while the log file receives the cleaned version without these codes.🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
42-42: The control character use in regex is intentional and properly commented.The static analysis tool flags the control character in the regex, but this is expected since we're specifically detecting ANSI escape codes. The eslint-disable comment properly documents this intentional use.
🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
src/main/modules/content-blocker.ts (4)
1-1: Good integration with the new unified web requests module.The import of
createBetterSessionfrom the new web requests utility is appropriate for the goal of this PR.
10-10: Clear and descriptive session key.Using a constant with a descriptive name
SESSION_KEYmakes the code more maintainable and helps identify which component is using the session wrapper.
63-63: Good implementation of enhanced session handling.Wrapping the session with
createBetterSessionwhen disabling the blocker ensures that the correct web request handler is accessed.
85-85: Consistent use of enhanced sessions.The implementation consistently uses the enhanced session wrapper for both enabling and disabling the blocker, which ensures proper web request handling.
src/main/browser/utility/match-pattern.ts (5)
3-10: Good documentation of the match pattern format.The documentation clearly explains the format and includes a reference to the Mozilla documentation, which is helpful for maintainers.
13-21: Comprehensive handling of the special<all_urls>pattern.The implementation correctly handles the special pattern and checks for supported schemes. The try-catch block properly handles potential URL parsing errors.
24-30: Well-structured pattern validation.The regex pattern for validating match patterns is comprehensive and handles the various components correctly. The debug print for invalid patterns is helpful for troubleshooting.
35-44: Thorough scheme matching logic.The scheme matching logic correctly handles both wildcard schemes and specific schemes, with appropriate protocol normalization.
46-57: Comprehensive host matching implementation.The host matching correctly handles wildcards, specific hostnames, and subdomain patterns with the
*.syntax. The check on line 52 ensures that the hostname isn't exactly the suffix, which is the correct behavior for subdomain matching.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/browser/utility/web-requests.ts (3)
685-770:betterWebRequestdrops any future WebRequest membersThis manual wrapper lists only the known event methods. If Electron adds a new
method in a minor release, client code will suddenly receiveundefined. A
proxy that forwards unknown properties tounifiedWebRequest(or the original
webRequest) future-proofs the API without maintenance overhead — exactly the
approach already used for the session proxy below.This mirrors a previous remark; consider it a friendly reminder.
190-197:⚠️ Potential issueRequest-header mutation discards multi-value headers and prior edits
The new
newRequestHeadersobject is rebuilt from scratch and forces every
value to a single string (Array.isArray(value) ? value[0] : value).
This:
- Loses legitimate multi-value headers such as
Set-CookieorAccept.- Obliterates modifications done by earlier listeners in the chain.
Consider merging instead of replacing and keep the full union
string | string[]type:- const newRequestHeaders: Record<string, string> = {}; - for (const [key, value] of Object.entries(response.requestHeaders)) { - newRequestHeaders[key] = Array.isArray(value) ? value[0] : value; - } - - currentRequestHeaders = newRequestHeaders; + currentRequestHeaders = { + ...currentRequestHeaders, + ...(response.requestHeaders ?? {}) + };Do the same for response headers below.
122-156: 🛠️ Refactor suggestionAdd a timeout (or fail-safe) to prevent requests from hanging indefinitely
Promise.all(promises)waits forever if any listener forgets/decides not to
call itsfakeCallback. In that situation the network request is never
released and the tab can freeze.A minimal mitigation is to race
Promise.allagainst a short timeout
(e.g. 200-300 ms, matching Electron’s own defaults) and fall back to
callback({})when the timeout fires. Re-use the same helper for
onBeforeSendHeadersandonHeadersReceived, which have identical risk.- Promise.all(promises).then((responses) => { + const TIMEOUT_MS = 250; + Promise.race([ + Promise.all(promises), + new Promise<CallbackResponse[]>(res => + setTimeout(() => res([]), TIMEOUT_MS) + ) + ]).then((responses) => {This removes the single-point-of-failure while still honouring fast listeners.
🧹 Nitpick comments (1)
src/main/browser/utility/web-requests.ts (1)
656-669: Caches grow unbounded – potential memory leak
betterWebRequestCache/betterSessionCacheare ordinaryMaps keyed by
(composite) IDs and are never cleaned up. Long-lived apps that create many
temporary sessions (e.g. incognito windows) will keep every wrapper alive.If eviction by design is hard, at least switch to
WeakMapkeyed by the
original object so wrappers are GC-eligible once the underlying
WebRequest/Sessionis gone.-const betterWebRequestCache = new Map<string, WebRequest>(); +const betterWebRequestCache = new WeakMap<object, WebRequest>();…and generate the cache key from the wrapper target instead of a string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/main/browser/utility/web-requests.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
tldr
This resolves the problem where webRequests intercept handlers replace each other.
Summary by CodeRabbit