Skip to content

Conversation

bassamsdata
Copy link
Contributor

@bassamsdata bassamsdata commented Aug 26, 2025

Description

Add floating diff layout with focus effects and workflow improvements

Floating Diff Layout

  • Added layout = "float" option to config.display.diff.inline.layout
  • Displays diffs in floating windows instead of regular splits (with child_window config)
  • Includes dimming background overlay (30% winblend) for focus effect

Automatic Chat Management

  • Chat windows are automatically hidden when floating diffs appear
  • Chat is restored to previous state after diff operations complete
  • Cursor automatically returns to chat buffer when diff workflow ends

Configuration

config.display.diff.inline.layout = "float" -- or "non_float"

though this is not the default, would you like to make it the default option?

Thank you

Related Issue(s)

#1991 (comment)

Screenshots

Screen.Recording.2025-08-26.at.2.01.12.AM.mp4

Checklist

  • I've read the contributing guidelines and have adhered to them in this PR
  • I've updated CodeCompanion.has in the init.lua file for my new feature
  • I've added test coverage for this fix/feature
  • I've updated the README and/or relevant docs pages
  • I've run make all to ensure docs are generated, tests pass and my formatting is applied

@Copilot Copilot AI review requested due to automatic review settings August 26, 2025 06:34
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a floating diff layout feature that displays diffs in floating windows with visual enhancements and improved workflow management.

Key changes:

  • Added floating window diff display with background dimming overlay
  • Implemented automatic chat window management during floating diff operations
  • Added LSP formatting integration before diff creation to reduce formatting noise

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
lua/codecompanion/config.lua Added configuration options for floating diff layout and background dimming
lua/codecompanion/utils/ui.lua Added background window management functions for focus effects
lua/codecompanion/strategies/chat/tools/catalog/helpers/diff.lua Added floating window creation and chat hiding logic
lua/codecompanion/providers/diff/inline.lua Enhanced InlineDiff class with floating window support and cleanup
lua/codecompanion/providers/diff/utils.lua Removed unified diff display function (no longer needed)
lua/codecompanion/strategies/chat/tools/catalog/insert_edit_into_file.lua Added automatic LSP formatting and chat hiding integration
lua/codecompanion/init.lua Enhanced chat restoration to focus on chat window
tests/providers/diff/test_utils.lua Updated tests to use hunk highlights instead of unified diff display
tests/providers/diff/test_inline.lua Added screenshot tests for floating diff functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@olimorris
Copy link
Owner

Thanks @bassamsdata this looks to be another epic PR. I've left some comments but pretty minor all in all.

I'm not going to be merging anyone else's PRs until the back end of this week. I'm planning on merging the "other" repository into main, some time tomorrow.

I expect that may cause some merge-conflicts with this PR, albeit nothing too crazy.

@olimorris olimorris added the P3 Low impact, low urgency label Aug 26, 2025
@bassamsdata
Copy link
Contributor Author

No issues at all, I think the merge conflicts should be minimal. Once you merge that repo, I’ll go ahead and rebase this.

@olimorris
Copy link
Owner

Hopefully not too painful.

In "request_permission.lua" you can see how I attempted to show a diff in a floating window. One interesting idea I came across was to put the keymaps in a winbar, localised to that window. It ensures they're always present no matter how the user scrolls or the size of their window.

If you like that, we should probably extract that to the utils/ui.lua file.

Oh and I added a display.chat.diff_window option table to the config. I realised that I may want to format the diff window slightly differently to the child window with line numbers etc.

If something doesn't make sense, tag me and I can take a look. Appreciate ACP support has moved a bunch of stuff around.

@bassamsdata
Copy link
Contributor Author

One interesting idea I came across was to put the keymaps in a winbar

Oh, I saw this yesterday and instantly fell in love with it, it’s definitely easier to implement! This should definitely be the default.

I also liked the diff_window because, honestly, it should have its own config.

I’ll try to extract some parts related to floating diffs into a utils/ui module since both the internal tool and the ACP use the same floating diffs.

I’ll start working on this later this evening.

@olimorris
Copy link
Owner

Amazing. Again, another one of your features that have me very excited. Your video at the top....it doesn't even look like Neovim which makes it super impressive.

