feat(doc): add --preview flag to docs +update for pre-write self-check#624
Open
herbertliu wants to merge 2 commits intomainfrom
Open
feat(doc): add --preview flag to docs +update for pre-write self-check#624herbertliu wants to merge 2 commits intomainfrom
herbertliu wants to merge 2 commits intomainfrom
Conversation
Case 11 in the pitfall list: docs +update's existing --dry-run prints the request body, which lets Agents verify the RPC shape but says nothing about whether their selection actually resolves in the target doc or where in the doc it lands. The gap shows up as Agents running blind writes, finding they hit the wrong block, and then reverse- engineering via fetch-doc. Add --preview as a content-aware alternative: pre-fetch the document, locate the selection in its markdown, and emit a structured result describing match positions (1-based line ranges), the raw markdown payload that would be sent, and a human-readable note flagging common traps (no match, multi-match under a mode that acts on first match, overwrite discarding everything). Execute short-circuits before the write — the response is purely advisory. append / overwrite skip selection lookup since neither anchors against existing content. The preview operates on the raw fetched markdown, not on the server's block tree, so a miss here is advisory (server may resolve the selection differently). This tradeoff is documented on the matcher helpers and in the --preview flag description. Coverage: 12-case ATX helper table, 4-subtest ellipsis matcher, 4- subtest title matcher, 5-subtest preview-result builder.
📝 WalkthroughWalkthroughAdds a Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Cmd as docs update (--preview)
participant MCP as MCP: fetch-doc
participant Parser as Markdown Parser
participant Matcher as Selection Matcher
participant Preview as Preview Builder
User->>Cmd: run docs update --preview
Cmd->>MCP: fetch current document markdown
MCP-->>Cmd: return markdown snapshot
Cmd->>Parser: parse markdown, infer headings
Parser-->>Cmd: heading metadata
Cmd->>Matcher: match selection (ellipsis or title)
Matcher-->>Cmd: match ranges & snippets
Cmd->>Preview: build previewResult (payload, counts, notes)
Preview-->>Cmd: preview object
Cmd-->>User: emit preview output and exit (no update)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
github-actions
Bot
added
domain/ccm
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #624 +/- ##
==========================================
+ Coverage 60.20% 60.28% +0.08%
==========================================
Files 407 408 +1
Lines 43340 43514 +174
==========================================
+ Hits 26091 26232 +141
- Misses 15221 15252 +31
- Partials 2028 2030 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ee53d9d762bfa5b0a8230b1dc2651b33c2889a30🧩 Skill updatenpx skills add larksuite/cli#feat/docs-update-preview -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: 4
🤖 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/docs_update_preview.go`:
- Around line 119-132: The current loop that builds previewMatch entries
(iterating over lines with indices i and j and appending previewMatch with
StartLine, EndLine and Snippet) can produce false positives when start and end
are on the same line but end appears before start; modify the inner loop so that
before appending a previewMatch you check if i == j and, in that case, compute
the positions of start and end in lines[i] (using strings.Index/LastIndex or
equivalent) and only append if the start position is <= end position (i.e.,
start occurs before end); keep existing behavior for multi-line matches and skip
adding the match when the same-line ordering check fails.
- Around line 221-228: Add a branch to set res.Note when res.MatchCount > 1 and
mode == "replace_all" so the preview includes an advisory for multi-match
replace_all operations; locate the switch that sets res.MatchCount and Note (the
block referencing res.Matches, res.MatchCount and the variable mode) and add a
case for res.MatchCount > 1 && mode == "replace_all" that assigns a helpful
message (similar tone to the existing multi-match message) describing that
replace_all will apply to all matches.
- Around line 166-182: The heading scanner must ignore fenced code blocks: add a
boolean (e.g., inFence) and toggle it when a trimmed line starts with "```" or
"~~~", and if inFence is true skip processing the line so previewAtxHeadingLevel
won't treat fenced-heading-like lines as real headings; apply this fence-check
both in the outer loop that finds the target heading (the for i, line := range
lines block that calls previewAtxHeadingLevel and compares to
targetLevel/targetText) and in the inner section-end scan (the for j := i+1 loop
that sets end) so fenced headings neither match the target nor terminate the
section.
In `@shortcuts/doc/docs_update.go`:
- Around line 108-116: The preview path drops the new title so preview output
can hide title mutations; update the preview result to include the same
new_title field that the real write path sends by extending buildPreviewResult
and the previewResult shape to accept and emit new_title (same value used at
write time), then pass that new_title when constructing preview in the preview
variable before calling runtime.Out(preview, nil) so `--preview --new-title`
reflects the pending title change.
🪄 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: 927f7da3-fe17-4012-a356-f28ba4c4fb08
📒 Files selected for processing (3)
shortcuts/doc/docs_update.goshortcuts/doc/docs_update_preview.goshortcuts/doc/docs_update_preview_test.go
…dings, replace_all note, new_title CodeRabbit review on the preview matcher flagged four edges: 1. findSelectionWithEllipsis on a same-line selection like 'A...B' was accepting lines where B appeared before A, because the scan only checked substring presence not relative position. Track the start offset and require end to appear after it within the same line. 2. findSelectionByTitle treated ATX-heading-looking lines inside '```' or '~~~' fences as real headings, producing false matches (for '## Example' pasted into a code sample) and incorrect section boundaries (fenced headings terminating outer sections). Pre-compute in-fence state per line and skip those rows in both the outer match scan and the inner section-end scan. 3. replace_all with multiple matches left Note empty — exactly the mode where the count is most actionable. Emit 'selection matches N locations; replace_all would apply to all matches'. 4. --new-title was silently dropped in preview output, so a user running --preview --new-title could not see the pending title change. Thread it through buildPreviewResult and surface it as previewResult.NewTitle in the JSON response. Coverage: three new tests pin the fixes — same-line reversed ordering, fenced heading false-positive, replace_all multi-match note — plus the existing table is extended with a new-title propagation assertion.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/docs_update_preview.go`:
- Around line 108-112: The no-ellipsis branch in findSelectionWithEllipsis only
calls findSingleLineMatches so a multi-line selection (contains '\n' but not
'...') is never matched; update findSelectionWithEllipsis to detect when
selection contains a newline and, in that case, search the whole markdown for an
exact substring match (e.g., using strings.Contains/Index) and return the
corresponding previewMatch(s) instead of deferring to findSingleLineMatches;
apply the same fix to the analogous logic around the code at the 148-161 region
and add a regression test that supplies a selection with newline(s) but no "..."
to ensure the exact multi-line match is returned.
- Around line 55-64: fetchMarkdownForPreview currently downcasts
result["markdown"] to string and returns an empty string on failure; instead
detect when the "markdown" key is missing or not a string and return a clear
error so callers can fail the preview. In fetchMarkdownForPreview (calling
common.CallMCPTool), check the type assertion result["markdown"].(string) with
the comma-ok idiom, and if ok is false or the string is empty return a
descriptive error (e.g., fmt.Errorf("fetch-doc returned no markdown for doc %s",
docID)) rather than returning "", nil.
🪄 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: 360672a0-56df-441e-8b61-e894ca2765de
📒 Files selected for processing (3)
shortcuts/doc/docs_update.goshortcuts/doc/docs_update_preview.goshortcuts/doc/docs_update_preview_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/doc/docs_update_preview_test.go
- shortcuts/doc/docs_update.go
Comment on lines
+55
to
+64
| func fetchMarkdownForPreview(runtime *common.RuntimeContext, docID string) (string, error) { | ||
| result, err := common.CallMCPTool(runtime, "fetch-doc", map[string]interface{}{ | ||
| "doc_id": docID, | ||
| "skip_task_detail": true, | ||
| }) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| md, _ := result["markdown"].(string) | ||
| return md, nil |
There was a problem hiding this comment.
Fail preview when fetch-doc does not return markdown.
Line 63 silently treats a missing/non-string markdown field as an empty document, which can produce a misleading “no match” or “0 lines” preview for a malformed response.
Proposed fix
md, _ := result["markdown"].(string)
+ if _, ok := result["markdown"]; !ok {
+ return "", fmt.Errorf("fetch-doc response missing markdown field")
+ }
+ md, ok := result["markdown"].(string)
+ if !ok {
+ return "", fmt.Errorf("fetch-doc response markdown field is not a string")
+ }
return md, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/doc/docs_update_preview.go` around lines 55 - 64,
fetchMarkdownForPreview currently downcasts result["markdown"] to string and
returns an empty string on failure; instead detect when the "markdown" key is
missing or not a string and return a clear error so callers can fail the
preview. In fetchMarkdownForPreview (calling common.CallMCPTool), check the type
assertion result["markdown"].(string) with the comma-ok idiom, and if ok is
false or the string is empty return a descriptive error (e.g.,
fmt.Errorf("fetch-doc returned no markdown for doc %s", docID)) rather than
returning "", nil.
Comment on lines
+108
to
+112
| func findSelectionWithEllipsis(markdown, selection string) []previewMatch { | ||
| lines := strings.Split(markdown, "\n") | ||
| if !strings.Contains(selection, "...") { | ||
| return findSingleLineMatches(lines, selection) | ||
| } |
There was a problem hiding this comment.
Handle exact multi-line selections without ellipsis.
The no-ellipsis path only scans individual lines, so a valid exact selection like first line\nsecond line always reports no match even when it exists in the fetched markdown.
Suggested direction
lines := strings.Split(markdown, "\n")
if !strings.Contains(selection, "...") {
+ if strings.Contains(selection, "\n") {
+ return findMultiLineExactMatches(markdown, selection)
+ }
return findSingleLineMatches(lines, selection)
}Add a regression test with a newline-containing selectionWithEllipsis value that does not include ....
Also applies to: 148-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/doc/docs_update_preview.go` around lines 108 - 112, The no-ellipsis
branch in findSelectionWithEllipsis only calls findSingleLineMatches so a
multi-line selection (contains '\n' but not '...') is never matched; update
findSelectionWithEllipsis to detect when selection contains a newline and, in
that case, search the whole markdown for an exact substring match (e.g., using
strings.Contains/Index) and return the corresponding previewMatch(s) instead of
deferring to findSingleLineMatches; apply the same fix to the analogous logic
around the code at the 148-161 region and add a regression test that supplies a
selection with newline(s) but no "..." to ensure the exact multi-line match is
returned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses Case 11 in the lark-cli pitfall list: the existing `--dry-run` on `docs +update` only prints the request body, so Agents can verify the RPC shape but have no idea whether their selection actually resolves in the target doc. The result is blind writes followed by fetch-doc forensics.
Add `--preview` as a content-aware alternative: pre-fetch the document, locate the selection in its markdown, emit a structured result describing match positions, the raw payload, and human-readable notes for common traps.
Changes
Shape
```bash
lark-cli docs +update --doc --mode replace_range \
--selection-with-ellipsis "old intro...ending here" \
--markdown "new intro" --preview
```
```json
{
"mode": "replace_range",
"doc_id": "",
"match_count": 1,
"matches": [
{"StartLine": 7, "EndLine": 8, "Snippet": "old intro\nending here"}
],
"payload_markdown": "new intro",
"payload_lines": 1,
"note": "selection matches exactly one location"
}
```
Scope notes
Test Plan
Summary by CodeRabbit
New Features
--previewflag todocs +updateso users can see a structured preview of proposed changes (matched locations, computed line counts, and outgoing payload) without modifying the document. Preview supports append/overwrite semantics and clear human-readable notes for 0/1/multiple matches.Tests