Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a SQLite-backed ORM and proc-macro for Entry, integrates a local DatabaseManager into app and MalClient for offline sync, implements extensive background synchronization UI (SyncPopup) and flows, refactors network auth to an Identifier, and adds server endpoints (/health, /pfp) plus a static index page. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (Screen)
participant App as App
participant MalClient as MalClient
participant LocalDB as DatabaseManager (SQLite)
participant RemoteAPI as MAL Remote API
UI->>App: Trigger sync (user or background)
App->>MalClient: Request local anime list
MalClient->>LocalDB: Query stored Anime
LocalDB-->>MalClient: Return local anime list
par Fetch remote
MalClient->>RemoteAPI: Fetch remote anime batches (Identifier)
RemoteAPI-->>MalClient: Return batch
end
MalClient->>MalClient: Compare local vs remote, build conflicts
MalClient->>App: Emit per-anime sync updates
App->>UI: Render/Update SyncPopup
UI->>App: User chooses action (sync/delete)
App->>MalClient: Execute update for item(s)
alt Logged in
MalClient->>RemoteAPI: Send update request
RemoteAPI-->>MalClient: Confirm
end
MalClient->>LocalDB: Upsert/delete local records
LocalDB-->>MalClient: Confirm
MalClient->>App: Emit SyncStatus(success)
App->>UI: Update UI / close popup
sequenceDiagram
participant Client as Client App
participant Server as Server
participant FFmpeg as FFmpeg
participant FS as Filesystem
Client->>Server: GET /pfp
Server->>Server: choose random timestamp
Server->>FFmpeg: run ffmpeg to extract frame (350x225) at timestamp
FFmpeg-->>FS: write temporary PNG
FS-->>Server: read PNG bytes
Server-->>Client: Respond image/png
Server->>FS: remove temp file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/login.rs (1)
175-183:⚠️ Potential issue | 🟠 MajorDocument MSRV requirement or use alternative syntax for the let-chains pattern.
The
if let ... && let ...syntax (let-chains) requires Rust 1.76.0 or higher. The project has no documented MSRV, which means this code introduces an undocumented dependency on a specific Rust version. Either:
- Add
rust-version = "1.76"toCargo.tomlto document the MSRV requirement, or- Refactor using nested
if letstatements to maintain compatibility with older Rust versions
🤖 Fix all issues with AI agents
In `@deps/database/core/Cargo.toml`:
- Around line 1-4: The crate named "core" in Cargo.toml shadows Rust's sysroot
core; rename the package from "core" to a non-conflicting name like "db_core" or
"core_lib" in the Cargo.toml package.name entry and update all workspace members
and dependent crates' Cargo.toml entries that list "core" as a dependency to use
the new name; also update any use/import paths or crate references (e.g., extern
crate core; or use core::...) inside source files to the new crate name where
they refer to your crate (but leave all references to the standard library's
::core unchanged).
In `@deps/database/core/src/lib.rs`:
- Around line 100-119: The update method currently interpolates the condition
string directly into the SQL causing SQL injection; change update<T:
Entryable>(&self, obj: T, condition: &str) to accept parameterized condition
plus its parameters (e.g., condition: &str, condition_params: impl
IntoIterator<Item=rusqlite::types::Value>) and do not concatenate the condition
into SQL raw — require the caller to use ? placeholders in condition and then
build the final query with placeholders only, collect the obj.bind_values() into
a values Vec and then extend that Vec with the condition_params, and finally
call connection.execute(&query, rusqlite::params_from_iter(combined_values)).
Reference symbols: update, Entryable::bind_values, table_name, set_clauses, and
rusqlite::params_from_iter.
- Around line 78-98: The upsert method currently ensures the table exists by
calling T::schema() and running "CREATE TABLE IF NOT EXISTS ..." but insert does
not, causing inconsistent behavior; fix by making behavior consistent: either
(A) add the same table-creation logic to insert (use T::table_name() and
T::schema() and run the CREATE TABLE IF NOT EXISTS before executing the INSERT),
or (B) extract the table-creation code into a helper (e.g.,
ensure_table_exists(&connection, &table_name, &schema) or
self.ensure_table<T>()) and call that helper from both upsert and insert,
ensuring both use T::table_name() and T::schema() to create the table if
missing.
- Around line 121-133: The delete method assumes the primary key column is "id"
which is brittle; add a method on the Entryable trait (for example
primary_key_name() or p_key_column()) that returns the column name for the PK,
implement it for types that already implement Entryable, and update the delete
function to use T::primary_key_name() together with obj.p_key() to build the
WHERE clause (referencing the delete method, Entryable trait, p_key(), and
table_name()); ensure the query properly escapes or formats the column name when
constructing the DELETE statement.
- Around line 40-57: The get method currently interpolates the condition string
directly into SQL (SQL injection risk) and has inconsistent indentation; change
the signature of get (pub fn get<T: Entryable>) to accept an optional
parameterized WHERE clause plus its bound parameters (e.g., Option<(&str, &[&dyn
ToSql])> or similar), build the SQL as "SELECT * FROM {table_name}" and append "
WHERE {clause}" only when the clause is provided, then pass the parameter slice
into connection.prepare/query_map (instead of embedding values into the string),
and tidy up the function indentation; reference the get function and
T::table_name() when making these changes.
In `@deps/database/entry/src/lib.rs`:
- Around line 311-316: The generated from_row code currently calls
serde_json::from_str(&row.get::<_, String>(`#index`)?).unwrap(), which can panic
on malformed JSON; replace the unwrap with propagation of a proper error by
mapping serde_json::Error into the function's error type and using ? (e.g., call
serde_json::from_str(...).map_err(|e| /* convert to rusqlite::Error or the
from_row Result's error */)?), so the from_row implementation returns an Err
instead of panicking when deserialization fails; update the generated template
around serde_json::from_str and row.get::<_, String>(`#index`) accordingly.
- Around line 207-221: The generated code in bind_values uses
serde_json::to_string(...).unwrap() for complex fields (occurrences of
serde_json::to_string in the quote! blocks), which can panic; change the
generator so bind_values propagates serialization errors instead of unwrapping:
update the bind_values signature to return Result<..., Error> (or
Result<Vec<rusqlite::types::Value>, rusqlite::Error/any appropriate error type)
and replace each .unwrap() with ? or map_err(...) to convert the
serde_json::Error into the chosen error type; ensure both branches that
currently produce
rusqlite::types::Value::from(serde_json::to_string(...).unwrap()) and the
.map(|v| serde_json::to_string(v).unwrap()) form use fallible serialization that
returns Err on failure so the caller can handle the error.
In `@server/src/main.rs`:
- Around line 285-317: The /pfp route handler currently panics on errors and has
fragile assumptions; replace the inline expect() usages (the ffmpeg
Command::new("ffmpeg") call and fs::read / fs::remove_file uses) with proper
error handling that returns an HTTP error response instead of crashing, verify
the source video exists via fs::metadata before running extraction, obtain
VIDEO_DURATION dynamically (e.g., via ffprobe) or fall back to a safe default,
use a collision-safe temp file creation strategy (e.g., tempfile::NamedTempFile
or a UUID) instead of rng.random::<u32>(), check Command::output() /
output.status.success() and log/return 500 on failure, read the temp file with
error handling and always attempt cleanup in a finally-style block to remove the
temp file (using fs::remove_file but ignore errors), and ensure all failures
produce a controlled rouille::Response with appropriate status instead of
calling expect().
In `@src/mal/models/mod.rs`:
- Around line 12-21: The DB ID fields use usize which rusqlite cannot
deserialize; change all ID field types from usize to i64 in the structs
referenced (WatchHistory, Anime, User) — specifically replace fields named id,
anime_id, user_id, etc., with i64 so the Entry-derived row conversion uses i64
and row.get::<_, i64>() succeeds; then update any code that assumes usize
(function params, returns, casts, comparisons) to convert from/to usize only
where necessary (e.g., when interfacing with non-DB APIs) to keep types
consistent with rusqlite.
In `@src/screens/search.rs`:
- Around line 291-298: The code uses "if let ... && ..." (let chains) in
expressions like the block calling search_input.handle_event(...) that sets
self.fetching and sends LocalEvent::Search via self.bg_sender, but your
Cargo.toml has an invalid edition "2024" and no toolchain guarantee: either
change the edition to a valid value (e.g., "2021") and ensure the
project/toolchain is Rust 1.76+ (update rust-toolchain or CI) so let_chains is
supported, or else refactor the three occurrences (the if-let around
search_input.handle_event, the similar checks that update self.fetching and use
bg_sender) into nested if let / if !text.is_empty() statements to avoid
let_chains entirely.
In `@src/screens/widgets/popup.rs`:
- Around line 125-131: The failure payload currently stores (index, String)
which mismatches the expected usize when later retrieved, so change the
.set("failure", ...) calls in the BackgroundUpdate creation to pass just the
index (usize) rather than a tuple; update the occurrence inside the match
handling update_user_list (the Err(e) branch that calls
BackgroundUpdate::new("popup").set("failure", ...)) and the second similar
occurrence later (lines ~178-184) so both failure branches send the same usize
type the consumer expects.
In `@src/screens/widgets/sync.rs`:
- Around line 338-342: The count for already_removed is using the wrong
predicate: in the closure passed to
self.finished_processing.iter().filter(|(sync, _)| *sync).count() you should
invert the boolean so it counts removed entries, i.e. use !*sync; update the
filter in the already_removed computation (referencing finished_processing and
already_removed) to use |(sync, _)| !*sync so it no longer duplicates the
already_synced logic.
🟡 Minor comments (11)
server/static/index.html-570-575 (1)
570-575:⚠️ Potential issue | 🟡 MinorAdd
rel="noopener noreferrer"for links opened in a new tab.Prevents tab‑nabbing and removes access to
window.opener.🔒 Suggested change
- <a href="https://github.com/swstl/mal-cli" class="btn btn-primary" target="_blank"> + <a href="https://github.com/swstl/mal-cli" class="btn btn-primary" target="_blank" rel="noopener noreferrer"> View on GitHub </a> - <a href="https://github.com/swstl/mal-cli#installation" class="btn btn-secondary" target="_blank"> + <a href="https://github.com/swstl/mal-cli#installation" class="btn btn-secondary" target="_blank" rel="noopener noreferrer"> Documentation </a>src/utils/stringManipulation.rs-91-109 (1)
91-109:⚠️ Potential issue | 🟡 MinorUse
NaiveDateTimefor timezone-less timestamps.
DateTime::parse_from_strrequires a timezone offset in the input and will fail on strings like2025-07-06T15:08. Parse asNaiveDateTimeinstead.Suggested fix
-use chrono::{DateTime, NaiveDate}; +use chrono::{DateTime, NaiveDate, NaiveDateTime}; @@ - if let Ok(dt) = DateTime::parse_from_str(date_str, "%Y-%m-%dT%H:%M") { + if let Ok(dt) = NaiveDateTime::parse_from_str(date_str, "%Y-%m-%dT%H:%M") { return dt.format("%b %d, %Y at %I:%M %p").to_string(); }deps/database/entry/src/lib.rs-128-130 (1)
128-130:⚠️ Potential issue | 🟡 MinorPotential truncation: casting to
usizemay lose precision.The generated
p_key()method casts the primary key field tousize. On 32-bit systems, this could truncate values if the primary key exceedsu32::MAX. Consider usingu64for the trait method or documenting this limitation.deps/database/core/src/lib.rs-143-152 (1)
143-152:⚠️ Potential issue | 🟡 MinorPotential issue: PRAGMA changes affect all subsequent operations.
clear_forcedisables and re-enables foreign keys, but if an error occurs between disable and re-enable, foreign keys remain disabled for subsequent operations on this connection.🛡️ Consider using a transaction or ensuring cleanup
pub fn clear_force<T: Entryable>(&self) -> Result<(), Error> { let connection = self.connection.lock().unwrap(); let table_name = T::table_name(); connection.execute("PRAGMA foreign_keys = OFF", [])?; let result = connection.execute(&format!("DELETE FROM {}", table_name), []); - connection.execute("PRAGMA foreign_keys = ON", [])?; - result?; + // Always re-enable foreign keys, even on error + let pragma_result = connection.execute("PRAGMA foreign_keys = ON", []); + result?; + pragma_result?; Ok(()) }src/screens/widgets/popup.rs-669-673 (1)
669-673:⚠️ Potential issue | 🟡 MinorGuard against underflow in narrow terminals.
title_area.width - 1can wrap/underflow; usesaturating_sub.🛡️ Suggested fix
- width: title_area.width - 1, + width: title_area.width.saturating_sub(1),src/screens/widgets/popup.rs-507-517 (1)
507-517:⚠️ Potential issue | 🟡 MinorClamp mouse-driven synopsis scrolling.
Keyboard scrolling clamps, but mouse scroll can overshoot and show blank space.🧭 Suggested fix
MouseEventKind::ScrollDown => { - self.synopsis_scroll += 1; + let max_scroll = self + .app_info + .anime_store + .get(&self.anime_id) + .map(|a| a.synopsis.len() as u16) + .unwrap_or(self.synopsis_scroll); + self.synopsis_scroll = min(self.synopsis_scroll + 1, max_scroll); }src/screens/mod.rs-86-93 (1)
86-93:⚠️ Potential issue | 🟡 MinorTypo:
needs_accoundshould beneeds_account.The method and macro have a typo - "accound" should be "account". This affects the public API and should be corrected.
✏️ Proposed fix for macro and trait method
#[macro_export] -macro_rules! check_for_account { +macro_rules! check_for_account { () => { - fn needs_accound(&self) -> bool { + fn needs_account(&self) -> bool { true } }; }fn uses_navbar(&self) -> bool { true } - fn needs_accound(&self) -> bool { + fn needs_account(&self) -> bool { false }And update line 330:
- if new_screen.needs_accound() && !MalClient::user_is_logged_in() { + if new_screen.needs_account() && !MalClient::user_is_logged_in() {Also applies to: 154-157
src/player/mod.rs-184-188 (1)
184-188:⚠️ Potential issue | 🟡 MinorBug: Error message incorrectly says "pre-playback hook" for post-playback hook.
The error message on line 187 says "Failed to run pre-playback hook" but this is actually the post-playback hook section.
🐛 Proposed fix
// hook if let Some(hook) = Config::global().player.post_playback_hook.clone() && let Err(e) = self.run_command(&hook, anime, episode, Some(&candidate)) { - eprintln!("Failed to run pre-playback hook: {}", e); + eprintln!("Failed to run post-playback hook: {}", e); };src/screens/overview.rs-315-315 (1)
315-315:⚠️ Potential issue | 🟡 MinorReplace
expect()with proper error handling to prevent panics on corrupted history.If the history file contains invalid data,
expect("Failed to read history")will panic. Useok()or proper error handling to skip malformed entries gracefully.🛡️ Proposed fix
- parts[1].parse::<AnimeId>().expect("Failed to read history"), + match parts[1].parse::<AnimeId>() { + Ok(id) => id, + Err(_) => continue, // Skip malformed entries + },Or more concisely, use
?with the surrounding logic:// idk what to do with this inforamiton yet but here it is. + let Ok(anime_id) = parts[1].parse::<AnimeId>() else { + continue; + }; let ( _timestamp, - anime_id, _title, ... ) = ( parts[0].to_string(), - parts[1].parse::<AnimeId>().expect("Failed to read history"), + anime_id, parts[2].to_string(),src/screens/list.rs-203-206 (1)
203-206:⚠️ Potential issue | 🟡 MinorSorting by
updated_atproduces inconsistent results when the field is not populated.The
my_list_status.updated_atfield defaults to"N/A"when missing. Direct string comparison via.cmp()sorts"N/A"lexicographically between other timestamps rather than grouping unpopulated entries together, breaking the intended "last updated" ordering. Consider filtering out or explicitly handling entries whereupdated_at == "N/A"before sorting.src/mal/mod.rs-318-324 (1)
318-324:⚠️ Potential issue | 🟡 MinorDuplicate login check.
Lines 318-320 and 321-323 perform the same
user_is_logged_in()check. Remove one of them.🐛 Proposed fix
pub fn get_anime_list( &self, status: Option<String>, offset: usize, limit: usize, ) -> Option<Vec<Anime>> { if !MalClient::user_is_logged_in() { return None; } - if !Self::user_is_logged_in() { - return None; - } self.get_anime_list_by_user("@me".to_string(), status, offset, limit) }
🧹 Nitpick comments (18)
src/screens/login.rs (1)
217-218: Consider handling thread join failure gracefully.
joinable.join().unwrap()will propagate a panic if the OAuth thread fails. While this runs in a background thread itself, unhandled panics can make debugging harder and leave the UI in an inconsistent state (e.g., stuck showing the URL without transitioning to success/error).Suggested improvement
- joinable.join().unwrap(); - mal_client.update_user_login(); - let new_url = "Login successful".to_string(); + match joinable.join() { + Ok(_) => { + mal_client.update_user_login(); + let new_url = "Login successful".to_string(); + let update = BackgroundUpdate::new(id.clone()).set("login_url", new_url); + let _ = info.app_sx.send(Event::BackgroundNotice(update)); + } + Err(_) => { + let new_url = "Login failed".to_string(); + let update = BackgroundUpdate::new(id.clone()).set("login_url", new_url); + let _ = info.app_sx.send(Event::BackgroundNotice(update)); + } + } + })) - let update = BackgroundUpdate::new(id.clone()).set("login_url", new_url); - let _ = info.app_sx.send(Event::BackgroundNotice(update)); - }))server/Dockerfile (1)
8-10: Consider--no-install-recommendsto keep the runtime image slimmer.Small size/attack‑surface improvement, no behavior change.
♻️ Suggested change
-RUN apt-get update && apt-get install -y ffmpeg && rm -rf /var/lib/apt/lists/* +RUN apt-get update && apt-get install -y --no-install-recommends ffmpeg \ + && rm -rf /var/lib/apt/lists/*server/static/index.html (1)
603-615: Handle clipboard failures gracefully.Avoids silent failures when the Clipboard API is unavailable (e.g., non‑HTTPS contexts).
💡 Suggested change
function copyCommand(btn) { - navigator.clipboard.writeText('yay -S mal').then(() => { - const originalText = btn.textContent; + const originalText = btn.textContent; + navigator.clipboard.writeText('yay -S mal').then(() => { btn.textContent = 'Copied!'; btn.classList.add('copied'); setTimeout(() => { btn.textContent = originalText; btn.classList.remove('copied'); }, 2000); - }); + }).catch(() => { + btn.textContent = 'Copy failed'; + btn.classList.remove('copied'); + setTimeout(() => { + btn.textContent = originalText; + }, 2000); + }); }server/src/main.rs (1)
271-275: Inconsistent error response format.The root endpoint returns a 500 status with plain text on failure, while other template-loading failures (Lines 205-208, 220-223) return 400. Consider aligning error handling patterns across the codebase.
src/utils/functionStreaming.rs (1)
7-7: Minor: Typo in comment."changeing" should be "changing".
✏️ Fix typo
- // for changeing + // for changingdeps/database/Cargo.toml (2)
7-7: Dependency name "core" shadows std library crate.The local dependency is named
core, which shadows Rust's standard librarycorecrate. While this works due to path resolution, it may cause confusion when reading code. Consider renaming todatabase_coreor similar.♻️ Suggested rename
[dependencies] -core = { path = "./core" } +database_core = { path = "./core", package = "core" } entry = { path = "./entry" }Or rename the actual crate in
./core/Cargo.tomltodatabase-core.
9-9: Consider pinning rusqlite to version "0.37.0" for explicit reproducibility.Version 0.37 exists on crates.io (currently 0.37.0 is the only patch version available). While
"0.37"will resolve to 0.37.0 deterministically, explicitly pinning to"0.37.0"follows reproducible builds best practices.Cargo.toml (1)
29-30: Consider bundling SQLite for portable installs.
If you ship binaries, relying on system SQLite can cause runtime link errors on minimal distros. Enabling rusqlite’sbundledfeature avoids that—please confirm it matches your distribution model.♻️ Suggested change
-rusqlite = "0.37.0" +rusqlite = { version = "0.37.0", features = ["bundled"] }src/screens/list.rs (2)
600-613: Remove commented-out dead code.This large block of commented code should be removed to improve maintainability. If this logic is needed for reference, it should be preserved in version control history instead.
🧹 Proposed fix
fn background(&mut self) -> Option<JoinHandle<()>> { - // let store_animes = self.app_info.anime_store.get_list(); - // let new_ids: Vec<AnimeId> = store_animes - // .iter() - // .filter(|anime| { - // !self.all_animes.contains(&anime.id) - // && status_is_known(anime.my_list_status.status.clone()) - // }) - // .map(|anime| anime.id) - // .collect(); - // - // if !new_ids.is_empty() { - // self.all_animes.extend(new_ids); - // self.filtered_animes = self.all_animes.clone(); - // } - if self.bg_loaded {
665-665: Minor typo: "recieved" should be "received".✏️ Proposed fix
- .set("fetching", false) // updates "fetching" to false once first batch of anime is recieved + .set("fetching", false) // updates "fetching" to false once first batch of anime is receivedsrc/screens/mod.rs (1)
175-175: Consider encapsulatingsyncing_popupinstead of making it public.The
syncing_popupfield is public and accessed directly inapp.rs(line 204). Consider providing methods onScreenManagerto interact with the popup state instead of exposing the field directly.♻️ Proposed encapsulation
- pub syncing_popup: sync::SyncPopup, + syncing_popup: sync::SyncPopup,Add a method to handle sync completion:
pub fn handle_sync_status(&mut self, success: bool, anime: Anime) { if success { self.syncing_popup.finished_syncing(anime); } else { self.show_error(format!("Failed to sync {}", anime.title)); } }src/app.rs (2)
231-234: Table creation on every write is inefficient.
create_tableis called every timelog_watched_infois invoked. Consider creating tables once during app initialization instead.♻️ Proposed fix - move table creation to initialization
In
App::new(), after creating thedb_manager:let db_manager = DatabaseManager::new(db_path).expect("Failed to initialize database manager"); db_manager.create_table::<Anime>().ok(); db_manager.create_table::<WatchHistory>().ok();Then remove from
log_watched_info:fn log_watched_info(&self, anime: &Anime, details: player::PlayResult) { let now: DateTime<Local> = Local::now(); let history = WatchHistory { id: 0, anime_id: anime.id, timestamp: now.format("%Y-%m-%d %H:%M:%S").to_string(), episode: details.episode as i32, current_time: details.current_time, total_time: details.total_time, percentage: details.percentage, is_completed: details.is_completed, }; - self.shared_info.local_db.create_table::<Anime>().ok(); - self.shared_info.local_db.create_table::<WatchHistory>().ok(); self.shared_info.local_db.upsert(anime.clone()).ok(); self.shared_info.local_db.upsert(history).ok(); }
202-210: Inconsistent brace formatting inSyncStatushandling.The
elseblock has inconsistent formatting compared to the rest of the codebase.🧹 Proposed formatting fix
- Event::SyncStatus(success, anime ) => { + Event::SyncStatus(success, anime) => { if success { self.screen_manager.syncing_popup.finished_syncing(*anime); - } - else { + } else { self.screen_manager.show_error(format!("Failed to sync {}", anime.title)); } - }src/screens/launch.rs (1)
64-64: Remove commented debug code.🧹 Proposed fix
2 => Some(Action::Quit), - // 2 => Some(Action::SwitchScreen(LAUNCH)), _ => None,src/mal/network.rs (1)
342-369: Consider returning a more descriptive error fromupdate_local.The error message
"local db error"on line 364 loses the original error context. Consider preserving the original error for better debugging.Proposed fix
fn update_local( mut self, database: &DatabaseManager, ) -> Result<(usize, Self::Response), Box<dyn std::error::Error>> { self.pre_update(); let updated = match database.upsert(self) { Ok(u) => u, Err(e) => { send_error!("Failed to update local database: {}", e); - return Err("local db error".into()); + return Err(format!("Failed to update local database: {}", e).into()); } }; let response = updated.to_offline_response(); Ok((updated.get_id(), response)) }src/screens/widgets/sync.rs (2)
83-95: Typo in method name:change_button_lablesshould bechange_button_labels.Proposed fix
- fn change_button_lables(&mut self) { + fn change_button_labels(&mut self) {Also update the call sites at lines 231 and 310.
716-720: Consider using more descriptive button labels.The labels
"plural"and"single"don't clearly communicate the toggle state to users. Consider more descriptive text like"Apply to All"/"One at a Time"or using icons/symbols.src/mal/mod.rs (1)
193-209: Consider adding error logging when client ID fetch fails.The
get_client_id()method silently returnsNoneon failure (line 205 uses.ok()?). Adding a log/error message would aid debugging when the auth server is unreachable.Proposed improvement
let client_id = send_request_expect_text( "GET", format!("{}/id", Config::global().network.auth_server), vec![], vec![], None, ) - .ok()?; + .map_err(|e| { + send_error!("Failed to fetch client ID: {}", e); + e + }) + .ok()?;
| quote! { | ||
| self.#field_name.as_ref() | ||
| .map(|v| serde_json::to_string(v).unwrap()) | ||
| .map(|s| rusqlite::types::Value::from(s)) | ||
| .unwrap_or(rusqlite::types::Value::Null) | ||
| } | ||
| } | ||
| } else { | ||
| quote! { rusqlite::types::Value::Null } | ||
| } | ||
| } else { | ||
| quote! { | ||
| rusqlite::types::Value::from(serde_json::to_string(&self.#field_name).unwrap()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Generated code uses unwrap() on JSON serialization - runtime panic risk.
The generated bind_values code uses .unwrap() on serde_json::to_string() calls (Lines 209, 219). If serialization fails, this will panic at runtime.
🛠️ Consider returning an error or using expect with context
For complex types, consider:
- rusqlite::types::Value::from(serde_json::to_string(&self.#field_name).unwrap())
+ rusqlite::types::Value::from(
+ serde_json::to_string(&self.#field_name)
+ .expect(concat!("Failed to serialize field: ", stringify!(`#field_name`)))
+ )Or better yet, consider if bind_values should return Result<Vec<...>, Error>.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| quote! { | |
| self.#field_name.as_ref() | |
| .map(|v| serde_json::to_string(v).unwrap()) | |
| .map(|s| rusqlite::types::Value::from(s)) | |
| .unwrap_or(rusqlite::types::Value::Null) | |
| } | |
| } | |
| } else { | |
| quote! { rusqlite::types::Value::Null } | |
| } | |
| } else { | |
| quote! { | |
| rusqlite::types::Value::from(serde_json::to_string(&self.#field_name).unwrap()) | |
| } | |
| } | |
| quote! { | |
| self.#field_name.as_ref() | |
| .map(|v| serde_json::to_string(v).unwrap()) | |
| .map(|s| rusqlite::types::Value::from(s)) | |
| .unwrap_or(rusqlite::types::Value::Null) | |
| } | |
| } | |
| } else { | |
| quote! { rusqlite::types::Value::Null } | |
| } | |
| } else { | |
| quote! { | |
| rusqlite::types::Value::from( | |
| serde_json::to_string(&self.#field_name) | |
| .expect(concat!("Failed to serialize field: ", stringify!(`#field_name`))) | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@deps/database/entry/src/lib.rs` around lines 207 - 221, The generated code in
bind_values uses serde_json::to_string(...).unwrap() for complex fields
(occurrences of serde_json::to_string in the quote! blocks), which can panic;
change the generator so bind_values propagates serialization errors instead of
unwrapping: update the bind_values signature to return Result<..., Error> (or
Result<Vec<rusqlite::types::Value>, rusqlite::Error/any appropriate error type)
and replace each .unwrap() with ? or map_err(...) to convert the
serde_json::Error into the chosen error type; ensure both branches that
currently produce
rusqlite::types::Value::from(serde_json::to_string(...).unwrap()) and the
.map(|v| serde_json::to_string(v).unwrap()) form use fallible serialization that
returns Err on failure so the caller can handle the error.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@deps/database/core/src/lib.rs`:
- Around line 144-153: The clear_force function toggles PRAGMA foreign_keys
around a DELETE but is not atomic: if the DELETE fails and re-enabling FKs also
fails you can lose the original error or leave DB in inconsistent state. Modify
clear_force (and reference T::table_name and Entryable) to run the FK toggle and
DELETE inside a robust scope that always restores the original FK state (use a
transaction or a guard pattern) so that foreign_keys is re-enabled even on
panic/early return; capture and propagate both errors correctly (prefer
returning the primary error and chaining the FK-restoration error) to ensure the
DB FK state is always restored.
- Around line 122-134: The delete method currently interpolates the primary key
value directly into the SQL string (in pub fn delete<T: Entryable>, using
T::p_key_column() and obj.p_key()); change it to use a parameterized query
instead: build the query with a placeholder for the p_key (e.g. "DELETE FROM
{table} WHERE {p_key_column} = ?") and pass obj.p_key() as the parameter to
connection.execute rather than embedding it into the SQL string so the value is
safely bound at execution time.
In `@src/app.rs`:
- Around line 224-240: In log_watched_info, don't silently ignore database
errors by calling .ok() on the Results returned from create_table::<Anime>(),
create_table::<WatchHistory>(), upsert(anime.clone()), and upsert(history);
instead handle each Result explicitly: match or use .map_err/.inspect_err to log
failures (e.g., via an existing logger in self.shared_info or tracing::error)
and surface or return errors if appropriate so callers can know when table
creation or upsert failed; update log_watched_info to propagate errors (change
signature to return Result<_, DbError> if necessary) or at minimum record
descriptive error messages including the operation and error value for
create_table::<Anime>(), create_table::<WatchHistory>(), upsert(anime.clone()),
and upsert(history).
In `@src/screens/widgets/popup.rs`:
- Around line 669-674: The info_area Rect sets width using title_area.width - 1
which can underflow for u16 in tiny terminals; update the calculation in the
info_area construction to use title_area.width.saturating_sub(1) (i.e., replace
the subtraction with saturating_sub on title_area.width) so the width never
wraps below zero while keeping the field type consistent.
In `@src/screens/widgets/sync.rs`:
- Around line 47-74: The comment for Navigatable dimensions is misleading: in
SyncPopup::new the call Navigatable::new((1, buttons.len() as u16)) creates 1
column and 2 rows, but the comment says "2 columns, 1 row"; fix this by swapping
the tuple to Navigatable::new((buttons.len() as u16, 1)) so it creates 2 columns
and 1 row (or alternatively update the comment to accurately describe the
current tuple); update the nav initialization in SyncPopup::new accordingly.
- Around line 83-95: Rename the method change_button_lables to
change_button_labels in the impl for the sync widget and update every call site
to use the new name; specifically change the method signature and body name in
src/screens/widgets/sync.rs (the impl that sets self.buttons based on
self.all_selected) and replace any references to change_button_lables with
change_button_labels where the method is invoked so the compiler and callers
remain consistent.
🧹 Nitpick comments (3)
src/app.rs (1)
202-215: Inconsistent brace formatting.The
elseblocks use different formatting styles. Consider consistent formatting for readability.♻️ Suggested formatting
Event::SyncStatus(success, sync, anime ) => { if success { if sync { self.screen_manager.syncing_popup.finished_syncing(*anime); - } - else{ + } else { self.screen_manager.syncing_popup.finished_deleting(*anime); } - } - else { + } else { self.screen_manager.show_error(format!("Failed to sync {}", anime.title)); } - }src/screens/widgets/sync.rs (2)
640-690: Redundant None handling in remote info rendering.The code checks
mal_anime.is_none()at Line 640 and renders "Not added to list." However, theelsebranch (Line 651) immediately matchesmal_anime.as_ref()and handles theNonecase again (Lines 653-656), which is unreachable.♻️ Simplify by removing redundant branch
- if mal_anime.is_none() { - // No remote info available - let no_remote_info = vec![Line::from(vec![Span::styled( - "Not added to list.", - Style::default().fg(Config::global().theme.error), - )])]; - - let remote_paragraph = Paragraph::new(no_remote_info) - .wrap(Wrap { trim: true }) - .block(Block::default().padding(Padding::new(1, 1, 1, 1))); - frame.render_widget(remote_paragraph, remote_text_area); - } else { - let remote_info_text = match mal_anime.as_ref() { - None => vec![Line::from(vec![Span::styled( - "Not added to list.", - Style::default().fg(Config::global().theme.error), - )])], - Some(remote) => vec![ + let remote_info_text = match mal_anime.as_ref() { + None => vec![Line::from(vec![Span::styled( + "Not added to list.", + Style::default().fg(Config::global().theme.error), + )])], + Some(remote) => vec![ Line::from(vec![ Span::raw("Status: "), // ... rest of remote info ]), // ... - ], - }; - - let remote_paragraph = Paragraph::new(remote_info_text) - .wrap(Wrap { trim: true }) - .block(Block::default().padding(Padding::new(1, 1, 1, 1))); - - frame.render_widget(remote_paragraph, remote_text_area); - } + ], + }; + + let remote_paragraph = Paragraph::new(remote_info_text) + .wrap(Wrap { trim: true }) + .block(Block::default().padding(Padding::new(1, 1, 1, 1))); + + frame.render_widget(remote_paragraph, remote_text_area);
716-732: Non-descriptive button labels "plural" and "single".The batch button displays "plural" or "single" which may be confusing to users. Consider more descriptive labels like "All" / "One" or using icons/symbols.
💡 Suggested improvement
let all_btn = if self.all_selected { - "plural" + "[All]" } else { - "single" + "[One]" };Or consider using a checkbox-style indicator like "☑ All" / "☐ All".
| // delete | ||
| pub fn delete<T: Entryable>(&self, obj: &T) -> Result<(), Error> { | ||
| let connection = self.connection.lock().unwrap(); | ||
| let table_name = T::table_name(); | ||
| let condition = format!("{} = {}", T::p_key_column(), obj.p_key()); | ||
| let query = format!( | ||
| "DELETE FROM {} WHERE {}", | ||
| table_name, | ||
| condition | ||
| ); | ||
| connection.execute(&query, [])?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Primary key value should be parameterized.
While p_key_column() addresses the column name concern, the primary key value obj.p_key() is still interpolated directly into SQL (Line 126). This should use a parameterized query for safety and consistency with other methods.
🔒 Proposed fix to parameterize the primary key value
pub fn delete<T: Entryable>(&self, obj: &T) -> Result<(), Error> {
let connection = self.connection.lock().unwrap();
let table_name = T::table_name();
- let condition = format!("{} = {}", T::p_key_column(), obj.p_key());
- let query = format!(
- "DELETE FROM {} WHERE {}",
- table_name,
- condition
- );
- connection.execute(&query, [])?;
+ let query = format!(
+ "DELETE FROM {} WHERE {} = ?1",
+ table_name,
+ T::p_key_column()
+ );
+ connection.execute(&query, [obj.p_key()])?;
Ok(())
}🤖 Prompt for AI Agents
In `@deps/database/core/src/lib.rs` around lines 122 - 134, The delete method
currently interpolates the primary key value directly into the SQL string (in
pub fn delete<T: Entryable>, using T::p_key_column() and obj.p_key()); change it
to use a parameterized query instead: build the query with a placeholder for the
p_key (e.g. "DELETE FROM {table} WHERE {p_key_column} = ?") and pass obj.p_key()
as the parameter to connection.execute rather than embedding it into the SQL
string so the value is safely bound at execution time.
| // clear all rows, ignoring foreign key constraints | ||
| pub fn clear_force<T: Entryable>(&self) -> Result<(), Error> { | ||
| let connection = self.connection.lock().unwrap(); | ||
| let table_name = T::table_name(); | ||
| connection.execute("PRAGMA foreign_keys = OFF", [])?; | ||
| let result = connection.execute(&format!("DELETE FROM {}", table_name), []); | ||
| connection.execute("PRAGMA foreign_keys = ON", [])?; | ||
| result?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Foreign key toggle is not atomic with the delete operation.
If the delete fails (Line 149), foreign keys are re-enabled (Line 150), but if re-enabling also fails, the error from the delete is returned while the FK state might be inconsistent. Consider using a transaction or ensuring the FK state is always restored.
🛡️ Proposed fix using a guard pattern
pub fn clear_force<T: Entryable>(&self) -> Result<(), Error> {
let connection = self.connection.lock().unwrap();
let table_name = T::table_name();
connection.execute("PRAGMA foreign_keys = OFF", [])?;
- let result = connection.execute(&format!("DELETE FROM {}", table_name), []);
- connection.execute("PRAGMA foreign_keys = ON", [])?;
- result?;
+ let result = connection.execute(&format!("DELETE FROM {}", table_name), []);
+ // Always re-enable foreign keys, even if delete failed
+ let _ = connection.execute("PRAGMA foreign_keys = ON", []);
+ result?;
Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // clear all rows, ignoring foreign key constraints | |
| pub fn clear_force<T: Entryable>(&self) -> Result<(), Error> { | |
| let connection = self.connection.lock().unwrap(); | |
| let table_name = T::table_name(); | |
| connection.execute("PRAGMA foreign_keys = OFF", [])?; | |
| let result = connection.execute(&format!("DELETE FROM {}", table_name), []); | |
| connection.execute("PRAGMA foreign_keys = ON", [])?; | |
| result?; | |
| Ok(()) | |
| } | |
| // clear all rows, ignoring foreign key constraints | |
| pub fn clear_force<T: Entryable>(&self) -> Result<(), Error> { | |
| let connection = self.connection.lock().unwrap(); | |
| let table_name = T::table_name(); | |
| connection.execute("PRAGMA foreign_keys = OFF", [])?; | |
| let result = connection.execute(&format!("DELETE FROM {}", table_name), []); | |
| // Always re-enable foreign keys, even if delete failed | |
| let _ = connection.execute("PRAGMA foreign_keys = ON", []); | |
| result?; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In `@deps/database/core/src/lib.rs` around lines 144 - 153, The clear_force
function toggles PRAGMA foreign_keys around a DELETE but is not atomic: if the
DELETE fails and re-enabling FKs also fails you can lose the original error or
leave DB in inconsistent state. Modify clear_force (and reference T::table_name
and Entryable) to run the FK toggle and DELETE inside a robust scope that always
restores the original FK state (use a transaction or a guard pattern) so that
foreign_keys is re-enabled even on panic/early return; capture and propagate
both errors correctly (prefer returning the primary error and chaining the
FK-restoration error) to ensure the DB FK state is always restored.
| fn log_watched_info(&self, anime: &Anime, details: player::PlayResult) { | ||
| let now: DateTime<Local> = Local::now(); | ||
| let timestamp = now.format("%Y-%m-%d %H:%M:%S"); | ||
| let log_file = app_dir.join("watch_history"); | ||
| let log_entry = format!( | ||
| "{} -> {} -> \"{}\" -> {} -> {}/{} -> {} -> {}\n", | ||
| timestamp, | ||
| anime.id, | ||
| anime.title, | ||
| details.episode, | ||
| details.current_time, | ||
| details.total_time, | ||
| details.percentage, | ||
| details.completed | ||
| ); | ||
|
|
||
| let mut file = OpenOptions::new() | ||
| .create(true) | ||
| .append(true) | ||
| .open(log_file) | ||
| .expect("Failed to open log file"); | ||
|
|
||
| file.write_all(log_entry.as_bytes()).ok(); | ||
| let history = WatchHistory { | ||
| id: 0, | ||
| anime_id: anime.id, | ||
| timestamp: now.format("%Y-%m-%d %H:%M:%S").to_string(), | ||
| episode: details.episode as i32, | ||
| current_time: details.current_time, | ||
| total_time: details.total_time, | ||
| percentage: details.percentage, | ||
| is_completed: details.is_completed, | ||
| }; | ||
| self.shared_info.local_db.create_table::<Anime>().ok(); | ||
| self.shared_info.local_db.create_table::<WatchHistory>().ok(); | ||
| self.shared_info.local_db.upsert(anime.clone()).ok(); | ||
| self.shared_info.local_db.upsert(history).ok(); | ||
| } |
There was a problem hiding this comment.
Silent error handling may hide database issues.
Using .ok() on Lines 236-239 silently discards any database errors. If table creation or upsert fails, the user won't know their watch history wasn't saved.
🛠️ Suggested improvement to log errors
- self.shared_info.local_db.create_table::<Anime>().ok();
- self.shared_info.local_db.create_table::<WatchHistory>().ok();
- self.shared_info.local_db.upsert(anime.clone()).ok();
- self.shared_info.local_db.upsert(history).ok();
+ if let Err(e) = self.shared_info.local_db.create_table::<Anime>() {
+ errorBus::push(format!("Failed to create Anime table: {}", e));
+ }
+ if let Err(e) = self.shared_info.local_db.create_table::<WatchHistory>() {
+ errorBus::push(format!("Failed to create WatchHistory table: {}", e));
+ }
+ if let Err(e) = self.shared_info.local_db.upsert(anime.clone()) {
+ errorBus::push(format!("Failed to save anime: {}", e));
+ }
+ if let Err(e) = self.shared_info.local_db.upsert(history) {
+ errorBus::push(format!("Failed to save watch history: {}", e));
+ }🤖 Prompt for AI Agents
In `@src/app.rs` around lines 224 - 240, In log_watched_info, don't silently
ignore database errors by calling .ok() on the Results returned from
create_table::<Anime>(), create_table::<WatchHistory>(), upsert(anime.clone()),
and upsert(history); instead handle each Result explicitly: match or use
.map_err/.inspect_err to log failures (e.g., via an existing logger in
self.shared_info or tracing::error) and surface or return errors if appropriate
so callers can know when table creation or upsert failed; update
log_watched_info to propagate errors (change signature to return Result<_,
DbError> if necessary) or at minimum record descriptive error messages including
the operation and error value for create_table::<Anime>(),
create_table::<WatchHistory>(), upsert(anime.clone()), and upsert(history).
| let info_area = Rect { | ||
| x: title_area.x, | ||
| y: info_area.y, | ||
| width: title_area.width-1, | ||
| width: title_area.width - 1, | ||
| height: info_area.height.saturating_sub(buttons_area.height), | ||
| }; |
There was a problem hiding this comment.
Avoid potential u16 underflow in info area width.
Line 672 subtracts 1 from a u16. If the width ever reaches 0 in tiny terminals, this will wrap. Prefer saturating_sub(1).
🐛 Safer width calculation
- width: title_area.width - 1,
+ width: title_area.width.saturating_sub(1),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let info_area = Rect { | |
| x: title_area.x, | |
| y: info_area.y, | |
| width: title_area.width-1, | |
| width: title_area.width - 1, | |
| height: info_area.height.saturating_sub(buttons_area.height), | |
| }; | |
| let info_area = Rect { | |
| x: title_area.x, | |
| y: info_area.y, | |
| width: title_area.width.saturating_sub(1), | |
| height: info_area.height.saturating_sub(buttons_area.height), | |
| }; |
🤖 Prompt for AI Agents
In `@src/screens/widgets/popup.rs` around lines 669 - 674, The info_area Rect sets
width using title_area.width - 1 which can underflow for u16 in tiny terminals;
update the calculation in the info_area construction to use
title_area.width.saturating_sub(1) (i.e., replace the subtraction with
saturating_sub on title_area.width) so the width never wraps below zero while
keeping the field type consistent.
| impl SyncPopup { | ||
| pub fn new(info: ExtraInfo) -> Self { | ||
| let buttons = vec![ | ||
| "Sync to MAL".to_string(), | ||
| "Remove local changes".to_string(), | ||
| ]; | ||
|
|
||
| let image_manager = Arc::new(Mutex::new(ImageManager::new())); | ||
|
|
||
| ImageManager::init_with_threads(&image_manager, info.app_sx.clone()); | ||
|
|
||
| Self { | ||
| toggled: false, | ||
| syncing: false, | ||
| current_anime: 0, | ||
| animes_to_sync: Vec::new(), | ||
| to_be_processed: Vec::new(), | ||
| finished_processing: Vec::new(), | ||
| nav: Navigatable::new((1, buttons.len() as u16)), // 2 columns, 1 row | ||
| buttons, | ||
| image_manager, | ||
| info, | ||
| all_selected: false, | ||
| focus: FocusedElement::SyncButtons, | ||
| back_btn_area: None, | ||
| batch_btn_area: None, | ||
| } | ||
| } |
There was a problem hiding this comment.
Comment doesn't match Navigatable dimensions.
Line 65 comment says "2 columns, 1 row" but the constructor Navigatable::new((1, buttons.len() as u16)) appears to create 1 column with 2 rows based on the tuple order.
📝 Fix the misleading comment
- nav: Navigatable::new((1, buttons.len() as u16)), // 2 columns, 1 row
+ nav: Navigatable::new((1, buttons.len() as u16)), // 1 column, 2 rows (or verify tuple order)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl SyncPopup { | |
| pub fn new(info: ExtraInfo) -> Self { | |
| let buttons = vec![ | |
| "Sync to MAL".to_string(), | |
| "Remove local changes".to_string(), | |
| ]; | |
| let image_manager = Arc::new(Mutex::new(ImageManager::new())); | |
| ImageManager::init_with_threads(&image_manager, info.app_sx.clone()); | |
| Self { | |
| toggled: false, | |
| syncing: false, | |
| current_anime: 0, | |
| animes_to_sync: Vec::new(), | |
| to_be_processed: Vec::new(), | |
| finished_processing: Vec::new(), | |
| nav: Navigatable::new((1, buttons.len() as u16)), // 2 columns, 1 row | |
| buttons, | |
| image_manager, | |
| info, | |
| all_selected: false, | |
| focus: FocusedElement::SyncButtons, | |
| back_btn_area: None, | |
| batch_btn_area: None, | |
| } | |
| } | |
| impl SyncPopup { | |
| pub fn new(info: ExtraInfo) -> Self { | |
| let buttons = vec![ | |
| "Sync to MAL".to_string(), | |
| "Remove local changes".to_string(), | |
| ]; | |
| let image_manager = Arc::new(Mutex::new(ImageManager::new())); | |
| ImageManager::init_with_threads(&image_manager, info.app_sx.clone()); | |
| Self { | |
| toggled: false, | |
| syncing: false, | |
| current_anime: 0, | |
| animes_to_sync: Vec::new(), | |
| to_be_processed: Vec::new(), | |
| finished_processing: Vec::new(), | |
| nav: Navigatable::new((1, buttons.len() as u16)), // 1 column, 2 rows (or verify tuple order) | |
| buttons, | |
| image_manager, | |
| info, | |
| all_selected: false, | |
| focus: FocusedElement::SyncButtons, | |
| back_btn_area: None, | |
| batch_btn_area: None, | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/screens/widgets/sync.rs` around lines 47 - 74, The comment for
Navigatable dimensions is misleading: in SyncPopup::new the call
Navigatable::new((1, buttons.len() as u16)) creates 1 column and 2 rows, but the
comment says "2 columns, 1 row"; fix this by swapping the tuple to
Navigatable::new((buttons.len() as u16, 1)) so it creates 2 columns and 1 row
(or alternatively update the comment to accurately describe the current tuple);
update the nav initialization in SyncPopup::new accordingly.
| fn change_button_lables(&mut self) { | ||
| if self.all_selected { | ||
| self.buttons = vec![ | ||
| "Sync to MAL (all)".to_string(), | ||
| "Remove local changes (all)".to_string(), | ||
| ]; | ||
| } else { | ||
| self.buttons = vec![ | ||
| "Sync to MAL".to_string(), | ||
| "Remove local changes".to_string(), | ||
| ]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Typo in method name: "lables" should be "labels".
📝 Fix typo
- fn change_button_lables(&mut self) {
+ fn change_button_labels(&mut self) {Also update the call sites at lines 231 and 310.
🤖 Prompt for AI Agents
In `@src/screens/widgets/sync.rs` around lines 83 - 95, Rename the method
change_button_lables to change_button_labels in the impl for the sync widget and
update every call site to use the new name; specifically change the method
signature and body name in src/screens/widgets/sync.rs (the impl that sets
self.buttons based on self.all_selected) and replace any references to
change_button_lables with change_button_labels where the method is invoked so
the compiler and callers remain consistent.
1 participant
Added option to use this TUI without logging in.
Saves watch history and changes to a local sqlite database instead of just a file.
Syncs local changes to MyAnimeList when/if logging in after changing anything locally.
Change name to mal-tui cus mal-cli was taken.
Small fixes
Added homepage
Summary by CodeRabbit
New Features
Improvements