Conversation
562c80c to
27d2b8e
Compare
b7b2a47 to
c6bb9e5
Compare
|
This pull request changes Lite's UI, so it is labelled Attach them however you like — drag images straight into a comment, or have an agent capture them for you (the Swap the label for cc @PavelLaptev — visual change. |
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Lite workspace base-update conflict previews and warning UI.
Changes:
- Extracts and enhances the Integrate control with dry-run conflict detection.
- Adds grouped conflict details, file lists, styling, unit tests, and E2E coverage.
File summaries
| File | Description |
|---|---|
| apps/lite/ui/src/routes/project/$id/workspace/Graph/Section.tsx | Updated as part of this pull request. |
| apps/lite/ui/src/routes/project/$id/workspace/Graph/Integrate.tsx | Updated as part of this pull request. |
| apps/lite/ui/src/routes/project/$id/workspace/Graph/Integrate.test.tsx | Updated as part of this pull request. |
| apps/lite/ui/src/routes/project/$id/workspace/Graph/Integrate.module.css | Updated as part of this pull request. |
| apps/lite/e2e/tests/base-update.spec.ts | Updated as part of this pull request. |
Review details
Suppressed comments (4)
apps/lite/ui/src/routes/project/$id/workspace/Graph/Integrate.tsx:213
worktreeConflictsreports paths that would conflict when dirty changes are applied to the resulting workspace; it does not prevent the integration, and this button remains enabled. This wording therefore tells users the update is blocked when it can proceed (with conflict markers), contradicting the backend contract and the action. Please describe the impending conflict instead.
<p className={styles.blocked}>These files block the update</p>
apps/lite/ui/src/routes/project/$id/workspace/Graph/Integrate.tsx:94
- These gates only check timestamps and
isFetching. If one of the source queries is invalidated and its refetch fails while retaining cached data, React Query leaves the old data/timestamp available andisFetchingbecomes false, so the old dry-run result can still produce a warning for stale inputs. Include the source queries' error state in the preview/display gate (or otherwise hide the result after a source failure).
const previewEnabled =
enabled &&
headRevision > 0 &&
targetRevision > 0 &&
worktreeRevision > 0 &&
apps/lite/ui/src/routes/project/$id/workspace/Graph/Integrate.tsx:132
- If the popup is open when any source query refreshes,
showWarningbecomes false and this conditional unmountsDropdown, butopenremainstrue. When the refreshed preview predicts conflicts again, the newly mounted controlled popup receivesopen={true}and opens itself without another user action. Reset the open state whenever the warning is hidden (or keep the popup mounted and explicitly close it while checking).
const showWarning =
previewEnabled &&
!previewFetching &&
!previewFailed &&
conflicts !== undefined &&
(conflicts.files.length > 0 || conflicts.branches.length > 0 || conflicts.checkoutConflict);
const rebase = () => integrate({ projectId, updates, dryRun: false });
return (
<span className={styles.control}>
{showWarning && (
<Dropdown
apps/lite/ui/src/routes/project/$id/workspace/Graph/Integrate.tsx:32
refName === nulldoes not always mean an unnamed branch: anonymous segments can sit below a named segment, and the existing branch resolution inWorkspaceLists/LastCommitLine.tsx:32-35attributes them to the next named segment. Using onlysegment.refNamehere (and again in the fallback at line 48) groups those conflicts underUnnamed branchinstead of their actual branch. Resolve the owning named segment in both loops.
branch: segment.refName?.displayName ?? "Unnamed branch",
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Conflict metadata deduplication is O(n²) and should use a set or equivalent for large integrations.
Review details
Suppressed comments (1)
crates/but-core/src/commit/mod.rs:716
- This now runs for every unresolved conflict, including merges where the index already contains thousands of content-conflict paths, but
Vec::containsscans the accumulated paths for each conflict. That makes metadata construction O(n²) in the number of conflicted files and can make large integrations disproportionately slow. Keep aHashSetof paths (or otherwise maintain membership separately) while appending so deduplication remains O(1).
fn push_unique(v: &mut Vec<PathBuf>, change: &gix::diff::tree_with_rewrites::Change) {
let path = gix::path::from_bstr(change.location()).into_owned();
if !v.contains(&path) {
v.push(path);
- Files reviewed: 9/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
8b08d8b to
fe526bd
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Preview requests can queue stale work and delay current previews or real integration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/12 changed files
- Comments generated: 1
- Review effort level: Lite
fe526bd to
751b393
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Preview cancellation does not stop native work before execution, and the workspace-combination warning condition is unreachable.
Review details
Suppressed comments (2)
apps/lite/ui/src/routes/project/$id/workspace/Graph/Integrate.tsx:191
cancelQueriesonly aborts the React Query observer; an already-startedworkspaceIntegrateUpstream(..., dryRun: true)continues running and holdsContext::exclusive_worktree_access()for the whole preview. Clicking Integrate here starts the real operation while that exclusive lock is still held, so the operation can block behind the check despite the button being intentionally enabled. The preview and execution need an explicit native-side coordination/cancellation strategy (and a concurrency test), rather than only cancelling the query.
void client.cancelQueries({ queryKey: [projectId, "dryRun", "workspaceIntegrateUpstream"] });
integrate({ projectId, updates, dryRun: false });
apps/lite/ui/src/routes/project/$id/workspace/Graph/Integrate.tsx:293
- This condition cannot be true for this dry-run flow:
WorkspaceState::from_rebase_previewalways passesfalseforcheckout_conflict_occurred, and this endpoint returns before materialization. As a result, the workspace-combination warning text is dead and cannot report the condition it describes; either remove this branch or add a preview-computable signal.
{conflicts.checkoutConflict && (
<p className={styles.description}>
The updated branches also conflict when combined in the workspace
</p>
)}
- Files reviewed: 10/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
751b393 to
21c6e28
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
A slow native preview can hold the exclusive worktree lock and block immediate integration behind an obsolete check.
Review details
Suppressed comments (1)
apps/lite/ui/src/routes/project/$id/workspace/Graph/useWorkspaceIntegrationPreview.ts:177
- Cancelling this query only aborts the debounce/query observer; it cannot cancel an IPC call that has already entered
workspace_integrate_upstream. That native preview holdsContext::exclusive_worktree_access()for the duration of the check, so invoking the real integration immediately here makes it wait on the same repository lock whenever a slow preview is active. The button appears non-blocking, but the operation is effectively serialized behind an obsolete check; coordinate the native preview and mutation (or make the preview use a cancellable/non-exclusive path) before starting the integration.
void client.cancelQueries({ queryKey: [projectId, "dryRun", "workspaceIntegrateUpstream"] });
integrate({ projectId, updates, dryRun: false });
- Files reviewed: 11/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
21c6e28 to
1bd4c7f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Preview progress/failure handling and lock-safe cancellation must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
apps/lite/ui/src/routes/project/$id/workspace/Graph/useWorkspaceIntegrationPreview.ts:169
- Cancelling this query only aborts the JS wait;
previewUpstreamConflictsdeliberately keeps an in-flight native request alive. The dry-run API holds the same exclusive worktree lock as the real integration (crates/but-api/src/workspace.rs:745-750), so clicking Pull latest during an active preview starts a mutation that blocks behind the preview instead of being non-blocking. Please make the preview lock-compatible or coordinate this transition so the real operation cannot wait on an abandoned check.
const rebase = () => {
void client.cancelQueries({ queryKey: [projectId, "dryRun", "workspaceIntegrateUpstream"] });
integrate({ projectId, updates, dryRun: false });
- Files reviewed: 11/13 changed files
- Comments generated: 1
- Review effort level: Lite
| const previewReady = | ||
| previewEnabled && !previewFetching && !previewFailed && conflicts !== undefined; | ||
| const rebase = () => { | ||
| void client.cancelQueries({ queryKey: [projectId, "dryRun", "workspaceIntegrateUpstream"] }); | ||
| integrate({ projectId, updates, dryRun: false }); | ||
| }; | ||
| return { | ||
| conflicts: previewReady ? conflicts : undefined, | ||
| enabled, | ||
| isPending, | ||
| rebase, | ||
| }; |
- Show affected branches, commits, and files. - Keep the preview fresh without changing the repo.
1bd4c7f to
677a899
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Some update entry points bypass the conflict preview, and post-apply conflict reporting can incorrectly surface an applied update as a failure.
Review details
Suppressed comments (3)
apps/lite/ui/src/routes/project/$id/workspace/Graph/Section.tsx:277
- This new button is the only update path wired to the preview hook. The existing
SidebarworkspaceHotkeys.updateWorkspacehandler (and the selected-stack update menu/hotkey) still callworkspaceIntegrateUpstreamwithdryRun: falsedirectly, so a user invoking those commands can apply the same conflict-producing update without seeing this warning. Route those entry points through the shared preview/action as well, or otherwise make them use the same guard.
{!target.current && <Integrate target={target.label} preview={preview} />}
apps/lite/ui/src/routes/project/$id/workspace/Graph/useWorkspaceIntegrationPreview.ts:152
updatesis rebuilt by walking every stack on each render, and putting that full array in the query key makes React Query re-hash all of those selectors whenever this hook re-renders for source/check/mutation state changes. The three revision timestamps already identify the inputs that should trigger a new preview, so keep this key scalar (and memoize the update derivation if the compiler output shows it remains hot).
updates,
headRevision,
targetRevision,
worktreeRevision,
crates/but-api/src/workspace.rs:916
- This conflict-reporting call runs after
materialize()andproject_meta.persist(). If any resulting conflicted commit is malformed andCommit::conflict_entries()returns an error, the API reports the already-applied update as a failure; the wrapper then skips the oplog commit and the frontend will not run its success cache synchronization. Compute/validate the conflict metadata before materialization, or make reporting failure non-fatal after the update has been applied.
let workspace_state = WorkspaceState::from_materialized(materialized, &repo)?;
let commit_conflicts = workspace_commit_conflicts(&repo, &workspace_state)?;
(workspace_state, worktree_conflicts, commit_conflicts)
- Files reviewed: 11/13 changed files
- Comments generated: 0 new
- Review effort level: Lite



Pull latestavailable.Local checks passed:
Fixes GB-2029.