Full app review (verified): 17 UX + performance findings #30
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: KofTwentyTwo/gclo#30
Reference in new issue
Block a user
The full-application review's adversarially verified findings: each was produced by a senior-expert reviewer for its dimension, then a second agent re-read the current code trying to refute it. Only survivors are listed. (Two further findings were already fixed directly on dev: PAT leak via
SyncRequest.ToString, and dev-channel self-update never finding releases.)UX / product design
[P1] No repository name search — subset selection is impractical at the app's stated scale
gclo/WorkspacePage.xaml:293The app's own name is 'Git Clone Large Organizations', yet the only table filtering is the status SelectorBar (All/Active/Failed/Pending, WorkspacePage.xaml:293-303). There is no text search anywhere in the UI. Failure scenario: an org with 800+ repos loads all-selected; a user who wants to sync only the ~15 'platform-*' repos must scroll a virtualized 800-row list and hunt for each checkbox by eye, and after a partial run there is no way to find a specific failed repo by name except scrolling the Failed filter. Sorting by Name helps only if you know the alphabet position of every repo. This is the single largest friction point in the core token→org→load→sync workflow for exactly the persona the product targets.
Proposed fix: Add an AutoSuggestBox/TextBox to the toolbar (Grid row 0, next to the SelectorBar) that substring-filters FilteredRepos by Name (combined with the status filter in WorkspaceViewModel.MatchesFilter), plus a 'Select visible / Deselect visible' affordance so search-then-select becomes the subset workflow.
[P1] Load and refresh failures render as quiet gray caption text, indistinguishable from normal status
gclo.ViewModels/WorkspaceViewModel.cs:509LoadReposAsync's catch does only 'StatusText = ex.Message' (WorkspaceViewModel.cs:507-511); org-lookup failures do the same (line 349). StatusText renders as a CaptionTextBlockStyle block in TextFillColorSecondaryBrush at the very bottom of the page (WorkspacePage.xaml:687-695) — the same surface and styling used for 'Loading repositories...' and '87 repositories loaded.' Failure scenario: a user's PAT expires; they press Refresh; the spinner stops and the stale table remains while the excellent 401 message from GitHubRepositoryLister.cs:77-78 appears as small gray text at the window's bottom edge, nowhere near the button they clicked. Nothing is red, no InfoBar opens (ResetRunResult at line 467 actually closes the previous one), and the run-results InfoBar pattern that exists for sync outcomes is not used. The message content is staff-level; the surface makes it invisible.
Proposed fix: Route load/org-lookup failures through an error surface: set ResultKind=Error/ResultOpen=true so the existing ResultInfoBar shows them (with a Retry action), or add a dedicated InfoBar under the connect card / toolbar. Keep StatusText for the live region.
[P2] Editing the organization in the chip flyout silently desynchronizes chip, table, and target path
gclo/WorkspacePage.xaml:222In Quick Sync, the Edit flyout's org ComboBox two-way binds ViewModel.Organization (WorkspacePage.xaml:222) and the connection chip binds the same property (line 158), but the loaded table keeps the previous org's rows — LoadRepos only runs via the Refresh button, and nothing marks the table stale. Failure scenario: user synced 'acme', opens Edit, types 'acme-labs', dismisses the flyout. The chip now reads 'acme-labs · 87 repositories · C:\src\acme-labs' while all 87 rows are acme repos. Pressing 'Sync 87 repos' clones acme's repositories into C:\src\acme-labs (SyncAsync uses the stale descriptors with the current EffectiveTargetRoot, WorkspaceViewModel.cs:556-567). The UI actively lies about what will happen, and the mistake writes gigabytes into the wrong folder.
Proposed fix: When Organization changes after HasLoadedRepos, treat the table as stale: clear/disable SyncCommand until a reload, and show a one-line prompt ('Organization changed — reload repositories') with a Reload button; or trigger LoadRepos automatically on flyout close when the org differs from the loaded one.
[P2] Account wizard steps 3 and 4: Next/Save silently does nothing when the field is empty
gclo.ViewModels/AccountWizardViewModel.cs:195TryAdvanceAsync returns false with no message for an empty organization (case 3, lines 194-200) and an empty target folder (default case, line 203). Steps 1 and 2 have NameError/TokenError properties with red live-region TextBlocks, but steps 3 and 4 have none — AccountWizardDialog.AnnounceStepError explicitly maps them to null (AccountWizardDialog.xaml.cs:189-197, comment: 'steps 3 and 4 have no message property'). Failure scenario: a first-run user reaches step 3 with the combo empty (their token saw no orgs), clicks Next — the dialog does absolutely nothing: no message, no shake, no disabled state, the button just appears broken. Same on step 4 Save with a blank folder. Silent rejection in a first-run wizard is where new users conclude the app is buggy.
Proposed fix: Add OrganizationError/TargetError properties set on failed advance ('Choose an organization from the list, or type one.' / 'Choose a target folder.'), bind them like NameError with LiveSetting=Polite, and extend AnnounceStepError to cover steps 3 and 4.
[P2] Account settings edit path is undiscoverable; the visible Edit affordance is a dead end that says '(next update)'
gclo/WorkspacePage.xaml:194For account workspaces, the connection chip's 'Edit' link — the one visible edit affordance on the page — opens a flyout of permanently disabled fields captioned 'Account settings are edited in the account wizard (next update).' (WorkspacePage.xaml:194-195, ConnectFieldsEnabled at WorkspacePage.xaml.cs:88-89 hard-disables for accounts). The functionality actually exists today: right-clicking the account's nav item shows an Edit… context menu (MainWindow.xaml.cs:344-364). Failure scenario: a user needs to rotate an expired token; they click the chip's Edit, find frozen fields and a note promising a future update, and conclude token rotation is impossible — right-click on a NavigationView item is not a discoverable gesture and no visible UI hints at the context menu. The '(next update)' microcopy is also stale, since the edit wizard shipped.
Proposed fix: Replace the flyout's disabled-fields-plus-note with a summary of the account plus an 'Edit account…' button that invokes ShowEditAccountWizardAsync (the page can expose a callback to MainWindow); keep the context menu as a secondary path.
[P2] No bridge from a working Quick Sync setup to a saved account; Quick Sync demands the token be re-pasted every launch
gclo.ViewModels/WorkspaceViewModel.cs:75Quick Sync never persists its token (ctor sets Token = "" unless account-backed, WorkspaceViewModel.cs:75; AppSettings stores only folder/parallelism/theme) and HasLoadedRepos resets each launch, so every session in the default landing workspace restarts at the connect card: find PAT → paste → wait for org lookup → pick org → load. Accounts solve this, but nothing connects the two: the connect card (WorkspacePage.xaml:34-124) never mentions accounts, and after a successful Quick Sync there is no 'Save as account' — the wizard makes the user re-enter name, token, org, and folder from scratch. Failure scenario: the natural first-run path (land on Quick Sync, get it working) leads users to re-type their PAT daily without ever learning the feature that fixes it lives behind a footer item.
Proposed fix: Add 'Save this connection as an account…' to the Quick Sync success InfoBar (or next to the chip), seeding AccountWizardViewModel from the workspace's current token/org/folder/subfolder/concurrency; mention accounts in the connect card's microcopy.
[P2] The most common pull failure (diverged local history) surfaces as a raw libgit2 message with no remediation
gclo.Engine/LibGit2GitClient.cs:228FetchAndPull merges FastForwardOnly (LibGit2Client.cs:228-233) and lets NonFastForwardException propagate raw; OrgSyncEngine puts ex.Message verbatim into the row (OrgSyncEngine.cs:135). The comment at line 224 claims 'Failed with a clear message', but no translation exists anywhere (grep confirms NonFastForwardException is only touched in tests). Failure scenario: a developer commits locally in a synced repo — the single most likely failure for this tool's audience — and the next sync shows LibGit2Sharp's 'cannot fast-forward' text, which explains neither that gclo deliberately never merges nor what to do (push, reset, or re-clone). Contrast with the exemplary curated messages for 401/404/rate-limit in GitHubRepositoryLister.cs:69-83 — the git-side error path never got the same treatment. 'HEAD is detached; fetched, but nothing was merged.' (line 189) has the same gap.
Proposed fix: Catch NonFastForwardException in FetchAndPull and rethrow InvalidOperationException with actionable copy: "'' has local commits that origin does not; gclo only fast-forwards. Push or reset your local commits, or delete the folder to re-clone."
[P3] Saving Settings silently resets Quick Sync's parallelism mid-session
gclo/MainWindow.xaml.cs:143ApplySettings unconditionally assigns 'viewModel.MaxConcurrency = _settings.DefaultMaxConcurrency' to the Quick Sync workspace (MainWindow.xaml.cs:143) and runs after every Settings save (line 669), while TargetFolder is correctly only filled when blank (lines 138-141). Failure scenario: user sets 'Parallel clones' to 24 in the workspace Options flyout for a big run, then opens File > Settings just to switch to Dark theme and clicks Save — Quick Sync's concurrency silently snaps back to the stored default (8), and the next sync runs 3x slower with no indication anything changed. A settings dialog mutating unrelated live workspace state on save is a trust-eroding surprise.
Proposed fix: Apply DefaultMaxConcurrency only when constructing the Quick Sync workspace (mirror the TargetFolder blank-check), or only push it when the user actually changed that setting in the dialog.
[P3] A successful load of an empty or wrong account yields a blank table with no empty state
gclo/WorkspacePage.xaml.cs:138The only empty-state text is gated on 'FilteredRepos.Count == 0 && Repos.Count > 0' (WorkspacePage.xaml.cs:137-140), i.e. it covers filters only. A load that returns zero repos still sets HasLoadedRepos = true (WorkspaceViewModel.cs:503), permanently swapping the connect card for the table view. Failure scenario: a user types a coworker's username (or an org where their fine-grained token grants no repo access, which lists as empty rather than 404), presses Load — the card vanishes and they get a chip saying '0 repositories', an empty white table region with headers, '0 of 0 selected', and a disabled 'Sync 0 repos' button. Nothing explains whether the org is empty, the name is wrong, or the token lacks scope; the only clue is caption text at the window bottom.
Proposed fix: Show a real empty state in the table body when Repos.Count == 0 after a load: "No repositories found in ''. Check the name, or make sure the token can see its repositories." with the org-edit affordance nearby.
[P3] Path-recovery dialog's defaults guarantee a second failure for suggestion-less paths
gclo/PathRecoveryDialog.xaml.cs:92Rows for paths with no safe rename (case-only collisions) prefill the TextBox with the original segment (_prefill = info.SuggestedName ?? _originalSegment, PathRecoveryDialog.xaml.cs:92), and BuildReplacementPath returns null for an unchanged name (line 124) — so the default state neither renames nor skips them. Failure scenario: a repo fails with 40 invalid paths of which 2 are case collisions; the user accepts the sensible-looking defaults and clicks 'Apply and check out' — ApplyRecoveryCore's validation (LibGit2GitClient.cs:335-339) rejects the set and the row lands back on Failed with a fresh path list, with no indication of which rows caused the round-trip or that the same click will fail again. The CLI already encodes the right default ('Paths with no safe rename... are skipped', SyncCommand.cs BuildRecovery:365-385); the GUI never adopted it.
Proposed fix: Pre-check Skip for rows whose InvalidPathInfo.SuggestedName is null (matching CLI behavior) and validate the effective path set in OnPrimaryButtonClick, flagging still-invalid rows inline instead of closing and failing.
Performance
[P2] FilteredRepos is rebuilt with Clear + re-Add on every membership change, causing O(n^2) CollectionChanged churn and scroll resets during a run
gclo.ViewModels/WorkspaceViewModel.cs:849RebuildFilteredRepos (lines 849-862) runs after EVERY terminal transition (HandleProgress -> RecomputeCompletedCount line 836) and after every non-terminal transition when Filter is Active/Pending (line 815). Two costs: (1) With the default All filter the desired set always matches, but each call still allocates an n-element List via Repos.Where(...).ToList() plus an O(n) SequenceEqual — at 1,000 repos that is ~1,000 terminal events x O(1,000) = ~2M UI-thread operations and ~8 MB of transient list garbage per run, for zero visible change. (2) When the filter is Pending/Active/Failed, every status transition changes membership, SequenceEqual fails, and the code does FilteredRepos.Clear() (a Reset notification — the ListView at WorkspacePage.xaml:573 discards all realized containers and snaps scroll to top, directly contradicting the method's own 'keep their scroll position' comment) followed by up to n individual Add notifications. Over a 1,000-repo run with the Pending filter that is ~500K Add events plus ~1,000 Resets, each Add additionally firing WorkspacePage.UpdateDerivedTexts (WorkspacePage.xaml.cs:50) which touches three UI elements. This is the single biggest obstacle to smooth 300-1,000-row runs.
Proposed fix: Maintain FilteredRepos incrementally: HandleProgress already knows the one item whose status changed, so on a membership change Remove(item) or Insert(item, indexDerivedFromReposOrder) — an O(visible) index can be kept in a dictionary updated on load/sort. Reserve the full rebuild for filter changes, loads, and sorts. This eliminates both the per-event O(n) allocation/compare and the Reset storms, and actually delivers the scroll stability the comment promises.
[P2] Every pull with incoming commits re-validates the entire tree against Windows path rules instead of just the diff
gclo.Engine/LibGit2GitClient.cs:216FetchAndPull calls WindowsPathValidator.Validate(tracked.Tip.Tree) whenever the local tip differs from the remote tip — i.e., on every pull that has anything new. Validate flattens the whole tree (WindowsPathValidator.cs:38-67): every TreeEntry is materialized through LibGit2Sharp (native ODB lookups + string marshaling per entry), and ValidatePaths then rebuilds every prefix string per path segment (O(total path characters) allocations plus dictionary/hashset inserts). For a monorepo with 100K+ files this is on the order of a million managed allocations and native round-trips per pull, per repo — typically dwarfing the actual fetch of a handful of objects. On a daily 'update everything' run this tax is paid by every repo that moved, making it the dominant engine-side cost for large repos even though the incoming change may touch one file.
Proposed fix: Validate only what the pull introduces: repo.Diff.Compare(repo.Head.Tip.Tree, tracked.Tip.Tree) and run ValidateSegment over Added/Renamed/Copied paths only. For the case-only-collision rule, check new paths against the existing checked-out path set (repo.Index paths in an OrdinalIgnoreCase set) rather than re-walking the full tree. Keep the full-tree walk for the clone path (first materialization) and for CompletePendingCheckout, where it is genuinely required.
[P2] Mass-failure runs do synchronous file I/O on the UI thread once per failed repo
gclo.ViewModels/WorkspaceViewModel.cs:799HandleProgress runs on the UI thread (Progress marshals there) and calls _log.Error for every Failed report. FileActivityLog.Write (FileActivityLog.cs:45-63) takes a lock, calls Directory.CreateDirectory, then File.AppendAllText — a full open/write/close per entry, each open subject to antivirus filter-driver latency (~0.5-2 ms typical). Failure scenario: a revoked PAT or dropped network makes every selected repo fail; at 1,000 repos with 8 workers failing fast, the UI thread absorbs ~1,000 file open/append/close cycles plus one UIA RaiseNotificationEvent per failure plus the O(n) RecomputeCompletedCount/RebuildFilteredRepos cascade per event — 1-2+ seconds of cumulative UI-thread stalls concentrated in the worst possible moment (with the Failed filter selected, each failure also triggers the finding-1 Clear+re-Add storm on a growing list).
Proposed fix: Decouple logging from the UI thread: have FileActivityLog enqueue entries to a System.Threading.Channels.Channel drained by a single background writer that preserves the open-append-close-per-entry durability contract. Alternatively (or additionally) log per-repo failures from the engine worker side rather than inside the UI-marshaled progress handler, and keep only cheap state mutation in HandleProgress.
[P3] Header select-all and Retry-failed trigger O(n^2) work: per-item CanExecute re-query and per-item O(n) recounts
gclo.ViewModels/WorkspaceViewModel.cs:431OnItemPropertyChanged calls SyncCommand.NotifyCanExecuteChanged() unconditionally — line 431 sits OUTSIDE the _syncingSelection guard. So OnAllSelectedChanged's loop (lines 397-408) over n rows raises CanExecuteChanged n times, and each raise makes the bound Sync button re-evaluate CanSync(), whose Repos.Any(r => r.IsSelected) scans the collection — unchecking the header at 1,000 rows costs ~n^2/2 = 500K scans plus 1,000 command-state UI updates in one dispatcher callback. RetryFailedAsync (lines 633-636) is worse per item: it flips IsSelected without setting _syncingSelection, so every changed row runs RecomputeSelectedCount (O(n) Count) + UpdateAllSelectedFromItems (O(n) All) + the CanExecute re-query — O(n^2) on a single click. At the app's stated 300-1,000-row scale this is a perceptible click-latency stutter, and it is pure redundant work: the code already recomputes everything once after the header loop (line 409-410).
Proposed fix: Move NotifyCanExecuteChanged inside the !_syncingSelection branch (the batch paths already notify once after their loops), and wrap RetryFailedAsync's selection loop in _syncingSelection = true / finally false followed by one RecomputeSelectedCount + UpdateAllSelectedFromItems + NotifyCanExecuteChanged.
[P3] Sort's Move-based reordering optimizes a collection nothing binds, at O(n^2), then resets the bound list anyway
gclo.ViewModels/WorkspaceViewModel.cs:737Sort() reorders Repos with a Repos.IndexOf(...) inside a loop — O(n^2) reference scans (~500K at 1,000 rows) — and up to n ObservableCollection.Move notifications, each of which fires WorkspacePage's Repos.CollectionChanged subscriber (UpdateDerivedTexts, WorkspacePage.xaml.cs:49). The stated justification ('Move rather than clear + re-add so list controls keep row containers stable', lines 736-744) cannot hold: no ItemsSource binds Repos — the table binds FilteredRepos (WorkspacePage.xaml:573) — and the very next line calls RebuildFilteredRepos, which sees a changed order, fails SequenceEqual, and does the Clear + n re-Adds (a full ListView Reset, containers discarded) that the Move dance was written to avoid. So a sort click at 1,000 rows pays O(n^2) + n Move events + n UpdateDerivedTexts calls + a full list reset, and gets none of the intended container stability.
Proposed fix: Either apply the stable sort as Move operations on FilteredRepos itself (the bound collection), using a position dictionary to make each move O(1) lookup instead of IndexOf, and reorder the backing Repos list cheaply with a plain in-place list sort — or accept the rebuild and delete the Move choreography on Repos entirely (List.Sort + rebuild), removing the O(n^2) loop and the misleading comment.
[P3] New Octokit GitHubClient per API call: no HTTP connection reuse across org lookups and repo loads, clients never disposed
gclo.Engine/GitHubRepositoryLister.cs:16GitHubRepositoryLister.ListOrganizationRepositoriesAsync constructs a fresh GitHubClient (own HttpClientAdapter -> own HttpClient/handler -> empty connection pool) on every call, and GitHubOrganizationLister.ListOrganizationsAsync (GitHubOrganizationLister.cs:14) does the same. Every interactive action — each debounced org lookup as a token is pasted/edited, every 'Load repositories', every Refresh, every account-wizard validation — therefore pays a fresh DNS + TCP + TLS handshake to api.github.com (~150-400 ms at typical RTTs) before the first byte, instead of riding an existing warm connection. The clients are also never disposed, so each abandoned handler's sockets linger until finalization. Pagination within one call reuses the client, so the cost is strictly the cross-call handshakes on the app's most latency-visible paths (the org dropdown spinner and the load button).
Proposed fix: Cache one GitHubClient per token (e.g., a small map keyed by token string, or a single client whose Credentials are swapped when the token changes — Octokit clients are thread-safe for concurrent requests with fixed credentials). Both listers should accept/share the same client instance; the ViewModels layer already constructs them centrally so injection is one constructor change.
[P3] Workspace cache retains full WinUI visual trees per account for the app lifetime
gclo/MainWindow.xaml.cs:46_workspaces caches (WorkspaceViewModel, WorkspacePage) per visited account and never trims; eviction happens only on account edit/delete (EvictWorkspace). Caching the ViewModel is required — it keeps background syncs alive across navigation — but the WorkspacePage half pins an entire ~700-line-XAML visual tree per account: the connect card, chip, toolbar, flyouts, and a ListView whose realized item containers (roughly 15-25 rows x ~15 elements each, plus composition visuals) stay allocated exactly as they were when the user navigated away. 'Sync all' makes it worse: ToggleSyncAllAsync calls EnsureWorkspace for every account, instantiating pages for workspaces the user never opened. With 10+ accounts each holding a 1,000-row table, idle memory grows by tens of MB of XAML/composition objects that serve no running work.
Proposed fix: Split the cache: keep ViewModels cached unconditionally (cheap — ~100 KB per 1,000 rows), but release the Page when navigating away from a workspace whose ViewModel.IsRunning is false, recreating it on revisit (x:Bind re-hydrates all state from the VM; only transient scroll position is lost). Also make EnsureWorkspace create only the ViewModel for 'Sync all' warm-up — the coordinator never needs the Page.
🤖 Generated with Claude Code
Progress update — items fixed since filing:
SyncRequest.ToString()— redacted (beta.12), regression test added.GithubSourcenow looks at prereleases for prerelease installs (beta.12).Still open (top of the list):
Keeping open for the remaining verified items.
All 17 findings are addressed on
dev(commits64814d1,9479a76, and the shell/dialogs commit after them); this issue closes with the nextdev → mainPR.LoadErrorBar: red, dismissable InfoBar with Retry, shown in both page states; organization-lookup failures use it too.IsTableStale+ warning bar with "Reload repositories"; Sync is blocked until the reload.OrganizationError/TargetError, bound and announced like steps 1/2.AccountWizardSeed); connect card mentions it.ApplySettingspushes the default only at startup or when the default itself changed.EmptyTableTextnames the organization and the likely causes.PathRecoveryPlannerpre-flights the choices and the dialog stays open listing problems.WindowsPathValidator.ValidateIncoming: diff-only (added/copied/renamed/type-changed) plus case-collision check against the index.FailureLoggingProgresslogs on the engine's worker thread before marshaling.Reposplus one rebuild of the bound list.OctokitGatewayCache: one client per token for the process.WorkspacePage.Detach), rebuilt from the cached view model on return; Sync all warms view models only.Verification: 367 Engine/ViewModels tests (14 new), coverage gate 100% on all three libraries, format gate clean, FlaUI e2e 8/8.