refactor(tui): prompt deep-module PR 4 — session-scoped resources + unified footer#228
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe change introduces session-scoped prompt resources, bounded prompt-history persistence with ChangesPrompt shell refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CustomPromptSession
participant FooterViewModel
participant LegacyFooter
participant CardFooter
CustomPromptSession->>FooterViewModel: capture per-frame footer state
FooterViewModel->>LegacyFooter: render shared snapshot
FooterViewModel->>CardFooter: render shared snapshot
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/pythinker_code/ui/shell/prompt.py`:
- Around line 3963-3964: Update the toast snapshot logic near _prompt_toast so
both left and right toast values are preserved when present, rather than
selecting only the left toast. Adjust FooterViewModel.toast and the consumers
such as _render_right_span to access the snapshot for their respective position,
ensuring simultaneous toasts remain visible.
In `@src/pythinker_code/ui/shell/prompting/clipboard.py`:
- Around line 53-59: Update paste_text to log the caught clipboard exception at
debug level immediately before returning None, while preserving the existing
silent-to-caller behavior and exception handling scope.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ee039f0-86f2-45f7-825a-65d45b64af64
📒 Files selected for processing (15)
CHANGELOG.mdsrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/prompting/__init__.pysrc/pythinker_code/ui/shell/prompting/clipboard.pysrc/pythinker_code/ui/shell/prompting/footer.pysrc/pythinker_code/ui/shell/prompting/git_status.pysrc/pythinker_code/ui/shell/prompting/history.pysrc/pythinker_code/ui/shell/prompting/toasts.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/statusline.pysrc/pythinker_code/ui/theme/registry.pytests/ui_and_conv/test_footer_view_model.pytests/ui_and_conv/test_prompt_history.pytests/ui_and_conv/test_prompt_resources.pytests/ui_and_conv/test_prompt_tips.py
💤 Files with no reviewable changes (1)
- tests/ui_and_conv/test_prompt_tips.py
…indings - Carry left and right toasts separately in FooterViewModel so a right-side toast is no longer dropped when a left toast is active (regression from the footer unification). - Log clipboard read failures at debug before returning None. - Drop the unused is_clipboard_available re-export from prompt.py. - Split side-effecting append() calls out of assert statements in history tests.
|
All review findings addressed in be35a74: right-side toast preservation (with regression test), clipboard debug logging, unused-import removal, and side-effect-free history asserts. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/ui_and_conv/test_footer_view_model.py (1)
154-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert display width, not code-point length.
len(row)counts Unicode code points, while production truncation uses display width. Add wide/combining-character cases and assert the same display-width constraint; otherwise a terminal overflow can pass this test unnoticed.As per coding guidelines, tests must cover relevant edge cases rather than only ASCII happy paths.
🤖 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 `@tests/ui_and_conv/test_footer_view_model.py` around lines 154 - 158, Update the assertions in the loop over legacy and card renderings to measure each row’s terminal display width instead of using len(row), matching the production truncation behavior. Add test inputs containing wide and combining Unicode characters, and assert every rendered row stays within width while preserving the existing update-available and ellipsis checks.Source: Coding guidelines
src/pythinker_code/ui/shell/prompt.py (1)
3148-3162: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRun lifecycle cleanup on synchronous exit.
with CustomPromptSession(...)still skips_lifecycle.aclose(), so registered closers never run on__exit__or_start()failure. Add a synchronous close path that drives lifecycle cleanup, or drop the sync context manager if teardown must stay async.🤖 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/pythinker_code/ui/shell/prompt.py` around lines 3148 - 3162, Update CustomPromptSession’s synchronous lifecycle cleanup in __exit__ and the _start() failure path so _lifecycle.aclose() is driven to completion and registered closers run. Add a synchronous close mechanism compatible with the existing context-manager API, or remove synchronous context-manager support if teardown must remain asynchronous; preserve resource-binding and task cleanup.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 `@tests/ui_and_conv/test_footer_view_model.py`:
- Around line 174-204: The simultaneous-toast regression test currently covers
only _render_legacy_bottom_toolbar. Extend
test_left_and_right_toasts_both_render to render the same model with
_render_card_bottom_toolbar and assert that both “left-toast” and “right-toast”
appear in the card renderer output as well.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/prompt.py`:
- Around line 3148-3162: Update CustomPromptSession’s synchronous lifecycle
cleanup in __exit__ and the _start() failure path so _lifecycle.aclose() is
driven to completion and registered closers run. Add a synchronous close
mechanism compatible with the existing context-manager API, or remove
synchronous context-manager support if teardown must remain asynchronous;
preserve resource-binding and task cleanup.
In `@tests/ui_and_conv/test_footer_view_model.py`:
- Around line 154-158: Update the assertions in the loop over legacy and card
renderings to measure each row’s terminal display width instead of using
len(row), matching the production truncation behavior. Add test inputs
containing wide and combining Unicode characters, and assert every rendered row
stays within width while preserving the existing update-available and ellipsis
checks.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ce87365-34ee-4dac-9978-ff571a41bb2a
📒 Files selected for processing (5)
src/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/prompting/clipboard.pysrc/pythinker_code/ui/shell/prompting/footer.pytests/ui_and_conv/test_footer_view_model.pytests/ui_and_conv/test_prompt_history.py
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Remove the unused _ensure_private_history_path / _load_history_entries aliases (and their now-unused imports); history I/O lives in PromptHistoryStore and nothing references these shims.
|
@coderabbitai review |
✅ Action performedReview finished.
|
CustomPromptSession.__enter__ scheduled a lifecycle task via asyncio.create_task (which requires a running loop) and __exit__ never drove the lifecycle's async closers, so a synchronous `with` either raised on entry or leaked history/toast/clipboard/index resources on exit. Production uses `async with` exclusively; remove the broken sync context manager so misuse fails loudly instead of silently leaking. Also assert footer rows stay within terminal display width (not code-point length) and add wide/combining Unicode coverage, so a wide glyph overflowing the row can no longer pass the width guard unnoticed.
|
Addressed the CodeRabbit review findings in 0501693:
Validated: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pythinker_code/ui/shell/prompt.py (2)
3221-3231: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLog clipboard-media failures before falling back.
The broad handler drops adapter failures silently, making headless clipboard or platform regressions untraceable. Preserve the text-paste fallback, but catch as
excand emit a debug log with the failure context.🤖 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/pythinker_code/ui/shell/prompt.py` around lines 3221 - 3231, Update the clipboard-media exception handler around clipboard_adapter.paste_media() and grab_media_from_clipboard() to bind the exception as exc and emit a debug log containing failure context before returning False. Preserve the existing return value and text-paste fallback behavior.Sources: Coding guidelines, Path instructions
3561-3567: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove history persistence off the event loop.
_prompt_once()calls this synchronously, whilePromptHistoryStore.append()waits on a file lock and performs filesystem I/O. A contended lock or slow home directory freezes prompt rendering and delays submissions. Make this path async and offload the store append throughasyncio.to_thread(...).As per coding guidelines, “Prefer explicit async boundaries and avoid blocking calls in asynchronous runtime paths.”
🤖 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/pythinker_code/ui/shell/prompt.py` around lines 3561 - 3567, Update the _prompt_once() history persistence path and its caller flow to be asynchronous, and await asyncio.to_thread(...) around PromptHistoryStore.append so file locking and filesystem I/O run off the event loop. Preserve the existing _last_history_content update only when append succeeds, and propagate the async boundary through any callers that currently invoke this path synchronously.Source: Coding guidelines
tests/ui_and_conv/test_footer_view_model.py (1)
242-260: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest footer caching through observable behavior.
This test stubs the private
_build_footer_view_modeland asserts its call count, so harmless implementation changes can break the test without changing behavior. Verify same-frame reuse and next-frame invalidation by asserting rendered output or snapshot contents instead.As per path instructions, tests should validate observable behavior rather than mock internal implementation details.
🤖 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 `@tests/ui_and_conv/test_footer_view_model.py` around lines 242 - 260, Rewrite test_footer_view_model_is_built_once_per_prompt_frame to validate observable rendered output or snapshot contents instead of stubbing _build_footer_view_model or asserting its call count. Cover both reuse within the same prompt frame and invalidation when _current_prompt_frame changes, while preserving the expected footer content.Source: Path instructions
🤖 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 `@tests/ui_and_conv/test_footer_view_model.py`:
- Around line 187-195: Update the regression test around wide_command to use a
width derived from the fixture that falls at the display-width boundary and
forces truncation, rather than the broad fixed widths. Assert the rendered
legacy and card toolbar rows remain within that width and contain the expected
capability-safe ellipsis behavior, preserving the existing Unicode display-width
coverage.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/prompt.py`:
- Around line 3221-3231: Update the clipboard-media exception handler around
clipboard_adapter.paste_media() and grab_media_from_clipboard() to bind the
exception as exc and emit a debug log containing failure context before
returning False. Preserve the existing return value and text-paste fallback
behavior.
- Around line 3561-3567: Update the _prompt_once() history persistence path and
its caller flow to be asynchronous, and await asyncio.to_thread(...) around
PromptHistoryStore.append so file locking and filesystem I/O run off the event
loop. Preserve the existing _last_history_content update only when append
succeeds, and propagate the async boundary through any callers that currently
invoke this path synchronously.
In `@tests/ui_and_conv/test_footer_view_model.py`:
- Around line 242-260: Rewrite
test_footer_view_model_is_built_once_per_prompt_frame to validate observable
rendered output or snapshot contents instead of stubbing
_build_footer_view_model or asserting its call count. Cover both reuse within
the same prompt frame and invalidation when _current_prompt_frame changes, while
preserving the expected footer content.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75a9abb0-652f-42ef-93cb-852327d25f16
📒 Files selected for processing (2)
src/pythinker_code/ui/shell/prompt.pytests/ui_and_conv/test_footer_view_model.py
The wide/combining-char footer test used broad terminal widths, so the wide command never approached the width boundary and a len()-based guard could still have passed. Derive the width from the fixture so the left region must truncate: a len()-based renderer would under-truncate and overflow the row, while display-width accounting keeps every row within bounds and emits the capability-safe ellipsis (now asserted).
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 57 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/ui/shell/statusline.py (1)
488-517: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate width-fit loop for line1 vs line2.
The "drop highest-
drop_prioritysegment until it fits or onlydrop_priority == 0remains" loop is now repeated verbatim forline1_parts(488-493) andline2_parts(511-516). Extracting a shared helper (parts, columns, joiner) → fits both call sites and keeps the fitting semantics in one place.♻️ Suggested consolidation
+ def fit_to_width( + parts: list[tuple[str, list[StyleFragment]]], + joiner: Callable[[list[tuple[str, list[StyleFragment]]]], list[StyleFragment]], + ) -> list[tuple[str, list[StyleFragment]]]: + while width(joiner(parts)) > ctx.columns and len(parts) > 1: + victim = max(parts, key=lambda p: SEGMENT_REGISTRY[p[0]].drop_priority) + if SEGMENT_REGISTRY[victim[0]].drop_priority == 0: + break + parts.remove(victim) + return parts + line1_parts = rendered(zones.line1) - while width(joined(line1_parts)) > ctx.columns and len(line1_parts) > 1: - victim = max(line1_parts, key=lambda p: SEGMENT_REGISTRY[p[0]].drop_priority) - if SEGMENT_REGISTRY[victim[0]].drop_priority == 0: - break - line1_parts.remove(victim) + line1_parts = fit_to_width(line1_parts, joined) ... line2_parts = rendered(line2_ids) - while width(joined_line2(line2_parts)) > ctx.columns and len(line2_parts) > 1: - victim = max(line2_parts, key=lambda part: SEGMENT_REGISTRY[part[0]].drop_priority) - if SEGMENT_REGISTRY[victim[0]].drop_priority == 0: - break - line2_parts.remove(victim) + line2_parts = fit_to_width(line2_parts, joined_line2)🤖 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/pythinker_code/ui/shell/statusline.py` around lines 488 - 517, Extract the repeated width-fitting logic from the line1_parts and line2_parts loops into a shared helper that accepts the segment parts, available columns, and joiner function. Preserve the existing behavior of repeatedly removing the highest-drop_priority segment until the content fits or only priority-0 segments remain, then use the helper for both lines.
🤖 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/pythinker_code/ui/shell/prompting/footer.py`:
- Around line 74-115: Extract the duplicated width and ellipsis handling from
truncate_footer_right and truncate_footer_left into one private helper,
parameterized by iteration direction or equivalent. Keep both public functions’
existing left/right retention, ordering, narrow-width fallback, and
capability-safe ellipsis behavior unchanged, delegating their results to the
shared core.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/statusline.py`:
- Around line 488-517: Extract the repeated width-fitting logic from the
line1_parts and line2_parts loops into a shared helper that accepts the segment
parts, available columns, and joiner function. Preserve the existing behavior of
repeatedly removing the highest-drop_priority segment until the content fits or
only priority-0 segments remain, then use the helper for both lines.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91ba0334-63f0-4bed-a257-5ed4ed29ad2a
📒 Files selected for processing (15)
CHANGELOG.mdsrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/prompting/__init__.pysrc/pythinker_code/ui/shell/prompting/clipboard.pysrc/pythinker_code/ui/shell/prompting/footer.pysrc/pythinker_code/ui/shell/prompting/git_status.pysrc/pythinker_code/ui/shell/prompting/history.pysrc/pythinker_code/ui/shell/prompting/toasts.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/statusline.pysrc/pythinker_code/ui/theme/registry.pytests/ui_and_conv/test_footer_view_model.pytests/ui_and_conv/test_prompt_history.pytests/ui_and_conv/test_prompt_resources.pytests/ui_and_conv/test_prompt_tips.py
💤 Files with no reviewable changes (1)
- tests/ui_and_conv/test_prompt_tips.py
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 6 minutes. |
1 participant
Part 4 of the TUI prompt deep-module refactor (plan: docs/superpowers/plans/2026-07-19-tui-prompt-deep-module-refactor.md; follows #218/#219/#220).
Task 8 — session-scoped prompt resources
prompting/git_status.py(GitStatusIndex): host-backed async Git branch/status/diffstat index keyed by repository root, lifecycle-owned, stale-result discard, degraded snapshots on failure/timeout — replaces the module-global subprocess caches.prompting/toasts.py(ToastManager+ frozenToastSnapshot): session-owned toasts with aContextVar-routedtoast()compatibility façade and a bounded bootstrap queue for pre-session toasts.prompting/history.py(PromptHistoryStore): locked JSONL history with tail-capped loading (1,000 entries), 256 KiB record cap, 10 MiB rotation with one.1file, secret redaction, sensitive-command exclusion, and honestPromptHistoryErrorfailures.prompting/clipboard.py(ClipboardAdapter): capability checks and text/media paste behind one adapter./prompt-history status|clearshell command (the/history→/statsalias is untouched);clearnever claims success on lock timeout or I/O failure.tests/ui_and_conv/test_prompt_resources.pyproves cross-session isolation (roots, toasts, closer independence, stale-result rejection).Task 9 — unified footer + capability-keyed theme cache
prompting/footer.py(FooterViewModel): one immutable per-frame snapshot of status, command line, extension statuses, background summary, toast, and update notice; both the legacy and card bottom toolbars are now adapters over the same model with shared left-content precedence and capability-safe truncation.get_resolvernow caches per(theme, TerminalCapabilities)so a capability change (color depth, NO_COLOR) yields a correct resolver without a theme switch.tests/ui_and_conv/test_footer_view_model.py: adapter parity at widths 20/40/80/120 across precedence cases, immutability, once-per-frame build, and capability-cache behavior.Verification
make check-pythinker-code: all checks passed (ruff format/check, strict pyright, ty).make test-pythinker-code: 8,315 passed unit + 65 passed e2e (7 skipped, 1 xfailed).test_prompt_tips.pytargeted the deleted module-global git subprocess internals; equivalent stale-result/timeout coverage lives in the new resources tests.Summary by CodeRabbit
/prompt-history statusand/prompt-history clearto view and manage saved prompt history.