feat(doc): warn when callout uses type= without background-color#467
feat(doc): warn when callout uses type= without background-color#467fangshuyu-768 merged 4 commits intomainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
github-actions
Bot
added
domain/ccm
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "CLI runtime"
participant Warn as "WarnCalloutType\n(hint writer -> stderr)"
participant MCP as "MCP create-doc / update-doc"
User->>CLI: invoke docs create/update with --markdown
CLI->>Warn: WarnCalloutType(markdown, stderr)
Warn-->>CLI: (hints printed to stderr, if any)
CLI->>MCP: call create-doc / update-doc with args (includes markdown)
MCP-->>CLI: response/result
CLI-->>User: print result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b33d5c21079107d6894204b79c5a4689832bbba5🧩 Skill updatenpx skills add larksuite/cli#feat/callout-type-to-color -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/doc/markdown_fix.go (1)
38-40: Broaden fence detection for create-time preprocessing.
prepareMarkdownForCreatenow runs on hand-authored markdown, butapplyOutsideCodeFencesonly recognizes bare triple-backtick fences. That can misclassify~~~, 4+-backtick fences, or blockquote-prefixed fences and lead to missed/incorrect transformations around code blocks. Consider centralizing fence parsing (prefix + marker + length) and reusing it here.Based on learnings:
applyOutsideCodeFences ... only recognize bare triple-backtick fences ... accepted limitation ... because fetch-doc export currently emits only bare triple-backtick fences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/markdown_fix.go` around lines 38 - 40, prepareMarkdownForCreate currently delegates to applyOutsideCodeFences which only recognizes bare triple-backtick fences; update the code so fence detection is centralized and supports fence prefix (optional blockquote '>' or spaces), marker char (` or ~), and variable length (3+). Introduce or reuse a shared fence-parsing helper (e.g., parseFence or fenceRegex used by applyOutsideCodeFences) and call it from both applyOutsideCodeFences and prepareMarkdownForCreate (or make applyOutsideCodeFences accept the new parser) so create-time preprocessing correctly skips/handles fences like ~~~, 4+ backticks, and blockquote-prefixed fences while preserving existing behavior for fetch-doc exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/markdown_fix.go`:
- Around line 262-265: fixCalloutType currently uses typeRe :=
regexp.MustCompile(`\btype="([^"]*)"`) which only matches double-quoted
attributes and leaves single-quoted callouts (e.g., <callout type='warning'>)
unchanged; update the regex used in fixCalloutType (variable typeRe and its use
with attrs and tag) to accept either single or double quotes (for example use a
pattern that captures quote with backreference or a pattern like
\btype=(?:'|")([^'"]*)(?:'|")) and adjust how you read the capture groups from
typeParts accordingly so single-quoted and double-quoted type attributes are
both handled.
---
Nitpick comments:
In `@shortcuts/doc/markdown_fix.go`:
- Around line 38-40: prepareMarkdownForCreate currently delegates to
applyOutsideCodeFences which only recognizes bare triple-backtick fences; update
the code so fence detection is centralized and supports fence prefix (optional
blockquote '>' or spaces), marker char (` or ~), and variable length (3+).
Introduce or reuse a shared fence-parsing helper (e.g., parseFence or fenceRegex
used by applyOutsideCodeFences) and call it from both applyOutsideCodeFences and
prepareMarkdownForCreate (or make applyOutsideCodeFences accept the new parser)
so create-time preprocessing correctly skips/handles fences like ~~~, 4+
backticks, and blockquote-prefixed fences while preserving existing behavior for
fetch-doc exports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99b5687c-bc19-48c5-bb18-d0fd4e78ba9a
📒 Files selected for processing (4)
shortcuts/doc/docs_create.goshortcuts/doc/docs_update.goshortcuts/doc/markdown_fix.goshortcuts/doc/markdown_fix_test.go
|
Thanks for the PR and the thorough test coverage! The mapping (e.g. warning → light-yellow) is not defined by any Feishu spec — it's a convention we'd be inventing and then silently applying. Suggested alternatives: File a feature request for create-doc to natively support type= on callout blocks. Happy to discuss further! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/markdown_fix.go`:
- Around line 261-263: The check that skips tags with an explicit background
color is too strict: it only looks for the literal substring "background-color="
using strings.Contains(attrs, "background-color="), so forms like
'background-color = "light-yellow"' or with varied spacing/quotes are missed;
update the detection in the markdown_fix.go logic that currently inspects attrs
(the variable checked and returns tag) to use a whitespace-tolerant match (e.g.,
a regex or normalize attrs by collapsing whitespace) to detect
"background-color" followed by optional spaces and an '='
(background-color\s*=?), and then skip returning the hint when that match is
present. Ensure the change targets the same check that returns tag and continues
to accept other attributes unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8d41789-3d3c-40cb-8744-954cabb9b237
📒 Files selected for processing (4)
shortcuts/doc/docs_create.goshortcuts/doc/docs_update.goshortcuts/doc/markdown_fix.goshortcuts/doc/markdown_fix_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/doc/docs_update.go
- shortcuts/doc/docs_create.go
- shortcuts/doc/markdown_fix_test.go
|
Thanks for the detailed review and the thoughtful suggestions! You're right — silently rewriting user Markdown is the wrong approach here. The latest commit (011da3d) replaces
This surfaces the issue visibly without any silent transformation. On the longer term, filing a feature request for |
herbertliu
force-pushed
the
feat/callout-type-to-color
branch
from
011da3d to
24fd1b7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/doc/markdown_fix.go (1)
261-266:⚠️ Potential issue | 🟡 MinorMake attribute matching whitespace-tolerant to avoid false hints/misses.
Line 262 only detects the exact token
background-color=, and Line 265 requirestype=with no surrounding spaces. Valid forms likebackground-color = "light-yellow"ortype = 'warning'can be misclassified.🔧 Suggested patch
var calloutOpenTagRe = regexp.MustCompile(`<callout(\s[^>]*)?>`) -var calloutTypeAttrRe = regexp.MustCompile(`\btype=(?:"([^"]*)"|'([^']*)')`) +var calloutTypeAttrRe = regexp.MustCompile(`\btype\s*=\s*(?:"([^"]*)"|'([^']*)')`) +var calloutBackgroundColorAttrRe = regexp.MustCompile(`\bbackground-color\s*=`) func WarnCalloutType(md string, w io.Writer) { calloutOpenTagRe.ReplaceAllStringFunc(md, func(tag string) string { attrs := "" if m := calloutOpenTagRe.FindStringSubmatch(tag); len(m) == 2 { attrs = m[1] } // Skip tags that already carry an explicit background-color. - if strings.Contains(attrs, "background-color=") { + if calloutBackgroundColorAttrRe.MatchString(attrs) { return tag }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/markdown_fix.go` around lines 261 - 266, The attribute checks in markdown_fix.go are too strict: replace the exact token checks so they tolerate optional whitespace around `=` and quotes; specifically, update the background-color test (currently using strings.Contains(attrs, "background-color=")) to use a whitespace-tolerant match (e.g., a regex like `background-color\s*=`) and adjust the calloutTypeAttrRe usage or its pattern so calloutTypeAttrRe.FindStringSubmatch(attrs) accepts `type` with spaces (e.g., `type\s*=\s*["']?(\w+)["']?`), ensuring both background-color and type attributes are correctly detected regardless of surrounding spaces or quote style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/doc/markdown_fix.go`:
- Around line 261-266: The attribute checks in markdown_fix.go are too strict:
replace the exact token checks so they tolerate optional whitespace around `=`
and quotes; specifically, update the background-color test (currently using
strings.Contains(attrs, "background-color=")) to use a whitespace-tolerant match
(e.g., a regex like `background-color\s*=`) and adjust the calloutTypeAttrRe
usage or its pattern so calloutTypeAttrRe.FindStringSubmatch(attrs) accepts
`type` with spaces (e.g., `type\s*=\s*["']?(\w+)["']?`), ensuring both
background-color and type attributes are correctly detected regardless of
surrounding spaces or quote style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe0a4202-ac68-4b52-9bcd-3ecd2721b210
📒 Files selected for processing (4)
shortcuts/doc/docs_create.goshortcuts/doc/docs_update.goshortcuts/doc/markdown_fix.goshortcuts/doc/markdown_fix_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/markdown_fix_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/doc/docs_update.go
- shortcuts/doc/docs_create.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/doc/markdown_fix.go (1)
261-287: Nit:ReplaceAllStringFuncused for side effects only.The returned string is discarded;
calloutOpenTagRe.FindAllStringSubmatchIndex(or iteratingFindAllStringSubmatch) would express intent more directly and also skip the redundant secondFindStringSubmatchon each matched tag (you already have the capture from the outer find). Purely stylistic — no behavior change needed.♻️ Optional refactor
-func WarnCalloutType(md string, w io.Writer) { - calloutOpenTagRe.ReplaceAllStringFunc(md, func(tag string) string { - attrs := "" - if m := calloutOpenTagRe.FindStringSubmatch(tag); len(m) == 2 { - attrs = m[1] - } - // Skip tags that already carry an explicit background-color. - if calloutBackgroundColorAttrRe.MatchString(attrs) { - return tag - } - parts := calloutTypeAttrRe.FindStringSubmatch(attrs) - if len(parts) < 2 { - return tag // no type= attribute - } - // parts[1] is the double-quoted capture, parts[2] is single-quoted. - typeName := parts[1] - if typeName == "" { - typeName = parts[2] - } - colors, ok := calloutTypeColors[typeName] - if !ok { - return tag // unknown type — no hint to give - } - fmt.Fprintf(w, - "hint: callout type=%q has no background-color; consider: background-color=%q border-color=%q\n", - typeName, colors[0], colors[1]) - return tag - }) -} +func WarnCalloutType(md string, w io.Writer) { + for _, m := range calloutOpenTagRe.FindAllStringSubmatch(md, -1) { + attrs := m[1] + if calloutBackgroundColorAttrRe.MatchString(attrs) { + continue + } + parts := calloutTypeAttrRe.FindStringSubmatch(attrs) + if len(parts) < 3 { + continue + } + typeName := parts[1] + if typeName == "" { + typeName = parts[2] + } + colors, ok := calloutTypeColors[typeName] + if !ok { + continue + } + fmt.Fprintf(w, + "hint: callout type=%q has no background-color; consider: background-color=%q border-color=%q\n", + typeName, colors[0], colors[1]) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/markdown_fix.go` around lines 261 - 287, Replace the ReplaceAllStringFunc usage (calloutOpenTagRe.ReplaceAllStringFunc) which discards its returned string with a direct iteration over matches using calloutOpenTagRe.FindAllStringSubmatch (or FindAllStringSubmatchIndex) so you can use the captured attrs directly (avoid calling calloutOpenTagRe.FindStringSubmatch again); for each match, skip when calloutBackgroundColorAttrRe matches, extract the type via calloutTypeAttrRe from the already-captured groups, look up calloutTypeColors[typeName], and write the hint to w when appropriate—this makes the intent explicit and removes the redundant regex call.
🤖 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/doc/markdown_fix.go`:
- Around line 261-287: Replace the ReplaceAllStringFunc usage
(calloutOpenTagRe.ReplaceAllStringFunc) which discards its returned string with
a direct iteration over matches using calloutOpenTagRe.FindAllStringSubmatch (or
FindAllStringSubmatchIndex) so you can use the captured attrs directly (avoid
calling calloutOpenTagRe.FindStringSubmatch again); for each match, skip when
calloutBackgroundColorAttrRe matches, extract the type via calloutTypeAttrRe
from the already-captured groups, look up calloutTypeColors[typeName], and write
the hint to w when appropriate—this makes the intent explicit and removes the
redundant regex call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 410deed8-c468-499b-b13e-aee867ffa0a3
📒 Files selected for processing (2)
shortcuts/doc/markdown_fix.goshortcuts/doc/markdown_fix_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/markdown_fix_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #467 +/- ##
==========================================
+ Coverage 63.20% 64.51% +1.31%
==========================================
Files 491 516 +25
Lines 42237 45765 +3528
==========================================
+ Hits 26694 29527 +2833
- Misses 13211 13652 +441
- Partials 2332 2586 +254 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Direction looks right — the switch from rewriting markdown to hint-only is a healthier boundary. A few notes on the current implementation, plus a PR-description update request.
herbertliu
force-pushed
the
feat/callout-type-to-color
branch
from
7d4b556 to
2d8285e
Compare
…order-color When users write <callout type="warning" emoji="📝"> without an explicit background-color, the Feishu doc renders the block with no color. This commit adds fixCalloutType() which maps the semantic type= attribute to the corresponding background-color/border-color pair accepted by create-doc. - warning → light-yellow/yellow - info/note → light-blue/blue - tip/success/check → light-green/green - error/danger → light-red/red - caution → light-orange/orange - important → light-purple/purple Explicit background-color or border-color attributes are always preserved. The fix is applied via prepareMarkdownForCreate() in both +create and +update paths, and also inside fixExportedMarkdown() for round-trip fidelity.
…output Per reviewer feedback (SunPeiYang996), silently rewriting user Markdown is the wrong layer for this adaptation. The type→color mapping is not part of the Feishu spec, and covert transforms make debugging harder. Replace fixCalloutType() (which rewrote the Markdown) with WarnCalloutType() which leaves the Markdown unchanged and instead writes a hint line to stderr for each callout tag that has type= but no background-color, telling the user the recommended explicit attributes to add: hint: callout type="warning" has no background-color; consider: background-color="light-yellow" border-color="yellow" Also fixes CodeRabbit feedback: the type= regex now accepts both single-quoted and double-quoted attribute values (type='warning' and type="warning").
CodeRabbit flagged that the previous strings.Contains(attrs, "background-color=") check missed forms like 'background-color = "light-red"' with whitespace around the equals sign. Replace with a regex that tolerates optional whitespace, and add a regression test.
herbertliu
force-pushed
the
feat/callout-type-to-color
branch
from
2d8285e to
1950398
Compare
PR #467's review thread had three substantive comments (`fangshuyu-768`, 2026-04-21) that the prior reply messages claimed were fixed in commit 7d4b556 — but that commit no longer exists on the branch (lost in a rebase / squash), and the head still ships the original buggy code. This commit makes the fixes real. Three behavior fixes in shortcuts/doc/markdown_fix.go: 1. (#5) Tighten the type= and background-color= regex anchors. \b sits at any word/non-word boundary, and `-` is a non-word char, so `\btype=` also matched the suffix of `data-type=` — a tag like `<callout data-type="warning">` would emit a bogus light-yellow hint. Switched both regexes to `(?:^|\s)…` so a real attribute separator is required. The same anchor on background-color closes the symmetric case where a `data-background-color=` attribute would silently suppress the real hint. 2. (#4) WarnCalloutType is now a fence-aware line walker. Previously the regex ran over the entire markdown body, so a callout sample inside a documentation code fence (```markdown … ```) would generate a phantom stderr hint every time the docs mentioned the feature. The walker tracks fence state via the existing codeFenceOpenMarker / isCodeFenceClose helpers from docs_update_check.go, which handle both backtick and tilde fences per CommonMark §4.5. 3. (#3) Drop the ReplaceAllStringFunc-as-iterator pattern. The previous code routed callout iteration through a rewrite primitive whose rebuilt-string return value was discarded, then ran the same regex a second time inside the callback to recover the capture groups. New scanCalloutTagsForWarning helper uses FindAllStringSubmatch — one pass, no thrown-away allocation, intent matches the surface (read-only scan, not a mutator). Tests: 5 new TestWarnCalloutType subtests pin each contract: - data-type attribute does not trigger hint (#5) - data-background-color does not suppress hint (#5, symmetric) - callout inside backtick fence emits no hint (#4) - callout inside tilde fence emits no hint (#4) - callout after fence close still emits hint (#4, fence-state reset) All 14 TestWarnCalloutType cases pass; go vet / golangci-lint --new-from-rev=origin/main both clean.
fangshuyu-768
changed the title
3 participants
Summary
When a user writes
<callout type="warning" emoji="📝">in markdown passed todocs +create/docs +update, the Feishu side renders the callout block with no color —type=is a semantic shorthand thecreate-docAPI does not consume; it only understandsbackground-color=/border-color=.This PR is a diagnostic, not a renderer fix: it does not rewrite the markdown. Instead it scans the input and, when
type=is used without an explicitbackground-color=, prints one hint line on stderr telling the user exactly which color attributes to write to get the color they probably wanted. The next manual edit fixes the document; the markdown reaching the API is byte-identical to what the user passed.After review by @SunPeiYang996 the original approach (silently expanding
type=into the matching color pair inside the shortcut) was rejected as protocol-adaptation logic in the wrong layer. The hint-on-stderr design adopted here is option 3 of his suggested alternatives.Behavior
create-docis unmodified.Changes
shortcuts/doc/markdown_fix.go— addWarnCalloutType(md, w io.Writer). Fence-aware line walker that scans<callout …>opening tags; for each tag with a recognizedtype=but nobackground-color=, writes one hint line tow. Read-only; markdown is never modified.shortcuts/doc/docs_create.go/shortcuts/doc/docs_update.go— invokeWarnCalloutType(runtime.Str("markdown"), runtime.IO().ErrOut)from the v1 Execute path, immediately before the API call. v2 and--dry-runpaths are intentionally not wired (out of scope; can be revisited if/when the same pattern proves useful there).shortcuts/doc/markdown_fix_test.go— 14TestWarnCalloutTypesubtests covering emits-hint, suppression, fence skipping, attribute-prefix isolation, and regressions for false positives.Robustness (review fixes from @fangshuyu-768)
(?:^|\s)type=and(?:^|\s)background-color\s*=— replace\b.\bsits at any word/non-word boundary, and-is a non-word character, so\btype=also matches the suffix ofdata-type=. Result before fix:<callout data-type="warning">would emit a phantomlight-yellowhint, and a futuredata-background-color=attribute would silently suppress the real hint. The new anchor requires a real attribute separator.codeFenceOpenMarker/isCodeFenceClosehelpers (CommonMark §4.5, both```and~~~). A documentation sample inside a fenced block no longer produces a phantom hint.scanCalloutTagsForWarningextracted as aFindAllStringSubmatch-based per-line scanner. Replaces the priorReplaceAllStringFunc-as-iterator pattern (rebuilt string was discarded; a second regex execution inside the callback recovered the capture groups). Same observable behavior, half the work.=tolerated —background-color = "..."is recognized as a present attribute, same asbackground-color="...".type → suggested color (advisory mapping used in hint text)
warninglight-yellowyellowinfo/notelight-bluebluetip/success/checklight-greengreenerror/dangerlight-redredcautionlight-orangeorangeimportantlight-purplepurpleThis mapping is the CLI's own convention and is used only to compose the hint string. The markdown is never modified, so callers and downstream MCP clients are not asked to know or replicate the mapping.
Test Plan
go test ./shortcuts/doc/...— 14TestWarnCalloutTypecases pass (incl. 5 review-driven regressions:data-typenot triggering,data-background-colornot suppressing, backtick fence skip, tilde fence skip, fence-state reset)go test -race ./shortcuts/doc/...— cleango vet ./...cleangofmt -l .cleangolangci-lint run --new-from-rev=origin/main— 0 issuesgo test $(go list ./... | grep -v cli_e2e) -count=1) — all packages greengo test ./tests/cli_e2e/docs/...) — passesOut of scope / follow-up
type=support insidecreate-docwould obsolete this hint entirely; that is option 1 of @SunPeiYang996's review and the right long-term fix.WarnCalloutTypeinto the v2 path or into--dry-runis a small follow-up if the diagnostic proves useful.