fix(mattermost): preserve Markdown formatting + native tables#12809
fix(mattermost): preserve Markdown formatting + native tables#12809echo931 wants to merge 3 commits intoopenclaw:mainfrom
Conversation
… natively Three related bugs in the Mattermost channel plugin: 1. normalizeMention() used \s+ replacement which collapsed newlines into spaces, destroying all block-level Markdown (headings, lists, blockquotes, task lists) in inbound messages. Fixed to only collapse horizontal whitespace (spaces/tabs) while preserving newlines. 2. DEFAULT_TABLE_MODES did not include mattermost, so it fell through to the "code" default which wraps tables in fenced code blocks. Mattermost clients render pipe tables natively, so the correct default is "off". 3. When tableMode was "code" and a message contained tables, the entire text was re-rendered through the IR pipeline which also stripped block formatting from non-table content. Also extracted normalizeMention to monitor-helpers.ts for testability and added unit tests for both fixes. Fixes: openclaw#12247, openclaw#12245, openclaw#12238
…eedback) Replaced blanket horizontal-whitespace collapse with a line-by-line approach that only collapses runs of multiple spaces after non-space characters, preserving leading indentation for nested lists and code blocks.
openclaw-barnacle
bot
added
the
channel: mattermost
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/config/markdown-tables.test.ts
Outdated
| describe("DEFAULT_TABLE_MODES (mattermost)", () => { | ||
| it("mattermost entry is present in source", async () => { | ||
| // Read the source to verify the entry exists — the runtime function | ||
| // requires plugin registry initialization which is heavy for unit tests. | ||
| const fs = await import("node:fs"); | ||
| const src = fs.readFileSync( | ||
| new URL("./markdown-tables.ts", import.meta.url).pathname.replace( | ||
| "markdown-tables.test.ts", | ||
| "markdown-tables.ts", | ||
| ), | ||
| "utf-8", | ||
| ); | ||
| expect(src).toContain('["mattermost", "off"]'); |
There was a problem hiding this comment.
Brittle source-based test
These tests are asserting DEFAULT_TABLE_MODES by reading markdown-tables.ts as text (fs.readFileSync(...) + toContain('["mattermost", "off"]')). That’s not testing runtime behavior and is fragile: it depends on exact source formatting and on the pathname string-replace logic to locate the file. This can break if the test file is renamed/moved or if path handling differs. It’d be more robust to test resolveMarkdownTableMode(...) by mocking/stubbing normalizeChannelId / plugin registry (or exporting the defaults for tests) instead of inspecting source text.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/markdown-tables.test.ts
Line: 7:19
Comment:
**Brittle source-based test**
These tests are asserting `DEFAULT_TABLE_MODES` by reading `markdown-tables.ts` as text (`fs.readFileSync(...)` + `toContain('["mattermost", "off"]')`). That’s not testing runtime behavior and is fragile: it depends on exact source formatting and on the pathname string-replace logic to locate the file. This can break if the test file is renamed/moved or if path handling differs. It’d be more robust to test `resolveMarkdownTableMode(...)` by mocking/stubbing `normalizeChannelId` / plugin registry (or exporting the defaults for tests) instead of inspecting source text.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch — replaced the source-text inspection with a direct import of the (now exported) DEFAULT_TABLE_MODES map. Tests now assert runtime values. Fixed in 316f9a7.
…of source text Address review feedback: replace brittle fs.readFileSync source inspection with direct import of the exported map.
|
Update: newline “loss” was a false positive.
This PR should remain focused on the user-visible Mattermost Markdown rendering fixes. |
thewilloftheshadow
force-pushed
the
main
branch
from
bfc1ccb to
f92900f
Compare
|
Closing this stale branch because it is heavily behind main and no longer up-to-date for review.\n\nI will reopen a fresh PR from current main for this feature/bug once rebased/reimplemented cleanly. |
1 participant
Fixes three related Mattermost Markdown issues:
Inbound Markdown block structure degraded (mentions + whitespace):
normalizeMention()used/\s+/greplacement which collapsed newlines into spaces, destroying block-level Markdown in inbound messages. Fixed with a line-by-line approach that preserves newlines and leading indentation.Tables wrapped in code blocks ([Bug]: Mattermost channel wraps Markdown tables in fenced code blocks instead of rendering tables #12238):
DEFAULT_TABLE_MODESdid not include mattermost, so it fell through to"code". Mattermost clients render pipe tables natively, so the correct default is"off".Block formatting degraded ([Bug]: Mattermost channel degrades Markdown block formatting (headings/quotes/lists/task lists) #12245): When
tableMode="code"and a message contained tables, the entire text was re-rendered through the IR pipeline which also stripped headings/blockquotes/lists from non-table content.Changes
markdown.tablesto"off"(native table support)normalizeMentiontomonitor-helpers.tsfor testabilitynormalizeMention+ table mode defaultsTests
Fixes #12245 #12238
Note: #12247 was closed as not a bug; inbound newlines are preserved. The confusion came from a system-event preview string that flattens whitespace via
/\s+/g(see #12850 for the feature request to optionally preserve newlines in previews).