Skip to content

Commit 902e094

Browse files
tianzhouclaude
andcommitted
fix: address review feedback on audit log page
- AuditLog: defer the admin gate until connections load so admins don't flash the denied state; keep the connection dropdown outside the gate so non-admins can switch to a connection they can access - App: gate useEditorNavigation to the editor route so /audit-log no longer writes editor URL params (schema/object) or fires schema/table queries; reset the invalid-connection toast guard on valid connections - useEditorNavigation/useQuery: thread an enabled flag through to disable schema/table queries and the URL-sync effect when inactive - ObjectSidebar: add aria-label to the audit-log icon button and make the version tooltip trigger keyboard-focusable Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c10a2b6 commit 902e094

5 files changed

Lines changed: 50 additions & 29 deletions

File tree

src/App.tsx

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,10 @@ function AppLayout() {
3939
return connections[0].id;
4040
})();
4141

42-
// Centralized URL state management - handles defaults and redirects
43-
const navigation = useEditorNavigation(selectedConnectionId);
42+
// Centralized URL state management - handles defaults and redirects.
43+
// Only active on the editor route so it doesn't write editor params
44+
// (schema/object) or fetch schemas while on other connection-scoped routes.
45+
const navigation = useEditorNavigation(selectedConnectionId, isEditorRoute);
4446

4547
const editorTabs = useEditorTabs(selectedConnectionId);
4648

@@ -50,9 +52,16 @@ function AppLayout() {
5052

5153
const isValidConnection = connectionIdFromUrl && connections.some(c => c.id === connectionIdFromUrl);
5254

55+
// Reset the once-per-invalid guard whenever we land on a valid connection,
56+
// so a later invalid connectionId (on any scoped route) surfaces its toast.
57+
if (isValidConnection) {
58+
hasShownInvalidToast.current = false;
59+
return;
60+
}
61+
5362
if (!connectionIdFromUrl) {
5463
navigate(`${location.pathname}?connectionId=${connections[0].id}`, { replace: true });
55-
} else if (!isValidConnection) {
64+
} else {
5665
if (!hasShownInvalidToast.current) {
5766
hasShownInvalidToast.current = true;
5867
toastManager.add({

src/components/sql-editor/ObjectSidebar.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,7 @@ export function ObjectSidebar({
173173
<Button
174174
variant="ghost"
175175
size="icon-sm"
176+
aria-label="Audit Log"
176177
onClick={() => navigate(`/audit-log?connectionId=${connectionId}`)}
177178
>
178179
<ScrollText className="h-4 w-4" />
@@ -185,7 +186,14 @@ export function ObjectSidebar({
185186
</Tooltip>
186187
<Tooltip>
187188
<TooltipTrigger
188-
render={<span className="text-xs text-gray-400">v{__APP_VERSION__}</span>}
189+
render={
190+
<span
191+
tabIndex={0}
192+
className="text-xs text-gray-400 outline-none focus-visible:ring-2 focus-visible:ring-ring rounded"
193+
>
194+
v{__APP_VERSION__}
195+
</span>
196+
}
189197
/>
190198
<TooltipContent side="top">
191199
Git commit {__GIT_COMMIT__}

src/hooks/useEditorNavigation.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ interface UseEditorNavigationResult {
3232
setObject: (object: SelectedObject | null, options?: { replace?: boolean }) => void
3333
}
3434

35-
export function useEditorNavigation(connectionId: string): UseEditorNavigationResult {
35+
export function useEditorNavigation(connectionId: string, enabled = true): UseEditorNavigationResult {
3636
const [searchParams, setSearchParams] = useSearchParams()
3737
const prevConnectionIdRef = useRef<string | null>(null)
3838

@@ -46,7 +46,7 @@ export function useEditorNavigation(connectionId: string): UseEditorNavigationRe
4646
isLoading: isSchemasLoading,
4747
isFetching: isSchemasFetching,
4848
error: schemasError,
49-
} = useSchemas(connectionId)
49+
} = useSchemas(connectionId, enabled)
5050

5151
// Determine the effective schema (from URL or default)
5252
const effectiveSchema = schemaFromUrl && schemas.includes(schemaFromUrl)
@@ -61,7 +61,7 @@ export function useEditorNavigation(connectionId: string): UseEditorNavigationRe
6161
isLoading: isTablesLoading,
6262
isFetching: isTablesFetching,
6363
error: tablesError,
64-
} = useTables(connectionId, effectiveSchema || '')
64+
} = useTables(connectionId, effectiveSchema || '', enabled)
6565

6666
// Determine the effective object (from URL or default)
6767
const effectiveObject: SelectedObject | null = (() => {
@@ -108,6 +108,9 @@ export function useEditorNavigation(connectionId: string): UseEditorNavigationRe
108108

109109
// Update URL when resolved state differs from URL state
110110
useEffect(() => {
111+
// Skip entirely when this hook isn't driving the current route (e.g. /audit-log),
112+
// so editor-specific params (schema/object) aren't written to unrelated URLs.
113+
if (!enabled) return
111114
// Don't update URL while still loading
112115
if (!connectionId || isSchemasLoading) return
113116

@@ -158,6 +161,7 @@ export function useEditorNavigation(connectionId: string): UseEditorNavigationRe
158161
)
159162
}
160163
}, [
164+
enabled,
161165
connectionId,
162166
isSchemasLoading,
163167
isTablesLoading,

src/hooks/useQuery.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,26 +38,26 @@ export const connectionKeys = {
3838
};
3939

4040
// Get schemas for a connection
41-
export function useSchemas(connectionId: string) {
41+
export function useSchemas(connectionId: string, enabled = true) {
4242
return useQuery({
4343
queryKey: queryKeys.schemas(connectionId),
4444
queryFn: async () => {
4545
const response = await queryClient.getSchemas({ connectionId });
4646
return response.schemas;
4747
},
48-
enabled: !!connectionId,
48+
enabled: enabled && !!connectionId,
4949
});
5050
}
5151

5252
// Get tables for a schema
53-
export function useTables(connectionId: string, schema: string) {
53+
export function useTables(connectionId: string, schema: string, enabled = true) {
5454
return useQuery({
5555
queryKey: queryKeys.tables(connectionId, schema),
5656
queryFn: async () => {
5757
const response = await queryClient.getTables({ connectionId, schema });
5858
return response.tables;
5959
},
60-
enabled: !!connectionId && !!schema,
60+
enabled: enabled && !!connectionId && !!schema,
6161
});
6262
}
6363

src/pages/AuditLog.tsx

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,28 +11,31 @@ interface AuditLogProps {
1111

1212
export default function AuditLog({ connectionId }: AuditLogProps) {
1313
const navigate = useNavigate()
14-
const { data: connections } = useConnections()
14+
const { data: connections, isLoading } = useConnections()
1515
const { hasAdmin } = useConnectionPermissions(connectionId)
1616

17-
// Admin-only page. Gate on the selected connection's admin permission.
18-
if (!hasAdmin) {
19-
return (
20-
<div className="flex-1 bg-white text-gray-900 overflow-auto">
21-
<div className="p-8">
22-
<h1 className="text-3xl font-bold mb-2">Audit Log</h1>
23-
<p className="text-gray-600">
24-
You need admin permission on this connection to view its audit log.
25-
</p>
26-
</div>
27-
</div>
28-
)
29-
}
17+
// Permissions are derived from the connections query, so defer the admin gate
18+
// until it resolves — otherwise admins briefly see the denied state on load.
19+
const content = isLoading || !connections ? (
20+
<div className="text-gray-500 text-sm">Loading…</div>
21+
) : !hasAdmin ? (
22+
<p className="text-gray-600">
23+
You need admin permission on this connection to view its audit log.
24+
</p>
25+
) : (
26+
<div className="flex flex-col items-center justify-center gap-3 rounded-lg border border-dashed border-gray-300 py-16 text-gray-500">
27+
<ScrollText size={32} />
28+
<p className="text-sm">No audit log entries yet.</p>
29+
</div>
30+
)
3031

3132
return (
3233
<div className="flex-1 bg-white text-gray-900 overflow-auto">
3334
<div className="p-8">
3435
<h1 className="text-3xl font-bold mb-8">Audit Log</h1>
3536

37+
{/* Connection selector stays outside the gate so a user without admin on the
38+
current connection can still switch to one where they do have access. */}
3639
<div className="space-y-2 max-w-xs mb-8">
3740
<Label htmlFor="audit-connection">Connection</Label>
3841
<Select
@@ -52,10 +55,7 @@ export default function AuditLog({ connectionId }: AuditLogProps) {
5255
</Select>
5356
</div>
5457

55-
<div className="flex flex-col items-center justify-center gap-3 rounded-lg border border-dashed border-gray-300 py-16 text-gray-500">
56-
<ScrollText size={32} />
57-
<p className="text-sm">No audit log entries yet.</p>
58-
</div>
58+
{content}
5959
</div>
6060
</div>
6161
)

0 commit comments

Comments
 (0)