Skip to content

feat: proper rendering of surge in all terminal sizes. #322

Merged
SuperCoolPencil merged 13 commits intomainfrom
tui-view-fix
Apr 6, 2026
Merged

feat: proper rendering of surge in all terminal sizes. #322
SuperCoolPencil merged 13 commits intomainfrom
tui-view-fix

Conversation

@junaid2005p
Copy link
Copy Markdown
Collaborator

@junaid2005p junaid2005p commented Apr 4, 2026 •
edited by greptile-apps Bot
Loading

closes #252

Greptile Summary

This PR delivers solid responsive terminal layout improvements to the Surge TUI, replacing the History feature with a HelpModal that shows keyboard shortcuts, and adding proper dimension clamping and cursor normalization throughout the category manager and settings views. The test suite is thorough with broad coverage across terminal sizes.

  • Stale iota comments: removing HistoryState shifted every subsequent constant's value by 1, but the inline "is N" comments were not updated — SearchState through HelpModalState all show one-higher-than-actual values.
  • Duplicate width logic: updateCategoryInputWidthsForViewport replicates the leftWidth/rightWidth calculation from renderCategoryTwoColumn but omits the rightWidth < minRightWidth re-adjustment and the bodyHeight >= 9 guard, which can leave model-side input widths mismatched until the next render.
  • Unexplained benchmark deletion: internal/benchmark/metrics.go and its test file are deleted with no mention in the PR description — please confirm this is intentional.

Confidence Score: 5/5

Safe to merge after confirming the benchmark deletion is intentional; no runtime correctness issues found in the rendering paths.

All remaining findings are P2 style/cleanup concerns. The responsive layout logic, state-machine transitions, and new tests are sound. No P0/P1 issues were identified.

internal/tui/model.go (stale iota comments), internal/tui/update_category.go (width logic duplication), internal/benchmark/metrics.go (unexplained deletion)

Important Files Changed

Filename Overview
internal/tui/model.go Removes HistoryState and adds HelpModalState correctly; iota comments off by 1 for SearchState through HelpModalState after HistoryState removal
internal/tui/view.go Help modal rendered with correct terminal-size clamping; responsive dashboard layout changes are correct
internal/tui/update.go WindowSizeMsg handler correctly triggers normalization and input-width update; HelpModalState esc-to-close is clean
internal/tui/update_category.go normalizeCategoryManagerSelection handles edge cases cleanly; updateCategoryInputWidthsForViewport duplicates view-side width logic with a missing bodyHeight guard
internal/tui/view_category.go Two-column/compact switching is correct; categoryModalDimensions properly clamps to terminal dimensions
internal/tui/view_settings.go Settings modal uses settingsModalDimensions consistently; compact/two-column switching is correct
internal/tui/components/help_modal.go Clean self-contained implementation; correctly uses the already-clamped Width/Height from the caller
internal/tui/keys.go ToggleHelp binding added correctly; HistoryKeyMap and related bindings cleanly removed
internal/tui/update_dashboard.go History handler removed cleanly; ToggleHelp transition to HelpModalState is correct
internal/tui/update_modals.go updateHistory removed cleanly with no orphan references
internal/tui/view_test.go Comprehensive new tests cover help modal, many terminal sizes, and layout integrity
internal/tui/update_test.go Existing tests unchanged and valid after state-machine modifications
internal/benchmark/metrics.go Entire file deleted in this PR with no explanation in the PR description
internal/benchmark/metrics_test.go Deleted alongside metrics.go with no explanation

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([DashboardState]) -->|"h: ToggleHelp"| B([HelpModalState])
    B -->|esc| A
    C([tea.WindowSizeMsg]) --> D{current state?}
    D -->|CategoryManagerState| E[normalizeCategoryManagerSelection]
    E --> F{catMgrEditing?}
    F -->|yes| G[updateCategoryInputWidthsForViewport]
    F -->|no| H[skip input resize]
    D -->|SettingsState| I[normalizeSettingsSelection]
    I --> J{SettingsIsEditing?}
    J -->|yes| K[updateSettingsInputWidthForViewport]
    J -->|no| L[skip]
    D -->|other states| M[update width/height only]
Loading

