v0.8.11: perf improvements, instant chat navigation, code hygiene - #7032
Open
waleedlatif1 wants to merge 21 commits into
Open
v0.8.11: perf improvements, instant chat navigation, code hygiene#7032waleedlatif1 wants to merge 21 commits into
waleedlatif1 wants to merge 21 commits into
Conversation
`normalizeLinkHref` rejected only `file` for a `scheme://` target, so any other scheme was returned unchanged. `scheme://` is well-formed for every scheme, so the check let through spellings that are not navigable targets at all. - Keep a scheme only when it is http(s), ftp(s), mailto, or tel; drop the rest - Leave an existing link alone when a committed target normalizes away, rather than unsetting it — the editor seeds that field with the current href, so committing an untouched one previously removed the link Detection is unchanged for relative, anchor, protocol-relative, and bare-domain targets. A document's stored markdown is untouched: normalization runs on the render and edit paths, never on parse or serialize, so a target that is refused still round-trips verbatim.
… assets (#7009) * fix(files): download a markdown file as a zip only when it really has assets A document that merely mentions an embed URL in prose or an inline code span counted as having attachments, so any document about the files API downloaded as a zip whose assets/ folder was empty. - Detect embeds with the markdown lexer instead of scanning raw text, so only real image embeds count: prose, code spans, fenced samples, and links no longer do - Choose the export format after resolving assets rather than from the candidate count, so a missing, unreadable, or oversized embed falls back to the plain document instead of an empty zip - Move the document scan out of the copilot tool tree into lib/uploads/server, where both file routes already live, and drop two pass-through wrappers - Share one <img> src reader between the clipboard handlers and the scan - Walk tokens explicitly: marked's walkTokens concatenates per token and costs O(n^2), measuring 5.4s on a 254KB document against 14ms here, on a path anonymous public-share traffic reaches * fix(files): keep an embed id spelled as the document spells it Decoding the id let a percent-encoded embed resolve and bundle its asset while the rewrite, which searches the document for that id, found nothing — the zip kept an API URL that renders as a broken image offline. Keys stay decoded; they are matched against stored keys, not against document text. * fix(files): resolve an export asset by its stored id, rewrite by its spelling An embed carries two representations and they are not interchangeable: metadata resolves by the stored id, while the rewrite finds the embed by searching the document for the spelling it used. Using one for both either drops a percent-encoded asset or bundles it behind a link still pointing at the API. * fix(files): resolve an embed by its stored id wherever one is read from a document The export bundler decoded an embed's spelling before looking it up, but the file-agent's embeddability warning did not, so a percent-encoded embed the export resolves and bundles could still be reported as one that will not survive an export. Both now share one helper. Request-supplied ids are untouched: their route contracts already constrain them to the plain id charset, so there is no spelling to decode.
* improvement(chat): speed up conversation navigation * fix(chat): prefetch direct navigation intent * fix(chat): preserve quick-click prefetch intent
…eads (#7017) Fail-open on unrecorded durable provenance rests on one compensating control: the audit entry telling the people who own the secrets that a read proceeded unvouched. An audit of all four surfaces found the control incomplete in exactly the places this closes, and confirmed the policy itself sound — so nothing here changes what any read or write does, only what gets recorded about it. Knowledge was the one surface with no audit trail at all: the per-record import reports without a workspace, and the report skips the audit row when it cannot name one, so fail-open knowledge reads emitted one error log line per record and zero audit entries. Both importers now count unrecorded records while the surface is open and report once per read with the workspace, actor, and count — the shape memory and tables already use. The search read reports once across chunks and rendered metadata, and only when the registry did not latch, since a latched read never reaches a model. Fault returns stay silent; those reads fail closed. Memory had the one silent local degrade: a record whose canonical hash outgrows its bounds, or whose entries fail normalization, was stored unknown with nothing logged anywhere — the table writer logs its equivalent. The binding now logs the cause at error where it is decided. An incoming unknown stays silent; its producer already reported. The memory list contract gains the page ceiling every other list already has (max 1000, matching the table convention); no caller in the repo passes a limit at all, and the route is internal-auth only. Workspace-file audit rows now carry the acting user where the caller already holds one — copilot vfs, the agent and mothership handlers, and the provider attachment filter. Everywhere else, including principals with no user to name, the actor stays null, which the report type has always permitted. Two comments catch up with the code: the file sidecar stores three statuses since the absence/taint split, and the mounted-file scanner's scan-overflow-to-taint is deliberate where the registry scan over-approximates — that scan only narrows an already-sound candidate set, while this one decides whether egress redaction would suffice for bytes the same matcher just failed on.
* perf(db): optimize recurring query paths * perf(logs): batch keyset export reads * fix(workspaces): type nullable member count targets * fix(db): harden query performance changes * fix(logs): guard export stream cancellation
…onses (#7015) An orchestration result carrying `errorCode: 'internal'` holds whatever text the fault happened to have — `workflow-lifecycle.ts` catch-alls return `toError(error).message`, which is the driver's failed SQL. Three application helpers projected that straight into an `OrchestrationError`, and the internal route policy rendered its message into a 500 body, so raw SQL reached clients. The v2 envelope already scrubbed the same failures; internal routes did not. `messageForOrchestrationError` already encoded the rule and two sites honored it. The three that hand-rolled it disagreed, and `workflow-vfs` disagreed with itself: it defaulted the code with `?? 'internal'` but compared the raw `errorCode` against `'internal'`, so an uncoded failure was classified internal and still rendered its own message. Pair the two in `throwOrchestrationFailure` so a code and its message cannot disagree, and scrub at the internal route boundary as well, matching v2 — no call site authors a curated `internal` message, so nothing legitimate is masked, and site N+1 cannot reopen this by forgetting the rule.
… forms (#7021) * refactor: replace hand-rolled utilities and dead code with the shared forms Each of these has a mandated helper or an established accessor in the repo that the site predates or missed. All are behavior-preserving: - `omit()` for the three `Object.fromEntries(Object.entries(x).filter(...))` block-input filters, which also recovers the `Omit<T, K>` typing that `Object.fromEntries` erases to an index signature. - `getErrorMessage()` for the inline `instanceof Error` message ternary. - `getBlock()` for two `getAllBlocks().find((b) => b.type === x)` scans, one of them inside a loop over selected tools. The same file already resolves the same values through `getBlock`. - A memoised `Map` for three `.find()`-by-id scans over the workspace skill list, one of them inside a render `.map()`. - `SELECTOR_SEARCH_STALE` for three copy-pasted `15 * 1000` literals. They are deliberately shorter than `SELECTOR_STALE`, so this is a new named constant rather than a fold into the existing one. - Tailwind classes for the static half of two duplicated anchor styles, keeping only the genuinely dynamic `left`/`top` inline. - Dropped the unused `catch` bindings on three intentional JSON-parse swallows. `panel.tsx`'s run-button gate loses a `TODO`-stubbed `hasValidationErrors = false` and the `isWorkflowBlocked` term built on it. That term was dead twice over: it reduced to `isExecuting`, and the enclosing expression is already guarded by `!isExecuting`. * fix: guard the registry lookups, and scope the search-stale doc to its callers `getBlock` normalizes its argument with `type.replace(...)`, so it throws on `undefined` where the `getAllBlocks().find(...)` it replaced returned `undefined` harmlessly. Both call sites can be reached without a type: `tool-input` reads `state.blocks[blockId]?.type`, which is undefined once the block is deleted while the panel is mounted — and `Record` indexing hides that from the compiler, so it would have thrown during render. `agent-handler`'s `tool.type` is optional and the compiler did catch it. Also index the skill lookup in `resolveSkillsLabel`, which runs a `.find()` inside a `.map()` for every block on the canvas — the case the memoised map in `skill-input` addressed for one component while leaving the hot path. `providers/utils.ts` keeps its `getAllBlocks().find(...)`: it takes the registry as an injected dependency precisely so a client-reachable module never imports it, and reaching for `getBlock` there would cross that boundary. The new constant's doc claimed search-backed selectors take a shorter window. Several still sit on `SELECTOR_STALE`, so it now describes the value its three callers share rather than asserting a rule the tree does not follow. * fix: guard the second registry lookup in tool-input `selectedTools` validates only `value[0]?.type` and then casts the whole array, so a persisted workflow whose later rows lost their `type` yields `undefined` here — the cast is what makes the compiler believe otherwise. `getBlock` normalizes with `type.replace`, so that throws during render.
…d-rolled it (#7018) The same three-step derivation — lowercase, collapse each non-alphanumeric run to a hyphen, strip the leading and trailing one — sat in eight files. Two of them carried a TSDoc line whose only job was to warn that they mirrored a third (`instance-org.ts`: "Derives a slug the same way the admin organization API does"; `consolidate-users-into-organization.ts`: "Mirrors the slug derivation used by POST /api/v1/admin/organizations"). A comment asserting two implementations agree is the shape duplication takes when it cannot be checked. All eight were semantically identical. Two anchored the strip with `-+` rather than `-`, and one followed it with a `--+` collapse, but `[^a-z0-9]+` has already collapsed every run by that point, so neither could ever match more than the single-hyphen form. Nothing changes. Truncation stays at the call sites. Four of them bound the result — at 24, 64 and 80 — and only `copy-chats.ts` strips again afterwards, because slicing can land mid-run and leave a trailing hyphen the earlier strip never saw. Folding a `maxLength` into the helper would have had to pick one of those behaviors and silently impose it on the others. `artifact-stylesheet.ts` keeps its copy: it lives inside the `SIM_ARTIFACT_SHELL` template literal and runs in the viewer's browser, where there is no import to resolve.
…never read (#7022) * fix(ci): stop the migration safety audit from passing on a branch it never read The zero-downtime audit reports the same empty file list for 'this branch adds no migrations' and 'I could not diff against the base', and the second prints as `✓ No new migrations to check` with exit 0. Reproduced on this checkout: $ bun run scripts/check-migrations-safety.ts origin/does-not-exist-branch ✓ No new migrations to check. exit=0 `changedMigrationFiles` returned `[]` whenever `git diff` failed, with a comment deferring the decision to the caller — but the caller only recognised a missing git binary (`git rev-parse HEAD === null`), never an unusable ref. CI supplied exactly that input. `git fetch --depth=1 … 2>/dev/null || true` hid a failed fetch, leaving `origin/<base>` absent, so a PR adding a destructive `DROP COLUMN` would clear the only guard on production DDL with a green check. Two halves: - The audit now distinguishes the cases. Absent git is still the one legitimate skip and is checked before the diff; a diff that fails with git present raises `BaseRefUnusableError` and exits 1. - The fetch is its own step with no `|| true`, so a failure fails the job. Depth stays 1: without a merge-base the audit diffs the two tips, which under `--diff-filter=AM` is exactly the migrations new on the branch. Covered by a test that runs the script end to end, since the defect was in the exit code rather than in any function's return value. Verified it fails when the throw is reverted to `return []`. * fix(ci): fetch the base ref once, and stop swallowing the failure The same `git fetch --depth=1 … 2>/dev/null || true` appeared in both base-ref audits. Fixing only the migration one would have left the identical defect a few steps above it. Neither audit can tell an absent base ref apart from a branch that changed nothing. The block-registry check at least degrades to a visible `⚠ Could not diff against base ref — skipping`; the migration audit printed `✓ No new migrations to check` and exited 0. Both now share one fetch step that fails the job when it fails.
`biome.json:101-102` turns off `noUnusedVariables` and `noUnusedFunctionParameters`, so none of this was ever going to be flagged. Everything here was confirmed by grepping the symbol across `apps/` and `packages/` and finding only its own declaration; `tsc --noEmit` then proves each deleted binding was unread, since a read one fails to compile. - Eleven module-scope loggers that nothing logs through, with the now-orphaned `createLogger` import each left behind. - `execute-platform-context-use-case.ts` — the whole file. No importer, no barrel, and neither export is named anywhere. - `routeToolCall` and, once it goes, `ToolRoute` and `ToolRouteTarget` with it. The catalog accessors around them stay live. - `processPastChat`, superseded by `processPastChatFromDb`. It carried the last `boundary-raw-fetch` exemption in the file. - `withMessageId`, pasted into three server tools and called in none. - Write-only locals: `activeSubagent` (assigned twice, read never — the scoped maps replaced it), `resolvedReadPath`, `workflowPath`, and `workflow` in an execution-core destructure. - `ACCEPTED_AUDIO_TYPES` / `ACCEPTED_VIDEO_TYPES`, never wired to an accept attribute the way their live sibling is. - Unused `catch` bindings in `error-extractors.ts` and `defaults.ts`. `diff-engine.ts` drops a `proposedSubKeys.includes(key)` guard that the `!proposedSub` check three lines down already covers: a key absent from the proposed block reads back `undefined` there, and so does a key present with a nullish value. Same answer on every input, without the O(n) scan per iteration.
…ccess (#7023) The PATCH catch answered `{ success: true }` with 200, so a failed upsert was indistinguishable from a saved one. `useUpdateGeneralSetting` is optimistic: `onMutate` writes the new value into the cache and calls `syncThemeToNextThemes`, and `onError` restores the previous settings. `requestJson` only throws on a non-2xx, so `onError` could never run — the rollback and its theme re-sync were unreachable code. A user toggling a consent-shaped setting (telemetry, email opt-out) saw it applied and it was not saved, until a later refetch quietly reverted it. The catch now returns 500, which is what the mutation was already written to handle. Left alone deliberately: GET still falls back to `defaultUserSettings` on error. Failing it would take the settings page down on a transient read, and the value of changing it is a separate judgement from this one. Covered by a route test that drives the failure through the real handler. Verified it fails when the 200 is put back.
Two places where a failure is reported as something smaller than it is.
**A run that is never billed logs as a notification problem.** The usage
safety net re-records billing when an earlier step threw before the single
record call, and its own failure went into a bare `catch {}`. With a degraded
database the user lookup throws first, the re-record hits the same database and
is swallowed, and the only line emitted reads "Usage threshold notification
check failed (non-fatal)" — which is true of the outer failure and badly wrong
about the inner one. It now logs at error with the execution and workflow ids,
and says the run may be unbilled. The outer warn still covers the email path it
was written for.
**google-docs can ask Drive for a negative page.** `remaining` was
`maxDocs - previouslyFetched` unclamped, where its google-slides twin carries
`Math.max(0, …)` under the comment "Last-page precision". Both then run
`if (documents.length > remaining) documents = documents.slice(0, remaining)`,
and a negative `remaining` makes that guard true for any non-empty page while
`slice` counts from the end — keeping the leading documents and dropping the
trailing ones, where the cap says to keep none. Reachable when `maxDocs` is
lowered while a sync cursor persists. google-drive guards the same case with an
early return; google-docs had neither.
#7020) `check-react-query-patterns.ts` reported a clean strict zone while never looking at part of it. Two gaps in one regex: `\buseQuery\s*\(` does not match `useQuery<Row[]>({ ... })` — a type argument sits between the name and the paren. Twenty query calls carry one, ten of them inside the zero-tolerance zone, so that zone's "0 violations" was partly a statement about what the scan could see. `useQueries` was absent from both the call pattern and the file pre-filter, where `\buse(Query|...)\b` rejects it on the trailing `s`. All sixteen call sites were unscanned, and its options nest one level deeper — inside a `queries` array — so it needs its own pass per entry rather than one that reads the wrapper and takes a single `staleTime` anywhere inside as covering them all. With both closed, three real violations surfaced: - `knowledge-base-selector` served `knowledgeKeys.detail(id)` with an inline `60 * 1000` while `useKnowledgeBaseQuery` serves the same cache key from `KNOWLEDGE_BASE_DETAIL_STALE_TIME`. The two agree only by coincidence, and TanStack resolves staleTime per observer, so tuning the constant would have left this component on the old window for the same entry. - The same call dropped the `AbortSignal`, which `fetchKnowledgeBase` accepts. - `use-permission-config` gave `staleTime` as a literal with no named constant. The new `stale-time-literal` category makes the second half of the CLAUDE.md rule enforceable — it required a named constant, and only the presence of `staleTime` was ever checked. `0` is exempt: it is the sentinel for "always refetch", not a window anyone keeps in step with a prefetch. Verified the new rules can fail by reverting each fix and watching the audit report it, then restoring.
…pages and layouts (#7026) The tool-registry guard collected `page.tsx` and `layout.tsx`, and Next composes three more entries by convention: `error.tsx`, `loading.tsx`, `not-found.tsx`. Twenty-six exist under `app/workspace` and none was walked. `error.tsx` is always a Client Component — Next requires it — so a registry edge there reaches the browser bundle exactly as one from a page does. Coverage goes from 34 entry graphs to 60. Nothing new is reported: the hole was unexploited, and closing it costs nothing. The root deliberately stays at `app/workspace`. Widening it to `app` reports `(interfaces)/resume/[workflowId]/[executionId]/page.tsx`, a Server Component (`runtime = 'nodejs'`, `force-dynamic`) whose `PauseResumeManager` import resolves server-side and never reaches a client bundle. The guard cannot distinguish server from client entries, so it stays where its premise holds.
`totalRoutes` sits at 1162 and the repo has exactly 1162 routes, so the next
route fails CI whether or not it is contract-backed:
API validation audit failed:
- route count increased from 1161 to 1162
The invariant worth holding is that every route has a contract, and
`nonZodRoutes` states exactly that. It is 0, and it rises the moment a route
ships without one — `zodRoutes === totalRoutes` today, so the total adds no
information the other two counters do not already carry.
What it adds instead is a habit. The only way past it is editing the number, and
this file holds seven other baselines that work only while nobody bumps a
baseline casually.
The total is still printed; it is no longer a failure. Verified both directions:
a compliant new route passes where it previously failed, and a route without a
contract still fails through `nonZodRoutes`.
* improvement(ui): standardize modal default actions * fix(ui): keep aggregate deletes on safe default
…ntry (#7028) * fix(ci): require a default export before treating a file as a route entry Follow-up to #7026, which added `error.tsx` to the entry filenames and with it picked up `[workspaceId]/components/error/error.tsx` — named like a boundary, and not one. It exports `ErrorShell` and `ErrorState` for the thirteen real boundaries to use; Next would reject it as a boundary for having no default export. Counting it inflated the coverage number and would have recorded a shared component in the graph-weight baseline as though it were a route. The filename was never the right test. Every convention-composed entry must default-export the thing Next renders, so that is the discriminator now. Entry count goes 60 → 59, and all thirteen real `error.tsx` boundaries still walk. Also adds `template.tsx` and `default.tsx`. Neither exists under `app/workspace` today, so this changes nothing now — but the enumeration claims to cover what Next composes, and leaving two out makes that claim false the day someone adds one. Both raised in review on #7026 (Cursor and Greptile respectively); I merged before reading them, so this lands separately. * fix(ci): count every form that declares a default export `export { default } from './page'` is a valid Next entry and the regex required `as default`, so such an entry would have dropped out of the walk and skipped both the registry gate and the graph-weight ratchet — silently, which is the dangerous direction for a discriminator to fail in. Latent rather than live: the form appears once under `app/workspace`, in a barrel, not in an entry filename. Four forms now count — `export default …`, `export { default } from`, `export { default, … } from`, and `export { X as default }`. `export { default as X }` still does not: it re-exports another module's default under a name and leaves this one without one. Verified all ten variants, including that last distinction. Raised by both Cursor and Greptile on #7028.
Contributor
|
Too many files changed for review (176 files, 100 file limit). Bypass the limit by tagging |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Security and data. Markdown links now use an allowlist ( Chat and settings chrome. Sidebar chat links prefetch the route and history only after hover dwell, keyboard focus, or a real click—not viewport or Modals. Destructive confirms default to dismiss/ Reviewed by Cursor Bugbot for commit 67fc2ae. Configure here. |
waleedlatif1
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 67fc2ae. Configure here.
| ? extractEmbeddedImageIds(docText).includes(ref.fileId) | ||
| : extractEmbeddedImageKeys(docText).includes(ref.key as string) | ||
| const { keys, ids } = extractEmbeddedFileRefs(docText) | ||
| const referenced = ref.fileId ? ids.includes(ref.fileId) : keys.includes(ref.key as string) |
There was a problem hiding this comment.
Public share id match fails
Medium Severity
The referenced-by-doc gate compares extractEmbeddedFileRefs ids to ref.fileId with raw includes. Those ids keep the document’s spelling (including percent-encoding), while the query fileId is normally decoded. Export already bridges that gap with storedFileId; this route does not, so a shared doc whose embeds use an encoded id can fail the gate and serve broken images.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 67fc2ae. Configure here.
* fix(workflow): hide idle nested subflow end handles * perf(workflow): avoid repeated subflow edge scans * perf(workflow): stabilize subflow edge selector --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> Co-authored-by: Waleed Latif <walif6@gmail.com>
Push builds fail the migration audit:
✗ Migration safety check could not run.
Cannot diff against 'HEAD~1'.
`actions/checkout` sets no `fetch-depth`, so it defaults to 1 — a single-commit
clone in which `HEAD~1` does not resolve. Both diff-based audits named `HEAD~1`
as their push base, so neither has ever had a base to read. The migration audit
answered that with `✓ No new migrations to check` and exit 0, so it had never
run on a push build at all; #7022 made it say it could not run instead, which is
what surfaced this. The block-registry check reports `⚠ … skipping` on the same
input — visible, and equally never run.
`HEAD~1` was the wrong base regardless. It names the last commit, so a push
carrying several commits audits the tip and lets every earlier commit through:
3-commit push, HEAD~1 base: mig3.sql
3-commit push, before base: mig1.sql mig2.sql mig3.sql
The base is now `github.event.before` — the tip the branch had before the push,
which is what GitHub provides for exactly this. It is fetched by SHA at depth 1;
the audits diff two tips and need no common ancestry between them. Resolved once
in a step both audits read, so the two cannot drift apart.
`HEAD~1` survives only as the fallback for an all-zero `before` (a new branch,
with no predecessor to diff), which is what `fetch-depth: 2` now covers.
Verified: both audits accept a raw SHA base and pass; the multi-commit case above
is a real reproduction, not a description.
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.
Uh oh!
There was an error while loading. Please reload this page.