Move Dioxus routes and extract web API#13
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:
📝 WalkthroughWalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Axum as Axum Router
participant DB as Database
participant QBit as qBittorrent API
participant FS as File System
Client->>Axum: GET /torrents/{id}/{filename}
Axum->>DB: Fetch Torrent by ID
alt Torrent found
DB-->>Axum: Torrent metadata
alt Has library_path + library_files
Axum->>FS: Resolve file path
else Query fallback
Axum->>QBit: Fetch qBittorrent data + files
QBit-->>Axum: Torrent + file list
Axum->>Axum: Apply path mapping
end
alt File resolved
Axum->>FS: Open file async
FS-->>Axum: File handle
Axum->>Axum: Wrap in ReaderStream
Axum-->>Client: 200 + Content-Type: text/toml + file stream
else File not found
Axum-->>Client: 404 NotFound
end
else Torrent not found
Axum-->>Client: 404 NotFound
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
📝 Coding Plan
Comment |
|
@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: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 5: Add a matching dev-profile entry for the workspace crate "mlm_web_api"
by adding a [profile.dev.package.mlm_web_api] table with opt-level = 0 to your
Cargo.toml (the same workspace manifest where "mlm_web_api" is listed) so the
dev profile is consistent with the other workspace crates; ensure the table key
exactly matches mlm_web_api.
In `@mlm_web_api/Cargo.toml`:
- Around line 13-14: The Cargo manifest uses floating/branch git dependencies
(native_db and qbit) which can drift; update each dependency declaration (e.g.,
native_db and qbit) to pin an immutable commit by replacing the branch/omitted
revision with a rev = "<commit-sha>" field (use the specific commit SHA you want
to lock to), and remove the branch field if present; apply the same change
pattern to other manifests that list these git dependencies (e.g.,
server/Cargo.toml, mlm_core/Cargo.toml) so all git dependencies use rev pins for
reproducible builds and supply-chain safety.
In `@mlm_web_api/src/download.rs`:
- Around line 52-61: The headers block incorrectly hardcodes "text/toml;
charset=utf-8" and uses an unsanitized filename; update the code that builds the
headers (the `headers` array where `CONTENT_TYPE` and `CONTENT_DISPOSITION` are
set) to: 1) derive the MIME type from the file name/path using a library such as
mime_guess (e.g.
mime_guess::from_path(filename).first_or_octet_stream().to_string()) and use
that value for `CONTENT_TYPE` (include charset only for text types if desired),
and 2) sanitize/escape the `filename` used in `CONTENT_DISPOSITION` to prevent
header injection (strip or replace newlines and quotes and/or use the RFC5987
encoding form `filename*=` with percent-encoding for UTF-8); ensure you fall
back to "application/octet-stream" if no MIME is found.
- Around line 45-48: When opening the file with tokio::fs::File::open(...) in
this function, the Err arm currently discards the error and returns
AppError::NotFound; change the match to capture the error (e.g., Err(e)) and log
the error details before returning, so you still return AppError::NotFound but
record the underlying I/O error (use the project’s logger/tracing facility) to
aid debugging while preserving the existing error mapping.
In `@mlm_web_api/src/error.rs`:
- Around line 24-31: The into_response implementation on AppError currently
returns self.to_string() which can leak internal details; change into_response
(impl IntoResponse for AppError) to map AppError::NotFound to
StatusCode::NOT_FOUND with a safe user-facing message, but for all other
variants (StatusCode::INTERNAL_SERVER_ERROR) return a generic error message like
"Internal server error" to clients while logging the full error
string/server-side (use your existing logging facility) so internal details are
not sent in the HTTP response.
In `@mlm_web_api/src/lib.rs`:
- Line 28: MaMState is double-wrapping the MaM in Arc
(Arc<Result<Arc<MaM<'static>>>>); if context.mam() already returns
Result<Arc<MaM<'static>>> remove the outer Arc and redefine MaMState as
Result<Arc<MaM<'static>>> so callers get the same type as context.mam();
alternatively, if you intended a shared, thread-safe container for the Result
itself, explicitly document that and use Arc<Result<MaM<'static>>> — update the
MaMState type alias and any code that constructs or clones it to match the
chosen single-Arc shape (refer to MaMState, MaM, Arc, Result and context.mam()).
- Around line 33-44: The GET and POST routes for "/api/search" use different
state types causing inconsistent handler extraction: GET uses
Arc::new(context.mam()) while POST uses context.clone(), forcing different State
types for search_api vs search_api_post; make them consistent by choosing one
shared state type (preferably context.clone() so both routes call
.with_state(context.clone())) and update the handler signature of search_api to
accept State<Context> (or alternatively convert POST to use
Arc::new(context.mam()) and change search_api_post to accept the same MaMState);
adjust any uses of MaMState/Context inside search_api/search_api_post
accordingly.
In `@mlm_web_api/src/search.rs`:
- Line 75: The code currently constructs a hardcoded PathBuf via
PathBuf::from("/data/torrents"); replace this with a configurable value (for
example add a field like torrents_path or derive it from an existing
Config.data_dir) and use that instead (e.g., Config::torrents_path or
config.data_dir.join("torrents")). Update the code that constructs the path in
search.rs to read the path from the Config instance passed to the function (or
fall back to an environment variable/default relative path), and replace
references to PathBuf::from("/data/torrents") with
PathBuf::from(config.torrents_path) or config.data_dir.join("torrents") so
deployments can override the location.
- Around line 71-78: The path construction has an off-by-one: third is extracted
with id_str.get(3..4) which skips index 2; change the slice to id_str.get(2..3)
so third corresponds to the third character of torrent.id (keep using id_str,
first, second, third and PathBuf::from(...).join(...) as-is); ensure
unwrap_or_default() remains to handle short IDs.
- Around line 81-82: The code uses blocking std::fs::File::create and
serde_json::to_writer which will block the async runtime; replace the blocking
call with tokio::fs::File::create(...).await and write the JSON asynchronously
by serializing first (e.g. serde_json::to_vec or to_string on torrent) and then
calling tokio::io::AsyncWriteExt::write_all on the tokio file (remember to
.await and map errors into anyhow::Error as before). Update the code around
File::create, serde_json::to_writer, file_path and torrent to use the async
tokio APIs and ensure the surrounding function is async.
In `@mlm_web_api/src/torrent.rs`:
- Around line 14-23: The current router in torrent_api uses a parse-to-u64
heuristic to decide between MAM and torrent-id handling; update it to use the
explicit id_is_hash flag (or at minimum document the heuristic). Replace the
parse-based dispatch inside torrent_api so it looks up the record via Context
(using the Torrent model/DB access) and checks the Torrent.id_is_hash boolean to
decide whether to call torrent_api_mam_id or torrent_api_id (using the same
State(context) and Path value), and if you keep the heuristic add a clarifying
comment above torrent_api noting “numeric-only → MAM id” and reference the
Torrent.id_is_hash field for future clarity; ensure any DB lookup handles “not
found” and maps errors to AppError consistently.
In `@mlm_web_askama/templates/pages/errors.html`:
- Around line 40-41: Both anchor tags that use target=_blank (the link to
"/old/torrents/{{ mam_id }}" and the link to "https://www.myanonamouse.net/t/{{
mam_id }}") must include rel="noopener noreferrer" to prevent exposing
window.opener; update those <a> elements in the errors.html template to add
rel="noopener noreferrer" alongside target=_blank.
In `@server/src/main.rs`:
- Line 38: Remove the unused import statement "use
tower_http::services::ServeDir;" from main.rs: locate the top-level use/import
block where ServeDir is declared and delete that line (ServeDir is actually used
in mlm_web_api::lib, not here), then run cargo check to verify no remaining
unused imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 422d35b3-49cd-4b1c-a822-ac61de567235
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
Cargo.tomlmlm_web_api/Cargo.tomlmlm_web_api/src/download.rsmlm_web_api/src/error.rsmlm_web_api/src/lib.rsmlm_web_api/src/search.rsmlm_web_api/src/torrent.rsmlm_web_askama/src/lib.rsmlm_web_askama/src/pages/torrent.rsmlm_web_askama/src/pages/torrent_edit.rsmlm_web_askama/templates/base.htmlmlm_web_askama/templates/pages/duplicate.htmlmlm_web_askama/templates/pages/errors.htmlmlm_web_askama/templates/pages/list.htmlmlm_web_askama/templates/pages/lists.htmlmlm_web_askama/templates/pages/replaced.htmlmlm_web_askama/templates/pages/torrent.htmlmlm_web_askama/templates/pages/torrents.htmlmlm_web_askama/templates/partials/mam_torrents.htmlmlm_web_dioxus/src/app.rsmlm_web_dioxus/src/components/filter_link.rsmlm_web_dioxus/src/components/search_row.rsmlm_web_dioxus/src/errors/components.rsmlm_web_dioxus/src/events/components.rsmlm_web_dioxus/src/list.rsmlm_web_dioxus/src/sse.rsmlm_web_dioxus/src/ssr.rsmlm_web_dioxus/src/torrent_detail/components.rsmlm_web_dioxus/src/torrent_edit.rsserver/Cargo.tomlserver/src/main.rs
| resolver = "3" | ||
| members = [ | ||
| "server", | ||
| "mlm_web_api", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a matching dev-profile entry for mlm_web_api.
Line 5 correctly adds the crate to the workspace. For consistency with other workspace crates, you may also add [profile.dev.package.mlm_web_api] opt-level = 0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` at line 5, Add a matching dev-profile entry for the workspace
crate "mlm_web_api" by adding a [profile.dev.package.mlm_web_api] table with
opt-level = 0 to your Cargo.toml (the same workspace manifest where
"mlm_web_api" is listed) so the dev profile is consistent with the other
workspace crates; ensure the table key exactly matches mlm_web_api.
| native_db = { git = "https://github.com/StirlingMouse/native_db.git", branch = "0.8.x" } | ||
| qbit = { git = "https://github.com/StirlingMouse/qbittorrent-webui-api.git" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Branch/floating git dependencies in Cargo.toml files =="
rg -n 'git\s*=\s*"[^"]+"|branch\s*=' --glob '**/Cargo.toml' -C1
echo
if [ -f Cargo.lock ]; then
echo "== Git sources in Cargo.lock (native_db/qbit) =="
rg -n 'name = "(native_db|qbit)"|source = "git\+' Cargo.lock -C2
else
echo "Cargo.lock not found."
fiRepository: StirlingMouse/MLM
Length of output: 4185
🏁 Script executed:
git ls-files | grep -E "^Cargo\.lock$"Repository: StirlingMouse/MLM
Length of output: 71
Pin git dependencies to immutable commits instead of floating branches.
Lines 13–14 use branch-based and floating git dependencies, which allow upstream movement. Even with Cargo.lock pinning current builds, manifests should explicitly specify rev to prevent unintended drift during lockfile updates and strengthen supply-chain transparency.
Suggested manifest change
-native_db = { git = "https://github.com/StirlingMouse/native_db.git", branch = "0.8.x" }
-qbit = { git = "https://github.com/StirlingMouse/qbittorrent-webui-api.git" }
+native_db = { git = "https://github.com/StirlingMouse/native_db.git", rev = "cddaafe0f6863146fd0879cae88fdb613bc50b47" }
+qbit = { git = "https://github.com/StirlingMouse/qbittorrent-webui-api.git", rev = "1038c6000e749b6b0d55fe2108618e82beeaebfe" }Note: This pattern also appears in server/Cargo.toml, mlm_core/Cargo.toml, and other manifests.
📝 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.
| native_db = { git = "https://github.com/StirlingMouse/native_db.git", branch = "0.8.x" } | |
| qbit = { git = "https://github.com/StirlingMouse/qbittorrent-webui-api.git" } | |
| native_db = { git = "https://github.com/StirlingMouse/native_db.git", rev = "cddaafe0f6863146fd0879cae88fdb613bc50b47" } | |
| qbit = { git = "https://github.com/StirlingMouse/qbittorrent-webui-api.git", rev = "1038c6000e749b6b0d55fe2108618e82beeaebfe" } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_web_api/Cargo.toml` around lines 13 - 14, The Cargo manifest uses
floating/branch git dependencies (native_db and qbit) which can drift; update
each dependency declaration (e.g., native_db and qbit) to pin an immutable
commit by replacing the branch/omitted revision with a rev = "<commit-sha>"
field (use the specific commit SHA you want to lock to), and remove the branch
field if present; apply the same change pattern to other manifests that list
these git dependencies (e.g., server/Cargo.toml, mlm_core/Cargo.toml) so all git
dependencies use rev pins for reproducible builds and supply-chain safety.
mlm_web_api/src/download.rs
Outdated
| let file = match tokio::fs::File::open(path).await { | ||
| Ok(file) => file, | ||
| Err(_) => return Err(AppError::NotFound), | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging the file open error for debugging.
The actual error is discarded when the file cannot be opened. This loses diagnostic information (e.g., permission denied vs. file not found) that could be useful for troubleshooting.
♻️ Proposed fix
let file = match tokio::fs::File::open(path).await {
Ok(file) => file,
- Err(_) => return Err(AppError::NotFound),
+ Err(e) => {
+ tracing::warn!("Failed to open file: {e}");
+ return Err(AppError::NotFound);
+ }
};📝 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 file = match tokio::fs::File::open(path).await { | |
| Ok(file) => file, | |
| Err(_) => return Err(AppError::NotFound), | |
| }; | |
| let file = match tokio::fs::File::open(path).await { | |
| Ok(file) => file, | |
| Err(e) => { | |
| tracing::warn!("Failed to open file: {e}"); | |
| return Err(AppError::NotFound); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_web_api/src/download.rs` around lines 45 - 48, When opening the file with
tokio::fs::File::open(...) in this function, the Err arm currently discards the
error and returns AppError::NotFound; change the match to capture the error
(e.g., Err(e)) and log the error details before returning, so you still return
AppError::NotFound but record the underlying I/O error (use the project’s
logger/tracing facility) to aid debugging while preserving the existing error
mapping.
| let headers = [ | ||
| ( | ||
| axum::http::header::CONTENT_TYPE, | ||
| "text/toml; charset=utf-8".to_string(), | ||
| ), | ||
| ( | ||
| axum::http::header::CONTENT_DISPOSITION, | ||
| format!("attachment; filename=\"{}\"", filename), | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
Incorrect Content-Type for file downloads.
The Content-Type is hardcoded to text/toml; charset=utf-8, but this endpoint serves arbitrary torrent-related files (audio files, etc.), not TOML configuration files. This will cause browsers to misinterpret the file type.
Additionally, the filename in Content-Disposition should be sanitized to prevent header injection with characters like " or newlines.
🐛 Proposed fix
let headers = [
(
axum::http::header::CONTENT_TYPE,
- "text/toml; charset=utf-8".to_string(),
+ "application/octet-stream".to_string(),
),
(
axum::http::header::CONTENT_DISPOSITION,
- format!("attachment; filename=\"{}\"", filename),
+ format!(
+ "attachment; filename=\"{}\"",
+ filename.replace('"', "\\\"").replace('\n', "").replace('\r', "")
+ ),
),
];📝 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 headers = [ | |
| ( | |
| axum::http::header::CONTENT_TYPE, | |
| "text/toml; charset=utf-8".to_string(), | |
| ), | |
| ( | |
| axum::http::header::CONTENT_DISPOSITION, | |
| format!("attachment; filename=\"{}\"", filename), | |
| ), | |
| ]; | |
| let headers = [ | |
| ( | |
| axum::http::header::CONTENT_TYPE, | |
| "application/octet-stream".to_string(), | |
| ), | |
| ( | |
| axum::http::header::CONTENT_DISPOSITION, | |
| format!( | |
| "attachment; filename=\"{}\"", | |
| filename.replace('"', "\\\"").replace('\n', "").replace('\r', "") | |
| ), | |
| ), | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_web_api/src/download.rs` around lines 52 - 61, The headers block
incorrectly hardcodes "text/toml; charset=utf-8" and uses an unsanitized
filename; update the code that builds the headers (the `headers` array where
`CONTENT_TYPE` and `CONTENT_DISPOSITION` are set) to: 1) derive the MIME type
from the file name/path using a library such as mime_guess (e.g.
mime_guess::from_path(filename).first_or_octet_stream().to_string()) and use
that value for `CONTENT_TYPE` (include charset only for text types if desired),
and 2) sanitize/escape the `filename` used in `CONTENT_DISPOSITION` to prevent
header injection (strip or replace newlines and quotes and/or use the RFC5987
encoding form `filename*=` with percent-encoding for UTF-8); ensure you fall
back to "application/octet-stream" if no MIME is found.
| impl IntoResponse for AppError { | ||
| fn into_response(self) -> Response { | ||
| let status = match self { | ||
| AppError::NotFound => StatusCode::NOT_FOUND, | ||
| _ => StatusCode::INTERNAL_SERVER_ERROR, | ||
| }; | ||
| (status, self.to_string()).into_response() | ||
| } |
There was a problem hiding this comment.
Consider sanitizing error messages in responses to avoid information leakage.
The current implementation returns self.to_string() as the response body for all errors, which may expose internal details like database error messages, file paths, or stack traces to API clients. This could be a security concern in production.
Consider returning generic messages for 500 errors while logging the actual error server-side.
🛡️ Proposed fix
impl IntoResponse for AppError {
fn into_response(self) -> Response {
let status = match self {
AppError::NotFound => StatusCode::NOT_FOUND,
_ => StatusCode::INTERNAL_SERVER_ERROR,
};
- (status, self.to_string()).into_response()
+ let message = match &self {
+ AppError::NotFound => "Not Found".to_string(),
+ _ => {
+ tracing::error!("API error: {self:?}");
+ "Internal Server Error".to_string()
+ }
+ };
+ (status, message).into_response()
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_web_api/src/error.rs` around lines 24 - 31, The into_response
implementation on AppError currently returns self.to_string() which can leak
internal details; change into_response (impl IntoResponse for AppError) to map
AppError::NotFound to StatusCode::NOT_FOUND with a safe user-facing message, but
for all other variants (StatusCode::INTERNAL_SERVER_ERROR) return a generic
error message like "Internal server error" to clients while logging the full
error string/server-side (use your existing logging facility) so internal
details are not sent in the HTTP response.
| let first = id_str.get(0..1).unwrap_or_default(); | ||
| let second = id_str.get(1..2).unwrap_or_default(); | ||
| let third = id_str.get(3..4).unwrap_or_default(); | ||
| let path = PathBuf::from("/data/torrents") |
There was a problem hiding this comment.
Hardcoded absolute path /data/torrents is not configurable.
The path /data/torrents is hardcoded, which assumes a specific deployment environment and may not work across different setups (development, Docker, bare metal, etc.). Consider making this configurable via the Config struct or using a path relative to the data directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_web_api/src/search.rs` at line 75, The code currently constructs a
hardcoded PathBuf via PathBuf::from("/data/torrents"); replace this with a
configurable value (for example add a field like torrents_path or derive it from
an existing Config.data_dir) and use that instead (e.g., Config::torrents_path
or config.data_dir.join("torrents")). Update the code that constructs the path
in search.rs to read the path from the Config instance passed to the function
(or fall back to an environment variable/default relative path), and replace
references to PathBuf::from("/data/torrents") with
PathBuf::from(config.torrents_path) or config.data_dir.join("torrents") so
deployments can override the location.
| let file = File::create(file_path).map_err(anyhow::Error::new)?; | ||
| serde_json::to_writer(file, torrent)?; |
There was a problem hiding this comment.
Blocking file I/O in async context.
std::fs::File::create is a blocking operation that will block the async runtime's thread. This should use tokio::fs::File::create for non-blocking I/O, especially since tokio::fs::create_dir_all is already used on line 79.
🐛 Proposed fix
use std::{fs::File, path::PathBuf};
+use std::io::Write;
-use anyhow::Result;
+use anyhow::Result;And update the file writing:
create_dir_all(&path).await.map_err(anyhow::Error::new)?;
let file_path = path.join(format!("{}.json", torrent.id));
- let file = File::create(file_path).map_err(anyhow::Error::new)?;
- serde_json::to_writer(file, torrent)?;
+ let json = serde_json::to_vec(torrent)?;
+ tokio::fs::write(file_path, json).await.map_err(anyhow::Error::new)?;📝 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 file = File::create(file_path).map_err(anyhow::Error::new)?; | |
| serde_json::to_writer(file, torrent)?; | |
| use std::path::PathBuf; | |
| use std::io::Write; | |
| use anyhow::Result; | |
| create_dir_all(&path).await.map_err(anyhow::Error::new)?; | |
| let file_path = path.join(format!("{}.json", torrent.id)); | |
| let json = serde_json::to_vec(torrent)?; | |
| tokio::fs::write(file_path, json).await.map_err(anyhow::Error::new)?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_web_api/src/search.rs` around lines 81 - 82, The code uses blocking
std::fs::File::create and serde_json::to_writer which will block the async
runtime; replace the blocking call with tokio::fs::File::create(...).await and
write the JSON asynchronously by serializing first (e.g. serde_json::to_vec or
to_string on torrent) and then calling tokio::io::AsyncWriteExt::write_all on
the tokio file (remember to .await and map errors into anyhow::Error as before).
Update the code around File::create, serde_json::to_writer, file_path and
torrent to use the async tokio APIs and ensure the surrounding function is
async.
| pub async fn torrent_api( | ||
| State(context): State<Context>, | ||
| Path(id_or_mam_id): Path<String>, | ||
| ) -> std::result::Result<Json<serde_json::Value>, AppError> { | ||
| if let Ok(id) = id_or_mam_id.parse() { | ||
| torrent_api_mam_id(State(context), Path(id)).await | ||
| } else { | ||
| torrent_api_id(State(context), Path(id_or_mam_id)).await | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if torrent IDs can be purely numeric by examining the codebase
rg -n "id_is_hash|torrent\.id" --type rust -C2 | head -50Repository: StirlingMouse/MLM
Length of output: 3320
🏁 Script executed:
cat -n mlm_web_api/src/torrent.rs | head -80Repository: StirlingMouse/MLM
Length of output: 2773
🏁 Script executed:
# Find the Torrent struct definition
rg -n "struct Torrent" --type rust -A 10Repository: StirlingMouse/MLM
Length of output: 42498
🏁 Script executed:
# Check if there's any id_is_hash usage in the API layer
rg -n "id_is_hash" mlm_web_api/ --type rust -C 3Repository: StirlingMouse/MLM
Length of output: 489
Consider documenting the ID parsing heuristic or using the explicit id_is_hash field.
The dispatch logic relies on attempting to parse the input as u64 to differentiate between MAM IDs and torrent IDs. While this works in practice because torrent hashes are typically alphanumeric, the code doesn't validate this assumption. The Torrent struct already tracks an id_is_hash field in the database schema; consider either:
- Adding a comment explaining that numeric-only IDs are assumed to be MAM IDs, or
- Refactoring to check the database's
id_is_hashflag directly for clarity
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_web_api/src/torrent.rs` around lines 14 - 23, The current router in
torrent_api uses a parse-to-u64 heuristic to decide between MAM and torrent-id
handling; update it to use the explicit id_is_hash flag (or at minimum document
the heuristic). Replace the parse-based dispatch inside torrent_api so it looks
up the record via Context (using the Torrent model/DB access) and checks the
Torrent.id_is_hash boolean to decide whether to call torrent_api_mam_id or
torrent_api_id (using the same State(context) and Path value), and if you keep
the heuristic add a clarifying comment above torrent_api noting “numeric-only →
MAM id” and reference the Torrent.id_is_hash field for future clarity; ensure
any DB lookup handles “not found” and maps errors to AppError consistently.
| <a href="/old/torrents/{{ mam_id }}" target=_blank>open</a> | ||
| <a href="https://www.myanonamouse.net/t/{{ mam_id }}" target=_blank>MaM</a> |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to _blank links.
Line 40 (and the adjacent Line 41) open a new tab without rel, which leaves window.opener exposed.
🔒 Suggested fix
- <a href="/old/torrents/{{ mam_id }}" target=_blank>open</a>
- <a href="https://www.myanonamouse.net/t/{{ mam_id }}" target=_blank>MaM</a>
+ <a href="/old/torrents/{{ mam_id }}" target="_blank" rel="noopener noreferrer">open</a>
+ <a href="https://www.myanonamouse.net/t/{{ mam_id }}" target="_blank" rel="noopener noreferrer">MaM</a>📝 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.
| <a href="/old/torrents/{{ mam_id }}" target=_blank>open</a> | |
| <a href="https://www.myanonamouse.net/t/{{ mam_id }}" target=_blank>MaM</a> | |
| <a href="/old/torrents/{{ mam_id }}" target="_blank" rel="noopener noreferrer">open</a> | |
| <a href="https://www.myanonamouse.net/t/{{ mam_id }}" target="_blank" rel="noopener noreferrer">MaM</a> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mlm_web_askama/templates/pages/errors.html` around lines 40 - 41, Both anchor
tags that use target=_blank (the link to "/old/torrents/{{ mam_id }}" and the
link to "https://www.myanonamouse.net/t/{{ mam_id }}") must include
rel="noopener noreferrer" to prevent exposing window.opener; update those <a>
elements in the errors.html template to add rel="noopener noreferrer" alongside
target=_blank.
| use mlm_web_api::router as api_router; | ||
| use mlm_web_askama::router as askama_router; | ||
| use mlm_web_dioxus::ssr::router as dioxus_router; | ||
| use tower_http::services::ServeDir; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused import: ServeDir is not used in this file.
The ServeDir import appears to be unused in server/src/main.rs. The actual ServeDir usage is in mlm_web_api/src/lib.rs. Consider removing this import to keep the code clean.
🧹 Proposed fix
use mlm_web_api::router as api_router;
use mlm_web_askama::router as askama_router;
use mlm_web_dioxus::ssr::router as dioxus_router;
-use tower_http::services::ServeDir;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/main.rs` at line 38, Remove the unused import statement "use
tower_http::services::ServeDir;" from main.rs: locate the top-level use/import
block where ServeDir is declared and delete that line (ServeDir is actually used
in mlm_web_api::lib, not here), then run cargo check to verify no remaining
unused imports.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
StirlingMouse
force-pushed
the
dioxus-switch
branch
from
bca5e5a to
1251f4c
Compare
1 participant
Summary by CodeRabbit
New Features
Refactor
/old/prefix.