trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRunsg rEventsFlows

small: every list that asks for a decision answers j/k, enter, esc and `?` #10717

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/10710-small-every-list-that-asks-for-a-decisio → main 3 commits fork 3 threads · 3 unresolved

  • Merged. scout/10710-small-every-list-that-asks-for-a-decisio was merged into main as 6fcc5a6.

Review

Sign in to review this change.
6 files changed +204 −10
docs/features/decision-keys.md added +15
@@ -0,0 +1,15 @@
1 # Lists that ask for a decision answer the same keys (#10710)
2
3 The asks inbox (`/asks`), a repository's issues and its pulls are `data-nav` lists on the web, and list screens in the terminal.
4
5 | Key | Web | Terminal |
6 | --- | --- | --- |
7 | `j` / `k` | focus the next / previous row | move the cursor (`ListState`, `src/tui/screens/common.ts`) |
8 | `Enter` or `o` | open the focused row | open the row |
9 | `Esc` | on the opened page, return to the list and focus the row you opened | `esc` / `q` is back (`src/tui/app/app.ts`) |
10 | `?` | opens the one help sheet on the first press | the screen's key legend |
11
12 - The terminal already has these: the dashboard's asks pane (`dashboard.ts`) and the issues and pulls list (`issues.ts`, one screen for both kinds) are `ListState` lists, so this change adds no new screen.
13 - The place is kept in `sessionStorage` (`nav-from`, this tab only). A malformed or off-site value is ignored.
14 - Esc closes the thing on top first: hints, an open menu, a dialog or the help sheet, and a field's own Esc. Only then does it leave.
15 - Tests: `test/keys-decide.test.ts` drives the real script in a DOM.
docs/features/hotkey-hints.md +1 −1
@@ -5,7 +5,7 @@ Every action with a key shows it without taking room. Hints are never a box in t
55 **See them**
66 - **Hover or keyboard focus** on a control: its key appears as a small tooltip. This is CSS alone and works with JavaScript off.
77 - **Hold Option (Alt)**: every hinted control shows its key at once; release to hide.
8 - **`?`**: the same overlay, kept until you press Escape or `?` again (the second `?` clears them and opens the full help).
8 - **`?`**: opens the one help sheet on the first press (#10710). Escape clears hints first and does nothing else.
99 - Touch screens never draw hints. Menu and palette rows keep their key in a column of their own.
1010
1111 **For builders**
src/web/client.ts +46 −3
@@ -1483,6 +1483,40 @@ const shellCore = /* js */ `
14831483 next.focus();
14841484 next.scrollIntoView({ block: 'nearest' });
14851485 }
1486 // Esc returns to the list a row was opened from, and the list focuses that
1487 // row again: the place is kept in sessionStorage for this tab only.
1488 function place(k, v) { try { if (v == null) sessionStorage.removeItem(k); else sessionStorage.setItem(k, v); } catch (_) {} }
1489 function placed(k) { try { return sessionStorage.getItem(k); } catch (_) { return null; } }
1490 doc.addEventListener('click', function (e) {
1491 var a = e.target.closest ? e.target.closest('[data-nav]') : null;
1492 if (a && !a.matches('a')) a = a.querySelector('a');
1493 if (a && a.pathname) place('nav-from', JSON.stringify({ list: location.pathname + location.search, to: a.pathname, href: a.getAttribute('href') }));
1494 });
1495 // A stored place is only ever read back through this: a malformed or
1496 // foreign value is no place, and never throws inside a key handler.
1497 function fromPlace() {
1498 try {
1499 var v = JSON.parse(placed('nav-from') || 'null');
1500 return v && typeof v.list === 'string' && typeof v.to === 'string' && v.list.charAt(0) === '/' && v.list.charAt(1) !== '/' ? v : null;
1501 } catch (_) { return null; }
1502 }
1503 (function () {
junipercrew outdated

Unguarded JSON.parse on a storage value: malformed data throws on page load and stops the shell. Wrap in try/catch (same in leaveToList).
1504 var from = fromPlace();
1505 if (!from) return;
1506 if (location.pathname + location.search === from.list) {
1507 place('nav-from', null);
1508 var back = rows().filter(function (r) { var a = r.matches('a') ? r : r.querySelector('a'); return a && a.getAttribute('href') === from.href; })[0];
1509 if (back) { back.focus(); back.scrollIntoView({ block: 'nearest' }); }
1510 }
1511 })();
1512 function leaveToList() {
1513 var from = fromPlace();
1514 // The thing on top closes first: an open dialog or menu keeps its Escape.
1515 if (doc.querySelector('dialog[open], details.menu[open]')) return false;
1516 if (!from || location.pathname !== from.to || doc.querySelector('[data-nav]')) return false;
1517 location.assign(from.list);
1518 return true;
1519 }
14861520 function openFocused() {
14871521 var el = doc.activeElement.closest ? doc.activeElement.closest('[data-nav]') : null;
14881522 var link = el && (el.matches('a') ? el : el.querySelector('a'));
@@ -1517,7 +1551,13 @@ const shellCore = /* js */ `
15171551 function hints(on) { if (on) doc.documentElement.setAttribute('data-hints', ''); else doc.documentElement.removeAttribute('data-hints'); }
15181552 doc.addEventListener('keydown', function (e) { if (e.key === 'Alt') hints(true); });
15191553 doc.addEventListener('keyup', function (e) { if (e.key === 'Alt') hints(false); });
1520 doc.addEventListener('keydown', function (e) { if (e.key === 'Escape') hints(false); });
1554 // Escape clears the hints and is then spent: it leaves nothing else.
1555 var escClosedHints = false;
1556 doc.addEventListener('keydown', function (e) {
1557 if (e.key !== 'Escape') return;
1558 escClosedHints = doc.documentElement.hasAttribute('data-hints');
1559 hints(false);
1560 });
15211561 window.addEventListener('blur', function () { hints(false); });
15221562 doc.addEventListener('keydown', function (e) {
15231563 var t = e.target;
@@ -1541,6 +1581,9 @@ const shellCore = /* js */ `
15411581 return;
15421582 }
15431583 if (e.metaKey || e.ctrlKey || e.altKey) return;
1584 if (e.key === 'Escape' && e.defaultPrevented) return;
1585 if (e.key === 'Escape' && escClosedHints) { escClosedHints = false; e.preventDefault(); return; }
1586 if (e.key === 'Escape' && leaveToList()) { e.preventDefault(); return; }
15441587 if (Date.now() - last > 1200) seq.length = 0;
15451588 last = Date.now();
15461589 seq.push(e.key);
@@ -1566,8 +1609,8 @@ const shellCore = /* js */ `
15661609 if (box) { seq.length = 0; e.preventDefault(); box.focus(); box.select(); }
15671610 } else if (e.key === '?') {
15681611 seq.length = 0; e.preventDefault();
1569 // First press lays the keys over the page; the second clears them and opens the help.
1570 if (doc.documentElement.hasAttribute('data-hints')) { hints(false); openHelp(); } else hints(true);
1612 // One press, one sheet: the help opens at once (Option still lays the keys over the page).
1613 hints(false); openHelp();
15711614 }
15721615 else if (e.key === ':') { seq.length = 0; e.preventDefault(); openGoto(); }
15731616 });
src/web/studio/asks_page.ts +3 −3
@@ -211,9 +211,9 @@ function proposalBody(ctx: Ctx, d: Decision): Html | null {
211211 function decisionCard(ctx: Ctx, d: Decision, picked: ReadonlySet<string>): Html {
212212 const base = repoBase(d.repo);
213213 const title = d.pull
214 ? html`<a href="${changeHref(base, d.pull.number)}">${d.pull.title}</a>`
214 ? html`<a href="${changeHref(base, d.pull.number)}" data-nav>${d.pull.title}</a>`
215215 : d.issue
216 ? html`<a href="${base}/issues/${d.issue.number}">${d.issue.title}</a>`
216 ? html`<a href="${base}/issues/${d.issue.number}" data-nav>${d.issue.title}</a>`
217217 : d.proposal
218218 ? html`${d.proposal.title}`
219219 : html`<a href="${base}/settings/autonomy">Paused</a>`;
@@ -320,7 +320,7 @@ function rowCard(ctx: Ctx, r: AskRow<Decision>, picked: ReadonlySet<string>): Ht
320320 const box = pick(d, picked);
321321 return row({
322322 ...(box === null ? {} : { lead: box }),
323 title: html`<a href="${href}">${d.pull?.title ?? d.issue?.title ?? ""}</a>`,
323 title: html`<a href="${href}" data-nav>${d.pull?.title ?? d.issue?.title ?? ""}</a>`,
324324 meta: acts(ctx, d),
325325 });
326326 });
test/hotkey-hints.test.ts +3 −3
@@ -121,10 +121,10 @@ test("an element does not carry the key twice: no title beside its data-hint", a
121121 }
122122 });
123123
124 test("? lays the hints over the page and a second press clears them and opens the help; Escape clears", () => {
124 test("? opens the help at once, Alt lays the hints; Escape clears them", () => {
125125 const client = readFileSync("src/web/client.ts", "utf8");
126 expect(client).toContain("hasAttribute('data-hints')) { hints(false); openHelp(); } else hints(true)");
127 expect(client).toContain("e.key === 'Escape') hints(false)");
126 expect(client).toContain("hints(false); openHelp();");
127 expect(client).toContain("escClosedHints = doc.documentElement.hasAttribute('data-hints');");
128128 });
129129
130130 test("holding Option reveals hints; releasing or leaving hides them", () => {
test/keys-decide.test.ts added +136
@@ -0,0 +1,136 @@
1 /** The lists that ask for a decision answer j/k, enter, esc and `?` (#10710): driven through a real DOM. */
2 import { afterAll, beforeAll, expect, test } from "bun:test";
3 import { readFileSync } from "node:fs";
4 import { snappyFixture, type SnappyFixture } from "../bench/snappy.ts";
5 import { shellJs } from "../src/web/client.ts";
6
7 type Win = InstanceType<Awaited<typeof import("happy-dom")>["Window"]>;
8
9 const LIST = `<ul><li><a class="row" href="/a/b/issues/1" data-nav tabindex="-1">One</a></li>
10 <li><a class="row" href="/a/b/issues/2" data-nav tabindex="-1">Two</a></li></ul>`;
11
12 /** A page with the shell script running; `store` is the tab's sessionStorage. */
13 async function page(path: string, body: string, store: Record<string, string> = {}) {
14 const { Window } = await import("happy-dom");
15 const win = new Window({ url: `http://forge.test${path}` });
16 for (const [k, v] of Object.entries(store)) win.sessionStorage.setItem(k, v);
17 win.document.body.innerHTML = body;
junipercrew outdated

This reads source text; it passes with a broken handler. Drive a real DOM and assert focus after j, Enter, Esc.
18 (win.HTMLElement.prototype as any).scrollIntoView ??= () => {};
19 const errors: unknown[] = [];
20 win.addEventListener('error', (e: any) => errors.push(e.message ?? e.error));
21 const assigned: string[] = [];
22 const loc = new Proxy(win.location, {
23 get: (t, k) => (k === "assign" ? (u: string) => void assigned.push(u) : (t as any)[k]),
24 });
25 new Function("document", "window", "location", "sessionStorage", "CSS", "fetch", "EventSource", shellJs)(
26 win.document, win, loc, win.sessionStorage, { escape: (x: string) => x.replace(/([^\w-])/g, "\\$1") }, async () => ({ ok: false, status: 500, text: async () => "", headers: { get: () => null } }), undefined,
27 );
28 const key = (k: string, target: unknown = win.document) => {
29 const e = new win.KeyboardEvent("keydown", { key: k, bubbles: true, cancelable: true });
30 (target as any).dispatchEvent(e);
31 return e;
32 };
33 return { win, doc: win.document, key, assigned, errors };
34 }
35
36 const focusedHref = (w: Win) => (w.document.activeElement as any)?.getAttribute?.("href");
37
38 test("j moves down the rows, k up, and Enter opens the focused one, remembering the list", async () => {
39 const p = await page("/a/b/issues", LIST);
40 p.key("j");
41 expect(focusedHref(p.win)).toBe("/a/b/issues/1");
42 p.key("j");
43 expect(focusedHref(p.win)).toBe("/a/b/issues/2");
44 p.key("k");
45 expect(focusedHref(p.win)).toBe("/a/b/issues/1");
46 p.key("j");
47 let clicked = "";
48 p.doc.addEventListener("click", (e) => { clicked = (e.target as any).getAttribute("href"); e.preventDefault(); }, true);
49 p.key("Enter");
50 expect(clicked).toBe("/a/b/issues/2");
51 expect(JSON.parse(p.win.sessionStorage.getItem("nav-from")!)).toMatchObject({ list: "/a/b/issues", to: "/a/b/issues/2", href: "/a/b/issues/2" });
52 });
53
54 const place = JSON.stringify({ list: "/a/b/issues", to: "/a/b/issues/2", href: "/a/b/issues/2" });
55
56 test("Esc on the opened page goes back to its list", async () => {
57 const p = await page("/a/b/issues/2", "<main>thread</main>", { "nav-from": place });
58 const e = p.key("Escape");
59 expect(p.assigned).toEqual(["/a/b/issues"]);
60 expect(e.defaultPrevented).toBe(true);
61 });
62
63 test("the list refocuses the row that was opened, once", async () => {
64 const p = await page("/a/b/issues", LIST, { "nav-from": place });
65 expect(focusedHref(p.win)).toBe("/a/b/issues/2");
66 expect(p.win.sessionStorage.getItem("nav-from")).toBeNull();
67 });
68
69 test("Esc elsewhere than the opened page does nothing", async () => {
70 const p = await page("/a/b/pulls", "<main>x</main>", { "nav-from": place });
71 p.key("Escape");
72 expect(p.assigned).toEqual([]);
73 });
74
75 test("Esc with a dialog, an open menu or the hints on closes that first and does not leave", async () => {
76 const dlg = await page("/a/b/issues/2", "<main>t</main><dialog open>sheet</dialog>", { "nav-from": place });
77 dlg.key("Escape");
78 expect(dlg.assigned).toEqual([]);
79
80 const menu = await page("/a/b/issues/2", '<main>t</main><details class="menu" open><summary>m</summary></details>', { "nav-from": place });
81 menu.key("Escape");
82 expect(menu.doc.querySelector("details.menu")!.hasAttribute("open")).toBe(false);
83 expect(menu.assigned).toEqual([]);
84
85 const hints = await page("/a/b/issues/2", "<main>t</main>", { "nav-from": place });
86 hints.key("Alt");
87 expect(hints.doc.documentElement.hasAttribute("data-hints")).toBe(true);
88 hints.key("Escape");
89 expect(hints.doc.documentElement.hasAttribute("data-hints")).toBe(false);
90 expect(hints.assigned).toEqual([]);
91 hints.key("Escape");
92 expect(hints.assigned).toEqual(["/a/b/issues"]);
93 });
94
95 test("Esc in a field leaves the field, not the page", async () => {
96 const p = await page("/a/b/issues/2", "<main><textarea id=t></textarea></main>", { "nav-from": place });
97 const t = p.doc.getElementById("t") as any;
98 t.focus();
99 p.key("Escape", t);
100 expect(p.assigned).toEqual([]);
101 });
102
103 test("a malformed or foreign stored place never throws and never navigates", async () => {
104 for (const bad of ["{not json", "42", '{"list":"//evil.test/","to":"/a/b/issues/2"}', '{"list":"https://evil.test","to":"/a/b/issues/2"}']) {
105 const p = await page("/a/b/issues/2", "<main>t</main>", { "nav-from": bad });
106 p.key("j");
107 p.key("Escape");
108 expect(p.assigned).toEqual([]);
109 }
110 const l = await page("/a/b/issues", LIST, { "nav-from": "{oops" });
111 l.key("j");
112 expect(focusedHref(l.win)).toBe("/a/b/issues/1");
113 });
114
115 test("? opens the one help sheet on the first press", async () => {
116 const p = await page("/a/b/issues", `${LIST}<dialog id="help-dialog"><div data-help-body></div></dialog>`);
117 let shown = 0;
118 (p.doc.getElementById("help-dialog") as any).showModal = () => void shown++;
119 p.key("?");
120 expect(shown).toBe(1);
121 expect(p.doc.documentElement.hasAttribute("data-hints")).toBe(false);
122 });
123
124 let f: SnappyFixture;
125 beforeAll(async () => { f = await snappyFixture(); }, 120_000);
126 afterAll(async () => { await f.close(); });
127
128 test("the issues list the server draws carries data-nav rows, and the pulls list's rows are declared nav", async () => {
129 const repo = f.repos[0]!;
130 const get = async (path: string) => (await fetch(`${f.base}${path}`, { headers: { cookie: f.ownerCookie } })).text();
131 expect(await get(`/${repo.ownerName}/${repo.name}/issues`)).toMatch(/<[^>]*\bdata-nav\b/);
132 // The fixture has no open pull to draw; the row the pulls list builds is the one declared here.
133 const lists = readFileSync(new URL("../src/web/studio/lists.ts", import.meta.url), "utf8");
134 const pulls = lists.slice(lists.indexOf("export async function changesList"), lists.indexOf("export function isWorkingNow"));
135 expect(pulls).toMatch(/row\(\{\s*href: changeHref\(base, pull\.number\),\s*nav: true,/);
136 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs