-
Notifications
You must be signed in to change notification settings - Fork 3
fix(tui): fix mention popup positioning and Unicode width calculation #59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes #5292 and #5293 - ToolResponseStore memory issues. Problem: 1. Store grows without limit 2. Consumed responses not removed Solution: - Added ToolResponseStore with configurable max size (default: 500 entries) - Entries are removed when consumed via take() method (#5293) - Automatic periodic cleanup of expired entries based on TTL - Eviction of oldest entries when capacity is reached (#5292) Features: - MAX_STORE_SIZE constant (500) to prevent unbounded growth - DEFAULT_TTL (5 minutes) for automatic expiration - CLEANUP_INTERVAL (1 minute) for periodic cleanup - get() for peeking without removal - take() for consuming and removing entries - cleanup_expired() and cleanup_read() for manual cleanup - Stats tracking for monitoring store behavior
- server.rs: Hold write lock during entire state check and modification in handle_initialize to prevent concurrent initialization races - registry.rs: Use HashMap entry API to atomically check-and-insert plugin registration to prevent duplicate registration races Fixes #5262, #5260
Fixes bypass attempts where blocked commands like 'rm -rf' could be evaded using: - Extra whitespace: 'rm -rf' → 'rm -rf' - Quoted parts: "'rm' -rf" → 'rm -rf' - Path variants: '/bin/rm -rf' → 'rm -rf' Added normalize_command() function that: 1. Collapses whitespace by splitting/joining 2. Strips surrounding quotes from command parts 3. Extracts basename for command (first part) Also added comprehensive tests for bypass scenarios.
- Fix #5181: validate_path_safety() now checks path components instead of substring match, preventing false positives for filenames like 'file..txt' - Fix #5183: expand_tilde() now handles bare '~' path in addition to '~/' prefix - Updated tests to verify correct behavior
Fixes #5208, #5210, #5244 - Session storage error handling improvements. Changes: - list_sessions: Now properly collects and reports IO errors instead of silently ignoring them. Returns available sessions while logging errors for failed ones. - SessionManager: Only updates in-memory state after successful storage operations. If storage fails, the session is marked as modified for later retry. - Default impl: Improved panic message to help diagnose initialization failures (permissions, disk space).
Greptile OverviewGreptile SummaryThis PR fixes two TUI rendering issues (#5189 and #5184) related to mention popup positioning and Unicode width calculation, while also including several unrelated but valuable fixes. Main Changes (TUI Fixes)
Additional Changes (Scope Expansion)The PR includes 14 additional files with unrelated fixes that go beyond the stated scope:
AssessmentThe code quality is excellent with comprehensive test coverage and well-documented changes. However, the PR title and description don't reflect the actual scope - this appears to be multiple unrelated fixes bundled together. Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| src/cortex-tui/src/widgets/mention_popup.rs | Fixed popup positioning bounds checking and Unicode width calculation - correctly handles off-screen rendering and multi-byte characters |
| src/cortex-tui-components/src/selection_list.rs | Fixed cursor positioning underflow and Unicode width calculation - prevents arithmetic underflow and correctly counts characters vs bytes |
| src/cortex-engine/src/validation.rs | Added command normalization to prevent validation bypass via whitespace, quotes, and path variants |
| src/cortex-engine/src/tools/response_store.rs | New bounded tool response store with TTL and automatic cleanup - prevents unbounded memory growth with comprehensive test coverage |
| src/cortex-tui/src/session/manager.rs | Enhanced transaction safety for message operations - only updates memory after successful storage operations, restores state on failure |
| src/cortex-mcp-server/src/server.rs | Fixed TOCTOU race condition in initialization using atomic check-and-set with single write lock |
Sequence Diagram
sequenceDiagram
participant User
participant TUI
participant MentionPopup
participant SelectionList
participant Buffer
User->>TUI: Types @ character
TUI->>MentionPopup: Activate mention mode
Note over MentionPopup: Calculate dimensions
MentionPopup->>MentionPopup: chars().count() for width
Note right of MentionPopup: Fixed: Uses character count<br/>instead of byte length
Note over MentionPopup: Calculate position
MentionPopup->>MentionPopup: Check if fits above
alt Fits above
MentionPopup->>Buffer: Render at y - height
else Does not fit above
MentionPopup->>Buffer: Render at y + 1
end
Note right of MentionPopup: Fixed: Bounds checking<br/>prevents off-screen rendering
User->>SelectionList: Types search query
SelectionList->>SelectionList: Calculate cursor position
SelectionList->>SelectionList: chars().count() for offset
Note right of SelectionList: Fixed: Character count<br/>for cursor positioning
SelectionList->>SelectionList: Calculate reason_x position
SelectionList->>SelectionList: saturating_add/saturating_sub
Note right of SelectionList: Fixed: Prevents<br/>arithmetic underflow
SelectionList->>Buffer: Render search bar with cursor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 files reviewed, no comments
… prevention This PR consolidates the following fixes: - #38: Prevent usize to u16 overflow in interactive renderer - #42: Prevent usize to u16 overflow in card count displays - #58: Fix cursor positioning and underflow in selection list - #59: Fix mention popup positioning and Unicode width calculation - #60: Improve autocomplete popup positioning and width calculation - #64: Prevent underflow in dropdown navigation and scroll calculations - #66: Prevent panic in HelpBrowserState when sections empty All changes target the TUI components to improve robustness: - Added saturating casts for u16 conversions - Fixed cursor positioning calculations - Added bounds checking for empty sections - Improved Unicode width handling for popups
|
Consolidated into #69 - fix(tui): consolidated TUI fixes for overflow, positioning, and panic prevention |
Summary
Fixes #5189 and #5184 - Mention popup rendering issues.
Problems
Solution
Added bounds checking and used char count for width.