fix: reuse saved path for repeat DevTools saves by claude[bot] · Pull Request #55431 · electron/electron

claude · 2026-10-28T19:07:58Z

Requested by David Babenko-Sanders · Slack thread

Description of Change

WebContents::DevToolsSaveToFile looked up the path already chosen for a URL in saved_files_ but discarded the result of base::FindOrNull, so path stayed null and every DevTools "Save" showed the save dialog again instead of writing to the file picked the first time. This PR assigns the lookup result to path, so a repeat save of the same URL writes straight to the stored file. The new spec in api-web-contents.spec.ts drives the save embedder message twice for the same URL against the mock xdg-desktop-portal FileChooser, which can now return a successful pick through a new QueueResponse method (the default is still to cancel, so existing specs are unchanged), and asserts that only one SaveFile request reaches the portal. The test was validated against a prebuilt nightly (47.0.0-nightly.20261028), where it fails as expected because the fix isn't built there; this environment can't build Electron, so the passing run will come from CI.

Opened an upstream CL to mark base::FindOrNull as [[nodiscard]] to prevent this type of bug from slipping in again: https://chromium-review.googlesource.com/c/chromium/src/+/8671329

Checklist

I have filled out the PR description
tests are changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, past tense, and no longer than 80 characters.

Release Notes

🤖 Generated with Claude Code

https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm

Notes: Fixed DevTools "Save" prompting for a location every time instead of reusing the previously chosen file.

claude

