fix(mobile): recover the provider review inbox when a connection needs reconnecting by iscekic · Pull Request #7895 · Kilo-Org/cloud

iscekic · 2026-10-17T14:16:20Z

Evidence

Evidence: demonstrated — an Android emulator run against the local dev stack, with the provider APIs and token brokers stubbed at the network boundary, showed the inbox reconnect notice where the permanent permission state had been, the Check-connection action re-checking the failing provider for that member, and the review list after the provider answered again.

Change

An organization member whose provider connection broke got a dead end in the provider review inbox: the read layer classified the failure as forbidden, the router surfaced FORBIDDEN, and the inbox rendered its permanent "you don't have permission to view this content" state — no list, no retry, no way out. The member's Kilo access was never the problem; the provider connection was.

Main now carries the GitLab half of that classification (an organization connection's 403 on a read resolves to reconnect), so this branch keeps the parts that main does not have and merges cleanly onto it:

Bitbucket: a released workspace token that is refused (401), an integration that reports reconnect_required, a workspace whose repository scope is insufficient, and a provider 403 on the inbox collection all classify as reconnect instead of forbidden/not_found. A genuine per-pull-request refusal keeps its permission semantics elsewhere.
GitLab inbox: listInbox reads the provider status before the owner-aware classification and answers every 403 on that read with reconnect and inbox copy — the list request names no merge request, so the connection wording must not either.
Router: providerReview maps reconnect to PRECONDITION_FAILED, and the branch listing maps both provider error types the same way.
Mobile: the merged inbox reports which provider produced its terminal state, and the reconnect notice re-checks THAT provider's connection in the caller's organization scope. Before, the inbox had no route provider scope, so the notice always re-checked GitHub — which cannot fix an expired GitLab or Bitbucket connection.
The mobile query-state type is exported from its classifier so screens and selectors share the name instead of inferring it.

No copy or catalog changes: the reconnect notice already carried the GitLab and Bitbucket titles and the "Check connection" action.

End-to-end evidence (Android emulator on the work VM)

Local dev stack (postgres + nextjs + Metro) with a seeded fixture organization and a signed-in member. Both forge APIs and all three token brokers are served by one local stub at the network boundary, so the provider collection can be refused and then served in the same run. The member's GitLab and Bitbucket inbox collections were refused with 403 first, then allowed.

Observed on the emulator, organization scope:

The inbox shows the GitLab reconnect notice — "GitLab connection expired" with a "Check connection" action — not the permission state.
Tapping "Check connection" re-checks the ORGANIZATION GitLab status for this member and re-invalidates the inbox; the next reads still answer PRECONDITION_FAILED.
With the providers answering again, the same Check-connection tap re-runs the inbox and it lists both the GitLab merge request and the Bitbucket pull request of that organization.
Personal scope still renders the normal "No review requests" empty state.

Backend observed for the same run: organizations.reviewAgent.getGitLabStatus for the member (no GitHub authorization check), and providerReview.listInbox answering 412 / PRECONDITION_FAILED — no FORBIDDEN on the inbox read.

Driver results against the real tRPC router (same stack, same stub):

PASS gitlab inbox answers PRECONDITION_FAILED for a refused connection
PASS gitlab inbox reconnect message names the connection
PASS bitbucket inbox answers PRECONDITION_FAILED for a refused connection
PASS bitbucket inbox reconnect message names the connection
PASS the stub served the gitlab inbox collection (scope=reviews_for_me)
PASS the stub served the bitbucket inbox collection
PASS gitlab inbox lists the member's review requests
PASS gitlab inbox row carries the project identity
PASS bitbucket inbox lists the workspace's open pull requests
PASS bitbucket inbox row carries the repository identity
ALL PASS

Provider boundary: the run exercised GET /api/v4/user, GET /api/v4/merge_requests?scope=reviews_for_me, GET /2.0/repositories/{workspace} and GET /2.0/repositories/{workspace}/{repo}/pullrequests, plus the GitLab and Bitbucket workspace token broker calls. Real provider accounts, review-job dispatch, webhooks and model calls were not exercised and stay source-inspected.

Tests

bitbucket-authorization.test.ts / bitbucket-read.test.ts: broker refusal, 401, Revoke-style reconnect_required, insufficient repository scope and an inbox 403 all classify as reconnect; a missing integration stays not-found; listInbox reclassifies a provider 403.
gitlab-read.test.ts: listInbox reclassifies a provider 403 as reconnect for a personal connection and keeps the inbox wording for an organization connection.
gitlab-authorization.test.ts, provider-review-router.test.ts, cloud-agent-next-router.branches.test.ts: unchanged behaviour after the merge; the router maps reconnect to PRECONDITION_FAILED and a real refusal to FORBIDDEN on both the read and inbox paths.
use-provider-inbox.test.ts: the merge names the provider of the terminal state, only when every enabled provider failed.
pr-review-inbox-list.mounted.test.tsx: the reconnect state passes the failing platform and organization to the notice and renders no permission view.

Verified on the work VM after the merge: apps/web jest for the four provider-review suites (129 tests) and the two router suites (46 tests), apps/mobile vitest for src/lib/pr-review + src/components/pr-review (1438 tests), tsgo --noEmit for apps/web, tsc --noEmit for apps/mobile, oxlint on the changed files.

Proxy answers

The owner's proxy gave these answers in the owner's place.

Question: Provider 403: A provider's 403 on the inbox LIST call (no specific PR is involved) is currently turned into the same permanent 'access denied' the member saw. Should the inbox treat a provider 403 as a connection problem (reconnect state with a Check-connection action), while keeping 403 as a real permission refusal on PR detail screens?
Answer: Reconnect on inbox 403 only
Confidence: 0.65 · Precedent: Fix the root cause and the class of the issue: session 7f3470e0ba6f, "fix class of issue preventing progress". An inbox list 403 has no PR context, and detail-screen permission semantics stay intact.
Question: Status check: The org GitLab status endpoint reports connected: true from the integration row alone, while the credential broker may refuse it. Should the status endpoint also ask the broker whether a usable credential exists?
Answer: No, fix the classification
Confidence: 0.55 · Precedent: Do the least work possible with no drop in quality: session 7f3470e0ba6f, "do the least amount of work possible, with no drop in quality". It also avoids a broker round trip on a hot status path, per the performance-reviewer rule.
Question: Bitbucket state: Bitbucket's 'connection no longer active / needs reconnect' results are currently classified as not-found (a dead-end state on mobile). Align them with the same reconnect state as GitLab?
Answer: Yes, one reconnect meaning
Confidence: 0.80 · Precedent: Keep similar elements consistent across surfaces, and fix the class: session 00eb2a1d3e4b, "if anything repeats the same mistake over and over again, fix the class".

Screenshots are attached in a separate comment.

A provider connection failure (credential refused by the broker, a rejected token, an inactive integration, or a missing repository scope) was classified as the provider 'forbidden' kind and surfaced as FORBIDDEN, which the mobile PR review inbox renders as a permanent 'you don't have permission' state with no action. An organization member's Kilo access is not what failed, so the state lied about the cause. The provider read layers now carry a 'reconnect' kind: credential refusals, rejected tokens, inactive integrations, and the insufficient-permission repository cache all map to it, the router surfaces it as PRECONDITION_FAILED (the mobile reconnect state), and an inbox list read reclassifies a provider 403 as a connection failure because the inbox has no per-merge-request permission context. The merged inbox reports which provider produced the terminal state so the reconnect notice re-checks that provider and its organization scope instead of GitHub, which cannot fix an expired GitLab connection.

kilo-code-bot · 2026-10-17T14:23:43Z

Code Review Summary

Status: No Issues Found | Recommendation: Merge

The branch's current diff against main reclassifies provider-connection failures as reconnect (GitLab inactive integration and inbox 403, Bitbucket 401 / broker refusal / reconnect_required / insufficient repository scope / inbox 403), maps that kind to PRECONDITION_FAILED, and carries the failing provider plus organization into the mobile reconnect notice so it re-checks the right connection. I found no correctness, security, or memory-safety defects in the changed code: the error-class classifiers preserve existing instances (so not_found/retryable are unchanged), only forbidden and the newly mapped statuses are rewritten, every kind switch that the new reconnect value reaches already handles it (provider-branch-listing.ts, provider-review-router.ts), and the merge/source-selector changes are behavior-preserving. No timers, listeners, or subscriptions are added; the refetch callback is only invoked from event handlers.

Assumptions: none. Read-only review; the suites listed in the PR body were not re-run in this session.

Files Reviewed (15 files)

apps/mobile/src/components/pr-review/pr-review-inbox-list.tsx
apps/mobile/src/components/pr-review/pr-review-reconnect-notice.tsx
apps/mobile/src/components/pr-review/pr-review-inbox-list.mounted.test.tsx
apps/mobile/src/lib/pr-review/use-provider-inbox.ts
apps/mobile/src/lib/pr-review/use-provider-inbox.test.ts
apps/web/src/lib/provider-review/gitlab-authorization.ts
apps/web/src/lib/provider-review/gitlab-read.ts
apps/web/src/lib/provider-review/bitbucket-authorization.ts
apps/web/src/lib/provider-review/bitbucket-read.ts
apps/web/src/lib/provider-review/gitlab-authorization.test.ts
apps/web/src/lib/provider-review/gitlab-read.test.ts
apps/web/src/lib/provider-review/bitbucket-authorization.test.ts
apps/web/src/lib/provider-review/bitbucket-read.test.ts
apps/web/src/routers/provider-review-router.test.ts
apps/web/src/routers/cloud-agent-next-router.branches.test.ts

Previous Review Summaries (3 snapshots, latest commit 1cc3061)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit `1cc3061`)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The only change since the previous review is the main-branch consolidation of the GitLab/Bitbucket error classes onto the shared ProviderReviewError base, and I found no behavioral or runtime regression in that adoption: the kind vocabulary, retryable derivation, isNetworkFailure probe, and router instanceof/kind mapping are all equivalent to the replaced implementations.

