feat(rendering): incremental rendering

[sudo-tee]

Introduces an incremental rendering that updates only the changed portion(s) of the buffer instead of re-rendering everything on every change. This reduces CPU usages for large sessions.

This PR also incorporate a functional test suite in tests/manual where you can record a run snapshot tests.

[cameronr]

Comment moved to #54 (comment)

[sudo-tee]

@cameronr

First of all great job I had the chance to play a little bit with your PR and it's coming along pretty nicely

I have a couple of commits on my branch named the same as yours for the PR but I cannot get them to show on this PR. There must be something I do wrong, or maybe I need access to the branch on your repo.

You should be able to rebase them without issues as they don't touch the streaming part.

I will be out for a couples of days so might not have to much time keeping an eye on the progress.

Keep up the good work on this. It's gonna be great.

Thanks again

[cameronr]

No worries, I'll pull those changes in.

Glad to hear it's looking good! I'm having fun so I'll keep chipping away at it.

ps: you might want to update the @ as my handle has an r at the end... or maybe cameron wants to join in :)

[cameron]

eMacs4lyfe

[sudo-tee]

@cameronr

I've added new commits to render the selected text and also to unify the part rendering.

so there is no need to have 2 seperate methods for formatting full session or streaming parts :)

You can see them here

cameronr/opencode.nvim@feat/incremental-rendering...sudo-tee:opencode.nvim:feat/incremental-rendering

[cameronr]

Oh nice! I had been waiting for the streaming rendering to be good enough to switch over but it seems like we're there.

Thanks for fixing the selection context issue! I didn't have any tests cases for that but I'll add a replay for that.

I also noticed the header separator change. No problem on that change but it does break the unit tests.

I'll add a script to regenerate all of the save replay data (only to be used when making this kind of format change) and I'll update the test data.

I'll also add a github workflow to run the tests on submit/pr.

[cameronr]

@sudo-tee I'm going to start cleaning up session_formatter (unify the code between full session loading and streaming events more, clean up the M.output hacks used by streaming currently). My quick thinking is:

1. session_formatter should be renamed formatter and it shouldn't store any state
2. streaming_renderer should be renamed renderer
3. output_renderer should be removed (at some point)
4. the full session loading code should move to renderer
5. renderer uses formatter to format the messages (as it does today, it's just helpful to me to have a clear idea of what each piece is doing)

[sudo-tee]

@sudo-tee I'm going to start cleaning up session_formatter (unify the code between full session loading and streaming events more, clean up the M.output hacks used by streaming currently). My quick thinking is:

1. session_formatter should be renamed formatter and it shouldn't store any state
2. streaming_renderer should be renamed renderer
3. output_renderer should be removed (at some point)
4. the full session loading code should move to renderer
5. renderer uses formatter to format the messages (as it does today, it's just helpful to me to have a clear idea of what each piece is doing)

This seems pretty reasonable to me 😁

[sudo-tee]

@cameronr

I added a managed event file.edited to refresh files when edited.

I also took the liberty to create a dedicated class to do message data lookups. A big chunk of the rendered was just looking up for data. I created a MessageMap with tests class that will do the actual indexing and searching of parts/messages callId.

I also fixed some issues with The Message type, since the migration to server api the types were not correct. I renames Message to OpencodeMessage, because my lsp was mixing it with an internal type of libuv.

I realized (before the refactor) that the contextual actions were not working properly.

Thanks :)

[cameronr]

Nice! That's a really nice improvement; cleans things up quite a bit. And love the tests!

I think context actions, undo/redo/snapshots, and prev/next message are the big remaining things that need to be hooked back up. output.lua is related and that's the code I'm looking at now so hoping to make some progress on how it should all fit together.

It's getting there!

[cameronr]

@sudo-tee I'm thinking about changing how we process messages / parts from a full session. Right now, there's two different code paths: the full session code path and the event code path. They leverage a lot of the same functionality but they're still subtly different. As one small example, the session reading code wasn't setting state.last_user_message anywhere.

It won't be quite as efficient (we'll end up looking up some indexes more often) but i think that the single code path simplicity is worth the extra lookups. Fortunately, MessageMap makes any perf penalty there prolly irrelevant :)

