Repository navigation
feat(gpui): History menu with labelled undo steps - #2844
YoavLevavi wants to merge 4 commits into
Conversation
Each history entry gets a short label ("Added zoom", "Changed background")
from a diff against the previous snapshot. The header's History button lists
them newest first; clicking one walks the undo stack straight there.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@YoavLevavi is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
|
Thanks @YoavLevavi, a jumpable history list is a nice touch, and the bookkeeping is solid: |
- Labels are worked out when the History menu opens, from consecutive snapshots, so record() no longer serializes two configs per entry (the screenshot editor never pays for them). - describe_change looks at clips and transitions first, then prefers a track whose segment count changed over one that only shifted, so a clip delete that ripples music or drops a caption reads "Deleted clip". - open_history_menu and render_history_menu sit above the presets menu's doc comment again. Merges origin/main (serializes_equal resume check). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @richiemcilroy, both are in 427befa (also merged main), plus the optional one:
@greptileai review |
| self.history_menu = Some(HistoryMenu { | ||
| origin: point(px(x), px(f32::from(anchor.y) + 8.)), | ||
| scroll: gpui::ScrollHandle::new(), | ||
| labels: self.history.labels(), |
There was a problem hiding this comment.
History rows target wrong steps
The menu keeps its original labels and row indices even when keyboard edits change the history. For example, open History, undo, then press C inside a clip. The split replaces the redo branch, but the menu still shows that branch's old labels. Clicking one can restore a different state or do nothing. Close or refresh the menu whenever the history changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/editor_window.rs
Line: 9151
Comment:
**History rows target wrong steps**
The menu keeps its original labels and row indices even when keyboard edits change the history. For example, open History, undo, then press `C` inside a clip. The split replaces the redo branch, but the menu still shows that branch's old labels. Clicking one can restore a different state or do nothing. Close or refresh the menu whenever the history changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| let timeline = |map: &serde_json::Map<String, serde_json::Value>| match map.get("timeline") { | ||
| Some(serde_json::Value::Object(timeline)) => timeline.clone(), | ||
| _ => empty.clone(), | ||
| }; | ||
| let (before_timeline, after_timeline) = (timeline(&before), timeline(&after)); |
There was a problem hiding this comment.
History copies timelines unnecessarily
describe_change deep-copies both timeline maps even though it only reads them. Opening History repeats this for up to 99 snapshot pairs, after already turning both projects into JSON trees. Caption-heavy projects therefore copy all timeline captions and words again just to open the menu. Borrow the maps instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/editor_edits.rs
Line: 295-299
Comment:
**History copies timelines unnecessarily**
`describe_change` deep-copies both timeline maps even though it only reads them. Opening History repeats this for up to 99 snapshot pairs, after already turning both projects into JSON trees. Caption-heavy projects therefore copy all timeline captions and words again just to open the menu. Borrow the maps instead.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| } | ||
| } | ||
|
|
||
| /// The History dropdown's width. |
There was a problem hiding this comment.
Added comments repeat the code
This comment only restates HISTORY_MENU_WIDTH. AGENTS.md explicitly bans comments that narrate code and requires non-obvious context instead. Remove this narration, along with the similar comments on history_menu and jump_to_history. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/editor_window.rs
Line: 206
Comment:
**Added comments repeat the code**
This comment only restates `HISTORY_MENU_WIDTH`. `AGENTS.md` explicitly bans comments that narrate code and requires non-obvious context instead. Remove this narration, along with the similar comments on `history_menu` and `jump_to_history`. This repository requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let x = f32::from(anchor.x) | ||
| .min(f32::from(viewport.width) - HISTORY_MENU_WIDTH - 12.) | ||
| .max(12.); |
There was a problem hiding this comment.
Bounds use the prohibited form
The new position calculation uses .min(...).max(12.), which AGENTS.md explicitly forbids in favor of clamp. Use clamp and ensure its upper bound is at least 12. so very narrow windows remain safe. This repository requirement must be satisfied before merging.
| let x = f32::from(anchor.x) | |
| .min(f32::from(viewport.width) - HISTORY_MENU_WIDTH - 12.) | |
| .max(12.); | |
| let x = f32::from(anchor.x).clamp( | |
| 12., | |
| (f32::from(viewport.width) - HISTORY_MENU_WIDTH - 12.).max(12.), | |
| ); |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/editor_window.rs
Line: 9145-9147
Comment:
**Bounds use the prohibited form**
The new position calculation uses `.min(...).max(12.)`, which `AGENTS.md` explicitly forbids in favor of `clamp`. Use `clamp` and ensure its upper bound is at least `12.` so very narrow windows remain safe. This repository requirement must be satisfied before merging.
```suggestion
let x = f32::from(anchor.x).clamp(
12.,
(f32::from(viewport.width) - HISTORY_MENU_WIDTH - 12.).max(12.),
);
```
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Done in dc784ac: removed the narrating comments on @greptileai review |
|
Thanks! Can you get Greptile to 5/5? Then we'll take a look. |
The open menu kept its labels and row indices after a keyboard edit replaced the redo branch, so a row could restore a different state. The history now carries a revision bumped whenever its entries change; a stale menu closes on the next render and its rows refuse to jump. describe_change borrows the timeline maps instead of copying them, and the menu position uses clamp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@greptile-apps review |
|
Thanks! Can you rebase on main to clear the conflicts and resolve the open review threads? Then we'll take a look. |
A History button next to Undo/Redo in the editor header lists every undo step, newest first, and jumps straight to one when clicked.
ProjectHistorykeeps a label next to each snapshot, made by diffing it with the previous one ("Added zoom", "Deleted text", "Split clip", "Changed background"...). Labels follow the same truncation and 100-entry limit as the snapshots.ProjectHistory::jump_tomoves the cursor directly. Steps after the cursor stay redoable and are dimmed in the menu.icons/history.svg(Lucide).Testing:
cargo test --bin cap-gpui(newhistory_labels_steps_and_jumps_between_them).🤖 Generated with Claude Code


Confidence Score: 5/5
The PR appears safe to merge; all previous findings are addressed.Summary
Adds a History menu with named undo steps and direct jumps between saved states.
All four previous findings are addressed. No new actionable issues were found. Tests and visual checks were not run.
Reviews (3) · Last reviewed commit: "History menu: close it when the history ..." · Reviewed by Greptile