onLoadDocument rebuilds a legacy page (page.content, no page.ydoc) into a Yjs
doc and seeds its 'title' fragment from the page.title column. Both
TiptapTransformer.toYdoc and buildTitleSeedYdoc mint fresh Yjs client-ids on
every call, so the heal must run exactly once per page. Three holes let it run
twice (or lose a write):
- Duplication trap: the initial page read took no row lock, so two processes
(the API process via openDirectConnection and the standalone collab process)
could both observe ydoc IS NULL and each rebuild with different client-ids; a
long-offline client merging an earlier rebuild then duplicates all content.
- Lost-update: persistYdoc wrote updatePage({ydoc}) outside any transaction, so
it could clobber a concurrent onStoreDocument write (which does take a lock).
- Swallowed write errors: a failed heal-persist was logged but the unpersisted
fresh-client-id doc was returned anyway, silently re-arming the trap.
Fix: the heal now runs in healUnderLock, which re-reads the row FOR UPDATE inside
one transaction and re-validates under the lock — if ydoc is now present it
adopts it (no rebuild, no write), otherwise it rebuilds, seeds, and persists the
ydoc in the SAME transaction. The healthy hot path still loads with no lock and
no write. Failure handling surfaces instead of hiding: a rebuild-persist failure
refuses the load (re-throw + error log) so an unpersisted rebuild is never handed
out, while a seed-only persist failure serves the existing healthy ydoc without
the unpersisted seed (non-fatal). Removed the non-transactional persistYdoc.
Deliberately does NOT use a fixed clientID: identical client-ids across docs
built from differing content violate Yjs per-actor uniqueness and corrupt worse
than the trap; serialization under the row lock is the correct fix.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Title now lives in the page's Yjs 'title' fragment, but two paths corrupted it:
- Rename-revert: a REST/MCP title change wrote only the page.title column,
never the Yjs fragment, so the next editor open replayed the stale Yjs title
and reverted the rename. PageService.update now mirrors the new title into the
Yjs 'title' fragment via CollaborationGateway.writePageTitle, which goes
through openDirectConnection directly (Redis-independent: works with
COLLAB_DISABLE_REDIS and in single-process deployments, unlike the
Redis-routed handleYjsEvent path). The write is best-effort: a Yjs failure is
logged and never rolls back the committed column write. Agent provenance
(actor/aiChatId) is threaded into the store context.
- Untitled-on-open: an empty/just-initialized 'title' fragment clobbered a
non-empty page.title to '' on open. onStoreDocument now treats the title as
changed only when the extracted text is non-empty, covering both the
title-only and body+title save branches. Empty-retitling via collab is
intentionally impossible; the REST DTO is the place to enforce non-empty.
writeTitleFragment does a full clear+seed of the 'title' fragment (no
duplication/concatenation) and leaves the body fragment intact. Removed the dead
useTreeMutation.handleRename path. Adds unit tests for writeTitleFragment, the
gateway write, the anti-empty-clobber guard, and agent provenance.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Carries the still-applicable findings from the PR #116 review into PR #120,
since #120 includes the mobile-bootstrap commit. CORS hardening (removing the
unconditional localhost/capacitor origins) is intentionally left out of scope.
Service worker routing (latent bug fix + testability):
- vite.config.ts: anchor Workbox path matching to a segment boundary
(^/<seg>(/|$)) instead of startsWith, so siblings like /apidocs,
/collaborators, /socket.iox are no longer mis-routed as API/realtime and
forced NetworkOnly; align navigateFallbackDenylist with the same anchors.
- new apps/client/src/pwa/sw-strategy.ts holds the canonical predicates
(isApiPath, isCollabOrSocketPath) + unit tests; the vite.config regexes
mirror it inline (Workbox generateSW serializes urlPattern fns standalone,
so they cannot import the module).
Server CORS (R1 extraction + coverage):
- extract buildCorsAllowlist / isOriginAllowed into cors.util.ts with unit
tests (evil-origin rejected, WebView/no-Origin allowed); main.ts rewired to
use them with byte-for-byte identical behavior.
Privacy — clear offline cache on logout:
- new clear-offline-cache.ts purges the persisted query cache
(idb-keyval gitmost-rq-cache), the Yjs page.* IndexedDB databases, and the
service-worker api-get-cache; wired into handleLogout (best-effort, before
the redirect) so a previous user's private data does not linger locally.
Conventions & docs:
- prettier fixes on main.ts and login.dto.ts.
- CHANGELOG: document offline reading, returnToken opt-in, optional Swagger,
new env vars, logout cache-clear, and the CORS open->allowlist breaking
change.
- docs/mobile-app-plan.md: correct the now-false §2.4 claims and update the
§12 checklist (native cap add ios left unchecked — generated locally,
gitignored).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR #120 rewrote auth.controller.spec.ts and environment.service.spec.ts in a
leaner style but dropped several edge cases that PR #116 covered. Port the
gaps so the server coverage matches the original review intent:
- auth.controller: returnToken=false must behave like the omitted case
(no token in the response body, cookie still set) — guards an
`!== undefined`-style regression.
- environment.getCorsAllowedOrigins: empty string -> [], single origin,
and leading/trailing/duplicate commas with spaces -> trimmed list.
- environment.isSwaggerEnabled: mixed-case "True" -> true; "false"/""/"1"
-> false.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds the unit tests called out in the PR #120 review (test-coverage
aspect). No production logic changes — the only non-test edit is exporting
the already-injectable warmInfiniteAll helper so it can be unit tested.
Server (Jest):
- persistence.extension.spec.ts: onStoreDocument classification matrix
(no-op / title-only / body+title / body-only), onLoadDocument seed +
persist gating (early-return, page-null, ydoc seed, already-seeded
no-persist, legacy content->ydoc), and seedTitleFragment 4-branch guard.
- collaboration.util.spec.ts: buildTitleSeedYdoc round-trip.
- environment.service.spec.ts: getCorsAllowedOrigins / isSwaggerEnabled.
- auth.controller.spec.ts: login returnToken opt-in branch.
Client (Vitest):
- query-persister.test.ts: shouldDehydrateOfflineQuery status + allowlist
gates and OFFLINE_PERSIST_ROOTS membership.
- is-capacitor.test.ts: isCapacitorNativePlatform platform detection.
- make-offline.test.ts: warmInfiniteAll cursor walk / maxPages / error
swallow, and warmPageYdoc settle-once + timeout + teardown.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The mobile bootstrap shipped a hand-written public/sw.js plus a manual
navigator.serviceWorker.register('/sw.js') in main.tsx. The offline-sync
Workbox SW (vite-plugin-pwa, generateSW) functionally supersedes it
(NetworkOnly for /api,/collab,/socket.io, navigateFallback to the app shell,
runtime caching) and adds precache + prompt-based updates, so:
- Remove the hand-written apps/client/public/sw.js.
- Remove the manual SW registration block from main.tsx; registration is now
owned by <PwaUpdatePrompt/> via useRegisterSW (skipped in Capacitor native).
- Regenerate pnpm-lock.yaml for the merged Capacitor + @nestjs/swagger deps.
Kept from mobile-app-bootstrap: the richer manifest.json (offline-sync uses
manifest:false), capacitor.config.ts, the apple-touch-icon, and all server
mobile-auth/CORS/Swagger changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Implements the §12 bootstrap from docs/mobile-app-plan.md.
Backend (§6):
- auth: optional returnToken flag on login returns the JWT in the body
(data.authToken) for native Keychain/Keystore + Bearer; web cookie flow
unchanged.
- main.ts: explicit CORS allowlist (APP_URL + CORS_ALLOWED_ORIGINS env +
Capacitor WebView origins), credentials enabled, replaces open enableCors().
- optional OpenAPI/Swagger at /api/docs behind SWAGGER_ENABLED.
- env: CORS_ALLOWED_ORIGINS, SWAGGER_ENABLED, CAP_SERVER_URL.
PWA:
- manifest metadata, hand-rolled service worker (network-first nav, SWR
assets, never intercepts /api,/socket.io,/collab), prod-only registration,
apple-touch-icon.
Capacitor:
- capacitor.config.ts (webDir apps/client/dist; iOS via CAP_SERVER_URL to
avoid bundling the AGPL client in the .ipa, see plan §9), cap:* scripts,
deps, .gitignore for native dirs.
- docs/mobile-bootstrap.md documenting what is done and the remaining manual
steps (cap add ios/android, APNs/FCM, stores).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Implements docs/offline-sync-plan.md milestones M0–M2.
M0 (PWA shell):
- Add vite-plugin-pwa (generateSW, registerType: 'prompt', manifest:false);
NetworkOnly for /api,/collab,/socket.io, NetworkFirst for GET /api,
navigateFallback to index.html.
- Register SW via useRegisterSW with a Mantine update prompt; skip
registration inside Capacitor native WebView (is-capacitor guard).
M1 (harden CRDT body + title into Yjs):
- Lift the per-page Y.Doc/Hocuspocus providers into a shared hook+context so
body and title editors share one doc.
- Move the page title into a dedicated 'title' Yjs fragment (CRDT, offline-
tolerant); drop the REST title save. Server persists the title fragment to
page.title and seeds it for legacy pages (empty-fragment guard); a collab
rename emits a treeUpdate so other users' tree/breadcrumbs refresh.
- Persist the rebuilt ydoc on the content->ydoc path to neutralize the Yjs
duplication trap. Add a 3-state sync indicator.
M2 (offline read/navigation):
- Persist React Query to IndexedDB (idb-keyval persister, version buster,
selected roots only).
- "Make available offline" action warms page, space, tree (root+ancestors+
children) and comments under exact hook keys, plus the page ydoc.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
develop merged 20260626T130000-share-aliases; rename this PR's migration to
20260626T140000 so the two no longer share a timestamp prefix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Port the only substantive fix#211 was missing relative to #203 (which is
being closed): the "Send now" handler branched on the closure-captured
isStreaming, but a turn can finish between render and click. In that window
stop() is a no-op, so arming flushOnAbortRef/interruptNextSendRef would strand
those one-shot flags and leak into a later, unrelated Stop (auto-sending a
queued message the user never asked to send).
- Mirror the live useChat status in statusRef (updated each render) and branch
sendNow on it instead of isStreaming, so the not-streaming path runs when the
turn has already ended and the interrupt flags are never armed against a
no-op stop().
- Belt-and-suspenders: clear flushOnAbortRef/interruptNextSendRef when a new
turn starts streaming, defusing the sub-render-tick window where a flag could
still be armed but the expected abort never fired. No-op for the legit
interrupt path (both refs are consumed synchronously beforehand).
Keeps #211's existing structure and its flushNext-returns-boolean fix. The
rest of #203's divergence is comment rewording, a server-side rename of the
same pure interrupt-gate, and fewer tests — nothing else to port.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Register the new AI-chat keys "Send now" and "Interrupt and send now" in
both en-US and ru-RU catalogs so the UI never renders mixed-language
tooltip/aria-label (i18n policy).
- Make INTERRUPT_NOTE module-private (drop the unused re-export), matching the
module's private DEFAULT_PROMPT/SAFETY_FRAMEWORK siblings.
- Reset interruptNextSendRef in the flush-on-abort branch when nothing is
actually sent, so a stuck one-shot interrupt flag cannot tag the next
unrelated send; flushNext now reports whether it sent.
- Add a CHANGELOG [Unreleased]/Added entry for #198.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The deletion guard skips a note when its re-read deadline is still in the
future (user disarmed-then-re-armed in the race window between the batch
SELECT and the per-row re-read). The default stub returns an epoch deadline
(always < now), so the existing race tests never exercised the
`new Date(temporaryExpiresAt) >= now` branch; a regression dropping it or
inverting the comparison would pass unnoticed. Add a test that re-reads a
fresh future deadline and asserts removePage is not called.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Must-fix:
- CHANGELOG: add [Unreleased]/Added entry for temporary notes (#201).
- temporary-note-cleanup: re-check temporary_expires_at at deletion time so a
concurrent "Make permanent" (sets it NULL) between the batch SELECT and the
per-row removePage wins the race and the note is not trashed. Add unit tests
for the make-permanent and already-trashed race windows.
Non-blocking review items:
- temporary-note-cleanup: cap the sweep batch (LIMIT 500) so a large backlog is
not loaded into memory; remainder drains on the next hourly run.
- client: extract duplicated post-toggle cache sync into
syncTemporaryExpiresInCache() shared by the header menu and the banner.
- Remove the tautological migration spec that mocked the whole Kysely builder.
- Tests: cover create() frozen temporaryExpiresAt (workspace override + NULL
default fallback + non-temporary skips lookup) and restorePage disarming the
timer (temporaryExpiresAt: null).
Deferred (forward-looking, non-blocking): extract
PageService.computeTemporaryExpiresAt() to dedupe the deadline formula and drop
the @InjectKysely from PageTemplateController; replace migration unit test with
a real Postgres up/down integration test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the PR #202 review (approve-with-comments). The only actionable
non-blocking item was the test-coverage suggestion: the source switch in
AiChatService.handle from findRecent(chatId, ws, 50) to findAllByChat(chatId,
ws) was not pinned by a test. handle() is a streaming method the project marks
as not unit-testable, so cover the behavioral guarantee it now relies on at the
repo/integration level — seed a chat of 60 messages and assert the default
findAllByChat (exactly how handle calls it) returns the FULL transcript in
chronological order, including the first turn the old 50-window would have
dropped.
Also document the behavior change under CHANGELOG [Unreleased] -> Changed.
The two stability items (token-budget trim before streamText; O(N) history
rebuild per turn) are deferred: the reviewer flagged both as non-blocking
conscious trade-offs aligned with the PR's stated goal, and the trim is a
larger architecture change out of scope for this follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-turn model conversation was rebuilt via findRecent(chatId, ws, 50),
a sliding window that dropped the beginning of any chat longer than ~50 stored
rows. Switch streamChat to the existing findAllByChat, which loads the full
non-deleted transcript chronologically with a 5000-row memory-safety backstop
(keeps the newest rows + logs a warning on overflow) — a safety net, not a
conversational limit. Remove the now-unused findRecent method and update the
comments/log text that referenced it (findAllByChat now feeds both the Markdown
export and the model history).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add coverage for the two untested branches in useGeneratePageTitle's
post-generation write: suppressing setContent when the live title editor
is focused (DB write + broadcast still happen, only the visible field
write is skipped), and the early return when the page editor is
destroyed (model never called).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- CHANGELOG: add an [Unreleased]/Added bullet documenting the
"generate title from content" byline button (reads live editor
content, generates via the workspace AI provider, applies through
/pages/update, gated by settings.ai.generative, throttled per user).
- use-generate-page-title: guard the visible title write against page
navigation during generation. The mutation awaits the model for 1-3s;
its closure captures the editors from the starting render, but the
global page/title atoms re-point on navigation. We now keep a live ref
to the current editors and skip setContent unless the live page editor
still belongs to the page the title was generated for
(editor.storage.pageId === pageId, mirroring TitleEditor's
activePageId guard). The DB write stays correct (keyed by the captured
pageId) and the websocket broadcast is unchanged, so only the wrong-page
field write is suppressed.
- Add a vitest suite for the hook: empty content, empty model response,
happy path, the navigation guard, and 403/503/429/other onError mapping.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an AI button in the page byline that generates a note's title from the
live editor content (including unsaved edits) and applies it immediately.
Server: one-shot, non-streaming POST /ai-chat/generate-page-title mirroring
the chat generateTitle path — gated by settings.ai.generative, throttled via
AI_CHAT_THROTTLER, resolves the workspace chat model and returns { title }.
The endpoint never touches the page; the client applies the title through the
existing /pages/update route (which enforces edit permission).
Client: ai-chat-service.generatePageTitle, a useGeneratePageTitle hook that
converts the editor HTML to markdown, calls the endpoint, applies the title
via updateTitle + updatePageData, reflects it in the unfocused title editor,
and broadcasts the UpdateEvent (mirroring TitleEditor.saveTitle). A sparkles
button (GenerateTitleGroup) renders next to dictation, edit-mode + flag gated.
Tests: pure cleanGeneratedTitle helper + controller gate/delegation/error-map.
i18n: en-US + ru-RU strings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
"Temporary notes" with a death timer: created via a dedicated hourglass button
in the space-tree header, a note auto-moves to Trash after a configurable X
hours (default 24) unless explicitly made permanent ("structure or die").
Reuses existing mechanisms, mirroring is_template and the trash-cleanup job:
- New nullable column pages.temporary_expires_at (NULL = permanent; non-NULL =
frozen deadline) + partial index for the sweep; workspace column
temporary_note_hours (default via DEFAULT_TEMPORARY_NOTE_HOURS = 24).
- create-page DTO `temporary` flag; the deadline is frozen at creation so later
setting changes never reschedule existing notes.
- POST /pages/toggle-temporary (mirror of toggle-template): arm/clear the timer,
CASL-guarded via validateCanEdit, cross-workspace NotFound defense-in-depth.
- TemporaryNoteCleanupService: hourly @Interval sweep that soft-deletes expired
notes through the exact PageRepo.removePage path (recursive over children,
emits PAGE_SOFT_DELETED), attributed to the creator; idempotent via
deletedAt IS NULL filters.
- restorePage clears temporary_expires_at so a restored note can't be re-trashed.
- Workspace setting temporary_note_hours (audit-tracked) + a hours editor in
workspace General settings.
- Client: second create button, orange tree icon, tree + page-header menu toggle
("Make temporary"/"Make permanent"), an open-note banner with a rescue action,
and en/ru i18n.
Tests (unit): toggle-temporary controller (toggle/explicit/permission/cross-ws +
DTO validation), cleanup-job sweep (selection filters, per-note removePage,
error isolation), and a migration up/down sanity. Server tsc, client tsc -b,
and the page+workspace jest suites are green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a "send now" button to queued AI-chat messages: it interrupts the
running agent and immediately sends that message, while the agent's
partial output at interruption is kept in history and the next turn is
marked as a user interrupt.
Client:
- queue-helpers: pure `promoteToHead` to move a queued message to the head.
- chat-thread: `sendNow` (promote head + abort + flush-on-abort), one-shot
`flushOnAbortRef`/`interruptNextSendRef`, `interrupted` flag in the
request body, and the "send now" ActionIcon in the queued list.
Server:
- `interrupted` on AiChatStreamBody; pure `isInterruptResume` confirms the
client hint against persisted history (prev assistant turn aborted/
streaming) before honouring it.
- prompt: INTERRUPT_NOTE injected in the context section only on a
confirmed interrupt-resume turn so the model treats the partial answer
above as incomplete.
Tests: promoteToHead, chat-thread send-now (abort + resend + one-shot
interrupt flag + non-streaming immediate send), isInterruptResume, and
the prompt interrupt-note injection.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The #205 share-aliases feature placed share-aliases.migration.spec.ts
inside src/database/migrations/. Kysely's FileMigrationProvider loads
EVERY file in that folder as a migration, so `migration:latest` imported
the test file and crashed with "ReferenceError: describe is not defined"
(no Jest globals under tsx). That broke the migration step shared by the
e2e-server, e2e-mcp and integration-test (test/test) jobs.
Move the spec one level up to src/database/ (matching the existing
src/database/jsonb-bind.spec.ts convention) so the migration runner no
longer sees it, and fix its relative imports
(./migrations/... and ./types/...). Jest still picks it up via the
src/**/*.spec.ts test glob. Verified locally: 3 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The e2e transform matches .js (required so ESM-only node_modules like
nanoid/@sindresorhus get transpiled), which also sweeps in editor-ext's
prebuilt CommonJS dist/*.js. ts-jest then warns "Got a .js file to
compile while allowJs is not set to true" for each footnote file. The
.js match cannot be dropped without reintroducing the ESM load errors, so
enable allowJs for ts-jest via an inline tsconfig override (merged with
apps/server/tsconfig.json — decorators/paths/module stay intact).
Verified locally: 0 allowJs warnings, app still compiles and boots to the
Redis connection (no DI/metadata regressions).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove top margin from the first paragraph in footnote definitions so the
marker aligns with the first line of text. Adjust the task‑list label to use
the editor line‑height variable for its height and center the checkbox
vertically, keeping it in line with the item’s first text line.
The previous jest-config fix let the module graph load further and exposed
two more reasons the server e2e never passed since it was added:
1. ESM transform chain: AppModule pulls in editor-ext -> @tiptap ->
@sindresorhus/slugify -> @sindresorhus/transliterate / escape-string-regexp,
plus p-limit -> yocto-queue — all ESM-only. Extend the e2e
transformIgnorePatterns whitelist to transform them (scoped packages need
both the pnpm `@scope+name` and nested `@scope/name` path forms, hence
`@sindresorhus[+/][a-z0-9-]+`). Verified locally: the graph now fully
transforms and resolves.
2. Wrong HTTP adapter: Docmost runs on Fastify (main.ts uses FastifyAdapter)
and does not depend on @nestjs/platform-express, but the scaffold test used
the default createNestApplication() (Express) and died with
"@nestjs/platform-express package is missing". Switch the test to
FastifyAdapter + getInstance().ready(), close in afterEach. Verified locally:
createNestApplication + app.init() now proceed to the live Redis/Postgres
connection (the infra CI provides via services + migrations).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up to the first e2e fix: with nanoid/editRes.edits resolved, the
suites failed one layer deeper. Both layers were never green since the
e2e jobs were added (non-blocking in CI), so the failures had stacked up.
server e2e (jest-e2e.json) — align module resolution/transform with the
working unit/integration jest configs so AppModule's full import graph
loads:
- moduleFileExtensions: add "tsx" (React-Email .tsx templates are pulled
in via the auth controller chain).
- transform: ^.+\.(t|j)s$ -> ^.+\.(t|j)sx?$ so .tsx is transformed.
- moduleNameMapper: add ^src/(.*)$ -> <rootDir>/../src/$1 (code imports
via the absolute 'src/...' alias). Verified locally: the module graph
now fully resolves (only env vars, supplied by CI, remain).
mcp e2e (test-e2e.mjs) — insert_image/replace_image accept only http(s)
URLs the server fetches; the test passed local file paths and died with
"Invalid image URL". Serve the PNG bytes over a throwaway 127.0.0.1 HTTP
server (the Docmost server runs on the same CI host) and pass URLs. The
featPng negative test is untouched: replaceImage checks the attachmentId
and throws before fetching, so its local path is never validated.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two unrelated CI failures on the 0.94.0 release PR:
- server e2e: jest-e2e.json lacked transformIgnorePatterns, so the
ESM-only nanoid@5 package was loaded as CommonJS and crashed with
"Cannot use import statement outside a module". Add the same
node_modules whitelist already present in the unit and integration
jest configs (nanoid|uuid|image-dimensions|marked|happy-dom|lib0).
- mcp e2e: test-e2e.mjs read editRes.edits, but editPageText() returns
the per-edit results under `applied` (not `edits`), so editRes.edits
was undefined and .every() threw. Read editRes.applied instead.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The controller's buildPageSlug truncates the page title via
`title?.substring(0, 70)` before slugifying, but no test drove that
branch (the only titled case was 16 chars). Add a resolvable-alias
case with a 119-char title whose 70-char boundary falls mid-word and
assert the 302 target's slug reflects only the first 70 characters.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-row MCP Test button derived its presentation solely from the test
mutation's data ({ ok, tools } | { ok, error }). When the request itself
rejected (401/403/500/network) there is no payload, so the row silently spun
back to the idle "Test" instead of reporting the failure.
Feed the mutation error into mcpTestButtonView so a reject also renders a red
"Failed", with the tooltip taken from the server message
(error.response.data.message) or a generic i18n fallback. Enable the tooltip
for any non-idle state. Cover the reject branch (with and without a server
message) in the helper unit test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code-review follow-ups (Approve-with-comments) for batch #197
(context badge #189 / e2e in CI #187 / inline MCP test #170):
- server: extract the duplicated chatContextWindow ::text->positive-int
coercion (resolve() + getMasked()) into an exported parsePositiveInt
helper and unit-test its branches (200000/1.9/0/-5/""/abc/undefined),
closing the untested read-path gap.
- client: merge the two backward scans over messageRows into one pure,
exported selectContextBadge helper (numerator and denominator still
taken from the most recent row carrying EACH value) and unit-test the
different-rows and fresh-zero-doesn't-shadow cases.
- client: extract the MCP "Test" button tristate presentation into a pure
mcpTestButtonView helper (collapses the two parallel if/else chains) and
unit-test idle/ok-with-tools/ok-no-tools/failed label+tooltip branches.
- ci: redirect the backgrounded prod server's stdout/stderr to a log file
in e2e-mcp and cat it on failure, so a start-up crash is diagnosable
instead of surfacing only as the generic health timeout.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the two blocking test-coverage specs requested in the PR #214 review and
clear the cheap non-blocking items.
Must-fix:
- share-alias-redirect.controller.spec.ts: routing/leak guard for the public
GET /l/:alias resolver (modeled on share-seo.controller.routing.spec). Pins
302-to-canonical on a hit; SPA index without a 302 for unknown/dangling/
unreadable aliases and a null workspace (no name-existence leak); defensive
percent-decoding treated as unknown; self-hosted findFirst vs subdomain
findByHostname workspace resolution; 404 when no built client index exists.
- share-alias.controller.spec.ts: authz gates with mocked PageRepo/ShareService/
ShareAliasService/PageAccessService. Covers cross-workspace/nonexistent page
-> NotFoundException, validateCanEdit, resolveReadableSharePage null ->
BadRequestException, isSharingAllowed false -> ForbiddenException, set happy
path delegation, remove() of a dangling alias (pageId null) skipping
validateCanEdit but still deleting, and for-page validateCanView.
Cheap review items:
- Remove dead Logger import/field from ShareAliasRedirectController.
- Remove dead PagePermissionRepo import/dependency from ShareAliasController.
- Register the new share-alias UI strings in en-US and ru-RU catalogs.
- Add an [Unreleased]/Added CHANGELOG entry for /l/:alias (#205).
- Drop the tautological boilerplate assertions from the migration spec
(exports up/down; runtime checks of typed entity literals).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
onFinish always receives a totalUsage object, so the `?? {}` guard and
optional chaining were dead. Extract the field-level extraction into a
recordTurnUsage method (totalTokens, else input+output) and unit-test that
recordShareTokens receives the summed value when totalTokens is absent and the
authoritative total when present.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address non-blocking review items on the AI-chat stream-perf PR:
- Drop the unused `metadata` param from the `msg` test factory in
message-item.test.ts; no caller passed it.
- Add a per-part-kind coupling guard to message-signature.test.ts that, for
each part kind rendered today (text, reasoning, tool-*) plus the metadata
banners, asserts that mutating a field the MessageItem render body DRAWS
flips messageSignature — an executable lock for the load-bearing memo
invariant documented in message-signature.ts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a retargetable, human-readable vanity link namespace /l/<alias> that
sits alongside the untouched /share/... routes.
- New share_aliases table (workspace-scoped, UNIQUE(workspace_id, alias),
page_id nullable ON DELETE SET NULL so the address outlives its target).
- ShareAliasRepo + ShareAliasService (create / no-op / 409 reassign guard /
availability / request-time readable-target resolution through the single
existing share boundary).
- Public ShareAliasRedirectController (GET /l/:alias) issues a 302 (never 301,
the target is mutable) to the canonical /share/:key/p/:slug page; unknown /
dangling / no-longer-readable aliases serve the SPA index with no leak.
'l/:alias' excluded from the global /api prefix.
- Authenticated ShareAliasController (set/remove/availability/for-page).
- Shared ASCII-only normalize/validate util (server + client copies).
- Client: Custom address block in the share modal (live normalize + debounced
availability + copy + reassign confirmation dialog).
- Unit tests: util, repo SQL-shape, service semantics, migration/entity sanity
(server jest) + client alias util (vitest).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The anonymous public-share assistant only capped the COUNT of requests
(100/hour/workspace), not their cost. One accepted turn runs the agent loop
up to stepCountIs(5), re-sending the whole client-held transcript as input on
every step, while maxOutputTokens caps only the output; the request window is
hourly with no daily ceiling, so a steady stream at the cap sustains ~24x its
count per day. Counting requests therefore does not bound the owner's LLM bill
(red-team finding #5).
Add a second cost contour: a cluster-wide, sliding-window per-workspace TOKEN
budget over a rolling day. It is checked read-only BEFORE a turn streams (429,
no request slot consumed, nothing spent) and the turn's real usage
(totalUsage: input re-sent per step + output, summed across all steps) is
recorded once it finishes via streamText onFinish. Fails closed on the check
(deny when Redis can't prove we're under budget); best-effort on the record.
Env-overridable via SHARE_AI_WORKSPACE_TOKEN_BUDGET_PER_DAY (default 1M/day).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
persist-1: onStoreDocument wrapped the page write in a try/catch that only
logged and swallowed the error, then resolved "successfully". hocuspocus
destroys/unloads the in-memory Y.Doc right after the hook resolves (the only
copy of the latest edit), so a transient DB error (deadlock, serialization
failure, dropped connection) silently lost the edit. Worse, the post-store
branch ran on the partially-assigned `page`, broadcasting a phantom
"page.updated" and enqueueing a history snapshot for content never written.
Wrap the write in a small bounded retry (3 attempts) so the save is
re-attempted while we still hold the doc, and clear `page` on failure so the
success-only side effects never report a save that didn't happen.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ui-state-races-1: the server-authoritative move path (placeByPosition, via
applyMoveTreeNode) lacked the isDescendant cycle guard that drag-drop `move`
has. When move events arrive out of order so the destination parent is still
nested inside the moved node's own subtree, remove(source) dropped the whole
subtree (incl. the future parent) and insertByPosition could not re-place it —
the node and all descendants silently vanished with no error/refetch.
Add the isDescendant guard to placeByPosition (returns same ref, like its other
no-op cases) and short-circuit applyMoveTreeNode on the same condition BEFORE
the placed===prev remove-fallback (which would otherwise still drop the
subtree). Leave the tree untouched so a later corrective event / reconnect
reconcile fixes it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
attach-1: when the same attachmentId was referenced by more than one page
in a duplicated subtree, the per-attachmentId map held only a single copy
entry, so the last page processed clobbered the others. The downstream
ownership guard (`attachment.pageId !== oldPageId`) then matched at most one
page and skipped the lone DB row entirely: no blob copied, no new row, every
copy's image 404'd. Key the map to a list of entries and copy one blob/row
per referencing page; drop the now-incorrect ownership guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getPageBreadCrumbs (ancestor CTE) and forceDelete (descendant CTE) used
withRecursive + unionAll with no CYCLE clause or depth cap. If a parent/child
cycle already exists in the data (e.g. one slipped in via the #7 TOCTOU race),
both queries loop forever — hang / statement timeout. Worse, the move guard
itself runs the ancestor CTE, so a cycle would disable the very guard meant to
prevent it (#207#8).
Add a depth counter bounded by MAX_PAGE_TREE_DEPTH to both recursive CTEs; the
walk stops at the cap, so a cycle yields a bounded result instead of hanging.
Real page trees are only a few levels deep, so the cap never truncates a
legitimate result. getPageBreadCrumbs selects an explicit column list (not
selectAll) so the internal depth counter never leaks into the breadcrumb shape.
Adds an integration test that seeds an A<->B cycle directly and asserts both
getPageBreadCrumbs and forceDelete return bounded / complete under a short
connection-level statement_timeout instead of hanging.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The server-side move cycle guard (getPageBreadCrumbs) and the move UPDATE ran
as two separate, unlocked statements. Two concurrent moves ("A under B" and
"B under A") could each read the same pre-write acyclic snapshot, both pass the
guard, then persist A.parentPageId=B AND B.parentPageId=A — a parent/child
cycle (TOCTOU, #207#7).
Run the cycle check and the UPDATE inside one transaction (executeTx) guarded
by a per-space advisory lock (pg_advisory_xact_lock, held until COMMIT) so all
moves within a space serialize: the second mover blocks until the first commits
and then sees the freshly written parent, so its guard rejects the cycle.
getPageBreadCrumbs gains an optional trx so the check runs on the locked snapshot.
Adds an integration test driving two opposing concurrent movePage calls and
asserting no cycle ever persists and exactly one move is rejected. Updates the
movePage unit-test stubs for the new transactional path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In-app AI-chat tools used bare zod schemas, so when the model dropped a
required arg (typically pageId) in a parallel/batch tool call, the AI SDK
forwarded zod's raw "expected string, received undefined" text to the model
— not actionable. Add a centralized modelFriendlyInput(shape) wrapper that
keeps the exact JSON Schema contract (required/description/constraints via
z.toJSONSchema draft-7) but replaces the raw zod text with a message naming
each missing/invalid parameter and reminding the model not to drop ids like
pageId in parallel batches. No value is guessed/backfilled (cf. #159).
Applied to every in-app tool: the sharedTool() builder and all inline
inputSchema in ai-chat-tools.service.ts, plus public-share-chat-tools.service.ts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR #182 review (post-fix pass) surfaced two latent correctness risks in the
new MessageItem memo: the per-message signature tracks only [type, text length,
state, error/output presence] + metadata, so a part kind whose VISIBLE content
can change WITHOUT changing those fields would silently freeze a stale row.
Neither is reachable with the current toolset (tool output is set once;
streaming is append-only with a fixed id), so the correct fix is to harden the
documented invariant rather than hash output content on every delta (getPage
returns full page content — hashing it per-delta would tax the hot path this
PR optimizes).
Add a WARNING in messageSignature naming the two future triggers (a tool that
streams `preliminary` output; a client-side regenerate/edit that mutates a
finalized row in place) and the required action (extend the signature).
No behavior change (comment only). vitest src/features/ai-chat 189/189 pass,
tsc clean for the touched files.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
