#+TITLE: Review: pearl — Modified-Ticket Indicator Spec, Round 2 #+AUTHOR: Codex #+DATE: 2026-06-07 #+STARTUP: showall * Scope reviewed - [[file:modified-ticket-indicator-spec.org][modified-ticket-indicator-spec.org]] after the round-1 response. - Round-1 review dispositions folded into the spec. - [[file:../pearl.el][pearl.el]] dirty scan, save lifecycle, comment ownership, overlay decoration, and mode-line lighter context. - Existing task [[file:../todo.org][todo.org]] entry "Visual indicator for modified tickets". * Implementation-readiness Rubric: =Ready with caveats=. The round-1 blockers are resolved. The spec now commits to a concrete, strong UI: face + explicit non-mutating text marker + mode-line count; it separates pushable dirty comments from read-only/local-only edits; it defines buffer-local lifecycle state and recompute triggers; it corrects the current-code helper names; and it has a drop-in task block plus review history. The remaining caveats are small but should be cleaned up before calling the spec fully =Ready=: - The status still says =Revised= rather than an implementation-ready rubric. - The =Open questions= section still asks Craig to confirm marker copy, even though defaults are declared non-blocking. - The drop-in implementation tasks are all =[#C]= despite being the v1 implementation plan for a user-visible feature. The workflow says v1 implementation work should be =[#B]= unless intentionally not near-term. * Overall assessment This is now a strong spec. The key product risk from round 1, a too-subtle face-only indicator, is fixed. The design now gives three redundant signals: where the unsaved work is, what it means, and whether any pushable work remains anywhere in the buffer. That is the right shape for "clear, easy, and hard to miss" in an Org buffer. The comment-ownership model is also now aligned with Pearl's save model. The spec no longer lies by calling all changed comments "needs pushing"; it has pushable, local-only, and unknown states with distinct display semantics. * High-priority findings None. * Medium-priority findings ** MP1: Promote the spec status to the actual review rubric Blocking: no. The body says the round-1 blockers are accepted and the implementation handoff is present, but the status still starts =Revised — Codex round 1 incorporated=. Change it to =Ready with caveats= now, or =Ready= after MP2/MP3 are resolved. ** MP2: Close or disposition the remaining wording questions Blocking: no, if defaults are accepted. The spec says the marker-text choices have defaults and are not implementation blockers, but they still live under =Open questions (for Craig)=. That can make a future implementer pause. Recommended: move those two wording choices into =v1 decisions= as final defaults: - Pushable marker: =" unsaved"=. - Local-only marker: =" local edit only"=. Keep the defcustoms so the wording remains adjustable without blocking implementation. ** MP3: Use =[#B]= for the v1 implementation drop-in tasks, or state why this is deliberately =[#C]= Blocking: no. The current drop-in block uses =[#C]= for every implementation phase. The workflow's tracking rule is v1 work = =[#B]=, vNext/someday = =[#D]=. Since this spec is solving a real user-facing save-model gap and the existing todo entry is a live feature task, the phase tasks should probably be =[#B]=. If Craig intentionally wants this as optional dogfooding polish, keep =[#C]= but say so in the status or task intro. * UX observations The recommended default UI is now right: - Dirty pushable issue: face + trailing =unsaved= marker + mode-line count. - Dirty own comment: comment marker plus parent issue bubble-up. - Dirty read-only comment: separate =local edit only= cue, no count and no bubble-up. - Unknown comment ownership: neutral state until viewer classification resolves. One implementation watchpoint is already correctly called out: trailing markers must coexist with org right-aligned tags. That should be tested against a heading with label tags. * Architecture observations The single =pearl--redecorate-buffer= entry point is the right abstraction. It reduces the risk of stale overlays as more display-only decorations accumulate. * Robustness and performance observations The lifecycle contract is now concrete enough: buffer-local count/cache/timer, change-scoped idle recompute, async post-save recompute from the actual dirty scan, full-pass redecorate after buffer rewrites, and viewer-callback liveness/account checks. * Test strategy recommendations The test surface is good. Add one explicit case for the tag coexistence caveat: a dirty issue heading with right-aligned org tags still shows the =unsaved= marker without corrupting the heading text or tags. * Documentation and tooling recommendations README should use the same vocabulary as the UI: =unsaved= for pushable work and =local edit only= for read-only comment drift. * Suggested spec edits - Update status to =Ready with caveats= or =Ready=. - Finalize marker wording defaults and move them out of open questions. - Change v1 task priorities to =[#B]=, or explicitly justify =[#C]=. - Add tag-coexistence to the test surface. * Agreed decisions None reached during this review. * Open questions None that block implementation if the declared defaults are accepted. * vNext candidates Already captured: optional leading glyph/fringe, next/previous modified navigation, header-line/side-window summary, and save-all prompt vocabulary alignment. * Implementation tasks (drop-in for todo.org) Use the spec's drop-in block after adjusting the task priorities per MP3.