feat(review): send pending feedback and archive finished reviews - #69
Conversation
backnotprop
left a comment
There was a problem hiding this comment.
Reviewed in full, with the gate run on macOS and in a Linux container (223 tests pass, clippy pedantic clean) and a v0.7.0 binary used to round-trip a record. The design is right and the tests are good. Five changes before merge, two optional.
Required
- Timestamp format.
store/review.rsnow writes RFC 3339 throughtimewith trimmed sub-second digits, socreated_at,updated_atanddeliveries[].atvary between 0 and 9 fractional digits in one record (v0.7.0 wrote…54Z, this branch wrote…55.425677Znext to it). Please write one fixed shape,YYYY-MM-DDTHH:MM:SS.mmmZ(three-digit milliseconds, likearchive::iso_millis), and advance an edited annotation by 1 ms rather than 1 ns. Keep parsing lenient. - Reply reviews changed on delete-after-send.
mark_unsentnow derives state fromall_delivered, which is "every remaining annotation is non-pending". Send A and B, delete B: the header saysSent,qquits without the unsent prompt. On main it re-armed toSend 1. This applies to reply reviews too, so "reply reviews keep their existing behavior" in the description isn't true. I'm fine with the new rule for both, but say so in the PR and add the delete-after-send expectation as a test so the behavior is deliberate. - Footer.
app/draw.rsreturns early when a status is set, so after "comment saved" the footer shows only that string and loses the file name, counts and key help until something clears it. On main the status led and the rest stayed. Please restore that. - Folder feedback trailing newline.
Feedback::addjoins withpush('\n')where the old code usedwriteln!per file, so--export <folder>and the folder send body end in one fewer newline. Please keep the old output byte for byte. - Folder open must tolerate one unreadable file.
open_folder, tree open andreloadnow propagaterefresh_review_counts()?, so a permission-denied document or a corrupt legacy sidecar for any annotated file aborts opening the whole folder; main swallowed those. Skip the file with a status note instead. Related:is_pendingtreats an unparsableupdated_atas pending forever, so such an annotation is resent on everyEand can never be archived; treat it as not pending, or surface it.
Optional
- On the concurrent-change path in
send.rs, when the reloaded record differs, still record the delivery for ids whose sent copy is unchanged, so a single external write doesn't make every delivered note pending again. - Add a fixture test that loads a literal v0.7.0 record (with
deliveries, withoutarchived) and asserts it writes back as a superset.
--snapshot output changes on the header line for file and folder reviews because of the new buttons; that's inherent and accepted. --print is untouched, good. The time dependency is acceptable: it was already compiled through ratatui, only the two features are new.
|
To save you a round trip, I'll push the five required changes as separate commits on top of yours in the next hour or so, with tests. Your commits stay as they are. If you've already started on any of them, say so here and I'll leave that one to you. |
|
Pushed the five changes as separate commits on top of yours:
Gate is green on macOS and in a Linux container (229 tests). Once CI passes on this head I'll re-review and we can merge. |
|
One more change coming on top, on the header only: the maintainer would rather not carry four buttons. The header will show the send button plus a single |
|
Pushed the header change: 29b3d11 replaces the three secondary buttons with |
File and folder reviews showed four header buttons. The header now carries only the send button and a `Review (m)` button. `m` or a click opens a small menu under the header's right edge, drawn like the message picker, with one row per action and a live count: resend all, finish review, undo finish, archive. Rows with nothing to act on are dimmed and skipped; enter, a click, or the row's own key runs a row and closes the menu. The direct keys R, F, U and H keep working without the menu, and the footer undo button is unchanged. Reply reviews keep their header. `--snapshot` accepts a trailing `menu` argument so the open menu can be rendered headlessly.
2afe1b9 to
ac454bc
Compare
|
Rebased onto main (#68 touched the README next to your paragraph; kept both). No code conflicts. Gate green on the rebased tree, 232 tests. |
backnotprop
left a comment
There was a problem hiding this comment.
All five review items are addressed, the header is now a single Review menu, the branch is rebased on main, and the flow was verified live inside Herdr with a real agent: send only new, sent tags, blocked-send handling, finish and archive with persistence, and the reply review unchanged.
|
Merged. Thank you for a well-designed feature and for the tests that made the review tractable. The header ended up as one Review menu per the maintainer's call; the behavior is yours throughout. This ships in the next TUI release. |
File reviews currently resend old feedback whenever another annotation is added.
Eandthe header button now send only new or edited notes: send A and B, add C, and the next send
contains just C.
R/ Resend all includes everything explicitly and shows the sentcount. The send button stays available at zero and reports “nothing new to send”. Folder
reviews send one combined message and show N new across M files.
F/ Finish review archives sent notes that have not changed since delivery.Uundoesthe last finish;
Hopens archived notes for restoration, including after a restart.Restoring keeps the complete annotation, its id and its delivery history. The archive is
an optional
archivedfield alongside the existingannotationsarray. Reply reviewskeep their existing behavior.
Pending detection uses the existing delivery ids/times and each note’s
updated_at.Failed or blocked sends stay pending. If another writer changes a record while a send is
in progress, the newer record is preserved and the UI warns that sent status could not be
saved. Unchanged files still record their delivered notes.
Fixes #66.
Validated locally on Linux x86_64 with Rust 1.96.0:
cargo fmt --all --checkand workspace Clippy with-D warningscargo test --locked --workspace: 223 tests passed, including 21 new regression testsrestart/restore and blocked-send retry
--bench samples/big.mdcompared with v0.7.0, five runs per binaryNative Windows validation is left to the PR’s Windows CI job.