HAR-8546 Telemetry Service Updated#262
Merged
Merged
Conversation
VladaHarbour
force-pushed
the
har-8546_telemetry-2
branch
from
41ff8e3 to
71f49d8
Compare
harbournick
requested changes
Jan 23, 2025
harbournick
left a comment
harbournick
left a comment
Collaborator
There was a problem hiding this comment.
Just one change regarding where to instantiate the service
harbournick
reviewed
Jan 23, 2025
caio-pizzol
reviewed
Jan 24, 2025
| * @param {string} dsn - Data Source Name for telemetry service | ||
| */ | ||
| initTelemetry(config, dsn) { | ||
| const { projectId, token, endpoint } = this.#parseDsn(dsn); |
Contributor
There was a problem hiding this comment.
No need for that anymore
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
caio-pizzol
reviewed
Jan 24, 2025
| onDocumentLocked: () => null, | ||
| onFirstRender: () => null, | ||
| onCollaborationReady: () => null, | ||
| onExceptionCaught: () => null, |
Contributor
There was a problem hiding this comment.
Mmmm maybe just onException for a more generic one? Later we can add onParsingException, etc...
@harbournick what do you think?
Collaborator
There was a problem hiding this comment.
Yeah, let's go with onException - I like it
| this.#endCollaboration(); | ||
| this.removeAllListeners(); | ||
|
|
||
| // Clean up telemetry when editor is destroyed |
Collaborator
There was a problem hiding this comment.
@VladaHarbour I'm guessing we should only destroy the telemetry in SuperDoc.js, right? If so let's jut remove this line
| * @returns {Object} Safe context object | ||
| */ | ||
| const nodeListHandlerFn = (elements, docx, insideTrackChange, filename) => { | ||
| const getSafeElementContext = (elements, index) => { |
Contributor
There was a problem hiding this comment.
@harbournick this is work around for cases where index goes to -1, probably here:
index += consumed - 1;
| * @returns {Promise<string>} CRC32 hash | ||
| * @private | ||
| */ | ||
| async generateCrc32Hash(file) { |
Contributor
There was a problem hiding this comment.
Nice! @harbournick using third-party dep here: https://www.npmjs.com/package/buffer-crc32
| this.on('sidebar-toggle', this.config.onSidebarToggle); | ||
| this.on('collaboration-ready', this.config.onCollaborationReady); | ||
| this.on('content-error', this.onContentError); | ||
| this.on('exception-caught', this.config.onExceptionCaught); |
LGTM Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
|
@VladaHarbour once ready - please ping me or @harbournick so we can merge it. |
superdoc-bot Bot
pushed a commit
that referenced
this pull request
Jul 2, 2026
…es + visible read model (#263) * feat(document-api): tracked-change actions, comment export, list/numbering read model Engine + Document API work backing the LLM-tools core preset: - tracked-change side-targeted reject (decide `side` selector), tracked w:pPrChange apply, tracked lists.attach - comment + comment-reply export; comments on tracked changes persist on export - blocks.list read model: paragraph indent projection and computed numbering (marker/path/kind) so agents can see legal clause numbers - list-item / list-sequence resolver hardening; table cell shading background Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(document-api): tighten trackChanges.decide id-target side selector Addresses review findings on the side-targeted reject surface: - id-target `side` runtime accepted `insert`/`delete` aliases the published schema forbids (strict `inserted`/`deleted`). Drop the aliases so runtime matches the contract. Range targets are unchanged. - narrow the id-target `side` type to a new `ReplacementSide` (`'inserted'|'deleted'`); it no longer advertises move-only `source`/ `destination` that always throw for id targets. - decision-engine: a stale `side` selector on a change whose targeted half was already resolved (only the other side survives as a standalone insertion/ deletion) fell through and silently resolved the surviving side. Fail closed unless the standalone side matches the requested side. Only id targets set selection.side, so range/all decisions are unaffected. Adds 5 regression tests (3 decide-validation, 2 decision-engine). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(super-editor): drop unused destructure var in tracked-numbering path `_existingChange` was flagged by @typescript-eslint/no-unused-vars as an ERROR (the file's TS lint config does not honor the /^_/ ignore pattern for rest-destructure siblings), failing CI lint. Snapshot the former paragraph properties with an explicit delete instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(super-editor): surface tracked paragraph-property changes in the review API Tracked numbering-attach / alignment changes (w:pPrChange) were export-only: they round-tripped to Word but never appeared in trackChanges.list and could not be accepted/rejected via trackChanges.decide, because they live on node attrs (paragraphProperties.change), not marks, and both enumeration sites (the review graph and the doc-api resolver) scan marks + structural rows only. To a customer that reads as "tracked numbering silently failed". Mirror the existing tableRow structural precedent for attr-based changes: - pprChanges.ts enumerator walks blocks for a valid paragraphProperties.change - review-graph projects each into a Formatting logical change (synthetic whole-block segment + non-enumerable change.pprChange payload) - decision-engine routes change.pprChange to planPprDecision before the type-based branches (it is typed Formatting but has no mark): accept drops the change record (numbering stays), reject restores the former properties; applied via setNodeMarkup like clearRowTrackChange - the doc-api resolver (groupTrackedChanges) appends them as formatting changes so they surface in trackChanges.list; the node-stored record id doubles as the public + command id, routing decide to the same review-graph change New integration test proves list + accept + reject end-to-end; 1476 tracked- change tests pass. Also drops an unused `retired` param (lint). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(super-editor): prettier-format the 3551 integration tests These integration tests were committed without prettier formatting, failing the CI format:check gate. No logic change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(super-editor): address pPrChange review — type, reject sync, schema - review-graph: declare the optional `pprChange` flag on TrackedSegment so the synthetic segment literal type-checks (fixes CI TS2353; mirrors `structural`) - decision-engine: rejecting a tracked pPrChange now also syncs the TOP-LEVEL numberingProperties + listRendering to the restored former state, not just paragraphProperties — otherwise a rejected block still reads/renders as a numbered list item in any path that doesn't re-run the numbering plugin - contract: publish the `numbering` and `indent` fields on the blocks.list output schema (they were returned + typed on BlockListEntry but omitted from the closed JSON schema, so schema-driven clients dropped them); regenerated reference docs + manifest check:types 0 errors, lint 0 errors, 1234 tracked-change tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(painter-dom): align thick-border test with SD-3028 authored-width rule border-utils.test.ts expected thick borders at max(width*2, 3), but SD-3028 deliberately paints `thick` at the authored w:sz width (no 2x, min 1px) — see getBorderBandWidthPx + border-band.test.ts ("thick paints at the authored width"). The painter test and the applyBorder comment were stale leftovers from before that decision and failed on origin/main too; they only surfaced here because doc-api/sdk changes trigger CI's `--project=!*super-editor*` vitest job. Helper is unchanged. 91 border tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(document-api): clear cell `background` on cell-scoped clearShading Shading a cell writes the `background` attr (the render/export source of truth), but tablesClearShadingAdapter only deleted `background` on the table-scoped path. A cell-scoped clear (incl. tables.setShading({ color: null })) removed tableCellProperties.shading but left `background`, so the cell stayed shaded on screen and on export while the receipt reported success. Delete `background` on the cell clear path too (mirrors the set path + the table-scoped per-cell clear). Adds a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(super-editor): export tracked pPrChange with a Word-safe decimal w:id A tracked pPrChange (tracked numbering/alignment) exported its w:id as the internal change.id, which for API-created changes is a uuidv4 — but OOXML w:id must be a decimal integer, so Word repairs/drops it and re-import can't match. Imported pPrChanges already carry a decimal id (kept as-is); API-created UUIDs are converted to a stable decimal in a high, allocator-clear range. Adds decode-path tests (imported-decimal preserved; UUID → deterministic decimal in range) + a decimal-id assertion on the numbering export test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(super-editor): make tracked pPr changes non-positional in the review graph The synthetic pPr segment spans the whole block, so treating it positionally caused three collateral bugs when a paragraph also had other content: a text-range decide captured it (partial Formatting → CAPABILITY_UNAVAILABLE), accepting/rejecting it detached unrelated comments in the block, and a pPr change inside a tracked table was routed through the mark-based contained-child planner (null-mark deref). Fix, one idea — pPr changes are resolved only by id/all via planPprDecision, never positionally: - keep the pPr segment off graph.segments / bySegmentId (review-graph) - skip the resolvedRanges push for pPr decisions (no comment detach) - skip pPr changes in the staying-table cascade 1435 tracked-change tests pass (list + accept + reject flow intact). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(super-editor): address SD-3551 review — tracked-change edge cases + test rigor Review round 2 (PR #262, @caio-pizzol). Each item reproduced, fixed where broken, and covered with a test: 1. One-sided replacement survivor: buildLogicalChange now downgrades a change labeled "replacement" with only one side present to the plain insertion/ deletion it is — so the survivor of a side-targeted accept/reject resolves normally instead of failing "replacement missing inserted or deleted side". 2. pPrChange inside a KEPT tracked table: the main staying-table cascade now excludes pPr changes (matching the side-effect sweep), so they resolve via planPprDecision by id instead of no-opping through the mark-based child planner (pPr is attr-based, not an inline mark). 3. Tracked lists.attach with no user: guarded with ensureTrackedCapability so a tracked pPrChange can no longer be stamped with a blank author (mirrors the ins/del and lists.insert tracked paths). 4. Comment export: assertions tightened to exact count + zero empty comments so a sidebar-only tracked-change row leaking as an empty comment is caught (the hasCommentBody filter itself was already correct — verified). 5. pPrChange w:id: routed through the shared Word revision-id allocator (threaded into the pPr export path) for doc-wide uniqueness; the FNV-1a hash is now only a no-allocator fallback. 194 review-model tests + touched integration tests + check:types + lint green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(super-editor): flatten pPrChange on final export + cascade it when a table is accepted Follow-ups from the automated review of the previous commit, plus test fixes: - Final-doc export now flattens a tracked paragraph-property revision: when isFinalDoc, drop the .change record so the "final" DOCX carries only the accepted numbering/alignment (no pending w:pPrChange) — matching how the ins/del translators strip their wrappers on isFinalDoc. - Accepting a tracked table BY ID now resolves contained pPr changes too: the staying-table side-effect sweep routes them through planPprDecision instead of skipping them, so a reviewed table has no leftover numbering revisions. (accept-all already handled this via the main loop.) Also: - Fix two unit tests broken by the earlier w:id-allocator threading: the generate-paragraph-properties decode assertion uses objectContaining (the call also carries the allocator + part path), and the lists.attach conformance throwCase drives its mode-independent TARGET_NOT_FOUND in direct mode (the tracked path's user guard would otherwise fire first on a no-user editor). - Drop ticket/review-process tags from code comments and test names. Full review-model + document-api-adapters suites, generate-paragraph-properties, and the pPrChange translator all green; check:types + lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(super-editor): drop synthetic tracked-change rows from comment export comments.list() returns one projection row per tracked change so the sidebar can render revisions beside real comments. Feeding those rows into exportDocx({ comments }) turned every tracked change into a spurious <w:comment>: the row carries the change excerpt as `text`, so the body-presence filter let it through. Identify synthetic rows by identity — commentId/id equals trackedChangeLink.trackedChangeId — and exclude them. A genuine comment anchored on a tracked change keeps its own distinct id, so it still exports. Add a regression test for the comments.list() -> exportDocx({ comments }) path, drop leftover console.log calls from the comment integration tests, and assert the threaded reply bumps the document revision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf(super-editor): large-document performance — headless linked-styles + visible read model - skip whole-doc linked-style inline-CSS decorations when the editor runs headless (view-only work); ~50s -> ~16s on a 38-page redline - blocks.list returns the VISIBLE text model (skips tracked-deleted runs) so a second edit to an already-edited block resolves offsets against the same text the plan engine applies against — no more "Offset N out of range" drift Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(super-editor): blocks.list length/ref/preview use the visible text model The visible read-model fix switched blocks.list `text` to visible but left `textLength`, `isEmpty`, and the encoded block `ref` (segments[].end) on the raw model via computeTextContentLength(node). For a redlined block that handed out a whole-block ref ending past the visible text, so re-editing it through the ref threw "text offset out of range" (the plan compiler resolves refs as visible) — the very drift the visible read model set out to remove, only half-applied. Compute length + preview on the visible model too. Adds two regression tests (partial-delete ref end == visible length; fully-deleted block reports isEmpty + no ref). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(super-editor): sample blocks.list formatting from visible runs only blocks.list moved text/textPreview/length/ref to the visible text model, but extractBlockFormatting still sampled the first RAW run, so a block whose leading run is a styled tracked deletion reported visible text with the DELETED run's fontFamily/fontSize/bold. The agent prompt tells callers to copy those values, so rejected formatting could be replicated into new content. Skip trackDelete runs when sampling; a fully deleted block emits no run-formatting fields. Also document the visible model in the contract (regenerated docs) and add headless linked-styles coverage for the SD-3552 decoration skip. Claude-Session: https://claude.ai/code/session_01EP45Zp5VcSNQgzG8xoVsTm --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Caio Pizzol <caio@harbourshare.com> Ported-From-Source-Repo: superdoc/orbit Ported-From-Source-Commit: ac0afb7a0a37c1f11b8771c1a505bd15829f4bb0 Ported-Public-Prefix: superdoc/public
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.