Skip to content

GitHub PR workflow - #6

Open
adamziel wants to merge 7 commits into
trunkfrom
create-pr
Open

GitHub PR workflow#6
adamziel wants to merge 7 commits into
trunkfrom
create-pr

Conversation

@adamziel

@adamziel adamziel commented Nov 8, 2025

Copy link
Copy Markdown
Contributor

Adds a UI path for connecting with GitHub and creating a PR:

Zrzut ekranu 2025-11-8 o 01 31 22

juanmaguitar added a commit that referenced this pull request Aug 7, 2026
- git:apply-patch rejects a sitePath that is not a registered site before
  writing anything, the same gate sites:set-ticket uses (#8).
- Persisting the revert record is now part of the apply transaction: if the
  store write fails the apply is undone, and if that also fails the result
  says the patch is applied-but-untracked rather than reporting a clean
  failure (#7).
- git:preview-patch surfaces a worktree-inspection failure instead of
  reporting "no collisions" when it could not look (#9).
- git:discard-changes clears the applied-patch record with the reset, so a
  later trunk-update network failure cannot leave a revert banner for a
  patch that is already gone (#6).
- git:apply-patch is now a WIRED handler with real tests (guard, reverse
  lookup, delegation, done event, metadata) via fakeSettingsStore, instead
  of a NOT_REACHABLE hole (#13).

Not gating git:preview-patch: it is read-only, and the registered-site
check would add a store dependency to a handler that has none.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar added a commit that referenced this pull request Aug 9, 2026
The patch used to sit under the destinations, which put the choice above the
thing being chosen for: a contributor scrolled past three cards to read their
own code, then scrolled back up to act on it. The code is what they opened
the screen to look at and the largest thing on it, so it takes the room, and
where it can go stands beside it — in view the whole time they are reading
rather than something to scroll back to.

This is the shape of an earlier take on the same screen (#6, unmerged since
November), rebuilt on the destinations this app has now.

Three things fell out of the restructure:

The Save and Copy buttons move from floating over the diff into the column
header. Floating was survivable across the full width and covers the first
line of a hunk once the pane is a column.

The columns and the breakpoint are CSS, not inline styles. Flex-wrap was
tried first and cannot express this: wrapping keeps both columns on one flex
line whose height the modal fixes, so the wrapped sidebar had nowhere to go.
Stacking has to change what scrolls as well as what sits where — side by side
the sidebar scrolls alone and the diff stays put; stacked, the pair scrolls
as one and the diff keeps a definite height so it cannot collapse.

And a pre-existing bug the new layout made visible: the diff pane is
height:100% with 12px of padding and content-box sizing, so it was always 24px
taller than its container. Harmless while it spanned the modal and the
overflow fell off the bottom; beside a sidebar it sat on top of the
destinations. Measured, not eyeballed: the pane now ends exactly on its
column, and stacked there are the full 16px between it and what follows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
juanmaguitar added a commit that referenced this pull request Aug 9, 2026
The patch sat under the destinations, which put the choice above the thing being chosen for: a contributor scrolled past three cards to read their own code, then scrolled back up to act on it. The code is what they opened the screen to look at and the largest thing on it, so it takes the room, and where it can go stands beside it.

This is the shape of #6, open since November and unmerged, rebuilt on the destinations this app has now.

Three things fell out of the restructure:

- Save and Copy move from floating over the diff into the column header, where they cannot cover the first line of a hunk. Copy now says that it copied, and says so when it could not.
- The columns and the stacking breakpoint are CSS with a media query. Flex-wrap was tried first and cannot express this — stacking has to change what scrolls as well as what sits where.
- A pre-existing bug the layout exposed: the diff pane is `height: 100%` with padding and content-box sizing, so it was always 24px taller than its container. Harmless while it spanned the modal; beside a sidebar it sat on top of the destinations.

Also fixes a false positive in the staleness guard reported from a real run — it refused a pull request over a file upstream had not touched since 2021. The base blob is now computed from the checkout instead of asked of the fork, which cannot answer for a commit it has not heard of.

Toward #186.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@juanmaguitar

Copy link
Copy Markdown
Collaborator

For the record, since this has been open a while: the flow it proposed has landed in trunk, and its layout with it.

Two things this branch got right that took the new implementation a day of testing against production GitHub to rediscover: device flow is the right sign-in, and repo is the scope it needs — public_repo is documented to cover public-repository writes and is refused by ref creation.

Where the two differ:

this branch trunk now
Token persisted in electron-store main-process memory, forgotten on quit
The pull request pushes a branch, then opens GitHub's compare form in the browser created through the API; the app never leaves

And one thing this branch handled that trunk does not: skipping .github/workflows/ files, which need a scope beyond repo. Filed as #208 — with the note that skipping them silently trades a confusing failure for a pull request quietly missing part of the change, so trunk will likely name the case instead.

Leaving this open rather than closing it; that call belongs to a maintainer. Thanks for the groundwork — the layout in particular was right, and it is what the app ships now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants