mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
47e8f1eb5bde4e07368aded2ebcfd8d84bb49d89
5787
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Waleed
|
47e8f1eb5b |
fix(ssh/sftp): cap remote file reads on received bytes, not stat() size (#6168)
The SFTP/SSH download routes buffered a remote file into memory with the only size guard being sftp.stat().size — a value the caller-supplied SSH server controls. A server that reports a tiny size and then streams endlessly drove unbounded heap growth until OOM. Read through the existing readNodeStreamToBufferWithLimit limiter, which enforces the cap on bytes actually received and destroys the stream on breach, via a small readSftpFileCapped wrapper in sftp/utils. All four SFTP-reading tool routes now share it — download, download-file, read-file-content, and the append path of write-file-content, which had no cap at all. The wrapper keeps a no-op error listener on the stream for its whole life: ssh2 rejects still-pending SFTP requests when the channel closes, which arrives as a late error event after the limiter has detached its own handlers, and an error event with no listener would take the process down. Too-large responses now return 413 to match how the rest of the routes surface PayloadSizeLimitError, and responses report the actual byte count rather than the server-reported stat size. |
||
|
|
18214158b9 |
fix(sanitization): secret exposure in function and agent trace spans (#6000)
* fix trace span secret sanitization * sanitize workflow output logs * preserve streaming usage estimates * Fix logging session test after staging merge * secrets sanitization correctness * fix(execution): address review regressions * fix(execution): harden secret trace provenance * fix(execution): preserve functional state during trace projection --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Mac.localdomain> Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai> |
||
|
|
3740c62112 |
feat(library): Agentic AI Coding Tools: What They Are and How the Top Options Compare (#6164)
* feat(library): Agentic AI Coding Tools: What They Are and How the Top Options Compare * feat(library): add generated cover for agentic AI coding tools post --------- Co-authored-by: Sim Pi Agent <pi@sim.ai> Co-authored-by: Waleed Latif <walif6@gmail.com> |
||
|
Waleed
|
51560390dd |
fix(styles): restore transition timing Tailwind was silently dropping (#6165)
`tailwindcss-animate` adds a `duration-*` utility for `animation-duration`, which collides with core's `transition-duration`. For a named value Tailwind emits both rules, but for an arbitrary value it cannot pick one — so it drops the class entirely and warns. The result is 52 usages across 13 files that produced **no CSS at all**. Elements marked `transition-transform duration-[1700ms] ease-[cubic-bezier(0.22,1,0.36,1)]` had neither a duration nor an easing function: they snapped instead of animating. Most of it is the landing hero. Verified by compiling the stylesheet on both sides: transition-duration: 1700ms before=0 after=1 transition-duration: 420ms before=0 after=1 transition-duration: 175ms before=0 after=1 transition-timing-function: cubic-bezier(0.22,1,0.36,1) before=0 after=1 transition-timing-function: cubic-bezier(0.23,1,0.32,1) before=0 after=1 The stylesheet grows 1,425 bytes — exactly the rules that were missing — and the 16 ambiguity warnings on every dev start go to zero. Every usage is in a transition context (`transition-opacity`, `transition-transform`, `transition-[...]`); none sits alongside an `animate-*` utility, so `transition-duration`/`transition-timing-function` is the right half of the collision in all 52 cases. |
||
|
Waleed
|
03649e934c |
refactor(dev): remove the minimal-registry escape hatch (#6163)
* refactor(dev): remove the minimal-registry escape hatch `dev:minimal` existed because the tool registry was 71-82% of every workspace route's module graph and aliasing it away was the only way to make dev bearable. The metadata work removed that reason, so the hatch now buys almost nothing: before this stack 31.7s -> 20.0s cold (-37%) after this stack 22.9s -> 20.7s cold (-10%) A 10% cold-compile win, on the run that happens once — restarts are ~4.2s either way — is not worth what it costs. `tools/registry.minimal.ts` and `blocks/registry-maps.minimal.ts` are 283 lines of hand-curated duplicates of the real registries that **nothing keeps in sync** (no lint, no CI check, no test); they are correct today only because someone remembered. And the mode is actively misleading: it silently drops ~250 services and ~280 blocks, so anything reproduced under it may not reproduce for real. Removes both files, the `SIM_DEV_MINIMAL_REGISTRY` branch from `next.config.ts` (including the whole `webpack()` hook, which existed only for this), and the `dev:minimal` / `dev:full:minimal-registry` scripts. Verified after removal: `tsc` clean, boundary + metadata + skills + monorepo gates pass, and `next dev` starts and serves the canvas at 22.6s cold / HTTP 200. * fix(setup): stop the wizard offering the removed minimal-registry mode The setup wizard prompted for a dev server on machines under 16GB and **defaulted** to `dev:full:minimal-registry` — a script this stack deletes. Anyone running `bun run setup` on a low-RAM machine would have accepted the default and hit "Script not found", which is exactly the contributor the mode existed to help. Repointed at `dev:full:capped`, which still exists and caps Node at 4GB without dropping ~250 integrations — a strictly better answer to the same question. The hints were also stale: they warned the full registry "can use 4-5GB+ on its own", which was true when a dev server sat at 11.5GB. It now sits at ~4GB, so they say that instead. Missed by an earlier sweep because the pattern searched for `dev:minimal` and `registry.minimal`, and this string is `dev:full:minimal-registry` — the two halves reversed. Re-swept across every file type for all spellings: zero references remain. Also audited every script value the wizard can return, so the class of bug is checked, not just this instance. |
||
|
Waleed
|
e8894a8764 |
perf(tools): guard the tool-registry client boundary in CI (#6156)
* perf(tools): guard the tool-registry client boundary in CI
The registry was 71-82% of every workspace route's module graph, and the two
edges that put it there were invisible at the call site: `providers/utils.ts`
imported `mergeToolParameters`, and `mcp-dynamic-args.tsx` imported
`formatParameterLabel`. Neither import looks remotely like "pull in 4,700
modules of SDK clients", which is why this needs a lint rather than a convention.
`check-tool-registry-boundary.ts` walks the value-import graph (skipping
`import type`, which is erased) from the workspace layout and the four routes
that mount inside it, and fails if `@/tools/registry` is reachable — printing
the exact chain that reintroduced it.
Verified it fails: reintroducing a `getTool` import in `serializer/index.ts`
exits 1 and names the chain through `stores/workflow-diff/store.ts`; removing it
returns to 0.
There is deliberately no allowlist. The fix for a failure is always to move the
symbol the file actually needs into a registry-free module, not to exempt the
route.
Documents the guard in the tool-registry-boundary skill.
* fix(tools): close two edge-detection gaps in the registry boundary guard
Review found the walker missed two forms, both verified against a matrix of
every import/export shape:
export * as ns from '…' namespace re-export — the star branch had no alias
import('…') dynamic import
A dynamic import splits the registry into its own chunk rather than the route's
initial one, so it does not show up in cold-compile time — but it still puts
4,300 tools' worth of executable config on a client path, which is what this
guard exists to prevent. It counts as reaching the registry. No such import
exists today; this is purely closing the hole.
Adding both raised the measured counts (tables 1,217 -> 1,261, files
1,310 -> 1,419) because lazily-loaded modules are now counted. The registry
stays unreachable from all five entries.
Also checked and rejected: side-effect imports (`import '@/x'`) were reported as
missed, but are matched both standalone and after another import — the `from`
clause is already optional.
* fix(tools): resolve extensionful specifiers in the boundary guard
`resolveSpecifier` probed `base + ext` and `base/index + ext` but never `base`
itself, so an already-extensioned specifier resolved to null and its edge
vanished from the walk — `import { tools } from '@/tools/registry.ts'` would
have passed the guard silently.
Not theoretical: `executor/execution/block-executor.ts` already imports
`@/executor/human-in-the-loop/utils.ts` with the extension, so real edges were
being dropped. Counts rise slightly now that they are followed (canvas
2,023 -> 2,029).
Verified: the extensionful import exits 1, and removing it returns to 0.
* fix(tools): discover guard entries instead of listing them
Review caught the guard checking the wrong shell: it named
`app/workspace/layout.tsx` as "the shared shell every route mounts inside", but
that file only wraps `SocketProvider`. The real shell is
`app/workspace/[workspaceId]/layout.tsx`, which pulls in `WorkspaceChrome`, the
loaders and the providers — and it was never checked.
Worse, layouts are composed by Next.js convention rather than imported, so a
page's graph never reaches its layout at all. Walking pages alone left every
layout module outside the guard.
So entries are now discovered: every `page.tsx` and `layout.tsx` under
`app/workspace`, 35 of them instead of a hand-written 5. A list goes stale
silently; discovery cannot. Refuses to pass vacuously if the walk finds none.
Immediately found a real edge the hand-written list had missed — the settings
route reaching the registry through a dynamically-imported access-control panel
(fixed in the previous commit). Full walk takes ~2s.
Also restores the extensionful-specifier fix, which a bad merge had dropped from
this file. Re-verified both directions: an extensionful `@/tools/registry.ts`
import exits 1, removing it returns to 0.
* fix(tools): restore the dynamic-import and namespace-alias edge detection
A bad merge during a rebase reverted this file to a pre-fix revision, silently
dropping `DYNAMIC_IMPORT_RE` and the `export * as ns from` alias branch that
earlier commits on this branch had already added. The guard still passed, which
is the worst way for a lint to break — it simply stopped following edges.
Caught it because the per-route counts fell after the rebase (files
1,424 -> 1,314, logs 1,610 -> 1,545) rather than staying put. A guard that
reports fewer modules after a no-op merge is not passing, it is blind.
Now verified against every bypass form rather than the one I happened to think
of, so a future regression of this kind fails loudly:
CAUGHT extensionful import { tools } from '@/tools/registry.ts'
CAUGHT dynamic import('@/tools/registry')
CAUGHT ns re-export export * as ns from '@/tools/registry'
CAUGHT side-effect import '@/tools/registry'
CAUGHT plain named import { tools } from '@/tools/registry'
clean tree passes
* fix(tools): traverse require() edges in the boundary guard
Review flagged `require()` as an untraversed edge form, and it is not
hypothetical here — this codebase uses lazy `require('@/…')` to break import
cycles, including from a client-reachable file (`tools/params.ts` reaches
`@/blocks` that way). Those edges are as real as static imports; a `require` of
the registry would have walked straight past the guard.
The audit now covers every form a module can be reached by, each verified rather
than assumed:
CAUGHT plain named import { tools } from '@/tools/registry'
CAUGHT side-effect import '@/tools/registry'
CAUGHT extensionful import { tools } from '@/tools/registry.ts'
CAUGHT ns re-export export * as ns from '@/tools/registry'
CAUGHT dynamic import('@/tools/registry')
CAUGHT require require('@/tools/registry')
clean tree passes
No new violations surfaced — the 35 guarded page/layout graphs stay clean with
require edges followed.
|
||
|
Waleed
|
452d82a636 |
perf(tools): read tool metadata instead of the registry on client paths (#6155)
* perf(tools): read tool metadata instead of the registry on client paths Cuts the last four edges that pulled `@/tools/registry` into the workspace shell. Every workspace route drops ~4,700 modules: route before after /w (canvas) 6,592 1,908 -71% /logs 6,227 1,543 -75% /tables 5,903 1,217 -79% /files 5,996 1,310 -78% workspace layout 5,751 1,063 -82% Dev cold compile of the canvas, n=3, cache cleared between runs: before 32.3s / 31.4s / 30.1s RSS 9.0-12.5 GB after 22.4s / 22.2s / 21.6s RSS 7.8-9.2 GB That lands where the `dev:minimal` escape hatch measured (20.0s / 6.7 GB) without its downside — `dev:minimal` swaps in curated registries that drop ~250 services, whereas this keeps every tool working. Rewired: - `block-outputs` -> `getToolOutputsMetadata` (needed `outputs`) - `serializer` -> `getToolParams` (needed `params`) - `validation` -> `hasToolId` (needed existence only) - `tools/params` -> `getToolMetadata` (needed `params`, `oauth`, `name`) `tools/params.ts` was the stubborn one: `mcp-dynamic-args.tsx` imports only `formatParameterLabel` from it, so the whole registry rode in behind a string helper — the same shape as the `mergeToolParameters` edge cut earlier. Adds a third generated artifact, `tool-ids.ts` (~110 KB). Resolution needs only the key set, so `@/tools/metadata` and `@/tools/metadata-outputs` both resolve through it and stay independent of each other, and an existence check costs ~110 KB instead of ~4 MB. Behaviour preservation was the risk here: `getTool` resolves an unversioned name onto its newest version, and a plain key lookup would have silently reported 246 versioned tools as missing. `resolveToolId` is reproduced against the id set and differentially tested — 4,404 probes (every id, every stripped base name, and an unknown) comparing old vs new resolution and existence: 0 mismatches. `ToolWithParameters.toolConfig` and `SubBlocksForToolInput.toolConfig` narrow from `ToolConfig` to `ToolMetadata`. The only external reader is `tool-input.tsx`, which uses `.name`. * docs(tools): point the boundary skill at the three metadata modules The skill still routed `hasToolMetadata` and `getToolIds` to `@/tools/metadata`, but this PR moved id resolution into `@/tools/tool-ids`. Left as-is it would send the next caller to the 4 MB module for an existence check that costs 110 KB — the exact mistake the skill exists to prevent. Also records the two properties a caller can silently get wrong: lookups guard with `Object.hasOwn` (a bare bracket lookup returns inherited prototype members), and they resolve unversioned names (246 tools are versioned, and a plain lookup reports them missing rather than crashing). * fix(tools): cut the settings-route registry edge and fix serializer test mocks Two findings from review, both real. The settings route still reached the registry: settings/[section]/page.tsx -> settings.tsx -> (dynamic import) ee/access-control/components/access-control.tsx -> group-detail.tsx -> tools/utils.ts -> tools/registry.ts It reads `getTool(id)?.name` — metadata — so it moves to `getToolMetadata`. The earlier audit missed it because it walked only from the canvas route, and the edge hides behind a dynamic `import()` that a static walk skips. Serializer tests mocked the wrong module. `Serializer` now reads params via `getToolParams` from `@/tools/metadata`, but the tests still only mocked `@/tools/utils`, so they controlled nothing and passed because the real generated artifacts happen to agree with the fixtures. Adds `toolsMetadataMock` to `@sim/testing/mocks`, backed by the same `mockToolConfigs` as `toolsUtilsMock` so a test mocking both sees one consistent tool universe, and mocks it in the three serializer suites. Verified the mock is now load-bearing: pointing it at a sentinel param makes the three user-only-required validation tests fail, and restoring it returns all 110 serializer tests to green. Before this they passed either way. * fix(tools): freeze the tool id array handed out by getToolIds `getToolIds()` returned the module's internal array by reference, so a caller doing `getToolIds().sort()` would reorder it in place and silently corrupt every later lookup — the in-place-mutation footgun `.claude/rules/sim-react-performance.md` calls out. Frozen rather than copied: the array is consumed in loops, so copying would allocate on every call. Freezing makes the mutation throw instead of corrupt, and `[...getToolIds()].sort()` still works. Return type is now `readonly string[]`, so the mistake is a compile error rather than a runtime surprise. No caller mutates it today; this is closing the hole, not fixing a live bug. * test(tools): enforce that the two tool-id resolvers never diverge `resolveToolId` now exists twice on purpose — `@/tools/utils` resolves against the live registry (so a tool added before regeneration still resolves at runtime), `@/tools/tool-ids` against the generated id list (so client code resolves without importing 4,300 tools). Nothing structurally kept them in step; a change to versioning logic in one would silently drift from the other. `tool-metadata:check` now asserts they agree across every id, every stripped base name, and an unknown — 4,404 probes — and only after the staleness check passes, so a missing regeneration reports as staleness rather than as drift. Verified it fails: breaking resolution for `gmail*` exits 1; restoring it passes. It cannot live in a vitest suite. `vitest.setup.ts` globally mocks `@/tools/registry` to an empty map, so `getTool` resolves nothing there — a parity test written as a spec passes or fails for the wrong reason. Both facts are recorded where the code is. Both resolvers stay exported. An earlier pass here un-exported the `@/tools/utils` one as dead; `tools/utils.server.ts` imports it through a multi-line import that a grep missed, and `tsc` caught it. Its doc now says which resolver a caller should reach for instead of leaving two identically-named functions unexplained. |
||
|
Waleed
|
d6e08d38d7 |
perf(tools): generate serializable tool metadata artifacts (#6153)
* perf(tools): generate serializable tool metadata artifacts
Adds `scripts/sync-tool-metadata.ts`, which projects the executable tool
registry down to the data half nobody needs a closure for, plus typed accessors
over the result. No consumer is rewired yet — that is the next PR.
`@/tools/registry` is a ~9,000-line barrel over 4,366 tools. Each `ToolConfig`
mixes plain data (`params`, `outputs`, `name`) with closures (`request.headers`,
`transformResponse`, `directExecution`, `postProcess`), and those closures reach
every integration's SDK client and parser — which is why reaching the barrel
costs ~4,700 modules. Every client-reachable caller was audited: none of them
need a closure. They need `outputs`, `params`, or an existence check.
Two artifacts, not one. `outputs` is ~4 MB of the ~8 MB and has a single
consumer, so it is emitted separately and exposed from its own module; callers
needing only params never load it.
The data is a JSON string parsed at runtime rather than an imported `.json` or
an object literal. That is not stylistic — with `resolveJsonModule` (enabled
repo-wide) a `.json` import makes TypeScript infer a literal type for all 4,366
entries:
tsc --noEmit, baseline 12.6s
tsc --noEmit, with `.json` imports 8m07s (38x)
tsc --noEmit, with string literals 12.0s
An ambient `declare module` does not short-circuit it (measured: 8m18s), and an
object literal is the same inference work. A single string literal is one cheap
token for the compiler and the bundler, and `JSON.parse` beats evaluating the
equivalent literal at runtime.
The generator refuses to emit any function value, so shipping executable config
to the client fails loudly instead of silently. `hosting` and `schemaEnrichment`
are excluded on those grounds — both hold functions and are server-only.
Also strips empty param entries: the registry has one (`stt_deepgram_v2`, an
`undefined`) which crashes callers that read `param.type` while iterating.
`JSON.stringify` drops `undefined` on its own, so the guard is there for an
explicit `null` — which serializes faithfully and would reach consumers — and to
warn either way.
Wires `tool-metadata:check` into CI alongside the other generated-contract
gates, and ignores the generated directory in biome (it exceeds the 1 MB limit
and was being skipped with a notice on every commit).
Adds a `tool-registry-boundary` skill covering which module to import, the three
non-obvious properties of the artifacts, and how to verify an edge is actually
cut — the canvas route reaches the registry through four redundant paths, so
cutting one alone moves the module count by ~1.
* fix(tools): harden the metadata accessors against inherited keys
Review found two real defects in the generated-metadata layer.
`JSON.parse` returns an object with the normal prototype, so a bare bracket
lookup resolved inherited members: `getToolMetadata('constructor')` returned a
*function* typed as `ToolMetadata`, and `getToolOutputsMetadata('toString')`
likewise — silently violating the accessors' documented "undefined if unknown"
contract. Guarded with `Object.hasOwn`, with a parameterised regression test
over `constructor`, `toString`, `valueOf`, `hasOwnProperty` and `__proto__`.
The generator's no-functions scan also gave up past ten levels of nesting. Param
and output schemas nest arbitrarily, so a deeper closure would have been dropped
silently by `JSON.stringify` while generation reported success — shipping an
incomplete schema and defeating the guarantee the scan exists to provide. The
depth cap is gone; a `WeakSet` handles the cycles that exposes.
* docs(tools): tell tool authors to regenerate the metadata artifacts
A new tool now has a second registration step. Client code reads `params` and
`outputs` from the generated artifacts rather than from the registry, so a tool
added without regenerating them is registered but invisible to the UI — and CI
fails on the stale artifacts.
`add-tools` and `add-integration` are where someone actually adds a tool, so the
step goes in both, next to the registry edit and in each checklist.
* docs(blocks): note when a block change needs tool-metadata regeneration
Adding a block alone needs no regeneration — it references existing tool IDs and
changes no tool's shape. But a change that touches a tool alongside the block
does, and this is where that is easy to miss: a block's `outputs` are authored
to match its tools' outputs, and the UI now reads those from the generated
metadata, so a stale artifact makes the block's declared outputs disagree with
what the panel renders (and fails CI).
Completes the tool-authoring surface alongside add-tools and add-integration.
* docs(tools): cover tool removal in the regeneration guidance
The three tool-authoring skills said to regenerate after adding or changing a
tool, but not after removing one. Removal is equally breaking and equally
guarded: deleting a tool from `tools/registry.ts` without regenerating fails
`tool-metadata:check` (verified — exit 1), so a contributor following the skill
literally would have hit a CI failure the skill never warned about.
|
||
|
Waleed
|
9299cee58d |
perf(tools): move mergeToolParameters into a registry-free leaf module (#6152)
* perf(tools): move mergeToolParameters into a registry-free leaf module
`providers/utils.ts` imports exactly one thing from `@/tools/params`:
`mergeToolParameters`. That function performs no tool lookup at all — it merges
two plain param objects. But `params.ts` imports `getTool` from `@/tools/utils`,
which statically imports the 4,300-entry `@/tools/registry` barrel, so that
one-symbol import was dragging the entire tool registry into every module graph
that reached it.
Measured with a module-graph walk from each entry:
tools/params.ts 4,926 modules (registry reachable)
providers/utils.ts 4,926 modules (registry reachable) -> 22 modules ✅
tools/merge-params.ts 2 modules (registry NOT reachable)
`mergeToolParameters`, `deepMergeInputMapping` and `isNonEmpty` move to
`@/tools/merge-params`, which is forbidden from importing `@/tools/utils`,
`@/tools/registry` or `@/tools/params`. `params.ts` now imports `isNonEmpty`
from there; its `isRecordLike` and `isEmptyTagValue` imports became unused and
are dropped. The two consumers (`providers/utils.ts`,
`executor/handlers/pi/sim-tools.ts`) import from the new module directly rather
than via a re-export, per the no-re-exports rule.
This is preparation, not the payoff. The canvas route still reaches the registry
through three other edges (block-outputs, serializer, sanitization/validation) —
all four are redundant paths and must all be cut before the route's module count
moves. Those follow in the metadata-manifest PRs.
Behaviour is unchanged: the moved functions are copied verbatim.
* refactor(tools): make deepMergeInputMapping module-private
It was exported from `@/tools/params` and imported by nothing — a private helper
of `mergeToolParameters` that had leaked into the public surface. Since this move
created the module, the export goes with it rather than being carried forward.
Verified zero consumers repo-wide before dropping it.
|
||
|
Waleed
|
28abbc94c6 |
perf(dev): re-enable the Turbopack dev filesystem cache (5.4x faster restarts) (#6151)
* perf(dev): re-enable the Turbopack dev filesystem cache (5.4x faster restarts) `turbopackFileSystemCacheForDev` has been `false` since #5408 — a landing-page homepage redesign whose description covers hero cards, feature-card aspect ratios, eyebrow chips and a voice-input button color, and never mentions Turbopack, caching, or dev performance. It was collateral, not a decision, and it overrode the Next default (true since v16.1). It is not the flag #6078/#6080 measured. That A/B was `...ForBuild` and its conclusion stands — the build cache is a 3.2x regression and stays off. The two flags look alike and are opposite decisions; both are now commented as such. Measured on `/workspace/[workspaceId]/w`, n=3 per arm, SIGINT between runs: cache OFF 31.4s / 30.1s / 31.9s RSS 9.0-9.8 GB cache ON 5.6s / 5.6s / 5.5s RSS 4.4-5.1 GB 5.4x faster restarts, ~2x less resident memory. Cold compile against an empty cache is unchanged (~32s either way) — the cache only pays back on restart, which is the loop that actually hurts. The cache is unbounded on disk: the abandoned one on this machine had reached 78 GB across 1,848 SST files, and a stale cache is slower to read back, so left alone it erodes the win it exists to provide. `prune-turbopack-cache.ts` runs on `predev` and drops it past a cap (default 20 GB, `SIM_TURBOPACK_CACHE_MAX_GB` to override); `bun run dev:cache:prune` forces it. It never blocks `next dev` on a maintenance failure. Adds a `dev-performance` skill recording the cost model, the reference numbers, and the benchmarking method — including that stopping the server with `kill -9` mid-cache-write discards the cache and makes this exact win read as no win. * improvement(dev): chain the cache prune into dev scripts instead of a predev hook Review read the root `bun run dev` path as bypassing the `predev` hook and so never capping the newly-enabled cache. Turbo does fire `pre*` hooks — verified live, the run prints the prune before `next dev` — but the concern is fair in that the guarantee rested on package-manager lifecycle semantics that are invisible at the call site. Chaining it explicitly removes the question entirely: every `dev` variant now runs `bun run dev:cache:cap && …`, which holds on any invocation path, is visible in the command itself, and drops the three duplicated `predev:*` entries for one shared script. Verified on both paths — direct `bun run dev` and root `turbo run dev`, the latter printing: sim:dev: $ bun run dev:cache:cap && next dev --port 3000 sim:dev: $ bun run ../../scripts/prune-turbopack-cache.ts * docs(dev): document cache-corruption recovery, the cost of enabling the cache Stress-tested the failure mode rather than assuming it: deliberately corrupting an SST block makes Turbopack abort with a FATAL panic — it does not self-heal. FATAL: An unexpected Turbopack error occurred. Cache corruption detected: checksum mismatch in block 4 of 00000221.sst `bun run dev:cache:prune` and restart fixes it; verified the canvas serves 200 again afterwards. Documented in the skill and in the script's header, since the symptom is a hard crash and the remedy is not guessable. This is the honest cost of turning the cache on. It is worth paying — a 5.4x faster restart against a rare, loud, single-command failure — but it should be written down rather than discovered. Worth distinguishing from the adjacent case: an ordinary hard kill does *not* corrupt the cache. Turbopack discards a partially-written cache and rebuilds it silently, which is exactly why a `kill -9`-based benchmark reads as "no cache win" (noted in the benchmarking section). * refactor(dev): drop the dev-performance skill, keep its findings at the code A whole skill was too much for what this is. The parts that are load-bearing — why the two lookalike cache flags are opposite decisions, the measured numbers, the corruption remedy, and the benchmarking trap — now live in the config and script they describe, where someone changing the flag actually reads them. The trap is the piece worth keeping: `next dev` compiles on demand so startup time is meaningless, and stopping the server with `kill -9` makes Turbopack discard a partially-written cache and rebuild silently — which reads as 'the cache does nothing' and is how this flag stayed wrong for a month. Dropped rather than relocated: generic advice that was not specific to this repo (antivirus, Docker-on-macOS, orphaned processes) and a measured no-op (`optimizePackageImports` for lucide-react changed nothing, 31.6s vs 31.7s). * docs(dev): record the measured cost and concurrency behaviour of cache pruning Stress-tested the maintenance path rather than assuming it is free. Cost: the size walk is ~30ms on a real cache and ~85ms at 2,000 files — under 2% of a 4.2s warm restart, and invisible against a cold one. It runs before every dev start, so it needed to be cheap; it is. Concurrency: pruning while a dev server is live (which happens when a second server is started from the same checkout) does not crash it. The running server keeps its in-memory state and kept serving HTTP 200 with zero panics. It does stop persisting for the rest of that session, so its next start is cold once — verified recovering at 23.4s then 4.5s. Worth writing down because the directory silently never reappears mid-session, which looks like a bug if you go looking. The cap is a backstop, not routine: a normal session sits at 1-2 GB against a 20 GB default. * fix(dev): cap every app's Turbopack cache, not just apps/sim `apps/docs` is a Next app too (`next dev --port 3001`) and overrides nothing, so it uses the Next default where the dev filesystem cache is on. It already had an uncapped 1.1 GB cache here, and the root `bun run dev` (`turbo run dev`) starts it — so a teammate using the documented command was accumulating a cache nothing would ever prune. The script now resolves its target from the working directory instead of hardcoding `apps/sim`, and each app chains its own cap. Per-app rather than one sweep on purpose: a single pass would let one app's dev start delete a cache another app is holding open, which costs that session its persistence. Verified both: `apps/sim` and `apps/docs` each report and cap their own 1.1 GB cache, and both dev servers start clean (`Ready in 299ms` / `229ms`, docs serving). * refactor(dev): drop dev:cache:prune in favour of the existing dev:clean `dev:cache:prune` duplicated `dev:clean`, which already existed in `apps/sim` and does strictly more (`rm -rf .next/dev/cache` covers the Turbopack cache plus the fetch and image caches). Two commands for one job is worse than one, and the docs pointed at the newer, narrower of the two. Removes it from both apps and gives `apps/docs` the `dev:clean` that `apps/sim` already had, so the recovery command is the same everywhere. `dev:cache:cap` stays — it is the chained step, used by more than one dev variant, and naming it keeps the relative script path out of each command. Verified `dev:clean` is a real remedy: corrupt a cache block, run it, restart — canvas serves 200 with no panic. Also corrects an overstatement. A damaged cache does not *always* abort Turbopack; whether it panics depends on whether the damaged region is read, so it is not reliably reproducible. Both notes now say "can abort" and give the same remedy either way. |
||
|
Justin Blumencranz
|
273ca06ddb |
fix(files): serve rendered documents instead of source code (#6139)
* fix(files): serve rendered documents to attachments and workflow reads Generated documents store their generation source under a .pdf/.docx name and keep the compiled binary in a separate content-addressed artifact store, so any consumer doing a raw read handed out source text under a document name. - Route attachments and readUserFileContent through the servable resolver so they get the compiled artifact, and stop the internal generation-source MIME marker reaching providers as a content type. - Render on read when the artifact is missing. The artifact key is (workspace, source hash), so forking a workspace, moving a file, or editing the source outside a recompiling writer orphaned it permanently and reported "still being generated" forever. Rendering self-heals those and stores the result. - Fall back to serving stored bytes as application/octet-stream when a render fails, instead of failing forever, and remember not to retry those bytes. - Only compile without a workspace context when the file's type positively says it is generation source, so unrelated stored bytes are never executed. - Make the doc-not-ready error opt-in per caller and give it a 409 via HttpError, so output decoration degrades instead of failing completed work. * fix(files): keep render failures retryable and never relabel unrendered bytes Addresses the first review round. - Only memoize a render failure when it is deterministic. A DocCompileUserError means the source will never render, so remembering it is safe; sandbox outages, timeouts, and cancellations are transient and were stranding valid documents for the life of the process. Infra failures now propagate, which also restores DocCompileUserError reaching callers that map it to 409. - Refuse to hand back bytes the resolver could not render. readUserFileContent returns a string, so the resolver's honest application/octet-stream could not travel with it and attachment builders re-inferred a document MIME from the filename — shipping generation source to a provider as a PDF. The file-serve route keeps the graceful passthrough, where a human downloading the bytes is useful. - Normalize the declared type once so a padded or upper-cased source marker cannot pass the resolver gate on one code path and fail it on the other. - Import the doc-not-ready guard lazily. The static import pulled the doc-compile module graph (remote sandbox, task runner, execution limits) into every hydration consumer and broke an unrelated test's module mock in CI. * perf(files): coalesce concurrent renders of the same missing artifact An artifact miss is identical for every concurrent reader — a freshly forked workspace whose document several viewers open at once, or one request whose blocks read the same file — and each was paying for its own compile of the same bytes. Share one in-flight render per (workspace, source, ext) key and drop the entry as soon as it settles, so a later read still re-renders normally. * fix(files): refuse unrendered bytes at the download boundary Addresses the second review round. - Throw UnrenderableDocumentError from downloadServableFileFromStorage instead of returning bytes with an `unrendered` flag. Around 45 call sites (email attachments, cloud uploads, zip entries, provider attachments) receive only a Buffer and re-infer the type from the filename, so a flag they must remember to check is a flag they will not check. Those callers already handled the previous not-ready throw, so failing is the shape they expect. The file-serve route is unaffected — it resolves bytes directly and keeps the graceful passthrough, where a human downloading the file has a use for it. - Surface that failure through hydration: with throwOnDocNotReady set, the caller cannot use a file with no content, so an unrenderable document now reaches it verbatim instead of degrading to null and reporting a misleading "may exceed size limit or no longer accessible". - Stop a shared render inheriting one caller's cancellation. The coalesced run no longer carries any caller's signal; each caller races its own instead, so an aborting reader gives up promptly while the render finishes for the others and still lands in the cache. * fix(files): move the unrenderable error out of the 'use server' module file-utils.server.ts carries 'use server', whose exports must all be async functions, so exporting an error class from it failed the production build with 67 cascading errors. The class now lives in the plain file-utils.ts beside the other shared file helpers, which also lets the hydration path import it directly instead of through a dynamic import. Also bounds how long a failed render is remembered. The isolated-vm engine cannot tell a bad source from a sandbox outage, so a permanent entry let one transient failure block re-rendering that source for the life of the process. Entries now expire after five minutes: long enough to stop a read loop spending a sandbox run per read, short enough that an outage self-heals without a deploy. * fix(files): finish the render cancellation and failure-surfacing edges - Race the E2B render against the caller's signal too. Only the isolated-vm branch did, so an aborted request on the E2B path waited for the sandbox to finish and could return a success the caller no longer wanted. - Attach a terminal handler to the shared render. Every caller races it against its own signal, so all of them can walk away; a later rejection with no waiters left would otherwise surface as an unhandled rejection. - Stop narrowing what throwOnDocNotReady rethrows. readUserFileContent now runs document compiles and can fail in ways this module has no business enumerating; narrowing produced three consecutive review rounds of "this particular failure is still swallowed". The flag means "do not degrade". - Do not mark an unrendered response immutable. The serve route caches versioned responses for a year, which would pin a one-off render failure to that URL long after a later compile succeeds on the same version. * revert(files): drop the concurrent-render coalescing The coalescing was an optional efficiency win — rendering is content-addressed and idempotent, so duplicate concurrent renders produced the same artifact and cost only extra sandbox time on an artifact miss. It bought that at the price of the most intricate code in the change set, and produced three concurrency findings across two review rounds: a shared render inheriting one caller's cancellation, an E2B/isolated-vm asymmetry in how the signal was raced, and orphaned rejections once every caller could race away. Removing it also restores true cancellation on the isolated-vm path: the caller's signal now reaches runSandboxTask again, so an abort cancels the sandbox work rather than only abandoning the wait for it. * fix(review): simplify generated document attachments * fix(files): mock servable downloads in hydration tests * fix(files): preserve rendered attachment semantics * fix(files): preserve cached artifact size * fix(files): refuse unresolved xlsx source * fix(files): resolve execution artifact workspace |
||
|
Waleed
|
06506bbd38 |
improvement(files): cache stream binding meta, tighten file cache-control, prune dead persist path (#6149)
* improvement(files): cache stream binding meta, tighten file cache-control, prune dead persist path - Cache the agent-stream ProseMirror↔Yjs binding metadata per session and reuse it across streamed frames instead of rebuilding it (O(doc)) every frame; matches how y-tiptap's own binding maintains the mapping in place. Safe because the shadow doc only ever sees the agent's own reconciles. - Default createFileResponse to a private, no-cache policy so auth-gated bytes are never stored in a shared cache; genuinely-public serve routes opt into public caching explicitly. - Serve content-addressed (key=) embedded images with an immutable private cache to avoid re-downloading them on every doc re-open; fileId= embeds and public shares keep revalidating (the underlying key can change / a share can be revoked). - Drop the dead conflict.version field from the collab-doc persist result and remove the wasted getWorkspaceFile re-read on the conflict path (the relay treats missing and conflict identically and never read version). - Decorate-sort the files/folders lists (compute each sort key once) and index the move-menu subtree build (O(N) vs O(N^2)); ordering is preserved exactly. * fix(files): serve inline images with no-cache so deletion/authorization is enforced per request Embedded images are authenticated content whose backing file can be deleted or have its access revoked at any time. Both key= and fileId= embeds serve private, no-cache, must-revalidate, so every request re-runs the server-side deletion/authorization check instead of serving a possibly-stale image from cache. |
||
|
Waleedandmzxchandra
|
10bfb5d139 |
feat(realtime): shared room spine + live Files/Tables collaboration + Yjs document editing (#5991)
* feat(realtime): add shared room identity + authorization spine (#5929) Introduces the foundation for a unified realtime "room" model spanning the Socket.IO presence server (apps/realtime), the durable SSE event log, and the ephemeral pub/sub fanout — all of which today reinvent their own room identity, naming, and authorization. - @sim/realtime-protocol/rooms: RoomRef { type, id }, ROOM_TYPES, and a roomName/parseRoomName codec. WORKFLOW deliberately maps to the bare id so the ~40 existing io.to(workflowId) callsites and presence state keys are unchanged; every other room type is namespaced so id spaces cannot collide. - @sim/platform-authz/rooms: authorizeRoom(userId, room, action) generalizing the exemplary authorizeWorkflowByWorkspacePermission — one resource->workspace resolver per room type, then the shared resolveEffectiveWorkspacePermission + permissionSatisfies gate. Pure foundation, no behavior change: nothing consumes these yet. Prune graph stays at 14/25 (platform-authz already depended transitively on realtime-protocol via apps/realtime). * refactor(realtime): generalize presence server to multi-room [2/N] (#5930) * refactor(realtime): generalize presence server to multi-room (RoomRef) Generalizes the Socket.IO presence layer from single-workflow-room-per-socket to a domain-neutral, multi-room-per-socket model keyed by RoomRef, so a second domain (workspace files, next PR) can reuse the same membership + presence engine. Behavior-preserving for workflow collaboration. IRoomManager is now domain-neutral (addUserToRoom/removeUserFromRoom/ getRoomForSocket/getRoomUsers/updateUserActivity/... all take a RoomRef). The workflow lifecycle broadcasts (deletion/revert/update/deploy) move out of the manager into WorkflowRoomService, composed over the generic manager. Backward-compat by design (no workflow migration, no regression): - Workflow Socket.IO room name stays the bare workflowId (roomName() maps workflow -> bare id), so the ~40 io.to(workflowId) callsites are untouched. - Workflow Redis presence keys stay workflow:{id}:users/:meta (the type prefix IS "workflow"). Multi-room correctness (from adversarial audit): - socket:{id}:workflow single-value key -> socket:{id}:rooms HASH (type->id). - The SHARED socket:{id}:session key is deleted only when the socket leaves its LAST room (refcount via HLEN) — a leave from one room no longer breaks the other room's handlers. - disconnect enumerates the socket's stored rooms and rebroadcasts presence per room, instead of picking an arbitrary socket.rooms entry. - presence broadcasts use a per-room-type event name (workflow keeps the bare presence-update; others are namespaced). Workflow handlers wrap manager calls with a shared workflowRoom(id) helper; UserPresence.workflowId -> room (the client never reads that field). Tests: existing 112 realtime tests pass unchanged (behavior gate) + 7 new multi-room tests (refcounted session, presence isolation, multi-room disconnect, per-type event names). tsc clean, boundaries + prune (14/25) green. * fix(realtime): harden multi-room disconnect + id-guard room removal Two fixes from an adversarial regression audit of the multi-room refactor: - Disconnect now handles `disconnecting` (where `socket.rooms` is still populated and authoritative) and falls back to the live Socket.IO room set for any room the manager's stored state no longer tracked. This restores reliable presence cleanup + departure broadcast even if the Redis `socket:{id}:rooms` key was evicted or TTL-expired — the one behavioral gap vs the pre-refactor disconnect. - REMOVE_ROOM_SCRIPT now only drops the socket's room mapping (and runs the last-room session cleanup) when the stored id matches the room being removed, matching the memory manager's existing id guard. Prevents a mismatched-room call from wiping a different room's mapping or the shared session. +1 test (id-guarded no-op removal). 120 realtime tests pass, tsc clean. * fix(realtime): only rebroadcast disconnect-fallback rooms whose removal succeeded Greptile 4/5 follow-up: the disconnecting-time fallback ignored removeUserFromRoom's boolean and rebroadcast presence even when the removal reported false. Now it only treats a room as removed (and rebroadcasts) when the manager confirms it — symmetric with removeSocketFromAllRooms, which already only returns rooms it actually removed. * fix(realtime): exclude the disconnecting socket from its farewell broadcast Greptile follow-up (transient-Redis-failure edge): if removeUserFromRoom fails on disconnect, the socket's presence entry can outlive it (room hashes have no TTL) and reappear as a ghost. Disconnect now broadcasts a correction to EVERY room the socket was in (union of the manager's removed rooms and the live Socket.IO membership) and passes the disconnecting socket id as excludeSocketId, so it is never shown as a collaborator regardless of whether the Redis delete succeeded. Any orphaned entry is still reclaimed by the next join's stale-presence sweep. broadcastPresenceUpdate gains an optional excludeSocketId; normal broadcasts are unchanged. +1 test. * fix(realtime): make presence broadcasts liveness-aware (root-cause ghost fix) Presence broadcasts now reconcile the stored list against the live Socket.IO membership (io.in(room).fetchSockets()) before emitting, via a shared filterVisiblePresence helper. This closes the residual behind the earlier disconnect fixes: an entry orphaned by a failed removal (room hashes have no TTL) could reappear in a LATER join's presence snapshot until the 75-min stale sweep. Now such an entry is never emitted, because a non-live socket is filtered out of every broadcast. Combined with excludeSocketId (which handles the disconnecting socket, still momentarily live). Fail-safe: on a fetchSockets throw or an empty result while entries remain, emit the unfiltered list rather than hide live collaborators. Also drops a dead guard in the disconnect union loop (rooms already removed are skipped by the wasInRooms check) and the now-unused isSameRoom import. +1 ghost-guard test. 122 realtime tests pass. * feat(files): live presence avatars + live file tree via realtime rooms (#5932) * refactor(tables): adopt shared durable event-log core (#5934) * fix(realtime): address post-merge review-comment findings (#5937) * fix(realtime): address post-merge review-comment findings A re-audit of every inline review comment on the merged stack surfaced real issues that the thread-resolutions and prior audits missed. Fixes: Presence server (#5930 comments): - connection.ts: snapshot `socket.rooms` SYNCHRONOUSLY before the first await. Socket.IO clears the room set once the synchronous part of a `disconnecting` handler returns, so reading it after `await removeSocketFromAllRooms` saw an empty set — the eviction fallback was dead. (Cursor: "Disconnect fallback misses live rooms".) - workflow-room-service: restore the original managers' final unconditional room-state wipe via a new `deleteRoom(room)` manager method, so a deleted workflow leaves no lingering presence/meta even if a per-socket removal failed or a socket joined mid-teardown. (Cursor: "Deletion skips final room wipe".) Files (#5932 comments): - workspace-file-manager.uploadWorkspaceFile now fans out the live-tree signal (all direct-upload paths: multipart fallback, copilot create, /api/files/upload, v1 files — the presigned path already notified). (Cursor: "Creates miss live tree fan-out".) - use-workspace-files-room: clear the pending retry timer on join success; and a module-scoped intended-room guard defers the unmount `leave` so a rapid remount re-claims the room and skips a stale leave — fixing presence flap + a leave-after-join race. (Cursor: "Retry timer survives join success" + "Remount churns files presence".) - workspace-files handler: roll back a partial join (leave room + remove presence) in the catch, mirroring the workflow join. (Cursor: "Join failure skips membership rollback".) +2 tests (deleteRoom). 127 realtime tests pass, both apps tsc clean, api-validation + boundaries green. * fix(files): scope workspace-files leave to a workspace (deferred-leave safety) Self-review of the deferred-leave guard found a real bug: leave-workspace-files was not workspace-scoped, so after a workspace switch (A->B) the deferred leave from A would evict the socket from its new room B. The leave now carries the workspaceId and the server no-ops if the socket's current files room differs. Also excludes the leaving socket from the leave broadcast (consistent with disconnect). * fix(realtime): close files-room presence leak + validate join payload Architecture-audit findings: - S1 (real Redis leak): the files room inherited the shared manager but not the workflow join's liveness sweep, so an UNGRACEFUL disconnect (pod crash — no `disconnecting` event) left its presence entry in the no-TTL room hash forever. Added a shared `sweepStalePresence(manager, room)` (fetchSockets liveness + remove not-live-AND-stale entries, matching the workflow 75min threshold) and run it on files join; also filter the join ack through `filterVisiblePresence` so a joiner never briefly sees an un-swept ghost. - S2: validate the client-supplied `workspaceId` on files join before it reaches the DB query (matches the /api/workspace-files-changed guard; fails closed). - N2: corrected the notify doc — it is awaited (guaranteed dispatch before a Node route returns) and hard-bounded to NOTIFY_TIMEOUT_MS, not "never block". +1 test (sweepStalePresence keeps live/fresh, reclaims not-live-stale). 128 realtime tests pass, both apps tsc clean, biome clean. * fix(realtime): workflow-deletion always notifies + cleans by socket.io membership Review-round findings on #5937: - Always emit `workflow-deleted` (was guarded by users.length>0), so a socket still in the Socket.IO room after a Redis presence eviction is told the workflow is gone before socketsLeave kicks it — the editor no longer keeps showing a deleted workflow. (Cursor: "Silent kick skips deletion event".) - Clean per-socket state for the UNION of live Socket.IO members and presence-tracked sockets, so an evicted/late-joined socket's room mapping + session are dropped too — not just presence-snapshot sockets. (Greptile: "Room deletion leaves reverse state".) - deleteRoom now logs AND rethrows on Redis failure (like addUserToRoom) so a failed wipe isn't reported as a clean deletion; the request surfaces it. (Greptile: "Room deletion failures are suppressed".) The two "deferred leave drops new membership" P1s were already fixed by the workspace-scoped leave in a prior commit (leave carries { workspaceId }; server no-ops on mismatch). 128 tests pass, tsc + biome clean. * refactor(files): drop module-scoped deferred-leave; rely on workspace-scoped leave Removes the one non-idiomatic construct (a module-level mutable `intendedFilesWorkspaceId` + queueMicrotask). It only guarded a same-workspace CONCURRENT remount, which doesn't occur in production (folder nav is shallow/no remount; list<->detail is sequential) — a dev-StrictMode-only case. The real cross-workspace race is already handled by the workspace-scoped leave: if B's join runs first (auto-leaving A), A's leave no-ops because the socket's current files room is B. Simpler, idiomatic, prod-correct. * feat(realtime): Yjs relay server for collaborative document editing [4/N] (#5941) Server-side Yjs relay for collaborative document editing (live carets + text selection) in the Files rich-markdown editor. Faithful y-websocket-style relay over the existing authenticated Socket.IO connection + shared room abstraction; in-memory Y.Doc + Awareness per file; awareness ownership binding, userId-keyed client-id uniqueness, seeder election with deadline re-election, concurrent-JOIN generation guard. 25 relay tests. Reviewed to Greptile 5/5 + Cursor pass across multiple rounds, plus an independent 4-lens audit (correctness/security/conventions/simplicity) and /simplify + /cleanup passes. * feat(files): collaborative document editing — client provider + editor (#5946) Client Yjs provider (FileDocProvider over the authenticated socket) + TipTap Collaboration/CollaborationCaret wiring for live carets + text-selection in the Files rich-markdown editor. Collaboration is a Files-page-only surface (explicit `collaborative` opt-in), disjoint from agent-streaming. Read-only + autosave-gated until synced+seeded. Merges into the realtime-rooms integration branch. * feat(tables): live collaboration — cell-selection presence + live mutation propagation (#5957) * feat(tables): live cell-selection presence — protocol + server + client hook The realtime spine for Google-Sheets-style table presence (mode A, socket): - @sim/realtime-protocol/table-presence: centralized wire protocol (events + TableCellSelection {anchor, focus, editing} + payloads) so server emits and client subscriptions can't drift. - ROOM_TYPES.TABLE + resolveTableWorkspace registered in ROOM_WORKSPACE_RESOLVERS (tableId -> workspace via userTableDefinitions, honoring archivedAt); roomName / presenceEventName / disconnect cleanup / authorizeRoom all derive automatically. - apps/realtime/src/handlers/tables.ts: join/leave (mirrors workspace-files) + a table-cell-selection relay (mirrors the workflow selection channel), broadcasting via roomName(room) since table rooms are namespaced. UserPresence gains a cell field threaded through the memory + Redis managers (Lua ARGV[7], null clears). - Extracted the duplicated resolveAvatarUrl into handlers/avatar.ts. - use-table-room.ts client hook: joins over the shared socket, tracks the roster (avatars) + patches per-socket cell deltas, exposes a throttled emitCellSelection. Grid UI (avatars + selection overlay) lands next; concurrent cell-value edits (last-write-wins via the durable log) are the follow-up PR. * feat(tables): render live cell-selection presence in the grid Wires the table presence room into the grid UI: - Page (table.tsx): useTableRoom (gated off in embedded/mothership mode) — renders <PresenceAvatars> in the header and passes remoteSelections + emitCellSelection down to the grid. - Grid emits its local selection: an effect resolves the index-based anchor/focus to stable (rowId, columnId) via refs and broadcasts it (with an editing flag for the active cell) through the throttled emitter. - RemoteSelectionOverlay: draws each remote viewer's selection in their color (getUserColor), a darker fill while editing, and name-on-hover — measured from live cell rects in the content wrapper's space (scrolls with the grid), hidden when rows are virtualized off-window, pointer-events-none so it never blocks cell clicks (hover via pointer hit-test). * test(tables): cover the table presence handler Mirrors workspace-files.test.ts: join auth/unavailable/denied/success, plus the cell-selection relay (asserts it persists via updateUserActivity and broadcasts on the namespaced roomName, not the bare id) and leave. * feat(tables): propagate manual cell edits live (last-write-wins) A manual row edit now appends a lightweight 'edit' event to the durable table stream; collaborators refetch the row (via the existing debounced rows-invalidate the job events use) so the winning value shows live. The event carries no value — peers refetch in their own wire format, so there's no auth-specific value translation on the wire, and last-write-wins falls out of the DB's committed order (the Google-Sheets model). Edits that also trigger a dispatch already emit dispatch/cell events; the debounce coalesces the two. * refactor(tables): apply /simplify findings - Drop the dead 'add unknown peer' upsert branch in use-table-room (Socket.IO ordering guarantees a peer is in the roster before their selection delta). - TableCellSelectionBroadcast = TablePresenceUser & { cell } (was a copy-paste). - Make TableGrid's presence props required + drop the unused empty-default/guard (only table.tsx mounts it, always passing both). - Drop the unused rowId from the 'edit' event (the handler invalidates all rows). - Overlay: subscribe scroll/resize/pointer listeners once per scroll element and cache the wrapper origin, so incoming deltas re-measure without re-subscribing and the pointer hit-test never forces a per-move layout read. - Server: cache the immutable socket session so a selection delta no longer reads it from Redis every time. * refactor(tables): apply /cleanup findings - Fix the remote-selection name label contrast: text-white is unreadable on the light-pastel user colors (same bug the Files caret fixed) → fixed dark #1a1a1a. - Re-measure via useLayoutEffect so a moving peer selection updates before paint (no one-frame position lag). - Drop 'mothership' from a comment (constitution copy rule). Six cleanup passes ran (effect, memo/callback, state, react-query, emcn, comment); the rest confirmed clean — all state/memos/callbacks/effects are load-bearing, presence correctly lives in useState (socket-pushed), and the edit→rows-invalidate granularity is right. * feat(tables): propagate every table mutation live (edit + schema signals) Comprehensive live collaboration for all user table mutations, via two value-less durable signals + named helpers (signalTableRowsChanged / signalTableSchemaChanged): - edit (rows refetch): single + batch row create, cell/row update, batch update, delete by id/filter, and upsert. - schema (definition + rows refetch): column add/update/delete, workflow-group add/update/delete, table rename, and CSV import (which can add columns). - Client handles 'schema' by invalidating the table detail (exact) + rows. Execution paths (column run, cancel-runs) and async jobs (delete/import-async, job-cancel) already propagate via cell/dispatch/job events — verified applyJob refetches on terminal. No reorder routes exist. Table archive (route DELETE) is a deliberate follow-up: it needs a table-deleted redirect event, not a refetch signal (which would 404). * refactor(tables): apply comprehensive /cleanup audit findings Holistic + react-query + comment audits over the whole PR: - Security/crash fix: a remote peer's rowId flowed unescaped into the overlay's querySelector — a hostile id ('x"]') threw SyntaxError inside a useLayoutEffect, crashing every other viewer's page. CSS.escape it, and validate + whitelist the untrusted cell payload server-side (shape + 200-char id bound) before it is stored/rebroadcast. - Simplify the CELL_SELECTION relay: the delta attached userId/userName/avatarUrl that the client discarded (identity comes from the roster). Drop them + the getUserSession lookup/cache entirely — the delta is now { socketId, cell }. - React Query: schema handler also invalidates lists() (parity with the local column-mutation set); document that the mutating client self-refetches by design. - Comment tightenings; biome fixed a stale import order in workspace-files.ts. * fix(tables): broadcast single-cell selections (focus falls back to anchor) Cursor High: a normal cell click leaves selectionFocus null (the grid treats it as a one-cell selection via focus ?? anchor), but the presence emit required BOTH anchor and focus to resolve — so the most common selection never broadcast and clicking even cleared a prior remote outline. Mirror the grid's focus ?? anchor semantics. * fix(tables): reviewer + regression + per-LOC audit findings Cursor review round (5 findings) + regression audit + per-LOC audit: - Presence roster snapshot now KEEPS the cell we already hold for a known socket, so a join/leave broadcast can't revert a fresher CELL_SELECTION delta. - Reset the selection throttle on table switch (was unmount-only), so a pending selection for table A can't flush into table B's room after a switch. - Metadata writes (column widths, display) use a new lightweight 'metadata' signal that refetches only the definition — a resize no longer forces peers to refetch rows. - Overlay re-measures on row add/remove/reorder via a tbody childList MutationObserver (a live refetch moves cells without a scroll/resize). - Document the actor self-refetch create caveat (scrolled multi-page insert) accurately. - isCellRef narrows to a partial instead of casting to the full type then re-checking; drop a redundant mount measure() (the layout effect covers it); text-[11px]→text-xs. * fix(tables): drop ineffective metadata propagation + re-measure overlay on column resize Cursor round on |
||
|
Bill LeoutsakosandBill Leoutsakos
|
ecf1d7dbbb |
refactor Instagram integration architecture (#6143)
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> |
||
|
Waleed
|
19b0312a04 |
feat(managed-agent): add session lifecycle operations (#6140)
* feat(managed-agent): add session lifecycle operations
Adds an operation selector to the Claude Managed Agents block, backed by
nine new tools alongside the existing run-session behavior:
- create session (non-blocking, seeds initial_events)
- send message to an existing session
- get session (surfaces tool calls awaiting approval)
- list events
- update session (title/metadata)
- interrupt session
- respond to tool confirmation (allow/deny)
- archive session
- delete session
Run Session stays the default, so blocks saved before the selector
existed keep their exact behavior and field layout.
* fix(managed-agent): address review findings on session lifecycle ops
- List Events kept the OLDEST slice when capped, dropping the agent's most
recent reply. Paging is exhaustive again and the cap now keeps the newest
N after ordering, with a `truncated` flag so callers know it is a tail.
- listPaginated returned whole pages past maxItems; it now trims to the
exact cap.
- A whitespace-only title passed the update guard and would have cleared an
existing session title. Blank is now treated as not provided.
- Interrupt had no request timeout and could hang; bounded at 15s while
still honoring the workflow signal.
- Custom-tool gates were surfaced by Get Session but could not be answered,
since they need user.custom_tool_result rather than a confirmation. Adds a
Respond To Custom Tool operation and a `kind` on each pending gate so a
workflow routes to the right one.
- Docs rendered a raw ${DEFAULT_EVENT_LIMIT} placeholder for the default.
* fix(managed-agent): correct truncation flag, gate lookup, custom tool result
- `truncated` was true whenever the history size equalled the limit, even
though nothing was dropped. Event reads now report the untrimmed total and
the flag compares against that.
- Pending-gate enrichment capped its read, which keeps the OLDEST events in
page order — the opposite of where blocking gates live. It now filters to
the ids being looked up as pages arrive, which is both correct regardless
of page order and bounded by the id count. Paging continues on the raw
page so a fully-filtered page is not mistaken for the end of the list.
- Respond To Custom Tool applied one result to every id, so multiple pending
tools would all receive the same output. It now answers a single call per
invocation.
* fix(managed-agent): stop fractional event limits reading unbounded
A limit below 1 passed the positivity check and then floored to 0, which
made `slice(-0)` hand back the ENTIRE history flagged as complete — the
opposite of the requested bound. The limit is now floored before it is
validated, so anything that does not resolve to a positive integer falls
back to the default.
Also hardened the library: a zero or negative cap short-circuits to an
empty result instead of falling through to `slice(-0)`, so no future
caller can hit the same trap.
* fix(managed-agent): stop gate lookup scanning the full tool history
The id filter keeps the collected array tiny, so `maxItems` never trips and
the walk continued to the end of a session's tool history even after every
blocking id had been found. `listPaginated` now takes a `stopWhen` predicate
and the gate lookup ends as soon as it has all the ids it came for.
Also makes a blocked-but-unnamed session observable: when a session reports
`requires_action` with no blocking event ids, `requiresAction` stays true —
reporting false would tell a workflow the session is fine while it is parked
indefinitely — and the dead end is logged and documented instead.
* fix(managed-agent): floor event cap and make metadata clearing explicit
- A `maxItems` between 0 and 1 slipped past the zero guard and became
`slice(-0)` — the whole history — because slice truncates its index toward
zero. The cap is now floored at the library boundary, so no caller can hit
it whatever they pass.
- Update Session documented full metadata replacement but could not express
a clear: an empty map normalizes to "absent". Inferring the clear from
emptiness would be worse, since an untouched table is also empty and would
wipe metadata on every title-only update. Adds an explicit `clearMetadata`
instead, and corrects the parameter's documentation.
* test(managed-agent): pin the HTTP shape of every session endpoint
Method, URL, and beta header for all 11 calls, plus the SSE accept header,
the separate memory-store beta (combining the two is a documented 400), and
content-type only on requests that carry a body. These are the details types
cannot catch and that break silently when a path is "tidied".
|
||
|
Justin Blumencranz
|
a5ca810cfe |
fix(chat): drop unparsable special-tag payloads instead of dumping raw JSON (#6142)
* fix(chat): drop unparsable special-tag payloads instead of dumping raw JSON * fix(chat): suppress broken payloads mid-stream; reserve marker rescan for mispaired quotes * improvement(chat): require key-value colon evidence before dropping an unparsable tag body |
||
|
Siddharth Ganesan
|
88f054e185 | fix(auto-model): fix auto model (#6144) | ||
|
Theodore Li
|
4ef7bbad53 | fix(db): index large-value workflow_id FKs so workflow deletes don't seq-scan (#6136) | ||
|
Vikhyath Mondreti
|
b77b9ee491 | improvement(workspace): remove tip + simplify modal header text (#6141) | ||
|
Theodore LiandClaude Opus 5
|
c5cc6ce26c |
feat(chat): hide the Chat module when NEXT_PUBLIC_CHAT_DISABLED is set (#6137)
* feat(chat): hide the Chat module when CHAT_ENABLED is unset A self-hosted deployment that skipped the chat key still rendered the full mothership Chat UI, landing on the composer and 401ing on every message. Gate it behind a CHAT_ENABLED / NEXT_PUBLIC_CHAT_ENABLED twin, written by the setup wizard alongside COPILOT_API_KEY and validated by the existing FLAG_TWINS doctor check. The flag resolves at module scope on both render passes, so no chat surface renders then disappears. With Chat off the workspace lands on its first workflow (resolved server-side, behind the cached host-context check so no workflow id leaks to non-members), and the chats list, scheduled tasks, editor Chat panel, and chat CTAs are absent. Routes are gated rather than deleted: /home redirects because it is baked into delivered invitation emails and the accept contract. Also fixes two bugs the gate exposed: a persisted activeTab of 'copilot' left the workflow panel blank from first paint, and the panel's handoff listener claimed MOTHERSHIP_SEND_MESSAGE events outside its own gate, silently swallowing "Fix in Chat" messages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * refactor(chat): gate the UI on NEXT_PUBLIC_CHAT_DISABLED, not an opt-in flag CHAT_ENABLED made Chat opt-in, so every existing deployment that already had COPILOT_API_KEY would have lost the module until it set a new variable. Invert to an opt-out so nothing changes for them. That also collapses the twin. The only reason the flag needed a server/client pair was that it projected a secret; NEXT_PUBLIC_CHAT_DISABLED is not one, so getEnv resolves the same value from process.env on the server and window.__ENV in the browser. Gone with it: the FLAG_TWINS entry and its doctor sync check, the two-variable wizard write, and the boot-time throw, whose contradiction (flag on, key absent) can no longer be expressed. Presentation and capability are now separate concerns. NEXT_PUBLIC_CHAT_DISABLED decides whether the surfaces render; COPILOT_API_KEY decides whether the work can run, and gates the paths that need it — the Sim Chat block, prompt-job claims, and inbox access — each failing on its own terms. The wizard writes the opt-out when you skip the chat key, which is the case this started from: a fresh self-host that never configured Chat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * feat(setup): prompt for the chat key in k8s mode The dev and compose flows minted a chat key and wrote the Chat opt-out alongside it; k8s did neither, so a cluster install with no COPILOT_API_KEY in its Helm values rendered a Chat module that rejects every message. Prompt with the same flow and feed both values into `app.env`, which the chart already renders as arbitrary container env. Reading the previous release's key matters here in a way it does not for the file-based modes: `helm upgrade` without `--reuse-values` keeps only what this document carries, so a key the user elects to keep has to be re-supplied or it is silently dropped. Splits the release-values read from the secret-reuse check so both the key and the secrets come from one `helm get values` call, and carries the mothership override across for the same mint-here-validate-there reason the other modes document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * fix(setup): write app-behavior flags to every env file the app can start from The wizard wrote the Chat opt-out only to the env file its own mode owns, so choosing compose put it in the root `.env` while `bun run dev` reads `apps/sim/.env` and never saw it. Skipping the chat key appeared to do nothing. Mirror values that change how the app behaves — as opposed to where it connects — across both targets. Connection settings deliberately do not go through this: DATABASE_URL and friends differ between the compose stack and a local dev run, which is why this takes an explicit set of values rather than the whole batch. The mirrored file is written even when absent, since missing is exactly the case that stranded the flag, but with seeding suppressed so a compose run leaves a one-line apps/sim/.env instead of a full .env.example for a stack the user is not running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * fix(compose): forward NEXT_PUBLIC_CHAT_DISABLED to the app container The wizard wrote the flag into the root .env, but compose only passes through variables the service's `environment` block names — and that block listed COPILOT_API_KEY without its companion. Skipping the chat key on a Docker install therefore did nothing: the value sat in .env and never reached the container. Add the passthrough to all four compose files. Reverts the previous commit's mirroring into apps/sim/.env, which treated the symptom — each mode writes only the env file it owns, and that file is now wired correctly. k8s needs no equivalent: its values flow into `app.env`, which the chart renders key by key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * fix(chat): resolve the landing route without blocking on the database Server-resolving the first workflow meant a session lookup, an access check and a query had to finish before anything rendered. A slow or unreachable database left the user on a blank page under a populated sidebar — worse than the instant redirect it replaced, and with no signal that anything was wrong. Redirect straight to `/w` instead and let it pick from the workflow list the layout already prefetches, so the choice costs no round trip and cannot hang. Repoints the sidebar's primary action rather than hiding it: the slot that offered "New chat" now offers "New workflow" and creates one, since with Chat off there is no composer to open but the intent is the same. Sends the CLI key handoff to signup rather than login. It is reached from a terminal — usually the setup wizard standing up a fresh self-host — where the visitor has no account yet. Both auth pages cross-link carrying the callback, so a returning user is one click from login with their destination intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * improvement(chat): address cleanup-pass findings on the Chat gate Effects: the panel's auto-select effect read the copilot chat list while the list query was deliberately skipped, took "empty" for "deleted in another tab", and cleared the user's selection — latching a ref that stopped it ever being restored. Guarded on the same condition as the handoff listener. Memo: `/w` filtered workflows through a useMemo whose array dependency was a fresh `[]` on every render while the query had no data — the exact window the page exists for — so it memoized nothing and re-fired the redirect effect. Keyed on the workflow id instead. Same unstable-default problem on the sidebar's chat list, where it invalidated five downstream memos; given a stable empty constant. Callback: `handleCreateWorkflow` listed the whole mutation object in its deps, which TanStack recreates every render. Harmless until this branch wired it into the top nav, where it defeated `memo(SidebarNavItem)`. React Query: Recently Deleted still fetched archived chats unconditionally and offered restores into routes that now 404. Also surfaces an error state on `/w` — it is the landing route now, so a failed list fetch would otherwise spin forever behind a log line — fixes a spinner using a token undefined in dark mode, and trims comments that restated code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * fix(chat): gate workflow creation on write access, pin the key in schedule tests The zero-workflow landing offered "Create workflow" to every member. Creation navigates optimistically, so a read-only member was sent to a workflow the server had already refused to create, with the failure never surfaced. Gate both entry points — the empty state and the sidebar's "New workflow" row — on the same `canEdit` check the rest of the sidebar uses, and tell read-only members who can make one instead of offering an action that cannot succeed. The schedule-execution tests only passed locally because vitest loads the developer's own `.env`, which supplied COPILOT_API_KEY; CI has none, so the prompt-job claim guard skipped the claims those cases assert on. Pin the key through the env mock so the suite states its own preconditions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha * fix(setup): name both variables in the chat-key failure hint The caller writes the Chat opt-out whenever the prompt returns no key, so the hint's "or set COPILOT_API_KEY yourself" restored capability while leaving the module hidden — the one path where following setup's own advice does not work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ErcRgvi7VQBeKDQ3MBMha --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Waleed
|
0bcf64a505 |
perf(bundle): stop three routes shipping the block and tool registries (#6138)
The public /integrations page downloaded, parsed and executed 19.54 MB of JavaScript, 15.52 MB of which was the tool registry — 4,351 tool configs, to render a catalog of names and icons. It arrived as `<script async>` in the initial HTML, so the browser fetched and ran it on load. Measured on a production build, chunks referenced by the page's own HTML: /integrations 19.54 MB -> 3.55 MB across 41 chunks (-82%) No chunk over 1 MB remains on that route. credit-usage also loses its copy. Three separate import edges, all the same shape: a module mixing pure helpers with registry-backed ones, so importing the pure half dragged in all 282 block configs and the tool registry behind them. - `lib/integrations/index.ts` built POPULAR_WORKFLOWS at module scope via `getAllBlockMeta()` and re-exported registry functions. The landing grid is a client component, so importing anything from that barrel inherited the whole graph. POPULAR_WORKFLOWS moves to its own module — its one real consumer is a server page — and the re-export goes, which .claude/rules/sim-imports.md already forbids and which nothing imported through the barrel. - `blocks/icon-color.ts` mixed pure contrast maths (`getTileIconColorClass`, `isLightTileColor`, used by the landing page) with `getBareIconStyle`, which reads `getAllBlocks()`. Split by dependency into `blocks/brand-icon-style.ts`; all six of its consumers are under app/workspace/** where the registry is already legitimately present. - `credit-usage-view.tsx` imported a date formatter from the logs feature's `utils.ts`, which also exports registry-backed badge components. `formatDateShort` has three consumers across three features, so it moves to `lib/core/utils/date-display.ts` per the repo's utils-location rule. Not addressed: account/settings/[section] still reaches the registry through workspace-permissions-provider -> socket-provider, one of the four known client edges from the module-graph audit rather than a stray import. It is also loaded via `dynamic()`, so it is likely lazy rather than on first paint. That belongs with the registry split, not here. workspace/* keeps its copy, which is correct — the editor needs tool params and outputs. |
||
|
Bill LeoutsakosandBill Leoutsakos
|
c0e7ea9421 |
fix(pi): surface provider errors from agent events (#6133)
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> |
||
|
Theodore Li
|
648dd345fa |
feat(notifications): email on schedule auto-disable and 100% usage limit (#6038)
* feat(notifications): email on schedule auto-disable and 100% usage limit * fix(notifications): keep partial recipients when one lookup fails |
||
|
Vikhyath Mondreti
|
422b4919f1 | chore(trigger): upgrade version (#6127) | ||
|
Theodore Li
|
b4f027eece |
fix(billing): point self-hosted upgrade CTAs at the hosted app (#6003)
* fix(billing): point self-hosted upgrade CTAs at the hosted app Self-hosted Chat bills against the sim.ai account behind COPILOT_API_KEY, but its 402 upgrade card linked to local billing settings that a self-hosted deployment does not have. Point those CTAs at the hosted app instead, and drop the local workspace-role gate that could hide the CTA from the only person able to act on it. Adds /upgrade, an account-scoped entry for callers that cannot know a workspace id. It delegates to /workspace?redirect=upgrade rather than re-deriving workspace resolution, inheriting local recency, stale-session recovery, and the no-workspace creation policy. * fix(billing): keep the upgrade intent through workspace creation A first-time visitor to /upgrade has no workspace to resolve, so /workspace creates one — and then hardcoded a redirect to home, silently dropping the upgrade intent. Route both exits through one destination helper so the created workspace lands on the plan picker with its reason intact. |
||
|
Waleed
|
e98715da01 |
fix(realtime): noindex the socket server's 404 responses (#6129)
* fix(realtime): noindex the socket server's 404 responses The sockets.* hostnames are served by this server and return a plain JSON 404, which Google Search Console reports as crawl errors. Mark unmatched routes noindex so crawlers drop the hostnames instead of retrying them. * fix(realtime): noindex every response, not just the 404 Setting the header only on the 404 fallback covered the one response that crawlers already drop on status code alone, while /health — the sole route returning 200 with a body, and so the only indexable surface on the socket hostnames — stayed uncovered, with a test pinning it that way. Set it once on the handler instead. Node merges setHeader values into writeHead and no branch sets X-Robots-Tag, so it reaches every response. |
||
|
Bill LeoutsakosandBill Leoutsakos
|
c0b19da782 |
feat(pi): install Bun in cloud sandboxes (#6123)
Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> |
||
|
Waleed
|
5c29d833af |
improvement(ui): drop full-content hover tooltips (#6128)
* improvement(ui): drop full-content hover tooltips - remove the chunk-content tooltip in the KB chunks table (it forced the full chunk body on every truncated row) - remove the sub-block value tooltip on collapsed workflow blocks (whole prompts/code/JSON on hover) - replace the native `title` on subflow and note blocks with the clip-gated OverflowSpan - clip-gate the KB documents tags cell so it stops firing on fully visible tags - drop the dead tooltip on the resource header root title, which can never truncate * fix(workflow-renderer): default the optional note name for OverflowSpan |
||
|
Waleed
|
ed23330f88 |
feat(knowledge): opt-in hybrid lexical + vector retrieval for KB search (#6124)
* feat(knowledge): hybrid lexical + vector retrieval for KB search KB search ranked purely on pgvector cosine distance, which retrieves exact tokens (error codes, ticket keys, identifiers, rare product names) poorly. Add a full-text leg over the already-present generated `embedding.content_tsv` column and its GIN index — no migration, no re-indexing — and fuse it with the vector leg by reciprocal rank. Both legs run concurrently and share the same visibility and tag-filter predicates; the lexical leg is best-effort and falls back to vector-only on failure. Hybrid is the default for every caller. `searchMode: 'vector'` on the internal and v1 contracts (and an advanced Retrieval Mode dropdown on the Knowledge block) restores the previous behavior. Both search routes now share one `executeKnowledgeSearch` dispatch instead of duplicating the three-branch retrieval logic. * change(knowledge): make vector the default search mode, hybrid opt-in Every existing caller — workflow block, v1 API, copilot, guardrail RAG — keeps its current ranking. Hybrid retrieval is now requested explicitly via `searchMode: 'hybrid'`. Also routes the copilot knowledge tool through the shared `executeKnowledgeSearch` dispatch so all four callers share one retrieval path, and documents `searchMode` on the public v1 search endpoint in the OpenAPI spec. * docs(knowledge): document the hybrid retrieval mode Regenerates the knowledge integration reference for the new searchMode tool param, and adds a Retrieval Mode section to the knowledge base workflow guide explaining when hybrid beats vector-only. * fix(knowledge): stop rank fusion from starving the lexical leg Rank n in one leg always ties rank n in the other, so ordering the fused list by score alone let whichever leg was scored first take every tied slot. At topK=1 that meant a hybrid search returned exactly the vector-only result and discarded the exact keyword match the mode exists to recover. Selection now orders by score and drains each tie group round-robin, taking from whichever leg has contributed fewest rows so far. The lexical leg is passed first so it wins a total tie, since a chunk the vector leg ranked below its distance threshold is the case hybrid was opted into for. * fix(knowledge): credit a shared hit to every leg that returned it Attributing a row found by both legs to a single leg left the round-robin owing the other leg a slot it had already been served. With a shared rank-1 hit and topK 2, that evicted the lexical-only row — the exact match hybrid was enabled to recover — in favor of the vector-only one. A shared row satisfied every leg that returned it, so every one of them is now charged for it. Tie-breaking prefers the candidate whose least-served leg has been served least, which also removes the arbitrary best-rank attribution. * fix(knowledge): reject a whitespace-only copilot query explicitly The shared dispatch treats a whitespace-only query as absent and throws when no tag filters accompany it, where the previous vector-only call would have embedded the blank string and searched. Tighten the existing guard so the tool returns its normal message instead. * fix(knowledge): fan the keyword leg out per knowledge base The vector leg caps candidates per base once getQueryStrategy sets useParallel, but the keyword leg always ran one global query with a single LIMIT. Searching several bases at once let whichever one ranks strongest lexically consume every slot, so an exact-token hit in a smaller base never reached fusion — the case hybrid exists to serve. The keyword leg now uses the same strategy: per-base queries under the same parallel limit, re-ranked globally on a selected ts_rank_cd. Both legs draw candidates the same way, so fusion combines rankings over the same pool. * perf(knowledge): stop the keyword leg detoasting every match's vector Selecting the cosine distance in the ranking query made Postgres detoast the 1536-dimension embedding and compute a distance for every full-text match before the LIMIT applied, so cost tracked how common the query term was rather than topK. On a 20k-chunk base with a term matching every row that was 61,055 buffer hits against 1,030 for the same query without the projection. Rank on ids and ts_rank_cd alone, then hydrate only the rows that survive the limit. Same results, and the worst case drops to ~27ms end to end. |
||
|
Waleed
|
898a10d2d2 |
improvement(files): match CSV/XLSX preview tables to the markdown table chrome (#6125)
* improvement(files): match CSV/XLSX preview tables to the markdown table chrome Tables in the file viewer looked different depending on the file: CSV and XLSX previews rendered their own chrome (rounded outer frame, --surface-2 header, 13px body / 12px header, --text-secondary cells) while markdown files rendered tables through the rich markdown editor (full cell borders on --divider, --surface-4 header, 14px text). Extract the markdown table chrome into document-table.css and style both surfaces from it. The editor stylesheet keeps only its own concerns (fixed layout for column resizing, prose block margin, cell paragraph reset); DataTable keeps only its edit affordances. * fix(files): wrap unbreakable cell values in preview tables like markdown does The markdown prose root sets overflow-wrap: anywhere; the preview root did not, so with whitespace-nowrap gone a long URL or hash in a CSV cell would overflow instead of breaking. |
||
|
Waleed
|
48aeac218c |
fix(uploads): set Content-Type once on presigned PUTs; document x-goog-meta-folderid for GCS CORS (#6121)
* fix(uploads): set Content-Type once on presigned PUTs; document x-goog-meta-folderid in the GCS CORS example XMLHttpRequest.setRequestHeader appends on repeated calls (values join with a comma), and GCS is the only provider whose signed uploadHeaders include Content-Type — so single-shot GCS uploads sent 'x, x', which fails V4 signature verification with 403 (headers canonicalize to a comma-separated value that must match what was signed; multipart part PUTs are unaffected since part URLs don't sign Content-Type). The client now sets its default Content-Type only when the server's signed headers don't already carry one, with regression tests for both paths. Also adds x-goog-meta-folderid to the documented GCS CORS responseHeader list — workspace uploads now sign a folderId metadata header, and GCS CORS matches preflight request headers against that list exactly (no wildcards), so the missing entry blocked browser uploads into folders. * chore(uploads): drop inline comment |
||
|
Vikhyath Mondreti
|
413784eceb | improvement(function): secrets access dropdown (#6118) | ||
|
Vikhyath MondretiandClaude
|
04a8c0ac8f |
improvement(admin): update defaults for better UX (#6112)
* improvement(admin): uupdate defaults for better UX * refactor: address review nits on the admin/invitation lock work Correct the attach lock-order comment. The order matches admin move, and what makes it mandatory is invitation acceptance: it holds `workspace-invitations:<id>` while waiting for the workspace row, so row-locking first (as this did) deadlocks against it. The previous ownership-transfer justification did not hold — that path takes the organization lock before its workspace rows too, so the two agree on order rather than inverting. Drop `cancelInvitation`. The `revokeInvitationAsAdmin` extraction left it with no callers, and an unlocked, unauthorized `status = 'cancelled'` flip sitting next to the fenced replacement is easy to reach for by mistake. Drop the unused `executor` parameters from `hasWorkspaceAdminAccess` and `isOrganizationAdminOrOwner`. No caller threads a transaction through either, and the former goes back to delegating to `checkWorkspaceAccess` instead of re-deriving the same permission itself. Import `chunkArray` from `@sim/utils/helpers` everywhere and remove the re-export from `batch-delete.ts`, so the symbol has one source rather than a non-barrel shim plus the package. Restore the bounded attachability check in `addDashboardOrganizationMember`: scope the query to the selected ids instead of listing every attachable workspace and scanning that array per selection. Resolve the credential-creation permission through `getEffectiveWorkspacePermission` rather than a second copy of the org-admin derivation ladder, so the rule cannot drift from the shared resolver. Co-Authored-By: Claude <noreply@anthropic.com> * refactor(invitations): drop dead code left by the revocation extraction `revokeInvitationWorkspaceGrant` lost its only caller when the DELETE route moved to `revokeInvitationAsAdmin`, leaving a locked wrapper nothing invoked. Remove it and fold its documentation into `revokeInvitationWorkspaceGrantTx`, which direct grants and scoped revocation still call. The grant-revocation test now drives the transactional form directly, so the sibling-grant and final-grant-cancels behaviour it covers stays under test. `isSameOrgMember` has had no caller since before this branch — direct grant resolves membership through `getUserOrganization` inside its own transaction — so it and its tests go too. `getWorkspaceMembership` is no longer imported outside its module now that credential creation reads `getCredentialCreationWorkspaceContext`; make it module-private rather than leave it on the public surface. Co-Authored-By: Claude <noreply@anthropic.com> * refactor: remove four uncalled billing and large-value helpers Each was checked by hand across every file type, including barrel re-exports and string references, rather than taken from a static analyzer. `isUserMemberOfOrganization` has no reference anywhere. `reapplyPaidOrgJoinBillingForExistingMember` only ever ran from two lock-ordering tests. The transaction-enlisted form it delegated to is what the subscription webhooks call and what those tests actually assert on, so they now drive it directly. The assertions are unchanged: the wrapper contributed a transaction, an organization lock and a membership existence check, none of which appear in the recorded operations. `replaceLargeValueReferences` and `replaceLargeValueReferencesWithClient` are both thin wrappers over `replaceLargeValueReferenceKeysWithClient`, which execution logging, human-in-the-loop resume and the trace backfill all still call. The single test covering a wrapper now composes the key collection itself and targets that live helper. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Waleed
|
c553de14e8 |
perf(tests): mock the tool registry globally, drop the dead deps optimizer (#6117)
* perf(tests): mock the tool registry globally, drop the dead deps optimizer
The test suite spent far more time importing modules than running them: on
the full suite, `import` was 1,399s aggregate against 88s of actual tests.
Per-file import cost showed exactly where it came from — lib/core, which
touches no registry, runs at 0.09s/file, while every area that reaches the
tool registry runs 12x-79x that (blocks 7.12s/file, providers 3.65, executor
3.07, tools 2.05, app/api 1.05).
The tool registry is 4,351 entries pulling ~5,907 modules, and almost nothing
under test needs the real thing. `@/blocks/registry` was already globally
mocked for this reason; this does the same for `@/tools/registry`.
Full suite, same commit, same machine:
baseline Duration 166.21s (transform 141.80s, import 1399.17s)
after Duration 91.56s (transform 61.98s, import 617.08s)
45% faster, import -56%, transform -56%. Identical results either way:
1252 files / 16873 tests pass, plus one failure that reproduces on unmodified
staging (cloud-review-tools.test.ts cannot find `rg` from its spawned python3
locally; CI installs ripgrep and it passes there).
Four test files genuinely assert tool registration or tool params, so they
opt out with `vi.unmock('@/tools/registry')` rather than being weakened or
deleted — outlook, azure_devops, and the two search-replace suites. No
coverage is lost.
Also removes `deps.optimizer.web`, which was dead config: it only applies to
client environments (jsdom/happy-dom) and 985 of 1,219 files declare
`@vitest-environment node`. Measured both ways to be sure — removing it is a
no-op (19.33s -> 19.23s), and switching it to the correct `ssr` side with an
include list for the heavy provider SDKs was also a no-op (19.19s). The cost
is first-party module graph, which the optimizer does not touch, so the
honest move is to delete it rather than leave config that reads as if it
does something.
Adds the missing `getBlockRegistry` accessor to the existing
`@/blocks/registry` mock. #6083 renamed that export and the mock was never
updated, so any test reaching those three consumers would have hit
"getBlockRegistry is not a function". Nothing exercises them today.
Not done, deliberately: `isolate: false` is ~20% faster but leaks state
between files and broke two doc-servable tests on the first run.
* docs(tests): explain why the search-replace registry opt-outs are load-bearing
Review read the missing direct import of @/tools/registry as evidence the
vi.unmock calls were no-ops. They are not: the dependency is transitive —
the search-replace planner resolves tool input params through real subblock
configs — and removing both opt-outs fails 8 tests across the two suites.
Comment now says that, so the next reader does not delete them.
|
||
|
Vikhyath Mondreti
|
e443a97ffb |
improvement(security): isolated-vm env construction (v8s escape case) (#6116)
* improvement(security): isolated-vm env construction (v8s escape case) * upgrade main node to v24 |
||
|
|
632041ce43 |
feat(library): What Is an AI Agent? Definition, How It Works, and Examples (#6115)
* feat(library): What Is an AI Agent? Definition, How It Works, and Examples * chore(library): generate cover for the AI agent post --------- Co-authored-by: Sim Pi Agent <pi@sim.ai> Co-authored-by: Waleed Latif <walif6@gmail.com> |
||
|
Waleed
|
e79bf81ebe |
fix(tables): refuse scale suffixes and resolve the lone-separator ambiguity (#6111)
* fix(tables): refuse scale suffixes and resolve the lone-separator ambiguity A verification pass over the previous commit found that my own fix opened a new hole of the class it closed. Widening the currency marker to 1-3 letters made scale suffixes parse: `1.2 M` read as 1.2, so a column of `1.2 M` / `3.4 M` — an ordinary spreadsheet paste — converted cleanly and rewrote every cell a millionfold too small. Before the widening those were rejected and the data was safe. Now refused explicitly, while `kr` and `zł` still parse despite starting with the same letters. Stripping the marker also newly routed formatted zero-decimal amounts into the lone-separator branch, where a single dot was always decimal: `1.235 ¥` read as 1.235 rather than 1235. That is a wrong number where there used to be a refusal, which is the worse failure. A lone separator followed by three digits is now resolved by two signals — a marker means a formatter produced it, and formatters group; a currency carrying three decimals (KWD, TND) reads them as decimals. `coerce` passes the column's code, so the parser can ask. Bare typed input keeps the decimal reading. Also: the unchanged-type throw fell through to a 500 for every type except currency, whose message happened to contain the word; it now maps to 400. And the last three comments describing the removed two-transaction architecture are gone — the previous commit claimed they were and two survived. Documented, and failing closed rather than guessing: markers written flush against the digits (`Rp12,00`) stay rejected, because a letter touching a digit is the only thing distinguishing a currency marker from a part number, and reading `SKU400` as 400 invents a value where refusing merely inconveniences. * fix(tables): fit the whole column-type list in the New column dropdown Adding currency made the list nine items (295px with its separator and padding), past the shared dropdown's 240px cap — so Select and Workflow sat below a scrollbar. Sized on this dropdown rather than on the shared `DropdownMenuContent`, which every other menu in the app relies on. Also gave the sidebar's type combobox headroom, since it gained the same entry and was within 4px of its own cap. * fix(tables): read a lone separator against the column's currency Review of the previous commit found the marker signal alone was too coarse. The evidence: a currency with decimal places ALWAYS formats with both separators — `1.234,56 €`, `$1,234.56`, `1.234,000 KWD`. Only a zero-decimal currency emits a lone one (`1.235 ¥`). So a marker does not imply grouping; the currency's own decimal places decide, and a three-decimal currency's trailing three digits are decimals however the value arrived. That matters most for CSV import, which passed no currency at all: a KWD column importing `0,500` read as five hundred rather than a half. The column's code is now forwarded, since nothing in a CSV carries a marker to fall back on. A bare typed `1.234` still reads as decimals. * fix(tables): let the separator decide a lone-separator amount, not the marker Greptile blocked on `$1.234` reading as 1234, and offered the alternative of stating the input contract explicitly. Looking for a precedent settled it against me: a spreadsheet with a USD-formatted cell stores a typed `1.234` as 1.234 and displays $1.23. Reinterpreting it as one thousand two hundred thirty-four is a thousandfold surprise, and my defense of it was wrong. The rule now keys on the separator, tempered by what the currency can express: - A dot is the decimal point in the notation most people type, so it stays a decimal — except for a currency with no decimal places, where `1.235 ¥` cannot be a fraction of a yen and is the one lone-separator form a formatter actually emits. - A comma is grouping by convention (`1,500` is fifteen hundred), except for a three-decimal currency, where `0,500` is a half. A currency with one or two decimal places always formats with both separators, so no formatter output is decided here at all. This also drops the `hadMarker` signal entirely — once the separator and the currency answer the question, where the string came from stops mattering. * fix(tables): check conversions against the target column, not a rebuilt stub Bugbot flagged that the conversion gate calls `isValueCompatibleWithType`, which hand-builds a column from loose arguments and never sets `currencyCode` — while the coercion a few lines later reads the real one. The reported impact does not occur. Whether an amount parses does not depend on the currency, only which number it yields, so the gate and the coercion always agree on accept/reject; and the value written back is already coerced against the real column, so no cell is stored wrong. I checked this rather than argued it: across nine currency codes and every separator shape in the parser's repertoire, acceptance never diverged. The stub is still worth removing. It is correct only because of an invariant nobody wrote down, and `convertedColumn` — sitting right there, whose own comment already says it exists so the scan reads the same option set and currency — is what the gate should have used all along. So the gate now takes the target column itself. The loose-argument form moves into the test file as local marshalling, leaving production with only the whole-column shape, which cannot silently drop a key a future type adds. No behavior change, so no test changes: a currency-divergence regression test could not fail today, and pinning an invariant this removes the need for would only block a legitimate future change. * fix(tables): vet a currency marker against an allowlist, not a scale denylist Bugbot found that `SCALE_SUFFIX` misses `mio`, `mrd`, `bio`, `tsd`, `mln` and `md`. Each is one to three letters, so each matched the currency-marker pattern and was stripped: `1,2 mio` read as 1.2, a millionfold too small, which is the same failure `1.2 M` had. Adding the six words would leave the next six. The denylist is the defect — it has to enumerate every magnitude abbreviation in every language, and each one it misses silently rescales a value rather than refusing it. So the check is inverted. A bare letter token is stripped only when it names a currency: an ISO 4217 code the runtime can enumerate, or one of the common non-ISO markers people type (`kr`, `zł`, `Kč`). Anything else is left in place, fails the amount-shape check, and is refused. `12 units` and `12 pcs` now fall out of the same rule the magnitude words do, without naming either. A marker carrying a currency symbol skips the check — no magnitude abbreviation contains one — so `R$`, `CHF`, `1 234,56 kr` are untouched. This rests on ISO codes and magnitude words being disjoint, which is true for all 162 codes the runtime knows, and is now pinned by a test so a future collision fails loudly instead of silently. `SCALE_SUFFIX` survives only for a runtime without `Intl.supportedValuesOf`, which cannot enumerate codes and so keeps the old permissive behaviour; rejecting every letter marker there would break `USD 12.50`. It no longer has to be complete, since every modern runtime rejects by non-recognition. Verified the new tests fail when the vetting is reverted. * fix(tables): group a lone dot only when a zero-decimal amount came formatted Greptile found that a bare `1.235` typed into a JPY column stored 1235. The zero-decimal branch read the dot as grouping without asking where the value came from, so typing, pasting, a tool write, or an import all inflated a thousandfold in silence. The rule was inverted relative to its own evidence. Grouping was justified as "the one lone-separator form a formatter emits" — and a formatter always emits its marker too. The marker is exactly what separates formatter output from someone typing, and I had removed it as redundant two commits ago. So a lone dot groups only for a zero-decimal currency AND only when a marker came with it. `1.235 ¥` and `JPY 1.235` still read as 1235; a bare `1.235` now stores 1.235 and displays `¥1` — wrong in a way the writer can see and correct, rather than a silent thousandfold error. This does not walk back the earlier fix. For a currency with one or two decimal places a formatter always emits BOTH separators, so a lone dot there never came from one and stays a decimal whether or not a marker is present — `$1.234` is still 1.234. The marker only carries information in the zero-decimal case, which is the only case that now consults it. The two overlapping lone-separator tests are replaced by three that each state one rule. Verified the new one fails when the marker requirement is reverted. |
||
|
Theodore Li
|
ef007507e7 | fix(demo): exempt landing routes from COEP so the Cal.com booker loads on soft nav (#6110) | ||
|
Waleed
|
9e9dc54d7e |
fix(emcn): stop the tab strip scrolling vertically by a pixel (#6113)
* fix(emcn): stop the tab strip scrolling vertically by a pixel The active tab extends one pixel past the strip so it covers the bottom border and reads as joined to the panel below. That pixel came from `-mb-px` on the tab itself, which meant it overflowed the row containing it — and that row is a scroll container, because `overflow-x: auto` computes a visible `overflow-y` to `auto` as well. The strip was therefore scrollable on the y axis by exactly one pixel, which is enough for a trackpad to nudge the tabs out of view. Moving the overlap onto the row keeps the tabs flush inside it, so the scroll container has nothing to scroll, while the row itself still hangs the pixel over the border. The strip is unchanged visually. Measured in a real renderer with the component's own classes: the row was scrollHeight 30 against clientHeight 29, and is now 30 against 30. A first attempt to reproduce it with hand-written inline styles showed no overflow at all — the real class output was needed to see it. Shared by the browser and terminal panels, so both stop scrolling. * test(emcn): pin the invariant, not the class placement Review pointed out the guard read source strings rather than rendered geometry, so it could pass while the overflow returned. Correct, and worth being precise about the compromise: this package's vitest runs in `node` with no browser mode, and the repo's only Playwright harness drives Electron against static HTML fixtures, so nothing here can lay out a React tree against the compiled CSS. Adding that capability is real infrastructure, not a line in this fix. What the guard can do is pin the invariant instead of two particular class strings: no negative bottom margin anywhere inside the scrolling row. That now catches the regression wherever a descendant reintroduces it, rather than only on the tab button it came from — verified against both shapes. The rendered behaviour stays measured rather than asserted, and the numbers are recorded next to the guard. |
||
|
Theodore Li
|
7293b67a54 |
feat(custom-blocks): log and bill child runs in the publisher's workspace (#6023)
* feat(custom-blocks): log and bill child runs in the publisher's workspace * fix(custom-blocks): sanitize every boundary failure and classify it for consumers * fix(custom-blocks): surface a cancelled child as cancelled, not a generic failure * fix(custom-blocks): share one large-value id list so nested blocks propagate * fix(custom-blocks): add a durable cancel backstop to the child bridge * fix(tools): carry Sim's own status through the tool-response boundary * fix(custom-blocks): stop forwarding the publisher's personal quota to consumers * fix(custom-blocks): correlate agent-tool runs to the real invoking execution * fix(custom-blocks): plumb the invoking execution id and abort signal to agent tools * fix(agent): forward the execution id through the provider payload * fix(executor): stop adopting an upstream target's HTTP status as our own * fix(custom-blocks): drain child log finalization when the parent is cancelled * fix(custom-blocks): require curated outputs instead of exposing the whole result * fix(custom-blocks): track the whole child run, not just its finalization |
||
|
Waleed
|
64bcfead34 |
fix(desktop): clear the traffic lights on every full-viewport surface, and enumerate them in CI (#6109)
* fix(desktop): clear the traffic lights on every auth-shell surface
Only /login reserved the macOS traffic-light lane, so signup drew its logo
underneath the lights — and so did reset-password, sso, verify, the CLI auth
handoff, and the invite pages. The pre-paint script marks the lane on every
desktop route, so any surface that did not reserve it overlapped.
Ownership moves to `AuthShell`, which is the single source of truth for the frame
all of these wear, and it now reserves unconditionally. Per-route gating was the
wrong shape rather than merely incomplete: `/invite/[id]` is a dynamic segment, so
no route list could have covered it. `supportsDesktopTitleBar` therefore drops its
pathname argument — the caller mounting the controller is the signal, and only
`AuthShell` mounts it. Workspace routes never render it and keep their existing
`WorkspaceChrome`-owned listener, so the two never contend for the attribute.
Off the desktop shell `--desktop-title-bar-height` is `0px`, so the reservation and
the drag strip collapse to nothing and `.desktop-title-bar-page` is exactly the
`min-h-screen` these surfaces had before — web is unchanged. With the prop gone the
client auth layout was a bare passthrough, so the route layout renders the shell
directly and the passthrough is deleted.
Measured in the Electron renderer over CDP across /signup, /login, /reset-password
and /cli/auth: lane 40px, logo top 56px, zero overflow on each. /invite/[id]
redirects to login when signed out and was not measured directly.
The surface audit gains a `stripComments` helper that every negative assertion runs
through. These files document the shapes they avoid, so a bare `not.toContain` was
matching the prose explaining the fix and failing on correct code.
* fix(desktop): cover the remaining traffic-light overlaps, and enumerate them in CI
Fixing signup by hand would have been the fourth time this bug was found by a person
hitting it. The audit now enumerates instead of listing what to inspect: it walks every
`.tsx` outside workspace chrome, flags each full-viewport root, and fails unless that
root either composes `.desktop-title-bar-page` or appears in an allowlist with a written
reason. A brand-new page that fills the viewport fails on arrival — verified by adding
one, and by reverting each fix below.
Running it found three more surfaces already overlapping:
- `/oauth-error` is Better Auth's `onAPIError.errorURL`, which is precisely where desktop
OAuth failures land, so the one page a user sees when sign-in breaks drew its content
under the lights.
- `/f/[token]` public file view, same shell family, same origin, reachable in the window.
- The signup and reset-password Suspense fallbacks are viewport-tall *inside* the
lane-reserving shell, so the page overflowed by the lane while the split chunk loaded.
A placeholder needs no viewport height.
Four surfaces are allowlisted with reasons: the two landing shells (the desktop shell
boots to /login or a workspace and has no path to marketing routes), the dev-only
playground, and the embedded resume interface.
Measured over CDP: /oauth-error reserves 40px with zero overflow.
* fix(desktop): cover the shells behind the allowlist, and make the guard fail
Greptile was right on both counts, and the first one is worse than reported.
`LogoShell` was allowlisted as "marketing chrome, not reachable in the desktop
shell". That claim was simply false: it is the frame for `not-found`, the interfaces
shell, the desktop handoff shell, and the public-file access gates — so the password,
email, and SSO gates for `/f/[token]` all still drew under the traffic lights. The
allowlist existed to make risk visible and instead hid four surfaces behind one
unverified sentence. It now carries two entries, both checked: the landing shell (every
consumer lives under `app/(landing)/`) and the playground (calls `notFound()` unless
`NEXT_PUBLIC_ENABLE_PLAYGROUND` is set).
The lane's two halves also travelled separately, so `/oauth-error` and the public-file
view reserved the space without the drag strip — clearing the lights but leaving the
window with no title bar on those pages. `DesktopTitleBarLane` now ships both together
and the audit enforces the pairing.
Both new checks were unfailable when first written, and mutation testing is the only
reason that surfaced:
- the pairing check matched `DesktopTitleBarLane` anywhere in the file, so the import
line satisfied it after the JSX was deleted;
- the coverage check matched `LogoShell` anywhere, so a shell's own definition file
self-certified as covered.
Both now match JSX usage (`/<LogoShell\b/`). Reverting either fix, and deleting either
half of the lane, now fails.
Measured over CDP: /oauth-error and the LogoShell-based 404 each reserve 40px, carry the
drag strip, and overflow by zero, with the logo at 56px.
* test(desktop): strip comments at read time so positive assertions can fail
Cursor caught the mirror of a trap this file already documents. `stripComments` was
applied to negative assertions only, so a positive like
`toContain('desktop-title-bar-page')` still ran on raw source — and `AuthShell`'s TSDoc
names that class, so deleting it from the markup left the assertion passing on a broken
lane reservation.
Stripping now happens in `read`, so every audit constant is comment-free and no
assertion in either direction can match prose. Verified: deleting the class from the
markup while leaving the TSDoc intact now fails two tests, where it previously failed
none.
* fix(desktop): audit workspace routes too, and cover the three that bypass the chrome
Greptile was right that the blanket `workspace/` exclusion was load-bearing in the wrong
direction. It assumed every workspace route reaches the lane through `WorkspaceChrome`,
and three do not: the workspace landing route, the access-denied early return, and the
workflow error boundary.
The exclusion is gone, so workspace files are audited like everything else and
`WorkspaceChrome` joins the lane-aware shells — the normal route passes through its
layout, and the three exceptions had to be dealt with on their merits:
- the landing route (status card and spinner) and access-denied now reserve the lane;
- the error boundary is allowlisted, verified: it renders `<Sidebar>`, which already owns
the workspace lane and its drag region, so padding that root would double it.
Their content was centred, so the lights were never covering text — the real gap was that
none of them rendered a drag strip, leaving the window immovable on those screens.
The guard's granularity is per file, not per JSX root: `workspace/page.tsx` holds two
full-viewport roots and still passes if only one reserves the lane. Verified by mutation
and documented rather than papered over — catching it needs an AST pass, and the check's
job is to stop a whole surface being forgotten, which is how every instance of this bug
has actually shipped.
* fix(desktop): teach the audit about nesting, and stop the resume skeleton double-reserving
Four findings, all correct, and the first is a bug this PR introduced.
The resume loading skeleton reserved the lane while already rendering inside
`(interfaces)/layout.tsx` -> `InterfacesShell` -> `LogoShell`, which reserves it too. Two
lots of padding, two drag strips, two controllers. It came from adding the lane there
before `LogoShell` became lane-aware and never reconciling the two. The skeleton now
reserves nothing and is no longer viewport-tall either — nesting a viewport-tall root
inside a viewport-tall shell overflowed even before this PR.
The public-file header pinned `sticky top-0`, which parks it inside the reserved lane and
under the lights. It now sticks below the lane, inert on web where the variable is `0px`.
Both audit gaps were real:
- The check was file-local, so it could not see the doubling above. It now resolves
ancestor layouts: a root counts as covered when it reserves OR sits inside a layout that
does, and reserving on both levels is its own failure. That also stops the check
demanding a second reservation from chat and the workspace overlays, which correctly
inherit theirs.
- Detection only matched `min-h-screen`/`h-screen`, so `fixed inset-0` roots never
entered it. Now included. With nesting understood, that addition resolved to a single
genuinely uncovered file rather than the ten it flagged beforehand.
Two allowlist entries added, both reasoned rather than assumed: the landing prefix (dozens
of files, one justification), and the desktop update gate — it centres its content, and
under `hiddenInset` macOS draws the lights above the web contents, so web UI cannot cover
them. This bug class is app chrome sitting under the lights, never the reverse.
Verified by mutation: reintroducing the double reservation fails the new check.
* test(desktop): do not credit inherited coverage across a layout's early return
Ancestor resolution is static, so it credits any file under a layout that mentions a
lane-aware shell. That is wrong when the layout returns the surface *instead of* its
chrome: `workspace/[workspaceId]/layout.tsx` returns `<WorkspaceAccessDenied />` at the
top and only reaches `<WorkspaceChrome>` far below, so at runtime the denied page has no
chrome at all. The page does reserve the lane today, but a regression would have read as
inherited and passed.
`SessionExpired` is deliberately not listed: it renders as a sibling within the chrome
tree, so its inherited coverage is real. The distinction is which side of the early return
the surface sits on, not which directory it lives in.
Verified by reverting the access-denied page exactly as described — it now fails.
* test(desktop): count the lane class itself as a viewport claim
Cursor caught the audit failing to watch exactly the files this PR converted. Detection
keyed on `min-h-screen`/`h-screen`/`fixed inset-0`, but converting a surface to
`.desktop-title-bar-page` removes those tokens — the class supplies `min-height: 100vh`
itself. So `/oauth-error`, the public-file view and `AuthShell` dropped out of the check
entirely, and a nested class-only reservation could ship green.
That also means the doubled-reservation check had never actually fired. The mutation I
used to "verify" it removed the lane component as well, so the pairing check caught it and
the doubled check was never exercised. It now fires on Cursor's exact scenario: a nested
class-only reservation, correctly paired, inside a lane-aware shell.
Pulling those files back in exposed a second-order bug: a shell's own definition file sits
under the layout that renders it, so ancestor resolution called `AuthShell` nested inside
itself. Shell definitions are excluded from inheritance.
One limit stays, documented rather than papered over: a root is in scope because of how it
claims the viewport, so deleting the reservation outright drops the file from the check.
That regression is loud, not silent — the surface stops being full height. Closing it
properly means treating every route entry point as a window root, which pulls in seven
account/organization/selfhost pages needing individual assessment. Worth doing separately;
allowlisting them on assumptions is the mistake that produced the `LogoShell` hole.
* fix(desktop): a fixed root escapes ancestor padding — chat drew under the lights
Cursor's sharpest catch, and the audit was actively hiding the bug rather than
missing it. `position: fixed` resolves against the viewport, not the parent, so a
lane-aware shell's `padding-top` never moves it. The chat surfaces sit inside
`LogoShell` and still painted at viewport top, under the traffic lights, while the
check reported them covered — and adding the correct reservation would then have
tripped the nested-reservation check, so the audit pushed toward the wrong answer.
Roots matching `fixed inset-0` no longer inherit coverage and are exempt from the
doubled check. That reclassified seven surfaces, each decided on evidence:
- chat, its loading boundary, the loading state and the voice interface are
full-window roots at `z-[100]` with their own top chrome — all now reserve;
- the file viewer wraps a full-bleed `<iframe>` whose content starts at viewport
top — now reserves;
- session-expired centres its content with nothing in the lane, and the search
modal's `fixed inset-0` is only its scrim (the panel sits at `top-[15%]`) — both
allowlisted with that reasoning rather than an assumption.
Verified by reverting chat to a bare `fixed inset-0` inside `LogoShell`: it is
flagged now and was called covered before.
* fix(desktop): stop the lane controller clobbering a mode another owner set
A regression I introduced last round. `DesktopTitleBarController` seeded `inset`
unconditionally on mount, before its own `getState()` resolved. That was harmless while
only `AuthShell` mounted it — but giving the file viewer a lane put a controller inside
workspace for the first time, exactly where this PR's own comments say `WorkspaceChrome`
owns the mode. Opening a file during native fullscreen therefore snapped the traffic-light
lane back on and jumped the content, and left it wrong permanently if `getState()`
rejected, since the rejection is swallowed.
It now seeds only when no owner has established a mode. The pre-paint script sets the
marker before first paint and `WorkspaceChrome` maintains it, so the unconditional write
was never the thing making the lane correct — it was only ever able to make it wrong.
Adds a controller test covering the case directly: mount during `fullscreen` with a
`getState` that never settles, and the mode survives. Verified it fails against the
previous behaviour.
* test(desktop): assert the lane on LogoShell directly, not via the sweep
Cursor found the one shell the enumeration could go silent on. `LogoShell` sits under the
`app/(landing)/` prefix allowlist, and this PR replaced its `min-h-screen` with
`desktop-title-bar-page` — so stripping the reservation also strips its last viewport
token and the sweep stops watching it. Doubly exempt, and it is the shell behind
not-found, the interfaces shell (chat, resume), the desktop handoff and the public-file
gates.
`AuthShell` already had a dedicated assertion; `LogoShell` now has the same. Verified by
stripping its reservation: caught now, silent before.
This is the documented enumeration limit made concrete rather than a new class of problem
— a root leaves the sweep when it stops claiming the viewport. The general fix is still to
treat route entry points as window roots, which pulls in seven unassessed pages and
belongs in its own change.
* docs(desktop): record why access-denied reserving is not a double reservation
Review read `WorkspaceHostProvider` as rendering the denied page inside the chrome. It is
the other way round: the provider wraps `<WorkspaceChrome>` in the layout, and its 403
branch returns the denied page instead of its children, so the chrome never mounts. The
server-side early return does the same thing higher up.
Both paths therefore bypass the chrome and the page's own reservation is the only one.
Recording that next to the entry, since the file lives under a workspace path where
inherited coverage is the norm and the reading is an easy one to repeat.
|
||
|
Waleed
|
ee0157df4b |
feat(tables): add currency column type on a new column-type registry (#6106)
* feat(tables): add currency column type on a new column-type registry
Adds a `currency` column type, and consolidates the per-type knowledge it
would otherwise have been scattered across.
**Currency.** Stores a plain number and carries an ISO 4217 `currencyCode`
as display metadata. That split is what keeps it cheap: filtering, sorting,
uniqueness and CSV export all reuse the numeric paths unchanged, changing a
column's currency rewrites no rows, and the public row output stays a number
rather than a locale-formatted string consumers would have to reparse.
Input accepts the shapes an amount actually arrives in — `$1,234.56`,
`1 234,56 €`, `(12.00)` — so pastes, CSV imports and tool writes land as
numbers instead of being nulled.
**The registry.** Adding this type initially required edits in ~40 places:
32 switch arms under `lib/table`, ~26 UI branches, two hand-maintained icon
maps, and a coercion implementation duplicated four times. Every one of those
failed silently when missed — a missing `jsonbCastForType` arm compares
numbers as text; a missing compatibility arm blocks all conversions.
`lib/table/column-types/` now holds one file per type carrying its label,
icon, badge colour, storage cast, filter operators, coercion, validation,
compatibility and formatting. `Record<ColumnType, …>` on both registries is
the completeness gate: adding a type to the union is a compile error naming
exactly the two files to fill in, and the interface then requires every
field. The 32 switch arms are down to 3.
Two duplicates collapse as a consequence:
- The client no longer mirrors the server's select id-resolution. Those
helpers lived in `validation.ts`, which imports drizzle, so anything
reaching them became server-only and the grid hand-rolled its own copy.
Extracting them to `select-options.ts` lets both sides share one
implementation, so the optimistic cache can no longer disagree with what
gets persisted.
- The two icon maps become one registry read.
It also fixes a live inconsistency it surfaced: currency got a numeric
keypad in the grid's inline editor but a plain text field in the row modal.
Behaviour-neutral by construction: all 1046 tests in the touched areas pass
unchanged, with no test edits.
* test(tables): guard the column-type registry's invariants
Property tests for the registry itself rather than any one type: entries key
by their own id, COLUMN_TYPES stays derived, an unknown type degrades to
string instead of throwing, only opaque-id types restrict filter operators,
only configuration-free types are CSV-inferable, and every type that can
reject a draft has a message to show.
Plus the metadata-ownership matrix, which pins the generic ownership check to
the same answers the hardcoded per-type rules gave.
These target the registry's silent-failure class — a wrong jsonbCast or a
stray operator whitelist used to be invisible until a filter failed in SQL.
Both are verified to fail under mutation.
* fix(tables): read exponent-form amounts and reject bad currency PATCHes up front
Two P1s from review.
Scientific notation lost magnitude. `String()` emits exponent form past 1e21,
so a stored amount round-trips through the editor as `1e+21` — and the
sanitizer treated the `e` as decoration to strip, reading it back as 121. An
untouched cell silently lost 19 orders of magnitude on its next edit. Exponent
form is now taken at face value, but only when the string is wholly a numeric
literal once symbols are removed, so `12 EUR` (whose `E` survives the strip)
still parses through the separator path.
A failed currency PATCH left a partial rename. `renameColumn` commits in its
own transaction before the currency write, so a `currencyCode` the service
would reject — an unsupported code, or any code on a non-currency column —
errored only after the rename had stuck. Both are now caught before the first
write, matching the guard the route already applies to unique-on-select for
exactly this reason.
* refactor(tables): finish the registry migration and drop the dead config
Audit pass over every consumer, closing the gaps the first cut left.
Functional gap: the copilot agent had no currency support at all — it could
create a currency column with no code and could never re-denominate one.
`add_column` and `update_column` now accept `currencyCode`, with the same
up-front validation and the same code-only routing as the HTTP routes.
Config that consumers were still restating, now read from the registry:
- `supportsUnique` replaces the unique-on-select guard stated in three places
(service, both column routes, the copilot tool).
- `editor === 'toggle'` replaces seven `type === 'boolean'` checks in the grid
and expanded popover, all of which meant the same thing.
- `defaultMetadata` replaces the per-type stamping in `addTableColumn` and
`updateColumnType`.
- `sampleValue` replaces the per-type example values in the LLM prompt
scaffolding.
- `storesOpaqueIds` replaces the select filter in the find-row matcher.
Dead config removed: `getTypeBadgeVariant` had zero callers (already dead on
staging), and it was the only reader of `badgeVariant` — so the field, its
union, and all seven values went with it. `inferFromCsv` was read by nothing
but a comment; CSV inference is an ordered heuristic a boolean cannot express,
so it is gone too and `InferredCsvColumnType` is no longer exported.
Fixes a latent crash found on the way: unique-constraint checking normalized a
cell keyed on its RUNTIME type but reconstructed it keyed on the column's
DECLARED type, so a unique `date` column stored a bare `2024-01-01` and then
threw `SyntaxError` parsing it back. Both directions now go through JSON
unconditionally. Pre-existing, unrelated to currency.
Adds the `/add-column-type` skill and a Tables section in CLAUDE.md/AGENTS.md
pointing at it, so the next type is one file plus two registry entries.
* fix(tables): run the column PATCH guards ahead of the rename, not after it
Greptile was right and my previous reply was wrong. The guards were added in
the right shape but the wrong place — below `renameColumn`, which is the
first write and commits in its own transaction. A PATCH combining a rename
with an invalid currency therefore still committed the rename and then
returned 400, exactly the counterexample reported.
Moved the column lookup and all three pre-flight guards above every write.
This also closes the same latent hole for the pre-existing unique-on-select
guard, which sat in the same position.
Adds route tests that assert `renameColumn` was never called on each
rejection path, and that a valid combined rename + currency change still
targets the new name. Verified to fail against the previous ordering.
* fix(tables): make the retype gate and the write path share one parser
A simplify pass over the registry found two real defects and several places
the abstraction was being worked around.
Silent data loss on conversion. `isCompatibleWith` was hand-written per type
and had already drifted from `coerce`, despite the interface promising they
could not: `boolean` accepted '1'/'0'/0/1 in the gate but only 'true'/'false'
in the write path, so converting a column holding "1" reported zero
incompatible rows and then nulled every one of them. `date` drifted the other
way. `isCompatibleWith` is now optional and defaults to `coerce(...).ok`, so
the two are the same code; only `select` overrides, because its rules are
about the column (cleared-vs-required, cardinality) not the value.
`isColumnType` used `in`, which matches inherited keys — `isColumnType('toString')`
was true and `columnTypeById('toString')` returned `Function.prototype.toString`,
which the validator would then call `.validateDefinition()` on. Now `Object.hasOwn`.
`defaultMetadata` only ran on the currency arm of a retype, so a future type
would get its defaults on create but silently not on conversion. It now runs
for every non-select target, carrying forward only metadata the TARGET type
declares it owns — a currency→text conversion no longer strands a currencyCode.
The index doc claimed the registry is kept out of the `@/lib/table` barrel so
44 server modules don't pull `@sim/emcn/icons`. That was false: `constants.ts`
re-exported `COLUMN_TYPES` from the icon-carrying `registry.ts`, and the barrel
re-exports `constants`. `COLUMN_TYPES` now lives in the icon-free `types.ts`;
verified with an import tracer that both are icon-free again.
Also: 5 no-op `validateDefinition`s and 4 duplicated formatters collapsed into
registry defaults; `CURRENCY_OPTIONS` was an eager module-load IIFE costing
~8ms of ICU work on every table API route for a list only the config sidebar
reads, now built on first call; and the skill's validation grep claimed 'should
return nothing' when it returns 8 legitimate hits — it now explains how to tell
a leak from a genuine special case.
* fix(tables): reject a non-leading sign so dates don't parse as amounts
Found by Cursor Bugbot. `parseCurrencyInput` dropped every `-` as decoration,
so an ISO date's hyphens vanished and its digit groups joined: `2024-01-01`
read as 20240101. With the gate now sharing the write path's parser, a
date → currency conversion reported zero incompatible rows and silently
turned every cell into a huge number.
A sign is only meaningful at the front; an interior one means the string is
not a single amount. Leading signs, accounting parentheses, symbols, ISO
codes, grouping separators, and exponent form all still parse — covered by
the existing cases plus new ones, verified to fail without the fix.
* fix(tables): use getErrorMessage in the columns route test mock
`check:utils` bans the inline `e instanceof Error ? e.message : fallback`
form; the mock for `rootErrorMessage` used it.
* fix(tables): rename the column last so a failed write leaves it untouched
Greptile's remaining concern: the pre-flight guards read a schema snapshot, so
a column-type change landing concurrently can still make a later write fail —
and with the rename running first, that failure returned an error with the
rename already committed.
Guards cannot close that window; each write is its own locked transaction and
only the write itself sees the authoritative state. Ordering can. The rename is
the one write that is purely cosmetic, so it now runs last: a failed typed
write leaves the column entirely untouched, and a failed rename leaves the
typed change applied under the old name — the recoverable half. The typed
writes target the column's current name, since no rename has happened yet.
Tests cover both directions: a typed write rejected mid-flight must not rename,
and a successful one must rename strictly after. Verified to fail under the
previous ordering.
* fix(tables): write back coerced values on every conversion
Round 4 findings, all real.
A conversion is allowed exactly when the target type's `coerce` accepts the
value — and `coerce` frequently TRANSFORMS it. Only `select` and `currency`
wrote the transformed value back, so a conversion to any other transforming
type left the cell holding its old bytes under the new type. Converting a
number column to `date` accepted epoch values, stored them unchanged, and then
`(data->>'col')::timestamptz` failed on EVERY query against that column. I
opened this myself by defaulting `isCompatibleWith` to `coerce(...).ok`.
Fixed at the class rather than the instance: the compatibility scan now records
whatever `coerce` produced whenever it differs from what is stored, and one
generic write-back applies it. That subsumes the currency-specific migration
entirely, so it and its helpers are gone. `select` keeps its own id↔name
migrations, which are not coerce-expressible in the outbound direction. The
post-conversion column definition is built once, before the scan, so the
coercion reads the same metadata the stored value is later validated against.
Exponent parsing was ambiguous when followed by text: `1e5 EUR` read as 15.
An `e` with a digit on both sides is an exponent marker, so if the string is
not a clean numeric literal it is refused rather than guessed — the digit on
both sides is what keeps the `E` inside `12 EUR` parsing normally.
A failed rename could still leave a typed change committed. The one rename
failure a caller can cause — a name already taken — is now rejected up front,
leaving only the concurrent-collision race, which no pre-flight check can close
without spanning all writes in one transaction.
* fix(tables): stop a blank cell blocking an optional type conversion
Found by Cursor Bugbot. `''` is incompatible with every numeric type, and the
compatibility scan counted it as a hard blocker regardless of whether the
target was optional — so a text column with a single empty cell could not be
converted to a number at all, and the error said 'to a required ...' either way.
An unreadable-but-empty cell is not a conversion failure. The write path
already turns an unreadable value into null on an optional column, so the
conversion now does the same and records null for it. A required target still
reports it, which the existing guard above already does with the message that
actually fits.
Also pins the two intentional divergences from the pre-registry behavior. A
differential run of the registry against the pre-refactor implementations (55
values x 7 column shapes) found ZERO coercion differences and exactly two
compatibility differences, both deliberate: boolean now rejects the '1'/'0'
conversions the old gate accepted and then nulled, and date now accepts the
epoch numbers its write path always accepted. Tests pin both so neither can be
silently reverted or widened.
* fix(tables): refuse conversions that would invent or destroy values
Final adversarial scan found two data-corrupting conversions, both opened by
defaulting the retype gate to the write path's parser.
number → date destroyed every value. `date.coerce` reads a number as epoch
milliseconds, which is right for one deliberate write and catastrophic applied
to a whole column: 1, 5, 42 became three timestamps in January 1970, and a
Unix-seconds column landed in 1970 rather than the year it meant. Irreversible.
`date` now overrides the gate to reject numbers, restoring the pre-refactor
behavior, and the contract states the rule the override obeys: a gate may be
STRICTER than `coerce`, never looser. Stricter refuses a bulk conversion while
single writes still work; looser is the direction that corrupts.
string → currency invented values. The parser stripped every non-digit and
joined what was left, so `01/02/2024` read as 1022024, `Room 101` as 101, and
`0.1.2` as 12 — a column of SKUs or phone numbers converted with zero reported
incompatibilities. What remains after removing symbols, spacing and an ISO code
must now be only digits and separators, and grouping must be well-formed (a
first group of 1-3 digits, the rest exactly 3). Every legitimate form still
parses, including all the locale variants.
Also generifies the last three metadata leaks: `buildConvertedColumn` strips
and carries back by iterating the key list rather than naming keys (naming them
meant a future type's metadata rode onto a target that rejects it, failing that
column's validation on every later write), `normalizeColumn` forwards metadata
through a shared `typeMetadataOf`, and `filterOperatorsFor` moved onto the
definition — it was a per-type branch inside the registry's own accessor, the
one thing the registry exists to forbid.
Skill corrected: it claimed COLUMN_TYPES derives from the registry (backwards),
promised exactly two compile errors (four once a type owns metadata), used a
grep that missed half the real branches, and never mentioned `import.ts`'s
second coercion path, whose silent default arm is the costliest miss available.
Differential re-run vs the pre-refactor implementations: 0 coercion
differences, 1 intentional compatibility difference (boolean no longer accepts
the 0/1 conversions the old gate accepted and then nulled).
* fix(tables): let the row modal accept formatted amounts again
Found by Cursor Bugbot. I unified the row modal's input type with the grid's
`inputMode` last round, but in the wrong direction: mapping `inputMode:
'decimal'` to `<input type="number">` made the modal reject $1,234.56,
1.234,56 and (12.00) — the exact formats `parseCurrencyInput` exists to accept,
and which the grid's inline editor takes fine.
A native number input and a numeric keypad are different things. Types whose
parser accepts formatted text now say so, and get a text field with
`inputMode='decimal'` — the shape the grid already uses. A plain number keeps
the native input, its spinner, and its validation.
* fix(tables): fold a rename into the write it accompanies
Closes the last partial-update window, properly rather than by pre-checking
around it.
A rename is metadata-only — `renameColumn`'s own comment says so: rows,
metadata, and workflow-group refs all key on the stable column id, so it is a
pure schema write. Nothing forced it to be its own transaction. Running it
separately is what created the window: whichever half committed first survived
a failure in the other, and no pre-flight guard can close a concurrent
collision because only the write itself sees authoritative state.
The four column writes now accept an optional `newName` and apply it through
one shared `applyPendingRename`, which validates the name shape and checks the
collision against the very schema snapshot that write is landing in. A combined
request rides the rename on whichever write runs last, so both halves commit
together or neither does — a concurrent claim on the name now aborts the whole
transaction instead of leaving the other change applied.
The routes also address every write by the column's stable id rather than its
name, so folding a rename into one write cannot break the next one's lookup.
A rename with nothing to ride on still runs standalone.
What remains partial is a type write followed by a failing constraints write —
two independently locked transactions, pre-existing, and untouched by this PR.
* fix(tables): migrate scalar cells when converting a column to select
Found by Cursor Bugbot. `resolveSelectOptionId` stringifies a number or
boolean before matching, so a `number` column whose values equal option NAMES
passes the compatibility gate — but `migrateCellsToSelectIds` only rewrote
JSONB `string` and `array` cells. Those cells stayed raw numbers inside a
select column, where they render as nothing and fail option membership on the
next write.
`data->>key` yields the text form for every scalar, so the existing lookup
already worked; the predicate was simply too narrow. Widened to cover
`number` and `boolean`. The outbound migration is unchanged — cells leaving a
select column are option ids, always strings or arrays.
Pre-existing on staging (both the resolver's scalar handling and the migration
SQL predate this branch), but it lives in a file this PR creates.
Tests pin the resolver behavior the predicate depends on, so narrowing either
one without the other now fails.
* fix(tables): validate a retype's unique against the values it writes
Validated the last partial-update seam with a focused investigation rather
than assuming. The answer was split.
`required` is already safe: `updateColumnType` runs the same `countEmptyCells`
against the constraint the request is about to set, which is why that check
exists.
`unique` was not, and the reachable case commits the unrecoverable half. A
text column holding "5" and "5.0", PATCHed with {type: number, unique: true}:
the conversion succeeds and coerces both to 5, then the separate constraint
write finds duplicates and 400s — with the column already numeric and "5.0"
irreversibly rewritten. A pre-scan of the raw text finds nothing; the
conversion is what manufactures the duplicate. The retype now carries `unique`
and checks it after the write-back, against the values it just wrote.
Constraint changes on a workflow-output column were the same shape — rejected
by the constraint write, after a type change had committed. Now rejected in the
route's pre-flight block, before any write.
The duplicate scan is extracted and shared between both paths for the same
reason `countEmptyCells` is: two copies of one rule is the drift that produced
the original required-check bug.
Deliberately NOT merging `updateColumnType` and `updateColumnConstraints`. They
assert different lock levels (destructive vs schema-only) and only the retype
needs the full row scan, so merging would either force a constraints-only
toggle to materialize every row or reintroduce the branching it was meant to
remove. With both reachable failures pre-validated, what remains at the seam is
concurrent races no in-process check can close.
* fix(tables): don't drop a rename when the write it rides on no-ops
Found by Cursor Bugbot — a bug I introduced folding the rename in.
`updateColumnCurrency` returns early when the code is unchanged, and that
return sat ahead of the rename, so PATCH {name, currencyCode} with the column's
current code answered 200 with the rename silently discarded.
Both early returns now treat a pending rename as work: the currency path only
no-ops when the code is unchanged AND no rename is riding along, and the retype
path applies a rename-only write when the type is unchanged. `applyPendingRename`
signals "nothing to do" by returning the same reference, which is what lets
both detect it cleanly.
Also extracts `persistColumns` — five sites were repeating the same
schema-write-and-return.
* fix(tables): make a combined column PATCH a single transaction
Finishes the fold-in rather than pre-validating around the seam. A retype now
APPLIES the constraints it already validates against — it checks empty cells for
`required` and post-conversion duplicates for `unique`, so it was doing the work
without persisting the result — and the route skips the separate constraint
write when the type changed.
A request combining a rename, a retype and constraint changes is now one
locked transaction: no half of it can commit while another fails. The separate
constraint write remains for requests that do not change type, which is the
only case that still needs it.
Deliberately still NOT merging the two service functions. They assert different
lock levels (destructive vs schema-only) and only the retype needs the full row
scan into memory, so a merged function would force a constraints-only toggle to
materialize every row or reintroduce the branching it was meant to remove.
Folding the payload in gets atomicity without either cost.
* fix(tables): reject a flattened list as an amount; fold constraints into every typed write
Two findings from round 11.
Multi-select converted to nonsense amounts. `selectValueForConversion`
flattens a multi cell to its comma-joined option names, and the parser read
that as a formatted number: options 12 and 34 became 12.34, and 100 and 200
became 100200. No real amount puts whitespace after a separator, but a
delimited list does — so a separator followed by whitespace is now refused.
Every legitimate form still parses, including space-grouped locales.
Combined options-or-currency + constraints could still commit partially. Those
two writes now carry constraints the same way the retype does, through one
shared `applyConstraints` that validates (workflow-output, empty cells for
required, supportsUnique and duplicates for unique) and applies them. The
separate constraint write now runs only when no typed write does. Three copies
of those rules is the drift that produced the original required-check bug, so
they live in one place.
* fix(tables): validate constraints after the migrations that rewrite cells
Self-caught while reviewing my own previous commit, which introduced both.
`updateColumnOptions` ran the shared `applyConstraints` BEFORE its cell
migrations. Those migrations rewrite stored values — a single<->multi toggle
changes the shape, removing an option clears cells — so a `unique` scan read
the pre-migration values, passed, and the rewrite could then produce the
duplicates the scan was meant to prevent. Moved to after the migrations, which
is where `updateColumnType` already had it.
The same commit also left the options path running `required`'s empty-cell
check twice: once in the shared helper and once in its original inline block,
whose comment still described a separate constraint write that no longer runs.
Removed the duplicate — one query, one rule, which is the whole point of the
shared helper.
Also routes the options path through `persistColumns` like the others.
* fix(tables): stop inventing amounts from identifiers; fix the copilot retype
Adversarial pass over the final state, seven real findings.
Two destroyed data. The copilot `update_column` still used the two-transaction
pattern the HTTP routes were fixed for: `unique` was never forwarded to the
typed write, so a retype+unique committed the conversion and then failed the
constraint — the same irrecoverable half. It now rides the typed write, and the
separate constraint write only runs when no typed write did.
And the parser's three-letter strip removed ANY three letters, not an ISO code:
`SKU400` parsed as 400, `ABC1234` as 1234. Converting a column of part numbers
to currency rewrote every cell with an invented value — while the comment two
lines above claimed a SKU was exactly what it prevented. The rule is now that a
letter touching a digit means identifier, not amount; a currency marker is
always separated by a space or a symbol.
That same change fixed a class the review surfaced: the pinned currencies could
not parse their own conventional notation. `R$ 1.234,56`, `1 234,56 kr`,
`1234,56 zł`, `CHF 1’234.56` and Indian lakh grouping (`₹12,34,567.89`) all
work now — these are what Intl emits, so a paste from a spreadsheet was being
rejected.
`updateColumnConstraints` was a fourth copy of the constraint rules the shared
helper exists to unify, and had already drifted: it hardcoded `type ===
'select'` where the helper asks the registry, so a future type declaring
`supportsUnique: false` would have been ignored on that path. It now uses the
helper.
`updateColumnType`'s unchanged-type early return silently discarded every
field except the rename. Callers gate on the type changing, but from a read
taken before the lock — so a concurrent change could land there with real work
pending and answer success. It now throws.
Also: `UpdateColumnCurrencyData` was missing `required`, which only compiled
because the routes pass it through a spread; a missing column returns 404
instead of a 400 reading "of type undefined"; and the comments describing the
old two-transaction architecture are gone.
Verified NOT a bug: CSV export of a currency column writes the raw number, so
export/import round-trips losslessly.
* fix(tables): read the negative and RTL forms Intl actually emits
An Intl sweep across 24 locales found two forms the parser rejected, both from
an ordinary spreadsheet paste.
`Intl` emits U+2212 MINUS SIGN rather than the ASCII hyphen for negatives in
several locales, so `−12,50 kr` read as null instead of -12.5. And it wraps
RTL-locale output in invisible bidi control marks, so `1,234.56 ₪` carried
characters that are not part of the amount. Both are now normalized away.
24 locales x 6 amounts now round-trip, up from 99/100 when the sweep started —
and the test generates them from `Intl` rather than listing them by hand, so a
parser change cannot quietly regress a locale nobody remembered to write down.
Locales that format with their own numeral systems (Arabic-Indic) are still
rejected, and now say so in the docstring. That is a safe failure — null rather
than a wrong value — and supporting them is a wider decision than this type,
since it would also touch `number`, display, and sorting.
|
||
|
Vikhyath MondretiandClaude
|
7798e83489 |
feat(function): custom sandboxes (#6071)
* feat(sandboxes): workspace dependency sets for Function blocks Named package sets a Function block can import from. The server canonicalizes and hashes the list; E2B prebuilds a content-addressed template per set, Daytona installs per execution. Create/edit is gated to Max or Enterprise via the shared workspace entitlement check; execution is deliberately ungated, so a downgraded workspace keeps running what it already built. Also on this branch: - Extract the duplicated dropdown/combobox option-fetch lifecycle into use-fetched-options. Only combobox had the dependency-change reset, so every dropdown with dependsOn + fetchOptions cleared its list and never repopulated until reopened. - Collapse the repeated Max-tier entitlement check onto one hasMaxTierWorkspaceAccess, shared by inbox, live sync, and sandboxes. - Resolve a personal payer's block state through getEffectiveBillingStatus in getBillingEntityBlockStatus, so the client-side Max gates agree with the server-side ones when blockOrgMembers' fan-out is stale. - Carve the Daytona dependency install out of the caller's execution budget instead of stacking on top of it. Co-Authored-By: Claude <noreply@anthropic.com> * chore(db): regenerate the sandboxes migration as 0273 Staging claimed 0271 and 0272 while this branch was out, so the hand-authored 0271_workspace_sandboxes was dropped before the merge and regenerated on top of the merged schema. Same DDL; drizzle emits plain CREATE TABLE/INDEX rather than the hand-added IF NOT EXISTS, which matches the repo default — that idempotent form is only needed for files with CONCURRENTLY ops below an embedded COMMIT. Regenerating also restores the meta snapshot the hand-authored migration never had. Co-Authored-By: Claude <noreply@anthropic.com> * chore(db): drop the sandboxes migration ahead of the staging merge Staging independently claims idx 0273, so remove ours before merging to avoid an add/add conflict on the drizzle migration index. Regenerated at the next free index once the merge lands. Co-Authored-By: Claude <noreply@anthropic.com> * chore(db): regenerate the sandboxes migration as 0275 Staging took 0273 and 0274, so the sandboxes DDL lands at the next free index. The emitted SQL is byte-identical to the dropped 0273. Co-Authored-By: Claude <noreply@anthropic.com> * fix(billing): consolidate the Max-tier entitlement onto one predicate The Max tier was spelled five ways. The odd one out — `isMax`, defined as `isPro(plan) && credits >= 25000` — excluded both `team_25000` and `enterprise`, and it was the sole input to the personal-workspace cap. A delinquent Max-for-Teams org admin got 1 personal workspace while a delinquent Max individual got 10. Only free/pro_6000/pro_25000 were tested, so the two broken tiers were unpinned. Separately, the server gate and the client `hasUsableMaxAccess` were independent copies of the same rule. The settings sidebar renders Sandboxes and Sim Mailer from the client one while the API answers 403 from the server one, so any drift renders a feature unlocked that the API refuses. - `MAX_TIER_CREDITS` is derived from the `CREDIT_TIERS` table; `isMaxTier` in plan-helpers is now the single definition, shared by the server gates, the client derivation, `getPlanTypeForLimits`, `plan-view`, and the cap - `hasWorkspaceTierAccess(id, predicate, { intent, onMissingWorkspace })` becomes the one org-vs-personal payer fork. `intent: 'active-use'` means active and not billing-blocked; `'retention'` means active/past_due with block state ignored, so the inbox teardown guard keeps its fail-open semantics instead of implying them through a duplicated fork - `isWorkspaceOnEnterprisePlan`'s personal branch now applies the status and block checks its own org branch always had, and its TSDoc names its real consumer (copilot BYOK, not Access Control) - the client live-sync gate gained the server's `isHosted` branch, so a self-hosted deploy with billing on no longer locks an interval the API accepts. It reads both flags directly rather than taking one as a parameter the callers sourced from the same module - `sqlIsPro`/`sqlIsTeam` escape the `_` LIKE wildcard, matching the already correct hand-rolled filter in seat-drift - deletes the `TERMINAL_SUBSCRIPTION_STATUSES` and `ENTITLED_STATUSES` shadow constants, and corrects three test mocks that asserted `trialing` was entitled or usable `max-tier-parity.test.ts` asserts the client and server answers match for every plan name. Both new guards were checked against the old code: the parity test fails 3 assertions with the previous predicate, and the self-hosted test fails without the `isHosted` branch. Co-Authored-By: Claude <noreply@anthropic.com> * chore(db): drop the sandboxes migration ahead of the staging merge Staging has claimed 0275 (table_views) and 0276 (drop_legacy_folder_tables) since the last merge, so our 0275_workspace_sandboxes collides on the index. Dropping ours first — the .sql, meta/0275_snapshot.json, and the journal entry — leaves packages/db/migrations byte-identical to the merge-base, so the merge sees no add/add conflict at all. Regenerated on the far side. Ours is the droppable side: plain additive DDL with no hand edits, which drizzle reproduces exactly. Staging's migrations are hand-written and must survive. Co-Authored-By: Claude <noreply@anthropic.com> * chore(db): regenerate the sandboxes migration as 0277 Staging claimed 0275 (table_views) and 0276 (drop_legacy_folder_tables), so the sandboxes migration dropped before the merge comes back on top as 0277. The emitted SQL is byte-identical to what was dropped — the original had no hand edits, so there is nothing to reapply. It is purely additive: two enums, sandbox_image and workspace_sandbox, their two FKs and six indexes. That it regenerated unchanged also confirms the schema.ts auto-merge was correct — had it lost staging's legacy-folder-table drops, drizzle would have emitted CREATE TABLE for them here. Snapshot chain is continuous (0273 -> 0277, each prevId matching the previous id) and the table counts track the DDL: 100 -> 101 (table_views) -> 99 (legacy folder tables dropped) -> 101 (the two sandbox tables). Co-Authored-By: Claude <noreply@anthropic.com> * feat(sandboxes): gate on the enterprise feature flags, drop the rollout switch Sandboxes shipped behind `custom-sandboxes`, an AppConfig rollout flag falling back to a `CUSTOM_SANDBOXES` secret. That made it the only Max-gated surface with no self-hosted path: `INBOX_ENABLED` can force Sim Mailer on for an operator running their own billing, and `ENTERPRISE_ENABLED` turns on the other nine features at once, but neither reached sandboxes. A self-hoster had to find a separately-named variable that was not part of that family, and one running with billing enabled could not enable it at all. Sandboxes now joins the enterprise feature set and the rollout flag is gone: - `sandboxes` is an `EnterpriseFeature` with `SANDBOXES_ENABLED` and its `NEXT_PUBLIC_` twin, so the master switch and the per-feature override both reach it like every sibling - `hasWorkspaceSandboxAccess` takes the inbox's shape exactly — the override wins, then a deployment without billing is unrestricted, then the workspace payer needs usable Max or Enterprise - the settings nav gains `selfHostedOverride`, so the section resolves through the same path as Sim Mailer instead of a second entitlement AND-ed in - `custom-sandboxes`, the `CUSTOM_SANDBOXES` secret, the now-unreachable `SANDBOXES_UNAVAILABLE` 403 copy, and the route's kill-switch branch are deleted Its legacy default is `true`, matching `inbox`: the gate already returns true whenever billing is off, so `false` would leave the nav override disagreeing with the gate that answers the request. Self-hosted builds run on the operator's own E2B/Daytona credentials, so there is no Sim-side cost to withhold — the docs now say so, since enabling the feature without a provider configured is the obvious trap. The new gate tests run with billing enabled on purpose; the `!isBillingEnabled` bail would otherwise answer every case and hide whether the override is wired. Verified by deleting the override line — exactly the one assertion fails. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): let the language menu match its trigger width `matchTriggerWidth={false}` exists for the opposite case — a narrow trigger whose option labels would truncate, letting the menu grow past it. The language field is a full-width form control with two short labels, so the override shrank the menu to "JavaScript" and pinned it to the right edge instead. The default (`true`) is correct here. Every other consumer passing `false` is a genuinely narrow trigger — a role picker in a member row, a table filter chip. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): re-queue a build when resolution finds the image unusable `ensureSandboxImage` only ran when a sandbox was saved, so resolution treated an unusable image as terminal and told the user to go fix a definition that was never wrong. Three states stuck permanently until someone re-saved in Settings: - a build that failed - a build whose worker died mid-flight, stranding the row in `building` - every sandbox created while the deployment ran a `runtime` provider, after a switch to a `prebuilt` one — `runtime` writes no image rows at all, so the whole fleet resolved to "no completed build" with nothing to repair it Resolution now re-queues through the registry's existing idempotent entry point before failing, and says a build is on its way instead of pointing at Settings. The conflict guard already claims only a `failed` row or a stale `pending`/ `building` one, so executions arriving during a healthy build enqueue nothing — no thundering herd from a hot workflow. The registry is imported dynamically for the same reason `sandboxDb` is: it pulls `@sim/db` into the static graph, which this module keeps out of the executor bundle. That also avoids a cycle, since the registry imports `invalidateSandboxResolution` from here. A repair that itself fails is logged and swallowed — it must never replace the build error naming the sandbox. Verified by deleting the repair call: exactly the three new assertions fail. Co-Authored-By: Claude <noreply@anthropic.com> * improvement(sandboxes): let the picker show just the sandbox name The label read "Test · Python · 1 package". The block's own list is already scoped to the language its sibling `language` subblock selects, so the language repeated on every row said nothing, and the package count is decoration next to the name that identifies the sandbox. The language stays for the one caller that cannot filter — agent tool-input renders this field under a synthetic id where the sibling `language` value is unreachable, so its list spans both languages and the name alone is ambiguous. That is the same missing value which disables filtering, so `showLanguage` is derived from it directly rather than passed independently and left to drift. A failed build is still marked: that suffix is the difference between a selection that runs and one that does not. Passing the flag also means dropping `.map(toSandboxOption)` for an explicit arrow — `Array.map` hands the index to the second parameter. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): show the sandbox name on the block card, not its uuid The card printed "443f4934-26ab-44ab-8...". `resolveDropdownLabel` only reads a subblock's static `options` array, and the sandbox picker is a `combobox` whose options load asynchronously, so its array is empty and the raw stored id fell through to the label. Resolved the same way skills and tools already are: a `resolveSandboxLabel` in the display layer, fed from the shared sandbox list query — the same cache entry the picker reads, so this adds no request. Two deliberate scopings: - the query is subscribed only for the sandbox row. `SubBlockRow` is memoized per subblock, and the list query polls while a build is in flight, so an unconditional hook would re-render every row on the canvas on each poll tick - the resolver matches the field id, not just the type. There is no dedicated subblock type for it, and matching `combobox` alone would relabel unrelated pickers An id with no matching sandbox resolves to null rather than a guess, so a deleted sandbox falls through to the caller's placeholder. The template preview surface is left alone: it is explicitly hook-free and passes empty lists for tools and skills too. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): hide the Sandboxes section with no provider configured Entitlement decides whether a workspace may author sandboxes; nothing decided whether anything could run one. A self-hosted deployment with SANDBOXES_ENABLED but no E2B or Daytona credentials got a fully functional tab whose output no Function block could select — the picker is gated on the provider vars, the tab was not. Both navigation planes now drop the section when neither NEXT_PUBLIC_SANDBOX_ENABLED nor the pre-Daytona NEXT_PUBLIC_E2B_ENABLED is set — the same pair the picker's `showWhenEnvSet` reads, so the two cannot disagree. Dropped rather than locked: an upgrade does not conjure a provider. The unified plane drops it in `buildUnifiedSettingsNavigation` rather than in the sidebar's filter, because the sidebar's `selfHostedOverride` short-circuit runs before its `requiresMax` check and would have revealed the tab anyway. It reads the browser twins, not the server's `isRemoteSandboxEnabled`, since this module renders on both sides. The predicate is a function, not a module constant, because the constant form was untestable and ambient: the env mock falls through to `process.env`, and `apps/sim/.env` (gitignored, so absent on CI) sets NEXT_PUBLIC_E2B_ENABLED=true. The nav tests passed locally and failed 6 assertions with the flag cleared. They now pin both flags, so the suite is identical with and without a local env file — verified by running it both ways. Co-Authored-By: Claude <noreply@anthropic.com> * docs(sandboxes): correct three claims the code no longer makes The Sandboxes section described behavior two commits on this branch changed, and led with an internal detail no reader needs. - entitlement is no longer Max/Enterprise only: self-hosted deployments unlock sandboxes with SANDBOXES_ENABLED, and the section is hidden outright when a deployment has no sandbox provider, which is the state a self-hoster is most likely to hit and least likely to diagnose - a build that is not Ready is no longer terminal. It is queued again on the next run, so the advice is to wait and re-run, not to go edit a package list that was never wrong - deleting a sandbox frees its build once nothing else references it. Builds are shared by content, so this is the one place a reader could reasonably assume deletion is immediate Dropped the `ModuleNotFoundError` aside: what the old code did instead is not something a reader needs to know to use the feature. The page is hand-written — `function` has category 'blocks' and is absent from `NATIVE_RESOURCE_BLOCK_TYPES`, so generate-docs skips it and these edits will not be overwritten. Co-Authored-By: Claude <noreply@anthropic.com> * feat(sandboxes): release the provider image when nothing references it Deleting a sandbox only removed its row, leaving the built template in E2B until the 30-day retention sweep — up to a month of paying to store an image nothing could select. Editing a package list had the same effect on the old content address, which is the more common case since every edit re-points the sandbox. `releaseSandboxImage(specHash)` now deletes the provider image and its row from both paths. It reuses the sweep's provider call and its ordering: image first, row second, so a refused delete leaves the row for the sweep to retry rather than orphaning a remote template nothing points at. Two guards make eager deletion safe: - builds are keyed by content, not by workspace, so two workspaces declaring the same package list share one image. The release no-ops while any sandbox still references the hash — otherwise one workspace's delete would break the other's - an in-flight build is left alone rather than raced; the sweep collects it once it settles Called detached from both routes. The row is already committed by then, so the user's action has succeeded whatever the provider says, and awaiting would hold a UI delete open on a remote call the sweep would retry anyway. Every failure inside is logged and swallowed for the same reason. E2B's delete verified against their API reference: DELETE /templates/{templateID} with X-API-Key, 204 on success. The existing implementation already matched, so this commit only adds the call sites and the guards. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): rate-limit the automatic rebuild, drop the one-off status dot Two follow-ups to the resolution repair. The repair had no rate limit. `ensureSandboxImage` re-claims a `failed` row on sight, and a bad package name fails in seconds, so the in-flight guard never closed the window: a workflow on a one-minute schedule would enqueue a build a minute against a package list that will never resolve, each one real provider build compute. Before the repair existed resolution simply threw, so this was introduced with it. The two callers want different things, so the cooldown is opt-in. A save is a person explicitly asking for another attempt and still retries immediately; resolution passes `FAILED_BUILD_RETRY_COOLDOWN_MS` and gets at most one attempt per window no matter how often the workflow runs. Ten minutes: long enough that per-minute runs cannot drive per-minute builds, short enough that a transient registry outage clears within the hour. The status line loses its colour dot. `size-[6px] rounded-full` appeared in exactly one file in the repo, so it was a new primitive rather than a pattern, and it duplicated state the text colour already carries — the label now turns `--text-error` on a failed build, which is what every other status row in settings does. `ChipTag` was the wrong home for this: its variants are `mono`/`invite`, with no semantic tone, so a status version would have meant overriding its chrome from the consumer. Also corrects the docs line this changes: a failed build is retried periodically, and saving is the way to retry now, so "wait a moment and run again" no longer describes it. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): claim the image row and its reference check in one statement Greptile P1. Reading references in one statement and deleting in another left a window — a wide one, since a provider delete is a network call — where a second workspace could declare the same package list, inherit the `ready` row, and have its next run fail against a template already on its way out. Content addressing is what makes that reachable: the image is shared, so one workspace's delete can strand another's sandbox. The reference check now lives in the conditional DELETE itself, so winning the delete is the proof that nothing referenced the hash. A workspace that adopts the hash first makes the delete match nothing and the release becomes a no-op. Claiming the row before the provider call would otherwise strand a template nothing points at if the provider then refused, so that path puts the row back and the retention sweep inherits the retry — the same property the previous ordering had. The sweep is deliberately left as it is: its equivalent window needs a hash unreferenced AND unused for 30 days, and its provider-first ordering encodes the documented retry-on-refusal behaviour this path now reproduces explicitly. No transaction is opened. The provider call sits between discrete statements rather than inside one, so no pooled connection is held across it — which is why this uses a conditional delete instead of the repo's `pg_advisory_xact_lock` pattern, whose lock only releases at commit. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): route the retention sweep through the same image claim Cursor and Greptile both flagged the sweep as still carrying the interleaving just fixed in releaseSandboxImage, and they are right — the reason given for leaving it alone last round does not survive scrutiny. That reason was that provider-first ordering encodes retry-on-refusal, so making the claim atomic would trade a race for an orphaned template. The release path already answers that: claim the row, and put it back if the provider refuses. The sweep can have both properties too. The rarity argument was also weaker than stated. The sweep nominates up to 200 candidates and then works through them eight network deletes at a time, so its check-to-delete gap is seconds to minutes — wider than the window that was just closed, not narrower. Both callers now share `claimAndDeleteImage`, which owns the whole contract: the unreferenced check lives inside the DELETE, the provider call runs only after the claim succeeds, and a refusal restores the row. Having written that ordering twice is what let the two paths drift, so it exists once now. The sweep's query becomes a nomination step only. Its retention cutoff is passed into the claim rather than trusted from the earlier read, so a candidate that stops qualifying mid-sweep fails its claim and is skipped instead of losing its image. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): rebuild a hash adopted while its image was being deleted Greptile's third pass on this path, and a case the previous two did not cover: the adopter starting a *fresh build* rather than inheriting a ready row. Claiming removes the registry row, so between that and the provider delete finishing, a workspace can declare the same package list, get a new row, and start a build under the same content-derived imageRef — which the in-flight delete then removes. The window itself is inherent. The registry row and the provider template are two systems with no shared transaction, so it can be narrowed but not closed. A Redis lock would not close it either: acquireLock returns true when Redis is absent, so it cannot be a correctness guarantee for self-hosted. Holding a Postgres advisory lock would, but only by pinning a pooled connection for the length of a provider call, which is a worse trade. What was avoidable is the adopter finding out the slow way. Its row is new and healthy-looking, so nothing noticed: resolution only repairs a row that is missing or failed, and a failed one waits out the retry cooldown first. The release path now re-checks after the delete and re-enqueues, so the rebuild starts immediately instead of one failed run plus a cooldown later. A build already in flight is left to the conflict guard, since it may still outlive the delete. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): reclaim a ready row whose image was deleted underneath it Greptile found the hole the previous commit left, and it is the case that made the claim in that commit's message wrong: this one is permanent, not transient. If a re-adopted hash reaches `ready` before the in-flight provider delete lands — plausible, since E2B layer caching can rebuild an identical spec in seconds — the row looks healthy while its imageRef points at nothing. Resolution repairs a row that is missing or failed, never one claiming to be ready, so nothing recovers it. The sandbox stays broken until someone re-saves it by hand. `rebuildIfReadopted` called `ensureSandboxImage` with no options, whose conflict guard reclaims only a failed or stale in-flight row, so it silently did nothing in exactly that case. The release path now passes `imageKnownGone`, which widens the re-claim to any settled row rather than only a failed one. It is the one caller that knows the image is gone regardless of what the row says. An in-flight build is still left alone: it either recreates the template it was building or fails into the normal repair path, and resetting it would only add a duplicate build. The three ways a settled row may be re-claimed now sit in one `settledRebuildBranch` helper — any settled row when the image is known gone, a failed one after the cooldown for an automatic caller, a failed one immediately for a person — because inlining the third case is what hid the gap. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): let a same-spec save retry a failed build Cursor Bugbot. `scheduleSandboxBuild` sat inside the changed-hash branch, so a save that did not alter the package list never reached the registry. The comment above it described the opposite — that an unchanged spec finds a ready row and enqueues nothing — which is what `ensureSandboxImage` does, but only if it is called. That made the docs wrong too. They tell a reader to save the sandbox again to retry a failed build immediately, and this branch is exactly why that did nothing: the only way to retry was to edit the package list into a different hash, which is not what someone recovering from a transient registry failure wants to do. The call is now unconditional and the registry decides what a save costs, which is what its conflict guard is for: a ready or in-flight row is left alone, a failed one is re-claimed at once. Releasing the previous image stays behind the hash check, since only a changed hash orphans one. Cache invalidation is unchanged — `scheduleSandboxBuild` already does it, which is why the else branch existed. Co-Authored-By: Claude <noreply@anthropic.com> * docs(sandboxes): correct the image cache's staleness invariant Cursor Bugbot found that a released image can still be served from another replica's cache. The finding is real, and the reason it went unnoticed is that the cache documented an invariant which eager release quietly broke. It claimed a `ready` row is terminal for its spec hash, so a cached hit could not go stale in a way that matters. That held while the only ways a row changed were an edit (new hash) or a delete (caught by the `workspace_sandbox` read). Releasing an image eagerly made a `ready` row disappear with the hash unchanged, so the premise no longer holds and the comment was actively misleading to the next reader. No behaviour change here — the exposure is bounded at IMAGE_TTL_MS on replicas other than the one that ran the release, and it self-heals once the entry expires and the row read finds nothing. Closing it properly needs cross-replica invalidation or a provider-error path that invalidates on "template not found", both of which are larger than a review fix; the comment now says so instead of implying the problem cannot exist. Co-Authored-By: Claude <noreply@anthropic.com> * docs(sandboxes): note that a JavaScript sandbox needs an import to apply Cursor Bugbot pointed out that `useRemoteSandbox` keys on detected static import/require and never on the selected sandbox, so JavaScript without one runs locally and the selection has no effect. Keeping the behaviour: honouring the selection would force those blocks remote, and the large-value-ref guard immediately below would then reject code that runs fine today. Documenting it instead, next to the picker, since a selection that silently does nothing is only surprising if nothing says so. Python is unaffected — it always runs remotely, so its sandbox always applies. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): stop create mode surviving a return to an open sandbox Cursor Bugbot. Create mode and having a sandbox open are mutually exclusive, but nothing enforced it, so both could be set at once — and the screen then lied about which sandbox its Delete pointed at. With `isCreating` true and `selectedId` restored, `baseline` is null, so the editor renders an empty "New sandbox" form, while the Delete action is built from `selected` and still targets the restored sandbox. An admin looking at a blank create form could delete a sandbox it never named. Two ways in, both closed: - Browser Forward after starting a new sandbox restores `selectedId` without going through `closeEditor`. The render-time sync that already drops a stale draft now also leaves create mode, which is the same class of correction and the reason that block exists. - "New sandbox" set `isCreating` without clearing `selectedId`, so the same contradiction was reachable without touching history at all. It now clears the selection, with `history: 'replace'` because switching mode is not a destination. Co-Authored-By: Claude <noreply@anthropic.com> * fix(ci): pin the sandbox flag in the second nav catalog test, bump the chart Two CI failures, both mine. `app/workspace/[workspaceId]/settings/navigation.test.ts` asserts the unified catalog and was left on ambient env. Dropping the Sandboxes section without a sandbox provider made it 26 items instead of 27 on CI, which has no `apps/sim/.env` — the same trap already fixed in the sibling `components/settings/navigation.test.ts`, in the one file that was missed. Fixing it needs `vi.hoisted` rather than the sibling's `beforeEach`, because this file reads `allNavigationItems`, built once at module load; a hook would run after the value it is trying to influence already exists. The chart gate is separate: this branch adds sandbox settings to `helm/sim/values.yaml`, and the workflow requires a Chart.yaml bump whenever `helm/sim/**` changes. Additive config, so 1.3.0 -> 1.4.0 by SemVer. Verified by running the whole suite with the flags forced off, not just the two navigation files — no other test depends on a local env file. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): keep the row restore to a refused delete only Cursor and Greptile, independently, on the same code. `deleteImage` and `rebuildIfReadopted` shared one try/catch, so a rebuild failure after a *successful* provider delete was handled as if the provider had refused: the catch put the claimed row back, `ready` status and all, pointing at a template that no longer exists. That is the one state resolution cannot repair — it fixes a row that is missing or failed, never one claiming to be ready — so it reintroduced the permanent breakage an earlier commit had just closed, through the error path rather than the happy one. Restoring now belongs strictly to a refused delete. Once the template is gone the row stays gone, and the rebuild runs past that catch. The rebuild also swallows its own failures: it follows a delete that already succeeded, so it must not be reported as a failed release, and inside the sweep it must not reject the rest of its chunk. The adopter's next run still reaches the normal repair path. The regression test drives a rebuild failure and asserts no row is restored. It fails against the original shape — rebuild inside the shared try, no inner catch — which is what the two reviewers were describing. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): drop the dead row when a re-adopt rebuild cannot be scheduled Greptile, one layer under the previous fix. Making the post-delete rebuild swallow its own failures kept it from being reported as a failed release, but left the adopter's row claiming a `ready` image whose template is already deleted — the one state resolution cannot repair, since it rebuilds a row that is missing or failed and never one that says ready. So the row is now dropped when the rebuild does not take. That turns the adopter into the missing-row case, which the next execution repairs on its own, instead of a sandbox that stays broken until someone re-saves it by hand. A failure to drop it is logged at error, because at that point two writes in a row have failed and there is nothing further this path can do. Also gives the release tests a default "nothing re-adopted" select. Without it the rebuild threw on an unstubbed mock and the cleanup delete overwrote the predicate the claim assertions read, so two of them were passing on the wrong statement. Co-Authored-By: Claude <noreply@anthropic.com> * feat(sandboxes): repair a missing image at create, where the truth is observable Six review rounds narrowed the window between deleting a shared template and another workspace adopting its content hash, and each fix exposed the next facet. They all share a cause: the registry row and the provider template are two systems with no shared transaction, so any scheme that keeps them in step is guessing. Create is the one step that does not have to guess. It either gets a sandbox or it does not, so a `ready` row pointing at a deleted template now corrects itself the first time it is used, rather than needing someone to re-save the sandbox. - `SandboxImageBuilder.isMissingImage` asks the provider to classify its own failure. Prebuilt-only, because a runtime provider has no image to miss - E2B answers it off `NotFoundError`, which the SDK maps from a 404. The only resource a create names is the template, and the two subclasses that describe other calls — a missing file, an exited sandbox — are excluded. The classifier stays deliberately narrow: treating auth or rate-limit failures as a missing image would turn a provider outage into a build storm - `repairMissingSandboxImage` invalidates the cache, rebuilds with `imageKnownGone` (no cooldown, since this observed the image is gone rather than inferring it), and returns copy telling the author to run again - `ResolvedSandbox` carries `specHash` so the failing execution can name what to rebuild This subsumes the open facets rather than adding another guard beside them: the stale per-replica cache, an adopter left `ready` against a deleted ref, and a rebuild that never took all end at the same place — the next run repairs itself. Co-Authored-By: Claude <noreply@anthropic.com> * fix(sandboxes): key the build trigger by attempt, not by spec Cursor Bugbot. The Trigger.dev idempotency key was the content address alone, so a second attempt at the same spec was deduped against the first: the SDK returns the finished run instead of starting one, and the row that `ensureSandboxImage` just flipped to `pending` sits there with no worker. Nothing can re-claim a `pending` row until it goes stale, so a retry inside the 5-minute TTL did nothing for the next half hour. That silently disabled every repair path — save-to-retry, which the docs name explicitly, and both the resolution and create-time rebuilds. The key's own comment already said it exists "to collapse concurrent saves of the same spec into one build, not to suppress a retry after one failed". The conditional update above it is what actually collapses concurrent saves: only one caller gets a row back, so only one ever reaches the trigger. Keying by the claim's `updatedAt` keeps that property and makes each genuine attempt distinct, while a duplicate delivery of one attempt still collapses. Co-Authored-By: Claude <noreply@anthropic.com> * feat(sandboxes): create a sandbox from the picker, and fix three UI papercuts The Function block's sandbox field now pins a "Create Sandbox" row above its options, matching the "Create Skill" / "Create Tool" rows it sits beside, so authoring a package list no longer means leaving the workflow for Settings. The row is declared by the field (`createAction`) rather than hardcoded by id; block configs are read by the serializer and executor, so the name maps to a modal in the picker rather than carrying a component. Two things the modal has to get right. It seeds the new sandbox's language from the sibling the list is scoped by, or a sandbox created off a JavaScript block would land in the Python list and vanish. And the created option is held locally until a real fetch carries it, or the field would sit on a raw uuid until hydration answered. Also: - The Sandboxes icon was the Logs block's icon (`blocks/blocks/logs.ts`), in both the settings nav and the list rows. It is the Function block's now. - "Default image (no extra packages)" claimed something untrue: E2B and Daytona base images both ship with packages installed. - A new sandbox opened in Python while the Function block defaults to JavaScript. The test pins the two together rather than the literal. Draft shape and helpers moved out of the editor component into `utils.ts` — three consumers now, and it makes the defaults testable without a DOM. * feat(settings): one Max-plan wall, and give the create modal the same one The create-sandbox modal answered a non-Max workspace with a red line under a form it could never submit, and no way to act on it. It now renders the same wall the Settings > Sandboxes tab does — heading, one sentence on what the plan unlocks, and an Upgrade to Max chip — instead of the fields. That wall existed twice already (sandboxes and Sim Mailer), so this extracts it rather than adding a third copy. `SettingsUpgradeNotice` owns the copy rhythm and the route, and `compact` trades the page's full-height centering for a modal's. Both settings consumers now compose it; neither keeps its own markup. The action lands on billing, which `resolveSettingsHref` already redirects to the plan-comparison page for a member who cannot manage billing — so it is a route to explore plans, never a dead end. The chip stays hidden for non-admins, exactly as the settings pages had it. A non-admin on an entitled workspace gets the muted reason rather than the upgrade wall: buying a plan is not what is in their way. --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Vikhyath MondretiandClaude
|
0e4f0e733f |
fix(invitations): report every reason a batch of invites failed (#6093)
* fix(invitations): report every reason a batch of invites failed
The invite modal collapsed a multi-failure batch to `N invitations
failed.` plus the first reason and discarded the rest. The batch endpoint
rejects per email and each reason names its own address, so inviting
several people who are all ineligible for the chosen membership — the
common multi-failure case, since External requires every invitee to
already be on a paid plan — named one of them and hid the others. The
failed addresses are reseeded into the field as ordinary chips carrying
no error state, so nothing else on screen identified them either.
`buildInviteFailureMessage` now lists the distinct reasons, collapses
identical ones, and reports the denominator ("2 of 5 invitations could
not be sent") the way add-people-modal already did for the same partial
-failure shape. Past three reasons the tail is counted rather than
listed, so a large batch cannot bury the modal in near-identical
sentences.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(invitations): name the membership field for the org it acts on
The dropdown grants organization standing — Member and Admin join the
org and take a seat, External does not — but the label said only
"Membership", which reads as membership of the workspace being invited
to, the thing the field directly above it already controls. The EE
access-control modal has its own "Membership" field for permission-group
membership, so the bare word was ambiguous across surfaces too.
Sentence case matches the sibling "Workspace access" in the same form.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Waleed
|
fadff0e35f |
fix(integrations): keep the Atlassian coverage hint visible, and trim dead surface (#6105)
* fix(integrations): keep the Atlassian coverage hint visible, and trim comments Cursor Bugbot: `ChipModalField` hides a `hint` whenever that field shows an `error`, so the multi-product coverage sentence vanished the moment the domain format check fired — exactly when someone mid-form most needs it. The sentence also describes the token, not the domain, so it read as domain guidance. Moves it to the API token field, which surfaces its errors through `ChipModalError` at the bottom rather than its own `error` prop, so the hint cannot be displaced. Also drops rationale comments that restated the code they sat above. * chore(integrations): trim the credential-display barrel to what consumers use * fix(emcn): let a custom modal field associate its hint with the control it wraps `ChipModalField` computes `aria-required`/`aria-invalid`/`aria-describedby` from its own state, but `type='custom'` returned its children untouched — so the `hint` and `error` text it renders was visible and never announced. The field cannot apply the ARIA itself here: a custom child may be a bare input or a wrapper several levels above one, which is the same reason `associatesLabel` already excludes custom from the label's `htmlFor`. Adds a function form for custom children that receives the ARIA, so the consumer — which knows where focus lands — attaches it. Existing `ReactNode` children are unaffected; all five current custom-field call sites keep working untouched. Uses it for the Atlassian API token field, whose coverage hint this branch had just relocated onto a custom field. |
||
|
WaleedandWaleed Latif
|
c9547dcfe5 |
fix(desktop): keep page headers clear of the traffic lights (#6098)
* fix(desktop): keep page headers clear of the traffic lights In the desktop app, collapsing the sidebar hands the top-left of the screen to the content pane — the macOS traffic lights and the sidebar expander then sit over page content rather than over the sidebar. Every top-of-page header bar drew straight underneath them, so back buttons and the integrations/skills switcher were unreadable and unclickable. The reason it hit all of them at once is that the bar's geometry was copied verbatim into seven files, so nothing could reserve that lane in one place. Extract it as PAGE_HEADER_BAR and fold the lane into its top padding via `--workspace-content-title-bar-inset`, the variable the content pane already publishes for exactly this. That variable is 0px everywhere except the macOS desktop app with the sidebar collapsed, so this is inert on the web and on an expanded sidebar, where the lights sit over sidebar chrome that already reserves its own lane. Covers the settings shell, the credential/skill detail layout, the integrations and skills switcher, integration block detail and its fallback, and both upgrade headers. A guard test fails with the file named if any page re-derives the bar instead of composing it. * fix(desktop): cover the Resource header bar too The first pass found its surfaces by grepping one exact class string, which turned up the minority header. `ResourceHeader` is the same bar written as `px-4 py-[8.5px]`, and it is the one logs, files, tables, knowledge and scheduled tasks use — plus every loading fallback. Those all still drew under the traffic lights, and on the breadcrumb surfaces the occluded element is an interactive popover button, not just a title. Share the lane math as TITLE_BAR_LANE_PT and compose it from both bar geometries, which genuinely differ (the Resource bar is bordered and has a min height). Nothing nests one bar inside the other, so no reset is needed: the only out-of-pane render is the landing tables preview. Move the variable's `0px` default to `:root` alongside the other desktop-title-bar vars, so it is defined for bars outside `.workspace-content-shell` — the standalone settings shell and that landing preview — where an undefined var() inside calc() would be invalid at computed-value time and drop padding-top entirely. That replaces the per-call fallback, leaving one default instead of two that can drift. Fold the assertions into the existing desktop title-bar surface audit rather than a second audit file with its own conventions. The standalone guard is gone: it keyed on one spelling of the geometry, so it was blind to `px-4 py-[8.5px]` — the very re-derivation that made this pass necessary. * fix(desktop): cover fullscreen routes, spare embedded panels Review round 2 found the two arrangements the collapsed-sidebar selector alone gets wrong. A fullscreen route (/upgrade) slides the sidebar to zero width without collapsing it, so `data-sidebar-collapsed` is absent and the lane stayed zero while the pane was in fact sitting under the traffic lights. The pane owns the lane whenever the sidebar is not there to own it, so the selector now matches a new `data-content-fullscreen` as well. The mirror error: the variable is inherited, and the mothership panel is the right half of the pane — never under the lights — yet it embeds whole pages (KnowledgeBase and friends) whose header bars reserve the lane. Those bars were gaining ~38px inside the panel. The panel now zeroes the variable for its subtree. I had checked for exactly this nesting and concluded it did not exist, having looked only at the settings pages that import `Resource` types without rendering `Resource.Header`. `resource-content.tsx` renders the knowledge page itself, which does. An assertion each, both verified to fail when the fix is reverted. * fix(desktop): size the peek card to its content, soften the overlay shadow The floating sidebar pinned both its top and bottom edges, so it always drew at full window height. On a short surface — the settings list — that left a tall empty slab of card hanging below the last entry. The card now hugs its content and caps at the pane height less the traffic-light lane and the bottom gutter. Dropping the bottom pin is most of it: the four `h-full` rules down the chain resolve against an auto-height parent and collapse to content on their own. But nothing would then bound the sidebar's own `flex-1 overflow-y-auto` region, so a long workflow list would be clipped by the card's `overflow-hidden` instead of scrolling. The card is therefore a capped flex column, and the shell is allowed to shrink inside it, which restores a definite height for the chain to resolve against. That rule is scoped to `[data-peek]` and is inert while docked, where the shell is not a flex item. Also eases `--shadow-overlay` in both themes (alpha ~27% lighter, bloom pulled in from 48px to 30px), keeping it clearly above `--shadow-medium` so the scale still reads in order. The peek card drops the shadow entirely and separates on the same `--border` hairline the content pane beside it uses. * fix(desktop): stop the login page scrolling, drop the pane border at the window edge Two reports against the desktop window chrome. The login page scrolled by exactly the traffic-light lane. `.desktop-title-bar-page` reserved the lane with `margin-top` plus a `calc(100vh - lane)` height, which sums to the viewport on its own — but `body` carries `min-height: 100vh`, and body is a plain block box with no padding, border, or BFC, so that top margin had nothing to collapse against and collapsed through, displacing body itself. The document came out one full lane taller than the viewport while the shell's `calc` saving was re-inflated underneath it. Reserving the lane with padding *inside* the box removes both the collapse and the `calc`: global `box-sizing: border-box` keeps the padding within the `100vh`. Measured in the Electron renderer over CDP — 40px of overflow before, 0 after, with the logo still clear of the lane. Collapsing the sidebar also left a hairline outline traced just inside the window. The shell drops to `p-0` there, but the content pane kept its border and 8px radius, so both drew flush against the square window frame. The pane now drops them exactly when it is flush. Keyed off the ancestor attributes rather than React state, because the title-bar attribute is written pre-paint and a state-driven rule would flash the border before hydration settles. * docs(desktop): correct the peek card chrome comment The card's TSDoc still listed `shadow-overlay` as part of its chrome after the shadow was deliberately dropped, so the comment contradicted the code. It now records that the card is unshadowed on purpose, and documents the content-hugging height and the flex-column cap that make the sidebar's scroll region bound itself. --------- Co-authored-by: Waleed Latif <waleed@simstudio.ai> |
||
|
Waleed
|
811a39ec05 |
fix(integrations): show family service accounts on every product they authenticate (#6102)
* fix(integrations): show family service accounts on every product they authenticate
An Atlassian API token authenticates Jira, Jira Service Management, and
Confluence alike, so it is modeled as an `atlassian` pseudo-provider whose
only service is named "Atlassian Service Account". Every credential display
surface resolved through `getServiceConfigByProviderId`, which walks
OAUTH_PROVIDERS in declaration order — so the credential resolved to that
pseudo-service instead of to any product.
The result: adding a service account from the Jira page, through a modal
titled "Add Jira service account", produced a credential that appeared under
neither Jira, JSM, nor Confluence, was titled "Atlassian Service Account" on
its detail page, and lost its brand tile and category on the list. The same
bug hid a Google service account everywhere except Gmail.
- match credentials with `credentialProviderMatchesService`, which accepts a
service's OAuth id or its service-account id
- add `lib/integrations/credential-display.ts` as the single resolver for
catalog join, mark, and copy, replacing three duplicated lookups that keyed
the catalog by OAuth service *display name* — the reason the pseudo-service
fell off the map
- derive "family service account" from the catalog (a service-account id
serving >1 integration) rather than hardcoding vendors, so a new integration
joining a family needs no edit
- title service-account detail pages by credential name, subtitle them with
their reach, and state that reach up front on the connect form
- keep the service description as the detail subtitle for every non-family
credential, unchanged
No schema, migration, contract, or persisted value changes; resolution is
computed at render time from static config. Coverage for all 22 service-account
provider ids is pinned in tests, including that the index and the predicate the
Connected list filters on cannot drift apart.
* chore(icons): use Atlassian's gradient marks for Jira and Confluence
Replaces the flat #1868DB Jira and Confluence marks with Atlassian's gradient
versions, matching the Atlassian mark added alongside them.
- gradient ids go through `useId()` rather than the source SVGs' static ids,
which would collide wherever two of these icons render on one page — the
integrations list and the landing loops both do
- pads the Atlassian viewBox so its artwork fills ~78% of the box, matching the
inset Atlassian ships on the Jira and Confluence marks; without it the mark
renders ~30% heavier than its siblings in the same tile
Visual-only, but these marks render in ~60 files, so it is split from the
credential fix to stay independently revertable.
* fix(integrations): route the editor's service-account setup modal through the shared target
The workflow editor's credential selector passed the OAuth service's own name
and icon straight to ConnectServiceAccountModal, so opening the setup form from
a Jira block titled it "Add Jira service account" while the integrations page
and the chat — both of which already resolve through
`useServiceAccountConnectTarget` — titled the same form "Add Atlassian service
account".
That is the exact confusion this branch set out to remove, surviving on the one
surface that bypassed the shared resolver.
* docs(atlassian): correct the service-account setup path and cover all three products
The setup section could not be followed. It sent readers to a "Settings →
Integrations tab" that does not exist (Integrations is a top-level workspace
module) and told them to search the integrations list for "Atlassian Service
Account", which matches no catalog entry — the catalog lists Jira, Jira Service
Management, and Confluence.
The page also described the credential as covering "Jira and Confluence" while
listing Jira Service Management scopes, and the product now spells the coverage
out in the connect form.
- correct the path: Integrations -> Jira/JSM/Confluence -> Add to Sim -> Add
service account
- name all three products consistently, and state that one service account
covers them
- match the real button label ("Add service account")
|
||
|
Siddharth Ganesan
|
aeb7eae3f8 |
feat(model): sim auto model (#6103)
* Add sim-auto: automatic model routing for the agent block (hosted) - 'Auto' model option (Sim wordmark icon, hosted-only, new-block default; runtime fallback for unset models stays claude-sonnet-5) - Resolver (lib/model-router): classifies each execution via mothership's /api/model-router and routes over two ladders — text: fireworks/glm-5.2 -> fireworks/kimi-k3 (new static hosted Fireworks catalog entries on the platform FIREWORKS_API_KEY); attachments: claude-haiku-4-5 -> gpt-5.5 (Fireworks OSS endpoints reject images). Trivial tasks skip the router; 5-min decision cache; 2s timeout; never fails the workflow - Hidden identity preamble on every auto run (English by default, don't volunteer the underlying model) - Fireworks executor: wire-name map for catalog ids (glm-5.2 -> glm-5p2), pricing keyed on the full catalog id - Billing: routing cost applied to non-streaming output cost only when mothership marks the call billable (bill-model-router flag, default off) - sim-auto special-cases: API-key condition, serialization tool lookup, edit-workflow validation (hosted), VFS model options projection * auto model * agent auto model * fix lint |
||
|
WaleedandWaleed Latif
|
66478087f7 |
fix(sidebar): stop the workspace switcher stranding a phantom hover (#6096)
* fix(sidebar): stop the workspace switcher stranding a phantom hover The switcher's keyboard cursor is set from `onMouseMove` and only cleared when the menu closes, and it paints in `--surface-active` — the same token hover uses. So the last row the pointer crossed keeps a background indistinguishable from hover long after the pointer has gone, sitting alongside the equally-`--surface-active` current workspace as a second phantom-hovered row. It shows without any hovering too: the cursor is seeded to row 0 on open, so any user whose current workspace is not first sees two marked rows immediately. Only bites above the 3-workspace search threshold, which is why it went unnoticed. Paint the cursor only while the user is actually navigating by keyboard. Arrow keys enter that mode, any pointer motion leaves it, so in pointer mode the sole mark is real CSS :hover — which follows the pointer and leaves with it. `highlightedId` still tracks the pointer, so Enter keeps targeting the row last touched; only whether it is drawn changes. This is the pattern emcn's own popover already uses (`isKeyboardNav`, commented "prevent dual highlights") and that `tag-dropdown` consumes, and it matches how Headless UI models a combobox: one modality-driven focus marker, separate from the selected value. The list carries no `aria-activedescendant`/`aria-selected`, so the highlight is purely visual and nothing in the a11y contract changes. * test(sidebar): assert the keyboard cursor on a row that isn't selected Review round 1: the keyboard-positive assertions landed on the current workspace, which carries its own `isActive` fill, so they held whether or not the cursor was painted — the tests could not have caught deleting keyboard-cursor rendering. Navigate with ArrowUp instead, wrapping from the seeded first row to the last, and assert on that row. Verified both directions now: removing the modality gate reddens three tests, removing keyboard painting reddens two. * fix(sidebar): do not arm Enter without a visible target Review round 1: gating the cursor's paint on keyboard mode left Enter still acting on the seeded first row while that row was unmarked. The search field takes focus on open, so Enter could switch workspace with nothing shown as the target — emcn's popover instead holds its selection at -1 and ignores Enter until keyboard nav begins. Enter now acts only once a cursor is on screen, and typing counts as keyboard intent so the common "filter, then Enter" flow lands on a visible top result. Every path to Enter therefore has a marked target. --------- Co-authored-by: Waleed Latif <waleed@simstudio.ai> |
||
|
Waleed
|
d95127da67 | fix(settings): add bottom padding to settings sidebar (#6097) |