Skip to content

feat(tasks): search tasks by PR number in command panel#1268

Merged
zeval merged 4 commits into
mainfrom
feature/cmd-k-search-per-pr-9fu
Jun 5, 2026
Merged

feat(tasks): search tasks by PR number in command panel#1268
zeval merged 4 commits into
mainfrom
feature/cmd-k-search-per-pr-9fu

Conversation

@zeval

@zeval zeval commented Jun 3, 2026

Copy link
Copy Markdown
Member

Searching the Cmd+K panel by a PR number used to return nothing — PR associations live in github_task_prs, which the task search never consulted. Typing a PR number (e.g. 1243 or #1243) now surfaces the task that PR belongs to, alongside the existing title/description/repo search.

Important Changes

  • github store: ListTaskIDsByPRNumber — workspace-scoped lookup over github_task_prs, exposed via Service.FindTaskIDsByPRNumber.
  • task service: new injected PRTaskResolver interface (github service wired in cmd/kandev); ListTasksByWorkspace merges PR matches into results — deduped, respecting archived/ephemeral/config filters. Kept at the service layer so the task repository stays decoupled from the github schema and existing search SQL/tests are untouched.
  • No frontend change needed — the query param already flows end-to-end.

Validation

  • go build ./...
  • go test ./internal/github/ ./internal/task/service/
  • golangci-lint run on the touched packages

Possible Improvements

PR matches are prepended on top of the LIKE-search page count, so total is approximate; the command panel only uses results for display, not precise paging.

Checklist

  • Tests added/updated
  • Docs updated if needed
  • Self-reviewed the diff

Typing a PR number (e.g. 1243 or #1243) in the Cmd+K panel now surfaces
the task that PR is associated with, alongside the existing
title/description/repo search.

The github store gains a workspace-scoped ListTaskIDsByPRNumber lookup
over github_task_prs; the task service resolves it through an injected
PRTaskResolver interface (github service wired in cmd/kandev) and merges
PR matches into search results, deduped and respecting the
archived/ephemeral/config filters. Keeping the lookup at the service
layer avoids coupling the task repository to the github schema, so
existing search queries and tests are untouched.
@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds workspace-scoped PR→task lookup in the GitHub store, exposes it via a new Service method, wires the GitHub service as a PR-task resolver into the task service, augments ListTasksByWorkspace to surface PR-matched tasks with deduplication and visibility filtering, and adds tests and repository batch APIs.

Changes

PR-Number Task Search

Layer / File(s) Summary
GitHub PR→Task Lookup Layer
apps/backend/internal/github/store.go, apps/backend/internal/github/service_pr_watch.go, apps/backend/internal/github/store_pr_lookup_test.go, apps/backend/internal/github/store_multi_repo_test.go
Adds SQLite index on github_task_prs(pr_number), implements Store.ListTaskIDsByPRNumber with workspace-scoped SELECT DISTINCT joined to tasks, exposes Service.FindTaskIDsByPRNumber, and updates tests/schema to assert workspace isolation and unknown-PR behavior.
Task Service Resolver Interface & Wiring
apps/backend/internal/task/service/service.go, apps/backend/cmd/kandev/services.go
Adds PRTaskResolver interface, stores an optional prTaskResolver on Service, provides SetPRTaskResolver, and wires githubSvc as the resolver when available.
Task Search Augmentation & Visibility Filtering
apps/backend/internal/task/service/service_tasks.go
ListTasksByWorkspace detects numeric PR queries and calls augmentWithPRMatches to resolve, batch-load, filter (archived/ephemeral/config-mode), dedupe, prepend matched tasks, update total, and enforce pagination; helpers parse queries and interpret config_mode.
PR Search Test Suite
apps/backend/internal/task/service/service_pr_search_test.go
Adds fakePRResolver and tests verifying PR-number queries surface resolver-provided tasks, deduplication with title matches, non-numeric queries skip resolver, nil resolver is safe, archived filter respected, and augmentation skipped for later pages/scoped searches.
Repository Batch API & Test Mocks
apps/backend/internal/task/repository/interface.go, apps/backend/internal/task/repository/sqlite/task.go, tests and multiple test fakes/mocks
Adds GetTasksByIDs to TaskRepository, sqlite implementation and tests, and updates/adds test fakes and mocks used by orchestrator, handlers, and service tests.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant TaskService
  participant GitHubService
  participant Store
  Client->>TaskService: ListTasksByWorkspace(query="`#123`", workspaceID)
  TaskService->>GitHubService: FindTaskIDsByPRNumber(ctx, workspaceID, 123)
  GitHubService->>Store: ListTaskIDsByPRNumber(ctx, workspaceID, 123)
  Store-->>GitHubService: []taskID
  GitHubService-->>TaskService: []taskID
  TaskService->>TaskService: batch-load tasks, filter, dedupe, merge, update total
  TaskService-->>Client: tasks, total
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • kdlbs/kandev#565: Maintains the underlying github_task_prs associations that this PR queries, including PR watch state reset and atomic link replacement.

Poem

A rabbit hops through PR numbers bright, 🐰
Finds tasks hidden in workspace light,
Deduped, filtered, and wired just right,
Resolver hums through day and night,
Search returns with newfound sight. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 17.65% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main feature: adding PR number search capability to the command panel for tasks.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description provides a clear summary, important architectural changes, validation steps, and a note on potential limitations. All required sections are present and substantive.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/cmd-k-search-per-pr-9fu

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @zeval's task in 3m 6s —— View job


Code Review

  • Gather context and understand request
  • Read changed files
  • Review for issues
  • Post findings

Findings

Suggestion (recommended, doesn't block)

  1. workflowID/repositoryID filters not propagated to PR-matched resultsservice_tasks.go:1078

    augmentWithPRMatches is called with a prSearchOptions struct that omits workflowID and repositoryID from ListTasksByWorkspace's parameters. The underlying LIKE search correctly scopes by workflow/repository (passed directly to s.tasks.ListTasksByWorkspace), but PR-matched tasks bypass those filters.

    Why it matters: ListTasksByWorkspace is a general HTTP endpoint (see task_http_handlers.go:55-56) that accepts workflow_id and repository_id query params. A board-scoped call with workflowID="wf-a" and query "1234" would correctly scope LIKE results to wf-a, but then prepend any PR fix(orchestrator): add workflow-queue watchdog to recover orphaned auto-start prompts #1234 tasks from any workflow in the workspace. The Cmd+K panel likely passes neither (making this a no-op in practice), but other callers could be surprised.

    Fix:

    type prSearchOptions struct {
        workspaceID      string
        workflowID       string  // add
        repositoryID     string  // add
        query            string
        // ...
    }

    Then add to prMatchFilteredOut:

    if opts.workflowID != "" && task.WorkflowID != opts.workflowID {
        return true
    }

    And pass them through in ListTasksByWorkspace:

    tasks, total = s.augmentWithPRMatches(ctx, tasks, total, prSearchOptions{
        workspaceID:  workspaceID,
        workflowID:   workflowID,   // add
        repositoryID: repositoryID, // add
        // ...
    })
  2. Missing index on github_task_prs.pr_numberstore.go:754

    The new ListTaskIDsByPRNumber query is:

    SELECT DISTINCT gtp.task_id FROM github_task_prs gtp
    INNER JOIN tasks t ON gtp.task_id = t.id
    WHERE t.workspace_id = ? AND gtp.pr_number = ?

    The existing UNIQUE(task_id, repository_id, pr_number) constraint on github_task_prs places pr_number as the 3rd column, so it won't be used as a leading key for this filter. On a large deployment with thousands of task-PR rows, this becomes a full table scan.

    Fix — add a partial index in initSchema:

    _, _ = s.db.Exec(`CREATE INDEX IF NOT EXISTS idx_github_task_prs_pr_number ON github_task_prs (pr_number)`)

    This is a single line and is idempotent.


Summary

Severity Count
Blocker 0
Suggestion 2

Verdict: Ready to merge — both suggestions are improvements on an already-correct implementation. The architecture (interface injection, workspace-scoped store query, test coverage across dedup/filter/nil-resolver cases) is solid. The PR description's note about approximate total is accurate and acceptable for the command panel's non-paginating use.

@greptile-apps

greptile-apps Bot commented Jun 3, 2026

Copy link
Copy Markdown

Greptile Summary

This PR wires PR-number search into the Cmd+K task panel by adding a workspace-scoped github_task_prs lookup path that runs alongside the existing title/description LIKE search. The task service stays decoupled from the GitHub schema via a narrow PRTaskResolver interface, and the augmentation is gated to page 1 of unscoped searches to avoid the pagination corruption described in the prior review cycle.

  • github layer: ListTaskIDsByPRNumber joins github_task_prstasks on workspace_id, backed by a new idx_github_task_prs_pr_number index; workspace isolation is enforced at the SQL level.
  • task/service layer: augmentWithPRMatches parses #NNN/NNN queries, deduplicates against LIKE results, applies the same archived/ephemeral/config visibility filters, and prepends matches on page 1 only; all edge cases (nil resolver, non-numeric query, later pages, scoped searches) short-circuit cleanly.
  • task/repository layer: New GetTasksByIDs batch-fetch uses a parameterized IN (?, ?, ...) query on the read-only connection, consistent with the existing repository pattern.

Confidence Score: 5/5

Safe to merge — the augmentation is additive, best-effort, and touches no existing data paths.

All guard conditions are present and tested: nil resolver, non-numeric query, page > 1, workflow/repo scopes, archived/ephemeral/config filters, and cross-workspace isolation at the SQL JOIN. The previous pagination and silent-error issues are addressed. No existing search, repository, or GitHub code paths are modified.

No files require special attention.

Important Files Changed

Filename Overview
apps/backend/internal/task/service/service_tasks.go Core PR-search augmentation logic: parsePRQuery, augmentWithPRMatches, fetchPRMatchedTasks, prMatchFilteredOut, isConfigTask — all guard conditions (page, scoped searches, nil resolver) are correctly applied; dedup and total accounting are sound.
apps/backend/internal/github/store.go Adds ListTaskIDsByPRNumber with workspace-scoped JOIN and a dedicated index on pr_number; DISTINCT handles multi-repo tasks correctly; index creation errors are intentionally ignored (CREATE INDEX IF NOT EXISTS).
apps/backend/internal/task/repository/sqlite/task.go GetTasksByIDs builds a parameterized IN-clause query; uses read-only connection; empty-slice guard prevents a malformed SQL edge case.
apps/backend/internal/task/service/service_pr_search_test.go Comprehensive test suite covering: PR match surfaces task, dedup, non-numeric query skips resolver, nil resolver no-panic, archived filter, and skip on page>1/scoped searches.
apps/backend/internal/github/store_pr_lookup_test.go New test verifies workspace isolation (cross-workspace PR number does not leak), single match, and unknown PR returns empty — the minimal critical paths are covered.

Sequence Diagram

sequenceDiagram
    participant UI as Cmd+K Panel
    participant SVC as task.Service
    participant REPO as task.Repository (SQLite)
    participant GH as github.Service
    participant GHDB as github.Store (SQLite)

    UI->>SVC: "ListTasksByWorkspace(ws, query="#1243")"
    SVC->>REPO: "ListTasksByWorkspace(LIKE "%#1243%")"
    REPO-->>SVC: "tasks=[], total=0"

    SVC->>SVC: "augmentWithPRMatches(page=1, no workflow/repo filter)"
    SVC->>SVC: "parsePRQuery("#1243") → 1243"

    SVC->>GH: FindTaskIDsByPRNumber(ws, 1243)
    GH->>GHDB: ListTaskIDsByPRNumber(ws, 1243)
    GHDB-->>GH: ["task-pr"]
    GH-->>SVC: ["task-pr"]

    SVC->>SVC: fetchPRMatchedTasks(dedup, filter archived/ephemeral/config)
    SVC->>REPO: GetTasksByIDs(["task-pr"])
    REPO-->>SVC: [task-pr]
    SVC->>SVC: merge + prepend PR matches, increment total

    SVC-->>UI: "tasks=[task-pr, ...], total=1"
Loading

Reviews (4): Last reviewed commit: "perf(tasks): batch-fetch PR-matched task..." | Re-trigger Greptile

Comment thread apps/backend/internal/task/service/service_tasks.go
Comment thread apps/backend/internal/github/store.go
Comment thread apps/backend/internal/task/service/service_tasks.go
Comment thread apps/backend/internal/task/service/service_tasks.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
apps/backend/cmd/kandev/services.go (1)

105-107: ⚡ Quick win

Keep the new GitHub→task wiring out of provideServices.

This works, but it adds another cross-domain setter directly into the main composer. Please move this into the same per-domain init/helper layer the file uses for service wiring so provideServices does not keep accumulating one-off integrations.

Based on learnings: "Backend: Wire the service via a per-domain init<Name>Service(...) helper in cmd/kandev/services.go, not inline in provideServices."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/backend/cmd/kandev/services.go` around lines 105 - 107, The wiring that
calls taskSvc.SetRemoteBranchLister(githubBranchListerAdapter{svc: githubSvc})
and taskSvc.SetPRTaskResolver(githubSvc) should be removed from provideServices
and relocated into a per-domain init helper (e.g., initTaskService or
initKandevService) that lives alongside the other domain-specific service
wiring; create an initTaskService(githubSvc, taskSvc, ...) helper that performs
the githubBranchListerAdapter setup and PR task resolver registration and call
that helper from the domain init layer instead of in provideServices so
cross-domain setters are contained in the domain init.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/backend/internal/task/service/service_tasks.go`:
- Around line 1078-1086: The bug is that s.augmentWithPRMatches is being called
after the task list has already been paginated, causing PR matches to be
prepended per-page and duplicate across pages; fix by moving PR augmentation to
run on the full unsliced result before applying pagination (or alter
augmentWithPRMatches to accept the complete result set and return augmented
results which are then paginated), update calls to augmentWithPRMatches (the
call sites around service_tasks.go where prSearchOptions is passed) so
augmentation/deduplication uses a global dedupe map across the entire result set
rather than a per-page map, and ensure final slicing for pageSize happens only
after augmentation.
- Around line 1078-1086: ListTasksByWorkspace advertises workflowID/repositoryID
as hard filters but calls augmentWithPRMatches with prSearchOptions that only
carries archived/ephemeral/config flags, allowing PR-linked tasks from other
workflows/repos to leak; update prSearchOptions (used by augmentWithPRMatches)
to include workflowID and repositoryID and ensure augmentWithPRMatches and its
internal matching logic (and any checks around lines with second/third calls)
reapply those filters when considering PR matches so only tasks matching the
requested workflowID and repositoryID are returned.

---

Nitpick comments:
In `@apps/backend/cmd/kandev/services.go`:
- Around line 105-107: The wiring that calls
taskSvc.SetRemoteBranchLister(githubBranchListerAdapter{svc: githubSvc}) and
taskSvc.SetPRTaskResolver(githubSvc) should be removed from provideServices and
relocated into a per-domain init helper (e.g., initTaskService or
initKandevService) that lives alongside the other domain-specific service
wiring; create an initTaskService(githubSvc, taskSvc, ...) helper that performs
the githubBranchListerAdapter setup and PR task resolver registration and call
that helper from the domain init layer instead of in provideServices so
cross-domain setters are contained in the domain init.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 43b5600e-012a-4a8e-8ab3-17f5c8b05337

📥 Commits

Reviewing files that changed from the base of the PR and between df9a68a and 8f19ca3.

📒 Files selected for processing (7)
  • apps/backend/cmd/kandev/services.go
  • apps/backend/internal/github/service_pr_watch.go
  • apps/backend/internal/github/store.go
  • apps/backend/internal/github/store_pr_lookup_test.go
  • apps/backend/internal/task/service/service.go
  • apps/backend/internal/task/service/service_pr_search_test.go
  • apps/backend/internal/task/service/service_tasks.go

Comment thread apps/backend/internal/task/service/service_tasks.go

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

3 issues found across 7 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread apps/backend/internal/task/service/service_tasks.go
Comment thread apps/backend/internal/task/service/service_tasks.go
Comment thread apps/backend/internal/github/store_pr_lookup_test.go Outdated
Address review feedback on PR-number search:

- Only augment results on the first page of an unscoped search. Running
  on every page re-prepended the PR match and truncated a real result
  off each subsequent page; running under a workflow/repository filter
  leaked tasks that don't satisfy the filter.
- Add idx_github_task_prs_pr_number so the lookup is index-backed
  instead of a full scan (pr_number is the 3rd column of the existing
  UNIQUE constraint, unusable as a leading key).
- Log instead of silently dropping a task when GetTask fails in the
  PR-match loop.
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @zeval's task in 3m 25s —— View job


Code Review

  • Gather context and understand request
  • Read SKILL.md for review instructions
  • Read changed files and PR diff
  • Analyze findings
  • Post inline comments
  • Post findings report

Findings

Suggestion (recommended, doesn't block)

  1. Duplicate store test helperapps/backend/internal/github/store_pr_lookup_test.go:16

    newTestStoreWithWorkspaces is nearly identical to newTestStore in store_multi_repo_test.go, differing only in workspace_id TEXT in the tasks DDL. Any future change to the shared setup needs two edits. See inline comment for the fix.


Summary

Severity Count
Blocker 0
Suggestion 1

Verdict: Ready to merge.

The two P2 issues raised by earlier automated reviewers (PR augmentation re-appearing on pages 2+, and workflowID/repositoryID filters bypassed for PR matches) were both addressed in commit 100af2e. The page > 1 guard, the workflow/repo early-exit, and the dedicated test case (TestListTasksByWorkspace_PRMatchSkippedOnLaterPagesAndScopedSearches) are all present and correct.

Architecture is clean: interface injection keeps the task service decoupled from the github schema, the workspace-scoped query prevents cross-workspace leakage, the pr_number index is already in place, and test coverage spans dedup, nil-resolver safety, archived-filter, and page/workflow/repo scoping.

Comment thread apps/backend/internal/github/store_pr_lookup_test.go Outdated
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Findings

Suggestion (recommended, doesn't block)

  1. Duplicate store test helperapps/backend/internal/github/store_pr_lookup_test.go:16
    • Issue: newTestStoreWithWorkspaces is nearly identical to newTestStore (store_multi_repo_test.go:14), same package, differing only in workspace_id TEXT in the tasks DDL. Any future change to the shared store setup needs to be applied in two places.
    • Fix: Add workspace_id TEXT to newTestStore's CREATE TABLE tasks — existing multi-repo tests don't depend on its absence — then collapse newTestStoreWithWorkspaces to a one-liner or remove it entirely.

Summary

Severity Count
Blocker 0
Suggestion 1

Verdict: Ready to merge.

The two P2 issues raised by earlier automated reviewers (PR augmentation re-appearing on pages 2+, and workflowID/repositoryID filters bypassed for PR matches) were both addressed in commit 100af2e. The page > 1 guard, the workflow/repo early-exit, and the dedicated test case (TestListTasksByWorkspace_PRMatchSkippedOnLaterPagesAndScopedSearches) are all present and correct.

Architecture is clean: interface injection keeps the task service decoupled from the github schema, the workspace-scoped store query prevents cross-workspace leakage, the CREATE INDEX IF NOT EXISTS for pr_number is already in place, and test coverage spans dedup, nil-resolver safety, archived-filter, and page/workflow/repo scoping.

Add workspace_id to newTestStore's tasks table and drop the near-identical
newTestStoreWithWorkspaces helper, per review feedback.
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @zeval's task in 3m 20s —— View job


Code Review

  • Gather context and understand request
  • Read SKILL.md for review instructions
  • Read changed files and PR diff
  • Analyze findings
  • Post findings report

Findings

Suggestion (recommended, doesn't block)

  1. N+1 GetTask calls in augmentWithPRMatchesservice_tasks.go:1176

    For each resolver ID not already in existing, we call s.tasks.GetTask(ctx, id) individually. In practice PR number → task cardinality is 1–2, so this never matters today. If a future BatchGetTasks method lands, this is the right place to swap it in.

    Not a blocker — just worth noting so the pattern isn't replicated elsewhere.


Summary

Severity Count
Blocker 0
Suggestion 1

Verdict: Ready to merge.

All three issues raised in earlier review rounds are fully addressed in commit 100af2e:

  • page > 1 guard prevents PR matches from re-appearing on subsequent pages
  • workflowID != "" || repositoryID != "" bail prevents PR matches from bypassing scope filters
  • GetTask failures now emit a Warn log with the task ID

Commit 21a30b8 cleaned up the duplicate newTestStoreWithWorkspaces helper by adding workspace_id TEXT to the shared newTestStore DDL.

Architecture is clean: PRTaskResolver interface keeps the task service decoupled from the github schema, the DISTINCT JOIN is workspace-scoped with a dedicated idx_github_task_prs_pr_number index, and test coverage spans dedup, nil-resolver safety, archived filter, and the page/workflow/repo scoping guards.

Add a repository GetTasksByIDs and use it in augmentWithPRMatches so PR-number
search loads all matched tasks in one query instead of one GetTask per id,
addressing the N+1 noted in review. Fetch+filter is extracted into a
fetchPRMatchedTasks helper that preserves resolver order and dedup.
@zeval

zeval commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

Addressed the remaining N+1 suggestion from the review in 67d9c77: added a repository `GetTasksByIDs` and switched `augmentWithPRMatches` to a single batch fetch (via a `fetchPRMatchedTasks` helper that preserves resolver order and dedup) instead of one `GetTask` per id.

@claude

claude Bot commented Jun 4, 2026

Copy link
Copy Markdown

Claude finished @zeval's task in 2m 33s —— View job


Code Review

  • Gather context and understand request
  • Read SKILL.md for review instructions
  • Read changed files and PR diff
  • Analyze findings
  • Post findings report

Findings

No new issues.

The batch-fetch implementation in 67d9c77 is correct across all axes:

  • GetTasksByIDs uses a single WHERE IN (?,...) with parameterized placeholders + Rebind. Missing IDs are silently omitted (documented in the comment). No N+1.
  • fetchPRMatchedTasks re-orders results by iterating fetchIDs (preserving resolver order) before building matched, rather than iterating the unordered byID map — correct.
  • Error path: GetTasksByIDs failure returns nil matched tasks → augmentWithPRMatches returns unmodified LIKE results. Correct best-effort behaviour.
  • Mocks: all four mock implementations handle the new interface method correctly.
  • Test coverage: TestSQLiteRepository_GetTasksByIDs covers nil/empty and mixed existing+missing IDs; service_pr_search_test.go integration tests exercise the full batch path against the real SQLite repository.

Summary

Severity Count
Blocker 0
Suggestion 0

Verdict: Ready to merge. All issues from prior review rounds have been addressed, and the batch-fetch followup is a clean, correct implementation.

@claude

claude Bot commented Jun 4, 2026

Copy link
Copy Markdown

Findings

No new issues.

The batch-fetch implementation in 67d9c77 is correct:

  • GetTasksByIDs builds a single WHERE IN (?,...) with parameterized placeholders and uses Rebind for dialect portability. Missing IDs are silently omitted (correct) and the comment says so.
  • fetchPRMatchedTasks correctly re-orders results by iterating fetchIDs (resolver order) before returning, even though byID is unordered — the "result order is not guaranteed" doc comment on the repository method is accurate and the caller handles it.
  • Error path: a GetTasksByIDs failure returns nil (no matches), which causes augmentWithPRMatches to return the unmodified LIKE results. That's the correct best-effort behaviour.
  • Mocks: all four mock implementations handle the new interface method correctly — three with proper in-memory iteration, one with nil, nil stub for test files that don't exercise PR search.
  • Test coverage: TestSQLiteRepository_GetTasksByIDs exercises nil/empty input and mixed existing+missing IDs; the service_pr_search_test.go integration tests exercise the full stack via the real SQLite repository.

Summary

Severity Count
Blocker 0
Suggestion 0

Verdict: Ready to merge. All issues from prior review rounds have been addressed, and the batch-fetch followup is a clean, correct implementation.

@zeval zeval merged commit 4601dbc into main Jun 5, 2026
43 checks passed
@zeval zeval deleted the feature/cmd-k-search-per-pr-9fu branch June 5, 2026 15:20
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.

1 participant