It would involve removing formatter._format_messages and replacing it with a loop in renderer._read_full_session_data:

  for i, msg in ipairs(session_data) do
    if state.active_session.revert and state.active_session.revert.messageID == msg.info.id then
      ---@type {messages: number, tool_calls: number, files: table<string, {additions: number, deletions: number}>}
      local revert_stats = M._calculate_revert_stats(state.messages, i, state.active_session.revert)
      local output = require('opencode.ui.output'):new()
      formatter._format_revert_message(output, revert_stats)
      M.write_output(output)

      -- FIXME: how does reverting work? why is it breaking out of the message reading loop?
      break
    end

    -- only pass in the info so, the parts will be processed as part of the loop
    -- TODO: remove part processing code in formatter
    M.on_message_updated({ info = msg.info })

    for j, part in ipairs(msg.parts or {}) do
      M.on_part_updated({ part = part })
    end

    -- FIXME: not sure how this error rendering code works when streaming
    -- if msg.info.error and msg.info.error ~= '' then
    --   vim.notify('calling _format_error')
    --   M._format_error(output, msg.info)
    -- end
  end

That way, we're always adding things exactly the same way. It's still possible for streaming to have bugs because of deltas / part replacement but I think the way to have the code be the most robust is to minimize the code paths.

I think it'll also help with cleaning up the types (and maybe even reduce the amount of index shuttling we're having to do) since there will only be one code path.

Does that sound ok?

[sudo-tee]

