Repository navigation
Move a site's status out of SiteRow into a hook - #593
Merged
Merged
Conversation
What the main process says about a site: 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. Thirteen pieces of state, the one function that reads them all, and the effect that calls it when the site's view mounts. It was the largest thing left in SiteRow after #554's eight domains, and it is what nearly every other hook and card reads. The redesign's page header and sites list (#556) need it from outside the site's view. A move: the lines are SiteRow's own. Two dependency lists gain a value that was already stable, the ref the status reports through and a state setter that now comes from the hook. The effect that reads the status on mount now runs before the one that clears the copy button's timer, which shares nothing with it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ollows the status The pre-PR review. The hook's header said a finished build sets "built", that everything comes from one read, and that the status is read when a site's view is opened. A finished build goes through `loadStatus`; it is the dev server's start that sets "built". The five setters handed out mean a value can be newer than the rest until the next read. And every listed site's view mounts as the app opens, so that is when each status is first read. The report a site's status makes to the sites list is the one wiring this move reshaped, into an argument, and no journey read it: the update journeys read the store, which the main process writes itself. `site-status.spec.js` seeds the store with an old trunk for a checkout whose trunk is recent, and reads the entry's name. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Deleting a ticket's work reads the status too, when it leaves the checkout on trunk; a site's view mounts when the site joins the list, which is at launch only for the sites already there; and the window does read the store again, after a site is created or deleted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: WordPress/contributor-toolkit/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zaerl
added a commit
that referenced
this pull request
Oct 2, 2026
Brings in the site-status hook (#593): what the main process says about a site leaves SiteRow for useSiteStatus, with a journey for the sites list following it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.jsxis 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.loadStatus.SiteRow's own.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/wpdsbecause 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:e2eruns 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, arelegacy-site,merge-in-progress,pr-checkout,patch-apply,trunk-update-watch,store-persistence,build-watchand the newsite-status; the audit in the review outcome says which.site-status.spec.jsseeds 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:
loadStatuskeeps its identity for as long as the site's path does, so the effect that calls it on mount runs once.Risks and limitations
loadStatuslists the ref it reports through, and the callback that skips the checklist lists a setter that now comes from the hook.loadStatusswallows a failed read, as it did inSiteRow: 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.Related
Follow-up to #554. Prepares #556, part of #542.
Review outcome (required — see AGENTS.md)
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 insideloadStatus; 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 throughloadStatus; 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 silentcatch, 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 bysite-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:
npx eslint . --max-warnings=0npm testnpm run test:electronnpm run test:e2e(journeys)62c5ab5; the head differs from it by comments, andsite-status.spec.jswas run again on itThat it is a move. Comparing the lines removed from
index.jsxwith 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 inSiteRowabove the hook's call.The wiring was audited by breaking it, one thing at a time, and running the journeys: 17 breaks, 11 caught.
build-watchlegacy-sitemerge-in-progresspr-checkouttrunk-update-watchpatch-applystore-persistencebuild-watchsetup-steps.cjshas unit tests for what it does with the flagsite-statussite-statussite-statusloadStatusreturns nothing to its caller🤖 Generated with Claude Code