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 ; }