Skip to content

Windows: stop counting already-open recent projects as rendered sidebar rows - #430

Merged
coneilen merged 1 commit into
mainfrom
coneilen-microsoft-fix-sidebar-row-count
Sep 23, 2026
Merged

coneilen merged 1 commit into
mainfrom
coneilen-microsoft-fix-sidebar-row-count

Conversation

@coneilen

Copy link
Copy Markdown
Collaborator

Fixes #428.

Three distinct changes. Only the first is a product change; the other two are test corrections with justification, called out separately so they are reviewable.

1. Product fix: layout counted rows that are never rendered

appendRows() skips recent projects that are already open:

Sidebar.zig:507   if (project.isRemote() or isProjectOpen(model, project.path)) continue;
Sidebar.zig:518   if (!project.isRemote() or isProjectOpen(model, project.path)) continue;

but layoutFor() counted every entry:

Sidebar.zig:588   .project_count = model.recent_projects.items.len,

and that count drives geometry at :485 ((project_count + project_heading_count) * 24). So the project section reserved space for rows that were never drawn whenever an open project also appeared in recents, inflating section height, every offset derived from it, and scroll extent - and causing hit testing to resolve a click to the wrong row kind.

The sibling helpers projectHeadingCount() at :647/:652 already applied !isProjectOpen(...), so heading visibility respected the skip and only the row count diverged. That asymmetry is what identified this as an oversight rather than an intentional model.

Fixed so the count is derived from the same predicate appendRows() uses, rather than duplicating the condition in a second place - duplication is what let producer and consumer drift apart originally.

2. Test correction: stale oracle in the scroll test (not a product bug)

sidebar scroll clamps overflow, shrink, and resize expected 334 and found 410. That 76px delta is not project overcount. It is exactly the Activity block:

