Skip to content

Commit 522b6ea

Browse files
zaerlclaude
zaerl
and
claude
authored
Move a site's status out of SiteRow into a hook (#593)
## Why #554 took eight domains out of `SiteRow`, and the largest thing it left behind is the one nearly every hook and card reads: what the main process says about the site. Thirteen pieces of state, set by one function, sat in the middle of the component. The redesign's page header and sites list (#556) need that status from outside the site's view, and a hook is the first step to handing it to them. ## What changes - **`src/renderer/hooks/use-site-status.jsx` is the site's status.** Whether it is installed and built, the ticket it is working on, what is applied to it, the age of its trunk, and the two states the app reads and never writes over: a site the old engine made, and a merge left unfinished outside the app. With them, `loadStatus`, which reads them all in one answer, and the effect that calls it when the site's view mounts. - **Five setters are handed out**, as they were used before: linking or leaving a ticket and moving one onto trunk, the dev server's start, skipping the first-run checklist, and discarding every change. The other eight values change only through `loadStatus`. - **It is a move.** The lines are `SiteRow`'s own. - **One new journey, `site-status.spec.js`**, for the one wiring the move reshaped: the status reports to the sites list through a ref that is now an argument, and nothing read that the report arrives. This goes to trunk and not to `redesign/wpds` because nothing on screen changes; trunk is merged into the redesign branch afterwards. ## How to test this Platforms: any. This has no surface of its own: the screen is the same before and after. From the repository root, `npm run test:e2e` runs the journeys, all of which read a site's status one way or another. The ones that would notice a status not read, or read wrong, are `legacy-site`, `merge-in-progress`, `pr-checkout`, `patch-apply`, `trunk-update-watch`, `store-persistence`, `build-watch` and the new `site-status`; the audit in the review outcome says which. `site-status.spec.js` seeds the store with a trunk thirty days old for a checkout whose trunk is minutes old, and expects the site's entry in the list to end up with the name of a site that has nothing to report. It is red when the report is dropped, when the hook is handed another ref, and when the date is left out of the report. **What must not have happened:** - A site's view does not read its status more often than before: `loadStatus` keeps its identity for as long as the site's path does, so the effect that calls it on mount runs once. ## Risks and limitations - **Two dependency lists gain a value** that was already stable: `loadStatus` lists the ref it reports through, and the callback that skips the checklist lists a setter that now comes from the hook. - **One effect changes place.** The read on mount now runs before the effect that clears the copy button's timer, which shares nothing with it. - **Six of the things the audit broke are not noticed by any journey**, and were not before the move. They are listed in the review outcome; none is new. - **`loadStatus` swallows a failed read**, as it did in `SiteRow`: the site then looks not installed and not built, nothing is logged, and the setup chain does not start. Fixing it would stop this being a move, so it is left for its own change. - **Discarding all changes with a patch applied has no journey.** That path does not read the status again, so one setter is all that clears the applied-patch banner. - **What is reported to the sites list is decided inside the hook**, out of the unit tests' reach. It should move to a module before #556 reads the status from outside the view. - **Checked on macOS only.** Windows is covered by CI. - **Pre-PR review, two passes: the move holds line for line; the hook's header comment was wrong in several places and is rewritten; four findings are left as follow-ups.** ## Related Follow-up to #554. Prepares #556, part of #542. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> Run as the repository's review procedure, with the judgement pass in a fresh context that took no part in the change. First pass on the move alone; second pass on the fix-up. The branch was rebased onto trunk after #592 merged, and the checks below are on the rebased head. **First pass: one finding to fix here, four follow-ups.** It diffed the hook's body against `SiteRow`'s old lines and found two differences, both intended; checked that every returned name is read only after the hook's call and that the eight setters not returned were only ever used inside `loadStatus`; and found the one effect that changed place, the copy button's timer, sharing nothing with the status. To fix: the hook's header said a finished build sets "built" (it goes through `loadStatus`; the dev server's start is what sets it), that everything comes from one read (the handed-out setters mean it does not always), and that the status is read when a site's view is opened (every listed site's view mounts as the app opens). Rewritten. Follow-ups, all in Risks above: the silent `catch`, the missing journey for a discard with a patch applied, the report to the sites list being untested, and that report's decision living in a hook. The third is closed here by `site-status.spec.js`. **Second pass: four findings, one medium, all fixed.** The medium one asked that the new journey be shown to go red, since it never sees the state it starts from: it does, three ways, as the table below records. The other three were sentences in the two headers that left a case out. **Checks, on macOS, on the head of this branch:** | Check | Result | | --- | --- | | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1733 pass, 2 skipped | | `npm run test:electron` | 1734 pass, 1 skipped | | `npm run test:e2e` (journeys) | 57 of 57 on `62c5ab5`; the head differs from it by comments, and `site-status.spec.js` was run again on it | **That it is a move.** Comparing the lines removed from `index.jsx` with the hook's, every removed line is in the hook except two dependency lists and a comment that said the trunk update was "below". No name the hook returns is read in `SiteRow` above the hook's call. **The wiring was audited by breaking it**, one thing at a time, and running the journeys: 17 breaks, 11 caught. | Break | Caught by | | --- | --- | | The status is not read when the view mounts | `build-watch` | | A legacy site is not told apart | `legacy-site` | | An unfinished merge is not seen | `merge-in-progress` | | The checked-out pull request is not known | `pr-checkout` | | An unfinished update is not known | `trunk-update-watch` | | The applied patch is not known | `patch-apply` | | The linked ticket is not read back | `store-persistence` | | Whether the site is built is not known | `build-watch` | | The dev server's start does not set "built" itself | **nothing**: it decides from its own read, and the state only changes what is shown until the next status read | | Skipping the checklist does not set it itself | **nothing**: every journey starts with the checklist already skipped | | Linking a ticket does not set it itself | **nothing**: the link reads the status straight after | | Discarding does not clear the applied patch itself | **nothing**: no journey discards with a patch applied | | The status never stops loading | **nothing**: it only disables the first-run checklist's buttons, which no journey shows; `setup-steps.cjs` has unit tests for what it does with the flag | | The status does not reach the sites list | `site-status` | | The hook is handed another ref to report through | `site-status` | | The trunk's date is left out of the report | `site-status` | | `loadStatus` returns nothing to its caller | **nothing**: only the setup chain reads it, and only the opt-in real-setup suite runs that | </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 77f4cb7 commit 522b6ea

