Repository navigation
ui: give Repositories the full row and move Add repository into a header button with a modal - #744
Conversation
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS at e73e092
Written-by: HugoXK-bot
Author: Fledge Platform Reviewer
Posted-by: HugoXK-bot
T3 gate change (#590): issue-gate now resolves the evidence a body cites before it can answer ok, so a PASS whose body names a file:line or a definition that is not there is refused.
Reviewed at the tip: scripts/lib/issue-evidence.mjs (new, 247 lines), scripts/issue-gate.mjs (+24), test/issueEvidence.test.ts (+253), docs/autonomous-dev.md (+23/−1). 4 files, +547/−1.
What I checked:
- the rule is narrow and stated: a
path:linemust exist in the pinned tree and be inside the file's length; a symbol the body claims is defined at a line must be declared on that line (DEF_AT_LINE/LINE_IS_DEF,scripts/lib/issue-evidence.mjs:33-39).definesLinecompares the line's tokens with the declaration keyword before the name and does not interpolate the name into a RegExp (:64-83), which is the failure mode the PR body describes (price$). - resolution happens at the pin, and the pin must be an ancestor of live
mainvia the compare API (:150-171):ahead/identicalaccepted, anything else is a failure with the compare status in a note. A body that cites no lined evidence returnsokand reads no file, which is the #698/#688 correction; the tests pin both halves separately (test/issueEvidence.test.ts:170-198,:236-253). - the integration point is after the hash check and after
hold(scripts/issue-gate.mjs:181-196), sono-pass/stale-pass/changeskeep their existing precedence and exit codes; only a body whose hash already matches reaches resolution, and one bad ref prints the machine line and exits 8. The exit-code list in the header is updated. - the tests use a real
issue-gatesubprocess against a fake API with a fixed tree, so they exercise the pin, the tree read and the contents read rather than a mocked resolver.
Ran (this turn, at this tip):
npx tsx --test test/issueEvidence.test.ts— pass 13/13
Red half: the test file is new at this tip; at the base the behaviour it pins does not exist (the gate answered ok on a body naming a line that is not there). I did not re-run the base case as a suite because the file is absent there.
Not run: a full npm run check; ci-ok covers the build at this tip.
Two operational notes, neither a defect against this tip:
- This makes the pin mandatory for lined evidence, so bodies approved before the rule now read
bad-evidence. I measured it on live cards:node scripts/issue-gate.mjs --issue 713and--issue 723both printbad-evidencetoday (src/jobs/service.ts:2449,src/views/pages.ts:1259) because their bodies cite a line withoutmain=<sha>. Adding the pin edits the body, which changes the hash and voids the standing PASS, so those two need a pin and a fresh verdict before they can be dispatched. Merge order matters: merge this, then re-pin and re-review #713/#723, and any other approved card with a lined ref. If you would rather not pay that now, the soft alternative is to resolve unpinned bodies at livemainwith a note; the code as written chooses strictness, and the docs state that choice. - A pin that is syntactically hex but names no commit makes the compare call return 404, which
getJsonturns into the "gate could not run" exit 6 (scripts/issue-gate.mjs:97-110) instead ofbad-evidence; a typo'd pin will therefore look like an outage. Passing a status-bearing fetch intoresolveEvidenceand reporting 404 as a bad pin would keep the two apart. Non-blocking.
Conclusion: PASS — the check is narrow, pinned, precedence-preserving and tested; the two notes are sequencing and diagnosis, not correctness of the check.
|
Written-by: HugoXK Folded into #678. #678 is reopened at Reason: duplicate of #678 after the original was reopened. Going forward: do not close a PR to repackage it. Reopen the original, merge the replacement into its branch, and close only the duplicate with this record. |
e73e092 to
fe254b6
Compare
|
Written-by: HugoXK Repurposed at the owner's request: this PR now implements #748 (Repositories full row + Add repository modal), not #590. The #590 gate fix remains in #678 at |
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS at fe254b6
Written-by: HugoXK-bot
Author: Fledge Platform Reviewer
Posted-by: HugoXK-bot
Tip SHA: fe254b6
T2 (#748): the Repositories card takes the full row and Add repository moves into a header button that opens a native dialog; the #add-repo deep link and the no-JavaScript form are kept.
What I checked in the tree at this tip:
src/views/pages.ts:1115-1120— one<section class="card" id="repositories">owns the row; the trigger is a real<button type="button" data-open-dialog="add-repo">witharia-haspopup="dialog"/aria-controls="add-repo"in the card header. The old<div class="card" id="add-repo">is gone, so no second#add-repoid exists.src/views/pages.ts:1043-1057— the<dialog id="add-repo" aria-labelledby="add-repo-title">sits after</details>, outside the secondary disclosure (the disclosure's own links still work:#repositoriesnow targets the card, and the jobs empty-state link is intercepted). The close control is the first focusable element, soshowModal()does not land in the token field; the token input keepstype="password",autocomplete="off"and${csrf(viewer)}.src/views/pages.ts:1006—reposPage > 1joins the deep-linked set, so a page loaded on repository page two opens the disclosure that now hides the table; the repository pager targets#repositories(:1121) and the tests follow it.public/app.js:228-269—[data-open-dialog]/[data-close-dialog], hash-open on load and onhashchange, delegateda[href="#add-repo"]handling withhistory.replaceState, and hash cleanup onclose. The script isdefer-loaded (src/views/layout.ts:238), so the dialogs exist when it runs. The existing#confirm-dialogkeeps its ownreturnValuelistener; the generic backdrop close resolves it as cancel, which is the safe direction.- No-JS:
src/views/pages.ts:1052-1057renders the dialog inline and hides the JS-only trigger and close control; an author rule beats the UAdialog:not([open])rule, so the form is reachable with scripting off. - Docs and fragment match the code:
docs/architecture.md:60-67states the full row, the deep link, the outside-disclosure placement and the no-JS fallback;changelog.d/748-repositories-add-dialog.changed.mdis one paragraph.
Ran:
npx tsc --noEmitat this tip — cleannpx tsx --test test/pages.test.ts test/nextAction.test.ts test/jobFilters.test.ts test/flow.test.ts test/setupChecklist.test.tsat this tip — 92/92npx tsx --test test/*.test.tsat this tip — 1833/1833- test-merge of this tip onto live
main0d6f89357db76255caeeaf98b9670fbfc87b2c37in a scratch worktree, nothing pushed: one conflict only,docs/architecture.md:86-105(both sides added a paragraph at the same anchor). Resolved by keeping both paragraphs —npx tsc --noEmitclean,npx tsx --test test/*.test.ts— 1883/1883 node scripts/risk-gate.mjs --base ada57f6206deab2e494fc43c840d154847d4d710—risk-tier: T2; basis: src/views/pages.tsnode scripts/changelog.mjs check— 21 fragments ok- check-runs at
fe254b60— 27 green/skipped,review-gatered; that is this verdict (the earlierCOMMENTEDreview5908802746is ate73e092fand does not bind this tip)
Not run: a browser pass over/dashboardon desktop and mobile, the Docker image, a real SMTP send.
Findings, none blocking:
- Merge mechanics, not content. The branch is
dirtyagainst livemain:docs/architecture.mdconflicts with#699's merged backup paragraph at the same anchor. The resolution I verified is to keep both paragraphs, and the merged tree is 1883/1883. Whichever way it is resolved, the forward-merge produces a new tip and voids this PASS — re-request at that sha. This verdict authorisesfe254b60only; do not merge a later tip under it. public/app.js:239-243closes a dialog wheneverevent.target === dialog. Because.add-repo-dialogcarries 22px of padding (public/app.css:1091), a click in that padding region targets the dialog and closes it, discarding a typed or pasted token. A bounding-rect check (close only when the click is outside the dialog's rect) would keep backdrop dismissal without the accident. Minor; worth a follow-up if the token form sees real use on mobile.src/views/pages.ts:1047repeatserrorinside the dialog whilesrc/views/layout.ts:223already renders it at the top of the page. After aPOST /reposfailure the redirect is/dashboard?error=…(src/app.ts:1818), with no hash, so the dialog stays closed and only the top banner is visible — correct, but the duplicate copy in the dialog is redundant.src/views/pages.ts:1052-1057puts a<style>inside<noscript>in the body, which browsers apply but the HTML spec does not place there. The fallback works; a single<noscript>-rendered block would be conforming if this is ever cleaned up.- On a browser without
<dialog>, the delegated handler atpublic/app.js:259-267prevents the anchor andopenDialogreturnsfalse, so the#add-repodeep link does nothing. The PR acknowledges the native-dialog baseline (the confirm dialog already assumes it), so this is not new, but it is the one path where the old "scroll to the form" behaviour is lost.
Conclusion: PASS at fe254b607c4cf25f6512771a4cc6e53397e5a4d3. The full row, the modal trigger, the outside-disclosure placement, the #add-repo deep links and the no-JS fallback all match the code, the affected suites and the full suite pass at this tip, and the merged tree with live main is green once the docs paragraph is kept beside #699's. The tip is not mergeable as it stands; forward-merge and re-request, and this verdict does not carry over.
Forward-merge of 4b8065f into the Repositories header change. One conflict, docs/architecture.md: both sides added an independent paragraph at the same anchor; both kept. No rebase, no force. Written-by: HugoXK
HugoXK-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS at f2299ce
Written-by: HugoXK-bot
Author: Fledge Platform Reviewer
Posted-by: HugoXK-bot
Tip SHA: f2299ce
T2 (#748): the Repositories card takes the full row and Add repository moves into a header button that opens a native dialog, with the #add-repo deep link and the no-JavaScript form kept.
This is the forward-merged tip: parents fe254b60 + 4b8065fb, tree a30e8a2a. My earlier PASS 5909141901 at fe254b60 is void because the tip moved; this one names the merge.
What I checked in the merged tree:
- The merge is mechanical.
git difffrom livemain4b8065fbto this tip is the eight feature files only;docs/architecture.mddiffers frommainby exactly the eight-line dialog paragraph inserted before#699's backup paragraph, and both stay.#755's next-run paragraph (docs/architecture.md:186-194) is untouched. No conflict markers anywhere, no hand edit beyond the documented resolution. src/views/pages.ts:1115-1120— one<section class="card" id="repositories">owns the row, with the<button type="button" data-open-dialog="add-repo" aria-haspopup="dialog" aria-controls="add-repo">in the card header; the old<div class="card" id="add-repo">is gone, so the id is unique.src/views/pages.ts:1043-1057— the<dialog id="add-repo" aria-labelledby="add-repo-title">sits after</details>and outside the secondary disclosure; the close control is first in the dialog, soshowModal()does not land on the token field; the token input keepstype="password",autocomplete="off"and its CSRF field.src/views/pages.ts:1006—reposPage > 1joins the deep-linked set, so the repository pager's new#repositoriestarget is visible when a page loaded on page two;:1052-1057renders the dialog inline under<noscript>and hides the JS-only trigger and close control.public/app.js:228-269— generic[data-open-dialog]/[data-close-dialog], hash-open on load andhashchange, delegateda[href="#add-repo"]handling withhistory.replaceState, and hash cleanup onclose; the script is deferred (src/views/layout.ts:238), so the dialogs exist when it runs. The existing#confirm-dialogkeeps its ownreturnValuelistener, and a backdrop close resolves it as cancel.docs/architecture.md:89-96andchangelog.d/748-repositories-add-dialog.changed.mddescribe the shipped behaviour: full row, deep link, outside the disclosure, no-JS fallback.
Ran:
npx tsc --noEmiton the merged tree — cleannpx tsx --test test/pages.test.ts test/nextAction.test.ts test/jobFilters.test.ts test/flow.test.ts test/setupChecklist.test.ts— 100/100npx tsx --test test/*.test.tson the merged tree — 1886/1886node scripts/risk-gate.mjswith the live-main content as the base commit —risk-tier: T2; basis: src/views/pages.ts- check-runs at
f2299ce— 27 green/skipped,review-gatered; that is this verdict
Not run: a browser pass over/dashboardon desktop and mobile, the Docker image, a real SMTP send.
Findings, none blocking (unchanged from my round on fe254b60):
public/app.js:239-243closes a dialog wheneverevent.target === dialog; with 22px of dialog padding (public/app.css:1091), a click in that padding closes the modal and discards a typed or pasted token. A bounding-rect check would keep backdrop dismissal without the accident.src/views/pages.ts:1047repeatserrorinside the dialog whilesrc/views/layout.ts:223already renders it at the page top; after aPOST /reposfailure the redirect is/dashboard?error=…with no hash (src/app.ts:1818), so only the top banner shows until the user opens the dialog. The in-dialog copy is redundant.src/views/pages.ts:1052-1057puts a<style>inside<noscript>in the body; browsers apply it, the HTML spec does not place it there.- On a browser without
<dialog>, the delegated handler atpublic/app.js:259-267prevents the anchor andopenDialogreturnsfalse, so the#add-repodeep link does nothing. The PR states the native-dialog baseline; this is the one path that used to scroll to the form.
Conclusion: PASS at f2299ce2543efa06348427134be3806dad7a93a8. The merge carries live main plus exactly this feature's diff, the docs conflict was resolved by keeping both paragraphs, the dialog/deep-link/no-JS behaviour matches the code, and the merged tree is green at 1886/1886 with only review-gate red. A later push voids this PASS.
Written-by: HugoXK
Fixes #748
risk-tier: T2; basis: src/views/pages.ts
What changed
src/views/pages.tssection.card#repositories; it no longer shares a.colsrow with the Add card.type="button"in the card header witharia-haspopup="dialog"andaria-controls="add-repo".<dialog id="add-repo">. The dialog is outside the secondary disclosure, soshowModal()is never hidden by a closed<details>.#add-repostays the deep link used by the empty state, next action, setup checklist and thePOST /reposerror. The repository pager now targets#repositories, and a repo-page deep link opens the disclosure.<noscript>rule renders the dialog inline (and hides the JS-only trigger/close control), so adding a repository still works without JavaScript.public/app.js: generic[data-open-dialog]/[data-close-dialog]handling, hash-open for#add-repo, delegated clicks ona[href="#add-repo"], backdrop/Escape close, and hash cleanup after close. The existing confirm dialog keeps its own return-value handling.public/app.css:.repos-head,.add-repo-dialog,::backdrop, sticky dialog header, mobile width and scroll lock.test/pages.test.ts,test/jobFilters.test.ts,test/nextAction.test.ts: the full-row card, trigger semantics, dialog markup, no-JS fallback, pager hash and disclosure placement.docs/architecture.mdandchangelog.d/748-repositories-add-dialog.changed.md.Why
The repository table has five columns and was rendered at ~57% of the row while a one-time Add form took the other 43%. On a 1280px console the table was cramped; on mobile the Add card pushed the actual list down. The Add flow is now one header action and the modal is only present when opened, without losing the
#add-repodeep link or the no-JS path.Accessibility
<button type="button">trigger witharia-haspopup/aria-controls.<dialog>focus trap, Escape and backdrop close; labelled heading and close control.type="password",autocomplete="off"and the existing CSRF field.Ran:
npm cinpx tsx --test test/pages.test.ts test/nextAction.test.ts test/jobFilters.test.ts test/flow.test.ts test/setupChecklist.test.ts— 100/100 pass atf2299cenpm run checkatf2299ce(forward-merged livemain4b8065fb) — lint 0 errors (11 pre-existing warnings, identical count at4b8065fb), typecheck clean, 1886/1886 tests, build cleannode scripts/version.mjs check— 0.3.6 oknode scripts/changelog.mjs check— 28 fragments oknode scripts/risk-gate.mjs --base origin/main—risk-tier: T2; basis: src/views/pages.tsNot run:
Risk and rollback
Risk: the modal depends on
HTMLDialogElement/showModal; the<noscript>path covers disabled JavaScript, but a browser without<dialog>support would not open the modal from the button. That is the same native-dialog baseline the existing confirm dialog already assumes. The hash handler is limited to#add-repo; it does not intercept other anchors. Rollback: revert the commit; the add form returns to its previous card and#add-repostill points at it.Repurpose note
This PR was repurposed from #590 to #748 at the owner's request. The #590 gate fix remains in #678 at
8c94734; the branch namedev3/590-gate-resolve-evidence-v2is legacy. The old PASS ate73e092belongs to the previous tip and does not apply tof2299ce, and neither does the one atfe254b6.Merge note
f2299ceis a forward-merge of livemain4b8065fbinto the previous tipfe254b6; parents arefe254b6and4b8065fb, treea30e8a2a. One conflict,docs/architecture.md: both sides added an independent paragraph at the same anchor, so both are kept. No rebase and no force-push;#723'ssrc/lib/nextRunFor.ts:59(enabled before the row), the dashboardenabled === 1gate and the one-next-run paragraph atdocs/architecture.md:173-194all survive the merge.Out of scope
POST /repos.