Observer mode, Ctrl+H help, and Disputes UI improvements#30
Open
arkanoider wants to merge 17 commits intomainfrom
Open
Observer mode, Ctrl+H help, and Disputes UI improvements#30arkanoider wants to merge 17 commits intomainfrom
arkanoider wants to merge 17 commits intomainfrom
Conversation
…docs - Add context-aware help popup (Ctrl+H) with per-tab shortcuts; restore previous mode on close so chat input stays enabled (HelpPopup stores Box<UiMode> to restore). - Add src/ui/constants.rs for shared help/footer strings; use in help popup and disputes footer. - Disputes in Progress footer: put Ctrl+H first (always visible), use two lines when width >= 90 to avoid overflow; width-aware layout. - Header: merge Amount/Fiat and Privacy onto one line; reduce header height from 8 to 7 for one more chat row. - Docs: TUI_INTERFACE.md, ADMIN_DISPUTES.md (help, footer, OperationResult). Made-with: Cursor
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRenamed OrderResult → OperationResult across UI and utils; added a context-aware Help popup (Ctrl+H); introduced an Admin Observer tab with local decryption and observer inputs; migrated dispute chat from List to ScrollView with scroll tracking; centralized UI help/footer constants; expanded and relocated AppState with observer/chat fields; settings init became fallible. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KeyHandler as Key Handler
participant AppState
participant HelpPopup as Help Popup Renderer
participant UI as UI Render
User->>KeyHandler: Press Ctrl+H
KeyHandler->>AppState: set mode = HelpPopup(current_tab, prev_mode)
AppState->>HelpPopup: request help content (tab, app state)
HelpPopup->>UI: render overlay with content
UI->>User: display Help popup
User->>KeyHandler: Esc / Enter / Ctrl+H
KeyHandler->>AppState: restore prev_mode from HelpPopup
AppState->>UI: resume normal rendering
sequenceDiagram
participant User
participant ObserverTab
participant KeyHandler as Key Handler
participant FS as FileSystem
participant Decrypt as Decryption Flow
participant AppState
participant UI as UI Render
User->>ObserverTab: Enter (with filepath + hex key)
ObserverTab->>KeyHandler: validate inputs
KeyHandler->>FS: read file
FS-->>KeyHandler: file bytes / I/O error
KeyHandler->>Decrypt: hex-decode key, decrypt bytes
Decrypt-->>AppState: update observer_chat_lines or observer_error
AppState->>UI: re-render Observer tab with decrypted preview or error
UI->>User: show decrypted chat or error message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/main.rs (1)
186-186: Consider making admin chat polling interval configurable.A fixed 2s interval can be costly on slower relays; moving this to settings (with a sane default) would improve deploy-time tuning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` at line 186, Replace the hardcoded interval(Duration::from_secs(2)) used to create admin_chat_interval with a configurable setting: add a new setting/field (e.g., admin_chat_poll_interval_secs or admin_chat_poll_interval) to your configuration/Settings struct or CLI options with a sane default (2 seconds), parse/validate it at startup, and construct admin_chat_interval using Duration::from_secs(config.admin_chat_poll_interval_secs.unwrap_or(2)) (or Duration::from(config.admin_chat_poll_interval)). Update any config loading/Defaults and docs so operators can tune the poll interval; locate the change around the admin_chat_interval binding where interval(Duration::from_secs(2)) is called.src/ui/tabs/observer_tab.rs (3)
169-169: Consider clarifying the key length in the title.The title says "32-byte hex" but the PR objectives describe "64-character hex shared key". While technically equivalent (32 bytes = 64 hex chars), users entering the key may find "64-character hex" clearer since they're typing characters, not bytes.
Suggested title text
- let key_title = Span::styled("Shared key (32-byte hex)", key_title_style); + let key_title = Span::styled("Shared key (64 hex characters)", key_title_style);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tabs/observer_tab.rs` at line 169, The label for the shared key is ambiguous; update the Span::styled call that creates key_title (the line with let key_title = Span::styled("Shared key (32-byte hex)", key_title_style);) to use clearer wording such as "Shared key (64-character hex)" so users understand they must enter 64 hex characters rather than interpreting byte length.
178-181: Consider adding Ctrl+H hint to the footer.Per PR objectives, Ctrl+H opens a context-aware help popup. The footer mentions Tab, Enter, Esc, and Ctrl+C but omits Ctrl+H. Adding it would align with the documented shortcuts.
Suggested footer text
let footer = Paragraph::new( - "Tab: Switch field | Enter: Load & decrypt | Esc: Clear error | Ctrl+C: Clear all", + "Ctrl+H: Help | Tab: Switch field | Enter: Load & decrypt | Esc: Clear error | Ctrl+C: Clear all", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tabs/observer_tab.rs` around lines 178 - 181, Update the footer string created in the observer_tab.rs UI so it includes the "Ctrl+H: Help" hint; specifically modify the text passed to Paragraph::new (the footer variable) where it's currently "Tab: Switch field | Enter: Load & decrypt | Esc: Clear error | Ctrl+C: Clear all" to append " | Ctrl+H: Help" (or similar wording), then keep calling f.render_widget(footer, input_chunks[2]) as before so the new hint is rendered.
125-155: Optional: Extract focus styling helper to reduce duplication.The focus-aware border/title styling logic is nearly identical for both input fields. This could be extracted into a small helper function, though the current approach is readable and acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tabs/observer_tab.rs` around lines 125 - 155, The file uses duplicated logic to set file_border/file_title_style and key_border/key_title_style based on app.observer_focus compared to ObserverFocus::FilePath and ObserverFocus::SharedKey; extract this into a small helper (e.g., a function like focus_styles(current: ObserverFocus, target: ObserverFocus) -> (Style, Style)) that returns the focused Style pair (PRIMARY_COLOR + BOLD) or the unfocused gray pair, then replace the two duplicated blocks to call that helper for ObserverFocus::FilePath and ObserverFocus::SharedKey respectively to eliminate repetition and keep behavior identical.src/ui/key_handler/enter_handlers.rs (1)
503-505: Avoid blocking file/decrypt work directly in the Enter handler.Line 503 (
std::fs::read) and Line 504 (decrypt_blob) are synchronous in the input path. On slow disks or oversized files, this can freeze the TUI event loop. Consider offloading this to a background task with a short-lived loading mode/state.Also applies to: 522-531
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/enter_handlers.rs` around lines 503 - 505, The Enter handler is performing synchronous file I/O and decryption (std::fs::read and crate::util::blossom::decrypt_blob) which can block the TUI; move that work into a background task (e.g., tokio::spawn_blocking or a dedicated thread pool) and update the UI state to a short-lived "loading" mode while the task runs; once the background task returns, send the result back to the main event loop to handle success/error and resume normal UI state. Apply the same refactor to the other synchronous block around lines 522–531 so all file reads and decrypt_blob calls run off the main/UI thread and the Enter handler only triggers the background job and toggles loading state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@create_pr.sh`:
- Around line 6-8: The script hardcodes the PR head with the string "--head
file-attachment"; change it to accept a head branch dynamically by using a
variable or parameter (e.g., an environment variable like GITHUB_HEAD_REF or a
script arg/variable that falls back to the current git branch via git rev-parse)
and substitute that variable where "--head file-attachment" appears so PRs are
created from the actual branch rather than a fixed name.
In `@docs/ADMIN_DISPUTES.md`:
- Around line 50-52: Update the docs text entries "Header (8 lines)" and "Footer
(1 line)" to match current UI behavior: change "Header (8 lines)" to "Header (7
lines)" and update "Footer (1 line)" to indicate it can render as "Footer (1–2
lines) (width-aware; always includes Ctrl+H)". Locate and replace the literal
strings "Header (8 lines)" and "Footer (1 line)" in the ADMIN_DISPUTES.md
content so the Empty State and footer descriptions reflect the 7-row header and
the optional 2-line footer on wide terminals.
In `@docs/STARTUP_AND_CONFIG.md`:
- Line 55: The docs claim init_settings() returns user-friendly
OperationResult::Error on failure but the implementation uses multiple
panic-inducing .expect() calls (e.g., in init_settings: ".expect(\"Could not
find home directory\")", ".expect(\"The configuration directory could not be
created\")", ".expect(\"Could not copy default settings.toml\")",
".expect(\"settings.toml malformed\")", ".expect(\"Error deserializing
settings.toml\")"), so either update the docs or refactor init_settings to
return Result/OperationResult::Error instead of panicking: replace each
.expect() with proper error handling that maps IO/deserialize errors into
OperationResult::Error (or a dedicated error enum), propagate failures to the
caller (use ? or map_err) and ensure callers handle the Result and convert to
OperationResult::Error for user-facing messages; update tests/docs accordingly
if you choose the refactor route.
In `@docs/TUI_INTERFACE.md`:
- Around line 113-120: The docs snippet for the enum UiMode is out of sync: it
documents HelpPopup(Tab) but the real enum variant is HelpPopup(Tab,
Box<UiMode>) which keeps the previous mode to restore on close; update the
UiMode enum example in docs/TUI_INTERFACE.md so the HelpPopup variant signature
matches the code (use HelpPopup(Tab, Box<UiMode>)) and add a short comment
describing the second field as the stored/restore mode to avoid future
confusion.
In `@src/ui/app_state.rs`:
- Line 25: The UiMode::HelpPopup enum variant declaration (HelpPopup(Tab,
Box<UiMode>)) has inconsistent spacing/comment layout causing rustfmt/CI
failure; open the UiMode declaration (look for the HelpPopup variant) and run
rustfmt or manually normalize the spacing and trailing comment so it matches the
project's formatting style (align spaces and the inline comment "Context-aware
shortcuts (Ctrl+H); 2nd = mode to restore on close"), then commit the formatted
change.
In `@src/ui/constants.rs`:
- Line 6: The help close hint constant HELP_CLOSE_HINT currently omits a
supported shortcut; update the string value of HELP_CLOSE_HINT to include
"Ctrl+H" (e.g., "Esc, Enter or Ctrl+H to close") so the displayed hint matches
the actual supported close shortcuts in the UI.
In `@src/ui/key_handler/mod.rs`:
- Around line 359-392: The observer input branch is processing KeyCode::Char and
KeyCode::Backspace even when a modal result popup (UiMode::OperationResult) is
active; update the guard around the Tab::Admin(AdminTab::Observer) block to only
run when the UI mode allows editing (e.g., ensure app.mode is not
UiMode::OperationResult or explicitly check for editable modes) before
inspecting key_event and mutating app.observer_file_path_input /
app.observer_shared_key_input based on app.observer_focus
(ObserverFocus::FilePath / ObserverFocus::SharedKey); keep the existing logic
for handling CONTROL, KeyCode::Char, and KeyCode::Backspace but skip it when the
popup mode is active.
In `@src/ui/tabs/disputes_in_progress_tab.rs`:
- Around line 759-773: The rustfmt failure is caused by the multi-line format!
call that builds the footer strings (the branch that formats "{} | {} | {} | {}
| {}" with HELP_KEY, FOOTER_TAB_SWITCH_PARTY, FOOTER_SHIFT_F_RESOLVE,
filter_hint, FOOTER_UP_DOWN_SELECT_DISPUTE and the subsequent format! using
FOOTER_PGUP_PGDN_SCROLL_CHAT, FOOTER_END_BOTTOM, ctrl_s_hint); reformat that
block to follow rustfmt style (each argument on its own line or properly
aligned, ensure trailing commas where appropriate and consistent indentation) so
the two format! calls compile with standard formatting rules while keeping the
same literals and variable order.
---
Nitpick comments:
In `@src/main.rs`:
- Line 186: Replace the hardcoded interval(Duration::from_secs(2)) used to
create admin_chat_interval with a configurable setting: add a new setting/field
(e.g., admin_chat_poll_interval_secs or admin_chat_poll_interval) to your
configuration/Settings struct or CLI options with a sane default (2 seconds),
parse/validate it at startup, and construct admin_chat_interval using
Duration::from_secs(config.admin_chat_poll_interval_secs.unwrap_or(2)) (or
Duration::from(config.admin_chat_poll_interval)). Update any config
loading/Defaults and docs so operators can tune the poll interval; locate the
change around the admin_chat_interval binding where
interval(Duration::from_secs(2)) is called.
In `@src/ui/key_handler/enter_handlers.rs`:
- Around line 503-505: The Enter handler is performing synchronous file I/O and
decryption (std::fs::read and crate::util::blossom::decrypt_blob) which can
block the TUI; move that work into a background task (e.g.,
tokio::spawn_blocking or a dedicated thread pool) and update the UI state to a
short-lived "loading" mode while the task runs; once the background task
returns, send the result back to the main event loop to handle success/error and
resume normal UI state. Apply the same refactor to the other synchronous block
around lines 522–531 so all file reads and decrypt_blob calls run off the
main/UI thread and the Enter handler only triggers the background job and
toggles loading state.
In `@src/ui/tabs/observer_tab.rs`:
- Line 169: The label for the shared key is ambiguous; update the Span::styled
call that creates key_title (the line with let key_title = Span::styled("Shared
key (32-byte hex)", key_title_style);) to use clearer wording such as "Shared
key (64-character hex)" so users understand they must enter 64 hex characters
rather than interpreting byte length.
- Around line 178-181: Update the footer string created in the observer_tab.rs
UI so it includes the "Ctrl+H: Help" hint; specifically modify the text passed
to Paragraph::new (the footer variable) where it's currently "Tab: Switch field
| Enter: Load & decrypt | Esc: Clear error | Ctrl+C: Clear all" to append " |
Ctrl+H: Help" (or similar wording), then keep calling f.render_widget(footer,
input_chunks[2]) as before so the new hint is rendered.
- Around line 125-155: The file uses duplicated logic to set
file_border/file_title_style and key_border/key_title_style based on
app.observer_focus compared to ObserverFocus::FilePath and
ObserverFocus::SharedKey; extract this into a small helper (e.g., a function
like focus_styles(current: ObserverFocus, target: ObserverFocus) -> (Style,
Style)) that returns the focused Style pair (PRIMARY_COLOR + BOLD) or the
unfocused gray pair, then replace the two duplicated blocks to call that helper
for ObserverFocus::FilePath and ObserverFocus::SharedKey respectively to
eliminate repetition and keep behavior identical.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
Cargo.tomlPR_DESCRIPTION.mdcreate_pr.shdocs/ADMIN_DISPUTES.mddocs/CODING_STANDARDS.mddocs/STARTUP_AND_CONFIG.mddocs/TUI_INTERFACE.mdsrc/main.rssrc/ui/app_state.rssrc/ui/constants.rssrc/ui/draw.rssrc/ui/help_popup.rssrc/ui/helpers.rssrc/ui/key_handler/admin_handlers.rssrc/ui/key_handler/chat_helpers.rssrc/ui/key_handler/confirmation.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/esc_handlers.rssrc/ui/key_handler/message_handlers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/navigation.rssrc/ui/key_handler/user_handlers.rssrc/ui/mod.rssrc/ui/navigation.rssrc/ui/operation_result.rssrc/ui/orders.rssrc/ui/state.rssrc/ui/tabs/disputes_in_progress_tab.rssrc/ui/tabs/mod.rssrc/ui/tabs/observer_tab.rssrc/util/blossom.rssrc/util/dm_utils/order_ch_mng.rssrc/util/order_utils/helper.rssrc/util/order_utils/send_new_order.rssrc/util/order_utils/take_order.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/app_state.rs`:
- Around line 71-80: When switching roles, clear any sensitive/transient
observer state so secrets don't linger: in the switch_role implementation(s)
reset observer_shared_key_input to an empty string, clear or replace
observer_chat_lines with an empty Vec, set observer_error to None, and clear
observer_file_path_input and reset observer_focus to its default/neutral
variant; apply the same resets in the other switch_role path mentioned (lines
118-131) so both role-switch code paths purge observer state.
In `@src/ui/tabs/disputes_in_progress_tab.rs`:
- Around line 740-756: The footer string built in the branch for when
is_input_focused && !is_input_enabled omits the attachment save hint; update
that tuple's first formatted string (currently using HELP_KEY,
FOOTER_TAB_SWITCH_PARTY, FOOTER_SHIFT_I_ENABLE, FOOTER_SHIFT_F_RESOLVE,
filter_hint) to also include the Ctrl+S save hint constant used in other
non-finalized branches (e.g., the same FOOTER_CTRL_S_SAVE_ATTACH/FOOTER_CTRL_S
constant used elsewhere), so the first format! call contains HELP_KEY,
FOOTER_TAB_SWITCH_PARTY, FOOTER_SHIFT_I_ENABLE, FOOTER_SHIFT_F_RESOLVE, the
Ctrl+S hint, and filter_hint.
mostronatorcoder bot
pushed a commit
that referenced
this pull request
Feb 26, 2026
Major fixes: - Clear observer sensitive state on role switch (file_path, shared_key, chat_lines, error, focus) - Block Observer text editing when modal popup is active (check UiMode before processing input) - Make create_pr.sh accept dynamic head branch and title parameters Minor fixes: - Update HELP_CLOSE_HINT to include Ctrl+H - Add ctrl_s_hint to input-disabled footer branch - Fix docs: Header (8→7 lines), Footer (1→1-2 lines) - Fix docs: Startup error handling uses .expect() currently - Fix docs: HelpPopup signature includes Box<UiMode> for restore mode
arkanoider
commented
Feb 27, 2026
Contributor
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)
docs/STARTUP_AND_CONFIG.md (1)
16-49:⚠️ Potential issue | 🟠 MajorDocumentation code block is inconsistent with actual implementation and won't compile.
The embedded code snippet shows a hybrid of old and new patterns that doesn't match
src/settings.rs:
- Wrong signature (line 17): Shows
-> &'static Settingsbut actual code returnsResult<&'static Settings, anyhow::Error>- Wrong initialization pattern (line 18): Shows
SETTINGS.get_or_init(|| { ... })but actual code usesSETTINGS.get()+SETTINGS.set()pattern- Mixed error handling: Lines 20, 32, 37 still use
.expect()while lines 44, 47 use.map_err(...)?- Won't compile: Using
?inside aget_or_initclosure that returnsSettings(notResult) is invalidPlease update the code block to reflect the actual implementation from
src/settings.rs:40-57.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/STARTUP_AND_CONFIG.md` around lines 16 - 49, Update the documentation code block to match the real implementation in src/settings.rs: use the actual signature init_settings() -> Result<&'static Settings, anyhow::Error>, the SETTINGS.get() / SETTINGS.set() pattern (not SETTINGS.get_or_init), and the same error handling used in the source (i.e., return Result with map_err/? where appropriate rather than .expect() inside init logic); ensure the snippet shows creating hidden_dir, copying default_file, building config with config::Config::builder().add_source(...).build().map_err(...)? and then cfg.try_deserialize::<Settings>().map_err(...), matching the code in init_settings and the unique symbols SETTINGS and init_settings.
🧹 Nitpick comments (2)
src/ui/key_handler/mod.rs (1)
370-376: Minor: Use importedObserverFocusfor consistency.The full path
crate::ui::tabs::observer_tab::ObserverFocusis used multiple times here. SinceObserverFocusisn't imported in this file, consider adding it to the imports at line 16 for cleaner code.♻️ Suggested change
Add to imports:
use crate::ui::tabs::observer_tab::ObserverFocus;Then simplify the match arms:
KeyCode::Char(c) => { match app.observer_focus { - crate::ui::tabs::observer_tab::ObserverFocus::FilePath => { + ObserverFocus::FilePath => { app.observer_file_path_input.push(c); } - crate::ui::tabs::observer_tab::ObserverFocus::SharedKey => { + ObserverFocus::SharedKey => { app.observer_shared_key_input.push(c); } }Also applies to: 380-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/mod.rs` around lines 370 - 376, The match arms are using the fully-qualified type crate::ui::tabs::observer_tab::ObserverFocus repeatedly; add a local import for ObserverFocus (use crate::ui::tabs::observer_tab::ObserverFocus;) near the top imports and then replace the fully-qualified references in the match arms (and the similar block at lines ~380-388) with the shorter ObserverFocus::FilePath and ObserverFocus::SharedKey to keep the code consistent and concise.src/ui/app_state.rs (1)
110-114: Minor: Use importedObserverFocusinstead of full path.Line 112 uses the full path
crate::ui::tabs::observer_tab::ObserverFocus::FilePath, butObserverFocusis already imported at line 15. Consider using the shorter form for consistency with line 76.♻️ Suggested simplification
observer_file_path_input: String::new(), observer_shared_key_input: String::new(), - observer_focus: crate::ui::tabs::observer_tab::ObserverFocus::FilePath, + observer_focus: ObserverFocus::FilePath, observer_chat_lines: Vec::new(), observer_error: None,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/app_state.rs` around lines 110 - 114, Replace the fully-qualified enum path used in the observer_focus initializer with the imported name: change the initializer that sets observer_focus (currently using crate::ui::tabs::observer_tab::ObserverFocus::FilePath) to use the shorter ObserverFocus::FilePath so it matches the import at the top and the style used elsewhere (e.g., line 76).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/STARTUP_AND_CONFIG.md`:
- Around line 16-49: Update the documentation code block to match the real
implementation in src/settings.rs: use the actual signature init_settings() ->
Result<&'static Settings, anyhow::Error>, the SETTINGS.get() / SETTINGS.set()
pattern (not SETTINGS.get_or_init), and the same error handling used in the
source (i.e., return Result with map_err/? where appropriate rather than
.expect() inside init logic); ensure the snippet shows creating hidden_dir,
copying default_file, building config with
config::Config::builder().add_source(...).build().map_err(...)? and then
cfg.try_deserialize::<Settings>().map_err(...), matching the code in
init_settings and the unique symbols SETTINGS and init_settings.
---
Nitpick comments:
In `@src/ui/app_state.rs`:
- Around line 110-114: Replace the fully-qualified enum path used in the
observer_focus initializer with the imported name: change the initializer that
sets observer_focus (currently using
crate::ui::tabs::observer_tab::ObserverFocus::FilePath) to use the shorter
ObserverFocus::FilePath so it matches the import at the top and the style used
elsewhere (e.g., line 76).
In `@src/ui/key_handler/mod.rs`:
- Around line 370-376: The match arms are using the fully-qualified type
crate::ui::tabs::observer_tab::ObserverFocus repeatedly; add a local import for
ObserverFocus (use crate::ui::tabs::observer_tab::ObserverFocus;) near the top
imports and then replace the fully-qualified references in the match arms (and
the similar block at lines ~380-388) with the shorter ObserverFocus::FilePath
and ObserverFocus::SharedKey to keep the code consistent and concise.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/ADMIN_DISPUTES.mddocs/STARTUP_AND_CONFIG.mddocs/TUI_INTERFACE.mdsrc/main.rssrc/settings.rssrc/ui/app_state.rssrc/ui/constants.rssrc/ui/key_handler/mod.rssrc/ui/tabs/disputes_in_progress_tab.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main.rs
- src/ui/constants.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/ui/key_handler/mod.rs (1)
362-391:⚠️ Potential issue | 🟠 MajorGate Observer typing to explicit editable modes (not just
!OperationResult).Line [363] only excludes
UiMode::OperationResult(_). If another modal mode is active on Observer,Char/Backspacecan still mutate background inputs and intercept modal keystrokes.Suggested guard tightening
if let Tab::Admin(AdminTab::Observer) = app.active_tab { - if !matches!(app.mode, UiMode::OperationResult(_)) { + let observer_inputs_editable = matches!( + app.mode, + UiMode::Normal | UiMode::AdminMode(AdminMode::Normal) + ); + if observer_inputs_editable { let is_ctrl = key_event .modifiers .contains(crossterm::event::KeyModifiers::CONTROL); if !is_ctrl { match code {#!/bin/bash # Verify whether tab switching/mode transitions can keep modal UiMode active while on Observer tab. rg -n -C4 "KeyCode::Tab|handle_tab_navigation" src/ui/key_handler/mod.rs fd navigation.rs --exec rg -n -C6 "pub fn handle_tab_navigation|match app.mode|UiMode::" {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/mod.rs` around lines 362 - 391, The current guard in the Observer key-handling branch (the if that checks Tab::Admin(AdminTab::Observer) and then !matches!(app.mode, UiMode::OperationResult(_))) is too permissive and allows Char/Backspace to mutate app.observer_file_path_input and app.observer_shared_key_input even when modal UiModes other than OperationResult are active; change the negative check into an explicit whitelist: only allow typing/backspace when app.mode matches the concrete editable modes you intend (e.g., UiMode::Normal or the specific Observer-editing mode(s) used in your app) rather than any mode that is not OperationResult; update the condition surrounding the match on KeyCode::Char/Backspace accordingly so that modifications to app.observer_file_path_input and app.observer_shared_key_input happen only when app.mode is one of those allowed variants (adjust pattern matches to use matches! or if let to match the exact UiMode variants you have).
🧹 Nitpick comments (1)
docs/STARTUP_AND_CONFIG.md (1)
28-28: Consider eliminating remaining.expect()calls for fully panic-free initialization.While these
.expect()calls are safe in the current control flow (lines 28 and 31 rely onSETTINGSbeing set either by the current thread or a racing thread), they still represent latent panic points if the initialization logic changes. For truly panic-free error propagation, consider returning early with an explicit error instead.♻️ Defensive alternative
// Safe to unwrap because we know some value is now present. - return Ok(SETTINGS.get().expect("SETTINGS should be initialized")); + return SETTINGS.get().ok_or_else(|| anyhow::anyhow!("SETTINGS initialization race condition")); } -Ok(SETTINGS.get().expect("SETTINGS should be initialized")) +SETTINGS.get().ok_or_else(|| anyhow::anyhow!("SETTINGS was not set"))Also applies to: 31-31
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/STARTUP_AND_CONFIG.md`:
- Line 72: The docs claim settings failures are reported "via
OperationResult::Error" but the code wraps and propagates errors with
anyhow::anyhow! from init_settings(), so either update the documentation to
state that init_settings() errors are propagated as anyhow::Error (reflecting
the current behavior), or change the call site that invokes init_settings() to
convert the anyhow error into an OperationResult::Error (e.g., map or match the
Result from init_settings() and return OperationResult::Error with the error
message) so the implementation matches the docs; locate the init_settings()
invocation and adjust either the docs or the error conversion accordingly.
In `@src/ui/app_state.rs`:
- Around line 73-80: The observer fields in AppState (observer_shared_key_input,
observer_chat_lines, observer_error, observer_focus) hold sensitive
plaintext/keys and must be zeroized rather than merely cleared; add a single
method on AppState (e.g., zeroize_observer_state or clear_observer_secrets) that
uses the Zeroize trait (from the zeroize crate) to overwrite
observer_shared_key_input's bytes and each String in observer_chat_lines before
clearing them and then reset observer_error and observer_focus as needed,
replace all scattered manual clear() calls (in switch_role, navigation.rs,
key_handler/mod.rs, enter_handlers.rs) to call this new central method so wiping
is consistent and guaranteed.
---
Duplicate comments:
In `@src/ui/key_handler/mod.rs`:
- Around line 362-391: The current guard in the Observer key-handling branch
(the if that checks Tab::Admin(AdminTab::Observer) and then !matches!(app.mode,
UiMode::OperationResult(_))) is too permissive and allows Char/Backspace to
mutate app.observer_file_path_input and app.observer_shared_key_input even when
modal UiModes other than OperationResult are active; change the negative check
into an explicit whitelist: only allow typing/backspace when app.mode matches
the concrete editable modes you intend (e.g., UiMode::Normal or the specific
Observer-editing mode(s) used in your app) rather than any mode that is not
OperationResult; update the condition surrounding the match on
KeyCode::Char/Backspace accordingly so that modifications to
app.observer_file_path_input and app.observer_shared_key_input happen only when
app.mode is one of those allowed variants (adjust pattern matches to use
matches! or if let to match the exact UiMode variants you have).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ui/key_handler/enter_handlers.rs (1)
456-534: Consider extracting observer error handling to reduce duplication.The pattern of
clear_observer_secrets() → set observer_error → set OperationResult::Erroris repeated five times. A helper method could reduce duplication and ensure consistency.♻️ Optional refactor suggestion
impl AppState { fn set_observer_error(&mut self, msg: String) { self.clear_observer_secrets(); self.observer_error = Some(msg.clone()); self.mode = UiMode::OperationResult(crate::ui::OperationResult::Error(msg)); } }Then each error site becomes:
app.set_observer_error("Both file path and shared key are required".to_string()); return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/enter_handlers.rs` around lines 456 - 534, The duplicated pattern clearing secrets and setting an observer error should be extracted into a helper on the AppState (e.g., impl AppState { fn set_observer_error(&mut self, msg: String) { ... } }) so all sites call app.set_observer_error(...) instead of repeating clear_observer_secrets(), assigning observer_error, and setting mode = UiMode::OperationResult(crate::ui::OperationResult::Error(msg)); implement set_observer_error to call clear_observer_secrets(), set observer_error = Some(msg.clone()), and set mode accordingly, then replace the five error sites in enter_handlers.rs with calls to this helper followed by return where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/key_handler/enter_handlers.rs`:
- Around line 468-534: The local secret material (the stack array key_bytes and
the temporary keys value from keys_from_shared_hex) must be zeroized on every
exit path after decryption/reading to avoid leaving secrets in memory. Fix by
limiting the lifetime of keys (put the match that produces keys in its own block
scope) or explicitly zero/overwrite key_bytes (e.g., key_bytes.fill(0)) and call
the secret type's zeroize/clear method (or overwrite its secret_bytes)
immediately after decrypt_blob returns or on all error branches; ensure this
happens before returning or setting app state (you can reuse
clear_observer_secrets() timing pattern) so both key_bytes and the keys' secret
are wiped on success and on each Err branch. Reference symbols:
keys_from_shared_hex, key_bytes, keys.secret_key(), decrypt_blob, and
clear_observer_secrets().
---
Nitpick comments:
In `@src/ui/key_handler/enter_handlers.rs`:
- Around line 456-534: The duplicated pattern clearing secrets and setting an
observer error should be extracted into a helper on the AppState (e.g., impl
AppState { fn set_observer_error(&mut self, msg: String) { ... } }) so all sites
call app.set_observer_error(...) instead of repeating clear_observer_secrets(),
assigning observer_error, and setting mode =
UiMode::OperationResult(crate::ui::OperationResult::Error(msg)); implement
set_observer_error to call clear_observer_secrets(), set observer_error =
Some(msg.clone()), and set mode accordingly, then replace the five error sites
in enter_handlers.rs with calls to this helper followed by return where needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomldocs/STARTUP_AND_CONFIG.mdsrc/ui/app_state.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/navigation.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ui/key_handler/navigation.rs
- Cargo.toml
Collaborator
Author
|
@coderabbitai please review! |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
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)
src/util/chat_utils.rs (1)
53-65:⚠️ Potential issue | 🔴 CriticalFix admin chat message format to match Mostro protocol specification.
The send path creates a plain Nostr
text_noteevent, but Mostro protocol specifies that dispute/admin chat messages must be JSON-serialized with structure[{ "dm": { "version": 1, "action": "send-dm", "payload": { "text_message": "..." } } }, null]. This breaks interoperability with protocol-compliant clients that will fail to parse or display messages correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/chat_utils.rs` around lines 53 - 65, build_custom_wrap_event currently creates a plain text_note from message (via EventBuilder::text_note) which violates Mostro DM format; instead construct the Mostro JSON DM array [{ "dm": { "version": 1, "action": "send-dm", "payload": { "text_message": "<message>" } } }, null], serialize it to a string (use serde_json to build objects to avoid manual escaping), and pass that serialized JSON string as the event content (replace the EventBuilder::text_note(message) call with EventBuilder::text_note(&json_string)); keep the subsequent .build(sender.public_key()).sign(sender).await? flow unchanged so signing and public_key usage remain the same.
🧹 Nitpick comments (2)
src/ui/save_attachment_popup.rs (1)
33-89: Add windowed rendering for long attachment lists.When attachments exceed visible popup rows, the selected item can move out of view. Rendering only a visible slice around
selected_idxkeeps navigation predictable.♻️ Suggested refactor
- let content: Vec<Line> = list - .iter() - .enumerate() - .map(|(i, msg)| { + let visible_rows = popup_height.saturating_sub(4) as usize; // borders + hint + let start = selected_idx.saturating_sub(visible_rows.saturating_sub(1)); + let end = (start + visible_rows).min(list.len()); + + let content: Vec<Line> = list[start..end] + .iter() + .enumerate() + .map(|(offset, msg)| { + let i = start + offset; let att = msg.attachment.as_ref().unwrap(); let icon = match att.file_type { ChatAttachmentType::Image => "🖼", ChatAttachmentType::File => "📎", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/save_attachment_popup.rs` around lines 33 - 89, The popup currently renders all items (building content from list) which lets the selected index scroll out of view; change rendering to compute a visible window slice around selected_idx using popup_height (or popup_height-4 header/foot padding) and only map/display that slice so navigation keeps the selected item centered when possible; adjust how you compute the displayed index vs selected_idx (e.g. window_start = selected_idx.saturating_sub(half_window)) and use that offset when styling (compare i to selected_idx) and when building all/paragraph so f.render_widget(paragraph, inner) only receives the visible lines plus the hint line.src/ui/key_handler/chat_helpers.rs (1)
68-76: Scroll sync depends on staleline_startsfrom previous render.The scroll position sync relies on
admin_chat_line_startspopulated during the last render pass. If the message list changed (e.g., new message arrived) since the last render, the index-to-line mapping may be stale, causing the scroll offset to be incorrect until the next frame. This is likely acceptable for typical use but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/chat_helpers.rs` around lines 68 - 76, The scroll sync can use stale admin_chat_line_starts (populated in the last render), so update the selection logic in the handler that sets admin_chat_selected_message_idx to either recompute or validate line starts before using them: clamp new_selection against app.admin_chat_line_starts.len(), and if the message list or layout may have changed, call or schedule the same routine that computes admin_chat_line_starts (or mark a re-layout) so you get fresh offsets, then set admin_chat_scrollview_state.set_offset(...) using the recomputed line_start; ensure this uses the same symbols (admin_chat_selected_message_idx, admin_chat_line_starts, admin_chat_scrollview_state) and avoids indexing into stale data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/util/chat_utils.rs`:
- Around line 53-65: build_custom_wrap_event currently creates a plain text_note
from message (via EventBuilder::text_note) which violates Mostro DM format;
instead construct the Mostro JSON DM array [{ "dm": { "version": 1, "action":
"send-dm", "payload": { "text_message": "<message>" } } }, null], serialize it
to a string (use serde_json to build objects to avoid manual escaping), and pass
that serialized JSON string as the event content (replace the
EventBuilder::text_note(message) call with
EventBuilder::text_note(&json_string)); keep the subsequent
.build(sender.public_key()).sign(sender).await? flow unchanged so signing and
public_key usage remain the same.
---
Nitpick comments:
In `@src/ui/key_handler/chat_helpers.rs`:
- Around line 68-76: The scroll sync can use stale admin_chat_line_starts
(populated in the last render), so update the selection logic in the handler
that sets admin_chat_selected_message_idx to either recompute or validate line
starts before using them: clamp new_selection against
app.admin_chat_line_starts.len(), and if the message list or layout may have
changed, call or schedule the same routine that computes admin_chat_line_starts
(or mark a re-layout) so you get fresh offsets, then set
admin_chat_scrollview_state.set_offset(...) using the recomputed line_start;
ensure this uses the same symbols (admin_chat_selected_message_idx,
admin_chat_line_starts, admin_chat_scrollview_state) and avoids indexing into
stale data.
In `@src/ui/save_attachment_popup.rs`:
- Around line 33-89: The popup currently renders all items (building content
from list) which lets the selected index scroll out of view; change rendering to
compute a visible window slice around selected_idx using popup_height (or
popup_height-4 header/foot padding) and only map/display that slice so
navigation keeps the selected item centered when possible; adjust how you
compute the displayed index vs selected_idx (e.g. window_start =
selected_idx.saturating_sub(half_window)) and use that offset when styling
(compare i to selected_idx) and when building all/paragraph so
f.render_widget(paragraph, inner) only receives the visible lines plus the hint
line.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/ADMIN_DISPUTES.mddocs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/TUI_INTERFACE.mdsrc/ui/app_state.rssrc/ui/constants.rssrc/ui/draw.rssrc/ui/helpers.rssrc/ui/key_handler/chat_helpers.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/esc_handlers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/navigation.rssrc/ui/mod.rssrc/ui/save_attachment_popup.rssrc/util/chat_utils.rs
✅ Files skipped from review due to trivial changes (1)
- docs/MESSAGE_FLOW_AND_PROTOCOL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/key_handler/navigation.rs
- src/ui/mod.rs
- src/ui/constants.rs
Catrya
reviewed
Mar 4, 2026
Member
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
It works very well. great work! Just a detail: in the the Observer mode is missing instructions on how to paste the shared key using ctrl shift v, also in the help info
Made-with: Cursor
Collaborator
Author
They are coming! will try to add also mouse right key |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds an Observer tab for admins to inspect Blossom-encrypted chat files offline, generalizes the operation-result popup, and improves the Disputes-in-Progress UI with a context-aware help popup, a two-line footer on wide terminals, a more compact header, a Save Attachment popup (Ctrl+S), and consistent behavior when dismissing the operation result from Disputes in Progress.
Changes
Observer tab (Admin)
~/.mostrix/downloads) with a 64-char hex shared key.blossomhelpers; decrypted chat shown in a scrollable preview.Operation result rename and behavior
OrderResult→OperationResult: Type and module renamed so the same popup is used for order outcomes, settings errors, admin actions, Blossom downloads, and Observer file/key errors.Ctrl+H help popup and constants
HelpPopup(Tab, Box<UiMode>)stores the mode when opening; closing restores it so e.g. chat input stays enabled in Disputes in Progress.src/ui/constants.rsfor help titles, footer hints, filter labels; used by the help popup and by the disputes footer.Save Attachment popup (Disputes in Progress)
UiMode::SaveAttachmentPopup(usize)andsrc/ui/save_attachment_popup.rs; helperget_visible_attachment_messages(app, dispute_id_key)for the list; key handler opens popup and handles Up/Down/Enter/Esc; Enter/Esc and navigation handle the new mode. ConstantsHELP_DIP_CTRL_S_ATTACH,SAVE_ATTACHMENT_POPUP_HINT. Doc comments and docs (ADMIN_DISPUTES, TUI_INTERFACE, MESSAGE_FLOW_AND_PROTOCOL) updated.Disputes in Progress UI
Docs
UiMode::HelpPopup,UiMode::SaveAttachmentPopup, operation result and help popup overlays, Save Attachment popup, stay-on-tab behavior for Disputes in Progress, UI constants, Ctrl+H, key handler layout.OperationResulttype name and examples.Breaking changes
None. Observer is additive; OperationResult is an internal rename; footer/header are layout-only; Save Attachment popup replaces the previous “save selected message” behavior with an explicit list selection.
Checklist
cargo test)cargo clippy)cargo fmt)Base branch
Target:
main(or repo default).Summary by CodeRabbit
New Features
Improvements
Chores