amadan

eleven / messenger Public

web: unwedge the GitHub app — auto-load fired mid-render into paint's TDZ (#518)

The shipped bug (caught live by Paul, console: "ReferenceError: Cannot access
uninitialized variable", github.js:165): render kicked off the auto-fetch
BEFORE `const paint` was initialized further down; fetchRepos called paint()
in its temporal dead zone, threw before the busy-reset, and the module-level
flag wedged — the app froze on "loading…" and every later Refresh silently
no-oped. Two changes, either of which alone would have prevented it:

- Auto-loads move to the END of render, after the surface (and every const
  the fetchers touch) is fully built.
- Both fetchers run their whole body under try/catch/finally: any throw lands
  in fail() as an honest inline error and always releases busy.

Plus the regression rig fold tests can't provide: test/github-render.test.mjs
drives render twice over a stub DOM and asserts the fetch path both runs and
RELEASES — it crashes outright on the pre-fix code.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell d7f96368b4441fe47a0738b19e769f79f8dfc168 parent 8027bc4
3 files changed, +139 −12
  • package.json +1 −1
  • test/github-render.test.mjs +111 −0
  • web/static/apps/github.js +27 −11
diff --git a/package.json b/package.json
index a69322f..c05ab62 100644
--- a/package.json
+++ b/package.json
@@ -3,6 +3,6 @@
"private": true,
"type": "module",
"scripts": {
- "test": "node test/crypto.test.mjs && node test/ui.test.mjs && node test/format.test.mjs && node test/bootscene.test.mjs && node test/message.test.mjs && node test/threadkey.test.mjs && node test/messageblob.test.mjs && node test/blobbytes.test.mjs && node test/nativeblob.test.mjs && node test/apps.test.mjs && node test/cards.test.mjs && node test/event-vectors.test.mjs && node test/flow.test.mjs && node test/tutorial.test.mjs && node test/actions.test.mjs && node test/commands.test.mjs && node test/context.test.mjs && node test/knowledge.test.mjs && node test/invite.test.mjs && node test/backup.test.mjs && node test/connect.test.mjs && node test/serviceop.test.mjs && node test/github.test.mjs && node test/check-app.test.mjs && node test/sandbox.test.mjs && node test/sandbox-bridge.test.mjs && node home/test/crypto.test.mjs && node home/test/restore-payload.test.mjs && node home/test/serverlist.test.mjs && node home/test/fader.test.mjs"
+ "test": "node test/crypto.test.mjs && node test/ui.test.mjs && node test/format.test.mjs && node test/bootscene.test.mjs && node test/message.test.mjs && node test/threadkey.test.mjs && node test/messageblob.test.mjs && node test/blobbytes.test.mjs && node test/nativeblob.test.mjs && node test/apps.test.mjs && node test/cards.test.mjs && node test/event-vectors.test.mjs && node test/flow.test.mjs && node test/tutorial.test.mjs && node test/actions.test.mjs && node test/commands.test.mjs && node test/context.test.mjs && node test/knowledge.test.mjs && node test/invite.test.mjs && node test/backup.test.mjs && node test/connect.test.mjs && node test/serviceop.test.mjs && node test/github.test.mjs && node test/github-render.test.mjs && node test/check-app.test.mjs && node test/sandbox.test.mjs && node test/sandbox-bridge.test.mjs && node home/test/crypto.test.mjs && node home/test/restore-payload.test.mjs && node home/test/serverlist.test.mjs && node home/test/fader.test.mjs"
}
}
diff --git a/test/github-render.test.mjs b/test/github-render.test.mjs
new file mode 100644
index 0000000..4f86e85
--- /dev/null
+++ b/test/github-render.test.mjs
@@ -0,0 +1,111 @@
+// Render smoke test for the GitHub app — the regression that motivated it:
+// the auto-load fired mid-render and hit `paint`'s temporal dead zone, threw
+// before the busy-reset, and wedged the app on "loading…" forever (every
+// later Refresh silently swallowed). Fold tests can't see render, so this
+// drives it twice over a stub DOM and proves the fetch path both RUNS and
+// RELEASES. node test/github-render.test.mjs
+import assert from "node:assert/strict";
+
+// A just-enough DOM: every element accepts children, classes, listeners.
+function fakeEl(tag = "div") {
+ return {
+ tagName: tag,
+ children: [],
+ textContent: "",
+ className: "",
+ hidden: false,
+ disabled: false,
+ value: "",
+ placeholder: "",
+ type: "",
+ onclick: null,
+ ondblclick: null,
+ appendChild(c) {
+ this.children.push(c);
+ return c;
+ },
+ append(...cs) {
+ this.children.push(...cs);
+ },
+ addEventListener() {},
+ querySelector() {
+ return fakeEl();
+ },
+ classList: { add() {}, remove() {}, toggle() {} },
+ focus() {},
+ scrollIntoView() {},
+ };
+}
+globalThis.document = {
+ createElement: (tag) => fakeEl(tag),
+ getElementById: () => null, // injectStyles' dedupe check
+ head: fakeEl("head"),
+ activeElement: null,
+ body: null,
+};
+globalThis.window = globalThis;
+
+const { app } = await import("../web/static/apps/github.js");
+
+let failures = 0;
+async function test(name, fn) {
+ try {
+ await fn();
+ console.log(`PASS ${name}`);
+ } catch (e) {
+ failures++;
+ console.log(`FAIL ${name} — ${e.message.split("\n")[0]}`);
+ }
+}
+
+const tick = () => new Promise((r) => setTimeout(r, 0));
+
+function ctxWith(requestOp) {
+ const state = { repos: [], fetchedAt: 0, issues: {} };
+ return {
+ threadId: "t1",
+ host: { connected: { requestOp } },
+ getState: () => state,
+ emit: () => {},
+ onCleanup: () => {},
+ id: () => "id",
+ };
+}
+
+await test("render survives the auto-load and RELEASES busy for the next fetch", async () => {
+ let calls = 0;
+ const ctx = ctxWith(async () => {
+ calls++;
+ return { ok: false, error: "stub" };
+ });
+ // First render auto-fetches (fetchedAt 0). A throw anywhere in that path —
+ // the shipped TDZ bug — would reject unhandled and wedge the module `busy`.
+ app.render(undefined, fakeEl(), ctx);
+ await tick();
+ await tick();
+ assert.equal(calls, 1, "auto-load ran");
+ // Second render must be able to fetch again: a wedged busy flag (the bug's
+ // signature) would swallow this silently and calls would stay at 1.
+ app.render(undefined, fakeEl(), ctx);
+ await tick();
+ await tick();
+ assert.equal(calls, 2, "busy was released — the app can fetch again");
+});
+
+await test("a requestOp that THROWS still lands in the error path and releases", async () => {
+ let calls = 0;
+ const ctx = ctxWith(async () => {
+ calls++;
+ throw new Error("boom");
+ });
+ app.render(undefined, fakeEl(), ctx);
+ await tick();
+ await tick();
+ app.render(undefined, fakeEl(), ctx);
+ await tick();
+ await tick();
+ assert.equal(calls, 2, "a thrown fetch must not wedge the app");
+});
+
+console.log(failures === 0 ? "\nALL PASSED" : `\n${failures} FAILED`);
+process.exit(failures === 0 ? 0 : 1);
diff --git a/web/static/apps/github.js b/web/static/apps/github.js
index 4575127..a8875ec 100644
--- a/web/static/apps/github.js
+++ b/web/static/apps/github.js
@@ -158,14 +158,22 @@ export const app = defineApp({
return;
}
+ // Both fetchers run their WHOLE body under try/finally: a throw anywhere
+ // (even a plain bug) must land in fail() and release `busy` — a wedged
+ // module-level flag silently swallows every later Refresh, which is
+ // exactly the failure that shipped: paint() here hit the temporal dead
+ // zone when the auto-load fired mid-render, threw before the old finally,
+ // and froze the app on "loading…" for good.
const fetchRepos = async () => {
if (busy) return;
busy = true;
- note.textContent = "";
- paint();
let res;
try {
+ note.textContent = "";
+ paint();
res = await host.requestOp("github", "repos.list", {});
+ } catch (e) {
+ res = { ok: false, error: (e && e.message) || String(e) };
} finally {
busy = false;
autoLoading = false;
@@ -189,11 +197,13 @@ export const app = defineApp({
const [owner, repo] = fullName.split("/");
if (!owner || !repo) return;
busy = true;
- note.textContent = "";
- paint();
let res;
try {
+ note.textContent = "";
+ paint();
res = await host.requestOp("github", "issues.list", { owner, repo });
+ } catch (e) {
+ res = { ok: false, error: (e && e.message) || String(e) };
} finally {
busy = false;
}
@@ -562,9 +572,6 @@ export const app = defineApp({
el.append(form, filter, listOrEmpty, note);
drawList = drawIssues;
- const entry = issues[openRepo];
- // Opened with nothing (or stale > 2 min) → pull once, automatically.
- if (!busy && (!entry || Date.now() - (entry.fetchedAt || 0) > 120000)) fetchIssues(openRepo);
} else {
title.textContent = "Your repositories";
const refresh = document.createElement("button");
@@ -576,10 +583,6 @@ export const app = defineApp({
el.append(filter, listOrEmpty, note);
drawList = drawRepos;
- if (!state.fetchedAt && !busy && !autoLoading) {
- autoLoading = true;
- fetchRepos();
- }
}
// Transient bits (busy label, count line) live outside folded state.
@@ -595,5 +598,18 @@ export const app = defineApp({
drawList();
paint();
+
+ // Auto-loads kick off ONLY after the surface is fully built — the
+ // fetchers call paint(), so firing them mid-render hit paint's temporal
+ // dead zone: a ReferenceError before the busy-reset, and the app wedged
+ // on "loading…" with every later Refresh silently swallowed.
+ if (openRepo) {
+ const entry = issues[openRepo];
+ // Opened with nothing (or stale > 2 min) → pull once, automatically.
+ if (!busy && (!entry || Date.now() - (entry.fetchedAt || 0) > 120000)) fetchIssues(openRepo);
+ } else if (!state.fetchedAt && !busy && !autoLoading) {
+ autoLoading = true;
+ fetchRepos();
+ }
},
});