Skip to content

Guided changes view#440

Open
cassiozen wants to merge 7 commits into
alltuner:mainfrom
cassiozen:feat/change-view
Open

Guided changes view#440
cassiozen wants to merge 7 commits into
alltuner:mainfrom
cassiozen:feat/change-view

Conversation

@cassiozen
Copy link
Copy Markdown
Contributor

Made this to scratch my own itch, but might be useful for others.

When reviewing changes from multiple agents in parallel, I often loose the context the agent had while writing the code. This PR adds a guided review system where the agent externalizes its reasoning directly into the diff.

The Coding Agent now maintains a review.json file as it works, describing what changed and why, which files to read first, and inline annotations on specific lines (trade-offs, non-obvious decisions, cross-file connections).

The new Changes tab (Cmd+D) renders the diff combined with this data on top of a stacked inline diff viewer using Monaco.

changes.view.mov

cassiozen and others added 7 commits April 14, 2026 17:05
Adds a Changes tab to workstreams showing branch and uncommitted diffs
via an embedded Monaco diff viewer. Includes an AI review guide system
prompt that has the agent maintain a review.json with design decisions,
reading order, and inline annotations.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@davidpoblador davidpoblador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cassiozen — this is an ambitious feature and the execution (Monaco stacked diffs, review.json integration) is cool. I want to merge it, but there are a few concrete things to sort out first. Splitting them into blockers (must fix) and design questions (need your input before I can approve).

Blockers

1. Two tests fail on this branch

Tests/WorkspaceTabStateTests.swift:147
  testStartupStatePreservesRestoredSnapshot
  expected: [info, agent]
  actual:   [info, agent, changes]

Tests/WorkspaceTabStateTests.swift:158
  testStartupStateUsesSavedFixedTabWithoutSnapshot
  expected: [info, agent]
  actual:   [info, agent, changes]

These tests encode the existing contract: Info and Coding Agent are the fixed tabs; everything else is on-demand. The PR changes that contract in TerminalContainerView.swift:193 by making .changes a third fixed tab, and migrates existing snapshots in TerminalContainerView.swift:185-188 to force-insert Changes. If this direction is correct (see design question #1 below), the tests need to be updated and the migration intent made explicit. If not, Changes should be on-demand and these tests should still pass as-is.

2. Docs are out of sync with the shortcut changes

Per CLAUDE.md, keyboard shortcut changes must land in all five places. Currently:

  • Sources/FF2App.swift — New Editor moved to ⌘E, Changes added at ⌘D
  • Sources/Views/TerminalContainerView.swift — ⌘D handled
  • Sources/Views/HelpView.swift — updated
  • README.md — lines 39, 60, 133 still say ⌘O for editor; no mention of ⌘D
  • website/content/docs.md — line 146 still says "⌘O" for editor tabs; Workstream table at line 164 still lists ⌘O; no Changes entry

Please update both. The five localized docs.md copies under website/content/{ca,de,es,sv}/docs.md probably need the same treatment.

3. PR title isn't Conventional Commits

"Guided changes view" has no type: prefix, so release-please will skip it for the changelog. Please retitle to something like feat: add guided changes view before merge (squash-merge uses the PR title as the commit subject).

Design questions I'd like your take on before approving

4. Is Changes a fixed tab or an on-demand tab?

CLAUDE.md says: "Info (⌘1) and Agent (⌘2) tabs always present; terminals/browsers added on demand." The PR makes Changes a third always-present tab. That's a real contract change. Arguments each way:

  • Always present (current PR behavior): Changes is always meaningful for a workstream, consistent placement is friendlier, muscle memory for ⌘D is learnable.
  • On-demand: matches existing Terminal/Browser/Editor pattern, keeps the always-present surface minimal, avoids forcing a new tab into every existing workstream on upgrade.

I lean slightly toward on-demand with easy opt-in (⌘D opens it; ⌘W closes it; don't force it into existing snapshots), but I'd like to hear your reasoning before deciding. @davidpoblador for awareness — this is an architecture decision worth your call.

5. ⌘O → ⌘E for the editor

Freeing ⌘O for nothing (⌘D handles Changes) is a breaking change for existing users' muscle memory. macOS standard is ⌘O = "Open", so ⌘E may actually be the better long-term choice, but it's a decision that deserves to stand on its own rather than ride along with the Changes feature. Could you either revert it (keep editor at ⌘O) or split it into a separate PR so it can be discussed independently?

Nits (not blocking)

  • editor/src/diff.js has a hardcoded "No changes" English string. The Monaco view runs in a WKWebView so the native locale strings don't reach it. Worth passing the translated string in from Swift or at least noting the gap.
  • Individual commit messages are casual ("polish", "cleanup", "fix bug with long streams"). Fine for a squash-merge since the PR title becomes the single commit, but flagging for awareness.

Summary: love the feature, blocked on test failures + doc sync + title. Once those three are cleaned up and the fixed-vs-on-demand question is answered, I'm happy to merge this.

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