Add fire actions to native chat history screen#8579
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
1e7249b to
42023af
Compare
0b7066d to
50cd37b
Compare
ecf63f0 to
0c07887
Compare
9ca6df8 to
fa16e62
Compare
fc2d91c to
23c4ac2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 23ad28e. Configure here.
malmstein
left a comment
There was a problem hiding this comment.
Did not install or smoke-test the APK. The feature itself is well-tested — the ViewModel side has thorough coverage and the plugin/dialog wiring is clean. The main concern is that SingleTabFireDialogViewModel.onDeleteAllClicked reuses the full fire-button telemetry path for the ChatHistory origin, which will contaminate fire-button counts, daily pixels, and the data-clearing wide event with chat-history-selected deletes. Secondary: the chat-history N=1 paths dispatch the clear directly via DataClearingTrigger, bypassing the showClearDuckAIChatHistory flag check that the dialog path honours — worth deciding which side is correct and making both consistent. The reconcile-from-flow side effect is a smaller style point.
Wires the toolbar fire icon to a confirmation dialog reusing SingleTabFireDialog via a new ChatHistory origin. Confirming clears all Duck.ai chats and closes any open Duck.ai tabs without touching browser tabs or stored fire preferences.
Entered from the toolbar overflow Select entry or a row long-press, with a Select-all header below the toolbar and a brand-blue check swap on each selected row. The toolbar fire icon doubles as Delete-selected when the selection is non-empty. Chat-resume, Duck.ai open, and fire-icon routing move to the ViewModel so the Fragment no longer holds a DuckChatInternal reference. Multi-chat deletes still flow through the existing repository path with a TODO for the upcoming `ClearableData.DuckChats.Selected` pipeline.
Adds a `ClearableData.DuckChats.Selected(chatUrls)` variant and a dedicated `ManualDataClearing.clearSelectedDuckAiChats` entry point so chat-history Delete-selected (and the N=1 Fire-all fast-path) clear only the targeted chats and their open tabs, with sync recording batched per user action. The chat-history origin carries the URL subset on `FireDialogOrigin.ChatHistory` so the dialog picks the surgical path on its own. Tab cleanup that used to live inline in `performGranularClear` moves to a new `DuckAiTabsCleanupPlugin` that handles both All and Selected dispatches.
The N≥2 Fire-all path used to wipe every Duck.ai chat (Pinned included) because it dispatched `DuckChats.All`. Capture the Recent chat IDs into `PendingConfirmation.FireAll(chatIds)` (mirroring `DeleteSelected`) and let the dialog clear only those URLs via `ClearableData.DuckChats.Selected`. Drops the now-unused `fireOptionsOverride` field and the `options` parameter on `clearDataUsingManualFireOptions` — both were only there to support the buggy options-driven path.
Tab URLs drift over a session (server redirects, accumulated params, fragments after replies), so exact-string set membership only matched the freshly-opened tab and missed older tabs of the same chat. Extract chatID from both sides of the comparison so every tab pointing at a cleared chat gets closed, regardless of URL drift.
The 3-dot overflow on each chat row now fires the chat-history-screen single-delete path (via the existing `dispatchSelectedClear` helper) instead of showing a Coming-soon snackbar. Pin / Rename / Download keep the placeholder snackbar.
Make `selectedChatUrls` non-nullable and derive `count` from its size so the two fields can't drift. An empty set is a valid no-op shape — the dialog still renders but the destructive dispatch becomes a no-op via the `Selected(emptySet())` plugin contract. Bundle plumbing drops `ARG_CHAT_HISTORY_COUNT` and deserialization no longer downgrades a malformed ChatHistory bundle to the Browser origin (which would have surfaced the wrong destructive UI). DuckAiTabsCleanup drops the lint-suppressed `:duckchat-impl` import in favour of the stable `"chatID"` literal.
Single(url) is just Selected(setOf(url)) on the data side; collapse the two variants so callers describe the shape in one place. The in-chat fire flow preserves the old "don't touch this tab" behaviour by replacing the tab URL before dispatching the clear, so the tabs-cleanup plugin's chatID match no longer finds this tab. Duplicate tabs at the same chatID now also get closed on the in-chat fire path — same multi-tab fix we already shipped for the chat-history path.
stateIn(WhileSubscribed) doesn't guarantee subscribers observe the Loading initial value, so manually awaiting Loading then initial Loaded races against the StateFlow replay. Route every test in the file through the existing awaitInitialLoaded helper that already tolerates both orderings.
searchState, confirmationState and modeState were three independent MutableStateFlows
fed into a 4-arg combine. Any action that needed to mutate more than one of them
(only onDeleteSelectedConfirmed today) produced two upstream signals to combine,
which could surface as one or two downstream frames depending on scheduler timing —
the test for that action was flaky in suite runs as a result.
Consolidate into a single UiControls(search, confirmation, mode) behind one
MutableStateFlow. Every action is now one .update { it.copy(...) } → exactly one
combine frame. Restores the deterministic awaitItem contract the test relies on.
Switches the 18 tests we authored on this branch (7 in DataClearingTest, 11 in the new DuckAiTabsCleanupPluginTest) from the camelCase whenX_thenY style to the backtick natural-English style already used elsewhere in duckchat-impl tests. The pre-existing 68 _then tests in DataClearingTest are left alone — only the tests this branch introduces are renamed.
onDeleteAllClicked routed ChatHistory clears to clearSelectedDuckAiChats but left shouldRestartAfterClearing at its default true, so after a bounded chat-subset clear the dialog still called killAndRestartProcess(). Set the flag to false on that branch, mirroring onDeleteThisTabClicked.
The callback's isEnabled flag was only assigned inside the Loaded branch of render(); a transition out of Loaded (e.g. last chat deleted externally while in select mode) left it enabled but with no matching case in handleOnBackPressed, silently consuming every back press and the toolbar nav arrow. Re-derive isEnabled from the rendered state at the end of render() via a pure helper, so every transition recomputes it and any future state defaults to off.
…after suspend applyDefaultToolbar now clears navigationContentDescription so the back arrow doesn't keep announcing "Exit selection mode" to screen readers after leaving select mode — that CD is only set by applySelectModeToolbar on the same toolbar instance. renderConfirmation re-checks the fragment tag after createFireDialog returns. The outer tag check is synchronous, but createFireDialog is suspending, so two near-simultaneous emissions could both pass the outer guard and race to show the dialog under the same tag. The post-suspend check serialises on show().
… too ChatHistory always takes the without-restart path now (shouldRestartAfterClearing is set to false on that branch), so the dialog emits EVENT_CLEAR_WITHOUT_RESTART_STARTED instead of EVENT_ON_CLEAR_STARTED. The fragment listener only handled the latter, leaving controls.confirmation non-null after the user confirmed — which let any subsequent render re-open the dialog. Handle both events under the same arm: from the fragment's perspective they mean the same thing (the dialog is now driving the clear; drop the pending confirmation state).
The unchecked drawable was r=9 inside a 24x24 viewport (18dp visible) while the checked drawable is a filled r=12 disc (24dp visible), matching the Figma spec of a 24px outlined circle. Resize the unchecked vector to a 2dp ring at full 24dp and drive the swap via a state-list drawable + duplicateParentState, replacing the imperative setImageResource toggle in the view holder.
Animate a 300ms Y-axis flip on the row's icon container when the user toggles a row in or out of selection, swapping the chat-type drawable and accent background at the apex. The flip runs only on selection changes detected via DiffUtil.getChangePayload and routed through the 3-arg onBindViewHolder, so scrolling and full rebinds stay snappy.
Internal discussion landed on: Fire-all should wipe every Duck.ai chat, Pinned included. onFireAllRequested now operates on the full item list rather than the non-pinned subset; N=1 fast-path covers a single pinned chat too, and N≥2 builds the confirmation set from every chatId. The fragment shows the fire icon whenever any chat is present, not just when Recent is non-empty.
Add onDeleteSelectedChatsClicked() for the ChatHistory dialog origin so chat-only deletes no longer ride the fire-button pixels, daily pixel, fire-button use count, FIRE_BUTTON_EXECUTED user event, or DataClearingWideEvent — those describe the fire button, not a chat subset wipe. The fragment routes the delete-all buttons to the new function when ARG_ORIGIN is CHAT_HISTORY; everything else still calls onDeleteAllClicked unchanged.
Drop the N=1 short-circuit so tapping the Fire icon outside select mode always opens the "Delete N chats?" dialog, even when only one chat exists. Brings the Fire-all entry point in line with the destructive- action affordance the icon implies. Per-row Delete and N=1 selected- delete keep their immediate-execution behaviour.
Fold the stale-id intersection into reduce so the uiState derivation is pure. The upstream observeChats no longer writes back into controls through reconcileSelection — the previous shape round-tripped through combine and made the dataflow harder to follow. onSelectAllToggled now filters its comparison through latestItems so a selection that lags a delete cannot skew the "all visible already selected?" check.
The dialog path went through DataClearing.clearSelectedDuckAiChats, which already drops the request when the showClearDuckAIChatHistory remote-config flag is off. The ViewModel paths (per-row Delete, N=1 fire-all, N=1 selected-delete) dispatched DuckChats.Selected through DataClearingTrigger directly and bypassed that check, so chats could still be deleted one at a time with the flag off. Apply the same gate inline so both paths agree.
23ad28e to
3f58bc3
Compare
malmstein
left a comment
There was a problem hiding this comment.
Did not install or smoke-test the APK. The state machine in ChatHistoryViewModel is clean — UiControls rolls search, mode, and confirmation into one source, the reduce step reconciles stale selection against the live list, and the fire-all / delete-selected paths both flow through the same dialog. Capturing chat IDs at confirmation time and resolving them to URLs via DuckChatInternal.buildChatUrl removes the URL-construction sprawl. The new DuckAiTabsCleanupPlugin matching by chatID rather than full URL equality is the right call and the test for tab URL drift documents it nicely. The reorder in DataClearing.clearTabFromBrowser with that explanatory comment also reads well. Test coverage on both the VM and the plugin is solid.
|
|
||
| private fun Uri.chatIdOrNull(): String? { | ||
| if (!duckChat.isDuckChatUrl(this)) return null | ||
| return getQueryParameter("chatID")?.takeIf { it.isNotBlank() } |
There was a problem hiding this comment.
nit: this is the third copy of "chatID" in the codebase — RealDuckChat has a private CHAT_ID_QUERY_NAME const, NativeInputManager hardcodes it too. can we promote it so we stop duplicating it?
There was a problem hiding this comment.
I'm opening an api proposal for this and I'll open a new PR for that.


Task/Issue URL: https://app.asana.com/1/137249556945/project/72649045549333/task/1214820120386820?focus=true
Description
Wires the data-clearing actions on the Duck.ai chat history screen so they perform real deletions.
Selectedvariant.Steps to test this PR
Note
Prerequisites:
duckAiChatHistory(self,historyScreen) is ON andduckChat → useNativeStorageChatDatais ON.Per-row Delete
Select mode
Fire-all
Empty / edge cases
UI changes
Note
Medium Risk
Adds new Duck.ai chat deletion flows and a tab-cleanup data-clearing plugin that can close tabs based on chat IDs; mistakes could delete/close the wrong tabs or fail to clear intended chats.
Overview
Wires the native Duck.ai chat history screen to perform real deletions: adds select mode (long-press/overflow), select-all header, and uses the existing fire dialog to confirm bulk deletion with a new pluralized title.
Introduces scoped clearing for a selected set of Duck.ai chats (
ClearableData.DuckChats.Selected) via newManualDataClearing.clearSelectedDuckAiChats, updatesDataClearingto dispatch selected/all chat clears throughDataClearingTrigger, and addsDuckAiTabsCleanupPluginto close any open Duck.ai tabs whosechatIDmatches cleared chats (including URL drift).Adjusts
SingleTabFireDialog/ViewModel to support a newFireDialogOrigin.ChatHistory, avoid tab-specific probes/pixels for that origin, and addsDuckChat.buildChatUrlas the single source of truth for chat URL construction; tests updated/added across app and duckchat modules.Reviewed by Cursor Bugbot for commit 3f58bc3. Bugbot is set up for automated code reviews on this repo. Configure here.