From 8d77f06eea389b6076655cc576fd79bdd5d4c664 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Tue, 28 Jul 2026 09:43:08 +0200 Subject: [PATCH 01/13] Review M1: a global Review destination with a real queue Review stops being a per-session sub-tab and becomes a destination that spans repositories. The old shape gated Review off for remote repositories -- but reviewing a remote PR is the primary use case -- and a per-session tab cannot show a queue that spans repos at all. OpenSessionTab.SubTab is now CLAUDE - TERMINAL - EXPLORER, and Cmd-4 is a navigation command that lands on Review scoped to the current session's checkout. Domain: - ReviewScope / ReviewScopeRegistry mint opaque rs_ handles (MCP schema section 0), with grants so a human can hand one session's agent another worktree's scope. Minting is idempotent on identity, because the queue is reassembled on every worktree change and a fresh id per assembly would orphan everything keyed by (scopeId, ...). - ReviewQueueService assembles MINE/AGENTS/REQUESTED/STACK from WorktreeService, GitStatusService and a new GhCliService.listReviewRequests. A failing repository or a missing gh contributes nothing and is logged; the rest of the queue still assembles. Review degrades, never blocks. UI: - ReviewDestinationView (36px title bar, two-row item header, a Host seam where the diff column lands) and ReviewQueueRail (236/206/44px, animated like the Explorer's search rail). Every row is a real focusable Button -- the prototype having zero focusable controls was a defect, not a style. - Sidebar destination row above the tree with a count badge, and a per- worktree finding badge that jumps into Review scoped to that worktree. - Showing Review hides every native terminal, through one writer over three independent conditions, so a modal closing over Review cannot un-hide a terminal through it. Theme and fonts: - Three chrome greys and ten semantic tokens added to BOTH theme sheets. Semantic names stay separate from same-valued existing ones: a MED-risk intent is not a dirty worktree, and collapsing them would make a future palette change to one silently move the other. - All new CSS is structure-only with px font literals, so the interface-size slider scales it. ThemeTokenContractTest now pins both invariants, which nothing guarded before. Testing: - A headless JavaFX harness (TestFX + Monocle), which this repository did not have: view tests show a real Stage, apply the real stylesheets and drive real key events with no window server. headless.geometry is load-bearing -- Monocle's default 1280x800 screen overflows the software pixel buffer for a wider Scene. - ReviewDestinationViewTest covers the keyboard table M1 binds and the focus-traversability rule; verified non-vacuous by mutating clamp-to-wrap and clearing focusTraversable, which failed exactly those two tests. - Queue assembly is tested against real temporary git repositories, including the missing-gh path. ReviewView (the per-session diff tab) is deleted rather than parked; its hunk-row model is kept for the diff column. docs/ui-redesign.md records the structure and eleven known deviations, and flags the five open decisions in section 10 of the handoff rather than inventing answers. Co-Authored-By: Claude Opus 5 (1M context) --- app/build.gradle.kts | 32 + .../java/app/drydock/DrydockApplication.java | 51 +- .../java/app/drydock/git/GhCliService.java | 98 ++ .../java/app/drydock/review/ReviewItem.java | 69 + .../drydock/review/ReviewQueueService.java | 246 ++++ .../java/app/drydock/review/ReviewScope.java | 96 ++ .../drydock/review/ReviewScopeRegistry.java | 206 +++ .../java/app/drydock/ui/MainWorkspace.java | 356 ++++- .../java/app/drydock/ui/OpenSessionTab.java | 94 +- .../app/drydock/ui/RepositorySidebar.java | 119 +- .../java/app/drydock/ui/ShortcutsOverlay.java | 78 +- .../app/drydock/ui/WorkspaceNavigator.java | 17 +- .../ui/review/ReviewDestinationView.java | 349 +++++ .../drydock/ui/review/ReviewQueueRail.java | 357 +++++ .../app/drydock/ui/review/ReviewView.java | 1163 ----------------- app/src/main/resources/app/drydock/ui/app.css | 309 +++++ .../resources/app/drydock/ui/theme-dark.css | 21 + .../resources/app/drydock/ui/theme-light.css | 21 + .../review/ReviewQueueServiceTest.java | 269 ++++ .../review/ReviewScopeRegistryTest.java | 159 +++ .../drydock/ui/ThemeTokenContractTest.java | 101 ++ .../ui/review/ReviewDestinationViewTest.java | 273 ++++ .../review/ReviewQueueRailSelectionTest.java | 48 + .../review/ReviewViewChangeRoutingTest.java | 107 -- docs/ui-redesign.md | 83 +- 25 files changed, 3305 insertions(+), 1417 deletions(-) create mode 100644 app/src/main/java/app/drydock/review/ReviewItem.java create mode 100644 app/src/main/java/app/drydock/review/ReviewQueueService.java create mode 100644 app/src/main/java/app/drydock/review/ReviewScope.java create mode 100644 app/src/main/java/app/drydock/review/ReviewScopeRegistry.java create mode 100644 app/src/main/java/app/drydock/ui/review/ReviewDestinationView.java create mode 100644 app/src/main/java/app/drydock/ui/review/ReviewQueueRail.java delete mode 100644 app/src/main/java/app/drydock/ui/review/ReviewView.java create mode 100644 app/src/test/java/app/drydock/review/ReviewQueueServiceTest.java create mode 100644 app/src/test/java/app/drydock/review/ReviewScopeRegistryTest.java create mode 100644 app/src/test/java/app/drydock/ui/ThemeTokenContractTest.java create mode 100644 app/src/test/java/app/drydock/ui/review/ReviewDestinationViewTest.java create mode 100644 app/src/test/java/app/drydock/ui/review/ReviewQueueRailSelectionTest.java delete mode 100644 app/src/test/java/app/drydock/ui/review/ReviewViewChangeRoutingTest.java diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 1e64576e..ceaf6521 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -93,11 +93,43 @@ dependencies { testImplementation(platform("org.junit:junit-bom:5.11.4")) testImplementation("org.junit.jupiter:junit-jupiter") testRuntimeOnly("org.junit.platform:junit-platform-launcher") + + // Headless JavaFX harness (see the Monocle block in tasks.test). TestFX + // supplies the robot and the node-query DSL; Monocle supplies a Glass + // platform that needs no window server, so view tests run identically on + // a developer's Mac and on a CI runner with no GUI session. hamcrest is + // explicit because testfx-core exposes org.hamcrest.Matcher in its public + // API but only declares the dependency at runtime scope. + testImplementation("org.hamcrest:hamcrest:2.2") + testImplementation("org.testfx:testfx-core:4.0.18") + testImplementation("org.testfx:testfx-junit5:4.0.18") + testRuntimeOnly("org.testfx:openjfx-monocle:21.0.2") } tasks.test { useJUnitPlatform() + // Headless JavaFX. Monocle replaces the platform's Glass backend with one + // that renders to memory, so a test can show a real Stage, apply the real + // stylesheets and drive real key events without a window server -- which + // is what lets the view tests run on the macos-14 CI runner. + // + // Set for the whole test JVM rather than per test class: these are read + // once at toolkit startup, and Gradle runs one JVM for the suite. The + // non-FX tests are unaffected. + // + // headless.geometry is load-bearing: Monocle's default virtual screen is + // 1280x800, and a Scene larger than the screen overflows the software + // pixel buffer (BufferOverflowException from the Prism SW pipeline). The + // Review view's rails only stay expanded above 1320px wide, so the + // virtual screen has to be bigger than the widest scene any test builds. + systemProperty("glass.platform", "Monocle") + systemProperty("monocle.platform", "Headless") + systemProperty("prism.order", "sw") + systemProperty("javafx.headless", "true") + systemProperty("headless.geometry", "1920x1200-32") + + // Inputs for RuntimeImageModuleListTest: the packaged app jar, its runtime // classpath, and the convention-plugin source that declares the jlink // --add-modules list. The test runs jdeps against the jar the packaging diff --git a/app/src/main/java/app/drydock/DrydockApplication.java b/app/src/main/java/app/drydock/DrydockApplication.java index 42444762..7ec72a4c 100644 --- a/app/src/main/java/app/drydock/DrydockApplication.java +++ b/app/src/main/java/app/drydock/DrydockApplication.java @@ -23,6 +23,7 @@ import app.drydock.mcp.McpToolRouter; import app.drydock.mcp.WorkspaceMcpSessionContext; import app.drydock.review.AnnotationStore; +import app.drydock.review.ReviewScopeRegistry; import app.drydock.search.SessionSearchService; import app.drydock.state.JsonApplicationStateRepository; import app.drydock.ui.AppShell; @@ -30,10 +31,10 @@ import app.drydock.ui.MainWorkspace; import app.drydock.ui.RemoteRepositoryModal; import app.drydock.ui.RepositorySidebar; +import app.drydock.ui.review.ReviewDestinationView; import app.drydock.ui.SettingsModal; import app.drydock.ui.SizeSetting; import app.drydock.ui.model.WorkspaceViewModel; -import app.drydock.ui.review.ReviewView; import javafx.application.Application; import javafx.application.Platform; import javafx.scene.control.Alert; @@ -125,6 +126,8 @@ public final class DrydockApplication extends Application { private AppShell appShell; private GitHubService gitHubService; private AnnotationStore annotationStore; + /** Cross-repo review scope handles, shared by the Review destination and the MCP tool router. */ + private ReviewScopeRegistry reviewScopeRegistry; private McpServer mcpServer; private boolean shutdownConfirmed; @@ -197,12 +200,20 @@ private void startOnFxThread(Stage primaryStage) { WorkspaceViewModel viewModel = new WorkspaceViewModel(); viewModel.setSessions(sessionManager.sessions()); + // Cross-repo review scope handles. Owned here rather than by the + // workspace because the MCP tool router addresses scopes too, and + // both must resolve the same handle to the same review. + reviewScopeRegistry = new ReviewScopeRegistry(); + mainWorkspace = new MainWorkspace(sessionManager, agentRegistry, repositoryManager, gitStatusService, - searchService, ghCliService, worktreeService, diffService, changedLineService, annotationStore, - viewModel, primaryStage); + searchService, ghCliService, worktreeService, changedLineService, annotationStore, + reviewScopeRegistry, viewModel, primaryStage); RepositorySidebar sidebar = new RepositorySidebar(repositoryManager, gitStatusService, worktreeService, sessionManager, mainWorkspace, viewModel); + sidebar.setReviewQueueSize(mainWorkspace::reviewQueueSize); + sidebar.setOpenFindingsAt(mainWorkspace::openFindingsAt); + mainWorkspace.setOnReviewQueueChanged(sidebar::refreshReviewBadges); installSessionActivityHooks(activityDir); startMcpServer(stateDir); @@ -220,6 +231,9 @@ private void startOnFxThread(Stage primaryStage) { DEFAULT_SCENE_WIDTH, DEFAULT_SCENE_HEIGHT); mainWorkspace.setThemeProvider(() -> appShell.themeManager().theme()); + // Review's ? button shares the one overlay, so the table stays in + // one place and cannot drift from what is actually bound. + mainWorkspace.setOnShowShortcuts(appShell::showShortcutsOverlay); mainWorkspace.setTerminalFontSizeProvider( () -> repositoryManager.state().ui().terminalFontSize()); // Warms the ghostty config cache for the pair the FIRST opened @@ -496,28 +510,20 @@ public CompletableFuture saveWorktreesDirectory(Optional directory) driver.start(); } - // Diagnostic hook for the Review tab's visual pass: switches the - // selected tab to Review and shows the working-tree diff (the only - // non-empty scope for a session opened in a repository root) after - // . Inert unless -Dapp.drydock.diag.openReview is set. + // Diagnostic hook for the Review destination's visual pass: shows + // Review and lets its queue assemble, after . Inert + // unless -Dapp.drydock.diag.openReview is set. String openReview = System.getProperty("app.drydock.diag.openReview"); if (openReview != null) { long reviewDelayMillis = (long) (Double.parseDouble(openReview.strip()) * 1000); Thread reviewOpener = new Thread(() -> { try { Thread.sleep(reviewDelayMillis); - ReviewView review = onFx(mainWorkspace::diagShowReview); - if (review == null) { - System.out.println("[diag] openReview: no Review view on the selected tab"); - return; - } - // The sub-tab switch and first diff render need a few pulses. - Thread.sleep(2_000); - onFx(() -> { - review.diagSelectWorkingTree(); - return null; - }); - System.out.println("[diag] review opened on the working-tree scope"); + ReviewDestinationView review = onFx(mainWorkspace::diagShowReview); + // The queue assembles off-thread (git + gh per repo). + Thread.sleep(3_000); + System.out.println("[diag] review opened with " + + onFx(() -> review.diagItems().size()) + " queue items"); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } catch (RuntimeException e) { @@ -661,9 +667,14 @@ private void installGlobalShortcuts(RepositorySidebar sidebar) { boolean cmd = event.isShortcutDown(); if (event.getCode() == KeyCode.ESCAPE) { + // Topmost-first (Review spec section 5): the modal, then the + // Review destination, then the tab selection. if (appShell.modalLayer().isShowingModal()) { appShell.modalLayer().close(); event.consume(); + } else if (!inTextInput && mainWorkspace.isReviewShowing()) { + mainWorkspace.hideReview(); + event.consume(); } else if (!inTextInput) { mainWorkspace.showPicker(); event.consume(); @@ -711,7 +722,7 @@ private void installGlobalShortcuts(RepositorySidebar sidebar) { mainWorkspace.showExplorerSubTab(); event.consume(); } else if (cmd && event.getCode() == KeyCode.DIGIT4) { - mainWorkspace.showReviewSubTab(); + mainWorkspace.showReviewForCurrentSession(); event.consume(); } else if (cmd && event.getCode() == KeyCode.R) { mainWorkspace.activeSessionId().flatMap(id -> sessionManager.sessions().stream() diff --git a/app/src/main/java/app/drydock/git/GhCliService.java b/app/src/main/java/app/drydock/git/GhCliService.java index 82746e2e..ace9fda8 100644 --- a/app/src/main/java/app/drydock/git/GhCliService.java +++ b/app/src/main/java/app/drydock/git/GhCliService.java @@ -6,6 +6,7 @@ import app.drydock.state.json.JsonParseException; import app.drydock.state.json.JsonParser; import app.drydock.state.json.JsonValue; +import app.drydock.state.json.JsonValue.JsonArray; import app.drydock.state.json.JsonValue.JsonNumber; import app.drydock.state.json.JsonValue.JsonObject; import app.drydock.state.json.JsonValue.JsonString; @@ -17,8 +18,10 @@ import java.nio.file.Files; import java.nio.file.Path; import java.time.Duration; +import java.util.ArrayList; import java.util.List; import java.util.Locale; +import java.util.Objects; import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutorService; @@ -53,6 +56,26 @@ public record PrInfo(int number, PrLifecycle state, Optional url) { public enum PrLifecycle { OPEN, MERGED, CLOSED, UNKNOWN } } + /** + * One row of {@code gh pr list --search "review-requested:@me"} (Review + * spec §4.1, the REQUESTED queue group). Carries what the queue rail + * renders plus the refs the checkout gate needs. + */ + public record ReviewRequest(int number, String title, String headRefName, String baseRefName, + Optional author, Optional url, int changedFiles, + boolean draft) { + public ReviewRequest { + Objects.requireNonNull(title, "title"); + Objects.requireNonNull(headRefName, "headRefName"); + Objects.requireNonNull(baseRefName, "baseRefName"); + Objects.requireNonNull(author, "author"); + Objects.requireNonNull(url, "url"); + } + } + + /** How many review-requested PRs one {@code gh pr list} call may return. */ + private static final int REVIEW_REQUEST_LIMIT = 50; + private final ExecutorService executor; private final boolean ownsExecutor; private volatile Optional cachedExecutable; @@ -130,6 +153,81 @@ Optional viewPrBlocking(Path root, String branch) { } } + /** + * The open PRs in {@code root}'s repository that ask this user for a + * review -- the REQUESTED group of the Review queue. + * + *

