Skip to content

Filter projects from review board via exclude list#207

Merged
dgershman merged 3 commits intomainfrom
feature/crow-190-filter-review-board-exclude
Apr 24, 2026
Merged

Filter projects from review board via exclude list#207
dgershman merged 3 commits intomainfrom
feature/crow-190-filter-review-board-exclude

Conversation

@dgershman
Copy link
Copy Markdown
Collaborator

Summary

  • Adds excludeReviewRepos setting to ConfigDefaults — a list of org/repo strings to hide from the review board
  • Filters excluded repos at the IssueTracker level (prevents notifications) and reactively in the view via AppState.filteredReviewRequests so changes take effect immediately
  • Adds "Reviews" section in Settings > General with a comma-separated text field

Closes #190

Test plan

  • Open Settings > General, verify "Reviews" section with "Excluded Repos" text field appears
  • Enter comma-separated repos (e.g. zarf-dev/zarf, bmlt-enabled/yap), press Enter
  • Verify review board no longer shows PRs from those repos
  • Verify sidebar badge count excludes filtered repos
  • Remove a repo from exclude list, press Enter — PRs reappear on next poll
  • Verify existing configs without the new key load without errors (forward compat)

🤖 Generated with Claude Code

Add excludeReviewRepos to ConfigDefaults so users can hide specific repos
from the review board. Filtering applies at both the IssueTracker level
(preventing notifications) and reactively in the view via AppState so
changes take effect immediately without waiting for the next poll cycle.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dgershman dgershman requested a review from dhilgaertner as a code owner April 24, 2026 21:31
Copy link
Copy Markdown
Contributor

@dhilgaertner dhilgaertner 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

Summary

Clean, well-scoped feature addition. Forward compatibility is handled correctly, tests cover the key cases, and the dual-filter architecture (IssueTracker for notifications, AppState.filteredReviewRequests for reactive UI) is a sensible design. No security concerns.

Security Review

Strengths:

  • Exclude list is a pure comparison key — no shell/SQL/query interpolation, no injection surface.
  • Case-insensitive matching is applied consistently on both sides ($0.repo.lowercased() vs excludeSet of .lowercased() entries).
  • Input sanitization in SettingsView is reasonable: trimmingCharacters(in: .whitespaces) + filter { !$0.isEmpty } prevents empty/whitespace entries from ever hitting the filter.
  • No new privileged actions, scopes, or network calls — just a post-fetch filter.

Concerns: None.

Code Quality

Strengths:

  • Custom init(from:) decoder on ConfigDefaults correctly uses decodeIfPresent for forward compat (matches the pattern already used on AppConfig). configDefaultsDecodeWithoutExcludeReviewRepos locks it in.
  • IssueTracker.refresh reloads config from disk each poll via ConfigStore.loadConfig, so the server-side filter is always fresh — no stale state bug.
  • AppState.excludeReviewRepos is written on both initial hydrate (AppDelegate.swift:117) and on every saveSettings (AppDelegate.swift:452), so the UI reacts immediately when the user updates settings.
  • @Observable ensures the computed filteredReviewRequests / unseenReviewCount re-render on changes to either reviewRequests or excludeReviewRepos.

Minor observations (non-blocking):

🟡 Re-notification after toggle (IssueTracker.swift:258-264): The exclude filter runs before previousReviewRequestIDs is updated. Flow:

  1. Exclude repo A → next poll drops A from reviewspreviousReviewRequestIDs loses A's IDs.
  2. Un-exclude repo A → next poll re-includes A → A's IDs now appear "new" → notification fires for PRs the user had already seen.

Not security-critical and arguably a rare workflow, but may confuse users who toggle the list. Could be fixed by computing newIDs against the unfiltered set before filtering, or by persisting previousReviewRequestIDs across filter changes. Worth a follow-up issue rather than a blocker.

🟡 Save-on-Enter only (SettingsView.swift:164-170): The .onSubmit modifier means users who type a repo and then click somewhere else (or close the window) will lose their edit silently. This matches the existing branchPrefix pattern on line 144, so it's consistent — but adding .onChange or a .onDisappear save would be more forgiving. Consistency vote probably wins here.

🟢 No format validation: "notarepo" (missing org/) is accepted but will never match any $0.repo (which is always org/repo from GraphQL). Silently ineffective rather than broken. A Text(...) caption below the field already documents the expected format, so this is fine.

🟢 Dual filter redundancy: Once the IssueTracker filter runs, appState.reviewRequests is already filtered, so filteredReviewRequests becomes a no-op on the next poll. This is intentional (per the PR description) to provide immediate UI feedback in the ~60s window before the next poll. Worth a brief comment on filteredReviewRequests noting the dual-layer rationale, but not necessary.

Tests

  • appConfigRoundTrip updated to include the new field — good.
  • configDefaultsDecodeWithoutExcludeReviewRepos explicitly locks in forward compat for configs predating this change — excellent.
  • appConfigDecodeFromEmptyJSON asserts the default empty list — good.
  • 113/113 tests pass locally.

Summary Table

Priority Issue
🔴 Red None
🟡 Yellow Re-notification on toggle (IssueTracker.swift:258); onSubmit-only save (SettingsView.swift:164)
🟢 Green Add format validation hint; document dual-filter rationale

Recommendation: Approve. The feature is well-implemented, tests are solid, forward compat is handled, and there are no blocking or security issues. The yellow items are follow-up polish, not merge-blockers.

…on change

- Track all review IDs (including excluded) in previousReviewRequestIDs so
  toggling the exclude list does not re-fire notifications for seen PRs
- Switch Settings text field from .onSubmit to .onChange so edits persist
  without requiring Enter
- Remove added code comments

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dgershman dgershman merged commit 21ad6a7 into main Apr 24, 2026
2 checks passed
@dgershman dgershman deleted the feature/crow-190-filter-review-board-exclude branch April 24, 2026 22:01
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.

Filter projects from review board via exclude list

2 participants