Files Reviewed (4 files)

packages/app-shared/src/provider-review/errors.ts
apps/web/src/lib/provider-review/gitlab-authorization.ts
apps/web/src/lib/provider-review/bitbucket-authorization.ts
apps/web/src/routers/provider-review-router.ts

Verification notes: GitLabReviewError/BitbucketReviewError now extend the shared base and still set their own name after super, so existing instanceof checks in provider-branch-listing.ts, gitlab-write.ts, bitbucket-write.ts, and the tests remain valid; the router's instanceof ProviderReviewError covers both subclasses for the same kind→tRPC mapping (including reconnect → PRECONDITION_FAILED at provider-review-router.ts:384-387) and outcomeCodeFromFailure still returns head_moved only for stale_head; ProviderApiStatusError retains the readonly status field used by the adapters' classification; isNetworkFailure is byte-equivalent to the two removed local copies and is no longer defined anywhere else in these modules; the new module exports no name that collides with capabilities.ts/contracts.ts, and @kilocode/app-shared/provider-review already resolves through the package's exports map with an ES2022/native-class target, so class identity and instanceof hold. No timers, listeners, subscriptions, or other leak-capable resources were added. No active inline comments existed, and none were needed.

Previous review (commit `a713292`)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental change set is a behavior-preserving refactor: extracting the hasError predicate in mergeProviderInboxSources/inboxRetryAction, inlining the previously memoized refetch callback, consolidating the inbox mounted tests, and aligning the revoked-connection branch-listing assertion with the new PRECONDITION_FAILED classification; no correctness, security, or memory-leak issues were found in the changed lines.

