Skip to content

Fix keyboard help dialog row overlap #122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
benvinegar merged 1 commit into from
Mar 26, 2026
Merged

Fix keyboard help dialog row overlap #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 37 additions & 15 deletions src/ui/components/chrome/HelpDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,32 +53,54 @@ export function HelpDialog({
const bodyWidth = Math.max(1, width - 4);
const keyWidth = Math.min(16, Math.max(12, Math.floor(bodyWidth * 0.28)));
const descriptionWidth = Math.max(1, bodyWidth - keyWidth);
const height = Math.min(
sections.reduce((total, section) => total + 1 + section.items.length, 0) + 3,
Math.max(8, terminalHeight - 2),
const sectionSpacerRowCount = Math.max(0, sections.length - 1);
const contentRowCount =
sections.reduce((rowCount, section) => rowCount + 1 + section.items.length, 0) +
sectionSpacerRowCount;
// ModalFrame contributes the border rows, title row, padding, and one blank spacer row.
const modalFrameChromeRowCount = 6;
const requiredModalHeight = contentRowCount + modalFrameChromeRowCount;
Comment on lines +60 to +62

@greptile-apps greptile-apps Bot Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 modalFrameChromeRowCount may be off by one

The comment says the 6 accounts for "border rows, title row, padding, and one blank spacer row," but tracing through ModalFrame's JSX I can only identify 5 rows of overhead:

ModalFrame structure Rows
Top border (border: true) 1
paddingTop: 1 on the header box 1
Title text row 1
paddingBottom: 1 on the content box 1
Bottom border 1
Total 5

There is no explicit blank spacer element in ModalFrame. If the real chrome overhead is 5, requiredModalHeight is overestimated by 1 — the modal will be 1 row taller than necessary in normal conditions, and shouldScroll will trigger one terminal row earlier than it needs to. For example, with contentRowCount = 18 and terminalHeight = 25:

  • Current (chrome=6): requiredModalHeight=24, modalHeight=23, shouldScroll=true ← unnecessary scrollbox
  • If chrome=5: requiredModalHeight=23, modalHeight=23, shouldScroll=false ← all content fits

It would be worth double-checking the opentui border and padding accounting, and if the actual overhead is indeed 5, updating the constant and comment:

Suggested change
// ModalFrame contributes the border rows, title row, padding, and one blank spacer row.
const modalFrameChromeRowCount = 6;
const requiredModalHeight = contentRowCount + modalFrameChromeRowCount;
// ModalFrame contributes: top border, paddingTop, title row, paddingBottom, bottom border = 5 rows.
const modalFrameChromeRowCount = 5;

@ferologics ferologics Mar 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I checked this against the actual rendered frame instead of just the JSX structure. With one fewer chrome row, the dialog still overlaps content (for example Review/ m), so 6 is intentional here even though the visible structure looks like 5 rows at a glance.

const modalHeight = Math.min(requiredModalHeight, Math.max(8, terminalHeight - 2));
const shouldScroll = modalHeight < requiredModalHeight;
const content = (
<box style={{ width: "100%", flexDirection: "column" }}>
{sections.map((section, sectionIndex) => (
<box key={section.title} style={{ width: "100%", flexDirection: "column" }}>
<box style={{ width: "100%", height: 1 }}>
<text fg={theme.badgeNeutral}>{section.title}</text>
</box>
{section.items.map(([keys, description]) => (
<box
key={`${section.title}:${keys}`}
style={{ width: "100%", height: 1, flexDirection: "row" }}
>
<text fg={theme.accent}>{padText(fitText(keys, keyWidth), keyWidth)}</text>
<text fg={theme.muted}>{fitText(description, descriptionWidth)}</text>
</box>
))}
{sectionIndex < sections.length - 1 ? <box style={{ width: "100%", height: 1 }} /> : null}
</box>
))}
</box>
);

return (
<ModalFrame
height={height}
height={modalHeight}
terminalHeight={terminalHeight}
terminalWidth={terminalWidth}
theme={theme}
title="Keyboard help"
width={width}
onClose={onClose}
>
{sections.map((section) => (
<box key={section.title} style={{ flexDirection: "column" }}>
<text fg={theme.badgeNeutral}>{section.title}</text>
{section.items.map(([keys, description]) => (
<box key={`${section.title}:${keys}`} style={{ flexDirection: "row" }}>
<text fg={theme.accent}>{padText(fitText(keys, keyWidth), keyWidth)}</text>
<text fg={theme.muted}>{fitText(description, descriptionWidth)}</text>
</box>
))}
</box>
))}
{shouldScroll ? (
<scrollbox focused={false} height="100%" scrollY={true} width="100%">
{content}
</scrollbox>
) : (
content
)}
</ModalFrame>
);
}
49 changes: 39 additions & 10 deletions test/ui-components.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,27 +836,56 @@ describe("UI components", () => {
expect(frame).toContain("beta");
});

