Skip to content

fix(chat): render the ephemeral create options inline at phone width - #7327

Merged
bolichen97 merged 1 commit into
kirodotdev:mainfrom
ash663:fix/ephemeral-menu-mobile-inline
Sep 1, 2026
Merged

fix(chat): render the ephemeral create options inline at phone width#7327
bolichen97 merged 1 commit into
kirodotdev:mainfrom
ash663:fix/ephemeral-menu-mobile-inline

Conversation

@ghost

@ghost ghost commented Aug 31, 2026

Copy link
Copy Markdown

Problem / Motivation

At phone width, the New ephemeral chat entry in the sidebar's + New caret menu (added in #6857) opens its options as a Radix flyout submenu. A flyout pins to its trigger's side and only shifts vertically on collision, and at phone width there is no horizontal room beside the parent menu — so it opens mostly past the left viewport edge. Measured at 390×844: the flyout renders at x = −181 with a 264px width; the Incognito/Temporary labels are unreadable, only the tail of each gloss shows.

Why it matters

On a phone the feature is effectively unusable: the two options can't be read, and tapping blind is the only way to use them. #6909 already established that flyouts don't work at phone width and converted the folder picker and crew submenu to inline lists — this entry merged in parallel and missed the treatment, so it's the one remaining submenu in this menu that still clips.

What changed (motivation → approach → change)

Goal: make the ephemeral options readable and tappable at phone width without touching the desktop experience. Approach: apply exactly the pattern #6909 codified for the sibling submenus in this same menu — at isMobile, render the rows inline under a DropdownMenuLabel caption; wider viewports keep the flyout.

