Ship active provider GitHub sync#7
Merged
Merged
Conversation
|
Linter diff in the way? Review this PR in Change Stack to focus on meaningful changes and expand context only when needed. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds active review-provider selection and orchestration with cache-first snapshot sync, polling/backoff, descriptor-based plugin discovery, a cached provider snapshot store, TUI provider picker and PR-sheet overlays with markdown rendering, and GitHub GraphQL-backed snapshot fetching and PR matching. ChangesActive Provider Sync & PR Overview
Sequence Diagram (high-level flow) sequenceDiagram
participant TUI as TUI (Model)
participant Service as ActiveProviderService
participant Cache as ProviderSnapshotCache
participant Plugin as ReviewProviderClient (plugin subprocess)
participant GitHub as GitHub GraphQL
TUI->>Service: Start/Switch/Refresh/Publish intents (review context)
Service->>Cache: LoadProviderSnapshot(key) (cache-first)
alt cache miss or refresh
Service->>Plugin: Initialize + LoadRemoteSnapshot/LoadRemoteThreads
Plugin->>GitHub: (if GitHub plugin) GraphQL list/snapshot queries
GitHub-->>Plugin: Snapshot result (threads, overview, metadata)
Plugin-->>Service: ProviderSnapshot
Service->>Cache: SaveProviderSnapshot(snapshot)
end
Service-->>TUI: ActiveProviderState (snapshot, sync status, next sync)
TUI->>Plugin: PublishReview request (via Service routing)
Plugin-->>TUI: Publish result
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR expands Ero’s plugin/review-provider system to support active provider selection with cache-first remote sync, adds a GitHub provider GraphQL snapshot implementation (PR metadata + comments/reviews + inline review threads), and exposes the data in the TUI via a provider picker, sync status, and a PR overview sheet.
Changes:
- Add a new plugin protocol capability (
load_remote_snapshot) and host/client plumbing to fetch normalized provider snapshots (threads + overview metadata). - Introduce an
ActiveProviderServicewith provider discovery, selection/switching, cache + preferences storage, and polling/backoff-driven refresh. - Implement GitHub provider snapshot fetching via
go-ghGraphQL and add TUI surfaces (provider picker, PR sheet, statusbar sync indicators, new keybindings).
Reviewed changes
Copilot reviewed 49 out of 56 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents new active provider sync + PR sheet features and updated GitHub auth wording. |
| plugins/github/cmd/ero-plugin-github/remote.go | Adds robust GitHub remote parsing + selection helpers. |
| plugins/github/cmd/ero-plugin-github/match.go | Adds PR candidate matching logic based on review context (base/head refs/SHA). |
| plugins/github/cmd/ero-plugin-github/map.go | Maps GitHub GraphQL nodes into normalized snapshot/thread/overview structures. |
| plugins/github/cmd/ero-plugin-github/main.go | Enables snapshot/thread loading + improved context detection + error classification. |
| plugins/github/cmd/ero-plugin-github/main_test.go | Adds tests for remote parsing, PR matching, detection behavior, and publish PR lookup. |
| plugins/github/cmd/ero-plugin-github/graphql.go | Implements GraphQL queries + pagination for candidate discovery and snapshots. |
| plugins/github/cmd/ero-plugin-github/graphql_test.go | Tests mapping and pagination behavior with a fake GraphQL client. |
| pkg/plugin/server.go | Adds optional load_remote_snapshot dispatch via interface assertion. |
| pkg/plugin/server_test.go | Tests snapshot dispatch and backward compatibility for older providers. |
| pkg/plugin/protocol/types.go | Adds wire types for snapshot requests/results and overview metadata. |
| pkg/plugin/plugin.go | Exposes snapshot/overview protocol types through the plugin SDK surface. |
| internal/ports/provider_sync.go | Introduces ports for snapshot caching and active provider preference storage. |
| internal/ports/plugin.go | Adds review provider descriptor catalog + client factory interfaces. |
| internal/ports/mocks/review_provider_loader_mock.go | Extends loader mock with catalog/factory methods. |
| internal/ports/mocks/review_provider_client_factory_mock.go | Adds mock for provider client factory port. |
| internal/ports/mocks/review_provider_catalog_mock.go | Adds mock for provider catalog port. |
| internal/core/review_provider.go | Adds core capability flag for snapshot loading. |
| internal/core/provider_snapshot.go | Defines normalized provider snapshot + stable context key + sync state model. |
| internal/core/provider_snapshot_test.go | Tests stability/identity behavior of snapshot context keys. |
| internal/core/provider_error.go | Adds typed provider error classification + retryable determination. |
| internal/core/provider_error_test.go | Tests provider error classification and retryable rules. |
| internal/app/tui_active_provider.go | TUI-facing controller wrapper around active provider service. |
| internal/app/review_providers.go | Updates provider loading to descriptor catalog + client factory creation. |
| internal/app/provider_polling_config.go | Reads provider sync polling/backoff config from viper. |
| internal/app/app.go | Wires active provider service + XDG cache/prefs store into app startup. |
| internal/app/app_test.go | Updates tests for new provider discovery/build path. |
| internal/app/active_provider_service.go | Implements provider selection, refresh, caching, preferences, polling/backoff. |
| internal/app/active_provider_service_test.go | Adds service-level tests (preference fallback, cache-first, generation, etc.). |
| internal/adapters/out/providercache/cache.go | Adds XDG-backed JSON store for snapshots and active provider preference. |
| internal/adapters/out/providercache/cache_test.go | Tests snapshot + preference persistence round-trips. |
| internal/adapters/out/plugin/provider_loader.go | Adds provider descriptor discovery + stable provider key derivation + factory. |
| internal/adapters/out/plugin/provider_loader_test.go | Tests descriptor listing (no subprocess start) + stable key behavior. |
| internal/adapters/out/plugin/client.go | Adds snapshot call support and maps protocol errors into core provider errors. |
| internal/adapters/out/plugin/client_test.go | Tests snapshot fallback, snapshot mapping, and protocol→provider error mapping. |
| internal/adapters/in/tui/review_publish.go | Ensures active provider is included in publish targets selection list. |
| internal/adapters/in/tui/review_providers.go | Adds active provider lifecycle cmds (start/refresh/switch/poll scheduling). |
| internal/adapters/in/tui/provider_picker.go | Implements provider picker overlay and keyboard interactions. |
| internal/adapters/in/tui/provider_picker_test.go | Tests picker rendering and switch/refresh/cycle shortcuts. |
| internal/adapters/in/tui/pr_sheet.go | Adds PR sheet overlay rendering of provider overview + markdown-rendered content. |
| internal/adapters/in/tui/pr_sheet_test.go | Tests PR sheet overlay layout, scroll independence, and markdown display. |
| internal/adapters/in/tui/model.go | Integrates active provider controller, PR sheet, picker, and statusbar sync fields. |
| internal/adapters/in/tui/markdown_renderer.go | Adds cached markdown rendering wrapper (glamour) with safe fallback. |
| internal/adapters/in/tui/markdown_renderer_test.go | Tests caching, fenced code rendering, and safe fallback behavior. |
| internal/adapters/in/tui/keymap/action.go | Adds keybindings for provider cycle/picker/refresh and PR sheet toggle. |
| internal/adapters/in/tui/keymap/action_test.go | Tests new keybindings. |
| internal/adapters/in/tui/help_pane_test.go | Verifies help modal includes new provider/PR sheet shortcuts. |
| internal/adapters/in/tui/component/statusbar.go | Adds provider sync status segment (with optional nerd font symbols). |
| internal/adapters/in/tui/component/statusbar_test.go | Tests statusbar sync labels/symbols and narrow-width behavior. |
| internal/adapters/in/tui/component/help_pane.go | Documents new shortcuts in help pane content. |
| internal/adapters/in/tui/active_provider_test.go | Tests TUI integration for active provider startup/refresh/switch/publish paths. |
| internal/adapters/in/cli/root.go | Adds CLI flag/config binding for provider sync interval. |
| go.mod | Adds glamour dependency and indirect deps required for markdown rendering/snapshots. |
| go.sum | Adds checksums for newly introduced dependencies. |
| docs/plugins.md | Updates plugin docs for descriptor discovery, stable keys, snapshots, and PR sheet. |
| docs/architecture.md | Updates architecture notes for active provider lifecycle/sync ownership in internal/app. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| st.Snapshot.Sync.NextSyncAt = nil | ||
| } else { | ||
| st.Snapshot.Sync.Status = core.ProviderSyncStatusBackingOff | ||
| st.Snapshot.Sync.NextSyncAt = new(st.NextSyncAt) |
| if snap.FetchedAt.IsZero() { | ||
| snap.FetchedAt = now | ||
| } | ||
| snap.Sync = core.ProviderSyncState{Status: core.ProviderSyncStatusSynced, LastSyncAt: new(now), NextSyncAt: new(next)} |
Comment on lines
22
to
+26
| func (m Model) closeReviewProvidersCmd() tea.Cmd { | ||
| if m.activeProvider != nil { | ||
| activeProvider := m.activeProvider | ||
| return func() tea.Msg { _ = activeProvider.Close(); return nil } | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@internal/adapters/in/tui/review_providers.go`:
- Around line 44-49: The code incorrectly folds catalogErr into the Start error
(err) which treats a successful Start as a failure when catalog retrieval fails;
change the logic to preserve Start's error separately and return catalogErr
separately instead of overwriting err. Modify the activeProviderStartedMsg to
include a distinct catalogErr (or similarly named) field, set that field to
catalogErr while leaving err as the Start error from activeProvider.Start(ctx,
reviewContext), and update callers (e.g., Model.Update) to check and handle
catalogErr independently of the Start error so startup success still triggers
initial refresh/poll.
In `@internal/adapters/out/providercache/cache.go`:
- Around line 50-67: LoadActiveProviderKey currently trusts any preference file
at preferencePath(repositoryIdentity) but safeName(repositoryIdentity) can
collide, so modify the stored preference format and loader to validate the
repository identity inside the file: update the JSON struct in
LoadActiveProviderKey to include a stored RepositoryIdentity field (or similar),
during read/unmarshal compare that stored value to the incoming
repositoryIdentity and return not-found (\"\", false, nil) when they differ, and
ensure preferencePath/safeName unchanged; reference LoadActiveProviderKey,
preferencePath and safeName when making this change.
- Line 100: The deferred call to os.Remove(tmpName) in cache.go should handle
errors explicitly: wrap the remove in a function that checks the returned error
and only swallows os.ErrNotExist (use os.IsNotExist(err)), while surfacing or
logging any other error; ensure this change is applied where tmpName is deferred
and after the code path that may call os.Rename so the expected "not exist" case
is ignored but real failures are not silently lost.
In `@internal/app/active_provider_service.go`:
- Around line 69-76: The code currently calls s.closeLocked() and drops its
returned error during provider replacement (in the blocks around
s.mu.Lock()/s.mu.Unlock(), e.g., in Start() and Switch()); capture the error
returned by closeLocked() and do not discard it—either return it to the caller,
aggregate it with the new operation's error, or at minimum log it via the
service logger (e.g., s.logger or existing logging helper) before proceeding;
update each call site that currently ignores s.closeLocked() (the blocks that
set s.stableKey, s.runtimeID, s.generation, s.state) to handle the error
consistently (propagate or log) so provider shutdown failures are surfaced.
In `@internal/app/provider_polling_config.go`:
- Around line 17-29: The backoff bounds can be misordered: after reading
provider-sync-min-backoff and provider-sync-max-backoff into poll.MinBackoff and
poll.MaxBackoff (via cfg.GetDuration), ensure the values are normalized so
MinBackoff <= MaxBackoff before returning; apply defaults first (set MinBackoff
to 5s and MaxBackoff to 1m when zero), then if poll.MinBackoff > poll.MaxBackoff
clamp poll.MinBackoff = poll.MaxBackoff (or swap) so retry timing remains
monotonic.
In `@internal/core/provider_snapshot_test.go`:
- Around line 36-43: The loop currently does a shallow copy with "ctx := base"
and then calls mutate(&ctx), which lets mutations to
ctx.Repository.Remotes[0].URL leak into base; fix by making a deep copy of the
Repository.Remotes before calling mutate so each case gets its own slice/struct
copies. Concretely, inside the loop (around the "ctx := base" and before
"mutate(&ctx)"), replace the shallow copy with defensive copies such as creating
a new Repository value on ctx and cloning the Remotes slice and each Remote
element (e.g. ctx.Repository.Remotes = append([]Remote(nil),
base.Repository.Remotes...) and copying each element) so mutate(&ctx) cannot
modify base.Repository.Remotes. Ensure you keep the same variable names (ctx,
base, Repository.Remotes, mutate) so the change is easy to locate.
In `@plugins/github/cmd/ero-plugin-github/graphql.go`:
- Around line 242-246: The code currently maps resp.Repository.PullRequest into
a page even when the GraphQL pullRequest field decoded as a zero-value; update
the block around resp.Repository.PullRequest, mapGitHubPRMetadata and accum so
you first guard that the pullRequest is present (e.g., check a stable field like
PullRequest.ID != "" or PullRequest.Number > 0) and skip mapping/accumulation
when it is absent, returning or continuing appropriately to avoid producing
empty snapshot pages from null responses.
In `@plugins/github/cmd/ero-plugin-github/main.go`:
- Around line 201-210: The publishPRLookupBranch function currently lowercases
the branch by calling normalizeRef before returning, which breaks case-sensitive
gh pr view lookups; change it to preserve the original branch case: keep the
trimming logic for reviewCtx.Target.HeadRef and fallback to
reviewCtx.Repository.CurrentBranch, but return the trimmed headRef directly
instead of calling normalizeRef (i.e., remove/replace the normalizeRef call in
publishPRLookupBranch so gh pr view receives the original-cased ref).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f9c549c3-5844-4a05-879a-e02eb85d9599
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (55)
README.mddocs/architecture.mddocs/plugins.mdgo.modinternal/adapters/in/cli/root.gointernal/adapters/in/tui/active_provider_test.gointernal/adapters/in/tui/component/help_pane.gointernal/adapters/in/tui/component/statusbar.gointernal/adapters/in/tui/component/statusbar_test.gointernal/adapters/in/tui/help_pane_test.gointernal/adapters/in/tui/keymap/action.gointernal/adapters/in/tui/keymap/action_test.gointernal/adapters/in/tui/markdown_renderer.gointernal/adapters/in/tui/markdown_renderer_test.gointernal/adapters/in/tui/model.gointernal/adapters/in/tui/pr_sheet.gointernal/adapters/in/tui/pr_sheet_test.gointernal/adapters/in/tui/provider_picker.gointernal/adapters/in/tui/provider_picker_test.gointernal/adapters/in/tui/review_providers.gointernal/adapters/in/tui/review_publish.gointernal/adapters/out/plugin/client.gointernal/adapters/out/plugin/client_test.gointernal/adapters/out/plugin/provider_loader.gointernal/adapters/out/plugin/provider_loader_test.gointernal/adapters/out/providercache/cache.gointernal/adapters/out/providercache/cache_test.gointernal/app/active_provider_service.gointernal/app/active_provider_service_test.gointernal/app/app.gointernal/app/app_test.gointernal/app/provider_polling_config.gointernal/app/review_providers.gointernal/app/tui_active_provider.gointernal/core/provider_error.gointernal/core/provider_error_test.gointernal/core/provider_snapshot.gointernal/core/provider_snapshot_test.gointernal/core/review_provider.gointernal/ports/mocks/review_provider_catalog_mock.gointernal/ports/mocks/review_provider_client_factory_mock.gointernal/ports/mocks/review_provider_loader_mock.gointernal/ports/plugin.gointernal/ports/provider_sync.gopkg/plugin/plugin.gopkg/plugin/protocol/types.gopkg/plugin/server.gopkg/plugin/server_test.goplugins/github/cmd/ero-plugin-github/graphql.goplugins/github/cmd/ero-plugin-github/graphql_test.goplugins/github/cmd/ero-plugin-github/main.goplugins/github/cmd/ero-plugin-github/main_test.goplugins/github/cmd/ero-plugin-github/map.goplugins/github/cmd/ero-plugin-github/match.goplugins/github/cmd/ero-plugin-github/remote.go
Comment on lines
+50
to
+67
| func (s *Store) LoadActiveProviderKey(_ context.Context, repositoryIdentity string) (string, bool, error) { | ||
| data, err := os.ReadFile(s.preferencePath(repositoryIdentity)) | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return "", false, nil | ||
| } | ||
| if err != nil { | ||
| return "", false, err | ||
| } | ||
| var pref struct { | ||
| StableProviderKey string `json:"stable_provider_key"` | ||
| } | ||
| if err := json.Unmarshal(data, &pref); err != nil { | ||
| return "", false, err | ||
| } | ||
| if pref.StableProviderKey == "" { | ||
| return "", false, nil | ||
| } | ||
| return pref.StableProviderKey, true, nil |
There was a problem hiding this comment.
Validate repository_identity before trusting a preference file.
preferencePath() is derived from safeName(repositoryIdentity), and that sanitizer collapses distinct identities like .../repo and ..._repo onto the same filename. Because LoadActiveProviderKey() ignores the stored repository_identity, a colliding file can activate the wrong provider preference for another repository.
Suggested fix
func (s *Store) LoadActiveProviderKey(_ context.Context, repositoryIdentity string) (string, bool, error) {
data, err := os.ReadFile(s.preferencePath(repositoryIdentity))
if errors.Is(err, os.ErrNotExist) {
return "", false, nil
}
if err != nil {
return "", false, err
}
var pref struct {
+ RepositoryIdentity string `json:"repository_identity"`
StableProviderKey string `json:"stable_provider_key"`
}
if err := json.Unmarshal(data, &pref); err != nil {
return "", false, err
}
+ if pref.RepositoryIdentity != repositoryIdentity {
+ return "", false, nil
+ }
if pref.StableProviderKey == "" {
return "", false, nil
}
return pref.StableProviderKey, true, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *Store) LoadActiveProviderKey(_ context.Context, repositoryIdentity string) (string, bool, error) { | |
| data, err := os.ReadFile(s.preferencePath(repositoryIdentity)) | |
| if errors.Is(err, os.ErrNotExist) { | |
| return "", false, nil | |
| } | |
| if err != nil { | |
| return "", false, err | |
| } | |
| var pref struct { | |
| StableProviderKey string `json:"stable_provider_key"` | |
| } | |
| if err := json.Unmarshal(data, &pref); err != nil { | |
| return "", false, err | |
| } | |
| if pref.StableProviderKey == "" { | |
| return "", false, nil | |
| } | |
| return pref.StableProviderKey, true, nil | |
| func (s *Store) LoadActiveProviderKey(_ context.Context, repositoryIdentity string) (string, bool, error) { | |
| data, err := os.ReadFile(s.preferencePath(repositoryIdentity)) | |
| if errors.Is(err, os.ErrNotExist) { | |
| return "", false, nil | |
| } | |
| if err != nil { | |
| return "", false, err | |
| } | |
| var pref struct { | |
| RepositoryIdentity string `json:"repository_identity"` | |
| StableProviderKey string `json:"stable_provider_key"` | |
| } | |
| if err := json.Unmarshal(data, &pref); err != nil { | |
| return "", false, err | |
| } | |
| if pref.RepositoryIdentity != repositoryIdentity { | |
| return "", false, nil | |
| } | |
| if pref.StableProviderKey == "" { | |
| return "", false, nil | |
| } | |
| return pref.StableProviderKey, true, nil | |
| } |
🤖 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 `@internal/adapters/out/providercache/cache.go` around lines 50 - 67,
LoadActiveProviderKey currently trusts any preference file at
preferencePath(repositoryIdentity) but safeName(repositoryIdentity) can collide,
so modify the stored preference format and loader to validate the repository
identity inside the file: update the JSON struct in LoadActiveProviderKey to
include a stored RepositoryIdentity field (or similar), during read/unmarshal
compare that stored value to the incoming repositoryIdentity and return
not-found (\"\", false, nil) when they differ, and ensure
preferencePath/safeName unchanged; reference LoadActiveProviderKey,
preferencePath and safeName when making this change.
| return err | ||
| } | ||
| tmpName := tmp.Name() | ||
| defer os.Remove(tmpName) |
There was a problem hiding this comment.
Handle deferred temp-file cleanup explicitly.
This unchecked os.Remove is already failing lint, and after a successful os.Rename it will usually return os.ErrNotExist. Please make the cleanup best-effort while swallowing only that expected case.
Suggested fix
tmpName := tmp.Name()
- defer os.Remove(tmpName)
+ defer func() {
+ err := os.Remove(tmpName)
+ if err != nil && !errors.Is(err, os.ErrNotExist) {
+ // Best-effort cleanup.
+ }
+ }()🧰 Tools
🪛 GitHub Actions: CI / 0_ci _ Lint.txt
[error] 100-100: golangci-lint (errcheck): Error return value of os.Remove is not checked (os.Remove returns an error).
🪛 GitHub Actions: CI / ci _ Lint
[error] 100-100: golangci-lint (errcheck): Error return value of os.Remove is not checked.
🪛 GitHub Check: ci / Lint
[failure] 100-100:
Error return value of os.Remove is not checked (errcheck)
🪛 golangci-lint (2.12.2)
[error] 100-100: Error return value of os.Remove is not checked
(errcheck)
🤖 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 `@internal/adapters/out/providercache/cache.go` at line 100, The deferred call
to os.Remove(tmpName) in cache.go should handle errors explicitly: wrap the
remove in a function that checks the returned error and only swallows
os.ErrNotExist (use os.IsNotExist(err)), while surfacing or logging any other
error; ensure this change is applied where tmpName is deferred and after the
code path that may call os.Rename so the expected "not exist" case is ignored
but real failures are not silently lost.
Comment on lines
+36
to
+43
| for name, mutate := range cases { | ||
| ctx := base | ||
| provider := "provider-a" | ||
| if mutate == nil { | ||
| provider = "provider-b" | ||
| } else { | ||
| mutate(&ctx) | ||
| } |
There was a problem hiding this comment.
Isolate case mutations to avoid shared-slice leakage.
Line 37 does a shallow struct copy; Line 42 mutates ctx.Repository.Remotes[0].URL, which can mutate base and make later cases pass for the wrong reason.
Proposed fix
for name, mutate := range cases {
ctx := base
+ ctx.Repository.Remotes = append([]GitRemote(nil), base.Repository.Remotes...)
provider := "provider-a"
if mutate == nil {
provider = "provider-b"
} else {
mutate(&ctx)
}🤖 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 `@internal/core/provider_snapshot_test.go` around lines 36 - 43, The loop
currently does a shallow copy with "ctx := base" and then calls mutate(&ctx),
which lets mutations to ctx.Repository.Remotes[0].URL leak into base; fix by
making a deep copy of the Repository.Remotes before calling mutate so each case
gets its own slice/struct copies. Concretely, inside the loop (around the "ctx
:= base" and before "mutate(&ctx)"), replace the shallow copy with defensive
copies such as creating a new Repository value on ctx and cloning the Remotes
slice and each Remote element (e.g. ctx.Repository.Remotes =
append([]Remote(nil), base.Repository.Remotes...) and copying each element) so
mutate(&ctx) cannot modify base.Repository.Remotes. Ensure you keep the same
variable names (ctx, base, Repository.Remotes, mutate) so the change is easy to
locate.
Comment on lines
+242
to
+246
| pr := resp.Repository.PullRequest | ||
| page := mapGitHubPRMetadata(pr) | ||
| if accum.Number == 0 { | ||
| accum = page | ||
| } |
There was a problem hiding this comment.
Handle null pullRequest responses before mapping snapshot pages.
At Line 242, a null GraphQL pullRequest decodes to a zero-value ghPRNode, and the loop can return an empty snapshot as success. Guard this explicitly.
Proposed fix
pr := resp.Repository.PullRequest
+ if pr.Number == 0 {
+ return githubPRSnapshot{}, plugin.NewError(
+ plugin.ErrorNotApplicable,
+ "GitHub pull request was not found while loading snapshot",
+ )
+ }
page := mapGitHubPRMetadata(pr)🤖 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 `@plugins/github/cmd/ero-plugin-github/graphql.go` around lines 242 - 246, The
code currently maps resp.Repository.PullRequest into a page even when the
GraphQL pullRequest field decoded as a zero-value; update the block around
resp.Repository.PullRequest, mapGitHubPRMetadata and accum so you first guard
that the pullRequest is present (e.g., check a stable field like PullRequest.ID
!= "" or PullRequest.Number > 0) and skip mapping/accumulation when it is
absent, returning or continuing appropriately to avoid producing empty snapshot
pages from null responses.
bnema
commented
Jun 5, 2026
bnema
left a comment
bnema
left a comment
Owner
Author
There was a problem hiding this comment.
Review de test aléatoire pour vérifier que le provider GitHub remonte les reviews dans Ero.
| } | ||
| } | ||
| func (s *ActiveProviderService) nextBackoff(err error) time.Time { | ||
| s.mu.Lock() |
Owner
Author
There was a problem hiding this comment.
Commentaire inline de test aléatoire pour vérifier l’affichage dans Ero. Tu peux l’ignorer.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/adapters/out/plugin/provider_loader.go`:
- Around line 177-196: The directory walk keeps iterating after you set newer =
true; modify the anonymous callback passed to filepath.WalkDir in
provider_loader.go so that when newer becomes true you return a sentinel error
to abort the walk immediately (e.g., return errors.New("stopWalk")), and then
after filepath.WalkDir check for and ignore that sentinel error; use the
existing variables newer, runtimeModTime, and runtimePath in the condition that
triggers the early return so the walk stops as soon as a newer source file is
found.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: da448d22-1e8a-450a-b69f-20accfcdb214
📒 Files selected for processing (3)
internal/adapters/out/plugin/provider_loader.gointernal/adapters/out/plugin/provider_loader_test.gointernal/app/active_provider_service.go
Comment on lines
+177
to
+196
| _ = filepath.WalkDir(pluginDir, func(path string, d os.DirEntry, err error) error { | ||
| if err != nil || newer { | ||
| return nil | ||
| } | ||
| if d.IsDir() { | ||
| if d.Name() == ".git" { | ||
| return filepath.SkipDir | ||
| } | ||
| return nil | ||
| } | ||
| path = filepath.Clean(path) | ||
| if path == runtimePath || !isPluginSourcePath(path) { | ||
| return nil | ||
| } | ||
| info, err := d.Info() | ||
| if err == nil && info.ModTime().After(runtimeModTime) { | ||
| newer = true | ||
| } | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider stopping the directory walk early once a newer source file is found.
When newer becomes true, the walk continues through all remaining files without doing any useful work. Using filepath.SkipAll would terminate the walk immediately.
♻️ Proposed optimization
_ = filepath.WalkDir(pluginDir, func(path string, d os.DirEntry, err error) error {
- if err != nil || newer {
+ if err != nil {
return nil
}
if d.IsDir() {
if d.Name() == ".git" {
return filepath.SkipDir
}
return nil
}
path = filepath.Clean(path)
if path == runtimePath || !isPluginSourcePath(path) {
return nil
}
info, err := d.Info()
if err == nil && info.ModTime().After(runtimeModTime) {
newer = true
+ return filepath.SkipAll
}
return nil
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _ = filepath.WalkDir(pluginDir, func(path string, d os.DirEntry, err error) error { | |
| if err != nil || newer { | |
| return nil | |
| } | |
| if d.IsDir() { | |
| if d.Name() == ".git" { | |
| return filepath.SkipDir | |
| } | |
| return nil | |
| } | |
| path = filepath.Clean(path) | |
| if path == runtimePath || !isPluginSourcePath(path) { | |
| return nil | |
| } | |
| info, err := d.Info() | |
| if err == nil && info.ModTime().After(runtimeModTime) { | |
| newer = true | |
| } | |
| return nil | |
| }) | |
| _ = filepath.WalkDir(pluginDir, func(path string, d os.DirEntry, err error) error { | |
| if err != nil { | |
| return nil | |
| } | |
| if d.IsDir() { | |
| if d.Name() == ".git" { | |
| return filepath.SkipDir | |
| } | |
| return nil | |
| } | |
| path = filepath.Clean(path) | |
| if path == runtimePath || !isPluginSourcePath(path) { | |
| return nil | |
| } | |
| info, err := d.Info() | |
| if err == nil && info.ModTime().After(runtimeModTime) { | |
| newer = true | |
| return filepath.SkipAll | |
| } | |
| return nil | |
| }) |
🤖 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 `@internal/adapters/out/plugin/provider_loader.go` around lines 177 - 196, The
directory walk keeps iterating after you set newer = true; modify the anonymous
callback passed to filepath.WalkDir in provider_loader.go so that when newer
becomes true you return a sentinel error to abort the walk immediately (e.g.,
return errors.New("stopWalk")), and then after filepath.WalkDir check for and
ignore that sentinel error; use the existing variables newer, runtimeModTime,
and runtimePath in the condition that triggers the early return so the walk
stops as soon as a newer source file is found.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plugins/github/cmd/ero-plugin-github/graphql_test.go`:
- Around line 89-92: The test is calling fetchGitHubPRSnapshot with
context.Background(), which can block indefinitely; replace it by creating a
cancellable context with a deadline (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), 30*time.Second)) and defer cancel(),
then pass that ctx into fetchGitHubPRSnapshot (the call in graphql_test.go that
currently uses context.Background()); ensure you import time and use the timeout
constant appropriate for integration tests.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 17b6f0d4-75d0-47bb-96a1-015466f44823
📒 Files selected for processing (2)
plugins/github/cmd/ero-plugin-github/graphql.goplugins/github/cmd/ero-plugin-github/graphql_test.go
Comment on lines
+89
to
+92
| got, err := fetchGitHubPRSnapshot(context.Background(), client, githubRemote{Owner: "bnema", Name: "ero"}, number) | ||
| if err != nil { | ||
| t.Fatalf("fetchGitHubPRSnapshot(%d): %v", number, err) | ||
| } |
There was a problem hiding this comment.
Add a timeout to the integration snapshot fetch.
The test uses context.Background() for a live GitHub call, so it can block indefinitely under auth/network issues.
Suggested fix
- got, err := fetchGitHubPRSnapshot(context.Background(), client, githubRemote{Owner: "bnema", Name: "ero"}, number)
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ got, err := fetchGitHubPRSnapshot(ctx, client, githubRemote{Owner: "bnema", Name: "ero"}, number)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| got, err := fetchGitHubPRSnapshot(context.Background(), client, githubRemote{Owner: "bnema", Name: "ero"}, number) | |
| if err != nil { | |
| t.Fatalf("fetchGitHubPRSnapshot(%d): %v", number, err) | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| got, err := fetchGitHubPRSnapshot(ctx, client, githubRemote{Owner: "bnema", Name: "ero"}, number) | |
| if err != nil { | |
| t.Fatalf("fetchGitHubPRSnapshot(%d): %v", number, err) | |
| } |
🤖 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 `@plugins/github/cmd/ero-plugin-github/graphql_test.go` around lines 89 - 92,
The test is calling fetchGitHubPRSnapshot with context.Background(), which can
block indefinitely; replace it by creating a cancellable context with a deadline
(e.g., ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second))
and defer cancel(), then pass that ctx into fetchGitHubPRSnapshot (the call in
graphql_test.go that currently uses context.Background()); ensure you import
time and use the timeout constant appropriate for integration tests.
bnema
commented
Jun 6, 2026
Comment on lines
+14
to
+29
| type fakeActiveProviderController struct { | ||
| catalog []ports.ReviewProviderDescriptor | ||
| startState ActiveProviderState | ||
| refreshState ActiveProviderState | ||
| switchStates map[string]ActiveProviderState | ||
| publishResult core.PublishReviewResult | ||
| startErr error | ||
| refreshErr error | ||
| switchErrs map[string]error | ||
| publishErr error | ||
| startCalls int | ||
| refreshManual []bool | ||
| switchKeys []string | ||
| publishRequests []core.PublishReviewRequest | ||
| closed bool | ||
| } |
Owner
Author
There was a problem hiding this comment.
why its a fake and not a mock ?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/adapters/in/tui/model.go`:
- Around line 356-357: The early-return condition that blocks UI interactions
misses the PR sheet flag, so add m.prSheet.open to the guard in the input
handling path (the if that currently checks m.helpActive, m.commentEditor !=
nil, m.search.active(), m.providerPicker.open, m.publish.active) so that when
m.prSheet.open is true the function returns early and mouse-wheel (and other
inputs) do not affect the underlying review cursor; update the same conditional
in model.go where that guard is located (reference: m.helpActive,
m.commentEditor, m.search.active, m.providerPicker.open, m.publish.active,
m.prSheet.open).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a893bbc9-3ec3-4077-9690-ab8768f3fef6
📒 Files selected for processing (3)
internal/adapters/in/tui/cursor_navigation_test.gointernal/adapters/in/tui/model.gointernal/adapters/in/tui/model_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/adapters/in/tui/active_provider_test.go`:
- Around line 17-20: The mock ActiveProviderController.Catalog method should be
nil-safe: instead of directly casting args.Get(0) to
[]ports.ReviewProviderDescriptor (which panics when the caller set Return(nil,
err)), check whether args.Get(0) is nil and return nil, args.Error(1) in that
case; otherwise cast and return the slice and the error. Update the
mockActiveProviderController.Catalog implementation to guard around args.Get(0)
before converting to []ports.ReviewProviderDescriptor.
In `@internal/adapters/in/tui/model_test.go`:
- Around line 41-45: The test assumes a hardcoded internal ID "comment-1";
instead capture the ID returned from model.reviewDraft.AddComment and use that
when calling model.reviewDraft.ApplyPublishedRefs so the test validates behavior
rather than ID formatting: change the second AddComment call to store its
returned comment ID (from the first AddComment if you meant the first comment)
and pass that variable as the LocalCommentID inside the
core.PublishedReviewCommentRef given to ApplyPublishedRefs (referencing
model.reviewDraft.AddComment return value and ApplyPublishedRefs and
core.PublishedReviewCommentRef/LocalCommentID).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: bee806da-1f14-4f29-8bd7-7c8512879932
📒 Files selected for processing (6)
internal/adapters/in/tui/active_provider_test.gointernal/adapters/in/tui/component/statusbar.gointernal/adapters/in/tui/component/statusbar_test.gointernal/adapters/in/tui/model.gointernal/adapters/in/tui/model_test.gointernal/adapters/in/tui/provider_picker_test.go
Comment on lines
+17
to
+20
| func (m *mockActiveProviderController) Catalog(ctx context.Context) ([]ports.ReviewProviderDescriptor, error) { | ||
| args := m.Called(ctx) | ||
| return args.Get(0).([]ports.ReviewProviderDescriptor), args.Error(1) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Make Catalog mock return nil-safe.
Direct assertion on args.Get(0) will panic for Return(nil, err) setups, which blocks clean error-path tests.
💡 Suggested change
func (m *mockActiveProviderController) Catalog(ctx context.Context) ([]ports.ReviewProviderDescriptor, error) {
args := m.Called(ctx)
- return args.Get(0).([]ports.ReviewProviderDescriptor), args.Error(1)
+ var catalog []ports.ReviewProviderDescriptor
+ if raw := args.Get(0); raw != nil {
+ catalog = raw.([]ports.ReviewProviderDescriptor)
+ }
+ return catalog, args.Error(1)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (m *mockActiveProviderController) Catalog(ctx context.Context) ([]ports.ReviewProviderDescriptor, error) { | |
| args := m.Called(ctx) | |
| return args.Get(0).([]ports.ReviewProviderDescriptor), args.Error(1) | |
| } | |
| func (m *mockActiveProviderController) Catalog(ctx context.Context) ([]ports.ReviewProviderDescriptor, error) { | |
| args := m.Called(ctx) | |
| var catalog []ports.ReviewProviderDescriptor | |
| if raw := args.Get(0); raw != nil { | |
| catalog = raw.([]ports.ReviewProviderDescriptor) | |
| } | |
| return catalog, args.Error(1) | |
| } |
🤖 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 `@internal/adapters/in/tui/active_provider_test.go` around lines 17 - 20, The
mock ActiveProviderController.Catalog method should be nil-safe: instead of
directly casting args.Get(0) to []ports.ReviewProviderDescriptor (which panics
when the caller set Return(nil, err)), check whether args.Get(0) is nil and
return nil, args.Error(1) in that case; otherwise cast and return the slice and
the error. Update the mockActiveProviderController.Catalog implementation to
guard around args.Get(0) before converting to []ports.ReviewProviderDescriptor.
Comment on lines
+41
to
+45
| _, err := model.reviewDraft.AddComment(core.ReviewCommentInput{FilePath: "demo.go", Range: core.ReviewLineRange{Start: core.ReviewLineRef{NewLineNumber: 1}, End: core.ReviewLineRef{NewLineNumber: 1}}, Body: "first"}) | ||
| require.NoError(t, err) | ||
| _, err = model.reviewDraft.AddComment(core.ReviewCommentInput{FilePath: "demo.go", Range: core.ReviewLineRange{Start: core.ReviewLineRef{NewLineNumber: 1}, End: core.ReviewLineRef{NewLineNumber: 1}}, Body: "second"}) | ||
| require.NoError(t, err) | ||
| model.reviewDraft.ApplyPublishedRefs("github", []core.PublishedReviewCommentRef{{LocalCommentID: "comment-1", ExternalID: "remote-1"}}) |
There was a problem hiding this comment.
Avoid hardcoding internal local comment IDs in this test.
Line [45] assumes the first generated ID is "comment-1". Capture the ID returned by AddComment and pass that into ApplyPublishedRefs so the test validates behavior, not ID formatting.
💡 Suggested change
- _, err := model.reviewDraft.AddComment(core.ReviewCommentInput{FilePath: "demo.go", Range: core.ReviewLineRange{Start: core.ReviewLineRef{NewLineNumber: 1}, End: core.ReviewLineRef{NewLineNumber: 1}}, Body: "first"})
+ first, err := model.reviewDraft.AddComment(core.ReviewCommentInput{FilePath: "demo.go", Range: core.ReviewLineRange{Start: core.ReviewLineRef{NewLineNumber: 1}, End: core.ReviewLineRef{NewLineNumber: 1}}, Body: "first"})
require.NoError(t, err)
_, err = model.reviewDraft.AddComment(core.ReviewCommentInput{FilePath: "demo.go", Range: core.ReviewLineRange{Start: core.ReviewLineRef{NewLineNumber: 1}, End: core.ReviewLineRef{NewLineNumber: 1}}, Body: "second"})
require.NoError(t, err)
- model.reviewDraft.ApplyPublishedRefs("github", []core.PublishedReviewCommentRef{{LocalCommentID: "comment-1", ExternalID: "remote-1"}})
+ model.reviewDraft.ApplyPublishedRefs("github", []core.PublishedReviewCommentRef{{LocalCommentID: first.ID, ExternalID: "remote-1"}})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _, err := model.reviewDraft.AddComment(core.ReviewCommentInput{FilePath: "demo.go", Range: core.ReviewLineRange{Start: core.ReviewLineRef{NewLineNumber: 1}, End: core.ReviewLineRef{NewLineNumber: 1}}, Body: "first"}) | |
| require.NoError(t, err) | |
| _, err = model.reviewDraft.AddComment(core.ReviewCommentInput{FilePath: "demo.go", Range: core.ReviewLineRange{Start: core.ReviewLineRef{NewLineNumber: 1}, End: core.ReviewLineRef{NewLineNumber: 1}}, Body: "second"}) | |
| require.NoError(t, err) | |
| model.reviewDraft.ApplyPublishedRefs("github", []core.PublishedReviewCommentRef{{LocalCommentID: "comment-1", ExternalID: "remote-1"}}) | |
| first, err := model.reviewDraft.AddComment(core.ReviewCommentInput{FilePath: "demo.go", Range: core.ReviewLineRange{Start: core.ReviewLineRef{NewLineNumber: 1}, End: core.ReviewLineRef{NewLineNumber: 1}}, Body: "first"}) | |
| require.NoError(t, err) | |
| _, err = model.reviewDraft.AddComment(core.ReviewCommentInput{FilePath: "demo.go", Range: core.ReviewLineRange{Start: core.ReviewLineRef{NewLineNumber: 1}, End: core.ReviewLineRef{NewLineNumber: 1}}, Body: "second"}) | |
| require.NoError(t, err) | |
| model.reviewDraft.ApplyPublishedRefs("github", []core.PublishedReviewCommentRef{{LocalCommentID: first.ID, ExternalID: "remote-1"}}) |
🤖 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 `@internal/adapters/in/tui/model_test.go` around lines 41 - 45, The test
assumes a hardcoded internal ID "comment-1"; instead capture the ID returned
from model.reviewDraft.AddComment and use that when calling
model.reviewDraft.ApplyPublishedRefs so the test validates behavior rather than
ID formatting: change the second AddComment call to store its returned comment
ID (from the first AddComment if you meant the first comment) and pass that
variable as the LocalCommentID inside the core.PublishedReviewCommentRef given
to ApplyPublishedRefs (referencing model.reviewDraft.AddComment return value and
ApplyPublishedRefs and core.PublishedReviewCommentRef/LocalCommentID).
bnema
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Key UX
popens the provider picker.alt+pcycles the active provider directly.Ppublishes the review.rrefreshes provider sync.otoggles the PR sheet.h/left andl/right move between files.Validation
rtk golangci-lint run --timeout=5mrtk go test ./...→ 553 passed / 24 packages