Skip to content

Commit fdb037f

Browse files
committed
fix(tables): stop the embedded table writing sort and view into its host'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.
1 parent db1b08f commit fdb037f

4 files changed

Lines changed: 241 additions & 30 deletions

File tree

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx

Lines changed: 9 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import { Download, Lock, Pencil, Table as TableIcon, Trash, Upload } from '@sim/
66
import { createLogger } from '@sim/logger'
77
import { getErrorMessage } from '@sim/utils/errors'
88
import { useRouter } from 'next/navigation'
9-
import { useQueryStates } from 'nuqs'
109
import { usePostHog } from 'posthog-js/react'
1110
import { PresenceAvatars } from '@/components/presence'
1211
import {
@@ -35,6 +34,10 @@ import type {
3534
} from '@/lib/table'
3635
import { getColumnId } from '@/lib/table/column-keys'
3736
import { TABLE_LIMITS } from '@/lib/table/constants'
37+
import {
38+
ALL_VIEW_PARAM,
39+
DEFAULT_TABLE_DETAIL_SORT_DIRECTION,
40+
} from '@/lib/table/detail-search-params'
3841
import { LogDetails } from '@/app/workspace/[workspaceId]/logs/components'
3942
import { useUserPermissionsContext } from '@/app/workspace/[workspaceId]/providers/workspace-permissions-provider'
4043
import { useLogByExecutionId } from '@/hooks/queries/logs'
@@ -54,6 +57,7 @@ import {
5457
} from '@/hooks/queries/tables'
5558
import { useInlineRename } from '@/hooks/use-inline-rename'
5659
import { useSettingsNavigation } from '@/hooks/use-settings-navigation'
60+
import { useTableDetailState } from '@/hooks/use-table-detail-state'
5761
import { hostOwnsUrl, type ResourceHost } from '@/resources'
5862
import { useLogDetailsUIStore } from '@/stores/logs/store'
5963
import type { DeletedRowSnapshot } from '@/stores/table/types'
@@ -78,12 +82,6 @@ import {
7882
} from './components'
7983
import { useTable, useTableEventStream, useTableRoom } from './hooks'
8084
import { type BlockedTableAction, describeBlockedAction, lockedNouns } from './lock-copy'
81-
import {
82-
ALL_VIEW_PARAM,
83-
DEFAULT_TABLE_DETAIL_SORT_DIRECTION,
84-
tableDetailParsers,
85-
tableDetailUrlKeys,
86-
} from './search-params'
8785
import type { QueryOptions } from './types'
8886

8987
const logger = createLogger('Table')
@@ -293,7 +291,7 @@ export function Table({
293291
const [hiddenColumns, setHiddenColumns] = useState<string[]>([])
294292

295293
const [{ sort: sortColumn, dir: sortDirection, view: activeViewId }, setTableParams] =
296-
useQueryStates(tableDetailParsers, tableDetailUrlKeys)
294+
useTableDetailState({ host })
297295

298296
// Read-only mirrors for the resolve effect: it must know whether the user has
299297
// already applied a filter / hidden columns without re-running when they change.
@@ -551,24 +549,9 @@ export function Table({
551549
ownerResolvedRef.current = true
552550

553551
if (seededViewIdRef.current === undefined) {
554-
// Embedded tables bind these parsers to the HOST page's URL, which the
555-
// mothership panel keeps across resource switches. A view id this table
556-
// can't resolve was left by the previously-open resource — ignore it so
557-
// this table picks its own default. A param it CAN resolve is honoured,
558-
// including an explicit All: that is a real bookmark or a remount after
559-
// switching resources away and back, not leakage.
560-
const inheritedParams =
561-
embedded &&
562-
activeViewId !== null &&
563-
activeViewId !== ALL_VIEW_PARAM &&
564-
!views.some((view) => view.id === activeViewId)
565-
566-
if (activeViewId === null || inheritedParams) {
552+
if (activeViewId === null) {
567553
const defaultView = views.find((view) => view.isDefault)
568-
// `sort` rides the same host URL, so when the view id is inherited the
569-
// sort beside it is too — not local work, and it must not suppress the
570-
// default view's own sort.
571-
const keep = inheritedParams ? { ...localWork(), sort: false } : localWork()
554+
const keep = localWork()
572555
if (defaultView) {
573556
seededViewIdRef.current = defaultView.id
574557
setTableParams({ view: defaultView.id })
@@ -577,10 +560,8 @@ export function Table({
577560
return
578561
}
579562
// No view to adopt. Deliberately does NOT apply an empty config — that
580-
// would clear a deep-linked `?sort=` on mount. Inherited params are the
581-
// exception: nothing about them refers to this table, so they're cleared.
563+
// would clear a deep-linked `?sort=` on mount.
582564
seededViewIdRef.current = null
583-
if (inheritedParams) setTableParams({ view: ALL_VIEW_PARAM, sort: null, dir: null })
584565
resolvePendingLayout(false)
585566
return
586567
}
@@ -639,7 +620,6 @@ export function Table({
639620
views,
640621
activeView,
641622
activeViewId,
642-
embedded,
643623
sortColumn,
644624
applyViewConfig,
645625
setTableParams,
Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*
4+
* The panel must never write the table's sort/view keys into its host page's
5+
* address bar. Before this hook, `useQueryStates` was called unconditionally and
6+
* all eleven writers ran in both hosts — the `view` -> `table-view` wire-key
7+
* rename exists precisely because the mothership bound these parsers to `/home`.
8+
*/
9+
import { act } from 'react'
10+
import { createRoot, type Root } from 'react-dom/client'
11+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
12+
13+
const { setUrlState, urlState } = vi.hoisted(() => ({
14+
setUrlState: vi.fn(),
15+
urlState: { current: { sort: null as string | null, dir: 'asc', view: null as string | null } },
16+
}))
17+
18+
vi.mock('nuqs', () => ({
19+
useQueryStates: () => [urlState.current, setUrlState],
20+
}))
21+
22+
import {
23+
type TableDetailState,
24+
type TableDetailUpdate,
25+
useTableDetailState,
26+
} from '@/hooks/use-table-detail-state'
27+
import type { ResourceHost } from '@/resources'
28+
29+
let container: HTMLDivElement
30+
let root: Root
31+
let latest: [TableDetailState, (update: TableDetailUpdate) => void] | null = null
32+
33+
function Probe({ host }: { host: ResourceHost }) {
34+
latest = useTableDetailState({ host })
35+
return null
36+
}
37+
38+
function render(host: ResourceHost) {
39+
act(() => root.render(<Probe host={host} />))
40+
}
41+
42+
beforeEach(() => {
43+
globalThis.IS_REACT_ACT_ENVIRONMENT = true
44+
vi.clearAllMocks()
45+
urlState.current = { sort: null, dir: 'asc', view: null }
46+
container = document.createElement('div')
47+
document.body.appendChild(container)
48+
root = createRoot(container)
49+
})
50+
51+
afterEach(() => {
52+
act(() => root.unmount())
53+
container.remove()
54+
})
55+
56+
describe('a host that owns the URL', () => {
57+
it('reads the query params', () => {
58+
urlState.current = { sort: 'name', dir: 'desc', view: 'view_1' }
59+
render('page')
60+
61+
expect(latest?.[0]).toEqual({ sort: 'name', dir: 'desc', view: 'view_1' })
62+
})
63+
64+
it('writes through to nuqs', () => {
65+
render('page')
66+
67+
act(() => latest?.[1]({ sort: 'name', dir: 'desc' }))
68+
69+
expect(setUrlState).toHaveBeenCalledWith({ sort: 'name', dir: 'desc' })
70+
})
71+
})
72+
73+
describe('an embedded host', () => {
74+
it('never writes to the address bar', () => {
75+
render('panel')
76+
77+
act(() => latest?.[1]({ sort: 'name', dir: 'desc' }))
78+
act(() => latest?.[1]({ view: 'view_1' }))
79+
80+
expect(setUrlState).not.toHaveBeenCalled()
81+
})
82+
83+
it('holds the identical values locally', () => {
84+
render('panel')
85+
86+
act(() => latest?.[1]({ sort: 'name', dir: 'desc' }))
87+
expect(latest?.[0]).toEqual({ sort: 'name', dir: 'desc', view: null })
88+
89+
act(() => latest?.[1]({ view: 'view_1' }))
90+
expect(latest?.[0]).toEqual({ sort: 'name', dir: 'desc', view: 'view_1' })
91+
})
92+
93+
/**
94+
* A key already sitting on the host page's URL must not steer the panel. This
95+
* is what the deleted `inheritedParams` guard used to defend against by hand.
96+
*/
97+
it('ignores a value already on the host page URL', () => {
98+
urlState.current = { sort: 'stale', dir: 'desc', view: 'someone_elses_view' }
99+
render('panel')
100+
101+
expect(latest?.[0]).toEqual({ sort: null, dir: 'asc', view: null })
102+
})
103+
104+
/**
105+
* Several writers set multiple keys at once and rely on one atomic update —
106+
* clearing an adopted view writes `{ view, sort, dir }` together. Three
107+
* separate setters would tear midway through the view-resolution latch.
108+
*/
109+
it('applies a multi-key write atomically', () => {
110+
render('panel')
111+
act(() => latest?.[1]({ sort: 'name', dir: 'desc', view: 'view_1' }))
112+
113+
act(() => latest?.[1]({ view: 'all', sort: null, dir: null }))
114+
115+
expect(latest?.[0]).toEqual({ sort: null, dir: 'asc', view: 'all' })
116+
})
117+
118+
it('resets a key to its default when written null, as nuqs does', () => {
119+
render('panel')
120+
act(() => latest?.[1]({ dir: 'desc' }))
121+
expect(latest?.[0].dir).toBe('desc')
122+
123+
act(() => latest?.[1]({ dir: null }))
124+
expect(latest?.[0].dir).toBe('asc')
125+
})
126+
127+
it('leaves untouched keys alone', () => {
128+
render('panel')
129+
act(() => latest?.[1]({ sort: 'name', view: 'view_1' }))
130+
131+
act(() => latest?.[1]({ sort: 'other' }))
132+
133+
expect(latest?.[0]).toEqual({ sort: 'other', dir: 'asc', view: 'view_1' })
134+
})
135+
})
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
'use client'
2+
3+
import { useCallback, useMemo, useState } from 'react'
4+
import { useQueryStates } from 'nuqs'
5+
import {
6+
DEFAULT_TABLE_DETAIL_SORT_DIRECTION,
7+
tableDetailParsers,
8+
tableDetailUrlKeys,
9+
} from '@/lib/table/detail-search-params'
10+
import type { SortDirection } from '@/lib/url-state'
11+
import { hostOwnsUrl, type ResourceHost } from '@/resources'
12+
13+
/** Sort column, direction, and active view id — the table's deep-linkable view state. */
14+
export interface TableDetailState {
15+
sort: string | null
16+
dir: SortDirection
17+
view: string | null
18+
}
19+
20+
/** A partial write. `null` resets a key to its default, exactly as nuqs does. */
21+
export type TableDetailUpdate = Partial<{
22+
sort: string | null
23+
dir: SortDirection | null
24+
view: string | null
25+
}>
26+
27+
const LOCAL_DEFAULTS: TableDetailState = {
28+
sort: null,
29+
dir: DEFAULT_TABLE_DETAIL_SORT_DIRECTION,
30+
view: null,
31+
}
32+
33+
/**
34+
* The table's sort/view state, stored where the host allows.
35+
*
36+
* A host that owns the URL keeps `sort` / `dir` / `table-view` as query params,
37+
* so a table is shareable and survives reload. An embedded host holds the
38+
* identical values locally, because writing unnamespaced keys would pollute the
39+
* address bar of whatever page is hosting the panel — the mothership binds these
40+
* parsers to `/home`, where they belong to the home page and not to whichever
41+
* table happens to be open in a tab.
42+
*
43+
* Both branches are wired unconditionally (hooks may not be called
44+
* conditionally) and only the returned pair differs, matching
45+
* `useKnowledgeListState`. In an embedded host the URL values are read but never
46+
* written, so a key that happens to already be on the host's URL cannot steer
47+
* the panel either.
48+
*
49+
* The local branch is deliberately ONE state object rather than three. Several
50+
* writers set multiple keys in a single call — clearing an inherited view writes
51+
* `{ view, sort, dir }` together — and rely on nuqs batching them into one
52+
* update. Three separate setters would produce a different render count and can
53+
* tear midway through the view-resolution effect's latch.
54+
*/
55+
export function useTableDetailState({
56+
host,
57+
}: {
58+
host: ResourceHost
59+
}): [TableDetailState, (update: TableDetailUpdate) => void] {
60+
const ownsUrl = hostOwnsUrl(host)
61+
62+
const [urlState, setUrlState] = useQueryStates(tableDetailParsers, tableDetailUrlKeys)
63+
const [localState, setLocalState] = useState<TableDetailState>(LOCAL_DEFAULTS)
64+
65+
const setLocal = useCallback((update: TableDetailUpdate) => {
66+
setLocalState((previous) => ({
67+
sort: 'sort' in update ? (update.sort ?? null) : previous.sort,
68+
dir: 'dir' in update ? (update.dir ?? DEFAULT_TABLE_DETAIL_SORT_DIRECTION) : previous.dir,
69+
view: 'view' in update ? (update.view ?? null) : previous.view,
70+
}))
71+
}, [])
72+
73+
const setState = useCallback(
74+
(update: TableDetailUpdate) => {
75+
if (ownsUrl) {
76+
void setUrlState(update)
77+
return
78+
}
79+
setLocal(update)
80+
},
81+
[ownsUrl, setUrlState, setLocal]
82+
)
83+
84+
const state = useMemo<TableDetailState>(
85+
() => (ownsUrl ? { sort: urlState.sort, dir: urlState.dir, view: urlState.view } : localState),
86+
[ownsUrl, urlState.sort, urlState.dir, urlState.view, localState]
87+
)
88+
89+
return [state, setState]
90+
}

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/search-params.ts renamed to apps/sim/lib/table/detail-search-params.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,13 @@ import { SORT_DIRECTIONS } from '@/lib/url-state'
55
export const DEFAULT_TABLE_DETAIL_SORT_DIRECTION = 'asc'
66

77
/**
8-
* Co-located, typed URL query-param definitions for the table-detail view.
8+
* Typed URL query-param definitions for the table-detail view.
9+
*
10+
* In `lib/` rather than co-located with the route because the two halves that
11+
* need them sit on opposite sides of a boundary: the table view is a canonical
12+
* resource unit, which may not call nuqs at all (see `useTableDetailState`), and
13+
* the hook that does own the URL may not import the route tree. A pure,
14+
* server-safe parser module is the one thing both may reach for.
915
*
1016
* - `sort` is the active sort column. Columns are user-defined table columns
1117
* (not a fixed set), so the column id is stored as a free-form string. A

0 commit comments

Comments
 (0)