Feat/fix issues for android app - #18
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
filepi-server/src/handlers/files.rs(3 hunks)filepi-server/src/models/file_info.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
filepi-server/src/handlers/files.rs (1)
filepi-server/src/models/file_info.rs (1)
from_path(22-107)
🔇 Additional comments (4)
filepi-server/src/handlers/files.rs (1)
104-109: LGTM!The FileInfo::from_path call correctly passes the new root_dir parameter with proper error handling.
filepi-server/src/models/file_info.rs (3)
22-29: LGTM!The function signature correctly adds the
root_dirparameter to enable root-relative path computations. The generic bounds and parameter extraction are appropriate.
61-70: LGTM!The Unix-specific fallback to
ctimeis a reasonable approach for systems wherecreated()is not available. The conditional compilation is properly used.
84-93: LGTM!The path computations correctly use
root_diras the reference point forparent_dirandrel_path. The use ofOptionfor graceful handling ofstrip_prefixfailures is appropriate.
| // full_name should be relative to current_dir (without leading /) | ||
| let full_name = path | ||
| .strip_prefix(current) | ||
| .ok() | ||
| .map(|rel| rel.to_string_lossy().to_string()) | ||
| .unwrap_or_else(|| String::from(path.to_str().unwrap())); |
There was a problem hiding this comment.
Consider handling non-UTF-8 paths more gracefully.
The fallback uses .unwrap() which will panic if the path contains invalid UTF-8 sequences. While rare, this could occur with certain filesystem edge cases.
Apply this diff for safer error handling:
let full_name = path
.strip_prefix(current)
.ok()
.map(|rel| rel.to_string_lossy().to_string())
- .unwrap_or_else(|| String::from(path.to_str().unwrap()));
+ .unwrap_or_else(|| path.to_string_lossy().to_string());📝 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.
| // full_name should be relative to current_dir (without leading /) | |
| let full_name = path | |
| .strip_prefix(current) | |
| .ok() | |
| .map(|rel| rel.to_string_lossy().to_string()) | |
| .unwrap_or_else(|| String::from(path.to_str().unwrap())); | |
| // full_name should be relative to current_dir (without leading /) | |
| let full_name = path | |
| .strip_prefix(current) | |
| .ok() | |
| .map(|rel| rel.to_string_lossy().to_string()) | |
| .unwrap_or_else(|| path.to_string_lossy().to_string()); |
🤖 Prompt for AI Agents
In filepi-server/src/models/file_info.rs around lines 38 to 43, the fallback
branch uses path.to_str().unwrap() which will panic on non-UTF-8 paths; replace
the unwrap with a safe conversion (e.g., use path.to_string_lossy().to_string()
or otherwise handle the non-UTF-8 case) so the code never panics on invalid
UTF-8; optionally log or propagate an error when lossy conversion occurs if you
need to track that condition.
d03db30 to
92f6b0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
filepi-server/src/handlers/files.rs (1)
292-297:searchhandler fix looks good; consider DRYing up the repeated error mappingThis hunk correctly uses
&full_pathas the directory context, passes&config.root_dir, and handlesFileInfo::from_patherrors consistently with the other handlers. If this pattern keeps repeating, you might later extract a small helper (e.g.fn build_file_info(...) -> Result<FileInfo, AppError>) to avoid duplicating themap_errblock inget_files,get_videos, andsearch.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
filepi-server/src/handlers/files.rs(3 hunks)filepi-server/src/models/file_info.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- filepi-server/src/models/file_info.rs
🧰 Additional context used
🧬 Code graph analysis (1)
filepi-server/src/handlers/files.rs (1)
filepi-server/src/models/file_info.rs (1)
from_path(22-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Rust (arm64)
- GitHub Check: Build Blazor Frontend
🔇 Additional comments (2)
filepi-server/src/handlers/files.rs (2)
104-109: Updated FileInfo construction and error handling inget_fileslooks correct
FileInfo::from_pathis now called with(absolute_path, current_dir, root_dir)and errors are mapped intoAppError::InternalErrorwith logging and?, which aligns with the new API and avoids panics.
199-204:get_videosno longer panics on FileInfo errorsReplacing the previous
.unwrap()withmap_err(...)?and passing(&file_path, &full_path, &config.root_dir)correctly integrates the newFileInfo::from_pathsignature and ensures failures don’t crash the handler.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.