feat(doc): expand callout type= shorthand into background-color and border-color#467
feat(doc): expand callout type= shorthand into background-color and border-color#467herbertliu wants to merge 4 commits intomainfrom
Conversation
…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.
![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@7d4b556f155c4704d3d8abb6dae18320d89a3848🧩 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! |
…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").
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
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.
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 60.74% 60.78% +0.03%
==========================================
Files 394 394
Lines 33910 33948 +38
==========================================
+ Hits 20600 20634 +34
- Misses 11400 11403 +3
- Partials 1910 1911 +1 ☔ 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.
- Tighten calloutTypeAttrRe boundary: the previous \btype= matched data-type="..." because \b sits between - (non-word) and t (word). Use (?:^|\s)type= so the attribute is only matched after start-of- string or whitespace. Regression test added. - Stop using ReplaceAllStringFunc purely as an iterator with its result discarded. Extracted scanCalloutTagsForWarning() that walks FindAllStringIndex matches and reports via the writer only. Intent is now self-evident. - Skip callout tags inside fenced code blocks. Previously a documentation sample like ```markdown <callout type="warning"> ``` would trigger a spurious hint. Walk lines with codeFenceOpenMarker / isCodeFenceClose (shared helpers from the docs +update checks) so both backtick and tilde fences are honored. Three regression tests added: backtick fence, tilde fence, and mixed prose + fence (prose hit still flags).
3 participants
Summary
When users write
<callout type="warning" emoji="📝">without an explicitbackground-color, the Feishu doc renders the callout block with no color.type=is a semantic shorthand — the Feishu API actually needsbackground-color/border-color.After Code Review (SunPeiYang996), the approach was changed from silently rewriting markdown to emitting a hint on stderr, keeping user input untouched and the CLI single-purpose.
Changes
shortcuts/doc/markdown_fix.go: addWarnCalloutType(md, w io.Writer)which scans callout opening tags and, for each tag that has a recognizedtype=but nobackground-color=, writes a single hint line towsuggesting the explicit Feishu color attributes. Markdown is not modified.shortcuts/doc/docs_create.goandshortcuts/doc/docs_update.go: wireWarnCalloutType(runtime.Str("markdown"), runtime.IO().ErrOut)into both DryRun and Execute paths.shortcuts/doc/markdown_fix_test.go: 14 table-driven cases covering emits-hint, suppression, boundary conditions, and regressions for false positives.Robustness fixes (follow-up to
fangshuyu-768review)calloutTypeAttrReboundary tightened from\btype=to(?:^|\s)type=sodata-type="warning"is no longer mistakenly matched.scanCalloutTagsForWarning()extracted fromWarnCalloutTypeand usesFindAllStringIndex+ a range loop (replaces the priorReplaceAllStringFuncwhose result was discarded).WarnCalloutTypenow walks lines withcodeFenceOpenMarker/isCodeFenceCloseso callout samples inside backtick or tilde fences are skipped — documentation that shows a literal<callout type="warning">no longer produces a spurious hint.calloutBackgroundColorAttrRetolerates whitespace around=(background-color = "..."is treated as present).type → suggested color
warninglight-yellowyellowinfo/notelight-bluebluetip/success/checklight-greengreenerror/dangerlight-redredcautionlight-orangeorangeimportantlight-purplepurpleTest Plan
go test ./shortcuts/doc/...)go vet ./...cleangofmt -l .cleangolangci-lint run --new-from-rev=origin/main— 0 issuesRelated Issues
type=semantic shorthand.🤖 Generated with Claude Code