perf(desktop): keep the shell mounted under settings, cheaper mounts - #91362
Merged
trunk-io[bot] merged 13 commits intoAug 30, 2026
Conversation
Contributor
|
React Doctor found 2 issues in 2 files · 2 warnings. 2 warnings
Reviewed by React Doctor for commit |
charlesvien
marked this pull request as ready for review
August 30, 2026 10:32
Contributor
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. |
Contributor
Prompt To Fix All With AI### Issue 1
products/desktop/packages/ui/src/router/routes/_shell/settings/$category.tsx:11
**Settings route state stays false**
When a tour is active and the user opens settings, the new `/_shell/settings/$category` route ID does not satisfy `useIsSettingsOpen()`'s unchanged `startsWith("/settings")` check. `TourOverlay` therefore remains active over the full-window settings overlay, obscuring settings controls or advancing the tour against hidden shell content.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Cut mount work in chat thread and tab pa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Settings routes moved under the pathless `_shell` layout, so their route IDs
now read `/_shell/settings/…`. `useIsSettingsOpen` still matched on
`startsWith("/settings")` and went permanently false, which left the tour
spotlight drawn over the settings overlay.
Share one `isSettingsRouteId` predicate across the three call sites instead of
carrying a copy of the substring in each.
Generated-By: PostHog Desktop
Task-Id: d3e0d459-5a9e-4627-866b-3a6fac9677b5
Generated-By: PostHog Desktop Task-Id: d3e0d459-5a9e-4627-866b-3a6fac9677b5
puemos
approved these changes
Aug 30, 2026
charlesvien
changed the title
charlesvien
added
reviewhog
Contributor
🦔 PostHog Review reviewed this pull requestFound 1 must fix, 6 should fix, 0 consider. Published 7 findings (view the review). Resolved comments: 4 fixed, 3 left for you |
Contributor
|
Reviews (2): Last reviewed commit: "Cut sidebar work on settings navigation" | Re-trigger Greptile |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Desktop-app-only performance refactor (mounting/memoization, no auth/billing/data-model/CI/dependency surface); the one substantive bug a bot reviewer flagged (settings route-ID matching breaking tour-overlay state) is already fixed consistently across the diff, and the author is a strong-familiarity member of the owning team.
- Author wrote 82% of the modified lines and has 15 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 450L, 20F substantive, 557L/24F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (557L, 24F, single-area, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 232ba24 · reviewed head 232ba24 |
Contributor
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
The bare QueryObserver for the workspaces map never tracks accessed fields the way useQuery does, so it notified every subscriber on any result change (fetchStatus flips, dataUpdatedAt bumps on refetch). All hooks in this file read only `data` and `isFetched`, so set notifyOnChangeProps to those two fields. Generated-By: PostHog Desktop Task-Id: 55c57757-4318-4446-8726-aa6d926d6a61
stamphog
Bot
dismissed
their stale review
August 30, 2026 12:07
A new stamphog review started for this PR — the fresh verdict replaces this approval.
|
A bot reviewer flagged a confirmed must-fix regression (the settings view type becomes unreachable after the route restructuring) plus six should-fix regressions (global banners disappearing, saved navigation state getting overwritten, a modal-dialog/settings-overlay conflict, and lost keyboard accessibility on turn actions) - all 7 threads are still unresolved per the review's own tracker.
Gate mechanics and policy version
|
ShellLayout and useActiveSession read the whole route param set via
useParams({ strict: false }). The nearest match carries the full param
chain, so switching a settings category changed `category` and
re-rendered the shell layout and every useActiveSession consumer (the
right panel included), even though none read `category`.
Select each param on its own instead. The router uses strict reference
equality on the selector result (no default structural sharing), so
selecting single primitives skips the re-render when only an unrelated
param changes; an object selector would return a new reference each time
and defeat that.
Update the useParams mocks in the two affected tests to honor `select`,
matching the real hook and the mocks already updated elsewhere in this
change.
Generated-By: PostHog Desktop
Task-Id: 55c57757-4318-4446-8726-aa6d926d6a61
deriveFromMatches switches on a match's fullPath, but the settings cases used the route-id form (/_shell/settings/$category, /_shell/settings/). The pathless _shell layout lives only in a route's id, never its fullPath — the generated tree assigns these routes fullPath /settings/$category and /settings/ — so those cases never matched. Settings fell through to the task-input view, making the `settings` AppView unreachable and mislabeling the settings route for every useAppView / getAppViewSnapshot consumer (SpaceSwitcher's isOnNewTask, the New-task sidebar highlight, and the task-input prefill merge). Use the fullPath form, matching the neighboring /spaces/context case. Add a regression test for the previously uncovered mapping. Generated-By: PostHog Desktop Task-Id: 55c57757-4318-4446-8726-aa6d926d6a61
The tab strip now stays mounted under the Settings overlay, so its settled-navigation effect runs on settings navigations. Settings is an unclaimed route, which railPaneForPath classifies as the spaces pane, so the effect wrote the settings href into the active tab's viewState.lastByPane.spaces. After leaving Settings, a Spaces rail click restored that href and reopened Settings, losing the tab's real last spaces location. Preserve the existing lastByPane map when the route is settings. The guard reads routeAppView === "settings", which now resolves correctly for the settings route. This restores the pre-mount behavior, where the strip never ran on settings at all. Generated-By: PostHog Desktop Task-Id: 55c57757-4318-4446-8726-aa6d926d6a61
Two things the shell staying mounted under settings took away. The connectivity and announcement banners now sit under the settings overlay and inside the inert subtree, so losing connectivity in settings hid both the offline state and its Retry. They render inside the overlay while it is up, and the shell drops its copies for that span so an announcement is not reported as seen twice. The turn footer mounted its copy and rating buttons on pointer hover only, and keyboard message navigation never marks an agent turn, so those controls left the DOM for anyone not using a pointer. The copy button stays mounted, a row reveals its footer on focus as well as hover, and the thumbs mount from there. Also reformats a ShellLayout line the formatter was failing on. Generated-By: PostHog Desktop Task-Id: fec4ff4f-19c1-4dfe-a58a-567e777c7365
Generated-By: PostHog Desktop Task-Id: fec4ff4f-19c1-4dfe-a58a-567e777c7365
|
😎 Stack merged successfully - details. |
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
2 participants
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
Stacked on #91360. Still slow after it:
/settingssat outside the_shelllayout. Packaged build: 516 to 607ms frozen per click.Changes
Not changed: the composer's
editableflip while connecting; removing it would let people type before the agent connects.Dev build, same state before and after (6 tabs open). The packaged sidebar is 4x larger and was not re-measured.
How did you test this code?
useAppView.test.tslocks the route-to-view mapping. It catches a settings case written in route-id form, which the generated tree never produces as afullPathand which silently made Settings report itself as the new-task view.ChannelsList.test.tsx,ChannelsSidebar.test.tsx,ShellLayout.test.tsxanduseReviewInRightPanel.test.tsnow honorselect.