Repository navigation
refactor(client): unify GitLab onto the shared remote UI - #25
Conversation
The GitLab integration shipped a separate controller and a rough standalone panel, while GitHub already had the polished list/detail/tab experience. This merges both providers into one GitRemoteController (v1 wire for legacy Hosts, scoped v2 for negotiated Hosts) and removes the manual GitHub/GitLab switch: the provider is derived from the remote-URL identities reported by remote-repositories v2, unmatched remotes are reported per-remote as unregistered or ambiguous, and a repository matching no supported provider is reported unsupported instead of silently defaulting to GitHub. GitLab merge requests and issues now reuse the shared list surface, detail tabs, timeline, composer, pull actions, auth controls and notices; notes, discussions, commits and approvals are projected onto shared view models, and a gitlab/<instance>/<path> five-segment resource-address grammar keeps tab identities collision-free against the four-segment GitHub form. The dedicated GitLabRemotePanel is deleted; capability-specific behavior (merge methods, review events, inline comments, approvals, glab authentication) stays at the controller and projection boundaries.
The replay script still exercised the removed gitlabControllerFor and select APIs, so the acceptance step threw a TypeError on every lane. It now uses controllerFor plus load(target), and its mock answers the unscoped remote-capability refresh instead of constructing a URL from an undefined instance.
remote-view imported sameRepository from the host-side remote/git.ts, which loads node:crypto at module scope and crashes the vite client bundle before any surface renders. The comparison is now inlined, and the e2e harness stubs the local session controller with a frozen snapshot so useSyncExternalStore sees a stable value; the e2e refresh selector is also disambiguated between the list and detail surfaces.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 182f58715b
ℹ️ 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".
| const sameList = this.state.listKind === kind && this.state.listState === filter && this.listKey(kind, this.state.target, filter) === this.listKey(kind, target, filter) | ||
| this.patch({ generation: facts.generation, scopedCapability: capability.value, listKind: kind, listState: filter, items: this.page('items', sameList ? this.state.items : [], list, item => item.number, true), itemsNext: list.next, detail: undefined, comments: [], commentsNext: undefined, inline: [], inlineNext: undefined, commits: [], commitsNext: undefined, reviews: [], reviewsNext: undefined, approvals: undefined, unavailable: [], outcome: undefined, confirmation: undefined, streamError: undefined, unknown: false, ...(reconcile && this.state.blocked ? { reconciled: list.next === undefined } : { blocked: false, reconciled: false, blockedAction: undefined, error: undefined, errorDetails: undefined }) }) |
There was a problem hiding this comment.
Clear previous repository rows when changing scoped targets
When switching to an uncached origin/upstream with the same list kind and filter, loadScoped() has already replaced state.target before this comparison, so sameList incorrectly evaluates to true for rows belonging to the previous repository. If the new list has a continuation cursor, page() retains unmatched old rows until pagination finishes; if loading fails, they remain visible indefinitely. The shared surface opens those rows using the new target, potentially navigating to an unrelated PR or issue. Clear the old items on target changes or compare against the identity associated with the existing rows.
Useful? React with 👍 / 👎.
| if (this.disposed || this.state.busy || target === undefined || generation === undefined || capability.state !== 'on') return | ||
| if (target.provider === 'gitlab') { | ||
| if (this.state.blocked) return |
There was a problem hiding this comment.
Skip notification polling while device authorization is active
If a GitLab device login remains pending when the sidebar's 60-second notification timer fires, login() has already released busy, so this guard permits pollNotices() to proceed. Its remote-capability request uses the same scoped lane as the outstanding remote-poll-login, advancing the lane sequence and causing the eventual login response to be rejected as stale-response. A successful authorization then displays login-failed instead of completing the authenticated reload. The previous GitLab controller's timer explicitly skipped active login attempts; retain that guard here or isolate the polling lanes.
Useful? React with 👍 / 👎.
| const gitlab = this.state.target?.provider === 'gitlab' && this.feature.scoped.available | ||
| if (endpoints[0] === 'remote-sign-out' && this.state.busy) { this.clear(this.state.repository); this.patch({}) } | ||
| if (gitlab) { |
There was a problem hiding this comment.
Preserve the GitLab target when interrupting work to sign out
When Disconnect is clicked during an in-flight GitLab operation, this calls clear(), which removes state.target, while the previously computed gitlab flag remains true. The GitLab branch subsequently calls scopedCall('remote-sign-out', ...), but scopedCall() rejects the missing target with invalid-target before sending any RPC. Consequently the credentials remain active despite the explicit sign-out request. Cancel the current operation without discarding its target, or capture the original scope for the sign-out RPC.
Useful? React with 👍 / 👎.
|
@codex review |
Add browser-assisted device login, release application defaults and Advanced grant cleanup. Persist authentication selection separately from OAuth grants, reuse saved authorization across CLI switches and Host restarts, and expose OAuth-only logout. Update bilingual documentation and isolated regression coverage.
Reacquire instance authentication capability after protected reads fail, retain signed-out as a normal state, and retry instance metadata before explicit authentication. Cover persisted logout markers and CLI/OAuth recovery with controller and isolated browser regressions.
Keep cooldowns, backoff and server polling intervals with a Host-only credential digest. Switching from limited CLI credentials to saved OAuth can recover without inheriting the CLI cooldown, while returning to the same token still honors its deadline. Cover the transition in unit and isolated browser regressions.
…thread GitLab's notes API echoes discussion members (DiffNote/DiscussionNote and resolvable notes), and its discussions API wraps every plain note in an individual_note discussion, so each posted comment appeared both in the comments feed and again inside a thread. remote-comments now keeps only individual notes and remote-discussions drops individual_note wrappers; the RPC contract documents the split.
selectMethod queued behind the serialized chain, so a device poll could hold a method switch (and every read) for its whole expiry window; it now aborts in-flight logins like signOut/clearGrant do on both GitHub and GitLab. A persisted authorizing record whose attempt no longer exists permanently rejected login with login-already-running; startLogin now only honors live, unaborted attempts and supersedes dead ones. GitHub invalidFingerprint was only cleared on dispose, so a credential that once returned 401 could never be re-verified; retry, signOut, clearLocalGrant and logoutOAuth now clear it, matching the GitLab engine.
Partial and unknown scoped results stacked a large outcome section on a blocking gate card plus an inline error line; they now surface as a persistent warning toast carrying the step results, receipt reason, reconcile hint and the unblock action once reconciled, while succeeded results replay through a brief success toast keyed by outcomeSeq. Writes stay locked until acknowledgeReconciliation runs after refresh. Commit rows also space the author from the timestamp.
…nance" This reverts commit fb36bcc.
This reverts commit 7759f25.
This reverts commit 3b8f89f.
GitLab viewers dropped avatar_url from /user, and the image URL policy rejected Gravatar defaults carrying d/r parameters. Project the viewer avatar through the same allowlisted policy and retry a failed avatar when its URL changes.
Sign-in could only be managed from the Remote tab, which needs an open repository. The plugin settings page now lists GitHub and each registered GitLab instance with its account state and opens the same authentication dialog. Host adds remote-accounts and remote-account so the list and viewer are available without a repository; sign-in changes from either surface refresh the other.
…to settings The dialog mixed GitLab-only notes and an unexplained Advanced fold into every platform. It now shows an identity card and one card per connection method with a single plain-language description, keeps GitLab notes in GitLab dialogs, and nests the device code under the OAuth choice. Clearing a removed instance's local token moves to a two-step dialog in the settings accounts section.
…etails Remote and local Git failures were shown as red inline text, often as raw codes such as git/remote-unavailable or Git stderr. They now surface through the shared Toast with a short localized summary; the code, reason and raw message are only reachable through a Copy details action. Panels that lose their content keep a neutral placeholder, and an unknown write result stays visible until it is resolved.
8d337e1 to
deab6b1
Compare
deab6b1 to
dff225c
Compare
The toast-based error and write-outcome feedback used primitives APIs that only exist from 0.1.7 (weighted icon names, Toast tone/actions), so the declared 0.1.5/0.1.6 lanes failed to compile. Route the icon through dsh-icons and add dsh-toast, which probes the host Toast for actions support and renders an equivalent floating alert with real buttons when actions would be silently dropped — that keeps the copy-details and reconcile controls reachable instead of locking writes. Also assert the new "Comment published" toast in the GitLab e2e; the persistent "Action result" section no longer exists.
dff225c to
119a29a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 119a29a861
ℹ️ 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".
| const write = (endpoint: string, values: Record<string, unknown> = {}): Promise<boolean> => { | ||
| return controller.write(endpoint, { number: detail?.number, expectedHeadSha: pr?.head.sha, ...values }).then(() => { | ||
| if (controller.getSnapshot().error !== undefined) return false | ||
| return controller.write(endpoint, values).then(() => { |
There was a problem hiding this comment.
Preserve object context for legacy GitHub writes
When connected to a Host without protocol v2, this call falls back to writeV1(), which forwards values unchanged and does not infer the current object. Removing number and expectedHeadSha here therefore breaks comments, reviews, PR checkout, and issue-branch creation on those Hosts: github-legacy.ts requires number for each operation and additionally validates the SHA for reviews and checkout. Restore these fields for the v1 path or populate them inside writeV1().
Useful? React with 👍 / 👎.
| if (endpoint === 'remote-comment' && (!pull || !facts.operationSupport.comment.supported || !acceptedAction(facts.actions.canComment))) return | ||
| if (endpoint === 'remote-review') { | ||
| const event = values['event'] | ||
| if (!pull || (event !== 'COMMENT' && event !== 'APPROVE' && event !== 'REQUEST_CHANGES') || !facts.operationSupport.review.supported || !(facts.supportedReviewEvents as readonly string[]).includes(String(event)) || !acceptedAction(facts.actions.canReview) || (event === 'APPROVE' && facts.reviewPermissions?.APPROVE !== true && facts.reviewPermissions?.APPROVE !== 'unknown')) return |
There was a problem hiding this comment.
Allow GitHub approvals without GitLab permission facts
For a GitHub PR using protocol v2, GitHubScopedProvider.business() advertises APPROVE but does not return reviewPermissions, so both comparisons against facts.reviewPermissions?.APPROVE are true and this guard silently returns. The enabled Approve button consequently sends no request, and the surface clears the user's body as though submission succeeded. Apply this additional permission requirement only where those facts are supplied, or project an explicit GitHub approval permission.
Useful? React with 👍 / 👎.
| if (endpoint === 'remote-issue-branch' && pull) return | ||
| await this.run(async epoch => { | ||
| const objectKind = pull ? 'pull' : 'issue' | ||
| const result = await this.mutation(endpoint, { ...values, ...expected, number: detail.number, ...(target.provider === 'gitlab' ? { objectKind } : { object: { kind: objectKind, number: detail.number } }), ...(pull && (endpoint === 'remote-checkout-pr' || endpoint === 'remote-review' && values['event'] === 'APPROVE') && detail.head.sha !== null ? { expectedHeadSha: detail.head.sha } : {}) }, epoch, this.state.generation, endpoint, detail.number) |
There was a problem hiding this comment.
Include the head SHA for every GitHub review event
For protocol-v2 GitHub PRs, submitting a COMMENT or REQUEST_CHANGES review omits expectedHeadSha because this condition supplies it only for APPROVE. The GitHub scoped adapter forwards the request to github-legacy.ts, whose remote-review handler calls sha(request) for every review event, so these submissions are rejected before reaching GitHub. Include the current head SHA for all GitHub review events while retaining GitLab's event-specific behavior.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed989c2e0a
ℹ️ 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".
| if (target.provider === 'gitlab') { | ||
| if (this.state.blocked) return | ||
| await this.run(async epoch => { | ||
| const cap = await this.scopedCall<ScopedRemoteCapabilityOn>('remote-capability', {}, epoch) |
There was a problem hiding this comment.
Isolate notification polling from the pagination lane
When the sidebar's 60-second notification timer fires while a GitLab continuation page is in flight, busy is false even though streaming is true, so this capability request proceeds on the same scoped lane as pagination. ScopedRemoteClient.call() advances that lane's sequence, causing the pending page to be rejected as stale-response; streamLoop() then stops without scheduling a retry, leaving the continuation cursor and an incomplete list until a manual refresh. I reproduced this against the reviewed commit with a delayed list continuation followed by pollNotices(). Skip polling during streaming or give notification requests an independent lane.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef261af056
ℹ️ 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".
| const needsApprovals = this.state.blockedAction === 'review' || this.state.blockedAction === 'approve' || this.state.blockedAction === 'remote-review' | ||
| const reconciled = (!needsComments || comments !== undefined && comments.next === undefined) && (!needsApprovals || approvals !== undefined) | ||
| const commentViews = comments === undefined ? undefined : { items: comments.items.map(projectGitLabNote), truncated: comments.truncated, ...(comments.next === undefined ? {} : { next: comments.next }) } | ||
| const inlineViews = discussions === undefined ? undefined : { items: discussions.items.flatMap(projectGitLabDiscussion), truncated: discussions.truncated, ...(discussions.next === undefined ? {} : { next: discussions.next }) } |
There was a problem hiding this comment.
Render projected discussions for GitLab issues too
When a GitLab issue contains a discussion thread, its notes are now projected into state.inline, but GitRemoteItemSurface.tsx passes inline={pr === undefined ? [] : state.inline} to the timeline, discarding all discussions for issues. Meanwhile, gitLabIndividualNote() filters DiscussionNote entries out of the comments response, so these conversations disappear entirely from the UI. The removed GitLab panel rendered discussions for both issues and merge requests. Pass issue discussions through to the shared timeline as well.
Useful? React with 👍 / 👎.
Issue notes projected from remote-discussions were dropped because the item surface only forwarded state.inline for pull requests, so GitLab issue threads vanished after gitLabIndividualNote() excluded them from the comments feed. Pass issue discussions through, and stop stamping a bogus path:line ref chip on threads that carry no diff position.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2034f6a8ff
ℹ️ 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".
| const accountsKey = this.state.state === 'on' ? `${this.state.generation.instanceId}#${this.state.generation.revision}` : 'off' | ||
| if (accountsKey !== this.accountsKey) { this.accountsKey = accountsKey; this.loadAccounts() } |
There was a problem hiding this comment.
Refresh account discovery independently of the GitHub generation
When the volatile GitLab instance configuration changes while the Client remains connected, the Host registry reconciles the added, removed, or renamed instances without changing the GitHub generation. This guard therefore prevents remote-accounts from being requested again, and RemoteAccountsSettings continues rendering the old account list—even after reopening settings. Newly configured instances cannot be managed there, while removed instances remain visible. I reproduced this by changing the accounts response between two capability refreshes with an unchanged GitHub generation; only one accounts request was made. Refresh discovery on settings entry, periodically, or using an independent registry revision.
Useful? React with 👍 / 👎.
remote-accounts was only refetched when the unscoped capability generation changed, but that generation belongs to the GitHub v1 owner; GitLab instance configuration reconciles independently in the Host registry, so added or removed instances never invalidated the cached list and the settings section kept showing stale rows. The settings component now reloads accounts on entry and on each capability poll while open, and loadAccounts keeps the previous snapshot when the response is unchanged so steady-state polls do not rerender.
remote-account can capture a newer auth revision than the last unscoped capability snapshot because establishing the credential bumps the generation. Treating that as a mismatch cleared freshly loaded account rows to a permanent Checking state. Only clear when the view's generation is foreign or strictly older, and let a cleared row re-read itself instead of requiring a settings reopen.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4689d233af
ℹ️ 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".
| const write = copy ?? ((text: string) => navigator.clipboard.writeText(text)) | ||
| void Promise.resolve(write(report.detail)).then(() => { setResult('copied') }, () => { setResult('failed') }) |
There was a problem hiding this comment.
Catch synchronous clipboard failures in the copy action
When the browser does not expose navigator.clipboard, clicking “Copy details” throws synchronously before Promise.resolve() is called, so the rejection handler never displays the copy-failed feedback. Since the new error UI hides technical details behind this action, affected users also cannot retrieve the diagnostic information. Check clipboard availability and invoke the writer inside a promise callback or a try/catch so synchronous failures follow the same failure path as rejected writes.
Useful? React with 👍 / 👎.
|
@codex review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
GitRemoteController(v1 wire for legacy Hosts, scoped v2 for negotiated Hosts) and deletedGitLabRemotePanelplus the manual provider switch.remote-repositoriesv2; unregistered/ambiguous remotes are reported per remote, and a repository matching no supported provider is reported unsupported instead of silently defaulting to GitHub.remote-view.ts.gitlab/<instance>/<path>/<kind>/<number>five-segment resource-address grammar so GitLab detail tabs stay collision-free against the four-segment GitHub form.Test plan
vitest70 files / 857 tests passtscstandalone client + host configs passgit diff --checkpass