preserve line breaks and indentation in welcome messages#1718
preserve line breaks and indentation in welcome messages#1718dgageot merged 3 commits intodocker:mainfrom
Conversation
Signed-off-by: maxcleme <maxime.clement@docker.com>
Signed-off-by: maxcleme <maxime.clement@docker.com>
Signed-off-by: maxcleme <maxime.clement@docker.com>
There was a problem hiding this comment.
Review Summary
Found 1 issue in the changed code that should be addressed.
The PR implements indentation and line break preservation well with comprehensive tests. However, there's an inconsistency in the error handling path where the processed content is not used on fallback.
| rendered, err := markdown.NewRenderer(width - messageStyle.GetHorizontalFrameSize()).Render(msg.Content) | ||
| // Convert explicit newlines to markdown hard line breaks (two trailing spaces) | ||
| // This preserves line breaks from YAML multiline syntax (|) while still | ||
| // allowing markdown formatting like **bold** and *italic* |
There was a problem hiding this comment.
Error fallback uses original content instead of processed content
The preserveLineBreaks() function is called on line 151 and stores the result in the content variable (leading spaces converted to non-breaking spaces). However, when markdown rendering fails, line 153 falls back to msg.Content instead of content, which loses the indentation processing.
| // allowing markdown formatting like **bold** and *italic* | |
| rendered = content |
This ensures consistent behavior where indentation is preserved regardless of whether markdown rendering succeeds or fails. While rendering errors should be rare, they can occur and would currently cause unexpected loss of indentation.
There was a problem hiding this comment.
The line numbers of this suggestion are off a bit. I'll take a look at fixing it today.
is rendered as:
TBH I wonder if it should be considered markdown implicitly as of today. It makes pretty hard to create banner and such. Maybe it should be printed "as-is" instead? Wdyt?