Skip to content

fix(context-actions): fix off by one context actions#124

Merged
sudo-tee merged 4 commits intomainfrom
fix/actions-off-by-1
Nov 19, 2025
Merged

fix(context-actions): fix off by one context actions#124
sudo-tee merged 4 commits intomainfrom
fix/actions-off-by-1

Conversation

@sudo-tee
Copy link
Owner

There was 2 off by one errors when adding contextual actions

When adding them in snapshots

When refreshing them after a part update

A couple of test needed to be regenerated for it to work

I added the possibility to regenerate a test snapshot by it's name or file-name

@sudo-tee sudo-tee requested a review from cameronr November 18, 2025 17:06
@sudo-tee
Copy link
Owner Author

sudo-tee commented Nov 18, 2025

@cameronr When you have a chance, can you do a quick check/review on this.

I was not seeing the context action on restore points anymore and I investigated.

This is my attempt at fixing it.

There was 2 off by one errors when adding contextual actions

When adding them in snapshots

When refreshing them after a part update

A couple of test needed to be regenerated for it to work

I added the possibility to regenerate a test snapshot by it's name or
file-name
@sudo-tee sudo-tee force-pushed the fix/actions-off-by-1 branch from 11a5015 to 1c136a0 Compare November 18, 2025 17:51
@cameronr
Copy link
Collaborator

Ah, I see the issue. When I added the extra newline at the end, I didn't account for that for actions. I didn't catch it with the replay tests because they needed to be regenerated for the newline so I missed that the actions were off after that change.

I think there's also some inconsistency on whether actions should be 0 indexed or 1 indexed. according to :h api-indexing most nvim functions are line 0 indexed with some notable exceptions, including nvim_win_get_cursor. I think we should standardize on api-indexing.

If that all sounds good, I'll fix the root issue and clean up the indexing to be consistent.

Also, the replay tool has a :ReplaySave command which is what I've used to update a single output. The idea is that you can save the output once you reviewed it to be correct.

@sudo-tee
Copy link
Owner Author

Sure If you want to fix the root cause I'm all for it.

It was confusing indeed, and the -1 indexing is also confusing

thanks for your help

Caused by the double newline we keep at the end.

Also, standardize on api-indexing (0 based line numbers) for actions.
Was still fighting with the automatic session fetching / rendering. I
think it makes sense to just disable that because we want to control the
renderer at a lower level.
@cameronr
Copy link
Collaborator

Was finally able to push my changes after GH resolved it's issue.

The main changes are here, with the underlying fix being in renderer.lua:

4099a8f (#124)

@cameronr
Copy link
Collaborator

I guess I spoke too soon, the tests failed because GH is still having issues. I'll rerun the tests

@sudo-tee
Copy link
Owner Author

Thanks for the fixes :)

@sudo-tee sudo-tee merged commit aa76c09 into main Nov 19, 2025
13 of 25 checks passed
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.

2 participants