refactor(resources): one canonical view per resource, mounted on source/grants/host - #6279
Open
waleedlatif1 wants to merge 27 commits into
Open
refactor(resources): one canonical view per resource, mounted on source/grants/host#6279waleedlatif1 wants to merge 27 commits into
waleedlatif1 wants to merge 27 commits into
Conversation
…ce/grants/host
Extracts the file and table views out of their route folders into canonical
units under `apps/sim/components/resources/**`, and gives every consumer one way
to mount them. Behavior-neutral: no page, no API route, and no migration is
added or removed, and every default is preserved.
The three axes (`apps/sim/resources/**`, pure TypeScript so a Server Component
can build one during SSR):
- `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.
- `grants` — what this viewer may do: `{ write, run }`. Replaces `canEdit`,
`canRun`, `canAdmin`, `disableEdit/Insert/Delete`.
- `host` — who owns the URL, the router, the document frame. Replaces
`embedded`; `hostOwnsUrl(host)` is the single home of the "embedded views do
not write nuqs keys" rule.
`file` is the only kind with a share seed, because `/f/[token]` is the only
public route that serves one. Every other kind is `never` and cannot be
addressed by a token at all — a compile-time fact rather than a runtime check.
Also in this PR:
- Public share auth (password / email-OTP / SSO) moves out of `app/f/[token]`
into `components/public-share/**`, behind one page-side gate. Security
hardening on the way: timing-safe OTP comparison, a per-IP bucket on OTP
verify separate from the send path, and share tokens removed from logs.
- Log status chrome (`StatusBadge`, `STATUS_CONFIG`, `getDisplayStatus`) moves
to `components/execution-status/`, shared instead of per-surface.
- `AnchoredContextMenu` and `ActionRow` move to `components/`, which closed a
real hole: `file-view` is mounted by `app/f/[token]` for anonymous visitors,
and with no `sideEffects: false` its import of the workspace barrel pulled the
whole authenticated tree into the public chunk.
- emcn: `ChipPasswordInput`, a shared `ChipChevronDown`, and byte-range /
untitled-title utilities behind the file preview routes.
- Deployed-chat header now renders the shared `Navbar` in `logoOnly` mode, so a
public chat wears the same wordmark, geometry, name and "Shared by" credit as
the shared file page. This drops the GitHub star chip from that header.
- `ActionRow` used `var(--divider)`, which is not a defined token, so its
dividers fell back to `currentColor`. Now `var(--border)`.
Nothing here advertises a capability it cannot deliver: the `form` trigger type
that arrived with this work drives nothing on its own, so it is not registered in
the Start block's trigger list, the Logs filter, the execute contract, or the
executor's streaming types. It lands with the surface that produces it.
Enforced by `bun run check:resources` (added to CI), which ratchets counters for
wrappers, imports past a unit barrel, cross-tree imports, unsanctioned props,
token-as-workspaceId, and context leaks. The cross-tree counter is re-based to
the current tree: every offender it counts is inherited, and this change strictly
reduces the number.
Verification: apps/sim + 22 packages typecheck, biome clean, 18598 tests pass,
and all 15 repo gates pass including both strict variants.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryCursor Bugbot is generating a summary for commit 001cbb4. Configure here. |
6 tasks
Collaborator
Author
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThe PR consolidates file and table resource rendering around canonical views parameterized by source, grants, and host, while reorganizing shared UI and public-share infrastructure.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/resources/source.ts | Introduces discriminated workspace/share sources, preventing share sources from carrying workspace identifiers or producing workspace links. |
| apps/sim/components/resources/file-view/file-view.tsx | Consolidates workspace, panel, streaming, and public file rendering into one canonical view driven by source, grants, and host. |
| apps/sim/app/f/[token]/public-file-view.tsx | Mounts the canonical file view with a token-based share source, read-only grants, and public host behavior. |
| apps/sim/app/api/files/public/[token]/content/route.ts | Refactors public content serving while retaining active-share resolution, deployment authentication, rate limiting, and ranged media responses before storage access. |
| apps/sim/components/resources/table-view/index.ts | Establishes shared table-view exports and data contracts used by the workspace table grid. |
| apps/sim/hooks/queries/tables.ts | Updates table query and mutation integration while preserving typed contracts and authoritative cache refresh behavior. |
| scripts/check-resource-views.ts | Adds static enforcement for canonical mounts, import boundaries, axis purity, and prohibited route or permission context access. |
| .github/workflows/test-build.yml | Adds the resource boundary audit to CI and removes the explicit ripgrep installation step. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Workspace[Workspace page] --> WS[Workspace source]
Panel[Embedded panel] --> WS
Public[Public share page] --> SS[Share source]
Permissions[Workspace permissions] --> WG[Workspace grants]
SharePolicy[Share policy] --> SG[Read-only share grants]
WS --> View[Canonical resource view]
SS --> View
WG --> View
SG --> View
PageHost[page host: owns URL] --> View
PanelHost[panel host: no URL ownership] --> View
PublicHost[public host: no workspace links] --> View
View --> File[File content and collaboration]
View --> Table[Table rendering and editing]
Reviews (3): Last reviewed commit: "fix(file-view): correct the data-table s..." | Re-trigger Greptile
Navbar's actions slot is not gated by hideBrand, so a whitelabeled public chat or shared file rendered a Share menu whose drafted post advertised Sim. Compose the mention from brand.name when brand.isWhitelabeled, matching the rule the email footer already applies.
Collaborator
Author
Collaborator
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c013365. Configure here.
data-table.tsx moved one directory deeper during the migration but kept its './document-table.css' import. The stylesheet is shared at the components/ level — both rich-markdown-editor importers already use '../document-table.css' — so this was the lone stale path. TypeScript does not resolve bare CSS imports, so only the production build caught it.
Collaborator
Author
Collaborator
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 4da3df5. Configure here.
…e tab chrome (#6280) Continues the resource-view layer. Three kinds now have a canonical view (file, log, knowledge) and only `table` remains — deliberately, because the mothership mounts its editing shell, which is a product decision rather than unfinished work. **log** — `log-details/` splits into `components/resources/log-view/` (the view) and a 198-line shell that keeps resize, keyboard nav and close chrome. Four leaked contexts resolved: `useParams` → `source.workspaceId`, `useRouter` → an `onNavigate` prop, `useQueryState(tab)` → view-owned state gated by `hostOwnsUrl(host)`, `usePermissionConfig` → a required `showExecutionInternals` prop (deliberately not folded into `grants`: it gates *seeing* trace internals, not write or run, and an optional field would default permissive — the wrong failure mode for a permission-group restriction). A fifth leak the plan missed — `file-download.tsx` holding its own `useRouter` — was caught by the gate. **knowledge** — `base.tsx` (1733 lines) splits into a 909-line canonical view and a 974-line shell. The read surface (document list, filters, sort, pagination, unavailable state) is the view; upload, connectors, tag editing, rename, delete and bulk operations stay in the shell, exactly as the tables editing grid kept its write path. All six nuqs keys now route through `useKnowledgeListState(host)`, whose every write sits behind `hostOwnsUrl`. **tab chrome** — the five `Embedded*Actions` collapse into one kind-keyed `ResourceTabActions`. They differed only in icon, copy and destination, which is a config table. `knowledge` and `table` destinations now resolve through `workspaceSource().hrefFor()` instead of hand-built strings. **schedule** is removed as a resource kind: its page was deleted upstream, its `resourceHref` case pointed at a route that no longer exists, and `MothershipResourceType` never carried it, so it was unreachable. Behavior change, intended and limited to embedded surfaces: the mothership panel no longer writes the log `?tab` key or the six knowledge keys into the host page's address bar. That is the bug the `host` axis exists to fix. Both route pages keep their deep-linkable params unchanged. `Resource` and `InlineRenameInput` moved to `components/` — mandatory, since a canonical unit may not import the workspace route tree. The workspace barrel re-exports both, so its consumers are byte-unchanged. Ratchets: shadow-named components 8 → 2 (the 2 left are deliberate — `EmbeddedWorkflow`, because a workflow is a live collaborative session rather than a document with an address, and `EmbeddedFolder`, because a folder is structure inside a resource); cross-tree imports 38 → 37. None raised. Verification: apps/sim + 22 packages typecheck, biome clean, 18626 tests pass, and all 15 repo gates pass including both strict variants. Not visually verified.
Conflict resolutions of substance: - files serve + public-share content routes: kept the branch's byte-range media streaming and added staging's `ServeOptions` / `preview=1` HEIC derivative path. The two are disjoint (`isMediaContentType` is audio/video only), so both branches now coexist ahead of the buffered compile path. - `hooks/use-file-content-source.tsx` is deleted on this branch; staging's additions to it (the `preview` URL flag and the `ImageDimensionsSource` capability) moved to `resources/file-source.ts`, and the workspace adapter now resolves from the mounted source rather than a second React context. - `UnsupportedPreview`: the branch's download-carrying variant replaces the moved copy in `preview-shared`, so the image preview's new decode-failure fallback lands on the version that works on chrome-less hosts. - logs: staging's workflow deep-link (#6275) ported into `LogView` through a new `resources/log-source.ts`, so the unit reads the href off its source instead of hand-building a workspace path. - `public-file-view`: kept the branch's `Navbar` + `h-dvh` scroll port, which supersedes staging's header layout fix on the header it replaced. - `test-build.yml`: took staging's single `check:audits` step; `run-audits.ts` derives the list from package.json, so `check:resources` is picked up. - API route baseline raised to the merged tree's real count (1008).
- cloud-review-tools.test.ts: the merge kept the branch's import line (which had dropped `symlink` along with the test that used it) while taking staging's reinstated test body, so the symlink-escape assertion would have thrown ReferenceError on CI. tsconfig excludes *.test.ts, so type-check could not catch it. Restored the import and the workflow's ripgrep install step, which the branch removed alongside the same test. - createByteRangeResponse defaulted to `public, max-age=31536000` while its sibling createFileResponse defaults to `private, no-cache` and says in a comment why. resolveServeCacheControl returns undefined for any unversioned non-`workspace` context, so those access-verified media responses landed on the public default — storable by a shared cache and re-servable cross-user, where staging sent `private, no-cache`. Defaults now match. - ReadOnlyTextPreview: restored staging's `flex … flex-col` on the rich preview wrapper. Every PreviewPanel renderer sizes itself as a flex item, so in a block parent HtmlPreview measures a zero-height box, never mounts its iframe, and nothing ever grows the container to un-wedge it — shared .html and .svg rendered blank.
The branch's workflow listed only the non-strict variant; staging replaced the hand-written step list with a runner that derives every zero-arg check:*. Merging the two ran the 1359-line resource scanner twice. Excluded the same way, and for the same reason, as check:api-validation.
…ated
A canonical unit is mounted by anonymous surfaces, so an import of the
authenticated route tree from inside one pulls the workspace bundle into the
public chunk and re-creates exactly the coupling the axes exist to delete. The
migration introduced two, and R3b's inherited 37-file budget was wide enough to
hide both.
- The collaborative socket provider moves to `components/socket-provider`. It
has consumers on both sides of the route tree — the workspace editor mounts
it, and the file view's collaboration hook reads it from inside a unit — so
`app/workspace/providers/**` was never its home. Same fix, and same reasoning,
as the earlier `ActionRow` / `AnchoredContextMenu` moves.
- `DELETED_WORKFLOW_LABEL` moves to `lib/workflows/labels.ts`. It is a workflow
concept, not a log one: four of its five consumers are workflow-editor
surfaces, and sourcing it from the log-view barrel made `lib/workflows/
subblocks/display.ts` — reachable from server routes — drag a 762-line
'use client' module behind it.
- The log view's frozen-canvas modal keeps its `Preview` import, annotated. That
component reads useParams<{ workspaceId }>() and pulls in the editor's
subblock and edge components; it IS editor UI, and hoisting it would drag half
the editor into components/. It is safe only because `log`'s share seed is
`never`.
Adds R3c to check:resources, held at 0: the same edge R3b counts, narrowed to
canonical units, where there is no budget. Verified it fails by removing the
annotation and watching the audit go red.
…it finished" headObject now answers local storage from `stat` instead of reporting every object as missing. That is the honest answer and the byte-range paths need it, but it also brought ten existing callers to life on self-hosted and dev deployments — including three that read "the object is there" as "the copy finished" and skip the work: the workspace fork copier, the KB document copier, and the table snapshot cache. Those guards were written against cloud backends, where an interrupted PUT leaves no object at all. The local path was a bare `writeFile`, which truncates before it writes, so a crash mid-write would have left a prefix under a key those callers now treat as complete — a fork retry marking a torn file copied. Writes now land on a sibling `.partial` path and are renamed onto the target; `rename` within a directory is atomic on POSIX and replaces in one step, so the key names either the previous bytes or the whole new ones. A failed write or rename removes the temp file and rethrows. Local multipart accumulates and commits through this same path, so snapshots are covered too. Reviewed the other seven callers: all strictly improve. materialize-file now charges quota against the real on-disk size rather than trusting the row, the CSV import progress bar is no longer permanently indeterminate on local, and the TikTok size probe skips its counting pass. Their comments claimed local returns null; refreshed. Tests assert the call sequence rather than an observed torn read — a timing test passes against a plain `writeFile` too, since even a multi-megabyte write resolves inside one macrotask, so it would be a test that cannot fail. Confirmed all four go red when the temp-and-rename is reverted.
Each of these is a side-effect of a deliberate change reaching further than the change needed. The features stay; what they touched by accident is put back. - Config files download again. Resolving unknown extensions through the canonical MIME table is what makes byte-range serving work, but it also promoted .env/.log/.conf/.ini/.cfg from "unknown bytes" to text/plain, which is on the inline allowlist — a shared .env started rendering in the tab instead of downloading. Those five join FORCE_ATTACHMENT_EXTENSIONS, so the MIME resolution stays honest and the disposition stays what it was. Verified these are the only five of the 47 newly-typed extensions that flipped. - The aggregate per-share ceiling is charged once per read, not once per seek. It is shared by every visitor to a link, so counting each range request let one person scrubbing a shared video 429 everyone else. The audit row beside it already deduped seeks; both now use one `isReadStart` predicate rather than two copies of the same string test that can drift apart. - MediaPreview clears a decode failure when the bytes change. It is keyed on `file.id`, which is stable across content edits, so one transient error wedged the player until the viewer opened a different file. Adjusted during render, the same pattern the markdown image node view uses. - The chat-trigger attachment guard accepts `dataUrl`, the preferred inline field, not just the legacy `data`. `processChatFiles` reads `dataUrl || data`, so testing only `data` skipped the upload for every current client and dropped its attachments silently — the exact failure the guard was added to prevent. - The navbar sticks below the macOS traffic-light lane instead of the viewport. Every shell mounting it wears `.desktop-title-bar-page`, which reserves the lane with padding, but sticky positions against the scroll port — so the bar slid under the traffic lights on scroll. The variable is 0px off-desktop. - The wordmark opens in a new tab on the deployed chat and the public file page. Reusing the landing navbar turned those two from `sim.ai` in a new tab into same-origin `/` in the same tab, so clicking the logo mid-conversation navigated away and destroyed it. `/` is the better href — self-hosters get their own landing — so only the target moves back, behind one `brandInNewTab` prop for surfaces hosting something the viewer would lose.
… have An audit of all four units against each other found one real divergence and a pile of small ones, all in table-view. file-view, log-view and knowledge-view are already uniform: `<unit>.tsx` + `index.ts` + `components/<child>/` + `utils/` (+ `hooks/`), absolute imports, tests colocated with what they test. table-view was flat — fifteen entries at the unit root, `cells/` and `headers/` as sibling groups rather than children of `components/`, three test files at the root, and relative imports throughout, which the repo bans everywhere else. Nothing about that was load-bearing. This moves the files and rewrites the specifiers; the only non-import lines in the diff are five type-only import members biome reflowed. Behaviour is unchanged by construction, and page and panel keep mounting the same components they did. cells/, headers/ -> components/ data-row, select-pill, ... -> components/<name>/<name>.tsx + index.ts utils.ts, values.ts, -> utils/selection.ts, utils/values.ts, constants.ts utils/constants.ts *.test.ts -> beside their subject What this does NOT fix, and deliberately: table-view still has no `TableView`. It exports 72 symbols and no view, because a table has no read-only surface yet — everything that draws a grid today also writes one, and the shell that does both reads useRouter and the permission context. Splitting it is a real refactor, not a move, and it is the one thing standing between `table` and the same axes the other three are mounted against. The barrel and the rule now say so plainly instead of implying uniformity that was not there. Also corrects three docs that claimed `knowledge` and `log` had no canonical view. They both do; only `table` does not.
6 tasks
Three zero-reference exports, found by walking every export in the modules the merge touched and counting non-test references. - `resolveMediaMimeType` + `MEDIA_FALLBACK_MIME` (`lib/uploads/utils/file-utils`). Staging added them in #6341 for `MediaPreview`'s blob path; this branch rewrote that preview to stream from the serve route, so the merge orphaned them. Their tests go too, and the `DUAL_CONTAINER_MIME` doc stops pointing at a function that no longer exists. Worth recording, since deleting the helper deletes the fix: it retagged a dual-container `.webm` to the element the viewer had already chosen. The serve route now declares the type instead, and derives it from the filename, where `webm` maps to `video/webm` — so an audio-only `.webm` reaches an `<audio>` element labelled `video/webm`. Browsers sniff `src=` responses rather than trusting the header, so this is inert in practice, but it is a real narrowing and the public-share route (which uses the stored `file.contentType`) does not share it. - `tableWorkspaceId` (`resources/table-source.ts`, the whole file). Added by this branch's own first commit and never called — `cell-render` takes the workspace id as a plain argument. Its TSDoc claimed to be "the value that decides whether a cell may render a sim-resource chip", which nothing enforced. The table migration will want this helper; it can arrive with a caller and an accurate docstring. - `isMediaFileType` (`lib/uploads/utils/file-utils`). Not ours — dead since #2068, zero references including tests. Removed while the file was open.
…ds (#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.
Step 1 of migrating tables onto the resource axes: pure relocations, no logic. A canonical unit may not import `@/app/**`, so everything the table shell reaches for has to either move somewhere both sides can see it, or be replaced later. - `Resource`/`BreadcrumbItem`/`ColumnOption`/`SortConfig` now come from `@/components/resource` directly. The workspace barrel was only re-exporting them, so this is the same module by a shorter path. - `useLogDetailsResize` moves to `@/hooks/`. Its only dependency is the logs UI store, which is already global, and it has consumers in two route trees. - `ImportCsvDialog` and `ImportProgressMenu` move to `components/table-import/`. Both are mounted by the tables *list* page as well as the detail shell, so they cannot live inside the unit — same reasoning as `Resource` and `AnchoredContextMenu` before them. Cross-route edges out of `tables/[tableId]` drop from 7 targets to 4. The two remaining `@/app/workspace/[workspaceId]/components` imports are in `error.tsx` and `loading.tsx`, which stay behind as route files.
Six commits from staging. Conflicts of substance: - special-tags. Staging landed ~1900 lines of credential-card work (a new submission protocol, OAuth chip connection, interaction cards) in the single 2630-line special-tags.tsx. This branch had split that file's pure half into components/chat/special-tags/parse.ts, so an anonymous chat surface can parse tags without the workspace route tree. Rather than pick a side, the split was re-applied to staging's newer content: every parser, type and tag constant went to parse.ts; SpecialTags/PendingTagIndicator/WorkspaceResourceDisplay/ CredentialDisplay stayed behind. Two of staging's new helpers moved with the parse half (`credentialTagHasVisibleCard` and its `CREDENTIAL_CARD_TYPES` / `isCredentialCardItemVisible` dependencies) because they are pure predicates the renderer merely calls. parse.ts gains its first import, `isSafeHttpUrl` — still no React and no workspace context, which is the invariant that matters. - `SpecialTagRendererProps`, the seam chat-content uses to render tags without importing the route-tree renderer, gains `interactionId` and `credentialSubmission`. Staging added both at the call site; the contract had to widen or the injected renderer no longer type-checks. - `useWorkspaceImageDimensionsAdapter` takes staging's fix — it now subscribes to the file list reactively instead of reading the cache once, so a cold direct file-view load reserves image boxes. Upstream gated that subscription with an `enabled` option because its caller passed a share token through the `workspaceId` slot; here the nullable workspace id already expresses it, which is the thing the axes exist to make unrepresentable. - Mention chips: staging's `editor.storage.mention` -> `mentionMenu` fix ported to the moved file. Worth noting it is a real bug — `use-editor-mentions` writes `storage.mentionMenu.navigable`, but the chip read `storage.mention`, which is the *node* extension and has no such field, so chips were never navigable. - `scaling-test-helpers.ts` was added by staging into a directory this branch restructured; the merge dropped it and the special-tags test could not resolve it. Restored.
Step 2 of the migration. A canonical unit may not read route context (R6, held at 0 with no annotation escape), and both of these would fail it the moment the tree moves. - `TableGrid` had `workspaceId`/`tableId` as optional props with a `useParams()` fallback. Its only mount already passes both, so the fallback was dead code that nonetheless made the grid unmountable more than once per page. Props are now required and the fallback is gone. - `RowModal` read `params.workspaceId` with no fallback at all — a hard route dependency feeding three mutations. It now takes `workspaceId` as a required prop from its two mount sites in `table.tsx`. No behavior change: every call site already had the values, and the panel was already passing them explicitly. What changes is that the panel's copy no longer silently resolves the *host page's* params when a prop is missing.
…ost's URL Step 3, and the one intended behavior change in this migration — landing alone so it is reviewable on its own. `useQueryStates(tableDetailParsers)` was called unconditionally and all eleven writers ran in both hosts, so opening a table in the mothership panel wrote `?sort` / `?dir` / `?table-view` onto `/home`. The `view` -> `table-view` wire-key rename exists precisely because of that collision — the workaround documented the bug instead of fixing it. This is the same fix #6280 already applied to the log `?tab` key and the six knowledge keys. `useTableDetailState({ host })` wires both branches unconditionally and returns the same `[state, setState]` shape `useQueryStates` did, so every one of the eleven call sites is unchanged. A host that owns the URL keeps the query params; an embedded one holds the identical values locally. The local branch is one state object, not three: several writers set multiple keys in a single call and rely on that landing as one update, and three setters would tear midway through the view-resolution latch. That deletes the `inheritedParams` guard (~28 lines) outright. It existed only to detect a view id left on the host URL by a previously-open resource; with the panel on local state there is nothing to inherit. The parsers move to `lib/table/detail-search-params.ts`. They cannot stay in the route tree — the hook that owns the URL may not import it — and they cannot move into the unit either, because a unit may not call nuqs at all. A pure, server-safe parser module is the one home both halves may reach. What changes for a panel user: sort/view no longer survive a hard reload of the host page, and no longer leak between two tabs open on different tables. Both route pages keep their deep-linkable params unchanged. Verified the six panel-isolation tests go red when the host gate is forced open.
…text Step 4, and the step where a mistake would actually change who can do what. 45 permission reads converted: - 38 `userPermissions.canEdit` -> `grants.write`. Mechanically, with no exceptions: `grants.run` is `canEdit || canRead`, so a single slip on one of the run/stop controls would have handed a read-only member the ability to trigger workflow runs from the panel. Verified zero `grants.run` references survive in the tables tree outside one TSDoc that explains the hazard. - 4 `userPermissions.canAdmin` -> `grants.manage`, all of them lock settings. - 2 `userPermissions.isLoading` -> `!grants.settled`. This is the one that needed the axis extension: the lock notice is a one-shot latched on `announcedLockTableIdRef`, so firing it before capabilities resolve permanently yields a toast whose "Lock settings" action is missing. `page.tsx` is a Server Component and cannot read a React context, so the table route gains a ~40-line client shell (`table-route.tsx`) that resolves the axes and mounts the view — the same shape as `fullscreen-file-view.tsx`. Reading `useParams()` there is legitimate where it was not inside the table: a route shell exists exactly once per page by definition, which is precisely the property the table lost when the panel started mounting it too. The panel already computed `grants` for its file and log branches; the table branch now passes the same value.
Step 5. Two `router.push` calls, both to the tables list, both hand-building
`/workspace/${workspaceId}/tables`.
The table now builds its own `workspaceSource` and asks it for the target —
`source.hrefFor({ to: 'list' })`, the member added for exactly this — and hands
the result to an `onNavigate` prop. The route table stays in one file, and a
host that owns no router simply omits the prop, which makes navigation inert by
construction rather than by a check at each call site.
The source is built inside the table rather than taken as a prop because
`page.tsx` is a Server Component and a source carries closures, which cannot
cross the RSC boundary. That is why the axes reach the table through its client
shell in the first place.
Both hosts supply `onNavigate`: the route shell as `router.push`, the panel
reusing the `navigate` callback its file and log branches already pass.
Step 6. The table borrowed the logs page's `LogDetails` shell to show a run. That shell reads `useParams`, `useRouter`, `useQueryState` and the permission context — four route couplings a table has no use for — and it hard-codes `host='page'`. The `useQueryState` is the interesting one: it deep-links the log's active tab unconditionally, so opening a run from the *tables* page wrote `?tab=trace` onto the tables URL, and onto the host page's URL from the chat panel. A second instance of the same bug step 3 fixed, in a component nobody thought of as URL-owning. `ExecutionSlideout` reproduces only the chrome the table actually uses — frame, resize handle, close button — and mounts the canonical `LogView` itself. It omits `tab`/`onTabChange`, so the view keeps the tab local, which is correct for a panel that was never addressable. The logs page's prev/next and retry controls are dropped deliberately: they navigate a list this surface does not have, and the table only ever passed three of that shell's ten props. `showExecutionInternals` becomes a required prop on the table, forwarded from both hosts. Not read from `usePermissionConfig` inside the table — although R6 would allow it — because passing R6 by an accident of regex coverage is not the same as being right, and a permission-group restriction should fail to compile when a host forgets it rather than default to revealing payloads. Cross-route edges out of the tables tree: 3 left, and only one of them is real.
Step 7. The table implementation moves out of the route tree and into `components/resources/table-view/`, joining the layout the unit already had: `table-view.tsx`, `index.ts`, `components/<child>/`, `hooks/`, `utils/`, `types.ts`. What stays behind is four route files — `page.tsx`, `loading.tsx`, `error.tsx`, and the ~50-line client shell that resolves the axes. `Table` becomes `TableView` and takes `source: ResourceSource<'table'>` instead of `workspaceId` + `tableId`. Both are banned prop names, and registering the view in `CANONICAL_UNITS` turns R4a/R4b on to say so — the gate found them immediately. The subtree below still takes plain ids, so the source is narrowed once at the top through `tableWorkspaceId`/`tableResourceId`, which is the helper deleted earlier for having no caller, restored now that it has one. The four preceding steps are what made this a move rather than a rewrite: with the router, the params and the permission context already severed, R6 was already 0 before a single file changed directory. 61 files, ~160 lines of non-import churn. One edge stays and is annotated: the workflow-column sidebar renders the editor's read-only canvas, which reads `useParams` and pulls in the editor's node and edge components. Safe for the same reason the log view's frozen canvas is — `ResourceSeedMap['table']` is `never`, so no anonymous surface can mount this unit — and `CROSS_TREE_ALLOWLIST` now names two files pointing at one component, which is the signal that `Preview` itself wants to move to `components/`. That is a separate change. Every rule back at baseline with the view registered: R2 0, R3c 0, R4a 0, R4b 0, R6 0.
The rules, CLAUDE.md and the migration doc all still described `table` as the holdout and the table unit as "view layer only". Corrects the status table, and two claims that are now wrong in a way that would mislead: - "A view is read-only; the shell writes." That was a staging point during the migration, never a rule — `FileView` edited from the start, and `TableView` does now. What a view must not hold is route context, not mutations. - "`table-grid.tsx` never moved, and did not need to." True of the first pass. It moved in the second, intact, and the ordering is the lesson worth keeping: the context reads were severed *in place* first, so R6 was already 0 before a single file changed directory. The 61-file move then carried no semantic risk. Reversed, four behavioural edits would have been buried inside a rename diff. Also drops the "mothership still mounts the whole editing page" note from the open list — it mounts `TableView` against a source now, like files and logs.
…nits use Two deviations this migration introduced, found by diffing all four views' prop surfaces against each other rather than reading them one at a time. - The axes did not lead the interface. `source` had replaced `workspaceId` + `tableId` in the slot those occupied at the bottom, so the view declared `host, grants, … source`. Every other unit reads `source, grants, host` first. - Two loose optional feature booleans. The rule says collapse into one object and never add a loose prop, and the optional default was load-bearing in the wrong direction: nobody decided the embedded table has no lock settings — `tableLocksEnabled?: boolean` decided it by omission. `features: TableViewFeatures` is required, so the panel now states that answer in a comment instead of inheriting it from a default. Both fields disappear together when the flags GA, which is the other reason they travel as one object rather than spread. Behaviour is unchanged: the panel passes `locks: false`, which is what the absent prop already resolved to.
Same ordering that made the table move safe: the behavioural edits land first,
as a diff against files that have not moved.
- `useParams` -> `knowledgeWorkspaceId(source)`. The shell took `workspaceId?`
optionally and fell back to route params, which the page relied on — so the
fallback was load-bearing, and it is what stopped this surface existing more
than once per page.
- `useRouter` -> `onNavigate`. Three pushes: the document detail route, the
post-delete redirect, and the breadcrumb root. The latter two already went
through `hrefFor({ to: 'list' })`; the first now goes through a new
`knowledgeDocumentHref`, which lives in a per-kind module because a document
is not a `ResourceKind` — it has no view, no grants and no share surface, so
it cannot be spelled through `hrefFor({ to: 'resource' })`.
- `useUserPermissionsContext` -> `grants`. 13 reads, including two of the form
`canEdit || isLoading` that gate whether a breadcrumb dropdown renders at all.
Those become `grants.write || !grants.settled` — the exact pattern `settled`
was added for, and without it the caret would have popped in after load
instead of rendering disabled.
Three supporting relocations, all pure moves:
- `knowledge/components/icons` deleted — byte-identical to
`@/components/icons/document-icons`, confirmed with `diff`.
- `use-knowledge-upload` -> `@/hooks/kb/`.
- `useContextMenu` -> `components/anchored-context-menu/`, beside the component
every one of its 13 consumers pairs it with.
Also makes table-view's internal imports absolute. They arrived relative with
the move and I did not rewrite them; log-view and knowledge-view have zero
relative imports, so table-view was the outlier against the rule this PR
documents. (file-view has 83 and is left alone — pre-existing, and not worth
83 files of churn here.)
The panel now mounts `KnowledgeView` and imports nothing from the workspace route tree. That was the last of the four kinds. The shape of this one differs from tables, and the difference is the useful part: the route shell owned the *whole* surface — header, breadcrumbs, ten modals, bulk operations, the context menu — while the extracted `KnowledgeView` was only the document list. So the shell became the view, and the previous view became its `DocumentList` child. Promoting the read-only half and leaving the mutations behind would have left the panel importing the route tree forever. Dropped `id` from the props: `source` already carries it (`knowledgeResourceId`), and two spellings of the same identity is exactly what the axes replace. `knowledgeBaseName` stays, documented as a title hint rather than identity — it is what keeps both hosts from flashing a placeholder header before load. Four boundary violations surfaced by `check:resources` once the files were inside a unit, each fixed at the cause rather than annotated: - `AddDocumentsModal` read `useParams()` for the workspace id. Inside the panel there is no `[workspaceId]` segment, so this was already returning undefined there — the modal is reachable from `/home`. Now a required prop. - `use-connector-config-fields` was route-tree-resident but is unit-internal; moved to `@/hooks/kb/`. - `BreadcrumbItem`/`ResourceAction` came from the route barrel that merely re-exports `@/components/resource`. Retargeted to the source. - The knowledge list page and the document route imported past the unit barrel. They render the same tag editor and selection bar against the same data, so the barrel exports those three children — and the rule now says when that is allowed, because "saves an import hop" must not qualify. `ConnectOAuthModal` and `WorkspaceHostProvider` looked like they pinned the connector modals to the route tree. Neither is route-specific: both moved to `components/` (with `WorkspaceAccessDenied`, which the provider renders), 22 importers repointed. Deleting the blocker beat splitting the surface around it. nuqs stays outside the unit — R6 has no escape hatch — so `use-knowledge-list-state` moved to `@/hooks/kb/` and its parsers to `@/lib/knowledge/detail-search-params`, mirroring the table. Suite: 20917 passed, unchanged from before the move. R6 and R3c both 0.
Six conflicts, all one cause: staging's unbolding sweep (#6400) landed on code this branch had moved out of the route tree. Resolved by keeping this branch's structure and replaying every one of staging's 21 `font-medium` removals at the element's new home — 16 applied, the rest already absorbed by the auto-merge. Verified none of the strings staging unbolded is still bold anywhere in the tree. One deliberate deviation: staging rewrote the file-preview fallback as `text-[14px]`, which the styling rule forbids (font-size only, inherits the wrong line-height). Took staging's intent — dropping `font-medium` — and kept the named `text-sm`, which is the same 14px. `base.tsx` and `file-viewer.tsx` came back as delete/modify conflicts because they no longer exist here; their changes were ported into `document-list.tsx` and the file-view unit rather than dropped. Suite: 20951 passed. R6/R3c 0, 23 audits green.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Every resource — file, table, log, knowledge — now has exactly one view, and every consumer mounts that one. The workspace page and the mothership panel render the same component; neither imports the other's implementation.
apps/sim/resources/(pure TS, no React) replace the ad-hoc props:source(workspace vs share, discriminated onvia),grants(write/run/manage/settled),host(page/panel/public)@/app/workspace/[workspaceId]/{tables,knowledge,files,logs}— that coupling is what the axes exist to deletescripts/check-resource-views.tsenforces it: no wrappers, no imports past a unit barrel, no route/permission context inside a unit, noworkspaceId/canEdit/embeddedprops on a view. R6 has no annotation escape hatch by designworkspaceId, and a kind whose seed isnever(table, knowledge, log) cannot be constructed anonymously at all — compile-time facts, not conventionstables and knowledge were the two remaining shells. Both were severed from route context in place first (router →
onNavigate, params →source, permission context →grants), so every semantic edit is reviewable as a diff against unmoved files and the move commits are import-only.Intended behavior changes
Two, both fixing the panel writing to its host's URL:
sort/dir/table-viewinto/home's address bar. Both route pages keep their deep-linkable params unchanged?tab=traceonto the tables URLEverything else is byte-identical.
grants.settledexists specifically socanEdit || isLoadinggating survives the migration exactly rather than flickering on first paint.Type of Change
Testing
20951 tests passing.
check:resources:strict,check:audits(23), type-check and lint green. Not yet browser-verified — the two URL-ownership changes and the header chrome on each surface are the pieces worth clicking through.Checklist