Phase 3 UI mostrod select #440
Merged
Conversation
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
Filter out custom nodes whose pubkey overlaps with trusted nodes during init(). This handles the case where a trusted node pubkey was persisted as a custom node before the trusted nodes registry existed. The cleaned list is persisted back to disk as a one-time migration.
Add bottom sheet node selector with trusted/custom sections, node avatars with kind 0 metadata, trusted badges, and custom node management dialog. Integrate selector into settings screen replacing the old pubkey text field. Update localization strings for en/es/it.
Contributor
WalkthroughAdds a modal bottom-sheet Mostro node selector, an Add Custom Node dialog, avatar and trusted-badge widgets; introduces hex pubkey validation on MostroNode; notifier migration to remove custom/trusted overlaps and persist metadata; refactors Settings to use node-centric selection and updates tests and localization. Changes
Sequence DiagramsequenceDiagram
actor User
participant Settings as "SettingsScreen"
participant Selector as "MostroNodeSelector"
participant Dialog as "AddCustomNodeDialog"
participant Notifier as "MostroNodesNotifier"
participant Restore as "RestoreService"
participant Meta as "MetadataService"
User->>Settings: tap Mostro card
Settings->>Selector: show()
Selector->>User: display trusted + custom nodes
User->>Selector: select node
Selector->>Notifier: updateSelectedNode(pubkey)
Notifier->>Restore: trigger restore flow
Restore-->>Notifier: restore complete
Notifier-->>Selector: confirm selection
Selector->>Settings: close()
User->>Selector: tap Add Custom Node
Selector->>Dialog: show()
Dialog->>User: collect pubkey & name
User->>Dialog: submit
Dialog->>Dialog: validate (hex/npub, duplicates)
Dialog->>Notifier: addCustomNode(pubkey, name)
Notifier->>Meta: fetchNodeMetadata(pubkey) [fire-and-forget]
Dialog->>User: show success snackbar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![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: 5
🤖 Fix all issues with AI agents
In `@docs/MULTI_MOSTRO_SUPPORT.md`:
- Around line 128-139: Update the fenced diagram block to include a language
specifier by changing the opening fence from ``` to ```text so markdownlint
MD040 is satisfied; modify the block that documents the Settings Screen flow
(references: _buildMostroCard, MostroNodeSelector.show, MostroNodeSelector,
_buildNodeItem, AddCustomNodeDialog.show) to begin with ```text and keep the
diagram content unchanged.
In `@lib/features/mostro/widgets/add_custom_node_dialog.dart`:
- Around line 162-199: Normalize the hex pubkey string immediately after
conversion so duplicate/trusted checks are case-insensitive: in the block that
sets hexPubkey from _convertToHex(input), call a consistent normalization (e.g.,
hexPubkey = hexPubkey.toLowerCase()) and then use that normalized hexPubkey for
MostroNode.isValidHexPubkey, the nodes.any duplicate check
(mostroNodesProvider), and when calling
ref.read(mostroNodesProvider.notifier).addCustomNode so stored pubkeys are
normalized and case variants cannot bypass checks.
In `@lib/features/mostro/widgets/mostro_node_avatar.dart`:
- Around line 39-44: The fallback and no-picture branches pass size / 2 to
NymAvatar causing a smaller avatar than the Image.network; update both
occurrences (the errorBuilder fallback and the final return) to pass size (not
size / 2) into NymAvatar so NymAvatar(pubkeyHex: node.pubkey, size: size) is
used consistently.
In `@lib/features/mostro/widgets/mostro_node_selector.dart`:
- Around line 381-389: When removeCustomNode returns false the dialog closes
with no feedback; update the block that checks `removed` (the result of
`notifier.removeCustomNode(node.pubkey)`) to show an error snackbar in the else
branch using `SnackBarHelper.showTopSnackBar` (e.g.
`localizations.nodeRemoveFailed` or a generic failure message), and guard that
call with the same `mounted` check as the success branch; keep the existing
success snackbar (`localizations.nodeRemovedSuccess`) unchanged and ensure the
new branch only triggers when `removed` is false.
- Around line 328-333: Replace the raw exception text shown to users in the
SnackBar: in the call to SnackBarHelper.showTopSnackBarAsync (the invocation
using messenger, mediaQuery.size.height and mediaQuery.padding.top), stop
passing e.toString() as message and instead pass a user-friendly localized
string (e.g., AppLocalizations.of(context).errorSwitchingNode). Add the new
errorSwitchingNode key to the ARB localization files with appropriate
translations and use that key here so internal error details are not exposed to
the user.
🧹 Nitpick comments (3)
lib/features/settings/settings_screen.dart (1)
601-605: Consider using the watched value directly instead of the watch+read pattern.
ref.watch(mostroNodesProvider)subscribes for rebuilds but the return value is discarded;selectedNodeis then read imperatively from the notifier. IfmostroNodesProvideralready exposes the node list as its state, you could deriveselectedNodefrom it (or from a dedicatedselectedNodeProvider) to keep the data flow purely declarative.That said, this works correctly as-is—just a Riverpod idiom nit.
lib/features/mostro/widgets/mostro_node_selector.dart (2)
264-276: Small touch target on the delete button.
BoxConstraints()withEdgeInsets.all(4)padding yields a hit area well below the recommended 48×48 dp minimum. Consider usingvisualDensityor aSizedBoxwrapper to ensure the target is large enough, while keeping the icon visually compact.
303-309: Silently swallowing restore errors may hide real issues.The inner
catch (_)discards any restore failure without logging. Even if restore errors are non-fatal, alogger.warning(...)here would aid debugging when node switches behave unexpectedly.Proposed fix
+ import 'package:mostro_mobile/services/logger_service.dart'; ... try { final restoreService = ref.read(restoreServiceProvider); await restoreService.initRestoreProcess(); - } catch (_) { - // Ignore errors during restore + } catch (e) { + logger.w('Restore after node switch failed: $e'); }As per coding guidelines, always use the pre-configured singleton
loggerinstance viaimport 'package:mostro_mobile/services/logger_service.dart'.
updateNodeMetadata() only updated in-memory state without calling _saveCustomNodes(), causing metadata (name, picture, about) to be lost on app restart. Also removes truncatedPubkey helper in favor of ellipsis overflow, shows node about text in selector, and fixes test count in docs.
Added .toLowerCase() at line 164 right after _convertToHex(input), so hexPubkey is normalized before it's used for validation, duplicate checking, and addCustomNode(). This prevents case variants from bypassing existing checks.
Member
|
Hello, When tapping Cancel or Add in the widget used to add nodes, an exception is thrown, However the node is added,it is necessary to close and reopen the app.. Below are the screenshots and the full error log for reference.
|
Catrya
reviewed
Feb 10, 2026
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/features/mostro/widgets/add_custom_node_dialog.dart`:
- Line 26: The dialog allows dismissal while awaiting the async addCustomNode
handler which can dispose controllers and cause setState (in the dialog's state)
or Navigator.pop to run on an unmounted tree; to fix, set barrierDismissible:
false on the showDialog call (replace barrierDismissible: true) to prevent
outside taps during the async operation, or alternatively add a mounted check
after the await in the onPressed handler (e.g., before calling setState or
Navigator.pop) to bail out if the dialog was dismissed; update the code paths in
the addCustomNode onPressed handler and any setState/Navigator.pop calls to use
this guard if you choose the mounted-check approach.
- Around line 23-248: The controllers (pubkeyController, nameController) are
being disposed in the finally after showDialog, which can run while the dialog's
exit animation is still active and cause "used after disposed" crashes; extract
the dialog UI into a standalone StatefulWidget (e.g.,
AddCustomNodeDialogStateful) that creates and owns pubkeyController and
nameController in its State and disposes them in State.dispose(), then change
the showDialog builder to return that StatefulWidget (leave logic like
navigator.pop(), _convertToHex, MostroNode.isValidHexPubkey, and
notifier.addCustomNode unchanged).
In `@lib/l10n/intl_en.arb`:
- Around line 1366-1402: Add the 27 missing localization entries from the
English ARB into the Italian ARB (intl_it.arb): include keys such as
selectMostroNode, mostroNodeDescription, trustedNodesSection,
customNodesSection, addCustomNode, addCustomNodeTitle, enterNodePubkey,
enterNodeName, pubkeyHint, nodeNameHint, nodeAlreadyExists, invalidPubkeyFormat,
nodeAddedSuccess, nodeRemovedSuccess, nodeRemoveFailed, cannotRemoveActiveNode,
deleteCustomNodeTitle, deleteCustomNodeMessage, deleteCustomNodeConfirm,
deleteCustomNodeCancel, switchingToNode, nodeSwitchedSuccess,
errorSwitchingNode, trusted, noCustomNodesYet, pubkeyRequired, tapToSelectNode;
also add the corresponding metadata blocks for placeholder keys (the
"@switchingToNode" and "@nodeSwitchedSuccess" objects with the "nodeName"
placeholder of type "String") so the Italian file mirrors the structure of
intl_en.arb.
…ctive Problem: TextEditingControllers were created in the static show() method and disposed in a finally block. Since showDialog's future resolves when Navigator.pop() is called (before the exit animation finishes), the controllers got disposed while the widget tree still referenced them, causing "used after disposed" crashes. Fix: Extracted the dialog body into a private _AddCustomNodeDialogContent StatefulWidget that: - Owns pubkeyController and nameController (created in initState()) - Owns errorMessage as local state (replacing the StatefulBuilder) - Disposes both controllers in State.dispose() — safe because Flutter calls dispose after the animation completes The show() method is now simplified to just capture parent context values and call showDialog with the new widget. The try/finally block is removed entirely.
When a user switches to a different Mostro node, the restore process clears all existing data before contacting the new node. If the new node is offline, the app was left in a broken state with no data and settings pointing to an unreachable node. Now if the restore fails, the app automatically reverts to the previous node and re-restores its data. The user sees an error snackbar and the bottom sheet stays open for them to pick a different node. - Make auto-hide timers in RestoreProgressNotifier cancellable to prevent stale timers from hiding the overlay during a revert restore - Return success/failure from initRestoreProcess() so callers can react - Handle restore failure in MostroNodeSelector with automatic revert - Add nodeNotRespondingReverted localization key (en/es/it)
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/features/mostro/widgets/mostro_node_selector.dart`:
- Around line 297-368: In _onNodeTap, guard against the widget being unmounted
before calling navigator.pop() and showing snackbars: after the async operations
that precede navigator.pop() (e.g., after await notifier.selectNode and any
restore flow), check if (mounted) before calling navigator.pop() and before
calling SnackBarHelper.showTopSnackBarAsync (using the captured messenger,
mediaQuery, and localizations); if not mounted, return early to avoid popping or
using an unmounted context and avoid calling setState unless mounted. Ensure
this mounted check covers both the successful-switch path (navigator.pop +
success snackbar) and the catch/error snackbar path.
In `@lib/features/restore/restore_manager.dart`:
- Around line 719-721: The catch block currently calls
ref.read(restoreProgressProvider.notifier).showError('') which passes an empty
message to the UI; replace that empty string with a user-facing, non-technical
message (e.g., a localized "Restore failed" string) while leaving the existing
logger.e('Restore: error during restore process', error: e, stackTrace: stack)
intact so full error details remain in logs; specifically update the call to
ref.read(restoreProgressProvider.notifier).showError(...) to use your app's
localization helper (or a hardcoded friendly message if localization isn't
available) instead of ''.
- Around line 684-685: The code calls progress.completeRestore() twice when
ordersMap is empty: once just before returning true in the early-exit
(progress.completeRestore()) and again from the finally block (which also runs
on normal completion), causing the auto-hide timer to restart; fix this by
removing the early progress.completeRestore() call in the branch that checks
ordersMap.isEmpty and just return true there so the finally block's
completeRestore() handles cleanup, ensuring no duplicate calls to
progress.completeRestore().
🧹 Nitpick comments (1)
lib/features/restore/restore_progress_notifier.dart (1)
62-68: Consider extracting the duplicated auto-hide scheduling.Lines 62–68 and 80–86 are identical. A small helper would reduce duplication.
♻️ Proposed refactor
+ void _scheduleAutoHide() { + _cancelAutoHideTimer(); + _autoHideTimer = Timer(const Duration(seconds: 3), () { + if (mounted) { + hide(); + } + }); + } + void completeRestore() { logger.i('Restore completed successfully'); _cancelTimeoutTimer(); state = state.copyWith( step: RestoreStep.completed, ); - // Auto-hide after 3 seconds (cancellable) - _cancelAutoHideTimer(); - _autoHideTimer = Timer(const Duration(seconds: 3), () { - if (mounted) { - hide(); - } - }); + _scheduleAutoHide(); } void showError(String message) { logger.w('Restore error: $message'); _cancelTimeoutTimer(); state = state.copyWith( step: RestoreStep.error, errorMessage: message, ); - // Auto-hide after 3 seconds (cancellable) - _cancelAutoHideTimer(); - _autoHideTimer = Timer(const Duration(seconds: 3), () { - if (mounted) { - hide(); - } - }); + _scheduleAutoHide(); }Also applies to: 80-86
Comment on lines
+297
to
+368
| Future<void> _onNodeTap(MostroNode node) async { | ||
| setState(() { | ||
| _isSwitching = true; | ||
| _switchingPubkey = node.pubkey; | ||
| }); | ||
|
|
||
| // Capture context values before async | ||
| final messenger = ScaffoldMessenger.of(context); | ||
| final mediaQuery = MediaQuery.of(context); | ||
| final localizations = S.of(context)!; | ||
| final navigator = Navigator.of(context); | ||
|
|
||
| try { | ||
| final notifier = ref.read(mostroNodesProvider.notifier); | ||
| final oldPubkey = ref.read(settingsProvider).mostroPublicKey; | ||
|
|
||
| await notifier.selectNode(node.pubkey); | ||
|
|
||
| if (oldPubkey != node.pubkey) { | ||
| final restoreService = ref.read(restoreServiceProvider); | ||
| final success = await restoreService.initRestoreProcess(); | ||
|
|
||
| if (!success) { | ||
| // Hide error overlay from failed restore | ||
| ref.read(restoreProgressProvider.notifier).hide(); | ||
|
|
||
| // Revert settings to old node | ||
| await notifier.selectNode(oldPubkey); | ||
|
|
||
| // Re-restore old node's data (since _clearAll already wiped it) | ||
| await restoreService.initRestoreProcess(); | ||
|
|
||
| if (mounted) { | ||
| setState(() { | ||
| _isSwitching = false; | ||
| _switchingPubkey = null; | ||
| }); | ||
| } | ||
|
|
||
| SnackBarHelper.showTopSnackBarAsync( | ||
| messenger: messenger, | ||
| screenHeight: mediaQuery.size.height, | ||
| statusBarHeight: mediaQuery.padding.top, | ||
| message: localizations.nodeNotRespondingReverted, | ||
| ); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| navigator.pop(); | ||
|
|
||
| SnackBarHelper.showTopSnackBarAsync( | ||
| messenger: messenger, | ||
| screenHeight: mediaQuery.size.height, | ||
| statusBarHeight: mediaQuery.padding.top, | ||
| message: localizations.nodeSwitchedSuccess(node.displayName), | ||
| backgroundColor: Colors.green, | ||
| ); | ||
| } catch (e) { | ||
| if (mounted) { | ||
| setState(() { | ||
| _isSwitching = false; | ||
| _switchingPubkey = null; | ||
| }); | ||
| } | ||
| SnackBarHelper.showTopSnackBarAsync( | ||
| messenger: messenger, | ||
| screenHeight: mediaQuery.size.height, | ||
| statusBarHeight: mediaQuery.padding.top, | ||
| message: localizations.errorSwitchingNode, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the widget class definition and build method
rg -A 30 "class MostroNodeSelector" lib/features/mostro/widgets/mostro_node_selector.dartRepository: MostroP2P/mobile
Length of output: 1077
🏁 Script executed:
# Check if there's WillPopScope or PopScope usage
rg -B 5 -A 10 "(WillPopScope|PopScope|barrierDismissible)" lib/features/mostro/widgets/mostro_node_selector.dartRepository: MostroP2P/mobile
Length of output: 42
🏁 Script executed:
# Look for how _isSwitching is used in the build method
rg -B 2 -A 5 "_isSwitching" lib/features/mostro/widgets/mostro_node_selector.dartRepository: MostroP2P/mobile
Length of output: 2064
🏁 Script executed:
# Find how this widget is displayed (showModalBottomSheet call)
rg -B 5 -A 10 "MostroNodeSelector\(\)" lib/Repository: MostroP2P/mobile
Length of output: 1408
🏁 Script executed:
# Check for other showModalBottomSheet calls with barrierDismissible: false
rg "barrierDismissible.*false" lib/Repository: MostroP2P/mobile
Length of output: 205
🏁 Script executed:
# Check if there are other async operations in bottom sheets with mounted checks before pop
rg -B 5 -A 3 "if.*mounted.*\).*\n.*navigator\.pop\(\)" lib/Repository: MostroP2P/mobile
Length of output: 251
🏁 Script executed:
# Check for mounted checks before navigator.pop() with multiline
rg -U "if.*mounted.*navigator\.pop\(\)" lib/Repository: MostroP2P/mobile
Length of output: 42
🏁 Script executed:
# Look at the relay_selector.dart to see if it has similar async operations
cat -n lib/features/relays/widgets/relay_selector.dart | head -100Repository: MostroP2P/mobile
Length of output: 4079
🏁 Script executed:
# Check how relay_selector.dart handles async operations
rg -A 20 "barrierDismissible.*false" lib/features/relays/widgets/relay_selector.dartRepository: MostroP2P/mobile
Length of output: 882
Guard navigator.pop() against unmounted context after async operations.
The bottom sheet defaults to barrierDismissible: true, allowing dismissal via swipe or outside tap during the async node switch. Without a mounted check, line 346's navigator.pop() could pop the underlying route if the sheet was already dismissed, and the success snackbar would execute on an unmounted context.
Suggested fix
- navigator.pop();
-
- SnackBarHelper.showTopSnackBarAsync(
+ if (!mounted) return;
+ navigator.pop();
+
+ SnackBarHelper.showTopSnackBarAsync(
messenger: messenger,
screenHeight: mediaQuery.size.height,
statusBarHeight: mediaQuery.padding.top,
message: localizations.nodeSwitchedSuccess(node.displayName),
backgroundColor: Colors.green,
);🤖 Prompt for AI Agents
In `@lib/features/mostro/widgets/mostro_node_selector.dart` around lines 297 -
368, In _onNodeTap, guard against the widget being unmounted before calling
navigator.pop() and showing snackbars: after the async operations that precede
navigator.pop() (e.g., after await notifier.selectNode and any restore flow),
check if (mounted) before calling navigator.pop() and before calling
SnackBarHelper.showTopSnackBarAsync (using the captured messenger, mediaQuery,
and localizations); if not mounted, return early to avoid popping or using an
unmounted context and avoid calling setState unless mounted. Ensure this mounted
check covers both the successful-switch path (navigator.pop + success snackbar)
and the catch/error snackbar path.
Comment on lines
684
to
+685
| progress.completeRestore(); | ||
| return; | ||
| return true; |
Contributor
There was a problem hiding this comment.
Double completeRestore() call on the early-exit path.
When ordersMap is empty, completeRestore() is called at line 684, then return true triggers the finally block, which calls completeRestore() again at line 734 (since step is completed, not error). This restarts the auto-hide timer unnecessarily.
Either remove the call at line 684 (let finally handle it), or guard the finally call to also skip the completed step.
Proposed fix (option A: let finally handle it)
if (ordersMap.isEmpty) {
logger.w('Restore: no orders or disputes to restore');
await _sendLastTradeIndexRequest();
final lastTradeIndexEvent = await _waitForEvent(RestoreStage.gettingTradeIndex);
final lastTradeIndexResponse = await _extractLastTradeIndex(lastTradeIndexEvent);
final lastTradeIndex = lastTradeIndexResponse.tradeIndex;
await keyManager.setCurrentKeyIndex(lastTradeIndex + 1);
- progress.completeRestore();
+ success = true;
return true;
}Proposed fix (option B: guard in finally)
// Only call completeRestore if not in error state
final currentState = ref.read(restoreProgressProvider);
- if (currentState.step != RestoreStep.error) {
+ if (currentState.step != RestoreStep.error && currentState.step != RestoreStep.completed) {
ref.read(restoreProgressProvider.notifier).completeRestore();
}🤖 Prompt for AI Agents
In `@lib/features/restore/restore_manager.dart` around lines 684 - 685, The code
calls progress.completeRestore() twice when ordersMap is empty: once just before
returning true in the early-exit (progress.completeRestore()) and again from the
finally block (which also runs on normal completion), causing the auto-hide
timer to restart; fix this by removing the early progress.completeRestore() call
in the branch that checks ordersMap.isEmpty and just return true there so the
finally block's completeRestore() handles cleanup, ensuring no duplicate calls
to progress.completeRestore().
Comment on lines
719
to
721
| } catch (e, stack) { | ||
| logger.e('Restore: error during restore process', error: e, stackTrace: stack); | ||
| ref.read(restoreProgressProvider.notifier).showError(''); |
Contributor
There was a problem hiding this comment.
Empty error message passed to showError.
showError('') will propagate an empty string to the UI overlay's error message field. Consider using a generic user-friendly message instead (e.g., a localized "Restore failed" string), while keeping the raw exception in the logger call at line 720. Based on learnings, the team prefers to keep technical error details out of the UI while logging the full error via logger.e().
Suggested change
} catch (e, stack) {
logger.e('Restore: error during restore process', error: e, stackTrace: stack);
- ref.read(restoreProgressProvider.notifier).showError('');
+ ref.read(restoreProgressProvider.notifier).showError('Restore failed');
} finally {🤖 Prompt for AI Agents
In `@lib/features/restore/restore_manager.dart` around lines 719 - 721, The catch
block currently calls ref.read(restoreProgressProvider.notifier).showError('')
which passes an empty message to the UI; replace that empty string with a
user-facing, non-technical message (e.g., a localized "Restore failed" string)
while leaving the existing logger.e('Restore: error during restore process',
error: e, stackTrace: stack) intact so full error details remain in logs;
specifically update the call to
ref.read(restoreProgressProvider.notifier).showError(...) to use your app's
localization helper (or a hardcoded friendly message if localization isn't
available) instead of ''.
mostronator
approved these changes
Feb 11, 2026
Collaborator
mostronator
left a comment
mostronator
left a comment
There was a problem hiding this comment.
✅ Approved — Excellent Phase 3 Implementation
Great work on this Multi-Mostro UI phase. Detailed review:
Strengths
- Clean architecture: Node selector as bottom sheet, custom node dialog as separate widget — good separation of concerns
- Solid validation: npub/hex input validation with
MostroNode.isValidHexPubkey(), duplicate detection, proper error states - Good UX: Non-blocking metadata fetch, debounced validation, loading spinners, prevent deleting active node
- Complete i18n: All 3 languages (en/es/it) with proper placeholders
- Smart migration: Auto-removing custom nodes that overlap with trusted nodes
- Proper state management: RestoreManager now returns
boolfor success tracking, RestoreProgressNotifier properly cancels timers - Test maintenance: Removed truncatedPubkey tests that no longer apply, updated assertions
Minor observations (non-blocking)
MostroNodeAvatarusesImage.networkdirectly — consider caching withCachedNetworkImagefor better performance on slow connections- The
_AddCustomNodeDialogContentStatecould benefit from aFormKeyfor more structured validation - Custom node metadata is persisted via
_saveCustomNodesinsideapplyMetadata— this side effect could be more explicit
Overall a well-thought-out implementation that follows the project patterns. Ship it! 🚀
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 by CodeRabbit
New Features
Bug Fixes
Documentation