Repository navigation
Conversation
…r on PR review The PR-review connect gate offered its integration-page action to every organization role. The server restricts every organization Bitbucket connect and an organization GitLab re-authorization to the billing roles, so a plain member pressed a button that led to a page refusing them with no action of its own. The gate now reads the caller's organization role through the same shared permission rule the code-reviewer screens use, and in the states the server refuses it keeps the provider title, states that an organization owner, admin, or billing manager has to act, and renders no action. A first-time organization GitLab connect keeps its action because the server allows member-level access there, and a personal scope keeps it for every role. An unsettled role read keeps the action too, so the provider procedures stay authorized on the server without stranding the reader. The gate also waits for the restored account selection before answering for a scope. Before that, a reader whose organization is being restored is briefly asked about the personal scope, so the screen painted the personal connect view and then repainted the organization's.
iscekic
added
drain
|
bot: Visual proof from the Android emulator against the local dev stack, with a seeded organization and a fake account. Each capture is the PR-review gate reached from a provider pull/merge-request link under that organization.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit extracts the connect gates' illustration tile and dark-mode glyph color into one shared module used by the GitHub, GitLab and Bitbucket arms, and moves the account switcher above the Bitbucket org-only explanation so its copy's "switch the account above" matches the control; no correctness, security, or memory-leak issue was found in the changed code. Assumptions: Files Reviewed (4 files, incremental 8af5cc2..492c4a2)
Previous Review Summaries (6 snapshots, latest commit 8af5cc2)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8af5cc2)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit 8af5cc2 adds the opt-in Assumptions: Files Reviewed (6 files, incremental a79f9f6..8af5cc2)
Previous review (commit a79f9f6)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit applies the previously requested fix: the GitLab read-only gate description now names only who can act, and the first sentence is dropped from all 87 catalogs, with the mounted test updated to pin the shorter copy. Assumptions: Files Reviewed (88 files, incremental 7193500..a79f9f6)
Previous review (commit 7193500)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe incremental commit rewords the read-only PR-review gate states per provider; the only finding is that the new GitLab read-only description repeats the expiry title it renders above. Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (89 files, incremental 40e456b..7193500)
Fix these issues in Kilo Cloud Previous review (commit 40e456b)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit resolves both prior findings: a failed selection restore now counts as settled ( Assumptions: Overview
Files Reviewed (2 files, incremental 63b0f11..40e456b)
Previous review (commit 63b0f11)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe incremental commit only reformats the role fixtures, so no new defect was introduced; the two prior findings against the live Assumptions: Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit 63de1e2)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe role split matches the server rules (verified in Assumptions: Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Reviewed by deepseek-flash-latest · Input: 54K · Output: 10K · Cached: 1M Review guidance: REVIEW.md from base branch |
iscekic
added
merge:blocked:kilobot-not-ready
iscekic
added
merge:blocked:kilobot-not-ready
The connect gate now waits for the stored account selection before it answers for a scope. That wait read only the restore's success flag, but a failed restore publishes its error without ever reporting the selection as loaded, so the gate would wait for an answer that never arrives. A restore also re-runs on a token change and briefly reports the selection as unrestored again, which would replace a reader's open review with the spinner. Treat the published error as a settled answer, and keep the first settled answer for the life of the mount.
iscekic
added
merge:blocked:kilobot-not-ready
|
bot: Design gate for the current head: 4 cells (base, dark, compact, rtl), 0 errors, 0 warnings. Two provider screens per cell: an organization GitLab merge request with an unusable integration, and an organization Bitbucket pull request. Attached is the contact sheet; the full run is uploaded under the work session's design-gate path for this head.
|
iscekic
added
the
merge:blocked:design-review
…vider The organization admin-only arm borrowed the code-reviewer read-only line. On a merge request that arm told a reader whose connection had expired that the provider was never connected, and on Bitbucket it left the state titled with a connect call the state no longer offers. Give each arm the copy its state needs: an expired connection says it expired and asks for a reconnection, and an absent connection states the absence in its title and names only the roles that can connect. The new strings land in every catalog. The Arabic reconnect titles no longer end with a period, so the headline style matches the English copy on the same screens.
iscekic
removed
the
merge:human
iscekic
added
merge:blocked:kilobot-not-ready
…st state The read-only state's line opened by repeating the expiry the title above it already states. Name the roles that can reconnect, and nothing else, in the English copy and in every translation.
iscekic
removed
merge:blocked:insufficient-e2e
|
bot: Updated proof for the read-only merge-request state at a79f9f6. The line under the expiry title no longer repeats the expiry: on the emulator, as a plain member of a seeded organization whose GitLab integration cannot serve reviews, the gate shows the title "GitLab connection expired", the organization context line, and "Ask an organization owner, admin, or billing manager to reconnect it." with no action. The Arabic cell renders the same sentence from the Arabic catalog under a title without a trailing period. The Bitbucket read-only state, the owner state, and the first-time connect state are unchanged by this follow-up; their captures are in the earlier comment. The design gate was re-run for this head and reports 4 cells, 0 errors, 0 warnings.
|
iscekic
added
merge:blocked:insufficient-e2e
iscekic
added
merge:blocked:kilobot-not-ready
|
bot: Design-gate rerun for the two findings at this head, on the Android emulator against the local dev stack with a seeded fake organization.
The gate run covers 4 cells (base, dark, compact, Arabic RTL) with both provider screens in each: 0 errors, 0 warnings, no skipped rules, and no layout shift from the new order. Attached: the run's contact sheet, a dark-mode capture of the read-only merge-request state, and an owner-state capture where the switcher and the action appear together.
|
…her above its copy
iscekic
added
merge:blocked:kilobot-not-ready
|
bot: Follow-up at this head, after re-checking the reorder against the copy it sits next to.
Re-verified on the Android emulator against the local dev stack: the organization-only state reads title, switcher pill, then the sentence naming the switcher above it; the GitHub connect gate in dark mode paints the same secondary tile with a visible border and the secondary-ink glyph. The design gate re-ran both organization screens at this head: 4 cells, 0 errors, 0 warnings, no skipped rules. Attached are the new contact sheet, the organization-only state, and the GitHub gate in dark mode.
|
iscekic
added
merge:blocked:insufficient-e2e
|
bot: Re-ran the design gate for head 492c4a2 on the Android emulator against the local dev stack. Head run and merge-base run on the same two links: 4 cells each, 0 errors, 0 warnings, and the merge-base run is uploaded under The four captures below come from the same session at this head, with a seeded organization and a fake account: the organization member's read-only gate for a connection that has to be re-authorized, the member's read-only gate for a provider that is not connected, the same caller raised to owner, and a member's first-time connect.
|
Evidence
Evidence: demonstrated — seeded a local organization with a suspended GitLab integration and a fake member account, ran the app on the Android emulator against the local stack, compared the PR-review gate for a plain member and an owner, and measured the account switcher's placement and the dark tile's contrast in the captured pixels, at head 492c4a2.
What changed
apps/mobile/src/components/pr-review/pr-review-provider-connect-gate.tsxoffered the integration-page action in the organization scope to every role. The web page that action opens is narrower than the gate assumed:ORGANIZATION_BILLING_ROLES(organizations.bitbucket.connect, and the Bitbucket OAuth connect route);ORGANIZATION_BILLING_ROLES, because the connect route only keeps member-level access when no integration row exists;So a plain member pressing the action in the first two cases reached a page that refused them, with no action of its own, and the screen attributed the dead end to the provider connection.
The gate reads the caller's organization role through the same shared permission rule the code-reviewer screens use (
useReviewerPermission→canManageOrganizationBilling), and a new pure selector decides the action:lib/pr-review/pr-review-connect-gate-view.tsselectPrReviewConnectActionreturnsorganization-admin-onlyonly where the server refuses: an organization GitLab re-authorization, and every organization Bitbucket connect. Personal scope and a first-time GitLab connect keep the action.The same selector also makes the gate wait for the restored account selection (
useOrganization().isLoaded) before answering for a scope, which removes a personal-scope paint that the organization's repaint followed on the same route (found by the design gate's layout-shift check).Not changed, deliberately:
Design review round 1
Each finding is addressed on the screen it names:
The three new English strings are translated in every one of the 86 other catalogs.
Design review round 2
Button'soutlinesurface and border pair, so the dark cell gets the visible edge the app's other outlined controls have. Measured in the base cell of the run below: the pill's pressable row, its label and the caret share one centre line (row centre 521.9pt, caret centre 522.1pt), and the explanation now sits above the pill.lib/pr-review/pr-review-gate-illustration.tsholds the tile class and the scheme-based glyph token, and all three connect gates (GitLab, Bitbucket, GitHub) use them, so the family cannot disagree in dark mode.Verification
Unit, at this head:
pnpm --filter kilo-app exec vitest run src/components/pr-review/— 70 files, 691 tests pass, including the role matrix (member, billing role, unsettled), the first-time-connect exception, the unsettled-selection wait, and both read-only wordings.pnpm --filter kilo-app exec vitest run src/i18n— 21 files, 1823 tests pass, including catalog parity.pnpm --filter kilo-app lint,typecheck,check:unused,format— clean.End to end on the Android emulator against the local dev stack at this head (fake account, seeded organization):
member, plus a GitLab integration row that is present but not serving (getGitLabStatusanswersreconnectRequired: true).ownerand reopened the GitLab link: the gate kept its action ("Connect GitLab") and the re-check instruction, with the switcher pill below them.Screenshots of the member, dark-mode, and owner states are attached in a comment on this pull request.
Design gate
Design gate: 4 cells, 0 errors, 0 warnings.
The run covers
/pr-review/gitlab/review-group/review-repo/7and/pr-review/bitbucket/e2e-workspace/e2e-repo/5, opened as a plain organization member of a seeded organization whose GitLab row cannot serve a review — the state this change settles. The Arabic cell renders the current copy from the Arabic catalog, and the dark cell carries the illustration change.It is uploaded for head 492c4a2 together with a second run of the same two links on the merge base
cee0605bb, underbase/in the same folder. The base run also reported 4 cells, 0 errors, 0 warnings, so the two runs agree cell by cell and this change introduces no design-gate finding; the base run only differs in what it paints, since a plain member there still gets the integration-page action and the pre-fix composition.In the same session at this head, the Android emulator re-showed each state the change settles: the member GitLab gate (expired title, admin-role line, switcher pill, no action), the member Bitbucket gate (absence title, admin-role line, switcher pill, no action), the owner gate (re-check line and the Connect GitLab action restored), and a member's first-time GitLab connect (the Connect GitLab action kept). Captures of those four states are in the comment below.
Production prerequisites
None.
Proxy answers
The owner's proxy gave these answers in the owner's place.
Answer: Only where the server refuses: GitLab reconnect + Bitbucket connect
Confidence: 0.75 · Precedent: Trust the code over documents: "those documents may be stale, only trust code" (session 269375d6e466). Hiding the first-time connect would remove a path a member can complete today.
Answer: Leave the notice unchanged
Confidence: 0.70 · Precedent: Do the least work possible with no drop in quality (session 7f3470e0ba6f). The re-check works for every role, so there is no dead end to fix there.
Answer: Reuse the existing read-only keys
Confidence: 0.70 · Precedent: Do the least work possible with no drop in quality: "do the least amount of work possible, with no drop in quality" (session 7f3470e0ba6f). The existing keys are accurate and already translated in every catalog.
Answer: Add dedicated keys and translate them
Confidence: 0.85 · Precedent: Localize every string in the mobile app and fix the whole class: "i18n is very important to me"; best-in-class UX standard ("design and UX MUST BE BEST-IN-CLASS")