Repository navigation
Conversation
|
@onmax is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
655f845 to
c0e804c
Compare
![roboeve[bot]](https://avatars.githubusercontent.com/b/10660?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for this, the fix is right for top-level quotes and the new tests are genuine: I reverted markdown.ts to main and all three fail, then pass again with the change (full Teams suite: 495/495).
I also ran ~15 edge cases through fromAst and re-parsed the output with the repo's own parseMarkdown (CommonMark, as a proxy for the Teams renderer; I could not check a live Teams client). Paragraphs, lists, nested lists, nested quotes, fenced code with blank lines, tables, and thematic breaks inside a quote all now round-trip to the same structure. One case regresses, so requesting changes:
Regression: a multi-line quote inside a list item now escapes the list
renderList (in packages/chat/src/markdown.ts) only prefixes the first line of a child with the bullet and never indents the rest. Before this PR the second line of a quote had no >, so CommonMark treated it as a lazy continuation and it accidentally stayed in place. Now it starts with > at column 0, which opens a new top-level block quote:
| Input | main output |
PR output | PR output re-parsed |
|---|---|---|---|
- > a / > b |
- > a / b |
- > a / > b |
list with quote a, then a separate top-level quote b |
A quote with two paragraphs inside a list item was already broken on main and stays broken.
A fix that keeps other adapters' output unchanged (Slack would show the extra indent inside code blocks, so I made it opt-in):
--- a/packages/chat/src/markdown.ts
+++ b/packages/chat/src/markdown.ts
@@ export abstract class BaseFormatConverter implements FormatConverter {
abstract fromAst(ast: Root): string;
abstract toAst(platformText: string): Root;
+ /**
+ * Whether `renderList` indents the continuation lines of multi-line item
+ * content (block quotes, code blocks) to the item's content column.
+ * Enable for CommonMark targets, where an unindented line escapes the list.
+ */
+ protected indentListContinuationLines = false;
+
protected renderList(
@@
- const text = nodeConverter(child);
+ let text = nodeConverter(child);
if (!text.trim()) {
continue;
}
+ if (this.indentListContinuationLines) {
+ const contentIndent = `${indent}${" ".repeat(prefix.length + 1)}`;
+ text = text.replaceAll("\n", `\n${contentIndent}`);
+ }
if (isFirstContent) {--- a/packages/adapter-teams/src/markdown.ts
+++ b/packages/adapter-teams/src/markdown.ts
export class TeamsFormatConverter extends BaseFormatConverter {
+ protected override indentListContinuationLines = true;
+With this plus the test suggestions below, I get: Teams 496/496, chat markdown tests 137/137, Teams typecheck and Biome clean. Because it touches chat, the changeset needs a chat entry too (suggestion inline).
Not blocking, just noting: I agree the Slack, Google Chat, and Discord converters have the same first-line-only prefix; Slack in particular needs > on every line, so a follow-up issue seems worthwhile.
| .map((child) => | ||
| this.nodeToTeams(child) | ||
| .split("\n") | ||
| .map((line) => (line ? `> ${line}` : ">")) |
There was a problem hiding this comment.
Prefixing every line is right, but it changes what a quote looks like when renderList embeds it: continuation lines now start with > at column 0 and escape the list item (e.g. - > a\n > b renders as - > a\n> b, which re-parses as a list plus a separate top-level quote). See the review summary for an opt-in renderList fix and the regression test suggested below.
There was a problem hiding this comment.
Fixed by #1104, which this branch is now stacked on: Teams opts into indentListContinuationLines, so every line of a quote inside a list item is indented to the item's content column.
| ); | ||
| }); |
There was a problem hiding this comment.
The exact-string check is good. Adding a structural check makes the test assert what Teams actually needs (the three steps stay one list inside the quote) rather than only the text shape, so it also catches lazy-continuation mistakes that look fine as strings. toAst on plain markdown is just parseMarkdown, so no new import is needed.
| ); | |
| }); | |
| ); | |
| const reparsed = converter.toAst(result); | |
| expect(reparsed.children).toHaveLength(1); | |
| expect(reparsed.children[0]).toMatchObject({ | |
| type: "blockquote", | |
| children: [ | |
| { type: "paragraph" }, | |
| { type: "paragraph" }, | |
| { type: "list", ordered: true, children: [{}, {}, {}] }, | |
| { type: "paragraph" }, | |
| ], | |
| }); | |
| }); |
| "> ```", | ||
| ].join("\n") | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Regression test for the quote-inside-a-list case. It fails on this branch as-is and passes with the indentListContinuationLines change from the review summary.
| }); | |
| }); | |
| it("should keep a multi-line blockquote inside its list item", () => { | |
| const ast = converter.toAst("- > first line\n > second line"); | |
| const result = converter.fromAst(ast); | |
| expect(result).toBe("- > first line\n > second line"); | |
| const reparsed = converter.toAst(result); | |
| expect(reparsed.children).toHaveLength(1); | |
| expect(reparsed.children[0]).toMatchObject({ | |
| type: "list", | |
| children: [{ type: "listItem", children: [{ type: "blockquote" }] }], | |
| }); | |
| }); |
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@chat-adapter/teams": patch | |||
There was a problem hiding this comment.
Needed if the renderList change lands, since it adds a protected member to BaseFormatConverter.
| "@chat-adapter/teams": patch | |
| "chat": patch | |
| "@chat-adapter/teams": patch |
There was a problem hiding this comment.
The chat entry is in #1104's changeset, which carries the renderList change, so this PR's changeset stays Teams-only.
c0e804c to
c318189
Compare
c318189 to
0c9f8f4
Compare
|
Thanks, the quote-inside-a-list regression was real. Here's what changed:
Teams suite: 497 passed. Once #1104 merges, I'll rebase this PR so it contains only the Teams commit. |
0c9f8f4 to
118e62b
Compare
There was a problem hiding this comment.
Thanks, all of my requested changes are addressed. This approval covers the Teams commit (118e62b), reviewed on top of #1104's a679006 as stacked here. Merge order is #1104 first, then the rebase you described.
What I checked locally on 118e62b:
- Suites: Teams 499/499 after rebuilding
chatfrom this branch. Typecheck and Biome are clean, and CI is green apart from the fork deploy authorization. - Mutation check on the Teams change: each mutation fails at least one test.
- Splitting on
\nonly fails the CR test. I confirmedparseMarkdownkeeps\rin code and text values, so that test really exercisesLINE_BREAK_REGEX. - Joining blocks with
\ninstead of\n>\nfails 5 tests. - Writing
>instead of a bare>on empty lines fails the CR test. - Turning Teams'
indentListContinuationLinesoff fails the 3 list-item tests.
- Splitting on
- Round trip: 17 edge cases went through
fromAst, thenparseMarkdownagain, and came back with the same structure.- They include soft breaks, quotes inside list items and ordered items, lists and code inside a quote inside a list, code inside a list inside a quote, nested quotes inside a list item, and tables inside a quote and inside a list item.
- They also include two lists inside a quote, a horizontal rule inside a quote, a loose item inside a quote, CR and CRLF input, and a quote under a nested ordered list. Every output also renders the same when converted a second time.
- The one case that doesn't round-trip is an empty
>inside a list item, which gets dropped. That matches the existing behavior of skipping empty item content, so I'm not blocking on it.
As before, this uses the repo's CommonMark parser to stand in for the Teams renderer. I didn't check a live Teams client.
## Problem `BaseFormatConverter.renderList` in `packages/chat/src/markdown.ts` loses list structure in three cases. Discord, Google Chat, Slack, Teams, Twitch, and X all use it to render lists. Teams output is shown below. A fenced code block, or any other block that spans several lines, keeps the indentation only on its first line, so the rest of the block falls out of the item: ````md - item ``` x ``` - next ```` Read back as Markdown, that gives an empty code block, a paragraph with `x`, and a second code block that swallows `- next`. A list nested under an ordered item gets two spaces, but `1. ` needs three. The child becomes a sibling of its parent: ```md 1. parent 1. child 2. next ``` Two paragraphs in one item are joined with a single newline, so they merge into one paragraph: ```md - first second ``` ## Fix Nested lists now start at the item's content column, which is the marker's width plus one space. That gives two spaces under `-` or `•`, three under `1. `, and four under `10. `, so nesting under an unordered list doesn't change. Two blocks in one item get a blank line between them. A nested list still follows its parent's text directly, unless it comes after HTML or is an ordered list that doesn't start at 1. Neither of those can follow a paragraph directly in CommonMark, so they keep the blank line. These changes apply to every adapter. Indenting the rest of a multi-line block, such as a code block, is opt-in through a new protected `indentListContinuationLines` property on `BaseFormatConverter`. It defaults to `false` and applies at every nesting level. Lines split on CR, LF, and CRLF. On targets like Slack the extra spaces would show inside code blocks, and an indented `>` may no longer read as a quote. The Teams converter turns it on, because Teams messages go out with `textFormat: "markdown"`, where an unindented line leaves the item. Discord, Google Chat, Slack, Twitch, and X keep the old behavior for these lines. I haven't checked whether Discord should opt in too. With Teams, the three inputs now come out unchanged: ````md - item ``` x ``` - next ```` ```md 1. parent 1. child 2. next ``` ```md - first second ``` #1103 builds on this. It quotes every line of a block quote in Teams, which only stays inside a list item once these lines are indented. ## Checklist - [x] All commits are signed and verified - [x] All commits are signed off for the DCO (`git commit -s`) - [ ] `pnpm validate` passes - [x] Changeset added - [x] Documentation updated (or N/A) Model: Claude Opus 5.5, harness: Claude Code --------- Signed-off-by: onmax <maximogarciamtnez@gmail.com> Signed-off-by: Ben Sabic <27636870+bensabic@users.noreply.github.com> Co-authored-by: Ben Sabic <27636870+bensabic@users.noreply.github.com> Co-authored-by: roboeve[bot] <311457182+roboeve[bot]@users.noreply.github.com>
The Teams converter added the `> ` prefix only to the first line of each block in a quote and joined blocks with a single newline. Teams then merged the quote's paragraphs and folded list items after the first into the first item. Prefix every line, split on CR, LF, and CRLF, and separate blocks with a lone `>` line. Signed-off-by: onmax <maximogarciamtnez@gmail.com>
118e62b to
4912904
Compare
Problem
TeamsFormatConverteradds the>prefix only to the first line of each block inside a block quote, and joins the blocks with a single newline. A quote that holds several paragraphs or a list comes out like this:Teams merges the paragraphs into one. List items 2 and 3 have no quote prefix, so Teams folds them into item 1. A bot that quotes a reply with numbered steps shows all the steps as one.
Fix
The
isBlockquoteNodebranch ofnodeToTeamsinpackages/adapter-teams/src/markdown.tsnow adds>to every line of each rendered child, splitting on CR, LF, and CRLF. Empty lines get a bare>, and a lone>line separates blocks. The same input now renders as:Nested quotes get
> >on every line, and fenced code blocks inside a quote keep the prefix on each of their lines.Depends on #1104
This branch includes the commit from #1104, so please review only the last commit here. Without #1104, prefixing every line would move a quote inside a list item out of the list.
- > afollowed by> bcame out as- > aand> b, and the second line read back as a separate quote. #1104 indents every line of a block in a list item for Teams, which keeps the quote in its item. Once #1104 merges, I'll rebase this PR so it contains only the Teams commit.Other adapters
The Slack (
packages/adapter-slack/src/markdown.ts), Google Chat (packages/adapter-gchat/src/markdown.ts), and Discord (packages/adapter-discord/src/markdown.ts) converters each have their own copy of the same first-line-only prefix. No shared helper causes it, so this PR changes only Teams. I haven't checked how those platforms render a quote with several blocks, so I left them alone.Checklist
git commit -s)pnpm validatepasses (ran only the Teams adapter's tests, typecheck, and lint above)Model: Claude Opus 5.5, harness: Claude Code