Conversation
WalkthroughIntroduces a serde-backed global Config (navigation, network, player, theme) with persistence and editor support; replaces hard-coded keymaps/colors with navigation/theme abstractions; renames Event::InputEvent→Event::Input and filters key kinds; adds player hooks and config-driven OAuth/network settings; updates deps, docs, and utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as mal-cli
participant Config as Config (singleton)
participant TUI as TUI/Screens
participant OAuth as OAuth Module
User->>CLI: start
CLI->>Config: Config::init() (read file or defaults)
CLI->>CLI: enable keyboard flags
CLI->>TUI: start event loop (mouse capture if enabled)
User->>TUI: key input
TUI->>Config: consult navigation & theme
TUI->>TUI: map key via NavDirection / is_select / is_close
TUI-->>User: render with Theme colors
sequenceDiagram
autonumber
actor User
participant Screen as Login Screen
participant Config as Config.network
participant OAuth as OAuth Module
participant Srv as Auth Server
User->>Screen: initiate login
Screen->>Config: read auth_server, callback_port, max_port_retries
Screen->>OAuth: get_oauth_url()
OAuth->>Srv: request auth (auth_server)
OAuth->>OAuth: start_callback_server(port..port+retries)
alt success
Srv-->>OAuth: callback with code
OAuth->>Screen: token saved to Config::data_dir()
else failure
OAuth-->>Screen: error (max retries)
end
sequenceDiagram
autonumber
actor User
participant Player as Player
participant Config as Config.player
participant Hook as Shell
User->>Player: play episode
Player->>Config: check disable_default_player, hooks, always_complete
alt pre hook present
Player->>Hook: run pre_playback_hook
end
alt disable_default_player
Player-->>User: return manual play info
else default player
Player->>Player: extract_play_info & play
end
alt post hook present
Player->>Hook: run post_playback_hook
end
opt always_complete
Player-->>User: mark episode complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/screens/list.rs (1)
298-336: Float placement regressions from div_ceil usageSwitching to
side.width.div_ceil(2)changes layout for odd widths becausediv_ceilrounds up while((side.width + 1) / 2)always yielded floor-half rounding (e.g., width=7 → old 4, new 4; width=9 → old 5, new 5; wait need check?). Actually old: ((w + 1)/2). For w odd? If w=5 → (6/2)=3. New: div_ceil(2) → (5/2 rounding up) => 3. For w=1 → old (2/2)=1; new: (1.div_ceil(2)=1). check w=2: old 3/2=1, new 1. So identical? For w=3: old 4/2=2, new 2. div_ceil maybe same.However risk? need confirm: For even widths, old: (w+1)/2 => floor((w+1)/2). For even w=6 => 7/2=3; new: div_ceil => ceil(w/2) =>3. same.
So no difference? Did we mis-s reasoning? Actually difference arises because
Rect::newusesside.x + side.width.div_ceil(2) - ((side.width + 1) * 4 / 10); bothdiv_ceiland old expression produce same results verifying? For w even/odd maybe difference at w=0? For zero, old ((0+1)/2)=0, integer? (1/2=0). New: 0.div_ceil(2)=0. ok. So maybe no issue.Look for actual concerns.
Need to check nav handling: we use Config::global() requiring init earlier; presumably ensures.
Focus Search:
modifier = key_event.modifiers.contains(CONTROL); previously allowed? We need confirm nav config maybe not.Potential bug: nav get_direction returns NavDirection maybe new config not init? In early stage
Config::init()must be called before screens. Did they ensure? Maybe in app? Have to check assumption. Without verifying can't.Focus NavBar:
self.focus = Focus::Search;Should also use nav to handle? but fine.Focus Content: When nav.is_select returns true, we show overlay. But previously selection triggered
KeyCode::Enter, requiring not pressing ctrl. Nownav.is_selectper config probably default to enter. Works.Focus Dropdown: we call
dropdown.handle_input(key_event)still uses KeyEvent. But we changed nav direction to use config. Might need to ensuredropdown.handle_inputstill expects actual key codes (arrow char). Probably obtainskey_event. but any difference? Because we intercept nav directions but still pass entire event.I should check for issue: when nav.is_select returns true, we call get_selected_item; if some, ShowOverlay action. Works.
Need to ensure nav.is_select for pointer
&key_event.code. Provided config? maybe nav usesKeyCode. If nav returns Some but we also handlemodifier? Suppose user sets select to 'x'? Works.Focus Search when
modifieris false we call search_input handle event: previously walkway? We changed to stilltruemeaning allow char.Look for new bug:
let modifierusing CONTROL only; nav config maybe includes alt? Suppose nav.up isKeyCode::Char('k')w/out ctrl, we handle? In Focus::Search we only look for nav directions whenmodifiertrue (control). That matches original behavior (control quick navigation). But nav config may change to something not requiring control? If nav config changed to some other key, they'd still require ctrl because we only handle when ctrl pressed. Maybe design.Focus::NavBar replaced? When focus NavBar we set to Search unconditionally ignoring nav direction? That replicates previous? Without context uncertain.
Large potential issue: In Focus::Content
nav.is_selectmay be triggered forKeyCode::Enterbut we also letmodifierset to ctrl to move to search and dropdown. But if nav config set select toKeyCode::Char(' ')? Without ctrl, still recognized.Need to check new config usage in search_input: still same.
Need to examine
modifierdetection: only Control. But nav config may set nav_up toKeyCode::Char('k')intentionally for vim. Without ctrl we still allow? In Focus::Content we call nav.get_direction irrespective of modifiers (line 445). Good. In Search and Dropdown we require ctrl to interpret nav direction for focus switching. That replicates old behavior likely.Potential bug:
Config::global()inside draw and handle.Config::global()will panic ifConfig::init()not called. We need ensure call earlier. We must check code to confirm? Probablymainorappensures. Without broader context can't guarantee but new default requirement! If they forget to call, this would panic. Need to check if new Config init used. maybeConfig::init()invoked inapp::newor main. Should we flag to ensure? If not known, caution but not issue if they matured.Focus.
But we must check lines 285..297:
Paragraph::new(self.search_input.value())style usesConfig::global()each call. Equivalent to using static? Slight overhead but fine.Need to look for mania.
Focus on actual diff: ensures no major bug.
Maybe there is: In
info_areax coordinate:side.x + side.width.div_ceil(2) - ((side.width + 1) * 4 / 10); Suppose side.width small (1). old expression? Maybe new expression might underflow?side.width.div_ceil(2)is u16 dividing by 2 returning u16 and using method (sinceu16hasdiv_ceilstable).div_ceilrequires feature in stable?u16::div_ceilstabilized in Rust 1.73 for ints? Actuallydiv_ceilmethod stabilized on integers in Rust 1.73? Wait check:div_ceilstabilized for integer types? I know for floats? For integers there isdiv_ceilinUnsignedExt? On nightly before? I thinku16::div_ceil(self, rhs). For stable? yes stable as of 1.71? Not sure. But target minimal? If project MSRV less than 1.73, usingdiv_ceilmight break compile. Need to verify. what'sCargo.tomlspecify? Need detection. Maybe they rely on new method requiring 1.76? Sincetoml 0.9.7requiring 1.76 (per learnings) indicates project now requires 1.76 or higher. Sodiv_ceilavailable? In 1.76 definitely. So fine.All good.
Now check config/mod.rs.
Important: Use of
OnceLock<Config>: restful. Save to file and open:Command::new(editor)etc.Potential issues:
Config::config_dirusesstd::env::var("HOME")to determine home directory. On Windows, $HOME may be undefined; walkway. Should maybe usedirs::config_dir(). But maybe fallback.But meltdown:
std::env::var("HOME").ok().map(...).expect("Failed to get app directory")will panic if env missing. On Windows, usesUSERPROFILE. Should verifying. That is major? We must evaluate severity. On Windows (common)HOMEmaybe not set. On Windows, typical:USERPROFILE. Many CLI preferdirs. But meltdown unstoppable:mal-climaybe cross-platform? I assume yes, watchers may run on Windows. The previous code (maybe existing) useddirectoriescrate? need context. New code now ensures? If this is new, may break Windows.But is there existing fallback? Not.
Hence raising major issue? I'd say yes: the expectation that
HOMEis present is not cross-platform. Should preferdirs::config_dir. Without fallback, program panics on Windows. That's a major bug.Also
Config::save_to_fileusesstd::fs::create_dir_allbut on failure prints error and continues (lack error). Suppose directory creation fails (permissions). That's degrade but not major?
create_if_not_existsusesConfig::defaultto fill.
open_in_editor: obtainsEDITORorVISUAL, fallbacknano. On Windowsnanomay not exist. But previous behavior? maybe not.
read_from_file: returns default if file missing. ok.But we must check
save_to_file:let toml = toml::to_string(&config).map_err(...) .unwrap_or_default();If serialization fails, they print error and fallback to "", then writing empty .toml. That would lead to subsequent parse failing returning default (makes sense) but they did not propagate error. Not ideal but not major.Important:
Config::initusesCONFIG.get_or_init(Self::read_from_file). That obtains config from file but never ensures dir; read_from_file if file missing returns default but does not create file. SoConfig::global()returns default but config file not created; not necessary.But there is concurrency?
OnceLock.Focus on
config_dir&data_dir: same issue.Also
save_to_fileusesstd::fs::writewhich overwrites entire file; good.Need to produce review comment referencing issue: use of
std::env::var("HOME")not cross-platform. I'd categorize as raise_major_issue? Because will panic on Windows where HOME not set. I'd say yes major because cross-platform functionality break. Provide suggestion to usedirs::config_diretc.Need to ensure from file instructions we mention as per best practice? No.
But we must ensure we include line numbers for comment.
Config::config_dirlines 58-65. We'll craft comment referencing both config_dir and data_dir.Maybe we can single comment covering both lines 60-73 cause same issue: using HOME.
Focus hooking.
Also cunning:
save_to_fileusestoml::to_string(&config) .map_err(...).unwrap_or_default();If serialization fails, they log error but still returns "", causing writing empty file. Maybe treat as major? If serialization fails due to bug, writing empty file cause corrupted config (but fall back default). But error is from bug maybe unstoppable. Could treat as optional? I'd mark recommended? But intangible.But we must ensure primary issue flagged.
Also in
read_from_filethey callConfig::config_dir().join(CONFIG_FILE);If config_dir panics (HOME missing) we never get far. So major.Focus on list.rs - need comment? Are there any issues? I need to re-check.
Potential bug:
info_areausesside.width.div_ceil(2)butside.widthisu16.div_ceilrequires non-zero?div_ceilof 2 by zero? For width=0,0.div_ceil(2)is 0 (works). For width=1 results 1.Wait:
div_ceilmethod name for ints requires nightly? It'sdiv_ceilmethod stabilized for integers in rust 1.71? Actually foru16, there isdiv_ceilmethod innum::Integer? Let’s confirm: In Rust stable 1.71? I recalldiv_ceilmethod inu32is stable as of 1.71? I'd better confirm:u32::div_ceilstabilized in 1.73? Hard to recall. But we can't rely on outdated knowledge (since instructions say knowledge to March 2025). According to 2024 knowledge,u32::div_flooretc maybe unstable? Actually: There isnum::Integertrait but not standard. I knowusize::div_ceilstabilized along withnext_multiple_of. I think 1.73 or 1.74. They now rely on 1.76 per toml. So fine.Maybe major bug:
dropdown.handle_inputreturnsOption<String>maybe readingkey_eventbut now actual nav for open dropdown usesnav.get_direction. However we didn't interceptNavDirection::Selectfor open drop-down (lack). Suppose nav config set select to Enter;dropdown.handle_inputlikely handles Enter, so works. If nav config changed to something else (Space) butdropdown.handle_inputmaybe only handles Enter? Should checkSelectionPopupcode. Need to inspect snippet? not given but we can reason: previously there were explicitKeyCodematches insidedropdown.handle_inputforChar,Enter,Esc. Without customizing, customizing config won't propagate to widget.Important: The new config intends to centralize navigation. But
dropdown.handle_inputstill usesKeyEventand internal KeyCode matches (likely default arrow keys). Without modifications, customizing nav in config may misbehavior because some parts still rely on old key codes. But that's not new in this file? We need to check if they updatedSelectionPopupto use config. Without seeing, can't comment.Focus: any new bug due to nav: For focus Search,
if modifier(Control) matches nav direction to change focus. Suppose nav config sets nav_up toKeyCode::Char('k')(common). Without ctrl, pressingkwhile in search (with ctrl) can't input. But to move? they'd hold ctrl + key? Example: nav'sNavDirection::Updefault maybeKeyCode::Char('k')? need look at default inconfig/navigation.rs. Should inspect but not provided. I'd ideally check default definitions from snippet? not there. Should run verifying by retrieving other file? but not allowed? We can rely on snippet? maybe def_up etc. Without actual mapping.But we can approximate: def setups maybe
KeyCode::Char('j')etc? Without, uncertain. But bridging.But if nav config default is arrow keys, then pressing ctrl+arrow to move focus is expected? The previous code probably responded to
KeyCode::Char('j')etc as well asKeyCode::Up? Without seeing old code tough. Maybe there is risk.Given not high.
Focus on
handle_keyboard: they read nav config on each call. Good.Potential concurrency: get Direction returns
NavDirection::Select? there is enumerations glimpsed? we only handle Up/Down etc. WhenNavDirection::Selectoccurs (maybe hitting Enter) we don't break loops exceptnav.is_select. Works.Need to ensure
NavDirection::Close(Esc) used? not in this code.Ok.
Therefore likely no major issue in list.rs? but we must ensure restful.
Wait, we changed to
Config::global()for theme color. Butdrawcalled beforeConfig::init()? Without, panic. But we must assume per new design they callConfig::init()early (maybe in main). But to be sure, we should verify other files? Not provided. Should we note to ensureConfig::init()invoked? maybe there is risk mania. Could mention as request verification? Perhaps watchers set.But we don't have evidence to know. Without verifying, can't convict.
Focus on config: we will make comment about using HOME. That's major.
Need to produce scratchpad: track tasks.
Start new scratchpad abiding instructions.
We must produce scratchpad after start marker? instructions say use hidden scratchpad in comments between responses. So before final response we should produce per file? In example they inserted scratchpad per file. Should we replicate? They insisted to maintain hidden comment for tasks. We'll do similar: After each
file_start, include scratchpad? In example they used multiple scratchpad comments. Should we replicate? Maybe lighten.Given we have 2 files, we can follow:
initial scratchpad for file list.
Evaluate more.
So craft scratchpad.
Need to produce final with file_start markers.
For file list, is there any issue to comment? If none, we still must output file_start and file_end with optional comment? But guidelines say include start and end markers even if no comment. Should we add general comment? If no issues, we can leave no review comment but maybe highlight.
However we might consider verifying
Config::global()in draw: we call thrice; should maybe storelet theme = Config::global().theme.clone()? but not necessary.No comment? For thoroughness, check for potential mistakes:
Focus Search:
self.search_input.handle_event(key_event, true)returnsOption<String>with new text? Suppose nav config usesKeyCode::Escto close, but search_input might process doping.Focus NavBar: previously there might have been ability to move through nav? Without more context.
Since no clear bug, we won't comment.
Therefore file list: output start and end with no comments? need we produce comment? Possibly optional to highlight all good? Could add "navigation abstraction integration looks good" as ? Could, but optional.
But we should be careful not to produce positive comment if no check verifying. Could skip. Without comment, we just have file_start, scratchpad (maybe optional?), file_end. Should we include scratchpad? instructions say use hidden scratchpad to track tasks for ourselves. Maybe we can include minimal.
Maybe we produce scratchpad with tasks done but check.
Let's craft final: For file list, no comments.
For config/mod.rs we need comment on config_dir/data_dir.
Construct comment referencing both functions in same comment maybe referencing lines 59-73. We'll highlight mania.
We should provide fix suggestion using
dirs::config_diretc? Provide diff.Maybe best to use
dirs::config_diranddirs::data_dir. Provide patch.But using crate? Already maybe using new dependencies? If not, they'd need to add dependency. But commit may not include. We can suggest using
dirscrate orhomecrate or fallback tostd::env::var("HOME").or_else(|_| std::env::var("USERPROFILE")). Provide diff adjusting to degrade.Since we must provide actionable suggestion. Maybe use
dirs::config_diranddirs::data_dir. But requiring new dependency? Possibly already used? Need check? Without verifying, restful.Maybe we can propose to fallback to
HOMEfirst elsedirs::home_dir. Standard approach: usedirs::config_dir.I'll craft diff that uses
dirs::config_diranddirs::data_dir. Provide snippet:use std::path::PathBuf; use std::process::Command; use std::sync::OnceLock; +fn resolve_dir<F>(fallback: F) -> PathBuf +where + F: FnOnce() -> PathBuf, +{ + fallback() +}Maybe too complicated.
Simpler: Suggest to use
dirs::config_dir()returning Option.pub fn config_dir() -> PathBuf { dirs::config_dir() .unwrap_or_else(|| { eprintln!("Failed to resolve config directory, defaulting to current dir"); PathBuf::from(".") }) .join("mal-cli") }But need to ensure
dirsis dependency; if not, mania. Without verifying we can't assume.Maybe we can use
homecrate? Eh.Maybe restful to rely on
dirs::config_diras standard.But they may not have crate. Should check
Cargo.toml? Not available. But we can still suggest. Provide diff requiring addingdirsto dependencies? But we can't changeCargo.tomlsnippet? Without verifying we can mention customizing.Alternatively we can implement using
dirs_next? Hmm.Maybe we can craft suggestion using platform-specific environment variables with fallback to
std::env::var("HOME").or_else(|_| std::env::var("USERPROFILE")). Without extra dependency.But the main bug is there is no fallback;
expectwill panic. We can fix by usingdirs::config_dir.Maybe we can propose:
pub fn config_dir() -> PathBuf { dirs::config_dir() .unwrap_or_else(|| { eprintln!("Could not resolve config directory, using current directory"); std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")) }) .join("mal-cli") }But we need to ensure we mention need to add
dirscrate.But evaluation: If
dirs::config_dirreturns None, we degrade.But to avoid new dependencies, we can use
homecrate? same.Maybe we can manually check:
pub fn config_dir() -> PathBuf { std::env::var_os("XDG_CONFIG_HOME") .map(PathBuf::from) .or_else(|| std::env::var_os("HOME").map(|home| PathBuf::from(home).join(".config"))) .or_else(|| std::env::var_os("USERPROFILE").map(PathBuf::from)) .unwrap_or_else(|| PathBuf::from(".")) .join("mal-cli") }Better? but mania.
I'll propose using
dirscrate but mention? Maybe degrade.But as reviewer, we can highlight issue and mention recommended fix maybe using
dirs::config_diror similar, but not supply diff requiring new dependency. Maybe provide diff that uses fallback toUSERPROFILEto avoid panic.Let's craft diff:
pub fn config_dir() -> PathBuf { std::env::var_os("XDG_CONFIG_HOME") .map(PathBuf::from) .or_else(|| std::env::var_os("HOME").map(|home| PathBuf::from(home).join(".config"))) .or_else(|| std::env::var_os("APPDATA").map(PathBuf::from)) .unwrap_or_else(|| { eprintln!("Failed to resolve config directory, defaulting to current directory"); std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")) }) .join("mal-cli") }But mania.
Better to produce comment referencing both functions and say to use
dirs::config_dir/dirs::data_dir(since doping). Provide diff that usesdirscrate functions. For diff we need to show modifications lines 58-73.Since we don't know they have directories crate, some watchers? But we can propose.
Ok.
Let's craft comment.
Before final comment, we need scratchpad info.
Construct hidden scratchpad.
Maybe include one overall scratchpad at top summarizing tasks.
Let's produce final message with the required structure.
Need to include per file markers.
For
src/screens/list.rs, we no comment. But to follow instructions, we might include scratchpad? optional.Maybe to be safe, include hidden scratchpad inside
file_startandfile_endeven if empty? We'll include minimal to show tasks done.For
src/config/mod.rs, we add review comment with diff.Need to ensure subject line is short and bold.
For comment, title like "Avoid panicking on platforms without $HOME". Provide explanation of Windows.
Add tag ``.
Add diff.
Let's craft diff:
diff @@ - pub fn config_dir() -> PathBuf { - std::env::var("HOME") - .ok() - .map(|home| PathBuf::from(home).join(".config/mal-cli")) - .expect("Failed to get app directory") - } + pub fn config_dir() -> PathBuf { + if let Some(dir) = std::env::var_os("XDG_CONFIG_HOME") { + return PathBuf::from(dir).join("mal-cli"); + } + + if let Some(home) = std::env::var_os("HOME") { + return PathBuf::from(home).join(".config/mal-cli"); + } + + if let Some(appdata) = std::env::var_os("APPDATA") { + return PathBuf::from(appdata).join("mal-cli"); + } + + std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")).join("mal-cli") + } @@ - pub fn data_dir() -> PathBuf { - std::env::var("HOME") - .ok() - .map(|home| PathBuf::from(home).join(".local/share/mal-cli")) - .expect("Failed to get app directory") - } + pub fn data_dir() -> PathBuf { + if let Some(dir) = std::env::var_os("XDG_DATA_HOME") { + return PathBuf::from(dir).join("mal-cli"); + } + + if let Some(home) = std::env::var_os("HOME") { + return PathBuf::from(home).join(".local/share/mal-cli"); + } + + if let Some(localappdata) = std::env::var_os("LOCALAPPDATA") { + return PathBuf::from(localappdata).join("mal-cli"); + } + + std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")).join("mal-cli") + }This adds fallback for Windows. That uses
std::env::var_os, good.But we referencing
.local/sharefor Linux; on Windows the fallback toLOCALAPPDATA. Accept.Need ensure comment mention e.g. "On Windows $HOME undefined, call to expect panic".
Add tagline maybe referencing instructions? Nothing else.
Ok.
Now craft final output.
Need to embed hidden scratchpad(s) as HTML comments.
Maybe structure similar to example.
Let's craft final message:
<!-- [scratchpad] ... --> </blockquote></details> <details> <summary>src/screens/widgets/popup.rs (2)</summary><blockquote> `141-152`: **Avoid panic on background send() failure** Don’t unwrap on cross-thread send; background threads should not crash when receiver is closed. Apply: ```diff - app_sx - .send(Event::StorageUpdate( + let _ = app_sx + .send(Event::StorageUpdate( anime.id, Box::new(move |anime: &mut Anime| { anime.num_released_episodes = Some(episodes); if anime.num_episodes == 0 { anime.num_episodes = episodes; anime.episode_count_ready = false; } }), )) - .unwrap(); + .ok();
1465-1474: Fix off-by-one causing out-of-bounds index on mouse selectRow→index mapping can set next_index == options.len(), then index into self.options[next_index].
- for (i, row) in popup_area.inner(Margin::new(0, 1)).rows().enumerate() { - if row.contains(pos) && i <= self.options.len() { - self.next_index = i + self.scroll; - if mouse_clicked { - let selected_option = self.options[self.next_index].clone(); + for (i, row) in popup_area.inner(Margin::new(0, 1)).rows().enumerate() { + if row.contains(pos) { + let idx = i + self.scroll; + if idx < self.options.len() { + self.next_index = idx; + if mouse_clicked { + let selected_option = self.options[self.next_index].clone(); self.selected_index = self.next_index; self.close(); - return Some(selected_option); + return Some(selected_option); + } } } }
🧹 Nitpick comments (11)
src/utils/imageManager.rs (1)
1-1: Consider removing or narrowing the blanket unused suppression.The
#![allow(unused)]directive suppresses all unused warnings file-wide, which can hide legitimate dead code or unused imports that should be cleaned up.If specific items must remain unused temporarily, use targeted attributes:
-#![allow(unused)]Or if specific suppressions are needed:
#[allow(dead_code)] fn specific_function() { ... }src/utils/stringManipulation.rs (1)
91-134: LGTM!The Levenshtein distance implementation is correct and handles edge cases properly. The algorithm uses standard dynamic programming with proper Unicode support via
chars().Optional optimization: Consider reducing space complexity from O(n×m) to O(min(n,m)) using two rolling arrays instead of the full matrix, if memory becomes a concern for large strings:
pub fn levenshtein_distance(s1: &str, s2: &str) -> usize { let chars1: Vec<char> = s1.chars().collect(); let chars2: Vec<char> = s2.chars().collect(); let len1 = chars1.len(); let len2 = chars2.len(); if len1 == 0 { return len2; } if len2 == 0 { return len1; } let mut prev = (0..=len2).collect::<Vec<_>>(); let mut curr = vec![0; len2 + 1]; for i in 1..=len1 { curr[0] = i; for j in 1..=len2 { let cost = if chars1[i - 1] == chars2[j - 1] { 0 } else { 1 }; curr[j] = std::cmp::min( std::cmp::min(prev[j] + 1, curr[j - 1] + 1), prev[j - 1] + cost ); } std::mem::swap(&mut prev, &mut curr); } prev[len2] }src/config/player.rs (1)
13-20: Playback hooks: verify safe execution (escaping/injection)If hooks are executed via a shell, ensure arguments are properly escaped or avoid shell invocation (exec directly) to prevent command injection or breakage on titles with spaces/special chars.
Would you like me to propose a safe execution helper that replaces placeholders and spawns the process without a shell?
src/screens/widgets/navbar.rs (1)
126-131: Minor: reduce repeatedConfig::global()callsCache a local reference to the theme to cut repetition and allocations in tight render loops.
Example:
let theme = &Config::global().theme; // then use: Style::default().fg(theme.highlight)Also applies to: 174-179
src/screens/widgets/animebox.rs (1)
49-61: Theme migration looks good; consider caching theme locallySwitch to
Config::global().themeis consistent across render paths. For readability, storelet theme = &Config::global().theme;and reuse.Also applies to: 207-216, 318-319
src/main.rs (2)
59-68: Unify terminal command target streamYou send mouse capture to stderr and keyboard flags to stdout. Prefer a single stream (stdout) for consistency.
Apply this diff:
- if config.navigation.enable_mouse_capture { - execute!(std::io::stderr(), EnableMouseCapture)?; - } - execute!( - std::io::stdout(), + if config.navigation.enable_mouse_capture { + execute!(std::io::stdout(), EnableMouseCapture)?; + } + execute!( + std::io::stdout(), PushKeyboardEnhancementFlags( KeyboardEnhancementFlags::DISAMBIGUATE_ESCAPE_CODES | KeyboardEnhancementFlags::REPORT_EVENT_TYPES ) )?;
62-68: Optional: gate keyboard enhancement flags by terminal support (crossterm 0.29)On some terminals these flags may be ignored. Consider querying support and adjusting behavior.
Based on learnings
src/screens/widgets/popup.rs (1)
1390-1398: Honor remapped “select” key to open dropdownUse nav.is_select to open the dropdown instead of hard‑coding Enter.
- if !self.is_open { - if key_event.code == KeyCode::Enter { - self.open(); - } - return None; - } + if !self.is_open { + if nav.is_select(&key_event.code) { + self.open(); + } + return None; + }src/player/mod.rs (1)
258-291: Improve show selection: include alt titles and better distance heuristicOnly anime.title is considered. Add exact match on English alt title and compute distance against both.
- // Try to match name exactly first: - let show = shows.iter() - .find(|s| s.name.eq_ignore_ascii_case(&anime.title)) + // Try exact match on any known title + let titles = [&anime.title, &anime.alternative_titles.en]; + let show = shows.iter() + .find(|s| titles.iter().any(|t| !t.is_empty() && s.name.eq_ignore_ascii_case(t))) .or_else(|| { - // If no exact match, find the one(s) with lowest distance + // If no exact match, find the one(s) with lowest distance across titles if shows.is_empty() { return None; } - let distances: Vec<(usize, &ShowEdge)> = shows.iter() - .map(|s| (levenshtein_distance(&s.name.to_lowercase(), &anime.title.to_lowercase()), s)) - .collect(); + let distances: Vec<(usize, &ShowEdge)> = shows.iter() + .map(|s| { + let s_low = s.name.to_lowercase(); + let d = titles.iter() + .filter(|t| !t.is_empty()) + .map(|t| levenshtein_distance(&s_low, &t.to_lowercase())) + .min() + .unwrap_or(usize::MAX); + (d, s) + }) + .collect();src/config/mod.rs (2)
58-73: Prefer OS-appropriate config/data dirsUsing $HOME/.config and $HOME/.local/share breaks on Windows and non-XDG setups. Use the directories crate (or dirs-next) for cross‑platform paths.
Example:
- Config: ProjectDirs::from("com", "you", "mal-cli").config_dir()
- Data: ProjectDirs::from(...).data_dir()
85-93: Don’t write empty config on serialization failureIf toml::to_string fails you write an empty file. Return early instead.
- let toml = toml::to_string(&config) - .map_err(|e| { - eprintln!("Failed to serialize config: {}", e); - }) - .unwrap_or_default(); + let toml = match toml::to_string(config) { + Ok(s) => s, + Err(e) => { + eprintln!("Failed to serialize config: {}", e); + return; + } + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockserver/docs/myanimelist.pngis excluded by!**/*.pngserver/docs/myanimelist2.pngis excluded by!**/*.pngserver/docs/myanimelist3.pngis excluded by!**/*.pngserver/docs/myanimelist4.pngis excluded by!**/*.png
📒 Files selected for processing (35)
Cargo.toml(1 hunks)PKGBUILD(1 hunks)README.md(2 hunks)server/.env.example(1 hunks)server/README.md(1 hunks)src/app.rs(4 hunks)src/config/mod.rs(1 hunks)src/config/navigation.rs(1 hunks)src/config/network.rs(1 hunks)src/config/player.rs(1 hunks)src/config/theme.rs(1 hunks)src/handlers/input_handler.rs(2 hunks)src/main.rs(3 hunks)src/mal/mod.rs(5 hunks)src/mal/models/anime.rs(3 hunks)src/mal/models/user.rs(1 hunks)src/mal/oauth.rs(5 hunks)src/player/mod.rs(9 hunks)src/screens/launch.rs(2 hunks)src/screens/list.rs(8 hunks)src/screens/login.rs(2 hunks)src/screens/mod.rs(1 hunks)src/screens/overview.rs(5 hunks)src/screens/profile.rs(7 hunks)src/screens/search.rs(6 hunks)src/screens/seasons.rs(8 hunks)src/screens/widgets/animebox.rs(6 hunks)src/screens/widgets/button.rs(3 hunks)src/screens/widgets/infobox.rs(2 hunks)src/screens/widgets/navbar.rs(4 hunks)src/screens/widgets/popup.rs(32 hunks)src/utils/imageManager.rs(2 hunks)src/utils/input.rs(2 hunks)src/utils/stringManipulation.rs(1 hunks)src/utils/terminalCapabilities.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (21)
src/screens/widgets/infobox.rs (2)
src/config/mod.rs (2)
default(48-55)global(44-46)src/config/theme.rs (1)
default(49-64)
src/screens/widgets/button.rs (2)
src/config/mod.rs (2)
global(44-46)default(48-55)src/config/theme.rs (1)
default(49-64)
src/screens/launch.rs (1)
src/config/mod.rs (1)
global(44-46)
src/screens/widgets/animebox.rs (2)
src/config/mod.rs (2)
global(44-46)default(48-55)src/config/theme.rs (1)
default(49-64)
src/screens/profile.rs (3)
src/config/mod.rs (2)
default(48-55)global(44-46)src/config/navigation.rs (1)
default(69-79)src/config/theme.rs (1)
default(49-64)
src/screens/overview.rs (2)
src/config/mod.rs (2)
global(44-46)data_dir(68-73)src/screens/widgets/popup.rs (4)
new(74-104)new(869-890)new(1282-1296)new(1642-1649)
src/mal/oauth.rs (1)
src/config/mod.rs (1)
global(44-46)
src/screens/seasons.rs (4)
src/config/mod.rs (2)
default(48-55)global(44-46)src/config/navigation.rs (1)
default(69-79)src/config/theme.rs (1)
default(49-64)src/screens/widgets/popup.rs (4)
new(74-104)new(869-890)new(1282-1296)new(1642-1649)
src/config/network.rs (1)
src/config/mod.rs (1)
default(48-55)
src/config/navigation.rs (1)
src/config/mod.rs (1)
default(48-55)
src/screens/widgets/navbar.rs (3)
src/config/mod.rs (2)
global(44-46)default(48-55)src/config/navigation.rs (1)
default(69-79)src/config/theme.rs (1)
default(49-64)
src/mal/mod.rs (1)
src/config/mod.rs (1)
data_dir(68-73)
src/config/theme.rs (1)
src/config/mod.rs (1)
default(48-55)
src/screens/widgets/popup.rs (5)
src/utils/stringManipulation.rs (1)
format_date(68-89)src/config/mod.rs (2)
global(44-46)default(48-55)src/config/theme.rs (2)
status_color(69-78)default(49-64)src/config/navigation.rs (1)
default(69-79)src/mal/mod.rs (1)
current_season(181-194)
src/player/mod.rs (3)
src/utils/stringManipulation.rs (1)
levenshtein_distance(91-134)src/config/mod.rs (1)
global(44-46)src/mal/models/anime.rs (1)
status(15-27)
src/screens/login.rs (1)
src/config/mod.rs (1)
global(44-46)
src/screens/list.rs (4)
src/config/mod.rs (2)
global(44-46)default(48-55)src/screens/widgets/popup.rs (4)
new(74-104)new(869-890)new(1282-1296)new(1642-1649)src/config/navigation.rs (1)
default(69-79)src/config/theme.rs (1)
default(49-64)
src/screens/search.rs (3)
src/config/mod.rs (2)
default(48-55)global(44-46)src/config/navigation.rs (1)
default(69-79)src/config/theme.rs (1)
default(49-64)
src/config/mod.rs (5)
src/screens/widgets/popup.rs (4)
new(74-104)new(869-890)new(1282-1296)new(1642-1649)src/mal/mod.rs (2)
new(33-41)save_to_file(43-65)src/config/navigation.rs (1)
default(69-79)src/config/network.rs (1)
default(33-39)src/config/theme.rs (1)
default(49-64)
src/app.rs (1)
src/config/mod.rs (1)
data_dir(68-73)
src/main.rs (1)
src/config/mod.rs (2)
open_in_editor(107-126)init(39-41)
🪛 markdownlint-cli2 (0.18.1)
server/README.md
15-15: Bare URL used
(MD034, no-bare-urls)
47-47: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
80-80: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
README.md
15-15: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (32)
src/utils/imageManager.rs (2)
199-200: LGTM! Cleaner Arc cloning.Removing the extra reference operator makes the code more direct since
instanceis already&Arc<Mutex<Self>>.
278-278: LGTM! Consistent Arc cloning improvement.Same cleanup as lines 199-200, removing unnecessary double-reference.
src/utils/input.rs (2)
52-79: LGTM! Standardized word deletion keybinding.The change from CONTROL+Char('h') to CONTROL+Backspace improves usability by aligning with modern terminal conventions. The word-boundary deletion logic correctly handles UTF-8 characters and placeholder restoration.
91-91: Confirm MSRV supportsis_none_or
is_none_orrequires Rust 1.82.0+. There’s norust-versionin Cargo.toml. Please verify the project’s MSRV meets this requirement (or add arust-versionfield).server/.env.example (1)
3-3: LGTM!The updated redirect URL aligns with standard OAuth callback patterns.
PKGBUILD (1)
7-7: LGTM!The repository URL update correctly reflects the project's new location.
src/mal/models/user.rs (1)
129-133: LGTM!The refactor from an explicit
Defaultimplementation toderive(Default)is correct. The derived implementation produces identical behavior (all fields default to their type'sDefault, which is0forusize).src/mal/models/anime.rs (5)
462-466: LGTM!The refactor to
derive(Default)forRankingis correct. Default implementation will producerank: 0andprevious_rank: None.
468-474: LGTM!The refactor to
derive(Default)forPicturesis correct and produces the same behavior as the removed explicit implementation.
476-484: LGTM!The refactor to
derive(Default)forAlternativeTitlesis correct. The derived implementation properly initializessynonymsas an empty vector and strings as empty.
498-520: LGTM!The refactor to
derive(Default)forMyListStatusis correct. All fields will receive appropriate zero/empty/None defaults.
522-528: LGTM!The refactor to
derive(Default)forStartSeasonis correct and maintains consistency with theDisplayimplementation that handles zero values.src/screens/widgets/infobox.rs (1)
3-3: LGTM!The migration from hard-coded
SECOND_TEXT_COLORtoConfig::global().theme.second_textcorrectly centralizes theme configuration. TheConfig::global()call is safe here as widgets are rendered after application initialization.Also applies to: 137-137
src/mal/mod.rs (1)
5-5: LGTM!The migration from
get_app_dirtoConfig::data_dir()correctly centralizes directory path management. Thedata_dir()method is static and doesn't require prior initialization, making these calls safe.Also applies to: 44-44, 85-85, 151-151, 167-167
src/utils/terminalCapabilities.rs (1)
49-49: LGTM!The refactor from a closure to a direct function item in
get_or_initis a good optimization that eliminates unnecessary closure allocation while preserving identical lazy initialization behavior.src/handlers/input_handler.rs (2)
10-15: LGTM: Event filtering aligns with kitty protocol.The filter correctly skips
Releaseevents (and other non-Press/Repeat kinds), which is appropriate for kitty protocol support where Release events are not needed for input processing.
17-17: LGTM: Event variant rename is consistent.The rename from
Event::InputEventtoEvent::Inputis applied consistently and aligns with the corresponding enum change insrc/app.rs.Also applies to: 40-40
src/screens/mod.rs (1)
127-130: LGTM: Useful default implementation for screen naming.The default
get_name()implementation provides a clean way to derive screen names from type names, which is useful for debugging, panic messages, and screen storage without requiring each Screen implementation to define it manually.src/app.rs (2)
56-56: LGTM: Event variant rename is consistent.The
Eventenum change fromInputEventtoInputis applied consistently across the file, matching the corresponding changes insrc/handlers/input_handler.rs.Also applies to: 170-170
200-200: Initialization order verified Config::init() is invoked before App::new(), so no changes required.Cargo.toml (2)
27-27: LGTM: toml crate addition supports Config persistence.The
toml0.9.7 addition enables TOML-based configuration file reading/writing for the new Config system. Based on learnings, this is a stable release with MSRV 1.76.
9-9: LGTM: Dependency updates enable serde-backed configuration, no ratatui breaking changes impact the code.README.md (1)
83-90: LGTM: Keybindings documentation accurately reflects config-driven approach.The updated keybindings section correctly documents that configurations can be generated and edited using
mal -e, aligning with the new Config system and navigation abstraction introduced in this PR.src/mal/oauth.rs (2)
43-43: LGTM: OAuth URLs now use configured auth server.The replacement of hard-coded
BACKEND_URLwithConfig::global().network.auth_servercorrectly enables configurable authentication server endpoints.Also applies to: 53-53
73-78: Verify Config initialization and callback port range.
- Confirm
Config::init()is always called before anyoauth_login()invocation (I didn’t detectinit_oauth()inmain.rs; please verify your login flow).- Default ports are 53400–53409 (unprivileged ephemeral range); ensure any custom overrides don’t overlap system or well-known ports.
src/screens/launch.rs (1)
116-130: LGTM: Config initialization verified
Config::init()inmain()(src/main.rs:56) precedesApp::new()(src/main.rs:71), which constructsLaunchScreen.src/screens/widgets/button.rs (1)
8-8: Config initialization verified before Button usage.Config::init()is invoked insrc/main.rs:56prior to anyButtoninstantiation, preventingConfig::global()panics.src/screens/search.rs (1)
233-257: Navigation abstraction and theming integration LGTMDirectional handling via
nav.get_directionand theme usage are consistent and correct.Also applies to: 271-287, 299-330, 171-179, 203-208
src/screens/widgets/popup.rs (1)
1281-1295: Ensure Config is initialized before widget instantiationSelectionPopup::new uses Config::global(); this will panic unless Config::init() ran earlier (e.g., at app startup).
Please confirm main/init calls Config::init() before constructing UI.
src/player/mod.rs (1)
108-120: Default PlayResult when stdout is empty makes manual/disabled flows coherentGood addition; prevents spurious errors when default player is disabled.
src/screens/seasons.rs (1)
228-268: Theming via Config looks consistentBorders/title styling now use theme.primary; season highlight uses theme.highlight/text. LGTM.
src/config/mod.rs (1)
43-46: Config::init invoked before any Config::global() usage Config::init() is called in main.rs (line 56) before any Config::global() calls, so the panic risk is mitigated.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config/README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
src/config/README.md
130-130: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Cargo.toml (1)
23-23: Re-check TTL call sites after upgradingcachedto 0.56.0.0.56.0 requires timed/expiring caches to receive
std::time::Durationvalues instead of raw integers. Please confirm any macros or constructors now passDuration::from_secs/from_millis, etc.#!/bin/bash # Find cached timed cache usages still passing numeric TTL/expiration args. rg -nP 'cached(?:_time|_async_time|_proc_macro)?!\([^)]*\b\d+\b' -C2src/config/player.rs (1)
9-11: Restore compatibility for renamed fieldRenaming
allways_complete_episodetoalways_complete_episodewithout a serde alias means existing config files keep the old key and the flag now deserializes asfalse, so previously enabled auto-complete stops working with no warning. Please add the alias (and fix the doc typo while you’re here) so older configs continue to load as intended.- /// allways marks the episode as watched after pressing play - #[serde(default)] - pub always_complete_episode: bool, + /// Always marks the episode as watched after pressing play + #[serde(default, alias = "allways_complete_episode")] + pub always_complete_episode: bool,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)src/config/README.md(1 hunks)src/config/mod.rs(1 hunks)src/config/player.rs(1 hunks)src/config/theme.rs(1 hunks)src/player/mod.rs(9 hunks)src/screens/widgets/navbar.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/config/README.md
- src/config/theme.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/screens/widgets/navbar.rs (3)
src/config/mod.rs (2)
global(44-46)default(48-55)src/config/theme.rs (1)
default(49-64)src/config/navigation.rs (1)
default(69-79)
src/player/mod.rs (3)
src/utils/stringManipulation.rs (1)
levenshtein_distance(91-134)src/config/mod.rs (1)
global(44-46)src/mal/models/anime.rs (1)
status(15-27)
src/config/mod.rs (4)
src/screens/widgets/popup.rs (4)
new(74-104)new(869-890)new(1282-1296)new(1642-1649)src/config/theme.rs (1)
default(49-64)src/config/navigation.rs (1)
default(69-79)src/config/network.rs (1)
default(33-39)
🔇 Additional comments (1)
src/screens/widgets/navbar.rs (1)
72-86: Good guardrail around navigation stateThe saturated upper bound plus the empty-options check prevent the old wraparound/panic paths while keeping the handler neat. Nice cleanup.
1 participant
added mal -e
should be selecting the correct anime when watching
update readme
update some cargos
configs:
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores