Skip to content

fix(sidebar): guard every worktree sort against undefined displayName (crash 99657ab1) - #9315

Merged
nwparker merged 2 commits into
mainfrom
nwparker/harden-worktree-displayname-sort
Jul 18, 2026
Merged

nwparker merged 2 commits into
mainfrom
nwparker/harden-worktree-displayname-sort

Conversation

@nwparker

@nwparker nwparker commented Jul 18, 2026 •

Copy link
Copy Markdown
Contributor

What

Extends the crash-99657ab1 guard — TypeError: Cannot read properties of undefined (reading 'localeCompare') — to every worktree-sort site, not just the sidebar board.

Worktree.displayName is typed string, but crash 99657ab1 proved it arrives undefined at runtime for persisted/discovered worktrees. PR #8683 fixed the sidebar kanban board. An adversarial review of this change found five other sort sites still calling bare a.displayName.localeCompare(b.displayName) on the same worktree objects — including the Cmd+J quick switcher, a high-traffic surface where an undefined name would crash harder than the original report.

This routes all raw-Worktree sorts through one shared, guarded comparator.

Change

  • New src/renderer/src/lib/worktree-display-name-order.ts — compareWorktreeDisplayName(a, b) = (a.displayName ?? '').localeCompare(b.displayName ?? ''). Placed in lib/ so every layer (sidebar, right-sidebar, Cmd+J, telemetry) shares one guard without a cross-feature import.
  • Migrated 6 raw-Worktree sort sites to the helper:
    • components/sidebar/workspace-kanban-worktree-groups.ts (board — dedupes the local guard from fix(sidebar): guard kanban worktree sort against undefined displayName (crash 99657ab1) #8683)
    • lib/order-empty-query-worktrees.ts (Cmd+J switcher)
    • components/right-sidebar/folder-workspace-attached-worktrees.ts
    • components/right-sidebar/parent-pr-checks-refresh.ts
    • components/sidebar/add-repo-existing-workspaces-telemetry.ts
    • components/sidebar/use-complete-git-repo-add.ts
  • Tests: helper unit tests + call-site wiring tests for the Cmd+J switcher and the add-repo telemetry sort (both fail without the guard).

Deliberately out of scope (not raw Worktree; only at risk if a builder copies an undefined Worktree.displayName, and one lives in the main process which must not import a renderer helper): WorkspaceSpaceWorktree (status-bar/workspace-space-presentation.ts, main/workspace-space-analysis.ts), WorkspaceCleanupCandidate (workspace-cleanup/…), and the non-Worktree project-options / port-group sorts.

CLEAR, UNDENIABLE fix evidence

Before (identical shape to the fixed kanban crash), e.g. the Cmd+J switcher:

return a.displayName.localeCompare(b.displayName)   // undefined.localeCompare → TypeError (crash 99657ab1 class)

After:

return compareWorktreeDisplayName(a, b)             // (a.displayName ?? '').localeCompare(b.displayName ?? '')

Call-site wiring tests reproduce the exact crash class (an undefined-displayName worktree inside the real production sort fns) and assert it no longer throws:

✓ orderEmptyQueryWorktrees > does not crash the switcher when a visible worktree has no displayName (crash 99657ab1)
✓ add repo existing workspace telemetry > sorts without crashing when a detected worktree has no displayName (crash 99657ab1)
✓ compareWorktreeDisplayName > does not throw when a displayName is undefined (crash 99657ab1)   (+3 more)

 Test Files  4 passed (4)
      Tests  19 passed (19)
  • npx tsc --noEmit -p config/tsconfig.tc.web.json → exit 0
  • oxlint (changed files) → clean · oxfmt --check → clean

ELI5

The app sorts your worktrees by name in several places. Some worktrees show up before they've been given a name (name = "nothing"/undefined). Asking nothing to alphabetically compare itself throws an error and takes down whatever panel is sorting — the sidebar board (already fixed), and it turns out the Cmd+J switcher and a few other lists too. This teaches every one of those places the same safe trick — "treat a missing name as empty" — and moves that trick into a single shared helper so a future sort can't forget it.

Trade-offs / behavior changes

  • None user-negative. Pure hardening. When names are present, ordering is byte-identical to before (?? only coalesces null/undefined, never '' or other values). The only behavioral difference is the previously-crashing case: a worktree with a missing name now sorts as if its name were "" (empty sorts first) instead of throwing.
  • Perf-neutral (see below).

Adversarial review

  • Reproduce-first: the failing case (undefined displayName in a sort) is captured by call-site wiring tests that fail without the guard and mirror the real crash 99657ab1.
  • Adversarial review loop: 2 rounds, converged clean. Round 1 (fresh subagent) caught that the fix was incomplete — 3 more unguarded raw-Worktree sort sites remained, including the Cmd+J switcher; I folded all of them in, relocated the comparator to lib/ for correct layering, and added call-site wiring tests. Round 2 (fresh subagent): CLEAN — no blockers or majors — independently verified completeness (all 6 raw-Worktree sites guarded; remaining .localeCompare sites are out-of-scope derived types), layering (no cycle, no main-process import), byte-identical ordering when names are present, correct nested-worktree passing in parent-pr-checks, and that each wiring test fails if the guard is removed.
  • Perf review (/perf skill, subagent): perf-neutral — no regression, no win. The kanban comparator is byte-identical to before (the local guard already had ?? ''); the new coalesce affects only cold, non-per-frame sort paths. ?? '' reuses an interned string literal (no allocation), same localeCompare call count, no new React subscriptions/effects.

Closes the remaining unguarded instances of the crash-99657ab1 class.

Made with Orca 🐋

@coderabbitai

coderabbitai Bot commented Jul 18, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ad85861b-7905-4466-8256-5af8039d301e

📥 Commits

Reviewing files that changed from the base of the PR and between 2360133 and ff858a0.

📒 Files selected for processing (1)
  • AGENTS.md

📝 Walkthrough

Walkthrough

Adds a shared compareWorktreeDisplayName helper that safely orders worktrees when displayName is undefined. Multiple sidebar, right-sidebar, telemetry, kanban, and switcher sorting paths now use the helper. Tests cover undefined-name comparisons, array sorting, telemetry event construction, and empty-query worktree ordering.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the code changes well, but it omits required template sections for Screenshots, Testing, AI Review Report, Security Audit, and Notes. Reformat the PR description to the required template and add the missing sections, especially testing, cross-platform review, security audit, and notes.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is specific and matches the main change, though 'sidebar' is slightly narrower than the full worktree-sort scope.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

… (crash 99657ab1)

Worktree.displayName is typed non-optional but arrives undefined at runtime for
persisted/discovered worktrees (crash 99657ab1). PR #8683 fixed the kanban board;
this extends the same guard to the five other sort sites that share those worktree
objects — including the Cmd+J switcher (order-empty-query-worktrees) — via one
shared compareWorktreeDisplayName helper in lib/. Behavior-neutral when names are
present; a missing name now sorts as '' instead of throwing.

Co-authored-by: Orca <help@stably.ai>
@nwparker
nwparker force-pushed the nwparker/harden-worktree-displayname-sort branch from b3e4f67 to 2360133 Compare July 18, 2026 08:51
… rule

Anchors the existing 'document the why, briefly' rule with a concrete
before/after (the worktree-displayName guard) so agents stop dumping the
crash id, mechanism, file location, and every call site into a comment.

Co-authored-by: Orca <help@stably.ai>
@nwparker
nwparker merged commit 057a814 into main Jul 18, 2026
1 check passed
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
… (crash 99657ab1) (stablyai#9315)

* fix(sidebar): guard every worktree sort against undefined displayName (crash 99657ab1)

Worktree.displayName is typed non-optional but arrives undefined at runtime for
persisted/discovered worktrees (crash 99657ab1). PR stablyai#8683 fixed the kanban board;
this extends the same guard to the five other sort sites that share those worktree
objects — including the Cmd+J switcher (order-empty-query-worktrees) — via one
shared compareWorktreeDisplayName helper in lib/. Behavior-neutral when names are
present; a missing name now sorts as '' instead of throwing.

Co-authored-by: Orca <help@stably.ai>

* docs(agents): add over-commenting anti-pattern example to the comment rule

Anchors the existing 'document the why, briefly' rule with a concrete
before/after (the worktree-displayName guard) so agents stop dumping the
crash id, mechanism, file location, and every call site into a comment.

Co-authored-by: Orca <help@stably.ai>

---------

Co-authored-by: Orca <help@stably.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant