Skip to content

Port the shared table engine so the core matches JetBrains and Notepad++ - #9

Merged
krotname merged 1 commit into
mainfrom
feature/core-parity-with-shared-engine
Aug 4, 2026
Merged

Port the shared table engine so the core matches JetBrains and Notepad++#9
krotname merged 1 commit into
mainfrom
feature/core-parity-with-shared-engine

Conversation

@krotname

@krotname krotname commented Aug 4, 2026

Copy link
Copy Markdown
Owner

What this is

An independent audit of the VS Code port found that src/core.ts was an independent
reimplementation of the shared Markdown table engine rather than a port of it, and that it
diverged from the reference on roughly 10% of a differential corpus — including several
divergences that corrupt documents.

This replaces the core with a faithful port of MarkdownTableEngine.java (mirrored by the
Notepad++ MarkdownTableCore.cpp), including the shared Unicode width tables.

How the divergences were found

A differential harness runs the same operations through the reference core
(markdown-table-core-0.3.1.jar) and through this core, comparing every field of EditResult.

corpus cases before after
hand-written edge cases + fuzz 11 719 8 048 differing 0
width probes, adversarial separators, CSV, deep Unicode 19 089 not run 0
chained operation sequences (up to 6 steps each) 6 000 not run 0

Defects fixed

  • Prose was pulled into the table and rewritten. tableRangeEnd now requires continuation
    rows to open with a pipe when the header and separator do, so a line such as tail | prose
    after a table is left alone. Previously any following line containing a pipe was absorbed into
    the table range and reformatted.
  • Cells were silently dropped. parseTable now takes the column count from the widest row and
    pads short rows. It previously truncated every row to the separator's width, so a row with an
    unescaped pipe (| x|y | 0 |) lost its trailing cells on any edit.
  • Delete Row could destroy the table. canDeleteRow refuses to delete the row above the
    separator, so deleting the header no longer leaves a separator-first block that is not a table.
  • The caret landed past the content in right and centre aligned columns. targetColumnOffset
    is now the character offset of the cell content inside the formatted line, matching the
    reference. It previously carried the padding width, which was then added to a position that had
    already skipped that padding — an overshoot equal to the padding. cellCharacterOffset is no
    longer needed and is removed.
  • Valid separator rows were rejected. isPotentialSeparatorLine accepts spaced dashes
    (| -- - |), = rules (|===|) and short |--- lines. Conversely, resolveTable now rejects
    a pipe block that has no separator row at all.
  • Fitting was not idempotent. applyWrappedToWidth and the manual resize actions unwrap
    continuation rows first and use the reference budget algorithm. Narrow and widen follow the
    content width instead of a fixed width-3 floor.
  • Conversion and creation differed. fromDelimited keeps cell text verbatim instead of
    collapsing internal whitespace, newTable emits Column N headers and puts the caret on the
    first data row, and both report the shared status messages.
  • Display widths depended on the host runtime. Widths now come from the same static tables the
    other cores use, instead of Intl.Segmenter plus \p{Extended_Pictographic}, so measurements no
    longer drift with the Node/ICU version.

Verification

  • npm run typecheck — clean
  • npm run test:unit — 61 passing (43 existing golden/contract tests unchanged, 18 new)
  • npm run test:coverage — core.ts 97.62% lines, 85.66% branches, 99.31% functions
  • npm run build, npm run package, npm run test:vsix — VSIX installs and enumerates
  • npm run test:e2e — 4 passing in the real Extension Host, including a caret-position check on a
    right aligned cell and a guard that prose after the table is untouched
  • npm audit — 0 vulnerabilities
  • 10 000-row table: ALIGN 39 ms, SORT 24 ms, FIT 56 ms (was ~500 ms)

The golden fixture file is unchanged and still matches the JetBrains copy byte for byte.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KsNFrStLUQaEZNtCybK4XD

The TypeScript core was an independent reimplementation rather than a port, and
it diverged from the shared engine on roughly 10% of a differential corpus.
Several of those divergences corrupted documents.

Replace it with a faithful port of MarkdownTableEngine.java (mirrored by
MarkdownTableCore.cpp), including the shared Unicode width tables, and fix the
resulting defects:

- tableRangeEnd requires continuation rows to open with a pipe when the header
  and separator do, so prose such as "tail | prose" is no longer pulled into the
  table and rewritten.
- parseTable takes the column count from the widest row and pads short rows
  instead of truncating rows to the separator's width, which silently dropped
  cells whenever a row carried an unescaped pipe.
- canDeleteRow refuses to delete the row above the separator, so DELETE_ROW on
  the header no longer leaves a separator-first block that is not a table.
- targetColumnOffset is the character offset of the cell content inside the
  formatted line, matching the reference. The previous value was the padding
  width and was then added to a position that had already skipped the padding,
  so the caret overshot the content in right and centre aligned columns.
  cellCharacterOffset is no longer needed and is removed.
- isPotentialSeparatorLine accepts spaced dashes, "=" rules and short "|---"
  separator lines; resolveTable rejects a pipe block that has no separator row.
- applyWrappedToWidth and the manual resize actions unwrap continuation rows
  first and use the reference budget algorithm, so fitting is idempotent and
  narrow/widen follow the content instead of a fixed width-3 floor.
- fromDelimited keeps cell text verbatim, newTable emits "Column N" headers and
  puts the caret on the first data row, and both report the shared messages.

Verified against the reference core (markdown-table-core 0.3.1) over 11719 +
19089 single operations and 6000 chained operation sequences: identical output
in every case, including targetRow, targetColumn and targetColumnOffset.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KsNFrStLUQaEZNtCybK4XD

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2d717b4474

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/core.ts
@krotname
krotname merged commit 5a62294 into main Aug 4, 2026
6 checks passed
@krotname
krotname deleted the feature/core-parity-with-shared-engine branch August 4, 2026 07:05
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