Skip to content

ui: give Repositories the full row and move Add repository into a header button with a modal - #744

Merged
HugoXK merged 2 commits into
mainfrom
dev3/590-gate-resolve-evidence-v2
Nov 15, 2026
Merged

HugoXK merged 2 commits into
mainfrom
dev3/590-gate-resolve-evidence-v2

Conversation

@HugoXK

@HugoXK HugoXK commented Nov 15, 2026 •

Copy link
Copy Markdown
Owner

Written-by: HugoXK

Fixes #748

risk-tier: T2; basis: src/views/pages.ts

What changed

  • src/views/pages.ts
    • The Repositories card is now a single full-width section.card#repositories; it no longer shares a .cols row with the Add card.
    • The Add repository control is a type="button" in the card header with aria-haspopup="dialog" and aria-controls="add-repo".
    • The two add options and the existing manual form moved into a native <dialog id="add-repo">. The dialog is outside the secondary disclosure, so showModal() is never hidden by a closed <details>.
    • #add-repo stays the deep link used by the empty state, next action, setup checklist and the POST /repos error. The repository pager now targets #repositories, and a repo-page deep link opens the disclosure.
    • A <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 on a[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.md and changelog.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-repo deep link or the no-JS path.

Accessibility

  • Real <button type="button"> trigger with aria-haspopup/aria-controls.
  • Native <dialog> focus trap, Escape and backdrop close; labelled heading and close control.
  • Token input keeps type="password", autocomplete="off" and the existing CSRF field.
  • The initial focus is the dialog header/close control, not the token field.
  • No-JS fallback renders the same options and form inline.

Ran:

  • npm ci
  • npx 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 at f2299ce
  • npm run check at f2299ce (forward-merged live main 4b8065fb) — lint 0 errors (11 pre-existing warnings, identical count at 4b8065fb), typecheck clean, 1886/1886 tests, build clean
  • node scripts/version.mjs check — 0.3.6 ok
  • node scripts/changelog.mjs check — 28 fragments ok
  • node scripts/risk-gate.mjs --base origin/main — risk-tier: T2; basis: src/views/pages.ts

Not run:

  • No production deploy.
  • No manual browser click-through; the server-rendered HTML and dialog markup are covered by the tests, and the visual matrix will run in CI.

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-repo still 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 name dev3/590-gate-resolve-evidence-v2 is legacy. The old PASS at e73e092 belongs to the previous tip and does not apply to f2299ce, and neither does the one at fe254b6.

Merge note

f2299ce is a forward-merge of live main 4b8065fb into the previous tip fe254b6; parents are fe254b6 and 4b8065fb, tree a30e8a2a. 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's src/lib/nextRunFor.ts:59 (enabled before the row), the dashboard enabled === 1 gate and the one-next-run paragraph at docs/architecture.md:173-194 all survive the merge.

Out of scope

@HugoXK-bot HugoXK-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:line must 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). definesLine compares 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 main via the compare API (:150-171): ahead/identical accepted, anything else is a failure with the compare status in a note. A body that cites no lined evidence returns ok and 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), so no-pass/stale-pass/changes keep 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-gate subprocess 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:

  1. 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 713 and --issue 723 both print bad-evidence today (src/jobs/service.ts:2449, src/views/pages.ts:1259) because their bodies cite a line without main = <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 live main with a note; the code as written chooses strictness, and the docs state that choice.
  2. A pin that is syntactically hex but names no commit makes the compare call return 404, which getJson turns into the "gate could not run" exit 6 (scripts/issue-gate.mjs:97-110) instead of bad-evidence; a typo'd pin will therefore look like an outage. Passing a status-bearing fetch into resolveEvidence and 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.

@HugoXK-bot HugoXK-bot added the review:approved Issue reviewed and approved by the Fledge Platform Reviewer label Nov 15, 2026
@HugoXK

HugoXK commented Nov 15, 2026

Copy link
Copy Markdown
Owner Author

Written-by: HugoXK

Folded into #678. #678 is reopened at 8c94734 (tree c6b0238, byte-identical to this branch's tested tip e73e092) and is the replacement. This is a duplicate consolidation, not a reset.

Reason: duplicate of #678 after the original was reopened.
Replacement: #678
Tip: 8c94734
Approved by: owner instruction to reuse #678.

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.

@HugoXK HugoXK closed this Nov 15, 2026
@HugoXK HugoXK reopened this Nov 15, 2026
@HugoXK

HugoXK commented Nov 15, 2026

Copy link
Copy Markdown
Owner Author

Written-by: HugoXK

Reopened. The duplicate-close was wrong: #744 remains the fix for #590 at e73e092, and #678 is reopened only as the original record pending an owner decision. Do not close either PR. Disregard the previous close comment.

