Fix cmd+click to open in new tab for logo and Start new conversation button (#8072)

Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: sp.wack <83104063+amanape@users.noreply.github.com>
This commit is contained in:
Robert Brennan 2025-05-05 12:26:58 -04:00 committed by GitHub
parent d9c10b0164
commit 6b8286e389
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
16 changed files with 165 additions and 287 deletions

View File

@ -1,6 +1,8 @@
import { describe, it, expect, afterEach, vi } from "vitest";
import { screen, render } from "@testing-library/react";
import React from "react";
// Mock useParams before importing components
// Mock modules before importing the component
vi.mock("react-router", async () => {
const actual = await vi.importActual("react-router");
return {
@ -9,7 +11,11 @@ vi.mock("react-router", async () => {
};
});
// Mock i18next
vi.mock("#/context/conversation-context", () => ({
useConversation: () => ({ conversationId: "test-conversation-id" }),
ConversationProvider: ({ children }: { children: React.ReactNode }) => children,
}));
vi.mock("react-i18next", async () => {
const actual = await vi.importActual("react-i18next");
return {
@ -23,38 +29,56 @@ vi.mock("react-i18next", async () => {
};
});
import { screen } from "@testing-library/react";
import { renderWithProviders } from "../../test-utils";
// Mock redux
const mockDispatch = vi.fn();
let mockBrowserState = {
url: "https://example.com",
screenshotSrc: "",
};
vi.mock("react-redux", async () => {
const actual = await vi.importActual("react-redux");
return {
...actual,
useDispatch: () => mockDispatch,
useSelector: () => mockBrowserState,
};
});
// Import the component after all mocks are set up
import { BrowserPanel } from "#/components/features/browser/browser";
describe("Browser", () => {
afterEach(() => {
vi.clearAllMocks();
// Reset the mock state
mockBrowserState = {
url: "https://example.com",
screenshotSrc: "",
};
});
it("renders a message if no screenshotSrc is provided", () => {
renderWithProviders(<BrowserPanel />, {
preloadedState: {
browser: {
url: "https://example.com",
screenshotSrc: "",
},
},
});
// Set the mock state for this test
mockBrowserState = {
url: "https://example.com",
screenshotSrc: "",
};
render(<BrowserPanel />);
// i18n empty message key
expect(screen.getByText("BROWSER$NO_PAGE_LOADED")).toBeInTheDocument();
});
it("renders the url and a screenshot", () => {
renderWithProviders(<BrowserPanel />, {
preloadedState: {
browser: {
url: "https://example.com",
screenshotSrc:
"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mN0uGvyHwAFCAJS091fQwAAAABJRU5ErkJggg==",
},
},
});
// Set the mock state for this test
mockBrowserState = {
url: "https://example.com",
screenshotSrc: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mN0uGvyHwAFCAJS091fQwAAAABJRU5ErkJggg==",
};
render(<BrowserPanel />);
expect(screen.getByText("https://example.com")).toBeInTheDocument();
expect(screen.getByAltText("BROWSER$SCREENSHOT_ALT")).toBeInTheDocument();

View File

@ -34,10 +34,6 @@ describe("ConversationPanel", () => {
}
});
const { endSessionMock } = vi.hoisted(() => ({
endSessionMock: vi.fn(),
}));
beforeAll(() => {
vi.mock("react-router", async (importOriginal) => ({
...(await importOriginal<typeof import("react-router")>()),
@ -46,11 +42,6 @@ describe("ConversationPanel", () => {
useLocation: vi.fn(() => ({ pathname: "/conversation" })),
useParams: vi.fn(() => ({ conversationId: "2" })),
}));
vi.mock("#/hooks/use-end-session", async (importOriginal) => ({
...(await importOriginal<typeof import("#/hooks/use-end-session")>()),
useEndSession: vi.fn(() => endSessionMock),
}));
});
const mockConversations = [
@ -145,47 +136,6 @@ describe("ConversationPanel", () => {
expect(cards).toHaveLength(3);
});
it("should call endSession after deleting a conversation that is the current session", async () => {
const user = userEvent.setup();
const mockData = [...mockConversations];
const getUserConversationsSpy = vi.spyOn(OpenHands, "getUserConversations");
getUserConversationsSpy.mockImplementation(async () => mockData);
const deleteUserConversationSpy = vi.spyOn(OpenHands, "deleteUserConversation");
deleteUserConversationSpy.mockImplementation(async (id: string) => {
const index = mockData.findIndex(conv => conv.conversation_id === id);
if (index !== -1) {
mockData.splice(index, 1);
}
// Wait for React Query to update its cache
await new Promise(resolve => setTimeout(resolve, 0));
});
renderConversationPanel();
let cards = await screen.findAllByTestId("conversation-card");
const ellipsisButton = within(cards[1]).getByTestId("ellipsis-button");
await user.click(ellipsisButton);
const deleteButton = screen.getByTestId("delete-button");
// Click the second delete button
await user.click(deleteButton);
// Confirm the deletion
const confirmButton = screen.getByRole("button", { name: /confirm/i });
await user.click(confirmButton);
expect(screen.queryByRole("button", { name: /confirm/i })).not.toBeInTheDocument();
// Wait for the cards to update with a longer timeout
await waitFor(() => {
const updatedCards = screen.getAllByTestId("conversation-card");
expect(updatedCards).toHaveLength(2);
}, { timeout: 2000 });
expect(endSessionMock).toHaveBeenCalledOnce();
});
it("should delete a conversation", async () => {
const user = userEvent.setup();
const mockData = [

View File

@ -13,15 +13,7 @@ describe("App", () => {
{ Component: App, path: "/conversation/:conversationId" },
]);
const { endSessionMock } = vi.hoisted(() => ({
endSessionMock: vi.fn(),
}));
beforeAll(() => {
vi.mock("#/hooks/use-end-session", () => ({
useEndSession: vi.fn(() => endSessionMock),
}));
vi.mock("#/hooks/use-terminal", () => ({
useTerminal: vi.fn(),
}));
@ -35,44 +27,4 @@ describe("App", () => {
renderWithProviders(<RouteStub initialEntries={["/conversation/123"]} />);
await screen.findByTestId("app-route");
});
it("should call endSession if the user does not have permission to view conversation", async () => {
const getConversationSpy = vi.spyOn(OpenHands, "getConversation");
getConversationSpy.mockResolvedValue(null);
renderWithProviders(<RouteStub initialEntries={["/conversation/9999"]} />);
await waitFor(() => {
expect(endSessionMock).toHaveBeenCalledOnce();
expect(errorToastSpy).toHaveBeenCalledOnce();
});
});
it("should not call endSession if the user has permission", async () => {
const getConversationSpy = vi.spyOn(OpenHands, "getConversation");
getConversationSpy.mockResolvedValue({
conversation_id: "9999",
last_updated_at: "",
created_at: "",
title: "",
selected_repository: "",
status: "STOPPED",
});
const { rerender } = renderWithProviders(
<RouteStub initialEntries={["/conversation/9999"]} />,
);
await waitFor(() => {
expect(endSessionMock).not.toHaveBeenCalled();
expect(errorToastSpy).not.toHaveBeenCalled();
});
rerender(<RouteStub initialEntries={["/conversation"]} />);
await waitFor(() => {
expect(endSessionMock).not.toHaveBeenCalled();
expect(errorToastSpy).not.toHaveBeenCalled();
});
});
});

View File

@ -1,12 +1,26 @@
import { useSelector } from "react-redux";
import { useEffect } from "react";
import { useSelector, useDispatch } from "react-redux";
import { RootState } from "#/store";
import { BrowserSnapshot } from "./browser-snapshot";
import { EmptyBrowserMessage } from "./empty-browser-message";
import { useConversation } from "#/context/conversation-context";
import {
initialState as browserInitialState,
setUrl,
setScreenshotSrc,
} from "#/state/browser-slice";
export function BrowserPanel() {
const { url, screenshotSrc } = useSelector(
(state: RootState) => state.browser,
);
const { conversationId } = useConversation();
const dispatch = useDispatch();
useEffect(() => {
dispatch(setUrl(browserInitialState.url));
dispatch(setScreenshotSrc(browserInitialState.screenshotSrc));
}, [conversationId]);
const imgSrc =
screenshotSrc && screenshotSrc.startsWith("data:image/png;base64,")

View File

@ -1,5 +1,5 @@
import React from "react";
import { NavLink, useParams } from "react-router";
import { NavLink, useParams, useNavigate } from "react-router";
import { useTranslation } from "react-i18next";
import { I18nKey } from "#/i18n/declaration";
import { ConversationCard } from "./conversation-card";
@ -8,7 +8,6 @@ import { useDeleteConversation } from "#/hooks/mutation/use-delete-conversation"
import { ConfirmDeleteModal } from "./confirm-delete-modal";
import { LoadingSpinner } from "#/components/shared/loading-spinner";
import { useUpdateConversation } from "#/hooks/mutation/use-update-conversation";
import { useEndSession } from "#/hooks/use-end-session";
import { ExitConversationModal } from "./exit-conversation-modal";
import { useClickOutsideElement } from "#/hooks/use-click-outside-element";
@ -18,9 +17,9 @@ interface ConversationPanelProps {
export function ConversationPanel({ onClose }: ConversationPanelProps) {
const { t } = useTranslation();
const { conversationId: cid } = useParams();
const endSession = useEndSession();
const { conversationId: currentConversationId } = useParams();
const ref = useClickOutsideElement<HTMLDivElement>(onClose);
const navigate = useNavigate();
const [confirmDeleteModalVisible, setConfirmDeleteModalVisible] =
React.useState(false);
@ -48,8 +47,8 @@ export function ConversationPanel({ onClose }: ConversationPanelProps) {
{ conversationId: selectedConversationId },
{
onSuccess: () => {
if (cid === selectedConversationId) {
endSession();
if (selectedConversationId === currentConversationId) {
navigate("/");
}
},
},
@ -129,7 +128,6 @@ export function ConversationPanel({ onClose }: ConversationPanelProps) {
{confirmExitConversationModalVisible && (
<ExitConversationModal
onConfirm={() => {
endSession();
onClose();
}}
onClose={() => setConfirmExitConversationModalVisible(false)}

View File

@ -1,34 +1,23 @@
import React from "react";
import { FaListUl } from "react-icons/fa";
import { useDispatch } from "react-redux";
import posthog from "posthog-js";
import { NavLink, useLocation } from "react-router";
import { useTranslation } from "react-i18next";
import { useLocation } from "react-router";
import { useGitUser } from "#/hooks/query/use-git-user";
import { UserActions } from "./user-actions";
import { AllHandsLogoButton } from "#/components/shared/buttons/all-hands-logo-button";
import { DocsButton } from "#/components/shared/buttons/docs-button";
import { ExitProjectButton } from "#/components/shared/buttons/exit-project-button";
import { NewProjectButton } from "#/components/shared/buttons/new-project-button";
import { SettingsButton } from "#/components/shared/buttons/settings-button";
import { ConversationPanelButton } from "#/components/shared/buttons/conversation-panel-button";
import { SettingsModal } from "#/components/shared/modals/settings/settings-modal";
import { useSettings } from "#/hooks/query/use-settings";
import { ConversationPanel } from "../conversation-panel/conversation-panel";
import { useEndSession } from "#/hooks/use-end-session";
import { setCurrentAgentState } from "#/state/agent-slice";
import { AgentState } from "#/types/agent-state";
import { TooltipButton } from "#/components/shared/buttons/tooltip-button";
import { ConversationPanelWrapper } from "../conversation-panel/conversation-panel-wrapper";
import { useLogout } from "#/hooks/mutation/use-logout";
import { useConfig } from "#/hooks/query/use-config";
import { cn } from "#/utils/utils";
import { displayErrorToast } from "#/utils/custom-toast-handlers";
import { I18nKey } from "#/i18n/declaration";
export function Sidebar() {
const { t } = useTranslation();
const location = useLocation();
const dispatch = useDispatch();
const endSession = useEndSession();
const user = useGitUser();
const { data: config } = useConfig();
const {
@ -73,11 +62,6 @@ export function Sidebar() {
location.pathname,
]);
const handleEndSession = () => {
dispatch(setCurrentAgentState(AgentState.LOADING));
endSession();
};
const handleLogout = async () => {
await logout();
posthog.reset();
@ -89,34 +73,18 @@ export function Sidebar() {
<nav className="flex flex-row md:flex-col items-center justify-between w-full h-auto md:w-auto md:h-full">
<div className="flex flex-row md:flex-col items-center gap-[26px]">
<div className="flex items-center justify-center">
<AllHandsLogoButton onClick={handleEndSession} />
<AllHandsLogoButton />
</div>
<ExitProjectButton onClick={handleEndSession} />
<TooltipButton
testId="toggle-conversation-panel"
tooltip={t(I18nKey.SIDEBAR$CONVERSATIONS)}
ariaLabel={t(I18nKey.SIDEBAR$CONVERSATIONS)}
<NewProjectButton />
<ConversationPanelButton
isOpen={conversationPanelIsOpen}
onClick={() => setConversationPanelIsOpen((prev) => !prev)}
>
<FaListUl
size={22}
className={cn(
conversationPanelIsOpen ? "text-white" : "text-[#9099AC]",
)}
/>
</TooltipButton>
/>
</div>
<div className="flex flex-row md:flex-col md:items-center gap-[26px] md:mb-4">
<DocsButton />
<NavLink
to="/settings"
className={({ isActive }) =>
`${isActive ? "text-white" : "text-[#9099AC]"} mt-0.5 md:mt-0`
}
>
<SettingsButton />
</NavLink>
<SettingsButton />
<UserActions
user={
user.data ? { avatar_url: user.data.avatar_url } : undefined

View File

@ -1,19 +1,16 @@
import { useTranslation } from "react-i18next";
import { I18nKey } from "#/i18n/declaration";
import AllHandsLogo from "#/assets/branding/all-hands-logo.svg?react";
import { I18nKey } from "#/i18n/declaration";
import { TooltipButton } from "./tooltip-button";
interface AllHandsLogoButtonProps {
onClick: () => void;
}
export function AllHandsLogoButton({ onClick }: AllHandsLogoButtonProps) {
export function AllHandsLogoButton() {
const { t } = useTranslation();
return (
<TooltipButton
tooltip={t(I18nKey.BRANDING$ALL_HANDS_AI)}
ariaLabel={t(I18nKey.BRANDING$ALL_HANDS_LOGO)}
onClick={onClick}
navLinkTo="/"
>
<AllHandsLogo width={34} height={34} />
</TooltipButton>

View File

@ -0,0 +1,32 @@
import React from "react";
import { FaListUl } from "react-icons/fa";
import { useTranslation } from "react-i18next";
import { I18nKey } from "#/i18n/declaration";
import { TooltipButton } from "./tooltip-button";
import { cn } from "#/utils/utils";
interface ConversationPanelButtonProps {
isOpen: boolean;
onClick: () => void;
}
export function ConversationPanelButton({
isOpen,
onClick,
}: ConversationPanelButtonProps) {
const { t } = useTranslation();
return (
<TooltipButton
testId="toggle-conversation-panel"
tooltip={t(I18nKey.SIDEBAR$CONVERSATIONS)}
ariaLabel={t(I18nKey.SIDEBAR$CONVERSATIONS)}
onClick={onClick}
>
<FaListUl
size={22}
className={cn(isOpen ? "text-white" : "text-[#9099AC]")}
/>
</TooltipButton>
);
}

View File

@ -3,21 +3,17 @@ import { I18nKey } from "#/i18n/declaration";
import PlusIcon from "#/icons/plus.svg?react";
import { TooltipButton } from "./tooltip-button";
interface ExitProjectButtonProps {
onClick: () => void;
}
export function ExitProjectButton({ onClick }: ExitProjectButtonProps) {
export function NewProjectButton() {
const { t } = useTranslation();
const startNewProject = t(I18nKey.CONVERSATION$START_NEW);
return (
<TooltipButton
tooltip={startNewProject}
ariaLabel={startNewProject}
onClick={onClick}
navLinkTo="/"
testId="new-project-button"
>
<PlusIcon width={28} height={28} className="text-[#9099AC]" />
<PlusIcon width={28} height={28} />
</TooltipButton>
);
}

View File

@ -16,6 +16,7 @@ export function SettingsButton({ onClick }: SettingsButtonProps) {
tooltip={t(I18nKey.SETTINGS$TITLE)}
ariaLabel={t(I18nKey.SETTINGS$TITLE)}
onClick={onClick}
navLinkTo="/settings"
>
<SettingsIcon width={28} height={28} />
</TooltipButton>

View File

@ -1,12 +1,14 @@
import { Tooltip } from "@heroui/react";
import React, { ReactNode } from "react";
import { NavLink } from "react-router";
import { cn } from "#/utils/utils";
interface TooltipButtonProps {
export interface TooltipButtonProps {
children: ReactNode;
tooltip: string;
onClick?: () => void;
href?: string;
navLinkTo?: string;
ariaLabel: string;
testId?: string;
className?: React.HTMLAttributes<HTMLButtonElement>["className"];
@ -17,35 +19,66 @@ export function TooltipButton({
tooltip,
onClick,
href,
navLinkTo,
ariaLabel,
testId,
className,
}: TooltipButtonProps) {
const handleClick = (e: React.MouseEvent) => {
if (onClick) {
onClick();
e.preventDefault();
}
};
const buttonContent = (
<button
type="button"
aria-label={ariaLabel}
data-testid={testId}
onClick={onClick}
onClick={handleClick}
className={cn("hover:opacity-80", className)}
>
{children}
</button>
);
const content = href ? (
<a
href={href}
target="_blank"
rel="noreferrer noopener"
className={cn("hover:opacity-80", className)}
aria-label={ariaLabel}
>
{children}
</a>
) : (
buttonContent
);
let content;
if (navLinkTo) {
content = (
<NavLink
to={navLinkTo}
onClick={handleClick}
className={({ isActive }) =>
cn(
"hover:opacity-80",
isActive ? "text-white" : "text-[#9099AC]",
className,
)
}
aria-label={ariaLabel}
data-testid={testId}
>
{children}
</NavLink>
);
} else if (href) {
content = (
<a
href={href}
target="_blank"
rel="noreferrer noopener"
className={cn("hover:opacity-80", className)}
aria-label={ariaLabel}
data-testid={testId}
>
{children}
</a>
);
} else {
content = buttonContent;
}
return (
<Tooltip content={tooltip} closeDelay={100} placement="right">

View File

@ -1,45 +0,0 @@
import { useDispatch } from "react-redux";
import { useTranslation } from "react-i18next";
import { useEndSession } from "#/hooks/use-end-session";
import { setCurrentAgentState } from "#/state/agent-slice";
import { AgentState } from "#/types/agent-state";
import { DangerModal } from "./confirmation-modals/danger-modal";
import { ModalBackdrop } from "./modal-backdrop";
import { I18nKey } from "#/i18n/declaration";
interface ExitProjectConfirmationModalProps {
onClose: () => void;
}
export function ExitProjectConfirmationModal({
onClose,
}: ExitProjectConfirmationModalProps) {
const { t } = useTranslation();
const dispatch = useDispatch();
const endSession = useEndSession();
const handleEndSession = () => {
onClose();
dispatch(setCurrentAgentState(AgentState.LOADING));
endSession();
};
return (
<ModalBackdrop onClose={onClose}>
<DangerModal
title={t(I18nKey.EXIT_PROJECT$CONFIRM)}
description={t(I18nKey.EXIT_PROJECT$WARNING)}
buttons={{
danger: {
text: t(I18nKey.EXIT_PROJECT$TITLE),
onClick: handleEndSession,
},
cancel: {
text: t(I18nKey.BUTTON$CANCEL),
onClick: onClose,
},
}}
/>
</ModalBackdrop>
);
}

View File

@ -6,7 +6,6 @@ import { I18nKey } from "#/i18n/declaration";
import { organizeModelsAndProviders } from "#/utils/organize-models-and-providers";
import { DangerModal } from "../confirmation-modals/danger-modal";
import { extractSettings } from "#/utils/settings-utils";
import { useEndSession } from "#/hooks/use-end-session";
import { ModalBackdrop } from "../modal-backdrop";
import { ModelSelector } from "./model-selector";
import { Settings } from "#/types/settings";
@ -24,7 +23,6 @@ interface SettingsFormProps {
export function SettingsForm({ settings, models, onClose }: SettingsFormProps) {
const { mutate: saveUserSettings } = useSaveSettings();
const endSession = useEndSession();
const location = useLocation();
const { t } = useTranslation();
@ -34,19 +32,12 @@ export function SettingsForm({ settings, models, onClose }: SettingsFormProps) {
const [confirmEndSessionModalOpen, setConfirmEndSessionModalOpen] =
React.useState(false);
const resetOngoingSession = () => {
if (location.pathname.startsWith("/conversations/")) {
endSession();
}
};
const handleFormSubmission = async (formData: FormData) => {
const newSettings = extractSettings(formData);
await saveUserSettings(newSettings, {
onSuccess: () => {
onClose();
resetOngoingSession();
posthog.capture("settings_saved", {
LLM_MODEL: newSettings.LLM_MODEL,

View File

@ -1,28 +0,0 @@
import { useDispatch } from "react-redux";
import { useNavigate } from "react-router";
import {
initialState as browserInitialState,
setScreenshotSrc,
setUrl,
} from "#/state/browser-slice";
import { clearSelectedRepository } from "#/state/initial-query-slice";
export const useEndSession = () => {
const navigate = useNavigate();
const dispatch = useDispatch();
/**
* End the current session by clearing the token and redirecting to the home page.
*/
const endSession = () => {
dispatch(clearSelectedRepository());
// Reset browser state to initial values
dispatch(setUrl(browserInitialState.url));
dispatch(setScreenshotSrc(browserInitialState.screenshotSrc));
navigate("/");
};
return endSession;
};

View File

@ -5,7 +5,6 @@ import { generateAgentStateChangeEvent } from "#/services/agent-state-service";
import { addErrorMessage } from "#/state/chat-slice";
import { AgentState } from "#/types/agent-state";
import { ErrorObservation } from "#/types/core/observations";
import { useEndSession } from "./use-end-session";
import { displayErrorToast } from "#/utils/custom-toast-handlers";
interface ServerError {
@ -21,7 +20,6 @@ const isErrorObservation = (data: object): data is ErrorObservation =>
export const useHandleWSEvents = () => {
const { events, send } = useWsClient();
const endSession = useEndSession();
const dispatch = useDispatch();
React.useEffect(() => {
@ -33,7 +31,6 @@ export const useHandleWSEvents = () => {
if (isServerError(event)) {
if (event.error_code === 401) {
displayErrorToast("Session expired.");
endSession();
return;
}

View File

@ -1,6 +1,6 @@
import { useDisclosure } from "@heroui/react";
import React from "react";
import { Outlet } from "react-router";
import { Outlet, useNavigate } from "react-router";
import { useDispatch, useSelector } from "react-redux";
import { FaServer, FaExternalLinkAlt } from "react-icons/fa";
import { useTranslation } from "react-i18next";
@ -16,7 +16,6 @@ import { Controls } from "#/components/features/controls/controls";
import { clearMessages, addUserMessage } from "#/state/chat-slice";
import { clearTerminal } from "#/state/command-slice";
import { useEffectOnce } from "#/hooks/use-effect-once";
import GlobeIcon from "#/icons/globe.svg?react";
import JupyterIcon from "#/icons/jupyter.svg?react";
import TerminalIcon from "#/icons/terminal.svg?react";
@ -32,7 +31,6 @@ import {
ResizablePanel,
} from "#/components/layout/resizable-panel";
import Security from "#/components/shared/modals/security/security";
import { useEndSession } from "#/hooks/use-end-session";
import { useUserConversation } from "#/hooks/query/use-user-conversation";
import { ServedAppLabel } from "#/components/layout/served-app-label";
import { useSettings } from "#/hooks/query/use-settings";
@ -56,7 +54,7 @@ function AppContent() {
);
const { curAgentState } = useSelector((state: RootState) => state.agent);
const dispatch = useDispatch();
const endSession = useEndSession();
const navigate = useNavigate();
// Set the document title to the conversation title when available
useDocumentTitleFromState();
@ -68,7 +66,7 @@ function AppContent() {
displayErrorToast(
"This conversation does not exist, or you do not have permission to access it.",
);
endSession();
navigate("/");
}
}, [conversation, isFetched]);