feat: add markdown +diff shortcut#876
Open
wittam-01 wants to merge 2 commits into
Open
Conversation
Change-Id: I87bb32c86e3c3362f541ccc6320c656eb795ec9b
📝 WalkthroughWalkthroughAdds Drive file version management shortcuts (+version-history, +version-get, +version-revert, +version-delete) and a Markdown diff shortcut (+diff). Changes include implementations, validation, file I/O, unified-diff logic, helpers, unit and e2e tests, SKILL/reference docs, go.mod dependency, and a default endpoint update to feishu-pre.cn. ChangesDrive Version Management
Markdown Diff Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
![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 (2)
tests/cli_e2e/drive/drive_version_workflow_test.go (1)
75-86: ⚡ Quick winStrengthen history assertions to validate actual versioning behavior.
Right now this only checks success envelopes. After the overwrite step, assert the history payload contains expected version records (for example, at least one/two items) so this test verifies behavior, not just endpoint availability.
As per coding guidelines, live E2E tests for new flows “must validate real API round-trips, be self-contained (create->use->cleanup)”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli_e2e/drive/drive_version_workflow_test.go` around lines 75 - 86, The test currently only checks envelopes; update the history assertions to validate actual version records by parsing historyResult.Stdout (from clie2e.RunCmd response) into the history payload returned by the "+version-history" call and assert expected versioning behavior (e.g., that the returned versions slice/array contains at least N entries after the overwrite step, or contains entries matching known metadata); locate the test around historyResult and use Request Args containing "+version-history" and the fileToken to extract and unmarshal the payload, then add assertions (using require/Assert helpers) to check length and/or expected fields to ensure a real round-trip was validated.shortcuts/drive/drive_version_test.go (1)
180-185: ⚡ Quick winPrefer structured JSON assertions over substring matching.
These checks are fragile against formatting-only JSON output changes. Decode stdout and assert fields directly to keep tests behavior-focused.
Refactor pattern (example)
- if !strings.Contains(stdout.String(), `"file_name": "report-v7.md"`) { - t.Fatalf("stdout missing file_name: %s", stdout.String()) - } - if !strings.Contains(stdout.String(), `"content": "# hello\n"`) { - t.Fatalf("stdout missing content: %s", stdout.String()) - } + var envelope struct { + Data struct { + FileName string `json:"file_name"` + Content string `json:"content"` + } `json:"data"` + } + if err := json.Unmarshal(stdout.Bytes(), &envelope); err != nil { + t.Fatalf("unmarshal stdout: %v", err) + } + if envelope.Data.FileName != "report-v7.md" { + t.Fatalf("file_name = %q, want %q", envelope.Data.FileName, "report-v7.md") + } + if envelope.Data.Content != "# hello\n" { + t.Fatalf("content = %q, want %q", envelope.Data.Content, "# hello\n") + }Also applies to: 211-216, 355-357, 386-388
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/drive/drive_version_test.go` around lines 180 - 185, Replace fragile substring checks on stdout.String() with proper JSON decoding and field assertions: parse stdout (use json.Unmarshal into a map[string]interface{} or small struct) and assert that the "version" key equals "7633658129540910621" and that "saved_path" exists/non-empty; update the checks around the stdout variable in the current test and replicate the same pattern for the other occurrences referenced (lines 211-216, 355-357, 386-388) so tests validate structured fields rather than substrings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/cli_e2e/drive/drive_version_dryrun_test.go`:
- Around line 143-207: Add a user-mode no-`--output` test to
TestDriveVersionDryRunSupportsUser: in the tests slice (variable tests) add a
new case (e.g., name "get-stdout-default") that mirrors the existing "get" case
but omits the "--output" arg (keep "--dry-run") and include the same expectation
used in TestDriveVersionGetDryRunWithoutOutputUsesStdout (e.g., wantContains
should check that the dry-run payload resolves output to stdout, such as
`"output": "-"` or the same marker used in the bot test).
---
Nitpick comments:
In `@shortcuts/drive/drive_version_test.go`:
- Around line 180-185: Replace fragile substring checks on stdout.String() with
proper JSON decoding and field assertions: parse stdout (use json.Unmarshal into
a map[string]interface{} or small struct) and assert that the "version" key
equals "7633658129540910621" and that "saved_path" exists/non-empty; update the
checks around the stdout variable in the current test and replicate the same
pattern for the other occurrences referenced (lines 211-216, 355-357, 386-388)
so tests validate structured fields rather than substrings.
In `@tests/cli_e2e/drive/drive_version_workflow_test.go`:
- Around line 75-86: The test currently only checks envelopes; update the
history assertions to validate actual version records by parsing
historyResult.Stdout (from clie2e.RunCmd response) into the history payload
returned by the "+version-history" call and assert expected versioning behavior
(e.g., that the returned versions slice/array contains at least N entries after
the overwrite step, or contains entries matching known metadata); locate the
test around historyResult and use Request Args containing "+version-history" and
the fileToken to extract and unmarshal the payload, then add assertions (using
require/Assert helpers) to check length and/or expected fields to ensure a real
round-trip was validated.
🪄 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: 7f3ed2a7-86ac-406d-927a-c0a818e6c2f4
📒 Files selected for processing (12)
internal/core/types.goshortcuts/drive/drive_version.goshortcuts/drive/drive_version_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-version-delete.mdskills/lark-drive/references/lark-drive-version-get.mdskills/lark-drive/references/lark-drive-version-history.mdskills/lark-drive/references/lark-drive-version-revert.mdtests/cli_e2e/drive/drive_version_dryrun_test.gotests/cli_e2e/drive/drive_version_workflow_test.go
Comment on lines
+143
to
+207
| func TestDriveVersionDryRunSupportsUser(t *testing.T) { | ||
| clie2e.SkipWithoutUserToken(t) | ||
| setDriveDryRunConfigEnv(t) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| args []string | ||
| wantContains []string | ||
| }{ | ||
| { | ||
| name: "history", | ||
| args: []string{ | ||
| "drive", "+version-history", | ||
| "--file-token", "boxcnHistoryDryRunUser", | ||
| "--limit", "5", | ||
| "--cursor", "1777013761763", | ||
| "--dry-run", | ||
| }, | ||
| wantContains: []string{ | ||
| "/open-apis/drive/v1/files/boxcnHistoryDryRunUser/history", | ||
| `"only_tag": true`, | ||
| `"page_size": 5`, | ||
| }, | ||
| }, | ||
| { | ||
| name: "get", | ||
| args: []string{ | ||
| "drive", "+version-get", | ||
| "--file-token", "boxcnVersionDryRunUser", | ||
| "--version", "7633658129540910621", | ||
| "--output", "./artifact-user.bin", | ||
| "--dry-run", | ||
| }, | ||
| wantContains: []string{ | ||
| "/open-apis/drive/v1/files/boxcnVersionDryRunUser/download", | ||
| `"version": "7633658129540910621"`, | ||
| `"output": "./artifact-user.bin"`, | ||
| }, | ||
| }, | ||
| { | ||
| name: "revert", | ||
| args: []string{ | ||
| "drive", "+version-revert", | ||
| "--file-token", "boxcnVersionDryRunUser", | ||
| "--version", "7633658129540910621", | ||
| "--dry-run", | ||
| }, | ||
| wantContains: []string{ | ||
| "/open-apis/drive/v1/files/boxcnVersionDryRunUser/revert", | ||
| `"version": "7633658129540910621"`, | ||
| }, | ||
| }, | ||
| { | ||
| name: "delete", | ||
| args: []string{ | ||
| "drive", "+version-delete", | ||
| "--file-token", "boxcnVersionDryRunUser", | ||
| "--version", "7633658129540910621", | ||
| "--dry-run", | ||
| }, | ||
| wantContains: []string{ | ||
| "/open-apis/drive/v1/files/boxcnVersionDryRunUser/version_del", | ||
| `"version": "7633658129540910621"`, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Add user-mode parity coverage for +version-get default stdout behavior.
TestDriveVersionGetDryRunWithoutOutputUsesStdout currently validates bot mode only. Please add the same no---output case in the user table to catch auth-specific regressions in default output resolution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli_e2e/drive/drive_version_dryrun_test.go` around lines 143 - 207, Add
a user-mode no-`--output` test to TestDriveVersionDryRunSupportsUser: in the
tests slice (variable tests) add a new case (e.g., name "get-stdout-default")
that mirrors the existing "get" case but omits the "--output" arg (keep
"--dry-run") and include the same expectation used in
TestDriveVersionGetDryRunWithoutOutputUsesStdout (e.g., wantContains should
check that the dry-run payload resolves output to stdout, such as `"output":
"-"` or the same marker used in the bot test).
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f69c3bafd84e1c37371e480c0729ea25736558b1🧩 Skill updatenpx skills add larksuite/cli#feat/markdown-diff -y -g |
github-actions
Bot
added
size/XL
wittam-01
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/markdown/markdown_diff_test.go`:
- Around line 19-272: For each test in this file (e.g.,
TestMarkdownDiffRejectsUnsupportedFormat,
TestMarkdownDiffRejectsToVersionWithoutFromVersion,
TestMarkdownDiffRemoteVsRemoteJSON, TestMarkdownDiffRemoteVsLocalPretty,
TestMarkdownDiffRemoteVsRemoteJSONMultipleHunks,
TestMarkdownDiffNoChangesPretty, TestMarkdownDiffDryRunRemoteVsLocal) set an
isolated config dir before calling cmdutil.TestFactory by adding
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) immediately at the start of
the test; this ensures config-state isolation across tests and should be applied
consistently in each test function that currently calls cmdutil.TestFactory(...)
in this file.
🪄 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: 275941d6-61a2-4fd1-9b17-c992911af119
📒 Files selected for processing (11)
shortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff.goshortcuts/markdown/markdown_diff_test.goshortcuts/markdown/markdown_test.goshortcuts/markdown/shortcuts.goshortcuts/register_markdown_test.goskills/lark-drive/SKILL.mdskills/lark-markdown/SKILL.mdskills/lark-markdown/references/lark-markdown-diff.mdtests/cli_e2e/markdown/markdown_dryrun_test.gotests/cli_e2e/markdown/markdown_workflow_test.go
✅ Files skipped from review due to trivial changes (4)
- shortcuts/markdown/markdown_test.go
- skills/lark-markdown/SKILL.md
- shortcuts/markdown/shortcuts.go
- skills/lark-markdown/references/lark-markdown-diff.md
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/lark-drive/SKILL.md
Comment on lines
+19
to
+272
| func TestMarkdownDiffRejectsUnsupportedFormat(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, markdownTestConfig()) | ||
|
|
||
| err := mountAndRunMarkdown(t, MarkdownDiff, []string{ | ||
| "+diff", | ||
| "--file-token", "box_md_diff", | ||
| "--from-version", "7633658129540910621", | ||
| "--format", "table", | ||
| }, f, stdout) | ||
| if err == nil || !strings.Contains(err.Error(), "only supports --format json or pretty") { | ||
| t.Fatalf("expected format validation error, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestMarkdownDiffRejectsToVersionWithoutFromVersion(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, markdownTestConfig()) | ||
|
|
||
| err := mountAndRunMarkdown(t, MarkdownDiff, []string{ | ||
| "+diff", | ||
| "--file-token", "box_md_diff", | ||
| "--to-version", "7633658129540910628", | ||
| }, f, stdout) | ||
| if err == nil || !strings.Contains(err.Error(), "--to-version requires --from-version") { | ||
| t.Fatalf("expected version validation error, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestMarkdownDiffRemoteVsRemoteJSON(t *testing.T) { | ||
| f, stdout, _, reg := cmdutil.TestFactory(t, markdownTestConfig()) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/drive/v1/files/box_md_diff/download?version=7633658129540910621", | ||
| Status: 200, | ||
| RawBody: []byte("# Title\n\n- alpha\n- beta\n"), | ||
| Headers: http.Header{ | ||
| "Content-Disposition": []string{`attachment; filename="README.md"`}, | ||
| }, | ||
| }) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/drive/v1/files/box_md_diff/download?version=7633658129540910628", | ||
| Status: 200, | ||
| RawBody: []byte("# Title\n\n- alpha\n- beta updated\n- gamma\n"), | ||
| Headers: http.Header{ | ||
| "Content-Disposition": []string{`attachment; filename="README.md"`}, | ||
| }, | ||
| }) | ||
|
|
||
| err := mountAndRunMarkdown(t, MarkdownDiff, []string{ | ||
| "+diff", | ||
| "--file-token", "box_md_diff", | ||
| "--from-version", "7633658129540910621", | ||
| "--to-version", "7633658129540910628", | ||
| "--as", "bot", | ||
| }, f, stdout) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| var env struct { | ||
| OK bool `json:"ok"` | ||
| Data struct { | ||
| Changed bool `json:"changed"` | ||
| Mode string `json:"mode"` | ||
| FromVersion string `json:"from_version"` | ||
| ToVersion string `json:"to_version"` | ||
| AddedLines int `json:"added_lines"` | ||
| DeletedLines int `json:"deleted_lines"` | ||
| Diff string `json:"diff"` | ||
| Hunks []markdownDiffHunk `json:"hunks"` | ||
| } `json:"data"` | ||
| } | ||
| if err := json.Unmarshal(stdout.Bytes(), &env); err != nil { | ||
| t.Fatalf("json unmarshal error: %v\n%s", err, stdout.String()) | ||
| } | ||
| if !env.OK { | ||
| t.Fatalf("expected ok=true, got false: %s", stdout.String()) | ||
| } | ||
| if !env.Data.Changed { | ||
| t.Fatalf("expected changed=true: %s", stdout.String()) | ||
| } | ||
| if env.Data.Mode != markdownDiffModeRemoteVsRemote { | ||
| t.Fatalf("mode = %q, want %q", env.Data.Mode, markdownDiffModeRemoteVsRemote) | ||
| } | ||
| if env.Data.FromVersion != "7633658129540910621" || env.Data.ToVersion != "7633658129540910628" { | ||
| t.Fatalf("versions = %q -> %q", env.Data.FromVersion, env.Data.ToVersion) | ||
| } | ||
| if env.Data.AddedLines != 2 || env.Data.DeletedLines != 1 { | ||
| t.Fatalf("added/deleted = %d/%d, want 2/1", env.Data.AddedLines, env.Data.DeletedLines) | ||
| } | ||
| if len(env.Data.Hunks) != 1 { | ||
| t.Fatalf("len(hunks) = %d, want 1", len(env.Data.Hunks)) | ||
| } | ||
| if !strings.Contains(env.Data.Diff, "@@") || !strings.Contains(env.Data.Diff, "+- gamma") { | ||
| t.Fatalf("diff missing expected content: %s", env.Data.Diff) | ||
| } | ||
| } | ||
|
|
||
| func TestMarkdownDiffRemoteVsLocalPretty(t *testing.T) { | ||
| f, stdout, _, reg := cmdutil.TestFactory(t, markdownTestConfig()) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/drive/v1/files/box_md_diff/download", | ||
| Status: 200, | ||
| RawBody: []byte("# Title\n\nhello old\n"), | ||
| Headers: http.Header{ | ||
| "Content-Disposition": []string{`attachment; filename="README.md"`}, | ||
| }, | ||
| }) | ||
|
|
||
| tmpDir := t.TempDir() | ||
| withMarkdownWorkingDir(t, tmpDir) | ||
| if err := os.WriteFile("local.md", []byte("# Title\n\nhello new\n"), 0o644); err != nil { | ||
| t.Fatalf("WriteFile() error: %v", err) | ||
| } | ||
|
|
||
| err := mountAndRunMarkdown(t, MarkdownDiff, []string{ | ||
| "+diff", | ||
| "--file-token", "box_md_diff", | ||
| "--file", "./local.md", | ||
| "--format", "pretty", | ||
| "--as", "bot", | ||
| }, f, stdout) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if !strings.Contains(stdout.String(), "@@") { | ||
| t.Fatalf("pretty output missing hunk header: %s", stdout.String()) | ||
| } | ||
| if !strings.Contains(stdout.String(), output.Red+"-hello old"+output.Reset) { | ||
| t.Fatalf("pretty output missing removed line color: %q", stdout.String()) | ||
| } | ||
| if !strings.Contains(stdout.String(), output.Green+"+hello new"+output.Reset) { | ||
| t.Fatalf("pretty output missing added line color: %q", stdout.String()) | ||
| } | ||
| } | ||
|
|
||
| func TestMarkdownDiffRemoteVsRemoteJSONMultipleHunks(t *testing.T) { | ||
| f, stdout, _, reg := cmdutil.TestFactory(t, markdownTestConfig()) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/drive/v1/files/box_md_diff/download?version=7633658129540910621", | ||
| Status: 200, | ||
| RawBody: []byte("line1\nline2\nline3\nline4\nline5\nline6\n"), | ||
| Headers: http.Header{ | ||
| "Content-Disposition": []string{`attachment; filename="README.md"`}, | ||
| }, | ||
| }) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/drive/v1/files/box_md_diff/download?version=7633658129540910628", | ||
| Status: 200, | ||
| RawBody: []byte("line1\nline2 changed\nline3\nline4\nline5 changed\nline6\n"), | ||
| Headers: http.Header{ | ||
| "Content-Disposition": []string{`attachment; filename="README.md"`}, | ||
| }, | ||
| }) | ||
|
|
||
| err := mountAndRunMarkdown(t, MarkdownDiff, []string{ | ||
| "+diff", | ||
| "--file-token", "box_md_diff", | ||
| "--from-version", "7633658129540910621", | ||
| "--to-version", "7633658129540910628", | ||
| "--context-lines", "0", | ||
| "--as", "bot", | ||
| }, f, stdout) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| var env struct { | ||
| OK bool `json:"ok"` | ||
| Data struct { | ||
| Changed bool `json:"changed"` | ||
| AddedLines int `json:"added_lines"` | ||
| DeletedLines int `json:"deleted_lines"` | ||
| Hunks []markdownDiffHunk `json:"hunks"` | ||
| Diff string `json:"diff"` | ||
| } `json:"data"` | ||
| } | ||
| if err := json.Unmarshal(stdout.Bytes(), &env); err != nil { | ||
| t.Fatalf("json unmarshal error: %v\n%s", err, stdout.String()) | ||
| } | ||
| if !env.OK || !env.Data.Changed { | ||
| t.Fatalf("expected changed=true: %s", stdout.String()) | ||
| } | ||
| if env.Data.AddedLines != 2 || env.Data.DeletedLines != 2 { | ||
| t.Fatalf("added/deleted = %d/%d, want 2/2", env.Data.AddedLines, env.Data.DeletedLines) | ||
| } | ||
| if len(env.Data.Hunks) != 2 { | ||
| t.Fatalf("len(hunks) = %d, want 2", len(env.Data.Hunks)) | ||
| } | ||
| if !strings.Contains(env.Data.Diff, "-line2") || !strings.Contains(env.Data.Diff, "+line5 changed") { | ||
| t.Fatalf("diff missing expected content: %s", env.Data.Diff) | ||
| } | ||
| } | ||
|
|
||
| func TestMarkdownDiffNoChangesPretty(t *testing.T) { | ||
| f, stdout, _, reg := cmdutil.TestFactory(t, markdownTestConfig()) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/drive/v1/files/box_md_diff/download?version=7633658129540910621", | ||
| Status: 200, | ||
| RawBody: []byte("# Title\n"), | ||
| }) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/drive/v1/files/box_md_diff/download", | ||
| Status: 200, | ||
| RawBody: []byte("# Title\n"), | ||
| }) | ||
|
|
||
| err := mountAndRunMarkdown(t, MarkdownDiff, []string{ | ||
| "+diff", | ||
| "--file-token", "box_md_diff", | ||
| "--from-version", "7633658129540910621", | ||
| "--format", "pretty", | ||
| "--as", "bot", | ||
| }, f, stdout) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got := strings.TrimSpace(stdout.String()); got != "No differences." { | ||
| t.Fatalf("pretty no-change output = %q, want %q", got, "No differences.") | ||
| } | ||
| } | ||
|
|
||
| func TestMarkdownDiffDryRunRemoteVsLocal(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, markdownTestConfig()) | ||
|
|
||
| tmpDir := t.TempDir() | ||
| withMarkdownWorkingDir(t, tmpDir) | ||
| localPath := filepath.Join(".", "local.md") | ||
| if err := os.WriteFile(localPath, []byte("# local\n"), 0o644); err != nil { | ||
| t.Fatalf("WriteFile() error: %v", err) | ||
| } | ||
|
|
||
| err := mountAndRunMarkdown(t, MarkdownDiff, []string{ | ||
| "+diff", | ||
| "--file-token", "box_md_diff", | ||
| "--file", localPath, | ||
| "--dry-run", | ||
| "--as", "bot", | ||
| }, f, stdout) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if !strings.Contains(stdout.String(), "/open-apis/drive/v1/files/:file_token/download") && !strings.Contains(stdout.String(), "/open-apis/drive/v1/files/box_md_diff/download") { | ||
| t.Fatalf("dry-run missing download call: %s", stdout.String()) | ||
| } | ||
| if !strings.Contains(stdout.String(), `"local_file": "local.md"`) && !strings.Contains(stdout.String(), `"local_file": "./local.md"`) { | ||
| t.Fatalf("dry-run missing local file metadata: %s", stdout.String()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add per-test config-dir isolation before creating the factory.
Each test should set an isolated config dir before cmdutil.TestFactory(...) to avoid config-state bleed across tests.
🔧 Suggested pattern
func TestMarkdownDiffRejectsUnsupportedFormat(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
f, stdout, _, _ := cmdutil.TestFactory(t, markdownTestConfig())Apply the same line to the other test functions in this file.
As per coding guidelines **/*_test.go: Use t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate config state in tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/markdown/markdown_diff_test.go` around lines 19 - 272, For each
test in this file (e.g., TestMarkdownDiffRejectsUnsupportedFormat,
TestMarkdownDiffRejectsToVersionWithoutFromVersion,
TestMarkdownDiffRemoteVsRemoteJSON, TestMarkdownDiffRemoteVsLocalPretty,
TestMarkdownDiffRemoteVsRemoteJSONMultipleHunks,
TestMarkdownDiffNoChangesPretty, TestMarkdownDiffDryRunRemoteVsLocal) set an
isolated config dir before calling cmdutil.TestFactory by adding
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) immediately at the start of
the test; this ensures config-state isolation across tests and should be applied
consistently in each test function that currently calls cmdutil.TestFactory(...)
in this file.
Change-Id: I475e0ba99b77f17535c0ac65622e38ddcf1ffd12
wittam-01
force-pushed
the
feat/markdown-diff
branch
from
c9cdf96 to
f69c3ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/markdown/markdown_diff.go`:
- Around line 341-344: The current logic writes op.Content and then naively
appends a '\n' when the content lacks a trailing newline, losing the standard "\
No newline at end of file" marker; change the behavior in the diff generation
(around b.WriteString(op.Content)) so that if !strings.HasSuffix(op.Content,
"\n") you do NOT append a raw '\n' but instead append the unified-diff marker
line (for example b.WriteString("\\ No newline at end of file\n") or equivalent)
after writing the content; reference op.Content and the bytes.Buffer variable b
to locate where to replace the existing b.WriteByte('\n') behavior.
🪄 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: f1b23586-26ed-4bb6-9899-45d9a8aa3331
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
go.modshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff.goshortcuts/markdown/markdown_diff_test.goshortcuts/markdown/markdown_test.goshortcuts/markdown/shortcuts.goshortcuts/register_markdown_test.goskills/lark-drive/SKILL.mdskills/lark-markdown/SKILL.mdskills/lark-markdown/references/lark-markdown-diff.mdtests/cli_e2e/markdown/markdown_dryrun_test.gotests/cli_e2e/markdown/markdown_workflow_test.go
✅ Files skipped from review due to trivial changes (4)
- shortcuts/markdown/shortcuts.go
- skills/lark-markdown/references/lark-markdown-diff.md
- skills/lark-drive/SKILL.md
- skills/lark-markdown/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/register_markdown_test.go
- shortcuts/markdown/markdown_test.go
- shortcuts/markdown/helpers.go
- tests/cli_e2e/markdown/markdown_workflow_test.go
- tests/cli_e2e/markdown/markdown_dryrun_test.go
- shortcuts/markdown/markdown_diff_test.go
Comment on lines
+341
to
+344
| b.WriteString(op.Content) | ||
| if !strings.HasSuffix(op.Content, "\n") { | ||
| b.WriteByte('\n') | ||
| } |
There was a problem hiding this comment.
Preserve missing trailing newlines in the unified diff.
These lines currently append a newline unconditionally when op.Content lacks \n, but they never emit the standard \ No newline at end of file marker. That makes a file ending with foo and a file ending with foo\n produce the same diff text, so the output is no longer a faithful unified diff.
Suggested fix
b.WriteByte(byte(prefix))
b.WriteString(op.Content)
if !strings.HasSuffix(op.Content, "\n") {
b.WriteByte('\n')
+ b.WriteString(`\ No newline at end of file`)
+ b.WriteByte('\n')
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| b.WriteString(op.Content) | |
| if !strings.HasSuffix(op.Content, "\n") { | |
| b.WriteByte('\n') | |
| } | |
| b.WriteString(op.Content) | |
| if !strings.HasSuffix(op.Content, "\n") { | |
| b.WriteByte('\n') | |
| b.WriteString(`\ No newline at end of file`) | |
| b.WriteByte('\n') | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/markdown/markdown_diff.go` around lines 341 - 344, The current
logic writes op.Content and then naively appends a '\n' when the content lacks a
trailing newline, losing the standard "\ No newline at end of file" marker;
change the behavior in the diff generation (around b.WriteString(op.Content)) so
that if !strings.HasSuffix(op.Content, "\n") you do NOT append a raw '\n' but
instead append the unified-diff marker line (for example b.WriteString("\\ No
newline at end of file\n") or equivalent) after writing the content; reference
op.Content and the bytes.Buffer variable b to locate where to replace the
existing b.WriteByte('\n') behavior.
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
Add a new
markdown +diffshortcut for comparing Drive-native Markdown versions or comparing remote Markdown against a local.mdfile. This PR also documents the response fields and adds automated plus real bot-based verification for the supported diff scenarios.Changes
markdown +diffforremote_vs_remoteandremote_vs_localcomparisons with unified diff outputlark-markdownandlark-driveskill docs, including full response field descriptions formarkdown +diffTest Plan
go test ./shortcuts ./shortcuts/markdown ./tests/cli_e2e/markdown -count=1lark-cli markdown +diffworks as expected with--as bot, covering:No differences.)Related Issues
Summary by CodeRabbit
New Features
Documentation
Tests