Skip to content

Add multi-select and batch Start Review to Review Board#212

Open
dhilgaertner wants to merge 1 commit intomainfrom
feature/crow-192-queue-multiple-items
Open

Add multi-select and batch Start Review to Review Board#212
dhilgaertner wants to merge 1 commit intomainfrom
feature/crow-192-queue-multiple-items

Conversation

@dhilgaertner
Copy link
Copy Markdown
Contributor

Summary

  • Mirrors the Ticket Board's multi-select pattern on the Review Board so reviewers can kick off several PR review sessions in one click.
  • Header adds a Select / Cancel toggle; per-row circle indicator with tap-to-toggle; bottom batch action bar shows selection count plus Start Review (N).
  • Rows already linked to an existing review session render dimmed and are non-selectable, matching how TicketCard disables rows with an active session.
  • New AppState.onBatchStartReview closure; AppDelegate fires one Task { await sessionService.createReviewSession(prURL:) } per selected URL (parallel — each PR clones into its own crow-reviews/<repo>-pr-<n> directory, so concurrent execution is safe).

Closes #192

Test plan

  • make build succeeds.
  • Open the app, switch to the Reviews tab.
  • With ≥2 pending reviews, click Select, tap two rows, confirm the bottom bar reads "2 reviews selected" and Start Review (2).
  • Click Start Review (2) → selection clears, two review sessions are created (watch Console.app for [SessionService] Cloning…).
  • Rows whose PR already has a linked session are dimmed in select mode and cannot be added to the selection.
  • Click Cancel (either header or action bar) → selection clears and per-row "Start Review" buttons return.
  • Ticket Board behavior is unchanged (regression check).

🤖 Generated with Claude Code

@dhilgaertner dhilgaertner requested a review from dgershman as a code owner April 24, 2026 22:18
@dhilgaertner dhilgaertner force-pushed the feature/crow-192-queue-multiple-items branch from d88efea to 9461d63 Compare April 24, 2026 22:20
Copy link
Copy Markdown
Collaborator

@dgershman dgershman left a comment

Choose a reason for hiding this comment

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

Code & Security Review

Critical Issues

None found.

Security Review

Strengths:

  • No user-controlled input reaches shell commands or file system operations in the new code — the batch handler only passes PR URL strings from AppState.filteredReviewRequests (already populated by IssueTracker from GitHub API responses) to sessionService.createReviewSession(prURL:).
  • Closure captures use [weak self] consistently, preventing retain cycles.
  • No secrets or credentials are handled in the changed code.

Concerns:

  • None. The new onBatchStartReview closure mirrors the existing onStartReview pattern, which already validates URLs downstream in SessionService.

Code Quality

Well done:

  • Cleanly mirrors the Ticket Board's established multi-select pattern (isSelectionMode + Set<String> + batch action bar + selectToggleButton), keeping the two boards consistent.
  • isSelectable correctly prevents selecting rows that already have a linked review session.
  • The batchActionBar button filters filteredReviewRequests by selectedRequestIDs before mapping to URLs, so stale IDs (from requests that disappeared mid-selection) are naturally excluded.
  • Proper pluralization in the selection count label.
  • Selection state is correctly cleared on both Cancel and Start Review actions.

Minor observations (non-blocking):

  • If filteredReviewRequests changes while in selection mode (e.g., a PR is merged or the exclude-repos list changes), selectedRequestIDs may contain IDs for rows no longer displayed. The batch action bar count would show the selected count (including invisible ones), but the actual URL list is filtered against current filteredReviewRequests — so no incorrect reviews would be started. This matches the Ticket Board's behavior and is acceptable.
  • The for url in prURLs { Task { ... } } pattern in AppDelegate (line 229) creates independent, unstructured Tasks for each PR. If the user kicks off a large batch, there's no upper bound. In practice, review request lists are small (low tens), so this is fine. If batch sizes grow, a TaskGroup with a concurrency limit would be safer — but not needed now.

Summary Table

Priority Issue
🟢 Green Stale selection IDs if review list changes mid-select (benign — filtered at dispatch time)
🟢 Green Unstructured tasks for batch dispatch (fine for realistic batch sizes)

Recommendation: Approve — clean, consistent implementation that mirrors the established Ticket Board pattern. No security concerns, no logic errors, and the code is well-structured.

Mirrors the Ticket Board pattern: Select/Cancel header toggle,
per-row checkmark indicator with tap-to-toggle, and a batch action
bar that fires createReviewSession for each selected PR in parallel.
Rows already linked to a review session are non-selectable.

Closes #192

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dhilgaertner dhilgaertner force-pushed the feature/crow-192-queue-multiple-items branch from 9461d63 to 310eb75 Compare April 25, 2026 00:23
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.

Queue multiple items for review or action at once

2 participants