fix(walletui): flow editor layout - #1593
Conversation
WalkthroughRefactors several web UI components: restructures FlowEditor layout and changes account/template selection to use Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant FE as FlowEditor (UI)
participant AS as Account Select
participant ST as Frontend State
User->>AS: open account dropdown, choose account
AS-->>FE: selected component_address
FE->>ST: onAccountChange(component_address)
Note right of FE: matching by component_address (was address)
sequenceDiagram
autonumber
actor User
participant FE as FlowEditor (UI)
participant TS as Builtin Template Select
participant ST as Frontend State
User->>TS: open templates, pick builtin template
TS-->>FE: template key/value
FE->>ST: lookup via KNOWN_TEMPLATES -> set templateId
Note right of FE: selection wrapped in FormControl/InputLabel + FormHelperText
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx (1)
276-302: Consider using styled components for inline styles.The layout refactoring improves consistency as intended. The nested Grid structure with explicit spacing is appropriate for the Flow Editor layout.
For consistency with the existing codebase pattern (e.g.,
StyledPaperon line 288), consider extracting the inline style object into a styled component:const StyledGridContainer = styled(Grid)(({ theme }) => ({ margin: 0, width: '100%', }));Then use it as:
- <Grid - container - spacing={3} - style={{ - margin: 0, - width: "100%", - }} - > + <StyledGridContainer container spacing={3}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx (2)
applications/tari_walletd/web_ui/src/utils/helpers.tsx (2)
substateIdToString(100-109)shortenString(134-143)crates/wallet/sdk/src/models/account.rs (1)
account(60-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check stable
- GitHub Check: machete
- GitHub Check: test
- GitHub Check: check nightly
- GitHub Check: clippy
- GitHub Check: fmt
🔇 Additional comments (5)
applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx (5)
50-50: LGTM!The
FormHelperTextimport is correctly added and used throughout the component for providing helper text to form controls.
326-356: Improved account selection UX.The changes improve the UI by:
- Simplifying the label to "Account" while moving the explanation to helper text
- Using
component_addressconsistently for MenuItem keys and values- Adding descriptive helper text "Select the account used to pay fees"
Note: The
placeholderprop on line 332 won't display in a controlled Select component with a value, but it doesn't cause any issues.
359-368: LGTM!The Transaction Fee input now uses an integrated label, which is cleaner and more consistent with Material UI form patterns.
384-423: Well-structured template selection.The template section improvements include:
- Labeled TextField for Template ID
- Proper FormControl wrapper for builtin templates
- Clear None option for deselection
- Helpful guidance text
The value lookup logic on line 403 correctly finds the matching template by address, and the MenuItem mapping is implemented correctly.
180-185: Account property usage verifiedThe
Accounttype inbindings/src/types/Account.tsincludescomponent_addressand noaddressfield exists; usingcomponent_addressfor matching is correct.
Test Results (CI)448 tests +27 447 ✅ +26 1h 33m 7s ⏱️ + 45m 8s For more details on these failures, see this check. Results for commit 5da9d0d. ± Comparison against base commit 7458abd. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
applications/tari_walletd/web_ui/src/routes/Templates/Templates.tsx (2)
95-100: Reset pagination when switching accountsIf I switch from an account that was on a later page to one with fewer templates, we keep requesting the old
pageindex. The API then returns an empty list even though the new account has data, so the table appears blank. Please reset pagination (and related state) when the account changes.const onAccountChange = (e: SelectChangeEvent) => { const selected = dataAccountsList?.accounts.find( (account: AccountInfo) => substateIdToString(account.account.component_address) === e.target.value, ); - setAccount(selected); + setAccount(selected); + setPage(0); + setOpen([]); + setTemplatesCount(0); };
102-109: Handle zero-template responses so counts stay correctWhen the backend returns zero templates (e.g., switching to an empty account), this effect never fires, leaving
templatesCountat the previous non-zero value and the pagination shows stale totals. It also leaves theopenarray from the prior response. Please update the state for every response, including whentemplatesResponse.templates.length === 0.- useEffect(() => { - if (templatesResponse && templatesResponse.templates.length > 0) { - let opens = new Array<boolean>(templatesResponse.templates.length); - opens.fill(false); - setOpen(opens); - setTemplatesCount(templatesResponse.total_templates); - } - }, [templatesResponse]); + useEffect(() => { + if (!templatesResponse) { + setOpen([]); + setTemplatesCount(0); + return; + } + + setOpen(Array.from({ length: templatesResponse.templates.length }, () => false)); + setTemplatesCount(templatesResponse.total_templates ?? 0); + }, [templatesResponse]);
🧹 Nitpick comments (1)
applications/tari_walletd/web_ui/src/routes/Templates/Templates.tsx (1)
159-258: Add keys to fragments for stable row renderingEach iteration returns a fragment (
<>…</>) without akey, triggering React warnings and risking diff instability. Wrap the fragment inReact.Fragment(or move thekeyto it) so React can reconcile the rows cleanly.- {templatesResponse?.templates.map((template: AuthoredTemplate, index: number) => { - return ( - <> + {templatesResponse?.templates.map((template: AuthoredTemplate, index: number) => { + return ( + <React.Fragment key={`template-${index}`}> <TableRow key={`template-${index}-1`}> @@ - {open[index] ? ( - <TableRow key={`template-${index}-open`}> + {open[index] ? ( + <TableRow key={`template-${index}-open`}> @@ - ) : null} - </> + ) : null} + </React.Fragment> ); })}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
applications/tari_walletd/web_ui/src/contexts/ErrorNotificationContext.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Manifest/Manifest.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Templates/Templates.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
applications/tari_walletd/web_ui/src/routes/Templates/Templates.tsx (7)
applications/tari_walletd/web_ui/src/services/api/hooks/useTemplatesAuthored.tsx (1)
useListTemplatesAuthored(8-19)applications/tari_walletd/web_ui/src/utils/helpers.tsx (3)
substateIdToString(100-109)handleChangePage(152-158)handleChangeRowsPerPage(160-167)bindings/src/types/wallet-daemon-client/AccountInfo.ts (1)
AccountInfo(5-5)bindings/src/types/wallet-daemon-client/AuthoredTemplate.ts (1)
AuthoredTemplate(6-12)applications/tari_walletd/web_ui/src/components/CopyAddress.tsx (1)
CopyAddress(32-43)bindings/src/types/FunctionDef.ts (1)
FunctionDef(5-5)bindings/src/types/ArgDef.ts (1)
ArgDef(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check-title
- GitHub Check: check stable
- GitHub Check: file licenses
- GitHub Check: machete
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: check nightly
🔇 Additional comments (2)
applications/tari_walletd/web_ui/src/contexts/ErrorNotificationContext.tsx (1)
91-91: LGTM! Visual consistency improvement.Adding
variant="filled"to the Alert component is a valid MUI styling prop that enhances visual consistency across the UI without affecting any behavior or functionality.applications/tari_walletd/web_ui/src/routes/Manifest/Manifest.tsx (1)
196-223: LGTM! Excellent UI improvements.The conditional rendering of the variables table is a great UX enhancement that prevents displaying an empty table. The complete table structure with proper headers improves accessibility, and the
color="error"for the Remove button is semantically appropriate for a destructive action. The added margin aligns perfectly with the PR's objective of tidying layouts.
3 participants
Description
Just tidied up the margins and layouts of the flow editor for better consistency
Also added ui fixes to Templates and Manifest pages.
Motivation and Context
UI Improvements
How Has This Been Tested?
Manually
What process can a PR reviewer use to test or verify this change?
Open the wallet and go to the flow editor page
Breaking Changes
Summary by CodeRabbit
Refactor
New Features / UX
Bug Fixes
Style