Skip to content

Commit 55287e0

Browse files
committed
refactor(workspaces): store workspace pins in pinned_item, not user settings
Workspace pins were a jsonb array on the settings row, replaced wholesale on every toggle. That shape is what forced the write serialization in 53ee94f: two overlapping toggles each sent the entire list, so the one that landed last won regardless of which the user clicked last. pinned_item is the canonical pinning table and its resource_type is plain text precisely so kinds can be added without a migration, so `workspace` joins it as a sixth kind. A pin is now one row: pinning inserts, unpinning deletes, and two toggles touch different rows and cannot overwrite each other. The serialization, the outstanding-write counter, the settings column, and its migration all go away, and deleting a workspace now cascades its pins. Reads stay on the /api/workspaces payload — the switcher needs the pins *of* every workspace, not the pins *inside* one — so the sidebar prefetch still hydrates them and pinned-first ordering is correct on first paint.
1 parent 53ee94f commit 55287e0

12 files changed

Lines changed: 154 additions & 18956 deletions

File tree

apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-workspace-management.ts

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -117,19 +117,13 @@ export function useWorkspaceManagement({
117117
const pinnedWorkspaceIdsRef = useRef<Set<string>>(pinnedWorkspaceIds)
118118
pinnedWorkspaceIdsRef.current = pinnedWorkspaceIds
119119

120-
/**
121-
* Reads the current pins through a ref so the callback identity stays stable for
122-
* the memoized switcher, and derives the whole next list here — the settings
123-
* endpoint replaces it wholesale, and this hook is the one place that already
124-
* owns the set.
125-
*/
120+
/** Reads the current pins through a ref so the callback identity stays stable for the memoized switcher. */
126121
const toggleWorkspacePin = useCallback(
127122
(workspaceId: string) => {
128-
const current = pinnedWorkspaceIdsRef.current
129-
const pinnedWorkspaceIds = current.has(workspaceId)
130-
? [...current].filter((id) => id !== workspaceId)
131-
: [...current, workspaceId]
132-
toggleWorkspacePinMutate({ pinnedWorkspaceIds })
123+
toggleWorkspacePinMutate({
124+
workspaceId,
125+
pinned: !pinnedWorkspaceIdsRef.current.has(workspaceId),
126+
})
133127
},
134128
[toggleWorkspacePinMutate]
135129
)

apps/sim/hooks/queries/workspace.test.tsx

Lines changed: 73 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ vi.mock('@/lib/api/client/request', () => ({
1515
requestJson: mockRequestJson,
1616
}))
1717

18+
import { ApiClientError } from '@/lib/api/client/errors'
19+
import { createPinnedItemContract, deletePinnedItemContract } from '@/lib/api/contracts'
1820
import { useToggleWorkspacePin, workspaceKeys } from '@/hooks/queries/workspace'
1921

2022
/** Trees rendered by a test, torn down in afterEach so observers do not leak across tests. */
@@ -62,6 +64,20 @@ async function flush() {
6264
})
6365
}
6466

