Open
Conversation
…ency scoring (#208) * feat(omnibox): implement Phase 1 — history collection, tokenized matching, frecency scoring * chore: formatt
…etion (#209) * feat(omnibox): Phase 2 — IMUI, HistoryQuickProvider, inline autocompletion * fix(omnibox): show page title instead of duplicate URL in history-url matches
…vance and navsuggest (#211) * feat(omnibox): Phase 3 — Search provider enhancement with server relevance and navsuggest * fix(omnibox): remove unused _input params from SearchProvider private methods
…213) * feat(omnibox): Phase 4 — Shortcuts provider + BookmarkProvider stub * fix(omnibox): invert shortcuts search query to match stored prefix against current input * chore: format
…226) * feat(omnibox): Phase 4 — Shortcuts provider + BookmarkProvider stub - Add omnibox_shortcuts + bookmarks table schemas and migration - Implement OmniboxShortcutsService with recordUsage, search, cleanup - Wire IPC handlers, FlowOmniboxShortcutsAPI interface, preload bridge - Build ShortcutsProvider (async, max 3, range 1000-1450, 7-day decay) - Record shortcut on omnibox selection in Omnibox.openMatch() - Add BookmarkProvider stub with TODOs (pending bookmarks system) - Add bookmarks data-provider stub (searchBookmarks, isUrlBookmarked) - Enhance dedup with cross-provider merge (bookmark+history, inline) - Update UI with bookmark/shortcut icons, labels, and descriptions * fix(omnibox): invert shortcuts search query to match stored prefix against current input The SQL LIKE pattern was checking if the stored inputText started with the current input (inputText LIKE 'gith%'), when it should check if the current input starts with the stored inputText ('gith' LIKE inputText || '%'). This fixes core shortcut matching: typing 'gith' now correctly matches shortcuts stored for 'gi', 'git', 'gith' rather than 'gith', 'github'. * chore: format * feat(omnibox): add centralized scoring engine Implement scoreMatchQuality() and computeRelevance() per design doc sections 6.3 and 6.4. Provides base relevance ranges per match type, frecency normalization, host/path/title match quality scoring, input length weighting, and bonus/penalty adjustments. * feat(omnibox): add default match stability, arrow-key lock, and provider timing diagnostics Rewrite autocomplete controller with: - Default match tracking with shouldUpdateDefault() logic (design doc §9) - Short input caution: 1-2 char inputs require >1300 relevance for non-verbatim default - Arrow-key navigation lock: onUserArrowKey() suppresses result updates, onUserKeystroke() clears lock and applies buffered matches - Provider timing diagnostics with ProviderTiming interface for debug page * feat(omnibox): enhance URL normalization for better dedup Add trailing slash normalization on all paths, default port removal (80/443), percent-encoding normalization (decode unreserved, uppercase hex digits), fragment stripping for dedup, and stable query parameter sorting per design doc §8. * feat(omnibox): add IMUI cache/restore with localStorage persistence Serialize IMUI index to localStorage on populate with version stamp, restore from cache on next open (1-hour max age), then refresh in background. Add diagnostic getters: wordCount, prefixCount, lastRefresh per design doc §4.5. * feat(omnibox): replace reload-based communication with IPC message passing Fix omnibox flickering by eliminating loadInterface() calls on show. The omnibox renderer is now loaded once and stays alive, receiving show/hide params via IPC messages instead of URL reloads. - Add OmniboxShowParams type and onShow/onHide to FlowOmniboxAPI - Add omnibox:do-show and omnibox:do-hide IPC channels in preload - Main process Omnibox class: track initialLoadComplete, add sendShowEvent() and sendHideEvent(), hide() notifies renderer - Update all 4 entry points (IPC handler, new-tab, command palette, address bar) to use sendShowEvent() instead of loadInterface() * feat(omnibox): make omnibox UI persistent with IPC-driven show/hide Convert OmniboxMain from reload-on-show to a persistent component: - Listen for flow.omnibox.onShow/onHide IPC events to control visibility - Reset state (input, matches, selection) on each show event - Use openInRef instead of URL params for current/new_tab mode - Wire up arrow key events to controller.onUserArrowKey() - Route keystroke input through controller.onUserKeystroke() - Expose diagnostic getters (providerTimings, IMUI stats) on Omnibox class - Maintain backward compat with URL params for initial load * feat(omnibox): enhance debug page with scoring, timings, and IMUI panels Expand debug page from 2 to 4 tabs: - Scoring tab: displays all ScoringSignals fields and dedup key - Timings tab: provider name, duration, match count per provider - Debug tab: add IMUI state panel (word count, prefix count, last refresh) - Fix inline completion display to use template literal * fix(omnibox): prevent show() from sending hide IPC event to renderer show() called this.hide() internally to reset state before showing, but hide() now sends 'omnibox:do-hide' to the renderer via IPC. Since callers invoke sendShowEvent() before show(), the hide event was undoing the show event, leaving isVisible=false and the omnibox invisible (opacity 0). Replace this.hide() with a direct view.setVisible(false) to reset the native view without notifying the renderer. * fix(omnibox): remove sendHideEvent from hide() to fix show race condition The blur event handler on the omnibox webcontents fires maybeHide() → hide() → sendHideEvent() during or shortly after the show() focus dance. Because sendShowEvent() is called BEFORE show(), the sequence was: renderer receives show (isVisible=true), then blur triggers hide (isVisible=false), leaving the omnibox invisible. The fix: stop sending hide IPC from hide(). Native view visibility (view.setVisible) is the sole mechanism for main-process-initiated hides. The renderer already manages its own isVisible state for user-initiated hides (Escape key, match selection), and the show handler always resets state on next open. * fix(omnibox): prevent server blending from boosting verbatim above URL navigation For URL-classified inputs (e.g. roblox.com), Google's API often returns high verbatim relevance (>=1300) for popular domains. The server blending logic was letting this override the input-type-based score, pushing verbatim from 1100 to 1300 and beating url-what-you-typed's 1200. Now URL inputs always use the base relevance (1100), ensuring navigation always wins over search for URL-like input. * feat: better omnibox styling * fix: issues * chore: format * fix: address Codex review feedback
…/main # Conflicts: # src/renderer/src/components/omnibox/main.tsx
Contributor
Build artifacts for all platforms are ready! 🚀Download the artifacts for: One-line installer (Unstable):bunx flow-debug-build@1.2.1 --open 23392875505(execution 23392875505 / attempt 1) |
Greptile SummaryThis PR delivers a large, well-designed omnibox refactor: tokenized multi-term matching, frecency scoring with exponential decay, an in-memory URL index (IMUI), inline autocomplete, new history/shortcut/bookmark providers, and the SQLite schema + IPC plumbing to back them. The architecture closely follows the design document and the provider orchestration (requestId guards, default-match stability, arrow-key lock) is implemented correctly. Overall the approach is solid, but there are a few bugs to fix before merging:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant OmniboxUI as OmniboxUI (Renderer)
participant Omnibox as Omnibox.ts
participant Controller as AutocompleteController
participant HQP as HistoryQuickProvider (sync)
participant HUP as HistoryURLProvider (async)
participant Search as SearchProvider (async)
participant IMUI as InMemoryURLIndex
participant IPC as Electron IPC
participant Main as Main Process
participant DB as SQLite DB
User->>OmniboxUI: Types in omnibox
OmniboxUI->>Omnibox: handleInput(text, "keystroke")
Omnibox->>Controller: onUserKeystroke(input)
Controller->>Controller: stop() — cancel previous query, reset requestId
Controller->>HQP: start(input, callback)
HQP->>IMUI: query(terms)
IMUI-->>HQP: IMUIQueryResult[]
HQP-->>Controller: onResults(matches) [sync]
Controller->>HUP: start(input, callback)
HUP-->>Controller: onResults([whatYouTypedMatch], continuous=true) [sync]
Controller->>Search: start(input, callback)
Search-->>Controller: onResults([verbatimMatch], continuous=true) [sync]
Controller->>OmniboxUI: onUpdate(topMatches) [initial render]
Note over HUP,DB: Async DB path
HUP->>IPC: flow.history.getSignificant()
IPC->>Main: history:get-significant
Main->>DB: SELECT significant rows
DB-->>Main: HistoryRow[]
Main-->>IPC: HistoryRow[]
IPC-->>HUP: HistoryEntry[]
HUP->>HUP: matchHistory() — score & rank
HUP-->>Controller: onResults(historyMatches)
Controller->>OmniboxUI: onUpdate(topMatches) [updated]
Note over Search,Main: Async search suggestions
Search->>Main: Google Suggest API (fetch, debounced 50ms)
Main-->>Search: ParsedSuggestion[]
Search-->>Controller: onResults([updatedVerbatim, ...suggestions])
Controller->>OmniboxUI: onUpdate(topMatches) [final]
User->>OmniboxUI: Presses Enter / selects match
OmniboxUI->>Omnibox: openMatch(match, whereToOpen)
Omnibox->>IPC: flow.omniboxShortcuts.recordUsage(...)
Omnibox->>IPC: flow.navigation.goTo(url)
IPC->>Main: navigation:go-to
Main->>DB: recordVisit(url, "", TYPED)
Main->>Main: tab.loadURL(url)
Note over Main,DB: ⚠️ Tab URL change also fires recordVisit(url, title, LINK) — double count
Last reviewed commit: 87de30d |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+578
to
+584
| // Record non-typed navigation in history. | ||
| // Typed navigations are already recorded in navigation:go-to IPC handler. | ||
| // We record here with LINK type as a fallback for link-clicks, redirects, etc. | ||
| // We use the current title (may be stale; will be updated on next title change). | ||
| if (newUrl && !newUrl.startsWith("about:")) { | ||
| historyService.recordVisit(newUrl, newTitle || this.title, VisitType.LINK); | ||
| } |
There was a problem hiding this comment.
Double history recording inflates visitCount and corrupts lastVisitType
The comment on line 579 correctly notes that typed navigations are already recorded in navigation:go-to, but the guard was never implemented. As a result, every omnibox navigation fires two recordVisit calls for the same URL:
navigation:go-to→recordVisit(url, "", VisitType.TYPED)— inserts a new row withvisitCount=1, typedCount=1- Tab URL change here →
recordVisit(newUrl, title, VisitType.LINK)— increments the same row tovisitCount=2, typedCount=1, lastVisitType=LINK
Consequences for frecency scoring:
visitCountis double-counted for every omnibox navigation (inflated by 2× vs. the true number of navigations)lastVisitTypeis always overwritten toLINK(weight 1.0) instead ofTYPED(weight 4.0), so typed-URL frecency bonuses are never awarded
The fix is to skip history recording here when the navigation was typed. Since detecting the origin in tab.ts is tricky, one approach is to only record in this listener when the URL was not already just recorded (e.g., check a small in-flight set managed by the tabs controller, or simply not record in navigation:go-to and rely on this listener for all visits, passing the typed flag via a tab attribute).
src/main/saving/db/schema.ts
Outdated
Comment on lines
+77
to
+92
| export const history = sqliteTable( | ||
| "history", | ||
| { | ||
| id: integer("id").primaryKey({ autoIncrement: true }), | ||
| url: text("url").notNull(), | ||
| title: text("title").notNull().default(""), | ||
| visitCount: integer("visit_count").notNull().default(1), | ||
| typedCount: integer("typed_count").notNull().default(0), | ||
| lastVisitTime: integer("last_visit_time").notNull(), // epoch ms | ||
| firstVisitTime: integer("first_visit_time").notNull(), // epoch ms | ||
| // Bitfield: 0=link, 1=typed, 2=bookmark, 3=redirect, 4=reload | ||
| lastVisitType: integer("last_visit_type").notNull().default(0) | ||
| }, | ||
| (table) => [ | ||
| index("idx_history_url").on(table.url), | ||
| index("idx_history_last_visit").on(table.lastVisitTime), |
There was a problem hiding this comment.
Non-unique index on history.url allows duplicate URL rows
The idx_history_url index is created as a plain (non-unique) index. The upsert logic in HistoryService.recordVisit relies on a select-then-insert/update pattern, but this is not atomic. Two concurrent writes for the same URL (which happens in practice because navigation:go-to and tab.ts both call recordVisit for every typed navigation) can both read an empty result before either has committed, resulting in two separate rows for the same URL.
Both the history table and omnibox_shortcuts table have the same issue (no unique constraint on their logical keys).
The schema should enforce uniqueness at the database level:
// history table
(table) => [
uniqueIndex("idx_history_url").on(table.url), // ← unique
index("idx_history_last_visit").on(table.lastVisitTime),
index("idx_history_typed_count").on(table.typedCount)
]
// omnibox_shortcuts table
(table) => [
index("idx_omnibox_shortcuts_input").on(table.inputText),
uniqueIndex("idx_omnibox_shortcuts_destination_input") // ← compound unique on (input_text, destination_url)
.on(table.inputText, table.destinationUrl),
]A new Drizzle migration would be needed after changing these.
Comment on lines
+26
to
+46
|
|
||
| // Split camelCase: insert boundary before uppercase letters | ||
| // Since we already lowercased, we need to do this on the original | ||
| // Actually, since we lowercased first, camelCase boundaries are gone. | ||
| // Let's handle this differently: split on the original, then lowercase. | ||
| tokens.push(part); | ||
| } | ||
|
|
||
| return tokens; | ||
| } | ||
|
|
||
| /** | ||
| * Tokenize with camelCase splitting (for cases where we want finer granularity). | ||
| */ | ||
| export function tokenizeWithCamelCase(input: string): string[] { | ||
| if (!input) return []; | ||
|
|
||
| // Split camelCase first, then split on non-alphanumeric | ||
| const withCamelSplit = input.replace(/([a-z])([A-Z])/g, "$1 $2"); | ||
| return tokenize(withCamelSplit); | ||
| } |
There was a problem hiding this comment.
CamelCase splitting is silently broken — acknowledged in a comment but never fixed
The function lowercases the string on line 33 before any camelCase processing. The comment in the loop body explicitly acknowledges the problem:
"Since we already lowercased, camelCase boundaries are gone."
The tokenizeWithCamelCase function below correctly handles this by applying replace(/([a-z])([A-Z])/g, ...) before lowercasing, but tokenize() never calls it. As a result:
- A title like
"JavaScriptReference"tokenizes as["javascriptreference"](one token) instead of["javascript", "reference"] - A URL path like
/myApiEndpointtokenizes as["myapiendpoint"]instead of["my", "api", "endpoint"]
This hurts recall for history/shortcut/bookmark matching in HQP and HUP. The fix is to apply the camelCase split before lowercasing:
export function tokenize(input: string): string[] {
if (!input) return [];
// Split camelCase BEFORE lowercasing to preserve boundaries
const withCamelSplit = input.replace(/([a-z])([A-Z])/g, "$1 $2");
const parts = withCamelSplit.toLowerCase().split(/[^a-z0-9]+/);
return parts.filter((p) => p.length > 0);
}
Comment on lines
128
to
151
| const verbatimMatch: AutocompleteMatch = { | ||
| providerName: this.name, | ||
| relevance: url ? 1250 : 1300, // High score to appear near top, but below strong nav | ||
| relevance: verbatimRelevance, | ||
| contents: inputText, | ||
| description: `Search for "${inputText}"`, // Or search engine name | ||
| description: `Search for "${inputText}"`, | ||
| destinationUrl: createSearchUrl(inputText), | ||
| type: "verbatim", // Special type for clarity, often treated as search | ||
| isDefault: true // Usually the fallback default action | ||
| type: "verbatim", | ||
| isDefault: input.inputType !== InputType.URL, | ||
| scoringSignals: { | ||
| typedCount: 0, | ||
| visitCount: 0, | ||
| elapsedTimeSinceLastVisit: 0, | ||
| frecency: 0, | ||
| matchQualityScore: 0, | ||
| hostMatchAtWordBoundary: false, | ||
| hasNonSchemeWwwMatch: false, | ||
| isHostOnly: false, | ||
| isBookmarked: false, | ||
| hasOpenTabMatch: false, | ||
| urlLength: 0, | ||
| isVerbatim: true, | ||
| searchSuggestRelevance: verbatimRelevance | ||
| } | ||
| }; |
There was a problem hiding this comment.
Verbatim AutocompleteMatch is missing allowedToBeDefault
The verbatim match emitted at line 128 does not set allowedToBeDefault. In AutocompleteResult.sort(), when two matches have equal relevance, matches with allowedToBeDefault: true are sorted above those without. In AutocompleteController.updateResults(), the short-input caution check also reads newTop.allowedToBeDefault (implicitly via shouldUpdateDefault). Setting this field ensures the verbatim match participates correctly in stability decisions.
Suggested change
| const verbatimMatch: AutocompleteMatch = { | |
| providerName: this.name, | |
| relevance: url ? 1250 : 1300, // High score to appear near top, but below strong nav | |
| relevance: verbatimRelevance, | |
| contents: inputText, | |
| description: `Search for "${inputText}"`, // Or search engine name | |
| description: `Search for "${inputText}"`, | |
| destinationUrl: createSearchUrl(inputText), | |
| type: "verbatim", // Special type for clarity, often treated as search | |
| isDefault: true // Usually the fallback default action | |
| type: "verbatim", | |
| isDefault: input.inputType !== InputType.URL, | |
| scoringSignals: { | |
| typedCount: 0, | |
| visitCount: 0, | |
| elapsedTimeSinceLastVisit: 0, | |
| frecency: 0, | |
| matchQualityScore: 0, | |
| hostMatchAtWordBoundary: false, | |
| hasNonSchemeWwwMatch: false, | |
| isHostOnly: false, | |
| isBookmarked: false, | |
| hasOpenTabMatch: false, | |
| urlLength: 0, | |
| isVerbatim: true, | |
| searchSuggestRelevance: verbatimRelevance | |
| } | |
| }; | |
| const verbatimMatch: AutocompleteMatch = { | |
| providerName: this.name, | |
| relevance: verbatimRelevance, | |
| contents: inputText, | |
| description: `Search for "${inputText}"`, | |
| destinationUrl: createSearchUrl(inputText), | |
| type: "verbatim", | |
| isDefault: input.inputType !== InputType.URL, | |
| allowedToBeDefault: input.inputType !== InputType.URL, | |
| scoringSignals: { |
Contributor
WalkthroughImplements a full omnibox redesign: new DB schema and shortcuts service, main↔renderer IPC show/hide readiness flow, renderer-side IMUI (in-memory URL index), tokenizer/frecency/scoring engine, multiple providers (history quick, shortcuts, bookmarks, search, etc.), deduplication, and inline completion support. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer as Renderer Process
participant Main as Main Process
participant DB as DB/Shortcuts
User->>Renderer: trigger omnibox (click/Cmd+K)
Renderer->>Main: omnibox:show IPC (currentInput, openIn)
Main->>Main: sendShowEvent() (queue if renderer not ready)
Main->>Renderer: omnibox:do-show IPC
Renderer->>Main: omnibox:renderer-ready IPC
Main->>Main: markRendererReady() (flush queued shows)
Renderer->>Renderer: render omnibox UI
User->>Renderer: type input
Renderer->>Renderer: classify/tokenize, query IMUI
Renderer->>DB: omnibox-shortcuts:search IPC
DB-->>Renderer: shortcuts results
Renderer->>Renderer: providers start, score, merge, dedupe, inline completion
Renderer->>Renderer: display suggestions
sequenceDiagram
participant User
participant Renderer as Omnibox (Renderer)
participant IMUI as InMemoryURLIndex
participant Providers as Data Providers
participant Main as Main Process
User->>Renderer: keystroke
Renderer->>Renderer: classifyInput(), tokenizeInput()
Renderer->>IMUI: query(terms)
IMUI-->>Renderer: IMUIQueryResult[]
Renderer->>Providers: start() (HistoryQuick, Search, Shortcuts, etc.)
Providers->>Main: omnibox-shortcuts:search (if needed)
Main-->>Providers: results
Providers-->>Renderer: onResults(matches[])
Renderer->>Renderer: merge/dedup via dedupKey, compute relevance, choose default, produce inline completion
Renderer->>Renderer: render updated suggestion list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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: 34
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/routes/omnibox-debug/page.tsx (1)
273-275:⚠️ Potential issue | 🟡 MinorString interpolation bug -
inputvariable not interpolated.The string literal
{'No suggestions found for "{input}"'}doesn't use template literal syntax, soinputis displayed literally instead of the actual input value.🐛 Proposed fix
- {'No suggestions found for "{input}"'} + {`No suggestions found for "${input}"`}🤖 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 273 - 275, The displayed message is using a plain string so the input variable isn't interpolated; update the JSX in the omnibox-debug page where the Search icon and message are rendered (the block containing Search and the text 'No suggestions found for "{input}"') to interpolate the input variable — e.g., replace the quoted literal with a JSX expression using a template literal or string concatenation (use `\`No suggestions found for "${input}"\`` or `'No suggestions found for "' + input + '"'`) so the actual value of the input variable is shown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@design/omnibox-redesign.md`:
- Around line 38-74: The fenced ASCII diagram containing nodes like "User
Input", "InputClassifier", "AutocompleteController", "AutocompleteResult",
"ScoringEngine", and "UI (Dropdown + Inline Complete)" should have a language
specifier (use ```text or ```plaintext) to satisfy markdown linting; update that
diagram block accordingly and apply the same fix to all other
pseudocode/file-list fenced blocks (the other blocks showing algorithm steps or
file lists) by adding an appropriate language tag such as text or plaintext so
syntax highlighting and lint checks pass.
In `@drizzle/0003_omnibox_shortcuts.sql`:
- Around line 1-9: The `pinned_tabs` CREATE TABLE in 0003_omnibox_shortcuts.sql
duplicates the same table from 0001_tough_scream.sql; either remove the
duplicate CREATE TABLE line from 0003_omnibox_shortcuts.sql (keeping only the
index creation idx_pinned_tabs_profile_id), or if you must keep the CREATE for
safety, add a clear comment in 0003_omnibox_shortcuts.sql stating that the table
is also defined in 0001_tough_scream.sql and that IF NOT EXISTS is used to
ensure idempotency—referencing `pinned_tabs`, `idx_pinned_tabs_profile_id`,
0003_omnibox_shortcuts.sql and 0001_tough_scream.sql so reviewers can track the
intent.
- Around line 11-23: Add an index on omnibox_shortcuts.last_access_time to speed
cleanup and recency ordering; update the migration to create an index such as
idx_omnibox_shortcuts_last_access_time ON `omnibox_shortcuts`
(`last_access_time`) so queries using .where(lastAccessTime < cutoff) and ORDER
BY last_access_time use the index.
In `@src/main/controllers/app-menu-controller/menu/items/file.ts`:
- Around line 50-53: Reorder the omnibox call sequence to match the project
convention: call omnibox.sendShowEvent(...) before omnibox.setBounds(...), and
then call omnibox.show(); specifically update the block that currently calls
omnibox.setBounds(null); omnibox.sendShowEvent({ currentInput: null, openIn:
"new_tab" }); omnibox.show(); so it becomes sendShowEvent -> setBounds -> show
using the omnibox.sendShowEvent, omnibox.setBounds, and omnibox.show methods.
In `@src/main/controllers/windows-controller/utils/browser/omnibox.ts`:
- Around line 157-160: The code must drop queued show payloads when a hide races
ahead: when hiding the omnibox (the method that dispatches
"omnibox:do-hide"/sets hidden state), clear pendingShowParams so stale show
requests won't be replayed; additionally, update markRendererReady() to check
the current hidden/shown state before flushing pendingShowParams and only emit
"omnibox:do-show" if the omnibox is still expected to be visible. In short,
clear pendingShowParams in the hide handler and gate the pendingShowParams flush
in markRendererReady() by checking the same visibility flag used for hide/show
to avoid replaying stale payloads.
In `@src/main/ipc/data/omnibox-shortcuts.ts`:
- Around line 18-23: The IPC handler registered with ipcMain.on for
"omnibox-shortcuts:record-usage" currently forwards renderer-provided parameters
directly to omniboxShortcutsService.recordUsage; add defensive validation at
that boundary: verify inputText, destinationUrl, destinationTitle, and matchType
are strings (and not excessively long), ensure inputText/destinationTitle are
non-empty, validate/sanitize destinationUrl (e.g., URL.parse/try-catch or
whitelist schemes like http/https), and ensure matchType is one of the expected
enum/values before calling omniboxShortcutsService.recordUsage; if validation
fails, log the rejection and return early (do not call the service).
In `@src/main/ipc/window/omnibox.ts`:
- Around line 48-59: The method sendHideEvent() in the Omnibox class is dead
code (defined but never called); remove the entire sendHideEvent()
implementation and any attendant unused imports or references, and keep the
hide() method comment/behavior as-is to preserve the intentional race-condition
avoidance; search for sendHideEvent, hide, and Omnibox to locate the code to
delete and ensure no remaining references or unused variables remain after
removal.
- Around line 23-27: The cast on params?.openIn is unsafe: instead of asserting
(params?.openIn as "current" | "new_tab"), validate the incoming value before
passing to omnibox.sendShowEvent; check params?.openIn against the allowed
strings "current" and "new_tab" (e.g., const openIn = params?.openIn ===
"current" ? "current" : "new_tab") and pass that validated/fallback value into
omnibox.sendShowEvent along with currentInput to avoid accepting malformed IPC
data.
In `@src/main/saving/db/schema.ts`:
- Around line 132-147: The omnibox_shortcuts schema (omniboxShortcuts) lacks a
database-level uniqueness constraint on (inputText, destinationUrl), so add a
unique constraint/index on those two columns to enforce uniqueness at the DB
level; update the sqliteTable definition for omniboxShortcuts to include a
unique composite index/constraint on table.inputText and table.destinationUrl
(so recordUsage's upsert logic remains compatible but the DB prevents duplicates
if application logic changes).
In `@src/main/saving/omnibox-shortcuts/shortcuts-service.ts`:
- Around line 82-90: The current WHERE clause uses SQL LIKE which treats stored
inputText values as patterns and lets SQL wildcards (%) and (_) alter matching;
update the predicate in shortcuts-service.ts (the query using normalizedInput
and schema.omniboxShortcuts.inputText) to use instr(normalizedInput,
schema.omniboxShortcuts.inputText) = 1 so you get true prefix matches without
wildcard interpretation, keeping the same ORDER BY on hitCount and
lastAccessTime.
In `@src/renderer/src/components/omnibox/main.tsx`:
- Around line 248-254: The current key handler accepts inline completions on Tab
or ArrowRight which hijacks normal Tab navigation; change the handler in the
block that checks event.key and inlineCompletion (the logic around
inputBox.selectionStart, inlineCompletion, and acceptInlineCompletion) so Tab is
no longer used alone to accept completions—either remove "Tab" from the
condition and only accept on "ArrowRight" at end-of-input, or require a modifier
with Tab (e.g., event.key === "Tab" && event.ctrlKey/altKey/metaKey) before
calling acceptInlineCompletion(); ensure event.preventDefault() and
acceptInlineCompletion() remain only for the chosen key+modifier combination and
preserve the atEnd check using inputBox.selectionStart ===
inputBox.value.length.
In `@src/renderer/src/lib/omnibox/autocomplete-controller.ts`:
- Around line 227-228: The current code calls this.currentResult.deduplicate()
before this.currentResult.sort(), but AutocompleteResult.deduplicate() expects
results to already be relevance-sorted; reverse the calls so you call
this.currentResult.sort() first and then this.currentResult.deduplicate().
Update the sequence around the AutocompleteResult handling (the methods named
sort() and deduplicate() on this.currentResult) so deduplicate runs on a
pre-sorted list.
- Around line 132-135: onUserKeystroke currently calls applyPendingUpdates()
which flushes buffered matches for the previous query and causes a stale-results
flash just before start() clears them; remove the applyPendingUpdates() call
from onUserKeystroke (or guard it so it only runs for navigation/commit events)
so that onUserKeystroke only sets _userIsNavigating, then calls start(input)
without replaying buffered matches; update any tests or callers expecting the
previous flush behavior accordingly and keep references to onUserKeystroke,
applyPendingUpdates, and start to locate the change.
- Around line 230-271: The current logic can leave multiple matches with
isDefault=true when a later provider outranks an earlier verbatim/default; fix
by clearing stale default flags on all candidates before promoting a new
default. In the block that handles topMatches (after const topMatches =
this.currentResult.getTopMatches()), iterate over topMatches and set m.isDefault
= false to reset prior defaults, then apply the existing promotion logic that
sets topMatches[0].isDefault = true or marks the moved existing default; update
references in the defaultMatch/shouldUpdateDefault flow so no old match retains
isDefault.
In `@src/renderer/src/lib/omnibox/autocomplete-result.ts`:
- Around line 56-62: In mergeMatches, when marking the winner as bookmarked you
currently only set winner.scoringSignals.isBookmarked if scoringSignals exists,
which drops the flag when it's missing; update mergeMatches (the method named
mergeMatches and its use of winner.scoringSignals) to ensure
winner.scoringSignals is initialized (create a scoringSignals object with
necessary defaults) when absent before setting isBookmarked=true so the bookmark
status from the duplicate is not lost.
- Around line 34-37: The deduplication mismatch stems from inconsistent dedupKey
generation: normalizeUrlForDedup() (used in autocomplete-result.ts during key =
match.dedupKey ?? normalizeUrlForDedup(match.destinationUrl)) lowercases non-URL
destinationUrl values while some providers (zero-suggest.ts) set dedupKey like
'tab:${tab.id}' and others (open-tab.ts) leave dedupKey undefined and rely on
normalizeUrlForDedup() producing a different key; to fix, standardize dedup keys
for non-URL match types by updating the providers that create open-tab matches
to set an explicit dedupKey using the same format as zero-suggest (e.g.,
'tab:${tab.id}' or include spaceId if needed), and/or adjust
normalizeUrlForDedup() to return a stable, type-prefixed key for non-URL
destinationUrl values so autocomplete-result.ts's uniqueMatches lookup
consistently deduplicates identical tabs across providers (update open-tab.ts,
zero-suggest.ts, and normalizeUrlForDedup to use the same key format).
In `@src/renderer/src/lib/omnibox/data-providers/bookmarks.ts`:
- Around line 1-60: Create a tracking issue to implement the bookmarks service +
renderer IPC and wire up the stubbed functions; include tasks to (1) ensure the
BookmarkEntry type matches the DB schema, (2) implement searchBookmarks to call
flow.bookmarks.search(query, limit), (3) implement getAllBookmarks to call
flow.bookmarks.getAll() for building an in-memory index, and (4) implement
isUrlBookmarked as a fast lookup (e.g., Set<string> refreshed periodically) and
update BookmarkProvider to use these functions.
In `@src/renderer/src/lib/omnibox/data-providers/history.ts`:
- Around line 8-10: getHistory currently always returns the full dataset (via
flow.history.list()) forcing callers like searchHistory, getRecentHistory and
getMostVisitedHistory to materialize the entire history; change the abstraction
so history queries are served with server-side limits/filters: either add
parameters to getHistory (e.g., getHistory({ limit, query, sort })) or create
dedicated RPC-backed helpers (e.g., flow.history.search(query, limit),
flow.history.listRecent(limit), flow.history.listMostVisited(limit)) and update
searchHistory, getRecentHistory and getMostVisitedHistory to call those new
parameterized functions instead of calling getHistory(); keep references to
getHistory, searchHistory, getRecentHistory, getMostVisitedHistory and
flow.history.list when making the change so you update all call sites.
In `@src/renderer/src/lib/omnibox/data-providers/shortcuts.ts`:
- Around line 28-32: The try/catch around flow.omniboxShortcuts.recordUsage in
the recordShortcutUsage path gives a false impression it will catch IPC errors
(which happen async in the main process); instead either remove the try/catch
and perform a defensive existence check or change the catch message to
explicitly state it only catches synchronous/local errors. Locate the
recordShortcutUsage caller and the flow.omniboxShortcuts.recordUsage call and:
1) replace the try/catch with a guard like if
(flow?.omniboxShortcuts?.recordUsage) flow.omniboxShortcuts.recordUsage(...); or
2) keep the catch but update the console.error text to say "synchronous/local
error while attempting to send IPC (main process errors are handled there):" so
logs aren't misleading. Ensure references to ipcRenderer.send remain untouched
since IPC errors are handled in main.
In `@src/renderer/src/lib/omnibox/in-memory-url-index.ts`:
- Around line 160-171: addOrUpdate() currently always inserts new IDs and
ignores MAX_ENTRIES, letting the in-memory index grow unbounded; modify
addOrUpdate (and related paths like buildEntry/addToIndexes/removeFromIndexes)
to enforce the same MAX_ENTRIES policy used by rebuild()/restore(): before
inserting a new id, check if this.entries.size >= MAX_ENTRIES and if so evict
the oldest entry(s) (remove from this.entries and call removeFromIndexes on
them) or skip inserting new items, then proceed to buildEntry and addToIndexes;
ensure updates to existing ids still re-index without counting against the cap.
In `@src/renderer/src/lib/omnibox/omnibox.ts`:
- Around line 44-46: The forced-search branch classifies inputs starting with
"?" as InputType.FORCED_QUERY but leaves the leading "?" in the text sent
downstream (e.g., into AutocompleteInput.text), which breaks browser-style
forced-search behavior; update the logic in omnibox.ts (the forced-search
handling where trimmed.startsWith("?") is checked, and the similar block around
lines 232-238) to normalize the input by stripping the leading "?" before
constructing/returning the AutocompleteInput (and anywhere the raw text is
passed on), so the classification remains FORCED_QUERY but the downstream text
is the query without the leading question mark.
In `@src/renderer/src/lib/omnibox/providers/history-quick.ts`:
- Around line 265-267: The allowedToBeDefault comparison is inconsistent with
HistoryURLProvider: here allowedToBeDefault uses >= INLINE_COMPLETION_THRESHOLD
while HistoryURLProvider uses > INLINE_COMPLETION_THRESHOLD; update the
comparison so both providers use the same operator (e.g., change
allowedToBeDefault to relevance > INLINE_COMPLETION_THRESHOLD to match
HistoryURLProvider) and ensure INLINE_COMPLETION_THRESHOLD logic remains
consistent across inlineCompletion, isDefault, allowedToBeDefault in the
HistoryURLProvider and this file (symbols: allowedToBeDefault,
INLINE_COMPLETION_THRESHOLD, HistoryURLProvider, inlineCompletion, isDefault).
- Around line 60-68: The current classification loop using
parsedHost.includes(token) misclassifies tokens that appear in both host and
path (e.g., "com"); update the logic in the area that builds hostTokens and
pathTokens (the loop that iterates entry.urlTokens using parsedHost) to instead
parse the URL once (e.g., create a URL object from entry.url or reuse the
existing parsedHost/path components) and produce separate token lists by
tokenizing the host and the pathname independently from their source strings;
ensure you derive host tokens from the host string and path tokens from the
pathname string rather than checking includes against parsedHost so tokens are
correctly assigned.
- Around line 240-244: The code recomputes scoreMatchQuality(terms, entry) when
building AutocompleteMatch objects; fix this by adding matchQuality to the
scored item structure where scoreMatchQuality is first computed (the "scored"
array), propagate that property through any filtering/sorting into "top", and
then use the carried matchQuality when mapping top to AutocompleteMatch instead
of calling scoreMatchQuality again; update the destructuring in the top.map
callback to read matchQuality from the item (and remove the second call to
scoreMatchQuality).
In `@src/renderer/src/lib/omnibox/providers/history-url.ts`:
- Around line 200-248: The cache and DB paths duplicate the scoring/relevance
logic; extract a shared helper (e.g., computeHistoryResult or
buildHistoryResult) that accepts the history entry, terms, inputText and
provider name and returns the result object (including relevance, contents,
description, destinationUrl, inlineCompletion from getInlineCompletion, dedupKey
from normalizeUrlForDedup, and scoringSignals including frecency from
calculateFrecency, matchQuality from scoreMatchQuality,
elapsedTimeSinceLastVisit as Date.now() - entry.lastVisitTime, urlLength, etc.).
Replace the inline scoring block in both the cache branch and the searchHistory
loop to call this helper for each entry, then push the returned result into
results and keep the single results.sort((a,b)=>b.relevance-a.relevance)
afterward. Ensure the helper reuses tokenize/allTermsMatch checks or returns
null when terms don’t match so callers can skip adding non-matching entries.
- Around line 203-206: The deduplication check inside the dbResults loop is
comparing r.destinationUrl to entry.url, but results store
normalizeUrlForDedup(entry.url) in dedupKey; update the check to compare dedup
keys instead: compute const entryKey = normalizeUrlForDedup(entry.url) (or call
normalizeUrlForDedup inline) and replace the condition if (results.some((r) =>
r.destinationUrl === entry.url)) with if (results.some((r) => r.dedupKey ===
entryKey)); this ensures duplicates with different URL formatting are correctly
filtered (see normalizeUrlForDedup and the loop over dbResults/results).
- Around line 70-81: getInlineCompletion calls stripSchemeAndWww(url) twice;
compute it once and reuse the result. In function getInlineCompletion, keep
inputLower and stripped (from stripSchemeAndWww(url)), use stripped for both the
startsWith check and to compute the completion (return
stripped.slice(inputLower.length)) instead of re-calling stripSchemeAndWww and
using fullStripped.
In `@src/renderer/src/lib/omnibox/providers/open-tab.ts`:
- Around line 66-74: The open-tab results currently set isDefault: true
unconditionally in the results.push call (see results.push in open-tab.ts and
the relevance variable), which conflicts with the controller's expectation that
only the top match is default; change that call to set isDefault
conditionally—either compute isDefault as (relevance > 1300) like
history-url.ts/history-quick.ts or only mark the highest-relevance tab as
default—so update the results.push object to set isDefault based on relevance
(or after ranking) rather than always true.
- Around line 26-77: The call to getOpenTabsInSpace() in the open-tab provider
lacks error handling; wrap its promise with a .catch() (or use try/catch if
converted to async) to handle rejections, log the error and ensure onResults is
invoked (e.g., onResults([]) or a fallback) so the provider doesn't hang; update
the block around getOpenTabsInSpace(), the results handling and any logging
(referencing getOpenTabsInSpace, onResults, this.name) to ensure failures are
caught, a useful error is logged, and onResults is always called.
In `@src/renderer/src/lib/omnibox/providers/search.ts`:
- Around line 248-254: When buildNavSuggestMatch(suggestion) returns null we
currently drop the suggestion; instead, call the search fallback builder and use
its result. Change the NAVIGATION handling in the block using
buildNavSuggestMatch to: if navMatch is null, call the search fallback builder
(e.g., buildSearchSuggestMatch or the existing search-match helper used
elsewhere) with the same suggestion and, if that returns a match, push it and
increment count; apply the identical fallback behavior to the second NAVIGATION
handling block around the buildNavSuggestMatch call at the later range so
malformed NAVIGATION suggestions are recovered as search matches.
In `@src/renderer/src/lib/omnibox/providers/shortcut.ts`:
- Around line 117-168: The promise returned by searchShortcuts is missing
rejection handling, causing unhandled rejections and leaving onResults uncalled
on IPC errors; add a .catch handler (or use try/catch with await) around the
searchShortcuts call in the autocomplete flow so any error is caught, log or
report the error via the existing logger, and call onResults([]) (or onResults
with an empty array) and return; update the block that currently starts with
searchShortcuts(inputText, MAX_RESULTS * 2).then(...) to ensure failures call
onResults([]) and respect signal.aborted.
In `@src/renderer/src/lib/omnibox/tokenizer.ts`:
- Around line 17-35: The tokenize function's comment falsely claims it splits
camelCase but lowercases the input first, removing case boundaries; either
update the docstring/comments of tokenize to say it only splits on
non-alphanumeric characters (and recommend using tokenizeWithCamelCase for
camel-case-aware tokenization), or change tokenize to perform camelCase-aware
splitting by operating on the original input to detect camel boundaries (e.g.,
split on case transitions and non-alphanumerics) and then lowercase resulting
tokens; locate and modify the tokenize function (and/or call
tokenizeWithCamelCase) to apply one of these two fixes so the implementation
matches the documentation.
In `@src/renderer/src/lib/omnibox/url-normalizer.ts`:
- Around line 20-58: The normalizeUrlForDedup function currently uses new
URL(url) which can throw in Electron; replace that with URL.parse(url) (or the
project's URL.parse wrapper) and handle the null-return case instead of relying
on try/catch: call URL.parse(url) into a variable (e.g., parsed), return
url.toLowerCase() if parsed is null, and then adapt uses of parsed.hostname,
parsed.port, parsed.pathname, parsed.search, and parsed.hash to the URL.parse
shape; keep the rest of the normalization logic (host lowercase and strip www.,
portSuffix, path trimming, normalizePercentEncoding call, sorting params via
URLSearchParams) the same but sourced from the parsed object.
In `@src/renderer/src/routes/omnibox-debug/page.tsx`:
- Around line 416-448: The UI reads selectedSuggestion.scoringSignals.* fields
directly and can throw if scoringSignals or individual fields are undefined;
update the rendering in page.tsx to defensively access these values (e.g., use
optional chaining and sensible defaults) for
selectedSuggestion.scoringSignals.frecency, .matchQualityScore, .visitCount,
.typedCount, .urlLength and .elapsedTimeSinceLastVisit so the components render
fallback values like "N/A" or 0 instead of crashing when a provider omits a
signal; ensure the conditional that formats elapsedTimeSinceLastVisit also
handles undefined.
---
Outside diff comments:
In `@src/renderer/src/routes/omnibox-debug/page.tsx`:
- Around line 273-275: The displayed message is using a plain string so the
input variable isn't interpolated; update the JSX in the omnibox-debug page
where the Search icon and message are rendered (the block containing Search and
the text 'No suggestions found for "{input}"') to interpolate the input variable
— e.g., replace the quoted literal with a JSX expression using a template
literal or string concatenation (use `\`No suggestions found for "${input}"\``
or `'No suggestions found for "' + input + '"'`) so the actual value of the
input variable is shown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: acbb7419-5f0e-436f-8a8d-7476b4911297
📒 Files selected for processing (40)
design/omnibox-redesign.mddrizzle/0001_tough_scream.sqldrizzle/0003_omnibox_shortcuts.sqldrizzle/meta/_journal.jsonsrc/main/controllers/app-menu-controller/menu/items/file.tssrc/main/controllers/windows-controller/utils/browser/omnibox.tssrc/main/ipc/app/new-tab.tssrc/main/ipc/data/omnibox-shortcuts.tssrc/main/ipc/index.tssrc/main/ipc/window/omnibox.tssrc/main/saving/db/schema.tssrc/main/saving/history/browsing-history.tssrc/main/saving/omnibox-shortcuts/shortcuts-service.tssrc/preload/index.tssrc/renderer/src/components/omnibox/main.tsxsrc/renderer/src/lib/omnibox/autocomplete-controller.tssrc/renderer/src/lib/omnibox/autocomplete-result.tssrc/renderer/src/lib/omnibox/data-providers/bookmarks.tssrc/renderer/src/lib/omnibox/data-providers/history.tssrc/renderer/src/lib/omnibox/data-providers/shortcuts.tssrc/renderer/src/lib/omnibox/frecency.tssrc/renderer/src/lib/omnibox/in-memory-url-index.tssrc/renderer/src/lib/omnibox/omnibox.tssrc/renderer/src/lib/omnibox/providers/bookmark.tssrc/renderer/src/lib/omnibox/providers/history-quick.tssrc/renderer/src/lib/omnibox/providers/history-url.tssrc/renderer/src/lib/omnibox/providers/open-tab.tssrc/renderer/src/lib/omnibox/providers/pedal.tssrc/renderer/src/lib/omnibox/providers/search.tssrc/renderer/src/lib/omnibox/providers/shortcut.tssrc/renderer/src/lib/omnibox/providers/zero-suggest.tssrc/renderer/src/lib/omnibox/scoring-engine.tssrc/renderer/src/lib/omnibox/tokenizer.tssrc/renderer/src/lib/omnibox/types.tssrc/renderer/src/lib/omnibox/url-normalizer.tssrc/renderer/src/lib/search.tssrc/renderer/src/routes/omnibox-debug/page.tsxsrc/shared/flow/flow.tssrc/shared/flow/interfaces/browser/omnibox-shortcuts.tssrc/shared/flow/interfaces/browser/omnibox.ts
Comment on lines
+38
to
+74
| ``` | ||
| User Input | ||
| | | ||
| v | ||
| +-------------------+ | ||
| | InputClassifier | URL vs Search vs Keyword | ||
| +-------------------+ | ||
| | | ||
| v | ||
| +-------------------+ | ||
| | AutocompleteController | | ||
| +-------------------+ | ||
| / | | | \ | ||
| v v v v v | ||
| +------+ +-----+ +----+ +-----+ +--------+ | ||
| | HQP | | HUP | | SP | | OTP | | Others | | ||
| +------+ +-----+ +----+ +-----+ +--------+ | ||
| History History Search OpenTab Bookmarks, | ||
| Quick URL Provider Shortcuts, | ||
| Provider Provider Pedals, | ||
| \ | | | / ZeroSuggest | ||
| v v v v v | ||
| +-------------------+ | ||
| | AutocompleteResult | Merge, dedupe, sort | ||
| +-------------------+ | ||
| | | ||
| v | ||
| +-------------------+ | ||
| | ScoringEngine | Re-rank with combined signals | ||
| +-------------------+ | ||
| | | ||
| v | ||
| +-------------------+ | ||
| | UI (Dropdown + | | ||
| | Inline Complete) | | ||
| +-------------------+ | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language specifiers to fenced code blocks.
Several code blocks in the design doc lack language specifiers, which affects syntax highlighting and markdown linting. The ASCII art diagram blocks should use a plaintext identifier.
📝 Example fix for the architecture diagram
-```
+```text
User Input
|
vSimilarly, apply language specifiers to the pseudocode blocks at lines 179, 202, 257, 273, 600, 993, and 1023 (e.g., text or plaintext for pseudocode/file lists).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| User Input | |
| | | |
| v | |
| +-------------------+ | |
| | InputClassifier | URL vs Search vs Keyword | |
| +-------------------+ | |
| | | |
| v | |
| +-------------------+ | |
| | AutocompleteController | | |
| +-------------------+ | |
| / | | | \ | |
| v v v v v | |
| +------+ +-----+ +----+ +-----+ +--------+ | |
| | HQP | | HUP | | SP | | OTP | | Others | | |
| +------+ +-----+ +----+ +-----+ +--------+ | |
| History History Search OpenTab Bookmarks, | |
| Quick URL Provider Shortcuts, | |
| Provider Provider Pedals, | |
| \ | | | / ZeroSuggest | |
| v v v v v | |
| +-------------------+ | |
| | AutocompleteResult | Merge, dedupe, sort | |
| +-------------------+ | |
| | | |
| v | |
| +-------------------+ | |
| | ScoringEngine | Re-rank with combined signals | |
| +-------------------+ | |
| | | |
| v | |
| +-------------------+ | |
| | UI (Dropdown + | | |
| | Inline Complete) | | |
| +-------------------+ | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@design/omnibox-redesign.md` around lines 38 - 74, The fenced ASCII diagram
containing nodes like "User Input", "InputClassifier", "AutocompleteController",
"AutocompleteResult", "ScoringEngine", and "UI (Dropdown + Inline Complete)"
should have a language specifier (use ```text or ```plaintext) to satisfy
markdown linting; update that diagram block accordingly and apply the same fix
to all other pseudocode/file-list fenced blocks (the other blocks showing
algorithm steps or file lists) by adding an appropriate language tag such as
text or plaintext so syntax highlighting and lint checks pass.
Comment on lines
+11
to
+23
| CREATE TABLE IF NOT EXISTS `omnibox_shortcuts` ( | ||
| `id` integer PRIMARY KEY AUTOINCREMENT NOT NULL, | ||
| `input_text` text NOT NULL, | ||
| `destination_url` text NOT NULL, | ||
| `destination_title` text DEFAULT '' NOT NULL, | ||
| `match_type` text NOT NULL, | ||
| `hit_count` integer DEFAULT 1 NOT NULL, | ||
| `last_access_time` integer NOT NULL | ||
| ); | ||
| --> statement-breakpoint | ||
| CREATE INDEX IF NOT EXISTS `idx_omnibox_shortcuts_input` ON `omnibox_shortcuts` (`input_text`); | ||
| --> statement-breakpoint | ||
| CREATE INDEX IF NOT EXISTS `idx_omnibox_shortcuts_destination` ON `omnibox_shortcuts` (`destination_url`); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for cleanup/deletion logic using lastAccessTime
rg -n -A3 "lastAccessTime|last_access_time" --type ts | grep -E "(delete|cleanup|prune|expire|WHERE)"Repository: MultiboxLabs/flow-browser
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Search for all references to omnibox_shortcuts (all file types)
echo "=== References to omnibox_shortcuts ==="
rg -i "omnibox_shortcuts" -A 2 -B 2
echo ""
echo "=== References to lastAccessTime or last_access_time (all file types) ==="
rg "lastAccessTime|last_access_time" -A 2 -B 2
echo ""
echo "=== Files in drizzle directory ==="
ls -la drizzle/ | head -20Repository: MultiboxLabs/flow-browser
Length of output: 6963
Consider adding an index on last_access_time for cleanup and sorting operations.
The schema includes indexes on input_text and destination_url, but last_access_time is used in both a cleanup query (.where(lastAccessTime < cutoff)) that performs record expiration and an ORDER BY clause for sorting results by recency. Without an index, these operations require a full table scan.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@drizzle/0003_omnibox_shortcuts.sql` around lines 11 - 23, Add an index on
omnibox_shortcuts.last_access_time to speed cleanup and recency ordering; update
the migration to create an index such as idx_omnibox_shortcuts_last_access_time
ON `omnibox_shortcuts` (`last_access_time`) so queries using
.where(lastAccessTime < cutoff) and ORDER BY last_access_time use the index.
Comment on lines
50
to
53
| } else { | ||
| omnibox.setBounds(null); | ||
| omnibox.loadInterface(null); | ||
| omnibox.sendShowEvent({ currentInput: null, openIn: "new_tab" }); | ||
| omnibox.show(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency in call order.
The call sequence here is setBounds → sendShowEvent → show, whereas src/main/ipc/app/new-tab.ts uses sendShowEvent → setBounds → show. While both work (since show() is the final trigger), consider unifying the order for maintainability.
♻️ Suggested consistent ordering
} else {
- omnibox.setBounds(null);
omnibox.sendShowEvent({ currentInput: null, openIn: "new_tab" });
+ omnibox.setBounds(null);
omnibox.show();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else { | |
| omnibox.setBounds(null); | |
| omnibox.loadInterface(null); | |
| omnibox.sendShowEvent({ currentInput: null, openIn: "new_tab" }); | |
| omnibox.show(); | |
| } else { | |
| omnibox.sendShowEvent({ currentInput: null, openIn: "new_tab" }); | |
| omnibox.setBounds(null); | |
| omnibox.show(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/controllers/app-menu-controller/menu/items/file.ts` around lines 50
- 53, Reorder the omnibox call sequence to match the project convention: call
omnibox.sendShowEvent(...) before omnibox.setBounds(...), and then call
omnibox.show(); specifically update the block that currently calls
omnibox.setBounds(null); omnibox.sendShowEvent({ currentInput: null, openIn:
"new_tab" }); omnibox.show(); so it becomes sendShowEvent -> setBounds -> show
using the omnibox.sendShowEvent, omnibox.setBounds, and omnibox.show methods.
Comment on lines
+157
to
+160
| if (!this.rendererReady) { | ||
| debugPrint("OMNIBOX", "Renderer not ready yet, queueing show event"); | ||
| this.pendingShowParams = params; | ||
| return; |
Contributor
There was a problem hiding this comment.
Drop queued show payloads when an explicit hide wins the race.
A show request can sit in pendingShowParams while rendererReady is false. If the omnibox is hidden before readiness, markRendererReady() still flushes that stale payload and replays omnibox:do-show into a renderer that should stay hidden, which brings the show/hide desync back.
Also applies to: 171-178, 284-291
🤖 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 157 - 160, The code must drop queued show payloads when a hide races
ahead: when hiding the omnibox (the method that dispatches
"omnibox:do-hide"/sets hidden state), clear pendingShowParams so stale show
requests won't be replayed; additionally, update markRendererReady() to check
the current hidden/shown state before flushing pendingShowParams and only emit
"omnibox:do-show" if the omnibox is still expected to be visible. In short,
clear pendingShowParams in the hide handler and gate the pendingShowParams flush
in markRendererReady() by checking the same visibility flag used for hide/show
to avoid replaying stale payloads.
Comment on lines
+248
to
+254
| if (suggestion.type === "NAVIGATION") { | ||
| // NavSuggest: server says this is a URL, not a query | ||
| const navMatch = this.buildNavSuggestMatch(suggestion); | ||
| if (navMatch) { | ||
| results.push(navMatch); | ||
| count++; | ||
| } |
Contributor
There was a problem hiding this comment.
Actually fall back to a search match for invalid NAVIGATION suggestions.
The comments say malformed NAVIGATION suggestions should fall back to search, but buildNavSuggestMatch() returns null and the caller just drops the entry. That turns a recoverable misclassification into a lost suggestion.
💡 Suggested change
if (suggestion.type === "NAVIGATION") {
// NavSuggest: server says this is a URL, not a query
const navMatch = this.buildNavSuggestMatch(suggestion);
if (navMatch) {
results.push(navMatch);
count++;
+ } else {
+ results.push(this.buildSearchQueryMatch({ ...suggestion, type: "QUERY" }));
+ count++;
}
} else {Also applies to: 273-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/lib/omnibox/providers/search.ts` around lines 248 - 254,
When buildNavSuggestMatch(suggestion) returns null we currently drop the
suggestion; instead, call the search fallback builder and use its result. Change
the NAVIGATION handling in the block using buildNavSuggestMatch to: if navMatch
is null, call the search fallback builder (e.g., buildSearchSuggestMatch or the
existing search-match helper used elsewhere) with the same suggestion and, if
that returns a match, push it and increment count; apply the identical fallback
behavior to the second NAVIGATION handling block around the buildNavSuggestMatch
call at the later range so malformed NAVIGATION suggestions are recovered as
search matches.
Comment on lines
+117
to
+168
| // Async IPC call to search shortcuts | ||
| searchShortcuts(inputText, MAX_RESULTS * 2).then((entries) => { | ||
| if (signal.aborted) return; | ||
|
|
||
| if (entries.length === 0) { | ||
| onResults([]); | ||
| return; | ||
| } | ||
|
|
||
| // Score and rank | ||
| const scored = entries.map((entry) => ({ | ||
| entry, | ||
| relevance: computeShortcutRelevance(entry, inputText.length) | ||
| })); | ||
|
|
||
| scored.sort((a, b) => b.relevance - a.relevance); | ||
| const top = scored.slice(0, MAX_RESULTS); | ||
|
|
||
| // Build matches | ||
| const matches: AutocompleteMatch[] = top.map(({ entry, relevance }) => { | ||
| const inlineCompletion = input.preventInlineAutocomplete | ||
| ? undefined | ||
| : computeInlineCompletion(inputText, entry.destinationUrl); | ||
|
|
||
| return { | ||
| providerName: this.name, | ||
| relevance, | ||
| contents: entry.destinationTitle || entry.destinationUrl, | ||
| description: entry.destinationUrl, | ||
| destinationUrl: entry.destinationUrl, | ||
| type: "shortcut" as const, | ||
| inlineCompletion: relevance >= INLINE_COMPLETION_THRESHOLD ? inlineCompletion : undefined, | ||
| allowedToBeDefault: relevance >= INLINE_COMPLETION_THRESHOLD, | ||
| dedupKey: normalizeUrlForDedup(entry.destinationUrl), | ||
| scoringSignals: { | ||
| typedCount: 0, | ||
| visitCount: entry.hitCount, | ||
| elapsedTimeSinceLastVisit: Date.now() - entry.lastAccessTime, | ||
| frecency: 0, | ||
| matchQualityScore: 1, // Exact match by definition | ||
| hostMatchAtWordBoundary: true, | ||
| hasNonSchemeWwwMatch: true, | ||
| isHostOnly: false, | ||
| isBookmarked: false, // TODO: Cross-ref with bookmarks when available | ||
| hasOpenTabMatch: false, | ||
| urlLength: entry.destinationUrl.length | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| onResults(matches); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing error handling for shortcut search IPC.
If searchShortcuts rejects (e.g., IPC failure), the promise goes unhandled and onResults is never called.
Proposed fix
searchShortcuts(inputText, MAX_RESULTS * 2).then((entries) => {
if (signal.aborted) return;
// ... existing logic ...
onResults(matches);
- });
+ }).catch(() => {
+ if (!signal.aborted) onResults([]);
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/lib/omnibox/providers/shortcut.ts` around lines 117 - 168,
The promise returned by searchShortcuts is missing rejection handling, causing
unhandled rejections and leaving onResults uncalled on IPC errors; add a .catch
handler (or use try/catch with await) around the searchShortcuts call in the
autocomplete flow so any error is caught, log or report the error via the
existing logger, and call onResults([]) (or onResults with an empty array) and
return; update the block that currently starts with searchShortcuts(inputText,
MAX_RESULTS * 2).then(...) to ensure failures call onResults([]) and respect
signal.aborted.
Comment on lines
+17
to
+35
| export function tokenize(input: string): string[] { | ||
| if (!input) return []; | ||
|
|
||
| // First split on non-alphanumeric characters | ||
| const parts = input.toLowerCase().split(/[^a-z0-9]+/); | ||
|
|
||
| const tokens: string[] = []; | ||
| for (const part of parts) { | ||
| if (part.length === 0) continue; | ||
|
|
||
| // Split camelCase: insert boundary before uppercase letters | ||
| // Since we already lowercased, we need to do this on the original | ||
| // Actually, since we lowercased first, camelCase boundaries are gone. | ||
| // Let's handle this differently: split on the original, then lowercase. | ||
| tokens.push(part); | ||
| } | ||
|
|
||
| return tokens; | ||
| } |
Contributor
There was a problem hiding this comment.
Misleading docstring: tokenize does not split camelCase.
The docstring claims the function splits on "camelCase boundaries," but the implementation lowercases the input first (line 21), which eliminates case information before any camelCase detection could occur. The comment at lines 27-30 acknowledges this limitation.
Consider updating the docstring to accurately reflect the behavior, or use tokenizeWithCamelCase which correctly handles camelCase splitting.
📝 Proposed docstring fix
/**
- * Tokenize a string by splitting on non-alphanumeric characters and camelCase boundaries.
+ * Tokenize a string by splitting on non-alphanumeric characters.
* All tokens are lowercased.
+ *
+ * Note: This does NOT split camelCase boundaries. Use tokenizeWithCamelCase()
+ * for finer granularity that preserves camelCase splits.
*
* Examples:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/lib/omnibox/tokenizer.ts` around lines 17 - 35, The tokenize
function's comment falsely claims it splits camelCase but lowercases the input
first, removing case boundaries; either update the docstring/comments of
tokenize to say it only splits on non-alphanumeric characters (and recommend
using tokenizeWithCamelCase for camel-case-aware tokenization), or change
tokenize to perform camelCase-aware splitting by operating on the original input
to detect camel boundaries (e.g., split on case transitions and
non-alphanumerics) and then lowercase resulting tokens; locate and modify the
tokenize function (and/or call tokenizeWithCamelCase) to apply one of these two
fixes so the implementation matches the documentation.
Comment on lines
+20
to
+58
| export function normalizeUrlForDedup(url: string): string { | ||
| try { | ||
| const parsed = new URL(url); | ||
|
|
||
| // Normalize host (lowercase, remove www.) | ||
| let host = parsed.hostname.toLowerCase(); | ||
| if (host.startsWith("www.")) host = host.slice(4); | ||
|
|
||
| // new URL() already strips scheme-default ports, so any remaining port is significant. | ||
| const port = parsed.port; | ||
| const hasNonDefaultPort = port !== ""; | ||
| const portSuffix = hasNonDefaultPort ? `:${port}` : ""; | ||
|
|
||
| // Normalize path: remove trailing slash (for all paths, not just root) | ||
| let path = parsed.pathname; | ||
| if (path.length > 1 && path.endsWith("/")) { | ||
| path = path.slice(0, -1); | ||
| } | ||
| if (path === "/") path = ""; | ||
|
|
||
| // Normalize percent-encoding (lowercase hex digits, decode unreserved) | ||
| path = normalizePercentEncoding(path); | ||
|
|
||
| // Sort query parameters for consistent comparison | ||
| const params = new URLSearchParams(parsed.search); | ||
| const sortedEntries = [...params.entries()].sort((a, b) => { | ||
| const keyCompare = a[0].localeCompare(b[0]); | ||
| if (keyCompare !== 0) return keyCompare; | ||
| return a[1].localeCompare(b[1]); | ||
| }); | ||
| const sortedParams = new URLSearchParams(sortedEntries); | ||
| const queryString = sortedParams.toString(); | ||
|
|
||
| // Ignore fragment for dedup (github.com/foo#bar == github.com/foo) | ||
| return `${host}${portSuffix}${path}${queryString ? "?" + queryString : ""}`; | ||
| } catch { | ||
| return url.toLowerCase(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using URL.parse for Electron compatibility.
Based on learnings from this codebase, URL.parse is preferred over new URL() because it works without errors in the Electron runtime environment. URL.parse returns null on failure instead of throwing, which simplifies error handling.
♻️ Proposed refactor using URL.parse
export function normalizeUrlForDedup(url: string): string {
- try {
- const parsed = new URL(url);
+ // `@ts-ignore` - URL.parse works in Electron runtime
+ const parsed = URL.parse(url);
+ if (!parsed) {
+ return url.toLowerCase();
+ }
// Normalize host (lowercase, remove www.)
let host = parsed.hostname.toLowerCase();
// ... rest of the logic unchanged ...
return `${host}${portSuffix}${path}${queryString ? "?" + queryString : ""}`;
- } catch {
- return url.toLowerCase();
- }
}Based on learnings: "In the Flow Browser codebase, URL.parse is preferred over new URL() for URL parsing because it works without errors in the Electron runtime environment."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/lib/omnibox/url-normalizer.ts` around lines 20 - 58, The
normalizeUrlForDedup function currently uses new URL(url) which can throw in
Electron; replace that with URL.parse(url) (or the project's URL.parse wrapper)
and handle the null-return case instead of relying on try/catch: call
URL.parse(url) into a variable (e.g., parsed), return url.toLowerCase() if
parsed is null, and then adapt uses of parsed.hostname, parsed.port,
parsed.pathname, parsed.search, and parsed.hash to the URL.parse shape; keep the
rest of the normalization logic (host lowercase and strip www., portSuffix, path
trimming, normalizePercentEncoding call, sorting params via URLSearchParams) the
same but sourced from the parsed object.
Comment on lines
+416
to
+448
| <div className="grid grid-cols-2 gap-2"> | ||
| <div className="bg-secondary/30 p-2 rounded"> | ||
| <span className="text-xs text-muted-foreground">Frecency</span> | ||
| <div className="font-mono font-medium"> | ||
| {selectedSuggestion.scoringSignals.frecency.toFixed(2)} | ||
| </div> | ||
| </div> | ||
| <div className="bg-secondary/30 p-2 rounded"> | ||
| <span className="text-xs text-muted-foreground">Match Quality</span> | ||
| <div className="font-mono font-medium"> | ||
| {selectedSuggestion.scoringSignals.matchQualityScore.toFixed(3)} | ||
| </div> | ||
| </div> | ||
| <div className="bg-secondary/30 p-2 rounded"> | ||
| <span className="text-xs text-muted-foreground">Visit Count</span> | ||
| <div className="font-mono font-medium">{selectedSuggestion.scoringSignals.visitCount}</div> | ||
| </div> | ||
| <div className="bg-secondary/30 p-2 rounded"> | ||
| <span className="text-xs text-muted-foreground">Typed Count</span> | ||
| <div className="font-mono font-medium">{selectedSuggestion.scoringSignals.typedCount}</div> | ||
| </div> | ||
| <div className="bg-secondary/30 p-2 rounded"> | ||
| <span className="text-xs text-muted-foreground">URL Length</span> | ||
| <div className="font-mono font-medium">{selectedSuggestion.scoringSignals.urlLength}</div> | ||
| </div> | ||
| <div className="bg-secondary/30 p-2 rounded"> | ||
| <span className="text-xs text-muted-foreground">Last Visit</span> | ||
| <div className="font-mono font-medium text-xs"> | ||
| {selectedSuggestion.scoringSignals.elapsedTimeSinceLastVisit > 0 | ||
| ? `${Math.round(selectedSuggestion.scoringSignals.elapsedTimeSinceLastVisit / 60000)}m ago` | ||
| : "N/A"} | ||
| </div> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Scoring signals properties accessed without null checks.
The code assumes all scoringSignals properties exist (e.g., frecency, matchQualityScore, visitCount), but different providers may populate different subsets of these fields. Consider using optional chaining or default values to prevent runtime errors when viewing matches from providers that don't set all signals.
🛡️ Suggested defensive access
<div className="font-mono font-medium">
- {selectedSuggestion.scoringSignals.frecency.toFixed(2)}
+ {selectedSuggestion.scoringSignals.frecency?.toFixed(2) ?? "N/A"}
</div>Apply similar pattern to other signal accesses.
🤖 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 416 - 448, The
UI reads selectedSuggestion.scoringSignals.* fields directly and can throw if
scoringSignals or individual fields are undefined; update the rendering in
page.tsx to defensively access these values (e.g., use optional chaining and
sensible defaults) for selectedSuggestion.scoringSignals.frecency,
.matchQualityScore, .visitCount, .typedCount, .urlLength and
.elapsedTimeSinceLastVisit so the components render fallback values like "N/A"
or 0 instead of crashing when a provider omits a signal; ensure the conditional
that formats elapsedTimeSinceLastVisit also handles undefined.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
drizzle/0003_omnibox_shortcuts.sql (1)
11-13:⚠️ Potential issue | 🟠 MajorAdd recency index for cleanup/order queries (still missing).
Line 11–13 add useful indexes, but
last_access_timeis still unindexed. If cleanup and recency sort paths use that column, this remains a full-scan risk at scale.Proposed migration addition
CREATE INDEX IF NOT EXISTS `idx_omnibox_shortcuts_input` ON `omnibox_shortcuts` (`input_text`); --> statement-breakpoint CREATE INDEX IF NOT EXISTS `idx_omnibox_shortcuts_destination` ON `omnibox_shortcuts` (`destination_url`); +--> statement-breakpoint +CREATE INDEX IF NOT EXISTS `idx_omnibox_shortcuts_last_access_time` ON `omnibox_shortcuts` (`last_access_time`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/0003_omnibox_shortcuts.sql` around lines 11 - 13, Add an index on the last_access_time column to avoid full-table scans for cleanup/recency queries: in the migration that creates indexes for table omnibox_shortcuts (see existing index names idx_omnibox_shortcuts_input and idx_omnibox_shortcuts_destination and the statement-breakpoint marker), add a CREATE INDEX IF NOT EXISTS for last_access_time (e.g., idx_omnibox_shortcuts_last_access_time) so cleanup and recency ORDER BY/WHERE on last_access_time use the index.src/renderer/src/lib/omnibox/in-memory-url-index.ts (1)
160-170:⚠️ Potential issue | 🟠 MajorEnforce
MAX_ENTRIESon incremental inserts.
rebuild()andrestoreFromCache()honor the 2k cap, butaddOrUpdate()still inserts every newentry.id. After enough visits, the in-memory index and the serialized snapshot can grow without bound. Apply the same eviction/skip policy here beforeset()when the ID is new.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/lib/omnibox/in-memory-url-index.ts` around lines 160 - 170, addOrUpdate currently always inserts new IDs and can grow past MAX_ENTRIES; before calling this.entries.set(...) when existing is falsy, enforce the same cap used by rebuild()/restoreFromCache(): if this.entries.size >= MAX_ENTRIES evict the oldest entry (get the first key from this.entries, call this.removeFromIndexes(oldEntry) and this.entries.delete(oldId)) or otherwise apply the same skip policy used elsewhere, then proceed to buildEntry and this.addToIndexes; make changes inside addOrUpdate near the existing/removeFromIndexes/buildEntry/addToIndexes calls so behavior matches rebuild()/restoreFromCache().
🤖 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/in-memory-url-index.ts`:
- Around line 240-250: saveToCache currently writes raw URL/title history
(IMUISnapshot via IMUI_CACHE_KEY) into renderer localStorage; instead stop
persisting raw entries in the renderer: either send the snapshot to the
privileged main process over IPC (e.g., emit a "save-imui-cache" message from
saveToCache) so the main process stores it in a secure location, or change
saveToCache to only persist non-reconstructable metadata
(counts/timestamps/hashes) and never full URLs/titles; also ensure the cache TTL
is enforced eagerly on save (drop stale data before persisting) and update the
complementary loadFromCache logic to read from the new main-process store or to
expect the reduced metadata shape.
- Around line 127-146: populate() can be called concurrently causing parallel
fetchAndRebuild() runs that race and can overwrite fresher data; serialize full
refreshes by introducing an in-flight promise (e.g. a private _populatePromise:
Promise<void> | null) and use it to deduplicate concurrent calls: if
_populatePromise exists return/await it, otherwise set _populatePromise to an
async wrapper that performs the existing populate logic (including the
cache-restore branch and calling fetchAndRebuild(now)), and clear
_populatePromise in a finally block; ensure fetchAndRebuild uses the supplied
now and that lastRefreshTime/_populated are only set by the single serialized
run. Apply the same in-flight-promise pattern for the other populate call site
referenced (around the 310-320 logic) so all full refreshes are serialized.
- Around line 333-339: rebuild() currently slices the raw upstream entries
before converting/ranking, so slice(0, InMemoryURLIndex.MAX_ENTRIES) keeps
arbitrary first arrivals; instead map all upstream entries through buildEntry to
produce imui entries, sort those imui entries by their significance/frecency
field (the score or comparable property produced by buildEntry) in descending
order, then take the top InMemoryURLIndex.MAX_ENTRIES and call
this.entries.set(...) and addToIndexes(...) for each of those capped, preserving
existing calls to buildEntry and addToIndexes.
---
Duplicate comments:
In `@drizzle/0003_omnibox_shortcuts.sql`:
- Around line 11-13: Add an index on the last_access_time column to avoid
full-table scans for cleanup/recency queries: in the migration that creates
indexes for table omnibox_shortcuts (see existing index names
idx_omnibox_shortcuts_input and idx_omnibox_shortcuts_destination and the
statement-breakpoint marker), add a CREATE INDEX IF NOT EXISTS for
last_access_time (e.g., idx_omnibox_shortcuts_last_access_time) so cleanup and
recency ORDER BY/WHERE on last_access_time use the index.
In `@src/renderer/src/lib/omnibox/in-memory-url-index.ts`:
- Around line 160-170: addOrUpdate currently always inserts new IDs and can grow
past MAX_ENTRIES; before calling this.entries.set(...) when existing is falsy,
enforce the same cap used by rebuild()/restoreFromCache(): if this.entries.size
>= MAX_ENTRIES evict the oldest entry (get the first key from this.entries, call
this.removeFromIndexes(oldEntry) and this.entries.delete(oldId)) or otherwise
apply the same skip policy used elsewhere, then proceed to buildEntry and
this.addToIndexes; make changes inside addOrUpdate near the
existing/removeFromIndexes/buildEntry/addToIndexes calls so behavior matches
rebuild()/restoreFromCache().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a3bda50-7d32-4189-ab86-149ef17fbda7
📒 Files selected for processing (2)
drizzle/0003_omnibox_shortcuts.sqlsrc/renderer/src/lib/omnibox/in-memory-url-index.ts
Comment on lines
+127
to
+146
| async populate(): Promise<void> { | ||
| const now = Date.now(); | ||
| if (this._populated && now - this.lastRefreshTime < InMemoryURLIndex.REFRESH_INTERVAL) { | ||
| return; // Too soon for a refresh | ||
| } | ||
|
|
||
| // On first population, try restoring from cache for instant results | ||
| if (!this._populated) { | ||
| const restored = this.restoreFromCache(); | ||
| if (restored) { | ||
| console.log(`[IMUI] Restored ${this.entries.size} entries from cache`); | ||
| // Still fetch fresh data in the background | ||
| this.fetchAndRebuild(now).catch((err) => { | ||
| console.error("[IMUI] Background refresh after cache restore failed:", err); | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| await this.fetchAndRebuild(now); |
Contributor
There was a problem hiding this comment.
Serialize full refreshes behind an in-flight promise.
populate() is kicked off during construction and again on focus, but _populated only flips after fetchAndRebuild() resolves. Two early calls can therefore issue parallel getSignificantHistory() IPCs and race to rebuild/save the cache; if the older request finishes last, it can overwrite fresher data and move lastRefreshTime backwards.
Also applies to: 310-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/lib/omnibox/in-memory-url-index.ts` around lines 127 - 146,
populate() can be called concurrently causing parallel fetchAndRebuild() runs
that race and can overwrite fresher data; serialize full refreshes by
introducing an in-flight promise (e.g. a private _populatePromise: Promise<void>
| null) and use it to deduplicate concurrent calls: if _populatePromise exists
return/await it, otherwise set _populatePromise to an async wrapper that
performs the existing populate logic (including the cache-restore branch and
calling fetchAndRebuild(now)), and clear _populatePromise in a finally block;
ensure fetchAndRebuild uses the supplied now and that lastRefreshTime/_populated
are only set by the single serialized run. Apply the same in-flight-promise
pattern for the other populate call site referenced (around the 310-320 logic)
so all full refreshes are serialized.
Comment on lines
+240
to
+250
| saveToCache(): void { | ||
| if (!this._populated || this.entries.size === 0) return; | ||
|
|
||
| try { | ||
| const snapshot: IMUISnapshot = { | ||
| version: IMUI_CACHE_VERSION, | ||
| timestamp: Date.now(), | ||
| entries: Array.from(this.entries.values()) | ||
| }; | ||
| localStorage.setItem(IMUI_CACHE_KEY, JSON.stringify(snapshot)); | ||
| console.log(`[IMUI] Saved ${snapshot.entries.length} entries to cache`); |
Contributor
There was a problem hiding this comment.
Don’t persist raw history snapshots in renderer localStorage.
This stores URLs and titles in a renderer-accessible, on-disk cache, which broadens exposure of sensitive browsing history. The one-hour TTL is only enforced lazily on the next read, so the data can sit at rest much longer. If startup caching is required, keep it in a privileged main-process store or persist only non-reconstructable metadata.
Also applies to: 260-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/lib/omnibox/in-memory-url-index.ts` around lines 240 - 250,
saveToCache currently writes raw URL/title history (IMUISnapshot via
IMUI_CACHE_KEY) into renderer localStorage; instead stop persisting raw entries
in the renderer: either send the snapshot to the privileged main process over
IPC (e.g., emit a "save-imui-cache" message from saveToCache) so the main
process stores it in a secure location, or change saveToCache to only persist
non-reconstructable metadata (counts/timestamps/hashes) and never full
URLs/titles; also ensure the cache TTL is enforced eagerly on save (drop stale
data before persisting) and update the complementary loadFromCache logic to read
from the new main-process store or to expect the reduced metadata shape.
Comment on lines
+333
to
+339
| // Take the most significant entries up to the cap | ||
| const capped = entries.slice(0, InMemoryURLIndex.MAX_ENTRIES); | ||
|
|
||
| for (const entry of capped) { | ||
| const imuiEntry = this.buildEntry(entry); | ||
| this.entries.set(entry.id, imuiEntry); | ||
| this.addToIndexes(imuiEntry); |
Contributor
There was a problem hiding this comment.
Cap after ranking, not by upstream order.
rebuild() says it keeps the “most significant” entries, but src/renderer/src/lib/omnibox/data-providers/history.ts:17-31 only filters; it does not rank. slice(0, MAX_ENTRIES) therefore keeps whichever 2k entries arrive first instead of the highest-frecency candidates once history grows past the cap.
♻️ Suggested fix
- // Take the most significant entries up to the cap
- const capped = entries.slice(0, InMemoryURLIndex.MAX_ENTRIES);
+ const capped = entries
+ .map((entry) => ({
+ entry,
+ frecency: calculateFrecency(entry.visitCount, entry.typedCount, entry.lastVisitTime)
+ }))
+ .sort((a, b) => b.frecency - a.frecency || b.entry.lastVisitTime - a.entry.lastVisitTime)
+ .slice(0, InMemoryURLIndex.MAX_ENTRIES);
- for (const entry of capped) {
+ for (const { entry } of capped) {
const imuiEntry = this.buildEntry(entry);
this.entries.set(entry.id, imuiEntry);
this.addToIndexes(imuiEntry);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/lib/omnibox/in-memory-url-index.ts` around lines 333 - 339,
rebuild() currently slices the raw upstream entries before converting/ranking,
so slice(0, InMemoryURLIndex.MAX_ENTRIES) keeps arbitrary first arrivals;
instead map all upstream entries through buildEntry to produce imui entries,
sort those imui entries by their significance/frecency field (the score or
comparable property produced by buildEntry) in descending order, then take the
top InMemoryURLIndex.MAX_ENTRIES and call this.entries.set(...) and
addToIndexes(...) for each of those capped, preserving existing calls to
buildEntry and addToIndexes.
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.
Summary
What this changes
Why this PR is large
Validation
bun typecheckSummary by CodeRabbit
New Features
Improvements