Fire Mode: Tab data isolation#8591
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
0nko
force-pushed
the
feature/ondrej/fire-mode-webview-profiles
branch
from
f843786 to
b7e580f
Compare
0nko
force-pushed
the
feature/ondrej/fire-mode-fire-tabs
branch
from
018e6ad to
53cfdf4
Compare
| return Room.databaseBuilder(context, FireModeDatabase::class.java, "fire_mode.db") | ||
| .setJournalMode(RoomDatabase.JournalMode.TRUNCATE) | ||
| .build() | ||
| } |
There was a problem hiding this comment.
Fire mode database missing destructive migration fallback
Medium Severity
The FireModeDatabase Room builder lacks fallbackToDestructiveMigration(). Since this database stores ephemeral fire-mode tabs, any future schema change (bumping version beyond 1) without an explicit migration will crash the app with an IllegalStateException. Other ephemeral databases in the codebase (e.g., TabVisitedSitesModule) already use fallbackToDestructiveMigration(). For a fire-mode database whose data is meant to be discarded, destructive migration is the correct safety net.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5372d76. Configure here.
There was a problem hiding this comment.
Seems reasonable to include fallbackToDestructiveMigration here. What do you think, @0nko ?
…ository Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…fied TabRepository
0nko
force-pushed
the
feature/ondrej/fire-mode-fire-tabs
branch
from
27e1503 to
a162357
Compare
0nko
force-pushed
the
feature/ondrej/fire-mode-webview-profiles
branch
from
b7e580f to
70343bf
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a162357. Configure here.
| duckChatContextualDataStore = duckChatContextualDataStore, | ||
| tabVisitedSitesRepository = tabVisitedSitesRepository, | ||
| nativeInputStatePublisher = nativeInputStatePublisher, | ||
| ) |
There was a problem hiding this comment.
Shared TabSwitcherDataStore causes cross-mode side effects
Medium Severity
Both the @RegularMode and @FireMode TabDataRepository instances receive the same tabSwitcherDataStore singleton. TabDataRepository.setIsUserNew() writes to this shared store — the fire repo instance could race with the regular one, and setTabLayoutType on either repo changes the preference for both. More critically, TabDataRepository.addDefaultTab() checks tabsDao.tabs().isNotEmpty() but the setIsUserNew logic in tabSwitcherDataStore evaluates a cross-mode UNKNOWN state. If the fire repo's setIsUserNew is called first, it would set the user state based on fire-mode context, preventing the regular repo from ever setting it correctly.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a162357. Configure here.
| } | ||
|
|
||
| private val tabRepository: TabRepository | ||
| get() = tabRepositoryProvider.forMode(currentMode.value) |
There was a problem hiding this comment.
AutoComplete tabRepository getter inconsistent with reactive flow
Low Severity
The tabRepository getter resolves via currentMode.value at call time, but getAutocompleteSwitchToTabResults uses currentMode.flatMapLatest for reactive mode changes. In fireAutocompletePixel, tabRepository.liveTabs.value reads from whichever mode is current at that instant. If a mode switch occurs between the user seeing switch-to-tab suggestions (from the old mode's reactive flow still in-flight) and tapping one (triggering the pixel), the pixel's hasTabs param could reflect the wrong mode's tab count.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a162357. Configure here.
There was a problem hiding this comment.
Smoke testing didn't reveal any problems; so that LGTM.
Can approve after addressing these:
- I don't think there's an API proposal for
AggregateTabRepository? If not, it should be covered somewhere. - I think the suggestion to use
fallbackToDestructiveMigrationis a good one to include.
| // we don't store isExternal in the tab model, as it's only meant for the first time the tab is loaded. | ||
| private val externalLaunchTabIds = mutableSetOf<String>() | ||
|
|
||
| private var skipTabPagerStateSaveOnRecreate = false |
There was a problem hiding this comment.
nit: can you document why this addition and when it should be set one way or the other (i see there's a related comment elsewhere but it would be useful to explain its purpose here too)
| return Room.databaseBuilder(context, FireModeDatabase::class.java, "fire_mode.db") | ||
| .setJournalMode(RoomDatabase.JournalMode.TRUNCATE) | ||
| .build() | ||
| } |
There was a problem hiding this comment.
Seems reasonable to include fallbackToDestructiveMigration here. What do you think, @0nko ?
| browserModeStateHolder.currentMode.value | ||
|
|
||
| /** | ||
| * AppScope-singleton consumers cannot reach this binding — they must inject the qualified |
| TabEntity::class, | ||
| TabSelectionEntity::class, |
There was a problem hiding this comment.
Nit: As per my warning in the tech design, two databases now use these same entities. it might be an issue if we change one of these entities and migrate one database but forget the other.
Can you add some docs to this class, the other database, and the entities themselves indicating they are all linked/shared (might just help prevent a future problem)
| * For everything that operates on one mode at a time, inject the mode-qualified | ||
| * [TabRepository] directly. | ||
| */ | ||
| interface AggregateTabRepository { |
2 participants
Task/Issue URL: https://app.asana.com/1/137249556945/project/1207418217763355/task/1214821628980689?focus=true
Description
Isolated tab storage per mode. Fire-mode tabs persist in their own Room database, separate from regular tabs. Other tab-scoped state stays shared and will be cleaned per-mode later by the data-clearing plugin.
Mode-aware tab consumers. The tab switcher and autocomplete react to mode changes at runtime, reading from whichever mode is currently active. All behavior is gated behind the
fireTabsfeature flag — when disabled, the app behaves exactly as before.No public API or burn-flow changes. The
TabRepositoryinterface is untouched and the existing burn (clear-all-data) path is untouched. This lays the data foundation; Fire-tab UI lands in a separate workstream.DI changes for fire-mode tab isolation
To support per-mode tab repositories without forcing every existing consumer to choose a side upfront, this PR introduces three coordinated bindings:
1. Unqualified
TabRepository— ActivityScope onlyPreviously
bindUnqualifiedTabRepository(@RegularMode impl)made every plaintabRepository: TabRepositoryinjection silently resolve to the regular repo — a footgun once fire mode exists. The unqualified binding now lives inUnqualifiedTabRepositoryActivityModuleand is contributed only toActivityScope. AppScope consumers can no longer resolve unqualifiedTabRepository(the build fails withMissingBinding), forcing them to declare@RegularMode,@FireMode, both,BrowserModeDataProvider<TabRepository>, orAggregateTabRepository. Activity- and fragment-scoped consumers transparently get whichever repo matches the activity's current browser mode.2. Activity-scoped
BrowserModeprovideActivityBrowserModeis@SingleInstanceIn(ActivityScope::class)so it capturesbrowserModeStateHolder.currentMode.valueonce at activity component creation. The unqualified repo binding andBrowserActivityitself both read from this frozen value, so the whole activity instance is locked to one mode for its lifetime. Mode changes triggerrecreate(), which builds a fresh activity component that captures the new value cleanly — the two readers can never disagree mid-render.3.
AggregateTabRepositoryNew interface in
browser-apiand impl in:app(@ContributesBinding(AppScope)) exposingflowTabs: Flow<List<TabEntity>>that combines both repos. For consumers that span modes (voice session lifecycle, "user has interacted" CTA hints), this is cleaner than injecting both qualifiers separately.Injection-point inventory
BrowserViewModelBrowserTabViewModelDefaultTabManagerOmnibarLayoutViewModelBrowserNavigationBarViewModelGranularFireDialogViewModelSingleTabFireDialogViewModelInputScreenViewModelNewTabReturnHatchViewModelTabSwitcherViewModelBrowserModeDataProvider<TabRepository>AutoCompleteBrowserModeDataProvider<TabRepository>flatMapLatestsince instances can outlive a mode changeBrowserViewModelmode-change observerrecreate()FirstScreenHandlerBrowserModeDataProvider<TabRepository>+ state holderExternalIntentProcessingStateBrowserModeDataProvider<TabRepository>+flatMapLatestVoiceSessionStateManagerAggregateTabRepositoryCtaViewModelAggregateTabRepositoryShowOnAppLaunchOptionHandler@RegularModeTabStatsBucketing@RegularModeTabsDbSanitizer@RegularMode+@FireModeDataClearing@RegularModePrivacyModule.clearDataAction→ClearPersonalDataAction@RegularModeSteps to test this PR
Smoke testing the existing functionality with the FF off is sufficient for now.
Note
Medium Risk
Medium risk because it restructures tab storage/DI and makes several tab consumers mode-aware; mistakes could cause missing tabs, incorrect mode routing, or state restoration bugs (though fire-mode is feature-gated and regular-mode paths are largely preserved).
Overview
Isolates tabs by browser mode. Adds a new Room
FireModeDatabase(fire_mode.db) with its ownTabsDao, exported schema, and test coverage so fire-mode tabs persist separately from regular tabs.Makes tab consumers mode-aware and safer to inject. Introduces
@RegularMode/@FireModequalifiers plus aBrowserModeDataProvider<TabRepository>andAggregateTabRepositoryto support per-mode lookup and cross-mode tab flows; moves the unqualifiedTabRepository/BrowserModebinding toActivityScope(freezing mode per-activity instance) and updates key consumers (AutoComplete,TabSwitcherViewModel,ExternalIntentProcessingState,FirstScreenHandler,TabsDbSanitizer, pixels/stats/CTA/voice-session) to use the appropriate qualified/provider/aggregate repositories.Fixes mode-switch recreation edge cases and adds dev tooling.
BrowserActivitynow injects an activity-scopedBrowserModeand skips saving/restoring tab pager state during mode-changerecreate()to avoid restoring old-mode webviews; internal dev tabs screen gains controls to add/clear fire tabs (with per-tab deletion to avoid clearing shared singletons).Reviewed by Cursor Bugbot for commit a162357. Bugbot is set up for automated code reviews on this repo. Configure here.