Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The update-driven close path mishandles jj views and lacks regression coverage.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
This PR moves auto_close_on_empty into the Diffview file-update path so it can respond when a comparison becomes empty outside a staging action.
Changes:
- Check the full file list before closing, so staged entries keep the view open.
- Report “Nothing to display.” for an initially empty comparison.
- Update the option documentation and a test stub.
| File | Description |
|---|---|
| lua/diffview/tests/functional/diff_view_spec.lua | Adds a staged bucket to an existing test stub. |
| lua/diffview/scene/views/diff/listeners.lua | Runs the close policy after file updates and waits for file loading to finish. |
| lua/diffview/config.lua | Revises the option description and default comment. |
| doc/diffview.txt | Explains the revised close behavior. |
| doc/diffview_defaults.txt | Updates the default-config comment. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The policy only ran after staging actions taken in the file panel, so a view stayed open after a commit, and opening one against an empty comparison left an empty panel behind. Run the policy on every file update so it applies however the view was emptied, and gate it on the whole file list rather than the `working` bucket alone, since staged entries are still shown. A view that would hold no files reports `Nothing to display.` instead of opening. Index-less adapters (jj) keep a resolved file in `working` as the resolution artifact, so emptiness cannot be the gate there. Pick the fallback by trigger rather than by adapter: only the conflict resolution action consults the `had_conflicts` latch, which stays set for the life of the view and would otherwise close the view on an unrelated refresh.
fc42615 to
dfbb696
Compare
|
Resolved the flagged conflicts |
There was a problem hiding this comment.
🟡 Changes recommended
Initially empty comparisons still execute opening hooks, and the documentation overstates protection for unsaved stage edits.
2 open findings
2 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| Automatically close a Diffview that holds no files, and report that | ||
| there is nothing to display instead of opening one that would. | ||
|
|
||
| A view with unsaved stage buffers is kept open until they are written. |
| if not was_initialized and view.files:len() == 0 then | ||
| utils.info("Nothing to display.") |
|
It seems that the proposed changes includes both a bug fix and a change in behaviour, and these should be separated. The bug fix (catching external events like However, showing "Nothing to display" on opening against an empty comparison is a new behavioural change. The promise is to close on empty, not to decline on open when empty, and the change breaks the case where a user has deliberate run The bug fix should go in, but the behaviour change needs a design review. |
The policy only ran after staging actions taken in the file panel, so a view stayed open after a commit, and opening one against an empty comparison left an empty panel behind.
Run the policy on every file update so it applies however the view was emptied, and gate it on the whole file list rather than the
workingbucket alone, since staged entries are still shown.A view that would hold no files reports
Nothing to display.instead of opening.