[Search] Close the search when opening an element from a full path tag - #4071
[Search] Close the search when opening an element from a full path tag#4071sholzer wants to merge 9 commits into
Conversation
The full path cell in the search modal's typed tabs opened the element but left the modal in front of it, so the element appeared to open in the background. A double click then hit the grid's row handler as well, which is what closed the modal. ElementTag's own onClick called openElement without closing the search, while every sibling open path — the three open-element decorators and the general tab's result item — pairs openElement with close(). Fixing it in ElementTag covers all three typed tabs plus any other element tag rendered inside the modal. ElementTag is a generic SDK component rendered both inside and outside the search, and SearchProvider only wraps the left sidebar slot, so useSearch() would throw wherever no provider is above. Added useSearchOptional() for that case; useSearch() keeps throwing for the call sites that require a provider. Fixes #1879. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The SDK component now depends on search-specific business logic, violating the component/module boundary.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates element-tag interactions to close an active search before opening an element.
Changes:
- Adds an optional search-context hook.
- Closes search when an
ElementTagopens an element.
File summaries
| File | Description |
|---|---|
use-search.ts |
Adds optional context access. |
element-tag.tsx |
Closes an enclosing search on click. |
Review details
- Files reviewed: 2/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses the review: ElementTag is an SDK-exported component, so importing the search module from it reversed the components/modules dependency boundary. ElementTag now calls onElementOpen() from a feature-neutral ElementOpenBehaviorContext, which defaults to a no-op, and knows nothing about the search. The three search listings supply close() as that callback, so the close logic sits in the search listing layer next to the OpenElementDecorator that already closes the search on row double click. Reverts useSearchOptional(), which is no longer needed. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The scoped context integration consistently fixes single-click navigation without changing behavior outside search listings.
Review details
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Renames ElementOpenBehavior to ElementClickBehavior throughout, and moves the callback to after the awaited openElement() call. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The modal-close callback currently waits for the element-opening network request to finish.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/6 changed files
- Comments generated: 1
- Review effort level: Balanced
Closes the search on the click itself rather than after the two sequential requests openElement performs, so the single click dismisses the modal immediately. Co-Authored-By: Claude <noreply@anthropic.com>
|
There was a problem hiding this comment.
More of an idea to check out to keep topics close together rather than a definite change request :)
Not so long ago I implemented select & close on doubleclick (for lisist in many to many relation fields)
Thats why I am thinking the tag click could live alongside it in the OpenElementDecorator
i.e.
They could both live in OpenElementDecorator. Decorators can wrap the ContextComponentslot with a provider, the same waywithRowSelectionContextorwithPagingContextdo. So next towith-open-element-event.tsthere could be awith-open-element-context.tsx`:
export const withOpenElementContext = (Component: AbstractDecoratorProps['ContextComponent']): AbstractDecoratorProps['ContextComponent'] => {
const OpenElementContextComponent: AbstractDecoratorProps['ContextComponent'] = () => {
const { close } = useSearch()
return (
<ElementClickBehaviorProvider onElementClick={ close }>
<Component />
</ElementClickBehaviorProvider>
)
}
return OpenElementContextComponent
}`
That would mean:
- the three listing files stay untouched
- one decorator owns both "open from search" behaviours
- any listing that composes OpenElementDecorator gets the tag close for free
Changes in this pull request
Resolves #1879
Clicking the full path in a search result opened the element but left the search modal in front of it, so the element appeared to open in the background. A double click then also hit the grid's row handler, which is what closed the modal.
ElementTag's own onClick called openElement without letting the surrounding UI react, while every sibling open path — the three open-element decorators and the general tab's result item — pairs openElement with close(). The fix covers all three typed tabs (Documents, Assets, Data Objects) plus any other element tag rendered inside the modal.
ElementTag is exported from the SDK, so it does not reference the search module. It fires onElementClick() from a feature-neutral ElementClickBehaviorContext that defaults to a no-op, and the three search listings supply close() as that callback — keeping the close logic in the search listing layer, next to the OpenElementDecorator that already closes the search on row double click. The callback fires after the awaited openElement().
Additional info
Steps to reproduce
Verified
npx eslinton both changed directories — cleannpx tsc --noEmit— clean🤖 Generated with Claude Code