skip terminal grid sizing when available width is non-positive - #3140
skip terminal grid sizing when available width is non-positive#3140thedhruvhegde wants to merge 1 commit into
Conversation
…terminal grid size.
Greptile SummaryThis PR prevents terminal grid sizing when horizontal padding and scrollbar space leave no usable width.
Confidence Score: 4/5The implementation appears safe to merge, with a non-blocking test-coverage gap at the exact-zero width boundary. The production guard is consistent with existing height handling and its callers safely handle null; only the precise Files Needing Attention: apps/emdash-desktop/src/core/features/terminals/browser/pty/pty-dimensions.test.ts
|
| Filename | Overview |
|---|---|
| apps/emdash-desktop/src/core/features/terminals/api/browser/pty/pty-dimensions.ts | Correctly returns null for non-positive available width, using an established contract already handled by callers. |
| apps/emdash-desktop/src/core/features/terminals/browser/pty/pty-dimensions.test.ts | Covers negative available width but omits the exact-zero boundary introduced by the new condition. |
Prompt To Fix All With AI
### Issue 1
apps/emdash-desktop/src/core/features/terminals/browser/pty/pty-dimensions.test.ts:43-49
**Zero width remains untested**
This fixture produces `availW = -12` (`4 - 8 - 8`), so it only tests negative available width. The changed contract also covers `availW === 0`; without an exact-zero case, changing the guard from `<= 0` to `< 0` would still pass this test and leave the boundary behavior unprotected.
```suggestion
computeGridDimensions({
widthPx: 16,
heightPx: 400,
cellWidth: 8,
cellHeight: 16,
paddingPx: 8,
})
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "treat non-positive available width the s..." | Re-trigger Greptile
| computeGridDimensions({ | ||
| widthPx: 4, | ||
| heightPx: 400, | ||
| cellWidth: 8, | ||
| cellHeight: 16, | ||
| paddingPx: 8, | ||
| }) |
There was a problem hiding this comment.
This fixture produces availW = -12 (4 - 8 - 8), so it only tests negative available width. The changed contract also covers availW === 0; without an exact-zero case, changing the guard from <= 0 to < 0 would still pass this test and leave the boundary behavior unprotected.
| computeGridDimensions({ | |
| widthPx: 4, | |
| heightPx: 400, | |
| cellWidth: 8, | |
| cellHeight: 16, | |
| paddingPx: 8, | |
| }) | |
| computeGridDimensions({ | |
| widthPx: 16, | |
| heightPx: 400, | |
| cellWidth: 8, | |
| cellHeight: 16, | |
| paddingPx: 8, | |
| }) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/emdash-desktop/src/core/features/terminals/browser/pty/pty-dimensions.test.ts
Line: 43-49
Comment:
**Zero width remains untested**
This fixture produces `availW = -12` (`4 - 8 - 8`), so it only tests negative available width. The changed contract also covers `availW === 0`; without an exact-zero case, changing the guard from `<= 0` to `< 0` would still pass this test and leave the boundary behavior unprotected.
```suggestion
computeGridDimensions({
widthPx: 16,
heightPx: 400,
cellWidth: 8,
cellHeight: 16,
paddingPx: 8,
})
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
summary
computeGridDimensionsalready bailed onavailH <= 0but not width. a collapsed pane could still produce a bogus column count.return null when
availW <= 0as well.test plan
pnpm --dir apps/emdash-desktop exec vitest run src/core/features/terminals/browser/pty/pty-dimensions.test.ts