fix: broken images in binary#10
Conversation
WalkthroughThe pull request introduces a new dependency to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Assets
User->>UI: Request to load icon
UI->>Assets: Attempt to retrieve icon
alt Icon found
Assets-->>UI: Return embedded icon
UI-->>User: Display icon
else Icon not found
Assets-->>UI: Return error message
UI-->>User: Display error message
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Cargo.toml (1 hunks)
- src/ui/components/left_panel.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/ui/components/left_panel.rs (3)
7-7: Importingrust_embed::RustEmbedis appropriateThe import statement correctly includes the
RustEmbedtrait necessary for embedding assets.
Line range hint
37-93: Verify that all icon paths specified are embedded correctlyEnsure that all icon files specified in the
buttonsarray are present in the embedded assets and that their paths are correct relative to theicons/folder.You can run the following script to check for the existence of the icon files:
9-11: Ensure the folder path in#[folder = "icons/"]is correctThe comment
// Adjust the folder path if necessarysuggests that the path may need verification. Please confirm that theicons/directory is the correct path relative to the crate root and contains the required icon assets.You can run the following script to verify that the
icons/directory exists and contains image files:
| image = { version = "0.25.2", default-features = false, features = ["png"] } | ||
| bitflags = "2.6.0" | ||
| libsqlite3-sys = { version = "0.30.1", features = ["bundled"] } | ||
| rust-embed = "6.4.0" No newline at end of file |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Update rust-embed to the Latest Version
The current dependency version rust-embed = "6.4.0" is outdated. The latest version available is 8.5.0, which includes important bug fixes and enhancements. Please consider updating to the latest version to ensure optimal performance and security.
- Review the rust-embed changelog for any breaking changes.
- Test the application to verify compatibility with the new version.
🔗 Analysis chain
Approved: Addition of rust-embed dependency.
The addition of the rust-embed dependency aligns well with the PR objectives of fixing broken images in the binary. This should allow for embedding icon assets directly into the application, potentially resolving the issue with loading icons and images for the sidebar.
Consider the following suggestions:
- Verify if version 6.4.0 is the latest stable version of
rust-embed. If not, consider updating to the latest stable version, unless there's a specific reason to use this version. - For better organization, you might want to group this dependency with other similar ones in the
Cargo.tomlfile, rather than adding it at the end.
Let's check if there's a newer version of rust-embed available:
This will help ensure we're using the most up-to-date version of the library.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the latest version of rust-embed
# Test: Query crates.io API for the latest version of rust-embed
curl -s https://crates.io/api/v1/crates/rust-embed | jq -r '.crate.max_version'
Length of output: 85
| eprintln!("Failed to load image from embedded data at path: {}", path); | ||
| None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a logging crate instead of eprintln!
Using eprintln! for error messages in a GUI application may not be ideal. Consider using a logging crate like log or env_logger for more flexible and configurable logging, which can be directed to files, consoles, or other outputs as needed.
| if let Some(content) = Assets::get(path) { | ||
| // Load the image from the embedded bytes | ||
| if let Ok(image) = image::load_from_memory(&content.data) { | ||
| let size = [image.width() as usize, image.height() as usize]; | ||
| let rgba_image = image.into_rgba8(); | ||
| let pixels = rgba_image.into_raw(); | ||
|
|
||
| Some(ctx.load_texture( | ||
| path, | ||
| egui::ColorImage::from_rgba_unmultiplied(size, &pixels), | ||
| Default::default(), | ||
| )) | ||
| Some(ctx.load_texture( | ||
| path, | ||
| egui::ColorImage::from_rgba_unmultiplied(size, &pixels), | ||
| Default::default(), | ||
| )) | ||
| } else { | ||
| eprintln!("Failed to load image from embedded data at path: {}", path); | ||
| None | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested if let statements for better readability
The nested if let statements in the load_icon function can be simplified to improve readability using combinators like and_then and map.
Here's an example of how you might refactor it:
fn load_icon(ctx: &Context, path: &str) -> Option<TextureHandle> {
Assets::get(path).and_then(|content| {
image::load_from_memory(&content.data).ok().map(|image| {
let size = [image.width() as usize, image.height() as usize];
let rgba_image = image.into_rgba8();
let pixels = rgba_image.into_raw();
ctx.load_texture(
path,
egui::ColorImage::from_rgba_unmultiplied(size, &pixels),
Default::default(),
)
})
})
}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/ui/components/left_panel.rs (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/ui/components/left_panel.rs (3)
7-7: LGTM: Asset embedding implementation looks good.The addition of the
RustEmbedimport and theAssetsstruct declaration with the#[folder = "icons/"]attribute is a good approach to embed the icon assets directly into the binary. This aligns well with the PR objective of fixing broken images in the final release binaries.Also applies to: 9-11
Line range hint
1-137: Overall implementation aligns well with PR objectives.The changes made in this file effectively address the PR's main objective of fixing broken images in the binary:
- The introduction of the
Assetsstruct withRustEmbedallows for embedding icon assets directly into the binary.- The
load_iconfunction has been updated to use these embedded assets instead of loading from file paths.- Icon paths in the
buttonsarray have been updated to reflect the new embedded asset structure.These changes should resolve the issue of images not loading in the final release binaries, as the icons are now part of the binary itself rather than being loaded from external files.
The implementation is solid, and with the suggested improvements (consistent error handling, function simplification, and fixing the inconsistent icon path), it will provide a robust solution to the image loading problem.
14-35: 🛠️ Refactor suggestionImprove error handling and simplify the
load_iconfunction.The new implementation correctly uses embedded assets, which is good. However, there are a few points to consider:
Error handling: Using
eprintln!for error messages in a GUI application may not be ideal. Consider using a logging crate likelogorenv_loggerfor more flexible and configurable logging.Function simplification: The nested
if letstatements can be simplified using combinators likeand_thenandmapfor better readability.Here's a suggested refactored version:
use log::error; fn load_icon(ctx: &Context, path: &str) -> Option<TextureHandle> { Assets::get(path) .and_then(|content| image::load_from_memory(&content.data).ok()) .map(|image| { let size = [image.width() as usize, image.height() as usize]; let rgba_image = image.into_rgba8(); let pixels = rgba_image.into_raw(); ctx.load_texture( path, egui::ColorImage::from_rgba_unmultiplied(size, &pixels), Default::default(), ) }) .or_else(|| { error!("Failed to load image from embedded assets at path: {}", path); None }) }This refactored version simplifies the code, improves error handling, and uses the
logcrate for logging errors.
| "I", | ||
| RootScreenType::RootScreenIdentities, | ||
| "icons/identity.png", | ||
| "identity.png", |
There was a problem hiding this comment.
Fix inconsistent icon path.
The icon paths have been updated to remove the "icons/" prefix, which is correct for the new embedded asset approach. However, there's an inconsistency in one of the paths:
"icons/tools.png", // Line 65This path still includes the "icons/" prefix, while others don't. To maintain consistency and ensure all icons load correctly, update this line to:
"tools.png",This change will align all icon paths with the new embedded asset structure.
Also applies to: 55-55, 60-60, 65-65, 70-70
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/ui/components/left_panel.rs (1)
13-36: Consider using a logging crate and simplifying the function.The changes to the
load_iconfunction look good overall and should resolve the image loading issues. However, there are two points to consider:
As mentioned in a previous review comment, using
eprintln!for logging in a GUI application might not be ideal. Consider using a logging crate likelogorenv_loggerfor more flexible and configurable logging.The function could be simplified for better readability. Here's a suggested refactor:
fn load_icon(ctx: &Context, path: &str) -> Option<TextureHandle> { Assets::get(path).and_then(|content| { image::load_from_memory(&content.data).ok().map(|image| { let size = [image.width() as usize, image.height() as usize]; let rgba_image = image.into_rgba8(); let pixels = rgba_image.into_raw(); ctx.load_texture( path, egui::ColorImage::from_rgba_unmultiplied(size, &pixels), Default::default(), ) }) }) }This refactored version uses combinators to simplify the nested
if letstatements, making the code more concise and easier to read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/ui/components/left_panel.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
src/ui/components/left_panel.rs (4)
7-11: LGTM: Embedded assets implementation looks good.The addition of the
RustEmbedcrate and theAssetsstruct is a good approach to embed icon assets directly into the application. This should resolve the issue with loading icons in the final release binaries.
50-50: LGTM: Icon paths updated correctly.The icon paths have been updated correctly to remove the "icons/" prefix, which is appropriate for the new embedded asset approach. The inconsistency mentioned in the previous review comment has been resolved, and all paths are now consistent.
Also applies to: 55-55, 60-60, 65-65, 70-70
Line range hint
37-137: LGTM: Panel rendering updated for embedded assets.The
add_left_panelfunction has been updated to work with the new embedded asset loading approach. The changes are minimal and should ensure that the panel renders correctly using the embedded icons.
Line range hint
1-137: Overall, good implementation of embedded assets.The changes in this file successfully implement embedded assets for icon loading, which should resolve the issue with broken images in the final release binaries. The code is well-structured and follows Rust best practices.
Key improvements:
- Introduction of the
Assetsstruct withRustEmbedfor embedding icon assets.- Updated
load_iconfunction to use embedded assets.- Improved error handling with more specific messages.
- Updated icon paths in the
buttonsarray to work with embedded assets.Consider the following suggestions for further improvement:
- Implement a proper logging solution instead of using
eprintln!.- Simplify the
load_iconfunction as suggested earlier for better readability.These changes should significantly improve the reliability of icon loading in the application.
- Remove dead `_is_sync_operation` parameter from `Database::set_platform_address_info` and all call sites (Finding #6) - Add doc comment on `set_platform_sync_info` explaining column name drift: `last_platform_sync_checkpoint` now stores SDK sync height (Finding #4) - Remove trivial `set_platform_address_info_from_sync` delegate and update callers to use `set_platform_address_info` directly (Finding #7) - Combine unnecessary `let provider` + `let mut provider = provider` rebinding into single `let mut` block (Finding #10) - Document UTXO selection race window on `broadcast_and_commit_asset_lock`: std::sync::RwLock guard is !Send so it cannot span async broadcast Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rmSyncMode (#635) * refactor(wallet): simplify platform sync by removing PlatformSyncMode Remove the PlatformSyncMode enum (Auto/ForceFull/TerminalOnly) and terminal sync logic (apply_recent_balance_changes, last_terminal_block, last_full_sync_balance). The SDK now handles incremental sync internally via AddressProvider::current_balances() and last_sync_height(). Key changes: - Remove PlatformSyncMode enum from backend_task::wallet - Simplify fetch_platform_address_balances to use new SDK API with stored state (with_stored_state, current_balances, last_sync_height) - Change CoreTask::RefreshWalletInfo to use bool instead of Option<PlatformSyncMode> - Remove last_full_sync_balance from PlatformAddressInfo - Simplify database sync info to 2-tuple (timestamp, height) - Remove set_last_terminal_block from database - Simplify RefreshMode enum (remove PlatformFull, PlatformTerminal, CoreAndPlatformFull, CoreAndPlatformTerminal variants) Note: requires updated dash-sdk with new sync_address_balances API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: update platform SDK to rev 0fa82e6652 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for platform sync simplification Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address PR #635 audit findings and extract broadcast helper - DB schema v28: drop obsolete columns (last_terminal_block, last_full_sync_balance), rename last_platform_sync_checkpoint → last_platform_sync_height, with SQLite ≥3.35 runtime check - Store asset lock TX before broadcast to prevent SPV InstantSend race - Defer UTXO removal until after successful broadcast - Replace .unwrap() on RwLock with .map_err() to avoid panics - Remove unused _is_sync_operation param and set_platform_address_info_from_sync wrapper - Fix redundant let-mut rebinding in fetch_platform_address_balances - Extract broadcast_and_commit_asset_lock() on AppContext to consolidate the store→broadcast→cleanup→UTXO-removal pattern from 5 code paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(db): schema v28 — drop obsolete sync columns and clean up DB interface (#653) - Bump DEFAULT_DB_VERSION 27 → 28 - Drop last_terminal_block from wallet table (unused after sync simplification) - Drop last_full_sync_balance from platform_address_balances table - Rename last_platform_sync_checkpoint → last_platform_sync_height - Add runtime SQLite ≥3.35 check (required for DROP COLUMN) - Idempotent migration: checks column existence before each ALTER - Remove unused _is_sync_operation param from set_platform_address_info() - Remove set_platform_address_info_from_sync() wrapper - Fix redundant let-mut rebinding in fetch_platform_address_balances Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * revert(db): remove schema v28 migration from this branch The v28 schema changes (drop obsolete sync columns, rename last_platform_sync_checkpoint → last_platform_sync_height) will be applied separately and should not ship on this branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review comments — remove dead code and document risks - Remove dead `_is_sync_operation` parameter from `Database::set_platform_address_info` and all call sites (Finding #6) - Add doc comment on `set_platform_sync_info` explaining column name drift: `last_platform_sync_checkpoint` now stores SDK sync height (Finding #4) - Remove trivial `set_platform_address_info_from_sync` delegate and update callers to use `set_platform_address_info` directly (Finding #7) - Combine unnecessary `let provider` + `let mut provider = provider` rebinding into single `let mut` block (Finding #10) - Document UTXO selection race window on `broadcast_and_commit_asset_lock`: std::sync::RwLock guard is !Send so it cannot span async broadcast Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: apply nightly fmt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
#10: Added top_up, identity_order, token_order to the v40 migration DELETE sweep. These tables FK to identity/token which are deleted, leaving orphaned rows. #11: Removed stale "may re-key the wallets map" comment from handle_wallet_unlocked (no re-keying occurs anymore). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
This should fix the issue in the final release binaries where we cannot load the icons/images for the sidebar in the application
Summary by CodeRabbit
New Features
Assetsstruct to embed icon assets directly into the application, improving loading efficiency.Bug Fixes
Chores
rust-embedversion6.4.0.