Skip to content

feat: parallel review generation + improved onboarding#76

Merged
oddur merged 11 commits intomainfrom
feat/improved-onboarding
Apr 16, 2026
Merged

feat: parallel review generation + improved onboarding#76
oddur merged 11 commits intomainfrom
feat/improved-onboarding

Conversation

@oddur
Copy link
Copy Markdown
Owner

@oddur oddur commented Apr 16, 2026

Summary

  • Two-phase parallel review generation: opt-in mode that splits review into a planner step (topic assignment) then fires all slide writers in parallel with scoped context and narrative consistency
  • Guided onboarding: first-run dialog walks users through selecting repos to watch, replacing the old welcome hero
  • Configurable max PRs per repo: dropdown in Settings (default 10)
  • GitHub diff size limit fix: falls back to assembling from file patches when full diff exceeds 20k lines

Parallel Review Architecture

  1. Planner — lightweight call with hunk index only → outputs topic assignments, story arc, and narrative briefs
  2. Writers — all topics fire simultaneously via Promise.all() → each gets scoped context (only relevant hunks + files) plus the story arc, its narrative brief, and dependency briefs for continuity
  3. Assembly — merges slides in planner order, catches unassigned hunks, renders syntax highlighting

Behind feature flag: Settings > Parallel review (off by default).

Test plan

  • Enable Parallel review in Settings, generate a review — verify slides have narrative consistency
  • Generate a review for a large PR (20+ files) — verify diff-too-large fallback works
  • Delete preferences.json, restart — verify onboarding repo setup dialog appears after auth
  • Verify max PRs per repo limits proactive polling
  • Verify single-shot mode still works with parallel review disabled

oddur and others added 2 commits April 16, 2026 16:24
After dismissing the first-run welcome hero, a dialog guides users through
selecting repos to automatically watch. Selecting repos enables proactive
mode and saves the watched repos list. Users can skip and configure later
in Settings. Replay onboarding also re-triggers the repo setup step.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a maxPrsPerRepo preference that limits how many open PRs are
auto-reviewed per watched repo. Shown as a dropdown in Settings under
the proactive mode section. Defaults to 10 (down from the previous
hardcoded 30).

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

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a first-run onboarding dialog to help users pick watched repos immediately after auth, and introduces a new preference to cap how many open PRs per watched repo are auto-reviewed in proactive mode.

Changes:

  • Introduces OnboardingRepoSetup dialog for repo search + selection and wires it into first-run flow.
  • Adds maxPrsPerRepo to preferences, a Settings control to edit it, and uses it when listing watched-repo PRs.
  • Updates proactive polling to respect the per-repo PR cap.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/pages/HomePage.tsx Opens the new repo-setup onboarding on first run and persists watched repos/proactive mode on completion.
components/OnboardingRepoSetup.tsx New onboarding dialog: repo search autocomplete + chip selection + explanatory copy.
lib/types.ts Extends Preferences with maxPrsPerRepo.
src/main.ts Adds default maxPrsPerRepo and passes it to watched-repo PR listing.
components/SettingsDialog.tsx Adds a dropdown to configure “Max PRs per repo” under proactive mode settings.

