perf(server): stop calling our own API over HTTP during render, execution, and tool runs - #6660
Open
waleedlatif1 wants to merge 12 commits into
Open
perf(server): stop calling our own API over HTTP during render, execution, and tool runs#6660waleedlatif1 wants to merge 12 commits into
waleedlatif1 wants to merge 12 commits into
Conversation
The audit found this key was the workspaceFilesKeys bug waiting to recur. The manager's record type and workspaceFileFolderSchema are two independent declarations that agree today by coincidence; the seed had no parse, so adding a column to one would have silently cached a shape a client fetch strips — and three of its fields are z.coerce.date(), the exact divergence that put ISO strings under the file-list key. Its sibling is immune because listWorkspaceFilesWithShares parses at the data layer. This does the same at the seed, and adds the shape-parity assertion the key never had. Verified falsifiable: removing the parse turns it red. Doing so also exposed the existing folder test as fixture-thin — a folder with only an id, which the contract rightly rejects — so it now uses a real row. Also points the credential block's fetchQuery at the exported staleTime constant instead of restating 60 * 1000; it was a fifth producer on that key free to drift from the four that share it.
…ling shut Two functional bugs found auditing the query layer. patchCachedRows walked tableKeys.rowsRoot non-exact, but rowsRoot is a prefix: the find (search results) and write (pending writes) subtrees hang off it with non-paged shapes, and the updater's old.pages.map threw on them. It runs inside onMutate, so the whole cell edit rejected before reaching the server — reachable as soon as a find entry exists, i.e. after the user searches the table once. The sibling isDefaultOrderRowsQuery already excluded those subtrees and its docstring claimed they "never match"; that was only true of the sibling. Both now share one isRowListQueryKey helper so they cannot drift apart again. useCloudStorageConfigured combined staleTime: Infinity, retry: false, and the global retryOnMount: false on a workspace-independent key, so one transient failure left it errored for the tab's lifetime with no way back — navigating or switching workspace cannot change the key, and the upload path fails closed, so cloud-backed uploads stayed disabled until a full reload. useVoiceSettings carries the same three options and already escapes this with retryOnMount: true; this one now matches. Note: hooks/queries/workspace-files.test.tsx cannot load in a git worktree (pre-existing postcss resolution failure), so CI is the first place that file runs against this change.
The workspace row was read ~3x per workspace route and ~5x on settings, and the same Max-tier entitlement was resolved twice on one render. Memoization is deliberately partial. getWorkspaceWithOwner accepts a transaction and forUpdate, and live callers use both, so only the plain no-options read routes through the memo; a row read inside one caller's transaction or under a lock it alone holds can never be served to a later caller. includeArchived is part of the key so the two variants cannot alias. Three substitutions were considered and rejected as behavior changes, not optimizations: hostContext.ownerBilling resolves subscriptions differently from hasWorkspaceTierAccess and exposes no Max tier, so it cannot answer the Inbox/Sandbox gates; isOrganizationOnEnterprisePlan carries self-host short-circuits ownerBilling has no equivalent for; and widening WorkspaceHostContext to carry the full row would push owner and org ids onto the wire for every viewer to save a server-side read, since that type is a response contract rather than an internal struct.
…sing the credential gate The CloudWatch log-group and log-stream selectors forwarded `search` into the request as `prefix` but left it out of the query key, so every keystroke resolved to the same fresh entry and no refetch fired. Server-side filtering was dead: a log group outside the first page could not be reached. An audit of all 69 selector definitions found these two and no others. useSelectorOptions resolved `args.enabled ?? definition.enabled(...)`, so a caller supplying its own gate replaced the definition's precondition rather than narrowing it. useSelectorDisplayName knows nothing about credentials, so a card holding a saved value with no credential context ran a query that could only reject. The two are now conjoined. The detail hooks keep the override deliberately — resolving one known id needs less context than listing, which their TSDoc already documents. The list-key fix has a test, proven to fail without it. The `enabled` change has none: loading use-selector-query pulls the selector registry and emcn CSS, which cannot resolve in a git worktree.
generateTempId used Date.now(), so two rows created in the same millisecond shared an id and the first server response overwrote both — leaving one row duplicated and the other's real id lost until a refetch. Now uses generateId(), matching what the workflow mutations already do. Reachable by double-clicking create, or by any scripted or bulk create. Also documents the contract of fetchOAuthConnections, which reports an unknown connection state as disconnected. No consumer reads that field today — both read names and icons, and connection state comes from useWorkspaceCredentials — so letting the query reject would blank the suggested-action rows and drop the credential page to raw provider ids. The note is what stops a future consumer branching on it silently.
Each was verified against the mutation that changes the data and the keys that expose it, not taken on report. - Workspace usage/credits were invalidated nowhere in the app. Six sites already refreshed subscriptionKeys after credits moved — post-run, post-wand, limit edits, upgrades, top-ups — and none touched workspace usage, so the credits chip and the run gate held their page-load values until a reload. Adds one shared invalidateWorkspaceUsage and calls it from all six. - Knowledge-base list doc counts went stale: document upload, delete, and bulk delete invalidated only the detail key, though the list carries docCount. - Plan switches that do not redirect refreshed only the host context, leaving subscription and credit state showing the previous plan. - The copilot tool-event handler invalidated a raw workflowKeys.list, which covers only the active scope and skips the selector prefix; it now uses the shared invalidateWorkflowLists like the other thirteen call sites. - scheduleKeys.byId was a strict prefix of scheduleKeys.schedule, so the two addressings aliased, and nothing invalidated byId. De-aliased and invalidated. Not changed: the CSV preview key already folds in the file version and storage key, so a content update addresses a different cache entry — version-in-key is the mechanism there, not a missing invalidation. Tests added for the usage and knowledge fixes, both proven to fail without them. The other four live in files that cannot load in a git worktree (pre-existing postcss resolution failure), so CI is where they first run.
…are the usage refresh Two corrections from reviewing the previous commits. patchCachedRows was fixed with a predicate naming the sibling subtrees to skip — a denylist that rots the moment a fifth subtree is added under rowsRoot. The key factory already separated row lists under an 'infinite' segment; it just had no prefix accessor, so every caller reached for the parent and subtracted. Adding infiniteRowsRoot lets the walk be an allowlist by construction and deletes the predicate, the helper, and both docblocks explaining the subtraction. The searched-rows view is consequently no longer patched by a cell edit and is left to its own refetch — it holds a flat result, not pages. That is recorded on the function rather than left to be rediscovered. The delayed usage refresh was written out three times across two files, a duplication the previous commit enlarged rather than introduced. It is now one scheduleUsageRefresh beside the keys it invalidates, which also gives the bare 1000ms a name and one place to change it.
…memoized workspace read actually dedupe
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Router and evaluator blocks no longer Workspace file list hydration is capped at React Query / UI consistency: table row optimistic updates walk Reviewed by Cursor Bugbot for commit 140b928. Configure here. |
Contributor
Greptile SummaryThe PR replaces several server-to-server HTTP calls with shared in-process execution paths while preserving authorization and persistence boundaries. It also corrects query caching, workspace-file prefetching, and optimistic table-update behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/executor/handlers/evaluator/evaluator-handler.test.ts | Replaces the previously reported loosely typed provider-request helper with a concrete ProviderRequest-based return type. |
| apps/sim/executor/utils/provider-request.ts | Centralizes in-process provider execution and its admission checks for executor handlers. |
| apps/sim/lib/oauth/token-resolution.ts | Extracts the shared credential authorization, token refresh, provider metadata, audit, and error-mapping flow. |
| apps/sim/lib/workflows/persistence/save-normalized-state.ts | Consolidates authorized, lock-aware normalized workflow persistence and post-write notification. |
| apps/sim/hooks/queries/tables.ts | Narrows optimistic cache traversal to compatible table-row query shapes. |
| apps/sim/lib/workspace-files/queries.ts | Projects only mapped file columns and supports budget-aware workspace-file loading. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Executor[Router / Evaluator handlers] --> ProviderRuntime[Provider runtime]
ToolExecutor[Credential-using tools] --> TokenResolution[Shared token resolution]
OAuthRoute[OAuth token route] --> TokenResolution
RevertRoute[Checkpoint revert route] --> StatePersistence[Normalized state persistence]
WorkflowStateRoute[Workflow state route] --> StatePersistence
StatePersistence --> Database[(Postgres)]
StatePersistence --> Realtime[Socket notification]
Reviews (2): Last reviewed commit: "chore(test): type the evaluator provider..." | Re-trigger Greptile
Collaborator
Author
Collaborator
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 140b928. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Server-side work that was being done over HTTP against our own API now runs in-process, plus a set of query-layer correctness fixes found while auditing the same code.
Self-HTTP hops removed — each one cost a round trip through the load balancer plus a full re-authentication to re-derive identity the caller already held:
POST /api/providersper block. They now call the provider runtime directly through a sharedexecuteBlockProviderRequest, which reproduces the two admission checks that route owned.POST /api/auth/oauth/tokenon every call, including retries. The route body moved tolib/oauth/token-resolution.ts; the route and the executor now run one identical authorization path. The browser path still goes over HTTP with the session cookie./api/workflows/[id]/state, forwarding cookies so the other side could re-verify the session it had just verified. The PUT body moved tolib/workflows/persistence/save-normalized-state.ts, which owns authorization, the lock check, the row-locked write, custom-tool extraction, and the socket notification — so no surface can skip a step by going through a different door.Payload and query work
select().getWorkspaceWithOwner's request memoization was keyed onincludeArchived, so the gates that disagree about archived visibility each got their own entry and it deduped nothing. It now reads the superset once and filters per caller.Correctness fixes
rows— a cell edit with a search view open threw inonMutateand rejected the mutation. Both walks now use an allowlist prefix.useCloudStorageConfiguredcombined an infinitestaleTimewithretry: falseunder the globalretryOnMount: false, so one transient error disabled cloud-backed uploads for the tab's lifetime with no way to recover.searchinto the fetch without it appearing in the query key, so different searches shared one cache entry.generateId().hooks/queries/utils/, matching the existing convention.Type of Change
Testing
1,511 tests pass across the touched areas, including new coverage for the extracted modules, the seed budget, and the row-cache prefix. Verified the new tests can fail: disabling the workspace-access guard turns the router suite red, and restoring the removed HTTP hop in the revert route turns four tests red.
Four test files fail to load in this worktree on a pre-existing postcss/CSS-module resolution issue unrelated to these changes (one of them is a file this branch never touches).
type-check,biome,check:api-validation,check:react-query,check:client-boundary, andcheck:tool-registry-boundaryall pass.Checklist