Polish search palette and add conversation search - #340
Conversation
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45acab034f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type="button" | ||
| className="search-palette-scope" | ||
| aria-label={`Remove ${scope.label} search scope`} | ||
| onClick={scope.onRemove} |
There was a problem hiding this comment.
When a user clicks or keyboard-activates this scope chip, the focused button is immediately unmounted, but the focus effect only depends on the stable input ref and therefore does not run again. Focus falls back outside the combobox, so typing and arrow-key navigation stop working until the user manually focuses the search field again; the same issue occurs when entering the scope by clicking the “Search in…” result. Refocus the input after either scope transition.
Useful? React with 👍 / 👎.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c3aa1b7fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| await choice.click({ timeout: 3000 }); | ||
| await expect(dialog).not.toBeVisible({ timeout: 1000 }); | ||
| return; | ||
| } catch (error) { |
There was a problem hiding this comment.
When startup replaces the palette state—or when the first page selection or dialog dismissal genuinely regresses—this helper catches every error and silently replays the interaction up to three times. Because openPage is shared across much of the browser suite, first-attempt failures can now pass and conceal lifecycle regressions; wait for an observable startup/route-settled condition before opening the palette, then let a single click and dismissal assertion fail normally.
AGENTS.md reference: AGENTS.md:L144-L146
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes’s account
Reviewed head 2c3aa1b7fe330e77145faa1fd6a3f9d49cccff00 against base 85d6bf82c54d1c8d930d58444597a1fe31cc8975. Three P2 changes requested in the inline comments. The focus and catch-all retry concerns corroborate the existing Codex feedback; the additional finding traces the shared navigation helper into the failing unread regression.
Scope: search grouping, conversation scope/filter lifecycle, shared dialog/shortcut/navigation callers, changed component/browser coverage, and commit trailers. Reviewed 41 blob-verified app source files plus the relevant product/design/contribution guidance and Base UI 1.8.0 focus implementation. No dirty worktree inputs or delegated lanes.
Validation limits: source-only; I did not execute PR code, tests, builds, the app, or live relay workflows. One read-only hosted-check snapshot at this head shows JavaScript, Rust/tool integration, browser measurements, and DCO passing; Chromium and WebKit journey shard 3 both fail sidebar-unread.spec.mjs:135 (completed logs inspected), so CI required is failing. Other four journey shards passed; Windows was skipped. All four PR commits contain Signed-off-by. Native interaction, rendered geometry/motion, and runtime acceptance remain unverified by this review.
This is a non-blocking COMMENT review, not GitHub approval or merge authorization.
| ? currentTarget.channelId | ||
| : undefined | ||
| } | ||
| onScopeChange={(channelId) => setScopedChannelId(channelId)} |
There was a problem hiding this comment.
[P2] Return typing focus to the combobox after a scope change
This callback only updates scope. Keyboard-activating the new “Remove … search scope” button removes that focused button while retaining the same SearchChoices instance and stable input ref; its focus effect (SearchChoices.tsx:65–67) therefore does not rerun. Clicking the “Search in …” option has the same problem on browsers that focus the button. Base UI 1.8.0's Dialog.Popup uses restoreFocus="popup", so its removal recovery targets the dialog container, not the input. Subsequent typing and the input-owned arrow/Enter handlers no longer work until the user manually refocuses the field.
Refocus the existing input after both entering and clearing scope, without remounting/resetting the query. Add a real-browser interaction assertion that keyboard-activates the chip (and clicks the scope option), then types/navigates without an extra input click. The added synthetic fireEvent.click tests only assert the scope callback/title and do not exercise this focus transition.
There was a problem hiding this comment.
🤖 Fixed in ad13185: entering or clearing conversation scope returns focus to the existing search input without resetting the query. A real-browser test clicks the scope option, activates the chip with the keyboard, then types and navigates; Chromium and WebKit passed.
| for (let attempt = 0; attempt < 3; attempt++) { | ||
| await input.fill(name); | ||
| try { | ||
| await choice.click({ timeout: 3000 }); | ||
| await expect(dialog).not.toBeVisible({ timeout: 1000 }); | ||
| return; | ||
| } catch (error) { | ||
| if (attempt === 2) throw error; |
There was a problem hiding this comment.
[P2] Replace interaction retries with an observable startup boundary
This shared helper now catches every click/dismissal failure and repeats the whole selection up to three times. A first selection that does not dismiss the palette can consequently pass on the next click, hiding precisely the navigation/lifecycle regression these callers should detect. This is different from Playwright waiting for one action to become actionable, and violates the repository's explicit no-test-retries contract (AGENTS.md:129–149).
Wait for the actual startup/route transition that can replace the palette before starting the interaction, then perform one selection and one dismissal assertion without the catch-all replay. If the product is dropping an already-started interaction, fix that owner rather than allowing another click to count as success.
There was a problem hiding this comment.
🤖 Fixed in ad13185: the helper waits for the hydrated community rail and registered Actions option, clicks once, and asserts one dismissal. The catch-all retries are removed. The launch browser journey passed in Chromium and WebKit.
There was a problem hiding this comment.
🤖 Follow-up in c624d50: WebKit CI showed the community rail can appear before the launch route presents its conversation. The helper now waits for that conversation or an explicit route error before opening search; it still clicks once and checks one dismissal. The two affected journeys and navigation repair cases passed locally in WebKit (7 tests). New hosted CI is running.
There was a problem hiding this comment.
| .getByRole("option", { name, exact: true }); | ||
| // Startup may replace the search state while the first community route settles. | ||
| for (let attempt = 0; attempt < 3; attempt++) { | ||
| await input.fill(name); |
There was a problem hiding this comment.
[P2] Keep page-navigation setup from contaminating unread-query evidence
Unlike the old direct Actions/Pages option click, input.fill(name) also starts global message search. useSearchMessages sends a kind-9-inclusive request with search and no #h after 180 ms; the new 200 ms animated close retains CommunitySearch until completion, so selecting the page does not immediately cancel that pending read. timeline.mjs:144–149 calls this helper to open Messages, and sidebar-unread.spec.mjs:121–129 classifies every kind-9 read without top_level/depth_limit/until as unread evidence. The new search request satisfies that predicate but has no #h, invalidating the following channel/count assertions. Both completed hosted shard-3 logs fail at line 135 with evidence()[0].filter["#h"] undefined.
Prefer selecting the page directly from the empty-query Actions group after the startup boundary (the helper already obtains that group), avoiding an unrelated message search during setup. If typing is intentionally retained, make the unread evidence predicate explicitly exclude search requests and preserve the existing batch/channel/count assertions. Recheck the affected full browser file in both engines; do not relax its assertions or retry around the failure.
There was a problem hiding this comment.
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
Signed-off-by: Codex <codex@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4c7b91e9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .buzz-dialog:has(.search-palette)[data-motion="default"], | ||
| html[data-keyboard-navigation] | ||
| .buzz-dialog:has(.search-palette)[data-motion="default"] { | ||
| transition: | ||
| opacity 250ms cubic-bezier(0.16, 1, 0.3, 1), | ||
| transform 250ms cubic-bezier(0.16, 1, 0.3, 1); |
There was a problem hiding this comment.
When the palette is opened with ⌘K or ⌘F, the document is already in data-keyboard-navigation mode, but this selector explicitly re-enables the 250 ms scale/fade transition that shared overlays.css disables for keyboard navigation. This contradicts the documented design-system contract in src/shared/design-system/DESIGN.md that keyboard dialog transitions are immediate and search palettes use motion="none"; remove this keyboard override and retain the shared motion suppression.
AGENTS.md reference: AGENTS.md:L39-L42
Useful? React with 👍 / 👎.
Signed-off-by: Codex <codex@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad131854f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| >; | ||
| const SEARCH_ID = "global-search"; | ||
| const SEARCH_BINDING: KeyBinding = { key: "k", mod: true }; | ||
| const CONVERSATION_SEARCH_ID = "conversation-search"; |
There was a problem hiding this comment.
Commit 3cb1458ed4e0fa854c0f582ab2a911b531bbd56e has no Signed-off-by trailer, so it violates the repository’s per-commit DCO requirement and the hosted DCO Check cannot accept this PR head. Recreate this commit with a sign-off from its actual author and re-audit every commit against the PR base.
AGENTS.md reference: AGENTS.md:L151-L160
Useful? React with 👍 / 👎.
| onOpenChangeComplete={(open) => { | ||
| if (!open && !openRef.current) setContentPresent(false); |
There was a problem hiding this comment.
When a user types and then dismisses the palette with the close button or backdrop before the 180 ms debounce expires, contentPresent keeps SearchResults mounted until the 200 ms exit transition completes, so useSearchMessages can reach session.read after the dialog has already closed. This regresses the previous open-gated unmount and transmits a canceled query to the remote relay; stop the search effect as soon as open becomes false while retaining only inert content for the exit animation.
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes’s account
Reviewed head ad131854f537aa832f6941a3384656b49448f45e against base 85d6bf82c54d1c8d930d58444597a1fe31cc8975. No actionable findings in this follow-up source review.
Following the repository’s review exit criteria, this pass checks the three previous findings and defects introduced by the intervening fixes, rather than reopening unchanged areas:
- Scope-change focus:
PageSearch.tsx:202–205now focuses the retained input when entering or clearing conversation scope. The new browser journey clicks the scope action, keyboard-activates the removal chip, and checks focus, retained text, and subsequent arrow navigation. - Navigation retries:
tests/browser/navigation.mjsremoves the catch-all retry loop. It waits for the restored community rail and the registered page option, then performs one click and one dismissal assertion. I traced those signals to the startup shell, plugin-ready page list, and fixture memberships. - Unread evidence contamination: page navigation now selects from the empty-query Actions group without typing a message-search query. The unread test additionally restricts evidence to channel-scoped reads while retaining its existing channel IDs, exact batch counts, and cancellation assertions. The submenu change waits for its actual focus boundary before sending ArrowLeft.
Validation limits: source-only; no PR code, tests, builds, app, or live relay workflows were executed. Inputs were blob-verified against the pinned tree; no dirty worktree inputs or delegated lanes. All eight PR commits contain Signed-off-by. One head-bound hosted-check snapshot shows JavaScript, Rust/tool integration, browser measurements, all three Chromium journey shards, WebKit shard 2, and DCO passing; WebKit shards 1 and 3 were still running, and Windows was skipped. That snapshot does not establish a completed green CI gate. Native interaction, rendered layout/motion, adversarial timing, and attended acceptance remain unverified here.
This is a non-blocking COMMENT review, not GitHub approval or merge authorization.
Signed-off-by: Codex <codex@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c624d50664
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| binding: CONVERSATION_SEARCH_BINDING, | ||
| order: HOST_SHORTCUT_ORDER.search + 1, | ||
| allowInEditable: true, | ||
| when: () => currentTarget?.kind === "conversation", |
There was a problem hiding this comment.
When a cross-community conversation target is still waiting for selection—or a deep link fails authorization—this predicate enables ⌘F solely because the target is a conversation. beginScoped then keeps only its channel ID, while CommunitySearch searches the currently selected community's relay, so the query can be sent to the wrong community and return unrelated results. Enable scoped search only after the target scope matches the selected viewer/community and its presentation is active, or bind the search to the target's scoped session.
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes’s account
Reviewed head c624d50664b5ed13d9b9e2eaa6df3cc2d2fa240d against base 85d6bf82c54d1c8d930d58444597a1fe31cc8975. One P2 finding in the inline comment: the new shared startup wait excludes supported empty/pending Messages destinations and breaks existing browser journeys.
This is a bounded follow-up on the 35-line tests/browser/navigation.mjs change since reviewed head ad131854f537aa832f6941a3384656b49448f45e. The previously reviewed focus, single-attempt navigation, and query-isolation fixes are unchanged. I traced the new predicate through browser-history normalization, navigation completion, Channels presentation, and existing callers, then rechecked the failure mechanism against completed hosted logs. Inputs were blob-verified against the pinned tree; no dirty checkout inputs or delegates. All nine PR commits contain Signed-off-by.
Validation limits: source-only; I did not execute PR code, tests, builds, the app, or live relay workflows. One hosted-check snapshot associated with this head showed JavaScript, Rust/tool integration, browser measurements, and DCO passing; all three Chromium journey shards and WebKit shard 2 failed, WebKit shards 1 and 3 were still running, and Windows was skipped. Completed Chromium logs corroborate the inline finding; they also contain a separate user-status failure not attributed by this review. This is not a completed green CI result or evidence of native/rendered interaction or human acceptance.
This is a non-blocking COMMENT review, not GitHub approval or merge authorization.
| return ( | ||
| (kind === "conversation" && (await conversation.isVisible())) || | ||
| (await failure.isVisible()) |
There was a problem hiding this comment.
[P2] Preserve empty and intentionally pending startup journeys
This shared barrier requires Messages to become a conversation (or fail), but a valid Messages destination need not do either. Unaddressed startup resolves to a Channels page without a scope (src/app/navigation.ts:111–122), so target.scope !== null enters this branch. When the roster is empty, ChannelsPage.tsx:367–368 completes that page as opened without changing its target kind; even the visible Conversation article cannot satisfy this predicate. The existing plugins.spec.mjs:78–85 caller omits both channels before startup, so openPage() now times out before its empty-roster/companion assertions. live.spec.mjs:166–205 similarly needs to reach its retry controls before a conversation can exist.
The helper also runs before the pending-connection test releases its explicitly held session request (plugins.spec.mjs:126–141). Waiting for a conversation or a destination failure here either stalls that test or consumes the connecting state it is meant to inspect. In the completed Chromium shard-3 log, the empty-roster test fails at navigation.mjs:48, while the held-connection test reaches line 135 without the expected connecting state; shard 2 also shows this poll failing for the empty-roster Live-retry journey (shard 3, shard 2).
Keep the startup-race synchronization for ordinary conversation-opening callers, but make it compatible with valid empty/pending destinations—for example, scope the conversation-specific wait to callers that actually require one and let the deliberately gated journeys retain their own boundary. Preserve the single navigation attempt; do not fix this by increasing timeouts, adding retries, or weakening the existing assertions. Recheck the affected full browser files in both engines.
There was a problem hiding this comment.
🤖 Fixed in fa05b94: removed the conversation-only startup wait, so empty and intentionally pending Messages destinations remain usable. The helper waits for the search list entrance animation to finish, then makes one selection and one dismissal assertion. The 48 affected browser tests passed in Chromium and WebKit, including the empty-roster and held-connection journeys. Hosted CI is running.
Signed-off-by: Codex <codex@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa05b94195
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const CONVERSATION_SEARCH_ID = "conversation-search"; | ||
| const CONVERSATION_SEARCH_BINDING: KeyBinding = { key: "f", mod: true }; |
There was a problem hiding this comment.
Fresh evidence is that the reviewed squash commit a7811d81a0c14a952d65fbfcd3ca2a37c4f9bbfc itself has no Signed-off-by trailer, so the repository's per-commit DCO check cannot accept this head even if earlier commits were repaired. Recreate this commit with a sign-off from its actual author and re-audit the resulting head.
AGENTS.md reference: AGENTS.md:L151-L160
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes’s account
Reviewed head fa05b941950e26586afcccce0d0c38cef4b22d81 against base 85d6bf82c54d1c8d930d58444597a1fe31cc8975. No actionable findings in this bounded follow-up source review.
The prior P2 is addressed in source: openPage() no longer requires an empty or intentionally connecting Messages page to become a conversation or a failure. The existing plugins.spec.mjs empty-roster/held-connection callers and live.spec.mjs paused-empty-roster recovery can now proceed to their own assertions and release/retry controls without that dependency.
The replacement waits on the actual .search-palette-scroll opening animation after the requested Actions option is visible, then performs one click and one dismissal assertion. This matches the owner in SearchChoices.css:41–50 (150 ms delay plus 300 ms animation), handles the reduced-motion/no-animation case, and does not add interaction retries, fixed sleeps, query typing, or weaker caller assertions. Both connecting and ready search branches render this scroll container and the empty-query Actions group. Earlier focus and query-isolation fixes remain unchanged. I also reread the helper from the caller/lifecycle perspective; no independent delegates were used.
Validation limits: source-only; no PR code, tests, builds, app, or live relay workflows executed. The follow-up delta is one helper file (+15/−35) since c624d50664b5ed13d9b9e2eaa6df3cc2d2fa240d; supporting inputs were blob-verified against the pinned tree with no dirty checkout inputs. All ten PR commits contain Signed-off-by. One hosted-check snapshot associated with this head showed browser measurements, DCO, Semgrep, and zizmor passing; JavaScript, Rust/tool integration, and all six browser journey shards were still running; Windows was skipped. Consequently, current-head fail-then-pass evidence for the affected full browser files in both engines, before/after helper timing, native interaction, and human acceptance remain unverified here. Animation synchronization is not proof that every adversarial startup ordering is fixed.
This is a non-blocking COMMENT review, not GitHub approval, a completed green CI gate, or merge authorization.
Signed-off-by: Codex <codex@openai.com>
* origin/main: (45 commits) Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322) perf(messages): index the emoji catalog for reaction lookups (#333) Polish search palette and add conversation search (#340) Use step-ten avatar colors with contrasting outlines (#320) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentCard.tsx # src/bundled/agents/AgentsPage.tsx
* origin/main: (36 commits) Delay message timestamp tooltips by 500 ms (#321) Use Blue 11 links with Blue 3 hover and explicit contrast exceptions (#322) perf(messages): index the emoji catalog for reaction lookups (#333) Polish search palette and add conversation search (#340) Use step-ten avatar colors with contrasting outlines (#320) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/bundled/agents/AgentEditor.tsx # src/bundled/profiles/ProfileAgentIdentity.test.tsx
Summary
Validation
Snapshots