Sidebar.zig:817   if (model.activity.items.len != 0) {
Sidebar.zig:818       bottom += 18 + 24 + 34;      // = 76

paint() at :282 renders that block - header, "Attention only" filter, prev/next controls, cards - whenever activity is non-empty, so contentBottom is correct to reserve the space. The test appends 3 activity events but its oracle omitted the Activity term.

410 - 334 = 76. Changing the product to satisfy the old expectation would have excluded visibly-rendered content from the scroll extent and clipped the Activity block - introducing a bug rather than removing one. contentBottom is unchanged; the expectation now includes the missing term, written as literals rather than as a call to contentBottom() so the oracle stays independent of the code it checks.

3. Test correction: clamp loop no longer coupled to the expected value

The overflow loop was for (0..10), i.e. exactly 10 * 40 = 400. Against the corrected maximum of 410 it never overflowed, so expectEqual(short_max, scroll) was no longer exercising clamping at all - the bound had been tuned to the old 334. Widened to overshoot decisively so it cannot silently stop testing clamping the next time this geometry moves.

Context

Sidebar.zig's 22 tests had never been executed by any harness (issue #424): WindowsShell.Tests.ps1 runs Zig tests from a hand-maintained list, this file was not on it, and Zig only runs test blocks from the root file passed to zig test. These failures surfaced on their first ever execution.

RED: three tests failed on unmodified main under pinned Zig 0.15.2 -> expected 0 found 1; expected 334 found 410; expected .open_project found .quick_chat_overview
GREEN: after the layout fix and the two justified test corrections -> all 22 Sidebar tests pass, 93/93 in the Sidebar root run
REGRESSION: the row-count and hit-test assertions re-fail if layout again counts unrendered project rows, and the scroll oracle re-fails if the Activity block reserved height changes -> drift between what is counted and what is drawn is caught

Verification

Pinned Zig 0.15.2. Sidebar root: 93/93 pass. The App root additionally verified at 231/231 with PR #426 pending App test-fixture fields applied temporarily, then reverted - App.zig does not compile its test build on current main without them (missing sidebar_state, declared_entry_ids, kept_worktree_paths at App.zig:6005), and that fix belongs to #426.

Tools/windows/Tests/WindowsShell.Tests.ps1 is deliberately untouched here. #426 quarantines these three tests by name; lifting that quarantine should follow after #426 lands.

coneilen added a commit that referenced this pull request Sep 23, 2026
…422; correct Sidebar quarantine attribution

Rebased onto origin/main after #422 (issue #418) merged as 06e092e, which
wired GraphContextMenu.zig and MainWindow.zig into the harness's zig test
invocations and source-list array. #422 never touched this PR's own
\ guard array, because that array did not exist on main
when #422 was authored -- it is this PR's own addition. Added both
filenames to the list and updated the guard's explanatory comment, which
was otherwise now stale (still described the two files as reserved and
not-yet-landed).

Also corrected the Sidebar.zig quarantine reason: an independent review
determined that only 2 of the 3 quarantined failures are the real
layoutFor()/appendRows() product bug (#428, fix in flight as #430); the
third (sidebar scroll clamps overflow, shrink, and resize, expected 334
found 410) is a separate, stale test expectation -- the 76px delta is the
Activity block height that contentBottom/paint() correctly account for
and the test's oracle omitted. Left it quarantined (not fixed) since this
PR must not modify Sidebar.zig source, but the reason string now
attributes each failure accurately instead of lumping all three under one
root cause.

RED: mechanically diffed this branch's wired-file set against origin/main
after rebasing -> main had 22 entries (20 plus #422's 2), this branch
still reported only 38 in its own \ guard list, and running
the guard's detection logic directly showed it still flagging
GraphContextMenu.zig and MainWindow.zig as unwired despite their zig test
invocations now existing in the script.
GREEN: added both names to \ and reran the same detection
logic -> zero missing files reported.
REGRESSION: ran the complete harness end-to-end locally with pinned Zig
0.15.2 and GRAPHCODE_WINGHOSTTY_ROOT set -> all 39 invocations execute
including the newly-landed GraphContextMenu.zig (8/8) and MainWindow.zig
(6/6, appearing twice via App.zig's transitive import), the same 3
Sidebar.zig failures are tolerated by name with the corrected attribution
text rendering intact, and the full script now exits 0 -- the guard no
longer fires at all.

Signed-off-by: Colin Neilens <coneilen@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@coneilen
coneilen force-pushed the coneilen-microsoft-fix-sidebar-row-count branch from 354eaca to d5125d7 Compare September 23, 2026 03:27
Count only recent projects that appendRows renders, preserve open projects without graph snapshots in the open-project section, and correct the stale Activity scroll oracle.

Fixes #428

Signed-off-by: Colin Neilens <coneilen@microsoft.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@coneilen
coneilen force-pushed the coneilen-microsoft-fix-sidebar-row-count branch from d5125d7 to 2d3a4c9 Compare September 23, 2026 03:56
@coneilen
coneilen merged commit ece5935 into main Sep 23, 2026
10 checks passed
coneilen added a commit that referenced this pull request Sep 23, 2026
…422; correct Sidebar quarantine attribution

Rebased onto origin/main after #422 (issue #418) merged as 06e092e, which
wired GraphContextMenu.zig and MainWindow.zig into the harness's zig test
invocations and source-list array. #422 never touched this PR's own
\ guard array, because that array did not exist on main
when #422 was authored -- it is this PR's own addition. Added both
filenames to the list and updated the guard's explanatory comment, which
was otherwise now stale (still described the two files as reserved and
not-yet-landed).

Also corrected the Sidebar.zig quarantine reason: an independent review
determined that only 2 of the 3 quarantined failures are the real
layoutFor()/appendRows() product bug (#428, fix in flight as #430); the
third (sidebar scroll clamps overflow, shrink, and resize, expected 334
found 410) is a separate, stale test expectation -- the 76px delta is the
Activity block height that contentBottom/paint() correctly account for
and the test's oracle omitted. Left it quarantined (not fixed) since this
PR must not modify Sidebar.zig source, but the reason string now
attributes each failure accurately instead of lumping all three under one
root cause.

RED: mechanically diffed this branch's wired-file set against origin/main
after rebasing -> main had 22 entries (20 plus #422's 2), this branch
still reported only 38 in its own \ guard list, and running
the guard's detection logic directly showed it still flagging
GraphContextMenu.zig and MainWindow.zig as unwired despite their zig test
invocations now existing in the script.
GREEN: added both names to \ and reran the same detection
logic -> zero missing files reported.
REGRESSION: ran the complete harness end-to-end locally with pinned Zig
0.15.2 and GRAPHCODE_WINGHOSTTY_ROOT set -> all 39 invocations execute
including the newly-landed GraphContextMenu.zig (8/8) and MainWindow.zig
(6/6, appearing twice via App.zig's transitive import), the same 3
Sidebar.zig failures are tolerated by name with the corrected attribution
text rendering intact, and the full script now exits 0 -- the guard no
longer fires at all.

Signed-off-by: Colin Neilens <coneilen@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
coneilen added a commit that referenced this pull request Sep 23, 2026
PR #430 (merged as ece5935) fixed the real Sidebar.zig layout/row-count
desync structurally via a single shared predicate,
projectIsVisibleInSection, so all three previously-quarantined tests
now pass unconditionally. Landing the quarantine would have shipped a
misleading tolerate-list for an already-fixed bug, so remove it
entirely instead:

- Delete Invoke-NativeQuarantined and its Get-FailingZigTestNames
  helper (Sidebar.zig/App.zig were their only consumers).
- Delete the sidebarLayoutOpenProjectKnownFailures/-Reason variables.
- Convert both call sites to plain Invoke-Native.

Verified end-to-end locally with pinned Zig 0.15.2: exit 0, all 93
Sidebar.zig tests and the full 232-test App.zig suite pass
unconditionally, zero quarantine output. Re-ran the guard's
set-difference check against origin/main (ece5935): empty, with
UpdateOfferPresentation.zig and the NativeForms Assert-Contract block
both still intact.

Signed-off-by: Colin Neilens <coneilen@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
coneilen added a commit that referenced this pull request Sep 23, 2026
…-drift guard (#426)

* Wire 17 orphaned Zig test files into WindowsShell.Tests.ps1 and add anti-drift guard

Fixes #424. WindowsShell.Tests.ps1 hand-maintained a list of zig test
invocations that missed 17 files (Accessibility.zig was fixed separately by
#421; GraphContextMenu.zig/MainWindow.zig are reserved for in-flight #418/
#422). Wires all 17 remaining orphaned files with per-file link flags
verified against pinned Zig 0.15.2, and adds a structural guard that
enumerates graphcode-windows\src\*.zig, detects files containing a 	est "
block, and throws if any is missing from the wired-file list -- so this
drift cannot recur silently.

First-run triage:
- WorktreeDialog.zig: one test used a stale fixture (dirty instead of
  locked) to exercise armConfirmation()'s fail-closed path; sweepSelectable()
  intentionally permits dirty rows. Fixed the test fixture, not the code.
- App.zig: one test's App struct literal predated three fields
  (sidebar_state, declared_entry_ids, kept_worktree_paths) added since it
  last compiled. Fixed the test to match App.init()'s initialization.
- Sidebar.zig: 3 real, pre-existing test failures traced to one root cause
  (layoutFor()/projectSectionHeight() count a recent_projects entry that is
  also the open project, but appendRows() correctly excludes it from
  rendered rows, desyncing row/scroll y-math). Per explicit instruction not
  to modify Sidebar.zig source, these are quarantined at the harness level
  with an explicit reason string, not fixed or deleted. Reported as a real
  product bug for separate follow-up.
- App.zig transitively reruns the same 3 Sidebar failures (it imports
  Sidebar.zig); quarantined identically with a note explaining why.

RED: WorktreeDialog and App.zig tests failed on first run -> both had stale
fixtures, not product bugs; fixed the tests to match current code.
GREEN: all 17 newly-wired files now execute; 90/93 Sidebar.zig and 228/231
App.zig tests pass -> the 3 residual failures are quarantined with cause.
REGRESSION: full harness run end-to-end with pinned Zig 0.15.2 exits 0 and
the anti-drift guard fails loudly on a real mutation removing a wired file
-> guard verified functional, not merely asserted.

Signed-off-by: Colin Neilens <coneilen@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* Run the anti-drift guard last so CI still validates every zig test invocation

Moves the structural guard added for #424 to the end of
WindowsShell.Tests.ps1, after all zig test invocations, instead of right
after Resolve-TestZig. Placed first, the guard's expected failure (reserving
GraphContextMenu.zig/MainWindow.zig for #418/#422) short-circuited the whole
script in CI before any of the newly-wired tests ever ran on the actual
runner, leaving only local verification as evidence. Placed last, CI now
executes and reports every invocation for real before the guard's contract
check runs, while the guard still fails the job overall until #418/#422
lands.

RED: with the guard first, CI failed at the guard on the first push and
never exercised a single newly-wired zig test -> no real CI signal existed
for the wiring itself, only local runs.
GREEN: relocated the guard after every Invoke-Native/Invoke-NativeQuarantined
call and reran the full harness locally with pinned Zig 0.15.2 -> all 37
invocations execute (90/93 Sidebar.zig, 228/231 App.zig, everything else
100%), quarantines are tolerated correctly, and the guard still throws last
naming exactly GraphContextMenu.zig and MainWindow.zig.
REGRESSION: reran the same real-mutation guard check (removing a wired file
name) after relocating the block -> guard still throws the identical
not-wired message immediately, confirming the guard's detection logic is
unchanged, only its position in the script moved.

Signed-off-by: Colin Neilens <coneilen@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* Point quarantine reason at issue #428 instead of #424

The three Sidebar.zig quarantine entries pointed at #424 (this PR's own
tracking issue) with the phrase "first-run finding" as a stand-in for a
real bug report, since no dedicated issue existed yet for the Sidebar
layout bug when it was first quarantined. The coordinating session filed
#428 with the full root-cause writeup (layoutFor()/projectSectionHeight()
vs appendRows()'s isProjectOpen skip, the 76px delta, and the hit-testing
failure). Repointing the quarantine reason string at #428 so it resolves
to the actual bug report instead of this wiring PR.

RED: quarantine reason string referenced #424, which is this very PR and
not a bug report -> anyone reading the quarantine message would have to
guess where the real Sidebar.zig fix should land.
GREEN: repointed both Sidebar.zig and App.zig quarantine reason strings to
#428 and reran the full harness locally with pinned Zig 0.15.2 -> same
90/93 Sidebar.zig and 228/231 App.zig pass counts, guard still fails last on
exactly GraphContextMenu.zig and MainWindow.zig, exit code unchanged.
REGRESSION: confirmed Invoke-NativeQuarantined's known-failure matching is
by test name string only, independent of the reason text -> the wording
change cannot affect which failures are tolerated.

Signed-off-by: Colin Neilens <coneilen@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* Add UpdateOfferPresentation.zig to the guard's wired-file list

Cherry-picked #420's NativeForms/UpdateOfferPresentation harness wiring
(commit 775d77b) onto this branch: my branch point (68eabe5) predated that
merge, so the reentrancy-guard Assert-Contract, the source-list entry, and
the zig test invocation for UpdateOfferPresentation.zig were all absent
here even though they exist on main. Restored via cherry-pick rather than
a full rebase, per instruction to hold on rebasing until #422 lands.

That cherry-pick alone was not sufficient: this PR's own anti-drift guard
maintains a second, independent file list (\) that #420
never touched (the guard did not exist on main). Newly restoring the
UpdateOfferPresentation.zig invocation without adding it to that list
would have made the guard itself flag it as unwired.

RED: after cherry-picking 775d77b, the guard's \ array
still lacked "UpdateOfferPresentation.zig" -> a manual simulation of the
guard's detection logic reported it as missing alongside the two entries
correctly reserved for #422.
GREEN: added "UpdateOfferPresentation.zig" to \ next to
"NativeForms.zig" -> the same simulation now reports exactly and only
GraphContextMenu.zig and MainWindow.zig as missing, matching the #422
reservation.
REGRESSION: reran the full harness end-to-end locally with pinned Zig
0.15.2 and GRAPHCODE_WINGHOSTTY_ROOT set -> all 37 invocations execute
(including the restored NativeForms 96/96 and UpdateOfferPresentation
1/1), the same 3 pre-existing Sidebar.zig failures are tolerated by name,
and the guard still throws on exactly GraphContextMenu.zig, MainWindow.zig.

Signed-off-by: Colin Neilens <coneilen@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* Wire GraphContextMenu.zig/MainWindow.zig into the guard's list after #422; correct Sidebar quarantine attribution

Rebased onto origin/main after #422 (issue #418) merged as 06e092e, which
wired GraphContextMenu.zig and MainWindow.zig into the harness's zig test
invocations and source-list array. #422 never touched this PR's own
\ guard array, because that array did not exist on main
when #422 was authored -- it is this PR's own addition. Added both
filenames to the list and updated the guard's explanatory comment, which
was otherwise now stale (still described the two files as reserved and
not-yet-landed).

Also corrected the Sidebar.zig quarantine reason: an independent review
determined that only 2 of the 3 quarantined failures are the real
layoutFor()/appendRows() product bug (#428, fix in flight as #430); the
third (sidebar scroll clamps overflow, shrink, and resize, expected 334
found 410) is a separate, stale test expectation -- the 76px delta is the
Activity block height that contentBottom/paint() correctly account for
and the test's oracle omitted. Left it quarantined (not fixed) since this
PR must not modify Sidebar.zig source, but the reason string now
attributes each failure accurately instead of lumping all three under one
root cause.

RED: mechanically diffed this branch's wired-file set against origin/main
after rebasing -> main had 22 entries (20 plus #422's 2), this branch
still reported only 38 in its own \ guard list, and running
the guard's detection logic directly showed it still flagging
GraphContextMenu.zig and MainWindow.zig as unwired despite their zig test
invocations now existing in the script.
GREEN: added both names to \ and reran the same detection
logic -> zero missing files reported.
REGRESSION: ran the complete harness end-to-end locally with pinned Zig
0.15.2 and GRAPHCODE_WINGHOSTTY_ROOT set -> all 39 invocations execute
including the newly-landed GraphContextMenu.zig (8/8) and MainWindow.zig
(6/6, appearing twice via App.zig's transitive import), the same 3
Sidebar.zig failures are tolerated by name with the corrected attribution
text rendering intact, and the full script now exits 0 -- the guard no
longer fires at all.

Signed-off-by: Colin Neilens <coneilen@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* Remove Sidebar/App.zig quarantine now that #430 fixed the underlying bug

PR #430 (merged as ece5935) fixed the real Sidebar.zig layout/row-count
desync structurally via a single shared predicate,
projectIsVisibleInSection, so all three previously-quarantined tests
now pass unconditionally. Landing the quarantine would have shipped a
misleading tolerate-list for an already-fixed bug, so remove it
entirely instead:

- Delete Invoke-NativeQuarantined and its Get-FailingZigTestNames
  helper (Sidebar.zig/App.zig were their only consumers).
- Delete the sidebarLayoutOpenProjectKnownFailures/-Reason variables.
- Convert both call sites to plain Invoke-Native.

Verified end-to-end locally with pinned Zig 0.15.2: exit 0, all 93
Sidebar.zig tests and the full 232-test App.zig suite pass
unconditionally, zero quarantine output. Re-ran the guard's
set-difference check against origin/main (ece5935): empty, with
UpdateOfferPresentation.zig and the NativeForms Assert-Contract block
both still intact.

Signed-off-by: Colin Neilens <coneilen@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

---------

Signed-off-by: Colin Neilens <coneilen@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.

Sidebar: layoutFor counts open recent projects as rendered rows, corrupting section height, scroll clamp, and hit testing

1 participant