fix(web): stop project-scope menu from highlighting first row without hover - #7950

Closed
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus
Closed

fix(web): stop project-scope menu from highlighting first row without hover#7950
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus

Conversation

@IzonIcy

@IzonIcyIzonIcy commented Aug 23, 2026

Copy link
Copy Markdown

What changed

apps/web/src/components/Sidebar.tsx: when the sidebar project-scope menu ("All projects" dropdown) is opened with a pointer, focus is redirected to the popup element itself during commit. Base UI's open-time autofocus — which queues a microtask and skips when focus already sits inside the popup — then no-ops instead of landing on the first tabbable element, the settings gear inside the first project row.

Keyboard opens keep Base UI's default behavior (the checked row is pre-highlighted and receives focus itself). Arrow-key navigation, Tab-to-gear, and mouse clicks on the gear are unchanged; only the misdirected open focus moved.

Why

Fixes#7915: opening this menu with the mouse rendered the first project row as highlighted although the pointer was elsewhere, because every row is tabIndex=-1 until highlighted and focus bubbling into the gear marked its row active.

Evidence

Menu opened with the pointer, nothing hovered.

Before — the t3-demo-project row renders highlighted because Base UI's open autofocus landed on its settings gear:

before: project row highlighted without hover

After — focus lands on the popup itself; the project row stays neutral (the checked "All projects" row keeps its selection background, as intended):

after: project row neutral on pointer open

Images live on the fork's pr-evidence/7950 branch so no PR-only assets land in the repo.

Verification

  • Scoped typecheck clean (tsgo --noEmit, 0 errors); lint clean
  • Focus-redirect mechanics verified against Base UI 1.5.0 internals (FloatingFocusManager skips its queued autofocus when focus is already inside the floating element)

--
Worked by ox-alpha via opencode (x-preview-f-free).

Note

Fix project-scope menu highlighting first row when opened by pointer

  • Adds a projectScopeOpenedWithPointerRef in Sidebar.tsx set via onPointerDown/onKeyDown on the menu trigger, distinguishing pointer opens from keyboard opens
  • Adds a FocusPopupOnMount component and optional focusOnMountRef prop to MenuPopup in menu.tsx; when enabled, the popup element itself receives focus on mount instead of the first tabbable descendant
  • When the project-scope menu is opened with a pointer, the popup is focused to avoid pre-highlighting the first row; keyboard opens retain the default focus behavior

Macroscope summarized 3a0f2aa.


Note

Low Risk
Localized focus-management workaround for one menu; keyboard behavior and other menus are unchanged unless they opt into focusOnMountRef.

Overview
Fixes incorrect row highlight when the sidebar All projects menu opens via click: Base UI autofocus was landing on the first row’s nested settings gear instead of leaving rows neutral.

MenuPopup gains an optional focusOnMountRef and a FocusPopupOnMount layout effect that focuses the popup container (with preventScroll) when the ref is true at mount, so Base UI’s focus manager skips autofocus to the first tabbable descendant.

Sidebar records pointer vs keyboard on the project-scope trigger (onPointerDown / onKeyDown) and passes that ref into MenuPopup so pointer opens redirect focus to the popup; keyboard opens keep the default pre-highlighted checked row.

Reviewed by Cursor Bugbot for commit 3a0f2aa. Bugbot is set up for automated code reviews on this repo. Configure here.

… hover
Base UI focuses the first tabbable element when a menu opens with the
mouse. Inside the sidebar project-scope menu every row is tabIndex=-1
until highlighted, so the first tabbable element was the settings gear
inside the first project row; focus bubbling into that row made Base UI
render it as highlighted although the pointer was elsewhere. Keyboard
opens were unaffected because the checked row is pre-highlighted and
receives focus itself.
When the trigger is activated by pointer, focus the popup element during
commit instead: Base UI's queued open-time autofocus sees focus is
already inside the popup and skips. Keyboard opens keep the default
behavior; arrow keys, Tab-to-gear, and mouse clicks on the gear still
work as before.
@coderabbitai

coderabbitaiBot commented Aug 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ad38b52-28e7-4308-a0c5-3cdefa322497

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 23, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding: the open-focus redirect is a component-local workaround that depends on Base UI's internal focus timing, while the actual cause (a tabbable Button nested inside a menu radio item) is left in place. Details inline.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
The Sidebar-level layout effect raced Base UI's FloatingFocusManager:
it only helps if the popup mounts in the same commit, and the manager's
autofocus decision can still land on the settings gear otherwise.
Move the redirect into the popup subtree as a sentinel component. React
runs child layout effects before the popup's own effect, so the manager
snapshots activeElement already inside the popup and skips its queued
autofocus deterministically, regardless of mount timing. Behavior is
unchanged for keyboard opens (sentinel not mounted) and for arrow keys,
Tab-to-gear, and gear clicks.
@IzonIcy

Copy link
Copy Markdown
Author

Fixed in 437a995 — the timing critique was fair, so the redirect moved from a Sidebar-level layout effect into the popup's own subtree: a hidden sentinel child focuses [data-slot='menu-popup'] during its layout effect, which React runs before the popup component's FloatingFocusManager effect (child-first). The manager therefore snapshots activeElement already inside the floating element and skips its queued autofocus outright — no dependency on which commit the popup mounts in, and no reliance on the enqueueFocus shouldFocus re-check. The sentinel only mounts for pointer-opened menus; keyboard opens keep Base UI's default pre-highlighted-row focus.

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the project-scope menu focus fix against the shared Menu primitive contract and Base UI 1.4.1 behavior. The layout-effect ordering the sentinel relies on does hold in 1.4.1 (FloatingFocusManager snapshots activeElement synchronously in its own layout effect and bails when focus is already inside the popup), so the mechanism itself is sound. Three smaller issues remain: a render-time ref read that the React Compiler rejects, a popup focus() call that does not mirror Base UI's preventScroll, and ownership of the behavior sitting at one call site while an identical menu composition exists elsewhere.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
@IzonIcy

Copy link
Copy Markdown
Author

Before (main): opening the project-scope menu highlights the first project row even though the pointer never left the trigger.

t3-pr7950-before-bug

After (fix branch): identical interaction — no row is highlighted until the pointer or keyboard actually touches one.

t3-pr7950-after-fix

Both captured on an isolated dev environment — same machine, same seeded project, menu opened with a mouse click each time. For extra rigor: on main, DOM inspection confirms the project row ends up data-highlighted=true with focus stolen from the trigger; with the fix, focus stays on the popup element and no item receives data-highlighted. Keyboard opens are unchanged.

@IzonIcy
IzonIcy marked this pull request as ready for review August 23, 2026 02:17
CopilotAI lite review requested due to automatic review settings August 23, 2026 02:17

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 437a995. Configure here.

Comment threadapps/web/src/components/Sidebar.tsx
@macroscopeapp

macroscopeappBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a focused, opt-in UI focus correction limited to the project-scope menu, with keyboard behavior and other menus preserved. An unresolved Medium finding identifies a popup-ref merging risk, which remains a separate correctness gate under the repository’s configured threshold.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

Addresses the three Macroscope UI-consistency findings on this PR:
- The sentinel no longer reaches through MenuPopup's internal DOM via
closest(). MenuPopup now owns a ref to its own popup element and
exposes an opt-in focusOnMountRef prop; the FocusPopupOnMount helper
lives beside it so the Base UI ordering workaround is documented in
one place.
- The mount focus now passes preventScroll:true, matching Base UI's own
open-focus behavior while the popup is still unpositioned.
- projectScopeOpenedWithPointerRef.current is no longer read during
render. The ref object is passed down and read inside the layout
effect, keeping the component React Compiler-safe.
Verified: pnpm exec tsgo --noEmit, vp lint on changed files, menu +
sidebar unit tests.
Model: ox-alpha (opencode/x-preview-f-free), opencode
className,
)}
data-slot="menu-popup"
ref={popupRef}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumui/menu.tsx:103

When a caller supplies ref, {...props} overwrites popupRef, so FocusPopupOnMount sees popupRef.current === null and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ui/menu.tsx around line 103:
When a caller supplies `ref`, `{...props}` overwrites `popupRef`, so `FocusPopupOnMount` sees `popupRef.current === null` and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.
Evidence trail:
3fe9398
apps/web/src/components/ui/menu.tsx:25-40
apps/web/src/components/ui/menu.tsx:54-108
apps/web/package.json:15,42

@IzonIcy

Copy link
Copy Markdown
Author

Attaching the before/after captures promised in the description (hosted on the fork's pr-evidence/7950 branch to keep PR-only assets out of the repo):

Before — pointer open, no hover: t3-demo-project row wrongly highlighted (autofocus landed on its gear):

before

After — pointer open: focus lands on the popup, project row neutral:

after

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings in the shared MenuPopup primitive. The behavior goal (no pre-highlighted row on pointer opens) looks right; the concern is where the behavior lives and how it is wired.

Posted via Macroscope — UI Consistency

Comment on lines +25 to +39
function FocusPopupOnMount({
enabled,
popupRef,
}: {
enabled: { current: boolean };
popupRef: { current: HTMLElement | null };
}) {
useLayoutEffect(() => {
if (!enabled.current) {
return;
}
// preventScroll matches Base UI's own open-focus behavior: this runs
// before the positioner has positioned the popup, so a scrolling focus
// could jump ancestor scrollers.
popupRef.current?.focus({ preventScroll: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts popup focus ownership in a hidden child whose correctness depends on React running its layout effect before Base UI's focus manager, and it pushes interaction-type detection out to every consumer (Sidebar.tsx now tracks onPointerDown/onKeyDown on the trigger by hand). Base UI's popups already own both halves of this: Menu.Popup accepts initialFocus — the sibling of the finalFocus prop this repo already uses on a Base UI popup in CommandPalette.tsx:535 — as a ref or a callback that receives the open interaction type, and the focus manager consults it instead of being raced by effect ordering.

Suggest replacing FocusPopupOnMount + focusOnMountRef with initialFocus on MenuPrimitive.Popup, returning popupRef.current for non-keyboard opens and falling through to the default for keyboard opens (verify the exact callback return contract with a typecheck against @base-ui/react@1.5.0). That keeps the fix declarative, survives Base UI upgrades that change when the focus manager runs, and lets MenuPopup apply it by default — the same nested-control-in-first-row pattern exists at ProjectScriptsControl.tsx:214, which the current opt-in ref prop does not cover.

No diff: the fix spans the prop signature and the Sidebar trigger handlers.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/ui/menu.tsx
@t3dotgg

Copy link
Copy Markdown
Member

Note

🤖 GPT-5.6 Sol responding on behalf of Theo

We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together.

Closing this alternate menu focus policy. The same project-menu highlight is covered by #7916. This version adds a shared layout-effect focus API and relies on open-time scheduling details. Keep the pointer-versus-keyboard reproduction in the selected fix and use the menu primitive's supported initial-focus behavior.

If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. If GitHub does not let you reopen it, leave a comment here and we'll take another look.

@t3dotggt3dotgg closed this Aug 27, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M30-99 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar project menu highlights the first project on open without hover

3 participants

@IzonIcy@t3dotgg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(web): stop project-scope menu from highlighting first row without hover - #7950

Closed
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus
Closed

fix(web): stop project-scope menu from highlighting first row without hover#7950
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus

Conversation

@IzonIcy

@IzonIcyIzonIcy commented Aug 23, 2026

Copy link
Copy Markdown

What changed

apps/web/src/components/Sidebar.tsx: when the sidebar project-scope menu ("All projects" dropdown) is opened with a pointer, focus is redirected to the popup element itself during commit. Base UI's open-time autofocus — which queues a microtask and skips when focus already sits inside the popup — then no-ops instead of landing on the first tabbable element, the settings gear inside the first project row.

Keyboard opens keep Base UI's default behavior (the checked row is pre-highlighted and receives focus itself). Arrow-key navigation, Tab-to-gear, and mouse clicks on the gear are unchanged; only the misdirected open focus moved.

Why

Fixes#7915: opening this menu with the mouse rendered the first project row as highlighted although the pointer was elsewhere, because every row is tabIndex=-1 until highlighted and focus bubbling into the gear marked its row active.

Evidence

Menu opened with the pointer, nothing hovered.

Before — the t3-demo-project row renders highlighted because Base UI's open autofocus landed on its settings gear:

before: project row highlighted without hover

After — focus lands on the popup itself; the project row stays neutral (the checked "All projects" row keeps its selection background, as intended):

after: project row neutral on pointer open

Images live on the fork's pr-evidence/7950 branch so no PR-only assets land in the repo.

Verification

  • Scoped typecheck clean (tsgo --noEmit, 0 errors); lint clean
  • Focus-redirect mechanics verified against Base UI 1.5.0 internals (FloatingFocusManager skips its queued autofocus when focus is already inside the floating element)

--
Worked by ox-alpha via opencode (x-preview-f-free).

Note

Fix project-scope menu highlighting first row when opened by pointer

  • Adds a projectScopeOpenedWithPointerRef in Sidebar.tsx set via onPointerDown/onKeyDown on the menu trigger, distinguishing pointer opens from keyboard opens
  • Adds a FocusPopupOnMount component and optional focusOnMountRef prop to MenuPopup in menu.tsx; when enabled, the popup element itself receives focus on mount instead of the first tabbable descendant
  • When the project-scope menu is opened with a pointer, the popup is focused to avoid pre-highlighting the first row; keyboard opens retain the default focus behavior

Macroscope summarized 3a0f2aa.


Note

Low Risk
Localized focus-management workaround for one menu; keyboard behavior and other menus are unchanged unless they opt into focusOnMountRef.

Overview
Fixes incorrect row highlight when the sidebar All projects menu opens via click: Base UI autofocus was landing on the first row’s nested settings gear instead of leaving rows neutral.

MenuPopup gains an optional focusOnMountRef and a FocusPopupOnMount layout effect that focuses the popup container (with preventScroll) when the ref is true at mount, so Base UI’s focus manager skips autofocus to the first tabbable descendant.

Sidebar records pointer vs keyboard on the project-scope trigger (onPointerDown / onKeyDown) and passes that ref into MenuPopup so pointer opens redirect focus to the popup; keyboard opens keep the default pre-highlighted checked row.

Reviewed by Cursor Bugbot for commit 3a0f2aa. Bugbot is set up for automated code reviews on this repo. Configure here.

… hover
Base UI focuses the first tabbable element when a menu opens with the
mouse. Inside the sidebar project-scope menu every row is tabIndex=-1
until highlighted, so the first tabbable element was the settings gear
inside the first project row; focus bubbling into that row made Base UI
render it as highlighted although the pointer was elsewhere. Keyboard
opens were unaffected because the checked row is pre-highlighted and
receives focus itself.
When the trigger is activated by pointer, focus the popup element during
commit instead: Base UI's queued open-time autofocus sees focus is
already inside the popup and skips. Keyboard opens keep the default
behavior; arrow keys, Tab-to-gear, and mouse clicks on the gear still
work as before.
@coderabbitai

coderabbitaiBot commented Aug 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ad38b52-28e7-4308-a0c5-3cdefa322497

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 23, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding: the open-focus redirect is a component-local workaround that depends on Base UI's internal focus timing, while the actual cause (a tabbable Button nested inside a menu radio item) is left in place. Details inline.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
The Sidebar-level layout effect raced Base UI's FloatingFocusManager:
it only helps if the popup mounts in the same commit, and the manager's
autofocus decision can still land on the settings gear otherwise.
Move the redirect into the popup subtree as a sentinel component. React
runs child layout effects before the popup's own effect, so the manager
snapshots activeElement already inside the popup and skips its queued
autofocus deterministically, regardless of mount timing. Behavior is
unchanged for keyboard opens (sentinel not mounted) and for arrow keys,
Tab-to-gear, and gear clicks.
@IzonIcy

Copy link
Copy Markdown
Author

Fixed in 437a995 — the timing critique was fair, so the redirect moved from a Sidebar-level layout effect into the popup's own subtree: a hidden sentinel child focuses [data-slot='menu-popup'] during its layout effect, which React runs before the popup component's FloatingFocusManager effect (child-first). The manager therefore snapshots activeElement already inside the floating element and skips its queued autofocus outright — no dependency on which commit the popup mounts in, and no reliance on the enqueueFocus shouldFocus re-check. The sentinel only mounts for pointer-opened menus; keyboard opens keep Base UI's default pre-highlighted-row focus.

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the project-scope menu focus fix against the shared Menu primitive contract and Base UI 1.4.1 behavior. The layout-effect ordering the sentinel relies on does hold in 1.4.1 (FloatingFocusManager snapshots activeElement synchronously in its own layout effect and bails when focus is already inside the popup), so the mechanism itself is sound. Three smaller issues remain: a render-time ref read that the React Compiler rejects, a popup focus() call that does not mirror Base UI's preventScroll, and ownership of the behavior sitting at one call site while an identical menu composition exists elsewhere.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
@IzonIcy

Copy link
Copy Markdown
Author

Before (main): opening the project-scope menu highlights the first project row even though the pointer never left the trigger.

t3-pr7950-before-bug

After (fix branch): identical interaction — no row is highlighted until the pointer or keyboard actually touches one.

t3-pr7950-after-fix

Both captured on an isolated dev environment — same machine, same seeded project, menu opened with a mouse click each time. For extra rigor: on main, DOM inspection confirms the project row ends up data-highlighted=true with focus stolen from the trigger; with the fix, focus stays on the popup element and no item receives data-highlighted. Keyboard opens are unchanged.

@IzonIcy
IzonIcy marked this pull request as ready for review August 23, 2026 02:17
CopilotAI lite review requested due to automatic review settings August 23, 2026 02:17

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 437a995. Configure here.

Comment threadapps/web/src/components/Sidebar.tsx
@macroscopeapp

macroscopeappBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a focused, opt-in UI focus correction limited to the project-scope menu, with keyboard behavior and other menus preserved. An unresolved Medium finding identifies a popup-ref merging risk, which remains a separate correctness gate under the repository’s configured threshold.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

Addresses the three Macroscope UI-consistency findings on this PR:
- The sentinel no longer reaches through MenuPopup's internal DOM via
closest(). MenuPopup now owns a ref to its own popup element and
exposes an opt-in focusOnMountRef prop; the FocusPopupOnMount helper
lives beside it so the Base UI ordering workaround is documented in
one place.
- The mount focus now passes preventScroll:true, matching Base UI's own
open-focus behavior while the popup is still unpositioned.
- projectScopeOpenedWithPointerRef.current is no longer read during
render. The ref object is passed down and read inside the layout
effect, keeping the component React Compiler-safe.
Verified: pnpm exec tsgo --noEmit, vp lint on changed files, menu +
sidebar unit tests.
Model: ox-alpha (opencode/x-preview-f-free), opencode
className,
)}
data-slot="menu-popup"
ref={popupRef}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumui/menu.tsx:103

When a caller supplies ref, {...props} overwrites popupRef, so FocusPopupOnMount sees popupRef.current === null and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ui/menu.tsx around line 103:
When a caller supplies `ref`, `{...props}` overwrites `popupRef`, so `FocusPopupOnMount` sees `popupRef.current === null` and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.
Evidence trail:
3fe9398
apps/web/src/components/ui/menu.tsx:25-40
apps/web/src/components/ui/menu.tsx:54-108
apps/web/package.json:15,42

@IzonIcy

Copy link
Copy Markdown
Author

Attaching the before/after captures promised in the description (hosted on the fork's pr-evidence/7950 branch to keep PR-only assets out of the repo):

Before — pointer open, no hover: t3-demo-project row wrongly highlighted (autofocus landed on its gear):

before

After — pointer open: focus lands on the popup, project row neutral:

after

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings in the shared MenuPopup primitive. The behavior goal (no pre-highlighted row on pointer opens) looks right; the concern is where the behavior lives and how it is wired.

Posted via Macroscope — UI Consistency

Comment on lines +25 to +39
function FocusPopupOnMount({
enabled,
popupRef,
}: {
enabled: { current: boolean };
popupRef: { current: HTMLElement | null };
}) {
useLayoutEffect(() => {
if (!enabled.current) {
return;
}
// preventScroll matches Base UI's own open-focus behavior: this runs
// before the positioner has positioned the popup, so a scrolling focus
// could jump ancestor scrollers.
popupRef.current?.focus({ preventScroll: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts popup focus ownership in a hidden child whose correctness depends on React running its layout effect before Base UI's focus manager, and it pushes interaction-type detection out to every consumer (Sidebar.tsx now tracks onPointerDown/onKeyDown on the trigger by hand). Base UI's popups already own both halves of this: Menu.Popup accepts initialFocus — the sibling of the finalFocus prop this repo already uses on a Base UI popup in CommandPalette.tsx:535 — as a ref or a callback that receives the open interaction type, and the focus manager consults it instead of being raced by effect ordering.

Suggest replacing FocusPopupOnMount + focusOnMountRef with initialFocus on MenuPrimitive.Popup, returning popupRef.current for non-keyboard opens and falling through to the default for keyboard opens (verify the exact callback return contract with a typecheck against @base-ui/react@1.5.0). That keeps the fix declarative, survives Base UI upgrades that change when the focus manager runs, and lets MenuPopup apply it by default — the same nested-control-in-first-row pattern exists at ProjectScriptsControl.tsx:214, which the current opt-in ref prop does not cover.

No diff: the fix spans the prop signature and the Sidebar trigger handlers.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/ui/menu.tsx
@t3dotgg

Copy link
Copy Markdown
Member

Note

🤖 GPT-5.6 Sol responding on behalf of Theo

We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together.

Closing this alternate menu focus policy. The same project-menu highlight is covered by #7916. This version adds a shared layout-effect focus API and relies on open-time scheduling details. Keep the pointer-versus-keyboard reproduction in the selected fix and use the menu primitive's supported initial-focus behavior.

If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. If GitHub does not let you reopen it, leave a comment here and we'll take another look.

@t3dotggt3dotgg closed this Aug 27, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M30-99 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar project menu highlights the first project on open without hover

3 participants

@IzonIcy@t3dotgg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(web): stop project-scope menu from highlighting first row without hover - #7950

Closed
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus
Closed

fix(web): stop project-scope menu from highlighting first row without hover#7950
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus

Conversation

@IzonIcy

@IzonIcyIzonIcy commented Aug 23, 2026

Copy link
Copy Markdown

What changed

apps/web/src/components/Sidebar.tsx: when the sidebar project-scope menu ("All projects" dropdown) is opened with a pointer, focus is redirected to the popup element itself during commit. Base UI's open-time autofocus — which queues a microtask and skips when focus already sits inside the popup — then no-ops instead of landing on the first tabbable element, the settings gear inside the first project row.

Keyboard opens keep Base UI's default behavior (the checked row is pre-highlighted and receives focus itself). Arrow-key navigation, Tab-to-gear, and mouse clicks on the gear are unchanged; only the misdirected open focus moved.

Why

Fixes#7915: opening this menu with the mouse rendered the first project row as highlighted although the pointer was elsewhere, because every row is tabIndex=-1 until highlighted and focus bubbling into the gear marked its row active.

Evidence

Menu opened with the pointer, nothing hovered.

Before — the t3-demo-project row renders highlighted because Base UI's open autofocus landed on its settings gear:

before: project row highlighted without hover

After — focus lands on the popup itself; the project row stays neutral (the checked "All projects" row keeps its selection background, as intended):

after: project row neutral on pointer open

Images live on the fork's pr-evidence/7950 branch so no PR-only assets land in the repo.

Verification

  • Scoped typecheck clean (tsgo --noEmit, 0 errors); lint clean
  • Focus-redirect mechanics verified against Base UI 1.5.0 internals (FloatingFocusManager skips its queued autofocus when focus is already inside the floating element)

--
Worked by ox-alpha via opencode (x-preview-f-free).

Note

Fix project-scope menu highlighting first row when opened by pointer

  • Adds a projectScopeOpenedWithPointerRef in Sidebar.tsx set via onPointerDown/onKeyDown on the menu trigger, distinguishing pointer opens from keyboard opens
  • Adds a FocusPopupOnMount component and optional focusOnMountRef prop to MenuPopup in menu.tsx; when enabled, the popup element itself receives focus on mount instead of the first tabbable descendant
  • When the project-scope menu is opened with a pointer, the popup is focused to avoid pre-highlighting the first row; keyboard opens retain the default focus behavior

Macroscope summarized 3a0f2aa.


Note

Low Risk
Localized focus-management workaround for one menu; keyboard behavior and other menus are unchanged unless they opt into focusOnMountRef.

Overview
Fixes incorrect row highlight when the sidebar All projects menu opens via click: Base UI autofocus was landing on the first row’s nested settings gear instead of leaving rows neutral.

MenuPopup gains an optional focusOnMountRef and a FocusPopupOnMount layout effect that focuses the popup container (with preventScroll) when the ref is true at mount, so Base UI’s focus manager skips autofocus to the first tabbable descendant.

Sidebar records pointer vs keyboard on the project-scope trigger (onPointerDown / onKeyDown) and passes that ref into MenuPopup so pointer opens redirect focus to the popup; keyboard opens keep the default pre-highlighted checked row.

Reviewed by Cursor Bugbot for commit 3a0f2aa. Bugbot is set up for automated code reviews on this repo. Configure here.

… hover
Base UI focuses the first tabbable element when a menu opens with the
mouse. Inside the sidebar project-scope menu every row is tabIndex=-1
until highlighted, so the first tabbable element was the settings gear
inside the first project row; focus bubbling into that row made Base UI
render it as highlighted although the pointer was elsewhere. Keyboard
opens were unaffected because the checked row is pre-highlighted and
receives focus itself.
When the trigger is activated by pointer, focus the popup element during
commit instead: Base UI's queued open-time autofocus sees focus is
already inside the popup and skips. Keyboard opens keep the default
behavior; arrow keys, Tab-to-gear, and mouse clicks on the gear still
work as before.
@coderabbitai

coderabbitaiBot commented Aug 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ad38b52-28e7-4308-a0c5-3cdefa322497

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 23, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding: the open-focus redirect is a component-local workaround that depends on Base UI's internal focus timing, while the actual cause (a tabbable Button nested inside a menu radio item) is left in place. Details inline.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
The Sidebar-level layout effect raced Base UI's FloatingFocusManager:
it only helps if the popup mounts in the same commit, and the manager's
autofocus decision can still land on the settings gear otherwise.
Move the redirect into the popup subtree as a sentinel component. React
runs child layout effects before the popup's own effect, so the manager
snapshots activeElement already inside the popup and skips its queued
autofocus deterministically, regardless of mount timing. Behavior is
unchanged for keyboard opens (sentinel not mounted) and for arrow keys,
Tab-to-gear, and gear clicks.
@IzonIcy

Copy link
Copy Markdown
Author

Fixed in 437a995 — the timing critique was fair, so the redirect moved from a Sidebar-level layout effect into the popup's own subtree: a hidden sentinel child focuses [data-slot='menu-popup'] during its layout effect, which React runs before the popup component's FloatingFocusManager effect (child-first). The manager therefore snapshots activeElement already inside the floating element and skips its queued autofocus outright — no dependency on which commit the popup mounts in, and no reliance on the enqueueFocus shouldFocus re-check. The sentinel only mounts for pointer-opened menus; keyboard opens keep Base UI's default pre-highlighted-row focus.

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the project-scope menu focus fix against the shared Menu primitive contract and Base UI 1.4.1 behavior. The layout-effect ordering the sentinel relies on does hold in 1.4.1 (FloatingFocusManager snapshots activeElement synchronously in its own layout effect and bails when focus is already inside the popup), so the mechanism itself is sound. Three smaller issues remain: a render-time ref read that the React Compiler rejects, a popup focus() call that does not mirror Base UI's preventScroll, and ownership of the behavior sitting at one call site while an identical menu composition exists elsewhere.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
@IzonIcy

Copy link
Copy Markdown
Author

Before (main): opening the project-scope menu highlights the first project row even though the pointer never left the trigger.

t3-pr7950-before-bug

After (fix branch): identical interaction — no row is highlighted until the pointer or keyboard actually touches one.

t3-pr7950-after-fix

Both captured on an isolated dev environment — same machine, same seeded project, menu opened with a mouse click each time. For extra rigor: on main, DOM inspection confirms the project row ends up data-highlighted=true with focus stolen from the trigger; with the fix, focus stays on the popup element and no item receives data-highlighted. Keyboard opens are unchanged.

@IzonIcy
IzonIcy marked this pull request as ready for review August 23, 2026 02:17
CopilotAI lite review requested due to automatic review settings August 23, 2026 02:17

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 437a995. Configure here.

Comment threadapps/web/src/components/Sidebar.tsx
@macroscopeapp

macroscopeappBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a focused, opt-in UI focus correction limited to the project-scope menu, with keyboard behavior and other menus preserved. An unresolved Medium finding identifies a popup-ref merging risk, which remains a separate correctness gate under the repository’s configured threshold.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

Addresses the three Macroscope UI-consistency findings on this PR:
- The sentinel no longer reaches through MenuPopup's internal DOM via
closest(). MenuPopup now owns a ref to its own popup element and
exposes an opt-in focusOnMountRef prop; the FocusPopupOnMount helper
lives beside it so the Base UI ordering workaround is documented in
one place.
- The mount focus now passes preventScroll:true, matching Base UI's own
open-focus behavior while the popup is still unpositioned.
- projectScopeOpenedWithPointerRef.current is no longer read during
render. The ref object is passed down and read inside the layout
effect, keeping the component React Compiler-safe.
Verified: pnpm exec tsgo --noEmit, vp lint on changed files, menu +
sidebar unit tests.
Model: ox-alpha (opencode/x-preview-f-free), opencode
className,
)}
data-slot="menu-popup"
ref={popupRef}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumui/menu.tsx:103

When a caller supplies ref, {...props} overwrites popupRef, so FocusPopupOnMount sees popupRef.current === null and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ui/menu.tsx around line 103:
When a caller supplies `ref`, `{...props}` overwrites `popupRef`, so `FocusPopupOnMount` sees `popupRef.current === null` and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.
Evidence trail:
3fe9398
apps/web/src/components/ui/menu.tsx:25-40
apps/web/src/components/ui/menu.tsx:54-108
apps/web/package.json:15,42

@IzonIcy

Copy link
Copy Markdown
Author

Attaching the before/after captures promised in the description (hosted on the fork's pr-evidence/7950 branch to keep PR-only assets out of the repo):

Before — pointer open, no hover: t3-demo-project row wrongly highlighted (autofocus landed on its gear):

before

After — pointer open: focus lands on the popup, project row neutral:

after

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings in the shared MenuPopup primitive. The behavior goal (no pre-highlighted row on pointer opens) looks right; the concern is where the behavior lives and how it is wired.

Posted via Macroscope — UI Consistency

Comment on lines +25 to +39
function FocusPopupOnMount({
enabled,
popupRef,
}: {
enabled: { current: boolean };
popupRef: { current: HTMLElement | null };
}) {
useLayoutEffect(() => {
if (!enabled.current) {
return;
}
// preventScroll matches Base UI's own open-focus behavior: this runs
// before the positioner has positioned the popup, so a scrolling focus
// could jump ancestor scrollers.
popupRef.current?.focus({ preventScroll: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts popup focus ownership in a hidden child whose correctness depends on React running its layout effect before Base UI's focus manager, and it pushes interaction-type detection out to every consumer (Sidebar.tsx now tracks onPointerDown/onKeyDown on the trigger by hand). Base UI's popups already own both halves of this: Menu.Popup accepts initialFocus — the sibling of the finalFocus prop this repo already uses on a Base UI popup in CommandPalette.tsx:535 — as a ref or a callback that receives the open interaction type, and the focus manager consults it instead of being raced by effect ordering.

Suggest replacing FocusPopupOnMount + focusOnMountRef with initialFocus on MenuPrimitive.Popup, returning popupRef.current for non-keyboard opens and falling through to the default for keyboard opens (verify the exact callback return contract with a typecheck against @base-ui/react@1.5.0). That keeps the fix declarative, survives Base UI upgrades that change when the focus manager runs, and lets MenuPopup apply it by default — the same nested-control-in-first-row pattern exists at ProjectScriptsControl.tsx:214, which the current opt-in ref prop does not cover.

No diff: the fix spans the prop signature and the Sidebar trigger handlers.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/ui/menu.tsx
@t3dotgg

Copy link
Copy Markdown
Member

Note

🤖 GPT-5.6 Sol responding on behalf of Theo

We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together.

Closing this alternate menu focus policy. The same project-menu highlight is covered by #7916. This version adds a shared layout-effect focus API and relies on open-time scheduling details. Keep the pointer-versus-keyboard reproduction in the selected fix and use the menu primitive's supported initial-focus behavior.

If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. If GitHub does not let you reopen it, leave a comment here and we'll take another look.

@t3dotggt3dotgg closed this Aug 27, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M30-99 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar project menu highlights the first project on open without hover

3 participants

@IzonIcy@t3dotgg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(web): stop project-scope menu from highlighting first row without hover - #7950

Closed
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus
Closed

fix(web): stop project-scope menu from highlighting first row without hover#7950
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus

Conversation

@IzonIcy

@IzonIcyIzonIcy commented Aug 23, 2026

Copy link
Copy Markdown

What changed

apps/web/src/components/Sidebar.tsx: when the sidebar project-scope menu ("All projects" dropdown) is opened with a pointer, focus is redirected to the popup element itself during commit. Base UI's open-time autofocus — which queues a microtask and skips when focus already sits inside the popup — then no-ops instead of landing on the first tabbable element, the settings gear inside the first project row.

Keyboard opens keep Base UI's default behavior (the checked row is pre-highlighted and receives focus itself). Arrow-key navigation, Tab-to-gear, and mouse clicks on the gear are unchanged; only the misdirected open focus moved.

Why

Fixes#7915: opening this menu with the mouse rendered the first project row as highlighted although the pointer was elsewhere, because every row is tabIndex=-1 until highlighted and focus bubbling into the gear marked its row active.

Evidence

Menu opened with the pointer, nothing hovered.

Before — the t3-demo-project row renders highlighted because Base UI's open autofocus landed on its settings gear:

before: project row highlighted without hover

After — focus lands on the popup itself; the project row stays neutral (the checked "All projects" row keeps its selection background, as intended):

after: project row neutral on pointer open

Images live on the fork's pr-evidence/7950 branch so no PR-only assets land in the repo.

Verification

  • Scoped typecheck clean (tsgo --noEmit, 0 errors); lint clean
  • Focus-redirect mechanics verified against Base UI 1.5.0 internals (FloatingFocusManager skips its queued autofocus when focus is already inside the floating element)

--
Worked by ox-alpha via opencode (x-preview-f-free).

Note

Fix project-scope menu highlighting first row when opened by pointer

  • Adds a projectScopeOpenedWithPointerRef in Sidebar.tsx set via onPointerDown/onKeyDown on the menu trigger, distinguishing pointer opens from keyboard opens
  • Adds a FocusPopupOnMount component and optional focusOnMountRef prop to MenuPopup in menu.tsx; when enabled, the popup element itself receives focus on mount instead of the first tabbable descendant
  • When the project-scope menu is opened with a pointer, the popup is focused to avoid pre-highlighting the first row; keyboard opens retain the default focus behavior

Macroscope summarized 3a0f2aa.


Note

Low Risk
Localized focus-management workaround for one menu; keyboard behavior and other menus are unchanged unless they opt into focusOnMountRef.

Overview
Fixes incorrect row highlight when the sidebar All projects menu opens via click: Base UI autofocus was landing on the first row’s nested settings gear instead of leaving rows neutral.

MenuPopup gains an optional focusOnMountRef and a FocusPopupOnMount layout effect that focuses the popup container (with preventScroll) when the ref is true at mount, so Base UI’s focus manager skips autofocus to the first tabbable descendant.

Sidebar records pointer vs keyboard on the project-scope trigger (onPointerDown / onKeyDown) and passes that ref into MenuPopup so pointer opens redirect focus to the popup; keyboard opens keep the default pre-highlighted checked row.

Reviewed by Cursor Bugbot for commit 3a0f2aa. Bugbot is set up for automated code reviews on this repo. Configure here.

… hover
Base UI focuses the first tabbable element when a menu opens with the
mouse. Inside the sidebar project-scope menu every row is tabIndex=-1
until highlighted, so the first tabbable element was the settings gear
inside the first project row; focus bubbling into that row made Base UI
render it as highlighted although the pointer was elsewhere. Keyboard
opens were unaffected because the checked row is pre-highlighted and
receives focus itself.
When the trigger is activated by pointer, focus the popup element during
commit instead: Base UI's queued open-time autofocus sees focus is
already inside the popup and skips. Keyboard opens keep the default
behavior; arrow keys, Tab-to-gear, and mouse clicks on the gear still
work as before.
@coderabbitai

coderabbitaiBot commented Aug 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ad38b52-28e7-4308-a0c5-3cdefa322497

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 23, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding: the open-focus redirect is a component-local workaround that depends on Base UI's internal focus timing, while the actual cause (a tabbable Button nested inside a menu radio item) is left in place. Details inline.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
The Sidebar-level layout effect raced Base UI's FloatingFocusManager:
it only helps if the popup mounts in the same commit, and the manager's
autofocus decision can still land on the settings gear otherwise.
Move the redirect into the popup subtree as a sentinel component. React
runs child layout effects before the popup's own effect, so the manager
snapshots activeElement already inside the popup and skips its queued
autofocus deterministically, regardless of mount timing. Behavior is
unchanged for keyboard opens (sentinel not mounted) and for arrow keys,
Tab-to-gear, and gear clicks.
@IzonIcy

Copy link
Copy Markdown
Author

Fixed in 437a995 — the timing critique was fair, so the redirect moved from a Sidebar-level layout effect into the popup's own subtree: a hidden sentinel child focuses [data-slot='menu-popup'] during its layout effect, which React runs before the popup component's FloatingFocusManager effect (child-first). The manager therefore snapshots activeElement already inside the floating element and skips its queued autofocus outright — no dependency on which commit the popup mounts in, and no reliance on the enqueueFocus shouldFocus re-check. The sentinel only mounts for pointer-opened menus; keyboard opens keep Base UI's default pre-highlighted-row focus.

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the project-scope menu focus fix against the shared Menu primitive contract and Base UI 1.4.1 behavior. The layout-effect ordering the sentinel relies on does hold in 1.4.1 (FloatingFocusManager snapshots activeElement synchronously in its own layout effect and bails when focus is already inside the popup), so the mechanism itself is sound. Three smaller issues remain: a render-time ref read that the React Compiler rejects, a popup focus() call that does not mirror Base UI's preventScroll, and ownership of the behavior sitting at one call site while an identical menu composition exists elsewhere.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
@IzonIcy

Copy link
Copy Markdown
Author

Before (main): opening the project-scope menu highlights the first project row even though the pointer never left the trigger.

t3-pr7950-before-bug

After (fix branch): identical interaction — no row is highlighted until the pointer or keyboard actually touches one.

t3-pr7950-after-fix

Both captured on an isolated dev environment — same machine, same seeded project, menu opened with a mouse click each time. For extra rigor: on main, DOM inspection confirms the project row ends up data-highlighted=true with focus stolen from the trigger; with the fix, focus stays on the popup element and no item receives data-highlighted. Keyboard opens are unchanged.

@IzonIcy
IzonIcy marked this pull request as ready for review August 23, 2026 02:17
CopilotAI lite review requested due to automatic review settings August 23, 2026 02:17

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 437a995. Configure here.

Comment threadapps/web/src/components/Sidebar.tsx
@macroscopeapp

macroscopeappBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a focused, opt-in UI focus correction limited to the project-scope menu, with keyboard behavior and other menus preserved. An unresolved Medium finding identifies a popup-ref merging risk, which remains a separate correctness gate under the repository’s configured threshold.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

Addresses the three Macroscope UI-consistency findings on this PR:
- The sentinel no longer reaches through MenuPopup's internal DOM via
closest(). MenuPopup now owns a ref to its own popup element and
exposes an opt-in focusOnMountRef prop; the FocusPopupOnMount helper
lives beside it so the Base UI ordering workaround is documented in
one place.
- The mount focus now passes preventScroll:true, matching Base UI's own
open-focus behavior while the popup is still unpositioned.
- projectScopeOpenedWithPointerRef.current is no longer read during
render. The ref object is passed down and read inside the layout
effect, keeping the component React Compiler-safe.
Verified: pnpm exec tsgo --noEmit, vp lint on changed files, menu +
sidebar unit tests.
Model: ox-alpha (opencode/x-preview-f-free), opencode
className,
)}
data-slot="menu-popup"
ref={popupRef}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumui/menu.tsx:103

When a caller supplies ref, {...props} overwrites popupRef, so FocusPopupOnMount sees popupRef.current === null and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ui/menu.tsx around line 103:
When a caller supplies `ref`, `{...props}` overwrites `popupRef`, so `FocusPopupOnMount` sees `popupRef.current === null` and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.
Evidence trail:
3fe9398
apps/web/src/components/ui/menu.tsx:25-40
apps/web/src/components/ui/menu.tsx:54-108
apps/web/package.json:15,42

@IzonIcy

Copy link
Copy Markdown
Author

Attaching the before/after captures promised in the description (hosted on the fork's pr-evidence/7950 branch to keep PR-only assets out of the repo):

Before — pointer open, no hover: t3-demo-project row wrongly highlighted (autofocus landed on its gear):

before

After — pointer open: focus lands on the popup, project row neutral:

after

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings in the shared MenuPopup primitive. The behavior goal (no pre-highlighted row on pointer opens) looks right; the concern is where the behavior lives and how it is wired.

Posted via Macroscope — UI Consistency

Comment on lines +25 to +39
function FocusPopupOnMount({
enabled,
popupRef,
}: {
enabled: { current: boolean };
popupRef: { current: HTMLElement | null };
}) {
useLayoutEffect(() => {
if (!enabled.current) {
return;
}
// preventScroll matches Base UI's own open-focus behavior: this runs
// before the positioner has positioned the popup, so a scrolling focus
// could jump ancestor scrollers.
popupRef.current?.focus({ preventScroll: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts popup focus ownership in a hidden child whose correctness depends on React running its layout effect before Base UI's focus manager, and it pushes interaction-type detection out to every consumer (Sidebar.tsx now tracks onPointerDown/onKeyDown on the trigger by hand). Base UI's popups already own both halves of this: Menu.Popup accepts initialFocus — the sibling of the finalFocus prop this repo already uses on a Base UI popup in CommandPalette.tsx:535 — as a ref or a callback that receives the open interaction type, and the focus manager consults it instead of being raced by effect ordering.

Suggest replacing FocusPopupOnMount + focusOnMountRef with initialFocus on MenuPrimitive.Popup, returning popupRef.current for non-keyboard opens and falling through to the default for keyboard opens (verify the exact callback return contract with a typecheck against @base-ui/react@1.5.0). That keeps the fix declarative, survives Base UI upgrades that change when the focus manager runs, and lets MenuPopup apply it by default — the same nested-control-in-first-row pattern exists at ProjectScriptsControl.tsx:214, which the current opt-in ref prop does not cover.

No diff: the fix spans the prop signature and the Sidebar trigger handlers.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/ui/menu.tsx
@t3dotgg

Copy link
Copy Markdown
Member

Note

🤖 GPT-5.6 Sol responding on behalf of Theo

We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together.

Closing this alternate menu focus policy. The same project-menu highlight is covered by #7916. This version adds a shared layout-effect focus API and relies on open-time scheduling details. Keep the pointer-versus-keyboard reproduction in the selected fix and use the menu primitive's supported initial-focus behavior.

If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. If GitHub does not let you reopen it, leave a comment here and we'll take another look.

@t3dotggt3dotgg closed this Aug 27, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M30-99 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar project menu highlights the first project on open without hover

3 participants

@IzonIcy@t3dotgg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(web): stop project-scope menu from highlighting first row without hover - #7950

Closed
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus
Closed

fix(web): stop project-scope menu from highlighting first row without hover#7950
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus

Conversation

@IzonIcy

@IzonIcyIzonIcy commented Aug 23, 2026

Copy link
Copy Markdown

What changed

apps/web/src/components/Sidebar.tsx: when the sidebar project-scope menu ("All projects" dropdown) is opened with a pointer, focus is redirected to the popup element itself during commit. Base UI's open-time autofocus — which queues a microtask and skips when focus already sits inside the popup — then no-ops instead of landing on the first tabbable element, the settings gear inside the first project row.

Keyboard opens keep Base UI's default behavior (the checked row is pre-highlighted and receives focus itself). Arrow-key navigation, Tab-to-gear, and mouse clicks on the gear are unchanged; only the misdirected open focus moved.

Why

Fixes#7915: opening this menu with the mouse rendered the first project row as highlighted although the pointer was elsewhere, because every row is tabIndex=-1 until highlighted and focus bubbling into the gear marked its row active.

Evidence

Menu opened with the pointer, nothing hovered.

Before — the t3-demo-project row renders highlighted because Base UI's open autofocus landed on its settings gear:

before: project row highlighted without hover

After — focus lands on the popup itself; the project row stays neutral (the checked "All projects" row keeps its selection background, as intended):

after: project row neutral on pointer open

Images live on the fork's pr-evidence/7950 branch so no PR-only assets land in the repo.

Verification

  • Scoped typecheck clean (tsgo --noEmit, 0 errors); lint clean
  • Focus-redirect mechanics verified against Base UI 1.5.0 internals (FloatingFocusManager skips its queued autofocus when focus is already inside the floating element)

--
Worked by ox-alpha via opencode (x-preview-f-free).

Note

Fix project-scope menu highlighting first row when opened by pointer

  • Adds a projectScopeOpenedWithPointerRef in Sidebar.tsx set via onPointerDown/onKeyDown on the menu trigger, distinguishing pointer opens from keyboard opens
  • Adds a FocusPopupOnMount component and optional focusOnMountRef prop to MenuPopup in menu.tsx; when enabled, the popup element itself receives focus on mount instead of the first tabbable descendant
  • When the project-scope menu is opened with a pointer, the popup is focused to avoid pre-highlighting the first row; keyboard opens retain the default focus behavior

Macroscope summarized 3a0f2aa.


Note

Low Risk
Localized focus-management workaround for one menu; keyboard behavior and other menus are unchanged unless they opt into focusOnMountRef.

Overview
Fixes incorrect row highlight when the sidebar All projects menu opens via click: Base UI autofocus was landing on the first row’s nested settings gear instead of leaving rows neutral.

MenuPopup gains an optional focusOnMountRef and a FocusPopupOnMount layout effect that focuses the popup container (with preventScroll) when the ref is true at mount, so Base UI’s focus manager skips autofocus to the first tabbable descendant.

Sidebar records pointer vs keyboard on the project-scope trigger (onPointerDown / onKeyDown) and passes that ref into MenuPopup so pointer opens redirect focus to the popup; keyboard opens keep the default pre-highlighted checked row.

Reviewed by Cursor Bugbot for commit 3a0f2aa. Bugbot is set up for automated code reviews on this repo. Configure here.

… hover
Base UI focuses the first tabbable element when a menu opens with the
mouse. Inside the sidebar project-scope menu every row is tabIndex=-1
until highlighted, so the first tabbable element was the settings gear
inside the first project row; focus bubbling into that row made Base UI
render it as highlighted although the pointer was elsewhere. Keyboard
opens were unaffected because the checked row is pre-highlighted and
receives focus itself.
When the trigger is activated by pointer, focus the popup element during
commit instead: Base UI's queued open-time autofocus sees focus is
already inside the popup and skips. Keyboard opens keep the default
behavior; arrow keys, Tab-to-gear, and mouse clicks on the gear still
work as before.
@coderabbitai

coderabbitaiBot commented Aug 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ad38b52-28e7-4308-a0c5-3cdefa322497

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 23, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding: the open-focus redirect is a component-local workaround that depends on Base UI's internal focus timing, while the actual cause (a tabbable Button nested inside a menu radio item) is left in place. Details inline.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
The Sidebar-level layout effect raced Base UI's FloatingFocusManager:
it only helps if the popup mounts in the same commit, and the manager's
autofocus decision can still land on the settings gear otherwise.
Move the redirect into the popup subtree as a sentinel component. React
runs child layout effects before the popup's own effect, so the manager
snapshots activeElement already inside the popup and skips its queued
autofocus deterministically, regardless of mount timing. Behavior is
unchanged for keyboard opens (sentinel not mounted) and for arrow keys,
Tab-to-gear, and gear clicks.
@IzonIcy

Copy link
Copy Markdown
Author

Fixed in 437a995 — the timing critique was fair, so the redirect moved from a Sidebar-level layout effect into the popup's own subtree: a hidden sentinel child focuses [data-slot='menu-popup'] during its layout effect, which React runs before the popup component's FloatingFocusManager effect (child-first). The manager therefore snapshots activeElement already inside the floating element and skips its queued autofocus outright — no dependency on which commit the popup mounts in, and no reliance on the enqueueFocus shouldFocus re-check. The sentinel only mounts for pointer-opened menus; keyboard opens keep Base UI's default pre-highlighted-row focus.

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the project-scope menu focus fix against the shared Menu primitive contract and Base UI 1.4.1 behavior. The layout-effect ordering the sentinel relies on does hold in 1.4.1 (FloatingFocusManager snapshots activeElement synchronously in its own layout effect and bails when focus is already inside the popup), so the mechanism itself is sound. Three smaller issues remain: a render-time ref read that the React Compiler rejects, a popup focus() call that does not mirror Base UI's preventScroll, and ownership of the behavior sitting at one call site while an identical menu composition exists elsewhere.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
@IzonIcy

Copy link
Copy Markdown
Author

Before (main): opening the project-scope menu highlights the first project row even though the pointer never left the trigger.

t3-pr7950-before-bug

After (fix branch): identical interaction — no row is highlighted until the pointer or keyboard actually touches one.

t3-pr7950-after-fix

Both captured on an isolated dev environment — same machine, same seeded project, menu opened with a mouse click each time. For extra rigor: on main, DOM inspection confirms the project row ends up data-highlighted=true with focus stolen from the trigger; with the fix, focus stays on the popup element and no item receives data-highlighted. Keyboard opens are unchanged.

@IzonIcy
IzonIcy marked this pull request as ready for review August 23, 2026 02:17
CopilotAI lite review requested due to automatic review settings August 23, 2026 02:17

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 437a995. Configure here.

Comment threadapps/web/src/components/Sidebar.tsx
@macroscopeapp

macroscopeappBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a focused, opt-in UI focus correction limited to the project-scope menu, with keyboard behavior and other menus preserved. An unresolved Medium finding identifies a popup-ref merging risk, which remains a separate correctness gate under the repository’s configured threshold.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

Addresses the three Macroscope UI-consistency findings on this PR:
- The sentinel no longer reaches through MenuPopup's internal DOM via
closest(). MenuPopup now owns a ref to its own popup element and
exposes an opt-in focusOnMountRef prop; the FocusPopupOnMount helper
lives beside it so the Base UI ordering workaround is documented in
one place.
- The mount focus now passes preventScroll:true, matching Base UI's own
open-focus behavior while the popup is still unpositioned.
- projectScopeOpenedWithPointerRef.current is no longer read during
render. The ref object is passed down and read inside the layout
effect, keeping the component React Compiler-safe.
Verified: pnpm exec tsgo --noEmit, vp lint on changed files, menu +
sidebar unit tests.
Model: ox-alpha (opencode/x-preview-f-free), opencode
className,
)}
data-slot="menu-popup"
ref={popupRef}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumui/menu.tsx:103

When a caller supplies ref, {...props} overwrites popupRef, so FocusPopupOnMount sees popupRef.current === null and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ui/menu.tsx around line 103:
When a caller supplies `ref`, `{...props}` overwrites `popupRef`, so `FocusPopupOnMount` sees `popupRef.current === null` and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.
Evidence trail:
3fe9398
apps/web/src/components/ui/menu.tsx:25-40
apps/web/src/components/ui/menu.tsx:54-108
apps/web/package.json:15,42

@IzonIcy

Copy link
Copy Markdown
Author

Attaching the before/after captures promised in the description (hosted on the fork's pr-evidence/7950 branch to keep PR-only assets out of the repo):

Before — pointer open, no hover: t3-demo-project row wrongly highlighted (autofocus landed on its gear):

before

After — pointer open: focus lands on the popup, project row neutral:

after

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings in the shared MenuPopup primitive. The behavior goal (no pre-highlighted row on pointer opens) looks right; the concern is where the behavior lives and how it is wired.

Posted via Macroscope — UI Consistency

Comment on lines +25 to +39
function FocusPopupOnMount({
enabled,
popupRef,
}: {
enabled: { current: boolean };
popupRef: { current: HTMLElement | null };
}) {
useLayoutEffect(() => {
if (!enabled.current) {
return;
}
// preventScroll matches Base UI's own open-focus behavior: this runs
// before the positioner has positioned the popup, so a scrolling focus
// could jump ancestor scrollers.
popupRef.current?.focus({ preventScroll: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts popup focus ownership in a hidden child whose correctness depends on React running its layout effect before Base UI's focus manager, and it pushes interaction-type detection out to every consumer (Sidebar.tsx now tracks onPointerDown/onKeyDown on the trigger by hand). Base UI's popups already own both halves of this: Menu.Popup accepts initialFocus — the sibling of the finalFocus prop this repo already uses on a Base UI popup in CommandPalette.tsx:535 — as a ref or a callback that receives the open interaction type, and the focus manager consults it instead of being raced by effect ordering.

Suggest replacing FocusPopupOnMount + focusOnMountRef with initialFocus on MenuPrimitive.Popup, returning popupRef.current for non-keyboard opens and falling through to the default for keyboard opens (verify the exact callback return contract with a typecheck against @base-ui/react@1.5.0). That keeps the fix declarative, survives Base UI upgrades that change when the focus manager runs, and lets MenuPopup apply it by default — the same nested-control-in-first-row pattern exists at ProjectScriptsControl.tsx:214, which the current opt-in ref prop does not cover.

No diff: the fix spans the prop signature and the Sidebar trigger handlers.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/ui/menu.tsx
@t3dotgg

Copy link
Copy Markdown
Member

Note

🤖 GPT-5.6 Sol responding on behalf of Theo

We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together.

Closing this alternate menu focus policy. The same project-menu highlight is covered by #7916. This version adds a shared layout-effect focus API and relies on open-time scheduling details. Keep the pointer-versus-keyboard reproduction in the selected fix and use the menu primitive's supported initial-focus behavior.

If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. If GitHub does not let you reopen it, leave a comment here and we'll take another look.

@t3dotggt3dotgg closed this Aug 27, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M30-99 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar project menu highlights the first project on open without hover

3 participants

@IzonIcy@t3dotgg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(web): stop project-scope menu from highlighting first row without hover - #7950

Closed
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus
Closed

fix(web): stop project-scope menu from highlighting first row without hover#7950
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus

Conversation

@IzonIcy

@IzonIcyIzonIcy commented Aug 23, 2026

Copy link
Copy Markdown

What changed

apps/web/src/components/Sidebar.tsx: when the sidebar project-scope menu ("All projects" dropdown) is opened with a pointer, focus is redirected to the popup element itself during commit. Base UI's open-time autofocus — which queues a microtask and skips when focus already sits inside the popup — then no-ops instead of landing on the first tabbable element, the settings gear inside the first project row.

Keyboard opens keep Base UI's default behavior (the checked row is pre-highlighted and receives focus itself). Arrow-key navigation, Tab-to-gear, and mouse clicks on the gear are unchanged; only the misdirected open focus moved.

Why

Fixes#7915: opening this menu with the mouse rendered the first project row as highlighted although the pointer was elsewhere, because every row is tabIndex=-1 until highlighted and focus bubbling into the gear marked its row active.

Evidence

Menu opened with the pointer, nothing hovered.

Before — the t3-demo-project row renders highlighted because Base UI's open autofocus landed on its settings gear:

before: project row highlighted without hover

After — focus lands on the popup itself; the project row stays neutral (the checked "All projects" row keeps its selection background, as intended):

after: project row neutral on pointer open

Images live on the fork's pr-evidence/7950 branch so no PR-only assets land in the repo.

Verification

  • Scoped typecheck clean (tsgo --noEmit, 0 errors); lint clean
  • Focus-redirect mechanics verified against Base UI 1.5.0 internals (FloatingFocusManager skips its queued autofocus when focus is already inside the floating element)

--
Worked by ox-alpha via opencode (x-preview-f-free).

Note

Fix project-scope menu highlighting first row when opened by pointer

  • Adds a projectScopeOpenedWithPointerRef in Sidebar.tsx set via onPointerDown/onKeyDown on the menu trigger, distinguishing pointer opens from keyboard opens
  • Adds a FocusPopupOnMount component and optional focusOnMountRef prop to MenuPopup in menu.tsx; when enabled, the popup element itself receives focus on mount instead of the first tabbable descendant
  • When the project-scope menu is opened with a pointer, the popup is focused to avoid pre-highlighting the first row; keyboard opens retain the default focus behavior

Macroscope summarized 3a0f2aa.


Note

Low Risk
Localized focus-management workaround for one menu; keyboard behavior and other menus are unchanged unless they opt into focusOnMountRef.

Overview
Fixes incorrect row highlight when the sidebar All projects menu opens via click: Base UI autofocus was landing on the first row’s nested settings gear instead of leaving rows neutral.

MenuPopup gains an optional focusOnMountRef and a FocusPopupOnMount layout effect that focuses the popup container (with preventScroll) when the ref is true at mount, so Base UI’s focus manager skips autofocus to the first tabbable descendant.

Sidebar records pointer vs keyboard on the project-scope trigger (onPointerDown / onKeyDown) and passes that ref into MenuPopup so pointer opens redirect focus to the popup; keyboard opens keep the default pre-highlighted checked row.

Reviewed by Cursor Bugbot for commit 3a0f2aa. Bugbot is set up for automated code reviews on this repo. Configure here.

… hover
Base UI focuses the first tabbable element when a menu opens with the
mouse. Inside the sidebar project-scope menu every row is tabIndex=-1
until highlighted, so the first tabbable element was the settings gear
inside the first project row; focus bubbling into that row made Base UI
render it as highlighted although the pointer was elsewhere. Keyboard
opens were unaffected because the checked row is pre-highlighted and
receives focus itself.
When the trigger is activated by pointer, focus the popup element during
commit instead: Base UI's queued open-time autofocus sees focus is
already inside the popup and skips. Keyboard opens keep the default
behavior; arrow keys, Tab-to-gear, and mouse clicks on the gear still
work as before.
@coderabbitai

coderabbitaiBot commented Aug 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ad38b52-28e7-4308-a0c5-3cdefa322497

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 23, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding: the open-focus redirect is a component-local workaround that depends on Base UI's internal focus timing, while the actual cause (a tabbable Button nested inside a menu radio item) is left in place. Details inline.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
The Sidebar-level layout effect raced Base UI's FloatingFocusManager:
it only helps if the popup mounts in the same commit, and the manager's
autofocus decision can still land on the settings gear otherwise.
Move the redirect into the popup subtree as a sentinel component. React
runs child layout effects before the popup's own effect, so the manager
snapshots activeElement already inside the popup and skips its queued
autofocus deterministically, regardless of mount timing. Behavior is
unchanged for keyboard opens (sentinel not mounted) and for arrow keys,
Tab-to-gear, and gear clicks.
@IzonIcy

Copy link
Copy Markdown
Author

Fixed in 437a995 — the timing critique was fair, so the redirect moved from a Sidebar-level layout effect into the popup's own subtree: a hidden sentinel child focuses [data-slot='menu-popup'] during its layout effect, which React runs before the popup component's FloatingFocusManager effect (child-first). The manager therefore snapshots activeElement already inside the floating element and skips its queued autofocus outright — no dependency on which commit the popup mounts in, and no reliance on the enqueueFocus shouldFocus re-check. The sentinel only mounts for pointer-opened menus; keyboard opens keep Base UI's default pre-highlighted-row focus.

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the project-scope menu focus fix against the shared Menu primitive contract and Base UI 1.4.1 behavior. The layout-effect ordering the sentinel relies on does hold in 1.4.1 (FloatingFocusManager snapshots activeElement synchronously in its own layout effect and bails when focus is already inside the popup), so the mechanism itself is sound. Three smaller issues remain: a render-time ref read that the React Compiler rejects, a popup focus() call that does not mirror Base UI's preventScroll, and ownership of the behavior sitting at one call site while an identical menu composition exists elsewhere.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
@IzonIcy

Copy link
Copy Markdown
Author

Before (main): opening the project-scope menu highlights the first project row even though the pointer never left the trigger.

t3-pr7950-before-bug

After (fix branch): identical interaction — no row is highlighted until the pointer or keyboard actually touches one.

t3-pr7950-after-fix

Both captured on an isolated dev environment — same machine, same seeded project, menu opened with a mouse click each time. For extra rigor: on main, DOM inspection confirms the project row ends up data-highlighted=true with focus stolen from the trigger; with the fix, focus stays on the popup element and no item receives data-highlighted. Keyboard opens are unchanged.

@IzonIcy
IzonIcy marked this pull request as ready for review August 23, 2026 02:17
CopilotAI lite review requested due to automatic review settings August 23, 2026 02:17

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 437a995. Configure here.

Comment threadapps/web/src/components/Sidebar.tsx
@macroscopeapp

macroscopeappBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a focused, opt-in UI focus correction limited to the project-scope menu, with keyboard behavior and other menus preserved. An unresolved Medium finding identifies a popup-ref merging risk, which remains a separate correctness gate under the repository’s configured threshold.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

Addresses the three Macroscope UI-consistency findings on this PR:
- The sentinel no longer reaches through MenuPopup's internal DOM via
closest(). MenuPopup now owns a ref to its own popup element and
exposes an opt-in focusOnMountRef prop; the FocusPopupOnMount helper
lives beside it so the Base UI ordering workaround is documented in
one place.
- The mount focus now passes preventScroll:true, matching Base UI's own
open-focus behavior while the popup is still unpositioned.
- projectScopeOpenedWithPointerRef.current is no longer read during
render. The ref object is passed down and read inside the layout
effect, keeping the component React Compiler-safe.
Verified: pnpm exec tsgo --noEmit, vp lint on changed files, menu +
sidebar unit tests.
Model: ox-alpha (opencode/x-preview-f-free), opencode
className,
)}
data-slot="menu-popup"
ref={popupRef}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumui/menu.tsx:103

When a caller supplies ref, {...props} overwrites popupRef, so FocusPopupOnMount sees popupRef.current === null and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ui/menu.tsx around line 103:
When a caller supplies `ref`, `{...props}` overwrites `popupRef`, so `FocusPopupOnMount` sees `popupRef.current === null` and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.
Evidence trail:
3fe9398
apps/web/src/components/ui/menu.tsx:25-40
apps/web/src/components/ui/menu.tsx:54-108
apps/web/package.json:15,42

@IzonIcy

Copy link
Copy Markdown
Author

Attaching the before/after captures promised in the description (hosted on the fork's pr-evidence/7950 branch to keep PR-only assets out of the repo):

Before — pointer open, no hover: t3-demo-project row wrongly highlighted (autofocus landed on its gear):

before

After — pointer open: focus lands on the popup, project row neutral:

after

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings in the shared MenuPopup primitive. The behavior goal (no pre-highlighted row on pointer opens) looks right; the concern is where the behavior lives and how it is wired.

Posted via Macroscope — UI Consistency

Comment on lines +25 to +39
function FocusPopupOnMount({
enabled,
popupRef,
}: {
enabled: { current: boolean };
popupRef: { current: HTMLElement | null };
}) {
useLayoutEffect(() => {
if (!enabled.current) {
return;
}
// preventScroll matches Base UI's own open-focus behavior: this runs
// before the positioner has positioned the popup, so a scrolling focus
// could jump ancestor scrollers.
popupRef.current?.focus({ preventScroll: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts popup focus ownership in a hidden child whose correctness depends on React running its layout effect before Base UI's focus manager, and it pushes interaction-type detection out to every consumer (Sidebar.tsx now tracks onPointerDown/onKeyDown on the trigger by hand). Base UI's popups already own both halves of this: Menu.Popup accepts initialFocus — the sibling of the finalFocus prop this repo already uses on a Base UI popup in CommandPalette.tsx:535 — as a ref or a callback that receives the open interaction type, and the focus manager consults it instead of being raced by effect ordering.

Suggest replacing FocusPopupOnMount + focusOnMountRef with initialFocus on MenuPrimitive.Popup, returning popupRef.current for non-keyboard opens and falling through to the default for keyboard opens (verify the exact callback return contract with a typecheck against @base-ui/react@1.5.0). That keeps the fix declarative, survives Base UI upgrades that change when the focus manager runs, and lets MenuPopup apply it by default — the same nested-control-in-first-row pattern exists at ProjectScriptsControl.tsx:214, which the current opt-in ref prop does not cover.

No diff: the fix spans the prop signature and the Sidebar trigger handlers.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/ui/menu.tsx
@t3dotgg

Copy link
Copy Markdown
Member

Note

🤖 GPT-5.6 Sol responding on behalf of Theo

We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together.

Closing this alternate menu focus policy. The same project-menu highlight is covered by #7916. This version adds a shared layout-effect focus API and relies on open-time scheduling details. Keep the pointer-versus-keyboard reproduction in the selected fix and use the menu primitive's supported initial-focus behavior.

If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. If GitHub does not let you reopen it, leave a comment here and we'll take another look.

@t3dotggt3dotgg closed this Aug 27, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M30-99 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar project menu highlights the first project on open without hover

3 participants

@IzonIcy@t3dotgg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(web): stop project-scope menu from highlighting first row without hover - #7950

Closed
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus
Closed

fix(web): stop project-scope menu from highlighting first row without hover#7950
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus

Conversation

@IzonIcy

@IzonIcyIzonIcy commented Aug 23, 2026

Copy link
Copy Markdown

What changed

apps/web/src/components/Sidebar.tsx: when the sidebar project-scope menu ("All projects" dropdown) is opened with a pointer, focus is redirected to the popup element itself during commit. Base UI's open-time autofocus — which queues a microtask and skips when focus already sits inside the popup — then no-ops instead of landing on the first tabbable element, the settings gear inside the first project row.

Keyboard opens keep Base UI's default behavior (the checked row is pre-highlighted and receives focus itself). Arrow-key navigation, Tab-to-gear, and mouse clicks on the gear are unchanged; only the misdirected open focus moved.

Why

Fixes#7915: opening this menu with the mouse rendered the first project row as highlighted although the pointer was elsewhere, because every row is tabIndex=-1 until highlighted and focus bubbling into the gear marked its row active.

Evidence

Menu opened with the pointer, nothing hovered.

Before — the t3-demo-project row renders highlighted because Base UI's open autofocus landed on its settings gear:

before: project row highlighted without hover

After — focus lands on the popup itself; the project row stays neutral (the checked "All projects" row keeps its selection background, as intended):

after: project row neutral on pointer open

Images live on the fork's pr-evidence/7950 branch so no PR-only assets land in the repo.

Verification

  • Scoped typecheck clean (tsgo --noEmit, 0 errors); lint clean
  • Focus-redirect mechanics verified against Base UI 1.5.0 internals (FloatingFocusManager skips its queued autofocus when focus is already inside the floating element)

--
Worked by ox-alpha via opencode (x-preview-f-free).

Note

Fix project-scope menu highlighting first row when opened by pointer

  • Adds a projectScopeOpenedWithPointerRef in Sidebar.tsx set via onPointerDown/onKeyDown on the menu trigger, distinguishing pointer opens from keyboard opens
  • Adds a FocusPopupOnMount component and optional focusOnMountRef prop to MenuPopup in menu.tsx; when enabled, the popup element itself receives focus on mount instead of the first tabbable descendant
  • When the project-scope menu is opened with a pointer, the popup is focused to avoid pre-highlighting the first row; keyboard opens retain the default focus behavior

Macroscope summarized 3a0f2aa.


Note

Low Risk
Localized focus-management workaround for one menu; keyboard behavior and other menus are unchanged unless they opt into focusOnMountRef.

Overview
Fixes incorrect row highlight when the sidebar All projects menu opens via click: Base UI autofocus was landing on the first row’s nested settings gear instead of leaving rows neutral.

MenuPopup gains an optional focusOnMountRef and a FocusPopupOnMount layout effect that focuses the popup container (with preventScroll) when the ref is true at mount, so Base UI’s focus manager skips autofocus to the first tabbable descendant.

Sidebar records pointer vs keyboard on the project-scope trigger (onPointerDown / onKeyDown) and passes that ref into MenuPopup so pointer opens redirect focus to the popup; keyboard opens keep the default pre-highlighted checked row.

Reviewed by Cursor Bugbot for commit 3a0f2aa. Bugbot is set up for automated code reviews on this repo. Configure here.

… hover
Base UI focuses the first tabbable element when a menu opens with the
mouse. Inside the sidebar project-scope menu every row is tabIndex=-1
until highlighted, so the first tabbable element was the settings gear
inside the first project row; focus bubbling into that row made Base UI
render it as highlighted although the pointer was elsewhere. Keyboard
opens were unaffected because the checked row is pre-highlighted and
receives focus itself.
When the trigger is activated by pointer, focus the popup element during
commit instead: Base UI's queued open-time autofocus sees focus is
already inside the popup and skips. Keyboard opens keep the default
behavior; arrow keys, Tab-to-gear, and mouse clicks on the gear still
work as before.
@coderabbitai

coderabbitaiBot commented Aug 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ad38b52-28e7-4308-a0c5-3cdefa322497

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 23, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding: the open-focus redirect is a component-local workaround that depends on Base UI's internal focus timing, while the actual cause (a tabbable Button nested inside a menu radio item) is left in place. Details inline.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
The Sidebar-level layout effect raced Base UI's FloatingFocusManager:
it only helps if the popup mounts in the same commit, and the manager's
autofocus decision can still land on the settings gear otherwise.
Move the redirect into the popup subtree as a sentinel component. React
runs child layout effects before the popup's own effect, so the manager
snapshots activeElement already inside the popup and skips its queued
autofocus deterministically, regardless of mount timing. Behavior is
unchanged for keyboard opens (sentinel not mounted) and for arrow keys,
Tab-to-gear, and gear clicks.
@IzonIcy

Copy link
Copy Markdown
Author

Fixed in 437a995 — the timing critique was fair, so the redirect moved from a Sidebar-level layout effect into the popup's own subtree: a hidden sentinel child focuses [data-slot='menu-popup'] during its layout effect, which React runs before the popup component's FloatingFocusManager effect (child-first). The manager therefore snapshots activeElement already inside the floating element and skips its queued autofocus outright — no dependency on which commit the popup mounts in, and no reliance on the enqueueFocus shouldFocus re-check. The sentinel only mounts for pointer-opened menus; keyboard opens keep Base UI's default pre-highlighted-row focus.

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the project-scope menu focus fix against the shared Menu primitive contract and Base UI 1.4.1 behavior. The layout-effect ordering the sentinel relies on does hold in 1.4.1 (FloatingFocusManager snapshots activeElement synchronously in its own layout effect and bails when focus is already inside the popup), so the mechanism itself is sound. Three smaller issues remain: a render-time ref read that the React Compiler rejects, a popup focus() call that does not mirror Base UI's preventScroll, and ownership of the behavior sitting at one call site while an identical menu composition exists elsewhere.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
@IzonIcy

Copy link
Copy Markdown
Author

Before (main): opening the project-scope menu highlights the first project row even though the pointer never left the trigger.

t3-pr7950-before-bug

After (fix branch): identical interaction — no row is highlighted until the pointer or keyboard actually touches one.

t3-pr7950-after-fix

Both captured on an isolated dev environment — same machine, same seeded project, menu opened with a mouse click each time. For extra rigor: on main, DOM inspection confirms the project row ends up data-highlighted=true with focus stolen from the trigger; with the fix, focus stays on the popup element and no item receives data-highlighted. Keyboard opens are unchanged.

@IzonIcy
IzonIcy marked this pull request as ready for review August 23, 2026 02:17
CopilotAI lite review requested due to automatic review settings August 23, 2026 02:17

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 437a995. Configure here.

Comment threadapps/web/src/components/Sidebar.tsx
@macroscopeapp

macroscopeappBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a focused, opt-in UI focus correction limited to the project-scope menu, with keyboard behavior and other menus preserved. An unresolved Medium finding identifies a popup-ref merging risk, which remains a separate correctness gate under the repository’s configured threshold.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

Addresses the three Macroscope UI-consistency findings on this PR:
- The sentinel no longer reaches through MenuPopup's internal DOM via
closest(). MenuPopup now owns a ref to its own popup element and
exposes an opt-in focusOnMountRef prop; the FocusPopupOnMount helper
lives beside it so the Base UI ordering workaround is documented in
one place.
- The mount focus now passes preventScroll:true, matching Base UI's own
open-focus behavior while the popup is still unpositioned.
- projectScopeOpenedWithPointerRef.current is no longer read during
render. The ref object is passed down and read inside the layout
effect, keeping the component React Compiler-safe.
Verified: pnpm exec tsgo --noEmit, vp lint on changed files, menu +
sidebar unit tests.
Model: ox-alpha (opencode/x-preview-f-free), opencode
className,
)}
data-slot="menu-popup"
ref={popupRef}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumui/menu.tsx:103

When a caller supplies ref, {...props} overwrites popupRef, so FocusPopupOnMount sees popupRef.current === null and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ui/menu.tsx around line 103:
When a caller supplies `ref`, `{...props}` overwrites `popupRef`, so `FocusPopupOnMount` sees `popupRef.current === null` and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.
Evidence trail:
3fe9398
apps/web/src/components/ui/menu.tsx:25-40
apps/web/src/components/ui/menu.tsx:54-108
apps/web/package.json:15,42

@IzonIcy

Copy link
Copy Markdown
Author

Attaching the before/after captures promised in the description (hosted on the fork's pr-evidence/7950 branch to keep PR-only assets out of the repo):

Before — pointer open, no hover: t3-demo-project row wrongly highlighted (autofocus landed on its gear):

before

After — pointer open: focus lands on the popup, project row neutral:

after

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings in the shared MenuPopup primitive. The behavior goal (no pre-highlighted row on pointer opens) looks right; the concern is where the behavior lives and how it is wired.

Posted via Macroscope — UI Consistency

Comment on lines +25 to +39
function FocusPopupOnMount({
enabled,
popupRef,
}: {
enabled: { current: boolean };
popupRef: { current: HTMLElement | null };
}) {
useLayoutEffect(() => {
if (!enabled.current) {
return;
}
// preventScroll matches Base UI's own open-focus behavior: this runs
// before the positioner has positioned the popup, so a scrolling focus
// could jump ancestor scrollers.
popupRef.current?.focus({ preventScroll: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts popup focus ownership in a hidden child whose correctness depends on React running its layout effect before Base UI's focus manager, and it pushes interaction-type detection out to every consumer (Sidebar.tsx now tracks onPointerDown/onKeyDown on the trigger by hand). Base UI's popups already own both halves of this: Menu.Popup accepts initialFocus — the sibling of the finalFocus prop this repo already uses on a Base UI popup in CommandPalette.tsx:535 — as a ref or a callback that receives the open interaction type, and the focus manager consults it instead of being raced by effect ordering.

Suggest replacing FocusPopupOnMount + focusOnMountRef with initialFocus on MenuPrimitive.Popup, returning popupRef.current for non-keyboard opens and falling through to the default for keyboard opens (verify the exact callback return contract with a typecheck against @base-ui/react@1.5.0). That keeps the fix declarative, survives Base UI upgrades that change when the focus manager runs, and lets MenuPopup apply it by default — the same nested-control-in-first-row pattern exists at ProjectScriptsControl.tsx:214, which the current opt-in ref prop does not cover.

No diff: the fix spans the prop signature and the Sidebar trigger handlers.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/ui/menu.tsx
@t3dotgg

Copy link
Copy Markdown
Member

Note

🤖 GPT-5.6 Sol responding on behalf of Theo

We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together.

Closing this alternate menu focus policy. The same project-menu highlight is covered by #7916. This version adds a shared layout-effect focus API and relies on open-time scheduling details. Keep the pointer-versus-keyboard reproduction in the selected fix and use the menu primitive's supported initial-focus behavior.

If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. If GitHub does not let you reopen it, leave a comment here and we'll take another look.

@t3dotggt3dotgg closed this Aug 27, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M30-99 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar project menu highlights the first project on open without hover

3 participants

@IzonIcy@t3dotgg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(web): stop project-scope menu from highlighting first row without hover - #7950

Closed
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus
Closed

fix(web): stop project-scope menu from highlighting first row without hover#7950
IzonIcy wants to merge 6 commits into
pingdotgg:mainfrom
IzonIcy:fix/sidebar-project-menu-autofocus

Conversation

@IzonIcy

@IzonIcyIzonIcy commented Aug 23, 2026

Copy link
Copy Markdown

What changed

apps/web/src/components/Sidebar.tsx: when the sidebar project-scope menu ("All projects" dropdown) is opened with a pointer, focus is redirected to the popup element itself during commit. Base UI's open-time autofocus — which queues a microtask and skips when focus already sits inside the popup — then no-ops instead of landing on the first tabbable element, the settings gear inside the first project row.

Keyboard opens keep Base UI's default behavior (the checked row is pre-highlighted and receives focus itself). Arrow-key navigation, Tab-to-gear, and mouse clicks on the gear are unchanged; only the misdirected open focus moved.

Why

Fixes#7915: opening this menu with the mouse rendered the first project row as highlighted although the pointer was elsewhere, because every row is tabIndex=-1 until highlighted and focus bubbling into the gear marked its row active.

Evidence

Menu opened with the pointer, nothing hovered.

Before — the t3-demo-project row renders highlighted because Base UI's open autofocus landed on its settings gear:

before: project row highlighted without hover

After — focus lands on the popup itself; the project row stays neutral (the checked "All projects" row keeps its selection background, as intended):

after: project row neutral on pointer open

Images live on the fork's pr-evidence/7950 branch so no PR-only assets land in the repo.

Verification

  • Scoped typecheck clean (tsgo --noEmit, 0 errors); lint clean
  • Focus-redirect mechanics verified against Base UI 1.5.0 internals (FloatingFocusManager skips its queued autofocus when focus is already inside the floating element)

--
Worked by ox-alpha via opencode (x-preview-f-free).

Note

Fix project-scope menu highlighting first row when opened by pointer

  • Adds a projectScopeOpenedWithPointerRef in Sidebar.tsx set via onPointerDown/onKeyDown on the menu trigger, distinguishing pointer opens from keyboard opens
  • Adds a FocusPopupOnMount component and optional focusOnMountRef prop to MenuPopup in menu.tsx; when enabled, the popup element itself receives focus on mount instead of the first tabbable descendant
  • When the project-scope menu is opened with a pointer, the popup is focused to avoid pre-highlighting the first row; keyboard opens retain the default focus behavior

Macroscope summarized 3a0f2aa.


Note

Low Risk
Localized focus-management workaround for one menu; keyboard behavior and other menus are unchanged unless they opt into focusOnMountRef.

Overview
Fixes incorrect row highlight when the sidebar All projects menu opens via click: Base UI autofocus was landing on the first row’s nested settings gear instead of leaving rows neutral.

MenuPopup gains an optional focusOnMountRef and a FocusPopupOnMount layout effect that focuses the popup container (with preventScroll) when the ref is true at mount, so Base UI’s focus manager skips autofocus to the first tabbable descendant.

Sidebar records pointer vs keyboard on the project-scope trigger (onPointerDown / onKeyDown) and passes that ref into MenuPopup so pointer opens redirect focus to the popup; keyboard opens keep the default pre-highlighted checked row.

Reviewed by Cursor Bugbot for commit 3a0f2aa. Bugbot is set up for automated code reviews on this repo. Configure here.

… hover
Base UI focuses the first tabbable element when a menu opens with the
mouse. Inside the sidebar project-scope menu every row is tabIndex=-1
until highlighted, so the first tabbable element was the settings gear
inside the first project row; focus bubbling into that row made Base UI
render it as highlighted although the pointer was elsewhere. Keyboard
opens were unaffected because the checked row is pre-highlighted and
receives focus itself.
When the trigger is activated by pointer, focus the popup element during
commit instead: Base UI's queued open-time autofocus sees focus is
already inside the popup and skips. Keyboard opens keep the default
behavior; arrow keys, Tab-to-gear, and mouse clicks on the gear still
work as before.
@coderabbitai

coderabbitaiBot commented Aug 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ad38b52-28e7-4308-a0c5-3cdefa322497

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 23, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding: the open-focus redirect is a component-local workaround that depends on Base UI's internal focus timing, while the actual cause (a tabbable Button nested inside a menu radio item) is left in place. Details inline.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
The Sidebar-level layout effect raced Base UI's FloatingFocusManager:
it only helps if the popup mounts in the same commit, and the manager's
autofocus decision can still land on the settings gear otherwise.
Move the redirect into the popup subtree as a sentinel component. React
runs child layout effects before the popup's own effect, so the manager
snapshots activeElement already inside the popup and skips its queued
autofocus deterministically, regardless of mount timing. Behavior is
unchanged for keyboard opens (sentinel not mounted) and for arrow keys,
Tab-to-gear, and gear clicks.
@IzonIcy

Copy link
Copy Markdown
Author

Fixed in 437a995 — the timing critique was fair, so the redirect moved from a Sidebar-level layout effect into the popup's own subtree: a hidden sentinel child focuses [data-slot='menu-popup'] during its layout effect, which React runs before the popup component's FloatingFocusManager effect (child-first). The manager therefore snapshots activeElement already inside the floating element and skips its queued autofocus outright — no dependency on which commit the popup mounts in, and no reliance on the enqueueFocus shouldFocus re-check. The sentinel only mounts for pointer-opened menus; keyboard opens keep Base UI's default pre-highlighted-row focus.

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the project-scope menu focus fix against the shared Menu primitive contract and Base UI 1.4.1 behavior. The layout-effect ordering the sentinel relies on does hold in 1.4.1 (FloatingFocusManager snapshots activeElement synchronously in its own layout effect and bails when focus is already inside the popup), so the mechanism itself is sound. Three smaller issues remain: a render-time ref read that the React Compiler rejects, a popup focus() call that does not mirror Base UI's preventScroll, and ownership of the behavior sitting at one call site while an identical menu composition exists elsewhere.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
Comment threadapps/web/src/components/Sidebar.tsx Outdated
@IzonIcy

Copy link
Copy Markdown
Author

Before (main): opening the project-scope menu highlights the first project row even though the pointer never left the trigger.

t3-pr7950-before-bug

After (fix branch): identical interaction — no row is highlighted until the pointer or keyboard actually touches one.

t3-pr7950-after-fix

Both captured on an isolated dev environment — same machine, same seeded project, menu opened with a mouse click each time. For extra rigor: on main, DOM inspection confirms the project row ends up data-highlighted=true with focus stolen from the trigger; with the fix, focus stays on the popup element and no item receives data-highlighted. Keyboard opens are unchanged.

@IzonIcy
IzonIcy marked this pull request as ready for review August 23, 2026 02:17
CopilotAI lite review requested due to automatic review settings August 23, 2026 02:17

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 437a995. Configure here.

Comment threadapps/web/src/components/Sidebar.tsx
@macroscopeapp

macroscopeappBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a focused, opt-in UI focus correction limited to the project-scope menu, with keyboard behavior and other menus preserved. An unresolved Medium finding identifies a popup-ref merging risk, which remains a separate correctness gate under the repository’s configured threshold.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

Addresses the three Macroscope UI-consistency findings on this PR:
- The sentinel no longer reaches through MenuPopup's internal DOM via
closest(). MenuPopup now owns a ref to its own popup element and
exposes an opt-in focusOnMountRef prop; the FocusPopupOnMount helper
lives beside it so the Base UI ordering workaround is documented in
one place.
- The mount focus now passes preventScroll:true, matching Base UI's own
open-focus behavior while the popup is still unpositioned.
- projectScopeOpenedWithPointerRef.current is no longer read during
render. The ref object is passed down and read inside the layout
effect, keeping the component React Compiler-safe.
Verified: pnpm exec tsgo --noEmit, vp lint on changed files, menu +
sidebar unit tests.
Model: ox-alpha (opencode/x-preview-f-free), opencode
className,
)}
data-slot="menu-popup"
ref={popupRef}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumui/menu.tsx:103

When a caller supplies ref, {...props} overwrites popupRef, so FocusPopupOnMount sees popupRef.current === null and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ui/menu.tsx around line 103:
When a caller supplies `ref`, `{...props}` overwrites `popupRef`, so `FocusPopupOnMount` sees `popupRef.current === null` and the opt-in popup is not focused; the caller's ref is also the only ref attached. Merge the internal and caller refs instead of allowing the spread to replace the internal ref.
Evidence trail:
3fe9398
apps/web/src/components/ui/menu.tsx:25-40
apps/web/src/components/ui/menu.tsx:54-108
apps/web/package.json:15,42

@IzonIcy

Copy link
Copy Markdown
Author

Attaching the before/after captures promised in the description (hosted on the fork's pr-evidence/7950 branch to keep PR-only assets out of the repo):

Before — pointer open, no hover: t3-demo-project row wrongly highlighted (autofocus landed on its gear):

before

After — pointer open: focus lands on the popup, project row neutral:

after

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings in the shared MenuPopup primitive. The behavior goal (no pre-highlighted row on pointer opens) looks right; the concern is where the behavior lives and how it is wired.

Posted via Macroscope — UI Consistency

Comment on lines +25 to +39
function FocusPopupOnMount({
enabled,
popupRef,
}: {
enabled: { current: boolean };
popupRef: { current: HTMLElement | null };
}) {
useLayoutEffect(() => {
if (!enabled.current) {
return;
}
// preventScroll matches Base UI's own open-focus behavior: this runs
// before the positioner has positioned the popup, so a scrolling focus
// could jump ancestor scrollers.
popupRef.current?.focus({ preventScroll: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts popup focus ownership in a hidden child whose correctness depends on React running its layout effect before Base UI's focus manager, and it pushes interaction-type detection out to every consumer (Sidebar.tsx now tracks onPointerDown/onKeyDown on the trigger by hand). Base UI's popups already own both halves of this: Menu.Popup accepts initialFocus — the sibling of the finalFocus prop this repo already uses on a Base UI popup in CommandPalette.tsx:535 — as a ref or a callback that receives the open interaction type, and the focus manager consults it instead of being raced by effect ordering.

Suggest replacing FocusPopupOnMount + focusOnMountRef with initialFocus on MenuPrimitive.Popup, returning popupRef.current for non-keyboard opens and falling through to the default for keyboard opens (verify the exact callback return contract with a typecheck against @base-ui/react@1.5.0). That keeps the fix declarative, survives Base UI upgrades that change when the focus manager runs, and lets MenuPopup apply it by default — the same nested-control-in-first-row pattern exists at ProjectScriptsControl.tsx:214, which the current opt-in ref prop does not cover.

No diff: the fix spans the prop signature and the Sidebar trigger handlers.

Posted via Macroscope — UI Consistency

Comment threadapps/web/src/components/ui/menu.tsx
@t3dotgg

Copy link
Copy Markdown
Member

Note

🤖 GPT-5.6 Sol responding on behalf of Theo

We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together.

Closing this alternate menu focus policy. The same project-menu highlight is covered by #7916. This version adds a shared layout-effect focus API and relies on open-time scheduling details. Keep the pointer-versus-keyboard reproduction in the selected fix and use the menu primitive's supported initial-focus behavior.

If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. If GitHub does not let you reopen it, leave a comment here and we'll take another look.

@t3dotggt3dotgg closed this Aug 27, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M30-99 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar project menu highlights the first project on open without hover

3 participants

@IzonIcy@t3dotgg