Skip to content

Commit 4b4ab24

Browse files
authored
improvement(menus): one separator per menu, before the destructive action (#6974)
* improvement(menus): one separator per menu, before the destructive action Menus banded themselves into semantic groups — navigation, status, edit, copy, destructive — behind two to four separators each. No toolbar in the app renders a divider: every header is a flat gap-1 chip row and every bulk action bar a flat gap-[5px] run. The bands therefore taught a taxonomy the user met on no other surface, and because each band is conditional, the same action landed in a different group depending on which siblings happened to be visible. Pin sat alone in one caller of the shared workflow menu and beside Duplicate in another. Every menu now carries at most one rule, immediately before the destructive group. Order is untouched, so the toolbar-mirroring the ordering rule requires is unaffected. Two separator bugs fixed. The logs row menu had two unconditional separators above conditional items, so a log already filtered by its workflow with no active filters ended on a dangling rule. The shared workflow menu guarded its destructive rule on showLeave alone while the Leave item required showLeave && onLeave, so a caller passing showLeave from a permission check with a conditional onLeave would trail a rule under the last item; every term in both guards is now the exact render condition of the item it stands for. Removes groupNonDestructiveActions and separateNavigationAction. Between them they moved one separator for one caller, four of six branches were unreachable, and separateNavigationAction had no observable effect anywhere in the repo. The separator matrix was previously untested, which is how the showLeave asymmetry survived; it now has invariants including a flag sweep. * fix(menus): build every separator guard from its items' exact conditions Review caught two places where the grouping rule and the code disagreed. The tables row menu guarded its rule on `onMove` while the Move submenu needs a non-empty `moveOptions`, so a table whose other actions were all absent and whose move list was empty would draw the rule with nothing above it — the exact looseness the rule warns about. The logs row menu puts its one rule after Retry and Cancel Run rather than before a destructive group, which the rule as written did not cover. Retry is the primary action on a failed run and belongs at the top; the rule now describes the separator as fencing the consequential group at whichever end it sits, and names the logs menu as the one place that group leads. Also aligns three empty-space menu labels with the header chips they mirror: "Add document" and "Create chunk" were the only create actions not matching their toolbar, and the files menu said "Upload file" where its header says "Upload". Run order in the two column run menus now matches the action bar and the row menu — incomplete before all, not all before incomplete. * fix(menus): apply the one-rule grouping to the folder context menu The folder row menu kept a separator at its canEdit permission boundary plus one before Delete — the same shape already corrected in the file row menu, missed because the sweep that found it did not cover this file. Open and Pin above are unconditional, so the surviving rule is always backed on both sides. Also records the standing exception the sweep surfaced: the text editor, terminal, and browser page menus emulate native OS menus, whose banding the user learns outside Sim. That is the ordering rule's own principle — mirror the surface they already read — so those keep their banding while our own resource and row menus, whose toolbars are flat, take one rule. * fix(menus): keep empty-row actions together
1 parent d1b0184 commit 4b4ab24

15 files changed

Lines changed: 269 additions & 126 deletions

File tree

.claude/rules/sim-list-ordering.md

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,71 @@ Left-to-right becomes top-to-bottom. A toolbar reading `Filter · Sort · Export
2626

2727
Platform-only entries (desktop **Browser** and **Terminal**) trail the shared set rather than interleaving, so the common prefix is identical on every platform.
2828

29+
## Grouping: one rule, against the consequential group
30+
31+
Order is governed above. **Separators are governed here** — and the answer is: use at most one.
32+
33+
Put a single `DropdownMenuSeparator` against the **consequential group** — the actions that
34+
delete, detach, or change a run — and nowhere else. Everything on the other side of it runs
35+
uninterrupted in toolbar-mirroring order.
36+
37+
That group trails in almost every menu, so in practice the rule reads "one rule immediately
38+
before Delete / Leave / Close / Hide". It leads in exactly one place: the **logs row menu**,
39+
where `Retry` and `Cancel Run` are the primary actions on a failed run and sit at the top, with
40+
the rule beneath them. Ordering follows the surface (see "The rule" above); the separator simply
41+
fences whichever end the consequential group occupies. A menu whose consequential actions are
42+
merely *disabled* still gets no extra rule — `disabled` is not a group.
43+
44+
```tsx
45+
// ✗ Bad — four semantic bands the user meets nowhere else
46+
Open in new tab │─── Rename, Lock │─── Duplicate, Export │─── Delete
47+
48+
// ✓ Good — one rule, isolating the irreversible action
49+
Open in new tab, Rename, Lock, Duplicate, Export │─── Delete
50+
```
51+
52+
**Why one.** No toolbar in this app renders a divider — every header is a flat
53+
`HEADER_ACTION_CLUSTER` (`gap-1`) chip row and every bulk action bar a flat `gap-[5px]` run. A
54+
menu banded into navigation / status / edit / copy / destructive therefore teaches a taxonomy
55+
that appears on no other surface, and because each band is conditional, the same action lands in
56+
a different group depending on which sibling items happen to be visible. The one thing a rule
57+
genuinely buys is a stop before the action you cannot undo.
58+
59+
A second rule is justified only when a menu mixes genuinely different *scopes* — cell-level and
60+
table-level actions in one menu, say — not different verbs.
61+
62+
**The one standing exception: menus that emulate a native menu.** The text-editor menu
63+
(`editor-context-menu.tsx`), the terminal menu (`terminal-context-menu.tsx`), and the browser
64+
page menu (`browser-session.tsx`) each mirror the OS menu the user already knows — clipboard
65+
banding (`Cut · Copy · Paste │ Select all`) is a convention every text field on their machine
66+
teaches them. These keep their native banding, and that is the *same* principle as the ordering
67+
rule above: mirror the surface the user already reads. The test is whether a real menu outside
68+
Sim taught them the grouping. Our own resource, row, and action menus have no such precedent —
69+
the toolbars they mirror are flat — so they take the single rule.
70+
71+
**Both sides of every rule must be guaranteed non-empty.** Write the separator's guard out of
72+
the *exact* render conditions of the items around it, never a looser approximation:
73+
74+
```tsx
75+
// ✗ Bad — `showLeave` alone, while the Leave item needs `showLeave && onLeave`.
76+
// A caller passing showLeave from a permission check with a conditional
77+
// onLeave renders a trailing rule under the last item.
78+
{hasActionsAbove && (showLeave || showDelete) && <DropdownMenuSeparator />}
79+
80+
// ✓ Good — each term is the item's own condition, verbatim
81+
const hasDestructiveSection = (showLeave && onLeave) || showDelete
82+
{hasActionsAboveDestructive && hasDestructiveSection && <DropdownMenuSeparator />}
83+
```
84+
85+
This is the failure that put a dangling rule at the bottom of the logs row menu, where two
86+
unconditional separators sat above conditional items.
87+
88+
**Do not add a prop to move a rule.** The shared workflow context menu grew
89+
`groupNonDestructiveActions` and `separateNavigationAction` for this; between them they moved one
90+
separator for one caller, four of six branches were unreachable, and `separateNavigationAction`
91+
had no observable effect anywhere in the repo. Both are gone. A menu that wants different
92+
grouping wants the standard grouping.
93+
2994
## Encode the order once
3095

3196
An order duplicated across surfaces is an order that will drift. Export **one** constant and sort by it — do not hand-maintain a matching literal per menu.

CLAUDE.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,9 @@ Co-locate a `search-params.ts` per feature exporting the parser map (single sour
386386

387387
A list orders itself the way the user already reads the same things somewhere else. Resource menus (`+` attach, `@` mention, resource-tab `+`) mirror the **sidebar** top-down; a row or root **context menu** mirrors that surface's **toolbar**, left-to-right becoming top-to-bottom; tab strips mirror their nav. Platform-only entries (desktop Browser, Terminal) trail the shared set.
388388

389-
Encode the order in ONE exported constant and sort by it — never a hand-maintained literal per menu (`RESOURCE_MENU_ORDER` / `byResourceMenuOrder` in `home/components/mothership-view/components/resource-registry`). Render mixed item kinds in a single ordered pass; emitting all submenu-backed families and then all flat ones silently pins every submenu to the top no matter what the constant says. Divergence is allowed only for search ranking, user-controlled ordering, and recency. Full rule in `.claude/rules/sim-list-ordering.md`.
389+
Encode the order in ONE exported constant and sort by it — never a hand-maintained literal per menu (`RESOURCE_MENU_ORDER` / `byResourceMenuOrder` in `home/components/mothership-view/components/resource-registry`). Render mixed item kinds in a single ordered pass; emitting all submenu-backed families and then all flat ones silently pins every submenu to the top no matter what the constant says. Divergence is allowed only for search ranking, user-controlled ordering, and recency.
390+
391+
**Grouping**: at most ONE `DropdownMenuSeparator` per menu, fencing the consequential group — immediately before Delete/Leave/Close/Hide in almost every menu, and immediately after Retry/Cancel Run in the logs row menu, where those lead. No toolbar in the app renders a divider, so multi-band menus teach a taxonomy that exists on no other surface. Build each separator's guard from the EXACT render conditions of the items on both sides — a looser guard is what leaves a dangling rule when its group is conditional. Never add a prop to move a rule. Full rule in `.claude/rules/sim-list-ordering.md`.
390392

391393
## Styling
392394

apps/sim/app/workspace/[workspaceId]/components/folders/folder-context-menu.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@ export const FolderContextMenu = memo(function FolderContextMenu({
9191
)}
9292
{canEdit && (
9393
<>
94-
<DropdownMenuSeparator />
9594
<DropdownMenuItem onSelect={onRename}>
9695
<Pencil />
9796
Rename

apps/sim/app/workspace/[workspaceId]/files/components/file-row-context-menu/file-row-context-menu.tsx

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,16 @@ export const FileRowContextMenu = memo(function FileRowContextMenu({
5555
}: FileRowContextMenuProps) {
5656
const isMultiSelect = selectedCount > 1
5757

58+
/**
59+
* Everything that can render above `Delete`: Open/Pin need a single selection,
60+
* Download needs its handler, and the edit trio needs `canEdit` — so a multi-select
61+
* with no download and only a move target leaves `Move to` alone above the rule.
62+
*
63+
* @see `.claude/rules/sim-list-ordering.md` — one rule, before the destructive group.
64+
*/
65+
const hasActionsAboveDestructive =
66+
!isMultiSelect || !!onDownload || (!!onMove && !!moveOptions && moveOptions.length > 0)
67+
5868
return (
5969
<DropdownMenu open={isOpen} onOpenChange={(open) => !open && onClose()} modal={false}>
6070
<DropdownMenuTrigger asChild>
@@ -91,7 +101,6 @@ export const FileRowContextMenu = memo(function FileRowContextMenu({
91101
)}
92102
{canEdit && (
93103
<>
94-
<DropdownMenuSeparator />
95104
{!isMultiSelect && (
96105
<DropdownMenuItem onSelect={onRename}>
97106
<Pencil />
@@ -120,6 +129,7 @@ export const FileRowContextMenu = memo(function FileRowContextMenu({
120129
</DropdownMenuSubContent>
121130
</DropdownMenuSub>
122131
)}
132+
{hasActionsAboveDestructive && <DropdownMenuSeparator />}
123133
<DropdownMenuItem onSelect={onDelete}>
124134
<Trash />
125135
{isMultiSelect ? `Delete ${selectedCount} items` : 'Delete'}

apps/sim/app/workspace/[workspaceId]/files/components/files-list-context-menu/files-list-context-menu.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export const FilesListContextMenu = memo(function FilesListContextMenu({
4848
{onUploadFile && (
4949
<DropdownMenuItem disabled={disableUpload} onSelect={onUploadFile}>
5050
<Upload />
51-
Upload file
51+
Upload
5252
</DropdownMenuItem>
5353
)}
5454
{onCreateFolder && (

apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/browser-tab-strip.tsx

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -216,9 +216,7 @@ export function BrowserTabStrip({
216216
onOpenInNewTab={openTabInExternalBrowser}
217217
openInNewTabLabel='Open in External Browser'
218218
openInNewTabPosition='last'
219-
separateNavigationAction
220219
showOpenInNewTab={Boolean(contextTab?.url && contextTab.url !== 'about:blank')}
221-
groupNonDestructiveActions
222220
onTogglePin={
223221
contextTab ? () => onSetTabPinned(contextTab.tabId, !contextTab.pinned) : undefined
224222
}

apps/sim/app/workspace/[workspaceId]/knowledge/[id]/[documentId]/components/chunk-context-menu/chunk-context-menu.tsx

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ interface ChunkContextMenuProps {
3333

3434
/**
3535
* Context menu for chunks table.
36-
* Shows chunk actions when right-clicking a row, or "Create chunk" when right-clicking empty space.
36+
* Shows chunk actions when right-clicking a row, or "New chunk" when right-clicking empty space.
3737
* Supports batch operations when multiple chunks are selected.
3838
*/
3939
export function ChunkContextMenu({
@@ -71,6 +71,7 @@ export function ChunkContextMenu({
7171
const hasEditSection = !isMultiSelect && (!!onEdit || !!onCopyContent)
7272
const hasStateSection = !!onToggleEnabled
7373
const hasDestructiveSection = !!onDelete
74+
const hasActionsAboveDestructive = hasNavigationSection || hasEditSection || hasStateSection
7475

7576
return (
7677
<DropdownMenu open={isOpen} onOpenChange={(open) => !open && onClose()} modal={false}>
@@ -102,11 +103,6 @@ export function ChunkContextMenu({
102103
Open in new tab
103104
</DropdownMenuItem>
104105
)}
105-
{hasNavigationSection &&
106-
(hasEditSection || hasStateSection || hasDestructiveSection) && (
107-
<DropdownMenuSeparator />
108-
)}
109-
110106
{!isMultiSelect && onEdit && (
111107
<DropdownMenuItem disabled={disableEdit} onSelect={onEdit}>
112108
<Pencil />
@@ -119,18 +115,14 @@ export function ChunkContextMenu({
119115
Copy content
120116
</DropdownMenuItem>
121117
)}
122-
{hasEditSection && (hasStateSection || hasDestructiveSection) && (
123-
<DropdownMenuSeparator />
124-
)}
125-
126118
{onToggleEnabled && (
127119
<DropdownMenuItem disabled={disableToggleEnabled} onSelect={onToggleEnabled}>
128120
<Eye />
129121
{getToggleLabel()}
130122
</DropdownMenuItem>
131123
)}
132124

133-
{hasStateSection && hasDestructiveSection && <DropdownMenuSeparator />}
125+
{hasActionsAboveDestructive && hasDestructiveSection && <DropdownMenuSeparator />}
134126
{onDelete && (
135127
<DropdownMenuItem disabled={disableDelete} onSelect={onDelete}>
136128
<Trash />
@@ -142,7 +134,7 @@ export function ChunkContextMenu({
142134
onAddChunk && (
143135
<DropdownMenuItem disabled={disableAddChunk} onSelect={onAddChunk}>
144136
<Plus />
145-
Create chunk
137+
New chunk
146138
</DropdownMenuItem>
147139
)
148140
)}

apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/document-context-menu/document-context-menu.tsx

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ export function DocumentContextMenu({
7171
const hasEditSection = !isMultiSelect && (!!onRename || !!onViewTags)
7272
const hasStateSection = !!onToggleEnabled
7373
const hasDestructiveSection = !!onDelete
74+
const hasActionsAboveDestructive = hasNavigationSection || hasEditSection || hasStateSection
7475

7576
return (
7677
<DropdownMenu open={isOpen} onOpenChange={(open) => !open && onClose()} modal={false}>
@@ -108,11 +109,6 @@ export function DocumentContextMenu({
108109
Open source
109110
</DropdownMenuItem>
110111
)}
111-
{hasNavigationSection &&
112-
(hasEditSection || hasStateSection || hasDestructiveSection) && (
113-
<DropdownMenuSeparator />
114-
)}
115-
116112
{!isMultiSelect && onRename && (
117113
<DropdownMenuItem disabled={disableRename} onSelect={onRename}>
118114
<Pencil />
@@ -125,18 +121,14 @@ export function DocumentContextMenu({
125121
Tags
126122
</DropdownMenuItem>
127123
)}
128-
{hasEditSection && (hasStateSection || hasDestructiveSection) && (
129-
<DropdownMenuSeparator />
130-
)}
131-
132124
{onToggleEnabled && (
133125
<DropdownMenuItem disabled={disableToggleEnabled} onSelect={onToggleEnabled}>
134126
<Eye />
135127
{getToggleLabel()}
136128
</DropdownMenuItem>
137129
)}
138130

139-
{hasStateSection && hasDestructiveSection && <DropdownMenuSeparator />}
131+
{hasActionsAboveDestructive && hasDestructiveSection && <DropdownMenuSeparator />}
140132
{onDelete && (
141133
<DropdownMenuItem disabled={disableDelete} onSelect={onDelete}>
142134
<Trash />
@@ -148,7 +140,7 @@ export function DocumentContextMenu({
148140
onAddDocument && (
149141
<DropdownMenuItem disabled={disableAddDocument} onSelect={onAddDocument}>
150142
<Plus />
151-
Add document
143+
New documents
152144
</DropdownMenuItem>
153145
)
154146
)}

apps/sim/app/workspace/[workspaceId]/knowledge/components/knowledge-base-context-menu/knowledge-base-context-menu.tsx

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ export const KnowledgeBaseContextMenu = memo(function KnowledgeBaseContextMenu({
7575
const hasMoveSection = !disableEdit && !!onMove && !!moveOptions && moveOptions.length > 0
7676
const hasEditSection = (showEdit && !!onEdit) || hasMoveSection
7777
const hasDestructiveSection = showDelete && !!onDelete
78+
const hasActionsAboveDestructive = hasNavigationSection || hasInfoSection || hasEditSection
7879

7980
return (
8081
<DropdownMenu open={isOpen} onOpenChange={(open) => !open && onClose()} modal={false}>
@@ -104,10 +105,6 @@ export const KnowledgeBaseContextMenu = memo(function KnowledgeBaseContextMenu({
104105
Open in new tab
105106
</DropdownMenuItem>
106107
)}
107-
{hasNavigationSection && (hasInfoSection || hasEditSection || hasDestructiveSection) && (
108-
<DropdownMenuSeparator />
109-
)}
110-
111108
{showViewTags && onViewTags && (
112109
<DropdownMenuItem onSelect={onViewTags}>
113110
<TagIcon />
@@ -126,8 +123,6 @@ export const KnowledgeBaseContextMenu = memo(function KnowledgeBaseContextMenu({
126123
{pinned ? 'Unpin' : 'Pin'}
127124
</DropdownMenuItem>
128125
)}
129-
{hasInfoSection && (hasEditSection || hasDestructiveSection) && <DropdownMenuSeparator />}
130-
131126
{showEdit && onEdit && (
132127
<DropdownMenuItem disabled={disableEdit} onSelect={onEdit}>
133128
<Pencil />
@@ -147,7 +142,7 @@ export const KnowledgeBaseContextMenu = memo(function KnowledgeBaseContextMenu({
147142
</DropdownMenuSub>
148143
)}
149144

150-
{hasEditSection && hasDestructiveSection && <DropdownMenuSeparator />}
145+
{hasActionsAboveDestructive && hasDestructiveSection && <DropdownMenuSeparator />}
151146
{showDelete && onDelete && (
152147
<DropdownMenuItem disabled={disableDelete} onSelect={onDelete}>
153148
<Trash />

apps/sim/app/workspace/[workspaceId]/logs/components/log-row-context-menu/log-row-context-menu.tsx

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -103,23 +103,18 @@ export const LogRowContextMenu = memo(function LogRowContextMenu({
103103
onCloseAutoFocus={(e) => e.preventDefault()}
104104
>
105105
{isRetryable && (
106-
<>
107-
<DropdownMenuItem onSelect={onRetryExecution} disabled={isRetryPending}>
108-
<Redo />
109-
{isRetryPending ? 'Retrying...' : 'Retry'}
110-
</DropdownMenuItem>
111-
<DropdownMenuSeparator />
112-
</>
106+
<DropdownMenuItem onSelect={onRetryExecution} disabled={isRetryPending}>
107+
<Redo />
108+
{isRetryPending ? 'Retrying...' : 'Retry'}
109+
</DropdownMenuItem>
113110
)}
114111
{showCancelAction && (
115-
<>
116-
<DropdownMenuItem onSelect={onCancelExecution} disabled={isStopping}>
117-
<X />
118-
{isStopping ? 'Stopping…' : 'Cancel Run'}
119-
</DropdownMenuItem>
120-
<DropdownMenuSeparator />
121-
</>
112+
<DropdownMenuItem onSelect={onCancelExecution} disabled={isStopping}>
113+
<X />
114+
{isStopping ? 'Stopping…' : 'Cancel Run'}
115+
</DropdownMenuItem>
122116
)}
117+
{(isRetryable || showCancelAction) && <DropdownMenuSeparator />}
123118
<DropdownMenuItem disabled={!hasExecutionId} onSelect={onCopyExecutionId}>
124119
<Duplicate />
125120
Copy Run ID
@@ -128,8 +123,6 @@ export const LogRowContextMenu = memo(function LogRowContextMenu({
128123
<Link />
129124
Copy Link
130125
</DropdownMenuItem>
131-
132-
<DropdownMenuSeparator />
133126
<DropdownMenuItem disabled={!hasOpenableWorkflow} onSelect={onOpenWorkflow}>
134127
<SquareArrowUpRight />
135128
Open Workflow
@@ -138,8 +131,6 @@ export const LogRowContextMenu = memo(function LogRowContextMenu({
138131
<Eye />
139132
Open Snapshot
140133
</DropdownMenuItem>
141-
142-
<DropdownMenuSeparator />
143134
{!isFilteredByThisWorkflow && (
144135
<DropdownMenuItem disabled={!hasWorkflow} onSelect={onToggleWorkflowFilter}>
145136
<ListFilter />

0 commit comments

Comments
 (0)