feat(ui): add FeatureGate system for conditional UI visibility#805
Merged
Conversation
Introduce predicate-based FeatureGate enum that centralizes feature availability decisions. Replaces scattered supports_shielded() and is_developer_mode() checks in UI code with typed gate queries. PoC covers: Shielded, DashPay, DeveloperMode, SpvBackend gates. Applied to sidebar filtering, wallet shielded tab, and DashPay payment subscreen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new feature-gating module ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "egui::Ui"
participant Gate as "FeatureGate"
participant Ctx as "AppContext"
UI->>Gate: feature_gated(ctx, gate, closure)
Gate->>Ctx: is_available(gate)
Ctx-->>Gate: bool (available / unavailable)
alt available
Gate-->>UI: execute closure (render UI)
else unavailable
Gate-->>UI: no-op (skip rendering)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
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 |
Add extension trait on egui::Ui for feature-gated multi-widget blocks. Convert representative callsites to demonstrate all three usage patterns: add_visible for single widgets, feature_gated for sections, direct predicate for conditional data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Review GateCommit:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/ui/wallets/wallets_screen/mod.rs (1)
1092-1092: Finish standardizing developer-mode checks onFeatureGate.Good first step here. Consider replacing remaining direct
is_developer_mode()uses in this file withFeatureGate::DeveloperMode.is_available(...)so behavior stays unified if gate logic changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/wallets_screen/mod.rs` at line 1092, Replace remaining direct calls to is_developer_mode() in this module with the unified gate check FeatureGate::DeveloperMode.is_available(&self.app_context); locate occurrences of is_developer_mode() (method/func uses in wallet screen UI handlers, e.g., anywhere comparing developer mode) and change them to call FeatureGate::DeveloperMode.is_available with the same self.app_context reference so behavior matches the new gate logic.src/ui/wallets/shield_screen.rs (1)
829-843: Use the same gate predicate for both visibility and execution behavior.This block now uses
FeatureGate::DeveloperMode, but repeat-count behavior still branches on direct developer mode elsewhere (Line 898). Consider using the same gate check for both to avoid future logic drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/shield_screen.rs` around lines 829 - 843, The UI shows the repeat-count controls behind ui.feature_gated(FeatureGate::DeveloperMode, &self.app_context, ...) but the runtime branching that acts on repeat-count still uses a direct developer-mode check elsewhere; make both use the same predicate by either wrapping the execution logic in the same ui.feature_gated call or by querying the same feature gate on the app context (e.g. replace the direct dev-mode boolean check with self.app_context.is_feature_enabled(FeatureGate::DeveloperMode) or equivalent) so FeatureGate::DeveloperMode governs both visibility and behavior for self.repeat_count_str / self.parallel.
🤖 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/model/feature_gate.rs`:
- Around line 71-76: The method signature for feature_gated has the AppContext
parameter in the wrong position; reorder the parameters so &AppContext comes
immediately after &mut self (i.e., change fn feature_gated(&mut self, gate:
FeatureGate, ctx: &AppContext, ...) to fn feature_gated(&mut self, ctx:
&AppContext, gate: FeatureGate, ...)), and apply the same reordering for the
other occurrences noted (around lines 80–85) to match the project convention;
update any internal uses/call sites of feature_gated and its trait impls to pass
ctx as the first argument after self.
---
Nitpick comments:
In `@src/ui/wallets/shield_screen.rs`:
- Around line 829-843: The UI shows the repeat-count controls behind
ui.feature_gated(FeatureGate::DeveloperMode, &self.app_context, ...) but the
runtime branching that acts on repeat-count still uses a direct developer-mode
check elsewhere; make both use the same predicate by either wrapping the
execution logic in the same ui.feature_gated call or by querying the same
feature gate on the app context (e.g. replace the direct dev-mode boolean check
with self.app_context.is_feature_enabled(FeatureGate::DeveloperMode) or
equivalent) so FeatureGate::DeveloperMode governs both visibility and behavior
for self.repeat_count_str / self.parallel.
In `@src/ui/wallets/wallets_screen/mod.rs`:
- Line 1092: Replace remaining direct calls to is_developer_mode() in this
module with the unified gate check
FeatureGate::DeveloperMode.is_available(&self.app_context); locate occurrences
of is_developer_mode() (method/func uses in wallet screen UI handlers, e.g.,
anywhere comparing developer mode) and change them to call
FeatureGate::DeveloperMode.is_available with the same self.app_context reference
so behavior matches the new gate logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae9f1091-93f1-44e1-8ef3-8b9886192506
📒 Files selected for processing (7)
src/model/feature_gate.rssrc/model/mod.rssrc/ui/components/dashpay_subscreen_chooser_panel.rssrc/ui/components/left_panel.rssrc/ui/dashpay/contact_profile_viewer.rssrc/ui/wallets/shield_screen.rssrc/ui/wallets/wallets_screen/mod.rs
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-documented FeatureGate abstraction with sound core design (Copy enum, exhaustive match, zero allocation). The main gap is that gating only hides the chooser button — it doesn't enforce the gate at the screen boundary, so Payment History remains reachable via persisted navigation state or direct construction. The FeatureGateUiExt trait introduces a UI framework dependency into the model layer.
Reviewed commit: bcda7a3
🟡 5 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/components/dashpay_subscreen_chooser_panel.rs`:
- [SUGGESTION] lines 21-23: Gated Payment History is still reachable via persisted navigation state
The gate only removes `DashPaySubscreen::Payments` from the chooser panel list. However, `DashPayScreen::render_subscreen()` renders the Payments subscreen unconditionally when it's the active subscreen (line 47), and `app.rs:346` restores `settings.root_screen_type` directly on startup — so a user who previously saved `RootScreenDashPayPayments` will land on Payment History after dev mode is turned off.
The wallet tabs already solve this exact problem by clamping the selected tab back to the first visible entry. DashPayScreen needs the same normalization — either in `render_subscreen()` or when the gate's underlying condition (dev mode) changes.
In `src/model/feature_gate.rs`:
- [SUGGESTION] lines 67-90: FeatureGateUiExt couples the model layer to egui
The `model/` module is for data types and business logic — `FeatureGateUiExt` imports `egui::Ui`, creating a UI framework dependency in the data-model layer. The `FeatureGate` enum and `is_available()` belong in `model/`; the extension trait should live in `src/ui/` (e.g., `src/ui/components/feature_gate_ext.rs`).
- [SUGGESTION] lines 57-65: all_available() and any_available() are unused dead code
Neither `all_available()` nor `any_available()` is called anywhere in the codebase. Public dead code in a library module is a maintenance burden — readers assume it's part of the contract and hesitate to remove it. The composability story is speculative; `gates.iter().all(|g| g.is_available(ctx))` is trivial to add when a callsite actually needs it.
- [SUGGESTION] lines 48-53: SpvBackend gate is semantically wrong — checks dev mode instead of backend mode
`FeatureGate::SpvBackend` returns `ctx.is_developer_mode()`, but the app already has `CoreBackendMode::{Rpc, Spv}`. The gate name implies it checks whether the SPV backend is active, but it actually just checks developer mode. This guarantees drift: some callsites now use `FeatureGate::SpvBackend`, while others like `contacts_list.rs:963` and `contact_details.rs:310` still use raw `is_developer_mode()` checks. When SPV availability diverges from dev mode, these will expose different behavior depending on which path the user takes.
- [SUGGESTION] lines 1-90: No unit tests for FeatureGate predicates
The enum has non-trivial predicate logic (protocol version threshold for Shielded, atomic bool reads for DeveloperMode). These are pure-function predicates — cheap to test and high-value to verify since the whole point of centralizing gates is making them the single source of truth.
- Reorder feature_gated() params: &AppContext first after self (convention) - Remove unused all_available() and any_available() dead code - Fix SpvBackend gate: check CoreBackendMode::Spv instead of dev mode - Replace hardcoded protocol version 12 for Shielded gate with platform version feature checks (shield, shielded_transfer, unshield, shield_from_asset_lock, shielded_withdrawal state transitions) - Remove now-unused supports_shielded() and SHIELDED_MIN_PROTOCOL_VERSION Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/model/feature_gate.rs (1)
1-93: Consider adding unit tests for FeatureGate predicates.The
Shieldedgate has non-trivial logic (checking five state transition types). A few unit tests with mockAppContextstates would validate the gate logic and serve as documentation for expected behavior. This is low-effort sinceis_availableis a pure-function predicate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/feature_gate.rs` around lines 1 - 93, Add unit tests covering FeatureGate::is_available behavior: create tests that construct minimal AppContext instances (or a test helper) with platform_version().dpp.state_transition_serialization_versions where you can set the five fields (shield_state_transition, shielded_transfer_state_transition, unshield_state_transition, shield_from_asset_lock_state_transition, shielded_withdrawal_state_transition) to verify FeatureGate::Shielded returns true when all max_version > 0 and false when any is 0; also add simple tests for FeatureGate::DeveloperMode (toggle ctx.is_developer_mode()) and FeatureGate::SpvBackend (set ctx.core_backend_mode() to CoreBackendMode::Spv and non-Spv) to assert expected results; place tests alongside the module (e.g., #[cfg(test)] mod tests) and use small helper constructors or mocks for AppContext and platform_version to keep tests focused on is_available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/model/feature_gate.rs`:
- Around line 1-93: Add unit tests covering FeatureGate::is_available behavior:
create tests that construct minimal AppContext instances (or a test helper) with
platform_version().dpp.state_transition_serialization_versions where you can set
the five fields (shield_state_transition, shielded_transfer_state_transition,
unshield_state_transition, shield_from_asset_lock_state_transition,
shielded_withdrawal_state_transition) to verify FeatureGate::Shielded returns
true when all max_version > 0 and false when any is 0; also add simple tests for
FeatureGate::DeveloperMode (toggle ctx.is_developer_mode()) and
FeatureGate::SpvBackend (set ctx.core_backend_mode() to CoreBackendMode::Spv and
non-Spv) to assert expected results; place tests alongside the module (e.g.,
#[cfg(test)] mod tests) and use small helper constructors or mocks for
AppContext and platform_version to keep tests focused on is_available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5d10f7c-f42a-41fb-a5c5-e91c71758e4f
📒 Files selected for processing (4)
src/context/mod.rssrc/model/feature_gate.rssrc/ui/dashpay/contact_profile_viewer.rssrc/ui/wallets/shield_screen.rs
💤 Files with no reviewable changes (1)
- src/context/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/dashpay/contact_profile_viewer.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a centralized FeatureGate mechanism to decide UI feature visibility based on AppContext, and refactors several UI locations to use it instead of ad-hoc checks.
Changes:
- Introduces
src/model/feature_gate.rswithFeatureGate::is_available()and anegui::Uiextension for gated UI sections. - Replaces scattered
is_developer_mode()/supports_shielded()conditionals in multiple UI screens withFeatureGatechecks. - Removes
AppContext::supports_shielded()and its protocol-version constant.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/model/feature_gate.rs | New centralized gate predicates + Ui helper for feature-gated sections. |
| src/model/mod.rs | Exposes the new feature_gate module. |
| src/context/mod.rs | Removes the older shielded protocol gating API (supports_shielded). |
| src/ui/wallets/wallets_screen/mod.rs | Gates Shielded tab + dev-only UI via FeatureGate. |
| src/ui/wallets/shield_screen.rs | Replaces dev-mode batch controls visibility with feature_gated. |
| src/ui/dashpay/contact_profile_viewer.rs | Gates “Pay” button via FeatureGate::SpvBackend. |
| src/ui/components/dashpay_subscreen_chooser_panel.rs | Gates Payments subscreen via FeatureGate::SpvBackend. |
| src/ui/components/left_panel.rs | Adds optional gating for sidebar buttons (PoC: DashPay). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
platform_version() returns a hardcoded PLATFORM_V11 default. Use platform_protocol_version() (fetched from network) with PlatformVersion::get_optional() to look up the actual version struct. This ensures the Shielded gate reflects what the network actually supports. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Both AddNewWalletScreen and ImportMnemonicScreen unconditionally fired CoreTask::ListCoreWallets on init, which makes an RPC call to Dash Core. In SPV mode there is no Dash Core — this produced "Could not connect to Dash Core at 127.0.0.1:19998" errors. Fix: set core_wallets to empty vec in SPV mode, skipping the RPC call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ui/wallets/add_new_wallet_screen.rs`:
- Around line 583-591: The code uses self.core_wallets = Some(vec![]) as a
sentinel to mean “SPV skipped”, which collides with the legitimate “loaded zero
wallets” state and prevents a later non-SPV mode from triggering a fetch;
introduce an explicit boolean flag (e.g. core_wallets_skipped_for_spv) on the
screen state and set it when app_context.core_backend_mode() ==
crate::spv::CoreBackendMode::Spv, change the is_none() condition to check that
core_wallets is None AND core_wallets_skipped_for_spv is false (or simply check
!core_wallets_skipped_for_spv), and when backend mode becomes non-SPV clear
core_wallets_skipped_for_spv so the existing code path can set
core_wallets_loading and dispatch
BackendTask::CoreTask(CoreTask::ListCoreWallets) to load wallets.
In `@src/ui/wallets/import_mnemonic_screen.rs`:
- Around line 486-493: The code sets self.core_wallets = Some(vec![]) to signal
SPV skip which conflates "skipped" with "loaded zero wallets" and prevents later
ListCoreWallets dispatch; change to use an explicit state/flag like the
add-wallet screen does (e.g., add a core_wallets_skipped: bool or convert
core_wallets to an enum CoreWalletsState { Uninitialized, Loading, Skipped,
Loaded(Vec<...>) }) and update the logic around core_wallets and
core_wallets_loading (the branches that set and check core_wallets,
core_wallets_loading, and where
AppAction::BackendTask(CoreTask::ListCoreWallets) is assigned) to set Skipped
instead of Some(vec![]), treat Skipped as distinct from Loaded(empty), and
clear/reset Skipped when app_context.core_backend_mode() changes so
ListCoreWallets can be dispatched when switching out of SPV.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55a23820-edfc-4c20-ab42-c6811480e15d
📒 Files selected for processing (2)
src/ui/wallets/add_new_wallet_screen.rssrc/ui/wallets/import_mnemonic_screen.rs
thepastaclaw
reviewed
Apr 1, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Well-designed feature gate abstraction that centralizes scattered visibility checks. The Shielded gate logic is correct. However, the PR replaces is_developer_mode() with FeatureGate::SpvBackend at two callsites where the backend supports both RPC and SPV modes, narrowing functionality for developer-mode RPC users without backend justification. One minor visual regression from add_visible allocating invisible layout space.
Reviewed commit: f1616d1
🟡 4 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
💬 nitpick: left_panel mixes FeatureGate and raw `is_developer_mode()` in the same function
src/ui/components/left_panel.rs (lines 193-195)
The button visibility array uses FeatureGate::DashPay (line 127), but the bottom_reserved height calculation (line 193) and the dev-mode label at the bottom still call app_context.is_developer_mode() directly. These are layout concerns rather than visibility gates, so using the raw accessor is defensible — but it mixes both abstractions in one function. Not a bug, just an inconsistency to be aware of.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/components/dashpay_subscreen_chooser_panel.rs`:
- [SUGGESTION] lines 18-24: Payment History gated behind SpvBackend but only reads local database
This changed from `is_developer_mode()` to `FeatureGate::SpvBackend`, which is strictly narrower — a developer using RPC mode loses access to Payment History. But `LoadPaymentHistory` is a pure SQLite read (`db.load_payment_history(...)`) with no network dependency. If a user previously made payments in SPV mode and later switches to RPC, their history becomes invisible.
Consider whether the intent is to gate on `DeveloperMode` (the old behavior) or `SpvBackend` (new). If payment history should remain accessible regardless of backend mode, revert to the `DeveloperMode` gate or remove the gate entirely for this read-only screen.
In `src/ui/dashpay/contact_profile_viewer.rs`:
- [SUGGESTION] lines 444-458: Pay button hidden in RPC mode despite backend supporting both modes
The Pay button changed from `is_developer_mode()` to `feature_gated(SpvBackend)`. However, `SendPaymentToContact` delegates to `CoreTask::SendWalletPayment`, which explicitly branches on `core_backend_mode()` and handles both SPV (`send_wallet_payment_via_spv`) and RPC (`send_wallet_payment_via_rpc`). Developer-mode users running the RPC backend lose the ability to send payments to contacts despite a working code path.
Same issue at lines 475-489 (the no-profile case).
In `src/ui/wallets/wallets_screen/mod.rs`:
- [SUGGESTION] lines 2013-2020: `add_visible(false, ...)` allocates invisible layout space for [DEV] badge
The previous code used `if is_developer_mode() { ui.label(...) }`, which allocated no space when false. The new code uses `ui.add_visible(FeatureGate::DeveloperMode.is_available(...), Label)`. egui's `add_visible(false, widget)` still calls `self.add(widget)` internally — the egui docs state: "An invisible widget still takes up the same space as if it were visible." This introduces a small invisible gap next to the wallet name when developer mode is off.
In `src/model/feature_gate.rs`:
- [SUGGESTION] lines 1-101: No unit tests for FeatureGate predicates
The Shielded gate has non-trivial logic — it looks up `PlatformVersion` by protocol version, then checks five `FeatureVersionBounds` fields. A test covering at least one protocol version where shielded is absent (`max_version == 0`) and one where it's present (`max_version > 0`) would guard against regressions. The DeveloperMode and SpvBackend predicates are trivial wrappers, but Shielded warrants coverage.
- Revert Payment History and Pay button gates from SpvBackend to DeveloperMode — these features work in both RPC and SPV modes - Extract duplicate Pay button into pay_button() method on ContactProfileViewerScreen (was copy-pasted in two branches) - Fix [DEV] badge: use `if` block instead of `add_visible(false, ...)` to avoid allocating invisible layout space - Update stale comment about "protocol version >= 12" to reflect actual FeatureGate::Shielded predicate Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
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.
Summary
Introduces a predicate-based
FeatureGateenum that centralizes UI feature availability decisions in a single module (src/model/feature_gate.rs).boolpredicate overAppContext— compiler enforces exhaustivenesssupports_shielded()andis_developer_mode()checks with uniformFeatureGate::X.is_available(&ctx)callsall_available()/any_available()for combined criteriaIntegration points
left_panel.rs)FeatureGate::DashPaywallets_screen/mod.rs)FeatureGate::Shieldeddashpay_subscreen_chooser_panel.rs)FeatureGate::SpvBackendAdding a new gate (3 steps)
FeatureGateenummatcharmFeatureGate::NewThing.is_available(&ctx)at callsiteUser story
Imagine you are a user running Dash Evo Tool on mainnet. Before the network upgrades to support shielded transactions, you don't see confusing shielded-related tabs or options — they simply aren't there. Once the network upgrades, the UI adapts automatically. Similarly, advanced features like SPV backend only appear when you've enabled expert mode in settings.
Test plan
cargo build --all-features— compiles clean🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Refactor
Bug Fixes