Additional findings (outside the current diff — GitHub can't attach inline comments there):

🟣 shell/browser/api/electron_api_web_contents.cc — pre-existing: DevTools users who pick a new file with "Save as" for an already-saved URL still get their content written to the first file. try_emplace at electron_api_web_contents.cc:6164 never replaces an existing key. When save_as is true and url is already in saved_files_, the newly chosen path is dropped and path points at the old entry. Fix: store the dialog result with insert_or_assign (or saved_files_[url] = *new_path) so a fresh pick always wins, and let later plain saves of that URL reuse it. A Save As case in the new spec would cover this.

Why this was flagged

The DevTools frontend sends the 'save' embedder message with save_as=true ("Save as...") for a URL saved earlier in the same WebContents. It arrives through InspectableWebContents::SaveToFile (shell/browser/ui/inspectable_web_contents.cc:856) and then WebContents::DevToolsSaveToFile. Because save_as is true, line 6154 skips the lookup and ShowSaveDialogSync asks the user for a new path B. Line 6164 calls saved_files_.try_emplace(url, B). The key already maps to the first path A, so nothing is inserted and iter->second is still A. The code reports savedURL with A (line 6176) and WriteToFile overwrites A (line 6180), while B is never created. The base has the same bug and also wrote every repeat save to A after showing the dialog. This PR fixes the plain-save path but leaves the save_as path writing to the stale file. The PathsMap is a std::map (electron_api_web_contents.h:920), and try_emplace on it never overwrites, so no safeguard catches this.

Verification: pre-existing. In shell/browser/api/electron_api_web_contents.cc:6154-6155, save_as=true skips the lookup, so the dialog returns new path B. Line 6164 runs saved_files_.try_emplace(url, std::move(*new_path)), which does nothing when the key already exists, so iter->second is still the old path A. WriteToFile (lines 6176-6181) writes the content to A. On the base commit the same try_emplace already dropped the new pick.

claude

I reviewed the latest push and didn't find any new bugs. My earlier comment about a cancelled Linux save storing an empty path is fixed by the !new_path->empty() check. A human look is still worthwhile because this changes a file-writing DevTools handler in shell/browser/api/, and the new spec hasn't had a passing run yet.

What was reviewed (from a standalone clone):

DevToolsSaveToFile: the stored path is reused only when save_as is false. Every stored path comes from the user's choice in the save dialog, never from the URL the frontend sends. insert_or_assign replaces the old entry on "Save as", and DevToolsAppendToFile reads the same map.
Mock portal: answers are handed out oldest first, and when nothing is queued the portal still cancels, so existing dialog specs keep the old behaviour. Each new test clears leftover queued answers through defer.
New suite: it is tagged serial, it awaits and collects the save events before asserting instead of asserting inside listeners, and defer/waitUntil/ifdescribe are already imported in the spec file.

Extended reasoning...

The change fixes a dropped base::FindOrNull result in WebContents::DevToolsSaveToFile, treats an empty Linux dialog path as a cancel, and switches try_emplace to insert_or_assign. It also adds a way to queue a successful answer in the mock xdg-desktop-portal FileChooser, plus a Linux-only serial spec suite. The C++ change is about 10 lines in a handler that writes files for messages from the DevTools frontend, and the repo's review instructions class code in shell/browser/api as security-sensitive. File paths still come only from the user's dialog choice, so the change opens no new path for a renderer to pick where files are written. The decision to defer rests on that security-sensitive location and on the spec only being confirmed to fail on a nightly build, with no passing CI run yet.

dsanders11 · 2026-10-28T20:21:09Z

#55434 is stacked on this for cleaner separation (tests and release notes), but this stack should be merged together to ensure both bug fixes go in, since they have overlapping responsibilities.

WebContents::DevToolsSaveToFile looked up the path already chosen for a URL but discarded the result, so every DevTools "Save" showed the save dialog again. Assign the lookup result so a repeat save writes to the stored path. Let the mock xdg-desktop-portal FileChooser queue a successful response and add a spec that saves the same URL twice and expects one SaveFile request. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm

Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm

…lace the path Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm

…e As replace the path" Moving to a separate PR. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm

claude

I reviewed the latest push and didn't find any bugs in the test or mock changes. One thing for a human to confirm: the diff from the base is now test-only. The base commit already has path = base::FindOrNull(saved_files_, url) at shell/browser/api/electron_api_web_contents.cc:6155, but the title, description and release note still say this PR contains that C++ fix.

What was reviewed:

QueueResponse/ClearQueuedResponses in the FileChooser mock: queued URIs are used oldest-first and returned as uris. When the queue is empty, requests are still cancelled, so existing dialog specs behave the same.
The new serial-tagged spec: the temp dir and queued responses are cleaned up with defer(), and it uses waitUntil on the file contents rather than asserting inside a listener. dbus-native is already a spec dependency.

Extended reasoning...

Two files change: the python-dbusmock FileChooser portal template, which gains a response queue on the Mock interface, and one new Linux-only, serial-tagged spec in api-web-contents.spec.ts. No security-sensitive surface is touched. It is test infrastructure only. A human should still look because the C++ change the PR describes is already in the base commit, so the "fix:" title, the release note and the 44/45/46 backport labels don't match a diff that only adds a test. Earlier inline threads were about the C++ cancel path, which is not part of this diff.

release-clerk · 2026-10-29T21:02:59Z

Release Notes Persisted

Fixed DevTools "Save" prompting for a location every time instead of reusing the previously chosen file.

The one-line change from #55431 that assigns the base::FindOrNull lookup result to `path` in WebContents::DevToolsSaveToFile was dropped from that PR's final diff after a Chromium roll on main fixed the same line. This branch does not have that roll, so restore the assignment here; without it every DevTools "Save" shows the save dialog again. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkLV75VR3EtD7cwybpM

* fix: reuse saved path for repeat DevTools saves WebContents::DevToolsSaveToFile looked up the path already chosen for a URL but discarded the result, so every DevTools "Save" showed the save dialog again. Assign the lookup result so a repeat save writes to the stored path. Let the mock xdg-desktop-portal FileChooser queue a successful response and add a spec that saves the same URL twice and expects one SaveFile request. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm Co-authored-by: Claude <[email protected]> * test: clear queued portal mock responses after each test Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm Co-authored-by: Claude <[email protected]> * test: clear queued portal responses with defer Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm Co-authored-by: Claude <[email protected]> * fix: treat a cancelled DevTools save as cancelled and let Save As replace the path Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm Co-authored-by: Claude <[email protected]> * Revert "fix: treat a cancelled DevTools save as cancelled and let Save As replace the path" Moving to a separate PR. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm Co-authored-by: Claude <[email protected]> * fix: treat a cancelled DevTools save as cancelled and let Save As replace the path Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm Co-authored-by: Claude <[email protected]> * fix: restore saved path reuse for repeat DevTools saves from #55431 The one-line change from #55431 that assigns the base::FindOrNull lookup result to `path` in WebContents::DevToolsSaveToFile was dropped from that PR's final diff after a Chromium roll on main fixed the same line. This branch does not have that roll, so restore the assignment here; without it every DevTools "Save" shows the save dialog again. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkLV75VR3EtD7cwybpM --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Claude <[email protected]>

claude Bot added semver/patch backwards-compatible bug fixes target/44-x-y PR should also be added to the "44-x-y" branch. target/45-x-y PR should also be added to the "45-x-y" branch. target/46-x-y PR should also be added to the "46-x-y" branch. labels Oct 28, 2026

dsanders11 marked this pull request as ready for review October 28, 2026 19:36

claude Bot commented Oct 28, 2026

View reviewed changes

Comment thread shell/browser/api/electron_api_web_contents.cc Resolved

claude Bot commented Oct 28, 2026

View reviewed changes

claude Bot mentioned this pull request Oct 28, 2026

fix: handle cancelled and Save As DevTools saves correctly #55434

Merged

3 tasks done

claude Bot commented Oct 28, 2026

View reviewed changes

Comment thread shell/browser/api/electron_api_web_contents.cc Resolved

dsanders11 added this pull request to stack #55435 October 28, 2026 20:07

dsanders11 requested a review from ckerr October 28, 2026 20:26

claude added 5 commits October 29, 2026 12:24

test: clear queued portal mock responses after each test

64ae98f

Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm

test: clear queued portal responses with defer

4b2e9ea

Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm

fix: treat a cancelled DevTools save as cancelled and let Save As rep…

bd27d58

…lace the path Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KX1hkNiiBq2fQEsXWusxpm

dsanders11 force-pushed the claude/fix-devtools-save-reprompt branch from 4f3a465 to 86be31b Compare October 29, 2026 19:24

ckerr approved these changes Oct 29, 2026

View reviewed changes

claude Bot commented Oct 29, 2026

View reviewed changes

dsanders11 approved these changes Oct 29, 2026

View reviewed changes

dsanders11 merged commit 552effa into main Oct 29, 2026
72 checks passed

dsanders11 deleted the claude/fix-devtools-save-reprompt branch October 29, 2026 21:02

github-actions Bot added the 47-x-y label Oct 29, 2026

This was referenced Oct 29, 2026

fix: handle cancelled and Save As DevTools saves correctly #55484

Merged

fix: handle cancelled and Save As DevTools saves correctly #55485

Merged

fix: handle cancelled and Save As DevTools saves correctly #55486

Merged

trop Bot added in-flight/44-x-y in-flight/45-x-y labels Oct 29, 2026

trop Bot added in-flight/46-x-y and removed target/44-x-y PR should also be added to the "44-x-y" branch. target/45-x-y PR should also be added to the "45-x-y" branch. target/46-x-y PR should also be added to the "46-x-y" branch. labels Oct 29, 2026

trop Bot added merged/44-x-y PR was merged to the "44-x-y" branch. and removed in-flight/44-x-y labels Oct 30, 2026

trop Bot added merged/45-x-y PR was merged to the "45-x-y" branch. and removed in-flight/45-x-y labels Oct 30, 2026

trop Bot added merged/46-x-y PR was merged to the "46-x-y" branch. and removed in-flight/46-x-y labels Oct 30, 2026

Navigation Menu

Repository navigation

fix: reuse saved path for repeat DevTools saves - #55431

fix: reuse saved path for repeat DevTools saves#55431
dsanders11 merged 5 commits into
mainfrom
claude/fix-devtools-save-reprompt

claude Bot commented Oct 28, 2026 •

edited by dsanders11

Loading

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

dsanders11 commented Oct 28, 2026

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

release-clerk Bot commented Oct 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

claude Bot commented Oct 28, 2026 • edited by dsanders11 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Checklist

Release Notes

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsanders11 commented Oct 28, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

release-clerk Bot commented Oct 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

claude Bot commented Oct 28, 2026 •

edited by dsanders11

Loading