Comments Outside Diff (5)

  1. internal/tui/view_test.go, line 195-206 (link)

    P1 Test width triggers hideGraphStats path — 5-axis labels never rendered

    At m.width = 140, rightWidth = int(138 × 0.4) = 55, which falls in the [50, 70) band that sets hideGraphStats = true. That path emits at most 3 Y-axis labels (maxSpeed, maxSpeed×0.5, "0 MB/s"). The labels "0.8 MB/s" and "0.2 MB/s" (from maxSpeed×0.75 / ×0.25) only appear in the full-stats else branch, which requires rightWidth ≥ 70 — i.e. m.width ≥ 177. This test will always fail as written.

    Prompt To Fix With AI 
    This is a comment left during a code review.
Path: internal/tui/view_test.go
Line: 195-206

Comment:
**Test width triggers `hideGraphStats` path — 5-axis labels never rendered**

At `m.width = 140`, `rightWidth = int(138 × 0.4) = 55`, which falls in the `[50, 70)` band that sets `hideGraphStats = true`. That path emits at most **3** Y-axis labels (`maxSpeed`, `maxSpeed×0.5`, `"0 MB/s"`). The labels `"0.8 MB/s"` and `"0.2 MB/s"` (from `maxSpeed×0.75` / `×0.25`) only appear in the full-stats `else` branch, which requires `rightWidth ≥ 70` — i.e. `m.width ≥ 177`. This test will always fail as written.



How can I resolve this? If you propose a fix, please make it concise.

  2. internal/benchmark/metrics.go

    P2 Unrelated deletion of entire benchmark package

    Both metrics.go and its test file are removed here, but the PR's stated purpose is responsive TUI rendering. No remaining Go file imports this package so it compiles cleanly, but bundling an unrelated deletion makes git bisect harder. Consider splitting this into a separate commit or PR.

    Prompt To Fix With AI 
    This is a comment left during a code review.
Path: internal/benchmark/metrics.go
Line: 1

Comment:
**Unrelated deletion of entire `benchmark` package**

Both `metrics.go` and its test file are removed here, but the PR's stated purpose is responsive TUI rendering. No remaining Go file imports this package so it compiles cleanly, but bundling an unrelated deletion makes `git bisect` harder. Consider splitting this into a separate commit or PR.

How can I resolve this? If you propose a fix, please make it concise.

  3. internal/tui/view_test.go, line 195-206 (link)

    P1 Test will always fail at the chosen terminal width

    At m.width = 140, the layout math resolves to:

    • availableWidth = 138
    • leftWidth = int(138 × 0.6) = 82
    • rightWidth = 138 − 82 = 56
    • hideGraphStats = rightWidth >= 50 && rightWidth < 70 → true

    When hideGraphStats is true the code takes the branch at view.go:588, which only emits 3 labels (max, 0.5×max, "0 MB/s"). The 5-label path (including the 0.75×max and 0.25×max entries that produce "0.8 MB/s" and "0.2 MB/s" with default maxSpeed=1.0) only executes in the else branch when hideGraphStats=false, i.e. when rightWidth ≥ 70. That requires width ≳ 177.

    Fix: use a width that gives rightWidth ≥ 70, for example width = 200:

    Prompt To Fix With AI 
    This is a comment left during a code review.
Path: internal/tui/view_test.go
Line: 195-206

Comment:
**Test will always fail at the chosen terminal width**

At `m.width = 140`, the layout math resolves to:
- `availableWidth = 138`
- `leftWidth = int(138 × 0.6) = 82`
- `rightWidth = 138 − 82 = 56`
- `hideGraphStats = rightWidth >= 50 && rightWidth < 70` → **`true`**

When `hideGraphStats` is `true` the code takes the branch at `view.go:588`, which only emits **3 labels** (max, 0.5×max, "0 MB/s"). The 5-label path (including the 0.75×max and 0.25×max entries that produce `"0.8 MB/s"` and `"0.2 MB/s"` with default `maxSpeed=1.0`) only executes in the `else` branch when `hideGraphStats=false`, i.e. when `rightWidth ≥ 70`. That requires `width ≳ 177`.

Fix: use a width that gives `rightWidth ≥ 70`, for example `width = 200`:



How can I resolve this? If you propose a fix, please make it concise.

  4. internal/tui/view.go, line 547-556 (link)

    P2 Redundant dual-variable loop — topSpeed duplicates maxSpeed logic

    Both variables are updated with identical comparisons (if v > x { x = v }) in the same loop, so topSpeed == maxSpeed immediately after the loop exits. The only distinction is that maxSpeed subsequently receives headroom (×1.1, rounded). topSpeed simply captures the raw peak, which means it can be derived from maxSpeed before the headroom block runs:

    topSpeed := 0.0
