Port the shared table engine so the core matches JetBrains and Notepad++ - #9
Merged
Merged
Conversation
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
There was a problem hiding this comment.
💡 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".
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
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.
What this is
An independent audit of the VS Code port found that
src/core.tswas an independentreimplementation 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 theNotepad++
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 ofEditResult.Defects fixed
tableRangeEndnow requires continuationrows to open with a pipe when the header and separator do, so a line such as
tail | proseafter a table is left alone. Previously any following line containing a pipe was absorbed into
the table range and reformatted.
parseTablenow takes the column count from the widest row andpads 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.canDeleteRowrefuses to delete the row above theseparator, so deleting the header no longer leaves a separator-first block that is not a table.
targetColumnOffsetis 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.
cellCharacterOffsetis nolonger needed and is removed.
isPotentialSeparatorLineaccepts spaced dashes(
| -- - |),=rules (|===|) and short|---lines. Conversely,resolveTablenow rejectsa pipe block that has no separator row at all.
applyWrappedToWidthand the manual resize actions unwrapcontinuation rows first and use the reference budget algorithm. Narrow and widen follow the
content width instead of a fixed width-3 floor.
fromDelimitedkeeps cell text verbatim instead ofcollapsing internal whitespace,
newTableemitsColumn Nheaders and puts the caret on thefirst data row, and both report the shared status messages.
other cores use, instead of
Intl.Segmenterplus\p{Extended_Pictographic}, so measurements nolonger drift with the Node/ICU version.
Verification
npm run typecheck— cleannpm 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% functionsnpm run build,npm run package,npm run test:vsix— VSIX installs and enumeratesnpm run test:e2e— 4 passing in the real Extension Host, including a caret-position check on aright aligned cell and a guard that prose after the table is untouched
npm audit— 0 vulnerabilitiesThe golden fixture file is unchanged and still matches the JetBrains copy byte for byte.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KsNFrStLUQaEZNtCybK4XD