improvement(ui): fix nav loading flash, skeleton mismatches, and React anti-patterns across resource pages#3864
improvement(ui): fix nav loading flash, skeleton mismatches, and React anti-patterns across resource pages#3864waleedlatif1 wants to merge 2 commits intostagingfrom
Conversation
…t anti-patterns across resource pages - Convert knowledge, files, tables, scheduled-tasks, and home page.tsx files from async server components to simple client re-exports, eliminating the loading.tsx flash on every navigation - Add client-side permission redirects (usePermissionConfig) to knowledge, files, and tables components to replace server-side checks - Fix knowledge loading.tsx skeleton column count (6→7) and tables loading.tsx (remove phantom checkbox column) - Fix connector document live updates: use isConnectorSyncingOrPending instead of status === 'syncing' so polling activates immediately after connector creation - Remove dead chunk-switch useEffect in ChunkEditor (redundant with key prop remount) - Replace useState+useEffect debounce with useDebounce hook in document.tsx - Replace useRef+useEffect URL init with lazy useState initializers in document.tsx and logs.tsx - Make handleToggleEnabled optimistic in document.tsx (cache first, onError rollback) - Replace mutate+new Promise wrapper with mutateAsync+try/catch in base.tsx - Fix schedule-modal.tsx: replace 15-setter useEffect with useState lazy initializers + key prop remount; wrap parseCronToScheduleType in useMemo - Fix logs search: eliminate mount-only useEffect with eslint-disable by passing initialQuery to useSearchState; parse query once via shared initialParsed state - Add useWorkspaceFileRecord hook to workspace-files.ts; refactor FileViewer to self-fetch - Fix value: any → value: string in useTagSelection and collaborativeSetTagSelection - Fix knowledge-tag-filters.tsx: pass '' instead of null when filters are cleared (type safety)
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Shifts tab-hiding behavior to the client via Cleans up several UI/state patterns: replaces mount/init Tightens types around tag-selection plumbing ( Written by Cursor Bugbot for commit 473cfcc. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
apps/sim/app/workspace/[workspaceId]/knowledge/[id]/[documentId]/document.tsx
Show resolved
Hide resolved
…th useWorkspaceFiles
|
@cursor review |
|
greptile |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| typeof window !== 'undefined' | ||
| ? new URLSearchParams(window.location.search).get('executionId') | ||
| : null | ||
| ) |
There was a problem hiding this comment.
useRef re-evaluates URL parsing on every render
Low Severity
useRef does not support lazy initialization like useState(() => ...). The expression new URLSearchParams(window.location.search).get('executionId') is evaluated on every render, allocating a throwaway URLSearchParams object each time, even though only the first render's value is used by the ref. The searchQuery useState on line 275 correctly uses a lazy initializer for the same pattern, but pendingExecutionIdRef does not get the same treatment.
Greptile SummaryThis PR introduces a broad set of improvements across resource pages (knowledge, files, tables, scheduled-tasks, home) in the Sim workspace. The central change converts async server-component
Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/documentation suggestions with no correctness or data-integrity impact No P0 or P1 defects found. The optimistic toggle rollback is correct, the mutateAsync refactor is clean, the useDebounce and lazy-useState replacements are idiomatic, and the connector polling fix correctly uses isConnectorSyncingOrPending. The three P2 comments cover a documentation gap, an accepted permission-flash trade-off, and a clarifying comment suggestion — none block merge. apps/sim/hooks/queries/workspace-files.ts and apps/sim/app/workspace/[workspaceId]/scheduled-tasks/components/create-schedule-modal/schedule-modal.tsx Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User navigates to /knowledge | /files | /tables"] --> B["page.tsx re-exports client component"]
B --> C["Client component mounts"]
C --> D["usePermissionConfig fetches async"]
D --> E{Config loaded?}
E -- "No (DEFAULT_CONFIG)" --> F["Render full page UI briefly"]
E -- "Yes" --> G{Tab hidden?}
G -- "Yes" --> H["router.replace to workspace home"]
G -- "No" --> I["Render page normally"]
F --> G
J["Old server-side flow"] --> K["getSession + verifyWorkspaceMembership"]
K --> L{Permitted?}
L -- "No" --> M["Instant server redirect - no flash"]
L -- "Yes" --> N["Render page"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function useWorkspaceFileRecord(workspaceId: string, fileId: string) { | ||
| return useQuery({ | ||
| queryKey: workspaceFilesKeys.list(workspaceId, 'active'), | ||
| queryFn: ({ signal }) => fetchWorkspaceFiles(workspaceId, 'active', signal), | ||
| enabled: !!workspaceId && !!fileId, | ||
| staleTime: 30 * 1000, | ||
| select: (files) => files.find((f) => f.id === fileId) ?? null, | ||
| }) | ||
| } |
There was a problem hiding this comment.
fileId missing from query key — cache hit works, but cold-path fetches entire list
The hook intentionally shares the list(workspaceId, 'active') query key so that an already-cached file list is served without a second request. That works great for the warm path.
On a cold path (e.g. a user bookmarks /files/[fileId]/view and navigates directly), useWorkspaceFileRecord triggers a full fetchWorkspaceFiles call — fetching every file in the workspace just to find one. Because fileId is not in the query key, there is also no way for React Query to deduplicate or cache a per-file lookup separately from the list.
If this is the intended trade-off (list-or-nothing), a short comment on the hook documenting the cold-path behaviour would prevent future confusion.
| const initialCronState = useMemo( | ||
| () => (schedule ? parseCronToScheduleType(schedule.cronExpression) : null), | ||
| [schedule?.cronExpression] | ||
| ) |
There was a problem hiding this comment.
key-based remount at call-site
Every useState below reads schedule.* fields only at mount time — the values are never re-synchronized when schedule changes. This works correctly because scheduled-tasks.tsx provides a key prop on the edit modal that forces remount on each different task. If this component is reused elsewhere without a matching key, the displayed form values will silently stay stale. A short JSDoc or inline comment documenting this requirement would make the contract explicit.
1 participant
Summary
Type of Change
Testing
Tested manually
Checklist