Data settings check: pin the legacy store outcome in the failed-delete check - #11763
Merged
Merged
Conversation
The failed-delete check asserted that the Clear chats confirmation closes when the backend refuses the clear. The app closes it only when the legacy IndexedDB clear succeeds: with both stores failed, clearStoredChats throws and the confirmation stays open, armed for a retry, by design since #7029. Which of the two happens is decided by the legacy store gate (#9446). Any legacy read slower than 1 s shuts it for the life of the page, after which a clear reports the legacy store as failed. Firefox on the Windows runner occasionally crosses that line, and the check then fails at line 216 with the confirmation still on screen. Seen on main at 288b1a3 and on #11730 and a87d180, with Chromium passing in the same job. The fixture now refuses legacy writes on request, so the check drives the outcome it asserts instead of inheriting whatever the gate did earlier: the clear settles, the confirmation is still open with its action and switch re-enabled, no chat is lost, and Cancel closes it with no further delete. It is pinned to the confirmation by its switch, not `.last`, which slides onto Settings once the confirmation goes.
A Firefox run on the Windows runner failed in reset() with one hidden dialog left 5 s after closing Settings, and the only evidence was a blank screenshot. The report now lists every [role=dialog] still in the DOM with its data-state, display, opacity and running animations, so the next occurrence says whether an exit animation held the dialog or something else kept it mounted.
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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.
Problem
python tests/studio/playwright_data_settings.pyintermittently fails in Frontend CI's Windows Firefox leg, while Chromium passes in the same job. The failure is:It has shown up on main at 288b1a3, on #11740 (twice, once on a rerun) and on #11730.
The failed-delete check sets the backend to refuse the clear, and then asserts that the Clear chats confirmation closes. The app closes it only when the legacy IndexedDB clear succeeds. When both stores fail,
clearStoredChatsthrows "both backend and legacy clear failed". The confirmation then stays open, armed for a retry, which has been the design since #7029.Which of the two outcomes the check gets depends on
LegacyStoreGate(studio/frontend/src/features/chat/utils/chat-history-storage.ts:187). Any legacy read slower than 1 s shuts the gate for the rest of the page, and after that a clear counts the legacy store as failed. Firefox on the Windows runner sometimes crosses that 1 s line earlier in the run. The check then inherits a shut gate and fails at line 216. So the product behaves as intended, and the test asserted an outcome it did not control.Fix
failLegacyWrites). The failed-delete check sets this, so it drives the outcome it asserts instead of inheriting whatever the gate did earlier. It asserts that:#clear-chats-delete-files) rather than.last..lastslides onto Settings once the confirmation closes.[role=dialog]still in the DOM: itsdata-state, display, opacity and running animations. The next unrelated failure will say what kept a dialog mounted, instead of leaving only a blank screenshot.Evidence
Firefox results, same machine and same
origin/main:#clear-chats-delete-filescount 1Earlier loops of this PR's script also passed with no failures: 48 runs in Firefox and 16 in Chromium. A plain Firefox loop on main (96 runs) never reproduced the failure locally. It only showed 3 unrelated
Page.gototimeouts under load, which is why the check had to force the shut-gate state to reproduce it.