Skip to content

Commit 5d1fbb7

Browse files
committed
fix(audit-logs): make the placeholder scope-aware, and separate a failed lookup
Two more from review, both on the same surface. The placeholder I added held previous pages across a workspace change, so following a scoped link from the organization-wide feed painted the organization's rows under a workspace-scoped URL until the scoped page arrived — with Export armed against them. Gating presentation on `isPlaceholderData` would have fixed it by throwing away the reason the placeholder exists, blanking the feed on every keystroke again. The scope now leads the query key instead, ahead of the filters, so "hold across a filter change, never across a scope change" is a prefix comparison — the same shape the breakdown query already uses, and it retires the hand-maintained key index. A failed workspace lookup was reported as a workspace that is not part of the organization. That is a wrong answer rather than a cautious one, and it offered nothing to do about it. The two states are now distinct, the error one says so, and Refresh retries the lookup alongside the feed so the control on screen can actually clear the state it is showing.
1 parent e71326c commit 5d1fbb7

3 files changed

Lines changed: 103 additions & 30 deletions

File tree

apps/sim/ee/audit-logs/components/audit-logs.tsx

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,17 @@ export function AuditLogs({ organizationId }: AuditLogsProps) {
322322
const isWorkspaceScopePending = Boolean(workspaceScope) && orgWorkspaces.isPending
323323

324324
/**
325-
* The link named a workspace this organization cannot resolve — deleted since, or
325+
* The lookup itself failed, so whether the workspace exists is simply unknown.
326+
*
327+
* Kept apart from {@link isWorkspaceScopeUnresolved}: telling an admin their
328+
* workspace is not part of the organization because a request timed out is a wrong
329+
* answer, not a cautious one, and it offers nothing to do about it. Refresh retries
330+
* this lookup alongside the feed.
331+
*/
332+
const isWorkspaceScopeUnavailable = Boolean(workspaceScope) && orgWorkspaces.isError
333+
334+
/**
335+
* The link named a workspace this organization does not have — deleted since, or
326336
* never one of ours.
327337
*
328338
* The feed stays closed rather than falling back to the organization. Every other
@@ -332,10 +342,14 @@ export function AuditLogs({ organizationId }: AuditLogsProps) {
332342
* that still claims to be scoped, and the CSV export would follow.
333343
*/
334344
const isWorkspaceScopeUnresolved =
335-
Boolean(workspaceScope) && !isWorkspaceScopePending && !scopedWorkspace
345+
Boolean(workspaceScope) &&
346+
!isWorkspaceScopePending &&
347+
!isWorkspaceScopeUnavailable &&
348+
!scopedWorkspace
336349

337350
/** The feed can answer the scope the URL asks for — the gate on reading or exporting. */
338-
const isScopeAnswerable = !isWorkspaceScopePending && !isWorkspaceScopeUnresolved
351+
const isScopeAnswerable =
352+
!isWorkspaceScopePending && !isWorkspaceScopeUnresolved && !isWorkspaceScopeUnavailable
339353
const {
340354
data,
341355
isLoading,
@@ -392,7 +406,12 @@ export function AuditLogs({ organizationId }: AuditLogsProps) {
392406
refreshTimers.delete(timerId)
393407
}, REFRESH_SPINNER_DURATION_MS)
394408
refreshTimers.add(timerId)
395-
refetch().catch((error: unknown) => {
409+
/*
410+
Both, because a failed workspace lookup closes the feed — refreshing only the
411+
feed would leave the one control on screen unable to clear the state it is
412+
showing.
413+
*/
414+
Promise.all([refetch(), orgWorkspaces.refetch()]).catch((error: unknown) => {
396415
logger.error('Failed to refresh audit logs', { error })
397416
})
398417
}
@@ -495,7 +514,7 @@ export function AuditLogs({ organizationId }: AuditLogsProps) {
495514
{/* Rendered for an unresolved scope too, or a bad link would leave the
496515
feed closed with no control to reopen it. */}
497516
<OverflowText
498-
label={`Workspace: ${scopedWorkspace?.name ?? 'not found'}`}
517+
label={`Workspace: ${scopedWorkspace?.name ?? (isWorkspaceScopeUnavailable ? 'unavailable' : 'not found')}`}
499518
className='block min-w-0'
500519
/>
501520
</Chip>
@@ -559,7 +578,11 @@ export function AuditLogs({ organizationId }: AuditLogsProps) {
559578
<ActivityLog
560579
entries={allEntries.map(toActivityEntry)}
561580
emptyState={
562-
isLoading || isWorkspaceScopePending ? undefined : isWorkspaceScopeUnresolved ? (
581+
isLoading || isWorkspaceScopePending ? undefined : isWorkspaceScopeUnavailable ? (
582+
<SettingsEmptyState tone='error'>
583+
Couldn't check that workspace. Refresh to try again.
584+
</SettingsEmptyState>
585+
) : isWorkspaceScopeUnresolved ? (
563586
<SettingsEmptyState>
564587
That workspace is not part of this organization.
565588
</SettingsEmptyState>

apps/sim/ee/audit-logs/hooks/audit-logs.test.tsx

Lines changed: 47 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,15 @@ let queryClient: QueryClient
5353
function AuditProbe({
5454
organizationId,
5555
workspaceId,
56+
search,
5657
enabled = true,
5758
}: {
5859
organizationId: string
5960
workspaceId?: string
61+
search?: string
6062
enabled?: boolean
6163
}) {
62-
const auditLogs = useAuditLogs(organizationId, { workspaceId }, enabled)
64+
const auditLogs = useAuditLogs(organizationId, { workspaceId, search }, enabled)
6365
const entries = auditLogs.data?.pages.flatMap((page) => page.data) ?? []
6466

6567
return (
@@ -70,11 +72,22 @@ function AuditProbe({
7072
)
7173
}
7274

73-
function renderAuditLogs(organizationId: string, workspaceId?: string, enabled = true) {
75+
interface RenderOptions {
76+
workspaceId?: string
77+
search?: string
78+
enabled?: boolean
79+
}
80+
81+
function renderAuditLogs(organizationId: string, options: RenderOptions = {}) {
7482
act(() => {
7583
root.render(
7684
<QueryClientProvider client={queryClient}>
77-
<AuditProbe organizationId={organizationId} workspaceId={workspaceId} enabled={enabled} />
85+
<AuditProbe
86+
organizationId={organizationId}
87+
workspaceId={options.workspaceId}
88+
search={options.search}
89+
enabled={options.enabled ?? true}
90+
/>
7891
</QueryClientProvider>
7992
)
8093
})
@@ -140,25 +153,51 @@ describe('useAuditLogs identity transitions', () => {
140153
})
141154

142155
/** Blanking the feed on each keystroke is what the placeholder exists to stop. */
143-
it('holds the current entries while a filter change loads, within one organization', async () => {
156+
it('holds the current entries while a filter change loads, within one scope', async () => {
144157
const filteredPage = createDeferred<AuditLogPage>()
145158
mockRequestJson.mockImplementation(
146-
(contract: unknown, input: { query?: { workspaceId?: string } }) => {
159+
(contract: unknown, input: { query?: { search?: string } }) => {
147160
if (contract !== listAuditLogsContract) throw new Error('Unexpected contract')
148-
return input.query?.workspaceId ? filteredPage.promise : Promise.resolve(AUDIT_PAGE_A)
161+
return input.query?.search ? filteredPage.promise : Promise.resolve(AUDIT_PAGE_A)
149162
}
150163
)
151164

152165
renderAuditLogs('org-a')
153166
await flushQueries()
154167
expect(container).toHaveTextContent('Updated Organization A')
155168

156-
renderAuditLogs('org-a', 'workspace-a')
169+
renderAuditLogs('org-a', { search: 'canary' })
157170
await flushQueries()
158171

159172
expect(container).toHaveTextContent('Updated Organization A')
160173
})
161174

175+
/**
176+
* The other side of that rule. A workspace is a scope, not a filter: holding the
177+
* organization-wide rows while the scoped page loads would show, under a
178+
* workspace-scoped URL, entries that scope does not cover — with Export armed
179+
* against them, since it gates on this list being non-empty.
180+
*/
181+
it('clears the entries when the workspace scope changes, within one organization', async () => {
182+
const scopedPage = createDeferred<AuditLogPage>()
183+
mockRequestJson.mockImplementation(
184+
(contract: unknown, input: { query?: { workspaceId?: string } }) => {
185+
if (contract !== listAuditLogsContract) throw new Error('Unexpected contract')
186+
return input.query?.workspaceId ? scopedPage.promise : Promise.resolve(AUDIT_PAGE_A)
187+
}
188+
)
189+
190+
renderAuditLogs('org-a')
191+
await flushQueries()
192+
expect(container).toHaveTextContent('Updated Organization A')
193+
194+
renderAuditLogs('org-a', { workspaceId: 'workspace-a' })
195+
await flushQueries()
196+
197+
expect(container).not.toHaveTextContent('Updated Organization A')
198+
expect(container.querySelector('button')).toBeNull()
199+
})
200+
162201
/**
163202
* The scope a link asks for is a ceiling, not a hint. A workspace id that no longer
164203
* resolves must leave the feed closed rather than answering with the whole
@@ -167,7 +206,7 @@ describe('useAuditLogs identity transitions', () => {
167206
it('never queries unscoped while a workspace scope is unresolved', async () => {
168207
mockRequestJson.mockResolvedValue(AUDIT_PAGE_A)
169208

170-
renderAuditLogs('org-a', undefined, false)
209+
renderAuditLogs('org-a', { enabled: false })
171210
await flushQueries()
172211

173212
expect(mockRequestJson).not.toHaveBeenCalled()

apps/sim/ee/audit-logs/hooks/audit-logs.ts

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useInfiniteQuery } from '@tanstack/react-query'
1+
import { hashKey, useInfiniteQuery } from '@tanstack/react-query'
22
import { requestJson } from '@/lib/api/client/request'
33
import { type AuditLogPage, listAuditLogsContract } from '@/lib/api/contracts/audit-logs'
44

@@ -7,16 +7,23 @@ export const AUDIT_LOG_LIST_STALE_TIME = 30 * 1000
77
export const auditLogKeys = {
88
all: ['audit-logs'] as const,
99
lists: () => [...auditLogKeys.all, 'list'] as const,
10+
/**
11+
* What a key is allowed to see: the organization, and the workspace within it.
12+
*
13+
* It leads the key, ahead of the filters, because previous data may be held across
14+
* a filter change but never across a scope change — and a leading scope makes that
15+
* a prefix comparison rather than a reach inside the filter object.
16+
*/
17+
scope: (organizationId: string, workspaceId?: string) =>
18+
[...auditLogKeys.lists(), organizationId, workspaceId ?? ''] as const,
1019
list: (organizationId: string, filters: AuditLogFilters) =>
11-
[...auditLogKeys.lists(), organizationId, filters] as const,
20+
[...auditLogKeys.scope(organizationId, filters.workspaceId), filters] as const,
1221
}
1322

14-
/**
15-
* Position of the organization id in a key built by {@link auditLogKeys.list} —
16-
* derived from the factory rather than restated, so a new prefix segment cannot
17-
* silently point this at the wrong element.
18-
*/
19-
const AUDIT_LOG_KEY_ORGANIZATION_INDEX = auditLogKeys.lists().length
23+
/** The scope a key reads from, which is everything but its trailing filter object. */
24+
function auditListScopeIdentity(key: readonly unknown[]): string {
25+
return hashKey(key.slice(0, -1))
26+
}
2027

2128
export interface AuditLogFilters {
2229
search?: string
@@ -53,24 +60,28 @@ async function fetchAuditLogs(
5360
}
5461

5562
export function useAuditLogs(organizationId: string, filters: AuditLogFilters, enabled = true) {
63+
const queryKey = auditLogKeys.list(organizationId, filters)
5664
return useInfiniteQuery({
57-
queryKey: auditLogKeys.list(organizationId, filters),
65+
queryKey,
5866
queryFn: ({ pageParam, signal }) => fetchAuditLogs(organizationId, filters, pageParam, signal),
5967
initialPageParam: undefined as string | undefined,
6068
getNextPageParam: (lastPage) => lastPage.nextCursor,
6169
enabled: Boolean(organizationId) && enabled,
6270
staleTime: AUDIT_LOG_LIST_STALE_TIME,
6371
/**
64-
* Held across a filter change, never across an organization change.
72+
* Held across a filter change, never across a scope change.
6573
*
66-
* Every filter — search, types, window, workspace — is part of the key, so
67-
* without a placeholder the feed blanks to its empty state on each keystroke, and
68-
* the Export action's `isPlaceholderData` guard was dead. But the organization is
69-
* in the key too, and `keepPreviousData` alone would paint one tenant's audit
70-
* entries under another tenant's heading while the new page loaded.
74+
* Search, types and the window are all part of the key, so without a placeholder
75+
* the feed blanks to its empty state on each keystroke and the Export action's
76+
* `isPlaceholderData` guard is dead. But the organization and the workspace are in
77+
* the key too, and holding across either shows rows the current scope does not
78+
* cover — one tenant's entries under another's heading, or the organization's
79+
* under a workspace-scoped URL — with Export armed against them.
7180
*/
7281
placeholderData: (previous, previousQuery) =>
73-
previous && previousQuery?.queryKey[AUDIT_LOG_KEY_ORGANIZATION_INDEX] === organizationId
82+
previous &&
83+
previousQuery &&
84+
auditListScopeIdentity(previousQuery.queryKey) === auditListScopeIdentity(queryKey)
7485
? previous
7586
: undefined,
7687
})

0 commit comments

Comments
 (0)