feat: corvid busy indicator - the mascot pecks while operations run (#143) - #154
Conversation
…143) A static ⏳label was easy to miss: during a 20s helm dry-run the UI read as frozen, and post-approval writes had no in-flight state at all between approval and the outcome toast. - StatusBar animates a pure-ASCII pecking corvid (( o> / ( o- / (_o>) next to the progress label - equal-width frames swapped in place by a 0.5s set_interval, started with the first live _progress_labels owner and stopped (frame reset) with the last. Frame cycling is a pure function (bird_frame) so tests never assert on wall-clock timing. - _run_write publishes '<action> <kind>/<name>' for exactly the write's span (issue #143 scope 2), riding the existing owner-scoped label machinery - overlapping operations keep their non-clobbering semantics unchanged. Fixes #143 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Ready to approve
The implementation is correct and tested; remaining feedback is limited to non-blocking docstring formatting.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds an animated ASCII corvid to status-bar progress labels and extends progress reporting across approved writes.
Changes:
- Adds deterministic bird frames and lifecycle-managed animation.
- Publishes progress throughout post-approval writes.
- Tests rendering, animation lifecycle, and write progress.
File summaries
| File | Description |
|---|---|
src/korvid/ui/widgets/status_bar.py |
Implements the busy-bird animation. |
src/korvid/ui/app.py |
Wraps approved writes with progress reporting. |
tests/ui/test_busy_bird.py |
Covers frames, lifecycle, and write progress. |
Review details
Suppressed comments (1)
src/korvid/ui/widgets/status_bar.py:29
- This uses the RST
:data:role, but AGENTS.md:65 requires Markdown docstrings with no RST syntax. Use a Markdown code span instead.
"""The frame for one animation tick; cycles through :data:`BIRD_FRAMES`."""
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…ings Review feedback on #154: AGENTS.md requires Markdown docstrings with no RST syntax. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
Animation ticks currently trigger layout work, contradicting the stated no-per-frame-layout requirement.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/korvid/ui/widgets/status_bar.py:4
- This double-backtick inline literal is RST syntax, but AGENTS.md:65 requires Markdown docstrings with no RST syntax. Use a single-backtick Markdown code span, consistent with the corrected references below.
``_progress_labels`` machinery; while any label is live the bar animates a
src/korvid/ui/widgets/status_bar.py:83
- The timer calls
_render_line(), which reachesStatic.update(line)with Textual's defaultlayout=True. That schedules a layout on every 500 ms frame despite the PR/issue requirement of no per-frame layout; equal-width frames prevent visual jitter but do not suppress layout work. Pass a layout flag through_render_lineand uselayout=Falsefor animation ticks while retaining normal layout updates for status changes.
self._render_line()
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Review round 2 on #154: the 500ms bird tick called Static.update with the default layout=True, scheduling a layout pass per frame despite the equal-width no-layout design. _advance_bird now renders with layout=False (status changes still lay out normally), pinned by test_animation_tick_does_not_schedule_layout. Also converts the last RST double-backtick literal to a Markdown code span. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
코비드 busy 인디케이터(#143) 구현이 깔끔합니다. 확인한 사항:
- 애니메이션 수명주기:
_sync_animation이 첫 라벨에서 타이머 시작, 마지막 라벨 해제 시 stop + tick 리셋 — owner 스코프 라벨 의미론(_progress_labels)은 건드리지 않음. 새 작업이 frame 0부터 재시작하는 것까지 테스트로 고정. - 프레임 로직:
bird_frame(tick)순수 함수 + 등폭/ASCII 테스트 — 타이밍 단언 없이 결정적._advance_bird()를 직접 호출하는 테스트 방식도 AGENTS.md 규칙에 부합. _run_write진행 라벨: 기존_progress()owner-token 컨텍스트 매니저를 그대로 재사용해 승인~outcome 토스트 사이 공백을 메움. try/finally로 어떤 종료 경로에서든 라벨 해제._run_write_inner분리도 1:1 이동 확인.- 게이트된 slow_op 테스트가 in-flight 라벨 존재/해제를 실제로 검증 (vacuous 아님).
사소한 제안 (비차단):
StatusBar._last_status에 클래스 기본값이 없습니다. 현재 코드 경로상 타이머는update_status이후에만 시작되므로 안전하지만,_render_line/_advance_bird가 첫update_status전에 호출되면AttributeError가 납니다._last_status: tuple | None = None기본값 + None 조기 반환 한 줄이면 방어됩니다._progress()의 owner 토큰 접두사가helm:{seq}인데 이제 일반 write 스팬(delete/scale 등)에도 쓰입니다. 동작엔 무관하지만 디버깅 시 오해 소지가 있으니op:{seq}정도로 일반화하면 좋겠습니다.
APPROVE
There was a problem hiding this comment.
🟢 Ready to approve
The implementation satisfies the stated scope with deterministic, comprehensive coverage and no unresolved correctness issues.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
재리뷰 (신규 커밋 1개, round 2 반영).
_advance_bird가_render_line(layout=False)로 전환되어 500ms 프레임 틱이 더 이상 레이아웃 패스를 스케줄하지 않고 repaint만 수행합니다. 프레임이 equal-width라는 기존 설계와 정합적이며, 실제 상태 변경(update_status경로)은 기본값layout=True를 유지합니다.test_animation_tick_does_not_schedule_layout는Static.update를 monkeypatch 스파이로 감싸 tick →calls == [False](음성 검증)와 상태 변경 →True in calls(양성 대조군)를 모두 단언하므로 공허한 테스트가 아닙니다.- 남은 docstring 수정은 RST 이중 백틱 → Markdown 코드 스팬 변환으로 무해합니다.
지적 사항 없음. (이전 라운드의 _last_status 클래스 기본값 부재 Suggestion은 여전히 유효하나 blocking 아님.)
APPROVE
CI on a slow runner ticked the 500ms timer between _set_progress and the render inspection, so frame-identity assertions against BIRD_FRAMES[0] flaked (test (3.13) failed with frame 1 showing). Assert against the bar's own tick counter / any frame instead - no wall-clock coupling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The reset test does not currently verify that a new operation starts at frame zero.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
tests/ui/test_busy_bird.py:157
- This does not verify the claimed frame-0 reset:
any(...)passes even if_tick = 0is removed. Assert the initial frame synchronously before yielding to the interval timer so that regression is covered without a timing assertion.
# a new operation restarts the animation from a reset tick counter
app._set_progress("test2", "next op")
await pilot.pause()
assert bar._anim_timer is not None
assert any(frame in str(bar.render()) for frame in BIRD_FRAMES)
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Review round 4 on #154: the any-frame relaxation stopped covering the _tick reset. _set_progress renders synchronously, so inspecting the bar before awaiting the event loop pins frame 0 deterministically - precise again, still no wall-clock coupling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Ready to approve
The implementation satisfies the issue’s scope and includes comprehensive deterministic coverage.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
재리뷰 (신규 커밋 2건, 테스트 전용)
CI 느린 러너에서 500ms 타이머가 _set_progress와 렌더 검사 사이에 tick되어 BIRD_FRAMES[0] 프레임 고정 단언이 flake하던 문제를 해결한 델타입니다.
_set_progress가 동기 렌더라는 점을 이용해 이벤트 루프에 양보(pilot.pause())하기 전에 바를 검사하도록 순서를 바꿔 frame 0을 결정적으로 고정 — wall-clock 결합 없이 정밀도 유지 (any-frame 완화로 잃었던_tick리셋 커버리지도 c9747a28에서 복원).- 리셋 테스트에
bar._anim_timer is not None단언 추가로 재시작 경로가 공허하지 않음.
지적사항 없음. 결정적이고 올바른 수정입니다.
APPROVE
Fixes #143
Decisions recorded on the issue: pure-ASCII pecking frames, 500ms, bird before the label, scope = progress labels + post-approval writes (modals later).
What
_progress_labelsowner is live, an ASCII corvid pecks next to the label —( o>→( o-→(_o>— equal-width frames swapped in place by a 0.5sset_interval(no per-frame layout). Started with the first owner, stopped and reset with the last. One animation, labels swap per owner: the owner-scoped non-clobbering semantics are untouched._run_writenow publishes<action> <kind>/<name>for exactly the write span — previously there was no in-flight state between approval and the outcome toast.bird_frame(tick)), tested without any timing assertions; the timer callback is invoked directly in tests.Tests
_run_write: gated op — label present during, cleared after.Full gate (make check) green: 2874 passed.