An empty list means "nothing to tell you", and covers every + * no-information case alike: {@code gh} missing, not authenticated, the + * repository having no GitHub remote, or genuinely no requests. Review + * degrades rather than blocks (spec §6), so no caller distinguishes + * them; each is logged.

+ */ + public CompletableFuture> listReviewRequests(Path root) { + return CompletableFuture.supplyAsync(() -> listReviewRequestsBlocking(root), executor); + } + + List listReviewRequestsBlocking(Path root) { + Path gh = locate().orElse(null); + if (gh == null) { + return List.of(); + } + ProcessResult result = runIn(root, List.of(gh.toString(), "pr", "list", + "--search", "review-requested:@me", + "--state", "open", + "--limit", String.valueOf(REVIEW_REQUEST_LIMIT), + "--json", "number,title,headRefName,baseRefName,author,url,changedFiles,isDraft")); + if (result == null) { + return List.of(); + } + if (result.exitCode() != 0) { + LOG.log(Level.DEBUG, "gh pr list (review-requested) in " + root + " exited " + result.exitCode() + + (result.stderr().isBlank() ? "" : ": " + ProcessRunner.excerpt(result.stderr()))); + return List.of(); + } + try { + if (!(JsonParser.parse(result.stdout()) instanceof JsonArray array)) { + return List.of(); + } + List requests = new ArrayList<>(); + for (JsonValue element : array.elements()) { + parseReviewRequest(element).ifPresent(requests::add); + } + return List.copyOf(requests); + } catch (JsonParseException | NumberFormatException e) { + LOG.log(Level.DEBUG, "Unparseable gh pr list output", e); + return List.of(); + } + } + + /** + * A row missing any of the fields the queue needs is skipped rather than + * failing the whole list: one malformed PR must not empty the REQUESTED + * group. + */ + private static Optional parseReviewRequest(JsonValue element) { + if (!(element instanceof JsonObject obj) + || !(obj.get("number") instanceof JsonNumber number) + || !(obj.get("headRefName") instanceof JsonString head) + || !(obj.get("baseRefName") instanceof JsonString base)) { + return Optional.empty(); + } + int prNumber = number.asInt(); + if (prNumber <= 0) { + return Optional.empty(); + } + String title = obj.get("title") instanceof JsonString t ? t.value() : head.value(); + Optional author = obj.get("author") instanceof JsonObject a + && a.get("login") instanceof JsonString login + ? Optional.of(login.value()) + : Optional.empty(); + Optional url = obj.get("url") instanceof JsonString u ? Optional.of(u.value()) : Optional.empty(); + int changedFiles = obj.get("changedFiles") instanceof JsonNumber c ? c.asInt() : 0; + boolean draft = obj.get("isDraft") instanceof JsonValue.JsonBoolean d && d.value(); + return Optional.of(new ReviewRequest(prNumber, title, head.value(), base.value(), + author, url, changedFiles, draft)); + } + private static PrInfo.PrLifecycle lifecycleOf(String raw) { return switch (raw.toUpperCase(Locale.ROOT)) { case "OPEN" -> PrInfo.PrLifecycle.OPEN; diff --git a/app/src/main/java/app/drydock/review/ReviewItem.java b/app/src/main/java/app/drydock/review/ReviewItem.java new file mode 100644 index 00000000..5353bd51 --- /dev/null +++ b/app/src/main/java/app/drydock/review/ReviewItem.java @@ -0,0 +1,69 @@ +package app.drydock.review; + +import java.util.Objects; +import java.util.Optional; + +/** + * One row of the Review queue: a {@link ReviewScope} plus what the rail + * renders for it (spec §4.1). + * + *

The open-finding count is deliberately not a field. It is + * derived from the findings that actually exist for the scope, so an item + * whose reviewer has never run shows no count at all -- rather than a + * confident zero, which would read as "reviewed, nothing found".

+ */ +public record ReviewItem(ReviewScope scope, Group group, String title, String subtitle) { + + /** The queue's four sections, in rail order. */ + public enum Group { + /** The human's own uncommitted work and local branches. */ + MINE("MINE"), + /** Worktrees an agent authored. */ + AGENTS("AGENTS"), + /** PRs where a review was requested from this user. */ + REQUESTED("REQUESTED"), + /** Dependent PR stacks. */ + STACK("STACK"); + + private final String label; + + Group(String label) { + this.label = label; + } + + public String label() { + return label; + } + } + + public ReviewItem { + Objects.requireNonNull(scope, "scope"); + Objects.requireNonNull(group, "group"); + Objects.requireNonNull(title, "title"); + Objects.requireNonNull(subtitle, "subtitle"); + } + + /** The rail's icon column glyph, keyed to the scope kind (spec §4.1). */ + public String icon() { + return switch (scope.kind()) { + case WORKING_TREE -> "❯_"; + case BRANCH -> "⎇"; + case WORKTREE -> "◫"; + case PR -> "◧"; + case STACK -> "⛁"; + }; + } + + /** The collapsed rail's tooltip: the full description a 44px column cannot show. */ + public String tooltip() { + String session = scope.sessionId() + .map(id -> " · session bound") + .orElse(" · no session yet"); + return title + " — " + subtitle + session; + } + + /** The PR number, when this item is one (used by the checkout gate). */ + public Optional prNumber() { + return scope.pr().map(ReviewScope.PullRequestRef::number); + } +} diff --git a/app/src/main/java/app/drydock/review/ReviewQueueService.java b/app/src/main/java/app/drydock/review/ReviewQueueService.java new file mode 100644 index 00000000..1d7fd123 --- /dev/null +++ b/app/src/main/java/app/drydock/review/ReviewQueueService.java @@ -0,0 +1,246 @@ +package app.drydock.review; + +import app.drydock.domain.ManagedSessionId; +import app.drydock.git.GhCliService; +import app.drydock.git.GitBranchState; +import app.drydock.git.GitStatus; +import app.drydock.git.GitStatusService; +import app.drydock.git.WorktreeService; + +import java.lang.System.Logger; +import java.lang.System.Logger.Level; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.function.Function; +import java.util.stream.Collectors; + +/** + * Assembles the Review queue (spec §4.1) from what drydock already knows: + * {@code git worktree list}, {@code git status}, and {@code gh pr list + * --search "review-requested:@me"}. + * + *

Every scope it produces is minted through {@link ReviewScopeRegistry}, + * so the same worktree keeps the same handle across rescans and everything + * keyed by {@code (scopeId, …)} survives a background refresh.

+ * + *

Nothing here blocks and nothing here fails the queue. + * A repository whose git commands fail, or a missing {@code gh}, contributes + * no items and is logged; the rest of the queue still assembles. Review + * degrades, never blocks (spec §6).

+ */ +public final class ReviewQueueService { + + private static final Logger LOG = System.getLogger(ReviewQueueService.class.getName()); + + /** A repository to scan: its main checkout and the name the queue shows. */ + public record RepositoryTarget(Path root, String displayName) { + public RepositoryTarget { + Objects.requireNonNull(root, "root"); + Objects.requireNonNull(displayName, "displayName"); + } + } + + /** + * Resolves the managed session running in a checkout, if any. Supplied + * by the workspace rather than read here, so this service depends on no + * session bookkeeping -- and so tests can drive the MINE/AGENTS split + * without a {@code SessionManager}. + */ + @FunctionalInterface + public interface SessionLookup { + Optional sessionAt(Path checkoutRoot); + } + + /** + * Where the REQUESTED group comes from -- in the app, {@code + * GhCliService::listReviewRequests}. A narrow function rather than the + * service itself, because "{@code gh} is absent or failing" is a state + * the queue must survive and therefore a state tests have to be able to + * produce, and {@code GhCliService} discovers its executable from + * {@code PATH} with no seam to take it away. + */ + @FunctionalInterface + public interface ReviewRequestSource { + CompletableFuture> forRepository(Path repositoryRoot); + } + + private final WorktreeService worktreeService; + private final GitStatusService gitStatusService; + private final ReviewRequestSource reviewRequests; + private final ReviewScopeRegistry scopeRegistry; + + public ReviewQueueService(WorktreeService worktreeService, GitStatusService gitStatusService, + ReviewRequestSource reviewRequests, ReviewScopeRegistry scopeRegistry) { + this.worktreeService = Objects.requireNonNull(worktreeService, "worktreeService"); + this.gitStatusService = Objects.requireNonNull(gitStatusService, "gitStatusService"); + this.reviewRequests = Objects.requireNonNull(reviewRequests, "reviewRequests"); + this.scopeRegistry = Objects.requireNonNull(scopeRegistry, "scopeRegistry"); + } + + /** + * Assembles the whole queue, grouped MINE · AGENTS · REQUESTED · STACK. + * Repositories are scanned concurrently; the future completes when all + * of them have either produced items or failed. + * + *

Handles for scopes that are no longer in the queue are revoked, so + * a worktree that was removed stops being addressable over MCP.

+ */ + public CompletableFuture> assemble(List repositories, + SessionLookup sessions) { + Objects.requireNonNull(sessions, "sessions"); + List>> perRepository = repositories.stream() + .map(repository -> assembleRepository(repository, sessions)) + .toList(); + return CompletableFuture.allOf(perRepository.toArray(CompletableFuture[]::new)) + .thenApply(ignored -> { + List items = perRepository.stream() + .flatMap(future -> future.join().stream()) + .sorted(Comparator.comparingInt(item -> item.group().ordinal())) + .toList(); + revokeDepartedScopes(items); + return items; + }); + } + + /** + * One repository's items. Worktrees, the main checkout's status and the + * review-requested PRs are fetched concurrently, then combined -- the + * PR list is the slow one (a network call), and it must not serialize + * behind two local git commands. + */ + private CompletableFuture> assembleRepository(RepositoryTarget repository, + SessionLookup sessions) { + CompletableFuture> worktrees = + worktreeService.list(repository.root()).exceptionally(failure -> { + LOG.log(Level.DEBUG, "Could not list worktrees of " + repository.root(), failure); + return List.of(); + }); + CompletableFuture> status = + gitStatusService.getStatus(repository.root()) + .>thenApply(Optional::of) + .exceptionally(failure -> { + LOG.log(Level.DEBUG, "Could not read status of " + repository.root(), failure); + return Optional.empty(); + }); + CompletableFuture> requests = + reviewRequests.forRepository(repository.root()).exceptionally(failure -> { + LOG.log(Level.DEBUG, "Could not list review requests for " + repository.root(), failure); + return List.of(); + }); + + return worktrees.thenCombine(status, PartialScan::new) + .thenCombine(requests, (scan, prs) -> build(repository, sessions, scan, prs)); + } + + private record PartialScan(List worktrees, Optional status) { } + + private List build(RepositoryTarget repository, SessionLookup sessions, + PartialScan scan, List requests) { + String base = baseBranchOf(scan.status()); + List items = new ArrayList<>(); + + // MINE -- the main checkout's uncommitted work, when there is any. + if (scan.status().map(GitStatus::dirty).orElse(false)) { + ReviewScope scope = scopeRegistry.mint(ReviewScopeRegistry.spec( + ReviewScope.Kind.WORKING_TREE, repository.root(), Optional.of(repository.root()), + base, base, Optional.empty(), sessions.sessionAt(repository.root()))); + items.add(new ReviewItem(scope, ReviewItem.Group.MINE, "Working tree", + repository.displayName() + " · uncommitted changes")); + } + + // MINE / AGENTS -- one item per non-main worktree. A worktree with a + // bound session is an agent's; one without is the human's own. + for (WorktreeService.Worktree worktree : scan.worktrees()) { + if (worktree.mainCheckout() || worktree.prunable()) { + continue; + } + String head = worktree.branch().orElse(worktree.detached() ? "(detached)" : "(no branch)"); + Optional session = sessions.sessionAt(worktree.path()); + ReviewScope scope = scopeRegistry.mint(ReviewScopeRegistry.spec( + ReviewScope.Kind.WORKTREE, repository.root(), Optional.of(worktree.path()), + base, head, Optional.empty(), session)); + items.add(new ReviewItem(scope, + session.isPresent() ? ReviewItem.Group.AGENTS : ReviewItem.Group.MINE, + head, repository.displayName() + " · vs " + base)); + } + + // REQUESTED -- PRs asking this user for a review. A PR whose head + // branch is already checked out in a worktree is that worktree's + // item; listing it twice would split its findings across two scopes. + Set checkedOutBranches = scan.worktrees().stream() + .map(WorktreeService.Worktree::branch) + .flatMap(Optional::stream) + .collect(Collectors.toUnmodifiableSet()); + for (GhCliService.ReviewRequest request : requests) { + if (checkedOutBranches.contains(request.headRefName())) { + continue; + } + ReviewScope scope = scopeRegistry.mint(ReviewScopeRegistry.spec( + ReviewScope.Kind.PR, repository.root(), Optional.empty(), + request.baseRefName(), request.headRefName(), + Optional.of(new ReviewScope.PullRequestRef(request.number(), request.url())), + Optional.empty())); + items.add(new ReviewItem(scope, ReviewItem.Group.REQUESTED, + "PR #" + request.number() + " " + request.headRefName(), + prSubtitle(repository, request))); + } + + return List.copyOf(items); + } + + private static String prSubtitle(RepositoryTarget repository, GhCliService.ReviewRequest request) { + StringBuilder subtitle = new StringBuilder(repository.displayName()); + request.author().ifPresent(author -> subtitle.append(" · @").append(author)); + if (request.changedFiles() > 0) { + subtitle.append(" · ").append(request.changedFiles()) + .append(request.changedFiles() == 1 ? " file" : " files"); + } + if (request.draft()) { + subtitle.append(" · draft"); + } + return subtitle.append(" · not checked out").toString(); + } + + /** + * The branch every worktree in this repository is reviewed against: the + * main checkout's current branch, which is what {@code DiffService}'s + * {@code BASE} scope already diffs against. A detached main checkout has + * no name to show, so {@code HEAD} stands in. + */ + private static String baseBranchOf(Optional status) { + return status.map(GitStatus::branch) + .filter(GitBranchState.OnBranch.class::isInstance) + .map(branch -> ((GitBranchState.OnBranch) branch).name()) + .orElse("HEAD"); + } + + /** + * Revokes handles for scopes that are no longer in the queue. Without + * this a pruned worktree's handle would stay addressable over MCP for + * the life of the process (schema §0: the handle is revoked when the + * item leaves the queue). + */ + private void revokeDepartedScopes(List items) { + Set live = items.stream() + .map(item -> item.scope().id()) + .collect(Collectors.toUnmodifiableSet()); + for (ReviewScope scope : scopeRegistry.scopes()) { + if (!live.contains(scope.id())) { + scopeRegistry.revoke(scope.id()); + } + } + } + + /** Exposed for the sidebar's per-worktree {@code ◨n} badge lookup. */ + public Function> scopeByWorktree() { + return path -> scopeRegistry.scopes().stream() + .filter(scope -> scope.worktree().filter(path::equals).isPresent()) + .findFirst(); + } +} diff --git a/app/src/main/java/app/drydock/review/ReviewScope.java b/app/src/main/java/app/drydock/review/ReviewScope.java new file mode 100644 index 00000000..9a204428 --- /dev/null +++ b/app/src/main/java/app/drydock/review/ReviewScope.java @@ -0,0 +1,96 @@ +package app.drydock.review; + +import app.drydock.domain.ManagedSessionId; + +import java.nio.file.Path; +import java.util.Objects; +import java.util.Optional; + +/** + * One reviewable thing, addressed by an opaque handle rather than by a + * session id (Review MCP schema §0). + * + *

The Review destination is cross-repo, but {@code McpSessionRegistry} + * binds an MCP caller to exactly one session. A scope handle is + * therefore the unit an agent addresses: {@link ReviewScopeRegistry} mints + * it, the human grants it to a session, and every review MCP call carries + * its {@link #id()}.

+ * + *

{@link #worktree()} is empty only for a PR that has not been checked + * out. That is the whole reason the checkout gate exists: no worktree means + * no session, which means no agent caller and therefore no reviewer -- the + * diff can still be read, but nothing can be asked of an agent.

+ */ +public record ReviewScope( + String id, + Kind kind, + Path repoRoot, + Optional worktree, + String base, + String head, + Optional pr, + Optional sessionId) { + + /** What is being reviewed; drives the queue icon and how the diff is resolved. */ + public enum Kind { + /** An agent-authored (or human) checkout under {@code git worktree}. */ + WORKTREE, + /** Uncommitted changes in a checkout ({@code ❯_}). */ + WORKING_TREE, + /** A branch against its base, in the main checkout. */ + BRANCH, + /** A GitHub pull request, checked out or not. */ + PR, + /** A stack of dependent PRs. */ + STACK + } + + /** The GitHub side of a {@link Kind#PR} scope. */ + public record PullRequestRef(int number, Optional url) { + public PullRequestRef { + if (number <= 0) { + throw new IllegalArgumentException("PR number must be positive: " + number); + } + Objects.requireNonNull(url, "url"); + } + } + + public ReviewScope { + Objects.requireNonNull(id, "id"); + Objects.requireNonNull(kind, "kind"); + Objects.requireNonNull(repoRoot, "repoRoot"); + Objects.requireNonNull(worktree, "worktree"); + Objects.requireNonNull(base, "base"); + Objects.requireNonNull(head, "head"); + Objects.requireNonNull(pr, "pr"); + Objects.requireNonNull(sessionId, "sessionId"); + if (id.isBlank()) { + throw new IllegalArgumentException("scope id must not be blank"); + } + if (kind == Kind.PR && pr.isEmpty()) { + throw new IllegalArgumentException("a PR scope must carry a PullRequestRef"); + } + } + + /** + * The checkout a diff is read from: the worktree when there is one, + * otherwise the repository's main checkout. A not-checked-out PR + * therefore still resolves to a directory git can be run in -- what it + * does not resolve to is a directory containing the PR's code. + */ + public Path diffRoot() { + return worktree.orElse(repoRoot); + } + + /** This scope with {@code session} bound (the checkout gate's result, M5). */ + public ReviewScope withSession(ManagedSessionId session) { + return new ReviewScope(id, kind, repoRoot, worktree, base, head, pr, + Optional.ofNullable(session)); + } + + /** This scope with {@code newWorktree} as its checkout. */ + public ReviewScope withWorktree(Path newWorktree) { + return new ReviewScope(id, kind, repoRoot, Optional.ofNullable(newWorktree), base, head, pr, + sessionId); + } +} diff --git a/app/src/main/java/app/drydock/review/ReviewScopeRegistry.java b/app/src/main/java/app/drydock/review/ReviewScopeRegistry.java new file mode 100644 index 00000000..254b68fb --- /dev/null +++ b/app/src/main/java/app/drydock/review/ReviewScopeRegistry.java @@ -0,0 +1,206 @@ +package app.drydock.review; + +import app.drydock.domain.ManagedSessionId; + +import java.nio.file.Path; +import java.security.SecureRandom; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Consumer; + +/** + * Mints, resolves and revokes {@link ReviewScope} handles, and records which + * sessions may address which scopes (Review MCP schema §0). + * + *

{@code McpSessionRegistry} binds an MCP caller to one session; the + * Review destination spans repositories. This registry is the bridge: an + * agent may touch the scope its own session is bound to, plus any + * scope a human explicitly granted it by pressing "Run review". That grant + * is what lets a session's agent review a worktree that is not its own.

+ * + *

Minting is idempotent on identity -- kind, repository + * root, worktree, base, head and PR number. The queue is reassembled every + * time repositories or worktrees change, and a fresh id per assembly would + * break everything keyed by {@code (scopeId, id)}: findings, threads, + * drafts and verdicts would all be orphaned by a background rescan. The id + * a scope was first minted with therefore survives until it is explicitly + * revoked (the item left the queue).

+ * + *

Thread-safe: the queue service assembles off the FX thread, the MCP + * router resolves on its own executor, and the UI reads on the FX + * thread.

+ */ +public final class ReviewScopeRegistry { + + /** Crockford base32, so ids are opaque, case-insensitive and free of look-alike glyphs. */ + private static final char[] BASE32 = "0123456789ABCDEFGHJKMNPQRSTVWXYZ".toCharArray(); + + private static final SecureRandom RANDOM = new SecureRandom(); + + /** Identity of a scope, independent of its minted id and of any bound session. */ + private record Identity(ReviewScope.Kind kind, Path repoRoot, Optional worktree, + String base, String head, Optional pr) { + static Identity of(ReviewScope scope) { + return new Identity(scope.kind(), scope.repoRoot(), scope.worktree(), scope.base(), + scope.head(), scope.pr().map(ReviewScope.PullRequestRef::number)); + } + } + + private final Map byId = new ConcurrentHashMap<>(); + private final Map idByIdentity = new ConcurrentHashMap<>(); + private final Map> grants = new ConcurrentHashMap<>(); + private final List> listeners = new CopyOnWriteArrayList<>(); + + /** + * Mints a handle for {@code spec}, or returns the existing handle when a + * scope with the same identity is already registered. The {@code id} + * carried by {@code spec} is ignored -- callers describe what they want + * reviewed and this registry owns identity (see the class note on + * idempotence). + * + *

An existing handle is updated in place from {@code spec}: + * a rescan that discovers the scope now has a bound session must not + * leave the registry serving the sessionless value it minted first.

+ */ + public ReviewScope mint(ReviewScope spec) { + Objects.requireNonNull(spec, "spec"); + Identity identity = Identity.of(spec); + String id = idByIdentity.computeIfAbsent(identity, key -> newId()); + ReviewScope scope = new ReviewScope(id, spec.kind(), spec.repoRoot(), spec.worktree(), + spec.base(), spec.head(), spec.pr(), spec.sessionId()); + ReviewScope previous = byId.put(id, scope); + if (!scope.equals(previous)) { + notifyChanged(id); + } + return scope; + } + + /** + * Builds a spec with a placeholder id. {@link #mint} replaces it, so no + * caller has to invent one. + */ + public static ReviewScope spec(ReviewScope.Kind kind, Path repoRoot, Optional worktree, + String base, String head, Optional pr, + Optional sessionId) { + return new ReviewScope("rs_pending", kind, repoRoot, worktree, base, head, pr, sessionId); + } + + public Optional byId(String id) { + return id == null ? Optional.empty() : Optional.ofNullable(byId.get(id)); + } + + /** Every live scope, in mint order. */ + public List scopes() { + List live = new ArrayList<>(); + for (Map.Entry entry : new LinkedHashMap<>(idByIdentity).entrySet()) { + ReviewScope scope = byId.get(entry.getValue()); + if (scope != null) { + live.add(scope); + } + } + return List.copyOf(live); + } + + /** + * Drops the handle and every grant against it -- the item left the + * queue. Findings keyed by this id are not touched here: the store owns + * their lifetime, and revoking a handle for an item that later comes + * back (a worktree that was pruned and re-added) mints a new id by + * design, because it is genuinely not the same review any more. + */ + public void revoke(String id) { + if (id == null) { + return; + } + ReviewScope removed = byId.remove(id); + grants.remove(id); + idByIdentity.values().removeIf(id::equals); + if (removed != null) { + notifyChanged(id); + } + } + + /** Lets {@code sessionId}'s agent address {@code scopeId} ("Run review" on someone else's worktree). */ + public void grant(String scopeId, ManagedSessionId sessionId) { + Objects.requireNonNull(sessionId, "sessionId"); + if (!byId.containsKey(scopeId)) { + throw new IllegalArgumentException("No such review scope: " + scopeId); + } + grants.computeIfAbsent(scopeId, key -> ConcurrentHashMap.newKeySet()).add(sessionId); + notifyChanged(scopeId); + } + + /** Withdraws a grant made by {@link #grant}; the scope's own bound session is unaffected. */ + public void revokeGrant(String scopeId, ManagedSessionId sessionId) { + Set granted = grants.get(scopeId); + if (granted != null && granted.remove(sessionId)) { + notifyChanged(scopeId); + } + } + + /** + * Whether {@code sessionId} may address {@code scopeId}: either the + * scope is bound to that session, or a human granted it. An unknown + * scope is never addressable -- the MCP router turns that into a tool + * error rather than a silent empty result. + */ + public boolean isAddressableBy(String scopeId, ManagedSessionId sessionId) { + ReviewScope scope = byId.get(scopeId); + if (scope == null || sessionId == null) { + return false; + } + if (scope.sessionId().filter(sessionId::equals).isPresent()) { + return true; + } + return grants.getOrDefault(scopeId, Set.of()).contains(sessionId); + } + + /** Sessions explicitly granted {@code scopeId} (excluding its own bound session). */ + public Set grantsFor(String scopeId) { + return Set.copyOf(grants.getOrDefault(scopeId, Set.of())); + } + + /** + * Subscribes to mint/update/revoke/grant notifications; the returned + * runnable unsubscribes. Listeners may be called from any thread. + */ + public Runnable addChangeListener(Consumer listener) { + Objects.requireNonNull(listener, "listener"); + listeners.add(listener); + return () -> listeners.remove(listener); + } + + private void notifyChanged(String scopeId) { + for (Consumer listener : listeners) { + listener.accept(scopeId); + } + } + + /** + * A time-ordered, opaque id in the shape the schema documents + * ({@code rs_01J…}): 48 bits of epoch milliseconds followed by 40 random + * bits, Crockford base32. Sortable by mint time, unguessable enough that + * an id leaking into a transcript is not an address an agent can walk. + */ + private static String newId() { + long millis = System.currentTimeMillis() & 0xFFFF_FFFF_FFFFL; + long random = ((long) RANDOM.nextInt()) & 0xFF_FFFF_FFFFL; + StringBuilder out = new StringBuilder("rs_"); + appendBase32(out, millis, 10); + appendBase32(out, random, 8); + return out.toString(); + } + + private static void appendBase32(StringBuilder out, long value, int chars) { + for (int shift = (chars - 1) * 5; shift >= 0; shift -= 5) { + out.append(BASE32[(int) ((value >>> shift) & 0x1F)]); + } + } +} diff --git a/app/src/main/java/app/drydock/ui/MainWorkspace.java b/app/src/main/java/app/drydock/ui/MainWorkspace.java index c1d8baba..6e52465b 100644 --- a/app/src/main/java/app/drydock/ui/MainWorkspace.java +++ b/app/src/main/java/app/drydock/ui/MainWorkspace.java @@ -17,7 +17,6 @@ import app.drydock.domain.UiTheme; import app.drydock.domain.WorkspaceUiState; import app.drydock.git.ChangedLineService; -import app.drydock.git.DiffService; import app.drydock.git.GhCliService; import app.drydock.git.GitBranchState; import app.drydock.git.GitStatusService; @@ -27,10 +26,14 @@ import app.drydock.mcp.WorkspaceMcpSessionContext; import app.drydock.process.SshCommandBuilder; import app.drydock.review.AnnotationStore; +import app.drydock.review.AnnotationStatus; +import app.drydock.review.ReviewQueueService; +import app.drydock.review.ReviewScope; +import app.drydock.review.ReviewScopeRegistry; import app.drydock.search.SessionSearchService; import app.drydock.ui.explorer.DiffOverlay; import app.drydock.ui.explorer.SessionExplorerView; -import app.drydock.ui.review.ReviewView; +import app.drydock.ui.review.ReviewDestinationView; import app.drydock.ui.model.WorkspaceViewModel; import app.drydock.terminal.TerminalFactory; import app.drydock.terminal.api.TerminalHostView; @@ -76,6 +79,7 @@ import java.util.List; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletableFuture; @@ -133,7 +137,6 @@ public final class MainWorkspace extends BorderPane implements WorkspaceNavigato private final WorktreeService worktreeService; private final SessionSearchService searchService; private final GhCliService ghCliService; - private final DiffService diffService; private final ChangedLineService changedLineService; private final AnnotationStore annotationStore; private final WorkspaceViewModel viewModel; @@ -149,6 +152,38 @@ public final class MainWorkspace extends BorderPane implements WorkspaceNavigato private final StackPane centerStack; private final MenuButton newTabButton = new MenuButton("＋"); + /** + * The Review destination (Review handoff §1). A scene-graph view like + * the Explorer, so showing it hides every native terminal -- see {@link + * #setReviewShowing}. Built once and kept in {@link #centerStack}; + * rebuilding it per visit would drop the queue selection and the + * remembered rail-collapse state. + */ + private final ReviewDestinationView reviewDestination; + private final ReviewScopeRegistry reviewScopeRegistry; + private final ReviewQueueService reviewQueueService; + + /** + * True while the Review destination owns the centre. Tracked separately + * from {@link #terminalsObscured}: a modal opening and closing over + * Review must not un-hide the terminals underneath it. + */ + private boolean reviewShowing; + + /** Fires when the Review queue changes, so the sidebar can restyle its badge. */ + private Runnable onReviewQueueChanged = () -> { }; + + /** Shows the shared shortcuts overlay (wired by DrydockApplication, which owns the modal layer). */ + private Runnable onShowShortcuts = () -> { }; + + /** + * A checkout whose queue item should be selected once the in-flight + * refresh lands. {@code ⌘4} arrives before the queue exists on a cold + * start, and dropping the request there would land the user on whatever + * item happened to sort first. + */ + private Path pendingReviewSelection; + /** * The per-worktree empty pane (worktree handoff: "No session in this * worktree yet"), shown while an UNOPENED worktree is selected in the @@ -244,9 +279,9 @@ public final class MainWorkspace extends BorderPane implements WorkspaceNavigato public MainWorkspace(SessionManager sessionManager, AgentRegistry agentRegistry, RepositoryManager repositoryManager, GitStatusService gitStatusService, SessionSearchService searchService, - GhCliService ghCliService, WorktreeService worktreeService, DiffService diffService, + GhCliService ghCliService, WorktreeService worktreeService, ChangedLineService changedLineService, AnnotationStore annotationStore, - WorkspaceViewModel viewModel, Stage stage) { + ReviewScopeRegistry reviewScopeRegistry, WorkspaceViewModel viewModel, Stage stage) { this.sessionManager = sessionManager; this.agentRegistry = agentRegistry; this.repositoryManager = repositoryManager; @@ -254,9 +289,11 @@ public MainWorkspace(SessionManager sessionManager, AgentRegistry agentRegistry, this.worktreeService = worktreeService; this.searchService = searchService; this.ghCliService = ghCliService; - this.diffService = diffService; this.changedLineService = changedLineService; this.annotationStore = annotationStore; + this.reviewScopeRegistry = reviewScopeRegistry; + this.reviewQueueService = new ReviewQueueService(worktreeService, gitStatusService, + ghCliService::listReviewRequests, reviewScopeRegistry); this.viewModel = viewModel; this.stage = stage; this.worktreeLifecycle = new WorktreeLifecycleController(sessionManager, gitStatusService, @@ -282,7 +319,11 @@ public MainWorkspace(SessionManager sessionManager, AgentRegistry agentRegistry, } }); - centerStack = new StackPane(tabPane, emptyState, newTabButton); + reviewDestination = new ReviewDestinationView(new ReviewHost()); + reviewDestination.setVisible(false); + reviewDestination.setManaged(false); + + centerStack = new StackPane(tabPane, emptyState, newTabButton, reviewDestination); StackPane.setAlignment(newTabButton, Pos.TOP_RIGHT); StackPane.setMargin(newTabButton, new Insets(10, 10, 0, 0)); setCenter(centerStack); @@ -291,9 +332,7 @@ public MainWorkspace(SessionManager sessionManager, AgentRegistry agentRegistry, if (newTab != null) { clearUnopenedWorktreeState(); } - for (OpenSessionTab open : openTabs.values()) { - open.setVisible(!terminalsObscured && open.tab == newTab); - } + updateTerminalVisibility(); updatePickerVisibility(); // Tab selection only moves the active-row highlight; the model // turns this into activeSessionChanged, never a tree rebuild. @@ -326,10 +365,23 @@ public void structureChanged() { } }); + // Finding counts drive the queue's per-item badge and the sidebar's + // ◨n badges, and the MCP tool router writes to the store from its own + // executor -- so the counts have to be told, not polled. No + // unsubscribe: this workspace and the store share the application's + // lifetime. + annotationStore.addChangeListener(id -> Platform.runLater(this::refreshReviewCounts)); + exitWatcher.setCycleCount(Animation.INDEFINITE); exitWatcher.play(); } + /** Re-reads the finding counts the Review queue and the sidebar badges render. */ + private void refreshReviewCounts() { + reviewDestination.refreshCounts(); + onReviewQueueChanged.run(); + } + /** Pushes the manager's current session snapshot into the view model (FX thread; no-op if unchanged). */ private void publishSessions() { viewModel.setSessions(sessionManager.sessions()); @@ -389,7 +441,9 @@ private void populateNewTabMenu() { * tabs at all it fills the pane. */ private void updatePickerVisibility() { - boolean show = tabPane.getSelectionModel().getSelectedItem() == null; + // Review owns the whole centre while it is showing; the no-session + // placeholder underneath must not paint through the gaps. + boolean show = !reviewShowing && tabPane.getSelectionModel().getSelectedItem() == null; boolean hasTabs = !tabPane.getTabs().isEmpty(); StackPane.setMargin(emptyState, new Insets(hasTabs ? TAB_STRIP_HEIGHT : 0, 0, 0, 0)); boolean unopenedShowing = show && unopenedWorktreeState != null; @@ -575,9 +629,228 @@ public void showExplorerSubTab() { currentlySelected().ifPresent(open -> open.showSubTab(OpenSessionTab.SubTab.EXPLORER)); } - /** ⌘4: switches the selected session tab to its Review sub-tab. */ - public void showReviewSubTab() { - currentlySelected().ifPresent(open -> open.showSubTab(OpenSessionTab.SubTab.REVIEW)); + /** + * {@code ⌘4} from anywhere: navigates to the Review destination, scoped + * to the selected session's checkout when there is one (Review handoff + * §2). A navigation command, not a view switch -- Review spans + * repositories, so it cannot live inside one session's tab. + */ + public void showReviewForCurrentSession() { + Optional checkout = currentlySelected() + .map(OpenSessionTab::sessionId) + .flatMap(id -> sessionManager.sessions().stream() + .filter(session -> session.id().equals(id)) + .findFirst()) + .map(session -> session.worktreeRoot().orElseGet(() -> + repositoryFor(session).map(Repository::root).orElse(null))); + checkout.filter(Objects::nonNull).ifPresentOrElse(this::showReviewForCheckout, + this::showReview); + } + + // ---- Review destination (Review handoff sections 1 & 2) ----------------- + + /** + * Shows the Review destination, keeping whatever the queue already had + * selected. Review is a scene-graph view stacked over the tab pane, so + * this must hide every native terminal exactly as the Explorer swap does + * -- the ghostty surfaces paint above the whole JavaFX scene and would + * otherwise sit on top of it. + */ + @Override + public void showReview() { + setReviewShowing(true); + } + + /** + * {@code ⌘4} and the sidebar's {@code ◨n} badge: shows Review with the + * item for {@code checkoutRoot} selected. The queue is reassembled + * asynchronously, so the selection is applied both now (for a scope that + * is already minted) and again when the refresh lands. + */ + @Override + public void showReviewForCheckout(Path checkoutRoot) { + // Recorded before the refresh is kicked off, so the completion + // handler cannot land on a null request. + pendingReviewSelection = checkoutRoot; + setReviewShowing(true); + selectReviewScopeFor(checkoutRoot); + } + + /** Whether Review currently owns the centre (the Esc unwind order asks). */ + public boolean isReviewShowing() { + return reviewShowing; + } + + /** Leaves Review, restoring the tab pane and the selected tab's terminal. */ + public void hideReview() { + setReviewShowing(false); + } + + /** + * The one writer of {@link #reviewShowing}. Restoring the terminals goes + * through {@link #updateTerminalVisibility} and then re-runs geometry on + * the next pulse: the centre swap only invalidates the placeholder's + * bounds at the next layout pass, so the native frame would otherwise + * track stale bounds (the same ordering {@code OpenSessionTab.showSubTab} + * relies on). + */ + private void setReviewShowing(boolean showing) { + if (reviewShowing == showing) { + if (showing) { + reviewDestination.onShown(); + } + return; + } + reviewShowing = showing; + reviewDestination.setVisible(showing); + reviewDestination.setManaged(showing); + tabPane.setVisible(!showing); + newTabButton.setVisible(!showing); + newTabButton.setManaged(!showing); + updatePickerVisibility(); + updateTerminalVisibility(); + if (showing) { + reviewDestination.onShown(); + } else { + Platform.runLater(() -> currentlySelected().ifPresent(OpenSessionTab::updateGeometryNow)); + } + } + + /** + * Reassembles the queue off the FX thread and pushes it into the view. + * Remote repositories are skipped: they have no local checkout for git + * to run in, and their root is a virtual placeholder that must never be + * resolved against the filesystem. + */ + private void refreshReviewQueue() { + List local = repositoryManager.repositories().stream() + .filter(repository -> !repository.isRemote()) + .toList(); + List targets = local.stream() + .map(repository -> new ReviewQueueService.RepositoryTarget( + repository.root(), repository.displayName())) + .toList(); + reviewQueueService.assemble(targets, this::sessionAtCheckout) + .whenComplete((items, failure) -> Platform.runLater(() -> { + if (failure != null) { + LOG.log(Level.WARNING, "Could not assemble the Review queue", failure); + return; + } + reviewDestination.setItems(items, local.size()); + if (pendingReviewSelection != null) { + selectReviewScopeFor(pendingReviewSelection); + pendingReviewSelection = null; + } + onReviewQueueChanged.run(); + })); + } + + /** Selects the queue item whose scope is checked out at {@code checkoutRoot}, if it exists yet. */ + private void selectReviewScopeFor(Path checkoutRoot) { + reviewScopeRegistry.scopes().stream() + .filter(scope -> scope.worktree().filter(checkoutRoot::equals).isPresent()) + .findFirst() + .ifPresent(scope -> reviewDestination.selectScope(scope.id())); + } + + /** The managed session running in {@code checkoutRoot}, if any (the queue's MINE/AGENTS split). */ + private Optional sessionAtCheckout(Path checkoutRoot) { + return sessionManager.sessions().stream() + .filter(session -> session.worktreeRoot().filter(checkoutRoot::equals).isPresent()) + .map(ManagedAgentSession::id) + .findFirst(); + } + + /** Open findings for {@code scope} -- empty until a reviewer has run against it (spec section 4.1). */ + private Optional openFindingsFor(ReviewScope scope) { + Optional session = scope.sessionId(); + if (session.isEmpty()) { + return Optional.empty(); + } + long open = annotationStore.forSession(session.get()).stream() + .filter(annotation -> annotation.status() == AnnotationStatus.OPEN) + .count(); + return Optional.of((int) open); + } + + /** {@code running} / {@code idle} for a scope's bound session; empty when none is bound. */ + private Optional reviewSessionState(ReviewScope scope) { + return scope.sessionId() + .flatMap(id -> sessionManager.sessions().stream() + .filter(session -> session.id().equals(id)) + .findFirst()) + .map(session -> session.status() == SessionStatus.RUNNING ? "running" : "idle"); + } + + /** How many Review queue items exist right now (the sidebar destination's badge). */ + public int reviewQueueSize() { + return reviewDestination.diagItems().size(); + } + + /** Notified after every queue reassembly, so the sidebar can re-render its badge. */ + public void setOnReviewQueueChanged(Runnable handler) { + this.onReviewQueueChanged = handler == null ? () -> { } : handler; + } + + /** Shows the shared shortcuts overlay (Review's {@code ?} button); the app shell owns the modal layer. */ + public void setOnShowShortcuts(Runnable handler) { + this.onShowShortcuts = handler == null ? () -> { } : handler; + } + + /** Open findings for the worktree at {@code checkoutRoot} (the sidebar's per-worktree ◨n badge). */ + public Optional openFindingsAt(Path checkoutRoot) { + return reviewScopeRegistry.scopes().stream() + .filter(scope -> scope.worktree().filter(checkoutRoot::equals).isPresent()) + .findFirst() + .flatMap(this::openFindingsFor) + .filter(count -> count > 0); + } + + /** The Review view's window onto the workspace (see {@link ReviewDestinationView.Host}). */ + private final class ReviewHost implements ReviewDestinationView.Host { + + @Override + public void refreshQueue() { + refreshReviewQueue(); + } + + @Override + public void openSession(ManagedSessionId sessionId) { + OpenSessionTab open = openTabs.get(sessionId); + if (open == null) { + // Not open: resume it, which opens a tab and leaves Review. + sessionManager.sessions().stream() + .filter(session -> session.id().equals(sessionId)) + .findFirst() + .ifPresent(MainWorkspace.this::resumeSession); + hideReview(); + return; + } + hideReview(); + tabPane.getSelectionModel().select(open.tab); + } + + @Override + public Optional bodyFor(ReviewScope scope) { + // M2 returns the diff column here; until then the view renders + // its own placeholder, which is what the empty Optional means. + return Optional.empty(); + } + + @Override + public Optional openFindings(ReviewScope scope) { + return openFindingsFor(scope); + } + + @Override + public Optional sessionState(ReviewScope scope) { + return reviewSessionState(scope); + } + + @Override + public void showShortcuts() { + onShowShortcuts.run(); + } } /** ⌘⇧]: selects the next session tab (wraps around). */ @@ -612,9 +885,21 @@ public void setOnToggleSidebar(Runnable handler) { /** Hides/restores every native terminal view while a modal is showing (see {@link #terminalsObscured}). */ public void setTerminalsObscured(boolean obscured) { this.terminalsObscured = obscured; + updateTerminalVisibility(); + } + + /** + * The single rule for whether a tab's native terminal paints: it must be + * the selected tab, no modal may be up, and Review must not own the + * centre. Three independent conditions with one writer -- the bug this + * prevents is a modal closing over Review and un-hiding the terminal + * through it, because the native view overlays the whole scene. + */ + private void updateTerminalVisibility() { Tab selected = tabPane.getSelectionModel().getSelectedItem(); + boolean allowed = !terminalsObscured && !reviewShowing; for (OpenSessionTab open : openTabs.values()) { - open.setVisible(!obscured && open.tab == selected); + open.setVisible(allowed && open.tab == selected); } } @@ -1138,7 +1423,7 @@ private void attachOpenedSession(OpenSessionTab placeholderTab, SessionOpenResul placeholderTab.setDisplayName(opened.session().displayName()); placeholderTab.setStatus(opened.session().status()); openTabs.put(opened.session().id(), placeholderTab); - placeholderTab.setVisible(!terminalsObscured + placeholderTab.setVisible(!terminalsObscured && !reviewShowing && tabPane.getSelectionModel().getSelectedItem() == placeholderTab.tab); opened.session().worktreeRoot().ifPresent(root -> worktreeLifecycle.setupWorktreeHeader(placeholderTab, opened.session().id(), root)); @@ -1342,12 +1627,13 @@ public void diagTypeInExplorer(String text) { } /** - * Diagnostic-only: switches the selected tab to the Review sub-tab and - * returns its view, so the automated visual pass can screenshot a - * populated Review pane -- the FX layer has no headless harness. + * Diagnostic-only: shows the Review destination and returns it, so the + * automated visual pass can screenshot a populated queue -- the FX layer + * has no headless harness. */ - public ReviewView diagShowReview() { - return currentlySelected().map(OpenSessionTab::diagShowReview).orElse(null); + public ReviewDestinationView diagShowReview() { + showReview(); + return reviewDestination; } // ---- Exit watcher -------------------------------------------------------- @@ -1575,13 +1861,14 @@ private OpenSessionTab createOpenSessionTab(ManagedSessionId sessionId, String d openTab.setOnPreviousSessionTab(this::selectPreviousSessionTab); openTab.setOnNextSessionTab(this::selectNextSessionTab); openTab.setOnToggleSidebar(() -> onToggleSidebar.run()); + openTab.setOnShowReview(this::showReviewForCurrentSession); if (repository.map(Repository::isRemote).orElse(false)) { - // Explorer (local file search) and Review (local diffs) have no - // local checkout to operate on -- spec: Feature gating. Leaving - // both factories unset (OpenSessionTab disables their toggle - // buttons for a remote tab; see its constructor) instead of - // wiring anything against the remote's placeholder root. + // The Explorer's file search has no local checkout to operate on + // -- spec: Feature gating. Leaving its factory unset + // (OpenSessionTab disables the toggle button for a remote tab; + // see its constructor) instead of wiring anything against the + // remote's placeholder root. repository.ifPresent(repo -> gitStatusService.getStatus(GitTarget.of(repo)) .whenComplete((status, failure) -> Platform.runLater(() -> { if (failure == null && status.branch() instanceof GitBranchState.OnBranch onBranch) { @@ -1597,23 +1884,6 @@ private OpenSessionTab createOpenSessionTab(ManagedSessionId sessionId, String d openExplorers.put(openTab, explorer); return explorer; }); - // openTab.sessionId() rather than the constructor parameter: the - // factory runs lazily (first Review open), by which time a created - // session's tab has adopted the real id -- annotations must key on it. - repository.ifPresent(repo -> openTab.setReviewFactory(() -> new ReviewView( - openTab.sessionId(), searchRoot, repo.root(), diffService, changedLineService, gitStatusService, - annotationStore, openTab::sendPrompt, new ReviewView.ExplorerBridge() { - @Override - public void openFileAtLine(Path relativeFile, int line) { - openTab.openExplorerAt(relativeFile, line); - } - - @Override - public void searchText(String token) { - openTab.searchInExplorer(token); - } - }))); - // Branch of the session's own checkout: for a worktree session the // search root IS the worktree, so its branch (not the main // checkout's) fills the header/sub-tab context lines. The main diff --git a/app/src/main/java/app/drydock/ui/OpenSessionTab.java b/app/src/main/java/app/drydock/ui/OpenSessionTab.java index b504fb2e..7c36179d 100644 --- a/app/src/main/java/app/drydock/ui/OpenSessionTab.java +++ b/app/src/main/java/app/drydock/ui/OpenSessionTab.java @@ -10,7 +10,6 @@ import app.drydock.terminal.api.TerminalRuntime; import app.drydock.terminal.api.TerminalSurface; import app.drydock.ui.explorer.SessionExplorerView; -import app.drydock.ui.review.ReviewView; import javafx.animation.PauseTransition; import javafx.application.Platform; import javafx.css.PseudoClass; @@ -81,8 +80,15 @@ final class OpenSessionTab { private static final long SHELL_CLOSE_GRACE_MILLIS = 3000; private static final long SHELL_CLOSE_POLL_MILLIS = 100; - /** The four views a session tab can show in its content area (design handoff "Session Explorer" / "Diff Review"). */ - enum SubTab { CLAUDE, TERMINAL, EXPLORER, REVIEW } + /** + * The three views a session tab can show in its content area (design + * handoff "Session Explorer"). Review used to be a fourth: it is now a + * global destination (Review handoff §1), because a per-session tab + * cannot show a queue that spans repositories, and because it gated + * review off for remote repositories -- while reviewing a remote PR is + * the primary use case. {@code ⌘4} navigates there instead. + */ + enum SubTab { CLAUDE, TERMINAL, EXPLORER } /** One lazily-created native trio for the shell sub-tab (runtime + host, themed by MainWorkspace). */ record ShellTerminal(TerminalRuntime runtime, TerminalHostView host) { } @@ -104,19 +110,15 @@ record ShellTerminal(TerminalRuntime runtime, TerminalHostView host) { } private final Label statusLabel = new Label("Starting session..."); private final BorderPane content = new BorderPane(); - // -- Bottom Terminal/Explorer/Review sub-tab bar (handoff "Session Explorer" / "Diff Review") -- + // -- Bottom Claude/Terminal/Explorer sub-tab bar (handoff "Session Explorer") -- private final ToggleButton claudeSubTabButton = new ToggleButton("✳ Claude"); private final ToggleButton terminalSubTabButton = new ToggleButton("❯_ Terminal"); private final ToggleButton explorerSubTabButton = new ToggleButton("▤ Explorer"); - private final ToggleButton reviewSubTabButton = new ToggleButton("◨ Review"); private final Label subTabContext = new Label(); private SubTab activeSubTab = SubTab.CLAUDE; /** Built on first switch to Explorer, via {@link #setExplorerFactory}. */ private Region explorerView; private Supplier explorerFactory; - /** Built on first switch to Review, via {@link #setReviewFactory}. */ - private Region reviewView; - private Supplier reviewFactory; // -- Ephemeral shell Terminal sub-tab (never persisted; created on first switch) -- /** Supplies a fresh shell runtime+host whose wakeup drives the argument (the shell bridge's tickAndDraw). */ @@ -173,6 +175,8 @@ record ShellTerminal(TerminalRuntime runtime, TerminalHostView host) { } private Runnable onPreviousSessionTab = () -> { }; private Runnable onNextSessionTab = () -> { }; private Runnable onToggleSidebar = () -> { }; + /** ⌘4 intercepted inside the terminal: navigate to Review, scoped to this session. */ + private Runnable onShowReview = () -> { }; private String displayName; @@ -291,23 +295,15 @@ private Region buildSubTabBar() { explorerSubTabButton.setFocusTraversable(false); explorerSubTabButton.setOnAction(e -> showSubTab(SubTab.EXPLORER)); - reviewSubTabButton.getStyleClass().add("session-subtab"); - reviewSubTabButton.setFocusTraversable(false); - reviewSubTabButton.setOnAction(e -> showSubTab(SubTab.REVIEW)); - - // Remote repositories have no local checkout for Explorer (local file - // search) or Review (local diffs) to operate on -- spec: Feature - // gating. MainWorkspace never wires their factories for a remote - // tab, so disable the toggles up front instead of letting a click - // silently no-op in showSubTab. + // A remote repository has no local checkout for the Explorer's file + // search to operate on -- spec: Feature gating. MainWorkspace never + // wires its factory for a remote tab, so disable the toggle up front + // instead of letting a click silently no-op in showSubTab. if (isRemote) { explorerSubTabButton.setDisable(true); - reviewSubTabButton.setDisable(true); explorerSubTabButton.setTooltip(new Tooltip("Not available for remote repositories")); - reviewSubTabButton.setTooltip(new Tooltip("Not available for remote repositories")); } else { explorerSubTabButton.setTooltip(new Tooltip("Explorer (⌘3)")); - reviewSubTabButton.setTooltip(new Tooltip("Review (⌘4)")); } subTabContext.getStyleClass().add("session-subtab-context"); @@ -315,7 +311,7 @@ private Region buildSubTabBar() { Region spacer = new Region(); HBox.setHgrow(spacer, Priority.ALWAYS); - HBox bar = new HBox(4, claudeSubTabButton, terminalSubTabButton, explorerSubTabButton, reviewSubTabButton, spacer, subTabContext); + HBox bar = new HBox(4, claudeSubTabButton, terminalSubTabButton, explorerSubTabButton, spacer, subTabContext); bar.setAlignment(Pos.CENTER_LEFT); bar.getStyleClass().add("session-subtab-bar"); return bar; @@ -326,11 +322,6 @@ void setExplorerFactory(Supplier factory) { this.explorerFactory = factory; } - /** Supplies the Review view on first use (MainWorkspace wires this; it knows the session's checkout + services). */ - void setReviewFactory(Supplier factory) { - this.reviewFactory = factory; - } - /** Supplies a fresh shell runtime+host on first switch to the Terminal sub-tab (MainWorkspace wires this). */ void setShellTerminalProvider(Function provider) { this.shellTerminalProvider = provider; @@ -347,9 +338,9 @@ void setShellCommand(String command) { } /** - * Explorer bridge for the Review tab (design handoff section C - * "Explorer integration"): builds the Explorer if needed, switches to - * it, and opens {@code relativeFile} at a 1-based line. + * Explorer bridge for the Review destination's {@code ⤢} (design handoff + * section C "Explorer integration"): builds the Explorer if needed, + * switches to it, and opens {@code relativeFile} at a 1-based line. */ void openExplorerAt(Path relativeFile, int line) { showSubTab(SubTab.EXPLORER); @@ -365,13 +356,7 @@ void diagTypeInExplorer(String text) { } } - /** Diagnostic-only (see MainWorkspace.diagShowReview): switches to the Review sub-tab and returns its view. */ - ReviewView diagShowReview() { - showSubTab(SubTab.REVIEW); - return reviewView instanceof ReviewView review ? review : null; - } - - /** Explorer bridge for the Review tab: switches to the Explorer and runs a text search for {@code token}. */ + /** Explorer bridge for Review: switches to the Explorer and runs a text search for {@code token}. */ void searchInExplorer(String token) { showSubTab(SubTab.EXPLORER); if (explorerView instanceof SessionExplorerView explorer) { @@ -385,8 +370,8 @@ SubTab activeSubTab() { /** * Switches between the native-surface sub-tabs (Claude, Terminal) and - * the scene-graph ones (Explorer, Review). The native views overlay the - * scene, so showing Explorer/Review must both swap the center node AND + * the scene-graph one (Explorer). The native views overlay the + * scene, so showing the Explorer must both swap the center node AND * hide the native hosts (else they keep painting over the view); * switching to a native sub-tab restores its placeholder center first * and re-runs geometry after the layout pass so the native frame tracks @@ -404,8 +389,8 @@ void showSubTab(SubTab subTab) { focusActiveNativeSubTab(); return; } - if (subTab == SubTab.EXPLORER || subTab == SubTab.REVIEW) { - Region view = subTab == SubTab.EXPLORER ? explorerViewOrBuild() : reviewViewOrBuild(); + if (subTab == SubTab.EXPLORER) { + Region view = explorerViewOrBuild(); if (view == null) { // Build failed: undo the button selection, stay put. selectSubTabButton(activeSubTab); @@ -440,6 +425,19 @@ void showSubTab(SubTab subTab) { } } + /** + * Re-runs the active native surface's geometry. Called after the + * workspace swaps the centre back from the Review destination: the swap + * only invalidates the placeholder's bounds at the next layout pass, so + * the native frame would otherwise keep tracking stale bounds. + */ + void updateGeometryNow() { + TerminalBridge active = activeSubTab == SubTab.TERMINAL ? shellBridge : bridge; + if (active != null) { + active.updateGeometry(); + } + } + /** Refocuses whichever native terminal (Claude or shell) the active sub-tab shows, if any. */ void focusActiveNativeSubTab() { if (activeSubTab == SubTab.CLAUDE) { @@ -453,7 +451,6 @@ private void selectSubTabButton(SubTab subTab) { claudeSubTabButton.setSelected(subTab == SubTab.CLAUDE); terminalSubTabButton.setSelected(subTab == SubTab.TERMINAL); explorerSubTabButton.setSelected(subTab == SubTab.EXPLORER); - reviewSubTabButton.setSelected(subTab == SubTab.REVIEW); } /** @@ -510,7 +507,9 @@ private void runShortcut(Shortcut shortcut) { case CLAUDE_SUB_TAB -> showSubTab(SubTab.CLAUDE); case TERMINAL_SUB_TAB -> showSubTab(SubTab.TERMINAL); case EXPLORER_SUB_TAB -> showSubTab(SubTab.EXPLORER); - case REVIEW_SUB_TAB -> showSubTab(SubTab.REVIEW); + // ⌘4 is a navigation command now, not a view switch: it selects + // the global Review destination, scoped to this session. + case REVIEW_SUB_TAB -> onShowReview.run(); case PREVIOUS_SESSION_TAB -> onPreviousSessionTab.run(); case NEXT_SESSION_TAB -> onNextSessionTab.run(); case TOGGLE_SIDEBAR -> onToggleSidebar.run(); @@ -524,12 +523,6 @@ private Region explorerViewOrBuild() { return explorerView; } - private Region reviewViewOrBuild() { - if (reviewView == null && reviewFactory != null) { - reviewView = reviewFactory.get(); - } - return reviewView; - } private HBox buildTabGraphic(Optional repository) { tabRepoLabel.getStyleClass().add("tab-repo-label"); @@ -777,6 +770,11 @@ void setOnToggleSidebar(Runnable handler) { this.onToggleSidebar = handler == null ? () -> { } : handler; } + /** ⌘4 target: the workspace navigates to the Review destination for this session's checkout. */ + void setOnShowReview(Runnable handler) { + this.onShowReview = handler == null ? () -> { } : handler; + } + /** * Immediate visual feedback while the session's graceful close runs * (up to the multi-second Ctrl+D grace period): without it the close diff --git a/app/src/main/java/app/drydock/ui/RepositorySidebar.java b/app/src/main/java/app/drydock/ui/RepositorySidebar.java index 006e0542..4bac4ffb 100644 --- a/app/src/main/java/app/drydock/ui/RepositorySidebar.java +++ b/app/src/main/java/app/drydock/ui/RepositorySidebar.java @@ -63,6 +63,7 @@ import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -72,6 +73,8 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; +import java.util.function.Function; +import java.util.function.IntSupplier; /** * The repository sidebar, rebuilt to the design handoff (README section 2) @@ -113,6 +116,29 @@ public final class RepositorySidebar extends VBox { private final ExternalEditorLauncher editorLauncher = new ExternalEditorLauncher(); private final TextField filterField = new TextField(); + + /** + * The Review destination row, pinned above the repository tree (Review + * handoff section 2). A real focus-traversable {@link Button}: the + * destination has to be reachable without the mouse, exactly like the + * tree below it. + */ + private final Button reviewDestinationButton = new Button(); + private final Label reviewDestinationBadge = new Label(); + + /** How many items the Review queue holds right now (drives the badge). */ + private IntSupplier reviewQueueSize = () -> 0; + + /** Open findings for a worktree checkout, if any -- the per-row ◨n badge. */ + private Function> openFindingsAt = path -> Optional.empty(); + + /** + * The finding counts the tree was last rendered with. A queue refresh + * that changes no count must not rebuild the tree: rebuild-the-world is + * a last resort (AGENTS.md), and Review reassembles its queue every time + * the destination is shown. + */ + private Map renderedFindingCounts = Map.of(); private final TreeItem treeRoot = new TreeItem<>(); private final TreeView tree = new TreeView<>(treeRoot); private final Label footerLabel = new Label(); @@ -214,6 +240,23 @@ public RepositorySidebar(RepositoryManager repositoryManager, GitStatusService g VBox header = new VBox(addButton, filterField); header.getStyleClass().add("sidebar-header"); + // -- Review destination, above the tree ------------------------------ + Label reviewGlyph = new Label("◨"); + reviewGlyph.getStyleClass().add("sidebar-destination-glyph"); + Label reviewLabel = new Label("Review"); + reviewLabel.getStyleClass().add("sidebar-destination-label"); + reviewDestinationBadge.getStyleClass().add("sidebar-destination-badge"); + Region reviewSpacer = new Region(); + HBox.setHgrow(reviewSpacer, Priority.ALWAYS); + HBox reviewRow = new HBox(8, reviewGlyph, reviewLabel, reviewSpacer, reviewDestinationBadge); + reviewRow.setAlignment(Pos.CENTER_LEFT); + reviewDestinationButton.setGraphic(reviewRow); + reviewDestinationButton.getStyleClass().add("sidebar-destination"); + reviewDestinationButton.setMaxWidth(Double.MAX_VALUE); + reviewDestinationButton.setTooltip(new Tooltip("Review — local changes, agent worktrees and PRs (⌘4)")); + reviewDestinationButton.setOnAction(e -> navigator.showReview()); + refreshReviewBadges(); + // -- Tree ----------------------------------------------------------- tree.getStyleClass().add("repo-tree"); tree.setShowRoot(false); @@ -238,7 +281,7 @@ public RepositorySidebar(RepositoryManager repositoryManager, GitStatusService g HBox footer = new HBox(footerDot, footerLabel); footer.getStyleClass().add("sidebar-footer"); - getChildren().addAll(header, tree, footer); + getChildren().addAll(header, reviewDestinationButton, tree, footer); // Keep the displayed list in sync with EVERY repository mutation, // not just the ones initiated by this sidebar's own handlers. The @@ -637,6 +680,54 @@ private void pruneRowCaches() { lockedBucketExpanded.retainAll(repoIds); } + /** Supplies the Review queue's item count for the destination badge. */ + public void setReviewQueueSize(IntSupplier supplier) { + this.reviewQueueSize = supplier == null ? () -> 0 : supplier; + refreshReviewBadges(); + } + + /** Supplies a worktree checkout's open-finding count for its {@code ◨n} badge. */ + public void setOpenFindingsAt(Function> lookup) { + this.openFindingsAt = lookup == null ? path -> Optional.empty() : lookup; + refreshReviewBadges(); + } + + /** + * Re-reads the Review counts: the destination's item badge, and the + * per-worktree {@code ◨n} badges the tree cells draw. Called after every + * queue reassembly. + */ + public void refreshReviewBadges() { + int items = reviewQueueSize.getAsInt(); + reviewDestinationBadge.setText(items == 0 ? "" : String.valueOf(items)); + reviewDestinationBadge.setVisible(items > 0); + reviewDestinationBadge.setManaged(items > 0); + + Map current = currentFindingCounts(); + if (!current.equals(renderedFindingCounts)) { + renderedFindingCounts = current; + rebuildTree(); + } + } + + /** Every worktree path the tree can show, mapped to its open-finding count. */ + private Map currentFindingCounts() { + Map counts = new LinkedHashMap<>(); + for (Repository repository : repositoryManager.repositories()) { + for (SidebarNode child : childNodesFor(repository)) { + Path checkout = switch (child) { + case SidebarNode.SessionNode node -> node.session().worktreeRoot().orElse(null); + case SidebarNode.UnopenedWorktreeNode node -> node.worktree().path(); + default -> null; + }; + if (checkout != null) { + openFindingsAt.apply(checkout).ifPresent(count -> counts.put(checkout, count)); + } + } + } + return Map.copyOf(counts); + } + /** Re-renders the one row backed by {@code worktreeRoot} (a worktree session row or an unopened row). */ private void updateWorktreeRow(Path worktreeRoot) { for (TreeItem repoItem : treeRoot.getChildren()) { @@ -1375,6 +1466,28 @@ private String repoCountsText(Repository repository) { return counts.toString(); } + /** + * The {@code ◨n} badge (Review handoff section 2): that worktree's + * open findings, and a jump into Review filtered to it. Absent + * rather than zero when no reviewer has run -- a confident zero would + * read as "reviewed, nothing found". + */ + private Optional