From b2540cff538ef8bca76afa6fed6a3cbfa273ddc2 Mon Sep 17 00:00:00 2001 From: phall Date: Sun, 20 Sep 2026 01:40:22 -0400 Subject: [PATCH] chore: drop unused list-all APIs, probes, and leftover ghui names Remove listAllPullRequests/listAllIssues (app paginates), the one-off atom/filter probes, stale keymap COMPARISON/MIGRATION docs, and the upstream publish.yml that never ran on this fork. Rename the bun.lock workspace from ghui to @phall/phui. --- .github/workflows/publish.yml | 165 -------------- .gitignore | 3 + AGENTS.md | 7 +- bun.lock | 2 +- dev/probe-atom-pipeline.ts | 125 ----------- dev/probe-filter.ts | 55 ----- packages/keymap/COMPARISON.md | 345 ------------------------------ packages/keymap/MIGRATION.md | 174 --------------- plans/daily-driver.md | 2 +- src/services/GitHubService.ts | 36 +--- src/services/MockGitHubService.ts | 10 - test/packagePins.test.ts | 2 +- 12 files changed, 10 insertions(+), 916 deletions(-) delete mode 100644 .github/workflows/publish.yml delete mode 100644 dev/probe-atom-pipeline.ts delete mode 100644 dev/probe-filter.ts delete mode 100644 packages/keymap/COMPARISON.md delete mode 100644 packages/keymap/MIGRATION.md diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml deleted file mode 100644 index f8e1a4ce..00000000 --- a/.github/workflows/publish.yml +++ /dev/null @@ -1,165 +0,0 @@ -name: Publish to npm - -on: - release: - types: [published] - workflow_dispatch: - -permissions: - contents: write - id-token: write - -jobs: - verify: - if: github.repository == 'kitlangton/ghui' - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v6 - - - uses: actions/setup-node@v6 - with: - node-version: 24 - - - uses: oven-sh/setup-bun@v2 - with: - bun-version-file: .bun-version - - - run: bun install --frozen-lockfile - - - run: bun run typecheck - - - run: bun run package:smoke - - - name: Verify release tag matches package version - run: | - version="$(node -p "require('./package.json').version")" - test "v${version}" = "${GITHUB_REF_NAME}" - - standalone: - if: github.repository == 'kitlangton/ghui' - needs: verify - environment: npm - strategy: - fail-fast: false - matrix: - include: - - target: darwin-arm64 - runner: macos-15 - - target: darwin-x64 - runner: macos-15-intel - - target: linux-arm64 - runner: ubuntu-24.04-arm - - target: linux-x64 - runner: ubuntu-24.04 - runs-on: ${{ matrix.runner }} - steps: - - uses: actions/checkout@v6 - - - uses: actions/setup-node@v6 - with: - node-version: 24 - registry-url: https://registry.npmjs.org/ - - - uses: oven-sh/setup-bun@v2 - with: - bun-version-file: .bun-version - - - run: bun install --frozen-lockfile - - - name: Verify release tag matches package version - run: | - version="$(node -p "require('./package.json').version")" - test "v${version}" = "${GITHUB_REF_NAME}" - - - run: bun run build:standalone -- ${{ matrix.target }} - - - run: bun run build:npm-packages -- ${{ matrix.target }} - env: - GHUI_REUSE_RELEASE_BINARY: "1" - - - name: Publish npm binary package - run: | - package_dir="dist/npm/binaries/${{ matrix.target }}" - name="$(node -p "require('./${package_dir}/package.json').name")" - version="$(node -p "require('./${package_dir}/package.json').version")" - if npm view "${name}@${version}" version >/dev/null 2>&1; then - echo "${name}@${version} already published." - exit 0 - fi - npm publish "${package_dir}" - - - name: Upload standalone release assets - env: - GH_TOKEN: ${{ github.token }} - run: gh release upload "${GITHUB_REF_NAME}" "dist/release/ghui-${{ matrix.target }}.tar.gz" "dist/release/ghui-${{ matrix.target }}.tar.gz.sha256" --clobber - - publish: - if: github.repository == 'kitlangton/ghui' - runs-on: ubuntu-latest - needs: standalone - environment: npm - steps: - - uses: actions/checkout@v6 - - - uses: actions/setup-node@v6 - with: - node-version: 24 - registry-url: https://registry.npmjs.org/ - - - uses: oven-sh/setup-bun@v2 - with: - bun-version-file: .bun-version - - - run: bun install --frozen-lockfile - - - name: Verify release tag matches package version - run: | - version="$(node -p "require('./package.json').version")" - test "v${version}" = "${GITHUB_REF_NAME}" - - - run: bun run build:npm-packages -- main - - - run: npm pack --dry-run - working-directory: dist/npm/main - - - name: Publish npm package - working-directory: dist/npm/main - run: | - name="$(node -p "require('./package.json').name")" - version="$(node -p "require('./package.json').version")" - if npm view "${name}@${version}" version >/dev/null 2>&1; then - echo "${name}@${version} already published." - exit 0 - fi - npm publish - - update-homebrew: - if: github.repository == 'kitlangton/ghui' - runs-on: ubuntu-latest - needs: [publish, standalone] - steps: - - name: Download standalone checksums - env: - GH_TOKEN: ${{ github.token }} - run: | - mkdir -p dist/release - gh release download "${GITHUB_REF_NAME}" --repo "${GITHUB_REPOSITORY}" --pattern "*.sha256" --dir dist/release - - - name: Update Homebrew tap - env: - GH_TOKEN: ${{ secrets.HOMEBREW_TAP_TOKEN }} - run: | - : "${GH_TOKEN:?Set HOMEBREW_TAP_TOKEN to a token with access to kitlangton/homebrew-tap}" - version="${GITHUB_REF_NAME#v}" - cat dist/release/*.sha256 > dist/release/checksums.txt - checksum() { - awk -v file="$1" '$2 == file { print $1 }' dist/release/checksums.txt - } - gh api --method POST repos/kitlangton/homebrew-tap/dispatches \ - -f event_type=ghui-release \ - -f client_payload[tag]="${GITHUB_REF_NAME}" \ - -f client_payload[version]="${version}" \ - -f client_payload[darwin_arm64_sha256]="$(checksum ghui-darwin-arm64.tar.gz)" \ - -f client_payload[darwin_x64_sha256]="$(checksum ghui-darwin-x64.tar.gz)" \ - -f client_payload[linux_arm64_sha256]="$(checksum ghui-linux-arm64.tar.gz)" \ - -f client_payload[linux_x64_sha256]="$(checksum ghui-linux-x64.tar.gz)" diff --git a/.gitignore b/.gitignore index 978da5e0..2ab984f5 100644 --- a/.gitignore +++ b/.gitignore @@ -23,6 +23,9 @@ coverage/ # local mock snapshots .phui/ +# playwright / tui capture leftovers +test-results/ + # Beads / Dolt files (added by bd init) .dolt/ *.db diff --git a/AGENTS.md b/AGENTS.md index c4cbdad5..ba029b89 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -31,10 +31,9 @@ - After merging a release PR, verify the `Release Please` run passes — it includes the called publish jobs and a `verify-published` matrix that installs the published package from the registry on every platform and runs it. -- The upstream npm workflow (`.github/workflows/publish.yml`) is - repository-gated to `kitlangton/ghui` and skips on the fork. Do not un-gate - it; fork publishing lives in `fork-publish.yml`, which is fork-owned and does - not conflict on merge. +- Fork publishing lives in `fork-publish.yml` (called from `release-please.yml`). + Do not copy upstream's `publish.yml` back in; it is gated to `kitlangton/ghui` + and would fight this fork's trusted-publisher setup. ### npm diff --git a/bun.lock b/bun.lock index 3a1e594d..0e790df3 100644 --- a/bun.lock +++ b/bun.lock @@ -3,7 +3,7 @@ "configVersion": 1, "workspaces": { "": { - "name": "ghui", + "name": "@phall/phui", "dependencies": { "@effect/atom-solid": "4.0.0-rc.115", "@effect/sql-sqlite-bun": "4.0.0-rc.115", diff --git a/dev/probe-atom-pipeline.ts b/dev/probe-atom-pipeline.ts deleted file mode 100644 index a62a3d73..00000000 --- a/dev/probe-atom-pipeline.ts +++ /dev/null @@ -1,125 +0,0 @@ -// Exercise the full atom pipeline outside the TUI: -// activeViewAtom -> pullRequestsAtom -> queueLoadCacheAtom -// -> pullRequestLoadAtom -// -> displayedPullRequestsAtom -// -> filteredPullRequestsAtom -// -> visiblePullRequestsAtom -// -// Steps: -// 1. Set activeView to Repository view (all PRs) and resolve. -// 2. Switch activeView to Queue authored and resolve. -// 3. Print what each atom contains AFTER the switch. -// -// If visiblePullRequestsAtom contains 50+ PRs from various authors after -// step 2, we've reproduced the bug in headless mode and can diff each -// atom's state to find where the broken data is coming from. -// -// Logs to /tmp/phui-debug.log (default PHUI_DEBUG_LOG path) so the -// devLog instrumentation in atoms.ts fires. - -process.env.PHUI_DEBUG_LOG ??= "/tmp/phui-debug.log" - -import * as Atom from "effect/unstable/reactivity/Atom" -import * as AtomRegistry from "effect/unstable/reactivity/AtomRegistry" -import * as AsyncResult from "effect/unstable/reactivity/AsyncResult" -import { activeViewAtom, displayedPullRequestsAtom, pullRequestsAtom, queueLoadCacheAtom, resolveLoad, visiblePullRequestsAtom } from "../src/ui/pullRequests/atoms.js" -import { filteredPullRequestsAtom } from "../src/ui/pullRequests/atoms.js" -import type { PullRequestView } from "../src/pullRequestViews.js" -import { viewCacheKey } from "../src/pullRequestViews.js" - -const REPO = process.argv[2] ?? "anomalyco/opencode" - -const repositoryView: PullRequestView = { _tag: "Repository", repository: REPO } -const authoredView: PullRequestView = { _tag: "Queue", mode: "authored", repository: REPO } - -const registry = AtomRegistry.make() - -// Wait for an atom's AsyncResult to settle (success or failure). -const waitForResult = (atom: Atom.Atom>): Promise> => - new Promise((resolve) => { - const initial = registry.get(atom) - if (!initial.waiting && (AsyncResult.isSuccess(initial) || AsyncResult.isFailure(initial))) { - resolve(initial) - return - } - const unsub = registry.subscribe(atom, (value) => { - if (!value.waiting && (AsyncResult.isSuccess(value) || AsyncResult.isFailure(value))) { - unsub() - resolve(value) - } - }) - }) - -const sample = (label: string) => { - const view = registry.get(activeViewAtom) - const result = registry.get(pullRequestsAtom) - const cache = registry.get(queueLoadCacheAtom) - const load = resolveLoad(view, cache, result) - const displayed = registry.get(displayedPullRequestsAtom) - const filtered = registry.get(filteredPullRequestsAtom) - const visible = registry.get(visiblePullRequestsAtom) - console.log(`\n=== ${label} ===`) - console.log("activeView: ", view) - console.log("activeView cacheKey: ", viewCacheKey(view)) - console.log("pullRequestsAtom: ", { waiting: result.waiting, kind: result._tag }) - const resolved = AsyncResult.getOrElse(result, () => null) - console.log(" resolved view: ", resolved?.view) - console.log(" resolved cacheKey: ", resolved ? viewCacheKey(resolved.view) : null) - console.log(" resolved dataLen: ", resolved?.data.length ?? null) - console.log("queueLoadCacheAtom keys:", Object.keys(cache)) - for (const [k, v] of Object.entries(cache)) { - console.log(` [${k}] dataLen=${v?.data.length} sampleAuthors=${JSON.stringify(v?.data.slice(0, 3).map((pr) => pr.author))}`) - } - console.log("resolveLoad: ", load ? { view: load.view, dataLen: load.data.length, sampleAuthors: load.data.slice(0, 3).map((pr) => pr.author) } : null) - console.log( - "displayedPullRequests:", - displayed.length, - "sampleAuthors:", - displayed.slice(0, 5).map((pr) => pr.author), - ) - console.log( - "filteredPullRequests: ", - filtered.length, - "sampleAuthors:", - filtered.slice(0, 5).map((pr) => pr.author), - ) - console.log( - "visiblePullRequests: ", - visible.length, - "sampleAuthors:", - visible.slice(0, 5).map((pr) => pr.author), - ) -} - -// Subscribe to displayedPullRequestsAtom to keep it active during transitions -// — this mimics what `useAtomValue(visiblePullRequestsAtom)` does in the React -// tree. Without an active subscriber, derived atoms may be GC'd between reads, -// which would mask any propagation bug. -const unsubDisplayed = registry.subscribe(displayedPullRequestsAtom, () => {}) -const unsubVisible = registry.subscribe(visiblePullRequestsAtom, () => {}) - -// 1. Start in Queue authored global (the initial-view state phui boots into). -const globalAuthored: PullRequestView = { _tag: "Queue", mode: "authored", repository: null } -console.log(">> Setting activeView to Queue(authored, global)") -registry.set(activeViewAtom, globalAuthored) -await waitForResult(pullRequestsAtom) -sample("STEP 1: AFTER global authored fetch") - -console.log("\n>> Switching activeView to Repository(anomalyco/opencode)") -registry.set(activeViewAtom, repositoryView) -await waitForResult(pullRequestsAtom) -sample("STEP 2: AFTER Repository fetch") - -console.log("\n>> Switching activeView to Queue(authored, anomalyco/opencode) — repro the bug") -registry.set(activeViewAtom, authoredView) -// Sample IMMEDIATELY (before fetch completes) to capture the transient state -// the user actually sees. -sample("STEP 3a: IMMEDIATELY AFTER set(authoredView)") -await waitForResult(pullRequestsAtom) -sample("STEP 3b: AFTER Queue(authored) fetch") - -unsubDisplayed() -unsubVisible() - -console.log("\n>> Done. See /tmp/phui-debug.log for atom-level trace.") -process.exit(0) diff --git a/dev/probe-filter.ts b/dev/probe-filter.ts deleted file mode 100644 index e5ad16e7..00000000 --- a/dev/probe-filter.ts +++ /dev/null @@ -1,55 +0,0 @@ -// Probe the GitHub search endpoint with the same query the authored-filter -// view would send. If the server returns Kit-only PRs here, the bug is -// somewhere in the atom layer (cache, key collision, stale read). If it -// returns the same broken list, the issue is gh-CLI / authentication. -// -// Run with: bun run dev/probe-filter.ts [owner/repo] - -import { Effect, Layer } from "effect" -import { searchQualifier } from "../src/item.js" -import { type PullRequestView, viewToListInput } from "../src/pullRequestViews.js" -import { CommandRunner } from "../src/services/CommandRunner.js" -import { GitHubService } from "../src/services/GitHubService.js" - -const repo = process.argv[2] ?? "anomalyco/opencode" -const view: PullRequestView = { _tag: "Queue", mode: "authored", repository: repo } -const listInput = viewToListInput(view, null, 50) - -console.log("=== probe-filter ===") -console.log("repo: ", repo) -console.log("view: ", view) -console.log("listInput: ", listInput) -console.log("searchQuery: ", searchQualifier(listInput)) -console.log("---") - -const program = Effect.gen(function* () { - const github = yield* GitHubService - const user = yield* github.getAuthenticatedUser().pipe(Effect.catch((e) => Effect.succeed(``))) - console.log("authenticated as:", user) - const page = yield* github.listPullRequestPage(listInput) - console.log("---") - console.log("page item count:", page.items.length) - console.log("hasNextPage: ", page.hasNextPage) - console.log("endCursor: ", page.endCursor) - console.log("---") - const byAuthor = new Map() - for (const pr of page.items) { - byAuthor.set(pr.author, (byAuthor.get(pr.author) ?? 0) + 1) - } - console.log("authors in result:") - for (const [author, count] of [...byAuthor.entries()].sort((a, b) => b[1] - a[1])) { - console.log(` - ${author}: ${count}`) - } - console.log("---") - console.log("first 10:") - for (const pr of page.items.slice(0, 10)) { - console.log(` - #${pr.number} by ${pr.author}: ${pr.title}`) - } -}) - -const layer = GitHubService.layerNoDeps.pipe(Layer.provide(CommandRunner.layer)) - -await Effect.runPromise(program.pipe(Effect.provide(layer))).catch((e) => { - console.error("FAILED:", e) - process.exit(1) -}) diff --git a/packages/keymap/COMPARISON.md b/packages/keymap/COMPARISON.md deleted file mode 100644 index 478588c5..00000000 --- a/packages/keymap/COMPARISON.md +++ /dev/null @@ -1,345 +0,0 @@ -# Real-app comparison: phui - -Side-by-side translations of actual phui keyboard layers from `src/App.tsx` -into the `@phui/keymap` API. Code on the left is *what's shipping today* -(`@opentui/keymap` + a custom `useScopedBindings` wrapper). Code on the right -is the same behavior re-expressed against `@phui/keymap`. - ---- - -## 1. CloseModal — the smallest case - -**Today** (`src/App.tsx`, 11 lines including the local helpers it depends on): - -```tsx -// Inside the App component, intermixed with ~80 useState/useAtom hooks -// and references to component-scope `closeActiveModal` + `confirmClosePullRequest`: - -useScopedBindings({ - when: closeModalActive, - bindings: { - escape: closeActiveModal, - return: confirmClosePullRequest, - }, -}) -``` - -**With `@phui/keymap`**: - -```ts -// src/keymap/closeModal.ts — separate file, importable, testable -import { command, Keymap } from "@phui/keymap" - -export interface CloseModalCtx { - readonly closeModal: () => void - readonly confirmClose: () => void -} - -export const closeModalKeymap: Keymap = Keymap.union( - command({ id: "modal.cancel", title: "Cancel", keys: ["escape"], run: (s) => s.closeModal() }), - command({ id: "modal.confirm", title: "Close pull request", keys: ["return"], run: (s) => s.confirmClose() }), -) -``` - -**Diff in shape:** - -| | Today | With `@phui/keymap` | -|---|---|---| -| Where the bindings live | Inside `App` component body | Importable module, top-level | -| What state they see | All of `App`'s closure | Just `CloseModalCtx` | -| Test without React | No (hook needs mounting) | Yes (just call `pureDispatch`) | -| Importable into a palette | No | Yes (`closeModalKeymap.commands(ctx)`) | - ---- - -## 2. MergeModal — selection state - -**Today** (12 lines): - -```tsx -const moveMergeSelection = (delta: -1 | 1) => setMergeModal((current) => { - const max = Math.max(0, availableMergeActions(mergeModal.info).length - 1) - return { ...current, selectedIndex: Math.max(0, Math.min(max, current.selectedIndex + delta)) } -}) -useScopedBindings({ - when: mergeModalActive, - bindings: { - escape: closeActiveModal, - return: () => { - if (availableMergeActions(mergeModal.info).length > 0) confirmMergeAction() - }, - up: () => moveMergeSelection(-1), - k: () => moveMergeSelection(-1), - down: () => moveMergeSelection(1), - j: () => moveMergeSelection(1), - }, -}) -``` - -**With `@phui/keymap`**: - -```ts -// src/keymap/mergeModal.ts -import { command, Keymap } from "@phui/keymap" - -export interface MergeModalCtx { - readonly availableActionCount: number - readonly closeModal: () => void - readonly confirmMerge: () => void - readonly moveSelection: (delta: -1 | 1) => void -} - -export const mergeModalKeymap: Keymap = Keymap.union( - command({ id: "merge.cancel", title: "Cancel", keys: ["escape"], run: (s) => s.closeModal() }), - command({ - id: "merge.confirm", - title: "Merge", - keys: ["return"], - enabled: (s) => s.availableActionCount > 0 ? true : "No merge actions available.", - run: (s) => s.confirmMerge(), - }), - command({ id: "merge.up", title: "Up", keys: ["k", "up"], run: (s) => s.moveSelection(-1) }), - command({ id: "merge.down", title: "Down", keys: ["j", "down"], run: (s) => s.moveSelection(1) }), -) -``` - -The "if there are no actions, don't fire" branch becomes a typed `enabled` — -the dispatcher reports `disabled` with reason instead of silently no-oping. - ---- - -## 3. diffFullView with sub-mode — where composition shines - -This is where today's design starts hurting. `diffFullView` has *two modes*: -regular scroll mode and `diffCommentMode`. Today they're two flat layers with -mutually-exclusive `when` predicates that both reach into App's closure: - -**Today** (~80 lines for both modes, inline in App.tsx): - -```tsx -useScopedBindings({ - when: diffFullView && !diffCommentMode, - bindings: { - ...scrollBindings(scrollDiffBy, halfPage, scrollDiffTo), - escape: "diff.close", - return: "diff.close", - c: "diff.comment-mode", - v: "diff.toggle-view", - w: "diff.toggle-wrap", - r: "diff.reload", - "]": "diff.next-file", right: "diff.next-file", l: "diff.next-file", - "[": "diff.previous-file", left: "diff.previous-file", h: "diff.previous-file", - o: "pull.open-browser", - }, -}) - -useScopedBindings({ - when: diffFullView && diffCommentMode, - bindings: { - escape: () => setDiffCommentMode(false), - c: "diff.comment-mode", - return: () => { - if (selectedDiffCommentThread.length > 0) openDiffCommentThreadModal() - else openDiffCommentModal() - }, - a: "diff.add-comment", - pageup: () => moveDiffCommentAnchor(-halfPage), - "ctrl+u": () => moveDiffCommentAnchor(-halfPage), - pagedown: () => moveDiffCommentAnchor(halfPage), - // ... 20 more lines ... - }, -}) -``` - -Both layers reference component-local helpers (`scrollDiffBy`, -`moveDiffCommentAnchor`, `openDiffCommentThreadModal`). Both are gated by -inline boolean expressions. The relationship between the two modes is -implicit — you have to know to read both `when` conditions. - -**With `@phui/keymap`**: - -```ts -// src/keymap/diff.ts — defined over the diff's own state shape -import { command, Keymap, scrollCommands } from "@phui/keymap" - -export interface DiffCtx { - readonly hasOpenPullRequest: boolean - readonly halfPage: number - readonly scrollBy: (delta: number) => void - readonly scrollTo: (line: number) => void - readonly closeDiff: () => void - readonly enterCommentMode: () => void - readonly toggleView: () => void - readonly toggleWrap: () => void - readonly reload: () => void - readonly nextFile: () => void - readonly previousFile: () => void - readonly openInBrowser: () => void -} - -export const diffViewKeymap: Keymap = Keymap.union( - scrollCommands((s) => s.scrollBy, (s) => s.scrollTo, (s) => s.halfPage), - command({ id: "diff.close", title: "Close", keys: ["escape", "return"], run: (s) => s.closeDiff() }), - command({ id: "diff.comment-mode", title: "Comment mode", keys: ["c"], run: (s) => s.enterCommentMode() }), - command({ id: "diff.toggle-view", title: "Toggle view", keys: ["v"], run: (s) => s.toggleView() }), - command({ id: "diff.toggle-wrap", title: "Toggle wrap", keys: ["w"], run: (s) => s.toggleWrap() }), - command({ id: "diff.reload", title: "Reload", keys: ["r"], run: (s) => s.reload() }), - command({ id: "diff.next-file", title: "Next file", keys: ["]", "right", "l"], run: (s) => s.nextFile() }), - command({ id: "diff.previous-file", title: "Prev file", keys: ["[", "left", "h"], run: (s) => s.previousFile() }), - command({ id: "diff.open-browser", title: "Open", keys: ["o"], run: (s) => s.openInBrowser() }), -) - - -// src/keymap/diffComment.ts — the sub-mode's OWN keymap, OWN context -import { command, Keymap } from "@phui/keymap" - -export interface DiffCommentCtx { - readonly halfPage: number - readonly hasThread: boolean - readonly exitCommentMode: () => void - readonly toggleCommentMode: () => void - readonly openInlineModal: () => void - readonly openThreadModal: () => void - readonly addComment: () => void - readonly moveAnchor: (delta: number) => void - readonly selectSide: (side: "LEFT" | "RIGHT") => void - readonly nextFile: () => void - readonly previousFile: () => void -} - -export const diffCommentKeymap: Keymap = Keymap.union( - command({ id: "diff-comment.exit", title: "Exit comment mode", keys: ["escape"], run: (s) => s.exitCommentMode() }), - command({ id: "diff-comment.toggle", title: "Toggle", keys: ["c"], run: (s) => s.toggleCommentMode() }), - command({ - id: "diff-comment.open", - title: "Open / reply", - keys: ["return"], - run: (s) => s.hasThread ? s.openThreadModal() : s.openInlineModal(), - }), - command({ id: "diff-comment.add", title: "Add comment", keys: ["a"], run: (s) => s.addComment() }), - command({ id: "diff-comment.up", title: "Up", keys: ["k", "up"], run: (s) => s.moveAnchor(-1) }), - command({ id: "diff-comment.down", title: "Down", keys: ["j", "down"], run: (s) => s.moveAnchor(1) }), - command({ id: "diff-comment.jump-up", title: "Jump up", keys: ["shift+k", "shift+up", "meta+k", "meta+up"], run: (s) => s.moveAnchor(-8) }), - command({ id: "diff-comment.jump-down", title: "Jump down", keys: ["shift+j", "shift+down", "meta+j", "meta+down"], run: (s) => s.moveAnchor(8) }), - command({ id: "diff-comment.half-up", title: "Half page up", keys: ["pageup", "ctrl+u"], run: (s) => s.moveAnchor(-s.halfPage) }), - command({ id: "diff-comment.half-down", title: "Half page down", keys: ["pagedown", "ctrl+d", "ctrl+v"], run: (s) => s.moveAnchor(s.halfPage) }), - command({ id: "diff-comment.left-side", title: "Old side", keys: ["left", "h"], run: (s) => s.selectSide("LEFT") }), - command({ id: "diff-comment.right-side", title: "New side", keys: ["right", "l"], run: (s) => s.selectSide("RIGHT") }), - command({ id: "diff-comment.next-file", title: "Next file", keys: ["]"], run: (s) => s.nextFile() }), - command({ id: "diff-comment.prev-file", title: "Prev file", keys: ["["], run: (s) => s.previousFile() }), -) -``` - -Now the **App** glues them together with their respective scopes. The two -sub-keymaps don't know about each other or about `AppCtx`: - -```ts -// src/keymap/all.ts -import { Keymap } from "@phui/keymap" -import { diffViewKeymap, type DiffCtx } from "./diff.ts" -import { diffCommentKeymap, type DiffCommentCtx } from "./diffComment.ts" -import type { AppCtx } from "./state.ts" - -const projectDiff = (a: AppCtx): DiffCtx | null => - a.diffFullView && !a.diffCommentMode ? a.diff : null - -const projectDiffComment = (a: AppCtx): DiffCommentCtx | null => - a.diffFullView && a.diffCommentMode ? a.diffComment : null - -export const appKeymap: Keymap = Keymap.union( - diffViewKeymap.contramapMaybe(projectDiff), - diffCommentKeymap.contramapMaybe(projectDiffComment), - // ... others ... -) -``` - -### What the diff bought us - -| | Today | With `@phui/keymap` | -|---|---|---| -| Sub-mode types | None — both layers see all of App | `DiffCtx` and `DiffCommentCtx` are independent | -| Where mode-exclusive logic lives | Inline `when` boolean | At the projection site, isolated | -| Can the diff layer be tested? | Only by mounting App | Yes, with a fake `DiffCtx` and a few `parseKey` calls | -| Adding a new sub-mode | Add another `useScopedBindings` block in App | Add a new file, glue with one line | - ---- - -## 4. The whole `App.tsx` body, before vs. after - -### Before (today, in `App.tsx`) - -```tsx -// ~80 lines of useState/useAtom/derived selectors -// ~10 useScopedBindings calls, each ~10–30 lines, scattered with helpers -// useKeyboard with text-input fallbacks for 6 modal types -// 200 lines of JSX -``` - -The keyboard surface is **not visible**. To answer "what does `r` do here?", -you grep for `"r":` across nine separate `useScopedBindings` blocks. - -### After - -```tsx -// src/App.tsx -import { useKeymap } from "@phui/keymap/react" -import { appKeymap } from "./keymap/all.ts" -import type { AppCtx } from "./keymap/state.ts" - -const App = () => { - // ... existing useState / useAtom hooks ... - - const ctx: AppCtx = { - // The state shape is the contract. App.tsx fills it in. - closeModal: { active: closeModalActive, closeModal: closeActiveModal, confirmClose: confirmClosePullRequest }, - diff: { ... }, - diffComment: { ... }, - // ... - } - - useKeymap(appKeymap, ctx, subscribeToOpenTuiKeys) - - return -} -``` - -The keyboard surface lives in `src/keymap/`. To answer "what does `r` do?", -read one file. To answer "what's bound globally right now?", call -`appKeymap.commands(ctx)`. To test that `escape` closes the close-modal, -you call `pureDispatch(closeModalKeymap, initialDispatchState, parseKey("escape"), fakeCtx, 0)` -— no React, no opentui, no mocks. - ---- - -## 5. The numbers - -For phui's actual keyboard surface (~12 layers, ~100 bindings): - -| Metric | Today | With `@phui/keymap` (estimated) | -|---|---|---| -| Lines of keyboard code in `App.tsx` | ~370 | ~5 (one `useKeymap` call + a `ctx` object literal) | -| Lines of importable keyboard code | 0 | ~250 (split across 8 files) | -| Tests that mount React | 13 (the scrolling tests) | Could drop to ~3 (just the integration ones) | -| Way to introspect "what's bound now" | Manual grep | `appKeymap.commands(ctx)` | -| State surface used by bindings | All of App's closure | Each binding sees only its narrow context type | - ---- - -## 6. The honest tradeoffs - -The library is genuinely better at: - -- **Locality**: each layer is a value in its own file with its own types. -- **Testability**: `pureDispatch` is a function; tests are calls. -- **Composition**: sub-modes are sub-keymaps; gluing is a one-liner with `contramapMaybe`. -- **Type safety**: command IDs typed via `meta`; sub-context shapes don't leak. -- **Introspection**: `appKeymap.commands(ctx)` and `snapshot(km, ctx)` give palette/footer/devtools a real API. - -The library is genuinely worse at: - -- **State plumbing**: each layer's narrow context (`DiffCtx`, `MergeModalCtx`, ...) needs to be assembled by `App.tsx` once per render. That's a ~50-field `ctx: AppCtx` object literal. With Zustand/Atom/Redux, this falls out naturally; with plain `useState` everywhere, it's busywork. -- **First-time friction**: users have to think about *what context this layer sees*. With the current opentui style you just close over component state and move on. -- **No focus-scoping yet**: `contramapMaybe` solves view-scoping; element/Renderable focus would need a target-ref primitive that we don't have. - -For phui specifically, the trades go positive. For a smaller app with fewer -keyboard layers, the plumbing cost might dominate. diff --git a/packages/keymap/MIGRATION.md b/packages/keymap/MIGRATION.md deleted file mode 100644 index 0bb8c9aa..00000000 --- a/packages/keymap/MIGRATION.md +++ /dev/null @@ -1,174 +0,0 @@ -# Migration sketch — phui using @phui/keymap - -A concrete before/after for phui's modal + global layers. Demonstrates that the -library's primary value is _organizational_: bindings move out of the component -into typed, importable values; state is a single shape passed in once. - -## Before — what phui ships today - -`src/App.tsx` gathers state + actions inside the component, then issues 9 -`useScopedBindings` calls peppered through the body. Each layer's bindings are -inline closures over component-local state. To answer "what does `r` do" the -reader greps; to answer "list every binding" they can't. - -```tsx -const App = () => { - // ... 80 lines of useState, useAtom, derived selectors ... - - useScopedBindings({ - when: closeModalActive, - bindings: { - escape: closeActiveModal, - return: confirmClosePullRequest, - }, - }) - - useScopedBindings({ - when: globalLayerActive, - bindings: { - "/": "filter.open", - r: "pull.refresh", - // ... 30 more entries ... - }, - }) - - // ... 7 more useScopedBindings calls ... - - // ... 200 lines of JSX ... -} -``` - -This works (it's what we just shipped) but the bindings live inside the -component's render function. They can't be tested without React, can't be -imported by a palette, and their gating conditions are scattered. - -## After — using @phui/keymap - -Bindings become an importable value. State becomes a single shape passed once. -The component is JSX + state + one `useKeymap` call. - -```ts -// src/keymap/state.ts — the shape, declared once -export interface AppState { - closeModalActive: boolean - diffFullView: boolean - diffCommentMode: boolean - selectedPullRequest: PullRequest | null - closeActiveModal: () => void - confirmClosePullRequest: () => void - refresh: () => void - scrollDiffBy: (delta: number) => void - // ... etc -} -``` - -```ts -// src/keymap/closeModal.ts -import { defineCommand, scope } from "@phui/keymap" -import type { AppState } from "./state.ts" - -export const closeModalCommands = scope( - (s) => s.closeModalActive, - [ - defineCommand({ - id: "close-modal.cancel", - title: "Cancel", - keys: ["escape"], - run: (s) => s.closeActiveModal(), - }), - defineCommand({ - id: "close-modal.confirm", - title: "Close pull request", - keys: ["return"], - run: (s) => s.confirmClosePullRequest(), - }), - ], -) -``` - -```ts -// src/keymap/diffView.ts -import { defineCommand, scope } from "@phui/keymap" - -const scrollBindings = (axis: "diff" | "detail"): readonly Command[] => [ - defineCommand({ id: `${axis}.up`, title: "Scroll up", keys: ["k", "up"], run: (s) => s.scrollDiffBy(-1) }), - defineCommand({ id: `${axis}.down`, title: "Scroll down", keys: ["j", "down"], run: (s) => s.scrollDiffBy(1) }), - defineCommand({ id: `${axis}.top`, title: "Top", keys: ["g g"], run: (s) => s.scrollDiffTo(0) }), - defineCommand({ id: `${axis}.bottom`, title: "Bottom", keys: ["shift+g"], run: (s) => s.scrollDiffTo(Number.MAX_SAFE_INTEGER) }), - // ... -] - -export const diffViewCommands = scope( - (s) => s.diffFullView && !s.diffCommentMode, - [ - ...scrollBindings("diff"), - defineCommand({ id: "diff.close", title: "Close diff", keys: ["escape", "return"], run: (s) => s.closeDiff() }), - // ... - ], -) -``` - -```ts -// src/keymap/all.ts -import { closeModalCommands } from "./closeModal.ts" -import { diffViewCommands } from "./diffView.ts" -// ... others ... - -export const allCommands = [ - ...closeModalCommands, - ...diffViewCommands, - // ... -] -``` - -```tsx -// src/App.tsx -import { useKeymap } from "@phui/keymap/react" -import { allCommands } from "./keymap/all.ts" - -const App = () => { - // ... existing useState / useAtom ... - - const state: AppState = { - closeModalActive, - diffFullView, - diffCommentMode, - selectedPullRequest, - closeActiveModal, - confirmClosePullRequest, - refresh, - scrollDiffBy, - // ... etc - } - - useKeymap(allCommands, state, subscribeToOpenTuiKeys) - - // ... JSX ... -} -``` - -## What this buys - -- **One source of truth.** `allCommands` is the entire keyboard surface of the - app. Import it anywhere. Render a "what's bound right now" view in two lines: - `getActiveCommands(allCommands, state).map(c =>
  • {c.title}
    • )`. -- **Tests without React.** The dispatcher tests (`createDispatcher` + state) - exercise binding behavior without mounting any UI. -- **Typed command IDs.** `Command['id']` is `string`, but each - command is a typed value — referencing `closeModalCommands[0]!.id` typechecks - and renames cleanly. -- **No ref dance, no `useRef`/`useEffect` in user code.** State is an argument. - Closures inside `run` are always fresh because state is read at dispatch time. -- **Sequence semantics, no parser surprise.** `keys: ["g g"]` is multi-stroke; - vim-style timeout disambiguation is built in. - -## What this trades - -- **One big state shape.** Action callbacks that previously closed over - component-local state now have to be put into the `state` object. If your app - uses Zustand / Redux / Atom, this falls out naturally; if everything's - `useState`, it's busywork. -- **Less power per layer.** No priority numbers, no fields, no transformers. If - you need those, this isn't the library. -- **No focus-scoped bindings yet.** Every command sees every key. You'd add a - `focusTarget` or similar if you wanted DOM/Renderable-level scoping. diff --git a/plans/daily-driver.md b/plans/daily-driver.md index 81de50f8..1a93c2aa 100644 --- a/plans/daily-driver.md +++ b/plans/daily-driver.md @@ -71,7 +71,7 @@ Nice-to-have, not the bar: 2. **Item load leftover** — [`item-load-deepening.md`](./item-load-deepening.md): one cache module, still-separate per-kind atom families. 3. **GitHubService carve** — ~40 methods in one `Effect.gen`. Split along Item / Review / Merge / Runs when Track A adds pending reviews, resolve, reviewers, update-branch. Do not split first for sport. 4. **Hook tests** — zero isolated Surface tests. `@testing-library/react` + atom `initialValues` + fixtures lifted from `MockGitHubService`. Pin the first eight invariants from the deepening brief. -5. **Dead weight** — `listAllPullRequests` / `listAllIssues` unused in app code; `bun.lock` workspace name still `ghui`; keymap `COMPARISON.md` / `MIGRATION.md` stale. +5. **Dead weight** — ~~`listAllPullRequests` / `listAllIssues`; `bun.lock` workspace name `ghui`; keymap `COMPARISON.md` / `MIGRATION.md`~~ removed. ### Track D — libraries and toolchain diff --git a/src/services/GitHubService.ts b/src/services/GitHubService.ts index 05c531bb..539d96c9 100644 --- a/src/services/GitHubService.ts +++ b/src/services/GitHubService.ts @@ -1,5 +1,4 @@ -import { Context, Effect, Layer, Schema, Stream } from "effect" -import * as Option from "effect/Option" +import { Context, Effect, Layer, Schema } from "effect" import { config } from "../config.js" import { type CreatePullRequestCommentInput, @@ -109,8 +108,6 @@ export class GitHubService extends Context.Service< { readonly listPullRequestPage: (input: ItemListInput<"pullRequest">) => Effect.Effect, GitHubError> readonly listIssuePage: (input: ItemListInput<"issue">) => Effect.Effect, GitHubError> - readonly listAllPullRequests: (input: Omit, "cursor" | "pageSize">) => Effect.Effect - readonly listAllIssues: (input: Omit, "cursor" | "pageSize">) => Effect.Effect readonly getPullRequestDetails: (repository: string, number: number) => Effect.Effect readonly getRepositoryDetails: (repository: string) => Effect.Effect readonly getAuthenticatedUser: () => Effect.Effect @@ -269,35 +266,6 @@ export class GitHubService extends Context.Service< return yield* listIssueSearchPage({ ...input, pageSize }) }) - // Drain every page for an item query into a single array, using - // `Stream.paginate`. Interrupting the surrounding fiber stops mid-flight. - const drainItemPages = ( - query: Omit, "cursor" | "pageSize">, - pageFetch: (input: ItemListInput) => Effect.Effect, GitHubError>, - limit: number, - ): Effect.Effect => { - type State = { readonly cursor: string | null; readonly fetched: number } - const stream = Stream.paginate({ cursor: null, fetched: 0 }, ({ cursor, fetched }) => { - const remaining = limit - fetched - if (remaining <= 0) return Effect.succeed([[], Option.none()] as const) - const pageSize = Math.min(100, remaining) - return pageFetch({ ...query, cursor, pageSize } as ItemListInput).pipe( - Effect.map((page): readonly [readonly Item[], Option.Option] => { - const items = page.items.slice(0, remaining) - const nextFetched = fetched + items.length - const next: Option.Option = - page.hasNextPage && page.endCursor && nextFetched < limit ? Option.some({ cursor: page.endCursor, fetched: nextFetched }) : Option.none() - return [items, next] - }), - ) - }) - return Stream.runCollect(stream).pipe(Effect.map((chunk) => Array.from(chunk))) - } - - const listAllPullRequests = (input: Omit, "cursor" | "pageSize">) => - drainItemPages<"pullRequest", PullRequestItem>(input, listPullRequestPage, config.prFetchLimit) - const listAllIssues = (input: Omit, "cursor" | "pageSize">) => drainItemPages<"issue", IssueItem>(input, listIssuePage, config.prFetchLimit) - const getPullRequestDetails = Effect.fn("GitHubService.getPullRequestDetails")(function* (repository: string, number: number) { const repo = repositoryParts(repository) if (!repo) { @@ -745,8 +713,6 @@ export class GitHubService extends Context.Service< return GitHubService.of({ listPullRequestPage, listIssuePage, - listAllPullRequests, - listAllIssues, getPullRequestDetails, getRepositoryDetails, getAuthenticatedUser, diff --git a/src/services/MockGitHubService.ts b/src/services/MockGitHubService.ts index 9dfb6948..b1901622 100644 --- a/src/services/MockGitHubService.ts +++ b/src/services/MockGitHubService.ts @@ -452,16 +452,6 @@ export const MockGitHubService = { return Effect.succeed(slicePage(filtered, input.cursor, input.pageSize)) }, listIssuePage: (input: ItemListInput<"issue">) => Effect.succeed(slicePage(filterIssuesByMode(input.mode, input.repository, issues), input.cursor, input.pageSize)), - listAllPullRequests: (input: { - readonly kind: "pullRequest" - readonly mode: "all" | "authored" | "review" | "assigned" | "mentioned" - readonly repository: string | null - }) => { - const queueMode = queueModeForListMode(input.mode) - return Effect.succeed(filterByView(queueMode, input.repository, pullRequestSource(queueMode, input.repository), username, strictUserScope)) - }, - listAllIssues: (input: { readonly kind: "issue"; readonly mode: "all" | "authored" | "assigned" | "mentioned"; readonly repository: string | null }) => - Effect.succeed(filterIssuesByMode(input.mode, input.repository, issues)), }), ) }, diff --git a/test/packagePins.test.ts b/test/packagePins.test.ts index 3d56d5df..70b07793 100644 --- a/test/packagePins.test.ts +++ b/test/packagePins.test.ts @@ -26,7 +26,7 @@ describe("daily-driver package pins", () => { expect(bunVersion).toBe("1.3.14") expect(packageJson.devDependencies["@types/bun"]).toBe(bunVersion) - for (const name of ["ci.yml", "fork-publish.yml", "publish.yml"]) { + for (const name of ["ci.yml", "fork-publish.yml"]) { const text = await Bun.file(`.github/workflows/${name}`).text() const setupCount = [...text.matchAll(/uses: oven-sh\/setup-bun@v2/g)].length const pinCount = [...text.matchAll(/bun-version-file: \.bun-version/g)].length