67+
function seedList(queryClient: QueryClient, pinnedWorkspaceIds: string[]) {
68+
queryClient.setQueryData(workspaceKeys.list('active'), {
69+
workspaces: [],
70+
lastActiveWorkspaceId: null,
71+
pinnedWorkspaceIds,
72+
creationPolicy: null,
73+
})
74+
}
75+
76+
function readPins(queryClient: QueryClient): string[] | undefined {
77+
return queryClient.getQueryData<{ pinnedWorkspaceIds: string[] }>(workspaceKeys.list('active'))
78+
?.pinnedWorkspaceIds
79+
}
80+
6581
afterEach(() => {
6682
act(() => {
6783
for (const root of mountedRoots.splice(0)) root.unmount()
@@ -73,96 +89,98 @@ beforeEach(() => {
7389
})
7490

7591
describe('useToggleWorkspacePin', () => {
76-
/**
77-
* Each request carries the whole pin list, so overlapping writes that the network
78-
* delivers out of order would let the earlier click win. The hook chains them, so
79-
* the second request must not be issued until the first has resolved.
80-
*/
81-
it('serializes overlapping writes so the last click is what persists', async () => {
82-
const resolvers: Array<() => void> = []
83-
mockRequestJson.mockImplementation(
84-
() =>
85-
new Promise<{ success: true }>((resolve) => {
86-
resolvers.push(() => resolve({ success: true }))
87-
})
88-
)
89-
90-
const { getResult } = renderHookWithClient(() => useToggleWorkspacePin())
92+
it('pins by creating a row addressed to the workspace itself', async () => {
93+
mockRequestJson.mockResolvedValue({ pinnedItem: {} })
94+
const { getResult, queryClient } = renderHookWithClient(() => useToggleWorkspacePin())
95+
seedList(queryClient, [])
9196

9297
act(() => {
93-
getResult().mutate({ pinnedWorkspaceIds: ['ws-a'] })
94-
getResult().mutate({ pinnedWorkspaceIds: ['ws-a', 'ws-b'] })
98+
getResult().mutate({ workspaceId: 'ws-a', pinned: true })
9599
})
96100
await flush()
97101

98-
// Only the first write is on the wire; the second is queued behind it.
99-
expect(mockRequestJson).toHaveBeenCalledOnce()
100-
expect(mockRequestJson.mock.calls[0][1]).toMatchObject({
101-
body: { pinnedWorkspaceIds: ['ws-a'] },
102+
expect(mockRequestJson).toHaveBeenCalledWith(createPinnedItemContract, {
103+
body: { workspaceId: 'ws-a', resourceType: 'workspace', resourceId: 'ws-a' },
102104
})
105+
})
106+
107+
it('unpins by deleting that row', async () => {
108+
mockRequestJson.mockResolvedValue({ success: true })
109+
const { getResult, queryClient } = renderHookWithClient(() => useToggleWorkspacePin())
110+
seedList(queryClient, ['ws-a'])
103111

104-
act(() => resolvers[0]())
112+
act(() => {
113+
getResult().mutate({ workspaceId: 'ws-a', pinned: false })
114+
})
105115
await flush()
106116

107-
expect(mockRequestJson).toHaveBeenCalledTimes(2)
108-
expect(mockRequestJson.mock.calls[1][1]).toMatchObject({
109-
body: { pinnedWorkspaceIds: ['ws-a', 'ws-b'] },
117+
expect(mockRequestJson).toHaveBeenCalledWith(deletePinnedItemContract, {
118+
params: { resourceType: 'workspace', resourceId: 'ws-a' },
110119
})
120+
expect(readPins(queryClient)).toEqual([])
111121
})
112122

113123
/**
114-
* Refetching between two queued writes would render the server's pre-second-write
115-
* state and visibly bounce the row out of the pinned group and back.
124+
* Two rapid toggles write different rows, so neither can overwrite the other and
125+
* both survive regardless of the order the requests land in.
116126
*/
117-
it('reconciles only after the last queued write settles', async () => {
127+
it('keeps both pins when two toggles overlap', async () => {
118128
const resolvers: Array<() => void> = []
119129
mockRequestJson.mockImplementation(
120-
() =>
121-
new Promise<{ success: true }>((resolve) => {
122-
resolvers.push(() => resolve({ success: true }))
123-
})
130+
() => new Promise((resolve) => resolvers.push(() => resolve({ pinnedItem: {} })))
124131
)
125-
126132
const { getResult, queryClient } = renderHookWithClient(() => useToggleWorkspacePin())
127-
const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries')
133+
seedList(queryClient, [])
128134

129135
act(() => {
130-
getResult().mutate({ pinnedWorkspaceIds: ['ws-a'] })
131-
getResult().mutate({ pinnedWorkspaceIds: ['ws-a', 'ws-b'] })
136+
getResult().mutate({ workspaceId: 'ws-a', pinned: true })
137+
getResult().mutate({ workspaceId: 'ws-b', pinned: true })
132138
})
133139
await flush()
134140

135-
act(() => resolvers[0]())
141+
expect(readPins(queryClient)).toEqual(['ws-a', 'ws-b'])
142+
143+
// Resolve out of order — the older request landing last must not undo the newer.
144+
act(() => {
145+
resolvers[1]()
146+
resolvers[0]()
147+
})
136148
await flush()
137149

138-
// First write settled, second still outstanding — nothing reconciled yet.
139-
expect(invalidateSpy).not.toHaveBeenCalled()
150+
const bodies = mockRequestJson.mock.calls.map((call) => call[1].body.resourceId)
151+
expect(bodies).toEqual(['ws-a', 'ws-b'])
152+
expect(readPins(queryClient)).toEqual(['ws-a', 'ws-b'])
153+
})
140154

141-
act(() => resolvers[1]())
155+
/** Re-pinning an already-pinned workspace is the desired end state, not a failure. */
156+
it('treats a 409 from a duplicate pin as success', async () => {
157+
mockRequestJson.mockRejectedValue(
158+
new ApiClientError({ status: 409, message: 'This item is already pinned', body: {} })
159+
)
160+
const { getResult, queryClient } = renderHookWithClient(() => useToggleWorkspacePin())
161+
seedList(queryClient, [])
162+
163+
act(() => {
164+
getResult().mutate({ workspaceId: 'ws-a', pinned: true })
165+
})
142166
await flush()
143167

144-
expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: workspaceKeys.lists() })
168+
expect(getResult().isError).toBe(false)
169+
expect(readPins(queryClient)).toEqual(['ws-a'])
145170
})
146171

147-
it('applies the pin optimistically before the request resolves', async () => {
148-
mockRequestJson.mockImplementation(() => new Promise<{ success: true }>(() => {}))
149-
172+
it('rolls the optimistic pin back when the write fails', async () => {
173+
mockRequestJson.mockRejectedValue(
174+
new ApiClientError({ status: 500, message: 'boom', body: {} })
175+
)
150176
const { getResult, queryClient } = renderHookWithClient(() => useToggleWorkspacePin())
151-
queryClient.setQueryData(workspaceKeys.list('active'), {
152-
workspaces: [],
153-
lastActiveWorkspaceId: null,
154-
pinnedWorkspaceIds: [],
155-
creationPolicy: null,
156-
})
177+
seedList(queryClient, [])
157178

158179
act(() => {
159-
getResult().mutate({ pinnedWorkspaceIds: ['ws-a'] })
180+
getResult().mutate({ workspaceId: 'ws-a', pinned: true })
160181
})
161182
await flush()
162183

163-
expect(
164-
queryClient.getQueryData<{ pinnedWorkspaceIds: string[] }>(workspaceKeys.list('active'))
165-
?.pinnedWorkspaceIds
166-
).toEqual(['ws-a'])
184+
expect(readPins(queryClient)).toEqual([])
167185
})
168186
})

apps/sim/hooks/queries/workspace.ts

Lines changed: 37 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,17 @@
1-
import { useRef } from 'react'
21
import type { QueryClient } from '@tanstack/react-query'
32
import { keepPreviousData, useMutation, useQuery, useQueryClient } from '@tanstack/react-query'
43
import { ApiClientError } from '@/lib/api/client/errors'
54
import { requestJson } from '@/lib/api/client/request'
65
import type { ContractBodyInput } from '@/lib/api/contracts'
76
import {
7+
createPinnedItemContract,
88
createWorkspaceContract,
9+
deletePinnedItemContract,
910
deleteWorkspaceContract,
1011
getWorkspaceContract,
1112
getWorkspaceMembersContract,
1213
getWorkspacePermissionsContract,
1314
listWorkspacesContract,
14-
updateUserSettingsContract,
1515
updateWorkspaceContract,
1616
type Workspace,
1717
type WorkspaceCreationPolicy,
@@ -20,6 +20,7 @@ import {
2020
type WorkspaceQueryScope,
2121
type WorkspacesResponse,
2222
} from '@/lib/api/contracts'
23+
import { pinnedItemKeys } from '@/hooks/queries/utils/pinned-item-keys'
2324
import {
2425
normalizeWorkspace,
2526
normalizeWorkspacesResponse,
@@ -119,62 +120,57 @@ export function usePinnedWorkspaceIds(enabled = true) {
119120
}
120121

121122
/**
122-
* Persists the viewer's pinned workspaces.
123+
* Pins or unpins a workspace in the switcher.
123124
*
124-
* The settings endpoint replaces the list wholesale, so the caller passes the full
125-
* set it wants rather than a delta. Optimistic because the pin re-sorts the list
126-
* under the user's cursor, where a round-trip delay reads as a dropped click.
125+
* Writes one `pinned_item` row per pin, so a pin is an insert and an unpin is a
126+
* delete. Two rapid toggles touch different rows and cannot overwrite each other,
127+
* which is why nothing here serializes or debounces. Re-pinning an already-pinned
128+
* workspace answers 409 and unpinning an absent pin is a no-op, so a duplicate
129+
* click is idempotent rather than an error.
130+
*
131+
* Optimistic against the workspace list, because that is where the pins are read
132+
* and the toggle re-sorts the row under the user's cursor.
127133
*/
128134
export function useToggleWorkspacePin() {
129135
const queryClient = useQueryClient()
130136
const queryKey = workspaceKeys.list('active')
131-
/**
132-
* Tail of the in-flight write chain, and the count of toggles still outstanding.
133-
* A burst of clicks is a burst of independent mutations, and this endpoint takes
134-
* the whole list — so both are needed, for the two different ways that races.
135-
*/
136-
const writeChainRef = useRef<Promise<unknown>>(Promise.resolve())
137-
const outstandingRef = useRef(0)
138137

139138
return useMutation({
140-
/**
141-
* Chained rather than fired concurrently: each request carries the complete
142-
* list, so if two overlap and the network delivers them out of order the older
143-
* one lands last and silently undoes the newer click. Serializing makes the
144-
* final stored state the last one the user actually asked for.
145-
*/
146-
mutationFn: ({ pinnedWorkspaceIds }: { pinnedWorkspaceIds: string[] }) => {
147-
const send = writeChainRef.current
148-
.catch(() => {})
149-
.then(() => requestJson(updateUserSettingsContract, { body: { pinnedWorkspaceIds } }))
150-
writeChainRef.current = send
151-
return send
139+
mutationFn: async ({ workspaceId, pinned }: { workspaceId: string; pinned: boolean }) => {
140+
if (!pinned) {
141+
await requestJson(deletePinnedItemContract, {
142+
params: { resourceType: 'workspace', resourceId: workspaceId },
143+
})
144+
return
145+
}
146+
try {
147+
await requestJson(createPinnedItemContract, {
148+
body: { workspaceId, resourceType: 'workspace', resourceId: workspaceId },
149+
})
150+
} catch (error) {
151+
/** Already pinned — the desired state, so not a failure to roll back. */
152+
if (error instanceof ApiClientError && error.status === 409) return
153+
throw error
154+
}
152155
},
153-
onMutate: async ({ pinnedWorkspaceIds }) => {
154-
outstandingRef.current += 1
156+
onMutate: async ({ workspaceId, pinned }) => {
155157
await queryClient.cancelQueries({ queryKey })
156158
const previous = queryClient.getQueryData<WorkspacesResponse>(queryKey)
157-
queryClient.setQueryData<WorkspacesResponse>(queryKey, (old) =>
158-
old ? { ...old, pinnedWorkspaceIds } : old
159-
)
159+
queryClient.setQueryData<WorkspacesResponse>(queryKey, (old) => {
160+
if (!old) return old
161+
const next = pinned
162+
? [...old.pinnedWorkspaceIds.filter((id) => id !== workspaceId), workspaceId]
163+
: old.pinnedWorkspaceIds.filter((id) => id !== workspaceId)
164+
return { ...old, pinnedWorkspaceIds: next }
165+
})
160166
return { previous }
161167
},
162168
onError: (_error, _variables, context) => {
163169
if (context?.previous) queryClient.setQueryData(queryKey, context.previous)
164170
},
165-
/**
166-
* Reconciles only once the last queued write has settled. Refetching while
167-
* another is still chained would render the server's pre-that-write state and
168-
* visibly bounce the row out of the pinned group and back.
169-
*
170-
* Reconciling at all is not optional: the settings route answers
171-
* `{ success: true }` even when the write throws, so a failed save never
172-
* reaches `onError` and the optimistic list would otherwise stay wrong.
173-
*/
174171
onSettled: () => {
175-
outstandingRef.current -= 1
176-
if (outstandingRef.current > 0) return
177172
queryClient.invalidateQueries({ queryKey: workspaceKeys.lists() })
173+
queryClient.invalidateQueries({ queryKey: pinnedItemKeys.all })
178174
},
179175
})
180176
}

apps/sim/lib/api/contracts/pinned-items.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,20 @@ import { defineRouteContract } from '@/lib/api/contracts/types'
1111
* namespace serves the file, knowledge-base, and table trees. It is separate from the
1212
* resource's own type, which is why a page listing folders alongside its resources resolves
1313
* two `usePinnedIds` sets.
14+
*
15+
* `workspace` pins the workspace itself, so its row stores `workspaceId === resourceId`.
16+
* It is the one kind that is not read back through `GET /api/pinned-items`: the switcher
17+
* needs every workspace the viewer pinned, not the pins inside one workspace, so those ids
18+
* ride along on the `/api/workspaces` payload it already loads. Writes still go through the
19+
* pin routes below, which is what keeps pin/unpin a per-row delta.
1420
*/
1521
export const pinnedResourceTypeSchema = z.enum([
1622
'workflow',
1723
'file',
1824
'knowledge_base',
1925
'table',
2026
'folder',
27+
'workspace',
2128
])
2229
export type PinnedResourceType = z.output<typeof pinnedResourceTypeSchema>
2330

apps/sim/lib/api/contracts/user.ts

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -114,16 +114,6 @@ export const updateUserSettingsBodySchema = z.object({
114114
timezone: ianaTimezoneSchema.nullable().optional(),
115115
/** Mirrors `userSettingsSchema.lastActiveWorkspaceId` so explicit `null` is accepted to clear the active workspace. */
116116
lastActiveWorkspaceId: z.string().nullable().optional(),
117-
/**
118-
* Replaces the pinned-workspace list wholesale — the client sends the full set
119-
* it wants, so pinning and unpinning are the same write. Write-only: the list is
120-
* read back on the `/api/workspaces` payload, alongside the workspaces it orders.
121-
* Bounded because it is an unvalidated client-supplied array persisted verbatim.
122-
*/
123-
pinnedWorkspaceIds: z
124-
.array(z.string().min(1, 'Workspace ID cannot be empty').max(255, 'Workspace ID is too long'))
125-
.max(200, 'Too many pinned workspaces')
126-
.optional(),
127117
})
128118

129119
export const getUserSettingsContract = defineRouteContract({

apps/sim/lib/api/contracts/workspaces.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -193,10 +193,10 @@ export const listWorkspacesContract = defineRouteContract({
193193
workspaces: z.array(workspaceSchema),
194194
lastActiveWorkspaceId: z.string().nullable(),
195195
/**
196-
* The viewer's pinned workspace ids as stored. May name workspaces absent
197-
* from `workspaces` (archived, or access since removed) — clients look pins
198-
* up per rendered workspace, so unmatched ids are inert, and preserving them
199-
* is what keeps a pin from being dropped by the next wholesale write.
196+
* Workspace ids the viewer pinned in the switcher, from `pinned_item`. May
197+
* name workspaces absent from `workspaces` (archived, or access since
198+
* removed); clients look pins up per rendered workspace, so unmatched ids
199+
* are inert and the pin survives if access is restored.
200200
*/
201201
pinnedWorkspaceIds: z.array(z.string()).default([]),
202202
creationPolicy: workspaceCreationPolicySchema.nullable(),

0 commit comments

Comments
 (0)