feat: scope GitHub integration settings by workspace#1572
Conversation
📝 WalkthroughWalkthroughThis PR scopes GitHub, Jira, Linear, Sentry, and Slack integration data, handlers, clients, and UI by workspace. It adds workspace-owned settings storage, migration paths, workspace-aware search/watch/polling behavior, and updated frontend/API wiring across all five integrations. ChangesShared workspace infrastructure
GitHub
Jira
Linear
Sentry
Slack
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant Service
participant Store
participant GitHubClient
Controller->>Service: SearchUserPRsPagedForWorkspace(workspaceID, query)
Service->>Store: GetWorkspaceSettings(workspaceID)
Store-->>Service: WorkspaceSettings
Service->>Service: append scope qualifiers to query
Service->>GitHubClient: search PRs with scoped query
GitHubClient-->>Service: PR results
Service->>Service: filterPRsByWorkspaceScope
Service-->>Controller: PRSearchPage
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
Claude finished @carlosflorencio's task in 5m 37s —— View job Code ReviewTodo
FindingsBlocker (must fix before merge)
Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — fix blockers first. The core feature — per-workspace repo scope, migrated presets, and watch filtering — is well-structured. The store roundtrip tests and the scoped watch tests are solid. The two blockers are both small fixes: correct the HTTP status code and add one test method. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| apps/backend/internal/github/workspace_settings_service.go | Core workspace-scoped search and filter logic; introduces scoped PR/issue search with cache, scope qualifiers, and post-filter. Case-insensitive comparison in repoAllowedByWorkspaceScope is correct. |
| apps/backend/internal/github/store.go | Adds github_workspace_settings table and all CRUD operations. Patch uses INSERT OR IGNORE + UPDATE; reads normalize via normalizeWorkspaceSettings. |
| apps/backend/internal/github/workspace_settings_test.go | Good test coverage: round-trip, scope appending, org scope, empty-scope fast-path, partial update, explicit null clear, and watch scope enforcement for both PR and issue pollers. |
| apps/backend/internal/github/controller.go | Adds workspace_settings GET/PUT endpoints and workspace_id-gated variants of PR/issue search and cleanup; IDOR guards added to update/delete watch paths. |
| apps/web/components/github/my-github/use-saved-presets.ts | Adds per-workspace preset storage with two-stage migration. Save/remove are guarded against writing while loading. Workspace sync failures are silently dropped without a retry flag. |
| apps/web/components/github/my-github/use-default-query-presets.ts | Mirrors use-saved-presets for default query presets with the same migration path and the same silent failure concern on network error. |
| apps/web/components/github/repo-filter-selector.tsx | repoAllowedByScope and orgAllowedByScope both use .toLowerCase() on both sides. Workspace scope filters the org badges and repo combobox correctly. |
| apps/backend/internal/github/service_cleanup.go | Adds CleanupReviewTasksForWorkspace and CleanupIssueTasksForWorkspace; fetches all tasks then filters in-memory by workspace watch IDs. |
| apps/backend/internal/github/models.go | Adds WorkspaceSettings and UpdateWorkspaceSettingsRequest types with custom UnmarshalJSON tracking explicit null vs absent fields. |
| apps/web/components/integrations/workspace-scoped-section.tsx | Clean workspace switcher wrapper; override survives until workspace list no longer contains it. |
| apps/web/lib/api/domains/github-api.ts | Adds fetchGitHubWorkspaceSettings and updateGitHubWorkspaceSettings; search functions pass workspace_id when provided. |
| apps/backend/internal/backendapp/e2e_reset.go | Adds cleanup of github_workspace_settings for the test workspace on e2e reset. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant FE as Frontend Hook
participant API as Backend API
participant DB as SQLite
note over FE,DB: First load for a workspace
FE->>API: "GET /workspace-settings?workspace_id=ws1"
API->>DB: "SELECT github_workspace_settings WHERE workspace_id=ws1"
DB-->>API: ErrNoRows (first time)
API-->>FE: "default settings (mode=all, empty orgs/repos)"
note over FE: Migration: push legacy localStorage/user-settings to workspace
FE->>API: "PUT /workspace-settings {workspace_id, saved_presets}"
API->>DB: INSERT OR IGNORE + UPDATE github_workspace_settings
DB-->>API: ok
API-->>FE: WorkspaceSettings
FE->>FE: markMigratedToWorkspace(ws1)
note over FE,DB: Scoped PR search
FE->>API: "GET /user/prs?workspace_id=ws1&query=is:open"
API->>DB: GetWorkspaceSettings(ws1)
DB-->>API: "{mode:repos, repos:[kdlbs/kandev]}"
API->>API: appendWorkspaceScopeToSearch
API->>API: GitHub Search API + filterPRsByWorkspaceScope
API-->>FE: "{prs:[], total_count: N}"
note over FE,DB: IDOR-guarded watch mutation
FE->>API: "PUT /watches/review/:id?workspace_id=ws1"
API->>DB: GetReviewWatch(id)
DB-->>API: "{watch.WorkspaceID=ws1}"
API->>API: requireReviewWatchInWorkspace match check
API->>DB: UpdateReviewWatch
API-->>FE: ReviewWatch
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant FE as Frontend Hook
participant API as Backend API
participant DB as SQLite
note over FE,DB: First load for a workspace
FE->>API: "GET /workspace-settings?workspace_id=ws1"
API->>DB: "SELECT github_workspace_settings WHERE workspace_id=ws1"
DB-->>API: ErrNoRows (first time)
API-->>FE: "default settings (mode=all, empty orgs/repos)"
note over FE: Migration: push legacy localStorage/user-settings to workspace
FE->>API: "PUT /workspace-settings {workspace_id, saved_presets}"
API->>DB: INSERT OR IGNORE + UPDATE github_workspace_settings
DB-->>API: ok
API-->>FE: WorkspaceSettings
FE->>FE: markMigratedToWorkspace(ws1)
note over FE,DB: Scoped PR search
FE->>API: "GET /user/prs?workspace_id=ws1&query=is:open"
API->>DB: GetWorkspaceSettings(ws1)
DB-->>API: "{mode:repos, repos:[kdlbs/kandev]}"
API->>API: appendWorkspaceScopeToSearch
API->>API: GitHub Search API + filterPRsByWorkspaceScope
API-->>FE: "{prs:[], total_count: N}"
note over FE,DB: IDOR-guarded watch mutation
FE->>API: "PUT /watches/review/:id?workspace_id=ws1"
API->>DB: GetReviewWatch(id)
DB-->>API: "{watch.WorkspaceID=ws1}"
API->>API: requireReviewWatchInWorkspace match check
API->>DB: UpdateReviewWatch
API-->>FE: ReviewWatch
Reviews (5): Last reviewed commit: "fix: make workspace switcher look select..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
kandev/apps/web/components/github/github-settings.tsx
Lines 347 to 348 in 7c2d30a
Scope cleanup actions to the selected workspace
These watch sections now live under a single selected workspace and hide the workspace column, but cleanupMergedReviewTasks and the matching issue cleanup still call install-wide backend sweeps. From workspace A's GitHub settings, clicking Clean up merged/closed can delete auto-created tasks owned by watches in workspace B. Pass the selected workspace_id to a scoped cleanup endpoint, or move these buttons to a clearly global area.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
24 issues found across 25 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Claude finished @carlosflorencio's task in 31s —— View job Code ReviewTodo
|
carlosflorencio
force-pushed
the
feature/integrations-per-wor-1rk
branch
from
87c5a84 to
a91f594
Compare
|
Claude finished @carlosflorencio's task in 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
6 issues found across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a91f594bdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
carlosflorencio
force-pushed
the
feature/integrations-per-wor-1rk
branch
from
a91f594 to
d774475
Compare
|
Claude finished @carlosflorencio's task in 5m 43s —— View job Code ReviewTodo
FindingsAll P1 blockers raised in prior review rounds are confirmed fixed in this round. The issues below are the only remaining concerns. Suggestion (recommended, doesn't block)
What this round verified as fixed
Summary
Verdict: Ready to merge — the two suggestions are follow-up scope (one is a pre-existing behavior that's now more noticeable, one is a minor test gap). No blockers remain. |
Code ReviewTodo
|
|
Claude finished @carlosflorencio's task in 3m 3s —— View job Code ReviewTodo
What this round verified as fixedThe last review round raised two required gaps and one suggestion. All three are addressed in commits
The lifecycle complexity fix ( FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge. All blockers from prior rounds are resolved. The two suggestions are low-probability edge cases in simple new caching code and can be followed up separately. |
There was a problem hiding this comment.
5 issues found across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Claude finished @carlosflorencio's task in 3m 33s —— View job Code ReviewTodo
What this round verified as fixedCommit
FindingsNo blockers. No suggestions. The only remaining note from the previous round (that Summary
Verdict: Ready to merge. All blockers from all prior review rounds are resolved, and the latest commit adds the coverage that was still outstanding. |
carlosflorencio
force-pushed
the
feature/integrations-per-wor-1rk
branch
from
31b475e to
3699460
Compare
|
Claude finished @carlosflorencio's task in 3m 15s —— View job Code ReviewTodo
Reviewed commits since last round:
Verdict: Ready to merge. All blockers from all prior review rounds are resolved. |
Code ReviewReviewed commits since last round: FindingsNo blockers. No suggestions. What this round verified
Summary
Verdict: Ready to merge. All blockers from all prior review rounds are resolved. This round closes the remaining gaps: |
|
Claude finished @carlosflorencio's task in 56s —— View job Code ReviewTodo
What this round verifiedOnly one new commit since the previous review round ( All prior findings from every review round remain resolved. FindingsNo findings. No new logic was introduced. Summary
Verdict: Ready to merge. This round is a formatting-only commit; no new issues introduced. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/components/linear/linear-issue-watch-fields.tsx (1)
50-107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope the per-team cache by
workspaceId.
clearWorkspaceScopedFormkeepsteamKey, so switching workspaces can hitfetchedTeams.has(teamKey)and reuse the previous workspace’s states/labels/users for that key. Key these caches by${workspaceId}:${teamKey}or clear them whenworkspaceIdchanges.🤖 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/web/components/linear/linear-issue-watch-fields.tsx` around lines 50 - 107, The per-team fetch cache in the useEffect for linear-issue-watch-fields.tsx is only keyed by teamKey, so switching workspaces can incorrectly reuse stale states/labels/users. Update the cache logic around fetchedTeams and the listLinearStates/listLinearLabels/listLinearUsers loading flow to scope entries by workspaceId as well, using a composite key like workspaceId:teamKey or clearing the cache when workspaceId changes. Make sure the cleanup and fetched.has/fetched.add/fetched.delete paths all use the same workspace-scoped key.apps/web/components/sentry/sentry-issue-dialog.tsx (1)
176-208: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset Sentry dialog state when
workspaceIdchanges. The dialog keepsloaded,projects,issues, andfilterin local state, but only clears them whenopenbecomes false. If the workspace switches while the dialog stays mounted, the old workspace’s projects and search results remain visible, and the next search can run with stale filters. Clear that state onworkspaceIdchanges, or remount the dialog per workspace.🤖 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/web/components/sentry/sentry-issue-dialog.tsx` around lines 176 - 208, Reset the Sentry dialog state when the active workspace changes, not just when `open` becomes false. Update the `useEffect` in `SentryIssueDialog` so a `workspaceId` switch clears `loaded`, `projects`, `issues`, and `filter` before fetching again, or otherwise force the dialog to remount per workspace. Make sure the reset logic is tied to the existing state setters (`setLoaded`, `setProjects`, `setIssues`, `setFilter`) so stale data and filters from the previous workspace cannot persist.apps/backend/internal/linear/handlers.go (1)
58-211: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAdd a workspace ownership gate before using
workspace_id. Inapps/backend/internal/linear/handlers.go:34-45,58-201, these routes takectx.Query("workspace_id")directly and mount without any auth/ownership middleware. That leaves Linear config reads/writes, secret-backed connection tests, and workspace-scoped team/state/label/user searches controlled entirely by a caller-supplied query param. Add caller/workspace validation here or behind shared middleware.🤖 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/internal/linear/handlers.go` around lines 58 - 211, The Linear handlers currently trust ctx.Query("workspace_id") via workspaceID and expose config, test, and workspace-scoped list/search endpoints without any ownership check. Add a caller/workspace validation gate before any use of workspaceID in Controller methods like httpGetConfig, httpSetConfig, httpDeleteConfig, httpTestConfig, httpListTeams, httpListStates, httpListLabels, httpListUsers, and httpSearchIssues, or move the check into shared middleware/service authorization. Ensure unauthorized callers cannot access or mutate data just by supplying a workspace_id query param.
🧹 Nitpick comments (6)
apps/backend/internal/slack/models.go (1)
135-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLegacySecretKeyFor wrappers are redundant.* No in-repo callers use them, and they just forward to
SecretKeyForToken/SecretKeyForCookie. Drop them unless you still need external compatibility.🤖 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/internal/slack/models.go` around lines 135 - 143, The LegacySecretKeyForToken and LegacySecretKeyForCookie wrappers in models.go are redundant passthroughs with no in-repo usage. Remove both wrapper functions and keep the existing SecretKeyForToken and SecretKeyForCookie helpers as the single source of truth, unless there is a confirmed external compatibility requirement.Source: Linters/SAST tools
apps/web/app/jira/jira-page-client.tsx (1)
73-111: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueStale
loaded/configured/projectsduring workspace switch.When
workspaceIdchanges, the effect re-fetches but never resetsloadedtofalsefirst, so the previous workspace'sconfigured/projects/defaultProjectKeyremain visible until the new fetch resolves, rather than showing the "Checking Jira status…" state. Combined with the missing remount above, this can briefly (or, for filters, persistently) display data belonging to the wrong workspace.🤖 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/web/app/jira/jira-page-client.tsx` around lines 73 - 111, Reset the Jira state when useJiraPageData runs for a new workspaceId so stale values from the previous workspace are not shown. In the useEffect inside useJiraPageData, clear loaded, configured, projects, and defaultProjectKey before starting the async load, and keep the existing cancellation handling so the UI returns to the “Checking Jira status…” state while getJiraConfig and loadUserProjects resolve.apps/backend/internal/linear/service.go (1)
278-291: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-workspace auth-health probing could stall the health-check cycle.
RecordAuthHealthloops over every configured workspace and callsRecordAuthHealthForWorkspacesynchronously, each of which can take up toauthProbeTimeout(15s) to complete. With more than a handful of workspaces configured, one health-check tick could take minutes, delaying freshness of the "connected/disconnected" indicator for all workspaces and likely running past the poller's own tick interval.Consider bounding concurrency (e.g.
errgroup.GroupwithSetLimit) so probes run in parallel instead of one-at-a-time.🤖 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/internal/linear/service.go` around lines 278 - 291, RecordAuthHealth currently probes each workspace sequentially by calling RecordAuthHealthForWorkspace in a loop, which can make the health-check tick run too long. Update RecordAuthHealth to run per-workspace probes concurrently with a bounded limit (for example using an errgroup.Group with SetLimit) so the workspace IDs from s.store.ListConfigWorkspaceIDs are processed in parallel without overwhelming the system. Keep the existing empty-workspace path that calls s.fireProbeHook, and preserve the current warning behavior on ListConfigWorkspaceIDs errors.apps/backend/internal/sentry/service.go (2)
284-297: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-workspace auth-health probing.
RecordAuthHealthloops over every configured workspace and callsRecordAuthHealthForWorkspacesynchronously, each with its ownauthProbeTimeout. With N workspaces and a slow/unreachable Sentry instance for some of them, a single health-check pass can take up to N ×authProbeTimeoutinstead of bounding total wall time. Consider probing workspaces concurrently (bounded worker pool) so one slow/unreachable workspace doesn't delay health updates for the rest.🤖 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/internal/sentry/service.go` around lines 284 - 297, RecordAuthHealth currently probes each workspace one after another by calling RecordAuthHealthForWorkspace in a loop, which can make total health-check time scale with the number of workspaces. Update RecordAuthHealth in Service to dispatch per-workspace probes concurrently, ideally with a bounded worker pool or similar limit, while preserving the existing empty-workspace path and the ListConfigWorkspaceIDs error handling.
338-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ListOrganizations/ListProjects/GetIssueweren't given*ForWorkspacesiblings, unlikeSearchIssues.Every other capability in this diff got a public
XxxForWorkspacemethod (GetConfigForWorkspace,SetConfigForWorkspace,DeleteConfigForWorkspace,TestConnectionForWorkspace,ProbeAuthForWorkspace,SearchIssuesForWorkspace). These three did not, sohandlers.goreaches into the unexportedclientFordirectly to get workspace scoping (seehttpListOrganizations/httpListProjects/httpGetIssue), duplicating client-resolution/error-wiring logic that already lives in the service. The unscoped versions here become effectively dead in production (only exercised by tests), which is an inconsistent API surface for future maintainers.func (s *Service) ListOrganizationsForWorkspace(ctx context.Context, workspaceID string) ([]SentryOrganization, error) { workspaceID, err := s.normalizeWorkspaceID(workspaceID) if err != nil { return nil, err } client, err := s.clientFor(ctx, workspaceID) if err != nil { return nil, err } return client.ListOrganizations(ctx) } // ...and analogous ListProjectsForWorkspace / GetIssueForWorkspace, // with handlers.go calling these instead of clientFor directly.🤖 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/internal/sentry/service.go` around lines 338 - 397, Add public workspace-scoped siblings for the unscoped Sentry APIs so the service has a consistent surface: create ListOrganizationsForWorkspace, ListProjectsForWorkspace, and GetIssueForWorkspace alongside SearchIssuesForWorkspace. Each should use normalizeWorkspaceID and clientFor inside Service, then delegate to the matching client method. Update the HTTP handlers in handlers.go to call these new Service methods instead of reaching into clientFor directly, so workspace resolution and error handling live in one place.apps/backend/internal/sentry/handlers.go (1)
96-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClient resolution duplicated across handlers instead of going through the service.
httpListOrganizations,httpListProjects, andhttpGetIssueall callc.service.clientFor(ctx.Request.Context(), c.workspaceID(ctx))directly, then invoke client methods and re-implement error handling thatSearchIssuesForWorkspacealready centralizes in the service. This reaches past the service's public API into an unexported implementation detail (clientFor) from three call sites, so any future change to client-resolution semantics (e.g. additional validation, metrics, or caching) must be updated in the handler layer instead of the service. See the companion comment onservice.goproposingListOrganizationsForWorkspace/ListProjectsForWorkspace/GetIssueForWorkspace.Also applies to: 144-161
🤖 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/internal/sentry/handlers.go` around lines 96 - 122, The handler methods are bypassing the service API by calling the unexported clientFor helper directly and duplicating client/error handling. Update httpListOrganizations, httpListProjects, and httpGetIssue to call the service’s workspace-level methods instead (such as the proposed ListOrganizationsForWorkspace, ListProjectsForWorkspace, and GetIssueForWorkspace) so client resolution and errors stay centralized inside the service. Keep the handlers focused on request/response wiring and remove the direct clientFor usage from Controller.
🤖 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/agent/runtime/lifecycle/manager_interaction.go`:
- Around line 1019-1037: The conditional in
Manager.restoreRecoveredFailedExecution is reading shared execution state
without synchronization, so the status/sessionInitialized/ACPSessionID check can
race with concurrent updates. Move the decision logic into a synchronized read
path by snapshotting execution.Status, execution.sessionInitialized, and
execution.ACPSessionID under executionStore.WithRLock (or an equivalent
lock-protected helper) before deciding whether to call
restoreRecoveredFailedExecution. Keep the existing
restoreRecoveredFailedExecution flow unchanged, but ensure the gate that reaches
it uses locked, consistent values.
In `@apps/backend/internal/integrations/workspacescope/workspacescope.go`:
- Around line 47-50: The error return from getWorkspaceID is inconsistent: the
tableExists/db failure path currently returns FallbackWorkspaceID with a non-nil
error, unlike the other error branches in this function that return an empty
string. Update the early return after tableExists so getWorkspaceID always
returns "" whenever err is set, matching the existing error contract and keeping
callers from mistaking a DB failure for a valid fallback workspace.
In `@apps/backend/internal/jira/service.go`:
- Around line 626-653: revealSecret currently falls back to SecretKey whenever
reading SecretKeyForWorkspace(workspaceID) fails, which can hide real
workspace-store errors and return the wrong secret source. Update revealSecret
in Service so it only tries the legacy SecretKey when the workspace secret is
actually missing/empty, and immediately return any non-missing error from
s.secrets.Reveal for SecretKeyForWorkspace. Keep the fallback behavior limited
to a true not-found case, and preserve the existing nil-secrets handling.
In `@apps/backend/internal/sentry/handlers.go`:
- Around line 159-161: The workspaceID helper currently trusts the raw
workspace_id query param, which allows selecting arbitrary workspaces. Add a
workspace-membership authorization check in the Sentry request flow before any
handler uses workspaceID from the Controller, and reject requests when the
caller is not a member of that workspace. Use the existing workspaceID(ctx)
entry point and the surrounding Sentry handler/controller methods to locate
where to enforce the check so the query param cannot be used to switch
workspaces.
In `@apps/web/app/jira/jira-page-client.tsx`:
- Around line 171-198: AuthenticatedView is preserving filter state across
workspace switches because useFilterState(defaultProjectKey) is not
re-initialized when workspaceId changes. Add a key based on workspaceId to the
AuthenticatedView component so it remounts on workspace change, ensuring
filters.projectKeys resets to the correct defaultProjectKey for the active
workspace and does not carry over stale Jira filters.
---
Outside diff comments:
In `@apps/backend/internal/linear/handlers.go`:
- Around line 58-211: The Linear handlers currently trust
ctx.Query("workspace_id") via workspaceID and expose config, test, and
workspace-scoped list/search endpoints without any ownership check. Add a
caller/workspace validation gate before any use of workspaceID in Controller
methods like httpGetConfig, httpSetConfig, httpDeleteConfig, httpTestConfig,
httpListTeams, httpListStates, httpListLabels, httpListUsers, and
httpSearchIssues, or move the check into shared middleware/service
authorization. Ensure unauthorized callers cannot access or mutate data just by
supplying a workspace_id query param.
In `@apps/web/components/linear/linear-issue-watch-fields.tsx`:
- Around line 50-107: The per-team fetch cache in the useEffect for
linear-issue-watch-fields.tsx is only keyed by teamKey, so switching workspaces
can incorrectly reuse stale states/labels/users. Update the cache logic around
fetchedTeams and the listLinearStates/listLinearLabels/listLinearUsers loading
flow to scope entries by workspaceId as well, using a composite key like
workspaceId:teamKey or clearing the cache when workspaceId changes. Make sure
the cleanup and fetched.has/fetched.add/fetched.delete paths all use the same
workspace-scoped key.
In `@apps/web/components/sentry/sentry-issue-dialog.tsx`:
- Around line 176-208: Reset the Sentry dialog state when the active workspace
changes, not just when `open` becomes false. Update the `useEffect` in
`SentryIssueDialog` so a `workspaceId` switch clears `loaded`, `projects`,
`issues`, and `filter` before fetching again, or otherwise force the dialog to
remount per workspace. Make sure the reset logic is tied to the existing state
setters (`setLoaded`, `setProjects`, `setIssues`, `setFilter`) so stale data and
filters from the previous workspace cannot persist.
---
Nitpick comments:
In `@apps/backend/internal/linear/service.go`:
- Around line 278-291: RecordAuthHealth currently probes each workspace
sequentially by calling RecordAuthHealthForWorkspace in a loop, which can make
the health-check tick run too long. Update RecordAuthHealth to run per-workspace
probes concurrently with a bounded limit (for example using an errgroup.Group
with SetLimit) so the workspace IDs from s.store.ListConfigWorkspaceIDs are
processed in parallel without overwhelming the system. Keep the existing
empty-workspace path that calls s.fireProbeHook, and preserve the current
warning behavior on ListConfigWorkspaceIDs errors.
In `@apps/backend/internal/sentry/handlers.go`:
- Around line 96-122: The handler methods are bypassing the service API by
calling the unexported clientFor helper directly and duplicating client/error
handling. Update httpListOrganizations, httpListProjects, and httpGetIssue to
call the service’s workspace-level methods instead (such as the proposed
ListOrganizationsForWorkspace, ListProjectsForWorkspace, and
GetIssueForWorkspace) so client resolution and errors stay centralized inside
the service. Keep the handlers focused on request/response wiring and remove the
direct clientFor usage from Controller.
In `@apps/backend/internal/sentry/service.go`:
- Around line 284-297: RecordAuthHealth currently probes each workspace one
after another by calling RecordAuthHealthForWorkspace in a loop, which can make
total health-check time scale with the number of workspaces. Update
RecordAuthHealth in Service to dispatch per-workspace probes concurrently,
ideally with a bounded worker pool or similar limit, while preserving the
existing empty-workspace path and the ListConfigWorkspaceIDs error handling.
- Around line 338-397: Add public workspace-scoped siblings for the unscoped
Sentry APIs so the service has a consistent surface: create
ListOrganizationsForWorkspace, ListProjectsForWorkspace, and
GetIssueForWorkspace alongside SearchIssuesForWorkspace. Each should use
normalizeWorkspaceID and clientFor inside Service, then delegate to the matching
client method. Update the HTTP handlers in handlers.go to call these new Service
methods instead of reaching into clientFor directly, so workspace resolution and
error handling live in one place.
In `@apps/backend/internal/slack/models.go`:
- Around line 135-143: The LegacySecretKeyForToken and LegacySecretKeyForCookie
wrappers in models.go are redundant passthroughs with no in-repo usage. Remove
both wrapper functions and keep the existing SecretKeyForToken and
SecretKeyForCookie helpers as the single source of truth, unless there is a
confirmed external compatibility requirement.
In `@apps/web/app/jira/jira-page-client.tsx`:
- Around line 73-111: Reset the Jira state when useJiraPageData runs for a new
workspaceId so stale values from the previous workspace are not shown. In the
useEffect inside useJiraPageData, clear loaded, configured, projects, and
defaultProjectKey before starting the async load, and keep the existing
cancellation handling so the UI returns to the “Checking Jira status…” state
while getJiraConfig and loadUserProjects resolve.
🪄 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: a85a7c5b-20d3-494d-a75a-a80f5fe30a2b
📒 Files selected for processing (114)
apps/backend/internal/agent/runtime/lifecycle/manager_interaction.goapps/backend/internal/backendapp/e2e_reset.goapps/backend/internal/github/controller.goapps/backend/internal/github/handlers.goapps/backend/internal/github/models.goapps/backend/internal/github/service_cleanup.goapps/backend/internal/github/service_issues.goapps/backend/internal/github/service_reviews.goapps/backend/internal/github/store.goapps/backend/internal/github/workspace_settings_service.goapps/backend/internal/github/workspace_settings_test.goapps/backend/internal/integrations/workspacescope/workspacescope.goapps/backend/internal/integrations/workspacescope/workspacescope_test.goapps/backend/internal/jira/handlers.goapps/backend/internal/jira/models.goapps/backend/internal/jira/poller_issue_watch_test.goapps/backend/internal/jira/poller_test.goapps/backend/internal/jira/provider.goapps/backend/internal/jira/service.goapps/backend/internal/jira/service_issue_watch_test.goapps/backend/internal/jira/service_test.goapps/backend/internal/jira/store.goapps/backend/internal/jira/store_test.goapps/backend/internal/linear/handlers.goapps/backend/internal/linear/models.goapps/backend/internal/linear/poller_issue_watch_test.goapps/backend/internal/linear/poller_test.goapps/backend/internal/linear/provider.goapps/backend/internal/linear/service.goapps/backend/internal/linear/service_issue_watch.goapps/backend/internal/linear/service_issue_watch_test.goapps/backend/internal/linear/service_test.goapps/backend/internal/linear/store.goapps/backend/internal/linear/store_test.goapps/backend/internal/sentry/handlers.goapps/backend/internal/sentry/models.goapps/backend/internal/sentry/poller_issue_watch_test.goapps/backend/internal/sentry/poller_test.goapps/backend/internal/sentry/provider.goapps/backend/internal/sentry/service.goapps/backend/internal/sentry/service_issue_watch.goapps/backend/internal/sentry/service_issue_watch_test.goapps/backend/internal/sentry/service_test.goapps/backend/internal/sentry/store.goapps/backend/internal/sentry/store_test.goapps/backend/internal/slack/handlers.goapps/backend/internal/slack/models.goapps/backend/internal/slack/provider.goapps/backend/internal/slack/service.goapps/backend/internal/slack/store.goapps/backend/internal/slack/store_test.goapps/backend/internal/slack/trigger.goapps/backend/internal/slack/trigger_test.goapps/web/app/github/github-page-client.tsxapps/web/app/jira/jira-page-client.tsxapps/web/app/linear/linear-page-client.tsxapps/web/components/app-sidebar/app-sidebar-workspace-picker.test.tsxapps/web/components/app-sidebar/app-sidebar-workspace-picker.tsxapps/web/components/github/default-queries-section.tsxapps/web/components/github/github-repo-scope-section.tsxapps/web/components/github/github-settings.tsxapps/web/components/github/issue-watch-dialog.tsxapps/web/components/github/my-github/use-default-query-presets.test.tsapps/web/components/github/my-github/use-default-query-presets.tsapps/web/components/github/my-github/use-github-search.tsapps/web/components/github/my-github/use-saved-presets.test.tsapps/web/components/github/my-github/use-saved-presets.tsapps/web/components/github/repo-filter-selector.tsxapps/web/components/github/review-watch-dialog.tsxapps/web/components/integrations/workspace-scoped-section.tsxapps/web/components/jira/jira-import-bar.tsxapps/web/components/jira/jira-issue-watch-dialog.tsxapps/web/components/jira/jira-link-button.tsxapps/web/components/jira/jira-settings.tsxapps/web/components/jira/jira-ticket-common.tsxapps/web/components/jira/my-jira/use-jira-search.tsapps/web/components/linear/linear-import-bar.tsxapps/web/components/linear/linear-issue-common.tsxapps/web/components/linear/linear-issue-watch-dialog.tsxapps/web/components/linear/linear-issue-watch-fields.tsxapps/web/components/linear/linear-link-button.tsxapps/web/components/linear/linear-settings.tsxapps/web/components/sentry/sentry-issue-button.tsxapps/web/components/sentry/sentry-issue-dialog.tsxapps/web/components/sentry/sentry-issue-watch-dialog.tsxapps/web/components/sentry/sentry-link-button.tsxapps/web/components/sentry/sentry-settings.tsxapps/web/components/settings/settings-layout-client.tsxapps/web/components/slack/slack-settings.tsxapps/web/components/task/task-top-bar.tsxapps/web/components/task/workspace-switcher.tsxapps/web/e2e/helpers/api-client.tsapps/web/e2e/pages/linear-settings-page.tsapps/web/e2e/tests/integrations/github-workspace-settings.spec.tsapps/web/e2e/tests/integrations/linear-settings.spec.tsapps/web/e2e/tests/integrations/mobile-linear-settings.spec.tsapps/web/e2e/tests/pr/pr-watcher-cleanup-policy.spec.tsapps/web/hooks/domains/github/use-issue-watches.tsapps/web/hooks/domains/github/use-review-watches.tsapps/web/hooks/domains/jira/use-jira-availability.tsapps/web/hooks/domains/linear/use-linear-availability.tsapps/web/hooks/domains/sentry/use-sentry-availability.tsapps/web/hooks/domains/slack/use-slack-availability.tsapps/web/lib/api/domains/github-api.tsapps/web/lib/api/domains/jira-api.tsapps/web/lib/api/domains/linear-api.test.tsapps/web/lib/api/domains/linear-api.tsapps/web/lib/api/domains/sentry-api.tsapps/web/lib/api/domains/slack-api.tsapps/web/lib/types/github.tsapps/web/lib/types/jira.tsapps/web/lib/types/linear.tsapps/web/lib/types/sentry.tsapps/web/lib/types/slack.ts
💤 Files with no reviewable changes (25)
- apps/web/e2e/tests/integrations/mobile-linear-settings.spec.ts
- apps/web/lib/types/slack.ts
- apps/web/lib/types/sentry.ts
- apps/web/e2e/tests/integrations/linear-settings.spec.ts
- apps/web/lib/types/linear.ts
- apps/web/lib/api/domains/linear-api.test.ts
- apps/web/e2e/pages/linear-settings-page.ts
- apps/web/lib/types/jira.ts
- apps/web/e2e/tests/integrations/github-workspace-settings.spec.ts
- apps/web/hooks/domains/linear/use-linear-availability.ts
- apps/web/components/task/task-top-bar.tsx
- apps/web/e2e/tests/pr/pr-watcher-cleanup-policy.spec.ts
- apps/web/hooks/domains/slack/use-slack-availability.ts
- apps/web/hooks/domains/github/use-issue-watches.ts
- apps/web/hooks/domains/sentry/use-sentry-availability.ts
- apps/web/lib/types/github.ts
- apps/web/components/task/workspace-switcher.tsx
- apps/web/hooks/domains/jira/use-jira-availability.ts
- apps/web/hooks/domains/github/use-review-watches.ts
- apps/web/lib/api/domains/sentry-api.ts
- apps/web/lib/api/domains/slack-api.ts
- apps/web/lib/api/domains/linear-api.ts
- apps/web/e2e/helpers/api-client.ts
- apps/web/lib/api/domains/jira-api.ts
- apps/web/lib/api/domains/github-api.ts
✅ Files skipped from review due to trivial changes (3)
- apps/web/components/app-sidebar/app-sidebar-workspace-picker.tsx
- apps/web/components/jira/jira-import-bar.tsx
- apps/backend/internal/jira/service_test.go
🚧 Files skipped from review as they are similar to previous changes (24)
- apps/web/components/github/review-watch-dialog.tsx
- apps/web/components/integrations/workspace-scoped-section.tsx
- apps/web/components/github/issue-watch-dialog.tsx
- apps/web/components/github/default-queries-section.tsx
- apps/backend/internal/backendapp/e2e_reset.go
- apps/backend/internal/github/service_issues.go
- apps/backend/internal/github/service_reviews.go
- apps/web/components/github/my-github/use-saved-presets.test.ts
- apps/backend/internal/github/service_cleanup.go
- apps/web/components/github/my-github/use-default-query-presets.test.ts
- apps/web/components/github/my-github/use-saved-presets.ts
- apps/web/components/github/my-github/use-github-search.ts
- apps/web/components/github/github-repo-scope-section.tsx
- apps/backend/internal/github/handlers.go
- apps/backend/internal/github/workspace_settings_service.go
- apps/web/components/settings/settings-layout-client.tsx
- apps/web/components/github/my-github/use-default-query-presets.ts
- apps/backend/internal/github/models.go
- apps/web/app/github/github-page-client.tsx
- apps/backend/internal/github/workspace_settings_test.go
- apps/web/components/github/github-settings.tsx
- apps/web/components/github/repo-filter-selector.tsx
- apps/backend/internal/github/store.go
- apps/backend/internal/github/controller.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/components/linear/linear-issue-watch-fields.tsx (1)
50-107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope the per-team cache by
workspaceId.
clearWorkspaceScopedFormkeepsteamKey, so switching workspaces can hitfetchedTeams.has(teamKey)and reuse the previous workspace’s states/labels/users for that key. Key these caches by${workspaceId}:${teamKey}or clear them whenworkspaceIdchanges.🤖 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/web/components/linear/linear-issue-watch-fields.tsx` around lines 50 - 107, The per-team fetch cache in the useEffect for linear-issue-watch-fields.tsx is only keyed by teamKey, so switching workspaces can incorrectly reuse stale states/labels/users. Update the cache logic around fetchedTeams and the listLinearStates/listLinearLabels/listLinearUsers loading flow to scope entries by workspaceId as well, using a composite key like workspaceId:teamKey or clearing the cache when workspaceId changes. Make sure the cleanup and fetched.has/fetched.add/fetched.delete paths all use the same workspace-scoped key.apps/web/components/sentry/sentry-issue-dialog.tsx (1)
176-208: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset Sentry dialog state when
workspaceIdchanges. The dialog keepsloaded,projects,issues, andfilterin local state, but only clears them whenopenbecomes false. If the workspace switches while the dialog stays mounted, the old workspace’s projects and search results remain visible, and the next search can run with stale filters. Clear that state onworkspaceIdchanges, or remount the dialog per workspace.🤖 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/web/components/sentry/sentry-issue-dialog.tsx` around lines 176 - 208, Reset the Sentry dialog state when the active workspace changes, not just when `open` becomes false. Update the `useEffect` in `SentryIssueDialog` so a `workspaceId` switch clears `loaded`, `projects`, `issues`, and `filter` before fetching again, or otherwise force the dialog to remount per workspace. Make sure the reset logic is tied to the existing state setters (`setLoaded`, `setProjects`, `setIssues`, `setFilter`) so stale data and filters from the previous workspace cannot persist.apps/backend/internal/linear/handlers.go (1)
58-211: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAdd a workspace ownership gate before using
workspace_id. Inapps/backend/internal/linear/handlers.go:34-45,58-201, these routes takectx.Query("workspace_id")directly and mount without any auth/ownership middleware. That leaves Linear config reads/writes, secret-backed connection tests, and workspace-scoped team/state/label/user searches controlled entirely by a caller-supplied query param. Add caller/workspace validation here or behind shared middleware.🤖 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/internal/linear/handlers.go` around lines 58 - 211, The Linear handlers currently trust ctx.Query("workspace_id") via workspaceID and expose config, test, and workspace-scoped list/search endpoints without any ownership check. Add a caller/workspace validation gate before any use of workspaceID in Controller methods like httpGetConfig, httpSetConfig, httpDeleteConfig, httpTestConfig, httpListTeams, httpListStates, httpListLabels, httpListUsers, and httpSearchIssues, or move the check into shared middleware/service authorization. Ensure unauthorized callers cannot access or mutate data just by supplying a workspace_id query param.
🧹 Nitpick comments (6)
apps/backend/internal/slack/models.go (1)
135-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLegacySecretKeyFor wrappers are redundant.* No in-repo callers use them, and they just forward to
SecretKeyForToken/SecretKeyForCookie. Drop them unless you still need external compatibility.🤖 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/internal/slack/models.go` around lines 135 - 143, The LegacySecretKeyForToken and LegacySecretKeyForCookie wrappers in models.go are redundant passthroughs with no in-repo usage. Remove both wrapper functions and keep the existing SecretKeyForToken and SecretKeyForCookie helpers as the single source of truth, unless there is a confirmed external compatibility requirement.Source: Linters/SAST tools
apps/web/app/jira/jira-page-client.tsx (1)
73-111: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueStale
loaded/configured/projectsduring workspace switch.When
workspaceIdchanges, the effect re-fetches but never resetsloadedtofalsefirst, so the previous workspace'sconfigured/projects/defaultProjectKeyremain visible until the new fetch resolves, rather than showing the "Checking Jira status…" state. Combined with the missing remount above, this can briefly (or, for filters, persistently) display data belonging to the wrong workspace.🤖 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/web/app/jira/jira-page-client.tsx` around lines 73 - 111, Reset the Jira state when useJiraPageData runs for a new workspaceId so stale values from the previous workspace are not shown. In the useEffect inside useJiraPageData, clear loaded, configured, projects, and defaultProjectKey before starting the async load, and keep the existing cancellation handling so the UI returns to the “Checking Jira status…” state while getJiraConfig and loadUserProjects resolve.apps/backend/internal/linear/service.go (1)
278-291: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-workspace auth-health probing could stall the health-check cycle.
RecordAuthHealthloops over every configured workspace and callsRecordAuthHealthForWorkspacesynchronously, each of which can take up toauthProbeTimeout(15s) to complete. With more than a handful of workspaces configured, one health-check tick could take minutes, delaying freshness of the "connected/disconnected" indicator for all workspaces and likely running past the poller's own tick interval.Consider bounding concurrency (e.g.
errgroup.GroupwithSetLimit) so probes run in parallel instead of one-at-a-time.🤖 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/internal/linear/service.go` around lines 278 - 291, RecordAuthHealth currently probes each workspace sequentially by calling RecordAuthHealthForWorkspace in a loop, which can make the health-check tick run too long. Update RecordAuthHealth to run per-workspace probes concurrently with a bounded limit (for example using an errgroup.Group with SetLimit) so the workspace IDs from s.store.ListConfigWorkspaceIDs are processed in parallel without overwhelming the system. Keep the existing empty-workspace path that calls s.fireProbeHook, and preserve the current warning behavior on ListConfigWorkspaceIDs errors.apps/backend/internal/sentry/service.go (2)
284-297: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-workspace auth-health probing.
RecordAuthHealthloops over every configured workspace and callsRecordAuthHealthForWorkspacesynchronously, each with its ownauthProbeTimeout. With N workspaces and a slow/unreachable Sentry instance for some of them, a single health-check pass can take up to N ×authProbeTimeoutinstead of bounding total wall time. Consider probing workspaces concurrently (bounded worker pool) so one slow/unreachable workspace doesn't delay health updates for the rest.🤖 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/internal/sentry/service.go` around lines 284 - 297, RecordAuthHealth currently probes each workspace one after another by calling RecordAuthHealthForWorkspace in a loop, which can make total health-check time scale with the number of workspaces. Update RecordAuthHealth in Service to dispatch per-workspace probes concurrently, ideally with a bounded worker pool or similar limit, while preserving the existing empty-workspace path and the ListConfigWorkspaceIDs error handling.
338-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ListOrganizations/ListProjects/GetIssueweren't given*ForWorkspacesiblings, unlikeSearchIssues.Every other capability in this diff got a public
XxxForWorkspacemethod (GetConfigForWorkspace,SetConfigForWorkspace,DeleteConfigForWorkspace,TestConnectionForWorkspace,ProbeAuthForWorkspace,SearchIssuesForWorkspace). These three did not, sohandlers.goreaches into the unexportedclientFordirectly to get workspace scoping (seehttpListOrganizations/httpListProjects/httpGetIssue), duplicating client-resolution/error-wiring logic that already lives in the service. The unscoped versions here become effectively dead in production (only exercised by tests), which is an inconsistent API surface for future maintainers.func (s *Service) ListOrganizationsForWorkspace(ctx context.Context, workspaceID string) ([]SentryOrganization, error) { workspaceID, err := s.normalizeWorkspaceID(workspaceID) if err != nil { return nil, err } client, err := s.clientFor(ctx, workspaceID) if err != nil { return nil, err } return client.ListOrganizations(ctx) } // ...and analogous ListProjectsForWorkspace / GetIssueForWorkspace, // with handlers.go calling these instead of clientFor directly.🤖 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/internal/sentry/service.go` around lines 338 - 397, Add public workspace-scoped siblings for the unscoped Sentry APIs so the service has a consistent surface: create ListOrganizationsForWorkspace, ListProjectsForWorkspace, and GetIssueForWorkspace alongside SearchIssuesForWorkspace. Each should use normalizeWorkspaceID and clientFor inside Service, then delegate to the matching client method. Update the HTTP handlers in handlers.go to call these new Service methods instead of reaching into clientFor directly, so workspace resolution and error handling live in one place.apps/backend/internal/sentry/handlers.go (1)
96-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClient resolution duplicated across handlers instead of going through the service.
httpListOrganizations,httpListProjects, andhttpGetIssueall callc.service.clientFor(ctx.Request.Context(), c.workspaceID(ctx))directly, then invoke client methods and re-implement error handling thatSearchIssuesForWorkspacealready centralizes in the service. This reaches past the service's public API into an unexported implementation detail (clientFor) from three call sites, so any future change to client-resolution semantics (e.g. additional validation, metrics, or caching) must be updated in the handler layer instead of the service. See the companion comment onservice.goproposingListOrganizationsForWorkspace/ListProjectsForWorkspace/GetIssueForWorkspace.Also applies to: 144-161
🤖 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/internal/sentry/handlers.go` around lines 96 - 122, The handler methods are bypassing the service API by calling the unexported clientFor helper directly and duplicating client/error handling. Update httpListOrganizations, httpListProjects, and httpGetIssue to call the service’s workspace-level methods instead (such as the proposed ListOrganizationsForWorkspace, ListProjectsForWorkspace, and GetIssueForWorkspace) so client resolution and errors stay centralized inside the service. Keep the handlers focused on request/response wiring and remove the direct clientFor usage from Controller.
🤖 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/agent/runtime/lifecycle/manager_interaction.go`:
- Around line 1019-1037: The conditional in
Manager.restoreRecoveredFailedExecution is reading shared execution state
without synchronization, so the status/sessionInitialized/ACPSessionID check can
race with concurrent updates. Move the decision logic into a synchronized read
path by snapshotting execution.Status, execution.sessionInitialized, and
execution.ACPSessionID under executionStore.WithRLock (or an equivalent
lock-protected helper) before deciding whether to call
restoreRecoveredFailedExecution. Keep the existing
restoreRecoveredFailedExecution flow unchanged, but ensure the gate that reaches
it uses locked, consistent values.
In `@apps/backend/internal/integrations/workspacescope/workspacescope.go`:
- Around line 47-50: The error return from getWorkspaceID is inconsistent: the
tableExists/db failure path currently returns FallbackWorkspaceID with a non-nil
error, unlike the other error branches in this function that return an empty
string. Update the early return after tableExists so getWorkspaceID always
returns "" whenever err is set, matching the existing error contract and keeping
callers from mistaking a DB failure for a valid fallback workspace.
In `@apps/backend/internal/jira/service.go`:
- Around line 626-653: revealSecret currently falls back to SecretKey whenever
reading SecretKeyForWorkspace(workspaceID) fails, which can hide real
workspace-store errors and return the wrong secret source. Update revealSecret
in Service so it only tries the legacy SecretKey when the workspace secret is
actually missing/empty, and immediately return any non-missing error from
s.secrets.Reveal for SecretKeyForWorkspace. Keep the fallback behavior limited
to a true not-found case, and preserve the existing nil-secrets handling.
In `@apps/backend/internal/sentry/handlers.go`:
- Around line 159-161: The workspaceID helper currently trusts the raw
workspace_id query param, which allows selecting arbitrary workspaces. Add a
workspace-membership authorization check in the Sentry request flow before any
handler uses workspaceID from the Controller, and reject requests when the
caller is not a member of that workspace. Use the existing workspaceID(ctx)
entry point and the surrounding Sentry handler/controller methods to locate
where to enforce the check so the query param cannot be used to switch
workspaces.
In `@apps/web/app/jira/jira-page-client.tsx`:
- Around line 171-198: AuthenticatedView is preserving filter state across
workspace switches because useFilterState(defaultProjectKey) is not
re-initialized when workspaceId changes. Add a key based on workspaceId to the
AuthenticatedView component so it remounts on workspace change, ensuring
filters.projectKeys resets to the correct defaultProjectKey for the active
workspace and does not carry over stale Jira filters.
---
Outside diff comments:
In `@apps/backend/internal/linear/handlers.go`:
- Around line 58-211: The Linear handlers currently trust
ctx.Query("workspace_id") via workspaceID and expose config, test, and
workspace-scoped list/search endpoints without any ownership check. Add a
caller/workspace validation gate before any use of workspaceID in Controller
methods like httpGetConfig, httpSetConfig, httpDeleteConfig, httpTestConfig,
httpListTeams, httpListStates, httpListLabels, httpListUsers, and
httpSearchIssues, or move the check into shared middleware/service
authorization. Ensure unauthorized callers cannot access or mutate data just by
supplying a workspace_id query param.
In `@apps/web/components/linear/linear-issue-watch-fields.tsx`:
- Around line 50-107: The per-team fetch cache in the useEffect for
linear-issue-watch-fields.tsx is only keyed by teamKey, so switching workspaces
can incorrectly reuse stale states/labels/users. Update the cache logic around
fetchedTeams and the listLinearStates/listLinearLabels/listLinearUsers loading
flow to scope entries by workspaceId as well, using a composite key like
workspaceId:teamKey or clearing the cache when workspaceId changes. Make sure
the cleanup and fetched.has/fetched.add/fetched.delete paths all use the same
workspace-scoped key.
In `@apps/web/components/sentry/sentry-issue-dialog.tsx`:
- Around line 176-208: Reset the Sentry dialog state when the active workspace
changes, not just when `open` becomes false. Update the `useEffect` in
`SentryIssueDialog` so a `workspaceId` switch clears `loaded`, `projects`,
`issues`, and `filter` before fetching again, or otherwise force the dialog to
remount per workspace. Make sure the reset logic is tied to the existing state
setters (`setLoaded`, `setProjects`, `setIssues`, `setFilter`) so stale data and
filters from the previous workspace cannot persist.
---
Nitpick comments:
In `@apps/backend/internal/linear/service.go`:
- Around line 278-291: RecordAuthHealth currently probes each workspace
sequentially by calling RecordAuthHealthForWorkspace in a loop, which can make
the health-check tick run too long. Update RecordAuthHealth to run per-workspace
probes concurrently with a bounded limit (for example using an errgroup.Group
with SetLimit) so the workspace IDs from s.store.ListConfigWorkspaceIDs are
processed in parallel without overwhelming the system. Keep the existing
empty-workspace path that calls s.fireProbeHook, and preserve the current
warning behavior on ListConfigWorkspaceIDs errors.
In `@apps/backend/internal/sentry/handlers.go`:
- Around line 96-122: The handler methods are bypassing the service API by
calling the unexported clientFor helper directly and duplicating client/error
handling. Update httpListOrganizations, httpListProjects, and httpGetIssue to
call the service’s workspace-level methods instead (such as the proposed
ListOrganizationsForWorkspace, ListProjectsForWorkspace, and
GetIssueForWorkspace) so client resolution and errors stay centralized inside
the service. Keep the handlers focused on request/response wiring and remove the
direct clientFor usage from Controller.
In `@apps/backend/internal/sentry/service.go`:
- Around line 284-297: RecordAuthHealth currently probes each workspace one
after another by calling RecordAuthHealthForWorkspace in a loop, which can make
total health-check time scale with the number of workspaces. Update
RecordAuthHealth in Service to dispatch per-workspace probes concurrently,
ideally with a bounded worker pool or similar limit, while preserving the
existing empty-workspace path and the ListConfigWorkspaceIDs error handling.
- Around line 338-397: Add public workspace-scoped siblings for the unscoped
Sentry APIs so the service has a consistent surface: create
ListOrganizationsForWorkspace, ListProjectsForWorkspace, and
GetIssueForWorkspace alongside SearchIssuesForWorkspace. Each should use
normalizeWorkspaceID and clientFor inside Service, then delegate to the matching
client method. Update the HTTP handlers in handlers.go to call these new Service
methods instead of reaching into clientFor directly, so workspace resolution and
error handling live in one place.
In `@apps/backend/internal/slack/models.go`:
- Around line 135-143: The LegacySecretKeyForToken and LegacySecretKeyForCookie
wrappers in models.go are redundant passthroughs with no in-repo usage. Remove
both wrapper functions and keep the existing SecretKeyForToken and
SecretKeyForCookie helpers as the single source of truth, unless there is a
confirmed external compatibility requirement.
In `@apps/web/app/jira/jira-page-client.tsx`:
- Around line 73-111: Reset the Jira state when useJiraPageData runs for a new
workspaceId so stale values from the previous workspace are not shown. In the
useEffect inside useJiraPageData, clear loaded, configured, projects, and
defaultProjectKey before starting the async load, and keep the existing
cancellation handling so the UI returns to the “Checking Jira status…” state
while getJiraConfig and loadUserProjects resolve.
🪄 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: a85a7c5b-20d3-494d-a75a-a80f5fe30a2b
📒 Files selected for processing (114)
apps/backend/internal/agent/runtime/lifecycle/manager_interaction.goapps/backend/internal/backendapp/e2e_reset.goapps/backend/internal/github/controller.goapps/backend/internal/github/handlers.goapps/backend/internal/github/models.goapps/backend/internal/github/service_cleanup.goapps/backend/internal/github/service_issues.goapps/backend/internal/github/service_reviews.goapps/backend/internal/github/store.goapps/backend/internal/github/workspace_settings_service.goapps/backend/internal/github/workspace_settings_test.goapps/backend/internal/integrations/workspacescope/workspacescope.goapps/backend/internal/integrations/workspacescope/workspacescope_test.goapps/backend/internal/jira/handlers.goapps/backend/internal/jira/models.goapps/backend/internal/jira/poller_issue_watch_test.goapps/backend/internal/jira/poller_test.goapps/backend/internal/jira/provider.goapps/backend/internal/jira/service.goapps/backend/internal/jira/service_issue_watch_test.goapps/backend/internal/jira/service_test.goapps/backend/internal/jira/store.goapps/backend/internal/jira/store_test.goapps/backend/internal/linear/handlers.goapps/backend/internal/linear/models.goapps/backend/internal/linear/poller_issue_watch_test.goapps/backend/internal/linear/poller_test.goapps/backend/internal/linear/provider.goapps/backend/internal/linear/service.goapps/backend/internal/linear/service_issue_watch.goapps/backend/internal/linear/service_issue_watch_test.goapps/backend/internal/linear/service_test.goapps/backend/internal/linear/store.goapps/backend/internal/linear/store_test.goapps/backend/internal/sentry/handlers.goapps/backend/internal/sentry/models.goapps/backend/internal/sentry/poller_issue_watch_test.goapps/backend/internal/sentry/poller_test.goapps/backend/internal/sentry/provider.goapps/backend/internal/sentry/service.goapps/backend/internal/sentry/service_issue_watch.goapps/backend/internal/sentry/service_issue_watch_test.goapps/backend/internal/sentry/service_test.goapps/backend/internal/sentry/store.goapps/backend/internal/sentry/store_test.goapps/backend/internal/slack/handlers.goapps/backend/internal/slack/models.goapps/backend/internal/slack/provider.goapps/backend/internal/slack/service.goapps/backend/internal/slack/store.goapps/backend/internal/slack/store_test.goapps/backend/internal/slack/trigger.goapps/backend/internal/slack/trigger_test.goapps/web/app/github/github-page-client.tsxapps/web/app/jira/jira-page-client.tsxapps/web/app/linear/linear-page-client.tsxapps/web/components/app-sidebar/app-sidebar-workspace-picker.test.tsxapps/web/components/app-sidebar/app-sidebar-workspace-picker.tsxapps/web/components/github/default-queries-section.tsxapps/web/components/github/github-repo-scope-section.tsxapps/web/components/github/github-settings.tsxapps/web/components/github/issue-watch-dialog.tsxapps/web/components/github/my-github/use-default-query-presets.test.tsapps/web/components/github/my-github/use-default-query-presets.tsapps/web/components/github/my-github/use-github-search.tsapps/web/components/github/my-github/use-saved-presets.test.tsapps/web/components/github/my-github/use-saved-presets.tsapps/web/components/github/repo-filter-selector.tsxapps/web/components/github/review-watch-dialog.tsxapps/web/components/integrations/workspace-scoped-section.tsxapps/web/components/jira/jira-import-bar.tsxapps/web/components/jira/jira-issue-watch-dialog.tsxapps/web/components/jira/jira-link-button.tsxapps/web/components/jira/jira-settings.tsxapps/web/components/jira/jira-ticket-common.tsxapps/web/components/jira/my-jira/use-jira-search.tsapps/web/components/linear/linear-import-bar.tsxapps/web/components/linear/linear-issue-common.tsxapps/web/components/linear/linear-issue-watch-dialog.tsxapps/web/components/linear/linear-issue-watch-fields.tsxapps/web/components/linear/linear-link-button.tsxapps/web/components/linear/linear-settings.tsxapps/web/components/sentry/sentry-issue-button.tsxapps/web/components/sentry/sentry-issue-dialog.tsxapps/web/components/sentry/sentry-issue-watch-dialog.tsxapps/web/components/sentry/sentry-link-button.tsxapps/web/components/sentry/sentry-settings.tsxapps/web/components/settings/settings-layout-client.tsxapps/web/components/slack/slack-settings.tsxapps/web/components/task/task-top-bar.tsxapps/web/components/task/workspace-switcher.tsxapps/web/e2e/helpers/api-client.tsapps/web/e2e/pages/linear-settings-page.tsapps/web/e2e/tests/integrations/github-workspace-settings.spec.tsapps/web/e2e/tests/integrations/linear-settings.spec.tsapps/web/e2e/tests/integrations/mobile-linear-settings.spec.tsapps/web/e2e/tests/pr/pr-watcher-cleanup-policy.spec.tsapps/web/hooks/domains/github/use-issue-watches.tsapps/web/hooks/domains/github/use-review-watches.tsapps/web/hooks/domains/jira/use-jira-availability.tsapps/web/hooks/domains/linear/use-linear-availability.tsapps/web/hooks/domains/sentry/use-sentry-availability.tsapps/web/hooks/domains/slack/use-slack-availability.tsapps/web/lib/api/domains/github-api.tsapps/web/lib/api/domains/jira-api.tsapps/web/lib/api/domains/linear-api.test.tsapps/web/lib/api/domains/linear-api.tsapps/web/lib/api/domains/sentry-api.tsapps/web/lib/api/domains/slack-api.tsapps/web/lib/types/github.tsapps/web/lib/types/jira.tsapps/web/lib/types/linear.tsapps/web/lib/types/sentry.tsapps/web/lib/types/slack.ts
💤 Files with no reviewable changes (25)
- apps/web/e2e/tests/integrations/mobile-linear-settings.spec.ts
- apps/web/lib/types/slack.ts
- apps/web/lib/types/sentry.ts
- apps/web/e2e/tests/integrations/linear-settings.spec.ts
- apps/web/lib/types/linear.ts
- apps/web/lib/api/domains/linear-api.test.ts
- apps/web/e2e/pages/linear-settings-page.ts
- apps/web/lib/types/jira.ts
- apps/web/e2e/tests/integrations/github-workspace-settings.spec.ts
- apps/web/hooks/domains/linear/use-linear-availability.ts
- apps/web/components/task/task-top-bar.tsx
- apps/web/e2e/tests/pr/pr-watcher-cleanup-policy.spec.ts
- apps/web/hooks/domains/slack/use-slack-availability.ts
- apps/web/hooks/domains/github/use-issue-watches.ts
- apps/web/hooks/domains/sentry/use-sentry-availability.ts
- apps/web/lib/types/github.ts
- apps/web/components/task/workspace-switcher.tsx
- apps/web/hooks/domains/jira/use-jira-availability.ts
- apps/web/hooks/domains/github/use-review-watches.ts
- apps/web/lib/api/domains/sentry-api.ts
- apps/web/lib/api/domains/slack-api.ts
- apps/web/lib/api/domains/linear-api.ts
- apps/web/e2e/helpers/api-client.ts
- apps/web/lib/api/domains/jira-api.ts
- apps/web/lib/api/domains/github-api.ts
✅ Files skipped from review due to trivial changes (3)
- apps/web/components/app-sidebar/app-sidebar-workspace-picker.tsx
- apps/web/components/jira/jira-import-bar.tsx
- apps/backend/internal/jira/service_test.go
🚧 Files skipped from review as they are similar to previous changes (24)
- apps/web/components/github/review-watch-dialog.tsx
- apps/web/components/integrations/workspace-scoped-section.tsx
- apps/web/components/github/issue-watch-dialog.tsx
- apps/web/components/github/default-queries-section.tsx
- apps/backend/internal/backendapp/e2e_reset.go
- apps/backend/internal/github/service_issues.go
- apps/backend/internal/github/service_reviews.go
- apps/web/components/github/my-github/use-saved-presets.test.ts
- apps/backend/internal/github/service_cleanup.go
- apps/web/components/github/my-github/use-default-query-presets.test.ts
- apps/web/components/github/my-github/use-saved-presets.ts
- apps/web/components/github/my-github/use-github-search.ts
- apps/web/components/github/github-repo-scope-section.tsx
- apps/backend/internal/github/handlers.go
- apps/backend/internal/github/workspace_settings_service.go
- apps/web/components/settings/settings-layout-client.tsx
- apps/web/components/github/my-github/use-default-query-presets.ts
- apps/backend/internal/github/models.go
- apps/web/app/github/github-page-client.tsx
- apps/backend/internal/github/workspace_settings_test.go
- apps/web/components/github/github-settings.tsx
- apps/web/components/github/repo-filter-selector.tsx
- apps/backend/internal/github/store.go
- apps/backend/internal/github/controller.go
🛑 Comments failed to post (5)
apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go (1)
1019-1037: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check whether AgentExecution field reads elsewhere are consistently guarded via WithLock/Get, # and whether Status/ACPSessionID are ever mutated concurrently outside WithLock. rg -nP 'execution\.(Status|sessionInitialized|ACPSessionID)\b' apps/backend/internal/agent/runtime/lifecycle -g '*.go' -C2Repository: kdlbs/kandev
Length of output: 23957
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the execution store and the relevant manager methods around the flagged reads/writes. files=( apps/backend/internal/agent/runtime/lifecycle/execution_store.go apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go apps/backend/internal/agent/runtime/lifecycle/session.go apps/backend/internal/agent/runtime/lifecycle/manager_launch.go ) for f in "${files[@]}"; do echo "===== $f =====" ast-grep outline "$f" --view expanded || true echo done echo "===== execution_store.go:1-260 =====" sed -n '1,260p' apps/backend/internal/agent/runtime/lifecycle/execution_store.go echo "===== manager_interaction.go:960-1055 =====" sed -n '960,1055p' apps/backend/internal/agent/runtime/lifecycle/manager_interaction.goRepository: kdlbs/kandev
Length of output: 25155
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find the exact locking API surface and all methods that mutate/read execution state. rg -n "WithLock|GetBy|GetOr|UpdateStatus|sessionInitialized|ACPSessionID|Status =" apps/backend/internal/agent/runtime/lifecycle/*.goRepository: kdlbs/kandev
Length of output: 35120
Read the execution flags under the store lock.
execution.Status,execution.sessionInitialized, andexecution.ACPSessionIDare shared mutable fields and are written underexecutionStore.WithLock, so this check can race with a concurrent transition. Snapshot them underWithRLock(or another synchronized path) before deciding whether to restore the failed execution.🤖 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/internal/agent/runtime/lifecycle/manager_interaction.go` around lines 1019 - 1037, The conditional in Manager.restoreRecoveredFailedExecution is reading shared execution state without synchronization, so the status/sessionInitialized/ACPSessionID check can race with concurrent updates. Move the decision logic into a synchronized read path by snapshotting execution.Status, execution.sessionInitialized, and execution.ACPSessionID under executionStore.WithRLock (or an equivalent lock-protected helper) before deciding whether to call restoreRecoveredFailedExecution. Keep the existing restoreRecoveredFailedExecution flow unchanged, but ensure the gate that reaches it uses locked, consistent values.apps/backend/internal/integrations/workspacescope/workspacescope.go (1)
47-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent error contract: returns non-zero value alongside error.
Line 49 returns
FallbackWorkspaceIDtogether with a non-nilerr, unlike every other error path in this function (lines 52, 62) which correctly return"". All current callers checkerrfirst, so there's no live bug, but this asymmetry is a latent trap for a migration-critical function—a future caller that inspects the value before the error would silently treat a real DB failure as "table missing" and migrate data into the wrong workspace.🔧 Proposed fix
hasWorkspaces, err := tableExists(db, "workspaces") - if err != nil || !hasWorkspaces { - return FallbackWorkspaceID, err + if err != nil { + return "", err + } + if !hasWorkspaces { + return FallbackWorkspaceID, 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.hasWorkspaces, err := tableExists(db, "workspaces") if err != nil { return "", err } if !hasWorkspaces { return FallbackWorkspaceID, 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 `@apps/backend/internal/integrations/workspacescope/workspacescope.go` around lines 47 - 50, The error return from getWorkspaceID is inconsistent: the tableExists/db failure path currently returns FallbackWorkspaceID with a non-nil error, unlike the other error branches in this function that return an empty string. Update the early return after tableExists so getWorkspaceID always returns "" whenever err is set, matching the existing error contract and keeping callers from mistaking a DB failure for a valid fallback workspace.apps/backend/internal/jira/service.go (1)
626-653: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Look for where the legacy jira SecretKey is migrated/deleted. rg -nP -C3 'SecretKey\b' apps/backend/internal/jira rg -nP -C3 'SecretKeyForWorkspace|secrets\.(Delete|Set|Reveal)\(' apps/backend/internal/jira/provider.go apps/backend/internal/jira/store.goRepository: kdlbs/kandev
Length of output: 5224
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the Jira provider migration and the service call sites around secret access. sed -n '1,140p' apps/backend/internal/jira/provider.go printf '\n---\n' sed -n '600,680p' apps/backend/internal/jira/service.go printf '\n---\n' sed -n '1,220p' apps/backend/internal/jira/service_test.go | sed -n '360,430p' printf '\n---\n' sed -n '1,140p' apps/backend/internal/jira/poller_test.goRepository: kdlbs/kandev
Length of output: 10389
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find all uses of revealSecret/secretExists and workspace binding logic. rg -n -C3 'revealSecret\(|secretExists\(|SetConfigForWorkspace|DeleteConfigForWorkspace|GetConfigForWorkspace|GetConfig\(' apps/backend/internal/jiraRepository: kdlbs/kandev
Length of output: 24093
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the store migration path that reports MigratedFromWorkspace. sed -n '1,240p' apps/backend/internal/jira/store.go printf '\n---\n' sed -n '240,420p' apps/backend/internal/jira/store.goRepository: kdlbs/kandev
Length of output: 13968
Only fall back to the legacy secret when the workspace key is missing
revealSecrettreats any error readingSecretKeyForWorkspace(...)as a miss, so a transient workspace-store failure can be masked by the legacySecretKeyand return the wrong credential source.🤖 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/internal/jira/service.go` around lines 626 - 653, revealSecret currently falls back to SecretKey whenever reading SecretKeyForWorkspace(workspaceID) fails, which can hide real workspace-store errors and return the wrong secret source. Update revealSecret in Service so it only tries the legacy SecretKey when the workspace secret is actually missing/empty, and immediately return any non-missing error from s.secrets.Reveal for SecretKeyForWorkspace. Keep the fallback behavior limited to a true not-found case, and preserve the existing nil-secrets handling.apps/backend/internal/sentry/handlers.go (1)
159-161: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Look for workspace-membership/ownership middleware wired around the Sentry routes. fd -a 'controller.go' apps/backend/internal/sentry rg -n 'RegisterHTTPRoutes|Group\(' apps/backend/internal/sentry/controller.go apps/backend/internal/sentry/handlers.go rg -ni 'RequireWorkspace|workspaceAccess|WorkspaceMembership|workspace_id' apps/backend --iglob '*.go' -g '!*_test.go' | rg -i 'sentry|middleware|router'Repository: kdlbs/kandev
Length of output: 4069
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect Sentry route wiring and workspace-scoping helpers. sed -n '1,140p' apps/backend/internal/sentry/handlers.go printf '\n---\n' sed -n '1,260p' apps/backend/internal/sentry/handlers_issue_watch.go printf '\n---\n' sed -n '1,220p' apps/backend/internal/sentry/handlers_config.go 2>/dev/null || true printf '\n---\n' sed -n '1,220p' apps/backend/internal/sentry/handlers_issue.go 2>/dev/null || trueRepository: kdlbs/kandev
Length of output: 10684
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check whether Sentry routes are wrapped by workspace-membership middleware or auth gates elsewhere. rg -n 'RegisterRoutes\(|RegisterHTTPRoutes\(|Use\(|middleware|auth|workspace|membership|ownership|RequireWorkspace|RequireAuth' apps/backend/internal apps/backend/cmd apps/backend -g '!**/*_test.go'Repository: kdlbs/kandev
Length of output: 50369
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the Sentry service methods to see whether they validate workspace access internally. sed -n '1,260p' apps/backend/internal/sentry/service.go printf '\n---\n' sed -n '260,560p' apps/backend/internal/sentry/service.goRepository: kdlbs/kandev
Length of output: 18525
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the top-level backend route setup for any router-level middleware before integration routes are registered. fd -a 'main.go|server.go|router.go|routes.go' apps/backendRepository: kdlbs/kandev
Length of output: 943
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect Sentry service methods for any workspace membership checks or caller authorization. sed -n '1,260p' apps/backend/internal/sentry/service.go printf '\n---\n' sed -n '260,620p' apps/backend/internal/sentry/service.goRepository: kdlbs/kandev
Length of output: 20463
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find where backend routers are initialized and whether any auth/membership middleware is applied globally. fd -a 'main.go|server.go|router.go|routes.go' apps/backend printf '\n---\n' rg -n 'Use\(|Middleware|auth|session|workspace|membership|ownership|RegisterRoutes\(' apps/backend -g '!**/*_test.go' | sed -n '1,220p'Repository: kdlbs/kandev
Length of output: 24025
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect top-level backend route setup to see what middleware protects integration routes. git ls-files 'apps/backend/**/*.go' | rg -n 'main.go|server.go|router.go|routes.go|http|router|route' printf '\n---\n' rg -n 'gin\.New|Use\(|RegisterRoutes\(|RegisterHTTPRoutes\(' apps/backend -g '!**/*_test.go'Repository: kdlbs/kandev
Length of output: 16585
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the backend router entrypoint and inspect the surrounding setup. fd -a 'main.go|server.go|router.go|routes.go' apps/backendRepository: kdlbs/kandev
Length of output: 943
Add a workspace access check before using
workspace_id
The Sentry routes are mounted directly on the shared backend router, which only adds logging/tracing/recovery/CORS. Add a workspace-membership check before usingworkspace_id; otherwise callers can switch workspaces by changing the query param.🤖 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/internal/sentry/handlers.go` around lines 159 - 161, The workspaceID helper currently trusts the raw workspace_id query param, which allows selecting arbitrary workspaces. Add a workspace-membership authorization check in the Sentry request flow before any handler uses workspaceID from the Controller, and reject requests when the caller is not a member of that workspace. Use the existing workspaceID(ctx) entry point and the surrounding Sentry handler/controller methods to locate where to enforce the check so the query param cannot be used to switch workspaces.apps/web/app/jira/jira-page-client.tsx (1)
171-198: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## jira-page-client outline\n' ast-grep outline apps/web/app/jira/jira-page-client.tsx --view expanded || true printf '\n## jira-page-client relevant ranges\n' sed -n '150,290p' apps/web/app/jira/jira-page-client.tsx printf '\n---\n' sed -n '290,380p' apps/web/app/jira/jira-page-client.tsx printf '\n## jira-settings outline\n' ast-grep outline apps/web/components/jira/jira-settings.tsx --view expanded || true printf '\n## jira-settings relevant ranges\n' sed -n '1,220p' apps/web/components/jira/jira-settings.tsxRepository: kdlbs/kandev
Length of output: 18557
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## JiraPageClient usages\n' rg -n "JiraPageClient" apps/web -g '!**/node_modules/**' || true printf '\n## workspaceId prop flow around Jira page route\n' rg -n "workspaceId" apps/web/app/jira apps/web/components/jira -g '!**/node_modules/**' || true printf '\n## route/page files for jira\n' git ls-files 'apps/web/app/jira/**' 'apps/web/components/jira/**' | sed -n '1,120p'Repository: kdlbs/kandev
Length of output: 13969
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## apps/web/app/jira/page.tsx\n' sed -n '1,120p' apps/web/app/jira/page.tsx printf '\n## apps/web/src/spa-routes.tsx around jira route\n' sed -n '330,380p' apps/web/src/spa-routes.tsxRepository: kdlbs/kandev
Length of output: 4161
Remount
AuthenticatedViewon workspace change. Addkey={workspaceId}souseFilterState(defaultProjectKey)re-initializes when the active workspace changes; otherwise the previous workspace’s default project can persist infilters.projectKeysand show a stale Jira filter.🤖 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/web/app/jira/jira-page-client.tsx` around lines 171 - 198, AuthenticatedView is preserving filter state across workspace switches because useFilterState(defaultProjectKey) is not re-initialized when workspaceId changes. Add a key based on workspaceId to the AuthenticatedView component so it remounts on workspace change, ensuring filters.projectKeys resets to the correct defaultProjectKey for the active workspace and does not carry over stale Jira filters.
|
Claude finished @carlosflorencio's task in 2m 38s —— View job Code ReviewTodo
Reviewed one new commit since last round: What this round verified
FindingsNo blockers. No suggestions. Summary
Verdict: Ready to merge. This commit scopes the remaining E2E test infrastructure (mock auth-health, project statuses, cleanup, API helpers) to workspaces, completing the test coverage layer for all four integrations. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/e2e/fixtures/test-base.ts (1)
196-212: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch the six cleanup DELETEs to cut per-test overhead.
The DELETE calls are independent and already individually swallow errors — running them in parallel via
Promise.all(alongside the existing mock-reset batch) would reduce latency added to every test by thisauto: truefixture.⚡ Suggested batching
async ({ apiClient, seedData }, use) => { const scoped = `workspace_id=${encodeURIComponent(seedData.workspaceId)}`; - await apiClient.rawRequest("DELETE", `/api/v1/jira/config?${scoped}`).catch(() => undefined); - await apiClient - .rawRequest("DELETE", `/api/v1/linear/config?${scoped}`) - .catch(() => undefined); - await apiClient - .rawRequest("DELETE", `/api/v1/sentry/config?${scoped}`) - .catch(() => undefined); - await apiClient.rawRequest("DELETE", `/api/v1/jira/config`).catch(() => undefined); - await apiClient.rawRequest("DELETE", `/api/v1/linear/config`).catch(() => undefined); - await apiClient.rawRequest("DELETE", `/api/v1/sentry/config`).catch(() => undefined); - await Promise.all([ + await Promise.all([ + apiClient.rawRequest("DELETE", `/api/v1/jira/config?${scoped}`).catch(() => undefined), + apiClient.rawRequest("DELETE", `/api/v1/linear/config?${scoped}`).catch(() => undefined), + apiClient.rawRequest("DELETE", `/api/v1/sentry/config?${scoped}`).catch(() => undefined), + apiClient.rawRequest("DELETE", `/api/v1/jira/config`).catch(() => undefined), + apiClient.rawRequest("DELETE", `/api/v1/linear/config`).catch(() => undefined), + apiClient.rawRequest("DELETE", `/api/v1/sentry/config`).catch(() => undefined), apiClient.mockJiraReset().catch(() => undefined), apiClient.mockLinearReset().catch(() => undefined), apiClient.mockSentryReset().catch(() => undefined), ]);Based on learnings, teardown here is intentionally best-effort with swallowed failures, which this refactor preserves.
🤖 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/web/e2e/fixtures/test-base.ts` around lines 196 - 212, The teardown in the auto fixture currently runs six independent DELETE requests sequentially in the cleanup block, adding unnecessary per-test latency. Refactor the cleanup around the scoped `workspace_id` and the three unscoped `/api/v1/*/config` deletes to execute in a single `Promise.all`, similar to the existing `mockJiraReset`/`mockLinearReset`/`mockSentryReset` batch, while keeping the current best-effort `.catch(() => undefined)` behavior on each request.Source: Learnings
🤖 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/web/components/jira/my-jira/use-project-statuses.test.ts`:
- Around line 5-8: The mock in use-project-statuses.test.ts is using the old
vi.fn<[...], ...>() generic form, which is not supported in Vitest 1.6. Update
listJiraProjectStatusesMock to use the single function-signature generic style
accepted by Vitest 1.6 so the typecheck stays green, and keep the mock’s
parameter and promise return types aligned with the existing
listJiraProjectStatuses usage.
---
Nitpick comments:
In `@apps/web/e2e/fixtures/test-base.ts`:
- Around line 196-212: The teardown in the auto fixture currently runs six
independent DELETE requests sequentially in the cleanup block, adding
unnecessary per-test latency. Refactor the cleanup around the scoped
`workspace_id` and the three unscoped `/api/v1/*/config` deletes to execute in a
single `Promise.all`, similar to the existing
`mockJiraReset`/`mockLinearReset`/`mockSentryReset` batch, while keeping the
current best-effort `.catch(() => undefined)` behavior on each request.
🪄 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: 2cc0e7d3-46e9-4fc9-a07c-6da02281fd36
📒 Files selected for processing (8)
apps/backend/internal/jira/mock_controller.goapps/backend/internal/linear/mock_controller.goapps/backend/internal/sentry/mock_controller.goapps/web/app/jira/jira-page-client.tsxapps/web/components/jira/my-jira/use-project-statuses.test.tsapps/web/components/jira/my-jira/use-project-statuses.tsapps/web/e2e/fixtures/test-base.tsapps/web/e2e/helpers/api-client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/jira/jira-page-client.tsx
1 participant
Workspace-specific integration setup was global enough that GitHub defaults, scopes, and watch behavior could bleed across workspaces. This adds a workspace-aware settings layer so each workspace can choose all GitHub repos, selected orgs, or selected repos while existing global settings migrate forward through the first workspace path.
Important Changes
Validation
make fmtmake typecheckmake testmake lintgo test ./internal/githubpnpm e2e:run --host tests/integrations/github-workspace-settings.spec.ts -- --project=chromiumPossible Improvements
Medium risk: Linear and other integrations still need to be moved onto the same workspace-scoped settings pattern.
Closes #1481
Checklist
apps/web/), I have added or updated Playwright e2e tests inapps/web/e2e/and verified them withmake test-e2e.