3 files changed

Lines changed: 160 additions & 56 deletions

File tree

‎src/renderer/hooks/use-site-status.jsx‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
import { useCallback, useEffect, useState } from 'react';
2+
3+
// What the main process says about a site (#554): whether it is installed and
4+
// built, which ticket it is working on, what is applied to it, how old its
5+
// trunk is, and the two states the app only reads and refuses to write over,
6+
// a site the old engine made and a merge left unfinished outside the app.
7+
//
8+
// `loadStatus` reads all of it in one answer. It runs when the site's view
9+
// mounts, which is when the site joins the list and not when it is first
10+
// shown: for the sites already listed, as the app opens. It runs again when
11+
// whoever calls it has changed what it would say: an install or a build
12+
// ending, a ticket linked, left, switched or moved onto trunk, a ticket's
13+
// work deleted when that leaves the checkout on trunk, an apply ending
14+
// however it ends, an update, the setup chain, and the window coming back
15+
// into focus while the site is the one on screen.
16+
//
17+
// `loadStatus` returns the status as well as storing it. The setup chain
18+
// re-probes when a clone finishes and decides from that read, which React has
19+
// not committed yet. It keeps its identity for as long as `sitePath` does, and
20+
// the effect that calls it on mount depends on that: `metaPatchRef` is a ref
21+
// for the same reason, so that a parent handing down a new callback does not
22+
// read the status again.
23+
//
24+
// Not everything here comes from that one answer. Five setters are handed
25+
// out, and what they set can be newer than the rest until the next read:
26+
// linking or leaving a ticket sets `tracTicket` and `ticketBehindTrunk` and
27+
// then reads the status, so until that read answers the screen shows the new
28+
// ticket beside the old branch's applied patch; moving a ticket onto trunk clears
29+
// `ticketBehindTrunk` the same way; the dev server's start asks the main
30+
// process itself whether the site is built and copies that into `hasBuilt`;
31+
// skipping the first-run checklist sets `skipInit`; and discarding every
32+
// change clears `appliedPatch`, with no read after it. The other eight change
33+
// only through `loadStatus`.
34+
export function useSiteStatus({ sitePath, metaPatchRef }) {
35+
const [hasNodeModules, setHasNodeModules] = useState(false);
36+
const [installFailed, setInstallFailed] = useState(false);
37+
const [hasBuilt, setHasBuilt] = useState(false);
38+
const [skipInit, setSkipInit] = useState(false);
39+
const [statusLoading, setStatusLoading] = useState(true);
40+
// Trac ticket association (#109)
41+
const [tracTicket, setTracTicket] = useState(null);
42+
const [ticketBehindTrunk, setTicketBehindTrunk] = useState(false);
43+
// A site the old engine made (#385): read, never written.
44+
const [legacy, setLegacy] = useState(false);
45+
// A merge started outside the app and not finished (#352): read, and
46+
// every checkout write refused until a terminal ends it.
47+
const [mergeInProgress, setMergeInProgress] = useState(null);
48+
// What the site's status says about its trunk (#94): the date of the commit
49+
// it is on, and whether an update was left incomplete. The update itself is
50+
// useTrunkUpdate.
51+
const [trunkDate, setTrunkDate] = useState(null);
52+
const [updateIncomplete, setUpdateIncomplete] = useState(false);
53+
const [appliedPatch, setAppliedPatch] = useState(null);
54+
const [pullRequest, setPullRequest] = useState(null);
55+
56+
const loadStatus = useCallback(async ()=>{
57+
try {
58+
setStatusLoading(true);
59+
const s = await window.api.getSiteStatus(sitePath);
60+
setHasNodeModules(Boolean(s?.hasNodeModules));
61+
setInstallFailed(Boolean(s?.installFailed));
62+
setHasBuilt(Boolean(s?.hasBuilt));
63+
setSkipInit(Boolean(s?.skipInitWizard));
64+
setTrunkDate(s?.trunkDate || null);
65+
setUpdateIncomplete(Boolean(s?.updateIncomplete));
66+
setTracTicket(s?.tracTicket || null);
67+
setTicketBehindTrunk(Boolean(s?.ticketBehindTrunk));
68+
setLegacy(Boolean(s?.legacy));
69+
setMergeInProgress(s?.mergeInProgress || null);
70+
setAppliedPatch(s?.appliedPatch || null);
71+
setPullRequest(s?.pullRequest || null);
72+
if (metaPatchRef.current) {
73+
// A null trunkDate here means the git read failed (e.g. clone still
74+
// running) — keep whatever the sidebar already shows in that case.
75+
const patch = { updateIncomplete: Boolean(s?.updateIncomplete), tracTicket: s?.tracTicket || null };
76+
if (s?.trunkDate) patch.trunkDate = s.trunkDate;
77+
metaPatchRef.current(sitePath, patch);
78+
}
79+
// Returned as well as stored: the setup chain (#246) re-probes when the
80+
// clone finishes and has to decide from that read, not from state React
81+
// has not committed yet.
82+
return s;
83+
} catch {}
84+
finally { setStatusLoading(false); }
85+
return null;
86+
}, [sitePath, metaPatchRef]);
87+
useEffect(()=>{ loadStatus(); }, [loadStatus]);
88+
89+
return {
90+
hasNodeModules,
91+
installFailed,
92+
hasBuilt,
93+
setHasBuilt,
94+
skipInit,
95+
setSkipInit,
96+
statusLoading,
97+
tracTicket,
98+
setTracTicket,
99+
ticketBehindTrunk,
100+
setTicketBehindTrunk,
101+
legacy,
102+
mergeInProgress,
103+
trunkDate,
104+
updateIncomplete,
105+
appliedPatch,
106+
setAppliedPatch,
107+
pullRequest,
108+
loadStatus
109+
};
110+
}

