diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md new file mode 100644 index 00000000..07150e7b --- /dev/null +++ b/.agents/skills/review-pr/SKILL.md @@ -0,0 +1,78 @@ +--- +name: review-pr +description: Review a pull request or local branch diff for correctness, security, lifecycle, error handling, tests, and meaningful performance risks. Use when asked to review PR changes, a branch, a commit range, or the current working tree and return a structured review. +--- + +# Review PR + +Review the requested change set and return one structured JSON review as the final response. + +## Establish scope + +1. Use the base branch, commit, or range named by the user. +2. Otherwise determine the merge base with the repository's default branch. +3. Include committed branch changes plus relevant staged, unstaged, and untracked files. +4. Use available change descriptions, specifications, and review context. +5. Focus on changed code and the unchanged call sites needed to prove or disprove a finding. + +## Review priorities + +Prioritize: + +1. Correctness, data loss, security, crashes, and broken user behavior. +2. Lifecycle, concurrency, stale state, cleanup, and error-handling problems. +3. Material performance regressions on demonstrated hot paths. +4. Missing tests only when they cover a distinct behavior or edge case. + +Avoid speculative findings. Trace relevant callers, state transitions, cleanup paths, and existing tests before reporting an issue. Do not flag untouched code unless the change makes it unsafe. + +Treat robustness ideas as suggestions unless they present a concrete correctness, security, or data-loss risk. Do not request tests that merely vary constructor inputs or fields already covered by the same behavior. + +## Finding rules + +Use these severities: + +- `critical`: security issue, data loss, crash, or broadly broken behavior. +- `important`: concrete logic, lifecycle, edge-case, or error-handling bug. +- `suggestion`: worthwhile non-blocking improvement. +- `nit`: precise cleanup with a concrete replacement. + +For every finding: + +- Cite a repository-relative path. +- Cite an exact changed line when possible; otherwise use `null`. +- Explain the failure mode and when it occurs. +- Give a concrete fix direction. +- Keep the body concise and actionable. + +Return `REQUEST_CHANGES` only when at least one `critical` or `important` finding remains. Suggestions and nits do not block approval. + +## Output + +Return only valid JSON with this shape. Do not wrap it in a Markdown fence and do not write it to disk. + +```json +{ + "verdict": "REQUEST_CHANGES", + "overview": "Short description of the reviewed change and overall assessment.", + "findings": [ + { + "severity": "important", + "path": "path/to/file.ts", + "line": 42, + "title": "Short finding title", + "body": "Failure mode, evidence, and concrete fix direction." + } + ], + "counts": { + "critical": 0, + "important": 1, + "suggestion": 0, + "nit": 0 + } +} +``` + +`verdict` must be exactly `APPROVE` or `REQUEST_CHANGES`. `findings` must be empty when no issues remain. Counts must match the findings array. + +Do not modify the repository or publish the review unless the user separately asks. diff --git a/.claude/skills/review-pr b/.claude/skills/review-pr new file mode 120000 index 00000000..effc6b72 --- /dev/null +++ b/.claude/skills/review-pr @@ -0,0 +1 @@ +../../.agents/skills/review-pr \ No newline at end of file diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 69d15905..bf10faba 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -66,5 +66,8 @@ jobs: - name: Build packages run: pnpm --filter "./packages/**" --if-present build + - name: Audit React Compiler coverage + run: pnpm check:react-compiler + - name: Run tests run: pnpm test diff --git a/.github/workflows/review-pull-requests.yml b/.github/workflows/review-pull-requests.yml index 4cd67c2b..afa94276 100644 --- a/.github/workflows/review-pull-requests.yml +++ b/.github/workflows/review-pull-requests.yml @@ -5,7 +5,7 @@ name: Review Pull Requests on: pull_request: - types: [opened, reopened, ready_for_review] + types: [opened, reopened, ready_for_review, synchronize] workflow_dispatch: inputs: pr_number: @@ -28,6 +28,10 @@ jobs: head_sha: ${{ steps.pr.outputs.head_sha }} pr_number: ${{ steps.pr.outputs.pr_number }} steps: + - name: Wait for pushes to settle + if: github.event_name == 'pull_request' + run: sleep 60 + - name: Resolve PR metadata id: pr env: @@ -294,8 +298,8 @@ jobs: raise SystemExit(f"Comment {index} has invalid range") if start_line not in locations.get(path, {}).get(side, set()): raise SystemExit(f"Comment {index} range starts outside the diff") - if start_line > line or line - start_line >= 10: - raise SystemExit(f"Comment {index} range exceeds 10 lines") + if start_line > line: + raise SystemExit(f"Comment {index} has an inverted range") normalized.append(item) payload = { diff --git a/CHANGELOG.md b/CHANGELOG.md index b6840db7..bc01807b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ Format loosely follows [Keep a Changelog](https://keepachangelog.com). ### Fixed +- Switching between notes now keeps navigation controls stable and saves pending edits to the correct file. [#167](https://github.com/bholmesdev/hubble.md/pull/167) - macOS text context menus now include Writing Tools, text services, and spelling suggestions. Thanks [@noahpatterson](https://github.com/noahpatterson) for the suggestion! [#164](https://github.com/bholmesdev/hubble.md/pull/164) - Update-check failures now show a concise, unobtrusive message instead of a raw error trace. diff --git a/apps/desktop/electron.vite.config.ts b/apps/desktop/electron.vite.config.ts index e608483b..2364516b 100644 --- a/apps/desktop/electron.vite.config.ts +++ b/apps/desktop/electron.vite.config.ts @@ -3,6 +3,7 @@ import tailwindcss from "@tailwindcss/vite"; import react from "@vitejs/plugin-react"; import { defineConfig, externalizeDepsPlugin } from "electron-vite"; import icons from "unplugin-icons/vite"; +import { reactCompilerPlugin } from "../../config/react-compiler-audit"; const devPort = Number(process.env.PORT ?? 1420); @@ -30,7 +31,11 @@ export default defineConfig({ renderer: { root: ".", plugins: [ - react(), + react({ + babel: { + plugins: [reactCompilerPlugin("desktop")], + }, + }), icons({ compiler: "jsx", jsx: "react", diff --git a/apps/desktop/package.json b/apps/desktop/package.json index d0030806..0ea36980 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -68,6 +68,7 @@ "@types/react": "^19.1.8", "@types/react-dom": "^19.1.6", "@vitejs/plugin-react": "^4.6.0", + "babel-plugin-react-compiler": "^1.0.0", "chokidar": "^4.0.3", "electron": "^42.3.1", "electron-builder": "^26.8.1", diff --git a/apps/desktop/src/App.tsx b/apps/desktop/src/App.tsx index 3654f74e..ab78b671 100644 --- a/apps/desktop/src/App.tsx +++ b/apps/desktop/src/App.tsx @@ -11,7 +11,7 @@ import { } from "@hubble.md/ui"; import { useStoreValue } from "@simplestack/store/react"; import { keymatch } from "keymatch"; -import { useCallback, useEffect, useMemo, useState } from "react"; +import { useEffect, useState } from "react"; import { toast } from "sonner"; import MingcutePencilLine from "~icons/mingcute/pencil-line"; import { HtmlAppEmptyState } from "./components/HtmlAppEmptyState"; @@ -103,6 +103,18 @@ async function revealPath(path: string | null) { } } +async function openFilePicker() { + const currentPath = viewerStore.get().currentPath; + const defaultPath = + (isChangelogPath(currentPath) ? null : currentPath) ?? + workspaceStore.get().workspacePath ?? + undefined; + const selected = await desktopApi.openFilePicker({ defaultPath }); + if (typeof selected === "string") { + await loadPath(selected); + } +} + let nextSearchRequestId = 0; /** @@ -141,15 +153,11 @@ function App() { const [dismissedVersion, setDismissedVersion] = useState(null); const [searchOpen, setSearchOpen] = useState(false); const workspaceFiles = useStoreValue(workspaceStore).files; - const paletteFiles: PaletteFile[] = useMemo( - () => - workspaceFiles.map((file) => ({ - path: file.path, - relativePath: relativeWorkspacePath(file.path, workspacePath ?? null), - modifiedAt: file.modified_at, - })), - [workspaceFiles, workspacePath], - ); + const paletteFiles: PaletteFile[] = workspaceFiles.map((file) => ({ + path: file.path, + relativePath: relativeWorkspacePath(file.path, workspacePath ?? null), + modifiedAt: file.modified_at, + })); const lastSeenVersion = useStoreValue(lastSeenVersionStore); const readyVersion = @@ -168,9 +176,9 @@ function App() { lastSeenVersion !== currentVersion ? currentVersion : null; - const markWhatsNewSeen = useCallback(() => { + const markWhatsNewSeen = () => { if (currentVersion) setLastSeenVersion(currentVersion); - }, [currentVersion]); + }; useEffect(() => { // First install has no update to announce; just record the version. @@ -179,15 +187,12 @@ function App() { } }, [currentVersion, lastSeenVersion]); - const openSettings = useCallback(() => { - setSettingsOpen(true); - }, []); - const openWhatsNew = useCallback(() => { + const openWhatsNew = () => { setSettingsOpen(false); void openChangelog(); - }, []); + }; - const installUpdate = useCallback(async () => { + const installUpdate = async () => { try { await desktopApi.installUpdate(); } catch (error) { @@ -195,16 +200,16 @@ function App() { description: error instanceof Error ? error.message : String(error), }); } - }, []); + }; - const triggerPrimaryUpdateAction = useCallback(async () => { + const triggerPrimaryUpdateAction = async () => { if (!updateState?.isSupported) return; if (updateState.status === "ready") { await installUpdate(); return; } await desktopApi.checkForUpdates(); - }, [installUpdate, updateState]); + }; useEffect(() => { const currentPath = state.currentPath; @@ -247,18 +252,6 @@ function App() { }; }, [state.currentPath]); - const openFilePicker = useCallback(async () => { - const currentPath = viewerStore.get().currentPath; - const defaultPath = - (isChangelogPath(currentPath) ? null : currentPath) ?? - workspaceStore.get().workspacePath ?? - undefined; - const selected = await desktopApi.openFilePicker({ defaultPath }); - if (typeof selected === "string") { - await loadPath(selected); - } - }, []); - useEffect(() => { const currentPath = state.currentPath; void desktopApi.setMenuState({ @@ -296,7 +289,7 @@ function App() { await createMarkdownFile(); } else if (keymatch(event, "CmdOrCtrl+,")) { event.preventDefault(); - openSettings(); + setSettingsOpen(true); } else if (keymatch(event, "CmdOrCtrl+Shift+O")) { if (!workspaceStore.get().workspacePath) return; event.preventDefault(); @@ -341,7 +334,7 @@ function App() { }; window.addEventListener("keydown", onKeyDown); return () => window.removeEventListener("keydown", onKeyDown); - }, [focusedSidebarPath, openFilePicker, openSettings]); + }, [focusedSidebarPath]); useEffect(() => { let active = true; @@ -372,8 +365,11 @@ function App() { desktopApi.onMenuCreateHtmlFile(() => void createHtmlFile()), desktopApi.onMenuOpenFile(() => void openFilePicker()), desktopApi.onMenuOpenFolder(() => void openWorkspaceWithSidebar()), - desktopApi.onMenuOpenSettings(() => openSettings()), - desktopApi.onMenuOpenChangelog(openWhatsNew), + desktopApi.onMenuOpenSettings(() => setSettingsOpen(true)), + desktopApi.onMenuOpenChangelog(() => { + setSettingsOpen(false); + void openChangelog(); + }), desktopApi.onMenuCopyAsMarkdown(() => setCopyAsMarkdownRequest((request) => request + 1), ), @@ -399,7 +395,7 @@ function App() { return () => { for (const dispose of disposers) dispose(); }; - }, [openFilePicker, openSettings, openWhatsNew]); + }, []); useEffect(() => { // Window focus can fire in bursts when switching apps, so debounce the @@ -731,40 +727,34 @@ function MarkdownEditor({ title: wikiDisplayNameForTarget(target), }; }); - const openExternalLink = useCallback( - async (href: string) => { - if (classifyHref(href) === "external") { - await desktopApi.openExternalUrl(href); + const openExternalLink = async (href: string) => { + if (classifyHref(href) === "external") { + await desktopApi.openExternalUrl(href); + return; + } + const resolved = resolveRelativeLinkPath({ + href, + currentFilePath: path, + workspacePath: workspace.workspacePath, + }); + try { + const result = await desktopApi.openPathFromLink(resolved); + if (result.kind === "markdown") await loadPath(result.path); + } catch (error) { + if (error instanceof Error && error.message.includes("Open cancelled")) { return; } - const resolved = resolveRelativeLinkPath({ - href, - currentFilePath: path, - workspacePath: workspace.workspacePath, - }); - try { - const result = await desktopApi.openPathFromLink(resolved); - if (result.kind === "markdown") await loadPath(result.path); - } catch (error) { - if ( - error instanceof Error && - error.message.includes("Open cancelled") - ) { - return; - } - if ( - hasMarkdownExtension(resolved) && - error instanceof Error && - error.message.includes("FILE_NOT_FOUND") - ) { - toast.error(`File not found: ${href.split("#", 1)[0] ?? href}`); - return; - } - throw error; + if ( + hasMarkdownExtension(resolved) && + error instanceof Error && + error.message.includes("FILE_NOT_FOUND") + ) { + toast.error(`File not found: ${href.split("#", 1)[0] ?? href}`); + return; } - }, - [path, workspace.workspacePath], - ); + throw error; + } + }; return ( void, +) { + while (true) { + const requestedWorkspacePath = workspacePathStore.get(); + if (!requestedWorkspacePath) return; + const notePath = viewerStore.get().currentPath ?? undefined; + const sessionId = await desktopApi.terminalStart(requestedWorkspacePath, { + ...(notePath ? { notePath } : {}), + ...options, + }); + if (workspacePathStore.get() === requestedWorkspacePath) { + onStart({ id: sessionId, title }); + return; + } + // The workspace changed while this shell booted. Adopting it would + // show a shell cwd'd to the old workspace, so drop it and retry. + void desktopApi.terminalStop(sessionId); + if (!terminalOpenStore.get()) return; + } +} + const LIGHT_THEME = { black: "#000000", red: "#cd3131", @@ -256,61 +289,15 @@ export function TerminalPanel() { ); }, [sessions, workspacePath]); - // Start sessions while the panel is open: a pending chat command gets its - // own tab; otherwise an empty panel boots a plain shell. sessions.length - // retries a chat launch that arrived while another session was starting. - // biome-ignore lint/correctness/useExhaustiveDependencies: startSession is render-local - useEffect(() => { - if (!isOpen || !workspacePath || isInitializingRef.current) return; - if (!pendingTerminalCommand && sessions.length > 0) return; - - isInitializingRef.current = true; - void (async () => { - try { - if (pendingTerminalCommand) { - await startSession( - pendingTerminalCommand.split(/\s+/, 1)[0] || "chat", - { initialCommand: pendingTerminalCommand }, - ); - clearPendingTerminalCommand(); - } else { - await startSession("bash"); - } - } finally { - isInitializingRef.current = false; - } - })(); - }, [isOpen, pendingTerminalCommand, workspacePath, sessions.length]); - const startSession = async ( title: string, - options: { initialCommand?: string } = {}, + options: StartSessionOptions = {}, ) => { setStartError(null); try { - while (true) { - const requestedWorkspacePath = workspacePathStore.get(); - if (!requestedWorkspacePath) return; - const notePath = viewerStore.get().currentPath ?? undefined; - const sessionId = await desktopApi.terminalStart( - requestedWorkspacePath, - { - ...(notePath ? { notePath } : {}), - ...options, - }, - ); - if (workspacePathStore.get() === requestedWorkspacePath) { - dispatch({ - type: "add", - session: { id: sessionId, title }, - }); - return; - } - // The workspace changed while this shell booted. Adopting it would - // show a shell cwd'd to the old workspace, so drop it and retry. - void desktopApi.terminalStop(sessionId); - if (!terminalOpenStore.get()) return; - } + await startTerminalSession(title, options, (session) => { + dispatch({ type: "add", session }); + }); } catch (error) { console.error("Failed to start terminal session:", error); setStartError( @@ -319,6 +306,31 @@ export function TerminalPanel() { } }; + // Start sessions while the panel is open: a pending chat command gets its + // own tab; otherwise an empty panel boots a plain shell. sessions.length + // retries a chat launch that arrived while another session was starting. + // biome-ignore lint/correctness/useExhaustiveDependencies: startSession is render-local + useEffect(() => { + if (!isOpen || !workspacePath || isInitializingRef.current) return; + if (!pendingTerminalCommand && sessions.length > 0) return; + + isInitializingRef.current = true; + const sessionPromise = pendingTerminalCommand + ? startSession(pendingTerminalCommand.split(/\s+/, 1)[0] || "chat", { + initialCommand: pendingTerminalCommand, + }) + : startSession("bash"); + void sessionPromise.then( + () => { + if (pendingTerminalCommand) clearPendingTerminalCommand(); + isInitializingRef.current = false; + }, + () => { + isInitializingRef.current = false; + }, + ); + }, [isOpen, pendingTerminalCommand, workspacePath, sessions.length]); + const handleCloseSession = async (sessionId: string) => { await desktopApi.terminalStop(sessionId); dispatch({ type: "remove", sessionId }); @@ -567,9 +579,12 @@ function TerminalInstance({ const containerRef = useRef(null); const termRef = useRef(null); const fitAddonRef = useRef(null); - // Read via ref so an unstable callback prop can never remount xterm. + // Sync before paint so xterm never sees a stale callback after a render; + // keeping it in a ref avoids remounting the terminal when the callback changes. const onExitRef = useRef(onExit); - onExitRef.current = onExit; + useLayoutEffect(() => { + onExitRef.current = onExit; + }, [onExit]); // biome-ignore lint/correctness/useExhaustiveDependencies: intentional useEffect(() => { @@ -676,11 +691,18 @@ function TerminalInstance({ fitAddonRef.current && containerRef.current?.offsetParent !== null ) { + const fitAddon = fitAddonRef.current; + const term = termRef.current; + try { + fitAddon.fit(); + } catch { + // Layout may disappear while the debounced fit is running. + } + if (!term) return; try { - fitAddonRef.current.fit(); - termRef.current?.focus(); + term.focus(); } catch { - // + // The terminal may be disposed during panel teardown. } } }, [isActive]); diff --git a/apps/desktop/src/components/useSidebarKeyboardNav.ts b/apps/desktop/src/components/useSidebarKeyboardNav.ts index 8c124c88..38a3208d 100644 --- a/apps/desktop/src/components/useSidebarKeyboardNav.ts +++ b/apps/desktop/src/components/useSidebarKeyboardNav.ts @@ -1,4 +1,4 @@ -import { type RefObject, useCallback, useEffect, useState } from "react"; +import { type RefObject, useEffect, useState } from "react"; import { EDITOR_INPUT_SELECTOR } from "../selectors"; export function useSidebarKeyboardNav({ @@ -13,10 +13,6 @@ export function useSidebarKeyboardNav({ activeIndex?: number; }) { const [focusedIndex, setFocusedIndex] = useState(null); - const getActionIndex = useCallback( - () => focusedIndex ?? (activeIndex >= 0 ? activeIndex : null), - [activeIndex, focusedIndex], - ); useEffect(() => { if (focusedIndex === null) return; @@ -25,39 +21,36 @@ export function useSidebarKeyboardNav({ ?.scrollIntoView({ block: "nearest" }); }, [focusedIndex, navRef]); - const onKeyDown = useCallback( - (event: React.KeyboardEvent) => { - if (items.length === 0) return; + const onKeyDown = (event: React.KeyboardEvent) => { + if (items.length === 0) return; - switch (event.key) { - case "ArrowDown": - case "ArrowUp": { - event.preventDefault(); - const delta = event.key === "ArrowDown" ? 1 : -1; - setFocusedIndex((prev) => { - const start = prev ?? (activeIndex >= 0 ? activeIndex : -1); - return Math.max(0, Math.min(start + delta, items.length - 1)); - }); - break; - } - case "Enter": { - const idx = getActionIndex(); - if (idx !== null && items[idx]) { - event.preventDefault(); - onSelect(items[idx]); - } - break; - } - case "Escape": { + switch (event.key) { + case "ArrowDown": + case "ArrowUp": { + event.preventDefault(); + const delta = event.key === "ArrowDown" ? 1 : -1; + setFocusedIndex((prev) => { + const start = prev ?? (activeIndex >= 0 ? activeIndex : -1); + return Math.max(0, Math.min(start + delta, items.length - 1)); + }); + break; + } + case "Enter": { + const idx = focusedIndex ?? (activeIndex >= 0 ? activeIndex : null); + if (idx !== null && items[idx]) { event.preventDefault(); - setFocusedIndex(null); - document.querySelector(EDITOR_INPUT_SELECTOR)?.focus(); - break; + onSelect(items[idx]); } + break; + } + case "Escape": { + event.preventDefault(); + setFocusedIndex(null); + document.querySelector(EDITOR_INPUT_SELECTOR)?.focus(); + break; } - }, - [items, onSelect, activeIndex, getActionIndex], - ); + } + }; return { focusedIndex, setFocusedIndex, onKeyDown }; } diff --git a/apps/desktop/src/editor/IframeView.tsx b/apps/desktop/src/editor/IframeView.tsx index d65a5bdf..00742b1d 100644 --- a/apps/desktop/src/editor/IframeView.tsx +++ b/apps/desktop/src/editor/IframeView.tsx @@ -87,7 +87,7 @@ export function IframeView({ onHeightChange, }: IframeViewProps) { const iframeRef = useRef(null); - const tokenRef = useRef(crypto.randomUUID()); + const [token] = useState(() => crypto.randomUUID()); const [error, setError] = useState(null); useEffect(() => { @@ -144,7 +144,7 @@ export function IframeView({