Allow switching category automatically to make torrents linked#14
Open
StirlingMouse wants to merge 26 commits intodioxus-switchfrom
Open
Allow switching category automatically to make torrents linked#14StirlingMouse wants to merge 26 commits intodioxus-switchfrom
StirlingMouse wants to merge 26 commits intodioxus-switchfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds logic to compute and apply qBittorrent categories for ignored torrents (based on linked libraries and tag-filter matches), hooks the update into metadata finalization, canonicalizes TorrentMeta earlier in update flows, and introduces multiple web, metadata, DB, provider, UI, Docker, and test changes across the codebase. Changes
Sequence DiagramsequenceDiagram
participant AutoGrabber as AutoGrabber
participant QBit as qBittorrent API
participant DB as Database
AutoGrabber->>AutoGrabber: finalize_torrent_meta_update(commit metadata)
AutoGrabber->>QBit: get_torrent(torrent_id)
QBit-->>AutoGrabber: torrent_info (including current_category)
AutoGrabber->>AutoGrabber: compute replacement_category_for_ignored_torrent(meta, linked_categories, tag_matches, current_category)
AutoGrabber->>DB: read library/tag filter info (linked_library_categories)
DB-->>AutoGrabber: library/tag filters
AutoGrabber->>QBit: ensure_category_exists(target_category)
QBit-->>AutoGrabber: category_confirmed
AutoGrabber->>QBit: set_torrent_category(torrent_id, target_category)
QBit-->>AutoGrabber: set_result
AutoGrabber->>DB: persist updated mlm_db::Torrent.category
DB-->>AutoGrabber: persist_result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlm_core/src/autograbber.rs`:
- Around line 778-782: The code clones an out-of-transaction snapshot
(`torrent`) and upserts it inside a new write transaction, risking overwriting
newer changes; instead, inside the transaction returned by db.rw_async() re-read
the current torrent record (e.g., via rw.get or equivalent) to obtain the fresh
state, set its category to new_category, then call rw.upsert(updated_torrent)
and rw.commit() so you upsert the latest snapshot rather than the stale
`torrent` clone.
- Line 895: The call to update_ignored_qbit_category should be made best-effort:
do not propagate errors after persistence; instead await the call, catch any
Err, log a warning with the error and context (including torrent id/title and
that metadata was already committed), and continue without returning the error
so later ABS/event finalization runs; modify the invocation of
update_ignored_qbit_category(...) to handle its Result (swallowing/falling back
on error) and emit a warning rather than using the ? operator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d6030b4-0c64-4d6c-ad9f-51e26994619f
📒 Files selected for processing (5)
mlm_core/src/autograbber.rsmlm_web_api/src/lib.rsmlm_web_askama/src/lib.rsmlm_web_askama/src/pages/torrent.rsmlm_web_dioxus/src/search.rs
StirlingMouse
force-pushed
the
upgrade-torrents-to-linked
branch
from
88fb449 to
3812c28
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mlm_core/src/autograbber.rs (1)
778-785:⚠️ Potential issue | 🔴 CriticalDon't recreate a deleted torrent from the pre-transaction snapshot.
If the row disappears between the metadata commit and this follow-up write,
unwrap_or_else(|| torrent.clone())re-inserts it. That can silently undo a concurrent delete just because recategorization ran later. Return early when the re-read misses instead of upserting the fallback snapshot.Suggested fix
let (_guard, rw) = db.rw_async().await?; - let mut updated_torrent = rw - .get() - .primary::<mlm_db::Torrent>(torrent.id.clone())? - .unwrap_or_else(|| torrent.clone()); + let Some(mut updated_torrent) = rw.get().primary::<mlm_db::Torrent>(torrent.id.clone())? else { + return Ok(()); + }; updated_torrent.category = Some(new_category.to_string()); rw.upsert(updated_torrent)?; rw.commit()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlm_core/src/autograbber.rs` around lines 778 - 785, The current code uses unwrap_or_else(|| torrent.clone()) when re-reading the torrent in db.rw_async()/rw.get().primary, which will recreate a row that was deleted concurrently; instead, check the primary lookup result and return early (or skip the update) if it is None. Change the logic around rw.get().primary::<mlm_db::Torrent>(torrent.id.clone()) to match on Option — e.g., if Some(mut updated_torrent) { updated_torrent.category = Some(new_category.to_string()); rw.upsert(updated_torrent)?; rw.commit()? } else { return Ok(()) /* or otherwise skip/abort the recategorization */ } so you do not upsert the pre-transaction snapshot when the row no longer exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mlm_core/src/autograbber.rs`:
- Around line 765-769: The replacement category chosen by
replacement_category_for_ignored_torrent must be validated against the actual
library matching logic before being applied: after computing new_category, call
the same resolver used in linker/torrent.rs (e.g., find_library or the
equivalent library lookup that enforces allow_tags/deny_tags) with the live
qbit_torrent (including qbit_torrent.tags) to ensure a LibraryByCategory
actually accepts the torrent; only call set_category when that lookup returns a
matching library, otherwise skip/apply a safe fallback and log the rejection.
Also add a regression test that sets LibraryByCategory.tag_filters (non-empty
allow/deny tags) and confirms the replacement category is rejected when tags
don’t match.
---
Duplicate comments:
In `@mlm_core/src/autograbber.rs`:
- Around line 778-785: The current code uses unwrap_or_else(|| torrent.clone())
when re-reading the torrent in db.rw_async()/rw.get().primary, which will
recreate a row that was deleted concurrently; instead, check the primary lookup
result and return early (or skip the update) if it is None. Change the logic
around rw.get().primary::<mlm_db::Torrent>(torrent.id.clone()) to match on
Option — e.g., if Some(mut updated_torrent) { updated_torrent.category =
Some(new_category.to_string()); rw.upsert(updated_torrent)?; rw.commit()? } else
{ return Ok(()) /* or otherwise skip/abort the recategorization */ } so you do
not upsert the pre-transaction snapshot when the row no longer exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df2edebb-8611-4658-84d3-acedd195d6b2
📒 Files selected for processing (5)
mlm_core/src/autograbber.rsmlm_web_api/src/lib.rsmlm_web_askama/src/lib.rsmlm_web_askama/src/pages/torrent.rsmlm_web_dioxus/src/search.rs
StirlingMouse
force-pushed
the
dioxus-switch
branch
from
bca5e5a to
1251f4c
Compare
StirlingMouse
force-pushed
the
upgrade-torrents-to-linked
branch
from
3812c28 to
83555dd
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
mlm_web_dioxus/src/replaced/components.rs (1)
487-497: 🧹 Nitpick | 🔵 TrivialConsider using
.iter().enumerate()to avoid unnecessary cloning.The pattern
.clone().into_iter().enumerate()clones the entire collection before iterating. For small collections in UI code this is fine, but.iter().enumerate()would be more efficient:-for (i, author) in pair.torrent.meta.authors.clone().into_iter().enumerate() { +for (i, author) in pair.torrent.meta.authors.iter().enumerate() {The same applies to other similar loops in this file (lines 502, 517, 549, 570, 580, 590).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlm_web_dioxus/src/replaced/components.rs` around lines 487 - 497, Replace the collection-level clone+into_iter with iterator-by-reference to avoid cloning the whole vector: change uses of pair.torrent.meta.authors.clone().into_iter().enumerate() to pair.torrent.meta.authors.iter().enumerate() and, where a owned String is required (e.g., FilterLink { field: ReplacedPageFilter::Author, value: author.clone(), ... } and the "{author}" display), clone or to_owned the individual author item instead of cloning the entire authors collection; apply the same change to the other loops noted (the ones around the other FilterLink usages on the listed lines).mlm_web_dioxus/src/list.rs (1)
350-393:⚠️ Potential issue | 🟡 MinorRadio buttons don't reflect the current filter state.
The "All" radio has
checked: truehardcoded, but the other radios don't have theircheckedattribute bound to theshowsignal state. If the user selects "Audio Missing" and the component re-renders, the visual state won't match the actual filter.🐛 Proposed fix to bind radio checked state
label { "All" input { r#type: "radio", name: "show", - checked: true, + checked: props.data.page_size == 500 && /* need show state propagated */, onclick: move |_| { props.on_filter_change.call(None); }, } }Consider passing the current
showfilter value intoListPageContentPropsso each radio can compute itscheckedstate correctly:// In ListPageContentProps: show: Option<String>, // In each radio: checked: props.show.is_none(), // for "All" checked: props.show.as_deref() == Some("any"), // for "Any Missing" // etc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlm_web_dioxus/src/list.rs` around lines 350 - 393, The radio inputs always use a hardcoded checked state so UI can drift from the actual filter; add a show: Option<String> field to ListPageContentProps and pass the current filter into the component, then update each radio's checked to be computed from props.show (e.g., checked true when props.show.is_none() for "All", checked when props.show.as_deref() == Some("any") for "Any Missing", etc.), leaving props.on_filter_change calls as-is so selection updates both state and visual checked rendering.mlm_web_dioxus/src/torrents/components.rs (1)
559-564: 🧹 Nitpick | 🔵 TrivialVerify signal shadowing is intentional.
The
let mut from = from;andlet mut page_size = page_size;lines shadow the outer scope signals inside theuse_effectclosure. While this works since you're rebinding the signal handles to mutable locals for use inset()calls, it adds cognitive overhead. This pattern appears in other signal variables (sort,asc, etc.) as well, so it seems intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlm_web_dioxus/src/torrents/components.rs` around lines 559 - 564, The closure is shadowing outer signals by rebinding names (sort, asc, filters_signal, show, from, page_size) to mutable locals which is confusing; either remove the shadowing and use the original signal handles directly inside the use_effect closure, or rename the local bindings to explicit names (e.g., sort_signal, asc_signal, from_signal, page_size_signal) so it's clear these are signal handles used with set() calls in the use_effect closure; update all references inside the closure (and any set() calls) to the new names and remove unnecessary mutability.mlm_core/src/snatchlist.rs (1)
313-328: 🧹 Nitpick | 🔵 TrivialRedundant condition:
if !diff.is_empty()is now always true.Since line 297-299 returns early when
diff.is_empty(), this condition at line 313 will always be true when reached. Consider removing the conditional wrapper.♻️ Proposed simplification
- if !diff.is_empty() { - let mam_id = mam_torrent.map(|m| m.id); - write_event( - db, - events, - Event::new( - Some(id), - mam_id, - EventType::Updated { - fields: diff, - source: (meta.source.clone(), String::new()), - }, - ), - ) - .await; - } + let mam_id = mam_torrent.map(|m| m.id); + write_event( + db, + events, + Event::new( + Some(id), + mam_id, + EventType::Updated { + fields: diff, + source: (meta.source.clone(), String::new()), + }, + ), + ) + .await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlm_core/src/snatchlist.rs` around lines 313 - 328, The check if !diff.is_empty() is redundant because earlier code returns when diff.is_empty(); remove the surrounding if and unconditionally call write_event(...) with mam_id derived from mam_torrent.map(|m| m.id) and the Event::new(..., EventType::Updated { fields: diff, source: (meta.source.clone(), String::new()) }). Ensure you preserve the await on write_event and keep the mam_id mapping and Event construction exactly as in the current block.mlm_core/src/autograbber.rs (1)
846-847:⚠️ Potential issue | 🟠 MajorDon't return
Unchangedbefore the linker backfill runs.
select_torrentsstill calls this path whenCost::MetadataOnlyAddneeds to populatetorrent.linkerfrommam_torrent.owner_nameeven if the metadata is identical. The new fast-path returnsUnchangedbefore thelinker_is_ownerblock executes, so those torrents never get a linker and remain unlinked. Move the equality fast-path after the linker update, or treat a linker-only change as a silent update.Also applies to: 890-895
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlm_core/src/autograbber.rs` around lines 846 - 847, The early fast-path that returns PreparedTorrentMetaUpdate::Unchanged when meta == torrent.meta is short-circuiting the linker backfill; modify the logic in the update path used by select_torrents (when handling Cost::MetadataOnlyAdd) so that linker backfill runs before returning Unchanged — either move the equality check after the linker_is_owner/linker population block that reads mam_torrent.owner_name into torrent.linker, or detect a linker-only change and treat it as a silent update rather than returning Unchanged; ensure references to meta, torrent.meta, torrent.linker, linker_is_owner, and PreparedTorrentMetaUpdate::Unchanged are updated accordingly so torrents get a linker populated even when other metadata is identical.mlm_web_dioxus/src/torrent_detail/components.rs (1)
942-950:⚠️ Potential issue | 🟠 MajorMissing
mam_idprop causes MaM provider option to never appear.The
MatchDialogcomponent expects anmam_id: Option<u64>parameter (defined at line 958), but it's not passed here. This means the MaM provider will never be available in the dialog's provider list, even when the torrent has a MaM ID.🐛 Proposed fix
if *dialog_open.read() { MatchDialog { torrent_id: torrent_id.clone(), providers: providers.clone(), + mam_id: torrent.mam_id, status_msg, on_close: move |_| dialog_open.set(false), on_refresh, } }Note: You'll need to capture
torrent.mam_idbefore destructuring or pass it explicitly. You may need to store it in a variable earlier (e.g., near line 347 wheretorrent_idis captured).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mlm_web_dioxus/src/torrent_detail/components.rs` around lines 942 - 950, The MatchDialog is missing the mam_id prop so MaM providers never show; when building the dialog (the MatchDialog {...} block using torrent_id, providers, status_msg, on_close, on_refresh) capture or retrieve the torrent's mam_id (e.g., store torrent.mam_id in a variable where torrent_id is captured) and pass it into the component as mam_id: mam_id.clone() (or mam_id: torrent.mam_id.clone() if available) so the MatchDialog's mam_id: Option<u64> parameter is populated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.dockerignore:
- Around line 1-18: Update the .dockerignore to exclude environment files by
adding patterns like .env and .env.* so local secret/config files are not sent
to the Docker build context; open the .dockerignore (file shown in the diff) and
append entries for .env and .env.* (and any other local env filenames your repo
uses) to ensure they are ignored during docker builds.
In `@Dockerfile`:
- Around line 32-41: Create and use an unprivileged user in the final Docker
stage: add a non-root group/user (e.g., mlm:mlm), ensure writable directories
referenced by ENV values or COPY targets (like /data, /config, /server/assets,
/dioxus-public and the /mlm binary location if needed) are chown'd/chmod'd to
that user, then set USER mlm before CMD ["/mlm"] so the container runs without
root privileges; update the Dockerfile's final stage after the COPY lines to
perform these steps.
In `@mlm_core/src/autograbber.rs`:
- Around line 839-843: Don't unconditionally copy stored IDs/tags back into the
incoming meta; instead preserve caller-supplied values from the new torrent-edit
flow (update_torrent_meta). In the block around torrent.meta.canonicalize(),
meta.canonicalize(), and the lines touching meta.ids/meta.tags, change the logic
so that torrent.meta.ids is only merged when meta.ids.is_empty() (or use extend
only when no caller IDs exist) and torrent.meta.tags is only assigned when
meta.tags.is_empty(); leave meta.ids and meta.tags untouched when the incoming
meta already contains values. This preserves user-edited
ABS/ASIN/Goodreads/MaM/tag fields while keeping canonicalization behavior.
In `@mlm_core/src/metadata/mam_meta.rs`:
- Around line 29-35: The metadata lock is held across an await
(ctx.ssr().metadata.lock().await.fetch_provider(...).await), which serializes
all metadata access; instead, acquire the MutexGuard from
ctx.ssr().metadata.lock().await only long enough to read/clone the minimal data
needed (e.g., any provider config or handles), then drop the guard before
calling fetch_provider so the long async call runs outside the critical section;
specifically, read/clone whatever fetch_provider needs while holding the lock
and then call fetch_provider (or an equivalent async helper) after releasing the
guard so cheap reads like enabled_providers() are not blocked.
In `@mlm_db/src/impls/categories.rs`:
- Around line 6-17: MediaType::all() currently allocates a new Vec on every
call; change its signature from pub fn all() -> Vec<MediaType> to pub fn all()
-> &'static [MediaType] and return a static slice (e.g. define a static/const
array or slice like ALL: [MediaType; 9] or static ALL: &'static [MediaType] =
&[..] and have all() return &ALL) so callers can iterate without heap
allocation; update any callers that expect a Vec to work with a slice (or call
.to_vec() where a Vec is actually required).
In `@mlm_db/src/impls/meta.rs`:
- Around line 45-50: The diff() method clones self and other and calls
canonicalize(), but canonicalize currently only normalizes categories; extend
canonicalize (or add helper normalization used by diff) to also canonicalize
tags, authors, narrators, and filetypes (e.g., sort and optionally
deduplicate/trim entries) so ordering differences in those lists won't produce
spurious TorrentMetaDiffs; update the canonicalize implementation referenced by
diff() and ensure both this.canonicalize() and other.canonicalize() normalize
the additional fields (tags, authors, narrators, filetypes).
In `@mlm_db/src/impls/old_categories.rs`:
- Around line 44-55: The all() function currently pushes categories by repeated
extend calls without reserving capacity; compute the total number of items by
summing lengths of AudiobookCategory::all(), EbookCategory::all(),
MusicologyCategory::all(), and RadioCategory::all() (or call their .len() if
available) and call categories.reserve(total) before the extend calls, then
proceed with the existing extends (Category::Audio, Category::Ebook,
Category::Musicology, Category::Radio) to avoid unnecessary reallocations.
In `@mlm_web_dioxus/src/components/query_params.rs`:
- Around line 131-148: The cache timestamp granularity is currently computed as
seconds (now = (performance.now() / 1000.0) as u32) causing up to 1s staleness;
change the timestamp to use milliseconds (e.g., use performance.now() directly
or multiply to get ms) and widen the timestamp type stored in PATHNAME_CACHE
(and the local now variable) so comparisons use millisecond precision; update
the code paths that read/store the cache (the now local, the PATHNAME_CACHE
entry, and the time == now comparison) to use the new integer type to avoid
second-level staleness during rapid navigation.
In `@mlm_web_dioxus/src/main.rs`:
- Around line 103-120: The MetadataService is being partially configured here:
create MetadataService via MetadataService::from_settings(...) and then
separately calling metadata.lock().await.register_mam(...); move MaM provider
wiring into the MetadataService constructor/factory so provider enabling and
timeout defaults are centralized. Modify MetadataService::from_settings (or add
a new factory like MetadataService::from_settings_with_mam) to accept the mam
provider (Option<&MaMType> or Arc/cloneable handle) and default_timeout, perform
any register_mam logic internally, and return a fully configured service; then
remove the external metadata.lock().await.register_mam(...) call from the
main.rs setup and update SsrBackend construction to use the resulting metadata
as before.
In `@mlm_web_dioxus/src/search.rs`:
- Around line 288-289: The code currently applies pagination before performing
the "series" sort (check the sort == "series" branch near the function returning
Result<SearchData, ServerFnError> with parameter from: usize), causing incorrect
global ordering across pages; fix by moving the "series" ordering to operate on
the full result set (fetch all matching items before slicing) or by pushing the
series sort down into the paginated data source/query so each page reflects the
global series order; update the same logic in the other affected blocks (around
the ranges corresponding to lines 304-308 and 344-356) to ensure sorting happens
before pagination or is implemented server-side.
- Around line 358-364: The total field is currently inflated by including the
unchecked from offset; change the total calculation in SearchData to rely only
on MaM counts (use result.found.max(result.total) and remove the
.max(from.saturating_add(page_len)) term) and do not derive total from
from/page_len. Additionally, add logic around the paging handler that validates
or clamps the requested offset (from) — e.g., if from >= total then clamp to the
last page or return/redirect to a valid page — so out-of-range page parameters
are handled separately from computing SearchData::total.
In `@mlm_web_dioxus/src/selected/server_fns.rs`:
- Around line 20-22: Remove the unnecessary #[allow(unused_imports)] attribute
on the import of SelectedMeta and SelectedRow; the types are used (SelectedRow
is the return type of convert_selected_row and both SelectedMeta and SelectedRow
are used when constructing structs), so simply delete the
#[allow(unused_imports)] line above the use super::types::{SelectedMeta,
SelectedRow} import to allow the compiler to correctly reflect actual usage.
In `@mlm_web_dioxus/src/torrent_detail/server_fns.rs`:
- Around line 275-286: The current abs_item_url construction uses an empty
string when config.audiobookshelf is None, producing a relative/malformed URL;
change the logic so abs_item_url is only Some when config.audiobookshelf exists:
check config.audiobookshelf (e.g., with and_then / if let Some(base) or map on
audiobookshelf) before formatting the URL so that torrent.meta.ids.get(ids::ABS)
is combined with the actual base URL only when config.audiobookshelf is Some,
otherwise leave abs_item_url as None; update the expression around abs_item_url,
config.audiobookshelf, and torrent.meta.ids.get(ids::ABS) accordingly.
- Around line 682-701: The match on d.field.as_str() currently falls through to
TorrentMetaField::Title for unknown names which can hide bugs; update the
fallback branch (the _ arm in the match) to log a warning containing d.field
(e.g., using tracing::warn! or log::warn!) that reports the unknown field name
and that you're defaulting to Title, then still return TorrentMetaField::Title
(or change to a dedicated Unknown variant if you prefer); this touches the match
block that maps string names to the TorrentMetaField enum.
- Around line 629-637: The code currently discards series entries by
constructing Series with SeriesEntries::new(vec![]) from merged_meta.series;
instead, map each DTO (s) to Series by calling Series::try_from((s.name.clone(),
s.entries.clone())) to parse the entries string and produce proper
SeriesEntries, and if try_from returns an Err fallback to Series { name:
s.name.clone(), entries: SeriesEntries::new(vec![]) } so parsing failures are
handled gracefully; replace the current map that constructs empty entries with
this try_from-with-fallback logic referencing merged_meta.series, Series,
Series::try_from, and SeriesEntries::new.
In `@mlm_web_dioxus/src/torrent_edit.rs`:
- Around line 409-430: The meta.ids map is being updated from the form (via
upsert_known_id) but the torrent.mam_id field is left unchanged, so the
TorrentKey::mam_id index will become stale; parse and validate the form's mam_id
the same way used elsewhere (e.g. the codepath that reads/validates mam IDs),
then assign the validated value into torrent.mam_id before calling
update_torrent_meta() (in addition to setting meta.ids and meta.source),
ensuring the handler updates both meta.ids["mam"] and the top-level
torrent.mam_id to keep the secondary index in sync.
- Around line 715-718: The banner currently uses "torrent-edit-banner
loading-indicator" whenever is_error is false, which causes successful saves to
still be treated as loading; update the rendering logic around status_msg.read()
(the tuple extracted as (msg, is_error) in the p element) so that the class is
"torrent-edit-banner error" when is_error is true, "torrent-edit-banner
loading-indicator" only when an explicit in-flight/loading state is true
(introduce or check a loading flag if needed), and "torrent-edit-banner success"
(or another non-loading class) for completed successful saves; adjust the p
element's class expression to use these conditions instead of defaulting to
loading-indicator for non-error.
In `@server/assets/style.css`:
- Around line 1049-1059: The .sr-only rule uses the deprecated clip property;
replace the clip declaration in the .sr-only selector with a modern clip-path
value (e.g., clip-path: inset(50%);) and optionally add a vendor-prefixed
fallback (e.g., -webkit-clip-path) if you need older WebKit support; update the
.sr-only block by removing the clip: rect(...) line and inserting clip-path:
inset(50%); (and -webkit-clip-path: inset(50%); if desired) so the
screen-reader-only behavior is preserved using the modern API.
In `@server/src/bin/create_test_db.rs`:
- Around line 4-7: The import list in create_test_db.rs includes unused symbols
ListItemTorrent and TorrentStatus; remove these two from the imported items to
clean up unused imports. Locate the import block that currently lists
DuplicateTorrent, ErroredTorrent, ErroredTorrentId, Event, EventType, List,
ListItem, ListItemTorrent, MODELS, MainCat, MediaType, MetadataSource,
SelectedTorrent, Series, SeriesEntries, SeriesEntry, Size, Timestamp, Torrent,
TorrentCost, TorrentMeta, TorrentStatus, Uuid, migrate and delete
ListItemTorrent and TorrentStatus from that comma-separated list so only used
types remain. Ensure the file still compiles after removal and run cargo check
to verify no remaining references to those symbols.
In `@server/src/main.rs`:
- Around line 248-250: Add a short safety comment explaining why the unsafe
block around std::env::set_var(&dioxus_public_path) is sound: note that
std::env::set_var is marked unsafe in Rust 2024 due to potential concurrent
access with std::env::var, but here it is called early in app_main before any
threads/tasks are spawned (so there is no concurrent access), and therefore the
operation is safe; place the comment immediately above the unsafe block that
sets DIOXUS_PUBLIC_PATH.
In `@server/tests/metadata_integration.rs`:
- Around line 255-273: The test currently only checks that fetch_provider
returned Ok, which can hide fallbacks; after calling
metadata.lock().await.fetch_provider(&ctx, q_with_id, "mam").await and
unwrapping the result, assert the returned metadata actually came from the
direct-ID path by checking the MaM ID and/or title in the response (e.g. that
the metadata contains mlm_db::ids::MAM => "12345" or the mock's updated title).
Update the assertion to inspect the returned Metadata/ProviderRecord (from
fetch_provider's result) and assert the id map contains "12345" for the MAM key
(or assert the title equals the mock-updated title) so the test fails if
get_torrent_info_by_id is not used.
- Around line 165-197: Replace the fixed-port + fixed-sleep startup with a
deterministic acquire-and-wait flow: allocate an unused port by binding a
TcpListener to "127.0.0.1:0" and reading the assigned port (use that to set
mock_port/mock_url), spawn mock_server with that port, then poll for readiness
by repeatedly checking the child with mock_server.try_wait() and performing an
HTTP GET to mock_url (or connecting a TcpStream) with short sleeps, until either
a successful response/connection or a timeout (e.g., a few seconds); if the
timeout elapses, capture the child's stderr/stdout, kill/wait the child and fail
the test with the collected logs so startup failures fail fast. Ensure you
reference/mock_server, mock_port, mock_url and try_wait when making these
changes.
- Around line 199-209: The test mutates the process-wide MLM_MAM_BASE_URL env
var and relies on MaM::new() reading it, causing race conditions; change the MaM
API to accept a configurable base URL (e.g., add an overload
MaM::new_with_base(url, user, db) or a builder like
MaM::builder().base_url(url).build()) and update this test to construct MaM with
the mock_url directly instead of setting/removing the environment variable, or
alternatively wrap the test in a global tokio::sync::Mutex guard if changing MaM
is infeasible; modify calls that currently use MaM::new(...) in the test to call
the new constructor/builder so no process-wide env mutation is needed.
In `@tests/e2e/torrent-detail.spec.ts`:
- Around line 46-71: The test "can edit torrent metadata and persist the change"
mutates shared fixture data (vars torrentIndex/torrentId and updatedDescription)
so re-runs can see stale state; fix by making the edited description unique per
run or restoring the original value: before filling Description (in the test
body), read and store the originalDescription from
page.getByLabel('Description'), then either (A) set updatedDescription to
include a unique suffix (e.g., testInfo.title or Date.now()) so it won't collide
across runs, or (B) after assertions, navigate back to the edit page and restore
originalDescription (using the same selectors: page.getByLabel('Description')
and Save button) to clean up; update references to torrentIndex/torrentId and
updatedDescription accordingly in the test.
---
Outside diff comments:
In `@mlm_core/src/autograbber.rs`:
- Around line 846-847: The early fast-path that returns
PreparedTorrentMetaUpdate::Unchanged when meta == torrent.meta is
short-circuiting the linker backfill; modify the logic in the update path used
by select_torrents (when handling Cost::MetadataOnlyAdd) so that linker backfill
runs before returning Unchanged — either move the equality check after the
linker_is_owner/linker population block that reads mam_torrent.owner_name into
torrent.linker, or detect a linker-only change and treat it as a silent update
rather than returning Unchanged; ensure references to meta, torrent.meta,
torrent.linker, linker_is_owner, and PreparedTorrentMetaUpdate::Unchanged are
updated accordingly so torrents get a linker populated even when other metadata
is identical.
In `@mlm_core/src/snatchlist.rs`:
- Around line 313-328: The check if !diff.is_empty() is redundant because
earlier code returns when diff.is_empty(); remove the surrounding if and
unconditionally call write_event(...) with mam_id derived from
mam_torrent.map(|m| m.id) and the Event::new(..., EventType::Updated { fields:
diff, source: (meta.source.clone(), String::new()) }). Ensure you preserve the
await on write_event and keep the mam_id mapping and Event construction exactly
as in the current block.
In `@mlm_web_dioxus/src/list.rs`:
- Around line 350-393: The radio inputs always use a hardcoded checked state so
UI can drift from the actual filter; add a show: Option<String> field to
ListPageContentProps and pass the current filter into the component, then update
each radio's checked to be computed from props.show (e.g., checked true when
props.show.is_none() for "All", checked when props.show.as_deref() ==
Some("any") for "Any Missing", etc.), leaving props.on_filter_change calls as-is
so selection updates both state and visual checked rendering.
In `@mlm_web_dioxus/src/replaced/components.rs`:
- Around line 487-497: Replace the collection-level clone+into_iter with
iterator-by-reference to avoid cloning the whole vector: change uses of
pair.torrent.meta.authors.clone().into_iter().enumerate() to
pair.torrent.meta.authors.iter().enumerate() and, where a owned String is
required (e.g., FilterLink { field: ReplacedPageFilter::Author, value:
author.clone(), ... } and the "{author}" display), clone or to_owned the
individual author item instead of cloning the entire authors collection; apply
the same change to the other loops noted (the ones around the other FilterLink
usages on the listed lines).
In `@mlm_web_dioxus/src/torrent_detail/components.rs`:
- Around line 942-950: The MatchDialog is missing the mam_id prop so MaM
providers never show; when building the dialog (the MatchDialog {...} block
using torrent_id, providers, status_msg, on_close, on_refresh) capture or
retrieve the torrent's mam_id (e.g., store torrent.mam_id in a variable where
torrent_id is captured) and pass it into the component as mam_id: mam_id.clone()
(or mam_id: torrent.mam_id.clone() if available) so the MatchDialog's mam_id:
Option<u64> parameter is populated.
In `@mlm_web_dioxus/src/torrents/components.rs`:
- Around line 559-564: The closure is shadowing outer signals by rebinding names
(sort, asc, filters_signal, show, from, page_size) to mutable locals which is
confusing; either remove the shadowing and use the original signal handles
directly inside the use_effect closure, or rename the local bindings to explicit
names (e.g., sort_signal, asc_signal, from_signal, page_size_signal) so it's
clear these are signal handles used with set() calls in the use_effect closure;
update all references inside the closure (and any set() calls) to the new names
and remove unnecessary mutability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67f0fdd5-09a7-4a58-b38d-8b76d7bbc11b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
.dockerignoreDockerfilemlm_core/src/autograbber.rsmlm_core/src/lib.rsmlm_core/src/linker/mod.rsmlm_core/src/linker/torrent.rsmlm_core/src/metadata/mam_meta.rsmlm_core/src/metadata/mod.rsmlm_core/src/runner.rsmlm_core/src/snatchlist.rsmlm_db/src/impls/categories.rsmlm_db/src/impls/meta.rsmlm_db/src/impls/old_categories.rsmlm_db/tests/meta_diff.rsmlm_mam/src/search.rsmlm_mam/src/user_torrent.rsmlm_meta/Cargo.tomlmlm_meta/src/providers/mam.rsmlm_meta/src/providers/mod.rsmlm_web_api/src/lib.rsmlm_web_api/src/torrent.rsmlm_web_askama/src/lib.rsmlm_web_askama/src/pages/torrent.rsmlm_web_askama/templates/pages/torrent.htmlmlm_web_dioxus/Cargo.tomlmlm_web_dioxus/assets/style.cssmlm_web_dioxus/public/index.htmlmlm_web_dioxus/src/app.rsmlm_web_dioxus/src/components/details.rsmlm_web_dioxus/src/components/query_params.rsmlm_web_dioxus/src/components/search_row.rsmlm_web_dioxus/src/config.rsmlm_web_dioxus/src/dto.rsmlm_web_dioxus/src/duplicate/components.rsmlm_web_dioxus/src/error.rsmlm_web_dioxus/src/events/components.rsmlm_web_dioxus/src/list.rsmlm_web_dioxus/src/lists.rsmlm_web_dioxus/src/main.rsmlm_web_dioxus/src/replaced/components.rsmlm_web_dioxus/src/replaced/server_fns.rsmlm_web_dioxus/src/search.rsmlm_web_dioxus/src/selected/components.rsmlm_web_dioxus/src/selected/server_fns.rsmlm_web_dioxus/src/selected/types.rsmlm_web_dioxus/src/ssr.rsmlm_web_dioxus/src/torrent_detail/components.rsmlm_web_dioxus/src/torrent_detail/server_fns.rsmlm_web_dioxus/src/torrent_detail/types.rsmlm_web_dioxus/src/torrent_edit.rsmlm_web_dioxus/src/torrents/components.rsmlm_web_dioxus/src/torrents/server_fns.rsserver/assets/style.cssserver/src/bin/create_test_db.rsserver/src/bin/mock_server.rsserver/src/main.rsserver/tests/metadata_integration.rstest/e2e-config.tomltests/e2e/pages.spec.tstests/e2e/torrent-detail.spec.ts
💤 Files with no reviewable changes (1)
- mlm_web_dioxus/assets/style.css
Comment on lines
+1
to
+18
| .git | ||
| .github | ||
| .agents | ||
|
|
||
| target | ||
| node_modules | ||
| test-results | ||
|
|
||
| data.db | ||
| *.db | ||
| *.sqlite | ||
| *.sqlite3 | ||
|
|
||
| .DS_Store | ||
|
|
||
| config.toml | ||
| finn.json | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding .env files to the exclusions.
The current exclusions look comprehensive. If the project uses .env files for secrets or local configuration, consider adding them to prevent accidental inclusion in the Docker build context:
config.toml
finn.json
+.env
+.env.*📝 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.
Suggested change
| .git | |
| .github | |
| .agents | |
| target | |
| node_modules | |
| test-results | |
| data.db | |
| *.db | |
| *.sqlite | |
| *.sqlite3 | |
| .DS_Store | |
| config.toml | |
| finn.json | |
| .git | |
| .github | |
| .agents | |
| target | |
| node_modules | |
| test-results | |
| data.db | |
| *.db | |
| *.sqlite | |
| *.sqlite3 | |
| .DS_Store | |
| config.toml | |
| finn.json | |
| .env | |
| .env.* | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.dockerignore around lines 1 - 18, Update the .dockerignore to exclude
environment files by adding patterns like .env and .env.* so local secret/config
files are not sent to the Docker build context; open the .dockerignore (file
shown in the diff) and append entries for .env and .env.* (and any other local
env filenames your repo uses) to ensure they are ignored during docker builds.
Comment on lines
32
to
41
| FROM debian:trixie-slim AS app | ||
| RUN apt update && apt install -y ca-certificates && apt clean | ||
| COPY ./server/assets /server/assets | ||
| COPY --from=build /app/target/release/mlm /mlm | ||
| COPY --from=builder /app/target/release/mlm /mlm | ||
| COPY --from=builder /app/target/dx/mlm_web_dioxus/release/web/public /dioxus-public | ||
| ENV MLM_LOG_DIR="" | ||
| ENV MLM_CONFIG_FILE="/config/config.toml" | ||
| ENV MLM_DB_FILE="/data/data.db" | ||
| ENV DIOXUS_PUBLIC_PATH="/dioxus-public" | ||
| CMD ["/mlm"] |
There was a problem hiding this comment.
Run the runtime image as a non-root user.
The final stage never switches away from root, so any compromise in /mlm gets full container privileges by default. Create an unprivileged user/group, make the writable paths accessible to that account, and add USER before CMD.
🧰 Tools
🪛 Checkov (3.2.508)
[low] 1-41: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[low] 1-41: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.14.0)
[info] 33-33: Delete the apt lists (/var/lib/apt/lists) after installing something
(DL3009)
[warning] 33-33: Do not use apt as it is meant to be an end-user tool, use apt-get or apt-cache instead
(DL3027)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 32 - 41, Create and use an unprivileged user in the
final Docker stage: add a non-root group/user (e.g., mlm:mlm), ensure writable
directories referenced by ENV values or COPY targets (like /data, /config,
/server/assets, /dioxus-public and the /mlm binary location if needed) are
chown'd/chmod'd to that user, then set USER mlm before CMD ["/mlm"] so the
container runs without root privileges; update the Dockerfile's final stage
after the COPY lines to perform these steps.
Comment on lines
+839
to
843
| torrent.meta.canonicalize(); | ||
| meta.canonicalize(); | ||
| meta.ids.extend(torrent.meta.ids.clone()); | ||
| meta.tags = torrent.meta.tags.clone(); | ||
| if meta.description.is_empty() { |
There was a problem hiding this comment.
Don't clobber caller-supplied ids and tags here.
The new torrent-edit flow sends user-edited IDs and tags through update_torrent_meta, but this path still restores torrent.meta.ids and torrent.meta.tags before diffing. That makes the new ABS/ASIN/Goodreads/MaM/tag fields a no-op. Preserve the incoming values for manual updates instead of unconditionally copying the stored ones back.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_core/src/autograbber.rs` around lines 839 - 843, Don't unconditionally
copy stored IDs/tags back into the incoming meta; instead preserve
caller-supplied values from the new torrent-edit flow (update_torrent_meta). In
the block around torrent.meta.canonicalize(), meta.canonicalize(), and the lines
touching meta.ids/meta.tags, change the logic so that torrent.meta.ids is only
merged when meta.ids.is_empty() (or use extend only when no caller IDs exist)
and torrent.meta.tags is only assigned when meta.tags.is_empty(); leave meta.ids
and meta.tags untouched when the incoming meta already contains values. This
preserves user-edited ABS/ASIN/Goodreads/MaM/tag fields while keeping
canonicalization behavior.
mlm_core/src/metadata/mam_meta.rs
Outdated
Comment on lines
29
to
35
| let fetched = ctx | ||
| .ssr() | ||
| .metadata | ||
| .lock() | ||
| .await | ||
| .fetch_provider(ctx, query, provider_id) | ||
| .await?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Awaited fetch_provider calls while holding the metadata lock:"
rg -nUP --glob '*.rs' -C2 '(?s)\bmetadata\s*\.\s*lock\(\)\s*\.\s*await.*?fetch_provider\([^;]*?\)\s*\.await' \
mlm_core mlm_web_askama mlm_web_dioxus server || true
echo
echo "Definitions of async fetch_provider:"
rg -nUP --glob '*.rs' -C3 '(?s)\basync\s+fn\s+fetch_provider\b' \
mlm_core/src mlm_meta/src server/src || trueRepository: StirlingMouse/MLM
Length of output: 11848
Drop the metadata lock before awaiting provider fetches.
lock().await.fetch_provider(...).await keeps the tokio::MutexGuard alive across the provider fetch. This serializes all metadata access—including cheap reads like enabled_providers() during page rendering—behind the slowest provider call.
Example at mlm_core/src/metadata/mam_meta.rs:29-35
let fetched = ctx
.ssr()
.metadata
.lock()
.await
.fetch_provider(ctx, query, provider_id)
.await?;Refactor to release the lock before awaiting: acquire only the data needed under lock, then call the async method outside the critical section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_core/src/metadata/mam_meta.rs` around lines 29 - 35, The metadata lock is
held across an await
(ctx.ssr().metadata.lock().await.fetch_provider(...).await), which serializes
all metadata access; instead, acquire the MutexGuard from
ctx.ssr().metadata.lock().await only long enough to read/clone the minimal data
needed (e.g., any provider config or handles), then drop the guard before
calling fetch_provider so the long async call runs outside the critical section;
specifically, read/clone whatever fetch_provider needs while holding the lock
and then call fetch_provider (or an equivalent async helper) after releasing the
guard so cheap reads like enabled_providers() are not blocked.
mlm_db/src/impls/categories.rs
Outdated
Comment on lines
+6
to
+17
| pub fn all() -> Vec<MediaType> { | ||
| vec![ | ||
| MediaType::Audiobook, | ||
| MediaType::Ebook, | ||
| MediaType::Musicology, | ||
| MediaType::Radio, | ||
| MediaType::Manga, | ||
| MediaType::ComicBook, | ||
| MediaType::PeriodicalEbook, | ||
| MediaType::PeriodicalAudiobook, | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider returning a static slice instead of allocating a Vec.
MediaType::all() allocates a new Vec on each call. If callers typically just iterate, consider returning &'static [MediaType] to avoid allocation.
♻️ Suggested alternative
- pub fn all() -> Vec<MediaType> {
- vec![
+ pub fn all() -> &'static [MediaType] {
+ &[
MediaType::Audiobook,
MediaType::Ebook,
MediaType::Musicology,
MediaType::Radio,
MediaType::Manga,
MediaType::ComicBook,
MediaType::PeriodicalEbook,
MediaType::PeriodicalAudiobook,
]
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_db/src/impls/categories.rs` around lines 6 - 17, MediaType::all()
currently allocates a new Vec on every call; change its signature from pub fn
all() -> Vec<MediaType> to pub fn all() -> &'static [MediaType] and return a
static slice (e.g. define a static/const array or slice like ALL: [MediaType; 9]
or static ALL: &'static [MediaType] = &[..] and have all() return &ALL) so
callers can iterate without heap allocation; update any callers that expect a
Vec to work with a slice (or call .to_vec() where a Vec is actually required).
Comment on lines
248
to
250
| unsafe { | ||
| std::env::set_var("DIOXUS_PUBLIC_PATH", &dioxus_public_path); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the safety rationale for the unsafe block.
std::env::set_var is unsafe in Rust 2024 edition due to potential thread safety issues when called concurrently with std::env::var. Since this runs early in app_main before spawning tasks, it's safe in practice. A brief safety comment would clarify this for future readers.
📝 Suggested documentation
+ // SAFETY: This is called early in main before spawning any tasks that might
+ // concurrently read environment variables.
unsafe {
std::env::set_var("DIOXUS_PUBLIC_PATH", &dioxus_public_path);
}📝 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.
Suggested change
| unsafe { | |
| std::env::set_var("DIOXUS_PUBLIC_PATH", &dioxus_public_path); | |
| } | |
| // SAFETY: This is called early in main before spawning any tasks that might | |
| // concurrently read environment variables. | |
| unsafe { | |
| std::env::set_var("DIOXUS_PUBLIC_PATH", &dioxus_public_path); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/main.rs` around lines 248 - 250, Add a short safety comment
explaining why the unsafe block around std::env::set_var(&dioxus_public_path) is
sound: note that std::env::set_var is marked unsafe in Rust 2024 due to
potential concurrent access with std::env::var, but here it is called early in
app_main before any threads/tasks are spawned (so there is no concurrent
access), and therefore the operation is safe; place the comment immediately
above the unsafe block that sets DIOXUS_PUBLIC_PATH.
Comment on lines
+165
to
+197
| let mock_port = 14997u16; | ||
| let mock_url = format!("http://127.0.0.1:{}", mock_port); | ||
|
|
||
| // Spawn mock server | ||
| let mock_bin = std::env::current_exe() | ||
| .unwrap() | ||
| .parent() | ||
| .unwrap() | ||
| .parent() | ||
| .unwrap() | ||
| .join("mock_server"); | ||
| let mock_bin = if mock_bin.exists() { | ||
| mock_bin | ||
| } else { | ||
| // Fallback: look in target/debug | ||
| std::path::PathBuf::from("target/debug/mock_server") | ||
| }; | ||
|
|
||
| let mut mock_server = std::process::Command::new(&mock_bin) | ||
| .env("MOCK_PORT", mock_port.to_string()) | ||
| .env("MLM_MAM_BASE_URL", &mock_url) | ||
| .stdout(std::process::Stdio::piped()) | ||
| .stderr(std::process::Stdio::piped()) | ||
| .spawn()?; | ||
|
|
||
| // Wait for mock server to start (synchronous sleep in test context) | ||
| std::thread::sleep(std::time::Duration::from_millis(500)); | ||
|
|
||
| // Check if mock server started successfully | ||
| let mock_running = mock_server.try_wait().map(|o| o.is_none()).unwrap_or(false); | ||
| if !mock_running { | ||
| eprintln!("Warning: mock_server may not have started, continuing anyway..."); | ||
| } |
There was a problem hiding this comment.
Make the mock-server startup deterministic.
Hard-coding port 14997, sleeping for 500 ms, and then continuing even if the child already exited will make this test flaky on CI. If the port is busy or startup is slower, the failure only surfaces later as a MaM error. Pick an unused port and poll the child or an HTTP endpoint until it's ready, failing fast with stderr if startup never succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/tests/metadata_integration.rs` around lines 165 - 197, Replace the
fixed-port + fixed-sleep startup with a deterministic acquire-and-wait flow:
allocate an unused port by binding a TcpListener to "127.0.0.1:0" and reading
the assigned port (use that to set mock_port/mock_url), spawn mock_server with
that port, then poll for readiness by repeatedly checking the child with
mock_server.try_wait() and performing an HTTP GET to mock_url (or connecting a
TcpStream) with short sleeps, until either a successful response/connection or a
timeout (e.g., a few seconds); if the timeout elapses, capture the child's
stderr/stdout, kill/wait the child and fail the test with the collected logs so
startup failures fail fast. Ensure you reference/mock_server, mock_port,
mock_url and try_wait when making these changes.
Comment on lines
+199
to
+209
| // Set env var for MaM to use mock server | ||
| unsafe { std::env::set_var("MLM_MAM_BASE_URL", &mock_url) }; | ||
|
|
||
| // Create MaM instance pointing to mock server | ||
| let mam_result = mlm_mam::api::MaM::new("test-user", test_db.db.clone()).await; | ||
| let mam: Arc<mlm_mam::api::MaM<'static>> = match mam_result { | ||
| Ok(m) => Arc::new(m), | ||
| Err(e) => { | ||
| let _ = mock_server.kill(); | ||
| unsafe { std::env::remove_var("MLM_MAM_BASE_URL") }; | ||
| return Err(anyhow::anyhow!("Failed to create MaM instance: {}", e)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file and the specific lines mentioned
head -220 server/tests/metadata_integration.rs | tail -30Repository: StirlingMouse/MLM
Length of output: 1291
🏁 Script executed:
# Also check the second location around line 275-277
sed -n '270,285p' server/tests/metadata_integration.rsRepository: StirlingMouse/MLM
Length of output: 457
🏁 Script executed:
# Search for all instances of set_var and remove_var in this test file
rg "set_var|remove_var" server/tests/metadata_integration.rs -nRepository: StirlingMouse/MLM
Length of output: 259
🏁 Script executed:
# Check if there's any synchronization mechanism (like Mutex) used for test isolation
rg "Mutex|SYNC|lazy_static|once_cell" server/tests/metadata_integration.rs -n -A 2 -B 2Repository: StirlingMouse/MLM
Length of output: 1256
🏁 Script executed:
# Check the test function structure and any test attributes
rg "#\[.*test.*\]|async fn|fn test_" server/tests/metadata_integration.rs -n -B 1Repository: StirlingMouse/MLM
Length of output: 650
🏁 Script executed:
# Check if there are other tests that might access the MLM_MAM_BASE_URL env var
rg "MLM_MAM_BASE_URL|set_var.*BASE_URL|remove_var.*BASE_URL" . -r --type rust -nRepository: StirlingMouse/MLM
Length of output: 747
🏁 Script executed:
# Check if there's any test configuration (Cargo.toml) that controls parallel execution
cat Cargo.toml | grep -A 10 "\[profile\." | head -20Repository: StirlingMouse/MLM
Length of output: 520
🏁 Script executed:
# Look at the MaM API to see if there's a way to inject the URL directly
fd -e rs "mam" --type f | head -10Repository: StirlingMouse/MLM
Length of output: 124
🏁 Script executed:
# Check the MaM::new signature to understand how it uses the env var
rg "impl.*MaM|fn new" mlm_mam/ -A 5 -B 2 --type rust | head -50Repository: StirlingMouse/MLM
Length of output: 981
🏁 Script executed:
# Check if there's a way to configure the base URL without env vars in the MaM API
rg "BASE_URL|base_url|set_url" mlm_mam/ --type rust -nRepository: StirlingMouse/MLM
Length of output: 1017
🏁 Script executed:
# Let's look at the full test function to understand the scope and any synchronization
sed -n '154,280p' server/tests/metadata_integration.rsRepository: StirlingMouse/MLM
Length of output: 4154
🏁 Script executed:
# Check the MaM::new signature in detail to confirm it doesn't accept URL parameter
sed -n '75,100p' mlm_mam/src/api.rsRepository: StirlingMouse/MLM
Length of output: 1035
🏁 Script executed:
# Search for any test utilities or setup/teardown that might handle synchronization
rg "mod tests|setup|teardown|fixture" server/tests/metadata_integration.rs -n -A 5Repository: StirlingMouse/MLM
Length of output: 43
🏁 Script executed:
# Check if there are other concurrent tests that might conflict
rg "#\[tokio::test\]" server/tests/metadata_integration.rs -nRepository: StirlingMouse/MLM
Length of output: 116
🏁 Script executed:
# Check if the MLM_MAM_BASE_URL env var is used anywhere else in production code
rg "MLM_MAM_BASE_URL" . --type rust -n | grep -v "server/tests"Repository: StirlingMouse/MLM
Length of output: 309
Refactor MaM API to accept configurable base URL instead of relying on process-wide environment mutation.
The MaM::new() function forces reliance on the MLM_MAM_BASE_URL environment variable with no way to inject the URL directly. This test mutates process-wide state without synchronization guards, creating race conditions in parallel test execution. Either extend MaM::new() to accept a URL parameter (or add a builder/configuration method), or serialize this test behind a tokio::sync::Mutex-based guard to prevent concurrent access.
Affects lines 200, 208, and 277.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/tests/metadata_integration.rs` around lines 199 - 209, The test
mutates the process-wide MLM_MAM_BASE_URL env var and relies on MaM::new()
reading it, causing race conditions; change the MaM API to accept a configurable
base URL (e.g., add an overload MaM::new_with_base(url, user, db) or a builder
like MaM::builder().base_url(url).build()) and update this test to construct MaM
with the mock_url directly instead of setting/removing the environment variable,
or alternatively wrap the test in a global tokio::sync::Mutex guard if changing
MaM is infeasible; modify calls that currently use MaM::new(...) in the test to
call the new constructor/builder so no process-wide env mutation is needed.
Comment on lines
+255
to
+273
| // Test 3: Fetch by MaM ID should work | ||
| let q_with_id = MetadataQuery { | ||
| ids: std::collections::BTreeMap::from([( | ||
| mlm_db::ids::MAM.to_string(), | ||
| "12345".to_string(), | ||
| )]), | ||
| title: "Test".to_string(), | ||
| ..Default::default() | ||
| }; | ||
| let result = metadata | ||
| .lock() | ||
| .await | ||
| .fetch_provider(&ctx, q_with_id, "mam") | ||
| .await; | ||
| assert!( | ||
| result.is_ok(), | ||
| "MaM fetch by ID should succeed: {:?}", | ||
| result.err() | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assert the ID-specific response in the direct-lookup test.
This still passes if get_torrent_info_by_id breaks, because fetch_provider can fall back to a title search on "Test". Assert that the returned metadata carries MaM ID 12345 or the mock's updated title so the test actually proves the direct-ID path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/tests/metadata_integration.rs` around lines 255 - 273, The test
currently only checks that fetch_provider returned Ok, which can hide fallbacks;
after calling metadata.lock().await.fetch_provider(&ctx, q_with_id, "mam").await
and unwrapping the result, assert the returned metadata actually came from the
direct-ID path by checking the MaM ID and/or title in the response (e.g. that
the metadata contains mlm_db::ids::MAM => "12345" or the mock's updated title).
Update the assertion to inspect the returned Metadata/ProviderRecord (from
fetch_provider's result) and assert the id map contains "12345" for the MAM key
(or assert the title equals the mock-updated title) so the test fails if
get_torrent_info_by_id is not used.
Comment on lines
+46
to
+71
| test('can edit torrent metadata and persist the change', async ({ page }, testInfo) => { | ||
| const torrentIndex = 11 + browserOffset(testInfo.project.name); | ||
| const torrentId = torrentIdFor(torrentIndex); | ||
| const updatedDescription = | ||
| `Description updated by Playwright for ${testInfo.project.name}.`; | ||
|
|
||
| await page.goto(`/torrent-edit/${torrentId}`); | ||
| await expect( | ||
| page.getByRole('heading', { name: 'Edit Torrent Metadata' }) | ||
| ).toBeVisible(); | ||
|
|
||
| const description = page.getByLabel('Description'); | ||
| await description.fill(updatedDescription); | ||
|
|
||
| await page.getByRole('button', { name: 'Save' }).click(); | ||
| await expect(page.locator('body')).toContainText('Metadata updated'); | ||
|
|
||
| await page.reload(); | ||
| await expect( | ||
| page.getByRole('heading', { name: 'Edit Torrent Metadata' }) | ||
| ).toBeVisible(); | ||
| await expect(page.getByLabel('Description')).toHaveValue(updatedDescription); | ||
|
|
||
| await page.goto(`/torrents/${torrentId}`); | ||
| await expect(page.locator('.torrent-description')).toContainText(updatedDescription); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding cleanup or using unique test data.
The test modifies torrent-011/012/013 (depending on browser) with a specific description. If tests are re-run without resetting the test database, the assertion on line 67 will still pass but the "before" state may already contain the updated value.
This is acceptable if the test database is reset between runs, but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/torrent-detail.spec.ts` around lines 46 - 71, The test "can edit
torrent metadata and persist the change" mutates shared fixture data (vars
torrentIndex/torrentId and updatedDescription) so re-runs can see stale state;
fix by making the edited description unique per run or restoring the original
value: before filling Description (in the test body), read and store the
originalDescription from page.getByLabel('Description'), then either (A) set
updatedDescription to include a unique suffix (e.g., testInfo.title or
Date.now()) so it won't collide across runs, or (B) after assertions, navigate
back to the edit page and restore originalDescription (using the same selectors:
page.getByLabel('Description') and Save button) to clean up; update references
to torrentIndex/torrentId and updatedDescription accordingly in the test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Style
Tests