fix: scope Copilot drafts by workflow - #6019
Closed
BillLeoutsakosvl346 wants to merge 5 commits into
Closed
Conversation
* feat(library): Best Gumloop Alternatives in 2026 * generate image --------- Co-authored-by: Sim Pi Agent <pi@sim.ai>
) X-RateLimit-Limit was the bucket's refill rate while X-RateLimit-Remaining was tokens left in the bucket, and createBucketConfig sets maxTokens = refillRate * burstMultiplier. The two headers described different quantities, so remaining routinely exceeded limit — observed live on a team plan as limit 200 alongside remaining 399 — and any client computing used = limit - remaining got a negative number. Report the bucket capacity, which is what remaining counts down from. This lives in the shared checkRateLimit, so it applies to every v1 endpoint: workflows, logs, tables, files, knowledge, audit-logs and copilot. Also stops publishing rate-limit headers on an authentication failure. That path never reaches the bucket, so the previous placeholder advertised a quota that does not exist and told unauthenticated callers they had been throttled. Separately, the shared id schemas reported Zod's default "expected string, received undefined" when a required field was omitted, because .min(1) only fires for a present-but-empty string. Adding the message to the z.string() constructor makes an omitted workspaceId/organizationId/workflowId/fileId name the field it is complaining about — the first thing an API consumer sees on a malformed request. Documents all three headers in the OpenAPI spec as reusable components, including the burst-capacity semantics and the fact that they are absent on authentication failures. They were previously undocumented. Adds the first tests for the v1 middleware.
…er the billing period aggregate (#6015) * perf(files): stop loading every workspace file to archive a workflow's backing files `cleanupWorkflowAliasBacking` runs on every workflow delete. It loaded every `context='workspace'` row for the workspace — active and soft-deleted, all columns — then discarded >99% of them in JS to find the handful of `.changelogs/<workflowId>.md` and `.plans/<workflowId>/**` rows it owns. In production this was the single worst query at 37.83% of total database runtime: p50 6s, p95 9s, max 13.1s, 641 calls/day, 34.1M rows read. It is index-served, so the cost is not a missing index — it is 59,061 rows and ~630MB of cold buffers read to archive a few files. A file's `folderPath` is derived solely from its `folderId`, so matching on folder membership is equivalent to the path comparison it replaces. The load and JS filter become one targeted UPDATE keyed on a handful of folder ids. Folders that are soft-deleted are still included when resolving which files a workflow owns: path resolution ignores `deletedAt`, so a live file parented to an archived folder previously matched and must continue to. Also: - Add `getWorkspaceShares`, replacing an id-list `IN` clause that grew with the file count (59,061 elements on the worst workspace) with one indexed lookup on `workspace_id`. Callers read the map by id, so a superset is equivalent. - Drop `all` from the client-reachable file scope enum. It drops the `deleted_at` predicate and so cannot use the partial index serving the other two. No client requests it; server callers reach that scope directly. - Set `fetch_types: false` on the app and realtime pools. postgres.js otherwise runs a blocking `pg_catalog.pg_type` roundtrip before each new connection's first query — 95,722 of them per day. It builds array parsers only; Drizzle already parses this schema's two `text[]` columns itself. * perf(nav): add route loading boundaries and cover the billing period aggregate Dynamic routes prefetch only down to the nearest loading boundary, and the server stops prefetching at the first one. With no loading.tsx anywhere in the workspace tree and a dynamic layout, Link prefetch was yielding almost nothing and every navigation waited on a full server round trip with no feedback. Add loading.tsx only where the fallback is provably what renders today, so perceived speed improves without changing what users see: - home, chat/[chatId]: reuse HomeFallback, already each page's own Suspense fallback. - integrations, skills: reuse the tab-header chrome, byte-identical to each page's own Suspense fallback. Deliberately not added to workspace root, settings, or w, whose pages are redirect-only or already self-fallbacking — a boundary there would paint a skeleton that does not match the destination. Billing: - Add usage_log_billing_period_cost_idx, trailing the remaining predicate columns and `cost` so the period aggregates resolve index-only. Confirmed against production: the aggregate currently runs as an Index Scan touching 478,559 buffers because `cost` is absent from the existing index. That index is superseded but left in place; dropping it is a separate migration so a planner regression costs nothing to revert. - Collapse the two aggregates behind /api/billing into one scan using SUM(...) FILTER. Besides halving the work on a nav-path query, it removes a latent inconsistency: as separate statements the two sums could observe different snapshots, making the copilot subset exceed the total. Caching was considered and rejected for the usage aggregate. Its callers include usage enforcement, threshold billing, and overage calculation, where a stale-low read permits overspend and a stale-high read double-charges. * test(files): cover cleanupWorkflowAliasBacking and correct a stale share mock cleanupWorkflowAliasBacking had no test coverage, and the rewrite that replaced its load-everything-then-filter body rests on a subtle equivalence: a file's folderPath is derived solely from its folderId, and path resolution ignores deletedAt. The second half is the easy part to get wrong — a live file parented to an archived folder still resolves to a backing path and must still be archived, so folders are filtered by deletedAt only when choosing which folders to archive, never when deciding which files the workflow owns. These tests pin that distinction: they fail if the archived-folder case is dropped from file ownership, and separately assert that archived folders stay out of the folder update and that unrelated workflows are never touched. Also point the workspace files route test at getWorkspaceShares. Its mock still named getSharesForResources, which the route no longer imports. The suite passed regardless because all three cases exercise the upload path, so the GET listing has no coverage — but the stale name would have handed the first GET test an undefined function. * fix(perf): drop the loading boundaries and correct the usage_log index order Adversarial review found real regressions in both. Route loading boundaries — all four removed: The premise in their TSDoc was wrong. Each page's existing `<Suspense>` exists so nuqs can prerender; `useSearchParams` only suspends during SSR, so on a client navigation those fallbacks never painted. Hoisting them to loading.tsx did not "show the same frame" — it made a previously invisible blank frame visible. For chat, Next keys the Suspense boundary by cache key, so a chatId change mounts a fresh suspended boundary and always commits its fallback. Switching chats would have gone from "previous chat stays on screen" to a blank surface for the whole RSC round trip, on the highest-frequency navigation in the product. Partial prefetch only warms the fallback, so no latency was saved to offset it. The integrations and skills fallbacks were correct for their own pages but also became the fallback for four detail routes that render different chrome, inserting a wrong intermediate frame. Fixing that needs per-child boundaries and new skeleton UI that cannot be verified without a browser, for pages whose only server work is `await params`. Not worth it. Every remaining loading.tsx in this app paints real chrome. A blank one was against the grain, and the measurable win was zero. usage_log index: Column order was wrong. The daily-refresh rollup filters entity type, id and period_start but NOT period_end, so putting period_end fourth ended the usable prefix at column three and left user_id and created_at as in-index filters rather than scan boundaries — turning a ~2.2k-entry bitmap scan into a ~266k-entry scan. Verified in production that period_start functionally determines period_end (13,612 groups, zero with more than one end), so the slot bought no selectivity. user_id and created_at now follow the shared prefix; period_end rides as payload. Also corrected the claim that this supersedes usage_log_billing_entity_period_idx. That index deduplicates to 17 MB across 1.42M entries because it has no high-cardinality key column, which is what keeps prefix-only bitmap scans cheap. It is retained deliberately, not pending a drop. Harden cleanupWorkflowAliasBacking: gate the UPDATE on the ownership filter list itself. `and()` and `or()` both drop undefined, so a clause that resolved to nothing would have left a WHERE of workspace + context + not-deleted and archived every file in the workspace. Tests now assert no UPDATE is issued when the workflow owns nothing, plus the filename and context predicates. Note fetch_types also disables array serializers, not just parsers; documented. * docs(db): correct the fetch_types constraint note after differential testing Ran both settings against a real Postgres with the schema's actual text[] columns. Drizzle-typed selects and .returning() are byte-identical either way; only a raw db.execute projecting an array column differs, yielding the wire form. The previous note also claimed a raw JS-array bind fails because the serializers come from the same catalog fetch. It does fail — but under both settings, because Drizzle expands an array into a row constructor before postgres.js ever sees it. That is unrelated to this flag, so the claim is removed rather than left implying a constraint this change introduces. * chore(db): generate the drizzle snapshot for the usage_log index migration 0271 was hand-written, so drizzle-kit's state never learned about the new index — the next `generate` would have re-emitted it as a fresh migration against an already-migrated database. Ran `drizzle-kit generate` to produce the snapshot, then restored the CONCURRENTLY form: drizzle emits a plain CREATE INDEX, which takes an ACCESS EXCLUSIVE lock and would block writes on a 4M-row table for the duration of the build. The generated column order matched the hand-written SQL exactly, which also confirms schema.ts and the migration agree. `generate` is now a no-op, and check:migrations still passes.
…he field (#6012) * fix(api): give every v1 endpoint quota headers and errors that name the field Three consistency gaps found by probing the live v1 surface end to end. Rate-limit headers were only published by routes built on createApiResponse — workflows, logs and audit-logs. Tables, files and knowledge are rate limited by the same bucket and will return 429, but published no quota on success, so a client discovered the ceiling only by hitting it. Adds a shared rateLimitHeaders() builder, reused by createRateLimitResponse, and attaches it to all 31 success responses on those three families. Missing required fields did not name themselves. .min(1, '...') only fires for a present-but-empty string, so an omitted field fell through to Zod's default "Invalid input: expected string, received undefined". A previous pass fixed the shared id schemas, but 22 of 23 v1 workspaceId declarations bypassed them, so the fix reached almost nothing. Adds requiredFieldSchema(message) and routes every v1 request input through it, preserving each site's existing, more specific wording (for example "workspaceId query parameter is required") instead of flattening them to the generic one. Response schemas are left alone — "required" wording would be wrong there. Validation failures on tables, files and knowledge reported the literal "Validation error" and discarded the schema's message. Adds v1ValidationErrorResponse, which surfaces the first issue while keeping details, and wires it into the 19 parseRequest calls that had no handler. Routes with deliberately specific wording keep theirs. The global default is untouched, since routes outside v1 assert the current string. * fix(api): finish the v1 consistency sweep at call level, not file level Review found three places the first pass missed, all from filters that worked on whole files instead of individual call sites. - GET /api/v1/files/{fileId} returns the file bytes via `new Response`, not a `success: true` JSON body, so the header pass skipped it. The download now carries the same quota headers as the DELETE beside it. - Four parseRequest calls in the table-row routes still reported the generic "Validation error". The first pass skipped any file that already had a handler anywhere in it, which excluded these two files wholesale. The check is now per call site, and no bare call remains. - POST /api/v1/tables takes its body from the shared tables contract, which still used the bare `.min(1)` form, so an omitted workspaceId did not name itself. Converted there and in the other v1-reachable contracts. Scope note: roughly a thousand `.min(1, '... is required')` declarations remain under contracts/tools/**. Those are block and tool definitions rather than the public REST surface, and converting them belongs in its own change. * refactor(api): publish quota headers from one chokepoint, not 32 call sites A quality pass found the previous commit only made the happy path consistent. Those three route families have 117 response sites; 32 got headers. The other 85 are the error paths — 400/403/404/500 — which are exactly the responses a client is deciding whether to retry, and they published no quota at all. `checkRateLimit` now records the bucket snapshot against the request, and `withRouteHandler` attaches the headers next to the `x-request-id` it already sets, on both the success and the unhandled-error branch. Every v1 response carries the quota now, and a new v1 route gets it without remembering to. The carrier is a WeakMap keyed by the request, so it needs no cleanup and routes that never record a snapshot — everything outside v1 — are untouched. This deletes more than it adds: the 32 decorations are gone, and so is the `rateLimit` parameter that had been threaded into `handleBatchInsert` purely so a business-logic helper could decorate its own response. Also from the same pass: - One definition of the header trio. `createApiResponse` had its own copy, so after the last commit there were two; both now build from `buildRateLimitHeaders`. - `v1ValidationErrorResponse` delegates to the shared `validationErrorResponse` instead of hand-rolling the same body, and takes a fallback message, which collapses four route closures that differed only in that string. - 36 sites wrote `requiredFieldSchema('Workspace ID is required')` — the verbatim definition of the exported `workspaceIdSchema`. Using the primitive is the whole point of having it; they now import it. - Dropped TSDoc that had gone stale or contradicted the call sites it advised. * docs(api): reattach the withRouteHandler docblock and drop stale wording The comment pass caught a real casualty of the previous commit: inserting `applyResponseHeaders` put it between `withRouteHandler`'s docblock and the function itself, so the file's most-used export lost its documentation to the new private helper. Reattached, and its header bullet now mentions the rate-limit trio it also emits. Remaining edits are wording only. The WeakMap rationale moved off `RateLimitSnapshot` — three self-evident fields — onto the `snapshots` declaration it actually describes. The record site no longer restates that rationale; it keeps only the part unique to it. And three id-schema docs claimed "same constraint as nonEmptyIdSchema", which stopped being true when that schema was documented as deliberately message-less. * docs(api): document the quota headers on every v1 success response The spec already asserted, in the RateLimited description, that the X-RateLimit-* trio accompanies every authenticated response. Before this branch that was false for tables, files and knowledge; it is true now, but no operation documented it — only 1 of 40 v1 success responses carried the headers. All 40 now reference the shared header components. The shared BadRequest, Forbidden and NotFound components are deliberately left alone: they are also $ref-ed by non-v1 operations that publish no quota, so annotating them there would over-claim. The RateLimited description carries the general rule instead, now stating explicitly that the only responses without the headers are the ones that failed authentication. * fix(api): stop the last v1 validation paths from swallowing the message Bugbot found GET /api/v1/tables/{tableId}/rows still answering with the bare "Validation error". Its handler special-cases malformed filter/sort JSON and then falls back to the shared helper — so the site looked handled to a check that only asked whether a handler existed, which is why the earlier call-level sweep passed over it. Auditing the whole class turned up more of the same shape: - The optional-body parses on deploy and rollback, where a bad `version` lost "version must be a positive integer". - Eleven catch-block `validationErrorResponseFromError` handlers across the table routes, which discard the message of any ZodError thrown deeper. Adds `v1ValidationErrorResponseFromError` as the v1 counterpart for unknown caught values, and routes every remaining v1 validation path through the v1 helpers. No call to the generic helpers survives under app/api/v1 outside admin, which keeps its own error envelope.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview
Reviewed by Cursor Bugbot for commit 197e765. Bugbot is set up for automated code reviews on this repo. Configure here. |
Contributor
Greptile SummaryScopes Copilot panel drafts per workspace/workflow and bundles several unrelated hardening changes.
Confidence Score: 5/5Safe to merge; no concrete changed-code defects were established on the draft scoping, cleanup, shares, rate-limit, billing, or migration paths. Draft keys are consumed by the mothership draft store; cleanup gates empty ownership and matches originalName; shares and Zod 4 required-field APIs are consistent with callers and tests; rate-limit and billing changes are covered by unit tests and existing SQL FILTER patterns.
|
| Filename | Overview |
|---|---|
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/panel.tsx | Types route params and passes a workflow-scoped draftScopeKey so panel drafts no longer share a global/unscoped key. |
| apps/sim/lib/copilot/vfs/workflow-alias-backing.ts | Folder-id ownership + empty-filter guard replace full-file listing; tests pin no workspace-wide archive. |
| apps/sim/app/api/v1/middleware.ts | Rate-limit limit uses bucket capacity; auth failures omit fake quota headers; shared v1 validation error helper. |
| apps/sim/lib/billing/core/usage-log.ts | Adds combined total/subset period cost aggregate used by personal billing summary. |
| packages/db/migrations/0271_usage_log_billing_period_cost_idx.sql | Idempotent CONCURRENTLY covering index aligned with schema and migrate conventions. |
Reviews (1): Last reviewed commit: "fix: scope Copilot drafts by workflow" | Re-trigger Greptile
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
3 participants
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
MothershipChat.Why
Prevent Copilot drafts from leaking between workflows or workspace-home chats.
How
Build
draftScopeKeyfrom the workspace and workflow IDs and pass it to the chat component.