fix(forest-cli): sync wait -w no longer leaves a stale Status: line - #7453
fix(forest-cli): sync wait -w no longer leaves a stale Status: line#7453EclesioMeloJunior wants to merge 8 commits into
sync wait -w no longer leaves a stale Status: line#7453Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe sync command now buffers terminal output, counts wrapped rows, and clears prior output by row count. Watch, one-time status, and initialization paths use explicit terminal writes and flushes. A changelog entry records the fix. ChangesSync terminal rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli/subcommands/sync_cmd.rs (1)
63-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context to direct output failures.
The new clear, completion write, and flush operations propagate bare I/O errors. Add operation-specific
.context(...)calls so a terminal write failure identifies the failed sync-status action.As per coding guidelines, use
anyhow::Result<T>for most operations and add context with.context()when errors occur.Also applies to: 69-74, 99-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/subcommands/sync_cmd.rs` at line 63, Add operation-specific anyhow context to the direct terminal I/O operations in the sync command: clear_previous_lines, the completion write, and stdout.flush. Update the surrounding operations at the referenced locations to call .context(...) before propagating errors, with messages identifying the failed sync-status action.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/subcommands/sync_cmd.rs`:
- Around line 63-67: Update the sync refresh flow around clear_previous_lines
and print_sync_report_details to recalculate the previous report’s row count
using the current terminal width before clearing it, rather than reusing
rows_printed_last_iteration from the old width. Preserve correct cursor movement
across terminal resizes and add a regression test covering a width change
between refreshes.
---
Nitpick comments:
In `@src/cli/subcommands/sync_cmd.rs`:
- Line 63: Add operation-specific anyhow context to the direct terminal I/O
operations in the sync command: clear_previous_lines, the completion write, and
stdout.flush. Update the surrounding operations at the referenced locations to
call .context(...) before propagating errors, with messages identifying the
failed sync-status action.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27b34606-5d35-4e44-826f-7b8437199b0f
📒 Files selected for processing (2)
CHANGELOG.mdsrc/cli/subcommands/sync_cmd.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| /// [`clear_previous_lines`] moves the cursor by rows. Counting `writeln!` calls | ||
| /// instead leaves the topmost row of each frame behind on narrow terminals. | ||
| /// See <https://github.com/ChainSafe/forest/issues/7366>. | ||
| fn print_sync_report_details( |
There was a problem hiding this comment.
High level question, do we need to do this line arithmetic dance ourselves? Do we have some crates in the tree that'd already do it for us?
There was a problem hiding this comment.
the one we have on the tree is dialoguer however it does not have methods that perform the line calcs and move the cursor/clean up to a new block print.
There is one called ratatui that is a whole TUI crate but IMO it will be too much to only display the status report, having that calc by done by ourselves seems ok (get the printed string and divide by the terminal width to get the amount of terminal rows used)
One thing that we could improve is the clean part, we were using a very verbose cursor move approach:
write!(
out,
"\r{}{}",
anes::MoveCursorUp(rows as u16),
anes::ClearBuffer::Below,
)?;that can be written just as:
term.clear_last_lines(rows)?;
There was a problem hiding this comment.
🐰 comment might be valid, I did some resizing while the sync wait -w was running and ended up with
❯ forest-cli sync wait -w
Status: Syncing (3861 epochs behind)
Node Head: Epoch 3954812 ([bafy2bzacebmet6slu6aoznsmvqdxqnxbbvt32vmiykg
Status: Syncing (3846 epochs behind)
Node Head: Epoch 3954827 ([bafy2bzaceasstg4lg6rfa7s3iwjrq7gbg57wmmomv4i5xpvb3dm22qqv63vb4, ...])
Network Head: Epoch 3958673
Last Update: 2026-08-07T07:09:48.671036974+00:00
Active Sync Tasks:
- Fork Target: 3958673 ([bafy2bzacedzursoxvdbugyvcsdw42yxsrhvdfdcl7j4emd46pkmgkl3un3te4, ...]), Stage: Validating Tipsets, Syncing Range: [3954828..3958673] (3845 epochs)
This gets properly cleared when I resize back to the original width.
|
@LesnyRumcajs tested and confirmed that it was buggy when resizing, applied what 🐰 suggested
and after the fix it is pretty solid against terminal resizes Screen.Recording.2026-08-07.at.09.29.09.mov |
…h to find how many lines to clean
@EclesioMeloJunior It's not fixed. I reproduce it by going from my terminal full screen to floating window (alt down/up on GNOME, alacritty, Fedora Linux 44 (Workstation Edition). |
2 participants
Summary of changes
print_sync_report_detailswas counting eachprintln!as lines written and use it after to clear the terminal (by moving the cursor) and happens that a line wider than the terminal wraps onto two of them which causes the cursor to land one row too low and the clear started below the first line, leaving oneStatus:line on screen per refresh.Changes introduced in this pull request:
src/cli/subcommands/sync_cmd.rs: count the terminal rows each line occupies instead of counting lines, so the cursor arithmetic matches what is on screen, fetch terminal width fromdialoguer::console::Term(already a dependency).BufWriterso the cursor moves, the clear and the whole report reach the terminal in a single write.Reference issue to close (if applicable)
Closes #7366
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Bug Fixes
forest-cli sync wait -wleaving staleStatus:lines when reports wrap across multiple terminal rows.Documentation