[DX-1371] Update search and Ask AI behaviour#3396
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jamiehenson
left a comment
jamiehenson
left a comment
There was a problem hiding this comment.
Mostly a heads-up plus a couple of nits — not blocking.
The bit I want to be sure of: the visible Search button now opens the modal by clicking into #inkeep-search's shadow DOM while that instance sits in a display:none holder — and that reach-in has only ever been done against the chat widget before. If Inkeep's search trigger isn't a bare <button> (or is nested differently) it silently no-ops and the button does nothing, with no error, and the jsdom tests only check it renders.
So — are we confident this works in both environments? i.e. unconfigured Inkeep (local / flags off, button inert by design) and configured Inkeep (prod / flags on, button actually opens the modal). I've kicked off a review app to poke at it.
Two nits inline. Also tiny: title typo, "behavor" → "behaviour".
~ 𝒞𝓁𝒶𝓊𝒹𝑒
| onClick={() => { | ||
| const chatContainer = document.querySelector('#inkeep-ai-chat > div'); | ||
| const chatButton = chatContainer?.shadowRoot?.querySelector('button'); | ||
|
|
||
| track('docs_ask_ai_button_clicked'); | ||
| track('docs_ask_ai_button_clicked'); | ||
|
|
||
| if (chatButton) { | ||
| chatButton.click(); | ||
| } | ||
| }} | ||
| > | ||
| <Icon name="icon-gui-sparkles-outline" size="20px" /> | ||
| <span>Ask AI</span> | ||
| </button> | ||
| )} | ||
| if (chatButton) { | ||
| chatButton.click(); | ||
| } |
There was a problem hiding this comment.
This Ask AI onClick is now a near-twin of openInkeepSearch up top — query #X > div → shadow button → track → click. Worth pulling both into one openInkeepWidget(hostSelector, trackEvent) so the shadow-DOM reach-in lives in a single place.
~ 𝒞𝓁𝒶𝓊𝒹𝑒
| expect(screen.getByText('Search')).toBeInTheDocument(); | ||
| expect(screen.getByText('Ask AI')).toBeInTheDocument(); |
There was a problem hiding this comment.
These only assert the buttons render, not that clicking them opens anything — so if the shadow-DOM open path breaks, the tests stay green. A click-wiring test (mock querySelector, assert the trigger fires) would catch that. The real shadow DOM isn't jsdom-friendly so it's partial at best, but the click path is the risky bit worth pinning down.
~ 𝒞𝓁𝒶𝓊𝒹𝑒
m-hulbert
changed the title
jamiehenson
left a comment
jamiehenson
left a comment
There was a problem hiding this comment.
Looks good in the review app — header search bar + Ask AI render identically local vs prod, and the inert local triggers behave. Good to merge after a couple of small things.
Mandatory
openInkeepWidgetcallstrack(eventName)before checking the trigger resolves, so inert clicks (local / no widget) still firedocs_search_button_clicked/docs_ask_ai_button_clickedanalytics with nothing actually opening. Reorder so we only track a real open:const openInkeepWidget = (hostSelector: string, eventName: string) => { const trigger = document.querySelector(`${hostSelector} > div`)?.shadowRoot?.querySelector('button'); if (!trigger) return; track(eventName); trigger.click(); };
Nits
SearchTriggeris a plain arrow; rest of the file types components asReact.FC. Tiny consistency thing.- The new block comments run a bit longer than the file's one-liner style — happy to leave them though, they earn it explaining the shadow-DOM reach-in.
- Worth squashing the
fixup!commit before merge.
~ 𝒞𝓁𝒶𝓊𝒹𝑒
m-hulbert
force-pushed
the
dx-1371-search
branch
from
91bad22 to
d9c17a2
Compare
m-hulbert
force-pushed
the
dx-1371-search
branch
from
d9c17a2 to
d757606
Compare
jamiehenson
left a comment
jamiehenson
left a comment
There was a problem hiding this comment.
Re-reviewed — all points addressed: track() now fires only on a real open, SearchTrigger typed as React.FC, and the branch is squashed + rebased on latest main (clean reconcile with the DX-1128 Icon migration). CI green. Approving.
~ 𝒞𝓁𝒶𝓊𝒹𝑒
Replace the production Inkeep-rendered search bar with our own design-matched trigger so the header search bar is identical in local and production. - Add a SearchTrigger button (200x36, grey fill, border, 'Search' placeholder, Cmd/K keycaps) rendered in both environments. It opens the existing Inkeep search modal on click (via the hidden #inkeep-search instance) and is inert locally; the modal itself is unchanged. - Park the Inkeep search instance in a hidden holder (#inkeep-search-holder) instead of surfacing its own bar on desktop; mobile behaviour is unchanged. - Always render the Ask AI button (drop the inkeepChatEnabled gate) so it shows locally too; it no-ops when the chat widget isn't loaded. - Update Header tests for the always-rendered trigger and Ask AI button. DX-1371 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
m-hulbert
force-pushed
the
dx-1371-search
branch
from
d757606 to
ec383cc
Compare
3 participants
Description
This PR updates the Search bar and Ask AI button to match styles between local and production. It also changes the rendering so that they're always visible in a local setup, however the triggers are disabled.
Checklist