build: optimize docker build and update dependencies #215
Conversation
🔧 Build Status for PR #215🔗 Commit: 📦 Build Artifacts
⏳ Builds are starting up... This comment will update automatically as each build completes. Auto-generated by GitHub Actions |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPin many Cargo dependencies to exact versions in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
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 |
There was a problem hiding this comment.
Pull request overview
This PR focuses on performance and resource-usage optimizations across search, HTTP usage, magnet generation, and container builds/runtime configuration.
Changes:
- Share
ban_wordsacross parallel searches viaArcto reduce cloning/allocation overhead. - Speed up magnet link generation by precomputing the tracker suffix once and pre-sizing the output buffer.
- Improve runtime/build behavior by introducing static
reqwest::Client, Aho–Corasick-based title normalization, Actix response compression, Docker dependency caching, and updated logging/TLS/release settings.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/search.rs |
Updates ban_words parameter to Arc-wrapped vector for cheaper sharing. |
src/rest/search.rs |
Propagates Arc<Vec<String>> ban-words through batch search paths and query parsing. |
src/nostr.rs |
Precomputes encoded tracker suffix once and uses capacity planning for magnet generation. |
src/main.rs |
Switches to env_logger, enables Actix compression middleware, and sets keep-alive. |
src/dbs.rs |
Adds shared static HTTP client and Aho–Corasick-based title normalization + dedupe. |
src/config.rs |
Changes default log level from debug to info. |
docker/Dockerfile |
Adds Cargo dependency caching layer and adjusts runtime deps/user creation. |
Cargo.toml |
Adds deps/features (Aho–Corasick, env_logger, reqwest gzip/brotli), changes TLS roots feature, and sets panic = "abort". |
Cargo.lock |
Lockfile updates for the new/updated dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docker/Dockerfile (1)
83-88: Remove the unused/app/sessionsdirectory from the Dockerfile.The application does not implement any session management or session storage. The codebase contains no references to session handling, no session directory operations, and no session-related configuration. The
/app/sessionsdirectory created at line 84 is unused and can be safely removed to simplify the image.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/Dockerfile` around lines 83 - 88, Remove the unused session dir creation in the Dockerfile by deleting the mkdir -p /app/sessions step from the RUN command that currently includes chmod +x /app/ygege && mkdir -p /app/sessions && groupadd ... && chown -R ygege:ygege /app /home/ygege; keep the rest of the chained commands intact (chmod +x /app/ygege, groupadd, useradd, mkdir -p /home/ygege, chown) so ownership and user setup still run as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/dbs.rs`:
- Around line 171-180: The current code aggressively trims any leading/trailing
"1" characters from variable title (used before calling fix_title and
push_title), which corrupts legitimate titles; replace the two match blocks with
a single targeted removal that only strips a standalone sequel "1" at the end
(e.g., use a regex or string check like replacing r"\s+1$" with "" then
.trim()), leaving leading "1" alone and preserving embedded digits; update the
logic around variable title (the same binding used before calling fix_title and
push_title) so only titles ending with a space + "1" drop that suffix and any
trailing whitespace.
---
Nitpick comments:
In `@docker/Dockerfile`:
- Around line 83-88: Remove the unused session dir creation in the Dockerfile by
deleting the mkdir -p /app/sessions step from the RUN command that currently
includes chmod +x /app/ygege && mkdir -p /app/sessions && groupadd ... && chown
-R ygege:ygege /app /home/ygege; keep the rest of the chained commands intact
(chmod +x /app/ygege, groupadd, useradd, mkdir -p /home/ygege, chown) so
ownership and user setup still run as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15cf87bd-2e62-4dbe-abe7-b521464fcfe8
📥 Commits
Reviewing files that changed from the base of the PR and between 600b122 and c5b1c5ad29999853cf08aa8cca023d88e266df30.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomldocker/Dockerfilesrc/config.rssrc/dbs.rssrc/main.rssrc/nostr.rssrc/rest/search.rssrc/search.rs
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dbs.rs (1)
75-80:⚠️ Potential issue | 🔴 CriticalParse the IMDb
/findpayload before reading movie fields.TMDB's
/find/{external_id}response nests movie matches undermovie_results[]. The code readsid,release_date,original_title, andtitlefrom the top-level object, causingDbQueryType::IMDBto fail on valid IMDb IDs instead of producing queries. (Confirmed via developer.themoviedb.org)🛠️ Suggested fix
let body = response.text().await?; let json: serde_json::Value = serde_json::from_str(&body)?; + let movie = match db_type { + DbQueryType::TMDB => &json, + DbQueryType::IMDB => json + .get("movie_results") + .and_then(|v| v.as_array()) + .and_then(|results| results.first()) + .ok_or("Movie not found in TMDB find response")?, + }; - let id = json + let id = movie .get("id") .and_then(|i| i.as_u64()) .ok_or("ID not found in TMDB response")?; - let year = json + let year = movie .get("release_date") .and_then(|rd| rd.as_str()) .and_then(|date_str| date_str.split('-').next()) .and_then(|year_str| year_str.parse::<u32>().ok()) .unwrap_or(0); - let original_title = json + let original_title = movie .get("original_title") .and_then(|ot| ot.as_str()) .ok_or("Original title not found in TMDB response")? .to_string(); - let title = json + let title = movie .get("title") .and_then(|t| t.as_str()) .ok_or("Title not found in TMDB response")? .to_string();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dbs.rs` around lines 75 - 80, The IMDB branch currently treats the /find response like a movie object; update the DbQueryType::IMDB handling in dbs.rs to parse the JSON response's "movie_results" array (take the first element if present) and then read id, release_date, original_title, and title from that nested object before building the query; ensure you handle the empty array case gracefully (no results) and reuse the same extraction logic used for DbQueryType::TMDB to construct the query.
♻️ Duplicate comments (1)
src/dbs.rs (1)
171-178:⚠️ Potential issue | 🟡 MinorOnly strip a standalone sequel suffix.
This still removes every boundary
1, so valid titles like1984or11:14get mangled before dedupe. Use a targeted trailing" 1"strip instead oftrim_*_matches("1").✂️ Suggested change
- let title = match title.ends_with("1") { - true => title.trim_end_matches("1").trim(), - false => title, - }; - let title = match title.starts_with("1") { - true => title.trim_start_matches("1").trim(), - false => title, - }; + let title = title.strip_suffix(" 1").unwrap_or(title).trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dbs.rs` around lines 171 - 178, The title normalization is removing any boundary "1" characters (mangling titles like "1984"); instead, only strip a standalone sequel suffix/prefix. Update the logic around the local variable title to check for trailing " 1" (use title.ends_with(" 1") and title.trim_end_matches(" 1").trim()) and for leading "1 " (use title.starts_with("1 ") and title.trim_start_matches("1 ").trim()) rather than trim_*_matches("1"), keeping the variable name title and the surrounding assignment style unchanged.
🧹 Nitpick comments (2)
src/main.rs (2)
125-125: Consider moving the keep-alive timeout into config.
75is deployment-sensitive. Hard-coding it here means tuning for different ingress/load balancer idle timeouts requires a code change instead of a config change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` at line 125, The hard-coded keep-alive Duration::from_secs(75) should be moved to configuration: add a server keep-alive setting (e.g., server.keep_alive_secs or keep_alive_seconds) in your app config/Settings type and parse it into a std::time::Duration, then replace the literal with .keep_alive(Duration::from_secs(config.server.keep_alive_secs.unwrap_or(75))); update the code locations that build the server (the builder call containing .keep_alive(...)) to use the new config value and provide a sensible default/fallback if the config key is missing.
116-123: Hoist sharedConfigintoweb::Dataonce.This still creates a fresh
Data<Config>per worker. Since the PR is explicitly reducing allocation churn, build theweb::Datawrapper once beforeHttpServer::newand clone the handle inside the factory instead.♻️ Proposed change
- let nostr_data = web::Data::new(nostr_client); - let config_clone = config.clone(); + let nostr_data = web::Data::new(nostr_client); + let config_data = web::Data::new(config.clone()); HttpServer::new(move || { App::new() .wrap(middleware::Compress::default()) .app_data(nostr_data.clone()) - .app_data(web::Data::new(config_clone.clone())) + .app_data(config_data.clone()) .configure(rest::config_routes) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 116 - 123, Currently a new web::Data<Config> is constructed per worker by calling web::Data::new(config_clone.clone()) inside the HttpServer factory; instead create the Data wrapper once (e.g., let config_data = web::Data::new(config.clone())) before HttpServer::new and then use config_data.clone() inside the closure (replace references to config_clone/web::Data::new in the App builder), so each worker reuses a cheap clone of the already-wrapped Config rather than allocating a new Data instance per worker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/Dockerfile`:
- Around line 68-70: The runtime Dockerfile removed curl causing the Docker
Compose healthcheck (compose.yml healthcheck using `curl --fail /health`) to
fail even though `/health` is served by src/rest/infos.rs; restore installation
of a minimal curl binary in the RUN apt-get install line (add `curl` to the
package list in the Dockerfile RUN that installs ca-certificates and passwd) so
the compose health probe can execute successfully.
- Around line 40-43: The RUN layer that pre-warms the dependency cache runs
"cargo build --release" but ends the && chain with a semicolon before "rm -rf
src", so a failing cargo build is masked by the succeeding rm command; change
the command chain in the RUN instruction so that "cargo build --release" is
joined to the cleanup with && (i.e., ensure the
BUILD_COMMIT/BUILD_DATE/BUILD_BRANCH wrapper and cargo build are followed by &&
rm -rf src) so the layer fails if cargo build fails.
In `@src/dbs.rs`:
- Around line 8-9: The shared HTTP client created in http_client() and stored in
HTTP_CLIENT needs explicit timeouts to avoid hanging requests; change the
initialization closure passed to HTTP_CLIENT.get_or_init to build a
reqwest::Client via Client::builder() and set both connect_timeout (e.g.,
Duration::from_secs(10)) and request timeout (ClientBuilder::timeout, e.g.,
Duration::from_secs(30)) before calling build().unwrap() (or propagate the error
appropriately), and ensure Duration is imported; keep the function signature
http_client() -> &'static Client and only replace Client::new with the
Client::builder(...).build() variant that includes the timeouts.
---
Outside diff comments:
In `@src/dbs.rs`:
- Around line 75-80: The IMDB branch currently treats the /find response like a
movie object; update the DbQueryType::IMDB handling in dbs.rs to parse the JSON
response's "movie_results" array (take the first element if present) and then
read id, release_date, original_title, and title from that nested object before
building the query; ensure you handle the empty array case gracefully (no
results) and reuse the same extraction logic used for DbQueryType::TMDB to
construct the query.
---
Duplicate comments:
In `@src/dbs.rs`:
- Around line 171-178: The title normalization is removing any boundary "1"
characters (mangling titles like "1984"); instead, only strip a standalone
sequel suffix/prefix. Update the logic around the local variable title to check
for trailing " 1" (use title.ends_with(" 1") and title.trim_end_matches("
1").trim()) and for leading "1 " (use title.starts_with("1 ") and
title.trim_start_matches("1 ").trim()) rather than trim_*_matches("1"), keeping
the variable name title and the surrounding assignment style unchanged.
---
Nitpick comments:
In `@src/main.rs`:
- Line 125: The hard-coded keep-alive Duration::from_secs(75) should be moved to
configuration: add a server keep-alive setting (e.g., server.keep_alive_secs or
keep_alive_seconds) in your app config/Settings type and parse it into a
std::time::Duration, then replace the literal with
.keep_alive(Duration::from_secs(config.server.keep_alive_secs.unwrap_or(75)));
update the code locations that build the server (the builder call containing
.keep_alive(...)) to use the new config value and provide a sensible
default/fallback if the config key is missing.
- Around line 116-123: Currently a new web::Data<Config> is constructed per
worker by calling web::Data::new(config_clone.clone()) inside the HttpServer
factory; instead create the Data wrapper once (e.g., let config_data =
web::Data::new(config.clone())) before HttpServer::new and then use
config_data.clone() inside the closure (replace references to
config_clone/web::Data::new in the App builder), so each worker reuses a cheap
clone of the already-wrapped Config rather than allocating a new Data instance
per worker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3120639-0eea-4c06-a234-72c2abc4dcd3
📥 Commits
Reviewing files that changed from the base of the PR and between c5b1c5ad29999853cf08aa8cca023d88e266df30 and c9fe40fc3c3def6ecb69c8de818f144d3f84e7ac.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomldocker/Dockerfilesrc/config.rssrc/dbs.rssrc/main.rssrc/nostr.rssrc/rest/search.rssrc/search.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/rest/search.rs
- Cargo.toml
- src/search.rs
- src/config.rs
- src/nostr.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Cargo.toml (1)
27-32:⚠️ Potential issue | 🟠 Major
panic = "abort"turns any remaining panic into a process exit.The release profile sets
panic = "abort", which terminates the process immediately on panic instead of unwinding. The codebase contains multiple unwrap/expect calls throughout request/relay paths (src/rest/search.rs, src/nostr.rs, src/rate_limiter.rs), meaning a panic in these hot paths will hard-crash the service with no cleanup or recovery mechanism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 27 - 32, The release profile currently sets panic = "abort", which will hard-crash on any remaining unwrap/expect; change the release profile to panic = "unwind" (or remove the abort override) so panics allow cleanup, and then audit and replace unwrap/expect usages in hot paths (notably functions in src/rest/search.rs, src/nostr.rs, and src/rate_limiter.rs) with proper Result-based error handling or graceful fallbacks; ensure each replaced unwrap/expect returns a meaningful error or logs context via the surrounding handlers so the service can recover or fail the request without terminating the process.src/dbs.rs (1)
82-88:⚠️ Potential issue | 🟠 MajorExtract TMDB movie ID from
/findresponse before parsing movie fields.The
DbQueryType::IMDBbranch calls TMDB's/3/find/{external_id}endpoint, which returns results inside amovie_results[]array; the/3/movie/{id}endpoint exposesid,title,original_title, andrelease_dateat the root level. The parsing code (lines 108–130) assumes the movie-details schema and will fail with "ID not found in TMDB response" for IMDB lookups. Extract the first movie frommovie_results[0], recover its TMDBid, and then continue with the existing parsing and alternative-titles flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dbs.rs` around lines 82 - 88, The IMDB branch builds a `/3/find/{id}` URL but the later parsing code (that expects root-level fields like id, title, original_title, release_date and the alternative-titles flow) fails because `/find` returns movie_results[]; update the DbQueryType::IMDB handling to: call the `/3/find/{external_id}` endpoint, extract the first entry from the response's movie_results array and read its TMDB id, then either set the TMDB id for downstream parsing or perform a second request to `/3/movie/{tmdb_id}` so the existing parsing logic (which reads id, title, original_title, release_date and then fetches alternative titles) can run unchanged; reference DbQueryType::IMDB, movie_results[0], and the code path that expects root-level movie fields.
🧹 Nitpick comments (1)
src/rest/search.rs (1)
134-145: Lowercaseban_wordsonce at the request boundary.
src/search.rs:35-40still lowercases every ban word for every torrent candidate. If you normalize here during parsing and drop the innerw.to_lowercase()downstream, the newArcsharing actually translates into less work instead of just fewer clones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rest/search.rs` around lines 134 - 145, Normalize ban_words to lowercase at parsing: when building the Arc<Vec<String>> assigned to ban_words, map each trimmed word to its lowercase form (e.g., word.trim().to_lowercase()) so the stored Vec contains lowercase entries; then remove the per-candidate lowercase calls (the downstream w.to_lowercase() usage in the candidate filtering code in src/search.rs) so the algorithm uses the pre-normalized Arc<String>s and avoids repeated allocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Cargo.toml`:
- Around line 27-32: The release profile currently sets panic = "abort", which
will hard-crash on any remaining unwrap/expect; change the release profile to
panic = "unwind" (or remove the abort override) so panics allow cleanup, and
then audit and replace unwrap/expect usages in hot paths (notably functions in
src/rest/search.rs, src/nostr.rs, and src/rate_limiter.rs) with proper
Result-based error handling or graceful fallbacks; ensure each replaced
unwrap/expect returns a meaningful error or logs context via the surrounding
handlers so the service can recover or fail the request without terminating the
process.
In `@src/dbs.rs`:
- Around line 82-88: The IMDB branch builds a `/3/find/{id}` URL but the later
parsing code (that expects root-level fields like id, title, original_title,
release_date and the alternative-titles flow) fails because `/find` returns
movie_results[]; update the DbQueryType::IMDB handling to: call the
`/3/find/{external_id}` endpoint, extract the first entry from the response's
movie_results array and read its TMDB id, then either set the TMDB id for
downstream parsing or perform a second request to `/3/movie/{tmdb_id}` so the
existing parsing logic (which reads id, title, original_title, release_date and
then fetches alternative titles) can run unchanged; reference DbQueryType::IMDB,
movie_results[0], and the code path that expects root-level movie fields.
---
Nitpick comments:
In `@src/rest/search.rs`:
- Around line 134-145: Normalize ban_words to lowercase at parsing: when
building the Arc<Vec<String>> assigned to ban_words, map each trimmed word to
its lowercase form (e.g., word.trim().to_lowercase()) so the stored Vec contains
lowercase entries; then remove the per-candidate lowercase calls (the downstream
w.to_lowercase() usage in the candidate filtering code in src/search.rs) so the
algorithm uses the pre-normalized Arc<String>s and avoids repeated allocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 917d25a3-1402-4da4-bdb1-ab2979444f20
📥 Commits
Reviewing files that changed from the base of the PR and between c9fe40fc3c3def6ecb69c8de818f144d3f84e7ac and 566d8d2409bd216e9f434a15f8f054070364daa3.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomldocker/Dockerfilesrc/config.rssrc/dbs.rssrc/main.rssrc/nostr.rssrc/rest/search.rssrc/search.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main.rs
- src/config.rs
UwUDev
left a comment
UwUDev
left a comment
There was a problem hiding this comment.
Aussi je ne vois pas bien l'interet de fotre des Arc partout ? juste clone suffit car le string ne vit que pendant la durée de vie de la requete
Gauvino
changed the title
|
@coderabbitai fill the pr description |
🧩 Analysis chainI'll update the PR description now based on all the changes in this PR. 🏁 Script executed: Length of output: 611
DescriptionThis PR introduces a series of performance optimizations targeting search efficiency, memory allocation, HTTP resource usage, magnet link generation, and build pipeline performance. Changes🔍 Title Normalization — Aho–Corasick (
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Cargo.toml (1)
31-31: Minor: trailing whitespace on line 31.This line appears to have been modified but only contains whitespace. Consider removing the trailing space.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` at line 31, Remove the trailing whitespace-only content on the empty line in Cargo.toml (the modified blank line around the existing dependency entries); simply delete the space so the line is truly empty (or remove the blank line entirely) to eliminate the whitespace-only change.
🤖 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 12: Your code currently removes the pretty_env_logger dependency but
still calls extern crate pretty_env_logger and
pretty_env_logger::formatted_builder(), causing a compile error; either re-add
pretty_env_logger to Cargo.toml or update the initialization in main by removing
extern crate pretty_env_logger and replacing
pretty_env_logger::formatted_builder() with
env_logger::Builder::from_default_env() then configure filters (filter,
filter_module) and call init() to match the previous behavior (refer to symbols
extern crate pretty_env_logger, pretty_env_logger::formatted_builder,
env_logger::Builder::from_default_env, filter_module, init).
---
Nitpick comments:
In `@Cargo.toml`:
- Line 31: Remove the trailing whitespace-only content on the empty line in
Cargo.toml (the modified blank line around the existing dependency entries);
simply delete the space so the line is truly empty (or remove the blank line
entirely) to eliminate the whitespace-only change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3a27258-dc0e-48a8-a815-c6020d7ebb00
📥 Commits
Reviewing files that changed from the base of the PR and between 62efebaa0a1911ca8d046797dfee2621c5ec3ad7 and 03517bad16df83ff5c5c4ca3cbc5d425235ed778.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomldocker/Dockerfile
pins crate versions in the project manifest to ensure reproducible builds. replaces the logging implementation and adds gzip compression support to the http client. implements dependency caching in the dockerfile to significantly reduce build times. improves container security and efficiency by refining user management and minimizing layer size.
|
@coderabbitai make a full review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docker/Dockerfile (1)
83-90: Reconsider baking in/app/sessionsas a volume.I don’t see an in-repo consumer for this path in the provided application entrypoint context, so this turns the container stateful by default without a clear owner. Unless another entrypoint or deployment artifact depends on it, I’d drop the directory/
VOLUMEor document why it must exist.Possible simplification
RUN chmod +x /app/ygege \ - && mkdir -p /app/sessions \ && groupadd --gid 10001 --system ygege \ && useradd --uid 10001 --system --gid ygege --home-dir /home/ygege --shell /sbin/nologin --no-log-init ygege \ && mkdir -p /home/ygege \ && chown -R ygege:ygege /app /home/ygege - -VOLUME ["/app/sessions"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/Dockerfile` around lines 83 - 90, The Dockerfile currently bakes in a container-scoped persistent mount by creating /app/sessions and declaring VOLUME ["/app/sessions"]; remove the stateful default by deleting the mkdir -p /app/sessions line and the VOLUME declaration (or alternatively replace them with a runtime-controlled approach such as creating the directory in your entrypoint only when an env flag indicates persistence is required), and if you must keep the volume, add a short comment documenting its intended owner/consumer and why it’s required so callers know who is responsible for managing that state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/Dockerfile`:
- Around line 45-46: The Dockerfile's "COPY . ." step re-copies the entire repo
each build and invalidates the dependency-cache split; add a root .dockerignore
file to exclude build artifacts and VCS metadata (at minimum include entries
like target/, .git/, .gitignore, .cargo/) so the dependency layer remains
cacheable and the COPY . . step doesn't bloat the build context.
---
Nitpick comments:
In `@docker/Dockerfile`:
- Around line 83-90: The Dockerfile currently bakes in a container-scoped
persistent mount by creating /app/sessions and declaring VOLUME
["/app/sessions"]; remove the stateful default by deleting the mkdir -p
/app/sessions line and the VOLUME declaration (or alternatively replace them
with a runtime-controlled approach such as creating the directory in your
entrypoint only when an env flag indicates persistence is required), and if you
must keep the volume, add a short comment documenting its intended
owner/consumer and why it’s required so callers know who is responsible for
managing that state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 280f98d5-3dc9-41a6-9842-135816d98909
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.tomldocker/Dockerfile
| # Copy project files and build | ||
| COPY . . |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
if [ -f .dockerignore ]; then
echo ".dockerignore found"
echo
sed -n '1,200p' .dockerignore
echo
echo "Relevant ignore rules:"
rg -n '^(target/|\.git/|\.cargo/)' .dockerignore || true
else
echo ".dockerignore is missing"
fiRepository: UwUDev/ygege
Length of output: 80
Add .dockerignore to prevent build context bloat and preserve cache benefits.
The dependency-cache split requires a root .dockerignore that excludes target/, .git/, .cargo/, and other build artifacts. Without it, COPY . . will re-copy all local files on every build, invalidating the cached dependency layer and defeating the optimization.
Create .dockerignore with at least:
target/
.git/
.gitignore
.cargo/
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/Dockerfile` around lines 45 - 46, The Dockerfile's "COPY . ." step
re-copies the entire repo each build and invalidates the dependency-cache split;
add a root .dockerignore file to exclude build artifacts and VCS metadata (at
minimum include entries like target/, .git/, .gitignore, .cargo/) so the
dependency layer remains cacheable and the COPY . . step doesn't bloat the build
context.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
updates the exclusion list to minimize the build context size and improve build performance. reorganizes ignored files into logical categories and includes additional patterns for documentation, ci configurations, nix files, and testing artifacts.
Summary by CodeRabbit