@bassamsdata bassamsdata force-pushed the diff_enhancments branch 2 times, most recently from 86aeca8 to 11b76d5 Compare September 1, 2025 17:24
@bassamsdata
Copy link
Contributor Author

bassamsdata commented Sep 3, 2025

Hi @olimorris, I think this PR is ready for review. Now both the HTTP and ACP floating diffs are identical, so users get the same CodeCompanion UX: inline diffs, dimming effect for the background, a title with the file name and relative path, a clear winbar display, and no virtual hints.

I made some changes to create_float so it is more flexible and works seamlessly with all modules and added some screenshot tests for it.

Below is a video showing the same code, first with HTTP Copilot and then with ACP Gemini-cli.

If you need any removals or modifications, please let me know and I’ll make the changes.
Edit: Ah, I forgot about the docs, should I write them?

Screen.Recording.2025-09-02.at.11.42.58.PM.mp4

@olimorris
Copy link
Owner

olimorris commented Sep 10, 2025

Hey @bassamsdata. So sorry I missed this! I'll try and take a look over the coming days. And no worries on the docs, I can handle those.

@bassamsdata
Copy link
Contributor Author

Hi @olimorris, No worries at all, take your time. I'll address the PR reviews as soon as I can.

---Resolve a config value that might be a function or static value
---@param value any
---@return any
function M.resolve_value(value)
Copy link
Owner

Choose a reason for hiding this comment

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

Ooooh this is a very clever idea! Should have thought of this long ago 👏🏼

@olimorris
Copy link
Owner

Loving the floating diff. Looks really, really nice. My preference is to set float as the default because we're asking the user to make a decision and I think it should interrupt what they're doing (if they're not monitoring the chat buffer).

From my side this is amazing and happy to merge if you have nothing else to add.

@olimorris
Copy link
Owner

@bassamsdata quick question regarding request_permission.lua and the merge conflict.

You currently have:

local function get_diff(tool_call)
  local absolute_path = tool_call.locations and tool_call.locations[1] and tool_call.locations[1].path
  local path = absolute_path or vim.fs.joinpath(vim.fn.getcwd(), tool_call.content[1].path)

  return {
    kind = tool_call.kind,
    new = tool_call.content[1].newText,
    old = tool_call.content[1].oldText,
    path = path,
    status = tool_call.status,
    title = tool_call.title,
    tool_call_id = tool_call.toolCallId,
  }
end

But I'm proposing:

local function get_diff(tool_call)
  return {
    kind = tool_call.kind,
    new = tool_call.content[1].newText,
    old = tool_call.content[1].oldText,
    path = tool_call.content[1].path,
    status = tool_call.status,
    title = tool_call.title,
    tool_call_id = tool_call.toolCallId,
  }
end

What was the reason for your path modifications? I believe the agents only ever work with absolute paths.

@bassamsdata
Copy link
Contributor Author

bassamsdata commented Sep 14, 2025

@olimorris, as I recall, gemini_cli was returning only the file name as the path, so the title showed just file_name. I changed it to use the relative path instead.

Edit: the current main branch shows only the file name when using gemini_cli:
Screenshot 2025-09-14 at 6 09 05 PM

@olimorris
Copy link
Owner

Ooooh you're right! Great spot. I hadn't realised that the diff path is relative (wtf Gemini?!). I've resolved that conflict now.

I'll have another final check tomorrow and then you cool with me merging?

@bassamsdata
Copy link
Contributor Author

I've noticed that gemini_cli doesn't fully follow the ACP in several places.

I'm fine with the current state of this PR as long as you're comfortable testing it. We should probably change the default to "float" as you suggested earlier.

@olimorris
Copy link
Owner

Yep will do both.

I hadn't appreciated how different both Gemini CLI and Claude Code would be, even though they follow the same protocol.

@olimorris
Copy link
Owner

I feel like the diff config is a little all over the place (my fault) so moving provider specific config options into provider_opts section

@olimorris
Copy link
Owner

Just need to update the docs now

@olimorris
Copy link
Owner

Think we're good, except I can't seem to get numbers to show up in the diff. Is it working for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Low impact, low urgency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants