browser: isolate favicon state by document - #336077
Draft
Dmitriy Vasyura (dmitrivMS) wants to merge 3 commits into
Draft
Dmitriy Vasyura (dmitrivMS) wants to merge 3 commits into
Dmitriy Vasyura (dmitrivMS) wants to merge 3 commits into
Conversation
Keep the committed favicon separate from pending navigation candidates. Promote candidates at commit, discard them on abort or failure, and reject superseded asynchronous requests without rewriting prior history. Preserve same-document and replacement history, and cancel favicon work on native destruction. Cover the state transitions with focused unit tests and a browser API/CDP navigation scenario. Separates favicon correctness from #335987 without introducing its snapshot protocol or the native-close fix in #336075. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Dmitriy Vasyura (dmitrivMS)
September 13, 2026 22:53
View session
Dmitriy Vasyura (dmitrivMS)
added
vscode-build
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Cancelling a pinned redirect leaves provisional favicon state active because subsequent cleanup events are suppressed.
Get a fresh assessment by requesting another Copilot review.
Review tier: Balanced
Findings: 1
Open findings (1)
What changed in this PR
Separates committed and provisional favicon state for integrated browser navigations.
Changes:
- Adds a document-aware favicon state manager.
- Integrates favicon lifecycle handling with navigation and history.
- Adds unit and browser API coverage.
| File | Description |
|---|---|
browserFavicon.ts |
Implements favicon state isolation. |
browserView.ts |
Connects favicon state to Electron navigation. |
browserFavicon.test.ts |
Covers favicon transitions and races. |
browser.cdp.test.ts |
Tests favicon behavior through CDP. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Dmitriy Vasyura (dmitrivMS)
added
the
browser-integration
Open an empty browser and attach CDP before navigating the local fixture. This keeps the favicon navigation test from depending on the separate initial URL/subscription handoff race that failed Linux CI. Preserve all URL and icon assertions, history checks, and existing timeouts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Invalidate favicon work only on document replacement, failure or disposal. Reacquire current-document candidates when Electron suppresses an unchanged URL set, while retaining native updates as the fast path. Cover outgoing-page updates and requests, shared favicon URLs, navigation history, and native candidate filtering. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Screenshot ChangesBase: Changed (1)
|
| Before | After |
|---|---|
 |
 |
2 insignificant change(s) omitted (≤20 px, Δ≤2). See CI logs for details.
Collaborator
Author
|
Validation follow-up for 6f0baf8:
The separate popup-title handoff and raw-close cleanup remain outside this PR. Setup failures were retained separately rather than counted as favicon regressions or passing runs. The raw-close exception still belongs to #336075. |
|
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
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.
Keep favicon work with its committed native document until that document is actually replaced.
Favicon-only extraction from #335987, redesigned rather than copied. Based directly on
main; no popup snapshot/version protocol or native-close cleanup from #336075.