Skip to content

refactor(ui): isolate theme selector state #864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
benvinegar wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(ui): isolate theme selector state #864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .changeset/fresh-themes-refactor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
---
---
195 changes: 43 additions & 152 deletions src/ui/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ import { useExtensionDialogController } from "./hooks/useExtensionDialogControll
import { useExtensionNotifications } from "./hooks/useExtensionNotifications";
import { useHunkSessionBridge } from "./hooks/useHunkSessionBridge";
import { useMenuController } from "./hooks/useMenuController";
import { useThemeSelectorController } from "./hooks/useThemeSelectorController";
import {
useTerminalReview,
type AgentNoteGeometrySnapshot,
Expand Down Expand Up @@ -139,15 +140,9 @@ import { verifyWorkspaceWriteTarget } from "./lib/workspaceWriteGuard";
import { openSelectedFileInEditor } from "./lib/openInEditor";
import { resolveResponsiveLayout } from "./lib/responsive";
import { resizeSidebarWidth } from "./lib/sidebar";
import { availableThemes, resolveTheme, withTransparentSurfaces } from "./themes";

type FocusArea = "files" | "filter" | "note";
type ActiveAddNoteTarget = ActiveAddNoteAffordance & { fileId: string };
type ThemeSelectorState = {
open: boolean;
selectedIndex: number;
previewThemeId: string | null;
};

const FAST_CODE_HORIZONTAL_SCROLL_COLUMNS = 8;

Expand Down Expand Up @@ -297,17 +292,26 @@ export function App({
const cancelCopySelectionRef = useRef<(() => void) | null>(null);
const [layoutToggleRequestId, setLayoutToggleRequestId] = useState(0);
const [transientNoticeText, setTransientNoticeText] = useState<string | null>(null);
const transientTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
/** Show a short-lived status-bar notice and replace any pending notice timer. */
const showTransientNotice = useCallback((text: string, durationMs = 3000) => {
if (transientTimerRef.current !== null) {
clearTimeout(transientTimerRef.current);
}
setTransientNoticeText(text);
transientTimerRef.current = setTimeout(() => {
transientTimerRef.current = null;
setTransientNoticeText((current) => (current === text ? null : current));
}, durationMs);
}, []);
useEffect(() => {
return () => {
if (transientTimerRef.current !== null) {
clearTimeout(transientTimerRef.current);
}
};
}, []);
const [layoutMode, setLayoutMode] = useState<LayoutMode>(bootstrap.initialMode);
const [themeId, setThemeId] = useState(
() =>
resolveTheme(
bootstrap.initialTheme,
bootstrap.initialThemeMode ?? renderer.themeMode,
bootstrap.customThemes,
).id,
);
// Soft reloads replace bootstrap without re-running startup terminal theme detection.
const [detectedThemeMode] = useState(() => bootstrap.initialThemeMode);
const [showLineNumbers, setShowLineNumbers] = useState(bootstrap.initialShowLineNumbers ?? true);
const [wrapLines, setWrapLines] = useState(bootstrap.initialWrapLines ?? false);
const [copyDecorations, setCopyDecorations] = useState(bootstrap.initialCopyDecorations ?? false);
Expand All @@ -319,11 +323,6 @@ export function App({
}>({ id: 0, alignment: "center" });
const [showHunkHeaders, setShowHunkHeaders] = useState(bootstrap.initialShowHunkHeaders ?? true);
const [showMenuBar, setShowMenuBar] = useState(bootstrap.initialShowMenuBar ?? true);
const [themeSelectorState, setThemeSelectorState] = useState<ThemeSelectorState>({
open: false,
selectedIndex: 0,
previewThemeId: null,
});
const [sidebarVisible, setSidebarVisible] = useState(() => !pagerMode);
const [forceSidebarOpen, setForceSidebarOpen] = useState(
() => !pagerMode && bootstrap.initialSidebar === true,
Expand Down Expand Up @@ -392,33 +391,26 @@ export function App({
const offeredTrustRepoRootsRef = useRef<Set<string>>(new Set());
const extensionTrustPromptOpen = extensionTrustPromptRoot !== null;

const themeOptions = useMemo(
() => availableThemes(bootstrap.customThemes),
[bootstrap.customThemes],
);
const effectiveThemeId = themeSelectorState.previewThemeId ?? themeId;
const baseTheme = useMemo(
() => resolveTheme(effectiveThemeId, detectedThemeMode ?? null, bootstrap.customThemes),
[effectiveThemeId, detectedThemeMode, bootstrap.customThemes],
);
const activeTheme = useMemo(
() =>
bootstrap.input.options.transparentBackground
? withTransparentSurfaces(baseTheme)
: baseTheme,
[baseTheme, bootstrap.input.options.transparentBackground],
);

const themeSelectorItems = useMemo(
() =>
themeOptions.map((theme) => ({
id: theme.id,
label: theme.label,
description: theme.id === activeTheme.id ? "active" : "",
active: theme.id === activeTheme.id,
})),
[activeTheme.id, themeOptions],
);
const {
activeTheme,
baseTheme,
themeId,
themeSelectorItems,
themeSelectorOpen,
themeSelectorSelectedIndex,
acceptThemeSelector,
acceptThemeSelectorItem,
closeThemeSelector,
moveThemeSelector,
openThemeSelector,
previewThemeSelectorItem,
} = useThemeSelectorController({
customThemes: bootstrap.customThemes,
initialTheme: bootstrap.initialTheme,
initialThemeMode: bootstrap.initialThemeMode ?? renderer.themeMode,
onTransientNotice: showTransientNotice,
transparentBackground: bootstrap.input.options.transparentBackground ?? false,
});
const currentViewPreferences = useMemo<PersistedViewPreferences>(
() => ({
mode: layoutMode,
Expand Down Expand Up @@ -1483,30 +1475,6 @@ export function App({
setCopyDecorations((current) => !current);
};

// Show a short-lived status-bar message. Used to surface clipboard-copy outcomes that would
// otherwise be invisible to the user (OSC52 unsupported, etc.).
// Track the timer so we can clear it on unmount and avoid React state updates after unmount.
const transientTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const showTransientNotice = useCallback((text: string, durationMs = 3000) => {
if (transientTimerRef.current !== null) {
clearTimeout(transientTimerRef.current);
}
setTransientNoticeText(text);
transientTimerRef.current = setTimeout(() => {
transientTimerRef.current = null;
setTransientNoticeText((current) => (current === text ? null : current));
}, durationMs);
}, []);

// Clear any pending transient-notice timer on unmount to avoid state updates after unmount.
useEffect(() => {
return () => {
if (transientTimerRef.current !== null) {
clearTimeout(transientTimerRef.current);
}
};
}, []);

/** Toggle whether diff code rows wrap instead of truncating to one terminal row. */
const toggleLineWrap = () => {
// Capture the pre-toggle viewport position synchronously so DiffPane can restore the same
Expand All @@ -1524,83 +1492,6 @@ export function App({
reportedThemeIdRef.current = themeId;
}, [extensions, themeId]);

/** Switch the active theme. */
const selectTheme = useCallback(
(nextThemeId: string) => {
const nextTheme = themeOptions.find((theme) => theme.id === nextThemeId);
setThemeId(nextThemeId);
showTransientNotice(`Theme: ${nextTheme?.label ?? nextThemeId}`);
},
[showTransientNotice, themeOptions],
);

/** Open the keyboard-driven theme selector with the current theme highlighted. */
const openThemeSelector = useCallback(() => {
const currentIndex = themeSelectorItems.findIndex((item) => item.id === activeTheme.id);
setThemeSelectorState({
open: true,
selectedIndex: Math.max(0, currentIndex),
previewThemeId: null,
});
}, [activeTheme.id, themeSelectorItems]);

const closeThemeSelector = useCallback(() => {
// Dropping the preview id reverts all previewed colors in the same state transition.
setThemeSelectorState((current) => ({ ...current, open: false, previewThemeId: null }));
}, []);

const moveThemeSelector = useCallback(
(delta: number) => {
setThemeSelectorState((current) => {
if (themeSelectorItems.length === 0) {
return { ...current, selectedIndex: 0, previewThemeId: null };
}

const nextIndex =
(current.selectedIndex + delta + themeSelectorItems.length) % themeSelectorItems.length;
const item = themeSelectorItems[nextIndex]!;
return { ...current, selectedIndex: nextIndex, previewThemeId: item.id };
});
},
[themeSelectorItems],
);

/** Preview the theme under the pointer without committing it. */
const previewThemeSelectorItem = useCallback(
(index: number) => {
const item = themeSelectorItems[index];
if (!item) {
return;
}

setThemeSelectorState((current) => ({
...current,
selectedIndex: index,
previewThemeId: item.id,
}));
},
[themeSelectorItems],
);

/** Commit one theme and close the selector. */
const acceptThemeSelectorItem = useCallback(
(index: number) => {
const item = themeSelectorItems[index];
if (!item) {
return;
}

selectTheme(item.id);
// Close without a preview id; the committed theme id now supplies the same effective theme.
setThemeSelectorState((current) => ({ ...current, open: false, previewThemeId: null }));
},
[selectTheme, themeSelectorItems],
);

const acceptThemeSelector = useCallback(() => {
acceptThemeSelectorItem(themeSelectorState.selectedIndex);
}, [acceptThemeSelectorItem, themeSelectorState.selectedIndex]);

/** Toggle only the active files pane without changing extension pane visibility. */
const toggleFilesPane = () => {
const filesPaneKey = resolvePaneSlotKey({
Expand Down Expand Up @@ -2164,7 +2055,7 @@ export function App({
showHelp,
switchMenu,
toggleFocusArea,
themeSelectorOpen: themeSelectorState.open,
themeSelectorOpen,
});

/** Start a mouse drag for one resizable pane. */
Expand Down Expand Up @@ -2604,11 +2495,11 @@ export function App({
</ConfirmDialog>
) : null}

{themeSelectorState.open ? (
{themeSelectorOpen ? (
<Suspense fallback={null}>
<LazyThemeSelectorDialog
items={themeSelectorItems}
selectedIndex={themeSelectorState.selectedIndex}
selectedIndex={themeSelectorSelectedIndex}
terminalHeight={terminal.height}
terminalWidth={terminal.width}
theme={baseTheme}
Expand Down
93 changes: 91 additions & 2 deletions src/ui/AppHost.interactions.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { tmpdir } from "node:os";
import { join } from "node:path";
import { describe, expect, mock, test } from "bun:test";
import { testRender } from "@opentui/react/test-utils";
import { act } from "react";
import { act, useState } from "react";
import { SESSION_BROKER_REGISTRATION_VERSION } from "@hunk/session-broker-core";
import type { HunkSessionBrokerClient } from "../session/broker/brokerClient";
import type {
Expand All @@ -19,9 +19,10 @@ import { capturedTestColorToHex } from "../../test/helpers/test-color-helpers";
import { createTestDiffFile as buildTestDiffFile, lines } from "../../test/helpers/diff-helpers";
import { createEmptyExtensionLoadResult } from "../extensions/types";
import { AGENT_SKILL_COMMAND, AGENT_SKILL_PROMPT } from "./components/chrome/AgentSkillDialog";
import { resolveTheme } from "./themes";
import { availableThemes, resolveTheme } from "./themes";

@greptile-apps greptile-apps Bot Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Avoid dynamic test imports

The newly added await import("./App") conflicts with the repository testing guideline requiring imports to be hoisted to the top level, making module initialization order less explicit; use a static top-level import instead.

Context Used: testing.mdc Cursor rule (source)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/ui/AppHost.interactions.test.tsx
Line: 23

Comment:
**Avoid dynamic test imports**

The newly added `await import("./App")` conflicts with the repository testing guideline requiring imports to be hoisted to the top level, making module initialization order less explicit; use a static top-level import instead.

**Context Used:** testing.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/testing.mdc))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

const { loadAppBootstrap } = await import("../core/changeset/loaders");
const { App } = await import("./App");
const { AppHost } = await import("./AppHost");

const TEST_KEY_PAGE_UP = "\x1B[5~";
Expand Down Expand Up @@ -1077,6 +1078,94 @@ describe("App interactions", () => {
}
});

test("theme events report only explicit acceptance, not previews or catalog projection", async () => {
const custom = {
id: "session-custom",
label: "Session custom",
base: "github-dark-default",
accent: "#8877cc",
};
const bootstrap = createSingleFileBootstrap();
const extensions = createEmptyExtensionLoadResult(process.cwd());
const themeEvents: string[] = [];
extensions.registry.eventHandlers.theme_changed.push({
extensionId: "theme-probe",
handler: ({ themeId }) => {
themeEvents.push(themeId);
},
});
bootstrap.initialTheme = custom.id;
bootstrap.customThemes = [custom];
bootstrap.extensions = extensions;
let replaceCustomThemes!: (themes: AppBootstrap["customThemes"]) => void;

function ThemeEventProbe() {
const [currentBootstrap, setCurrentBootstrap] = useState(bootstrap);
replaceCustomThemes = (themes) =>
setCurrentBootstrap((current) => ({ ...current, customThemes: themes }));
return (
<App
bootstrap={currentBootstrap}
onRegisterWorkspaceRefreshRequest={() => () => {}}
onReloadSession={async () => {
throw new Error("Theme event test does not reload the session.");
}}
onWorkspaceWriteCompleted={() => {}}
runWorkspaceWrite={async (write) => {
await write();
return true;
}}
/>
);
}

const setup = await testRender(<ThemeEventProbe />, { width: 240, height: 24 });
try {
await flush(setup);
expect(themeEvents).toEqual([]);

await act(async () => {
await setup.mockInput.typeText("t");
});
await waitForFrame(setup, (frame) => frame.includes("Theme selector"));
await act(async () => {
await setup.mockInput.pressArrow("down");
});
await flush(setup);
expect(themeEvents).toEqual([]);

await act(async () => {
await setup.mockInput.pressEscape();
});
await waitForFrame(setup, (frame) => !frame.includes("Theme selector"));
expect(themeEvents).toEqual([]);

await act(async () => replaceCustomThemes([]));
await flush(setup);
expect(themeEvents).toEqual([]);

await act(async () => replaceCustomThemes([custom]));
await flush(setup);
expect(themeEvents).toEqual([]);

await act(async () => {
await setup.mockInput.typeText("t");
});
await waitForFrame(setup, (frame) => frame.includes("Theme selector"));
await act(async () => {
await setup.mockInput.pressArrow("down");
await setup.mockInput.pressEnter();
});
await flush(setup);

expect(themeEvents).toEqual([availableThemes([custom])[0]!.id]);
} finally {
await act(async () => {
setup.renderer.destroy();
});
}
});

test("keyboard shortcut can wrap long lines in the app", async () => {
const setup = await testRender(<AppHost bootstrap={createWrapBootstrap()} />, {
width: 140,
Expand Down
Loading
Loading