Files Reviewed (4 files)

apps/mobile/src/components/pr-review/pr-review-inbox-list.mounted.test.tsx
apps/mobile/src/lib/pr-review/use-provider-inbox.test.ts
apps/mobile/src/lib/pr-review/use-provider-inbox.ts
apps/web/src/routers/cloud-agent-next-router.branches.test.ts

Notes on what was checked: the new hasError (source.error != null) is equivalent to the inline checks it replaces in firstPageFailures, laterPageError, and inboxRetryAction; the de-memoized refetch keeps identical logic and its only consumer (pr-review-inbox-list.tsx:57) calls it from an event handler, not from an effect or memo dependency, so the per-render identity is harmless and introduces no listener or subscription; resolveIntegration in gitlab-authorization.ts:226-231 still throws the reconnect kind with the "no longer active" copy that cloud-agent-next-router.branches.test.ts:224-227 now asserts as PRECONDITION_FAILED, so the updated assertion matches current production behavior; useCallback remains imported and used by fetchNextPage, and no new timers, listeners, or subscriptions were added.

Previous review (commit `b634dff`)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The provider-connection failure reclassification (reconnect kind flowing from the GitLab/Bitbucket read layers through PRECONDITION_FAILED to the mobile reconnect notice) is internally consistent, correctly gated on enabled-provider state, and covered by matching tests; no correctness, security, or memory-leak issues were found in the changed code.

Files Reviewed (17 files)

apps/mobile/src/components/pr-review/pr-review-inbox-list.mounted.test.tsx
apps/mobile/src/components/pr-review/pr-review-inbox-list.tsx
apps/mobile/src/components/pr-review/pr-review-reconnect-notice.tsx
apps/mobile/src/lib/pr-review/classify-pr-review-query-state.ts
apps/mobile/src/lib/pr-review/use-provider-inbox.test.ts
apps/mobile/src/lib/pr-review/use-provider-inbox.ts
apps/web/src/lib/cloud-agent/provider-branch-listing.ts
apps/web/src/lib/provider-review/bitbucket-authorization.test.ts
apps/web/src/lib/provider-review/bitbucket-authorization.ts
apps/web/src/lib/provider-review/bitbucket-read.test.ts
apps/web/src/lib/provider-review/bitbucket-read.ts
apps/web/src/lib/provider-review/gitlab-authorization.test.ts
apps/web/src/lib/provider-review/gitlab-authorization.ts
apps/web/src/lib/provider-review/gitlab-read.test.ts
apps/web/src/lib/provider-review/gitlab-read.ts
apps/web/src/routers/provider-review-router.test.ts
apps/web/src/routers/provider-review-router.ts

Notes on what was checked: the mergeProviderInboxSources refactor is behavior-preserving for laterPageError/firstPageErrorState and the new firstPageErrorPlatform is always non-null when allFailed (since platform is required on every source), so the notice always names a real provider; the notice's explicit ?? scope fallbacks preserve the GitHub/no-route-scope surfaces; the new reconnect cases are handled in every switch (error.kind) (router mapper and both branch-listing mappers) and every existing error-kind assertion that remains valid still passes; no new subscriptions, timers, or listeners were introduced.

_{Reviewed by deepseek-flash-latest · Input: 140.5K · Output: 28.4K · Cached: 2.6M}

_{Review guidance: REVIEW.md from base branch main}

Fold the reconnect-notice coverage into the existing mounted suites and merge duplicate merge tests so every touched mobile file stays under the 300-line cap. Also update the branch listing test to the reconnect classification: an inactive integration is PRECONDITION_FAILED, not a missing one.

iscekic · 2026-10-17T18:07:36Z

bot: Follow-up session. The head commit keeps the branch green (lint, tests, typecheck all pass; the earlier lint/test failures were fixed by a713292). The PR body now lists all three proxy answers in the required question/answer format, and the placeholder screenshot link was replaced with two real Android emulator screenshots attached from this session's verification: the organization scope shows the GitLab reconnect notice with the Check connection action, and the Personal scope shows the normal empty state. No code changes were needed; the review loop is converged.

