diff --git a/CHANGELOG.md b/CHANGELOG.md index 795c7502..f256b539 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -117,6 +117,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **A drifted comment suggestion can be re-synced instead of failing forever + with a 409.** A suggestion whose stored anchor no longer matched the live + document used to reject every apply attempt with an unrecoverable conflict; a + new resync path re-reads the live anchor so the suggestion applies against the + current text, and orphaned anchors (whose marked run was deleted) are + reconciled rather than left blocking. (#496) + - **Place several images side by side in a row.** A new "Inline (side by side)" alignment mode in the image bubble menu renders consecutive inline images as a row that wraps onto the next line on narrow screens. The row is @@ -302,6 +309,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Decisions on comment suggestions now leave a durable audit record.** + Applying or dismissing a comment suggestion hard-deletes the (childless) + subject comment, so the only surviving trace of who decided what is the audit + event — but the audit trail was wired to a Noop service that silently + swallowed every event. The trail is now DB-backed, so + `comment.suggestion_applied` / `comment.suggestion_dismissed` (and the other + comment-decision events) persist to the `audit` table and can be reviewed + after the comment is gone. A persistence failure is still swallowed with a + warning so it never breaks the originating request. (#496) +- **Applying a comment suggestion no longer strips the replaced run's inline + formatting.** The suggested text was re-inserted carrying only the comment + anchor mark, silently dropping bold/italic/code/link on the affected run; the + prevailing formatting of the replaced run is now carried onto the applied + text. (#496) - **The server no longer runs out of heap during long autonomous agent runs.** A new pnpm patch on `ai@6.0.134` stops the SDK from building a cumulative snapshot of the ENTIRE turn text on every streamed text-delta when no output diff --git a/apps/server/src/collaboration/yjs.util.spec.ts b/apps/server/src/collaboration/yjs.util.spec.ts index 93f4ea5a..f35e44f4 100644 --- a/apps/server/src/collaboration/yjs.util.spec.ts +++ b/apps/server/src/collaboration/yjs.util.spec.ts @@ -583,18 +583,20 @@ describe('replaceYjsMarkedText', () => { ]); }); - it('mixed formatting under the mark: replacement takes the DOMINANT run marks', () => { - // Marked run = "bold" (bold, 4 chars) + "x" (plain, 1 char), same commentId. - // The dominant (longer) segment is bold, so the flat replacement is bold. + it('mixed formatting under the mark: replacement takes the DOMINANT (longest) run, NOT the leading one', () => { + // Leading run is SHORT + plain ("x", 1 char); the following run is LONGER + + // bold ("bolded", 6 chars), same commentId. The longest run is deliberately + // NOT first: a "first-wins" pick would carry plain (no bold), so asserting + // bold on the result only holds if the code genuinely selects the LONGEST run. const { fragment, text } = buildFormatted([ + { text: 'x', attrs: { comment: { commentId: 'c1', resolved: false } } }, { - text: 'bold', + text: 'bolded', attrs: { comment: { commentId: 'c1', resolved: false }, bold: true }, }, - { text: 'x', attrs: { comment: { commentId: 'c1', resolved: false } } }, ]); - const result = replaceYjsMarkedText(fragment, 'c1', 'boldx', 'Z'); + const result = replaceYjsMarkedText(fragment, 'c1', 'xbolded', 'Z'); expect(result).toEqual({ applied: true, currentText: 'Z' }); expect(text.toDelta()).toEqual([ @@ -604,4 +606,30 @@ describe('replaceYjsMarkedText', () => { }, ]); }); + + it('mixed formatting under the mark: on a length tie the FIRST run wins', () => { + // Two equal-length runs (2 chars each) with different formatting, same + // commentId. The reduce keeps the accumulator on a tie, so the FIRST run + // (italic) prevails over the later bold one. + const { fragment, text } = buildFormatted([ + { + text: 'AA', + attrs: { comment: { commentId: 'c1', resolved: false }, italic: true }, + }, + { + text: 'BB', + attrs: { comment: { commentId: 'c1', resolved: false }, bold: true }, + }, + ]); + + const result = replaceYjsMarkedText(fragment, 'c1', 'AABB', 'Z'); + + expect(result).toEqual({ applied: true, currentText: 'Z' }); + expect(text.toDelta()).toEqual([ + { + insert: 'Z', + attributes: { comment: { commentId: 'c1', resolved: false }, italic: true }, + }, + ]); + }); }); diff --git a/apps/server/src/collaboration/yjs.util.ts b/apps/server/src/collaboration/yjs.util.ts index 25c6af27..03e9d687 100644 --- a/apps/server/src/collaboration/yjs.util.ts +++ b/apps/server/src/collaboration/yjs.util.ts @@ -267,12 +267,11 @@ export function replaceYjsMarkedText( // when the marked run mixes formatting we pick the DOMINANT segment (the one // covering the most characters) and apply its attributes — a v1 that preserves // the common single-format case exactly and, for a mixed run, keeps the - // prevailing style rather than losing all of it. The dominant segment already - // carries the `comment` mark (every collected segment does), so the anchor is - // preserved; we defensively re-assert it in case a future attribute shape - // omits it. + // prevailing style rather than losing all of it. `attributes` already carries + // the `comment` mark (every collected segment is filtered on it above), so the + // anchor is preserved by copying the run's attribute set verbatim. const dominant = segments.reduce((a, b) => (b.length > a.length ? b : a)); - const insertAttrs = { ...dominant.attributes, comment: dominant.markAttrs }; + const insertAttrs = { ...dominant.attributes }; node.delete(start, len); node.insert(start, newText, insertAttrs); diff --git a/apps/server/src/integrations/audit/database-audit.service.spec.ts b/apps/server/src/integrations/audit/database-audit.service.spec.ts index 90456c72..ba586166 100644 --- a/apps/server/src/integrations/audit/database-audit.service.spec.ts +++ b/apps/server/src/integrations/audit/database-audit.service.spec.ts @@ -1,3 +1,4 @@ +import { Logger } from '@nestjs/common'; import { DatabaseAuditService } from './database-audit.service'; import { AUDIT_CONTEXT_KEY } from '../../common/middlewares/audit-context.middleware'; import { AuditEvent, AuditResource } from '../../common/events/audit-events'; @@ -115,13 +116,39 @@ describe('DatabaseAuditService', () => { expect(inserted).toHaveLength(0); }); - it('a failed insert never throws to the caller (audit is a side-record)', async () => { - const { service, failInsert } = makeService(ctx()); - failInsert(); - // Must not reject / throw. - expect(() => service.log(applyPayload())).not.toThrow(); - await Promise.resolve(); - await Promise.resolve(); + it('a failed insert is swallowed with a warn and floats no rejection (audit is a side-record)', async () => { + // This pins the load-bearing swallow inside persist(). Because log() is + // fire-and-forget (`void this.persist(...)`), it always returns synchronously + // without throwing — so `not.toThrow()` alone would stay green even if the + // try/catch were removed. We instead observe the two effects the catch is + // responsible for: a warn IS emitted, and NO unhandled rejection floats. + // Removing persist()'s try/catch reddens both assertions (warn count 0 + a + // captured rejection). + const warnSpy = jest + .spyOn(Logger.prototype, 'warn') + .mockImplementation(() => undefined as any); + const rejections: unknown[] = []; + const onRejection = (err: unknown) => rejections.push(err); + process.on('unhandledRejection', onRejection); + try { + const { service, failInsert } = makeService(ctx()); + failInsert(); + expect(() => service.log(applyPayload())).not.toThrow(); + // Flush microtasks so the rejected insert settles, then give any floated + // rejection a macrotask tick to be reported by the runtime. + await Promise.resolve(); + await Promise.resolve(); + await new Promise((r) => setImmediate(r)); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(String(warnSpy.mock.calls[0][0])).toContain( + 'Failed to persist audit event', + ); + expect(rejections).toHaveLength(0); + } finally { + process.off('unhandledRejection', onRejection); + warnSpy.mockRestore(); + } }); it('logWithContext() persists with an explicit (non-request) context', async () => {