Merged
Conversation
- Add wasm based plugin - Add plugin manager, Plugin Registry and wit - Add file picker function and integrate with file-loader plugin - Add doc for plugin system - Add card component - Add plugin setting tab - Add plugin errors to error enum
…on component system with consistent theming - Create new Button component with StatelessComponent architecture * Support for 2 button types: Elevated (solid background) and Text (transparent with hover effect) * Support for 3 color variants: Default, Danger, and Success * Automatic text color contrast based on background luminance (WCAG 2.0) * Optional width/height sizing with ui.add_sized wrapper - Enhance ToggleSwitch component * Add hover_text Option<String> support * Implement smooth toggle animation using animate_bool() and lerp interpolation * Improve visual feedback with animation - Refactor IconButton component * Integrate theme system colors for consistency * Apply same hover effect pattern as text buttons (40-alpha background overlay) * Maintain all icon-specific features (badges, disabled state, tooltips) - Replace all egui::Button::new instances throughout codebase * Update 12+ files with new Button component (settings dialogs, modals, panels, cards, search, etc.) * Update ButtonProps field initializers to include width/height fields * Add StatelessComponent trait imports where needed - Update theme system * Add get_contrast_text_color(bg_color) function using WCAG 2.0 relative luminance formula * Determine optimal black or white text color based on background brightness * Integrate with ThemeColors and Catppuccin theme palette - Improve plugin system and other components with enhanced styling consistency
|
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:
📝 WalkthroughWalkthroughAdds a Wasmtime-based WebAssembly plugin system (discovery, registry, WASM-backed loaders/viewers), WIT SDK and bindings, a CSV plugin, UI for plugin management, notifications, FileKind/FileType refactor, egui::Ui threading changes, and a build script to compile plugins into Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main (startup)
participant Notify as NotificationManager
participant PluginMgr as PluginManager
participant FS as Filesystem
participant Wasm as Wasmtime Engine
participant Registry as PluginRegistry
participant Store as Wasm Store
Main->>Notify: init NOTIFICATION_MANAGER
Main->>PluginMgr: spawn plugin loader thread
PluginMgr->>FS: scan plugin directories (bundled, user, debug)
FS-->>PluginMgr: found plugin dirs (plugin.toml, plugin.wasm, icon)
PluginMgr->>Wasm: load component (validate exports, WASI ctx, fuel)
Wasm-->>Store: instantiate component
Store-->>PluginMgr: lifecycle call on_load / open checks
PluginMgr->>Registry: register plugin metadata & capabilities
PluginMgr->>Notify: emit loading/completion notifications
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related issues
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add notification indicator - Add notification dropdown - Add running task indicator - Create List component
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add table viewer support for csv plugin - Use `egui_extras` for table rendering - Add render node struct to integrate rendering from plugins - Update wit bindings to support render nodes - Add wasm file viewer loader component to handle plugins with both viewer and loader
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Allow individual plugins to be disabled - Add plugin uninstall method - Update plugin card design for setting
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add SearchProvider - Add NewUiComponent
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
anitnilay20
force-pushed
the
feat/plugin-system#16
branch
from
57a143b to
00ca0e0
Compare
Bug fixes:
- plugin/manager: bundled_plugins_dir returned assets/plugins causing
scan_directory to produce .../assets/plugins/plugins; now returns assets/
- plugin/manager: per-entry plugin.toml parse errors now log + continue
instead of aborting the whole scan; remove stale BUNDLE location assignment;
remove #[derive(Default)] (default Engine lacks consume_fuel)
- plugin/plugin_registry: find_loader_for_extension was nondeterministic
(HashSet iteration); now sorts IDs before iterating; add_plugin now removes
stale capability entries when replacing a plugin with the same ID
- wasm_loader: invoke on_load lifecycle hook after instantiation; add Drop
impl calling on_close; document intentional WASI stdio/filesystem grants
- context_menu: Copy Key and Copy Path buttons were rendered unconditionally
but click-handling was gated on config flags; wrap entire render+handler
in the guard so visibility and behavior are consistent
- file_picker: respect plugins.enabled flag; remove duplicate JSON filter
(was inserted both hardcoded and via supported_files()); add plugins_enabled
param to pick_file() and propagate through toolbar and thoth_app call sites
- build.rs: move cargo-component availability check outside plugin loop;
replace create_dir_all().unwrap() with graceful error + continue
- Cargo.toml: replace misleading exclude=[] with default-members=["."] so
cargo build at workspace root does not attempt to compile wasm plugins
- icon_button: hover background was painted after the icon widget, overlaying
the glyph; now uses allocate_exact_size first, paints background, then
paints the icon with painter().text() so z-order is correct
Nitpicks:
- plugin/mod.rs: remove unnecessary .to_string() allocations in Display impl
- settings_panel: height: Some(0.0) -> height: None on all four buttons
- error/handler: fix comment spacing (//Plugins -> // Plugins)
- card.rs: rename loop variable events -> event; log icon read/decode errors
with path context instead of swallowing silently
- appearance.rs: placeholder dropdowns now show "Coming soon" tooltip
- plugins.rs: add comment explaining OnceLock<Option<PluginManager>> pattern
- csv-loader: replace manual split(',') with csv::ReaderBuilder for correct
quoted-field and TSV handling; raw_bytes now returns original delimited
bytes via ByteRecord instead of JSON-projection bytes
Fix test compilation errors and update assertions
- file_loading_tests: replace removed LazyJsonFile with FileType alias;
raw_slice() exists on FileType so raw_bytes test unchanged; restore correct
is_err() assertion for empty ndjson (sniff_file_type still returns Err)
- component_tests / mocks: ContextComponent::render signature changed from
(&egui::Context) to (&mut egui::Ui); update MockContextComponent impl,
run_context_test helper, and all call sites to pass ui instead of ctx
- common/mod.rs: run_context_test now delegates to run_ui_test so both
provide &mut Ui; suppress deprecated egui API warnings with #[allow]
- settings_dialog/tests: Plugins tab was added to SettingsTab::all() raising
count from 7 to 8; update assertion and add Plugins to the ordered list
Fix bench: use FileKind instead of FileType in start_scanning_internal
start_scanning_internal expects &FileKind (the lightweight Copy discriminant),
not &FileType (the fat enum with file handles). Replace the import and all six
call sites accordingly.
Fix deprecated Panel::show, code review bugs, and table header shadow
egui/eframe migration:
- Replace Panel::show(ctx) with show_inside(ui) across toolbar, status bar,
sidebar, central panel, and settings dialog (egui 0.34 deprecation)
- Move rendering from App::update to App::ui; non-rendering logic stays in update
- Fix settings dialog viewport callback signature (now |ui, class| not |ctx, class|)
- Drop puffin_egui git dep (incompatible with egui 0.34); remove flamegraph call
Bug fixes:
- button.rs: use painter.add(Noop) placeholder so hover bg is drawn behind
label text instead of on top of it
- toggle_switch.rs: always set PointingHand cursor, not only when hover_text is Some
- file_viewer/mod.rs: build InvalidFileType message from JSON_EXTENSIONS so
.jsonl and .geojson are included
- plugin_table_viewer.rs: replace Box::leak with TableCell::custom to stop
unbounded memory growth per frame
- plugin/manager.rs: log scan_directory errors and continue instead of aborting
remaining directories via ?
- wasm_file_viewer_loader.rs: use lock().unwrap_or_else(|e| e.into_inner()) to
recover from poisoned mutex without panicking
- table_view.rs: use ui.max_rect() for header shadow so it spans the full column
width instead of just the heading text width
Make table header shadow scroll-conditional and fix misc code review findings
Shadow below the table header is now only painted when the user has scrolled
past the first row (detected via egui temp-data from the previous frame); it
disappears when scrolled back to the top.
Also includes fixes surfaced during code review: card/icon-button/error-modal
style clean-ups, context-menu and recent-files minor corrections, search and
wasm-loader robustness improvements, theme and build tweaks, and CI workflow
update.
anitnilay20
force-pushed
the
feat/plugin-system#16
branch
from
00ca0e0 to
bbca8e8
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #65 +/- ##
==========================================
- Coverage 27.22% 21.66% -5.56%
==========================================
Files 59 79 +20
Lines 5154 7003 +1849
==========================================
+ Hits 1403 1517 +114
- Misses 3751 5486 +1735 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (12)
src/components/common/card.rs (1)
222-244:⚠️ Potential issue | 🟠 MajorAdd a file size guard before reading icon files.
The function reads and decodes the entire file without checking its size first. For plugin icons sourced from user-provided paths, a crafted or oversized image could block the UI thread or exhaust memory.
This was flagged in a previous review and remains unaddressed.
🛡️ Proposed fix to add size guard
fn decode_png_to_texture(ctx: &egui::Context, path: &Path) -> TextureHandle { + const MAX_ICON_BYTES: u64 = 2 * 1024 * 1024; // 2 MiB // Attempt to decode; on failure log a warning and fall back to a 1×1 transparent pixel. - let color_image = match std::fs::read(path) { + let color_image = match std::fs::metadata(path) { + Ok(meta) if meta.is_file() && meta.len() <= MAX_ICON_BYTES => { + match std::fs::read(path) { + Err(e) => { + eprintln!("warn: failed to read icon at {}: {e}", path.display()); + None + } + Ok(bytes) => match image::load_from_memory(&bytes) { + Err(e) => { + eprintln!("warn: failed to decode icon at {}: {e}", path.display()); + None + } + Ok(img) => { + let rgba = img.to_rgba8(); + let (w, h) = rgba.dimensions(); + Some(egui::ColorImage::from_rgba_unmultiplied( + [w as usize, h as usize], + &rgba, + )) + } + }, + } + } + Ok(meta) => { + eprintln!( + "warn: icon at {} is too large ({} bytes) or not a file", + path.display(), + meta.len() + ); + None + } Err(e) => { eprintln!("warn: failed to read icon at {}: {e}", path.display()); None } - Ok(bytes) => match image::load_from_memory(&bytes) { - Err(e) => { - eprintln!("warn: failed to decode icon at {}: {e}", path.display()); - None - } - Ok(img) => { - let rgba = img.to_rgba8(); - let (w, h) = rgba.dimensions(); - Some(egui::ColorImage::from_rgba_unmultiplied( - [w as usize, h as usize], - &rgba, - )) - } - }, } .unwrap_or_else(|| egui::ColorImage::from_rgba_unmultiplied([1, 1], &[0, 0, 0, 0]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/common/card.rs` around lines 222 - 244, In decode_png_to_texture, add a file-size guard before calling std::fs::read: obtain metadata via std::fs::metadata(path) and check metadata.len() against a sensible MAX_ICON_SIZE_BYTES (e.g., 5_000_000); if the file is missing metadata or exceeds the limit, log a warning (using the existing eprintln! message style) and immediately return the 1×1 transparent egui::ColorImage fallback instead of reading/decoding; keep the rest of the read + image::load_from_memory flow unchanged and reference the same TextureHandle return path.src/plugin/mod.rs (2)
81-84:⚠️ Potential issue | 🟠 MajorGive lifecycle hooks an instance receiver.
As declared, Lines 82-83 are associated functions, so the loader cannot call them on a loaded plugin instance or use them to clean up per-instance state on drop.
Suggested fix
pub trait PluginLifeCycle { - fn on_load(); - fn on_close(); + fn on_load(&mut self); + fn on_close(&mut self); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/mod.rs` around lines 81 - 84, The trait methods are declared as associated functions but must be instance methods so the loader can call them on plugin instances; change PluginLifeCycle so on_load and on_close take an instance receiver (use &mut self for both to allow per-instance initialization/cleanup), then update all impl PluginLifeCycle blocks and any call sites to invoke instance.on_load() and instance.on_close() instead of PluginType::on_load()/on_close().
65-66:⚠️ Potential issue | 🟠 MajorDefault
file_loaderto an empty list.Line 66 currently makes the
file-loaderentry mandatory for every manifest. Any plugin that only declaresdata-source,exporter,search-provider, ornew-ui-componentwill fail deserialization and be skipped during discovery.Suggested fix
#[serde(rename = "file-loader")] + #[serde(default)] pub file_loader: Vec<FileLoaderMeta>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/mod.rs` around lines 65 - 66, The manifest deserialization requires file_loader but should default to an empty list; add serde's default behavior to the file_loader field so missing "file-loader" entries deserialize as an empty Vec<FileLoaderMeta> (keep the field type Vec<FileLoaderMeta> and the serde(rename = "file-loader") attribute, but annotate the field with #[serde(default)] in src/plugin/mod.rs on the file_loader field).docs/PLUGIN_SYSTEM.md (3)
678-710:⚠️ Potential issue | 🟠 MajorUse the data-source guest trait in this example.
The methods in this block implement the
data-sourceinterface, but the example still saysimpl FileViewerGuest for MyPlugin. Anyone copying it will get a trait/signature mismatch against the generated bindings. Import and implementbindings::exports::thoth::plugin::data_source::Guestinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/PLUGIN_SYSTEM.md` around lines 678 - 710, The example implements the wrong trait (FileViewerGuest) which will cause a signature mismatch; change the impl to implement the data-source guest trait by importing and using bindings::exports::thoth::plugin::data_source::Guest and updating the impl line to "impl Guest for MyPlugin", keeping the existing method names (required_config, connect, schema, query, close) so their signatures align with the generated bindings and the example compiles against the data-source interface.
39-65:⚠️ Potential issue | 🟡 MinorAdd language tags to these fenced diagram blocks.
These bare fences still trip MD040. Using a
textlanguage tag preserves rendering and keeps markdownlint clean.Also applies to: 134-140, 335-349, 353-369, 777-785
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/PLUGIN_SYSTEM.md` around lines 39 - 65, The fenced ASCII diagram blocks (the ones starting with "┌─────────────────────────────────────────────────┐" and similar diagram blocks) are missing language tags and trigger MD040; update each of these triple-backtick fences (including the instances at the shown block and the other occurrences: lines around the blocks starting with "┌─────────────────────────────────────────────────┐", the block at the csv/postgres diagram, and the other listed diagram regions) by adding a language tag of text (```text) to the opening fence so markdownlint stops flagging them and rendering is preserved.
486-524:⚠️ Potential issue | 🟠 MajorDon’t teach a broken CSV loader in the walkthrough.
This sample still uses
split(',')and implementsraw_bytes()asSelf::get(idx).into_bytes(). That breaks quoted/TSV inputs and returns transformed JSON bytes instead of the raw record. Please switch the walkthrough to a proper CSV parser / raw-byte implementation, or clearly mark the block as simplified pseudo-code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/PLUGIN_SYSTEM.md` around lines 486 - 524, The CSV loader uses naive split(',') and raw_bytes returns JSON bytes—replace this with a proper CSV parser and preserve raw record bytes: in open, parse the file with csv::ReaderBuilder (allowing configurable delimiter and handling quotes/escapes) and store headers and rows as csv::StringRecord or keep the original raw bytes per row in STATE (e.g., a Vec<Vec<u8>> alongside headers) instead of splitting on commas; update get to build the JSON object by iterating headers and the csv::StringRecord values and mapping errors to PluginError as before; change raw_bytes to return the preserved raw bytes for the requested idx (not serde_json output), and ensure error mapping for missing state or out-of-range index mirrors get's behavior.src/components/search.rs (1)
298-304:⚠️ Potential issue | 🟠 MajorPreserve the styled preview instead of flattening it to
String.
button_text.text().to_string()on Line 303 drops theLayoutJobspans built inbuild_result_preview_text(), so result rows lose match highlighting and secondary styling. Please threadWidgetTextthroughButtonProps/Button::renderhere instead of converting it to plain text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/search.rs` around lines 298 - 304, The button currently flattens the styled preview by calling build_result_preview_text(...) and then converting button_text.text().to_string(), which loses LayoutJob spans and match highlighting; instead, propagate the WidgetText returned by build_result_preview_text through ButtonProps and Button::render (update ButtonProps.label to accept WidgetText and adjust Button::render to use the WidgetText directly when painting) so the LayoutJob/spans are preserved; modify any call sites (e.g., where Button::render is invoked) to pass the WidgetText instead of a String and update type signatures for ButtonProps, Button::render, and any related helpers to accept WidgetText.src/components/file_viewer/plugin_table_viewer.rs (1)
60-67:⚠️ Potential issue | 🟠 MajorKeep the unfiltered case implicit instead of cloning
0..total_len.When no root filter is active,
visible_indicesmaterializes every row index inrebuild_view(), andrender()clones that vector again each frame. On large plugin tables this adds O(n) memory and per-frame copy cost right on the virtualized path. Represent “all rows visible” implicitly and mapi -> iin the row builder.Also applies to: 81-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/file_viewer/plugin_table_viewer.rs` around lines 60 - 67, The code eagerly materializes all row indices into self.visible_indices (in rebuild_view()) causing O(n) allocations and per-frame clones in render(); change the representation to be implicit: make visible_indices an Option<Vec<usize>> (or add an all_visible boolean) so None means "all rows visible" instead of a full 0..total_len vector, update rebuild_view() to set Some(roots.clone()) only when a filter exists and None when not, update render() and the row builder to map virtual index i -> i when visible_indices is None and to index into the Vec when Some, and ensure you still clear render_cache when the visibility mode changes; apply the same change to the other similar block around the 81-85 region.src/components/settings_dialog/mod.rs (1)
307-325:⚠️ Potential issue | 🟠 MajorSurface uninstall failures and refresh the live plugin state.
Line 320 drops
remove_dir_allerrors, so the UI can say “uninstalled” even when the plugin directory is still on disk. The registry also stays populated until restart, which means the just-removed plugin can continue to appear usable in the current session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings_dialog/mod.rs` around lines 307 - 325, Handle and surface filesystem errors from the uninstall flow and refresh the in-memory plugin registry so the UI reflects removal immediately: in PluginsTabEvent::UninstallPlugin, change the blind remove_dir_all call to propagate or log errors (capture the Result from std::fs::remove_dir_all and call the app logger or return an error to the caller so failures are visible) and after removing the on-disk files and updating settings.plugins.disabled_plugin_ids.retain(|x| x != &id), update or reload the in-memory plugin state from PLUGIN_MANAGER (or call the manager's refresh/reload method) so the registry no longer lists the plugin in the current session; make sure to reference PLUGIN_MANAGER and the UninstallPlugin handler when adding the logging/refresh.plugins/csv-loader/src/lib.rs (1)
100-158:⚠️ Potential issue | 🟠 MajorIndex row byte ranges once in
open().
get()andraw_bytes()both reopen the file and scan tonth(idx)on every call. That makes scrolling/copy O(n) per row, andraw_bytes()still reconstructs the record from parsed fields, so quoted values and escaping are no longer truly raw. Recording each record’s byte range duringopen()lets both methods seek directly and preserves the original bytes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/csv-loader/src/lib.rs` around lines 100 - 158, The code reopens and rescans the file in get() and raw_bytes(), causing O(n) per-row access and losing original raw bytes; fix by indexing record byte ranges once in open(): add a field to STATE (e.g., record_ranges: Vec<(u64,u64)>) and, when opening the file in open(), iterate over byte_records() while tracking reader.position().byte() (or otherwise capture start/end offsets) to populate the ranges for every record; then change get() and raw_bytes() to use the stored range for idx—open the file (std::fs::File), seek to the start offset, read the exact byte length, and for raw_bytes() return those bytes verbatim while for get() parse the single-record bytes with a CSV reader (has_headers = false) and map fields to state.headers as before so quoting/escaping are preserved.src/plugin/manager.rs (2)
224-226:⚠️ Potential issue | 🟠 MajorAll plugins are still unconditionally marked as
bundled = true.User-installed plugins from
user_plugin_dir()will also havebundled = true, preventing them from being uninstalled viauninstall_plugin()(which rejects bundled plugins at line 69-73). This defeats the purpose of the uninstall functionality.Consider threading a
is_bundledflag throughplugin_directories()andscan_directory()so only plugins from the bundled directory are marked as such.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/manager.rs` around lines 224 - 226, The code currently sets plugin.bundled = true unconditionally; change the flow so only plugins discovered in the bundled plugin directory get bundled=true by threading an is_bundled boolean through plugin_directories() and scan_directory(): have plugin_directories() annotate or return which directory entries are bundled, pass that is_bundled flag into scan_directory(), and set plugin.bundled = is_bundled inside the scan/load path (where plugin.bundled is currently set) so user_plugin_dir() results remain bundled=false and uninstall_plugin() will work as intended.
280-293:⚠️ Potential issue | 🟠 MajorCapability exports are not validated against manifest declarations.
The validation only checks for
thoth:plugin/plugin-metabut doesn't verify that plugins actually export the interfaces they claim inplugin.toml(e.g.,file-loader,file-viewer). A plugin could declarefile-loadercapability without exporting that interface, causing runtime failures on firstopen_file()call.Consider iterating over the plugin's declared capabilities and verifying each has a matching export.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin/manager.rs` around lines 280 - 293, The current check only ensures the presence of "thoth:plugin/plugin-meta" (via component.component_type().exports(&self.engine) and has_plugin_meta) but does not verify the plugin actually exports each capability listed in its plugin.toml, which can lead to runtime failures in functions like open_file(); update the load/validation logic to read the declared capabilities from the plugin manifest and iterate them, for each declared capability (e.g., "file-loader", "file-viewer") check that component.component_type().exports(&self.engine) contains a matching export name (with the expected prefix/suffix), and if any are missing return Err(ThothError::PluginLoadError { path: wasm_path, reason: /* clear message listing missing capabilities */ }) so missing interfaces are caught at load time rather than at open_file().
🧹 Nitpick comments (3)
src/components/common/toggle_switch.rs (1)
64-67: Background color jumps while knob animates smoothly.The knob position interpolates via
animation_progress, but the background color is determined solely by theenabledboolean (line 66). This causes the background to snap instantly while the knob slides—a subtle visual mismatch.Consider interpolating the background color as well for a polished transition.
✨ Optional fix to animate background color
- let bg = if enabled { on_color } else { off_color }; + let bg = interpolate_color(off_color, on_color, animation_progress); ui.painter().rect_filled(rect, CornerRadius::same(10), bg);Add a helper (or use an existing utility):
fn interpolate_color(a: Color32, b: Color32, t: f32) -> Color32 { Color32::from_rgba_unmultiplied( (a.r() as f32 + (b.r() as f32 - a.r() as f32) * t) as u8, (a.g() as f32 + (b.g() as f32 - a.g() as f32) * t) as u8, (a.b() as f32 + (b.b() as f32 - a.b() as f32) * t) as u8, (a.a() as f32 + (b.a() as f32 - a.a() as f32) * t) as u8, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/common/toggle_switch.rs` around lines 64 - 67, The background color is snapping because rect_filled uses the boolean `enabled` instead of the animated `animation_progress`; update the painting to interpolate between `off_color` and `on_color` using `animation_progress` (e.g., add an `interpolate_color(a,b,t)` helper and compute `bg = interpolate_color(off_color, on_color, animation_progress)`), then pass that `bg` into `ui.painter().rect_filled(rect, CornerRadius::same(10), bg)` so the background transitions smoothly alongside the knob animation.src/components/common/table_view.rs (1)
39-42: Consider documenting thebuild_rowcontract more explicitly.The comment states "return exactly
headers.len()cells" but there's no runtime validation. If a plugin returns fewer cells, this could cause visual inconsistency. Consider adding a debug assertion or gracefully handling mismatched cell counts.🛡️ Optional: Add debug assertion for cell count
let cells = build_row(row.index()); + debug_assert_eq!( + cells.len(), + props.headers.len(), + "build_row must return exactly headers.len() cells" + ); let mut row_clicked = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/common/table_view.rs` around lines 39 - 42, The build_row closure currently has a contract ("return exactly headers.len() cells") but no runtime checks; when invoking build_row (the TableView struct's build_row field, where rows are constructed for display), add a runtime validation: call the closure to get cells, then debug_assert_eq!(cells.len(), self.headers.len()) to catch plugin mistakes in debug builds, and also handle mismatches in release by truncating extra cells or padding missing ones with empty/default TableCell instances so rendering never panics or misaligns; update the row-rendering code path (where build_row is invoked) to perform these checks and the truncate/pad logic.src/plugin/wasm_file_viewer_loader.rs (1)
163-174: Consider propagating errors frompreferred_display()andcolumn_headers()rather than silently falling back.
preferred_display()silently returnsDisplayMode::Tableon error (line 172), andcolumn_headers()returnsNoneon error (line 182-183). While fallbacks are reasonable, logging these failures would aid debugging plugin issues.🔧 Optional: Add debug logging for plugin call failures
pub fn preferred_display(&mut self) -> DisplayMode { let WasmViewerInner { store, bindings } = self.inner.get_mut().unwrap(); let _ = store.set_fuel(u64::MAX / 2); match bindings .thoth_plugin_file_viewer() .call_preferred_display(store) { Ok(exports::thoth::plugin::file_viewer::DisplayMode::Table) => DisplayMode::Table, Ok(exports::thoth::plugin::file_viewer::DisplayMode::Custom) => DisplayMode::Custom, - Err(_) => DisplayMode::Table, + Err(e) => { + eprintln!("Plugin preferred_display failed, defaulting to Table: {e}"); + DisplayMode::Table + } } }Also applies to: 176-184, 186-201
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/search.rs`:
- Line 25: Update the comment string in benches/search.rs that currently reads
"Create a file with some matches at different densities" to remove the extra
space so it reads "Create a file with some matches at different densities";
locate the comment near the benchmark setup (search.rs comment above the
file-generation/benchmark block) and correct the typo.
In `@build.rs`:
- Around line 73-95: The current build copies can leave stale assets in dst_dir
when a copy fails and plugin.toml copy errors are ignored; update the copy logic
so that after creating dst_dir you attempt each fs::copy (wasm, plugin.toml,
icon.png) and treat any Err when copying plugin.toml or wasm as fatal for that
plugin: on error call fs::remove_dir_all(&dst_dir) to clean up and then continue
to the next plugin, and do not silently ignore toml copy failures—check the
Result from fs::copy(toml_src, format!("{dst_dir}/plugin.toml")) and handle Err
by removing dst_dir and skipping the plugin; apply the same cleanup/continue
behavior for icon.png copy errors if you want consistent semantics.
- Around line 5-6: The build script currently only watches the plugins directory
via the println!("cargo:rerun-if-changed=plugins/"); statement, so changes to
the WIT contract (wit/thoth-plugin.wit) won't trigger rebuilds; add a
rerun-if-changed line for the WIT contract (e.g., emit
cargo:rerun-if-changed=wit/thoth-plugin.wit or cargo:rerun-if-changed=wit/)
alongside the existing println to ensure changes to the WIT file trigger
rebuilds and regenerate bindings used by the bundled plugin binaries.
In `@src/components/file_viewer/plugin_table_viewer.rs`:
- Around line 53-57: When loader.column_headers() returns None, currently
self.headers remains empty causing zero-column rendering; update the block in
plugin_table_viewer.rs (the code that checks self.headers.is_empty() and calls
loader.preferred_display()/loader.column_headers()) to fall back to deriving
headers from the first data record’s object keys: if column_headers() is Some
use those, otherwise inspect the first record returned by the loader (the first
Row/Record the plugin returns), extract its object/map keys in deterministic
order and assign them to self.headers and set display_mode appropriately so
table mode renders with those derived columns.
In `@src/components/toolbar.rs`:
- Around line 203-206: The code currently falls back to *props.file_type when
infer_file_type(&path) returns None, which can reuse the previous FileKind for a
new path (functions: pick_file, infer_file_type, and symbols
ToolbarEvent::FileOpen, previous_file_type, props.file_type). Fix by resolving
the file kind from the selected path or plugin registry instead of reusing
props.file_type: call a registry-aware resolver (or ask the plugin system) to
infer kind from path, or emit only the path and let the central loader infer the
kind; do not assign previous_file_type from a fallback that came from
props.file_type. Apply the same change to the other instance noted around the
299-304 region.
In `@src/plugin/manager.rs`:
- Around line 205-208: The eprintln! calls in src/plugin/manager.rs that
currently print messages prefixed with "cargo:warning=" are incorrect for
runtime logging; remove the "cargo:warning=" prefix (or replace the eprintln!
with an appropriate runtime logger like log::warn!) in the messages that mention
"Skipping plugin at {}: failed to read plugin.toml: {e}" and the similar
messages that reference path.display(), as well as the other two occurrences
flagged (the invalid plugin.toml and failed open messages). Update the eprintln!
invocations (or switch to log::warn!) so they emit plain runtime warnings
without the cargo build-script prefix and apply the same change to the other
matching eprintln! sites in manager.rs.
In `@src/theme.rs`:
- Around line 417-448: The get_contrast_text_color function currently uses a
fixed luminance > 0.5 cutoff; instead compute WCAG contrast ratios against black
and white and choose the text color with the higher contrast. After you compute
luminance (as done in get_contrast_text_color), calculate contrast_with_white =
(1.0 + 0.05) / (luminance + 0.05) and contrast_with_black = (luminance + 0.05) /
(0.0 + 0.05), then return Color32::WHITE if contrast_with_white >=
contrast_with_black, otherwise return Color32::BLACK so the function picks the
color that yields the stronger readable contrast.
---
Duplicate comments:
In `@docs/PLUGIN_SYSTEM.md`:
- Around line 678-710: The example implements the wrong trait (FileViewerGuest)
which will cause a signature mismatch; change the impl to implement the
data-source guest trait by importing and using
bindings::exports::thoth::plugin::data_source::Guest and updating the impl line
to "impl Guest for MyPlugin", keeping the existing method names
(required_config, connect, schema, query, close) so their signatures align with
the generated bindings and the example compiles against the data-source
interface.
- Around line 39-65: The fenced ASCII diagram blocks (the ones starting with
"┌─────────────────────────────────────────────────┐" and similar diagram
blocks) are missing language tags and trigger MD040; update each of these
triple-backtick fences (including the instances at the shown block and the other
occurrences: lines around the blocks starting with
"┌─────────────────────────────────────────────────┐", the block at the
csv/postgres diagram, and the other listed diagram regions) by adding a language
tag of text (```text) to the opening fence so markdownlint stops flagging them
and rendering is preserved.
- Around line 486-524: The CSV loader uses naive split(',') and raw_bytes
returns JSON bytes—replace this with a proper CSV parser and preserve raw record
bytes: in open, parse the file with csv::ReaderBuilder (allowing configurable
delimiter and handling quotes/escapes) and store headers and rows as
csv::StringRecord or keep the original raw bytes per row in STATE (e.g., a
Vec<Vec<u8>> alongside headers) instead of splitting on commas; update get to
build the JSON object by iterating headers and the csv::StringRecord values and
mapping errors to PluginError as before; change raw_bytes to return the
preserved raw bytes for the requested idx (not serde_json output), and ensure
error mapping for missing state or out-of-range index mirrors get's behavior.
In `@plugins/csv-loader/src/lib.rs`:
- Around line 100-158: The code reopens and rescans the file in get() and
raw_bytes(), causing O(n) per-row access and losing original raw bytes; fix by
indexing record byte ranges once in open(): add a field to STATE (e.g.,
record_ranges: Vec<(u64,u64)>) and, when opening the file in open(), iterate
over byte_records() while tracking reader.position().byte() (or otherwise
capture start/end offsets) to populate the ranges for every record; then change
get() and raw_bytes() to use the stored range for idx—open the file
(std::fs::File), seek to the start offset, read the exact byte length, and for
raw_bytes() return those bytes verbatim while for get() parse the single-record
bytes with a CSV reader (has_headers = false) and map fields to state.headers as
before so quoting/escaping are preserved.
In `@src/components/common/card.rs`:
- Around line 222-244: In decode_png_to_texture, add a file-size guard before
calling std::fs::read: obtain metadata via std::fs::metadata(path) and check
metadata.len() against a sensible MAX_ICON_SIZE_BYTES (e.g., 5_000_000); if the
file is missing metadata or exceeds the limit, log a warning (using the existing
eprintln! message style) and immediately return the 1×1 transparent
egui::ColorImage fallback instead of reading/decoding; keep the rest of the read
+ image::load_from_memory flow unchanged and reference the same TextureHandle
return path.
In `@src/components/file_viewer/plugin_table_viewer.rs`:
- Around line 60-67: The code eagerly materializes all row indices into
self.visible_indices (in rebuild_view()) causing O(n) allocations and per-frame
clones in render(); change the representation to be implicit: make
visible_indices an Option<Vec<usize>> (or add an all_visible boolean) so None
means "all rows visible" instead of a full 0..total_len vector, update
rebuild_view() to set Some(roots.clone()) only when a filter exists and None
when not, update render() and the row builder to map virtual index i -> i when
visible_indices is None and to index into the Vec when Some, and ensure you
still clear render_cache when the visibility mode changes; apply the same change
to the other similar block around the 81-85 region.
In `@src/components/search.rs`:
- Around line 298-304: The button currently flattens the styled preview by
calling build_result_preview_text(...) and then converting
button_text.text().to_string(), which loses LayoutJob spans and match
highlighting; instead, propagate the WidgetText returned by
build_result_preview_text through ButtonProps and Button::render (update
ButtonProps.label to accept WidgetText and adjust Button::render to use the
WidgetText directly when painting) so the LayoutJob/spans are preserved; modify
any call sites (e.g., where Button::render is invoked) to pass the WidgetText
instead of a String and update type signatures for ButtonProps, Button::render,
and any related helpers to accept WidgetText.
In `@src/components/settings_dialog/mod.rs`:
- Around line 307-325: Handle and surface filesystem errors from the uninstall
flow and refresh the in-memory plugin registry so the UI reflects removal
immediately: in PluginsTabEvent::UninstallPlugin, change the blind
remove_dir_all call to propagate or log errors (capture the Result from
std::fs::remove_dir_all and call the app logger or return an error to the caller
so failures are visible) and after removing the on-disk files and updating
settings.plugins.disabled_plugin_ids.retain(|x| x != &id), update or reload the
in-memory plugin state from PLUGIN_MANAGER (or call the manager's refresh/reload
method) so the registry no longer lists the plugin in the current session; make
sure to reference PLUGIN_MANAGER and the UninstallPlugin handler when adding the
logging/refresh.
In `@src/plugin/manager.rs`:
- Around line 224-226: The code currently sets plugin.bundled = true
unconditionally; change the flow so only plugins discovered in the bundled
plugin directory get bundled=true by threading an is_bundled boolean through
plugin_directories() and scan_directory(): have plugin_directories() annotate or
return which directory entries are bundled, pass that is_bundled flag into
scan_directory(), and set plugin.bundled = is_bundled inside the scan/load path
(where plugin.bundled is currently set) so user_plugin_dir() results remain
bundled=false and uninstall_plugin() will work as intended.
- Around line 280-293: The current check only ensures the presence of
"thoth:plugin/plugin-meta" (via component.component_type().exports(&self.engine)
and has_plugin_meta) but does not verify the plugin actually exports each
capability listed in its plugin.toml, which can lead to runtime failures in
functions like open_file(); update the load/validation logic to read the
declared capabilities from the plugin manifest and iterate them, for each
declared capability (e.g., "file-loader", "file-viewer") check that
component.component_type().exports(&self.engine) contains a matching export name
(with the expected prefix/suffix), and if any are missing return
Err(ThothError::PluginLoadError { path: wasm_path, reason: /* clear message
listing missing capabilities */ }) so missing interfaces are caught at load time
rather than at open_file().
In `@src/plugin/mod.rs`:
- Around line 81-84: The trait methods are declared as associated functions but
must be instance methods so the loader can call them on plugin instances; change
PluginLifeCycle so on_load and on_close take an instance receiver (use &mut self
for both to allow per-instance initialization/cleanup), then update all impl
PluginLifeCycle blocks and any call sites to invoke instance.on_load() and
instance.on_close() instead of PluginType::on_load()/on_close().
- Around line 65-66: The manifest deserialization requires file_loader but
should default to an empty list; add serde's default behavior to the file_loader
field so missing "file-loader" entries deserialize as an empty
Vec<FileLoaderMeta> (keep the field type Vec<FileLoaderMeta> and the
serde(rename = "file-loader") attribute, but annotate the field with
#[serde(default)] in src/plugin/mod.rs on the file_loader field).
---
Nitpick comments:
In `@src/components/common/table_view.rs`:
- Around line 39-42: The build_row closure currently has a contract ("return
exactly headers.len() cells") but no runtime checks; when invoking build_row
(the TableView struct's build_row field, where rows are constructed for
display), add a runtime validation: call the closure to get cells, then
debug_assert_eq!(cells.len(), self.headers.len()) to catch plugin mistakes in
debug builds, and also handle mismatches in release by truncating extra cells or
padding missing ones with empty/default TableCell instances so rendering never
panics or misaligns; update the row-rendering code path (where build_row is
invoked) to perform these checks and the truncate/pad logic.
In `@src/components/common/toggle_switch.rs`:
- Around line 64-67: The background color is snapping because rect_filled uses
the boolean `enabled` instead of the animated `animation_progress`; update the
painting to interpolate between `off_color` and `on_color` using
`animation_progress` (e.g., add an `interpolate_color(a,b,t)` helper and compute
`bg = interpolate_color(off_color, on_color, animation_progress)`), then pass
that `bg` into `ui.painter().rect_filled(rect, CornerRadius::same(10), bg)` so
the background transitions smoothly alongside the knob animation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2435e5a4-579a-4adf-a32f-1f1209ff75a2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
.github/workflows/ci.ymlCargo.tomlbenches/search.rsbuild.rsdocs/PLUGIN_SYSTEM.mdplugins/csv-loader/Cargo.tomlplugins/csv-loader/src/bindings.rsplugins/csv-loader/src/lib.rssrc/app/file_picker.rssrc/app/mod.rssrc/app/thoth_app.rssrc/components/common/button.rssrc/components/common/card.rssrc/components/common/icon_button.rssrc/components/common/table_view.rssrc/components/common/toggle_switch.rssrc/components/error_modal.rssrc/components/file_viewer/context_menu.rssrc/components/file_viewer/mod.rssrc/components/file_viewer/plugin_table_viewer.rssrc/components/recent_files.rssrc/components/search.rssrc/components/settings_dialog/advanced.rssrc/components/settings_dialog/appearance.rssrc/components/settings_dialog/mod.rssrc/components/settings_dialog/plugins.rssrc/components/settings_dialog/tests.rssrc/components/settings_dialog/updates.rssrc/components/settings_panel.rssrc/components/status_bar.rssrc/components/toolbar.rssrc/error/handler.rssrc/lib.rssrc/plugin/manager.rssrc/plugin/mod.rssrc/plugin/plugin_registry.rssrc/plugin/wasm_file_viewer_loader.rssrc/plugin/wasm_loader.rssrc/theme.rstests/common/mocks.rstests/common/mod.rstests/component_tests.rstests/file_loading_tests.rswit/thoth-plugin.wit
✅ Files skipped from review due to trivial changes (9)
- .github/workflows/ci.yml
- src/app/mod.rs
- tests/file_loading_tests.rs
- src/components/settings_dialog/advanced.rs
- plugins/csv-loader/Cargo.toml
- tests/component_tests.rs
- src/components/status_bar.rs
- plugins/csv-loader/src/bindings.rs
- wit/thoth-plugin.wit
🚧 Files skipped from review as they are similar to previous changes (10)
- src/components/settings_dialog/updates.rs
- src/components/recent_files.rs
- src/components/settings_dialog/tests.rs
- src/error/handler.rs
- src/components/file_viewer/context_menu.rs
- src/lib.rs
- src/components/settings_dialog/plugins.rs
- src/plugin/plugin_registry.rs
- src/components/common/icon_button.rs
- src/plugin/wasm_loader.rs
- benches: fix double-space typo in comment
- build: add wit/ rerun-if-changed; make wasm/toml copy failures fatal with
dst_dir cleanup so stale partial assets are never left behind
- manager: remove erroneous cargo:warning= prefix from all runtime eprintln!
calls; thread is_bundled through plugin_directories/scan_directory so only
bundled-dir plugins are immutable (user plugins now correctly bundled=false)
- theme: switch get_contrast_text_color to WCAG 2.1 contrast-ratio comparison
instead of fixed luminance > 0.5 cutoff
- toggle_switch: interpolate background color through animation_progress so the
track transitions smoothly alongside the knob
- table_view: add Default for TableCell; add debug_assert + release truncate/pad
in build_row to catch and survive cell-count mismatches
- plugin/mod: add #[serde(default)] to file_loader so missing sections
deserialise as empty Vec; make PluginLifeCycle methods instance methods
- settings_dialog: log remove_dir_all errors on plugin uninstall instead of
silently ignoring them
- card: add MAX_ICON_SIZE_BYTES (5 MB) guard in decode_png_to_texture before
reading icon files into memory
- toolbar: infer_file_type now consults PLUGIN_MANAGER for unknown extensions
instead of falling back to the previous file's type; skip FileOpen event
entirely if the extension is unrecognised
- docs: tag ASCII diagram fences with "text" (MD040); fix data-source example
to implement data_source::Guest instead of FileViewerGuest; update CSV loader
example to use csv::ReaderBuilder instead of naive split(',')
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
anitnilay20
force-pushed
the
feat/plugin-system#16
branch
from
241e440 to
098b621
Compare
…t column_headers - table_view: replace manual Default impl with #[derive(Default)] on TableCell to fix clippy::derivable_impls error under -D warnings - plugin_table_viewer: when column_headers() returns None and the file has records, derive headers from the sorted object keys of the first record so the table always has columns to render
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #16
Summary by CodeRabbit
New Features
Improvements
Bug Fixes