We need to have sane behavior in error handling because the running
program that sends the restore cursor command has no way to realize it
failed. So if our style fails to add (our only fail case) then we revert
to no style.
https://ampcode.com/threads/T-019bd7dc-cf0b-7439-ad2f-218b3406277a
Fixes#10369
When `resizeWithoutReflowGrowCols` copies rows to a previous page with
spare capacity, tracked pins pointing to those rows were not being
remapped. This left pins pointing to the original page which was
subsequently destroyed.
The fix adds pin remapping for rows copied to the previous page,
matching the existing remapping logic for rows copied to new pages.
I also added new integrity checks to verify that our tracked pins are
always valid at points where internal operations complete.
Thanks to @grishy for finding this!
**AI disclosure:** Amp used for verifying and fixing this bug. I
reviewed the results and just did minor manual tweaks.
https://ampcode.com/threads/T-019bd6d7-0645-73dd-8fd7-659f019fa83d and
https://ampcode.com/threads/T-019bd6c9-cc2e-73bc-bbaa-f8766e11c234
Fixes#10369
When `resizeWithoutReflowGrowCols` copies rows to a previous page with
spare capacity, tracked pins pointing to those rows were not being remapped.
This left pins pointing to the original page which was subsequently destroyed.
The fix adds pin remapping for rows copied to the previous page,
matching the existing remapping logic for rows copied to new pages.
I also added new integrity checks to verify that our tracked pins are
always valid at points where internal operations complete.
Thanks to @grishy for finding this!
This never caused any known issues, but it's a bug! `increaseCapacity`
should produce a node with identical contents, just more capacity. We
were forgetting to copy over the dirty flag.
I looked back at `adjustCapacity` and it also didn't preserve the dirty
flag so presumably downstream consumers have been handling this case
manually. But, I think semantically it makes sense for
`increaseCapacity` to preserve the dirty flag.
This bug was found by AI (while I was doing another task). I fixed it
and wrote the test by hand though.
We previously wrote our new cache file into a temporary directory and
the (atomically) renamed it to the canonical cache file path. This
rename operation unfortunately only works when both files are on the
same file system, and that's not always the case (e.g. when $TMPDIR is
on its own file system).
Instead, we can use Zig's AtomicFile to safely perform this operation
inside of the cache directory.
There's a new risk of a crash leaving the temporary file around in this
directory (and not getting cleaned up like $TMPDIR-based files), but the
probability is low and those files will only be readable by the creating
user (mode 0o600).
There's a new test cash that verifies the expected AtomicFile clean up
behavior. I also switched the file-oriented tests to use testing.tmpDir
rather than using our application-level TempDir type.
Fixes#10364
---
**AI Disclosure:** I asked Claude to write the initial test case to
verify the AtomicFile cleanup behavior.
This never caused any known issues, but it's a bug! `increaseCapacity`
should produce a node with identical contents, just more capacity. We
were forgetting to copy over the dirty flag.
I looked back at `adjustCapacity` and it also didn't preserve the dirty
flag so presumably downstream consumers have been handling this case
manually. But, I think semantically it makes sense for
`increaseCapacity` to preserve the dirty flag.
This bug was found by AI (while I was doing another task). I fixed it
and wrote the test by hand though.
We previously wrote our new cache file into a temporary directory and
the (atomically) renamed it to the canonical cache file path. This
rename operation unfortunately only works when both files are on the
same file system, and that's not always the case (e.g. when $TMPDIR is
on its own file system).
Instead, we can use Zig's AtomicFile to safely perform this operation
inside of the cache directory.
There's a new risk of a crash leaving the temporary file around in this
directory (and not getting cleaned up like $TMPDIR-based files), but the
probability is low and those files will only be readable by the creating
user (mode 0o600).
There's a new test cash that verifies the expected AtomicFile clean up
behavior. I also switched the file-oriented tests to use testing.tmpDir
rather than using our application-level TempDir type.
Fixes#10352
The bug was that non-standard pages would mix the old
`growRequiredForActive` check and make our active area insufficient in
the PageList.
But, since scrollbars now require we have a cached `total_rows` that our
safety checks always verify, we can remove the old linked list traversal
and switch to some simple math in general across all page sizes.
Fixes#10352
The bug was that non-standard pages would mix the old
`growRequiredForActive` check and make our active area insufficient in
the PageList.
But, since scrollbars now require we have a cached `total_rows` that our
safety checks always verify, we can remove the old linked list traversal
and switch to some simple math in general across all page sizes.
Fixes#10258
Replaces #10284
1. `Page.Capacity` now uses smaller bit-width integers that represent a
true maximum capacity for various fields.
2. On 64-bit systems, a maxed out `Page.Capacity` (every field `maxInt`)
can be represented in an addressable allocation (total required memory
less than 64 bits). This means `Page.layout` can't overflow.
3. All `adjustCapacity` functions replaced with `increaseCapacity` which
doesn't allow specifying the resulting value, which makes it so overflow
is only possible in significantly fewer places, making it easier to
handle in general.
4. `increaseCapacity` can return a new error `OutOfSpace` which happens
when overflow is detected. This means that no valid page can accommodate
the desired capacity increase because we're already at the max. The
caller is expected to handle this.
5. Updated our resize so that the only possible error is system OOM, we
handle the new `OutOfSpace` by copying the recent reflowed row into a
new page and continuing.
A very, very high-level overview is below. The "overflow" here papers
over a bunch of details where the prior usize capacities flowed through
to Page.layout and ultimately RefCountedSet and other managed types
which then caused incorrect calculations on total memory size required.
```mermaid
flowchart TB
subgraph Before["Before: adjustCapacity"]
A1[capacity: usize] --> A2["capacity *= 2"]
A2 --> A3{Overflow?}
A3 -->|"Not detected"| A4["Massive allocation or crash"]
end
subgraph After["After: increaseCapacity"]
B1["capacity: bounded int<br/>(u16/u32)"] --> B2["capacity *= 2"]
B2 --> B3{Overflow?}
B3 -->|"OutOfSpace error"| B4["Graceful handling:<br/>move row to new page"]
B3 -->|"Success"| B5["Normal allocation"]
end
Before --> After
classDef beforeStyle fill:#3d1a1a,stroke:#ff6b6b,color:#ff6b6b
classDef afterStyle fill:#1a3d3a,stroke:#4ecdc4,color:#4ecdc4
class A1,A2,A3,A4 beforeStyle
class B1,B2,B3,B4,B5 afterStyle
```
Fixes#10282
The function `cursorChangePin` is supposed to be called anytime the
cursor page pin changes, but it itself may alter the page pin if setting
up the underlying managed memory forces a page size adjustment.
Multiple callers to this function were erroneously reusing the old page
pin value.
**AI disclosure:** I had Amp help me write the test. I eyeballed and
found the bug myself, verified it by asking Amp to write the test,
reviewed that manually, then implemented the fixes manually and got it
to pass.
Fixes#10282
The function `cursorChangePin` is supposed to be called anytime the
cursor page pin changes, but it itself may alter the page pin if setting
up the underlying managed memory forces a page size adjustment.
Multiple callers to this function were erroneously reusing the old page
pin value.
This simplifies our CI command line and makes it easier to document
expected usage (in HACKING.md).
There unfortunately isn't a way to set --checked-sourced or our default
warning level in .shellcheckrc, and our `find` command is still a bit
unwieldy, but this is still a net improvement.
This simplifies our CI command line and makes it easier to document
expected usage (in HACKING.md).
There unfortunately isn't a way to set --checked-sourced or our default
warning level in .shellcheckrc, and our `find` command is still a bit
unwieldy, but this is still a net improvement.
## Summary
- Fix literal \n appearing in window titles when running commands in zsh
## Description
The zsh shell integration was using ${(V)1} parameter expansion to set
the window title. The (V) flag converts control characters to their
visible escape sequence representations, causing commands ending with a
newline to display as command\n in the title bar.
Changed to use ${1//[[:cntrl:]]} which strips control characters
entirely, matching the behavior of the bash integration.
<img width="231" height="47" alt="image"
src="https://github.com/user-attachments/assets/356c77a1-32da-47b0-bf86-29735cc67e90"
/>
## Test
- Open Ghostty with zsh shell integration enabled
- Run a command (e.g., cargo install trunk)
---
AI Disclosure: This PR was written primarily by Claude Code. (Opus 4.5)
The zsh shell integration was using `${(V)1}` parameter expansion to set
the window title, which converts control characters to their visible
escape sequence representations. This caused commands ending with a
newline to display as `command\n` in the title bar.
Changed to use `${1//[[:cntrl:]]}` which strips control characters
entirely, matching the behavior of the bash integration.
