feat: redesigned omnibox interface & implementation#247
Conversation
WalkthroughThis PR overhauls the omnibox: replaces the cmdk UI with a custom input-driven implementation, adds a modular suggestion pipeline (verbatim, quick-history, open-tabs, pedals, search), introduces typed IPC/state sync and preload APIs, adds bangs data/route, and wires main-process window/bounds/blur handling and renderer/provider integration. Changes
Sequence DiagramsequenceDiagram
participant User
participant OmniboxUI as OmniboxMain
participant Requestor as requestOmniboxSuggestions
participant Suggestors as Suggestor Modules
participant SearchAPI as Google Search API
participant Main as Main Process (IPC)
User->>OmniboxUI: types input / focuses
OmniboxUI->>Requestor: requestOmniboxSuggestions(input, requestId, applySuggestions)
Requestor->>Suggestors: getOmniboxSuggestions(input, flush, signal)
Suggestors->>Requestor: flush(synchronous suggestions)
Requestor->>OmniboxUI: update suggestions (sync)
Suggestors->>SearchAPI: async fetch search suggestions
SearchAPI-->>Suggestors: search completions
Suggestors->>Requestor: flush(merged async suggestions)
Requestor->>OmniboxUI: update suggestions (async final)
User->>OmniboxUI: select / commit suggestion
OmniboxUI->>Main: commitSuggestion -> navigate/open/execute pedal
Main-->>OmniboxUI: hide omnibox / set open state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Build artifacts for all platforms are ready! 🚀Download the artifacts for: One-line installer (Unstable):bunx flow-debug-build@1.2.1 --open 23622946795(execution 23622946795 / attempt 1) |
c5ad8a9 to
540d0a9
Compare
Greptile SummaryThis PR replaces the existing omnibox implementation with a fully redesigned interface and suggestion engine ( Key changes:
Confidence Score: 3/5Mergeable after addressing the silent bang-miss on first open and the cache error-recovery race; other issues are lower-severity. The overall architecture is solid and well-structured. However, two P0/P1 issues affect core features on the happy path: (1) getBangs() returns [] before async load completes, causing bang substitution to silently fail the first time a user types a !bang — a key advertised feature. (2) The bang cleanQuery regex is non-global, leaving residual bang tokens in edge-case inputs. The cache error-path race and the redundant openIn cache-prime dependency are secondary but worth fixing. src/renderer/src/lib/omnibox-new/bangs-initializer.ts and src/renderer/src/lib/omnibox-new/suggestors/verbatim.ts need attention for the bang loading race condition Important Files Changed
Sequence DiagramsequenceDiagram
participant AB as Address Bar
participant Main as Main Process (Omnibox.ts)
participant Pre as Preload API
participant OC as OmniboxMain (React)
participant Sug as Suggestor Engine
participant GSP as Google Suggest API
AB->>Main: ipcMain omnibox:show(bounds, OmniboxOpenParams)
Main->>Main: setOpenState() — increments sequence
Main->>OC: IPC omnibox:on-state-changed(OmniboxOpenState)
OC->>OC: setOpenState → triggers input/cache effects
OC->>Sug: primeQuickHistoryCache() + primeOpenTabsCache()
OC->>Sug: requestOmniboxSuggestions(input)
Sug->>Sug: getVerbatimSuggestions() [sync]
Sug->>Sug: getQuickHistorySuggestions() [sync, from cache]
Sug->>Sug: getOpenTabSuggestions() [sync, from cache]
Sug->>Sug: getPedalSuggestions() [sync]
Sug-->>OC: flush(initial suggestions)
Sug->>GSP: fetch Google Suggest (async, AbortSignal)
GSP-->>Sug: completions[]
Sug-->>OC: flush(merged search suggestions)
OC->>OC: mergeOmniboxSuggestions → render rows
OC->>Pre: commitSuggestion → flow.navigation.goTo / flow.tabs.newTab
Pre->>Main: IPC omnibox:hide
Reviews (2): Last reviewed commit: "fix: coderabbit identified issues (batch..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 24
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/sync-bangs.ts`:
- Line 5: The SOURCE_URL constant currently points to a mutable branch ref
("refs/heads/main"); replace its value with a URL referencing an immutable
commit SHA (e.g., ".../raw/<commit-sha>/src/bang.ts") so the sync uses a fixed
revision; update the SOURCE_URL constant in scripts/sync-bangs.ts to the chosen
commit hash (or accept one via env/arg) and ensure any tests or CI that rely on
this value are updated accordingly.
- Line 2: Remove the Node fs/promises import and replace any use of fs.writeFile
with Bun-native APIs: delete the line importing fs from "fs/promises" and change
the fs.writeFile(...) call to use Bun.write(...). Specifically, locate the
fs.writeFile invocation in this script and replace it with an awaited Bun.write
call (e.g., await Bun.write(pathOrBunFile, data)) and, if you need a file
handle, use Bun.file(...) when constructing the first argument; ensure no other
fs/promises symbols remain.
In `@src/main/controllers/windows-controller/types/browser.ts`:
- Around line 171-175: The delayed refocus can steal focus if the window lost
focus before the timer fires; inside the setTimeout callback (the block that
currently checks this.destroyed and this.browserWindow.isDestroyed()) add
re-checks for this.browserWindow.isFocused() and this.browserWindow.isVisible()
(along with the existing this.destroyed and this.browserWindow.isDestroyed()
checks) and only call this.omnibox.refocus() when the window is still
focused/visible and not destroyed; update the timeout callback around the
existing this.omnibox.refocus() invocation to perform these additional guards.
In `@src/main/controllers/windows-controller/utils/browser/omnibox.ts`:
- Around line 196-206: In setOpenState, avoid logging raw user input: when
constructing debugPrint("OMNIBOX", `Updating open state:
${JSON.stringify(this.openState)}`) redact or remove the currentInput field
before stringifying; e.g., build a shallow copy of this.openState (or create a
logPayload) that replaces currentInput with a fixed placeholder like
"<REDACTED>" or omits it, then pass that to debugPrint so only non-sensitive
metadata from this.openState (openIn via normalizeOpenIn, sequence,
shadowPadding) is logged; keep the rest of setOpenState and emitOpenState
unchanged.
- Around line 117-121: The current constructor uses setTimeout(() => {
this.loadInterface(); this.updateBounds(); this.hide(); }, 0) which causes
OmniboxMain to mount and immediately trigger primeQuickHistoryCache() and
primeOpenTabsCache() for every BrowserWindow; remove or stop calling
this.loadInterface() during window construction and instead call loadInterface()
(or invoke the cache-priming) only when the omnibox is actually shown—e.g., on
the BrowserWindow 'show' event or from the OmniboxMain visibility lifecycle—so
that primeQuickHistoryCache() and primeOpenTabsCache() (the functions invoked on
mount) run only when show() is called and the UI is visible. Ensure
updateBounds() and hide() remain but do not cause mounting that triggers the
caches.
In `@src/renderer/src/components/omnibox/main.tsx`:
- Around line 179-184: The effect that runs on open (useEffect in main.tsx) is
calling requestSuggestions(openState.currentInput) while also setting inputValue
to openState.currentInput, which triggers the other effect that calls
requestSuggestions(inputValue), causing duplicate requests; fix this by removing
the direct requestSuggestions(openState.currentInput) call from the open-effect
(keep setInputValue and pendingSelectionModeRef update) OR add a dedupe/guard
inside requestOmniboxSuggestions/requestSuggestions to ignore a new request if
the query equals the lastRequestedQuery (or if a request for that input is
already in-flight); update references to requestSuggestions,
requestOmniboxSuggestions, setInputValue, pendingSelectionModeRef, and
openState.currentInput accordingly.
In `@src/renderer/src/components/omnibox/omnibox-suggestion.tsx`:
- Around line 141-145: The code calls generateTitleFromUrl(suggestion.url)
twice; compute it once into a local variable (e.g., const inferredTitle =
generateTitleFromUrl(suggestion.url)) near where title and suggestion are
available, then use inferredTitle in both the conditional (replace
generateTitleFromUrl(...) in the suggestion.type === "website" && ... check) and
the render (use inferredTitle instead of calling generateTitleFromUrl again) so
you avoid redundant computation in the OmniboxSuggestion component.
In `@src/renderer/src/lib/omnibox-new/bangs-initializer.ts`:
- Around line 23-24: The code performs redundant type assertions when importing
the bangs module: remove the unnecessary second cast and directly assign the
typed import to bangs; specifically, change the pattern using bangsModule =
(await import("./bangs")) as unknown as { bangs: BangEntry[] } and then bangs =
bangsModule.bangs as BangEntry[] to a single-step assignment that reads the
imported module's bangs property (use bangs = (await import("./bangs")) as
unknown as { bangs: BangEntry[] } .bangs) or better assign bangsModule.bangs
without the final as BangEntry[] cast, keeping references to bangsModule, bangs
and BangEntry to locate the code.
- Around line 21-26: preloadBangs can race with getBangs causing duplicate
dynamic imports because it only checks bangs; update preloadBangs to reuse the
shared bangsPromise the same way getBangs does: if bangs is already loaded
return false, otherwise if bangsPromise exists await it and set bangs from its
result, else assign bangsPromise = import("./bangs")... (returning the
BangEntry[]), await that promise, set bangs from the resolved module (e.g.,
bangsModule.bangs) and finally clear bangsPromise; reference the functions/vars
preloadBangs, getBangs, bangsPromise, bangs, and the dynamic import of "./bangs"
to implement this reuse and avoid duplicate imports.
In `@src/renderer/src/lib/omnibox-new/helpers.ts`:
- Around line 115-132: The current getUniqueKeyFromUrl function strips the URL
scheme causing http/https to collide; update getUniqueKeyFromUrl to include the
protocol portion (e.g., parsed.protocol or parsed.protocol + "//") in the
returned key so protocol-level identity is preserved, while keeping the existing
trimming, www removal (hostname.replace(/^www\./i, "")), port handling
(parsed.port), pathname trimming, and inclusion of parsed.search and
parsed.hash.
In `@src/renderer/src/lib/omnibox-new/index.ts`:
- Around line 59-72: In requestOmniboxSuggestions the UI can show stale rows
because currentSuggestions is reset but applySuggestions isn't called until
flush runs; immediately clear the UI for the new request by invoking
applySuggestions([]) right after resetting currentSuggestions (or at the start
of requestOmniboxSuggestions) so the previous suggestions are removed while
awaiting async flushes; reference requestOmniboxSuggestions, currentSuggestions,
flush, and applySuggestions when making this change.
In `@src/renderer/src/lib/omnibox-new/search-providers/google.ts`:
- Around line 87-88: The fetch call for Google suggestions currently parses JSON
without checking HTTP status; update the code around the fetch(url, { signal })
call to first verify response.ok and throw or return a handled error when false
(include status and optionally await response.text() for diagnostics), and wrap
the response.json() call in a try/catch to handle malformed JSON before casting
to GoogleSuggestResponse so callers get a clear, handled error instead of an
unhandled exception.
- Around line 22-29: The calculation in mapSuggestionRelevance uses
`serverRelevance ?? fallback - index * 25` which is parsed as `serverRelevance
?? (fallback - index * 25)`; change it to use parentheses so the per-index
degradation applies to the chosen relevance: compute `(serverRelevance ??
fallback) - index * 25` (then continue clamping with Math.max/Math.min and
rounding as before) to ensure the fallback is selected first and then reduced by
index.
In `@src/renderer/src/lib/omnibox-new/search-providers/helpers.ts`:
- Around line 4-14: The fallback identity in completionIdentity produces
identical keys when completion.title is null; update completionIdentity to avoid
collisions by including an additional distinguishing field or unique fallback
(e.g., completion.id, completion.url/query when present, a generated
UUID/timestamp, or JSON-serialized distinguishing properties) so that different
completions of the same kind with null titles yield unique identities; modify
the function completionIdentity to check for a usable title and otherwise return
a composite key using those extra fields (or a generated unique token) to
prevent unintended deduplication.
In `@src/renderer/src/lib/omnibox-new/search-providers/types.ts`:
- Around line 9-18: The SearchProviderCompletion interface allows invalid combos
(e.g., kind: "navigation" without url), so replace SearchProviderCompletion with
a discriminated union keyed on SearchCompletionKind (e.g., NavigationCompletion
{ kind: "navigation"; url: string; title: string | null; relevance: number; ...
}, QueryCompletion { kind: "query"; query: string; ... }, and a fallback
GenericCompletion for other kinds) to ensure required fields are present at
compile time; update any creator code (e.g., google.ts) and consumers to use the
new union type and narrow on completion.kind before accessing kind-specific
fields.
In `@src/renderer/src/lib/omnibox-new/states.ts`:
- Around line 21-29: The urlTitleCache Map currently grows unbounded (symbols:
urlTitleCache, cacheUrlTitle, getCachedUrlTitle, getUniqueKeyFromUrl); change
the cache to enforce a maximum size or an LRU eviction policy so memory can't
grow indefinitely: replace the plain Map with either an LRU-capable structure or
wrap Map operations in helper logic that, inside cacheUrlTitle, after
urlTitleCache.set(key, title) checks the map size against a configured
MAX_CACHE_SIZE and evicts the least-recently-used or oldest entry (update access
tracking in getCachedUrlTitle to mark entries as recently used if using LRU) to
keep the number of cached entries bounded.
In `@src/renderer/src/lib/omnibox-new/suggestors/open-tabs.ts`:
- Around line 151-169: The catch block for primeOpenTabsCache currently leaves
the previously-set refreshPromise in the cache when flow.tabs.getData() fails
and no stale exists, preventing retries; update the catch handler to clear the
in-flight marker by setting openTabsCache for currentSpaceId to a non-inflight
state (e.g., set refreshPromise: null and sensible defaults: tabs: [],
focusedTabIds: {}, loadedAt: 0) or delete the cache entry so subsequent
primeOpenTabsCache calls will retry; reference openTabsCache,
primeOpenTabsCache, currentSpaceId and refreshPromise when making this change.
In `@src/renderer/src/lib/omnibox-new/suggestors/quick-history.ts`:
- Around line 195-213: The current failure path for the refreshPromise leaves a
wedged cache entry when getHistory() rejects and there is no stale entry; modify
the catch block in quick-history.ts (the prime/refresh logic using
quickHistoryCache, profileId and refreshPromise) so that on error you delete the
cache entry for that profileId (quickHistoryCache.delete(profileId)) if there is
no stale value, and only restore the stale entry with refreshPromise:null when a
stale exists (instead of silently leaving the failing refreshPromise in cache).
In `@src/renderer/src/lib/omnibox-new/suggestors/search-suggestions.ts`:
- Around line 36-55: The AbortController created in this block (controller) is
never used to cancel in-flight getSuggestions calls, causing stale results to
flush; update the suggestor to return a cleanup/teardown that calls
controller.abort() (or manage a shared controller and call abort() before
starting a new request) so any previous signal passed to
searchProvider.getSuggestions({ input, limit: SEARCH_SUGGESTION_LIMIT, signal:
controller.signal }) is cancelled when a new input arrives; ensure you still
handle the AbortError in the .catch and only call flush(...) for non-aborted
responses after mergeSearchCompletions and mapCompletionToSuggestion.
In `@src/renderer/src/lib/omnibox-new/suggestors/verbatim.ts`:
- Around line 27-32: selectedBang is already guaranteed to be defined earlier,
so remove the unnecessary optional chaining when building searchUrl: use
selectedBang.u.replace(...) instead of selectedBang?.u.replace(...). Update the
creation of searchUrl (variable name searchUrl) to call replace on
selectedBang.u and keep the subsequent if (!searchUrl) return null check
unchanged.
- Line 23: The conditional return in verbatim.ts uses a redundant ternary: since
selectedBang is guaranteed truthy (earlier return at line 17), change the
expression if (cleanQuery === "") return selectedBang ?
`https://${selectedBang.d}` : null; to directly return the URL when cleanQuery
is empty (i.e., return `https://${selectedBang.d}`) and remove the unnecessary
ternary check involving selectedBang.
In `@src/renderer/src/routes/bangs/page.tsx`:
- Around line 7-17: The code in getBangDestination incorrectly treats any string
with a dot (entry.d) as a valid hostname; instead try to construct a URL from
`https://${entry.d}` and only return it if URL parsing succeeds (catching
errors), otherwise fall back to parsing entry.u via new
URL(entry.u.replace("{{{s}}}", "")) and if that fails return the raw replaced
entry.u; update getBangDestination to validate entry.d by attempting new
URL(...) rather than using entry.d.includes("."), referencing the function name
getBangDestination and the symbols entry.d and entry.u.
In `@src/renderer/src/routes/omnibox-debug/page.tsx`:
- Around line 63-66: The applySuggestions handler currently calls
setSuggestions(items) and always clears selection with
setSelectedSuggestion(null), which drops the details pane whenever an async
suggester flushes; change applySuggestions (or surrounding logic in the
component using applySuggestions) to preserve the current selectedSuggestion
when possible by: 1) matching the existing selectedSuggestion against the new
items by a stable identifier (e.g. suggestion.id or another stable key) and only
clearing it if the selected id is no longer present, or 2) only clearing
selectedSuggestion when the user input actually changes (hook into the input
change handler) instead of on every flush; update references to
applySuggestions, setSuggestions, setSelectedSuggestion and selectedSuggestion
accordingly so selection is remapped or retained rather than always nulled.
- Around line 139-149: Replace Tailwind v3 class names with Tailwind v4
equivalents: search the JSX for className strings using "flex-grow" and
"flex-shrink-0" (for example the div rendered as <div className="relative
flex-grow"> and other elements around the Input and controls) and change
"flex-grow" to "grow" and "flex-shrink-0" to "shrink-0" throughout the file so
the layout utilities work under Tailwind CSS 4; ensure you update every
occurrence (including sibling/related JSX elements that render inputs, buttons,
or layout containers) and keep the rest of the className string intact.
🪄 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: 4f06f85c-f5e1-4d4c-8ee7-73beff568393
📒 Files selected for processing (48)
.prettierignoredocs/contributing/dependencies.mdeslint.config.mjspackage.jsonscripts/sync-bangs.tssrc/main/controllers/app-menu-controller/menu/items/file.tssrc/main/controllers/sessions-controller/protocols/static-domains/config.tssrc/main/controllers/windows-controller/types/browser.tssrc/main/controllers/windows-controller/utils/browser/omnibox.tssrc/main/ipc/app/new-tab.tssrc/main/ipc/window/omnibox.tssrc/main/modules/basic-settings.tssrc/preload/index.tssrc/renderer/src/components/browser-ui/browser-sidebar/_components/address-bar.tsxsrc/renderer/src/components/browser-ui/types.tssrc/renderer/src/components/main/website-favicon.tsxsrc/renderer/src/components/omnibox/main.tsxsrc/renderer/src/components/omnibox/omnibox-suggestion.tsxsrc/renderer/src/components/omnibox/pedal-glyph.tsxsrc/renderer/src/components/providers/settings-provider.tsxsrc/renderer/src/components/providers/spaces-provider.tsxsrc/renderer/src/css/border.csssrc/renderer/src/lib/omnibox-new/README.mdsrc/renderer/src/lib/omnibox-new/bangs-initializer.tssrc/renderer/src/lib/omnibox-new/bangs.tssrc/renderer/src/lib/omnibox-new/helpers.tssrc/renderer/src/lib/omnibox-new/index.tssrc/renderer/src/lib/omnibox-new/search-providers/google.tssrc/renderer/src/lib/omnibox-new/search-providers/helpers.tssrc/renderer/src/lib/omnibox-new/search-providers/index.tssrc/renderer/src/lib/omnibox-new/search-providers/types.tssrc/renderer/src/lib/omnibox-new/states.tssrc/renderer/src/lib/omnibox-new/suggestions.tssrc/renderer/src/lib/omnibox-new/suggestor.tssrc/renderer/src/lib/omnibox-new/suggestors/index.tssrc/renderer/src/lib/omnibox-new/suggestors/open-tabs.tssrc/renderer/src/lib/omnibox-new/suggestors/pedals.tssrc/renderer/src/lib/omnibox-new/suggestors/quick-history.tssrc/renderer/src/lib/omnibox-new/suggestors/search-suggestions.tssrc/renderer/src/lib/omnibox-new/suggestors/verbatim.tssrc/renderer/src/lib/omnibox-new/types.tssrc/renderer/src/lib/omnibox-new/zero-suggest.tssrc/renderer/src/routes/bangs/config.tsxsrc/renderer/src/routes/bangs/page.tsxsrc/renderer/src/routes/omnibox-debug/config.tsxsrc/renderer/src/routes/omnibox-debug/page.tsxsrc/renderer/src/routes/omnibox/config.tsxsrc/shared/flow/interfaces/browser/omnibox.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/renderer/src/lib/omnibox-new/search-providers/google.ts (1)
87-89:⚠️ Potential issue | 🟠 MajorHandle non-2xx and malformed JSON from Google suggest endpoint.
Line 87 and Line 88 still parse JSON unconditionally. Non-OK responses or invalid payloads can throw and break suggestion flow.
🛡️ Proposed defensive handling
- const response = await fetch(url, { signal }); - const data = (await response.json()) as GoogleSuggestResponse; + let data: GoogleSuggestResponse; + try { + const response = await fetch(url, { signal }); + if (!response.ok) { + return []; + } + data = (await response.json()) as GoogleSuggestResponse; + } catch (error) { + if (error instanceof DOMException && error.name === "AbortError") { + return []; + } + return []; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/lib/omnibox-new/search-providers/google.ts` around lines 87 - 89, The fetch in google.ts currently calls response.json() unconditionally (variables: response, data, type GoogleSuggestResponse), which will throw on non-2xx or malformed JSON; wrap the network + parse in a try/catch, first check response.ok and handle non-OK by logging/warning and returning an empty suggestion list, then attempt await response.json() inside the try and validate the shape (e.g., ensure data[1] exists/is an array) before using it; if parsing or validation fails, catch the error, log it (or return []), and ensure the calling function (e.g., the omnibox suggestions provider) receives an empty/fallback result instead of throwing.src/main/ipc/window/omnibox.ts (1)
7-10:⚠️ Potential issue | 🟠 MajorRedact omnibox params before logging.
Line 9 logs
paramsverbatim;currentInputcan include sensitive queries/URLs/file paths and should not be written to main-process logs.🔒 Suggested redaction
ipcMain.on("omnibox:show", (event, bounds: Electron.Rectangle | null, params: OmniboxOpenParams | null) => { + const logParams = params + ? { + openIn: params.openIn ?? "current", + hasCurrentInput: Boolean(params.currentInput && params.currentInput.length > 0) + } + : null; debugPrint( "OMNIBOX", - `IPC: show-omnibox received with bounds: ${JSON.stringify(bounds)} and params: ${JSON.stringify(params)}` + `IPC: show-omnibox received with bounds: ${JSON.stringify(bounds)} and params: ${JSON.stringify(logParams)}` );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/window/omnibox.ts` around lines 7 - 10, The debugPrint call in the IPC handler logs params verbatim (via JSON.stringify(params)), which may include sensitive values like params.currentInput; before logging, create a sanitized copy (e.g., in the handler that calls debugPrint) that replaces or removes sensitive fields such as currentInput (or any URL/path-like fields) and log JSON.stringify(sanitizedParams) instead of the raw params; update the call referencing debugPrint/OMNIBOX so only non-sensitive data is emitted to the main-process log.src/main/controllers/windows-controller/utils/browser/omnibox.ts (2)
209-210:⚠️ Potential issue | 🟠 MajorDo not log raw
currentInputin open-state updates.Line 209 serializes full open state, including user-entered omnibox text. Redact input before logging.
🔒 Suggested redaction
- debugPrint("OMNIBOX", `Updating open state: ${JSON.stringify(this.openState)}`); + debugPrint( + "OMNIBOX", + `Updating open state: ${JSON.stringify({ + openIn: this.openState.openIn, + sequence: this.openState.sequence, + shadowPadding: this.openState.shadowPadding, + hasCurrentInput: this.openState.currentInput.length > 0 + })}` + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/controllers/windows-controller/utils/browser/omnibox.ts` around lines 209 - 210, The debug log in the open-state update is serializing and logging sensitive user input from this.openState.currentInput; before calling debugPrint in the update routine (where debugPrint("OMNIBOX", `Updating open state: ${JSON.stringify(this.openState)}`) is used, create a shallow copy of this.openState, replace or mask the currentInput field (e.g., set currentInput to "<redacted>" or a masked string) and then JSON.stringify that copy for the log, leaving emitOpenState() and the original this.openState unmodified; update the code locations around debugPrint, emitOpenState, and any other logging of this.openState to use the redacted copy.
121-123:⚠️ Potential issue | 🟠 MajorAvoid eager omnibox interface loading during construction.
Line 122 still preloads the omnibox UI for every window at startup, even when the omnibox is never opened. This introduces avoidable startup work.
⚡ Suggested change
setTimeout(() => { - this.loadInterface(); this.updateBounds(); this.hide(); }, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/controllers/windows-controller/utils/browser/omnibox.ts` around lines 121 - 123, The constructor (or startup flow) currently calls this.loadInterface() and this.updateBounds() inside a setTimeout, which eagerly preloads the omnibox UI for every window; remove that eager call and instead invoke this.loadInterface() and this.updateBounds() lazily when the omnibox is actually requested (e.g., in the method that shows/opens the omnibox such as openOmnibox()/show()/onToggleOmnibox or the omnibox click/shortcut handler). If you need to preserve a one-time initialization guarantee, add a boolean flag (e.g., this._omniboxInitialized) checked before calling loadInterface/updateBounds so initialization runs only on first open.
🤖 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/renderer/src/lib/omnibox-new/search-providers/google.ts`:
- Around line 31-40: The normalizeNavigationUrl function currently returns any
parseable URL; update normalizeNavigationUrl to enforce an explicit protocol
allowlist by parsing the URL (using new URL(value) and the fallback new
URL(`http://${value}`)) and only returning its toString() if url.protocol is
exactly "http:" or "https:" (otherwise return null); ensure both the primary
parse path and the fallback path perform this protocol check so schemes like
"javascript:" or "data:" are never returned to callers.
In `@src/shared/flow/interfaces/browser/omnibox.ts`:
- Line 33: The interface's getState promise must reflect that the IPC handler
can return null: change the return type of getState from
Promise<OmniboxOpenState> to Promise<OmniboxOpenState | null> (or nullable
equivalent) and update any callers that assume a non-null result to handle the
null case; specifically update the OmniboxOpenState reference in the interface
and adjust usages of getState to check for null before accessing state fields to
match the sender-resolution behavior in the omnibox IPC handler.
---
Duplicate comments:
In `@src/main/controllers/windows-controller/utils/browser/omnibox.ts`:
- Around line 209-210: The debug log in the open-state update is serializing and
logging sensitive user input from this.openState.currentInput; before calling
debugPrint in the update routine (where debugPrint("OMNIBOX", `Updating open
state: ${JSON.stringify(this.openState)}`) is used, create a shallow copy of
this.openState, replace or mask the currentInput field (e.g., set currentInput
to "<redacted>" or a masked string) and then JSON.stringify that copy for the
log, leaving emitOpenState() and the original this.openState unmodified; update
the code locations around debugPrint, emitOpenState, and any other logging of
this.openState to use the redacted copy.
- Around line 121-123: The constructor (or startup flow) currently calls
this.loadInterface() and this.updateBounds() inside a setTimeout, which eagerly
preloads the omnibox UI for every window; remove that eager call and instead
invoke this.loadInterface() and this.updateBounds() lazily when the omnibox is
actually requested (e.g., in the method that shows/opens the omnibox such as
openOmnibox()/show()/onToggleOmnibox or the omnibox click/shortcut handler). If
you need to preserve a one-time initialization guarantee, add a boolean flag
(e.g., this._omniboxInitialized) checked before calling
loadInterface/updateBounds so initialization runs only on first open.
In `@src/main/ipc/window/omnibox.ts`:
- Around line 7-10: The debugPrint call in the IPC handler logs params verbatim
(via JSON.stringify(params)), which may include sensitive values like
params.currentInput; before logging, create a sanitized copy (e.g., in the
handler that calls debugPrint) that replaces or removes sensitive fields such as
currentInput (or any URL/path-like fields) and log
JSON.stringify(sanitizedParams) instead of the raw params; update the call
referencing debugPrint/OMNIBOX so only non-sensitive data is emitted to the
main-process log.
In `@src/renderer/src/lib/omnibox-new/search-providers/google.ts`:
- Around line 87-89: The fetch in google.ts currently calls response.json()
unconditionally (variables: response, data, type GoogleSuggestResponse), which
will throw on non-2xx or malformed JSON; wrap the network + parse in a
try/catch, first check response.ok and handle non-OK by logging/warning and
returning an empty suggestion list, then attempt await response.json() inside
the try and validate the shape (e.g., ensure data[1] exists/is an array) before
using it; if parsing or validation fails, catch the error, log it (or return
[]), and ensure the calling function (e.g., the omnibox suggestions provider)
receives an empty/fallback result instead of throwing.
🪄 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: fdfc5bdf-f43f-4d77-a27a-e2517a9770e1
📒 Files selected for processing (5)
src/main/controllers/windows-controller/utils/browser/omnibox.tssrc/main/ipc/window/omnibox.tssrc/preload/index.tssrc/renderer/src/lib/omnibox-new/search-providers/google.tssrc/shared/flow/interfaces/browser/omnibox.ts
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/renderer/src/routes/omnibox-debug/page.tsx (2)
65-68:⚠️ Potential issue | 🟡 MinorClearing selection on every async flush disrupts debugger usability.
setSelectedSuggestion(null)is called every timeapplySuggestionsruns. Since async suggestors can flush multiple times for a single input, this drops the details pane unexpectedly, making it difficult to inspect async search results.Consider preserving the selection when the selected suggestion still exists in the new list:
🛡️ Proposed fix: preserve selection by identity
+function getSuggestionKey(s: OmniboxSuggestion): string { + switch (s.type) { + case "search": + return `search:${s.url}`; + case "website": + return `website:${s.url}`; + case "open-tab": + return `open-tab:${s.spaceId}:${s.tabId}`; + case "pedal": + return `pedal:${s.action}`; + } +} applySuggestions: (items) => { setSuggestions(items); - setSelectedSuggestion(null); + setSelectedSuggestion((prev) => { + if (!prev) return null; + const prevKey = getSuggestionKey(prev); + return items.find((s) => getSuggestionKey(s) === prevKey) ?? null; + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/routes/omnibox-debug/page.tsx` around lines 65 - 68, The handler applySuggestions currently calls setSelectedSuggestion(null) on every flush, which drops the details pane; change it to preserve the current selection when possible by only clearing selection if the existing selected suggestion is no longer present in the new items array — use the same identity/key you use elsewhere (e.g., compare suggestion.id or strict object equality) to check membership before calling setSelectedSuggestion(null); update applySuggestions to setSuggestions(items) and then conditionally clear selection only when items.every(item => item.id !== selected?.id) (or the equivalent identity check used in your components).
147-147:⚠️ Potential issue | 🟡 MinorUpdate Tailwind v3 flex utilities to Tailwind v4 equivalents.
Per Tailwind CSS 4,
flex-growis renamed togrowandflex-shrink-0is renamed toshrink-0. Multiple occurrences need updating throughout this file.Affected lines using
flex-grow: 147, 190, 197, 300, 322, 328, 348, 411, 416.
Affected line usingflex-shrink-0: 341.🔧 Proposed fix for line 147
- <div className="relative flex-grow"> + <div className="relative grow">As per coding guidelines: "Use TailwindCSS 4 for styling"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/routes/omnibox-debug/page.tsx` at line 147, Replace Tailwind v3 utilities with v4 equivalents: update the JSX element className "relative flex-grow" (and every other className containing "flex-grow") to use "grow", and change the className "flex-shrink-0" to "shrink-0" where present; search for occurrences such as the div with className="relative flex-grow" and the element containing "flex-shrink-0" and apply these replacements across the file so all listed instances (previously at lines with flex-grow and the one flex-shrink-0) use Tailwind v4 utilities.src/renderer/src/components/omnibox/main.tsx (1)
181-186: 🧹 Nitpick | 🔵 TrivialMinor inefficiency: duplicate suggestion request on omnibox open.
When the omnibox opens, this effect sets
inputValueand directly callsrequestSuggestions. However, thesetInputValuewill trigger the effect on lines 129-131, causing a secondrequestSuggestionscall with the same input. While the abort logic prevents stale results, this causes unnecessary work.Consider removing the direct call since the
inputValuechange will trigger the request:♻️ Proposed fix
useEffect(() => { setInputValue(openState.currentInput); setSelectedIndex(0); - requestSuggestions(openState.currentInput); pendingSelectionModeRef.current = openState.currentInput ? "all" : "end"; }, [currentSpace?.profileId, openState.sequence, openState.currentInput, requestSuggestions]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/omnibox/main.tsx` around lines 181 - 186, The effect that runs on omnibox open sets inputValue, selectedIndex and pendingSelectionModeRef, but also directly calls requestSuggestions causing a duplicate request because setInputValue triggers the other effect that calls requestSuggestions; remove the direct requestSuggestions(...) call from the useEffect (the one that calls setInputValue, setSelectedIndex, and updates pendingSelectionModeRef) so that updating inputValue alone drives the suggestion request via the existing effect that listens to inputValue, leaving requestSuggestions invoked only in the single dedicated effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/src/components/omnibox/main.tsx`:
- Around line 181-186: The effect that runs on omnibox open sets inputValue,
selectedIndex and pendingSelectionModeRef, but also directly calls
requestSuggestions causing a duplicate request because setInputValue triggers
the other effect that calls requestSuggestions; remove the direct
requestSuggestions(...) call from the useEffect (the one that calls
setInputValue, setSelectedIndex, and updates pendingSelectionModeRef) so that
updating inputValue alone drives the suggestion request via the existing effect
that listens to inputValue, leaving requestSuggestions invoked only in the
single dedicated effect.
In `@src/renderer/src/routes/omnibox-debug/page.tsx`:
- Around line 65-68: The handler applySuggestions currently calls
setSelectedSuggestion(null) on every flush, which drops the details pane; change
it to preserve the current selection when possible by only clearing selection if
the existing selected suggestion is no longer present in the new items array —
use the same identity/key you use elsewhere (e.g., compare suggestion.id or
strict object equality) to check membership before calling
setSelectedSuggestion(null); update applySuggestions to setSuggestions(items)
and then conditionally clear selection only when items.every(item => item.id !==
selected?.id) (or the equivalent identity check used in your components).
- Line 147: Replace Tailwind v3 utilities with v4 equivalents: update the JSX
element className "relative flex-grow" (and every other className containing
"flex-grow") to use "grow", and change the className "flex-shrink-0" to
"shrink-0" where present; search for occurrences such as the div with
className="relative flex-grow" and the element containing "flex-shrink-0" and
apply these replacements across the file so all listed instances (previously at
lines with flex-grow and the one flex-shrink-0) use Tailwind v4 utilities.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c480a2b1-8fa3-40de-8d4f-058f6653ee4d
📒 Files selected for processing (5)
src/renderer/src/components/omnibox/main.tsxsrc/renderer/src/lib/omnibox-new/index.tssrc/renderer/src/lib/omnibox-new/suggestor.tssrc/renderer/src/lib/omnibox-new/suggestors/search-suggestions.tssrc/renderer/src/routes/omnibox-debug/page.tsx
|
@greptile review |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
src/renderer/src/routes/bangs/page.tsx (1)
8-15:⚠️ Potential issue | 🟡 MinorValidate
entry.dbefore turning it intohttps://....At Line 8,
entry.d.includes(".")is too weak and can produce malformed destinations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/routes/bangs/page.tsx` around lines 8 - 15, The check entry.d.includes(".") is too weak and can produce malformed URLs; replace it with a strict validation that ensures entry.d is a valid hostname (e.g., contains only allowed hostname characters, contains at least one dot, does not contain slashes or a scheme, and does not start/end with '.' or '-') before prepending "https://". Concretely, in the branch that currently uses entry.d (the if using entry.d.includes(".")), validate entry.d with a hostname regex (or attempt to construct new URL(`https://${entry.d}`) inside a try/catch) and only return `https://${entry.d}` when that validation succeeds; otherwise fall back to the existing entry.u replacement path (the new URL(entry.u.replace("{{{s}}}", "")) origin logic) to avoid producing malformed destinations.src/renderer/src/routes/omnibox-debug/page.tsx (1)
66-74:⚠️ Potential issue | 🟡 MinorFocus refreshes still clear the selected details row.
handleInputFocus()reissues the same request, and the first flush still callssetSelectedSuggestion(null). Clicking back into the input collapses the details pane even when the query and selected suggestion are unchanged. Clear selection on actual input changes, or remap it by stable suggestion identity instead of doing it insideapplySuggestions.Also applies to: 101-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/routes/omnibox-debug/page.tsx` around lines 66 - 74, The current applySuggestions callback clears the selected details row on the first non-empty flush (didHandleFirstNonEmptyFlush) which causes focus refreshes (handleInputFocus) to collapse the details even when the query and selection haven't changed; to fix, stop unconditionally calling setSelectedSuggestion(null) inside applySuggestions and instead: clear selection only when the actual input query changes or when the incoming suggestions no longer contain the currently selected suggestion by stable identity (e.g., compare selectedSuggestion.id against items.map(i => i.id)); update applySuggestions to preserve selectedSuggestion when ids match and move any first-flush-only selection-reset logic out to the input-change handler so focus events that re-run the request don’t collapse the details pane.
🤖 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/renderer/src/components/omnibox/omnibox-suggestion.tsx`:
- Line 2: The module references React.ReactNode as the return type for
SuggestionIcon but only imports named bindings from "react", causing a
TypeScript error; update the import to include the ReactNode type (import type {
ReactNode } from "react") and change the SuggestionIcon return type to ReactNode
(or use the imported ReactNode where it's currently using React.ReactNode) so
the type resolves correctly.
In `@src/renderer/src/lib/omnibox-new/bangs-initializer.ts`:
- Around line 39-44: The cached bangsPromise is only cleared on successful
preload, so a failing preload leaves bangsPromise set and prevents retries;
update the logic around bangsPromise/preloadBangs() (the promise creation block
that currently sets bangsPromise = preloadBangs().then(...)) to ensure
bangsPromise is cleared when preloadBangs() rejects as well (use a .catch or
.finally to set bangsPromise = undefined on failure), rethrow or propagate the
error so callers still see the failure, and keep the existing behavior of
returning bangs when preload succeeds.
In `@src/renderer/src/lib/omnibox-new/search-providers/google.ts`:
- Around line 37-47: normalizeNavigationUrl currently uses exception-driven new
URL() calls which fail under Electron; replace that logic to use the
repo-standard URL.parse() API (the same parser used elsewhere) to parse the
incoming value and fallback to parsing with "http://" prefix if needed, and
retain/consult the existing allowlist in normalizeAndValidateUrl for final
validation; add the necessary ts-ignore/typing shim around URL.parse() as used
elsewhere in the codebase so TypeScript compiles cleanly and ensure
normalizeNavigationUrl returns null on parse failure consistent with its current
contract.
- Around line 139-146: In getSuggestions, don't blanket-catch all errors from
fetchGoogleSuggestions; instead call fetchGoogleSuggestions({ ...request, input:
trimmedInput }) inside a try/catch, return [] only when the error is an
AbortError, and rethrow any other errors so the caller can observe/log them;
update the logic around fetchGoogleSuggestions in the getSuggestions function to
discriminate AbortError versus other exceptions.
In `@src/renderer/src/lib/omnibox-new/suggestors/open-tabs.ts`:
- Around line 37-59: Move URL parsing/title lowercasing out of the hot path by
computing and storing a NormalizedOpenTab[] when primeOpenTabsCache() runs, then
have getEligibleOpenTabs() read and filter that cached NormalizedOpenTab[]
instead of calling normalizeOpenTab/new URL/generateTitleFromUrl on every
keystroke; update primeOpenTabsCache() to build the normalized entries
(hostname, normalizedHostname, titleLower, urlLower, displayUrlLower) and expose
a module-scoped cache variable, and change getEligibleOpenTabs() to bail when
the cache is empty and to filter the cached array, removing repeated parsing in
normalizeOpenTab() or replacing its usage with the cached objects.
In `@src/renderer/src/lib/omnibox-new/suggestors/search-suggestions.ts`:
- Around line 30-41: flushSearchSuggestions currently calls getSuggestions for
every non-empty input, which can leak partial URLs to remote providers; modify
it to consult the user's privacy opt-in setting (e.g., a setting accessor like
isSearchSuggestionsEnabled or similar) before invoking
getSearchProvider().getSuggestions and return early if disabled, and also detect
and skip obvious navigation inputs (URLs, hostnames, file: schemes) — implement
or call a helper like isLikelyNavigation(input) and only call getSuggestions
when the setting is enabled and isLikelyNavigation(input) is false to avoid
sending navigation fragments to remote providers.
In `@src/renderer/src/routes/bangs/page.tsx`:
- Around line 80-85: The promise returned by waitForBangsLoad() is missing a
.catch() handler, so any thrown error leaves the UI in a loading state; update
the call that currently does void waitForBangsLoad().then(...) to append a
.catch(err => { if (cancelled) return; handle the error (e.g., log via
console.error or app logger), stop the loading state and/or call
setBangEntries([]) or set an error state) }), ensuring you still check cancelled
before mutating state; keep existing usages of cancelled, startTransition,
setBangEntries and getBangs when handling the success path.
In `@src/renderer/src/routes/omnibox-debug/page.tsx`:
- Around line 86-95: The mount/space-change effect sets IDs and starts
primeQuickHistoryCache and primeOpenTabsCache but does not wait for them, so
requestSuggestions(input) (which reads those caches) runs before they finish;
modify the effect that currently calls primeQuickHistoryCache/primeOpenTabsCache
to be async (or spawn an async IIFE), await both
primeQuickHistoryCache(currentSpace?.profileId, { force: true }) and
primeOpenTabsCache(currentSpace?.id, { force: true }) (handle undefined ids),
then call requestSuggestions(input) (or requestOmniboxSuggestions()) after both
promises resolve; keep the existing input-driven effect for subsequent
keystrokes, and wrap awaits in try/catch so errors don’t block the UI.
- Line 1: The code uses React.ChangeEvent types without importing React; add an
explicit type import "import type { ChangeEvent } from 'react'" at the top (next
to the existing import of hooks) and replace references to React.ChangeEvent
with ChangeEvent (e.g., in any event handler signatures used with
useCallback/useState in page.tsx) so the type is available under the modern JSX
transform.
---
Duplicate comments:
In `@src/renderer/src/routes/bangs/page.tsx`:
- Around line 8-15: The check entry.d.includes(".") is too weak and can produce
malformed URLs; replace it with a strict validation that ensures entry.d is a
valid hostname (e.g., contains only allowed hostname characters, contains at
least one dot, does not contain slashes or a scheme, and does not start/end with
'.' or '-') before prepending "https://". Concretely, in the branch that
currently uses entry.d (the if using entry.d.includes(".")), validate entry.d
with a hostname regex (or attempt to construct new URL(`https://${entry.d}`)
inside a try/catch) and only return `https://${entry.d}` when that validation
succeeds; otherwise fall back to the existing entry.u replacement path (the new
URL(entry.u.replace("{{{s}}}", "")) origin logic) to avoid producing malformed
destinations.
In `@src/renderer/src/routes/omnibox-debug/page.tsx`:
- Around line 66-74: The current applySuggestions callback clears the selected
details row on the first non-empty flush (didHandleFirstNonEmptyFlush) which
causes focus refreshes (handleInputFocus) to collapse the details even when the
query and selection haven't changed; to fix, stop unconditionally calling
setSelectedSuggestion(null) inside applySuggestions and instead: clear selection
only when the actual input query changes or when the incoming suggestions no
longer contain the currently selected suggestion by stable identity (e.g.,
compare selectedSuggestion.id against items.map(i => i.id)); update
applySuggestions to preserve selectedSuggestion when ids match and move any
first-flush-only selection-reset logic out to the input-change handler so focus
events that re-run the request don’t collapse the details pane.
🪄 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: 432bcefc-3cf2-4654-9ee2-0b970ab3b329
📒 Files selected for processing (11)
src/renderer/src/components/omnibox/omnibox-suggestion.tsxsrc/renderer/src/lib/omnibox-new/bangs-initializer.tssrc/renderer/src/lib/omnibox-new/search-providers/google.tssrc/renderer/src/lib/omnibox-new/search-providers/helpers.tssrc/renderer/src/lib/omnibox-new/search-providers/types.tssrc/renderer/src/lib/omnibox-new/suggestors/open-tabs.tssrc/renderer/src/lib/omnibox-new/suggestors/quick-history.tssrc/renderer/src/lib/omnibox-new/suggestors/search-suggestions.tssrc/renderer/src/routes/bangs/page.tsxsrc/renderer/src/routes/omnibox-debug/page.tsxsrc/shared/flow/interfaces/browser/omnibox.ts
src/renderer/src/lib/omnibox-newiamEvanYT !ghrorelectron-webauthn !npm)flow://omniboxflow://bangsSummary by CodeRabbit
New Features
Bug Fixes
Documentation