‎src/renderer/index.jsx‎

Lines changed: 9 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ import { useSiteScripts } from './hooks/use-site-scripts.jsx';
8181
import { useBuildWatch } from './hooks/use-build-watch.jsx';
8282
import { useDevServer } from './hooks/use-dev-server.jsx';
8383
import { useTrunkUpdate } from './hooks/use-trunk-update.jsx';
84+
import { useSiteStatus } from './hooks/use-site-status.jsx';
8485
import { useSiteTicket } from './hooks/use-site-ticket.jsx';
8586
import { useApplyPatch } from './hooks/use-apply-patch.jsx';
8687
import { ConfirmationContext, useConfirmation } from './hooks/use-confirmation.jsx';
@@ -818,8 +819,8 @@ function SiteRow({ sitePath, initialized, createdAt, label, projectType = null,
818819
// completes, so the outcome is announced rather than left silent or buried in
819820
// the terminal.
820821
const confirm = useConfirmation();
821-
// Kept in a ref so loadStatus's dependency list stays [sitePath] — a
822-
// recreated callback prop must not retrigger the status-loading effect.
822+
// Kept in a ref so that `loadStatus`, in useSiteStatus, keeps its identity:
823+
// a recreated callback prop must not retrigger the status-loading effect.
823824
const metaPatchRef = useRef(onSiteMetaPatch);
824825
useEffect(() => { metaPatchRef.current = onSiteMetaPatch; }, [onSiteMetaPatch]);
825826
// What this site's processes have said (#554): the text of the Logs panel's
@@ -856,9 +857,11 @@ function SiteRow({ sitePath, initialized, createdAt, label, projectType = null,
856857
// identity, which the callbacks that list them as dependencies rely on.
857858
const mail = useSiteMail({ sitePath });
858859
const { listen: listenForMail, stopListening: stopListeningForMail, load: loadMail } = mail;
859-
const [hasNodeModules, setHasNodeModules] = useState(false);
860-
const [installFailed, setInstallFailed] = useState(false);
861-
const [hasBuilt, setHasBuilt] = useState(false);
860+
// What the main process says about this site (#554): installed, built, the
861+
// ticket, what is applied, the trunk's age, and the two states the app only
862+
// reads. `loadStatus` reads it all again, and is what every chain below
863+
// calls when it has changed what the answer would be.
864+
const { hasNodeModules, installFailed, hasBuilt, setHasBuilt, skipInit, setSkipInit, statusLoading, tracTicket, setTracTicket, ticketBehindTrunk, setTicketBehindTrunk, legacy, mergeInProgress, trunkDate, updateIncomplete, appliedPatch, setAppliedPatch, pullRequest, loadStatus } = useSiteStatus({ sitePath, metaPatchRef });
862865
// Which target this site is a checkout of (#251): the site record's field,
863866
// carried by the placeholder from the moment the dialog closes, and Core
864867
// for any site made before the field existed. `build` is what the watcher
@@ -881,21 +884,6 @@ function SiteRow({ sitePath, initialized, createdAt, label, projectType = null,
881884
() => workItemProvider(project.workItem.provider, `${project.upstream.owner}/${project.upstream.repo}`),
882885
[project]
883886
);
884-
const [skipInit, setSkipInit] = useState(false);
885-
const [statusLoading, setStatusLoading] = useState(true);
886-
// Trac ticket association (#109)
887-
const [tracTicket, setTracTicket] = useState(null);
888-
const [ticketBehindTrunk, setTicketBehindTrunk] = useState(false);
889-
// A site the old engine made (#385): read, never written.
890-
const [legacy, setLegacy] = useState(false);
891-
// A merge started outside the app and not finished (#352): read, and
892-
// every checkout write refused until a terminal ends it.
893-
const [mergeInProgress, setMergeInProgress] = useState(null);
894-
// What the site's status says about its trunk (#94): the date of the commit
895-
// it is on, and whether an update was left incomplete. The update itself is
896-
// useTrunkUpdate, below.
897-
const [trunkDate, setTrunkDate] = useState(null);
898-
const [updateIncomplete, setUpdateIncomplete] = useState(false);
899887
// Initial setup chain (#246): install then build, started by the clone
900888
// finishing rather than by a click. Same shape as the two chains below.
901889
const [setupChainState, setSetupChainState] = useState('idle'); // idle | installing | building
@@ -905,8 +893,6 @@ function SiteRow({ sitePath, initialized, createdAt, label, projectType = null,
905893
// fails — three rows above, in the case that prompted this — so the way out
906894
// is a scroll, not a fetch.
907895
const ticketPatchesRef = useRef(null);
908-
const [appliedPatch, setAppliedPatch] = useState(null);
909-
const [pullRequest, setPullRequest] = useState(null);
910896
const setupLogsRef = useRef('');
911897

912898
const siteName = pathBasename(sitePath);
@@ -999,39 +985,6 @@ function SiteRow({ sitePath, initialized, createdAt, label, projectType = null,
999985
setEditorNotice(noticeForOpenResult(result));
1000986
}, [sitePath]);
1001987

1002-
const loadStatus = useCallback(async ()=>{
1003-
try {
1004-
setStatusLoading(true);
1005-
const s = await window.api.getSiteStatus(sitePath);
1006-
setHasNodeModules(Boolean(s?.hasNodeModules));
1007-
setInstallFailed(Boolean(s?.installFailed));
1008-
setHasBuilt(Boolean(s?.hasBuilt));
1009-
setSkipInit(Boolean(s?.skipInitWizard));
1010-
setTrunkDate(s?.trunkDate || null);
1011-
setUpdateIncomplete(Boolean(s?.updateIncomplete));
1012-
setTracTicket(s?.tracTicket || null);
1013-
setTicketBehindTrunk(Boolean(s?.ticketBehindTrunk));
1014-
setLegacy(Boolean(s?.legacy));
1015-
setMergeInProgress(s?.mergeInProgress || null);
1016-
setAppliedPatch(s?.appliedPatch || null);
1017-
setPullRequest(s?.pullRequest || null);
1018-
if (metaPatchRef.current) {
1019-
// A null trunkDate here means the git read failed (e.g. clone still
1020-
// running) — keep whatever the sidebar already shows in that case.
1021-
const patch = { updateIncomplete: Boolean(s?.updateIncomplete), tracTicket: s?.tracTicket || null };
1022-
if (s?.trunkDate) patch.trunkDate = s.trunkDate;
1023-
metaPatchRef.current(sitePath, patch);
1024-
}
1025-
// Returned as well as stored: the setup chain (#246) re-probes when the
1026-
// clone finishes and has to decide from that read, not from state React
1027-
// has not committed yet.
1028-
return s;
1029-
} catch {}
1030-
finally { setStatusLoading(false); }
1031-
return null;
1032-
}, [sitePath]);
1033-
useEffect(()=>{ loadStatus(); }, [loadStatus]);
1034-
1035988
// The note's probe. It asks the wide question — unsubmitted work measured
1036989
// from the ticket's branch point, the same measurement the patch makes —
1037990
// not whether the tree has uncommitted edits (#239): under the ticket-as-
@@ -1257,7 +1210,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, projectType = null,
12571210
const markSkipWizard = useCallback(async () => {
12581211
await window.api.setSkipInitWizard(sitePath, true);
12591212
setSkipInit(true);
1260-
}, [sitePath]);
1213+
}, [sitePath, setSkipInit]);
12611214
// eslint-disable-next-line no-alert -- see the note above onRename.
12621215
const confirmAnd = async (m,a)=>{ if(window.confirm(m)) await a(); };
12631216

‎tests/e2e/journeys/site-status.spec.js‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
/**
2+
* What the sites list says about a site follows the site's status (#94, #554).
3+
*
4+
* The list is drawn from what the window keeps for every site, which it read
5+
* once, from the store, as it opened. A site's status is read later and from
6+
* the checkout itself, and it is the one that is right: the store's copy of a
7+
* trunk's date is whatever was true the last time anyone looked. So each
8+
* site's view reports what its status found to the window, and the list is
9+
* redrawn from that. Nothing else carries the answer across: the main process
10+
* writes the same date to the store, which the window reads again only when
11+
* a site is created or deleted.
12+
*
13+
* The checkout is real, made a moment ago, so its trunk is recent, and the
14+
* store is seeded to say otherwise.
15+
*
16+
* Assertions are marked INVARIANT or CHARACTERISATION; see
17+
* ticket-branches.spec.js for why.
18+
*/
19+
20+
const { test, expect } = require( '../helpers/app.cjs' );
21+
const ui = require( '../helpers/ui.cjs' );
22+
const { makeSite } = require( '../helpers/git-site.cjs' );
23+
24+
const DAY = 24 * 60 * 60 * 1000;
25+
26+
test( 'a site the store calls old loses that mark in the sites list once its status has read the checkout', async ( { session } ) => {
27+
const site = await makeSite( session, { label: 'status-site' } );
28+
site.settings.siteMeta[ site.dir ].trunkDate = new Date( Date.now() - 30 * DAY ).toISOString();
29+
const { page } = await session.start( site.settings );
30+
await expect( ui.siteHeading( page, 'status-site' ) ).toBeVisible( { timeout: 30_000 } );
31+
32+
// INVARIANT — the entry ends up with the name of a site that has nothing
33+
// to report. While the list still goes by the store's date, the entry's
34+
// name carries what its mark says, and `ui.sidebarEntry` does not find it.
35+
await expect( ui.sidebarEntry( page, 'status-site' ) ).toBeVisible();
36+
// CHARACTERISATION — and the store was put right too, by the main process.
37+
await expect.poll( () => {
38+
const stored = session.readSettings().siteMeta[ site.dir ].trunkDate;
39+
return Date.now() - new Date( stored ).getTime() < DAY;
40+
} ).toBe( true );
41+
} );

0 commit comments

Comments
 (0)