Fix rendering of table rows with trailing empty cells #25
Conversation
Fixes mattermost/mattermost#34369 When a table row ended with an empty cell followed by content on a new line, the empty cell would not be rendered. For example: | A | B | |:-------|:------| |x|| test Would incorrectly render with only one cell in the data row instead of two (with the second being empty). The root cause was the regex pattern on line 424 that was too aggressive in removing trailing content. The pattern `/(?: *\| *)?\n$/` would optionally match and remove the trailing `||` from a line like `|x||\n`, causing the loss of the empty cell delimiter. The fix simplifies the regex to only remove the trailing newline: `/\n$/`. This preserves all pipe delimiters including those for empty cells, and makes the code consistent with the nptable processing which already used this simpler pattern. Added test case to verify the fix and prevent regressions.
|
@hmhealey, not sure how exhaustive the table tests are in this repository in order to build confidence in this fix. Do we have any other markdown-related pipeline tests, or would that be caught during e2e tests when we bump the dependency? |
|
There's a handful of tests in the web app repo, but the vast majority of them are here. If you wanted to run them, you could update the web app to point to the PR commit of this change. There's also a load of tests in https://github.com/mattermost/commonmark.js, but many of them won't pass here because this library predates the CommonMark spec. That said, while this change makes it behave like the reporter expects it to, that doesn't actually match the way GitHub handles it which actually puts the
Also, note that we previously closed this issue as Won't Fix with the assumption that we'd eventually get to https://mattermost.atlassian.net/browse/MM-10003. I'm not opposed to trying to make things work on Marked, but I think it would be better to try to make them match GitHub here |
|
Digging more into the upstream repo to see if/when they addressed this, I didn't find when they changed it to behave like GitHub's current behaviour, but I did find where they actually made this same change to fix that extra pipe being removed (markedjs#1439). Ideally, I still think we'd want this to behave the same as GitHub, but this change seems fine enough to at least remove the obvious bug even if we're not fully spec-compliant here |
The issue number refers to a GitHub issue, not a Jira ticket.
|
Thanks, @hmhealey! Testing with webapp locally gets the expected results: 
|
Like I mentioned, it fixes that last cell being removed from the table, but it's still not "correct" technically so we'll have to remember that in case it comes up later |
hmhealey
added
the
3: Reviews Complete
|
Once this is merged in, we'll also need a PR on the monorepo to update it to the new version of this library |
Summary
Fixes mattermost/mattermost#34369
This PR fixes a bug where table rows ending with empty cells would not render correctly when followed by content on a new line.
Before
Would render with only one cell in the data row instead of two:
After
Now correctly renders with both cells (the second being empty):
Technical Details
Root Cause: The regex pattern on line 424 was too aggressive in removing trailing content. The pattern
/(?: *\| *)?\n$/would optionally match and remove the trailing||from a line like|x||\n, causing the loss of the empty cell delimiter.Solution: Simplified the regex to only remove the trailing newline:
/\n$/. This preserves all pipe delimiters including those for empty cells, and makes the code consistent with thenptableprocessing on line 231 which already used this simpler pattern.Testing
mm34369_empty_cells_with_newlineto verify the fix~ Claude