I3eka commented on code in PR #44490:
URL: https://github.com/apache/superset/pull/44490#discussion_r4060245931
##########
superset-frontend/src/views/App.tsx:
##########
@@ -214,13 +225,17 @@ const AppContent = ({
>
<Splitter.Panel>{layoutContent}</Splitter.Panel>
<Splitter.Panel size={storedWidth} min={CHAT_PANEL_MIN_WIDTH}>
- <ChatPanelHost />
+ <OutPortal node={chatPortalNode} />
</Splitter.Panel>
</Splitter>
) : (
<>
{layoutContent}
- {hasChatExtension && <ChatFloatingHost />}
+ {hasChatExtension && (
+ <ChatFloatingHost>
+ <OutPortal node={chatPortalNode} />
+ </ChatFloatingHost>
Review Comment:
I checked this against the parent commit: `ChatFloatingHost` was already
rendered only in the non-docked branch. This PR preserves that behavior; it
moves the same panel instead of mounting a new one. It does not remove a
previously available trigger.
The docked provider supplies its panel controls. For Native AI, the header
still renders `Undock the assistant` and `Close the assistant`
(`chat.close()`); the floating trigger returns when the panel is closed. The
app regression also verifies that closing unmounts the shared panel. I don't
see a new missing-control regression here, so I am keeping this change scoped
to preserving state across display modes.
##########
superset-frontend/src/core/chat/ChatHost.tsx:
##########
@@ -116,13 +116,15 @@ export const ChatFloatingHost = () => {
Separate boundaries so a crashing panel cannot take the trigger down
with it — the trigger is the user's only way back.
*/}
- {panelOpen && mode !== 'panel' && (
- <ChatBoundary
- key={`panel-${chat.id}`}
- component={panel}
- onError={onError}
- />
- )}
+ {panelOpen &&
+ mode !== 'panel' &&
+ (children ?? (
+ <ChatBoundary
+ key={`panel-${chat.id}`}
+ component={panel}
+ onError={onError}
+ />
+ ))}
Review Comment:
This path is covered by the new `src/views/App.test.tsx`, not only
`ChatHost.test.tsx`. It renders the real `App`, `ChatFloatingHost`,
`ChatPanelHost`, and reverse portals; those components are not mocked. Opening
the chat takes the children/OutPortal branch, and the test then switches
panel/floating four times while asserting the same DOM, draft, expanded tool
result and stream subscription. It also verifies another stream chunk after
switching and cleanup on close.
The regression fails on the parent and passes with this fix; the standalone
no-children behavior remains covered in `ChatHost.test.tsx`. A second test
merely duplicating the app-level branch coverage would not exercise more of
this failure, so I am retaining the existing focused regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
