Skip to content

Commit e929bcf

Browse files
committed
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.
1 parent 6b9e5ea commit e929bcf

8 files changed

Lines changed: 47 additions & 20 deletions

File tree

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,20 @@ 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, before the destructive action
29+
## Grouping: one rule, against the consequential group
3030

3131
Order is governed above. **Separators are governed here** — and the answer is: use at most one.
3232

33-
Put a single `DropdownMenuSeparator` immediately before the destructive group (Delete, Leave,
34-
Close, Hide) and nowhere else. Everything above it runs uninterrupted in toolbar-mirroring order.
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.
3543

3644
```tsx
3745
// ✗ Bad — four semantic bands the user meets nowhere else

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,7 @@ A list orders itself the way the user already reads the same things somewhere el
388388

389389
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.
390390

391-
**Grouping**: at most ONE `DropdownMenuSeparator` per menu, immediately before the destructive group (Delete/Leave/Close/Hide). 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`.
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`.
392392

393393
## Styling
394394

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]/knowledge/[id]/[documentId]/components/chunk-context-menu/chunk-context-menu.tsx

Lines changed: 2 additions & 2 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({
@@ -135,7 +135,7 @@ export function ChunkContextMenu({
135135
onAddChunk && (
136136
<DropdownMenuItem disabled={disableAddChunk} onSelect={onAddChunk}>
137137
<Plus />
138-
Create chunk
138+
New chunk
139139
</DropdownMenuItem>
140140
)
141141
)}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ export function DocumentContextMenu({
141141
onAddDocument && (
142142
<DropdownMenuItem disabled={disableAddDocument} onSelect={onAddDocument}>
143143
<Plus />
144-
Add document
144+
New documents
145145
</DropdownMenuItem>
146146
)
147147
)}

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/headers/workflow-group-meta-cell.tsx

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,12 @@ const LIMITED_RUN_PRESETS = [10, 1000] as const
4444
/** Labels for the table-scoped run items. With an active filter the run is
4545
* scoped to matching rows, so the labels say "filtered rows" to make the
4646
* narrowed target visible. Shared by both menu surfaces. */
47+
/**
48+
* Incomplete before all, matching the action bar and the row context menu, which both
49+
* present Play (empty or failed) ahead of Refresh (every row). These two menus read the
50+
* same four run actions the user already met on the action bar, so they must not invert
51+
* the pair — see `.claude/rules/sim-list-ordering.md`.
52+
*/
4753
function runMenuLabels(hasActiveFilter: boolean) {
4854
const rows = hasActiveFilter ? 'filtered rows' : 'rows'
4955
return {
@@ -171,12 +177,12 @@ export function ColumnOptionsMenu({
171177
{`Run ${selectedRowCount} selected ${selectedRowCount === 1 ? 'row' : 'rows'}`}
172178
</DropdownMenuItem>
173179
)}
174-
<DropdownMenuItem onSelect={() => onRunColumnAll?.()}>
175-
{runLabels.all}
176-
</DropdownMenuItem>
177180
<DropdownMenuItem onSelect={() => onRunColumnIncomplete?.()}>
178181
{runLabels.incomplete}
179182
</DropdownMenuItem>
183+
<DropdownMenuItem onSelect={() => onRunColumnAll?.()}>
184+
{runLabels.all}
185+
</DropdownMenuItem>
180186
{onRunColumnLimited &&
181187
LIMITED_RUN_PRESETS.map((max) => (
182188
<DropdownMenuItem key={max} onSelect={() => onRunColumnLimited(max)}>
@@ -510,10 +516,10 @@ export function WorkflowGroupMetaCell({
510516
{`Run ${selectedCount} selected ${selectedCount === 1 ? 'row' : 'rows'}`}
511517
</DropdownMenuItem>
512518
)}
513-
<DropdownMenuItem onSelect={handleRunAll}>{runLabels.all}</DropdownMenuItem>
514519
<DropdownMenuItem onSelect={handleRunIncomplete}>
515520
{runLabels.incomplete}
516521
</DropdownMenuItem>
522+
<DropdownMenuItem onSelect={handleRunAll}>{runLabels.all}</DropdownMenuItem>
517523
{LIMITED_RUN_PRESETS.map((max) => (
518524
<DropdownMenuItem key={max} onSelect={() => handleRunLimited(max)}>
519525
{runLabels.limited(max)}

apps/sim/app/workspace/[workspaceId]/tables/components/table-context-menu/table-context-menu.tsx

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,23 @@ export function TableContextMenu({
5757
disableImport = false,
5858
disableExport = false,
5959
}: TableContextMenuProps) {
60+
/**
61+
* `Move to` needs a NON-EMPTY `moveOptions`, not just the handler — the looser
62+
* `onMove` alone draws the rule with nothing above it for a table whose other
63+
* actions are all absent and whose move list is empty.
64+
*
65+
* @see `.claude/rules/sim-list-ordering.md` — one rule, before the destructive
66+
* group, with both sides built from the items' exact render conditions.
67+
*/
68+
const hasActionsAboveDestructive =
69+
onViewSchema ||
70+
onRename ||
71+
onImportCsv ||
72+
onExportCsv ||
73+
(onMove && moveOptions && moveOptions.length > 0) ||
74+
onCopyId ||
75+
onTogglePin
76+
6077
return (
6178
<DropdownMenu open={isOpen} onOpenChange={(open) => !open && onClose()} modal={false}>
6279
<DropdownMenuTrigger asChild>
@@ -126,14 +143,7 @@ export function TableContextMenu({
126143
Copy ID
127144
</DropdownMenuItem>
128145
)}
129-
{(onViewSchema ||
130-
onRename ||
131-
onImportCsv ||
132-
onExportCsv ||
133-
onMove ||
134-
onCopyId ||
135-
onTogglePin) &&
136-
onDelete && <DropdownMenuSeparator />}
146+
{hasActionsAboveDestructive && onDelete && <DropdownMenuSeparator />}
137147
{onDelete && (
138148
<DropdownMenuItem disabled={disableDelete} onSelect={onDelete}>
139149
<Trash />

apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/context-menu/context-menu.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,9 @@ export function ContextMenu({
185185
* rendered a trailing rule under the last item.
186186
*/
187187
const hasActionsAboveDestructive =
188+
/* No `openInNewTabPosition` term: the item renders in the 'first' slot or the
189+
'last' one, and the prop is a closed two-value union, so `showOpenInNewTab &&
190+
onOpenInNewTab` already means exactly "the nav item renders somewhere above". */
188191
(showOpenInNewTab && onOpenInNewTab) ||
189192
(showMarkAsRead && onMarkAsRead) ||
190193
(showMarkAsUnread && onMarkAsUnread) ||

0 commit comments

Comments
 (0)