test("HelpDialog renders the grouped keyboard help modal", async () => {
test("HelpDialog renders every keyboard shortcut row without overlap", async () => {
const theme = resolveTheme("midnight", null);
const frame = await captureFrame(
<HelpDialog
canRefresh={true}
terminalHeight={20}
terminalHeight={28}
terminalWidth={76}
theme={theme}
onClose={() => {}}
/>,
76,
20,
28,
);

expect(frame).toContain("Keyboard help");
expect(frame).toContain("[Esc]");
expect(frame).toContain("Navigation");
expect(frame).toContain("View");
expect(frame).toContain("Review");
expect(frame).toContain("F10");
expect(frame).toContain("reload / quit");
const expectedRows = [
"Keyboard help",
"[Esc]",
"Navigation",
"↑ / ↓ move line-by-line",
"Space / f page down (alt: f)",
"b page up",
"Shift+Space page up (alt)",
"d / u half page down / up",
"[ / ] previous / next hunk",
"Home / End jump to top / bottom",
Comment on lines +839 to +863

@greptile-apps greptile-apps Bot Mar 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 No test coverage for the scroll fallback path

The updated test validates that content renders without overlap when the terminal is large enough (terminalHeight=26), which maps to shouldScroll=false. The new shouldScroll=true code path (the <scrollbox> branch) is not exercised by any test — a terminal height of, say, 22 would trigger it.

Consider adding a companion test that renders at a smaller terminal height (e.g., terminalHeight=20) and asserts that the first and last sections are still present/readable, confirming that the scroll fallback at least mounts without crashing and doesn't silently clip all content.

@ferologics ferologics Mar 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair point. I kept this PR focused on the overlap regression path and asserted the full visible help content that was previously getting corrupted. I’d rather cover the small-terminal scroll fallback in a separate follow-up test than broaden this fix further.

"View",
"1 / 2 / 0 split / stack / auto",
"s / t sidebar / theme",
"a toggle AI notes",
"l / w / m lines / wrap / metadata",
"Review",
"/ focus file filter",
"Tab swap files / filter focus",
"F10 open menus",
"r / q reload / quit",
] as const;

for (const expectedRow of expectedRows) {
expect(frame).toContain(expectedRow);
}

const lines = frame.split("\n");
const blankModalRow = /│\s+│/;
const viewHeaderIndex = lines.findIndex((line) => line.includes("│ View"));
const reviewHeaderIndex = lines.findIndex((line) => line.includes("│ Review"));

expect(lines[viewHeaderIndex - 1]).toMatch(blankModalRow);
expect(lines[reviewHeaderIndex - 1]).toMatch(blankModalRow);
expect(frame).not.toContain("linese/Awrapt/smetadata");
expect(frame).not.toContain("reloade/uquit");
});

test("DiffSectionPlaceholder preserves offscreen section chrome without mounting rows", async () => {
Expand Down
Loading