Merged
Conversation
…cific headers in user agent transformation
Contributor
Build artifacts for all platforms are ready! 🚀Download the artifacts for: One-line installer (Unstable):bunx flow-debug-build@1.2.1 --open 24008846717(execution 24008846717 / attempt 1) |
Contributor
WalkthroughRefactors extension handling and user-agent interception: updates dependencies, delegates CRX handling to the electron-chrome-extensions library, adds extension asset loading and service-worker startup helpers, changes user-agent transformer to return browser-header flags, and adjusts tab/favicons flows to use new extension APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant LoadedProfiles as LoadedProfilesController
participant ExtensionsMgr as ExtensionManager
participant ExtModule as extensions module
participant Session as Electron.Session
participant FS as Filesystem
LoadedProfiles->>ExtensionsMgr: persist installed extension
ExtensionsMgr->>ExtModule: initializeLoadedExtension(extension)
ExtModule->>Session: session.extensions.getExtension(id)
ExtModule->>Session: startExtensionServiceWorker(scope) (startSWPromise)
note right of Session: attempts to start service worker\n(retries internally)
ExtModule->>FS: resolve & read manifest/icons when needed (getExtensionAsset)
ExtModule-->>ExtensionsMgr: return { startSWPromise }
ExtensionsMgr-->>LoadedProfiles: dispatch runtime "installed" event after startSWPromise resolves
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR improves extension support in the Flow browser by upgrading Key changes:
Issues found:
Confidence Score: 4/5Mostly safe to merge; one P1 issue (unguarded throw in Chrome validation header generation) could break Chrome Web Store access on non-standard platforms or after aggressive UA stripping. The P1 finding in chrome.ts means a missing OS match in the user agent throws synchronously inside a web-request interceptor, which would prevent the Chrome Web Store from loading rather than gracefully degrading. All other findings are P2 style or cleanup items that do not block merge. src/main/modules/chrome.ts (unguarded throw) and src/main/controllers/sessions-controller/intercept-rules/user-agent-transformer.ts (duplicate header). Important Files Changed
Sequence DiagramsequenceDiagram
participant CWS as Chrome Web Store
participant UAT as UserAgentTransformer
participant Chrome as chrome.ts
participant ECE as ElectronChromeExtensions
participant SW as ServiceWorkers
participant ECW as electron-chrome-web-store
Note over ECE: App ready
ECE->>ECE: handleCRXProtocol(defaultSession)
CWS->>UAT: onBeforeSendHeaders (chromewebstore.google.com)
UAT->>Chrome: generateChromeValidationHeader(ua)
Chrome-->>UAT: x-browser-validation hash
UAT-->>CWS: inject x-browser-* headers
ECW->>ECW: afterInstall(details)
ECW->>SW: startExtensionServiceWorker() [retry up to 600x]
SW-->>ECW: success
ECW->>ECE: dispatchRuntimeInstalled(extensionId, install)
Reviews (1): Last reviewed commit: "chore: bump extension packages" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/controllers/loaded-profiles-controller/index.ts (1)
186-196: 🧹 Nitpick | 🔵 TrivialPrefer
const tab = await ...over mixingawaitwith.then().The current pattern works but is less idiomatic. Consider separating the await and the callback for clarity.
Suggested refactor
for (const url of urls) { const currentTabIndex = tabIndex; - await tabsController.createTab(window.id, profileId, undefined, undefined, { url }).then((tab) => { - if (currentTabIndex === 0) { - tabsController.activateTab(tab); - } - }); + const tab = await tabsController.createTab(window.id, profileId, undefined, undefined, { url }); + if (currentTabIndex === 0) { + tabsController.activateTab(tab); + } tabIndex++; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/controllers/loaded-profiles-controller/index.ts` around lines 186 - 196, Replace the mixed await/.then pattern in the loop by awaiting the promise directly: call tabsController.createTab(...) with await into a const (e.g., const tab = await tabsController.createTab(window.id, profileId, undefined, undefined, { url })), then run the activation logic (if (currentTabIndex === 0) tabsController.activateTab(tab)); keep the currentTabIndex const and tabIndex++ logic intact and remove the .then() callback to improve clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/controllers/sessions-controller/intercept-rules/user-agent-transformer.ts`:
- Around line 36-47: The x-browser-validation header is being assigned twice:
once when spreading the result of generateBrowserClientHeaders(newValue) into
newHeaders and again explicitly via generateChromeValidationHeader(newValue);
remove the redundant explicit assignment. In the block gated by
includeChromeBrowserHeaders, keep the loop that merges
generateBrowserClientHeaders(newValue) into newHeaders and delete the separate
call that computes and sets newHeaders["x-browser-validation"] =
validationHeader (you may also remove the const validationHeader assignment).
Ensure updated = true remains.
In `@src/main/modules/chrome.ts`:
- Around line 18-29: The code in chrome.ts that maps userAgent -> apiKey (using
PLATFORM_API_KEYS and the local apiKey variable) must not throw on unknown UAs;
change the branch that currently throws ("Unknown OS in user agent. Supply
apiKey manually.") to instead return a null/undefined apiKey (or simply leave
apiKey unset) so header generation is best-effort. Update
setupUserAgentTransformer() callers (specifically the user-agent transformer in
sessions-controller/intercept-rules/user-agent-transformer.ts) to only set the
x-browser-validation header when the transformer returns a non-null value.
Ensure no exception is thrown from the request-header generation path and that
x-browser-validation is added conditionally.
In `@src/main/modules/extensions/assets.ts`:
- Around line 71-78: matchesWildcardPattern can exhibit ReDoS risks with many
wildcards; update it to mitigate by collapsing consecutive '*' into a single '*'
(normalize pattern), enforce a safe maximum pattern length and a max number of
wildcards (e.g., reject or return false when limits exceeded), then call
escapePattern and build the RegExp as before; reference matchesWildcardPattern
and escapePattern when locating the code and add configurable constants like
MAX_PATTERN_LENGTH and MAX_WILDCARDS to centralize limits.
In `@src/main/modules/extensions/helpers.ts`:
- Around line 7-12: The scope string for starting the service worker is missing
a trailing slash so startWorkerForScope() won't match; update the scope
construction (where scope is defined from extensionId in the block that checks
extension.manifest.manifest_version === 3 &&
extension.manifest.background?.service_worker) to include a trailing slash
(i.e., chrome-extension://${extensionId}/) before calling
session.serviceWorkers.startWorkerForScope(scope) so the retry loop can
successfully find the scope.
In `@src/main/modules/extensions/locales.ts`:
- Around line 115-121: The function translateManifestString currently returns a
plain string for non-__MSG_ values and a Promise<string> when using
transformStringToLocale, producing a string | Promise<string> API; make
translateManifestString consistently async by declaring it async and always
returning a Promise<string> (i.e., await or return the result of
transformStringToLocale for the __MSG_ path and wrap the plain string in a
resolved Promise for the non-match path), updating the function signature and
callers as needed; locate translateManifestString and transformStringToLocale to
implement this change.
In `@src/main/modules/extensions/management.ts`:
- Around line 308-310: The code calls _afterLoadExtension(...) which now returns
an object containing startSWPromise but currently ignores it, causing
loadExtensionWithData to resolve before the MV3 service worker is running;
modify both branches in loadExtensionWithData (the branch using
session.extensions.getExtension(extensionId) and the branch that creates/loads a
new extension) to capture the returned { startSWPromise } from
this._afterLoadExtension(loadedExtensionOrNew) and await startSWPromise before
returning so the method only resolves once the service-worker startup completes.
---
Outside diff comments:
In `@src/main/controllers/loaded-profiles-controller/index.ts`:
- Around line 186-196: Replace the mixed await/.then pattern in the loop by
awaiting the promise directly: call tabsController.createTab(...) with await
into a const (e.g., const tab = await tabsController.createTab(window.id,
profileId, undefined, undefined, { url })), then run the activation logic (if
(currentTabIndex === 0) tabsController.activateTab(tab)); keep the
currentTabIndex const and tabIndex++ logic intact and remove the .then()
callback to improve clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2edc4082-6b41-4e6f-b09c-bb2d4d4e0e93
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
package.jsonsrc/main/controllers/loaded-profiles-controller/index.tssrc/main/controllers/sessions-controller/intercept-rules/user-agent-transformer.tssrc/main/controllers/sessions-controller/protocols/_protocols/flow-internal/active-favicon.tssrc/main/controllers/tabs-controller/tab.tssrc/main/ipc/app/extensions.tssrc/main/modules/chrome.tssrc/main/modules/extensions/assets.tssrc/main/modules/extensions/helpers.tssrc/main/modules/extensions/locales.tssrc/main/modules/extensions/main.tssrc/main/modules/extensions/management.tssrc/main/modules/user-agent.ts
iamEvanYT
changed the title
…_afterLoadExtension` on extensions management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/controllers/loaded-profiles-controller/index.ts (1)
189-193: 🧹 Nitpick | 🔵 TrivialRedundant
awaitwith.then()chain.Using
awaitbefore.then()is an antipattern. Theawaitresolves the promise, then.then()wraps it in another promise. Simplify by using the resolved value directly:♻️ Proposed fix
- await tabsController.createTab(window.id, profileId, undefined, undefined, { url }).then((tab) => { - if (currentTabIndex === 0) { - tabsController.activateTab(tab); - } - }); + const tab = await tabsController.createTab(window.id, profileId, undefined, undefined, { url }); + if (currentTabIndex === 0) { + tabsController.activateTab(tab); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/controllers/loaded-profiles-controller/index.ts` around lines 189 - 193, The code uses await together with a .then() chain on tabsController.createTab which is redundant; replace the await + .then with a single awaited assignment: call tabsController.createTab(...) with the same args and assign its resolved value to a const (e.g., const tab = await tabsController.createTab(window.id, profileId, undefined, undefined, { url })), then use the existing currentTabIndex check to call tabsController.activateTab(tab); remove the .then() wrapper entirely while keeping the same activation logic.src/main/modules/extensions/management.ts (1)
314-335:⚠️ Potential issue | 🟡 MinorService worker may not be ready when
loadExtensionWithDataresolves.Both branches call
_afterLoadExtension()but don't await the returnedstartSWPromise. This means the method resolves before the MV3 background service worker is running. While the comment on line 284 indicates this is intentional ("Runs asynchronously, so we don't block the main thread"), callers expecting the extension to be fully operational may encounter race conditions.The
afterInstallhandler inloaded-profiles-controllercorrectly awaitsstartSWPromise, but bulk loading vialoadExtensions()won't wait for service workers. If this is intentional for startup performance, consider documenting this behavior in the method's JSDoc.📝 Proposed documentation addition
/** * Load an extension with data * `@param` extensionId - The ID of the extension * `@param` extensionData - The data of the extension * `@returns` The loaded extension + * `@remarks` Service worker startup is not awaited. Use `initializeLoadedExtension` + * and await `startSWPromise` if immediate SW readiness is required. */ private async loadExtensionWithData(extensionId: string, extensionData: ExtensionData) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/modules/extensions/management.ts` around lines 314 - 335, loadExtensionWithData currently calls _afterLoadExtension but does not await the service-worker startup promise (startSWPromise), so callers can get an extension object before the MV3 background service worker is running; either await the startSWPromise inside _afterLoadExtension or update loadExtensionWithData to await the service-worker startup before returning (ensure both the "loadedExtension" and newly loaded branches wait), or if the non-blocking behavior is intentional, add a JSDoc to loadExtensionWithData explaining that it does not wait for startSWPromise and callers that need a running service worker must await startSWPromise themselves (reference methods: loadExtensionWithData, _afterLoadExtension, startSWPromise, and callers like loadExtensions and afterInstall).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/controllers/loaded-profiles-controller/index.ts`:
- Around line 300-354: The getAllExtensions handler currently reads
manifest.options_url which may be absent for Manifest V3; update
getAllExtensions to prefer manifest.options_url but fall back to
manifest.options_page and then manifest.options_ui (or options_ui.page when
applicable) when building the optionsUrl field so extensions using
options_page/options_ui are supported; locate the optionsUrl assignment in the
returned object inside getAllExtensions and modify it to check
manifest.options_url || manifest.options_page || (manifest.options_ui &&
manifest.options_ui.page) (preserving existing translateManifestString usage
where needed).
In `@src/main/modules/extensions/assets.ts`:
- Around line 177-187: There is a TOCTOU risk between getFsStat ->
isPathInsideRoot -> fs.readFile on resolvedAssetPath; to fix, open the file
first (using fs.open with O_NOFOLLOW if available) and perform your
stat/containment checks against the open file descriptor or its realpath, then
read from the handle instead of calling fs.readFile after the checks; update the
logic around getFsStat, isPathInsideRoot and the final fs.readFile usage (and
handle errors from fs.open) so the containment and file-type checks are done on
the opened file to eliminate the check-then-use window.
---
Outside diff comments:
In `@src/main/controllers/loaded-profiles-controller/index.ts`:
- Around line 189-193: The code uses await together with a .then() chain on
tabsController.createTab which is redundant; replace the await + .then with a
single awaited assignment: call tabsController.createTab(...) with the same args
and assign its resolved value to a const (e.g., const tab = await
tabsController.createTab(window.id, profileId, undefined, undefined, { url })),
then use the existing currentTabIndex check to call
tabsController.activateTab(tab); remove the .then() wrapper entirely while
keeping the same activation logic.
In `@src/main/modules/extensions/management.ts`:
- Around line 314-335: loadExtensionWithData currently calls _afterLoadExtension
but does not await the service-worker startup promise (startSWPromise), so
callers can get an extension object before the MV3 background service worker is
running; either await the startSWPromise inside _afterLoadExtension or update
loadExtensionWithData to await the service-worker startup before returning
(ensure both the "loadedExtension" and newly loaded branches wait), or if the
non-blocking behavior is intentional, add a JSDoc to loadExtensionWithData
explaining that it does not wait for startSWPromise and callers that need a
running service worker must await startSWPromise themselves (reference methods:
loadExtensionWithData, _afterLoadExtension, startSWPromise, and callers like
loadExtensions and afterInstall).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 353eedaf-ffe7-4195-9ed1-2f2d1de7dadd
📒 Files selected for processing (6)
src/main/controllers/loaded-profiles-controller/index.tssrc/main/controllers/sessions-controller/intercept-rules/user-agent-transformer.tssrc/main/modules/chrome.tssrc/main/modules/extensions/assets.tssrc/main/modules/extensions/locales.tssrc/main/modules/extensions/management.ts
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
chrome.runtime.onInstalledto extensions for installation setupfixes #101
Summary by CodeRabbit
Chores
Improvements