Skip to content

feat(review): send pending feedback and archive finished reviews - #69

Merged
backnotprop merged 10 commits into
plannotator:mainfrom
Shujakuinkuraudo:feat/review-pending-archive
Sep 9, 2026
Merged

feat(review): send pending feedback and archive finished reviews#69
backnotprop merged 10 commits into
plannotator:mainfrom
Shujakuinkuraudo:feat/review-pending-archive

Conversation

@Shujakuinkuraudo

Copy link
Copy Markdown
Contributor

File reviews currently resend old feedback whenever another annotation is added. E and
the 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 sent
count. 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. U undoes
the last finish; H opens archived notes for restoration, including after a restart.
Restoring keeps the complete annotation, its id and its delivery history. The archive is
an optional archived field alongside the existing annotations array. Reply reviews
keep 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 --check and workspace Clippy with -D warnings
  • cargo test --locked --workspace: 223 tests passed, including 21 new regression tests
  • Herdr manifest check, release build, Unix plugin staging, byte comparison and version check
  • Real PTY smoke: incremental sends, editing, explicit resend, finish/undo,
    restart/restore and blocked-send retry
  • --bench samples/big.md compared with v0.7.0, five runs per binary

Native Windows validation is left to the PR’s Windows CI job.

@backnotprop backnotprop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Timestamp format. store/review.rs now writes RFC 3339 through time with trimmed sub-second digits, so created_at, updated_at and deliveries[].at vary between 0 and 9 fractional digits in one record (v0.7.0 wrote …54Z, this branch wrote …55.425677Z next to it). Please write one fixed shape, YYYY-MM-DDTHH:MM:SS.mmmZ (three-digit milliseconds, like archive::iso_millis), and advance an edited annotation by 1 ms rather than 1 ns. Keep parsing lenient.
  2. Reply reviews changed on delete-after-send. mark_unsent now derives state from all_delivered, which is "every remaining annotation is non-pending". Send A and B, delete B: the header says Sent, q quits without the unsent prompt. On main it re-armed to Send 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.
  3. Footer. app/draw.rs returns 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.
  4. Folder feedback trailing newline. Feedback::add joins with push('\n') where the old code used writeln! per file, so --export <folder> and the folder send body end in one fewer newline. Please keep the old output byte for byte.
  5. Folder open must tolerate one unreadable file. open_folder, tree open and reload now propagate refresh_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_pending treats an unparsable updated_at as pending forever, so such an annotation is resent on every E and 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, without archived) 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.

@backnotprop

Copy link
Copy Markdown
Contributor

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.

@backnotprop

Copy link
Copy Markdown
Contributor

Pushed the five changes as separate commits on top of yours:

  • a90d71b timestamps: one fixed millisecond format for created_at, updated_at and deliveries[].at; edits advance by 1 ms; delivery times round up to the next millisecond so a legacy sub-millisecond updated_at can't outrank its own delivery.
  • 757c84a test only: removing a sent annotation keeps the review Sent, for file and reply reviews. Please add a line to the description saying the rule now applies to reply reviews as well.
  • ab35164 footer: the status leads, and the file name, counters and key help still follow; the key help yields width to the status on narrow panes.
  • 9fa2c1f folder feedback ends with the same trailing newline as 0.7.0; verified byte-identical against a main build.
  • a3179f2 folder open and reload skip unreadable annotated files with a one-time status note instead of failing; an annotation with an unparsable updated_at is not pending once it has been delivered, and stays pending if it never was.

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.

@backnotprop

Copy link
Copy Markdown
Contributor

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 Review ▾ (m) that opens a small menu listing resend all, finish review, undo and archive with their counts and keys; the direct keys R, F, U and H keep working as you built them. Behavior underneath is unchanged. I'll push it as a separate commit and ping here.

@backnotprop

Copy link
Copy Markdown
Contributor

Pushed the header change: 29b3d11 replaces the three secondary buttons with Review ▾ (m), a popup in the picker's style listing resend all, finish review, undo and archive with live counts; rows that don't apply are dimmed and skipped. R, F, U and H keep working directly and now share one action runner with the menu, so status messages are identical. 2afe1b9 updates the key tables. --snapshot accepts a trailing menu argument to render the open menu. Gate is green on macOS and Linux; CI running now.

Shujakuinkuraudo and others added 10 commits September 9, 2026 12:22
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.
@backnotprop
backnotprop force-pushed the feat/review-pending-archive branch from 2afe1b9 to ac454bc Compare September 9, 2026 19:22
@backnotprop

Copy link
Copy Markdown
Contributor

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 backnotprop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@backnotprop
backnotprop added this pull request to the merge queue Sep 9, 2026
Merged via the queue into plannotator:main with commit f221277 Sep 9, 2026
2 checks passed
@backnotprop

Copy link
Copy Markdown
Contributor

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.

@backnotprop backnotprop mentioned this pull request Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

File reviews: clear or archive sent annotations before the next review

2 participants