Improve table layouts with consolidated columns and fixed widths #2416
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
Combine Order, Owner, Orderbook columns into single Addresses column with stacked layout to reduce horizontal space usage. Update tests to assert on new combined Addresses cells.
Reduces OrdersListTable from 7 to 5 columns by consolidating metadata into a compact stacked layout with network name and status badge inline.
Apply table-fixed layout to TanstackAppTable and define explicit column widths for Orders and Vaults tables to prevent layout shift. Combine Token name and Balance into a single column in Vaults table.
Combine Sender and Transaction Hash into single Transaction column. Show Input/Output with token symbol and truncated amounts with tooltips. Update tests to cover new column structure.
Combine Type and Date into Info column with colored badge. Add badge colors: green (deposit), yellow (withdrawal), blue (trade), purple (bounty). Format camelCase types to readable labels. Combine Sender and Tx into Transaction column. Update tests to cover badge formatting and new column structure.
Add empty Actions header for consistent column count with table-fixed layout. Move action cell outside conditional to always render. Update test to verify Token column contains both name and balance.
Contributor
WalkthroughThis PR restructures multiple table components (OrderTrades, Orders, VaultBalances, Vaults) to consolidate related information into combined cells with improved visual organization. Updates include: unified Transaction/Addresses cells with labels, compact Input/Output blocks with tooltips, Badge components for type categorization, and explicit column width allocations. Tests are updated to validate the new UI structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
🤖 Fix all issues with AI agents
In `@packages/ui-components/src/__tests__/TanstackAppTable.test.ts`:
- Around line 234-243: The test "applies table-fixed layout for consistent
column widths" should not assert Tailwind class names; update the test (the case
using renderTable(mockQuery) and helpers createPages/createMockQuery) to assert
behavior instead—for example, verify consistent column widths by comparing
getBoundingClientRect widths of header cells and body cells, assert that long
cell content is truncated/ellipsis or that there is no horizontal overflow, or
move the class-presence check into a visual/regression test; remove
expect(table).toHaveClass('table-fixed') and expect(table).toHaveClass('w-full')
and replace them with one of these functional assertions.
In `@packages/ui-components/src/lib/components/tables/OrdersListTable.svelte`:
- Around line 131-145: The table head currently defines five TableHeadCell
headers (Order Info, Addresses, Input Token(s), Output Token(s), Trades) but the
row rendering (where the owner action cell is conditionally rendered) can add an
extra actions cell causing misalignment under table-fixed; update
OrdersListTable.svelte so the head always includes an Actions header (visually
hidden with sr-only) by adding a sixth TableHeadCell (label “Actions” or
sr-only) to match the conditional action cell, and ensure that row rendering for
non-owner rows emits an empty cell (<td> equivalent) where the action cell would
be so column counts remain consistent; identify TableHeadCell usage in the
header block and the conditional action button render in the row template to
implement this.
In
`@packages/ui-components/src/lib/components/tables/OrderTradesListTable.svelte`:
- Around line 103-154: The tooltip anchor IDs currently use item.transaction.id
which can collide across trades; update all ID and Tooltip triggeredBy usages in
OrderTradesListTable.svelte (the span ids and the div id for io-ratio, and their
corresponding Tooltip triggeredBy props) to use a unique trade identifier such
as item.id (or append the row index) instead of item.transaction.id so each
Tooltip binds to the correct row; ensure you change every occurrence: input-...,
output-..., io-ratio-... and their Tooltip triggeredBy values to the new unique
ID.
In
`@packages/ui-components/src/lib/components/tables/VaultBalanceChangesTable.svelte`:
- Around line 91-114: The Tooltip children currently render
{item.formattedAmount}{item.token.symbol} and
{item.formattedNewBalance}{item.token.symbol} which concatenates values (e.g.,
"0.1TKN1"); update both Tooltip usages in VaultBalanceChangesTable.svelte (the
Tooltip triggeredBy for `#change-${item.transaction.id}` and
`#balance-${item.transaction.id}`) to insert a separator (e.g., a space or
non‑breaking space or " ") between item.formattedAmount and item.token.symbol
and likewise between item.formattedNewBalance and item.token.symbol so the
tooltip displays like "0.1 TKN1".
Comment on lines
+234
to
+243
| test('applies table-fixed layout for consistent column widths', async () => { | ||
| const pages = createPages(); | ||
| const mockQuery = createMockQuery(pages); | ||
| renderTable(mockQuery); | ||
|
|
||
| await waitFor(() => { | ||
| const table = document.querySelector('table'); | ||
| expect(table).toHaveClass('table-fixed'); | ||
| expect(table).toHaveClass('w-full'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid asserting Tailwind class names in unit tests.
Line 240-242 checks table-fixed/w-full, which couples tests to styling. Prefer functional assertions or move this to visual/regression testing. Based on learnings, focus UI unit tests on behavior rather than CSS class presence.
🤖 Prompt for AI Agents
In `@packages/ui-components/src/__tests__/TanstackAppTable.test.ts` around lines
234 - 243, The test "applies table-fixed layout for consistent column widths"
should not assert Tailwind class names; update the test (the case using
renderTable(mockQuery) and helpers createPages/createMockQuery) to assert
behavior instead—for example, verify consistent column widths by comparing
getBoundingClientRect widths of header cells and body cells, assert that long
cell content is truncated/ellipsis or that there is no horizontal overflow, or
move the class-presence check into a visual/regression test; remove
expect(table).toHaveClass('table-fixed') and expect(table).toHaveClass('w-full')
and replace them with one of these functional assertions.
Comment on lines
+131
to
+145
| <TableHeadCell data-testid="orderListHeadingOrderInfo" padding="p-4" class="w-[15%]" | ||
| >Order Info</TableHeadCell | ||
| > | ||
| <TableHeadCell data-testid="orderListHeadingAddresses" padding="p-4" class="w-[20%]" | ||
| >Addresses</TableHeadCell | ||
| > | ||
| <TableHeadCell data-testid="orderListHeadingInputs" padding="px-2 py-4" class="w-[27.5%]" | ||
| >Input Token(s)</TableHeadCell | ||
| > | ||
| <TableHeadCell data-testid="orderListHeadingOutputs" padding="px-2 py-4" | ||
| <TableHeadCell data-testid="orderListHeadingOutputs" padding="px-2 py-4" class="w-[27.5%]" | ||
| >Output Token(s)</TableHeadCell | ||
| > | ||
| <TableHeadCell data-testid="orderListHeadingTrades" padding="px-2 py-4">Trades</TableHeadCell> | ||
| <TableHeadCell data-testid="orderListHeadingTrades" padding="px-2 py-4" class="w-[10%]" | ||
| >Trades</TableHeadCell | ||
| > |
Contributor
There was a problem hiding this comment.
Add an Actions header to keep table-fixed columns aligned.
Line 131-145 defines 5 headers, but Line 210 can render an extra action cell for owner rows. With table-fixed, mixed column counts can misalign columns. Consider adding an Actions header (sr-only) and rendering an empty Actions cell when actions aren’t available, or move the action button into an existing column.
🤖 Prompt for AI Agents
In `@packages/ui-components/src/lib/components/tables/OrdersListTable.svelte`
around lines 131 - 145, The table head currently defines five TableHeadCell
headers (Order Info, Addresses, Input Token(s), Output Token(s), Trades) but the
row rendering (where the owner action cell is conditionally rendered) can add an
extra actions cell causing misalignment under table-fixed; update
OrdersListTable.svelte so the head always includes an Actions header (visually
hidden with sr-only) by adding a sixth TableHeadCell (label “Actions” or
sr-only) to match the conditional action cell, and ensure that row rendering for
non-owner rows emits an empty cell (<td> equivalent) where the action cell would
be so column counts remain consistent; identify TableHeadCell usage in the
header block and the conditional action button render in the row template to
implement this.
Comment on lines
+103
to
154
| <TableBodyCell tdClass="p-2" data-testid="input"> | ||
| <div class="flex flex-col overflow-hidden"> | ||
| <span class="truncate font-medium">{item.inputVaultBalanceChange.token.symbol}</span> | ||
| <span | ||
| id={`input-${item.transaction.id}`} | ||
| class="truncate text-sm text-gray-500 dark:text-gray-400" | ||
| >{item.inputVaultBalanceChange.formattedAmount}</span | ||
| > | ||
| <Tooltip triggeredBy={`#input-${item.transaction.id}`}> | ||
| {item.inputVaultBalanceChange.formattedAmount} | ||
| {item.inputVaultBalanceChange.token.symbol} | ||
| </Tooltip> | ||
| </div> | ||
| </TableBodyCell> | ||
| <TableBodyCell tdClass="break-all py-2"> | ||
| {item.outputVaultBalanceChange.formattedAmount} | ||
| {item.outputVaultBalanceChange.token.symbol} | ||
| <TableBodyCell tdClass="p-2" data-testid="output"> | ||
| <div class="flex flex-col overflow-hidden"> | ||
| <span class="truncate font-medium">{item.outputVaultBalanceChange.token.symbol}</span> | ||
| <span | ||
| id={`output-${item.transaction.id}`} | ||
| class="truncate text-sm text-gray-500 dark:text-gray-400" | ||
| >{item.outputVaultBalanceChange.formattedAmount}</span | ||
| > | ||
| <Tooltip triggeredBy={`#output-${item.transaction.id}`}> | ||
| {item.outputVaultBalanceChange.formattedAmount} | ||
| {item.outputVaultBalanceChange.token.symbol} | ||
| </Tooltip> | ||
| </div> | ||
| </TableBodyCell> | ||
| <TableBodyCell tdClass="break-all py-2" data-testid="io-ratio"> | ||
| {Math.abs( | ||
| Number(item.inputVaultBalanceChange.formattedAmount) / | ||
| Number(item.outputVaultBalanceChange.formattedAmount) | ||
| )} | ||
| <span class="text-gray-400"> | ||
| <TableBodyCell tdClass="p-2" data-testid="io-ratio"> | ||
| <div id={`io-ratio-${item.transaction.id}`} class="truncate"> | ||
| {Math.abs( | ||
| Number(item.inputVaultBalanceChange.formattedAmount) / | ||
| Number(item.outputVaultBalanceChange.formattedAmount) | ||
| )} | ||
| <span class="text-gray-400"> | ||
| ({Math.abs( | ||
| Number(item.outputVaultBalanceChange.formattedAmount) / | ||
| Number(item.inputVaultBalanceChange.formattedAmount) | ||
| )}) | ||
| </span> | ||
| </div> | ||
| <Tooltip triggeredBy={`#io-ratio-${item.transaction.id}`}> | ||
| {Math.abs( | ||
| Number(item.inputVaultBalanceChange.formattedAmount) / | ||
| Number(item.outputVaultBalanceChange.formattedAmount) | ||
| )} | ||
| ({Math.abs( | ||
| Number(item.outputVaultBalanceChange.formattedAmount) / | ||
| Number(item.inputVaultBalanceChange.formattedAmount) | ||
| )}) | ||
| </span> | ||
| </Tooltip> | ||
| </TableBodyCell> |
Contributor
There was a problem hiding this comment.
Ensure tooltip anchor IDs are unique per trade.
Line 107/121/132 derive element IDs from item.transaction.id; if multiple trades share a transaction hash, IDs collide and tooltips bind to the first row only. Use item.id (or include row index) to keep anchors unique.
🔧 Proposed fix (use trade id for unique anchors)
- <span
- id={`input-${item.transaction.id}`}
+ <span
+ id={`input-${item.id}`}
class="truncate text-sm text-gray-500 dark:text-gray-400"
>{item.inputVaultBalanceChange.formattedAmount}</span
>
- <Tooltip triggeredBy={`#input-${item.transaction.id}`}>
+ <Tooltip triggeredBy={`#input-${item.id}`}>
{item.inputVaultBalanceChange.formattedAmount}
{item.inputVaultBalanceChange.token.symbol}
</Tooltip>
</div>
@@
- <span
- id={`output-${item.transaction.id}`}
+ <span
+ id={`output-${item.id}`}
class="truncate text-sm text-gray-500 dark:text-gray-400"
>{item.outputVaultBalanceChange.formattedAmount}</span
>
- <Tooltip triggeredBy={`#output-${item.transaction.id}`}>
+ <Tooltip triggeredBy={`#output-${item.id}`}>
{item.outputVaultBalanceChange.formattedAmount}
{item.outputVaultBalanceChange.token.symbol}
</Tooltip>
</div>
@@
- <div id={`io-ratio-${item.transaction.id}`} class="truncate">
+ <div id={`io-ratio-${item.id}`} class="truncate">
{Math.abs(
Number(item.inputVaultBalanceChange.formattedAmount) /
Number(item.outputVaultBalanceChange.formattedAmount)
)}
<span class="text-gray-400">
({Math.abs(
Number(item.outputVaultBalanceChange.formattedAmount) /
Number(item.inputVaultBalanceChange.formattedAmount)
)})
</span>
</div>
- <Tooltip triggeredBy={`#io-ratio-${item.transaction.id}`}>
+ <Tooltip triggeredBy={`#io-ratio-${item.id}`}>
{Math.abs(
Number(item.inputVaultBalanceChange.formattedAmount) /
Number(item.outputVaultBalanceChange.formattedAmount)
)}
({Math.abs(
Number(item.outputVaultBalanceChange.formattedAmount) /
Number(item.inputVaultBalanceChange.formattedAmount)
)})
</Tooltip>🤖 Prompt for AI Agents
In `@packages/ui-components/src/lib/components/tables/OrderTradesListTable.svelte`
around lines 103 - 154, The tooltip anchor IDs currently use item.transaction.id
which can collide across trades; update all ID and Tooltip triggeredBy usages in
OrderTradesListTable.svelte (the span ids and the div id for io-ratio, and their
corresponding Tooltip triggeredBy props) to use a unique trade identifier such
as item.id (or append the row index) instead of item.transaction.id so each
Tooltip binds to the correct row; ensure you change every occurrence: input-...,
output-..., io-ratio-... and their Tooltip triggeredBy values to the new unique
ID.
Comment on lines
+91
to
+114
| <TableBodyCell tdClass="p-2" data-testid="vaultBalanceChangesTableBalanceChange"> | ||
| <div class="flex flex-col overflow-hidden"> | ||
| <span class="truncate font-medium">{item.token.symbol}</span> | ||
| <span | ||
| id={`change-${item.transaction.id}`} | ||
| class="truncate text-sm text-gray-500 dark:text-gray-400">{item.formattedAmount}</span | ||
| > | ||
| <Tooltip triggeredBy={`#change-${item.transaction.id}`}> | ||
| {item.formattedAmount} | ||
| {item.token.symbol} | ||
| </Tooltip> | ||
| </div> | ||
| </TableBodyCell> | ||
| <TableBodyCell tdClass="break-word p-0 text-left" data-testid="vaultBalanceChangesTableType"> | ||
| {item.type} | ||
| <TableBodyCell tdClass="p-2" data-testid="vaultBalanceChangesTableBalance"> | ||
| <div class="flex flex-col overflow-hidden"> | ||
| <span class="truncate font-medium">{item.token.symbol}</span> | ||
| <span | ||
| id={`balance-${item.transaction.id}`} | ||
| class="truncate text-sm text-gray-500 dark:text-gray-400">{item.formattedNewBalance}</span | ||
| > | ||
| <Tooltip triggeredBy={`#balance-${item.transaction.id}`}> | ||
| {item.formattedNewBalance} | ||
| {item.token.symbol} | ||
| </Tooltip> |
Contributor
There was a problem hiding this comment.
Tooltip text concatenates amount and symbol. Add a separator to avoid “0.1TKN1”-style output.
💡 Suggested fix
- <Tooltip triggeredBy={`#change-${item.transaction.id}`}>
- {item.formattedAmount}
- {item.token.symbol}
- </Tooltip>
+ <Tooltip triggeredBy={`#change-${item.transaction.id}`}>
+ {item.formattedAmount} {item.token.symbol}
+ </Tooltip>
...
- <Tooltip triggeredBy={`#balance-${item.transaction.id}`}>
- {item.formattedNewBalance}
- {item.token.symbol}
- </Tooltip>
+ <Tooltip triggeredBy={`#balance-${item.transaction.id}`}>
+ {item.formattedNewBalance} {item.token.symbol}
+ </Tooltip>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <TableBodyCell tdClass="p-2" data-testid="vaultBalanceChangesTableBalanceChange"> | |
| <div class="flex flex-col overflow-hidden"> | |
| <span class="truncate font-medium">{item.token.symbol}</span> | |
| <span | |
| id={`change-${item.transaction.id}`} | |
| class="truncate text-sm text-gray-500 dark:text-gray-400">{item.formattedAmount}</span | |
| > | |
| <Tooltip triggeredBy={`#change-${item.transaction.id}`}> | |
| {item.formattedAmount} | |
| {item.token.symbol} | |
| </Tooltip> | |
| </div> | |
| </TableBodyCell> | |
| <TableBodyCell tdClass="break-word p-0 text-left" data-testid="vaultBalanceChangesTableType"> | |
| {item.type} | |
| <TableBodyCell tdClass="p-2" data-testid="vaultBalanceChangesTableBalance"> | |
| <div class="flex flex-col overflow-hidden"> | |
| <span class="truncate font-medium">{item.token.symbol}</span> | |
| <span | |
| id={`balance-${item.transaction.id}`} | |
| class="truncate text-sm text-gray-500 dark:text-gray-400">{item.formattedNewBalance}</span | |
| > | |
| <Tooltip triggeredBy={`#balance-${item.transaction.id}`}> | |
| {item.formattedNewBalance} | |
| {item.token.symbol} | |
| </Tooltip> | |
| <TableBodyCell tdClass="p-2" data-testid="vaultBalanceChangesTableBalanceChange"> | |
| <div class="flex flex-col overflow-hidden"> | |
| <span class="truncate font-medium">{item.token.symbol}</span> | |
| <span | |
| id={`change-${item.transaction.id}`} | |
| class="truncate text-sm text-gray-500 dark:text-gray-400">{item.formattedAmount}</span | |
| > | |
| <Tooltip triggeredBy={`#change-${item.transaction.id}`}> | |
| {item.formattedAmount} {item.token.symbol} | |
| </Tooltip> | |
| </div> | |
| </TableBodyCell> | |
| <TableBodyCell tdClass="p-2" data-testid="vaultBalanceChangesTableBalance"> | |
| <div class="flex flex-col overflow-hidden"> | |
| <span class="truncate font-medium">{item.token.symbol}</span> | |
| <span | |
| id={`balance-${item.transaction.id}`} | |
| class="truncate text-sm text-gray-500 dark:text-gray-400">{item.formattedNewBalance}</span | |
| > | |
| <Tooltip triggeredBy={`#balance-${item.transaction.id}`}> | |
| {item.formattedNewBalance} {item.token.symbol} | |
| </Tooltip> |
🤖 Prompt for AI Agents
In
`@packages/ui-components/src/lib/components/tables/VaultBalanceChangesTable.svelte`
around lines 91 - 114, The Tooltip children currently render
{item.formattedAmount}{item.token.symbol} and
{item.formattedNewBalance}{item.token.symbol} which concatenates values (e.g.,
"0.1TKN1"); update both Tooltip usages in VaultBalanceChangesTable.svelte (the
Tooltip triggeredBy for `#change-${item.transaction.id}` and
`#balance-${item.transaction.id}`) to insert a separator (e.g., a space or
non‑breaking space or " ") between item.formattedAmount and item.token.symbol
and likewise between item.formattedNewBalance and item.token.symbol so the
tooltip displays like "0.1 TKN1".
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.
Motivation
The list and detail tables in the UI had several layout issues:
Solution
Consolidated columns and improved table layouts across all major table components:
OrdersListTable:
VaultsListTable:
OrderTradesListTable:
VaultBalanceChangesTable:
TanstackAppTable:
table-fixedclass with explicit percentage-based column widths to prevent column jumpingUpdated tests to cover new column structure and badge formatting.
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Release Notes
Tests
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.