Skip to content

Commit 17de4bb

Browse files
authored
refactor(resources): extend the axes for what every kind actually needs (#6391)
* refactor(resources): extend the axes for what every kind actually needs Three gaps the file and log migrations never hit, all of which tables and knowledge hit immediately. Extending the axis once beats four per-kind workarounds, and each addition is uniform across every kind. - `ResourceGrants.manage` — admin-only governance of the resource, as distinct from writing its content. Table column locks are the first: an owner decides which columns an editor may not touch, and the settings an editor is locked out of are the ones that lock them out. `grantsFromPermissions` already received `canAdmin` and dropped it on the floor. - `ResourceGrants.settled` — whether the capabilities above are final. The one member that describes the value rather than the viewer, and it has to live beside them: a resolving membership and a genuinely denied one produce identical booleans, so `write === false` could not be told from "not yet". Surfaces that render an affordance disabled during load need that, and one-shot latched effects need it badly — the table's lock notice fires once and permanently loses its action if it fires before `manage` resolves. Without this field both surfaces would have had to accept a first-paint flicker; with it they stay byte-identical. - `ResourceLink` gains `{ to: 'list' }` — the index route a kind lives under. Every kind has one, every detail surface needs it (breadcrumb root, and the redirect after the thing it was showing is deleted), and five call sites across two route trees hand-built that path. `hrefFor` still returns null in share scope, so the list route cannot be hand-built from a token either. Knowledge's two list pushes now go through `hrefFor`. The table's two follow in its own PR, once it builds a source. Verified the new tests fail without the code: breaking `settled` to a constant and pointing the list link at `resourceHref` turns three of them red. * docs(resources): describe the extended axis where the rules already live CLAUDE.md, .claude/rules and .cursor/rules all still spelled `grants` as `{ write, run }` and `hrefFor` as self-or-resource. Adds `manage`/`settled` and the `{ to: 'list' }` destination, plus the one thing a reader has to know about `settled`: a denied member and a loading one produce identical capability booleans, so `write === false` is not a decision until `settled` says it is. Also splits the TSDoc that `resourceListHref` landed under — it was describing `resourceHref` and would have documented the wrong function.
1 parent 45e8671 commit 17de4bb

9 files changed

Lines changed: 192 additions & 19 deletions

File tree

.claude/rules/sim-resource-views.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ Enforced by `bun run check:resources` (strict CI gate: `bun run check:resources:
2525
| Axis | Type | Replaces |
2626
| --- | --- | --- |
2727
| `source` | `WorkspaceSource<K> \| ShareSource<K>`, discriminated on `via` | `workspaceId`, `token`, `contentSource`, `isPublic`, `isShared` |
28-
| `grants` | `{ write: boolean; run: boolean }` | `canEdit`, `canRun`, `canAdmin`, `canDelete`, `disableEdit/Insert/Delete` |
28+
| `grants` | `{ write; run; manage; settled }` | `canEdit`, `canRun`, `canAdmin`, `canDelete`, `disableEdit/Insert/Delete` |
2929
| `host` | `'page' \| 'panel' \| 'public'` | `embedded`, `isEmbedded`, `compact`, `minimal` |
3030

3131
There is no fourth axis. Agent streaming is **one optional prop on `FileView`** (`streaming?: FileViewStreaming`), because only files stream.
@@ -87,7 +87,8 @@ return <FileView source={source} grants={grants} host='panel' streaming={streami
8787

8888
- Import from the **unit barrel** (`@/components/resources/file-view`), never a file inside it.
8989
- Copy that differs between workspace and share belongs on the **source** (`source.unavailableCopy`), not in the view. A share must never say "workspace"that is what stops the view becoming an existence oracle.
90-
- Links belong on the source too (`source.hrefFor(link)`), which returns `null` in share scope so nobody hand-builds `/workspace/${token}/…`.
90+
- Links belong on the source too (`source.hrefFor(link)`), which returns `null` in share scope so nobody hand-builds `/workspace/${token}/…`. Three destinations: `{ to: 'self' }`, `{ to: 'resource', kind, id }`, and `{ to: 'list' }` for the index route the kind lives undera breadcrumb root, or where to go after the resource being shown is deleted.
91+
- `grants.settled` says whether `write`/`run`/`manage` are final. A resolving membership and a denied one produce identical booleans, so a surface that renders an affordance disabled while permissions loador fires a one-shot effectmust check it rather than reading `write === false` as a decision.
9192
- `host` decides chrome and URL ownership. `hostOwnsUrl(host)` is the single place the "embedded views do not write nuqs keys" rule lives.
9293

9394
## Never do this

.cursor/rules/sim-resource-views.mdc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ Enforced by `bun run check:resources`; strict gate `bun run check:resources:stri
1515
| Axis | Type | Replaces |
1616
| --- | --- | --- |
1717
| `source` | `WorkspaceSource<K> \| ShareSource<K>`, discriminated on `via` | `workspaceId`, `token`, `contentSource`, `isPublic` |
18-
| `grants` | `{ write: boolean; run: boolean }` | `canEdit`, `canRun`, `canAdmin`, `disableEdit/Insert/Delete` |
18+
| `grants` | `{ write; run; manage; settled }` | `canEdit`, `canRun`, `canAdmin`, `disableEdit/Insert/Delete` |
1919
| `host` | `'page' \| 'panel' \| 'public'` | `embedded`, `isEmbedded`, `compact`, `minimal` |
2020

2121
There is no fourth axis. Agent streaming is one optional prop on `FileView` (`streaming?`), because only files stream.
@@ -41,7 +41,7 @@ const source = workspaceSource({ kind: 'file', workspaceId, resourceId: file.id
4141
return <FileView source={source} grants={grantsFromPermissions(permissions)} host='panel' />
4242
```
4343

44-
Import from the unit barrel (`@/components/resources/file-view`), never a file inside it. Scope-dependent copy lives on `source.unavailableCopy`; links on `source.hrefFor(link)` (which returns `null` in share scope). `hostOwnsUrl(host)` is the one place the "embedded views do not write nuqs keys" rule lives.
44+
Import from the unit barrel (`@/components/resources/file-view`), never a file inside it. Scope-dependent copy lives on `source.unavailableCopy`; links on `source.hrefFor(link)` — `{ to: 'self' | 'resource' | 'list' }`, returning `null` in share scope. `hostOwnsUrl(host)` is the one place the "embedded views do not write nuqs keys" rule lives.
4545

4646
## Never
4747

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ A **resource** is a thing a workspace holds that can also be shared — a file,
422422
Views are mounted against exactly **three axes**, defined in `apps/sim/resources/**` (pure TypeScript — no React, no `'use client'`, because a Server Component builds a share source during SSR):
423423

424424
- `source` — where the data comes from and by what address: `WorkspaceSource<K> | ShareSource<K>`, discriminated on `via`. Replaces `workspaceId`, `token`, `contentSource`, `isPublic`. `ShareSource` declares `workspaceId?: never`, so a share token can no longer be laundered through a workspace-shaped slot.
425-
- `grants` — what this viewer may do: `{ write, run }`. Replaces `canEdit`, `canRun`, `canAdmin`, `disableEdit/Insert/Delete`.
425+
- `grants` — what this viewer may do: `{ write, run, manage }`, plus `settled` (whether those three are final, or still resolving — a denied member and a loading one are otherwise indistinguishable). Replaces `canEdit`, `canRun`, `canAdmin`, `disableEdit/Insert/Delete`.
426426
- `host` — who owns the URL, the router, the document frame: `'page' | 'panel' | 'public'`. Replaces `embedded`. `hostOwnsUrl(host)` is the one place the "embedded views do not write nuqs keys" rule lives.
427427

428428
There is no fourth axis; agent streaming is one optional prop on `FileView`. Consumers CONSTRUCT the axes and MOUNT the view — never wrap it in a passthrough, never reach past its barrel, never reimplement its UI because it lacks a seam (add the seam), never import `@/app/workspace/[workspaceId]/**` from an anonymous surface (`app/f/**`, `app/(interfaces)/**`), and never read `useRouter`/`useParams`/`useQueryState`/`useUserPermissionsContext` inside a unit. A kind with no canonical view yet — `table` alone today — is simply absent from the view list in the check's `CANONICAL_UNITS` — no flag, shim, or placeholder. Every unit has the same layout (`<unit>.tsx` · `index.ts` · `components/<child>/` · `hooks/` · `utils/` · `types.ts`), so moving between them costs nothing.

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,16 @@ interface ResourceContentProps {
129129
onBrowserOverlayControllerChange?: (controller: BrowserPanelOverlayController | null) => void
130130
}
131131

132-
/** The agent owns the file while it is streaming; nothing is edited from here. */
133-
const STREAMING_FILE_GRANTS: ResourceGrants = { write: false, run: false }
132+
/**
133+
* The agent owns the file while it is streaming; nothing is edited from here.
134+
* Settled by construction — this is a literal, not a resolving membership.
135+
*/
136+
const STREAMING_FILE_GRANTS: ResourceGrants = {
137+
write: false,
138+
run: false,
139+
manage: false,
140+
settled: true,
141+
}
134142

135143
/**
136144
* Grace window kept locked after the agent stops streaming into the file, so the lock bridges the

apps/sim/app/workspace/[workspaceId]/knowledge/[id]/knowledge-base.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,8 @@ export function KnowledgeBase({
422422
{
423423
onSuccess: () => {
424424
removeKnowledgeBase(id)
425-
router.push(`/workspace/${workspaceId}/knowledge`)
425+
const list = source.hrefFor({ to: 'list' })
426+
if (list) router.push(list)
426427
},
427428
}
428429
)
@@ -622,7 +623,10 @@ export function KnowledgeBase({
622623
{
623624
label: 'Knowledge Base',
624625
icon: Database,
625-
onClick: () => router.push(`/workspace/${workspaceId}/knowledge`),
626+
onClick: () => {
627+
const list = source.hrefFor({ to: 'list' })
628+
if (list) router.push(list)
629+
},
626630
},
627631
{
628632
label: knowledgeBaseCrumbLabel,

apps/sim/resources/grants.test.ts

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,20 +21,26 @@ describe('grantsFromPermissions', () => {
2121
expect(grantsFromPermissions({ canRead: false, canEdit: false, canAdmin: false })).toEqual({
2222
write: false,
2323
run: false,
24+
manage: false,
25+
settled: true,
2426
})
2527
})
2628

2729
it('lets a read-only member run', () => {
2830
expect(grantsFromPermissions({ canRead: true, canEdit: false, canAdmin: false })).toEqual({
2931
write: false,
3032
run: true,
33+
manage: false,
34+
settled: true,
3135
})
3236
})
3337

3438
it('lets an editor run', () => {
3539
expect(grantsFromPermissions({ canRead: true, canEdit: true, canAdmin: false })).toEqual({
3640
write: true,
3741
run: true,
42+
manage: false,
43+
settled: true,
3844
})
3945
})
4046

@@ -45,6 +51,40 @@ describe('grantsFromPermissions', () => {
4551
}
4652
})
4753

54+
it('grants manage exactly to an admin', () => {
55+
for (const permissions of ALL_PERMISSIONS) {
56+
expect(grantsFromPermissions(permissions).manage).toBe(permissions.canAdmin)
57+
}
58+
})
59+
60+
/**
61+
* The distinction the field exists for. A resolving membership and a genuine
62+
* no-access member produce identical capability booleans, so without `settled`
63+
* a surface cannot tell "you may not" from "we do not know yet" — and both
64+
* disabled-during-load chrome and one-shot latched effects need to.
65+
*/
66+
it('reports an unresolved membership as unsettled, with the same capabilities as a denied one', () => {
67+
const loading = grantsFromPermissions({
68+
canRead: false,
69+
canEdit: false,
70+
canAdmin: false,
71+
isLoading: true,
72+
})
73+
const denied = grantsFromPermissions({ canRead: false, canEdit: false, canAdmin: false })
74+
75+
expect(loading.settled).toBe(false)
76+
expect(denied.settled).toBe(true)
77+
expect(loading.write).toBe(denied.write)
78+
expect(loading.run).toBe(denied.run)
79+
expect(loading.manage).toBe(denied.manage)
80+
})
81+
82+
it('treats a caller that tracks no loading state as settled', () => {
83+
for (const permissions of ALL_PERMISSIONS) {
84+
expect(grantsFromPermissions(permissions).settled).toBe(true)
85+
}
86+
})
87+
4888
it('never runs anything without at least read', () => {
4989
for (const permissions of ALL_PERMISSIONS) {
5090
if (permissions.canRead || permissions.canEdit) continue
@@ -66,6 +106,18 @@ describe('grantsForShare', () => {
66106
}
67107
})
68108

109+
it('never manages, for any kind', () => {
110+
for (const kind of RESOURCE_KINDS) {
111+
expect(grantsForShare(kind).manage).toBe(false)
112+
}
113+
})
114+
115+
it('is always settled — a token resolves capabilities outright', () => {
116+
for (const kind of RESOURCE_KINDS) {
117+
expect(grantsForShare(kind).settled).toBe(true)
118+
}
119+
})
120+
69121
it('is never more capable than a read-only member', () => {
70122
const member = grantsFromPermissions({ canRead: true, canEdit: false, canAdmin: false })
71123
for (const kind of RESOURCE_KINDS) {

apps/sim/resources/grants.ts

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,34 @@ export interface ResourceGrants {
1616
* surface sends.
1717
*/
1818
readonly run: boolean
19+
/**
20+
* May change how the resource is governed rather than what it contains — the
21+
* admin-only affordances. Table column locks are the first: an owner decides
22+
* which columns an editor may not touch, which is a different question from
23+
* whether this viewer may write, and the settings an editor is locked out of
24+
* are the ones that lock them out.
25+
*
26+
* On this axis rather than as a per-view `canAdmin` prop because that is the
27+
* vocabulary the axis exists to replace — `check-resource-views.ts` bans the
28+
* name outright.
29+
*/
30+
readonly manage: boolean
31+
/**
32+
* Whether the three capabilities above are final, or still resolving.
33+
*
34+
* The one member that describes this *value* rather than the viewer, and it
35+
* has to sit here: a consumer reading `write === false` cannot otherwise tell
36+
* "this viewer may not write" from "we do not know yet", because
37+
* {@link grantsFromPermissions} maps both to the same booleans. Surfaces that
38+
* render an affordance disabled while permissions load — rather than popping
39+
* it in afterwards — need that distinction, and one-shot effects need it
40+
* badly: firing a latched notice before `manage` resolves permanently drops
41+
* the action it was supposed to carry.
42+
*
43+
* `true` wherever capabilities are known at construction, which is every
44+
* caller that tracks no loading state at all.
45+
*/
46+
readonly settled: boolean
1947
}
2048

2149
/**
@@ -27,6 +55,12 @@ export interface WorkspacePermissionSnapshot {
2755
readonly canRead: boolean
2856
readonly canEdit: boolean
2957
readonly canAdmin: boolean
58+
/**
59+
* Whether the membership is still being fetched. Optional because a caller
60+
* that resolves permissions synchronously has no such state — and its absence
61+
* correctly reads as settled.
62+
*/
63+
readonly isLoading?: boolean
3064
}
3165

3266
/**
@@ -38,24 +72,31 @@ export interface WorkspacePermissionSnapshot {
3872
* That is precisely the state this function maps to `run: false`.
3973
*/
4074
export function grantsFromPermissions(permissions: WorkspacePermissionSnapshot): ResourceGrants {
41-
const { canRead, canEdit } = permissions
75+
const { canRead, canEdit, canAdmin, isLoading } = permissions
4276
return {
4377
write: canEdit,
4478
run: canEdit || canRead,
79+
manage: canAdmin,
80+
settled: !isLoading,
4581
}
4682
}
4783

4884
/**
4985
* Grants for an anonymous share visitor.
5086
*
5187
* A share never writes, and today never runs: every shareable kind is served as
52-
* read-only bytes. `kind` is taken anyway because running is a per-kind property
53-
* — it turns on for a kind whose public surface gains an execution route — so
54-
* callers already pass what that decision will be keyed on.
88+
* read-only bytes, and never manages. `kind` is taken anyway because running is
89+
* a per-kind property — it turns on for a kind whose public surface gains an
90+
* execution route — so callers already pass what that decision will be keyed on.
91+
*
92+
* Always settled: an anonymous visitor's capabilities are known the moment the
93+
* token resolves, so there is no loading state to represent.
5594
*/
5695
export function grantsForShare(_kind: ResourceKind): ResourceGrants {
5796
return {
5897
write: false,
5998
run: false,
99+
manage: false,
100+
settled: true,
60101
}
61102
}

apps/sim/resources/source.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,35 @@ describe('workspaceSource', () => {
9191
)
9292
})
9393

94+
/**
95+
* The five call sites that hand-built these paths before `{ to: 'list' }`
96+
* existed lived in two different route trees, which is exactly how a route
97+
* rename escapes one of them.
98+
*/
99+
it('resolves a list link to its own kind index, for every kind', () => {
100+
const expected: Record<ResourceKind, string> = {
101+
file: '/workspace/ws_1/files',
102+
table: '/workspace/ws_1/tables',
103+
knowledge: '/workspace/ws_1/knowledge',
104+
log: '/workspace/ws_1/logs',
105+
}
106+
107+
for (const kind of RESOURCE_KINDS) {
108+
const source = workspaceSource({ kind, workspaceId: 'ws_1', resourceId: 'id_1' })
109+
expect(source.hrefFor({ to: 'list' })).toBe(expected[kind])
110+
}
111+
})
112+
113+
it('escapes the workspace id on a list link too', () => {
114+
const source = workspaceSource({
115+
kind: 'table',
116+
workspaceId: 'ws/../../evil',
117+
resourceId: 'tbl_1',
118+
})
119+
120+
expect(source.hrefFor({ to: 'list' })).toBe('/workspace/ws%2F..%2F..%2Fevil/tables')
121+
})
122+
94123
it('escapes ids so a hostile id cannot graft extra path or query onto the route', () => {
95124
const source = workspaceSource({
96125
kind: 'file',
@@ -164,6 +193,7 @@ describe('shareSource', () => {
164193
for (const kind of SHAREABLE_KINDS) {
165194
const source = makeShareSource(kind)
166195
expect(source.hrefFor({ to: 'self' })).toBeNull()
196+
expect(source.hrefFor({ to: 'list' })).toBeNull()
167197
for (const target of RESOURCE_KINDS) {
168198
expect(source.hrefFor({ to: 'resource', kind: target, id: 'id_1' })).toBeNull()
169199
}

apps/sim/resources/source.ts

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,19 @@ import type { ResourceKind, ResourceSeed, ShareableKind } from '@/resources/kind
33
/** Why a resource could not be shown. */
44
export type UnavailableReason = 'missing' | 'transient'
55

6-
/** A destination a view may want to link to: itself, or another resource by id. */
7-
export type ResourceLink = { to: 'self' } | { to: 'resource'; kind: ResourceKind; id: string }
6+
/**
7+
* A destination a view may want to link to: itself, another resource by id, or
8+
* the index route its own kind lives under.
9+
*
10+
* `list` is here rather than in a per-kind module because every kind has one and
11+
* every detail surface needs it — a breadcrumb root, and the redirect after the
12+
* resource it was showing is deleted. Five call sites hand-built that path
13+
* before this member existed.
14+
*/
15+
export type ResourceLink =
16+
| { to: 'self' }
17+
| { to: 'resource'; kind: ResourceKind; id: string }
18+
| { to: 'list' }
819

920
/** Display noun per kind, used by the copy the base builds. */
1021
const RESOURCE_NOUN: Record<ResourceKind, string> = {
@@ -77,6 +88,26 @@ export type ResourceSource<K extends ResourceKind = ResourceKind> = K extends Re
7788
? WorkspaceSource<K> | ShareSource<K>
7889
: never
7990

91+
/**
92+
* The in-app index route a kind lives under, used by {@link workspaceSource}'s
93+
* `hrefFor` for `{ to: 'list' }`. Sibling to {@link resourceHref}, and exhaustive
94+
* for the same reason: a kind added without a list route fails to compile here
95+
* rather than sending a breadcrumb somewhere that does not exist.
96+
*/
97+
function resourceListHref(workspaceId: string, kind: ResourceKind): string {
98+
const workspace = `/workspace/${encodeURIComponent(workspaceId)}`
99+
switch (kind) {
100+
case 'file':
101+
return `${workspace}/files`
102+
case 'table':
103+
return `${workspace}/tables`
104+
case 'knowledge':
105+
return `${workspace}/knowledge`
106+
case 'log':
107+
return `${workspace}/logs`
108+
}
109+
}
110+
80111
/**
81112
* The in-app route for a resource, used by {@link workspaceSource}'s `hrefFor`.
82113
* The one table — every in-app destination for a resource is spelled here and
@@ -124,10 +155,16 @@ export function workspaceSource<K extends ResourceKind>({
124155
return `Something went wrong loading this ${noun}. Try again.`
125156
}
126157
},
127-
hrefFor: (link) =>
128-
link.to === 'self'
129-
? resourceHref(workspaceId, kind, resourceId)
130-
: resourceHref(workspaceId, link.kind, link.id),
158+
hrefFor: (link) => {
159+
switch (link.to) {
160+
case 'self':
161+
return resourceHref(workspaceId, kind, resourceId)
162+
case 'resource':
163+
return resourceHref(workspaceId, link.kind, link.id)
164+
case 'list':
165+
return resourceListHref(workspaceId, kind)
166+
}
167+
},
131168
}
132169
}
133170

0 commit comments

Comments
 (0)