Skip to content

fix(core): correct TUI sidebar viewport height off-by-one#34682

Open
comp615 wants to merge 1 commit intonrwl:masterfrom
comp615:fix/tui-sidebar-viewport-off-by-one
Open

fix(core): correct TUI sidebar viewport height off-by-one#34682
comp615 wants to merge 1 commit intonrwl:masterfrom
comp615:fix/tui-sidebar-viewport-off-by-one

Conversation

@comp615
Copy link
Copy Markdown
Contributor

@comp615 comp615 commented Mar 3, 2026

This PR description was generated by Amp.

Current Behavior

The TUI task sidebar appears scrolled down by one entry on render, hiding the first continuous task from view.

Screenshot 2026-03-03 at 10 56 09 AM

Here you can see that watch-deps is running in addition to dev.

Expected Behavior

All continuous tasks should be visible in the sidebar without any initial scroll offset.

Related Issue(s)

N/A (discovered via visual inspection)

Details

The viewport height calculation in tasks_list.rs subtracted 4 rows for header overhead, but the actual overhead is only 3 rows:

  1. top_margin (1 row)
  2. Header content (1 row)
  3. Spacing row (1 row)

This off-by-one caused the viewport to be 1 row too small, making the task list appear scrolled down by one entry on initial render.

Changes

  1. Unified overhead constant: Introduced TABLE_HEADER_OVERHEAD_ROWS = 3 and SCROLLBAR_Y_OFFSET = 2, replacing scattered hardcoded values. The scrollbar y-offset is 2 (not 3) because the scrollbar visually spans from the spacing row downward, while the viewport only counts content rows.

  2. Stale scroll correction: Added logic in set_viewport_height to reset scroll_offset to 0 when the viewport grows from a small placeholder size (≤ 5) to the real terminal size, provided the selected item is still visible from the top. This prevents stale scroll positions set during initialization from hiding top items.

  3. Regression tests: Added test_viewport_growth_resets_stale_scroll_offset and test_viewport_growth_preserves_scroll_when_selection_not_visible to cover the stale scroll correction.

  4. Snapshot updates: 14 snapshot files updated to reflect one additional visible row.

History

The 4 originated in commit 6541751a (James Henry, Apr 2025) with the comment "Reserve space for pagination and borders." However, the table had no borders (Block::default() without Borders), and pagination lived in its own layout chunk. The 4 was a rough estimate that was never precisely derived from the actual header structure. When scrolling replaced pagination in commit 1782e8c7 (Leosvel Perez Espinola, Sep 2025), the value was carried forward unchanged. The same commit also introduced a separate header_and_spacing_rows = 2 for the scrollbar y-offset, confirming these values were set independently without unified accounting.

Also documents the copy-built-package script in AGENTS.md for AI-assisted development workflows.

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 3, 2026

Deploy Preview for nx-docs canceled.

Name Link
🔨 Latest commit 8fa6f29
🔍 Latest deploy log https://app.netlify.com/projects/nx-docs/deploys/69a88f962036cc0008506f58

@comp615 comp615 force-pushed the fix/tui-sidebar-viewport-off-by-one branch from 4ffe6af to 6ddcac7 Compare March 3, 2026 16:17
@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 3, 2026

Deploy Preview for nx-dev canceled.

Name Link
🔨 Latest commit 8fa6f29
🔍 Latest deploy log https://app.netlify.com/projects/nx-dev/deploys/69a88f962036cc0008506f5d

@comp615 comp615 force-pushed the fix/tui-sidebar-viewport-off-by-one branch from 6ddcac7 to a3e1759 Compare March 4, 2026 02:00
@comp615 comp615 marked this pull request as ready for review March 4, 2026 02:14
@comp615 comp615 requested review from a team, Coly010 and FrozenPandaz as code owners March 4, 2026 02:14
@comp615 comp615 force-pushed the fix/tui-sidebar-viewport-off-by-one branch 4 times, most recently from 6092e30 to 70f3a2c Compare March 4, 2026 13:35
@nx-cloud
Copy link
Copy Markdown
Contributor

nx-cloud Bot commented Mar 4, 2026

View your CI Pipeline Execution ↗ for commit 8fa6f29

Command Status Duration Result
nx affected --targets=lint,test,build,e2e,e2e-c... ✅ Succeeded 53m 2s View ↗
nx run-many -t check-imports check-lock-files c... ✅ Succeeded 3m 21s View ↗
nx-cloud record -- nx-cloud conformance:check ✅ Succeeded 8s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 1s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded <1s View ↗

☁️ Nx Cloud last updated this comment at 2026-03-04 22:49:28 UTC

Copy link
Copy Markdown
Contributor

@nx-cloud nx-cloud Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.

Nx Cloud is proposing a fix for your failed CI:

These changes fix the nx:format-native failure by reformatting the assert_eq! macro in the newly added viewport growth test to comply with Rust's cargo fmt standards. The multi-line assertion was condensed to a single line, which is the expected format for this macro signature, ensuring the code passes formatting validation without altering the test's behavior or the PR's core viewport fix.

Tip

We verified this fix by re-running nx:format-native.

diff --git a/packages/nx/src/native/tui/components/task_selection_manager.rs b/packages/nx/src/native/tui/components/task_selection_manager.rs
index 0606acc26a..f30175effe 100644
--- a/packages/nx/src/native/tui/components/task_selection_manager.rs
+++ b/packages/nx/src/native/tui/components/task_selection_manager.rs
@@ -928,8 +928,7 @@ mod tests {
 
         // scroll_offset should reset to 0 because task-1 (selected, index 0) fits
         assert_eq!(
-            manager.scroll_offset,
-            0,
+            manager.scroll_offset, 0,
             "scroll_offset should reset to 0 when viewport grows from placeholder"
         );
 

🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.

Because this branch comes from a fork, it is not possible for us to apply fixes directly, but you can apply the changes locally using the available options below.

Apply changes locally with:

npx nx-cloud apply-locally TBaO-24o5

Apply fix locally with your editor ↗   View interactive diff ↗



🎓 Learn more about Self-Healing CI on nx.dev

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 4, 2026

Deploy Preview for nx-dev canceled.

Name Link
🔨 Latest commit 4ffe6af
🔍 Latest deploy log https://app.netlify.com/projects/nx-dev/deploys/69a7088ec2a4bf0008531a73

The viewport height calculation subtracted 4 rows for header overhead,
but the actual overhead is only 3 rows (top_margin + header content +
spacing row). This caused the task list to appear scrolled down by one
entry on initial render.

Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019cb994-7041-720d-a299-402ff210e978
@comp615 comp615 force-pushed the fix/tui-sidebar-viewport-off-by-one branch from 70f3a2c to 8fa6f29 Compare March 4, 2026 20:01
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