@sudo-tee I'm thinking about changing how we process messages / parts from a full session. Right now, there's two different code paths: the full session code path and the event code path. The leverage a lot of the same functionality but they're still subtly different. It won't be quite as efficient (we'll end up looking up some indexes more often) but i think that the single code path simplicity is worth the (likely tiny perf penalty).

It would involve removing formatter._format_messages and replacing it with a loop in renderer._read_full_session_data:

  for i, msg in ipairs(session_data) do
    if state.active_session.revert and state.active_session.revert.messageID == msg.info.id then
      ---@type {messages: number, tool_calls: number, files: table<string, {additions: number, deletions: number}>}
      local revert_stats = M._calculate_revert_stats(state.messages, i, state.active_session.revert)
      local output = require('opencode.ui.output'):new()
      formatter._format_revert_message(output, revert_stats)
      M.write_output(output)

      -- FIXME: how does reverting work? why is it breaking out of the message reading loop?
      break
    end

    -- only pass in the info so, the parts will be processed as part of the loop
    -- TODO: remove part processing code in formatter
    M.on_message_updated({ info = msg.info })

    for j, part in ipairs(msg.parts or {}) do
      M.on_part_updated({ part = part })
    end

    -- FIXME: not sure how this error rendering code works when streaming
    -- if msg.info.error and msg.info.error ~= '' then
    --   vim.notify('calling _format_error')
    --   M._format_error(output, msg.info)
    -- end
  end

That way, we're always adding things exactly the same way. It's still possible for streaming to have bugs because of deltas / part replacement but I think the way to have the code be the most robust is to minimize the code paths.

Does that sound ok?

Yes that sounds reasonable to me. It's always good to have a common code path

I could explain a little more how revert works. But I only have access to my phone for the next couple of days.

But essentially opencode will store the message Id of where the revert happens. And the rest of the messages don't technically exists. So the stats are calculated from that revert point. If you call /revert another time it will move up to the previous messageId. and so one.

At the moment this is buggy in opencode as once the server is closed.. or the opencode tui is closed you cannot revert anymore it gives an error and Delete the files entirely 😵. There are a couple of open bugs on the open code repo about it.

Again I am very grateful for your help on complex topics. Keep up the good work.

[cameronr]

Ok, great, I'll keep moving in that direction.

And thanks for the explanation of revert. I'll see if I can figure out how to handle that for session load and when streaming events.

[cameronr]

I did a fairly big refactor and introduced a new RenderState object that subsumed _part_cache, _message_map, and _actions to better handle finding parts/messages and tracking their rendered position. I think I'm happy with how that code is coming along :)

Separately, I haven't had a chance to look at snapshots/undo/redo yet but I've noticed a few times where canceling a request didn't result in job_count == 0. I have some ideas on that so I'm going to take a look at that next.

[sudo-tee]

I did a fairly big refactor and introduced a new RenderState object that subsumed _part_cache, _message_map, and _actions to better handle finding parts/messages and tracking their rendered position. I think I'm happy with how that code is coming along :)

Separately, I haven't had a chance to look at snapshots/undo/redo yet but I've noticed a few times where canceling a request didn't result in job_count == 0. I have some ideas on that so I'm going to take a look at that next.

Awesome. I think the RenderState reflect the idea I had in my head with The Output class and the MessageMap. I think you found the pattern I was searching for. Great idea. For the undo redo I will be back home this weekend and will have a look if you want.

There is also a couple of bugs that are piling up in the repo that I need to address.

Thanks again

[cameronr]

I'll actually be away for much of this weekend so it would be amazing if you were able to look into undo / redo / snapshots!

I'm hoping the big changes are done and now it's mostly about hooking things back up and doing a bunch of testing to build confidence that it's all working.

[sudo-tee]

@cameronr

I'm almost there for the revert part. I only need to make it work with the test replay.

[cameronr]

that's great! let me know if you have any trouble with getting the replay testing working (or with the rendering).

one note is that each replay tests both incremental rendering (streamed events) and full session loads (to make sure they render the same as incremental). the full session loads are reconstructed from the events, tho, and that reconstruction doesn't handle some things (like permissions) so some tests need to disable full session testing.

[sudo-tee]

@cameronr

I pushed my fix for undo/redo.

I fixed redo to match the opencode tui behavior

I also added some tests for undo redo

I will have a look at snapshots next.

[sudo-tee]

@cameronr

Snapshots are a custom thing so there is no event for it... With the proper undo redo behavior. I am wondering if simply calling undo on the message instead of having 2 different workflow with slightly different meaning..

Although the snapshot handling is more granular than the undo redo..

[cameronr]

Nice! I think we're getting pretty close!

There's a FIXME in output_window about restore_points. Can you take a look at that whenever you next have a chance?

[cameronr]

I have a todo item for mentions but I'm not sure how they work so could use some help.

In the old code, output_renderer called highlight_all_mentions after rendering:

opencode.nvim/lua/opencode/ui/output_renderer.lua

Lines 155 to 160 in fed6941

	pcall(function()
	render()
	require('opencode.ui.mention').highlight_all_mentions(windows.output_buf)
	require('opencode.ui.contextual_actions').setup_contextual_actions()
	require('opencode.ui.footer').render(windows)
	end)

My reading of the highlight_all_mentions code says it would apply the OpencodeMention extmark to anything in that output that was like @word (where word can include _-./. That doesn't seem right as many things might match that (lua comments like @param, as just one example).

One option might be to only apply the highlight to the text property of a part but I don't have any examples of what I think highlights is attempting to match. Do you have any examples (or have a good way to generate them)?

thanks!

[sudo-tee]

Nice! I think we're getting pretty close!

There's a FIXME in output_window about restore_points. Can you take a look at that whenever you next have a chance?

@cameronr

I was able to plug restore_points back. But unfortunately I cold not get it to work properly in the replay renderer as it reads the filesystem for data.

This is where I question if I should keep the feature or not. It's useful but it differs From the Opencode way of doing it.

[sudo-tee]

I have a todo item for mentions but I'm not sure how they work so could use some help.

In the old code, output_renderer called highlight_all_mentions after rendering:

opencode.nvim/lua/opencode/ui/output_renderer.lua

Lines 155 to 160 in fed6941

	pcall(function()
	render()
	require('opencode.ui.mention').highlight_all_mentions(windows.output_buf)
	require('opencode.ui.contextual_actions').setup_contextual_actions()
	require('opencode.ui.footer').render(windows)
	end)

My reading of the highlight_all_mentions code says it would apply the OpencodeMention extmark to anything in that output that was like @word (where word can include _-./. That doesn't seem right as many things might match that (lua comments like @param, as just one example).

One option might be to only apply the highlight to the text property of a part but I don't have any examples of what I think highlights is attempting to match. Do you have any examples (or have a good way to generate them)?

thanks!

@cameronr

The highlight_all_mentions is indeed finding @{subagent} or the @path/to/file.lua and add highlight to them.

When you type @ in the input_window and select either a file or a subagent , it will call highlight_all_mentions also on the input_window to highlight them.

You are right that in the output_window it might be error prone.

However if you look at the message you at the messages generated you will see them with a range

{
      "messageID": "msg_a06c4164a0015W5xZmeTCOAuhE",
      "source": {
        "text": {
          "value": "@test.txt",
          "end": 8,
          "start": 0
        },
        "path": "/home/francis/Projects/_nvim/opencode.nvim/test.txt",
        "type": "file"
      },
      "mime": "text/plain",
      "type": "file",
      "id": "prt_a06c416520063e9hNFYlVzbCvm",
      "url": "file:///home/francis/Projects/_nvim/opencode.nvim/test.txt",
      "filename": "test.txt",
      "sessionID": "ses_5f9558f7affeMXh93WS3cQpzjB"
    },

    {
      "messageID": "msg_a06c60f89001za14H2cqggp6uq",
      "type": "agent",
      "id": "prt_a06c60f89003AyAUE4WJWAXoHK",
      "source": {
        "value": "@foo",
        "end": 4,
        "start": 0
      },
      "name": "foo",
      "sessionID": "ses_5f9558f7affeMXh93WS3cQpzjB"
    },

The source part indicates the range in the text (this is how they do in internally in opencode)

Maybe we can use theses to generate the proper highlight ? or maybe use the value part for the match something.

UPDATE:

It's more complicated than it looks as the user prompt is rendered before the other message parts.

[cameronr]

I have a little bit of debug logging to remove. After that, we should just give a few days of bake time to make sure I didn't break anything / no other issues pop up. Will be great to get it out!

I'm soooo glad we had the unit tests and replay system. It wouldn't have been possible to improve things so quickly without them!

[sudo-tee]

On a different note.

I am thinking of changing the plugin name and maybe soon having release numbers. GitHub supports renaming project so it should be fine

There is already another plugin named opencode.nvim.

I have a couple of names and wanted your input.

• openkode.nvim
• nvim-opencode
• neocoder.nvim
• opencoder.nvim
• opencode-ui.nvim
• codepal.nvim
• libre-code.nvim

I'm open for ideas ⁉️

[cameronr]

Oh funny, I didn't even know about the other one. Native neovim UI and keymaps/navigation is such an improvement over a terminal integration, at least in my opinion... but I'm biased :)

On names, here are some that stand out to me:

• opencode.nvim (leave it as is and let the community sort it out)
• opencode-native.nvim (highlight the native aspect of it)
• neopencode.nvim (it's a little cumbersome but i think maintaining the connection to opencode is important)
• neo-opencode (another take on the idea above).

I do think keeping a direct connection to opencode is important from a "branding" perspective.

[cameronr]

@sudo-tee It's purely cosmetic, but what do you think about switching the default thinking animation to:

The one above is smoother/less jumpy/distracting than the current default. If not, no worries.

[sudo-tee]

@sudo-tee It's purely cosmetic, but what do you think about switching the default thinking animation to:

The one above is smoother/less jumpy/distracting than the current default. If not, no worries.

Yes I'm ok with it. The current one can be distracting at time.

[cameronr]

I've just been cleaning up some diagnostics. I did change the emmyrc to not include $HOME/.local/share/nvim/lazy as that can introduce some type conflicts if plugins define the same types (that was happening to me). I think it's probably the better choice but let me know if you think otherwise.

[cameronr]

I did a bunch of testing and things look pretty good. I did find a few issues, summarized:

• clear all extmarks in output_window (needed because RenderMarkdown sometimes leaves extmarks behind) 6f424e0
• @ mentions were resetting the cursor position if you'd previously left the input window 4f1d172
• snacks file picker wasn't restoring the window, mode, cursor position c1b9568
• report promise errors. this will surface problems like opencode config errors e5cb4a1
• hopefully a permanent fix to spinner spinning at wrong times 097874c
• more resilient aborting, including handling permissions 4b7187d

[sudo-tee]

This is really nice. A bunch of little fixes.

We are almost ready to merge.

Maybe one last thing. I usually run with debug mode on. Can we have a setting for the events notifications under debug ? So I can run without them ?

After this I will probably merge as it looks stable to me also

EDIT:
My bad I was checking the wrong version

So if you feel it's safe to merge I will proceed.

[sudo-tee]

@cameronr

I just merged your amazing work. 🎉

Github didn't let me do a simple merge, only a squash would work. I did add you as a co-author because I din't want you to loose credit for all your work :).

Thank you again for your work 🔥

[cameronr]

Oh great! I was just about to comment and say it looks good. Glad it's finally out!