Guus/egui

[guusw]

No description provided.

[sinkingsugar]

Code Review: PR #1179 (Guus/egui)

Overview

This PR contains 1455 additions and 765 deletions across multiple subsystems:

• Complete rewrite of the egui docking system (from shard-based to table-based configuration)
• Major refactoring of error handling and composition error reporting
• Updates to documentation and sample code
• Build configuration improvements

---

Error Handling Refactoring

✅ Strengths

• Better context preservation: ComposeError now includes shard/wire references for precise error location
• Cleaner error messages: Centralized logFormatErrorStack provides consistent formatting
• Reduced duplication: Eliminated redundant SHLOG_ERROR calls scattered throughout codebase
• Improved stack traces: Error stack now stores structured ComposeError objects instead of strings

⚠️ Concerns

shards/core/runtime.cpp:1123-1127: Uninitialized union field

union {
  Shard *shard;
  SHWire *wire;
};
ContextType type;

The union members aren't initialized in the CTX_Unknown constructor. This could lead to undefined behavior if code accidentally accesses the union fields.

shards/core/runtime.cpp:1343: Commented throw statement

throw;
// if (ownedContext) {
//   logFormatErrorStack(ownedContext);
//   // Format the error stack
//   throw std::runtime_error("Failed to compose wire");
// } else {
//   throw;
// }

Dead code should be removed rather than commented out.

shards/core/runtime.cpp:1476: Duplicate error logging path

if (!data.privateContext) {
  return prettyComposeWithContext(wire_, data);
} else {
  // existing path
}

This creates two different composition paths which could diverge in behavior over time.

---

Egui/Docking System Rewrite

✅ Strengths

• Modern architecture: Shift from child shards to table-based configuration is more flexible
• Serialization support: Added proper serde serialization for dock state persistence
• Better state management: DockState is now a proper ref-counted object
• Enhanced features: Tab IDs, closeable tabs, better tab management
• Cleaner API: Table-based configuration is more declarative

⚠️ Concerns

shards/egui/src/containers/docking.rs:86-89: UnsafeCell usage

struct DockState {
  pub tabs: Arc<UnsafeCell<egui_dock::DockState<TabRef>>>,
}

Using Arc<UnsafeCell<T>> bypasses Rust's borrow checker. This could lead to data races if accessed incorrectly from multiple threads. Consider Arc<Mutex<T>> or Arc<RwLock<T>> for thread-safe access.

shards/egui/src/containers/docking.rs:254-260: Error handling incomplete

let tab_data = TabData::from_table(&table)?;
tab_data_vec.push(tab_data);

No validation that tabs sequence contains valid table structures. Missing checks for required fields could cause panics.

shards/egui/src/containers/docking.rs:584: Unsafe block without clear safety documentation

let tabs = unsafe { &mut *self.tabs.get() };

Multiple unsafe dereferences throughout the code lack safety comments explaining why they're sound.

Missing migration path: No guidance for users upgrading from old UI.Tab shard to new table-based syntax. Breaking change needs documentation.

---

Build & Documentation Changes

✅ Good Updates

• Added local/ and .claude/ to .gitignore
• Simplified CLAUDE.md build instructions
• Updated samples to match new API
• Fixed UI property name (RemainingSpace → RemainingSize)

⚠️ Minor Issues

• CLAUDE.md:8: Removed bootstrap/update.sh instructions but git submodules still require initialization
• docs/samples: Only 2 samples updated; may be more files using old UI.Tab syntax

---

Overall Assessment

Code Quality: 7/10

• Well-structured refactoring with clear improvements
• Some unsafe code usage needs better documentation
• Commented-out code should be cleaned up

Testing Coverage: ⚠️ Unknown

• No tests visible in diff
• Major API breaking changes should have migration tests
• Error handling refactor needs validation tests

Security: 8/10

• No obvious vulnerabilities
• UnsafeCell usage is a concern for thread safety
• Error messages don't appear to leak sensitive information

Performance: Neutral

• Changes appear performance-neutral
• Centralized error formatting may have minor overhead but improves debuggability

---

Recommendations

High Priority

1. Initialize union members in ComposeError::CTX_Unknown constructor
2. Document safety invariants for all unsafe blocks in docking.rs
3. Remove commented code at runtime.cpp:1343
4. Add migration guide for UI.Tab → table-based tabs transition

Medium Priority

5. Consider thread-safe alternatives to Arc<UnsafeCell<>>
6. Add validation for required table fields in tab configuration
7. Test coverage for error stack formatting and composition errors
8. Audit remaining samples for old UI.Tab usage

Low Priority

9. Clean up duplicate error handling paths
10. Add debug assertions to verify error stack consistency

---

Security Considerations

✅ No credential exposure risks
✅ No injection vulnerabilities observed
⚠️ Thread safety concerns with UnsafeCell (mitigated if single-threaded UI)

---

Summary

This PR makes significant improvements to error handling infrastructure and completely modernizes the UI docking system. The changes are generally well-architected but need attention to:

1. Memory safety in error handling (uninitialized union)
2. Thread safety in dock state management (UnsafeCell usage)
3. Documentation for breaking changes
4. Code cleanup (remove commented sections)

Recommendation: Request changes for the high-priority issues before merging. The core functionality appears sound, but the unsafe code needs better safety documentation and the union initialization issue should be fixed to prevent undefined behavior.

[sinkingsugar]

added some small notes

[codecov]

Codecov Report

❌ Patch coverage is 71.91781% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.39%. Comparing base (e4cf069) to head (c05978b).

Files with missing lines	Patch %	Lines
shards/core/runtime.cpp	76.41%	25 Missing ⚠️
shards/core/ops_internal.cpp	16.66%	10 Missing ⚠️
shards/modules/core/logging.cpp	20.00%	4 Missing ⚠️
include/shards/shardwrapper.hpp	66.66%	1 Missing ⚠️
shards/modules/core/wires.hpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            devel    #1179      +/-   ##
==========================================
+ Coverage   77.37%   77.39%   +0.02%     
==========================================
  Files         387      387              
  Lines       61432    61477      +45     
==========================================
+ Hits        47532    47582      +50     
+ Misses      13900    13895       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.