fix(docs): trim trailing code block blanks#561
fix(docs): trim trailing code block blanks#561OwenYWT wants to merge 1 commit intolarksuite:mainfrom
Conversation
📝 WalkthroughWalkthroughA new Markdown utility function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
domain/ccm
OwenYWT
force-pushed
the
fix/docs-code-block-trailing-line
branch
from
add93e0 to
5387ee8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
shortcuts/doc/docs_fetch.go (1)
67-73: Collapse the tworesult["markdown"]type-assert blocks.Lines 67–69 and 71–73 do the same
.(string)assertion back-to-back. They can be merged into a single block that applies both transforms, which also avoids an unnecessary re-fetch from the map:♻️ Proposed refactor
- if md, ok := result["markdown"].(string); ok { - result["markdown"] = common.TrimMarkdownCodeBlockTrailingBlanks(md) - } - - if md, ok := result["markdown"].(string); ok { - result["markdown"] = fixExportedMarkdown(md) - } + if md, ok := result["markdown"].(string); ok { + result["markdown"] = fixExportedMarkdown(common.TrimMarkdownCodeBlockTrailingBlanks(md)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_fetch.go` around lines 67 - 73, Combine the two repeated type-assert blocks that check result["markdown"] into a single type assertion: perform one check like `md, ok := result["markdown"].(string)` and if ok set `result["markdown"]` after applying both transformations in sequence (first call common.TrimMarkdownCodeBlockTrailingBlanks(md) and then pass the result to fixExportedMarkdown), thus avoiding the double map lookup and applying both transforms before writing back to result.shortcuts/doc/docs_create.go (1)
55-66: Pass the trimmed markdown tonormalizeDocsUpdateResultfor consistency withdocs_update.go.
buildDocsCreateArgstrims the markdown before sending it to the MCP tool, but line 63 then callsnormalizeDocsUpdateResult(result, runtime.Str("markdown"))with the untrimmed raw value. Indocs_update.go(line 115) the trimmed value is passed. The downstream check isisWhiteboardCreateMarkdownwhich scans for```mermaid/```plantuml/<whiteboard>tokens — trimming trailing blanks inside code blocks won't change those detections, so this is a consistency nit rather than a correctness bug, but worth aligning:♻️ Proposed refactor
- Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { - args := buildDocsCreateArgs(runtime) - result, err := common.CallMCPTool(runtime, "create-doc", args) + Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { + args := buildDocsCreateArgs(runtime) + result, err := common.CallMCPTool(runtime, "create-doc", args) if err != nil { return err } augmentDocsCreateResult(runtime, result) - normalizeDocsUpdateResult(result, runtime.Str("markdown")) + normalizeDocsUpdateResult(result, common.GetString(args, "markdown")) runtime.Out(result, nil) return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_create.go` around lines 55 - 66, The call to normalizeDocsUpdateResult uses the raw runtime.Str("markdown") but buildDocsCreateArgs trims the markdown first; update the Execute block so normalizeDocsUpdateResult receives the same trimmed markdown used by buildDocsCreateArgs (e.g., obtain the trimmed markdown from the args returned by buildDocsCreateArgs or apply the same trim to runtime.Str("markdown")) to match docs_update.go behavior and keep isWhiteboardCreateMarkdown detection consistent; target functions/values: buildDocsCreateArgs, normalizeDocsUpdateResult, and runtime.Str("markdown").shortcuts/doc/docs_update.go (1)
68-90: DryRun and Execute paths duplicate the trim call — consider factoring.Both branches independently call
common.TrimMarkdownCodeBlockTrailingBlanks(runtime.Str("markdown"))with slightly different conditional shapes (line 73–75 gates on the raw string being non-empty; line 97 gates on the trimmed value). Trimming never turns a non-empty string into empty (it only drops blank lines inside fences), so both conditions are equivalent in practice — but extracting a smallbuildDocsUpdateArgs(runtime) (args map[string]interface{}, markdown string)helper (mirroringbuildDocsCreateArgsindocs_create.go) would remove the duplication and guarantee DryRun and Execute stay aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/docs_update.go` around lines 68 - 90, The DryRun block and the Execute path both duplicate trimming of the markdown string; create a helper function buildDocsUpdateArgs(runtime *common.RuntimeContext) (args map[string]interface{}, markdown string) (mirroring buildDocsCreateArgs in docs_create.go) that constructs the args map, calls common.TrimMarkdownCodeBlockTrailingBlanks(runtime.Str("markdown")) once, and returns the args plus the trimmed markdown; then replace the inline arg-building in DryRun (the anonymous DryRun func) and the Execute handler to call buildDocsUpdateArgs and use its returned args/markdown so both paths share the same logic and eliminate the duplicated Trim call referenced in the DryRun closure and the Execute code paths.shortcuts/common/markdown_test.go (1)
8-52: Consider adding a few more edge-case cases.Good coverage for the common scenarios. A couple of inexpensive additions would guard against regressions:
- Empty input (
"") — exercises the early-return path.- Unterminated fence (no closing ``` before EOF) — verifies buffered blanks are flushed rather than dropped.
- Nested/longer fences, e.g. a
```` opener containing a ``` line inside — verifies the closer match uses the opener's size.- Input without a trailing newline on the last line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/markdown_test.go` around lines 8 - 52, Add tests to TestTrimMarkdownCodeBlockTrailingBlanks that cover the empty string case (""), an unterminated fence (e.g., an opening ``` with EOF and trailing blank lines to ensure buffered blanks are preserved), a longer/multi-backtick opener (e.g., a ```` opener that contains a ``` line to ensure the closer matches opener length), and a case where the input does not end with a trailing newline; reference the existing test harness and the TrimMarkdownCodeBlockTrailingBlanks function when adding these new table-driven entries so they run under t.Run alongside the current cases.shortcuts/common/markdown.go (1)
13-42: Fence detection edge cases to be aware of.A couple of non-blocking gotchas in
parseMarkdownFence/isMarkdownFenceClose:
strings.TrimSpacestrips leading whitespace, so an indented-code-block line that happens to start with ` ```` (4+ spaces then backticks) will be misclassified as a fence opener. In practice, Lark's markdown round-trip emits fenced code blocks, so this is unlikely to trigger — but worth noting.- For backtick fences, CommonMark forbids backticks in the info string (e.g.
```foo`bar). You don't validate this, which means such a line could be treated as a fence opener and the closer would then eat content. Again, unlikely in real Lark docs.isMarkdownFenceClosecorrectly requires no info string after the closing run; good.No action required if the scope is intentionally limited to the shapes Lark emits — matches the existing fence-detection posture elsewhere in
shortcuts/doc/markdown_fix.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/markdown.go` around lines 13 - 42, parseMarkdownFence and isMarkdownFenceClose misclassify indented code blocks and don't validate backticks in the info string: update parseMarkdownFence to first detect and reject lines with 4 or more leading spaces (preserve leading whitespace instead of using strings.TrimSpace) so indented code blocks aren't treated as fences, then compute the fence run on the untrimmed suffix; additionally, when the detected fence char is '`', validate that the remainder of the line after the fence run (the info string) does not contain any backticks and reject as a fence if it does; keep isMarkdownFenceClose behavior but ensure it also uses the same leading-whitespace rule (reject closing fences that are indented >=4 spaces) and still requires no trailing non-space content after the closing run. Reference functions: parseMarkdownFence and isMarkdownFenceClose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/common/markdown_test.go`:
- Around line 8-52: Add tests to TestTrimMarkdownCodeBlockTrailingBlanks that
cover the empty string case (""), an unterminated fence (e.g., an opening ```
with EOF and trailing blank lines to ensure buffered blanks are preserved), a
longer/multi-backtick opener (e.g., a ```` opener that contains a ``` line to
ensure the closer matches opener length), and a case where the input does not
end with a trailing newline; reference the existing test harness and the
TrimMarkdownCodeBlockTrailingBlanks function when adding these new table-driven
entries so they run under t.Run alongside the current cases.
In `@shortcuts/common/markdown.go`:
- Around line 13-42: parseMarkdownFence and isMarkdownFenceClose misclassify
indented code blocks and don't validate backticks in the info string: update
parseMarkdownFence to first detect and reject lines with 4 or more leading
spaces (preserve leading whitespace instead of using strings.TrimSpace) so
indented code blocks aren't treated as fences, then compute the fence run on the
untrimmed suffix; additionally, when the detected fence char is '`', validate
that the remainder of the line after the fence run (the info string) does not
contain any backticks and reject as a fence if it does; keep
isMarkdownFenceClose behavior but ensure it also uses the same
leading-whitespace rule (reject closing fences that are indented >=4 spaces) and
still requires no trailing non-space content after the closing run. Reference
functions: parseMarkdownFence and isMarkdownFenceClose.
In `@shortcuts/doc/docs_create.go`:
- Around line 55-66: The call to normalizeDocsUpdateResult uses the raw
runtime.Str("markdown") but buildDocsCreateArgs trims the markdown first; update
the Execute block so normalizeDocsUpdateResult receives the same trimmed
markdown used by buildDocsCreateArgs (e.g., obtain the trimmed markdown from the
args returned by buildDocsCreateArgs or apply the same trim to
runtime.Str("markdown")) to match docs_update.go behavior and keep
isWhiteboardCreateMarkdown detection consistent; target functions/values:
buildDocsCreateArgs, normalizeDocsUpdateResult, and runtime.Str("markdown").
In `@shortcuts/doc/docs_fetch.go`:
- Around line 67-73: Combine the two repeated type-assert blocks that check
result["markdown"] into a single type assertion: perform one check like `md, ok
:= result["markdown"].(string)` and if ok set `result["markdown"]` after
applying both transformations in sequence (first call
common.TrimMarkdownCodeBlockTrailingBlanks(md) and then pass the result to
fixExportedMarkdown), thus avoiding the double map lookup and applying both
transforms before writing back to result.
In `@shortcuts/doc/docs_update.go`:
- Around line 68-90: The DryRun block and the Execute path both duplicate
trimming of the markdown string; create a helper function
buildDocsUpdateArgs(runtime *common.RuntimeContext) (args
map[string]interface{}, markdown string) (mirroring buildDocsCreateArgs in
docs_create.go) that constructs the args map, calls
common.TrimMarkdownCodeBlockTrailingBlanks(runtime.Str("markdown")) once, and
returns the args plus the trimmed markdown; then replace the inline arg-building
in DryRun (the anonymous DryRun func) and the Execute handler to call
buildDocsUpdateArgs and use its returned args/markdown so both paths share the
same logic and eliminate the duplicated Trim call referenced in the DryRun
closure and the Execute code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bca7f9a-47d4-4980-a2da-fc0cc57735af
📒 Files selected for processing (6)
shortcuts/common/markdown.goshortcuts/common/markdown_test.goshortcuts/doc/docs_create.goshortcuts/doc/docs_fetch.goshortcuts/doc/docs_update.goshortcuts/drive/drive_export.go
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
A few notes on top of what CodeRabbit already flagged (the docs_fetch.go double type-assert and the normalizeDocsUpdateResult consistency in docs_create.go). Biggest concern is scope vs. issue #53 — the rest are minor correctness / test-gap / DRY notes.
| func buildDocsCreateArgs(runtime *common.RuntimeContext) map[string]interface{} { | ||
| args := map[string]interface{}{ | ||
| "markdown": runtime.Str("markdown"), | ||
| "markdown": common.TrimMarkdownCodeBlockTrailingBlanks(runtime.Str("markdown")), |
There was a problem hiding this comment.
Does this actually close issue #53?
Issue #53 reports that a freshly-created Feishu document, opened in the Feishu UI, renders an extra blank line at the end of every code block — the user explicitly confirms the markdown sent to the API was already clean. That suggests the blank line is introduced by the MCP create-doc tool or the downstream docx block conversion, not by the CLI-side markdown.
This PR normalises markdown on the CLI side of the boundary (trim on create input, trim on fetch/export output). That will:
- ✅ Prevent accumulation across fetch → edit → update round-trips (good and worth having).
- ✅ Make
docs +fetch/drive +exportoutput look tidy. - ❌ Not change what renders inside Feishu after
docs +create— the docx block created by the server still contains the server-added trailing blank.
If the intent is to close #53 as reported, the trim-on-input is a no-op here (the user's source markdown already didn't have the blank). Could we either (a) verify the fix in Feishu UI after this PR and update the issue, or (b) reword the PR / issue link from "Fixes #53" to "Mitigates round-trip growth related to #53" so it's clear the rendering issue remains open?
|
|
||
| func isMarkdownBlankLine(line string) bool { | ||
| return strings.TrimSpace(strings.TrimRight(line, "\r\n")) == "" | ||
| } |
There was a problem hiding this comment.
strings.TrimRight(line, "\r\n") is redundant here.
strings.TrimSpace already strips \r, \n, \t, and space, so the chained TrimRight(line, "\r\n") has no effect on the result. Can simplify to:
func isMarkdownBlankLine(line string) bool {
return strings.TrimSpace(line) == ""
}| } | ||
|
|
||
| func parseMarkdownFence(line string) (markdownFence, bool) { | ||
| trimmed := strings.TrimSpace(line) |
There was a problem hiding this comment.
TrimSpace over-tolerates indentation — 4-space-indented "fences" are actually indented code block content.
CommonMark allows 0–3 spaces of indentation before a fence marker; 4+ spaces means the line is an indented code block inside the surrounding context and the backticks are literal. Because parseMarkdownFence does strings.TrimSpace(line), a line like " ```" (four leading spaces) is treated as an opening fence, and subsequent lines are then ignored by TrimMarkdownCodeBlockTrailingBlanks until it sees a matching close.
In practice this is mostly academic for markdown coming out of Lark (server output isn't deeply indented), but it can trigger false positives if a user's source passed through docs +create/docs +update nests a literal fence example inside an indented block. Suggest matching CommonMark: count leading spaces, bail out if ≥ 4.
Same applies to isMarkdownFenceClose at L33.
| out = append(out, pendingBlanks...) | ||
| } | ||
| return strings.Join(out, "") | ||
| } |
There was a problem hiding this comment.
Two parallel fence parsers now live in the repo — worth a DRY note for a follow-up.
shortcuts/doc/markdown_fix.go has its own applyOutsideCodeFences that does fence tracking with a simpler, less-robust rule (it only recognises triple backticks, not ~~~, not variable fence length). This new shortcuts/common/markdown.go has more correct fence handling (either char, >= 3 length, matching close length).
Not a blocker for this PR, but once this lands it'd be worth switching applyOutsideCodeFences to drive off parseMarkdownFence / isMarkdownFenceClose so the two implementations can't drift (e.g. the old one doesn't currently handle ~~~ fences at all — see fixExportedMarkdown pipeline). Feel free to file a follow-up and leave this PR as-is.
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
A few test-coverage gaps worth plugging while the test file is fresh:
- CRLF line endings (Windows-authored markdown — the issue reporter is on Windows 11).
- Fences longer than 3 chars:
`````go\n…\n\n`````(5-backtick open, 5-backtick close) must be preserved; a 3-backtick line in the middle is not a close. - Input that does not end with a trailing newline (
SplitAfterretains the final token without\n; worth confirming round-trip is byte-exact). - Unclosed fenced block (fence opened, EOF reached before a close) — current code keeps
pendingBlanksat the end; the assertion just protects against regression. - Tab-only "blank" lines inside code (
\t\n): currently dropped as blank byTrimSpace; confirm that matches intent for code blocks where tab-only lines can have semantic meaning (e.g. Makefiles, whitespace-sensitive DSLs). - Tilde fence with info string:
~~~python\nx\n\n~~~(already have one tilde case, but without info string).
None of these are likely to be broken in the current implementation, but the test file doubles as a spec for future changes.
2 participants
Summary
Fixes #53.
Test plan
go test ./shortcuts/common ./shortcuts/doc ./shortcuts/drivegit diff --cached --checkSummary by CodeRabbit