for _, v := range graphData {
    if v > topSpeed {
        topSpeed = v
    }
}
maxSpeed := topSpeed // raw peak; will be scaled below

if maxSpeed == 0 {
    maxSpeed = 1.0
} else {
    maxSpeed = maxSpeed * 1.1
    // ...
}

    This eliminates the duplicate conditional per element without changing any observable behaviour.

    Prompt To Fix With AI 
    This is a comment left during a code review.
Path: internal/tui/view.go
Line: 547-556

Comment:
**Redundant dual-variable loop — `topSpeed` duplicates `maxSpeed` logic**

Both variables are updated with identical comparisons (`if v > x { x = v }`) in the same loop, so `topSpeed == maxSpeed` immediately after the loop exits. The only distinction is that `maxSpeed` subsequently receives headroom (×1.1, rounded). `topSpeed` simply captures the raw peak, which means it can be derived from `maxSpeed` *before* the headroom block runs:

```go
topSpeed := 0.0
for _, v := range graphData {
    if v > topSpeed {
        topSpeed = v
    }
}
maxSpeed := topSpeed // raw peak; will be scaled below

if maxSpeed == 0 {
    maxSpeed = 1.0
} else {
    maxSpeed = maxSpeed * 1.1
    // ...
}
```

This eliminates the duplicate conditional per element without changing any observable behaviour.

How can I resolve this? If you propose a fix, please make it concise.

  5. internal/tui/view.go, line 388 (link)

    P1 Hardcoded graph minimum ignores adaptive minGraphHeight

    Line 388 subtracts a literal 9 for the graph minimum when computing the available chunk-map height, but the PR introduces minGraphHeight (lines 321–323) which can be 5 on short terminals. On a shortTerminal layout the chunk map is allocated 4 fewer rows than the actual graph minimum allows, producing a tighter chunk map than necessary and an inconsistent layout budget.

    Prompt To Fix With AI 
    This is a comment left during a code review.
Path: internal/tui/view.go
Line: 388

Comment:
**Hardcoded graph minimum ignores adaptive `minGraphHeight`**

Line 388 subtracts a literal `9` for the graph minimum when computing the available chunk-map height, but the PR introduces `minGraphHeight` (lines 321–323) which can be `5` on short terminals. On a `shortTerminal` layout the chunk map is allocated 4 fewer rows than the actual graph minimum allows, producing a tighter chunk map than necessary and an inconsistent layout budget.



How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI 
This is a comment left during a code review.
Path: internal/tui/model.go
Line: 36-45

Comment:
**Stale iota values in comments**

Removing `HistoryState` (which held iota 5) shifted every subsequent constant down by one, but none of the inline "is N" comments were updated. As written, `SearchState` says "is 6" but is actually 5, and the off-by-one cascades all the way to `HelpModalState` (says "is 15", actually 14).

Either correct all the numbers or remove the numeric annotations — the iota sequence is self-documenting and wrong values are actively misleading.

```suggestion
	SearchState                               // SearchState is 5
	SettingsState                             // SettingsState is 6
	ExtensionConfirmationState                // ExtensionConfirmationState is 7
	BatchFilePickerState                      // BatchFilePickerState is 8
	BatchConfirmState                         // BatchConfirmState is 9
	UpdateAvailableState                      // UpdateAvailableState is 10
	URLUpdateState                            // URLUpdateState is 11
	CategoryManagerState                      // CategoryManagerState is 12
	QuitConfirmState                          // QuitConfirmState is 13
	HelpModalState                            // HelpModalState is 14
```

