From b12d4553dd7e40bf22a59581421d801588de13ed Mon Sep 17 00:00:00 2001 From: HomeLab Agent Date: Mon, 10 Aug 2026 09:54:53 -0400 Subject: [PATCH] fix(web): refresh the project list when a Discover run ends (#808) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from real first-run use: Discovery found the user's directories and the import succeeded, but nothing refreshed — a manual browser reload was needed before the imported projects appeared. The refresh was deferred all the way to "Get started" on purpose, because on the Home mount DiscoverView exists only while the instance is empty: importing is the act that ends that condition, so refreshing mid-run made the first successful row unmount the screen the user was watching the rest of the run on. What the deferral cost was a success screen asserting "They are in the sidebar now" over a sidebar that still read "No projects yet". Refresh once, on completion, and latch Home's empty-instance decision for the life of the mount so that refresh cannot yank its own success screen — including the failed rows, which are the ones with something to say. The latch is released by "Get started", which is what `useInstanceEmpty`'s hitherto-uncalled `recheck()` was written for; RootHome now passes it to DiscoverView as `onLeave`, since a screen rendered INSTEAD of Home cannot leave by navigating to Home. Co-Authored-By: Claude --- .changeset/discover-refresh-on-completion.md | 16 ++ .../web/src/components/DiscoverView.test.tsx | 24 ++- packages/web/src/components/DiscoverView.tsx | 48 ++++- .../web/src/lib/useInstanceEmpty.test.tsx | 26 +++ packages/web/src/lib/useInstanceEmpty.ts | 44 +++- .../web/src/routes/RootHome.discover.test.tsx | 190 ++++++++++++++++++ packages/web/src/routes/RootHome.test.tsx | 23 ++- packages/web/src/routes/RootHome.tsx | 9 +- 8 files changed, 365 insertions(+), 15 deletions(-) create mode 100644 .changeset/discover-refresh-on-completion.md create mode 100644 packages/web/src/routes/RootHome.discover.test.tsx diff --git a/.changeset/discover-refresh-on-completion.md b/.changeset/discover-refresh-on-completion.md new file mode 100644 index 00000000..faf45d87 --- /dev/null +++ b/.changeset/discover-refresh-on-completion.md @@ -0,0 +1,16 @@ +--- +"@paddock/web": patch +--- + +Discover: imported projects now appear without a manual browser reload (#808) + +The first-run import run refreshes the project list the moment it finishes, so +the sidebar matches the success screen's own claim that "they are in the sidebar +now" — previously the list was refetched only by the **Get started** button, and +anyone who read the result and looked at the sidebar saw an instance that still +said "No projects yet". + +Home's empty-instance decision is now latched for the life of the mount and +released by **Get started**, so that refresh cannot unmount the screen reporting +the run: the success headline and every per-row outcome — including the rows that +failed and have something to say about it — stay on screen until the user leaves. diff --git a/packages/web/src/components/DiscoverView.test.tsx b/packages/web/src/components/DiscoverView.test.tsx index bf9ad1d2..f85f2342 100644 --- a/packages/web/src/components/DiscoverView.test.tsx +++ b/packages/web/src/components/DiscoverView.test.tsx @@ -75,7 +75,7 @@ function sessionList(over: Partial = {}): DiscoverSessions { }; } -function renderView(props: { firstRun?: boolean } = {}) { +function renderView(props: { firstRun?: boolean; onLeave?: () => void } = {}) { return render( @@ -243,6 +243,28 @@ describe("DiscoverView", () => { expect(navigate).toHaveBeenCalledWith("/"); }); + it("refreshes the project list as soon as the run ends, not only on Get started (#808)", async () => { + // The success screen says "They are in the sidebar now", and until this + // refresh that sentence was false — the sidebar still read "No projects yet" + // and a manual browser reload was the only way to see the import land. + const user = userEvent.setup(); + renderView(); + await user.click(await screen.findByRole("button", { name: /Import 1 project/ })); + await screen.findByRole("button", { name: "Get started" }); + expect(refresh).toHaveBeenCalledTimes(1); + }); + + it("tells its host to re-ask rather than relying on navigation (#808)", async () => { + // On the Home mount `navigate("/")` cannot end this screen — it already IS + // "/". `RootHome` passes its `recheck` in, and that is what hands over. + const user = userEvent.setup(); + const onLeave = vi.fn(); + renderView({ firstRun: true, onLeave }); + await user.click(await screen.findByRole("button", { name: /Import 1 project/ })); + await user.click(await screen.findByRole("button", { name: "Get started" })); + await waitFor(() => expect(onLeave).toHaveBeenCalled()); + }); + it("warns about a divergent recorded path BEFORE the import, not after", async () => { discover.mockResolvedValue( result({ candidates: [candidate({ recordedPath: "/private/home/ed/code/paddock" })] }), diff --git a/packages/web/src/components/DiscoverView.tsx b/packages/web/src/components/DiscoverView.tsx index 8bc8d418..eeb26983 100644 --- a/packages/web/src/components/DiscoverView.tsx +++ b/packages/web/src/components/DiscoverView.tsx @@ -63,8 +63,31 @@ import { * the opposite problem and gets the exclusion tally plus the two toggles that * would relax it. Rendering a blank table for either is how you get told the * feature is broken. + * + * ## Refreshing without unmounting yourself (#808) + * + * On the Home mount this component exists only while the instance is empty, so + * the very act of importing removes its own reason to be on screen. The run + * therefore refreshes the project list ONCE, on completion, and `RootHome` + * latches its empty-instance decision so that refresh cannot pull the success + * screen — and every per-row outcome the user still has to read — out from under + * them. Deferring the refresh all the way to "Get started" was the original + * answer, and it is what the bug was: the finished screen claims the projects + * are in the sidebar while the sidebar still says there are none. */ -export function DiscoverView({ firstRun = false }: { firstRun?: boolean }) { +export function DiscoverView({ + firstRun = false, + onLeave, +}: { + firstRun?: boolean; + /** + * The Home mount's way out. `RootHome` passes its `recheck` here, because a + * screen rendered INSTEAD of Home cannot leave by navigating to Home — it has + * to tell Home to ask again. The `/discover` route passes nothing: it is an + * ordinary route, and `navigate` is enough. + */ + onLeave?: () => void; +}) { const navigate = useNavigate(); const { refresh } = useProjects(); const shell = useOutletContext(); @@ -153,20 +176,31 @@ export function DiscoverView({ firstRun = false }: { firstRun?: boolean }) { }); setSubmitting(false); setFinished(true); - }, [accepted, rows, submitting]); + // The rest of the app learns about the new projects HERE, once the run is + // over — not per row (that is the mid-run unmount this screen must not + // suffer) and not only on "Get started". The success screen says "They are + // in the sidebar now", and until this call that sentence was false: the + // sidebar still read "No projects yet" and a manual browser reload was the + // only way out (#808). Safe to do now only because `RootHome` latches its + // empty-instance decision — see {@link useInstanceEmpty}. + await refresh(); + }, [accepted, rows, submitting, refresh]); /** * Leave for Home. * - * The project list is refreshed HERE rather than after each row, on purpose: - * on the Home mount this component only exists while the instance is empty, so - * refreshing mid-run would make the first successful row unmount the screen the - * user is watching the rest of the run on. + * Two steps, and the second is the one that actually moves on the Home mount: + * `navigate("/")` is a no-op when this screen already IS `/`, so what ends it + * is `onLeave` — `RootHome`'s `recheck`, releasing the latch that has been + * holding this component on screen since the run finished. The refresh is kept + * even though {@link submit} already ran one: it is the only thing standing + * between a failed background refresh and a Home rendered from a stale list. */ const getStarted = useCallback(async () => { await refresh(); + onLeave?.(); navigate("/"); - }, [navigate, refresh]); + }, [navigate, refresh, onLeave]); const imported = Object.values(outcomes).reduce((n, o) => n + o.adopted, 0); const createdCount = Object.values(outcomes).filter((o) => o.project).length; diff --git a/packages/web/src/lib/useInstanceEmpty.test.tsx b/packages/web/src/lib/useInstanceEmpty.test.tsx index 787f29d1..e091711d 100644 --- a/packages/web/src/lib/useInstanceEmpty.test.tsx +++ b/packages/web/src/lib/useInstanceEmpty.test.tsx @@ -75,4 +75,30 @@ describe("useInstanceEmpty", () => { act(() => result.current.recheck()); await waitFor(() => expect(result.current.empty).toBe(false)); }); + + it("HOLDS its answer when projects appear underneath it (#808)", async () => { + // The import run is what makes an empty instance non-empty, so an unlatched + // answer would flip the moment Discovery refreshed the project list — taking + // the success screen, and every per-row failure the user still has to read, + // off the page with it. `projects` stays live; only the ANSWER is pinned. + const { result, rerender } = renderHook(() => useInstanceEmpty()); + await waitFor(() => expect(result.current.empty).toBe(true)); + + mockProjects = [makeProject({ slug: "alpha" })]; + rerender(); + expect(result.current.empty).toBe(true); + }); + + it("releases the latch on recheck, with no undecided flicker in between", async () => { + // "Get started" is the only caller. It must land on the answer directly: + // going back through `null` would blank Home for a frame on the one screen + // whose whole tri-state exists to avoid exactly that. + const { result, rerender } = renderHook(() => useInstanceEmpty()); + await waitFor(() => expect(result.current.empty).toBe(true)); + + mockProjects = [makeProject({ slug: "alpha" })]; + rerender(); + act(() => result.current.recheck()); + expect(result.current.empty).toBe(false); + }); }); diff --git a/packages/web/src/lib/useInstanceEmpty.ts b/packages/web/src/lib/useInstanceEmpty.ts index c9da53de..d9a3e2d8 100644 --- a/packages/web/src/lib/useInstanceEmpty.ts +++ b/packages/web/src/lib/useInstanceEmpty.ts @@ -27,6 +27,22 @@ import { ROOT_KEY } from "../routes/ProjectView/urls"; * Having any project at all settles it, and that is the overwhelmingly common * case, so the extra request is issued only by an instance with no projects. A * populated instance pays nothing for this hook beyond a render. + * + * ## The answer is LATCHED, and {@link recheck} is how it is released (#808) + * + * The instance's emptiness is live data — but Home's *front door* must not be. + * Discovery is rendered instead of Home while the instance is empty, and + * importing is precisely the act that makes it non-empty, so an unlatched answer + * means the import run destroys the screen reporting it: the success headline + * and every per-row outcome vanish mid-read, replaced by the root workspace. + * That is why the refresh used to be deferred all the way to "Get started", and + * deferring it is what left the sidebar stale until a manual browser reload. + * + * So the question is asked once per mount and the answer held until someone asks + * again. `recheck` is that ask, and "Get started" is the only caller — the user + * saying they have finished reading. Latching only pins the answer, never the + * data: `projects` stays live underneath, which is the whole point (the sidebar + * populates the moment the run ends, while this screen stays put). */ export function useInstanceEmpty(): { empty: boolean | null; @@ -36,7 +52,17 @@ export function useInstanceEmpty(): { const { projects, loading } = useProjects(); const [rootHasChats, setRootHasChats] = useState(null); const [nonce, setNonce] = useState(0); - const recheck = useCallback(() => setNonce((n) => n + 1), []); + /** + * Bumping the nonce is not enough on its own: it releases the latch below in + * the SAME render that still holds the pre-recheck `rootHasChats`, which would + * re-pin the very answer we are trying to discard and never move again. The + * effect clears it too, but a frame later — so clear it here, where the two + * halves of "forget what you knew" cannot come apart. + */ + const recheck = useCallback(() => { + setRootHasChats(null); + setNonce((n) => n + 1); + }, []); const noProjects = !loading && projects.length === 0; useEffect(() => { @@ -59,7 +85,7 @@ export function useInstanceEmpty(): { }; }, [noProjects, nonce]); - const empty = loading + const answer = loading ? null : projects.length > 0 ? false @@ -67,5 +93,19 @@ export function useInstanceEmpty(): { ? null : !rootHasChats; + /** + * The latch: the first non-null answer of each generation, kept. + * + * Adjusted during render rather than in an effect, deliberately. An effect + * lands a frame late, and this hook's contract is that the undecided state is + * `null` — a one-frame `false` before the effect catches up is exactly the + * flash the tri-state exists to prevent, on exactly the fresh install it exists + * for. React re-runs the component immediately on a set-during-render, so the + * committed render is the latched one. + */ + const [latched, setLatched] = useState<{ gen: number; value: boolean } | null>(null); + if (answer !== null && latched?.gen !== nonce) setLatched({ gen: nonce, value: answer }); + const empty = latched?.gen === nonce ? latched.value : null; + return { empty, recheck }; } diff --git a/packages/web/src/routes/RootHome.discover.test.tsx b/packages/web/src/routes/RootHome.discover.test.tsx new file mode 100644 index 00000000..ab827e0c --- /dev/null +++ b/packages/web/src/routes/RootHome.discover.test.tsx @@ -0,0 +1,190 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { MemoryRouter } from "react-router-dom"; +import { RootHome } from "./RootHome"; +import { ProjectsProvider } from "../lib/projects-context"; +import { makeProject } from "../test/factories"; +import type { DiscoverResult, Project } from "../lib/types"; + +/** + * The first-run loop, end to end (#808). + * + * Reported from real use: Discovery found the user's directories and the import + * succeeded, but nothing refreshed — a manual browser reload was needed before + * the imported projects appeared. + * + * The unit tests either side of this one each pass with the bug present. The + * defect only exists where `RootHome`, `useInstanceEmpty`, `ProjectsProvider` + * and `DiscoverView` meet: the import is precisely the act that stops the + * instance being empty, which is what Discovery's own existence on Home is + * conditional on. So this file mocks the API and nothing else — every one of + * those four is the real thing, because the seam between them IS the bug. + * + * Two assertions, and they pull in opposite directions. Both must hold: + * + * - the sidebar is current the moment the run ends (the reported symptom), and + * - the success screen and its per-row outcomes are still on screen (the + * deliberate design the naive fix destroys — a refresh that flips `empty` + * unmounts the very screen reporting the result, which is worse than the bug + * because the failure rows are the ones that had something to say). + */ + +const CANDIDATE_PATH = "/home/ed/code/alpha"; + +const listProjects = vi.fn(); +const listProjectChats = vi.fn(); +const discover = vi.fn(); +const discoverSessions = vi.fn(); +const createProject = vi.fn(); +const adoptChats = vi.fn(); + +vi.mock("../lib/api", () => ({ + api: { + listProjects: (...a: unknown[]) => listProjects(...a), + listProjectChats: (...a: unknown[]) => listProjectChats(...a), + discover: (...a: unknown[]) => discover(...a), + discoverSessions: (...a: unknown[]) => discoverSessions(...a), + createProject: (...a: unknown[]) => createProject(...a), + adoptChats: (...a: unknown[]) => adoptChats(...a), + }, +})); + +// The only stand-in: `ProjectView` opens sockets and fetches a workspace, and +// this test cares solely about WHICH screen Home is showing. +vi.mock("./ProjectView", () => ({ + ProjectView: ({ root }: { root?: boolean }) => ( +
{root ? "root" : "project"}
+ ), +})); + +function discoverResult(): DiscoverResult { + return { + claudeHome: "/data/claude-home", + homeDir: "/home/ed", + scanned: 3, + candidates: [ + { + path: CANDIDATE_PATH, + name: "alpha", + suggestedSlug: "alpha", + hasGit: true, + insideHome: true, + sessionCount: 2, + filteredCount: 0, + lastSessionAt: "2026-08-01T00:00:00.000Z", + }, + ], + excluded: {}, + }; +} + +/** What `GET /api/projects` returns; flipped by the import, as on a real server. */ +let projects: Project[] = []; + +function renderHome() { + return render( + + + + + , + ); +} + +describe("RootHome + Discovery, first run", () => { + beforeEach(() => { + vi.clearAllMocks(); + projects = []; + listProjects.mockImplementation(async () => ({ projects, root: makeProject({ slug: "" }) })); + listProjectChats.mockResolvedValue([]); + discover.mockResolvedValue(discoverResult()); + discoverSessions.mockResolvedValue({ path: CANDIDATE_PATH, sessions: [], filtered: [] }); + createProject.mockImplementation(async (input: { slug?: string; path?: string }) => { + const created = makeProject({ + slug: input.slug ?? "alpha", + name: "alpha", + workingDir: input.path ?? CANDIDATE_PATH, + }); + // The server now has it, so the next `GET /api/projects` says so. + projects = [created]; + return created; + }); + adoptChats.mockResolvedValue({ adopted: ["s1", "s2"], skipped: [] }); + }); + + it("shows the imported projects without a browser reload, and keeps the result on screen", async () => { + const user = userEvent.setup(); + renderHome(); + + await user.click(await screen.findByRole("button", { name: /Import 1 project/ })); + await screen.findByRole("button", { name: "Get started" }); + + // The symptom. Before the fix this stayed at one call — the list was + // refetched only by "Get started", so anyone who read the success screen and + // looked at the sidebar saw an instance that still had nothing in it, and + // reloading the browser was the only way to move on. + await waitFor(() => expect(listProjects).toHaveBeenCalledTimes(2)); + + // …and the screen reporting the run survived that refresh. Asserted on the + // row's own sentence rather than the headline, because the per-row outcomes + // are the part that carries information a failed row cannot get twice. + expect(screen.getByText(/Imported 2 chats/)).toBeInTheDocument(); + expect(screen.queryByTestId("project-view")).not.toBeInTheDocument(); + }); + + it("hands over to the ordinary root workspace on Get started", async () => { + const user = userEvent.setup(); + renderHome(); + + await user.click(await screen.findByRole("button", { name: /Import 1 project/ })); + await user.click(await screen.findByRole("button", { name: "Get started" })); + + // `navigate("/")` cannot do this — Home already IS "/". The latch releasing + // is what ends the screen, so this fails if `onLeave` is ever dropped on the + // way through, which would strand the user on Discovery for good. + expect(await screen.findByTestId("project-view")).toHaveTextContent("root"); + expect(screen.queryByText(/Imported 2 chats/)).not.toBeInTheDocument(); + }); + + it("does not refresh between rows, only when the whole run is over", async () => { + // The original reason the refresh was deferred, and still binding: on this + // mount the first successful row would otherwise unmount the screen the user + // is watching the remaining rows on. + const user = userEvent.setup(); + discover.mockResolvedValue({ + ...discoverResult(), + candidates: [ + ...discoverResult().candidates, + { + path: "/home/ed/code/beta", + name: "beta", + suggestedSlug: "beta", + hasGit: true, + insideHome: true, + sessionCount: 1, + filteredCount: 0, + }, + ], + }); + let releaseSecond: (p: Project) => void = () => {}; + createProject.mockImplementation(async (input: { slug?: string; path?: string }) => { + const created = makeProject({ slug: input.slug ?? "x", workingDir: input.path ?? "/x" }); + projects = [...projects, created]; + if (input.slug === "beta") return new Promise((r) => (releaseSecond = r)); + return created; + }); + + renderHome(); + await user.click(await screen.findByRole("button", { name: /Import 2 projects/ })); + + // First row landed, second still in flight: one refresh — the provider's own + // mount — and no more. + await waitFor(() => expect(screen.getByText(/Imported 2 chats into “alpha”/)).toBeInTheDocument()); + expect(listProjects).toHaveBeenCalledTimes(1); + + releaseSecond(makeProject({ slug: "beta", workingDir: "/home/ed/code/beta" })); + await screen.findByRole("button", { name: "Get started" }); + await waitFor(() => expect(listProjects).toHaveBeenCalledTimes(2)); + }); +}); diff --git a/packages/web/src/routes/RootHome.test.tsx b/packages/web/src/routes/RootHome.test.tsx index d789954c..006c83d3 100644 --- a/packages/web/src/routes/RootHome.test.tsx +++ b/packages/web/src/routes/RootHome.test.tsx @@ -4,10 +4,16 @@ import { MemoryRouter } from "react-router-dom"; import { RootHome } from "./RootHome"; let empty: boolean | null = null; -vi.mock("../lib/useInstanceEmpty", () => ({ useInstanceEmpty: () => ({ empty, recheck: vi.fn() }) })); +const recheck = vi.fn(); +vi.mock("../lib/useInstanceEmpty", () => ({ useInstanceEmpty: () => ({ empty, recheck }) })); vi.mock("../components/DiscoverView", () => ({ - DiscoverView: ({ firstRun }: { firstRun?: boolean }) => ( -
{firstRun ? "first-run" : "page"}
+ DiscoverView: ({ firstRun, onLeave }: { firstRun?: boolean; onLeave?: () => void }) => ( +
+ {firstRun ? "first-run" : "page"} + +
), })); vi.mock("./ProjectView", () => ({ @@ -27,6 +33,7 @@ function renderHome() { describe("RootHome", () => { beforeEach(() => { empty = null; + recheck.mockReset(); }); it("renders Discovery as Home when the instance is empty", () => { @@ -43,6 +50,16 @@ describe("RootHome", () => { expect(screen.queryByTestId("discover")).not.toBeInTheDocument(); }); + it("gives Discovery a way to make Home re-ask (#808)", () => { + // Discovery is rendered INSTEAD of Home, so it cannot leave by navigating to + // Home. Without this prop the import run's refresh has nothing to release the + // latch and the user is stranded on the success screen. + empty = true; + renderHome(); + screen.getByRole("button", { name: "leave" }).click(); + expect(recheck).toHaveBeenCalled(); + }); + it("mounts NEITHER while the answer is still unknown", () => { // `ProjectView` opens sockets and fetches a workspace; mounting it for a beat // and then replacing it with a completely different screen is worse than a diff --git a/packages/web/src/routes/RootHome.tsx b/packages/web/src/routes/RootHome.tsx index 1d5502d2..f56e6b51 100644 --- a/packages/web/src/routes/RootHome.tsx +++ b/packages/web/src/routes/RootHome.tsx @@ -19,10 +19,15 @@ import { ProjectView } from "./ProjectView"; * heavy mount that opens sockets and fetches a workspace; rendering it for a beat * and then replacing it with a completely different screen is worse than a blank * moment, and the flash would land squarely on the fresh install this is for. + * + * The choice is made once per mount and released only by `recheck` (#808), which + * is what lets Discovery refresh the project list the moment its run ends — + * populating the sidebar — without that refresh yanking its own success screen + * off the page. See {@link useInstanceEmpty} for why the answer is latched. */ export function RootHome() { - const { empty } = useInstanceEmpty(); + const { empty, recheck } = useInstanceEmpty(); if (empty === null) return
; - if (empty) return ; + if (empty) return ; return ; }