Conversation
WalkthroughThe PR removes LWJGL-specific configuration and processing: deletes Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "retrieve_data()"
participant Patcher as "Library Patcher"
participant Store as "Manifest / S3"
rect rgb(240, 255, 240)
Note over Client,Patcher: Before (old flow with LWJGL variants)
Client->>Client: create LWJGL variant buckets
Client->>Patcher: call lwjgl-specific processing
Patcher->>Patcher: validate native classifiers & decide accept/reject
alt accepted
Patcher->>Store: upload LWJGL-specific artifacts
Store->>Client: update manifest with variant entry
end
end
rect rgb(255, 250, 240)
Note over Client,Patcher: After (new flow)
Client->>Patcher: apply generic library patches to all libraries
Patcher->>Store: upload patched libraries uniformly
Store->>Client: update manifest without LWJGL-specific variant entries
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)daedalus_client/src/minecraft/mod.rs (4)
🔇 Additional comments (6)
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: 0
🧹 Nitpick comments (4)
daedalus_client/src/minecraft/mod.rs (4)
155-157: Avoid Arc<&Vec<_>>; hold patches as Arc<[LibraryPatch]> and pass slices explicitly.Current setup uses
Arc<&Vec<_>>captured across async tasks; this is non‑idiomatic and fragile. Store patches as an owned Arc and pass&[LibraryPatch]topatch_library.Apply this at callsite:
- let mut libs = library_patches::patch_library(&patches, library.clone()); + let mut libs = library_patches::patch_library(Arc::as_ref(&patches), library.clone()); new_libraries.append(&mut libs);And make these supporting changes outside this hunk:
@@ - let patches = library_patches::get_library_patches().await?; - let cloned_patches = Arc::new(&patches); + // Own the patches and share as an Arc slice to avoid borrowed refs in futures + let patches: Arc<[LibraryPatch]> = + Arc::from(library_patches::get_library_patches().await?.into_boxed_slice()); @@ - let patches = Arc::clone(&cloned_patches); + let patches = Arc::clone(&patches);Rationale:
patch_librarytakes&[LibraryPatch](see library_patches.rs), soArc::as_ref(&patches)cleanly yields a slice without relying on layered deref coercions. Based on relevant code snippet in library_patches.rs.
137-151: Also patch the Log4j replacement (or confirm no rules target Log4j).Right now, Log4j libs are either replaced or passed through unmodified. If any
LibraryPatchrules target Log4j artifacts, they won’t run for the replaced case.Minimal change:
- let replacement_library = log4j::create_log4j_replacement_library( + let mut replacement_library = log4j::create_log4j_replacement_library( &spec.artifact, &version_override, &maven_override, library.include_in_classpath, )?; - new_libraries.push(replacement_library); + // Mark for traceability and run through patcher for consistency + replacement_library.patched = true; + let mut libs = library_patches::patch_library( + Arc::as_ref(&patches), + replacement_library, + ); + new_libraries.append(&mut libs);If you intentionally excluded Log4j from patch rules, please confirm and we’ll keep the current flow. Based on relevant code snippet in library_patches.rs.
46-50: Doc tweak: clarify LWJGL “fixes” vs removed variant processing.To avoid confusion now that variant infra is removed, consider clarifying that LWJGL “fixes” are handled via generic library patches, not variant detection.
Example:
- “Applies library patches (e.g., LWJGL-related fixes; no LWJGL variant processing).”
333-346: Run each chunk concurrently (join_all or FuturesUnordered) for speed.Within each chunk you await futures sequentially. To actually process up to 100 versions in parallel (and still rely on the semaphore for I/O backpressure), consider
join_all.Sketch:
@@ - for future in chunk { - match future.await { + for result in futures::future::join_all(chunk).await { - Ok(_) => { + match result { Ok(_) => { successful += 1; } Err(e) => { warn!("⚠️ Minecraft - Failed to process version: {}", e); failed += 1; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
daedalus_client/lwjgl-config.json(0 hunks)daedalus_client/src/minecraft/lwjgl.rs(0 hunks)daedalus_client/src/minecraft/mod.rs(5 hunks)daedalus_client/src/minecraft/types.rs(0 hunks)
💤 Files with no reviewable changes (3)
- daedalus_client/lwjgl-config.json
- daedalus_client/src/minecraft/types.rs
- daedalus_client/src/minecraft/lwjgl.rs
🧰 Additional context used
🧬 Code graph analysis (1)
daedalus_client/src/minecraft/mod.rs (2)
daedalus_client/utils/restore_split_natives.py (1)
LibraryPatch(297-306)daedalus_client/src/minecraft/library_patches.rs (2)
patch_library(16-56)patches(19-22)
🔇 Additional comments (2)
daedalus_client/src/minecraft/mod.rs (2)
28-28: Public API change: confirm downstream impact and changelog.Re-exports now surface only
types::LibraryPatch. If consumers previously imported LWJGL types from this module, this is breaking. Add a changelog note and, if applicable, a semver bump.
118-118: Nice: centralized library processing path.Routing all non‑Log4j libs through
patch_librarykeeps LWJGL fixes alive post‑refactor and simplifies the flow.
1 participant
Summary by CodeRabbit