Comment on lines +13 to +20
export function OnboardingRepoSetup({ open, onComplete, onSkip }: Props) {
const [query, setQuery] = useState('');
const [suggestions, setSuggestions] = useState<RepoSearchResult[]>([]);
const [selectedRepos, setSelectedRepos] = useState<string[]>([]);
const [loading, setLoading] = useState(false);
const debounceRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const inputRef = useRef<HTMLInputElement>(null);

Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

Because this component stays mounted and is controlled via the open prop, selectedRepos/query/suggestions will persist across closes/reopens. If the dialog can be reopened later (e.g. via a replay action), users may see stale selections. Consider resetting the local state when open transitions from false → true.

Copilot uses AI. Check for mistakes.
Comment on lines +21 to +34
const search = useCallback((q: string) => {
if (debounceRef.current) clearTimeout(debounceRef.current);
if (q.trim().length < 2) {
setSuggestions([]);
return;
}
setLoading(true);
debounceRef.current = setTimeout(() => {
void window.electronAPI.searchRepos(q.trim()).then((results) => {
setSuggestions(results.filter((r) => !selectedRepos.includes(r.fullName)));
setLoading(false);
});
}, 300);
}, [selectedRepos]);
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

search() starts a debounced async request but doesn’t clean up the pending timer (or guard against an in-flight promise) when the component closes/unmounts. This can lead to state updates after unmount and leave loading stuck true. Consider clearing the timeout in an effect cleanup and using a cancelled/request-id guard; also ensure loading is reset in a .catch/.finally path.

Copilot uses AI. Check for mistakes.
Comment on lines +84 to +89
onKeyDown={(e) => {
if (e.key === 'Enter' && query.includes('/')) {
e.preventDefault();
addRepo(query.trim());
}
}}
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

The Enter-to-add path only checks query.includes('/'), so inputs like owner/repo/extra will be accepted and later parsed incorrectly (split('/') elsewhere only takes the first two segments). Validate the repo ref more strictly (e.g., exactly one / and no whitespace) before calling addRepo.

Copilot uses AI. Check for mistakes.
Comment thread components/OnboardingRepoSetup.tsx Outdated
Comment on lines +28 to +32
debounceRef.current = setTimeout(() => {
void window.electronAPI.searchRepos(q.trim()).then((results) => {
setSuggestions(results.filter((r) => !selectedRepos.includes(r.fullName)));
setLoading(false);
});
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

searchRepos(...).then(...) has no error handling. If the IPC call rejects (e.g. transient auth issue), this will produce an unhandled promise rejection and loading won’t be cleared. Add a .catch (or try/await inside the timeout) that sets loading false and clears suggestions as appropriate.

Suggested change
debounceRef.current = setTimeout(() => {
void window.electronAPI.searchRepos(q.trim()).then((results) => {
setSuggestions(results.filter((r) => !selectedRepos.includes(r.fullName)));
setLoading(false);
});
debounceRef.current = setTimeout(async () => {
try {
const results = await window.electronAPI.searchRepos(q.trim());
setSuggestions(results.filter((r) => !selectedRepos.includes(r.fullName)));
} catch {
setSuggestions([]);
} finally {
setLoading(false);
}

Copilot uses AI. Check for mistakes.
Comment thread src/main.ts
Comment on lines 520 to 524
prefs.watchedRepos.map(async (repoRef) => {
const [owner, repo] = repoRef.split('/');
if (!owner || !repo) return [];
return listRepoPullRequests(octokit, owner, repo);
return listRepoPullRequests(octokit, owner, repo, prefs.maxPrsPerRepo);
})
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

prefs.maxPrsPerRepo is read from JSON and passed directly to Octokit as per_page. If the stored value is non-numeric, NaN, or outside GitHub’s supported range (1–100), watched-repo PR discovery will fail (silently, due to Promise.allSettled). Consider normalizing/clamping this value when loading preferences or right before calling listRepoPullRequests (e.g., default to 10 when invalid).

Copilot uses AI. Check for mistakes.
Comment thread src/pages/HomePage.tsx
Comment on lines 272 to 274
if (!prefs.firstRunSeen) {
setFirstRunOpen(true);
setRepoSetupOpen(true);
}
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

PR description says “Replay first-time welcome” should re-trigger the repo setup dialog, but the first-run gate now opens OnboardingRepoSetup while the replay flow still appears to open the older welcome/about modal. Either update the replay handler to open the repo setup dialog (and re-run its first-run logic) or adjust the PR description/UI text so behavior matches.

Copilot uses AI. Check for mistakes.
Comment thread components/SettingsDialog.tsx Outdated
<div className="flex flex-col gap-0.5">
<label className="text-sm font-medium text-foreground">Max PRs per repo</label>
<p className="text-xs text-muted-foreground">
How many of the latest open PRs to review per watched repo.
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

The UI copy says “latest open PRs”, but the underlying GitHub API call is sorted by updated (most recently updated), not necessarily newest by creation time. Consider updating this description to avoid misleading users about which PRs get picked.

Suggested change
How many of the latest open PRs to review per watched repo.
How many recently updated open PRs to review per watched repo.

Copilot uses AI. Check for mistakes.
Comment on lines +59 to +68
<div className="flex flex-col gap-2 text-sm text-muted-foreground">
<p>
Pick repos to watch and Gnosis will automatically review every open PR — no URL pasting needed.
</p>
<p>
Each review becomes a guided walkthrough: diffs grouped by theme, ordered by dependency, with a short narrative on every slide explaining <em>why</em> the change is there.
</p>
<p>
Reviews refresh when PRs update and you'll get a notification when they're ready.
</p>
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

Onboarding copy says Gnosis will “automatically review every open PR”, but proactive mode is capped (e.g. PROACTIVE_MAX_CONCURRENT_UPDATES, repo PR limit, and a 24h staleness cutoff). Consider softening/qualifying this text so it matches actual behavior (e.g., “automatically review open PRs (recently updated), up to your configured limits”).

Copilot uses AI. Check for mistakes.
oddur and others added 5 commits April 16, 2026 17:28
Adds an opt-in "Parallel review" mode that splits review generation into:

1. **Planner phase** — lightweight call with just the hunk index to plan
   topics, assign hunks, and determine ordering
2. **Writer phase** — parallel calls (3 concurrent) with scoped context
   per topic to generate individual slide narratives

Benefits for large PRs: each writer gets focused context (only relevant
hunks + file contents), reducing token waste and improving slide quality.
The planner call is cheap (~5-10k tokens vs 150k for single-shot).

Behind a feature flag (Settings > Parallel review, off by default).
Falls back gracefully — if the planner fails, the existing single-shot
path can still be used.

New files/functions:
- lib/agent.ts: planReview(), generateSlide()
- lib/context-builder.ts: buildPlannerContext(), buildTopicContext()
- lib/types.ts: TopicPlan, PlannerOutput, WriterSlideOutput

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When GitHub returns 'too_large' for the full PR diff, fall back to
assembling a unified diff from individual file patches returned by
the listFiles endpoint (which has no such limit). The patch field
is now captured per ChangedFile.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The planner now outputs a storyArc (overall narrative thread) and a
narrativeBrief per topic (writing direction for each slide). Each
writer receives:
- The story arc for big-picture context
- Its own narrative brief for angle/emphasis
- Briefs of topics it depends on for continuity references

Writers are instructed to acknowledge dependencies ("Building on the
Token type introduced earlier...") and connect to the broader story.
Zero extra API calls — all context comes from the planner output.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When a writer call returns invalid JSON (e.g., unescaped characters in
narrative text), retry once with a directive to return raw JSON only.
Matches the retry pattern used in single-shot generateReviewGuide.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the batched concurrency limit — all topic writers now run
simultaneously via a single Promise.all(). The CLI and API handle
their own rate limiting, so artificial batching just adds wall-clock
time.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@oddur oddur changed the title feat: improved onboarding with guided repo setup feat: parallel review generation + improved onboarding Apr 16, 2026
oddur and others added 4 commits April 16, 2026 18:14
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
OnboardingRepoSetup:
- Reset state (query, suggestions, selectedRepos) when dialog reopens
- Clean up debounce timer on unmount
- Validate repo refs strictly (exactly one slash, no whitespace)
  via REPO_REF_RE pattern; rejects both Enter and suggestion paths
- Add try/catch/finally to searchRepos with request-id guard so
  stale or failed requests don't leave loading stuck
- Soften onboarding copy from "every open PR" to "recently updated
  open PRs" to match actual behavior

main.ts:
- Clamp maxPrsPerRepo to GitHub's 1-100 range, default to 10 if
  the stored value is non-numeric or NaN

SettingsDialog:
- Change "latest open PRs" to "recently updated open PRs" to match
  the GitHub API's sort=updated ordering

HomePage:
- Make replayOnboarding trigger the repo setup dialog (matches the
  PR description's intent)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Skip buildContextPackage + expandFullDiff in parallel mode (wasted
  work on the default hot path)
- Move mcpConfigPath/allowedTools setup inside single-shot branch;
  fixes a resource leak when enableTools + parallelReview were both on
- Replace plannerSummary/plannerRiskLevel/plannerRiskRationale trio
  with a single nullable plan reference
- Extract resolveDiffHunks helper — eliminates 3 copies of hunk-ID →
  DiffHunk mapping (parallel writer, single-shot writer, catch-all)
- buildTopicContext now takes a pre-built Map<id, hunk> instead of
  iterating all hunks per call (O(topic.hunkIds) per call instead of
  O(all_hunks) × topics)
- Fix shadowing bug: parallel branch's inner `const plan` was shadowing
  the outer `let plan`

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@oddur oddur merged commit e103bd9 into main Apr 16, 2026
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