Repository navigation
perf: a new browser tab shows its toolbar on its first frame (perceived speed) - #21059
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Loading the New Tab page is quiet, like Chromium's: no Stop button and no progress line, so a new tab's chrome does not change over its first frames. A real page still shows its load. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adds ChromiumNewTabNow (not wired in yet) so the test can name its seam. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With Chromium running, a tab on the New Tab page needs nothing asynchronous, so its page and toolbar are made in the turn the pane first shows it instead of a main-actor turn later, which left one frame with the strip selected and no content. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Not verified: macOS jobs did not run: compile and app tests were skipped on CI passes on Written by |
|
Bead |
|
Found 1 test failure on Blacksmith runners: Failure
|
# Conflicts: # Packages/macOS/CmuxNext/Sources/CmuxNextApp/TabContentCache.swift
|
all web/size checks green |
… red Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s toward warm start A load the New Tab page starts names no address until it commits, so showsLoad hid Stop and the progress line for a slow site opened from a tile. The quiet load is now the tab's opening one only. The sync New Tab path also records the tab open like makeUserTab does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It grows CmuxNextApp, which the app size check refuses, and it needs its App-only configuration moved out of the App target first. This PR keeps the quiet New Tab load. The removed path is at 4d3cb99. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
[Medium impact] The PR appears safe to merge; both previous findings are addressed and no new blocking issue was found. SummaryNew Chromium tabs can show their toolbar immediately when Chromium already runs. The opening New Tab load stays quiet, while later loads still show Stop and progress.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Request browser tab] --> B{New Tab page and Chromium running?}
B -->|No| E[Use asynchronous creation]
B -->|Yes| C{Store checks allow immediate creation?}
C -->|No| E
C -->|Yes| D[Apply saved history]
D --> F[Create and install page now]
Reviews (4) · Last reviewed commit: "browser: a New Tab page claims a closed ..." · Reviewed by Greptile |
This reverts commit 9751560.
The synchronous New Tab page grows CmuxNextApp by 22 source lines, which the app size check refuses. ClosedBrowserHistories is self-contained browser history across closes, so it moves to CmuxNextBrowserHistory (Browser + Daemon, no UI). It is unchanged apart from the public surface the App uses. CmuxNextApp now has fewer source lines than its base. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| /// Makes the page when Chromium runs, nil while it starts (a seam for tests). | ||
| static var make: (CEFEngine, BrowserTabConfiguration) -> (any BrowserTab)? = { $0.makeTabIfRunning($1) } |
There was a problem hiding this comment.
ChromiumNewTabNow.make adds a mutable test hook to production source. ChromiumNewTabFirstFrameTests replaces it to pretend Chromium is running, changing tab creation for every cache in the process.
The repository forbids new test/debug seams under Sources/. Pass the factory through a scoped instance dependency instead. This requirement must be satisfied before merging.
Rule Used: Do not add new test/debug seams (ForTesting-style members, properties, or methods) to production source files under Sources/. Tests must reach internal state via @testable import instead. Existing occurrences are grandfathered but new ones are ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…for the later page ChromiumNewTabNow claims a reopened tab's closed back/forward history before its bail-outs, so when Chromium is still starting (or the tab is proxied or needs a remote-localhost store) the asynchronous path finds nothing to restore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
| #expect(ChromiumNewTabNow(cache: cache).page(tab, url: url) == nil) | ||
| let later = histories.restoring(BrowserTabConfiguration(id: BrowserTabID(rawValue: tab.id), profile: .default, initialURL: url), | ||
| url: url, local: true, tab: tab.id) | ||
| #expect(later.restoreState == .chromium("back-forward"), "the asynchronous path's page restores the closed tab's history") |
There was a problem hiding this comment.
The new test fails against the current implementation. ChromiumNewTabNow.page calls closedHistories.restoring before the mocked make returns nil. That removes the saved history, so the second restoring call returns no restoreState and this assertion fails.
Keep the regression test, but include the fix that leaves history unclaimed when no page is created.
…makes now ChromiumNewTabNow now checks that Chromium runs, the proxy plan and the remote-localhost store first and claims a reopened tab's closed history last, so every bail-out leaves it for the asynchronous path's page. The make seam moves from a process-wide static to TabContentCache.chromiumNow, an instance seam like makeCEFTab (REVIEW.md: no global test-only state). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review fixes (test first), from the correctness review of
Focused command (fleet):
Base reds on this PR, not touched by its files: the same six cmux-next checks fail on #22146, which changes only Observed / Not run
|
|
Shipped in:
|
A new browser tab (Cmd-Shift-L) didn't open settled. In recordings on feat c9c01bd (9 runs), the tab's chrome kept changing for up to 500 ms after it appeared:
newtabarrived a frame or more later. With Chromium already running,TabContentCache.browser(for:)still built the page in aTask, which runs on a later main-actor turn, so the pane got no content when it first showed the tab.Changes:
ChromiumNewTabNow). Its configuration matches the async path for a page that needs nothing async: a closed-tab history, the proxied store check and the remote-localhost guard. Any other address, a proxied or remote-localhost store, or a Chromium that's still starting keeps the async path.BrowserTabState.showsLoad). Chromium hides its throbber on its New Tab page the same way. A real page still shows Stop and the line while it loads.The tab strip is unchanged. Its tab icon still spins briefly on the New Tab page; that's Lawrence's area and is left as a follow-up.
Before / after
Measured with
scripts/cmux-next/perceived-speed-slot.sh --surface browser, 3 runs per build, on DEV builds of c9c01bd plus these commits. Current feat DEV builds crash at launch inLocalRouterClient.exchange(#20878).Each row is the harness median over its 3 runs. In 1 of 3 runs, the quiet-load-only build still showed the strip with an empty pane for one frame (0.99% area, 8 dropped). That's the race the second change removes. On the build with both changes, every run's first frame has the toolbar, the address bar and the Reload button in their final state. One run showed the sidebar row's old title for one frame (0.07%, 4 dropped). This PR doesn't change where that title comes from. Two of three runs had no change at all after the first frame.
Tests
Each regression test was committed before its fix and run on the fleet with
scripts/ci/run-swift-testing-suites.sh Packages/macOS/CmuxNext:NewTabQuietLoadTests: red on aaa5f03 (Stop label and progress line on achrome://newtab/and anabout:blankload; a real page still shows its load) in step 8ca017e40956a70e156866ed. Green on 45b13d4 in step 90854336e25cc8d31f6c440c.ChromiumNewTabFirstFrameTests: red on 69e85e5 (browser(for:)returned nil with Chromium running) in step 7ce377877734a41deb851e92. Green on 5e22c90 in step 9755e88ac26ff388ccb7ce3c.The only other failure in those runs is
ClassicShortcutsUpgradeTests, a known failure on the base.Local checks passed:
check-app-size.sh --base origin/feat-cmux-next,check-concurrency.sh,check-no-godfiles.sh --only swift --base origin/feat-cmux-next(TabContentCacheandBrowserChromeViewdidn't grow),crash_ratchet.py, andlint-ios-package-conventions.sh(its only ERROR is the known one on the base,BuiltInAdBlockerSetting).Release compile (
check-release-compile.sh): passed on 45b13d4 in step ca8191ae848a800251193e21. Passed on the head 5e22c90 in step 896512db1605ca7ef185ef56.Follow-up (lane takeover, 2026-10-19)
ClosedBrowserHistories, which is self-contained browser history kept across closes, moves unchanged from CmuxNextApp toCmuxNextBrowserHistory(Browser + Daemon, no UI). Only its public surface changed. CmuxNextApp now has 83,214 source lines against 83,246 on its base (scripts/cmux-next/count-swift-code-lines.py).showsLoadhid Stop and the progress line until a slow site committed. Now only the tab's opening load is quiet (BrowserTabState.hasEndedLoad).NewTabQuietLoadTests.aPageOpenedFromTheNewTabPageShowsItsLoadBeforeItCommitswas committed red at f968390.noteTabOpened) the same waymakeUserTabdoes.Swift tests run in CI only. Nothing was built locally.
Changelog
🤖 Generated with Claude Code