**Rule Used:** What: Comments must explain *why* code exists, not... ([source](https://app.greptile.com/review/custom-context?memory=4e45ef13-32c2-4753-8060-a44ef767144d))

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: internal/tui/update_category.go
Line: 69-98

Comment:
**Duplicate width calculation diverges from the view**

`updateCategoryInputWidthsForViewport` replicates the `leftWidth`/`rightWidth` logic from `renderCategoryTwoColumn` (`view_category.go` lines 299–315) but is missing two guards present in the view:

1. **`rightWidth < minRightWidth` re-adjustment** — `renderCategoryTwoColumn` re-clamps `rightWidth` and adjusts `leftWidth` a second time (lines 310–315); this function does not.
2. **`bodyHeight >= 9` condition** — `viewCategoryManager` chooses compact mode when `bodyHeight < 9` even if `modalWidth >= 76` (line 81). This function uses the two-column formula whenever `modalWidth >= 76`, so on a wide but short terminal it sets two-column input widths while the renderer takes the compact path.

The mismatch only persists until the next repaint (both rendering helpers call `updateCategoryInputWidthsForViewport` on a local copy and correct it), but it means the text-input cursor/scroll position can be set to the wrong size between a resize event and the following render. Consider extracting a shared `categoryInputWidth(modalWidth, bodyHeight int) int` helper used by both paths.

**Rule Used:** What: Eliminate duplicate logic, functions, or cod... ([source](https://app.greptile.com/review/custom-context?memory=f0a796a9-684f-4dfb-a5cf-8c99c16b63a1))

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: internal/benchmark/metrics.go
Line: 1

Comment:
**Benchmark package deleted without explanation**

`internal/benchmark/metrics.go` and `internal/benchmark/metrics_test.go` are entirely removed in this PR, which is described as a terminal-rendering fix. No mention of this deletion appears in the PR description or commit message.

Please confirm this is intentional and, if so, add a brief note to the PR description (e.g., "removed unused benchmark package"). If the deletion was accidental, the files should be restored.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (8): Last reviewed commit: "feat(tui): make category page appear con..." | Re-trigger Greptile

Context used:

  • Rule used - What: Comments must explain why code exists, not... (source)
  • Rule used - What: Eliminate duplicate logic, functions, or cod... (source)

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 4, 2026 •
edited
Loading

Binary Size Analysis

⚠️ Size Increased

Version Human Readable Raw Bytes
Main 18.84 MB 19755300
PR 18.91 MB 19829028
Difference 72.00 KB 73728

greptile-apps[bot]
greptile-apps Bot reviewed Apr 4, 2026
View reviewed changes
Comment thread internal/tui/view.go Outdated
Comment thread internal/tui/view.go
greptile-apps[bot]
greptile-apps Bot reviewed Apr 4, 2026
View reviewed changes
Comment thread internal/tui/view.go Outdated
Comment thread internal/tui/view.go
junaid2005p and others added 6 commits April 5, 2026 02:26
@junaid2005p
test(tui): fixed failing tests
51cefbd
@junaid2005p
Merge branch 'tui-view-fix' of github.com:JunaidIslam2105/surge into …
cda6aee 
…tui-view-fix
@junaid2005p
Merge branch 'main' into tui-view-fix
573adfc
@SuperCoolPencil @claude
feat(tui): compact keyboard shortcuts behind a help overlay
3fd3292 
- Show only [h] keybindings in the footer by default
- Press h to open a styled modal with all shortcuts
- Press esc to close the modal
- Remove broken history feature (no View handler, caused crash)

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@SuperCoolPencil @claude
fix(tui): resolve review comments on help modal and graph layout
1e58f8d 
- Use minGraphHeight variable instead of hardcoded 9 for chunk map budget
- Remove double spaces in help modal title
- Add test for HelpModalState rendering and esc-to-close behavior
- Omit network activity graph box when too small to render

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@SuperCoolPencil
add colors to help modal
549e949
greptile-apps[bot]
greptile-apps Bot reviewed Apr 4, 2026
View reviewed changes
Comment thread internal/tui/view.go
@SuperCoolPencil
Copy link
Copy Markdown
Member

SuperCoolPencil commented Apr 6, 2026

@junaid2005p

Are you planning to fix settings TUI in this PR or are we just targeting the dashboard?

@junaid2005p
Copy link
Copy Markdown
Collaborator Author

junaid2005p commented Apr 6, 2026

I am planning to I'll let you know when done :)

SuperCoolPencil
SuperCoolPencil approved these changes Apr 6, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@SuperCoolPencil SuperCoolPencil left a comment

Choose a reason for hiding this comment

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

Looks amazing!

@SuperCoolPencil SuperCoolPencil merged commit 673d9a7 into main Apr 6, 2026
15 checks passed
@SuperCoolPencil SuperCoolPencil deleted the tui-view-fix branch April 6, 2026 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

@SuperCoolPencil SuperCoolPencil SuperCoolPencil approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unexpected behaviour when the terminal is in a non-standard size.

2 participants

@junaid2005p @SuperCoolPencil