Repository navigation
feat(web): replace the list pages with one Triage page - #674
TheGreatAxios merged 3 commits into
Conversation
|
Review: needs fixes before ready.
|
e5a2b92 to
467109d
Compare
467109d to
b328614
Compare
54ce9d5 to
9fa72c3
Compare
|
Triage: Unsure Asking a maintainer about:
Notes:
|
corbits-triage
Bot
added
Triage: Needs Human Review
and removed
Triage: Waiting for CI
labels
![corbits-triage[bot]](https://avatars.githubusercontent.com/u/275831056?s=60&v=4){kind=small}
There was a problem hiding this comment.
Replaces the separate Pulse, Inbox, Ready and Open list pages with one Triage page that holds Needs you, Waiting and filters for Ready, Open and All, and makes Triage the post-sign-in landing. The most important defects are route collisions that make some pull requests unopenable and a removed sign-out cleanup that leaves private pull request titles in localStorage.
Not reviewed in full: 11 files, over the size limit.
docs/ARCHITECTURE.mddocs/Corbits Triage User Manual/Corbits Triage User Manual.mddocs/Corbits Triage User Manual/images/16.pngdocs/Corbits Triage User Manual/images/37.pngdocs/Corbits Triage User Manual/images/45.pngdocs/PRODUCT.mddocs/SELF_HOST.mddocs/provisioners/docker-remote.mddocs/provisioners/railway-sandboxes.mdskills/corbits-triage-install/SKILL.mdskills/corbits-triage-setup/reference.md
| useEffect(function followSession() { | ||
| if (state.kind === "signed-out") { | ||
| try { | ||
| forgetSavedVisits(localStorage); | ||
| } catch { | ||
| // Storage can be blocked (private mode); nothing was saved then. | ||
| } | ||
| } | ||
| void cache.update(state); |
There was a problem hiding this comment.
P2 · Saved Pulse visits with pull request titles are no longer cleared from localStorage on sign-out
Why it is wrong: the sign-out cleanup that removed corbits-triage:pulse:* keys from localStorage was deleted in user-cache.tsx, and nothing replaces it. Those keys hold pull request titles from private repositories, so they persist after sign-out.
Trigger: a user who visited Pulse on an older build upgrades, then signs out on a shared machine. The saved visit entries remain in localStorage.
Suggested fix: restore the removal of corbits-triage:pulse:* keys in the sign-out path, or move it into the session-ending handler in followSession so it runs whenever the session ends.
for (const key of Object.keys(localStorage)) if (key.startsWith('corbits-triage:pulse:')) localStorage.removeItem(key);
| <Route path="/repositories/:owner?/:repo?" element={<Repositories />} /> | ||
| <Route path="/settings" element={<Settings />} /> | ||
| <Route path="/settings/:tab" element={<Settings />} /> | ||
| <Route path="/" element={<InboxRedirect />} /> | ||
| <Route path="/" element={<Inbox />} /> | ||
| <Route path="/:owner/:repo/:number" element={<Inbox />} /> |
There was a problem hiding this comment.
P2 · Pull requests whose owner is repositories open the Repositories page
Why it is wrong: the static-prefix route /repositories/:owner?/:repo? at line 180 matches before the new /:owner/:repo/:number at line 184, so /repositories/x/1 never reaches the Inbox pane. Legacy /inbox/repositories/x/1 links are redirected to the same path by legacyTriageTarget, so those pull requests cannot be opened either.
Trigger: a pull request in an owner named repositories, such as repositories/app#7, is opened from Triage or from an old /inbox/... link.
Suggested fix: move the pull request route above the Repositories route, or rename the Repositories route to a prefix that cannot collide with an owner name, for example /repos/:owner?/:repo?, and update the links that point to it.
| <Route path="/triage/pr/:id" element={<PRDetail />} /> | ||
| <Route path="/prs/:owner/:repo/:number" element={<PRDetail />} /> | ||
| <Route path="/prs/:id" element={<PRDetail />} /> | ||
| <Route path="/triage/*" element={<InboxRedirect />} /> | ||
| <Route path="/queue/*" element={<InboxRedirect />} /> | ||
| <Route path="/approvals" element={<InboxRedirect />} /> | ||
| <Route path="/triage/*" element={<TriageRedirect />} /> | ||
| <Route path="/queue/*" element={<TriageRedirect />} /> | ||
| <Route path="/approvals" element={<TriageRedirect />} /> | ||
| <Route path="/audit" element={<Audit />} /> | ||
| <Route path="/runs/*" element={<InboxRedirect />} /> | ||
| <Route path="/runs/*" element={<TriageRedirect />} /> | ||
| <Route path="/repositories/:owner?/:repo?" element={<Repositories />} /> | ||
| <Route path="/settings" element={<Settings />} /> | ||
| <Route path="/settings/:tab" element={<Settings />} /> | ||
| <Route path="/" element={<InboxRedirect />} /> | ||
| <Route path="/" element={<Inbox />} /> | ||
| <Route path="/:owner/:repo/:number" element={<Inbox />} /> |
There was a problem hiding this comment.
P2 · Pull request in owner triage, repo pr renders the PR detail route with the number as an id
Why it is wrong: the static route /triage/pr/:id at line 172 matches /triage/pr/5 before /:owner/:repo/:number at line 184. Triage's own selection link for triage/pr#5 therefore renders PRDetail with id 5 instead of the Inbox selection, and Layout marks the PR page as current.
Trigger: selecting pull request number 5 in repository triage/pr on Triage, which links to /triage/pr/5.
Suggested fix: remove the /triage/pr/:id route, or move the pull request route above it and give the detail page a distinct prefix such as /pr/:id, updating the links that use it.
0e975f5 to
825f789
Compare
There was a problem hiding this comment.
Replaces the separate list pages with a single Triage page that holds Needs you and Waiting tabs and Ready, Open and All filters in the address, and makes Triage the landing page after sign-in. The most important defect is the new three-segment catch-all route, which outranks the existing redirects for old /triage, /queue and /runs links and sends them to the wrong place.
Not reviewed in full: 11 files, over the size limit.
docs/ARCHITECTURE.mddocs/Corbits Triage User Manual/Corbits Triage User Manual.mddocs/Corbits Triage User Manual/images/16.pngdocs/Corbits Triage User Manual/images/37.pngdocs/Corbits Triage User Manual/images/45.pngdocs/PRODUCT.mddocs/SELF_HOST.mddocs/provisioners/docker-remote.mddocs/provisioners/railway-sandboxes.mdskills/corbits-triage-install/SKILL.mdskills/corbits-triage-setup/reference.md
| return `/pr/${item.repo}/${item.number ?? ""}`; | ||
| } | ||
|
|
There was a problem hiding this comment.
P2 · A pull request with no number links to a non-existent route
Why it is wrong: listHref writes item.number ?? "" into the path. Inbox.tsx treats a null number as a real case, so the link becomes /pr/<repo>/ with an empty number. That path does not match /pr/:owner/:repo/:number, so it falls through to the new catch-all and ends at the 'Page not found' wildcard.
Trigger: selecting a row in Triage whose PrItem has number set to null.
Suggested fix: skip or disable the link when the number is null, or handle the null case explicitly in listHref instead of writing an empty segment into the path.
| <Route path="/" element={<InboxRedirect />} /> | ||
| <Route path="/" element={<Inbox />} /> | ||
| <Route path="/pr/:owner/:repo/:number" element={<Inbox />} /> | ||
| <Route path="/:owner/:repo/:number" element={<SelectedPullRedirect />} /> |
There was a problem hiding this comment.
P3 · New 3-segment catch-all outranks the /triage/, /queue/ and /runs/* redirects
Why it is wrong: React Router ranks /:owner/:repo/:number (score 12) above splat routes such as /triage/* (score 10). Three-segment addresses under those roots now go to /pr/triage/... instead of Triage, and Inbox finds no pull request for them. Three-segment /settings/... and /audit/... addresses also become fake pull request addresses instead of showing Page not found. This brings back the route collision the PR says it removes.
Trigger: opening an old link such as /runs/abc/def or /triage/acme/widgets.
Suggested fix: declare the explicit redirect routes for /triage, /queue, /runs and the other reserved roots before the catch-all, or restrict the catch-all so it cannot match reserved first segments.
| } | ||
|
|
||
| if (onlyMissing && ("text" in read || read.missing.length === 0)) return null; | ||
| const headingId = onlyMissing ? `grants-${repo}` : "rs-alone"; |
There was a problem hiding this comment.
P3 · Heading id is built only from the repo name, so two sources can produce the same id
Why it is wrong: GrantsStrip keys each card by source:name, which allows the same repository name under two sources. Both cards then render id="grants-<repo>". The duplicate ids make aria-labelledby point at the wrong heading.
Trigger: Triage is enabled for repositories with the same name on GitHub and on another source.
Suggested fix: include the source in the id, for example grants-${source}-${repo}, and sanitise the value so it is a valid unique id.
825f789 to
249b92b
Compare
There was a problem hiding this comment.
Replaces the separate pull request list pages with a single Triage page that holds Needs you, Waiting, and filtered Ready, Open, and All views, and makes Triage the post-sign-in landing page with redirects from the old routes. The main risks are keyboard navigation getting stuck on rows without a number, duplicate heading ids between similarly named repositories, and an N+1 policy query on the Triage landing page, plus a route collision that breaks bare owner/repo/number links for owners named like app pages.
Not reviewed in full: 12 files, over the size limit.
apps/web/src/shell.cssdocs/ARCHITECTURE.mddocs/Corbits Triage User Manual/Corbits Triage User Manual.mddocs/Corbits Triage User Manual/images/16.pngdocs/Corbits Triage User Manual/images/37.pngdocs/Corbits Triage User Manual/images/45.pngdocs/PRODUCT.mddocs/SELF_HOST.mddocs/provisioners/docker-remote.mddocs/provisioners/railway-sandboxes.mdskills/corbits-triage-install/SKILL.mdskills/corbits-triage-setup/reference.md
| const href = next === undefined ? undefined : listHref(next.item); | ||
| if (href !== undefined && next?.item.key !== selected?.key) navigate({ pathname: href, search }); |
There was a problem hiding this comment.
P3 · j/k navigation stops at a row with no number
Why it is wrong: when the adjacent row has no number, listHref returns undefined and the handler does nothing instead of skipping that row, so keyboard users cannot move past it to any later row.
Trigger: press j or k next to a pull request whose number is null in the current list.
Suggested fix: when stepping, skip over items whose listHref is undefined and continue to the next item that has a href, for example by looping in the step direction until one resolves or the list ends.
const href = next === undefined ? undefined : listHref(next.item);
| } | ||
|
|
||
| if (onlyMissing && ("text" in read || read.missing.length === 0)) return null; | ||
| const headingId = onlyMissing ? `grants-${source}-${repo}`.replace(/[^A-Za-z0-9_-]/g, "-") : "rs-alone"; |
There was a problem hiding this comment.
P3 · Grants heading ids can collide between repositories
Why it is wrong: the id replaces every non-alphanumeric character with '-', so repositories such as acme/my.repo and acme/my-repo produce the same id. The Triage page then renders duplicate ids and aria-labelledby points both sections at one heading.
Trigger: two triage-enabled repositories whose names differ only in punctuation, both with missing grants.
Suggested fix: build the id from a collision-free encoding, such as encodeURIComponent of the source and repo joined with a separator, or append a short hash of the full name.
const headingId = onlyMissing ? grants-${source}-${repo}.replace(/[^A-Za-z0-9_-]/g, "-") : "rs-alone";
| {shown.map((repo) => <PolicyGrants key={`${repo.source}:${repo.name}`} repo={repo.name} source={repo.source} disabled={disabled} onlyMissing />)} | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| function needsYouPiles(piles: InboxPile[]): OpenPile[] { | ||
| return piles.map((pile) => ({ key: pile.key, label: pile.label, items: pile.items.map((item) => ({ item, handled: null })) })); | ||
| } | ||
|
|
||
| export default function Inbox({ view }: { view: ListView }) { | ||
| export default function Inbox() { | ||
| const params = useParams(); | ||
| const navigate = useNavigate(); |
There was a problem hiding this comment.
P3 · Triage list mounts a policy and roles query per enabled repo
Why it is wrong: GrantsStrip renders a PolicyGrants component for every triage-enabled repository on each Triage view, and each one runs its own check-pack query only to decide whether to show anything. On the landing page this adds N extra fetches.
Trigger: a workspace with many triage-enabled repositories opens Triage.
Suggested fix: fetch policy and roles data once for the workspace in the parent, then pass the computed missing-grants list down to each repo, or batch the queries into one request.
{shown.map((repo) => <PolicyGrants key={${repo.source}:${repo.name}} repo={repo.name} source={repo.source} disabled={disabled} onlyMissing />)}
| <Route path="*" element={<div className="empty">Page not found. Choose a page from the navigation.</div>} /> | ||
| <Route path="/" element={<Inbox />} /> | ||
| <Route path="/pr/:owner/:repo/:number" element={<Inbox />} /> | ||
| <Route path="/:owner/:repo/:number" element={<SelectedPullRedirect />} /> |
There was a problem hiding this comment.
P3 · Old-style /owner/repo/number URLs for owners named like an app page show Not found
Why it is wrong: SelectedPullRedirect answers NotFound for any first segment in PAGE_ROOTS (settings, audit, repositories, connect, login, pr, prs). A real GitHub owner with one of those names can no longer reach its pull request through the bare three-segment address, and the list has no other way to open it.
Trigger: visiting /audit// or /settings// for a repository whose owner is named audit or settings.
Suggested fix: resolve the three-segment route as a pull request when the segment count and number match, and only fall back to NotFound when it does not, or move the lookup under the /pr/ prefix only and redirect old links there before the page-root check.
<Route path="/:owner/:repo/:number" element={<SelectedPullRedirect />} />
249b92b to
aa18582
Compare
There was a problem hiding this comment.
Replaces the separate pull request list pages with a single Triage page that becomes the post-sign-in landing, with Needs you and Waiting tabs and Ready, Open and All filters in the address, and redirects the old routes to it. The main defects are that the All filter shows closed pull requests and that the Triage page now fetches policy and role data on every load.
Not reviewed in full: 12 files, over the size limit.
apps/web/src/shell.cssdocs/ARCHITECTURE.mddocs/Corbits Triage User Manual/Corbits Triage User Manual.mddocs/Corbits Triage User Manual/images/16.pngdocs/Corbits Triage User Manual/images/37.pngdocs/Corbits Triage User Manual/images/45.pngdocs/PRODUCT.mddocs/SELF_HOST.mddocs/provisioners/docker-remote.mddocs/provisioners/railway-sandboxes.mdskills/corbits-triage-install/SKILL.mdskills/corbits-triage-setup/reference.md
| return items.map((item) => ({ item, handled: handledMark(held, item) ?? handledMark(handled, item) })); | ||
| } | ||
|
|
There was a problem hiding this comment.
P1 · All view lists closed pull requests
Why it is wrong: knownItems maps every item without dropping closed ones, and entriesFor('all') passes that result to allOpen. The description and the user manual both define All as every open pull request, but openItems filters on !item.closed while knownItems does not, so closed pull requests stay in All.
Trigger: a pull request is closed on GitHub while the list is loaded, or is closed or merged during the session. It remains in the All filter in a closed state. The new test asserts that closed #9 is included, which locks in the wrong behaviour.
Fix: filter closed items in knownItems or in allOpen, and change the test to expect #9 to be excluded.
return items.map((item) => ({ item, handled: handledMark(held, item) ?? handledMark(handled, item) }));
| mock.module("./lib/open-pulls.ts", () => ({ | ||
| leavesOpen() { return false; }, | ||
| useOpenItems() { return open; }, | ||
| useMarkedItems() { return { open, known: open }; }, | ||
| useOpenPulls() { return { data: undefined, isError: false, isEnabled: false }; }, | ||
| useOpenPullsUnanswered() { return false; }, | ||
| useQueueLoading() { return false; }, | ||
| })); |
There was a problem hiding this comment.
P2 · dom test mock of open-pulls omits exports the app imports
Why it is wrong: the mock for ./lib/open-pulls.ts does not provide useOpenItems, which Layout.tsx imports, or usePullPageItems, which PRDetail.tsx imports. A missing named export in a Bun module mock can break linking, so the sign-in gate test may not run at all, even though the description says it was verified.
Trigger: running the apps/web dom tests, since App.dom.test.tsx imports App and therefore Layout and PRDetail.
Fix: add useOpenItems and usePullPageItems to the mock factory, or use importActual and override only what the test needs, then confirm the sign-in gate test actually executes.
`mock.module("./lib/open-pulls.ts", () => ({
| const policies = useQueries({ queries: repos.map((repo) => checkPackQuery(snapshot.workspace.tenantId, repo.name, repo.source)) }); | ||
| const roles = useRoles(); | ||
| const shown = repos | ||
| .map((repo, i) => ({ repo, reading: readGrants(policies[i]!, roles, repo.source) })) | ||
| .filter((card) => missesGrants(card.reading)); |
There was a problem hiding this comment.
P3 · Triage now fetches every enabled repository's policy and the roles on each load
Why it is wrong: MissingGrants runs useQueries with one check-pack query per triage-enabled repository, plus useRoles, every time the default landing page renders. These used to load only when a user opened a single repository page. The comment says the list adds no requests, but a cold load now sends one policy request per repository plus a roles request. Users who cannot read roles also trigger the roles request.
Trigger: a workspace with many triage-enabled repositories opens Triage after sign-in.
Fix: only run the policy queries when the grants card can actually show, or defer them until after the list renders. Gate useRoles on the user's permission, and cap or batch the per-repository requests.
const policies = useQueries({ queries: repos.map((repo) => checkPackQuery(snapshot.workspace.tenantId, repo.name, repo.source)) });
aa18582 to
3684052
Compare
There was a problem hiding this comment.
Replaces the separate pull request list pages with a single Triage page that becomes the post-sign-in landing, with Needs you and Waiting tabs and Ready, Open and All as address filters. The most important defect is in the grants check, where a policy lacking a decide list now shows an error instead of its actions and grants card.
Not reviewed in full: 12 files, over the size limit.
apps/web/src/shell.cssdocs/ARCHITECTURE.mddocs/Corbits Triage User Manual/Corbits Triage User Manual.mddocs/Corbits Triage User Manual/images/16.pngdocs/Corbits Triage User Manual/images/37.pngdocs/Corbits Triage User Manual/images/45.pngdocs/PRODUCT.mddocs/SELF_HOST.mddocs/provisioners/docker-remote.mddocs/provisioners/railway-sandboxes.mdskills/corbits-triage-install/SKILL.mdskills/corbits-triage-setup/reference.md
| if (policy.isError) return { text: "Could not read what Triage may do here, so it asks before every action." }; | ||
| const found = policy.data; | ||
| if (found?.kind !== "file" && found?.kind !== "pack") return { text: "This repository has no policy to allow, so Triage asks before every action." }; | ||
| if (!found.automatic || !found.decide || roles.unavailable || roles.denied) return { text: "Could not read what Triage may do here, so it asks before every action." }; |
There was a problem hiding this comment.
P2 · A policy with no decide list now reads as unreadable
Why it is wrong: the old code used found.decide ?? [], so a policy without a decide list still showed its actions and the missing grants. The new !found.decide check short-circuits to the "Could not read what Triage may do here" text instead. On the repository page this removes the policy's action list and Allow button, and on Triage missesGrants comes out false, so the grants card never appears.
Trigger: a check-pack artifact of kind file or pack that has automatic set but no decide field.
Suggested fix: restore the fallback so a missing decide is treated as an empty list, and only return the error for the automatic, roles unavailable, or roles denied cases:
if (!found.automatic || roles.unavailable || roles.denied) return { text: "Could not read what Triage may do here, so it asks before every action." };
const decide = found.decide ?? [];
Add a test for a policy with automatic set and no decide field.
Triage is now the only list of pull requests, and it is the page after sign-in. Needs you and Waiting stay as tabs, and Waiting still shows how many pull requests wait on others. Ready, Open and All are filters in the address. Open is the old All open list. All is every open pull request across the repositories Triage runs on.
A workspace with no decision model still lands on Triage. A card at the top of the list says Connect a decision model and links to Settings → Triage, where one is added. A workspace with no repositories lands on Repositories.
Pulse, Inbox, Ready and Open, including a pull request open on them, redirect to the same place on Triage with the matching filter. A pull request selected on Triage opens at
/pr/<owner>/<repo>/<number>, so no owner or repository name can collide with another page. The pull request page is unchanged. The navigation lists Triage. When a policy still wants actions that are not allowed, that grants card shows at the top of Triage and stays on the repository page. Signing out still removes the visits the old Pulse page saved in the browser, since they hold pull request titles.Screenshots 37 (Waiting) and 45 (Ready) in the user manual showed the old navigation. They could not be retaken without a browser, so they and their references are removed, along with the unused Pulse screenshot 16.
Verified with bun run check, bun run lint, the apps/web tests, and the dom tests. One dom test renders the portal through its sign-in gate for a workspace without a decision model and checks that Triage lists the pull request that needs you, shows the Connect card and the Waiting count.
Closes CL-11563