…-org-member # Conflicts: # apps/web/src/lib/provider-review/gitlab-authorization.test.ts # apps/web/src/lib/provider-review/gitlab-authorization.ts # apps/web/src/lib/provider-review/gitlab-read.ts # apps/web/src/routers/provider-review-router.test.ts # apps/web/src/routers/provider-review-router.ts

…nect The owner-aware classifier rewrites an organization connection's 403 into a reconnect whose message names a merge request. The inbox list request never mentions one, so the classification now reads the provider status first and answers every 403 on that read with the inbox copy.

iscekic · 2026-10-22T02:19:53Z

bot: Android emulator proof of the provider review inbox, on the local dev stack with the fixture organization and one local stub serving the provider APIs and token brokers.

Organization scope, inbox collection refused: the reconnect notice names GitLab and offers "Check connection" (no permission state).
Same organization scope after the connection re-check and with the collections answering: the inbox lists the merge request and the pull request of that organization.
Personal scope: the normal empty state.

Backend observed for the same run: the organization GitLab status is what "Check connection" re-checks for the member, and both inbox reads answer PRECONDITION_FAILED instead of FORBIDDEN.

…-org-member # Conflicts: # apps/mobile/src/lib/pr-review/classify-pr-review-query-state.ts

…ssifier change

iscekic added drain Work item from the findings drain explorer:android Finding surfaced by the Android explorer labels Oct 17, 2026

iscekic added the merge:human Substantive functional risk requires human review. label Oct 19, 2026

iscekic added 2 commits October 19, 2026 18:12

merge: main into drain/pr-review-inbox-org-member

4f00b46

chore(provider-review): request review of the reconnect classification

1cc3061

pandemicsyn approved these changes Oct 20, 2026

View reviewed changes

iscekic mentioned this pull request Oct 21, 2026

fix(mobile): open organization GitLab merge requests for members #8400

Merged

iscekic added this pull request to the merge queue Oct 21, 2026

iscekic removed this pull request from the merge queue due to a manual request Oct 21, 2026

iscekic added this pull request to the merge queue Oct 21, 2026

github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 21, 2026

iscekic added the merge:blocked:insufficient-e2e Verification does not cover the change label Oct 21, 2026

iscekic added 2 commits October 22, 2026 02:00

iscekic removed merge:human Substantive functional risk requires human review. merge:blocked:insufficient-e2e Verification does not cover the change labels Oct 22, 2026

iscekic added 2 commits October 22, 2026 02:46

Merge remote-tracking branch 'origin/main' into drain/pr-review-inbox…

ba8c7b1

…-org-member # Conflicts: # apps/mobile/src/lib/pr-review/classify-pr-review-query-state.ts

chore(mobile): drop the duplicated type comment after merging the cla…

79111cc

…ssifier change

iscekic added the merge:human Substantive functional risk requires human review. label Oct 22, 2026

iscekic added this pull request to the merge queue Oct 22, 2026

Merged via the queue into main with commit 17d6d74 Oct 22, 2026
60 checks passed

iscekic deleted the drain/pr-review-inbox-org-member branch October 22, 2026 20:34

kilo-code-bot Bot mentioned this pull request Oct 22, 2026

chore(kilo-app): bump version to 1.0.15 #7300

Merged

github-actions Bot mentioned this pull request Oct 23, 2026

docs: auto-sync cli with merged PRs (through 2026-10-31) Kilo-Org/kilocode#15139

Open

Navigation Menu

Repository navigation

fix(mobile): recover the provider review inbox when a connection needs reconnecting - #7895

fix(mobile): recover the provider review inbox when a connection needs reconnecting#7895
iscekic merged 8 commits into
mainfrom
drain/pr-review-inbox-org-member

iscekic commented Oct 17, 2026 •

edited

Loading

Uh oh!