Change, in website/src/pages/ChatSidebar.tsx (one hunk):

  • The two ephemeral rows are hoisted into a shared ephemeralRows fragment (mirroring the folder picker's folderRows), so the mobile and desktop branches render the same rows and cannot drift.
  • isMobile (already in scope — it gates the folder picker a few lines below) selects between the inline caption + rows and the existing DropdownMenuSub flyout. The flyout branch's markup, handlers, testids, and disabled states are unchanged; the caption copies the folder caption's styling and reuses the existing pages.chatSidebar.new_ephemeral_chat key. No new i18n keys, no changes to the shared dropdown component.

Tests

website/src/test/ChatSidebar.ephemeralCreate.test.tsx grows from 4 to 6 tests. The useIsMobile hook is mocked with the same mutable-box approach ChatSidebar.narrowDurationPickers.test.tsx (#6909's test) uses, defaulting to desktop so the 4 existing flyout tests are unaffected. New: (a) at mobile the caret menu shows the ephemeral caption with both rows inline and no flyout trigger; (b) at mobile, activating a row still issues the create with the correct memory_mode (and the run mode stays non-orchestrator).

Full frontend suite on the branch: 26,889 passed, 0 real failures (the 17 Electron reds are the known missing-nested-dev-deps set, reproduced on the base). tsc -b, eslint, jscpd clean.

Manual verification

Ran the branch under a dev server at a 390×844 viewport and exercised the real menu; the screenshots below are captures from that session (scripted Playwright). Desktop flyout re-verified unchanged by the pre-existing tests.

Screenshots / video

Before — the flyout opens off the left edge (parent menu at x≈78, flyout at x = −181; only gloss tails visible):

Before: ephemeral submenu clipped off-screen at phone width

After — caption + both options inline, fully readable:

After: ephemeral options inline under a caption at phone width

Pattern harvest

Pattern: a DropdownMenuSub flyout added to the sidebar create menu without the isMobile inline branch — at phone width a Radix flyout pins to the trigger's side and clips off-viewport. This is a recurring class, not a one-off: #6909 fixed the folder and crew submenus, and this entry (developed in parallel) recurred it.

Rule candidate: ast-grep/semgrep — flag <DropdownMenuSub> added in a component that imports useIsMobile when the JSX is not guarded by an isMobile conditional. Durable alternative: extract the #6909 branch into a shared ResponsiveMenuSub wrapper (flyout on desktop, caption + inline rows on mobile) so new submenus get the correct behavior by construction.

Related Issues

Fixes #7326

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A
  • No secrets, credentials, or internal references in the diff

The 'New ephemeral chat' entry in the + New caret dropdown was an unconditional Radix flyout submenu. At phone width a flyout pins to the trigger's side and only shifts vertically, so it opened past the left viewport edge and the Incognito/Temporary rows were unreadable. Apply the same treatment the folder picker and crew submenu received in kirodotdev#6909: at phone width, list the two ephemeral rows inline under a caption label; wider viewports keep the flyout unchanged. Rows are hoisted into a shared fragment (folderRows pattern) so the two branches cannot drift. Two mobile-viewport tests added alongside the existing four desktop tests.
@ghost
ghost self-requested a review August 31, 2026 18:01
@ghost
ghost self-requested a review as a code owner August 31, 2026 18:01
@ghost
ghost requested a review from CrysisDeu August 31, 2026 18:01
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 96929c2d34609d056e715137f962a2200bff86ab via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 96929c2

@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 96929c2d34609d056e715137f962a2200bff86ab via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The change faithfully replicates the #6909 pattern already used twice in this same menu (folder picker at line 5254, crew submenu at 5306), shares the rows between branches to prevent drift, and the temp-screenshots/ convention is established (2,800+ committed files, named a "committed deliverable" in .gitignore). Tests cover both branches and pin that the caption is not a sub-trigger. No design-level issues survive the kill-filter — the third duplication of the inline-vs-flyout branch is real, but the author already proposed the ResponsiveMenuSub extraction as a follow-up in the description.

Design-Verdict: PASS

A real phone-width clipping bug fixed with the exact pattern its sibling submenus already use; shared rows prevent branch drift.

[DESIGN-REVIEWED] 96929c2

@github-actions

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — ✅ PASS

UX-level review of 96929c2d34609d056e715137f962a2200bff86ab via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The network fetch isn't permitted in this sandbox, so I'll review the added screenshots from the diff and description as the instructions direct. I have everything needed: the change applies, verbatim, the same isMobile inline-caption pattern the folder picker already uses in this exact menu (base ChatSidebar.tsx:5267-5278) — same DropdownMenuLabel styling, same reused i18n keys, identical rows/handlers/disabled states in both branches via the shared fragment, and tests pin that the caption is not a menuitem and the memory mode still rides the create call. No new strings, no new interaction patterns, no state or feedback changes.

UX-Verdict: PASS

Fixes an unusable off-screen flyout by reusing the exact inline-caption pattern this menu already teaches; nothing new for a user to learn.

[UX-REVIEWED] 96929c2

@github-actions

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 96929c2d34609d056e715137f962a2200bff86ab via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Verification is complete. The review:

First-Principles-Verdict: PASS

A measured phone-width defect (flyout at x = −181) gets the exact treatment #6909 already codified for its sibling submenus; nothing rides along.

What this change ships

Intent: make the ephemeral-chat create options readable and tappable on a phone — a FIX.

  1. At phone width, Incognito/Temporary list inline under a caption instead of an off-screen flyout — justified (defect measured, fixes New ephemeral chat submenu opens off-screen at phone width #7326)
  2. Desktop flyout, handlers, testids unchanged — justified (scope containment)
  3. Two before/after PNGs under temp-screenshots/ephemeral-menu-mobile/ — justified (committed-deliverables convention, .gitignore:97; ~700 sibling dirs exist)
  4. Two tests plus a per-test useIsMobile mock — justified (pins the new branch and the shared handlers)

Checks run: no shared responsive-submenu wrapper exists to reuse (ResponsiveMenuSub|MenuSubOrInline: 0 hits in website/src); every other DropdownMenuSub in ChatSidebar.tsx (5281, 5325, 5602, 5718) is already isMobile-guarded, so this was the last unguarded one in the menu, as the description claims; the inline shape byte-matches the folder/crew idiom at 5270/5314.

Watch

  • This is the fifth copy of the if (isMobile) {caption + inline} else {DropdownMenuSub} branch in ChatSidebar.tsx (5270, 5314, 5568, 5686, plus this one). The description itself names the general fix (a shared responsive-submenu wrapper) and defers it — accepted-and-deferred, but the count is now high enough that the next recurrence should ship the wrapper instead of a sixth copy.
  • One cross-file instance of the named class remains: WebPreviewPanel.tsx:949 has an unguarded DropdownMenuSub in a component that imports useIsMobile (line 10). Whether it clips at phone width is unverified — its trigger is right-aligned — so it is a candidate to check, not a defect claim.

[FIRST-PRINCIPLES-REVIEWED] 96929c2

@github-actions

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 96929c2d34609d056e715137f962a2200bff86ab via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 96929c2

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 31, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix(chat) renders ephemeral create options inline at phone width, clear root cause (menu clipped at narrow width). CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.

@bolichen97
bolichen97 merged commit d87f28d into kirodotdev:main Sep 1, 2026
79 of 80 checks passed

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix with a clear root cause — the ephemeral create options overflowed their popover at phone width, and the fix renders them inline instead. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.

@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 1, 2026
@ghost
ghost deleted the fix/ephemeral-menu-mobile-inline branch September 7, 2026 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New ephemeral chat submenu opens off-screen at phone width

2 participants