@HugoXK
HugoXK force-pushed the dev3/590-gate-resolve-evidence-v2 branch from e73e092 to fe254b6 Compare November 15, 2026 03:38
@HugoXK HugoXK changed the title fix(gate): resolve the cited evidence before a PASS can land (#590) ui: give Repositories the full row and move Add repository into a header button with a modal Nov 15, 2026
@HugoXK HugoXK added enhancement New feature or request accessibility Barrier affecting people with disabilities area:ui Views, CSS, accessibility, i18n and removed review:approved Issue reviewed and approved by the Fledge Platform Reviewer labels Nov 15, 2026
@HugoXK

HugoXK commented Nov 15, 2026

Copy link
Copy Markdown
Owner Author

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 8c94734; the branch name dev3/590-gate-resolve-evidence-v2 is legacy. The old PASS at e73e092 belongs to the previous tip and does not apply. Please review the new tip fe254b6.

@HugoXK
HugoXK requested a review from HugoXK-bot November 15, 2026 03:39

@HugoXK-bot HugoXK-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"> with aria-haspopup="dialog"/aria-controls="add-repo" in the card header. The old <div class="card" id="add-repo"> is gone, so no second #add-repo id 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: #repositories now targets the card, and the jobs empty-state link is intercepted). The close control is the first focusable element, so showModal() does not land in the token field; the token input keeps type="password", autocomplete="off" and ${csrf(viewer)}.
  • src/views/pages.ts:1006 — reposPage > 1 joins 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 on hashchange, delegated a[href="#add-repo"] handling with history.replaceState, and hash cleanup on close. The script is defer-loaded (src/views/layout.ts:238), so the dialogs exist when it runs. The existing #confirm-dialog keeps its own returnValue listener; the generic backdrop close resolves it as cancel, which is the safe direction.
  • No-JS: src/views/pages.ts:1052-1057 renders the dialog inline and hides the JS-only trigger and close control; an author rule beats the UA dialog:not([open]) rule, so the form is reachable with scripting off.
  • Docs and fragment match the code: docs/architecture.md:60-67 states the full row, the deep link, the outside-disclosure placement and the no-JS fallback; changelog.d/748-repositories-add-dialog.changed.md is one paragraph.

Ran:

  • npx tsc --noEmit at this tip — clean
  • npx tsx --test test/pages.test.ts test/nextAction.test.ts test/jobFilters.test.ts test/flow.test.ts test/setupChecklist.test.ts at this tip — 92/92
  • npx tsx --test test/*.test.ts at this tip — 1833/1833
  • test-merge of this tip onto live main 0d6f89357db76255caeeaf98b9670fbfc87b2c37 in 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 --noEmit clean, npx tsx --test test/*.test.ts — 1883/1883
  • node scripts/risk-gate.mjs --base ada57f6206deab2e494fc43c840d154847d4d710 — risk-tier: T2; basis: src/views/pages.ts
  • node scripts/changelog.mjs check — 21 fragments ok
  • check-runs at fe254b60 — 27 green/skipped, review-gate red; that is this verdict (the earlier COMMENTED review 5908802746 is at e73e092f and does not bind this tip)
    Not run: a browser pass over /dashboard on desktop and mobile, the Docker image, a real SMTP send.

Findings, none blocking:

  1. Merge mechanics, not content. The branch is dirty against live main: docs/architecture.md conflicts 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 authorises fe254b60 only; do not merge a later tip under it.
  2. public/app.js:239-243 closes a dialog whenever event.target === dialog. Because .add-repo-dialog carries 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.
  3. src/views/pages.ts:1047 repeats error inside the dialog while src/views/layout.ts:223 already renders it at the top of the page. After a POST /repos failure 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.
  4. src/views/pages.ts:1052-1057 puts 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.
  5. On a browser without <dialog>, the delegated handler at public/app.js:259-267 prevents the anchor and openDialog returns false, so the #add-repo deep 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.

@HugoXK-bot HugoXK-bot added the review:approved Issue reviewed and approved by the Fledge Platform Reviewer label Nov 15, 2026
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 HugoXK-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 diff from live main 4b8065fb to this tip is the eight feature files only; docs/architecture.md differs from main by 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, so showModal() does not land on the token field; the token input keeps type="password", autocomplete="off" and its CSRF field.
  • src/views/pages.ts:1006 — reposPage > 1 joins the deep-linked set, so the repository pager's new #repositories target is visible when a page loaded on page two; :1052-1057 renders 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 and hashchange, delegated a[href="#add-repo"] handling with history.replaceState, and hash cleanup on close; the script is deferred (src/views/layout.ts:238), so the dialogs exist when it runs. The existing #confirm-dialog keeps its own returnValue listener, and a backdrop close resolves it as cancel.
  • docs/architecture.md:89-96 and changelog.d/748-repositories-add-dialog.changed.md describe the shipped behaviour: full row, deep link, outside the disclosure, no-JS fallback.

Ran:

  • npx tsc --noEmit on the merged tree — clean
  • npx tsx --test test/pages.test.ts test/nextAction.test.ts test/jobFilters.test.ts test/flow.test.ts test/setupChecklist.test.ts — 100/100
  • npx tsx --test test/*.test.ts on the merged tree — 1886/1886
  • node scripts/risk-gate.mjs with the live-main content as the base commit — risk-tier: T2; basis: src/views/pages.ts
  • check-runs at f2299ce — 27 green/skipped, review-gate red; that is this verdict
    Not run: a browser pass over /dashboard on desktop and mobile, the Docker image, a real SMTP send.

Findings, none blocking (unchanged from my round on fe254b60):

  1. public/app.js:239-243 closes a dialog whenever event.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.
  2. src/views/pages.ts:1047 repeats error inside the dialog while src/views/layout.ts:223 already renders it at the page top; after a POST /repos failure 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.
  3. src/views/pages.ts:1052-1057 puts a <style> inside <noscript> in the body; browsers apply it, the HTML spec does not place it there.
  4. On a browser without <dialog>, the delegated handler at public/app.js:259-267 prevents the anchor and openDialog returns false, so the #add-repo deep 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.

@HugoXK
HugoXK merged commit bf5a77b into main Nov 15, 2026
35 of 36 checks passed
@HugoXK
HugoXK deleted the dev3/590-gate-resolve-evidence-v2 branch November 15, 2026 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility Barrier affecting people with disabilities area:ui Views, CSS, accessibility, i18n enhancement New feature or request review:approved Issue reviewed and approved by the Fledge Platform Reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ui: give Repositories the full row and move Add repository into a header button with a modal

2 participants