fix: fix .envrc body prompt does not collapse#501
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a collapsible, scrollable ChangesCollapsible envrc body expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Greptile SummaryThis PR replaces the always-expanded
Confidence Score: 5/5Safe to merge — the envrc body scroll is applied by skipping logical lines rather than scrolling the Paragraph widget, so the path/capability/pattern header stays visible at all times, and all edge cases are covered by tests. The change is self-contained: new state fields are initialised in every PermissionModel constructor, key handlers are guarded by is_envrc_body_expanded()/envrc_body_is_collapsible() and are no-ops for non-envrc prompts, and the scroll-position arithmetic uses saturating arithmetic throughout. Seven targeted unit tests give high confidence the feature behaves correctly. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
K[KeyEvent] --> CO{Ctrl+O?}
CO -- yes --> TC[toggle_envrc_body]
TC --> CC{is_collapsible?}
CC -- no --> NOP[no-op]
CC -- yes --> TOG[flip expanded flag\nclear scroll offset if collapsing]
CO -- no --> PG{PageUp / PageDn?}
PG -- PageDn --> SD[scroll_envrc_body_down\noffset += STEP, clamp to max_scroll_offset]
PG -- PageUp --> SU[scroll_envrc_body_up\noffset -= STEP, floor 0]
PG -- no --> OTHER[other key handlers]
TOG --> RENDER
SD --> RENDER
SU --> RENDER
NOP --> RENDER
RENDER[draw_body / push_envrc_body_lines]
RENDER --> EXP{is_expanded?}
EXP -- yes --> SKIP[skip scroll_offset lines\ntake MAX-skip lines\nshow scroll hint]
EXP -- no --> COL{is_collapsible?}
COL -- yes --> PRV[take 5 preview lines\nshow … +N lines hint\nhide picker detail lines]
COL -- no --> FULL[render all lines]
Reviews (3): Last reviewed commit: "fix: clamp max scroll" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 1
🤖 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 `@crates/seal-tui/src/permission_prompt.rs`:
- Around line 878-883: The envrc_body_line_count() repeatedly iterates
body.lines().count(); add a cached usize field (e.g., cached_envrc_body_lines)
to the struct, initialize it to 0 in all constructors, set/update it whenever
envrc_info is attached/changed (compute info.body.lines().count() then), and
change envrc_body_line_count() to just return the cached value; be sure code
paths that remove/replace envrc_info also reset/update the cached value so
envrc_body_is_collapsible, scroll_envrc_body_down, and push_envrc_body_lines
read the cached count.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6151fa98-3527-48ab-82b0-5460d487d388
📒 Files selected for processing (1)
crates/seal-tui/src/permission_prompt.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/seal-tui/src/permission_prompt.rs (1)
911-918:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClamp the envrc scroll offset before it can skip the whole body.
scroll_envrc_body_down()currently allowsenvrc_body_scroll_offsetto reachmin(line_count, MAX_ENVRC_BODY_LINES).push_envrc_body_lines()then doeslines().skip(offset), so the lastPageDownon a 50-line body skips all 50 accessible lines and renders an empty expanded body. Clamp to the last valid start line instead of the line count itself.🛠️ Minimal fix
fn scroll_envrc_body_down(&mut self) { if self.is_envrc_body_expanded() { - let max = self.envrc_body_line_count().min(MAX_ENVRC_BODY_LINES); + let max = self + .envrc_body_line_count() + .min(MAX_ENVRC_BODY_LINES) + .saturating_sub(1); self.envrc_body_scroll_offset = self .envrc_body_scroll_offset .saturating_add(ENVRC_BODY_SCROLL_STEP) .min(max); } }Also applies to: 1653-1668
🤖 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 `@crates/seal-tui/src/permission_prompt.rs` around lines 911 - 918, The scroll logic for envrc allows envrc_body_scroll_offset to equal the total accessible line count, which then makes push_envrc_body_lines().lines().skip(offset) render nothing; change the clamp in scroll_envrc_body_down() to cap at the last valid start line (i.e., compute max = self.envrc_body_line_count().min(MAX_ENVRC_BODY_LINES).saturating_sub(1) and then set envrc_body_scroll_offset = self.envrc_body_scroll_offset.saturating_add(ENVRC_BODY_SCROLL_STEP).min(max)); do the same adjustment for the other identical block mentioned (around the region noted at lines 1653-1668) so push_envrc_body_lines() never skips the entire body.
🤖 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.
Outside diff comments:
In `@crates/seal-tui/src/permission_prompt.rs`:
- Around line 911-918: The scroll logic for envrc allows
envrc_body_scroll_offset to equal the total accessible line count, which then
makes push_envrc_body_lines().lines().skip(offset) render nothing; change the
clamp in scroll_envrc_body_down() to cap at the last valid start line (i.e.,
compute max =
self.envrc_body_line_count().min(MAX_ENVRC_BODY_LINES).saturating_sub(1) and
then set envrc_body_scroll_offset =
self.envrc_body_scroll_offset.saturating_add(ENVRC_BODY_SCROLL_STEP).min(max));
do the same adjustment for the other identical block mentioned (around the
region noted at lines 1653-1668) so push_envrc_body_lines() never skips the
entire body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7c3d72e4-e1be-43ee-9915-6ee13dd64b0e
📒 Files selected for processing (1)
crates/seal-tui/src/permission_prompt.rs
Merge activity
|
2 participants
Pull request
Summary
Long
.envrcbodies in the permission prompt now start collapsed to a 5-line preview so the action options remain visible. Users can expand the full body withCtrl+Oand scroll through it withPageUp/PageDn, with a hard cap of 200 rendered lines for very large files.Changes
.envrcbody rendering defaults to a collapsed 5-line preview when the body exceeds that threshold, with a… +N lines (Ctrl+O to expand)truncation hintCtrl+Otoggles the body between collapsed and expanded; collapsing resets the scroll offset to zeroPageUp/PageDnscroll the expanded body in 10-line stepsCtrl+O/PgUp/PgDnactionsdraw_bodyapplies the scroll offset to theParagraphwidget when the body is expandedTest plan
envrc_long_body_collapsed_by_default— verifies only the first 5 lines render and the expand hint appearsenvrc_short_body_renders_fully_without_expand_hint— verifies short bodies render in full with no expand affordanceenvrc_ctrl_o_expands_long_body— verifiesCtrl+Oexpands the body and shows the collapse hintenvrc_ctrl_o_again_toggles_back_to_collapsed_and_resets_scroll— verifies toggling back collapses and zeroes the scroll offsetenvrc_collapsed_options_block_present_on_standard_height— renders to aTestBackendat 80×24 and asserts all four picker options are visibleenvrc_expanded_page_down_scrolls_body— verifiesPageDn/PageUpadvance and retreat the scroll offset byENVRC_BODY_SCROLL_STEPenvrc_expanded_body_keeps_200_line_safety_cap— verifies the 200-line cap and truncation message for a 250-line body