Skip to content

refactor(app): let each workspace command hook declare what it reads - #1

Open
Nedian0Brien wants to merge 41 commits into
mainfrom
claude/gifted-leavitt-fc4a23
Open

Nedian0Brien wants to merge 41 commits into
mainfrom
claude/gifted-leavitt-fc4a23

Conversation

@Nedian0Brien

Copy link
Copy Markdown
Owner

문제

DatabaseWorkspaceControllerContextRecord<string, any>라, 커맨드 훅 다섯 개(useDatabaseWorkspaceMutationCommands, …RecordCommands, …ViewCommands, …SchemaCommands, …BulkCommands)가 구조 분해하는 필드를 호출부가 실제로 넘겼는지 아무도 검사하지 않았습니다.

실제로 물렸습니다. useDatabaseWorkspaceMutationCommandsrefreshNow 의존성을 추가했을 때 호출부가 그걸 넘기지 않는데도 컴파일이 통과했고, refreshNow()가 promise .then 안에서 던지면서 생성 직후 refresh가 조용히 삼켜졌습니다.

변경

  • Record<string, any> 별칭을 없애고, 런타임이 넘길 수 있는 모든 의존성을 실제 타입으로 한 번만 적은 비공개 카탈로그 DatabaseWorkspaceControllerFields를 도입했습니다.
  • 각 훅은 자기가 구조 분해하는 필드만 Pick 한 컨텍스트 타입을 받습니다. 훅별 계약이 이름 목록으로 읽히고, 필드 타입은 한 곳에만 존재합니다.
  • runMutation, runMarkdownTable을 이름 있는 타입으로 뽑아 뮤테이션 훅이 그 타입으로 구현을 선언합니다. 계약과 구현이 어긋날 수 없습니다.
  • 엄격해진 타입이 드러낸 약 60개의 죽은 pass-through(훅이 이미 직접 import 하는 모듈 함수들, 어떤 훅도 읽지 않는 상태)를 호출부에서 걷어내고, 그것만 먹이던 import와 preflight 지역 변수도 함께 제거했습니다. 런타임 파일이 182줄 줄었습니다.

동작이 걸린 줄은 하나입니다. runMarkdownTabletypeof refreshNow === 'function' 가드와 "컨트롤러 컨텍스트가 Record<string, any>라서"라고 설명하던 주석을 지웠습니다. 그 설명은 이제 사실이 아니고, 호출부가 하나뿐이라 refreshNow 없이는 컴파일 자체가 되지 않아 가드는 도달 불가능합니다.

계약이 실제로 잡는지 확인

호출부를 일부러 망가뜨려 양방향으로 검증했습니다.

  • refreshNow를 빼면 → TS2345: Property 'refreshNow' is missing … but required in type 'DatabaseWorkspaceMutationCommandsContext' (원래 새어나갔던 그 버그)
  • 없는 필드를 넣으면 → TS2353: Object literal may only specify known properties

검증

  • bun run --filter @nedian0brien/synapsenote-app typecheck — 통과
  • biome check packages/app/src/components/ — 통과 (799 파일)
  • 데이터베이스 DOM 스위트를 파일 단위로 실행: DatabaseTableDialog 100/100, database-tests/* 7개, DatabaseTableInteractionLayer, DatabaseTableViewState, DatabaseRecordPageChrome, use-database-refresh-scheduler, database-read-model — 한 파일 빼고 전부 통과
  • 같은 8개 파일 배치를 변경본 / stash한 베이스라인으로 비교: 변경본 109 pass · 1 fail, 베이스라인 108 pass · 2 fail (베이스라인의 추가 실패는 평소 1초짜리 테스트의 189초 타임아웃 — 머신 부하)

유일하게 일관된 실패인 database-tests/DatabaseProperty.dom.test.tsx는 이 변경을 stash한 c8c78324에서도 똑같이 실패합니다. 테스트가 DatabasePropertyInsertPopover를 필수 prop sourceProperties 없이 렌더링하는데, .dom.test.tsxpackages/app/tsconfig.json의 exclude에 걸려 typecheck가 못 보는 같은 계열의 구멍입니다. 이 PR 범위 밖이라 손대지 않았습니다.

module-boundaries.test.ts의 크기 예산 실패(lib/database-mutations/database-property-commands.ts, 587 > 400)도 기존부터 있던, 이 PR이 건드리지 않는 파일의 문제입니다.

사용자에게 보이는 동작 변화가 없는 내부 타입 리팩터라 changeset은 넣지 않았습니다.

🤖 Generated with Claude Code

Nedian0Brien and others added 30 commits July 27, 2026 04:13
The v2 data plane started returning `storageCapabilities` from
/api/databases/describe, but the app parses that response with a strict
schema. Every describe therefore failed with an unrecognized key, which
the UI classified as `invalid_schema`, so every linked database view
rendered "Database setup needs attention" instead of its table.

Model the field explicitly in the describe client, defaulting it to an
empty list so an older server that does not send it still parses, and
pin the contract with a regression test that also validates the fixture
against the server's own response schema.

The inline creation placeholder is rewritten to mirror the loaded empty
inline surface block by block - same surface/table attribute hooks,
header and toolbar geometry, 280px title and 144px actions tracks, and
the ghost new-record row - so the create-to-ready swap no longer jumps.
The single-view tab strip is dropped because the real header omits it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DatabaseView.dom.test.tsx could not run at all: the slash-command hover
preview imports two PNGs, and while bun resolves a binary import on the
module graph's first pass, re-entering the graph re-resolved the same
file and parsed it as JavaScript, aborting the file with `Unexpected `
on the PNG magic bytes. Because the crash happened before any test ran,
the whole suite was silently absent from `check:database:interaction`.

Pin an explicit loader in a test preload, following the lingui macro
shim already registered there, and give the import the root-relative URL
Vite serves the asset at so assertions about asset URLs stay meaningful.

With the suite running again it exposed a leak it had been hiding: the
linked-view cache is a module-level Map backed by sessionStorage, and
the file's teardown never reset it, so a view remembered by an earlier
test seeded a later render with a stale projection. Reset it in
afterEach, as the other database DOM suites already do.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Splitting the inline database commands module gets it back inside its RFC
0002 budget: the saved-view routing and lifecycle commands move to a
sibling factory, mirroring the option-commands split, and the new
boundary is registered in the contract.

The DOM tier's failures were three separate classes, none of them a
product regression:

Stale expectations. A fixture built a definition literal without the
schema-defaulted `people`, crashing the comment surfaces. The peek's
dismiss control, the board's live region, and the share fixture's
owner/repo had all moved on without their assertions. A `claude`/`codex`
launch is now routed to CliChatSession, so the dock's tests stubbed a
surface that no longer renders it, and its tab carries the CLI label
rather than an ordinal. The baked-launch deny list is now derived from
OK_GATED_TOOL_NAMES instead of restated, since production already asks
for the two to be kept in lockstep and a literal drifted out of it.

Partial module mocks. Factories that list only the names a file stubs
drop everything else the real module exports, so `useConfigContext`,
`useOptionalPageList`, and `useOptionalDocumentContext` failed to link
and took three files down before a single test ran.

Process accumulation. `--isolate` gives each file a fresh global, but
~285 files shared one process and the tail ran measurably slower than
the head — surfacing as 30s timeouts and blown render budgets in files
that pass alone. The runner now bounds process lifetime by batching.

The tier runs 283 files green; the app unit tier and lint are clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two defects that both showed up as "the editor ate my text".

Soft wraps were being rewritten as hard breaks. HardBreak is the schema's
`linebreakReplacement`, so every literal newline inside a paragraph comes
back as a hardBreak built from bare defaults whenever ProseMirror
re-parses a block it just touched — and the default style was
`backslash`. Opening a document with a `<DatabaseView />` in it was
enough: `covers\nyour change:` became `covers\` on disk, with no edit.
Default to `soft` and serialize that as the newline it came from; breaks
that really are hard breaks still carry an explicit style, stamped by the
Markdown parser, by `parseHTML` for authored `<br>`, and by the
Shift+Enter shortcut.

Paragraphs went blank unless the caret was inside them. Every top-level
block got `content-visibility: auto`, in every document, regardless of
size — and a skipped block only paints while the browser considers it
relevant, which focus is one way to become. Gate the chunk decoration on
the large-document threshold the rest of the editor already uses, so only
the documents this was built for pay for it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`refresh('transaction')` is a full `rebuild()`: a re-scan of the whole
content tree plus a re-read of every Markdown file in it, so wikilinks
resolve globally. The writer called it after every single-row write, and
paid it twice — the watcher events for the writer's own files re-armed
the drain loop, so one row insert triggered two full rebuilds and left
the table 503-ing `index is not current` at the client until they
finished.

The writer knows exactly which files it wrote, so it now hands them to
the coordinator, which routes them through the same incremental path the
watcher already uses for external edits. A write that also moves the
manifest still rebuilds: the store itself changed, not just a row. When
a canonical rebuild is already in flight the coordinator awaits that
instead, which subsumes the write and keeps the caller's guarantee that
the index is current once the refresh resolves.

Measured on a monorepo-sized workspace, one row insert: 4689ms -> 2863ms,
with the client's stale-index retries dropping from 6 to 0.

The remaining time is a second copy of the same problem: the writer's own
`#listDocumentCandidates()` walks the whole content tree — `node_modules`
included — to build wikilink resolution candidates, 2469ms of the 2863ms.
The index already maintains that candidate set incrementally; making it
the single owner is the follow-up.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…walk

`#resolveRow` built its `[[wikilink]]` candidate set by walking the whole
content tree and reading every Markdown file in it — `node_modules`
included, since the walk skips only symlinks. That was 2469ms of the
2863ms a one-row insert still cost after the index refresh was made
incremental: the same full-workspace read the refresh had just stopped
doing, in the other half of the write path.

The index already discovers identity-bearing documents and keeps them
current, so expose them and let the writer resolve from there. The walk
stays as the fallback for the one case that means the supplied set was
short — a link that will not resolve. `record_not_found` deliberately
does not fall back: it is the normal answer when creating a row, and
retrying it was re-reading the workspace on the common path.

One row insert on a monorepo-sized workspace: 2863ms -> 31ms server-side,
and the row lands in the table in 373ms instead of 8123ms.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The database record index discovered Markdown documents by walking the
whole content tree with no skip list. On a repo-root content dir that
meant reading every `node_modules/**/README.md`: 4417 files instead of
423 here, ~90% of them dependency docs.

That is a correctness bug before it is a cost one — each of those files
became a wikilink resolution candidate, so `[[README]]` could bind a
database row to a package's docs. Pruning the same ALWAYS_SKIP floor the
content walker already enforces fixes both.

Only directories discovered beneath the configured source root are
checked, never the root itself, so a database deliberately rooted at one
of these names still indexes.

Startup index rebuild: 22207ms -> ~180ms.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`commitWipInner` seeded a temporary index with `read-tree` and deleted it
in `finally`, so every WIP commit started from an index with empty stat
data. `git add --all` then had to open and hash every file in the work
tree to decide what changed — measured here at ~650ms cold against ~20ms
once the index is stat-warm.

Keep the per-writer index file and skip the re-seed when it already holds
exactly the ref's tree, which it does right after the write-tree that
produced the commit the ref now points at. Any writer that moves the ref,
a missing index file, a fresh process, or any error all fall back to
seeding, so the cache can only save work — it never decides what gets
committed.

Database creation commits two shadow snapshots; both drop from
~1100ms/~610ms to ~250ms/~330ms. Document saves take the same path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A database transaction writes the manifest and then refreshes the index
itself. The file watcher reported that same write moments later and
`requestDatabaseManifestRefresh` fired unconditionally, so every
transaction paid a SECOND full rebuild — a whole-content-tree rescan, to
re-read a manifest the index had just read.

Compare the store revision on disk against the one the index built
against: equal means the event is the echo of a write already indexed.
Anything else — an external edit, a Git checkout, a manifest repaired by
hand — differs, and still rebuilds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`markdown-table-record.ts` and `markdown-table-migration.ts` each carried
half of the same partition — 16 stored types in one, their 8 derived
complements in the other — with nothing tying them together. A third
caller now needs it, so lift it to one exported set plus
`isStoredDatabasePropertyType` and `databaseStoredPropertyIds`.

`databaseStoredPropertyIds` derives the column order from `properties`
rather than reading `storage.storedPropertyIds`, because a desired state
arrives carrying the previous storage block verbatim: clients edit
`properties` and leave `storage` alone, so the stored field cannot answer
"did the columns change?".

A test asserts the two halves still cover every declared property type, so
a newly added type cannot land in neither.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The planner refused EVERY manifest update on a v2 database:

    if (byId?.version === 2 && manifestAction === 'update') → conflict

Only creation and row-level writes survived that. Adding, renaming,
removing or reordering a property; adding a view; changing a filter, sort,
layout or projection; renaming the database — all of it returned
`source_record_migration_required` and could not be committed. Every v2
database has been schema-frozen since edbd7ac added the guard, which is
most of what the database surface does.

The guard was aimed at a real hazard, just far too broadly: only a change
to which properties occupy owner-table COLUMNS needs the Markdown table
transaction boundary. Derived property types (formula, rollup, the
created/last-edited metadata, verification, button) and every view edit
leave the owner table byte-identical, and for those the manifest writer is
the correct and only writer.

Narrow it to compare the DERIVED stored-property set on both sides, and
report the conflict per affected source rather than for the database.
Rewriting the table for a genuine column change is the next step; until it
lands those cases still refuse, now with a message that says column.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three pickers — the table header popover, the Manage properties dialog,
and the inline block popover — each hardcoded their own copy of the same
eleven types. Their shared comment explained the limit honestly: those
were the types that construct with no extra required configuration.

Replace all three with one grouped list beside the seeder that must be
able to complete each entry, and extend it with the seven types that
qualify today: person, unique_id, verification, and the four
created/last-edited metadata types. Notion's grouping (Basic / Advanced /
Record metadata) makes the longer list scannable.

`unique_id` needs `prefix` and `nextNumber`, neither of which has a schema
default; it seeds an empty prefix (Notion's own default rendering) and
`DatabaseUniqueIdPropertyDialog` edits it afterwards.

The test validates each entry's seed against the desired-state draft
schema the command actually posts — the contract that would otherwise
only be checked at commit time.

Still missing: status, button, relation, formula, rollup. Each needs
either a config editor that does not exist yet or a target picker, and a
type that can be created but not reconfigured is worse than one that is
absent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The format layer could replace a cell, a row, or the marker, but had no
way to change the COLUMN set — the one edit a schema change needs. Add
`reshapeDatabaseMarkdownOwnerColumns`, which rewrites the marker's
`columns=`, the header, the delimiter, and every data row in one pass.

Cells follow their property rather than their position, so an add or a
reorder is value-preserving and removal is the only lossy shape. Bytes
outside the marker and the table are untouched, so prose around the block
survives.

Two details the tests pin down because they are easy to get wrong:
`markerRange` spans the line ending that closes the marker while the
serializer does not emit it (dropping it would delete the blank line
before the table), and cells are sliced from `valueRange` rather than
read from `cell.raw` (which carries source padding) or `cell.value`
(already GFM-unescaped, so re-emitting it would change the value).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adding or removing a stored property was still refused after the guard
narrowed, because nothing rewrote the owner table to match — which is
every property type people actually add: text, number, select, person,
date, files, relation, and the rest.

Plan the table rewrite alongside the manifest instead. The normalizer
already derives `storage.storedPropertyIds` from `properties`, so the
manifest half was correct all along; the missing half was the file. Both
now land in the same plan, so a commit cannot move one without the other
and leave the marker disagreeing with the manifest.

An owner table that cannot be read or parsed still refuses, now naming
the file and the reason instead of blanket-refusing every column change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nedian0Brien and others added 11 commits July 28, 2026 19:56
Removing a property dropped it from `properties` and left every reference
to it behind, so the manifest schema refused the result:

    V2 stored property "prop_…" is not defined by this source
    View references property "prop_…" outside source "ds_…"

Add `pruneDatabasePropertyReferences` in core, next to the schema that
defines the reference surface, and use it from the removal command.

It prunes only the sites where the property's absence has one sensible
answer — the owner-table column set, the view projection, sorts, groups,
table column widths, agent write grants. It deliberately leaves `where`
filters, conditional colours, and layout-required properties alone:
silently dropping a filter changes which rows a view shows, and that is
the user's call, not a repair to make on their behalf. Those still
surface as a refusal naming the view.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
These three were the parity gap that mattered: a complete formula engine
(parser, evaluator, dependency graph, ~4,400 lines), a rollup aggregator,
and a relation resolver all shipped, plus a 733-line editor dialog for
formula and rollup that could only ever open for a property that already
existed — and nothing could create one. The editor was unreachable from
any database made in the app.

Each type seeds a valid, inert starting point and relies on its editor to
refine it, the way Notion behaves when you pick Formula:

  - relation targets its own source. A relation can only point at a source
    of the SAME database, so that is the one target guaranteed to exist,
    and a self-relation is also the shape Notion's sub-items use.
  - formula compiles `""` — an empty text literal, valid and evaluable.
  - rollup counts through the first relation, since `count_all` ignores
    the target value and so means something before anything is chosen.

Rollup is offered only once the source has a relation to summarise
through; with none there is no default that would mean anything, so
`databaseAddablePropertyGroups` hides it rather than handing back a column
the user cannot complete.

The seeder now takes the database and source it is seeding into. Making
those required rather than optional is what turned the three call sites
into compile errors instead of runtime ones.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A relation could only point at a source of its own database, so a workspace
made of separate databases could not link them — the thing Notion relations
are mostly for. Three predicates held it there, all of them checks rather
than mechanism:

  - schema.ts required targetSourceId to be in the same definition
  - database-record-index.ts matched bindings on `database.id`
  - database-plan.ts required targetDatabaseId === definition.id

Storage never had the restriction. A v2 relation cell is a wikilink to a
DOCUMENT, and the index's document map already spans every database, so a
cross-database target needs no new resolution — only a wider predicate.

Add an optional `targetDatabaseId`. Absent means the same database, which
is exactly what every relation written until now says, so no manifest
changes meaning. It is named rather than inferred from a global source
lookup so a manifest still describes what it points at when the other
database is gone.

Validation moves rather than disappears: one manifest cannot confirm a
target it does not contain, so the planner — the first layer that sees
every database — now refuses a missing target database or source.

Two-way pairing stays same-database. The paired side lives in the other
database's manifest, so keeping both ends consistent needs a transaction
across two manifests; until that exists a cross-database relation is
one-way, which Notion also offers.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Choosing Relation now offers every database in the workspace, defaulting
to "This database (sub-items)" — the self-relation that was the only
possible shape before. Without this the cross-database support would have
repeated the pattern this whole effort exists to remove: a capability the
engine has and no human surface reaches.

The catalog loads in the popover, the way `InlineDatabasePicker` already
does, and only once the user actually reaches for a relation — every other
type needs no target. A catalog that fails to load does not block
creation; the seed falls back to the self-relation, which is always valid.

The database and source travel as one Select value, so they are joined and
split through named helpers rather than an inline separator literal: a
mangled separator would have silently resolved every chosen target back to
this source, which is exactly the kind of failure that looks like it works.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Status was the last common Notion property with no way to create it. It
needed no new editor in the end: its options ARE select options with a
group attached, so widening the existing option engine reaches them, and
the property menu's option surface follows.

The seed is just key/name/type — the planner already fills in the default
To-do / In progress / Complete blueprint when groups and options are
omitted, which is exactly Notion's default board.

One rule is genuinely status-only: the manifest requires each of the three
fixed groups to keep at least one option, so delete and merge can both
produce a schema the server refuses. Both now report `last_group_option`
during preview, where the user can still choose differently, instead of
failing at commit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Button was the last of the 24 property types the manifest describes and
the pickers could not reach. It is also the only one that needed a new
editor: its config is a list of steps over a discriminated union, so
there was nothing existing to widen.

The seed creates a record in its own source — Notion's archetype, and
the one default whose effect is visible on the first click. It fills the
required Title because the manifest refuses a create step that does not,
and refuses to seed at all when some other required property has no
default, rather than committing a Button the server would reject.

The editor validates by rebuilding the definition and parsing it, not by
restating the rules: a Button's legality is not local (operands must be
writable properties of THIS source, a create step must satisfy the
target source's required properties, webhook steps must follow database
steps), so parsing the candidate is both the complete check and exactly
what the server runs on commit. Switching a step's kind replaces the
action instead of merging into it, since the members share only `id`.

Two things the editor deliberately does not offer: link/unlink operands,
which name a record by ID and want the relation picker; and webhook
steps, which name a `conn_` connection no surface in the app issues.
Both are preserved when present and removable, so nothing already in a
manifest is silently dropped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured first. `git --version` — which touches no repository at all —
costs 10-36ms here depending on load, and that is the floor for every
step `commitWip` takes. Of its six git processes, `add` was the only one
whose work exceeded its own spawn: rev-parse, write-tree and commit-tree
each cost about what an empty git invocation costs.

Two of the six existed only to re-derive a SHA the same process had
written moments earlier. WIP refs are always loose, so their value is a
40-byte file: reading it costs 0.01ms against ~12ms for `rev-parse`, and
it yields both facts the two calls were after — that the index stat
cache is still valid, and what the new commit's parent is. Any mismatch,
a packed or symbolic ref, a missing index, or any error falls back to
the spawning path, so the shortcut can only save work.

Reading the ref off disk widens the window between observing the head
and moving it, so `update-ref` now passes the expected old value and
fails loudly instead of silently overwriting a ref another process
advanced. That is stricter than what it replaced.

`reuseUnchanged` then lets a caller take the ref's existing head when
the work tree has not moved, rather than writing an empty commit over an
identical tree. It is off by default — a checkpoint records an act of
saving, and a caller listing checkpoints would lose an entry. The base
snapshot of a database transaction wants a handle on the current state,
not an entry, so it opts in.

  base snapshot     149ms -> 76ms
  transaction pair  299ms -> 224ms   (424-file vault, median of 9)

Also collapses the two byte-identical `CommitGit` builders in
database-commit.ts into one, which is what made the opt-in a one-line
change rather than two.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`scheduleRefresh` cleared its pending timer and set a new one on every
call, making it a trailing debounce. A commit does not produce one
invalidation — it produces the local success callback, the collaboration
broadcast, and whatever the resulting file writes echo back — so each
arrival inside the 75ms window pushed the refresh further out. Nothing
bounded that: invalidations arriving faster than the window defer the
table's read indefinitely.

Holding the deadline the first invalidation set collapses the same set
of equivalent invalidations into one generation, which is what the hook
documents itself as doing, and bounds the wait at 75ms.

Found while measuring a one-row insert, but NOT the cause of its
latency: with and without this change the row still appears at ~1075ms
(three samples each, same vault). The dominant wait is elsewhere — the
describe that finally refreshes the table is issued from a React passive
MOUNT effect ~900ms in, so the read model is being remounted rather than
re-keyed. This is a latent starvation bug fixed on the way past, and the
test fails without it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`SkillsSidebarSection` was the only always-mounted `useSkills` consumer,
and it never passed `enabled`. The hook re-fetches on the CC1 `files`
signal, which every file write raises — including the writes a database
row insert makes — so each insert refreshed the whole skill catalog for
a list that is collapsed by default and that nobody is looking at.

The section now owns its open state and gates the hook on it. `enabled`
already existed for exactly this case; `use-reconcile-skill-tabs` uses
it the same way. The other three callers are already conditional:
`NewSkillDialog` mounts its body only while open, and the settings and
editor callers mount with their surfaces.

Measured on a row insert: `/api/skills` drops out of the post-commit
request wave entirely (5 requests to 4). Its isolated cost is ~5ms — the
160-220ms it showed in the wave was contention with the four requests
beside it, so the gain is one less request competing, not 200ms back.
Expanding the section still loads the list on demand.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The refresh scheduler's 75ms window exists to merge a mutation's local
success callback with the collaboration broadcast that follows it. That
is right when the change is already on screen — an edited cell renders
optimistically, so the wait costs nothing. A created row has nothing to
render until the read lands, so the same wait is the entire latency the
user sees.

`runMarkdownTable` takes an `immediate` flag that uses the scheduler's
existing `refreshNow` instead, and row creation passes it.

Not an optimistic insert. The view for this table has `sort: []`, so the
server returns rows in index order, which the client cannot predict — an
optimistically appended row would land in the wrong place often enough
to visibly jump when the authoritative query arrives. The row still
comes from the server; it just stops waiting on a window it has no
reason to be in.

`refreshNow` is guarded with a typeof check because
`DatabaseWorkspaceControllerContext` is `Record<string, any>`: the first
version of this change compiled with the call site never passing
`refreshNow`, and the resulting throw inside `.then` swallowed the
create's refresh entirely. The guard degrades to the coalesced path
rather than losing the refresh.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`DatabaseWorkspaceControllerContext` was `Record<string, any>`, so the five
command hooks destructured a bag no call site had to satisfy. Adding a
dependency to a hook compiled even when the runtime never passed it: that is
how `refreshNow` reached `runMarkdownTable` as `undefined` and threw inside a
`.then`, swallowing the post-create refresh.

Replace the alias with one field catalogue typed once, and give each hook the
`Pick` of it that matches what the hook actually destructures. Omitting a field
is now TS2345 at the call site and a stray field is TS2353.

The stricter type exposed roughly sixty dead pass-throughs — module functions
the hooks already import directly, plus state no hook reads — so they and the
imports and preflight locals that only fed them are gone. `runMarkdownTable`
also loses its `typeof refreshNow === 'function'` fallback, which existed only
because the old type could not promise the field.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant