From 4f12cd14a75baa3522887bcafcbc845163d5eda7 Mon Sep 17 00:00:00 2001 From: testvalue Date: Tue, 7 Apr 2026 22:12:38 -0400 Subject: [PATCH 01/12] fix(onboarding): freezes org display order after initial sort --- .../components/onboarding/RepoSelector.tsx | 45 ++- .../onboarding/RepoSelector.test.tsx | 329 ++++++++++++++++++ 2 files changed, 367 insertions(+), 7 deletions(-) diff --git a/src/app/components/onboarding/RepoSelector.tsx b/src/app/components/onboarding/RepoSelector.tsx index cdb991a0..b20548a9 100644 --- a/src/app/components/onboarding/RepoSelector.tsx +++ b/src/app/components/onboarding/RepoSelector.tsx @@ -342,21 +342,52 @@ export default function RepoSelector(props: RepoSelectorProps) { new Set((props.monitoredRepos ?? []).map((r) => r.fullName)) ); + // Plain lets (not signals) — memoization cache inside createMemo; signals would cause reactive cycles. + let frozenOrder: string[] | null = null; + let frozenOrgsKey = ""; + const sortedOrgStates = createMemo(() => { const states = orgStates(); - // Defer sorting until all orgs have loaded: prevents layout shift during - // trickle-in, and ensures each org's type ("user" vs "org") is resolved - // from fetchOrgs before we sort on it. loadedCount is not reset by retryOrg, - // so sorting stays active during retries. + + // Invalidate frozen order when org membership changes (key is order-independent). + const currentKey = [...props.selectedOrgs].sort().join(","); + if (currentKey !== frozenOrgsKey) { + frozenOrder = null; + frozenOrgsKey = currentKey; + } + + // Replay frozen order if available, appending any orgs not yet in the list. + if (frozenOrder !== null) { + const stateMap = new Map(states.map((s) => [s.org, s])); + const result: OrgRepoState[] = []; + for (const org of frozenOrder) { + const s = stateMap.get(org); + if (s) { + result.push(s); + stateMap.delete(org); + } + } + for (const s of stateMap.values()) result.push(s); + return result; + } + + // Defer sorting until all orgs have loaded (prevents layout shift during trickle-in). if (loadedCount() < props.selectedOrgs.length) return states; - // Order: personal org first, then remaining orgs alphabetically. - // Repos within each org retain their existing recency order from fetchRepos. - return [...states].sort((a, b) => { + + // Guard against stale orgStates: the memo runs synchronously when selectedOrgs changes, + // but createEffect resets orgStates asynchronously, so stale entries may still be present. + const selectedOrgSet = new Set(props.selectedOrgs); + if (states.some((s) => !selectedOrgSet.has(s.org))) return states; + + // Sort: personal org first, then alphabetically. Capture order to freeze it. + const sorted = [...states].sort((a, b) => { const aIsUser = a.type === "user" ? 0 : 1; const bIsUser = b.type === "user" ? 0 : 1; if (aIsUser !== bIsUser) return aIsUser - bIsUser; return a.org.localeCompare(b.org, "en"); }); + frozenOrder = sorted.map((s) => s.org); + return sorted; }); function toRepoRef(entry: RepoEntry): RepoRef { diff --git a/tests/components/onboarding/RepoSelector.test.tsx b/tests/components/onboarding/RepoSelector.test.tsx index 17496fa5..c81e91b4 100644 --- a/tests/components/onboarding/RepoSelector.test.tsx +++ b/tests/components/onboarding/RepoSelector.test.tsx @@ -1490,3 +1490,332 @@ describe("RepoSelector — org accordion", () => { }); }); }); + +// ── Frozen org order (C5) ───────────────────────────────────────────────────── +// S4 (initial sort order) is covered by the existing test at line 238, which verifies orgs appear +// in sorted order after all fetchRepos calls complete. S1 and S2 below cover post-sort stability +// (frozen order unchanged after repo retry and checkbox toggle respectively). +// S5 (frozen order in accordion mode with pre-loaded entries) is deferred pending accordion merge. + +describe("RepoSelector — frozen org order", () => { + function makeOrgRepos(org: string): RepoEntry[] { + return [{ owner: org, name: `${org}-repo`, fullName: `${org}/${org}-repo`, pushedAt: "2026-03-20T10:00:00Z" }]; + } + + // Flat (non-accordion) mode only: org names appear as plain text nodes in headers. + // In accordion mode, org names are inside button triggers — use different query selectors (see S6). + function getOrgHeaderOrder(orgNames: string[]): string[] { + const pattern = new RegExp(`^(${orgNames.join("|")})$`); + return screen.getAllByText(pattern).map((el) => el.textContent!); + } + + beforeEach(() => { + vi.clearAllMocks(); + vi.restoreAllMocks(); + }); + + // S1: Org order remains stable after repo retry + it("org order remains stable after repo retry for a failed org", async () => { + const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; + const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; + const betaEntry = { login: "beta-org", avatarUrl: "", type: "org" as const }; + + vi.mocked(api.fetchRepos) + .mockImplementation((_client, org) => { + if (org === "beta-org") return Promise.reject(new Error("beta load failed")); + return Promise.resolve(makeOrgRepos(org as string)); + }); + + render(() => ( + + )); + + // Wait for alice and acme-corp to succeed and beta-org to fail (Retry visible) + await waitFor(() => { + screen.getByText("alice-repo"); + screen.getByText("acme-corp-repo"); + screen.getByText("Retry"); + }); + + // Verify initial sorted order: alice (user first), acme-corp, beta-org + const initialOrder = getOrgHeaderOrder(["alice", "acme-corp", "beta-org"]); + expect(initialOrder).toEqual(["alice", "acme-corp", "beta-org"]); + + // Set up retry to succeed for all + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + fireEvent.click(screen.getByText("Retry")); + + await waitFor(() => { + screen.getByText("beta-org-repo"); + }); + + // Order must be unchanged after retry + const orderAfterRetry = getOrgHeaderOrder(["alice", "acme-corp", "beta-org"]); + expect(orderAfterRetry).toEqual(["alice", "acme-corp", "beta-org"]); + }); + + // S2: Org order remains stable when toggling a repo checkbox + it("org order remains stable when toggling a repo checkbox", async () => { + const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; + const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; + const betaEntry = { login: "beta-org", avatarUrl: "", type: "org" as const }; + + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + render(() => ( + + )); + + await waitFor(() => { + screen.getByText("alice-repo"); + screen.getByText("acme-corp-repo"); + screen.getByText("beta-org-repo"); + }); + + // Verify initial order + const initialOrder = getOrgHeaderOrder(["alice", "acme-corp", "beta-org"]); + expect(initialOrder).toEqual(["alice", "acme-corp", "beta-org"]); + + // Toggle a checkbox under acme-corp + const acmeCheckbox = screen.getAllByRole("checkbox").find((cb) => { + const label = cb.closest("label"); + return label?.textContent?.includes("acme-corp-repo"); + }); + expect(acmeCheckbox).not.toBeUndefined(); + fireEvent.click(acmeCheckbox!); + + // Order must be unchanged after toggle + const orderAfterToggle = getOrgHeaderOrder(["alice", "acme-corp", "beta-org"]); + expect(orderAfterToggle).toEqual(["alice", "acme-corp", "beta-org"]); + }); + + // S3: Frozen order invalidated when a new org is added + it("frozen order is invalidated and re-sorted when a new org is added", async () => { + const { createSignal } = await import("solid-js"); + + const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; + const deltaEntry = { login: "delta-inc", avatarUrl: "", type: "org" as const }; + const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; + + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + const [selectedOrgs, setSelectedOrgs] = createSignal(["alice", "delta-inc"]); + const [orgEntries, setOrgEntries] = createSignal([aliceEntry, deltaEntry]); + + render(() => ( + + )); + + await waitFor(() => { + screen.getByText("alice-repo"); + screen.getByText("delta-inc-repo"); + }); + + // Verify initial frozen order: alice (user first), delta-inc + const initialOrder = getOrgHeaderOrder(["alice", "delta-inc"]); + expect(initialOrder).toEqual(["alice", "delta-inc"]); + + // Add acme-corp — this should invalidate the frozen order + setSelectedOrgs(["alice", "delta-inc", "acme-corp"]); + setOrgEntries([aliceEntry, deltaEntry, acmeEntry]); + + await waitFor(() => { + screen.getByText("acme-corp-repo"); + }); + + // New order should be re-sorted: alice (user), acme-corp, delta-inc + const orderAfterAdd = getOrgHeaderOrder(["alice", "acme-corp", "delta-inc"]); + expect(orderAfterAdd).toEqual(["alice", "acme-corp", "delta-inc"]); + }); + + // S6: New org appears in correct sorted position with 6+ orgs (accordion mode) + it("new org appears in correct sorted position with 6+ orgs in accordion mode", async () => { + const { createSignal } = await import("solid-js"); + + const startOrgs = ["alice", "acme-corp", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]; + const startEntries = startOrgs.map((login) => ({ + login, + avatarUrl: "", + type: login === "alice" ? ("user" as const) : ("org" as const), + })); + + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + const [selectedOrgs, setSelectedOrgs] = createSignal(startOrgs); + const [orgEntries, setOrgEntries] = createSignal(startEntries); + + render(() => ( + + )); + + // Wait for accordion mode to render (first org expanded by default) + await waitFor(() => { + screen.getByRole("button", { name: /alice/ }); + }); + + // Verify initial sorted order via accordion trigger buttons + const getAccordionOrder = (orgNames: string[]) => { + return orgNames + .map((name) => screen.getByRole("button", { name: new RegExp(name) })) + .sort((a, b) => { + const pos = a.compareDocumentPosition(b); + return pos & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1; + }) + .map((btn) => { + const match = btn.textContent?.match(/^(alice|acme-corp|charlie-co|delta-inc|echo-labs|foxtrot-io|beta-org)/); + return match ? match[1] : ""; + }); + }; + + const initialOrder = getAccordionOrder(startOrgs); + expect(initialOrder).toEqual(["alice", "acme-corp", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]); + + // Add beta-org (7 total) + const betaEntry = { login: "beta-org", avatarUrl: "", type: "org" as const }; + const newOrgs = ["alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]; + setSelectedOrgs(newOrgs); + setOrgEntries([...startEntries, betaEntry]); + + await waitFor(() => { + screen.getByRole("button", { name: /beta-org/ }); + }); + + // Verify new order with beta-org inserted between acme-corp and charlie-co + expect(getAccordionOrder(newOrgs)).toEqual(["alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]); + }); + + // S7: Frozen order invalidated on simultaneous add and remove + // Uses aaa-org (sorts BEFORE acme-corp alphabetically) to expose the stale-freeze bug: + // if the freeze is not invalidated, aaa-org would be appended at the end instead + // of appearing before acme-corp in sorted order. + it("frozen order is invalidated when orgs are simultaneously added and removed", async () => { + const { createSignal } = await import("solid-js"); + + const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; + const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; + const deltaEntry = { login: "delta-inc", avatarUrl: "", type: "org" as const }; + const aaaEntry = { login: "aaa-org", avatarUrl: "", type: "org" as const }; + + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + const [selectedOrgs, setSelectedOrgs] = createSignal(["alice", "acme-corp", "delta-inc"]); + const [orgEntries, setOrgEntries] = createSignal([aliceEntry, acmeEntry, deltaEntry]); + + render(() => ( + + )); + + await waitFor(() => { + screen.getByText("alice-repo"); + screen.getByText("acme-corp-repo"); + screen.getByText("delta-inc-repo"); + }); + + // Verify initial frozen order + const initialOrder = getOrgHeaderOrder(["alice", "acme-corp", "delta-inc"]); + expect(initialOrder).toEqual(["alice", "acme-corp", "delta-inc"]); + + // Simultaneously remove delta-inc and add aaa-org (same count, different membership) + // aaa-org sorts alphabetically BEFORE acme-corp, so correct order is: alice, aaa-org, acme-corp + // If the stale-freeze bug is present, aaa-org would be appended at end: alice, acme-corp, aaa-org + setSelectedOrgs(["alice", "acme-corp", "aaa-org"]); + setOrgEntries([aliceEntry, acmeEntry, aaaEntry]); + + await waitFor(() => { + screen.getByText("aaa-org-repo"); + }); + + // delta-inc should be gone + expect(screen.queryByText("delta-inc")).toBeNull(); + + // New order should be re-sorted: alice (user first), aaa-org, acme-corp (alpha sort) + const orderAfterSwap = getOrgHeaderOrder(["alice", "aaa-org", "acme-corp"]); + expect(orderAfterSwap).toEqual(["alice", "aaa-org", "acme-corp"]); + }); + + // QA-012: Pure org-removal invalidates frozen order and re-sorts correctly + it("removing an org invalidates frozen order and re-sorts correctly", async () => { + const { createSignal } = await import("solid-js"); + + const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; + const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; + const deltaEntry = { login: "delta-inc", avatarUrl: "", type: "org" as const }; + + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + const [selectedOrgs, setSelectedOrgs] = createSignal(["alice", "acme-corp", "delta-inc"]); + const [orgEntries, setOrgEntries] = createSignal([aliceEntry, acmeEntry, deltaEntry]); + + render(() => ( + + )); + + await waitFor(() => { + screen.getByText("alice-repo"); + screen.getByText("acme-corp-repo"); + screen.getByText("delta-inc-repo"); + }); + + // Verify initial frozen order: alice (user first), acme-corp, delta-inc + const initialOrder = getOrgHeaderOrder(["alice", "acme-corp", "delta-inc"]); + expect(initialOrder).toEqual(["alice", "acme-corp", "delta-inc"]); + + // Remove delta-inc (3 orgs → 2 orgs) + setSelectedOrgs(["alice", "acme-corp"]); + setOrgEntries([aliceEntry, acmeEntry]); + + await waitFor(() => { + expect(screen.queryByText("delta-inc")).toBeNull(); + }); + + // Verify only 2 orgs remain in correct sorted order + const orderAfterRemoval = getOrgHeaderOrder(["alice", "acme-corp"]); + expect(orderAfterRemoval).toEqual(["alice", "acme-corp"]); + + // delta-inc repo content should also be gone from the display + expect(screen.queryByText("delta-inc-repo")).toBeNull(); + }); +}); From 13c974d0ba3b4d6642407f9888f7999cec82440e Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 08:13:26 -0400 Subject: [PATCH 02/12] fix(onboarding): addresses quality gate review findings --- .../components/onboarding/RepoSelector.tsx | 4 +++- .../onboarding/RepoSelector.test.tsx | 20 ++++++++----------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/app/components/onboarding/RepoSelector.tsx b/src/app/components/onboarding/RepoSelector.tsx index b20548a9..0ce10cad 100644 --- a/src/app/components/onboarding/RepoSelector.tsx +++ b/src/app/components/onboarding/RepoSelector.tsx @@ -342,7 +342,8 @@ export default function RepoSelector(props: RepoSelectorProps) { new Set((props.monitoredRepos ?? []).map((r) => r.fullName)) ); - // Plain lets (not signals) — memoization cache inside createMemo; signals would cause reactive cycles. + // Plain let variables, not signals — writing to a signal from inside createMemo is a + // side effect that triggers re-evaluation, causing an infinite loop. let frozenOrder: string[] | null = null; let frozenOrgsKey = ""; @@ -376,6 +377,7 @@ export default function RepoSelector(props: RepoSelectorProps) { // Guard against stale orgStates: the memo runs synchronously when selectedOrgs changes, // but createEffect resets orgStates asynchronously, so stale entries may still be present. + // Return the stale list (not []) to avoid a flash-to-empty before the effect resets state. const selectedOrgSet = new Set(props.selectedOrgs); if (states.some((s) => !selectedOrgSet.has(s.org))) return states; diff --git a/tests/components/onboarding/RepoSelector.test.tsx b/tests/components/onboarding/RepoSelector.test.tsx index c81e91b4..81233d92 100644 --- a/tests/components/onboarding/RepoSelector.test.tsx +++ b/tests/components/onboarding/RepoSelector.test.tsx @@ -1492,9 +1492,9 @@ describe("RepoSelector — org accordion", () => { }); // ── Frozen org order (C5) ───────────────────────────────────────────────────── -// S4 (initial sort order) is covered by the existing test at line 238, which verifies orgs appear -// in sorted order after all fetchRepos calls complete. S1 and S2 below cover post-sort stability -// (frozen order unchanged after repo retry and checkbox toggle respectively). +// Initial sort order is covered by the existing "shows personal org first" test (line 238). +// S1 and S2 below cover post-sort stability (frozen order unchanged after repo retry and +// checkbox toggle respectively). // S5 (frozen order in accordion mode with pre-loaded entries) is deferred pending accordion merge. describe("RepoSelector — frozen org order", () => { @@ -1683,18 +1683,14 @@ describe("RepoSelector — frozen org order", () => { }); // Verify initial sorted order via accordion trigger buttons - const getAccordionOrder = (orgNames: string[]) => { - return orgNames - .map((name) => screen.getByRole("button", { name: new RegExp(name) })) + const getAccordionOrder = (orgNames: string[]) => + orgNames + .map((name) => ({ name, btn: screen.getByRole("button", { name: new RegExp(name) }) })) .sort((a, b) => { - const pos = a.compareDocumentPosition(b); + const pos = a.btn.compareDocumentPosition(b.btn); return pos & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1; }) - .map((btn) => { - const match = btn.textContent?.match(/^(alice|acme-corp|charlie-co|delta-inc|echo-labs|foxtrot-io|beta-org)/); - return match ? match[1] : ""; - }); - }; + .map(({ name }) => name); const initialOrder = getAccordionOrder(startOrgs); expect(initialOrder).toEqual(["alice", "acme-corp", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]); From 15ca414e732e24ca2c8e3d8aeee5eb0a48e953f8 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 09:25:41 -0400 Subject: [PATCH 03/12] test(onboarding): implements S5 accordion retry stability test --- .../onboarding/RepoSelector.test.tsx | 72 ++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/tests/components/onboarding/RepoSelector.test.tsx b/tests/components/onboarding/RepoSelector.test.tsx index 81233d92..d08f7583 100644 --- a/tests/components/onboarding/RepoSelector.test.tsx +++ b/tests/components/onboarding/RepoSelector.test.tsx @@ -1495,7 +1495,7 @@ describe("RepoSelector — org accordion", () => { // Initial sort order is covered by the existing "shows personal org first" test (line 238). // S1 and S2 below cover post-sort stability (frozen order unchanged after repo retry and // checkbox toggle respectively). -// S5 (frozen order in accordion mode with pre-loaded entries) is deferred pending accordion merge. +// S5 below covers retry stability in accordion mode (aria-expanded preservation after retry). describe("RepoSelector — frozen org order", () => { function makeOrgRepos(org: string): RepoEntry[] { @@ -1709,6 +1709,76 @@ describe("RepoSelector — frozen org order", () => { expect(getAccordionOrder(newOrgs)).toEqual(["alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]); }); + // S5: Org order stable with 7 orgs after retry in accordion mode + it("org order stable with 7 orgs after retry in accordion mode (S5)", async () => { + const sevenOrgs = ["alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]; + const sevenEntries = sevenOrgs.map((login) => ({ + login, + avatarUrl: "", + type: login === "alice" ? ("user" as const) : ("org" as const), + })); + + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => { + if (org === "echo-labs") return Promise.reject(new Error("echo load failed")); + return Promise.resolve(makeOrgRepos(org as string)); + }); + + render(() => ( + + )); + + // Wait for accordion mode to render + await waitFor(() => { + screen.getByRole("button", { name: /alice/ }); + }); + + // Expand echo-labs to reveal its error state and Retry button + const echoBtn = screen.getByRole("button", { name: /echo-labs/ }); + fireEvent.click(echoBtn); + expect(echoBtn.getAttribute("aria-expanded")).toBe("true"); + + await waitFor(() => { + screen.getByText("Retry"); + }); + + // Verify initial sorted order via accordion trigger buttons + const getAccordionOrder = (orgNames: string[]) => + orgNames + .map((name) => ({ name, btn: screen.getByRole("button", { name: new RegExp(name) }) })) + .sort((a, b) => { + const pos = a.btn.compareDocumentPosition(b.btn); + return pos & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1; + }) + .map(({ name }) => name); + + const initialOrder = getAccordionOrder(sevenOrgs); + expect(initialOrder).toEqual(["alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]); + + // Set up retry to succeed + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + fireEvent.click(screen.getByText("Retry")); + + await waitFor(() => { + screen.getByText("echo-labs-repo"); + }); + + // Order must be unchanged after retry + const orderAfterRetry = getAccordionOrder(sevenOrgs); + expect(orderAfterRetry).toEqual(["alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]); + + // The expanded panel (echo-labs) must remain expanded + const echoBtnAfter = screen.getByRole("button", { name: /echo-labs/ }); + expect(echoBtnAfter.getAttribute("aria-expanded")).toBe("true"); + }); + // S7: Frozen order invalidated on simultaneous add and remove // Uses aaa-org (sorts BEFORE acme-corp alphabetically) to expose the stale-freeze bug: // if the freeze is not invalidated, aaa-org would be appended at the end instead From f69083e1d584f4d308a11b7b2f2f6e821237b9bf Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 09:57:13 -0400 Subject: [PATCH 04/12] test(onboarding): adds BDD step definitions for org order stability --- package.json | 1 + pnpm-lock.yaml | 82 ++++ tests/acceptance/org-order-stability.feature | 52 +++ .../steps/org-order-stability.steps.tsx | 428 ++++++++++++++++++ 4 files changed, 563 insertions(+) create mode 100644 tests/acceptance/org-order-stability.feature create mode 100644 tests/acceptance/steps/org-order-stability.steps.tsx diff --git a/package.json b/package.json index bd1d9e7e..949fd5af 100644 --- a/package.json +++ b/package.json @@ -30,6 +30,7 @@ "zod": "4.3.6" }, "devDependencies": { + "@amiceli/vitest-cucumber": "^6.3.0", "@cloudflare/vite-plugin": "1.30.1", "@cloudflare/vitest-pool-workers": "0.13.4", "@playwright/test": "1.58.2", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6f56606c..4ab70dd1 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -39,6 +39,9 @@ importers: specifier: 4.3.6 version: 4.3.6 devDependencies: + '@amiceli/vitest-cucumber': + specifier: ^6.3.0 + version: 6.3.0(vitest@4.1.1(@types/node@25.5.0)(happy-dom@20.8.9)(vite@8.0.1(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0))) '@cloudflare/vite-plugin': specifier: 1.30.1 version: 1.30.1(vite@8.0.5(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0))(workerd@1.20260317.1)(wrangler@4.77.0) @@ -127,6 +130,12 @@ importers: packages: + '@amiceli/vitest-cucumber@6.3.0': + resolution: {integrity: sha512-oTpWz4T2V/uCXIxIDfSX6TQ3SQ1E53oANhXJzV7iLIdVwDsu43y9zLbizNoR62Dgq76BiOj/DJpO31yU97cYhw==} + hasBin: true + peerDependencies: + vitest: ^4.0.4 + '@babel/code-frame@7.29.0': resolution: {integrity: sha512-9NhCeYjq9+3uxgdtp20LSiJXJvN0FeCtNGpJxuMFZ1Kv3cWUNb6DOhJwUvcVCzKGR66cw4njwM6hrJLqgOwbcw==} engines: {node: '>=6.9.0'} @@ -1183,6 +1192,9 @@ packages: peerDependencies: '@testing-library/dom': '>=7.21.4' + '@ts-morph/common@0.28.1': + resolution: {integrity: sha512-W74iWf7ILp1ZKNYXY5qbddNaml7e9Sedv5lvU1V8lftlitkc9Pq1A+jlH23ltDgWYeZFFEqGCD1Ies9hqu3O+g==} + '@tybys/wasm-util@0.10.1': resolution: {integrity: sha512-9tTaPJLSiejZKx+Bmog4uSubteqTvFrVrURwkmHixBo0G4seD0zUxp98E1DzUBJxLQ3NPwXrGKDiVjwx/DpPsg==} @@ -1329,6 +1341,10 @@ packages: solid-js: optional: true + balanced-match@4.0.4: + resolution: {integrity: sha512-BLrgEcRTwX2o6gGxGOCNyMvGSp35YofuYzw9h1IMTRmKqttAZZVU67bdb9Pr2vUHA8+j3i2tJfjO6C6+4myGTA==} + engines: {node: 18 || 20 || >=22} + baseline-browser-mapping@2.10.9: resolution: {integrity: sha512-OZd0e2mU11ClX8+IdXe3r0dbqMEznRiT4TfbhYIbcRPZkqJ7Qwer8ij3GZAmLsRKa+II9V1v5czCkvmHH3XZBg==} engines: {node: '>=6.0.0'} @@ -1347,6 +1363,10 @@ packages: bottleneck@2.19.5: resolution: {integrity: sha512-VHiNCbI1lKdl44tGrhNfU3lup0Tj/ZBMJB5/2ZbNXRCPuRCO7ed2mgcK4r17y+KB2EfuYuRaVlwNbAeaWGSpbw==} + brace-expansion@5.0.5: + resolution: {integrity: sha512-VZznLgtwhn+Mact9tfiwx64fA9erHH/MCXEUfB/0bX/6Fz6ny5EGTXYltMocqg4xFAQZtnO3DHWWXi8RiuN7cQ==} + engines: {node: 18 || 20 || >=22} + browserslist@4.28.1: resolution: {integrity: sha512-ZC5Bd0LgJXgwGqUknZY/vkUQ04r8NXnJZ3yYi4vDmSiZmC/pdSN0NbNRPxZpbtO4uAfDUAFffO8IZoM3Gj8IkA==} engines: {node: ^6 || ^7 || ^8 || ^9 || ^10 || ^11 || ^12 || >=13.7} @@ -1374,6 +1394,10 @@ packages: resolution: {integrity: sha512-+ys997U96po4Kx/ABpBCqhA9EuxJaQWDQg7295H4hBphv3IZg0boBKuwYpt4YXp6MZ5AmZQnU/tyMTlRpaSejg==} engines: {node: '>= 0.4'} + callsites@4.2.0: + resolution: {integrity: sha512-kfzR4zzQtAE9PC7CzZsjl3aBNbXWuXiSeOCdLcPpBfGW8YuCqQHcRPFDbr/BPVmd3EEPVpuFzLyuT/cUhPr4OQ==} + engines: {node: '>=12.20'} + caniuse-lite@1.0.30001780: resolution: {integrity: sha512-llngX0E7nQci5BPJDqoZSbuZ5Bcs9F5db7EtgfwBerX9XGtkkiO4NwfDDIRzHTTwcYC8vC7bmeUEPGrKlR/TkQ==} @@ -1388,6 +1412,9 @@ packages: cjs-module-lexer@1.4.3: resolution: {integrity: sha512-9z8TZaGM1pfswYeXrUpzPrkx8UnWYdhJclsiYMm6x/w5+nN+8Tf/LnAgfLGQCm59qAOxU8WwHEq2vNwF6i4j+Q==} + code-block-writer@13.0.3: + resolution: {integrity: sha512-Oofo0pq3IKnsFtuHqSF7TqBfr71aeyZDVJ0HpmqB7FBM2qEigL0iPONSCZSO9pE9dZTAxANe5XHG9Uy0YMv8cg==} + commander@4.1.1: resolution: {integrity: sha512-NOKm8xhkzAjzFx8B2v5OAHT+u5pRQc2UCa2Vq9jYL/31o2wi9mxBA7LIFs3sV5VSC49z6pEhfbMULvShKj26WA==} engines: {node: '>= 6'} @@ -1829,6 +1856,13 @@ packages: engines: {node: '>=18.0.0'} hasBin: true + minimatch@10.2.5: + resolution: {integrity: sha512-MULkVLfKGYDFYejP07QOurDLLQpcjk7Fw+7jXS2R2czRQzR56yHRveU5NDJEOviH+hETZKSkIk5c+T23GjFUMg==} + engines: {node: 18 || 20 || >=22} + + minimist@1.2.8: + resolution: {integrity: sha512-2yyAR8qBkN3YuheJanUpWC5U3bb5osDywNB8RzDVlDwDHbocAJveqqj1u8+SVD7jkWT4yvsHCpWqqWqAxb0zCA==} + mlly@1.8.2: resolution: {integrity: sha512-d+ObxMQFmbt10sretNDytwt85VrbkhhUA/JBGm1MPaWJ65Cl4wOgLaB1NYvJSZ0Ef03MMEU/0xpPMXUIQ29UfA==} @@ -1871,10 +1905,16 @@ packages: parse5@7.3.0: resolution: {integrity: sha512-IInvU7fabl34qmi9gY8XOVxhYyMyuH2xUNpb2q8/Y+7552KlejkRvqvD19nMoUW/uQGGbqNpA6Tufu5FL5BZgw==} + parsecurrency@1.1.1: + resolution: {integrity: sha512-IAw/8PSFgiko70KfZGv63rbEXhmVu+zpb42PvEtgHAm83Mze3eQJHWV1ZoOhPnrYeOyufvv0GS6hZDuQOdBH4Q==} + parseurl@1.3.3: resolution: {integrity: sha512-CiyeOxFT/JZyN5m0z9PfXw4SCBJ6Sygz1Dpl0wqjlhDEGGBP1GnsUVEL0p63hoG1fcj3fHynXi9NYO4nWOL+qQ==} engines: {node: '>= 0.8'} + path-browserify@1.0.1: + resolution: {integrity: sha512-b7uo2UCUOYZcnF/3ID0lulOJi/bafxa1xPe7ZPsammBSpjSWQkjNxlt635YGS2MiR9GjvuXCtz2emr3jbsz98g==} + path-key@3.1.1: resolution: {integrity: sha512-ojmeN0qd+y0jszEtoY48r0Peq5dwMEkIlCOu6Q5f41lfkswXuKtYrhgoTpLnyIcHm24Uhqx+5Tqm2InSwLhE6Q==} engines: {node: '>=8'} @@ -2146,6 +2186,9 @@ packages: ts-interface-checker@0.1.13: resolution: {integrity: sha512-Y/arvbn+rrz3JCKl9C4kVNfTfSm2/mEp5FSz5EsZSANGPSlQrpRI5M4PKF+mJnE52jOO90PnPSc3Ur3bTQw0gA==} + ts-morph@27.0.2: + resolution: {integrity: sha512-fhUhgeljcrdZ+9DZND1De1029PrE+cMkIP7ooqkLRTrRLTqcki2AstsyJm0vRNbTbVCNJ0idGlbBrfqc7/nA8w==} + tslib@2.8.1: resolution: {integrity: sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w==} @@ -2421,6 +2464,14 @@ packages: snapshots: + '@amiceli/vitest-cucumber@6.3.0(vitest@4.1.1(@types/node@25.5.0)(happy-dom@20.8.9)(vite@8.0.1(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0)))': + dependencies: + callsites: 4.2.0 + minimist: 1.2.8 + parsecurrency: 1.1.1 + ts-morph: 27.0.2 + vitest: 4.1.1(@types/node@25.5.0)(happy-dom@20.8.9)(vite@8.0.1(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0)) + '@babel/code-frame@7.29.0': dependencies: '@babel/helper-validator-identifier': 7.28.5 @@ -3284,6 +3335,12 @@ snapshots: dependencies: '@testing-library/dom': 10.4.1 + '@ts-morph/common@0.28.1': + dependencies: + minimatch: 10.2.5 + path-browserify: 1.0.1 + tinyglobby: 0.2.15 + '@tybys/wasm-util@0.10.1': dependencies: tslib: 2.8.1 @@ -3459,6 +3516,8 @@ snapshots: optionalDependencies: solid-js: 1.9.11 + balanced-match@4.0.4: {} + baseline-browser-mapping@2.10.9: {} before-after-hook@4.0.0: {} @@ -3481,6 +3540,10 @@ snapshots: bottleneck@2.19.5: {} + brace-expansion@5.0.5: + dependencies: + balanced-match: 4.0.4 + browserslist@4.28.1: dependencies: baseline-browser-mapping: 2.10.9 @@ -3508,6 +3571,8 @@ snapshots: call-bind-apply-helpers: 1.0.2 get-intrinsic: 1.3.0 + callsites@4.2.0: {} + caniuse-lite@1.0.30001780: {} chai@6.2.2: {} @@ -3518,6 +3583,8 @@ snapshots: cjs-module-lexer@1.4.3: {} + code-block-writer@13.0.3: {} + commander@4.1.1: {} confbox@0.1.8: {} @@ -3911,6 +3978,12 @@ snapshots: - bufferutil - utf-8-validate + minimatch@10.2.5: + dependencies: + brace-expansion: 5.0.5 + + minimist@1.2.8: {} + mlly@1.8.2: dependencies: acorn: 8.16.0 @@ -3950,8 +4023,12 @@ snapshots: dependencies: entities: 6.0.1 + parsecurrency@1.1.1: {} + parseurl@1.3.3: {} + path-browserify@1.0.1: {} + path-key@3.1.1: {} path-to-regexp@6.3.0: {} @@ -4278,6 +4355,11 @@ snapshots: ts-interface-checker@0.1.13: {} + ts-morph@27.0.2: + dependencies: + '@ts-morph/common': 0.28.1 + code-block-writer: 13.0.3 + tslib@2.8.1: {} tsup@8.5.1(jiti@2.6.1)(postcss@8.5.8)(tsx@4.21.0)(typescript@5.9.3): diff --git a/tests/acceptance/org-order-stability.feature b/tests/acceptance/org-order-stability.feature new file mode 100644 index 00000000..2a486d73 --- /dev/null +++ b/tests/acceptance/org-order-stability.feature @@ -0,0 +1,52 @@ +Feature: Freeze org display order in RepoSelector after initial sort + + The RepoSelector component sorts organizations (personal first, then + alphabetical) when all orgs finish loading. After the initial sort, the + order is frozen to prevent visual re-ordering on reactive updates like + repo retries or checkbox toggles. The frozen order is invalidated when + the set of selected organizations changes (e.g., granting access to a + new org or revoking access), triggering a fresh sort. Invalidation uses + a serialized sorted Set comparison, not length, to detect membership + changes even when the org count stays the same. + + Background: + Given the user is authenticated with a GitHub account + + Scenario: S1 - Org order remains stable after repo retry + Given the RepoSelector displays 3 orgs sorted as "alice", "acme-corp", "beta-org" with beta-org showing a Retry button + When the user clicks the Retry button on "beta-org" and the repos load successfully + Then the org header order remains "alice", "acme-corp", "beta-org" + + Scenario: S2 - Org order remains stable when toggling a repo checkbox + Given the RepoSelector displays 3 orgs sorted as "alice", "acme-corp", "beta-org" with all repos loaded + When the user toggles a repo checkbox under "acme-corp" + Then the org header order remains "alice", "acme-corp", "beta-org" + + Scenario: S3 - Frozen order invalidated when a new org is granted + Given the RepoSelector displays 2 orgs sorted as "alice", "delta-inc" with order frozen + When the user grants access to a new org "acme-corp" and it finishes loading + Then the org header order becomes "alice", "acme-corp", "delta-inc" + + Scenario: S4 - Initial sort applies personal org first + Given the RepoSelector is displayed with 4 orgs "charlie", "acme-corp", "beta-org", "delta-inc" where "charlie" is the personal org + When all orgs finish loading + Then the org header order is "charlie", "acme-corp", "beta-org", "delta-inc" + + # S5 DEFERRED until accordion layout merges from worktree-ux-improvements. + # Without accordion, S5 duplicates S1's code path with more orgs. + # Scenario: S5 - Org order stable in accordion layout after retry + # Given the RepoSelector displays 7 orgs in accordion layout with "alice" first and "echo-labs" showing a Retry button + # When the user clicks Retry on "echo-labs" and its repos load successfully + # Then the org header order remains "alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io" + # And the currently expanded accordion panel remains expanded + + Scenario: S6 - New org appears in correct sorted position with 6+ orgs + Given the RepoSelector displays 6 orgs all loaded and sorted with "alice" as the personal org + When the user grants access to a new org "beta-org" and it finishes loading + Then the org header order becomes "alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io" + + Scenario: S7 - Frozen order invalidated on simultaneous add and remove + Given the RepoSelector displays 3 orgs sorted as "alice", "acme-corp", "delta-inc" with order frozen + When the user's org access changes so that "delta-inc" is removed and "beta-org" is added and beta-org finishes loading + Then the org header order becomes "alice", "acme-corp", "beta-org" + And "delta-inc" no longer appears in the list diff --git a/tests/acceptance/steps/org-order-stability.steps.tsx b/tests/acceptance/steps/org-order-stability.steps.tsx new file mode 100644 index 00000000..c9ae5dd3 --- /dev/null +++ b/tests/acceptance/steps/org-order-stability.steps.tsx @@ -0,0 +1,428 @@ +import { vi, expect } from "vitest"; +import type { RepoEntry } from "../../../src/app/services/api"; + +// Mock getClient before importing component +const mockRequest = vi.fn().mockResolvedValue({ data: {} }); +vi.mock("../../../src/app/services/github", () => ({ + getClient: () => ({ request: mockRequest }), +})); + +vi.mock("../../../src/app/stores/auth", () => ({ + user: () => ({ login: "alice", name: "Alice", avatar_url: "" }), + token: () => "fake-token", + onAuthCleared: vi.fn(), +})); + +vi.mock("../../../src/app/services/api", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + fetchOrgs: vi.fn().mockResolvedValue([]), + fetchRepos: vi.fn(), + discoverUpstreamRepos: vi.fn().mockResolvedValue([]), + }; +}); + +import * as api from "../../../src/app/services/api"; +import RepoSelector from "../../../src/app/components/onboarding/RepoSelector"; + +import { loadFeature, describeFeature } from "@amiceli/vitest-cucumber"; +import { render, screen, waitFor, fireEvent } from "@solidjs/testing-library"; + +const feature = await loadFeature("../org-order-stability.feature"); + +// ── Org entry fixtures ──────────────────────────────────────────────────────── +const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; +const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; +const betaEntry = { login: "beta-org", avatarUrl: "", type: "org" as const }; +const deltaEntry = { login: "delta-inc", avatarUrl: "", type: "org" as const }; + + +// ── Helper: create one repo per org ────────────────────────────────────────── +function makeOrgRepos(org: string): RepoEntry[] { + return [ + { + owner: org, + name: `${org}-repo`, + fullName: `${org}/${org}-repo`, + pushedAt: "2026-03-20T10:00:00Z", + }, + ]; +} + +// ── Helper: flat (non-accordion) org header order ──────────────────────────── +function getOrgHeaderOrder(orgNames: string[]): string[] { + const pattern = new RegExp(`^(${orgNames.join("|")})$`); + return screen.getAllByText(pattern).map((el) => el.textContent!); +} + +// ── Helper: accordion (6+ orgs) org header order ───────────────────────────── +function getAccordionOrder(orgNames: string[]): string[] { + return orgNames + .map((name) => ({ name, btn: screen.getByRole("button", { name: new RegExp(name) }) })) + .sort((a, b) => { + const pos = a.btn.compareDocumentPosition(b.btn); + return pos & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1; + }) + .map(({ name }) => name); +} + +// ── State shared across steps within a scenario ─────────────────────────────── +type OrgEntry = { login: string; avatarUrl: string; type: "user" | "org" }; +let setSelectedOrgs: (orgs: string[]) => void; +let setOrgEntries: (entries: OrgEntry[]) => void; + +describeFeature(feature, ({ Scenario, Background, BeforeEachScenario }) => { + BeforeEachScenario(() => { + vi.clearAllMocks(); + vi.restoreAllMocks(); + mockRequest.mockResolvedValue({ data: {} }); + vi.mocked(api.fetchOrgs).mockResolvedValue([]); + vi.mocked(api.discoverUpstreamRepos).mockResolvedValue([]); + setSelectedOrgs = () => {}; + setOrgEntries = () => {}; + }); + + // Background is handled by module-level vi.mock for auth store. + Background(({ Given }) => { + Given("the user is authenticated with a GitHub account", () => { + // Auth mock is set at module level — nothing to do here. + }); + }); + + // ── S1: Org order remains stable after repo retry ───────────────────────── + Scenario("S1 - Org order remains stable after repo retry", ({ Given, When, Then }) => { + Given( + 'the RepoSelector displays 3 orgs sorted as "alice", "acme-corp", "beta-org" with beta-org showing a Retry button', + async () => { + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => { + if (org === "beta-org") return Promise.reject(new Error("beta load failed")); + return Promise.resolve(makeOrgRepos(org as string)); + }); + + render(() => ( + + )); + + await waitFor(() => { + screen.getByText("alice-repo"); + screen.getByText("acme-corp-repo"); + screen.getByText("Retry"); + }); + } + ); + + When( + 'the user clicks the Retry button on "beta-org" and the repos load successfully', + async () => { + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + fireEvent.click(screen.getByText("Retry")); + + await waitFor(() => { + screen.getByText("beta-org-repo"); + }); + } + ); + + Then('the org header order remains "alice", "acme-corp", "beta-org"', () => { + const order = getOrgHeaderOrder(["alice", "acme-corp", "beta-org"]); + expect(order).toEqual(["alice", "acme-corp", "beta-org"]); + }); + }); + + // ── S2: Org order remains stable when toggling a repo checkbox ──────────── + Scenario( + "S2 - Org order remains stable when toggling a repo checkbox", + ({ Given, When, Then }) => { + Given( + 'the RepoSelector displays 3 orgs sorted as "alice", "acme-corp", "beta-org" with all repos loaded', + async () => { + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + render(() => ( + + )); + + await waitFor(() => { + screen.getByText("alice-repo"); + screen.getByText("acme-corp-repo"); + screen.getByText("beta-org-repo"); + }); + } + ); + + When('the user toggles a repo checkbox under "acme-corp"', () => { + const acmeCheckbox = screen.getAllByRole("checkbox").find((cb) => { + const label = cb.closest("label"); + return label?.textContent?.includes("acme-corp-repo"); + }); + expect(acmeCheckbox).not.toBeUndefined(); + fireEvent.click(acmeCheckbox!); + }); + + Then('the org header order remains "alice", "acme-corp", "beta-org"', () => { + const order = getOrgHeaderOrder(["alice", "acme-corp", "beta-org"]); + expect(order).toEqual(["alice", "acme-corp", "beta-org"]); + }); + } + ); + + // ── S3: Frozen order invalidated when a new org is granted ──────────────── + Scenario( + "S3 - Frozen order invalidated when a new org is granted", + ({ Given, When, Then }) => { + Given( + 'the RepoSelector displays 2 orgs sorted as "alice", "delta-inc" with order frozen', + async () => { + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + const { createSignal } = await import("solid-js"); + const [orgs, setOrgs] = createSignal(["alice", "delta-inc"]); + const [entries, setEntries] = createSignal([aliceEntry, deltaEntry]); + + setSelectedOrgs = setOrgs; + setOrgEntries = setEntries; + + render(() => ( + + )); + + await waitFor(() => { + screen.getByText("alice-repo"); + screen.getByText("delta-inc-repo"); + }); + } + ); + + When( + 'the user grants access to a new org "acme-corp" and it finishes loading', + async () => { + setSelectedOrgs(["alice", "delta-inc", "acme-corp"]); + setOrgEntries([aliceEntry, deltaEntry, acmeEntry]); + + await waitFor(() => { + screen.getByText("acme-corp-repo"); + }); + } + ); + + Then('the org header order becomes "alice", "acme-corp", "delta-inc"', () => { + const order = getOrgHeaderOrder(["alice", "acme-corp", "delta-inc"]); + expect(order).toEqual(["alice", "acme-corp", "delta-inc"]); + }); + } + ); + + // ── S4: Initial sort applies personal org first ─────────────────────────── + Scenario("S4 - Initial sort applies personal org first", ({ Given, When, Then }) => { + Given( + 'the RepoSelector is displayed with 4 orgs "charlie", "acme-corp", "beta-org", "delta-inc" where "charlie" is the personal org', + async () => { + // Override the auth mock so charlie is the authenticated user + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + // charlie is type "user" (personal org), the rest are "org" + const charlieAsPersonal = { login: "charlie", avatarUrl: "", type: "user" as const }; + const acme = { login: "acme-corp", avatarUrl: "", type: "org" as const }; + const beta = { login: "beta-org", avatarUrl: "", type: "org" as const }; + const delta = { login: "delta-inc", avatarUrl: "", type: "org" as const }; + + render(() => ( + + )); + } + ); + + When("all orgs finish loading", async () => { + await waitFor(() => { + screen.getByText("charlie-repo"); + screen.getByText("acme-corp-repo"); + screen.getByText("beta-org-repo"); + screen.getByText("delta-inc-repo"); + }); + }); + + Then( + 'the org header order is "charlie", "acme-corp", "beta-org", "delta-inc"', + () => { + const order = getOrgHeaderOrder(["charlie", "acme-corp", "beta-org", "delta-inc"]); + expect(order).toEqual(["charlie", "acme-corp", "beta-org", "delta-inc"]); + } + ); + }); + + // ── S6: New org appears in correct sorted position with 6+ orgs ────────── + Scenario( + "S6 - New org appears in correct sorted position with 6+ orgs", + ({ Given, When, Then }) => { + const startOrgs = ["alice", "acme-corp", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]; + + Given( + 'the RepoSelector displays 6 orgs all loaded and sorted with "alice" as the personal org', + async () => { + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + const startEntries = startOrgs.map((login) => ({ + login, + avatarUrl: "", + type: login === "alice" ? ("user" as const) : ("org" as const), + })); + + const { createSignal } = await import("solid-js"); + const [orgs, setOrgs] = createSignal(startOrgs); + const [entries, setEntries] = createSignal(startEntries); + + setSelectedOrgs = setOrgs; + setOrgEntries = setEntries; + + render(() => ( + + )); + + // Wait for accordion mode to render + await waitFor(() => { + screen.getByRole("button", { name: /alice/ }); + }); + } + ); + + When( + 'the user grants access to a new org "beta-org" and it finishes loading', + async () => { + const newOrgs = [ + "alice", + "acme-corp", + "beta-org", + "charlie-co", + "delta-inc", + "echo-labs", + "foxtrot-io", + ]; + const newEntries = newOrgs.map((login) => ({ + login, + avatarUrl: "", + type: login === "alice" ? ("user" as const) : ("org" as const), + })); + + setSelectedOrgs(newOrgs); + setOrgEntries(newEntries); + + await waitFor(() => { + screen.getByRole("button", { name: /beta-org/ }); + }); + } + ); + + Then( + 'the org header order becomes "alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"', + () => { + const allOrgs = [ + "alice", + "acme-corp", + "beta-org", + "charlie-co", + "delta-inc", + "echo-labs", + "foxtrot-io", + ]; + const order = getAccordionOrder(allOrgs); + expect(order).toEqual(allOrgs); + } + ); + } + ); + + // ── S7: Frozen order invalidated on simultaneous add and remove ─────────── + Scenario( + "S7 - Frozen order invalidated on simultaneous add and remove", + ({ Given, When, Then, And }) => { + Given( + 'the RepoSelector displays 3 orgs sorted as "alice", "acme-corp", "delta-inc" with order frozen', + async () => { + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + const { createSignal } = await import("solid-js"); + const [orgs, setOrgs] = createSignal(["alice", "acme-corp", "delta-inc"]); + const [entries, setEntries] = createSignal([aliceEntry, acmeEntry, deltaEntry]); + + setSelectedOrgs = setOrgs; + setOrgEntries = setEntries; + + render(() => ( + + )); + + await waitFor(() => { + screen.getByText("alice-repo"); + screen.getByText("acme-corp-repo"); + screen.getByText("delta-inc-repo"); + }); + } + ); + + When( + 'the user\'s org access changes so that "delta-inc" is removed and "beta-org" is added and beta-org finishes loading', + async () => { + setSelectedOrgs(["alice", "acme-corp", "beta-org"]); + setOrgEntries([aliceEntry, acmeEntry, betaEntry]); + + await waitFor(() => { + screen.getByText("beta-org-repo"); + }); + } + ); + + Then('the org header order becomes "alice", "acme-corp", "beta-org"', () => { + const order = getOrgHeaderOrder(["alice", "acme-corp", "beta-org"]); + expect(order).toEqual(["alice", "acme-corp", "beta-org"]); + }); + + And('"delta-inc" no longer appears in the list', () => { + expect(screen.queryByText("delta-inc")).toBeNull(); + }); + } + ); +}); From fc07c28ca114e42f6b505b4e7b738764abbdc72f Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 10:03:05 -0400 Subject: [PATCH 05/12] fix(test): includes BDD steps in vitest config, fixes cleanup --- .../steps/org-order-stability.steps.tsx | 16 ++++++++++++++-- vitest.workspace.ts | 2 +- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/tests/acceptance/steps/org-order-stability.steps.tsx b/tests/acceptance/steps/org-order-stability.steps.tsx index c9ae5dd3..72a374e6 100644 --- a/tests/acceptance/steps/org-order-stability.steps.tsx +++ b/tests/acceptance/steps/org-order-stability.steps.tsx @@ -1,4 +1,12 @@ import { vi, expect } from "vitest"; + +// Prevent @solidjs/testing-library from auto-cleaning the DOM between steps. +// vitest-cucumber maps each Given/When/Then to a separate test(), so the DOM +// must persist across steps within a scenario. Cleanup is done manually in +// AfterEachScenario instead. +vi.hoisted(() => { + process.env.STL_SKIP_AUTO_CLEANUP = "true"; +}); import type { RepoEntry } from "../../../src/app/services/api"; // Mock getClient before importing component @@ -27,7 +35,7 @@ import * as api from "../../../src/app/services/api"; import RepoSelector from "../../../src/app/components/onboarding/RepoSelector"; import { loadFeature, describeFeature } from "@amiceli/vitest-cucumber"; -import { render, screen, waitFor, fireEvent } from "@solidjs/testing-library"; +import { render, screen, waitFor, fireEvent, cleanup } from "@solidjs/testing-library"; const feature = await loadFeature("../org-order-stability.feature"); @@ -72,7 +80,7 @@ type OrgEntry = { login: string; avatarUrl: string; type: "user" | "org" }; let setSelectedOrgs: (orgs: string[]) => void; let setOrgEntries: (entries: OrgEntry[]) => void; -describeFeature(feature, ({ Scenario, Background, BeforeEachScenario }) => { +describeFeature(feature, ({ Scenario, Background, BeforeEachScenario, AfterEachScenario }) => { BeforeEachScenario(() => { vi.clearAllMocks(); vi.restoreAllMocks(); @@ -83,6 +91,10 @@ describeFeature(feature, ({ Scenario, Background, BeforeEachScenario }) => { setOrgEntries = () => {}; }); + AfterEachScenario(() => { + cleanup(); + }); + // Background is handled by module-level vi.mock for auth store. Background(({ Given }) => { Given("the user is authenticated with a GitHub account", () => { diff --git a/vitest.workspace.ts b/vitest.workspace.ts index 88901674..595997ba 100644 --- a/vitest.workspace.ts +++ b/vitest.workspace.ts @@ -17,7 +17,7 @@ export default defineConfig({ globals: true, hookTimeout: 30_000, setupFiles: ["tests/setup.ts"], - include: ["tests/**/*.test.ts", "tests/**/*.test.tsx"], + include: ["tests/**/*.test.ts", "tests/**/*.test.tsx", "tests/**/*.steps.tsx"], exclude: ["tests/worker/**"], }, }), From 83962137601f7ffa2a33a80a775525c7558020ff Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 10:08:17 -0400 Subject: [PATCH 06/12] refactor(test): removes misleading comment and redundant vars in S4 --- tests/acceptance/steps/org-order-stability.steps.tsx | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/tests/acceptance/steps/org-order-stability.steps.tsx b/tests/acceptance/steps/org-order-stability.steps.tsx index 72a374e6..3b3cad33 100644 --- a/tests/acceptance/steps/org-order-stability.steps.tsx +++ b/tests/acceptance/steps/org-order-stability.steps.tsx @@ -45,7 +45,6 @@ const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; const betaEntry = { login: "beta-org", avatarUrl: "", type: "org" as const }; const deltaEntry = { login: "delta-inc", avatarUrl: "", type: "org" as const }; - // ── Helper: create one repo per org ────────────────────────────────────────── function makeOrgRepos(org: string): RepoEntry[] { return [ @@ -252,21 +251,17 @@ describeFeature(feature, ({ Scenario, Background, BeforeEachScenario, AfterEachS Given( 'the RepoSelector is displayed with 4 orgs "charlie", "acme-corp", "beta-org", "delta-inc" where "charlie" is the personal org', async () => { - // Override the auth mock so charlie is the authenticated user vi.mocked(api.fetchRepos).mockImplementation((_client, org) => Promise.resolve(makeOrgRepos(org as string)) ); - // charlie is type "user" (personal org), the rest are "org" - const charlieAsPersonal = { login: "charlie", avatarUrl: "", type: "user" as const }; - const acme = { login: "acme-corp", avatarUrl: "", type: "org" as const }; - const beta = { login: "beta-org", avatarUrl: "", type: "org" as const }; - const delta = { login: "delta-inc", avatarUrl: "", type: "org" as const }; + // charlie has type "user" — sort puts it first (personal org) + const charlieEntry = { login: "charlie", avatarUrl: "", type: "user" as const }; render(() => ( From 93b56421a46fbbeb9a06dd07f99d4ef95343dcff Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 10:10:27 -0400 Subject: [PATCH 07/12] chore: pins vitest-cucumber to exact version --- package.json | 2 +- pnpm-lock.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 949fd5af..178a0bed 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,7 @@ "zod": "4.3.6" }, "devDependencies": { - "@amiceli/vitest-cucumber": "^6.3.0", + "@amiceli/vitest-cucumber": "6.3.0", "@cloudflare/vite-plugin": "1.30.1", "@cloudflare/vitest-pool-workers": "0.13.4", "@playwright/test": "1.58.2", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4ab70dd1..b2ff44e8 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -40,7 +40,7 @@ importers: version: 4.3.6 devDependencies: '@amiceli/vitest-cucumber': - specifier: ^6.3.0 + specifier: 6.3.0 version: 6.3.0(vitest@4.1.1(@types/node@25.5.0)(happy-dom@20.8.9)(vite@8.0.1(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0))) '@cloudflare/vite-plugin': specifier: 1.30.1 From 5795041692fee850e37292109cea45ebdd62607c Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 10:12:45 -0400 Subject: [PATCH 08/12] refactor(test): cleans up BDD step definitions per review --- .../steps/org-order-stability.steps.tsx | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/acceptance/steps/org-order-stability.steps.tsx b/tests/acceptance/steps/org-order-stability.steps.tsx index 3b3cad33..d2111471 100644 --- a/tests/acceptance/steps/org-order-stability.steps.tsx +++ b/tests/acceptance/steps/org-order-stability.steps.tsx @@ -1,13 +1,13 @@ import { vi, expect } from "vitest"; -// Prevent @solidjs/testing-library from auto-cleaning the DOM between steps. -// vitest-cucumber maps each Given/When/Then to a separate test(), so the DOM -// must persist across steps within a scenario. Cleanup is done manually in -// AfterEachScenario instead. +// vitest-cucumber maps each Given/When/Then to a separate test(). The DOM must +// persist across steps within a scenario, but @solidjs/testing-library registers +// afterEach(cleanup) at import time — vi.hoisted ensures the env var is set +// BEFORE that import evaluates. Manual cleanup in AfterEachScenario replaces it. vi.hoisted(() => { process.env.STL_SKIP_AUTO_CLEANUP = "true"; }); -import type { RepoEntry } from "../../../src/app/services/api"; +import type { RepoEntry, OrgEntry } from "../../../src/app/services/api"; // Mock getClient before importing component const mockRequest = vi.fn().mockResolvedValue({ data: {} }); @@ -75,14 +75,12 @@ function getAccordionOrder(orgNames: string[]): string[] { } // ── State shared across steps within a scenario ─────────────────────────────── -type OrgEntry = { login: string; avatarUrl: string; type: "user" | "org" }; -let setSelectedOrgs: (orgs: string[]) => void; -let setOrgEntries: (entries: OrgEntry[]) => void; +let setSelectedOrgs: (orgs: string[]) => void = () => {}; +let setOrgEntries: (entries: OrgEntry[]) => void = () => {}; describeFeature(feature, ({ Scenario, Background, BeforeEachScenario, AfterEachScenario }) => { BeforeEachScenario(() => { vi.clearAllMocks(); - vi.restoreAllMocks(); mockRequest.mockResolvedValue({ data: {} }); vi.mocked(api.fetchOrgs).mockResolvedValue([]); vi.mocked(api.discoverUpstreamRepos).mockResolvedValue([]); From 036a74ae835a130e9c13569935db280e3da82233 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 10:18:33 -0400 Subject: [PATCH 09/12] test(onboarding): adds S5 BDD scenario, fixes review gaps --- tests/acceptance/org-order-stability.feature | 12 ++- .../steps/org-order-stability.steps.tsx | 86 ++++++++++++++++++- 2 files changed, 89 insertions(+), 9 deletions(-) diff --git a/tests/acceptance/org-order-stability.feature b/tests/acceptance/org-order-stability.feature index 2a486d73..e9898af5 100644 --- a/tests/acceptance/org-order-stability.feature +++ b/tests/acceptance/org-order-stability.feature @@ -32,13 +32,11 @@ Feature: Freeze org display order in RepoSelector after initial sort When all orgs finish loading Then the org header order is "charlie", "acme-corp", "beta-org", "delta-inc" - # S5 DEFERRED until accordion layout merges from worktree-ux-improvements. - # Without accordion, S5 duplicates S1's code path with more orgs. - # Scenario: S5 - Org order stable in accordion layout after retry - # Given the RepoSelector displays 7 orgs in accordion layout with "alice" first and "echo-labs" showing a Retry button - # When the user clicks Retry on "echo-labs" and its repos load successfully - # Then the org header order remains "alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io" - # And the currently expanded accordion panel remains expanded + Scenario: S5 - Org order stable in accordion layout after retry + Given the RepoSelector displays 7 orgs in accordion layout with "alice" first and "echo-labs" showing a Retry button + When the user clicks Retry on "echo-labs" and its repos load successfully + Then the org header order remains "alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io" + And the currently expanded accordion panel remains expanded Scenario: S6 - New org appears in correct sorted position with 6+ orgs Given the RepoSelector displays 6 orgs all loaded and sorted with "alice" as the personal org diff --git a/tests/acceptance/steps/org-order-stability.steps.tsx b/tests/acceptance/steps/org-order-stability.steps.tsx index d2111471..79cecdb4 100644 --- a/tests/acceptance/steps/org-order-stability.steps.tsx +++ b/tests/acceptance/steps/org-order-stability.steps.tsx @@ -1,4 +1,4 @@ -import { vi, expect } from "vitest"; +import { vi, expect, afterAll } from "vitest"; // vitest-cucumber maps each Given/When/Then to a separate test(). The DOM must // persist across steps within a scenario, but @solidjs/testing-library registers @@ -39,6 +39,13 @@ import { render, screen, waitFor, fireEvent, cleanup } from "@solidjs/testing-li const feature = await loadFeature("../org-order-stability.feature"); +// STL_SKIP_AUTO_CLEANUP is file-scoped (Vitest isolates each file in its own +// module context), but clean it up explicitly so the env doesn't leak if +// Vitest's isolation model changes in future versions. +afterAll(() => { + delete process.env.STL_SKIP_AUTO_CLEANUP; +}); + // ── Org entry fixtures ──────────────────────────────────────────────────────── const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; @@ -58,8 +65,10 @@ function makeOrgRepos(org: string): RepoEntry[] { } // ── Helper: flat (non-accordion) org header order ──────────────────────────── +// Org names follow GitHub's [A-Za-z0-9-] pattern — no regex escaping needed. function getOrgHeaderOrder(orgNames: string[]): string[] { - const pattern = new RegExp(`^(${orgNames.join("|")})$`); + const escaped = orgNames.map((n) => n.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")); + const pattern = new RegExp(`^(${escaped.join("|")})$`); return screen.getAllByText(pattern).map((el) => el.textContent!); } @@ -285,6 +294,79 @@ describeFeature(feature, ({ Scenario, Background, BeforeEachScenario, AfterEachS ); }); + // ── S5: Org order stable in accordion layout after retry ──────────────── + Scenario( + "S5 - Org order stable in accordion layout after retry", + ({ Given, When, Then, And }) => { + const sevenOrgs = ["alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]; + + Given( + 'the RepoSelector displays 7 orgs in accordion layout with "alice" first and "echo-labs" showing a Retry button', + async () => { + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => { + if (org === "echo-labs") return Promise.reject(new Error("echo load failed")); + return Promise.resolve(makeOrgRepos(org as string)); + }); + + const sevenEntries = sevenOrgs.map((login) => ({ + login, + avatarUrl: "", + type: login === "alice" ? ("user" as const) : ("org" as const), + })); + + render(() => ( + + )); + + // Wait for accordion mode to render, expand echo-labs to show Retry + await waitFor(() => { + screen.getByRole("button", { name: /alice/ }); + }); + + const echoBtn = screen.getByRole("button", { name: /echo-labs/ }); + fireEvent.click(echoBtn); + + await waitFor(() => { + screen.getByText("Retry"); + }); + } + ); + + When( + 'the user clicks Retry on "echo-labs" and its repos load successfully', + async () => { + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => + Promise.resolve(makeOrgRepos(org as string)) + ); + + fireEvent.click(screen.getByText("Retry")); + + await waitFor(() => { + screen.getByText("echo-labs-repo"); + }); + } + ); + + Then( + 'the org header order remains "alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"', + () => { + const order = getAccordionOrder(sevenOrgs); + expect(order).toEqual(sevenOrgs); + } + ); + + And("the currently expanded accordion panel remains expanded", () => { + const echoBtn = screen.getByRole("button", { name: /echo-labs/ }); + expect(echoBtn.getAttribute("aria-expanded")).toBe("true"); + }); + } + ); + // ── S6: New org appears in correct sorted position with 6+ orgs ────────── Scenario( "S6 - New org appears in correct sorted position with 6+ orgs", From e63619566660b753b86e3687dd658925403d22fb Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 10:23:02 -0400 Subject: [PATCH 10/12] fix(test): strengthens S7 with discriminating org name --- tests/acceptance/org-order-stability.feature | 4 ++-- .../steps/org-order-stability.steps.tsx | 20 +++++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/tests/acceptance/org-order-stability.feature b/tests/acceptance/org-order-stability.feature index e9898af5..037f83cc 100644 --- a/tests/acceptance/org-order-stability.feature +++ b/tests/acceptance/org-order-stability.feature @@ -45,6 +45,6 @@ Feature: Freeze org display order in RepoSelector after initial sort Scenario: S7 - Frozen order invalidated on simultaneous add and remove Given the RepoSelector displays 3 orgs sorted as "alice", "acme-corp", "delta-inc" with order frozen - When the user's org access changes so that "delta-inc" is removed and "beta-org" is added and beta-org finishes loading - Then the org header order becomes "alice", "acme-corp", "beta-org" + When the user's org access changes so that "delta-inc" is removed and "aaa-org" is added and aaa-org finishes loading + Then the org header order becomes "alice", "aaa-org", "acme-corp" And "delta-inc" no longer appears in the list diff --git a/tests/acceptance/steps/org-order-stability.steps.tsx b/tests/acceptance/steps/org-order-stability.steps.tsx index 79cecdb4..b34dfbfa 100644 --- a/tests/acceptance/steps/org-order-stability.steps.tsx +++ b/tests/acceptance/steps/org-order-stability.steps.tsx @@ -51,6 +51,7 @@ const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; const betaEntry = { login: "beta-org", avatarUrl: "", type: "org" as const }; const deltaEntry = { login: "delta-inc", avatarUrl: "", type: "org" as const }; +const aaaEntry = { login: "aaa-org", avatarUrl: "", type: "org" as const }; // ── Helper: create one repo per org ────────────────────────────────────────── function makeOrgRepos(org: string): RepoEntry[] { @@ -75,7 +76,7 @@ function getOrgHeaderOrder(orgNames: string[]): string[] { // ── Helper: accordion (6+ orgs) org header order ───────────────────────────── function getAccordionOrder(orgNames: string[]): string[] { return orgNames - .map((name) => ({ name, btn: screen.getByRole("button", { name: new RegExp(name) }) })) + .map((name) => ({ name, btn: screen.getByRole("button", { name: new RegExp(name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")) }) })) .sort((a, b) => { const pos = a.btn.compareDocumentPosition(b.btn); return pos & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1; @@ -491,20 +492,23 @@ describeFeature(feature, ({ Scenario, Background, BeforeEachScenario, AfterEachS ); When( - 'the user\'s org access changes so that "delta-inc" is removed and "beta-org" is added and beta-org finishes loading', + 'the user\'s org access changes so that "delta-inc" is removed and "aaa-org" is added and aaa-org finishes loading', async () => { - setSelectedOrgs(["alice", "acme-corp", "beta-org"]); - setOrgEntries([aliceEntry, acmeEntry, betaEntry]); + // aaa-org sorts BEFORE acme-corp alphabetically. If the frozen order + // is NOT invalidated, the replay logic appends aaa-org at the end + // (alice, acme-corp, aaa-org) instead of re-sorting (alice, aaa-org, acme-corp). + setSelectedOrgs(["alice", "acme-corp", "aaa-org"]); + setOrgEntries([aliceEntry, acmeEntry, aaaEntry]); await waitFor(() => { - screen.getByText("beta-org-repo"); + screen.getByText("aaa-org-repo"); }); } ); - Then('the org header order becomes "alice", "acme-corp", "beta-org"', () => { - const order = getOrgHeaderOrder(["alice", "acme-corp", "beta-org"]); - expect(order).toEqual(["alice", "acme-corp", "beta-org"]); + Then('the org header order becomes "alice", "aaa-org", "acme-corp"', () => { + const order = getOrgHeaderOrder(["alice", "aaa-org", "acme-corp"]); + expect(order).toEqual(["alice", "aaa-org", "acme-corp"]); }); And('"delta-inc" no longer appears in the list', () => { From 916e09a351ede20f35eaae3d646b859317bf0d77 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 12:00:59 -0400 Subject: [PATCH 11/12] refactor(test): deduplicates helpers, adds staggered-load test Hoists getOrgHeaderOrder, getAccordionOrder, and 5 entry fixtures to describe scope. Adds regex metacharacter escaping to both helpers. Adds S8 test for invalidation-trickle-in guard with deferred promises. Documents .steps.tsx BDD convention in CONTRIBUTING.md. Trims verbose comment in RepoSelector.tsx. Fixes stale comment in BDD step file. --- CONTRIBUTING.md | 2 +- .../components/onboarding/RepoSelector.tsx | 3 +- .../steps/org-order-stability.steps.tsx | 2 +- .../onboarding/RepoSelector.test.tsx | 125 ++++++++++++------ 4 files changed, 86 insertions(+), 46 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 57ba87b9..e2cd53bc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -65,7 +65,7 @@ pnpm test -- tests/path/to/test.ts ## Testing -Tests live in `tests/` and mirror the `src/` directory structure. Test files end in `.test.ts` or `.test.tsx`. +Tests live in `tests/` and mirror the `src/` directory structure. Test files end in `.test.ts` or `.test.tsx`. BDD step definitions end in `.steps.tsx` and are also picked up by Vitest automatically. Factory helpers in `tests/helpers/index.tsx` (`makeIssue`, `makePullRequest`, `makeWorkflowRun`) give you typed test fixtures — use them instead of hand-rolling objects. diff --git a/src/app/components/onboarding/RepoSelector.tsx b/src/app/components/onboarding/RepoSelector.tsx index 0ce10cad..f65588ba 100644 --- a/src/app/components/onboarding/RepoSelector.tsx +++ b/src/app/components/onboarding/RepoSelector.tsx @@ -342,8 +342,7 @@ export default function RepoSelector(props: RepoSelectorProps) { new Set((props.monitoredRepos ?? []).map((r) => r.fullName)) ); - // Plain let variables, not signals — writing to a signal from inside createMemo is a - // side effect that triggers re-evaluation, causing an infinite loop. + // Plain let — not signals; mutating a signal inside createMemo causes infinite re-evaluation. let frozenOrder: string[] | null = null; let frozenOrgsKey = ""; diff --git a/tests/acceptance/steps/org-order-stability.steps.tsx b/tests/acceptance/steps/org-order-stability.steps.tsx index b34dfbfa..155308cd 100644 --- a/tests/acceptance/steps/org-order-stability.steps.tsx +++ b/tests/acceptance/steps/org-order-stability.steps.tsx @@ -66,7 +66,7 @@ function makeOrgRepos(org: string): RepoEntry[] { } // ── Helper: flat (non-accordion) org header order ──────────────────────────── -// Org names follow GitHub's [A-Za-z0-9-] pattern — no regex escaping needed. +// Org names follow GitHub's [A-Za-z0-9-] pattern — escaping is defensive. function getOrgHeaderOrder(orgNames: string[]): string[] { const escaped = orgNames.map((n) => n.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")); const pattern = new RegExp(`^(${escaped.join("|")})$`); diff --git a/tests/components/onboarding/RepoSelector.test.tsx b/tests/components/onboarding/RepoSelector.test.tsx index d08f7583..f9efd875 100644 --- a/tests/components/onboarding/RepoSelector.test.tsx +++ b/tests/components/onboarding/RepoSelector.test.tsx @@ -1503,23 +1503,37 @@ describe("RepoSelector — frozen org order", () => { } // Flat (non-accordion) mode only: org names appear as plain text nodes in headers. - // In accordion mode, org names are inside button triggers — use different query selectors (see S6). + // In accordion mode, org names are inside button triggers — use getAccordionOrder below. function getOrgHeaderOrder(orgNames: string[]): string[] { - const pattern = new RegExp(`^(${orgNames.join("|")})$`); + const escaped = orgNames.map((n) => n.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")); + const pattern = new RegExp(`^(${escaped.join("|")})$`); return screen.getAllByText(pattern).map((el) => el.textContent!); } + // Accordion mode: org names are inside button triggers — sort by DOM position. + function getAccordionOrder(orgNames: string[]): string[] { + return orgNames + .map((name) => ({ name, btn: screen.getByRole("button", { name: new RegExp(name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")) }) })) + .sort((a, b) => { + const pos = a.btn.compareDocumentPosition(b.btn); + return pos & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1; + }) + .map(({ name }) => name); + } + beforeEach(() => { vi.clearAllMocks(); vi.restoreAllMocks(); }); + const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; + const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; + const betaEntry = { login: "beta-org", avatarUrl: "", type: "org" as const }; + const deltaEntry = { login: "delta-inc", avatarUrl: "", type: "org" as const }; + const aaaEntry = { login: "aaa-org", avatarUrl: "", type: "org" as const }; + // S1: Org order remains stable after repo retry it("org order remains stable after repo retry for a failed org", async () => { - const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; - const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; - const betaEntry = { login: "beta-org", avatarUrl: "", type: "org" as const }; - vi.mocked(api.fetchRepos) .mockImplementation((_client, org) => { if (org === "beta-org") return Promise.reject(new Error("beta load failed")); @@ -1564,10 +1578,6 @@ describe("RepoSelector — frozen org order", () => { // S2: Org order remains stable when toggling a repo checkbox it("org order remains stable when toggling a repo checkbox", async () => { - const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; - const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; - const betaEntry = { login: "beta-org", avatarUrl: "", type: "org" as const }; - vi.mocked(api.fetchRepos).mockImplementation((_client, org) => Promise.resolve(makeOrgRepos(org as string)) ); @@ -1608,10 +1618,6 @@ describe("RepoSelector — frozen org order", () => { it("frozen order is invalidated and re-sorted when a new org is added", async () => { const { createSignal } = await import("solid-js"); - const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; - const deltaEntry = { login: "delta-inc", avatarUrl: "", type: "org" as const }; - const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; - vi.mocked(api.fetchRepos).mockImplementation((_client, org) => Promise.resolve(makeOrgRepos(org as string)) ); @@ -1683,20 +1689,10 @@ describe("RepoSelector — frozen org order", () => { }); // Verify initial sorted order via accordion trigger buttons - const getAccordionOrder = (orgNames: string[]) => - orgNames - .map((name) => ({ name, btn: screen.getByRole("button", { name: new RegExp(name) }) })) - .sort((a, b) => { - const pos = a.btn.compareDocumentPosition(b.btn); - return pos & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1; - }) - .map(({ name }) => name); - const initialOrder = getAccordionOrder(startOrgs); expect(initialOrder).toEqual(["alice", "acme-corp", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]); // Add beta-org (7 total) - const betaEntry = { login: "beta-org", avatarUrl: "", type: "org" as const }; const newOrgs = ["alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]; setSelectedOrgs(newOrgs); setOrgEntries([...startEntries, betaEntry]); @@ -1747,15 +1743,6 @@ describe("RepoSelector — frozen org order", () => { }); // Verify initial sorted order via accordion trigger buttons - const getAccordionOrder = (orgNames: string[]) => - orgNames - .map((name) => ({ name, btn: screen.getByRole("button", { name: new RegExp(name) }) })) - .sort((a, b) => { - const pos = a.btn.compareDocumentPosition(b.btn); - return pos & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1; - }) - .map(({ name }) => name); - const initialOrder = getAccordionOrder(sevenOrgs); expect(initialOrder).toEqual(["alice", "acme-corp", "beta-org", "charlie-co", "delta-inc", "echo-labs", "foxtrot-io"]); @@ -1786,11 +1773,6 @@ describe("RepoSelector — frozen org order", () => { it("frozen order is invalidated when orgs are simultaneously added and removed", async () => { const { createSignal } = await import("solid-js"); - const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; - const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; - const deltaEntry = { login: "delta-inc", avatarUrl: "", type: "org" as const }; - const aaaEntry = { login: "aaa-org", avatarUrl: "", type: "org" as const }; - vi.mocked(api.fetchRepos).mockImplementation((_client, org) => Promise.resolve(makeOrgRepos(org as string)) ); @@ -1839,10 +1821,6 @@ describe("RepoSelector — frozen org order", () => { it("removing an org invalidates frozen order and re-sorts correctly", async () => { const { createSignal } = await import("solid-js"); - const aliceEntry = { login: "alice", avatarUrl: "", type: "user" as const }; - const acmeEntry = { login: "acme-corp", avatarUrl: "", type: "org" as const }; - const deltaEntry = { login: "delta-inc", avatarUrl: "", type: "org" as const }; - vi.mocked(api.fetchRepos).mockImplementation((_client, org) => Promise.resolve(makeOrgRepos(org as string)) ); @@ -1884,4 +1862,67 @@ describe("RepoSelector — frozen org order", () => { // delta-inc repo content should also be gone from the display expect(screen.queryByText("delta-inc-repo")).toBeNull(); }); + + // S8: Adding 2+ orgs simultaneously with staggered loading produces correct final sort + // Verifies the invalidation→loadedCount guard interaction during trickle-in: + // after frozenOrder is nulled (key changed), sorting is deferred until all new orgs settle. + it("adding 2+ orgs simultaneously with staggered loading produces correct final sorted order", async () => { + const { createSignal } = await import("solid-js"); + + let resolveEcho!: (repos: RepoEntry[]) => void; + let resolveFoxtrot!: (repos: RepoEntry[]) => void; + const echoPending = new Promise((res) => { resolveEcho = res; }); + const foxtrotPending = new Promise((res) => { resolveFoxtrot = res; }); + + vi.mocked(api.fetchRepos).mockImplementation((_client, org) => { + if (org === "echo-labs") return echoPending; + if (org === "foxtrot-io") return foxtrotPending; + return Promise.resolve(makeOrgRepos(org as string)); + }); + + const [selectedOrgs, setSelectedOrgs] = createSignal(["alice", "acme-corp"]); + const [orgEntries, setOrgEntries] = createSignal([aliceEntry, acmeEntry]); + + render(() => ( + + )); + + // Wait for initial 2 orgs to load and freeze + await waitFor(() => { + screen.getByText("alice-repo"); + screen.getByText("acme-corp-repo"); + }); + + const initialOrder = getOrgHeaderOrder(["alice", "acme-corp"]); + expect(initialOrder).toEqual(["alice", "acme-corp"]); + + // Simultaneously add echo-labs and foxtrot-io (both slow) + const echoEntry = { login: "echo-labs", avatarUrl: "", type: "org" as const }; + const foxtrotEntry = { login: "foxtrot-io", avatarUrl: "", type: "org" as const }; + setSelectedOrgs(["alice", "acme-corp", "echo-labs", "foxtrot-io"]); + setOrgEntries([aliceEntry, acmeEntry, echoEntry, foxtrotEntry]); + + // Resolve echo-labs first — foxtrot-io is still loading. + // loadedCount (3 settled out of 4) < selectedOrgs.length (4), + // so the loadedCount guard should prevent a premature sort. + resolveEcho(makeOrgRepos("echo-labs")); + await waitFor(() => { + screen.getByText("echo-labs-repo"); + }); + + // Now resolve foxtrot-io — all 4 orgs settled, sort should fire and freeze. + resolveFoxtrot(makeOrgRepos("foxtrot-io")); + await waitFor(() => { + screen.getByText("foxtrot-io-repo"); + }); + + // Final order: alice (user first), then alphabetical: acme-corp, echo-labs, foxtrot-io + const finalOrder = getOrgHeaderOrder(["alice", "acme-corp", "echo-labs", "foxtrot-io"]); + expect(finalOrder).toEqual(["alice", "acme-corp", "echo-labs", "foxtrot-io"]); + }); }); From 41785ad20702b69af525a9769b3b0033acc9e590 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 8 Apr 2026 12:05:56 -0400 Subject: [PATCH 12/12] chore: regenerates lockfile after vite 8.0.5 rebase --- pnpm-lock.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b2ff44e8..37471151 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -41,7 +41,7 @@ importers: devDependencies: '@amiceli/vitest-cucumber': specifier: 6.3.0 - version: 6.3.0(vitest@4.1.1(@types/node@25.5.0)(happy-dom@20.8.9)(vite@8.0.1(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0))) + version: 6.3.0(vitest@4.1.1(@types/node@25.5.0)(happy-dom@20.8.9)(vite@8.0.5(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0))) '@cloudflare/vite-plugin': specifier: 1.30.1 version: 1.30.1(vite@8.0.5(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0))(workerd@1.20260317.1)(wrangler@4.77.0) @@ -2464,13 +2464,13 @@ packages: snapshots: - '@amiceli/vitest-cucumber@6.3.0(vitest@4.1.1(@types/node@25.5.0)(happy-dom@20.8.9)(vite@8.0.1(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0)))': + '@amiceli/vitest-cucumber@6.3.0(vitest@4.1.1(@types/node@25.5.0)(happy-dom@20.8.9)(vite@8.0.5(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0)))': dependencies: callsites: 4.2.0 minimist: 1.2.8 parsecurrency: 1.1.1 ts-morph: 27.0.2 - vitest: 4.1.1(@types/node@25.5.0)(happy-dom@20.8.9)(vite@8.0.1(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0)) + vitest: 4.1.1(@types/node@25.5.0)(happy-dom@20.8.9)(vite@8.0.5(@types/node@25.5.0)(esbuild@0.27.3)(jiti@2.6.1)(tsx@4.21.0)) '@babel/code-frame@7.29.0': dependencies: