feat(tui): expand restore snapshot listing#2513
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the /restore command by increasing the default listing limit from 10 to 20 snapshots, introducing a /restore list [N] subcommand to list up to 100 snapshots, and displaying UTC timestamps for each snapshot. It also updates the documentation and adds corresponding unit tests. The reviewer suggested capping the query limit to 1000 when restoring by a numeric index to prevent potential failures with extremely large inputs.
| )); | ||
| } | ||
| }; | ||
| let snapshots = match repo.list(n.max(DEFAULT_LIST_LIMIT)) { |
There was a problem hiding this comment.
If n is extremely large (e.g., close to usize::MAX), passing it directly to repo.list can cause git log to fail or overflow its internal parser for --max-count. Since the snapshot repository is capped and typically has far fewer than 1000 entries, we can safely cap the query limit at 1000 using n.min(1000). If n is larger than 1000, the check n > snapshots.len() will still correctly fail and report the actual number of available snapshots.
| let snapshots = match repo.list(n.max(DEFAULT_LIST_LIMIT)) { | |
| let snapshots = match repo.list(n.min(1000).max(DEFAULT_LIST_LIMIT)) { |
| let mut out = String::from( | ||
| "Recent snapshots (newest first; pass /restore <N> to revert; /restore list 50 shows more):\n", | ||
| ); |
There was a problem hiding this comment.
The listing header is hardcoded to suggest
/restore list 50 shows more regardless of how the listing was invoked. When a user runs /restore list 100 (the maximum), the hint implies more entries exist beyond the 100 they already fetched, which is misleading. Consider omitting the hint — or making it conditional on whether the returned count equals the requested limit.
| let mut out = String::from( | |
| "Recent snapshots (newest first; pass /restore <N> to revert; /restore list 50 shows more):\n", | |
| ); | |
| let mut out = String::from( | |
| "Recent snapshots (newest first; pass /restore <N> to revert; /restore list [N] shows more, up to 100):\n", | |
| ); |
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 Some(value) = parts.next() else { | ||
| return Ok(Some(DEFAULT_LIST_LIMIT)); | ||
| }; |
There was a problem hiding this comment.
/restore list with no N falls through to Ok(Some(DEFAULT_LIST_LIMIT)), so it returns the same 20 entries as a plain /restore. A user who types /restore list expecting to see more snapshots will see identical output and no indication of why. Defaulting to a higher count (e.g. MAX_LIST_LIMIT) when list is given without a number would match the subcommand's stated purpose of revealing more rollback points.
| let Some(value) = parts.next() else { | |
| return Ok(Some(DEFAULT_LIST_LIMIT)); | |
| }; | |
| let Some(value) = parts.next() else { | |
| return Ok(Some(MAX_LIST_LIMIT)); | |
| }; |
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!
|
Thanks @cyq1017 — the restore snapshot listing slice was harvested into the public v0.9.0 integration branch ( The landed version adds Verification recorded for the harvest: Closing this PR as harvested into the integration branch. Issue #2494 remains open because this landed only the restore-listing slice, not the full rollback UX report. |
Refs #2494
Problem
The
/restorelist can be too shallow when a user has many recent checkpoints, and the entries do not show when a checkpoint was created.Change
/restoreshow the 20 most recent snapshots by default/restore list [N], capped at 100, to inspect more rollback points/restore <N>to target entries beyond the default list sizeVerification
cargo test -p codewhale-tui restore_ --all-features --locked -- --nocapturecargo fmt --all -- --checkcargo check -p codewhale-tui --all-features --lockedcargo clippy -p codewhale-tui --all-features --locked -- -D warningsgit diff --check origin/main..HEADGreptile Summary
This PR expands the
/restorecommand by bumping the default listing from 10 to 20 snapshots, adding a/restore list [N]subcommand (capped at 100), and including UTC timestamps in each listing row.DEFAULT_LIST_LIMITraised to 20;parse_list_argrouteslist [N]through a separate fetch path capped atMAX_LIST_LIMIT = 100, while plain numeric args are guarded by a newMAX_RESTORE_INDEX = 1000ceiling.format_snapshot_timeconverts the stored Unix timestamp viachrono::Utcand formats it asYYYY-MM-DD HH:MM UTC, with\"unknown time\"as a safe fallback for invalid values.Confidence Score: 5/5
Safe to merge — all changed paths are additive and well-tested; the only rough edge is a silent truncation of oversized list requests.
The restore command logic is straightforward and all new branches are exercised by the six added tests. Snapshot fetching, timestamp formatting, and the trust-mode gate are unchanged. The silent capping of
/restore list Nwhen N > 100 is a UX nit rather than a data-correctness problem.crates/tui/src/commands/restore.rs — specifically the silent cap in parse_list_arg
Important Files Changed
/restore [N]to `/restore [N/restore list [N]alongside the existing restore description. Accurate and concise.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["/restore [arg]"] --> B{arg present?} B -- No --> C["repo.list(DEFAULT_LIST_LIMIT=20)"] C --> D{empty?} D -- Yes --> E["'No snapshots yet'"] D -- No --> F["format_listing()"] B -- Yes --> G["parse_list_arg(arg)"] G -- "Err" --> H["CommandResult::error"] G -- "Ok(Some(limit))" --> I["repo.list(limit ≤ 100)"] I --> D2{empty?} D2 -- Yes --> E D2 -- No --> F G -- "Ok(None)" --> J["parse arg as usize N"] J -- "0 or parse fail" --> K["Usage error"] J -- "N > MAX_RESTORE_INDEX(1000)" --> L["'index must be ≤ 1000' error"] J -- "1 ≤ N ≤ 1000" --> M["repo.list(max(N, 20))"] M --> N{empty?} N -- Yes --> E N -- No --> O{N > snapshots.len?} O -- Yes --> P["'Only X available' error"] O -- No --> Q{yolo or trust_mode?} Q -- No --> R["'Refusing to restore' message"] Q -- Yes --> S["repo.restore(target.id)"] S -- Err --> T["'Restore failed' error"] S -- Ok --> U["'Restored snapshot #N' message"]Reviews (2): Last reviewed commit: "fix(tui): bound restore snapshot lookup" | Re-trigger Greptile