feat(doc): add --selection-with-ellipsis position flag to +media-insert#335
Merged
fangshuyu-768 merged 8 commits intomainfrom Apr 20, 2026
Merged
feat(doc): add --selection-with-ellipsis position flag to +media-insert#335fangshuyu-768 merged 8 commits intomainfrom
fangshuyu-768 merged 8 commits intomainfrom
Conversation
Allows inserting images/files at a position relative to the first block whose plain text matches a keyword (case-insensitive substring match). - Add --after-keyword: insert after the matched root-level block - Add --before-keyword: insert before the matched root-level block - Flags are mutually exclusive; default behavior (append to end) unchanged - fetchAllBlocks: paginated block listing (up to 50 pages × 200 blocks) - extractBlockPlainText: covers text, heading1-9, bullet, ordered, todo, code, quote - findInsertIndexByKeyword: walks parent_id chain to resolve nested blocks to their root-level ancestor - DryRun updated to show block-listing step when keyword flag is set
- TestFetchAllBlocksPaginationViaExecute: exercises fetchAllBlocks via a full Execute flow with --after-keyword, covering multi-page block listing (fetchAllBlocks was previously at 0% coverage) - TestDocMediaInsertDryRunWithAfterKeyword: verifies that the dry-run output includes a block-listing step and mentions "search blocks" in the description when --after-keyword is provided fetchAllBlocks coverage: 0% → 76.2%
|
|
|
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 selection-aware insertion to Changes
Sequence DiagramsequenceDiagram
participant CLI as Client CLI
participant Cmd as DocMediaInsert Command
participant MCP as MCP (locate-doc)
participant API as Document API
CLI->>Cmd: run with --selection-with-ellipsis [--before]
Cmd->>Cmd: validate flags (selection, --before requirement)
Cmd->>MCP: POST locate-doc(selection)
MCP-->>Cmd: return matches (blocks, anchor_block_id)
Cmd->>API: GET /blocks/{id} while walking parent chain toward root
API-->>Cmd: return parent/ancestor blocks
Cmd->>Cmd: compute insertIndex (root-level index, before/after)
Cmd->>API: POST create-block at computed insertIndex
API-->>Cmd: respond with created block
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
Greptile SummaryThis PR replaces the old Confidence Score: 5/5Safe to merge; core logic is correct with proper cycle detection, rollback, and dry-run step accounting. No P0 or P1 issues found. The single P2 finding (integration tests not asserting on the computed insert index) is a test-quality suggestion that does not affect production behavior. The locateInsertIndex traversal, visited-set cycle guard, and rollback all look correct. shortcuts/doc/doc_media_insert_test.go — the create-block stub should match on the expected index to catch future regressions.
|
| Filename | Overview |
|---|---|
| shortcuts/doc/doc_media_insert.go | New locateInsertIndex function and updated DryRun/Execute paths for --selection-with-ellipsis; logic is sound but integration tests don't verify the computed index value sent to the create-block API. |
| shortcuts/doc/doc_media_insert_test.go | Good coverage of after/before/nested/no-match/dry-run paths, but the create-block stub accepts any body so index correctness isn't verified by the integration tests. |
Sequence Diagram
sequenceDiagram
participant CLI
participant DocxAPI as Docx API
participant MCPAPI as MCP locate-doc
participant DriveAPI as Drive API
CLI->>DocxAPI: GET /documents/{id}/blocks/{id} (root block)
DocxAPI-->>CLI: root children list
alt --selection-with-ellipsis provided
CLI->>MCPAPI: POST /mcp tools/call locate-doc
MCPAPI-->>CLI: {anchor_block_id, parent_block_id}
CLI->>CLI: Walk parent chain (parent_block_id hint or GET block)
CLI->>CLI: Resolve root-level insert index (before ? idx : idx+1)
else default append
CLI->>CLI: insertIndex = len(rootChildren)
end
CLI->>DocxAPI: POST /documents/{id}/blocks/{id}/children (index=N)
DocxAPI-->>CLI: {block_id}
CLI->>DriveAPI: POST /medias/upload_all (or multipart)
DriveAPI-->>CLI: {file_token}
CLI->>DocxAPI: PATCH /documents/{id}/blocks/batch_update
DocxAPI-->>CLI: ok
note over CLI,DocxAPI: On upload/bind failure: DELETE /children/batch_delete (rollback)
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1e5de85f35e224d5f623305e7df0bd557882235e🧩 Skill updatenpx skills add larksuite/cli#feat/media-insert-keyword-position -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: 5
🧹 Nitpick comments (1)
shortcuts/doc/doc_media_insert_test.go (1)
112-124: Assert the actual root-child IDs and order.The new
childrenreturn value is used to map keyword hits back to a root-level insertion index. Checking onlylen(children)will miss regressions that reorder or replace the IDs.Suggested patch
blockID, index, children, err := extractAppendTarget(rootData, "fallback") @@ - if len(children) != 3 { - t.Fatalf("extractAppendTarget() children len = %d, want 3", len(children)) + wantChildren := []interface{}{"c1", "c2", "c3"} + if !reflect.DeepEqual(children, wantChildren) { + t.Fatalf("extractAppendTarget() children = %#v, want %#v", children, wantChildren) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert_test.go` around lines 112 - 124, The test currently only checks len(children) which misses regressions in order or identity; update the test around the call to extractAppendTarget(rootData, "fallback") to assert that children equals the expected slice of root child IDs in the correct order (e.g., compare children to the explicit expected []string{"child1","child2","child3"} using reflect.DeepEqual or your test helper) and fail the test with a clear message if they differ; keep the existing checks for blockID and index and replace the len(children) assertion with this full equality check.
🤖 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/doc_media_insert.go`:
- Around line 377-410: fetchAllBlocks currently can silently return a partial
result when the API still indicates more pages (has_more==true) or when
has_more==true but no page_token is provided; update fetchAllBlocks to detect
these cases and return an explicit error instead of a partial slice: inside the
loop in fetchAllBlocks, if has_more is true and page == maxPages-1 (i.e. we've
hit the maxPages limit) return an error indicating pagination truncated;
likewise if has_more is true but page_token is empty return an error indicating
inconsistent pagination (has_more without page_token); reference the
variables/functions fetchAllBlocks, maxPages, pageToken and has_more when
implementing the checks and return a descriptive error rather than breaking and
returning partial all.
- Around line 64-80: The dry-run output still shows index: "<children_len>" even
when keyword insertion is used; update the dry-run create block payload to
reflect the computed insertion index when hasKeyword is true. In the block
around afterKeyword/beforeKeyword/hasKeyword and createBlockData (built via
buildCreateBlockData) replace or override createBlockData["index"] with a
descriptive placeholder that represents the computed position (e.g.,
"<insertion_index_from_search>") when hasKeyword is true, and ensure any related
batchUpdateData or totalSteps logic (the DryRunAPI usage and totalSteps
increment) remains consistent so the dry-run plan accurately describes
keyword-mode insertion.
In `@shortcuts/doc/doc_media_test.go`:
- Around line 445-455: The test's dry-run check falsely matches document root
block fetches because it only checks for substring "/blocks" in step.URL; update
the loop in the test (the foundListBlocks logic that iterates dry.API and
inspects step.URL) to assert the exact list-all-blocks endpoint (e.g., equality
to "/blocks" or the full expected path) or alternatively check step.Description
(or another explicit field) for the "List all blocks" label to reliably detect
the extra list-all-blocks step instead of any URL containing "/blocks".
- Around line 347-405: Change the first
/open-apis/docx/v1/documents/doxcnFB/blocks stub to return "has_more": true and
only non-keyword items, then add a second GET stub for the same URL that
emulates page 2 (e.g., using a page token) containing the keyword block so
pagination must be followed; update the create-child POST stub registered for
"/open-apis/docx/v1/documents/doxcnFB/blocks/doxcnFB/children" to validate the
incoming request body (assert the "index" field or placement in the JSON matches
the computed insert index) instead of accepting any body; apply the same changes
to the analogous stubs referenced around lines 411-420 so the test actually
verifies page-token handling and that Execute inserts at the expected index.
- Around line 330-331: Before calling cmdutil.TestFactory in
TestFetchAllBlocksPaginationViaExecute, isolate the CLI config by setting the
LARKSUITE_CLI_CONFIG_DIR env var to a test temp directory; add
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of
TestFetchAllBlocksPaginationViaExecute (before invoking cmdutil.TestFactory) so
the test uses an isolated config directory and won’t share state with other
tests.
---
Nitpick comments:
In `@shortcuts/doc/doc_media_insert_test.go`:
- Around line 112-124: The test currently only checks len(children) which misses
regressions in order or identity; update the test around the call to
extractAppendTarget(rootData, "fallback") to assert that children equals the
expected slice of root child IDs in the correct order (e.g., compare children to
the explicit expected []string{"child1","child2","child3"} using
reflect.DeepEqual or your test helper) and fail the test with a clear message if
they differ; keep the existing checks for blockID and index and replace the
len(children) assertion with this full equality check.
🪄 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: f0c5801d-351b-4e8b-8a79-34e0b5cc29ec
📒 Files selected for processing (3)
shortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_insert_test.goshortcuts/doc/doc_media_test.go
Replace fetchAllBlocks + keyword scan with MCP locate-doc tool, consistent with DriveAddComment. Flags changed from --after-keyword / --before-keyword to --selection-with-ellipsis + --before.
herbertliu
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/doc/doc_media_insert.go (1)
64-69:⚠️ Potential issue | 🟡 MinorSelection-mode dry-run still advertises append-to-end.
Once
--selection-with-ellipsisis set, the plan resolves a target position, but the create-block payload still showsindex: "<children_len>". That keeps the dry-run misleading for the new flow. Switch it to a target-position placeholder in selection mode.Suggested patch
createBlockData := buildCreateBlockData(mediaType, 0) createBlockData["index"] = "<children_len>" + if hasSelection { + createBlockData["index"] = "<target_index>" + }Also applies to: 77-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert.go` around lines 64 - 69, The dry-run always sets createBlockData["index"] to "<children_len>" even when selection mode is active; update the logic that builds the create-block payload so when selection := strings.TrimSpace(runtime.Str("selection-with-ellipsis")) yields hasSelection (selection != ""), replace the index placeholder with a target-position placeholder (e.g. "<target-position>") instead of "<children_len>". Adjust both the initial creation path around parentTypeForMediaType(mediaType) / buildCreateBlockData(mediaType, 0) and the duplicate handling later in the same function (the code covering the 77-123 region) so any createBlockData["index"] assignment uses the selection-mode placeholder when hasSelection is true.
🧹 Nitpick comments (2)
shortcuts/doc/doc_media_insert.go (1)
435-468: Avoid hard-coding an 8-level ancestry limit here.
visitedalready prevents parent-chain loops.const maxDepth = 8adds an undocumented client-side nesting cap, so a reachable match deeper than that will be reported asblock_not_reachable. Prefer terminating onparent == ""/parent == curinstead of a fixed depth ceiling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert.go` around lines 435 - 468, Remove the artificial const maxDepth = 8 and the depth-limited for loop; instead iterate until you hit the root or a visited node so only the visited map prevents infinite loops. Replace the for depth := 0; depth < maxDepth; depth++ { ... } with an unconditional loop (for { ... }) that keeps the existing visited[cur] check, the rootSet lookup (returning idx or idx+1), the parent hint handling via nextParent, the runtime.CallAPI fetch using documentID and cur to get parent_id, and the break conditions parent == "" or parent == cur; ensure no other implicit depth cap remains.shortcuts/doc/doc_media_insert_test.go (1)
153-205: These tests still don't prove the computed insertion position.The new behavior lives in the MCP request args and the
indexsent to the create-block call, but the stubs accept any request body and the assertions only check success / presence of alocate-docstep. A regression that always appends or flips--beforewould still pass. Please assert the outgoingselection_with_ellipsisand create-blockindexfor each scenario.Also applies to: 208-321, 367-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert_test.go` around lines 153 - 205, The tests currently register permissive stubs in registerInsertWithSelectionStubs (and the similar stubs around the other ranges) which accept any request bodies, so they don't verify the computed insertion position; update those stubs to assert the outgoing MCP POST contains the expected selection_with_ellipsis value and that the create-block POST to "/open-apis/docx/v1/documents/{docID}/blocks/{docID}/children" includes the correct "index" in its request body. Concretely, modify registerInsertWithSelectionStubs (and the corresponding stub registrations in the 208-321 and 367-410 blocks) to set an expected request body or a request matcher on the MCP stub that checks selection_with_ellipsis equals the scenario value, and on the create-block stub to validate the "index" field (failing the test if mismatched); keep existing response bodies but add these request assertions so regressions flipping/always-appending the index will fail.
🤖 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/doc_media_insert.go`:
- Around line 64-69: The dry-run always sets createBlockData["index"] to
"<children_len>" even when selection mode is active; update the logic that
builds the create-block payload so when selection :=
strings.TrimSpace(runtime.Str("selection-with-ellipsis")) yields hasSelection
(selection != ""), replace the index placeholder with a target-position
placeholder (e.g. "<target-position>") instead of "<children_len>". Adjust both
the initial creation path around parentTypeForMediaType(mediaType) /
buildCreateBlockData(mediaType, 0) and the duplicate handling later in the same
function (the code covering the 77-123 region) so any createBlockData["index"]
assignment uses the selection-mode placeholder when hasSelection is true.
---
Nitpick comments:
In `@shortcuts/doc/doc_media_insert_test.go`:
- Around line 153-205: The tests currently register permissive stubs in
registerInsertWithSelectionStubs (and the similar stubs around the other ranges)
which accept any request bodies, so they don't verify the computed insertion
position; update those stubs to assert the outgoing MCP POST contains the
expected selection_with_ellipsis value and that the create-block POST to
"/open-apis/docx/v1/documents/{docID}/blocks/{docID}/children" includes the
correct "index" in its request body. Concretely, modify
registerInsertWithSelectionStubs (and the corresponding stub registrations in
the 208-321 and 367-410 blocks) to set an expected request body or a request
matcher on the MCP stub that checks selection_with_ellipsis equals the scenario
value, and on the create-block stub to validate the "index" field (failing the
test if mismatched); keep existing response bodies but add these request
assertions so regressions flipping/always-appending the index will fail.
In `@shortcuts/doc/doc_media_insert.go`:
- Around line 435-468: Remove the artificial const maxDepth = 8 and the
depth-limited for loop; instead iterate until you hit the root or a visited node
so only the visited map prevents infinite loops. Replace the for depth := 0;
depth < maxDepth; depth++ { ... } with an unconditional loop (for { ... }) that
keeps the existing visited[cur] check, the rootSet lookup (returning idx or
idx+1), the parent hint handling via nextParent, the runtime.CallAPI fetch using
documentID and cur to get parent_id, and the break conditions parent == "" or
parent == cur; ensure no other implicit depth cap remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 433c3a8d-8d5f-45a9-9df9-90756c31b610
📒 Files selected for processing (2)
shortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_insert_test.go
…is set When --selection-with-ellipsis is provided, the create-block step in dry-run now shows index: "<locate_index>" instead of "<children_len>" to accurately reflect that the insertion position is computed from MCP locate-doc, not appended to end.
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/doc_media_insert.go`:
- Line 185: The stderr logging prints the raw variable selection (selection) via
fmt.Fprintf(runtime.IO().ErrOut, ...) which can leak document content; update
all occurrences (the fmt.Fprintf calls around "Locating block matching
selection" and the error paths around lines ~402-407 and ~474-479) to sanitize
the value before logging—e.g., replace full selection with a redacted/trimmed
representation (mask middle with "…", show only safe prefix/suffix or a length
and hash), and use that sanitized string in the fmt.Fprintf/error messages
instead of the raw selection variable.
- Around line 48-50: Add validation in the Validate function to reject blank or
whitespace-only values for the --selection-with-ellipsis flag so it cannot
silently fall back to append-mode; specifically, call
runtime.Str("selection-with-ellipsis") (RuntimeContext.Str /
RuntimeContext.Validate) and trim it (strings.TrimSpace) and return an error if
the result is empty, regardless of runtime.Bool("before") — this mirrors the
existing before-mode check around the if using runtime.Bool("before") but
enforces it unconditionally in Validate to catch cases where the flag was
provided but blank.
🪄 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: db0c7312-a58c-47ae-ba53-7100d2510fae
📒 Files selected for processing (2)
shortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_insert_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/doc/doc_media_insert_test.go
7 tasks
herbertliu
force-pushed
the
feat/media-insert-keyword-position
branch
from
4ccb830 to
319f9fc
Compare
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/doc_media_insert_test.go`:
- Around line 255-305: The test TestLocateInsertIndexNestedBlockViaExecute never
exercises the fallback ancestor-walk because the MCP response returns
parent_block_id "blk_section" which is already a direct child of the root; to
cover the deeper-ancestor branch, change the MCP response to point the anchor
under an intermediate block (e.g., parent_block_id "blk_section_child" whose
parent is "blk_section") and add a reg.Register stub for the GET
/open-apis/docx/v1/documents/{docID}/blocks/{parent_block_id} call for that
intermediate block (returning its "parent" as "blk_section"), so the code path
in the parent-walk is triggered; update the test bodies that assert
uploads/patches if needed to reflect the new nesting.
- Around line 210-250: Tests TestLocateInsertIndexAfterModeViaExecute and
TestLocateInsertIndexBeforeModeViaExecute currently only check Execute() success
but use fixtures that resolve to the same target index; update the tests so the
two modes resolve to different indices (e.g., alter the registered root children
in registerInsertWithSelectionStubs or use a fixture with an extra block like
"blk_c" so "after blk_a" != "before blk_b"), then add an assertion on the mocked
/children request body (the request sent by mountAndRunDocs/DocMediaInsert) to
verify the "index" field matches the expected value for each test; reference the
test helpers TestLocateInsertIndexAfterModeViaExecute,
TestLocateInsertIndexBeforeModeViaExecute, registerInsertWithSelectionStubs,
mountAndRunDocs and ensure the assertion inspects the request payload produced
by the code under test.
🪄 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: 40b9ace0-5dce-498d-bd46-f8f052c013c4
📒 Files selected for processing (2)
shortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_insert_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/doc_media_insert.go
…word-position # Conflicts: # shortcuts/doc/doc_media_insert.go # shortcuts/doc/doc_media_insert_test.go
- Validate: reject blank/whitespace --selection-with-ellipsis unconditionally
so a mis-typed empty value cannot silently fall back to append-mode.
- Redact the raw selection string when logging to stderr and when emitting
error messages. --selection-with-ellipsis is copied verbatim from document
content and may contain confidential text; the new redactSelection helper
keeps a short prefix and rune count so operators can still identify the
failing selection.
- Harden the after/before mode tests: root children now have three entries
so the two modes land on different indices, and the tests decode the
create-block request body to assert the computed `index` actually reaches
the /children API. A regression that ignored --before would now fail.
- Harden the nested-block test so it exercises the fallback parent-walk:
the anchor is now two levels deep (blk_grandchild under blk_section_child
under blk_section), which forces the walk to fetch the intermediate block
via GET /blocks/{id} to discover the root-level ancestor.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/doc/doc_media_insert_test.go (2)
488-492: Nit:CapturedBody == nilis always true for this GET.The intermediate block lookup is a
GET, which sends no body, sointermediateStub.CapturedBodyis always nil. The combined&&condition means this assertion effectively only testsCapturedHeaders == nil. Consider simplifying tointermediateStub.CapturedHeaders == nil(or whatever "was hit" signal the stub exposes) to make the intent clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert_test.go` around lines 488 - 492, The test currently checks both intermediateStub.CapturedBody == nil and intermediateStub.CapturedHeaders == nil, but CapturedBody is always nil for a GET; change the assertion to only check the stub's actual "was hit" signal (e.g. intermediateStub.CapturedHeaders == nil) or the stub's explicit hit flag if available (e.g. intermediateStub.WasHit) so the assertion clearly tests that the GET /blocks/blk_section_child stub was invoked; update the failing t.Errorf message accordingly to reflect that the header/hit check failed.
587-596: Dry-run index assertion passes silently if the key is missing.The
indexcheck only fires whenstep.Body["index"]is present. A regression that drops the placeholder altogether (or renames the field) would leaveok == falseand the test would pass. Consider failing when the create-block step is found but has noindexkey, so the contract "<locate_index>is the placeholder in selection mode" is actually enforced.Proposed tightening
- for _, step := range dry.API { - if strings.Contains(step.URL, "/children") && step.Body != nil { - if idx, ok := step.Body["index"]; ok { - if idx != "<locate_index>" { - t.Fatalf("create-block index in selection mode = %q, want <locate_index>", idx) - } - } - } - } + foundCreate := false + for _, step := range dry.API { + if strings.Contains(step.URL, "/children") && step.Body != nil { + foundCreate = true + idx, ok := step.Body["index"] + if !ok { + t.Fatalf("create-block step missing 'index' field in selection mode: %+v", step.Body) + } + if idx != "<locate_index>" { + t.Fatalf("create-block index in selection mode = %q, want <locate_index>", idx) + } + } + } + if !foundCreate { + t.Fatalf("dry-run should include a create-block step, got: %+v", dry.API) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert_test.go` around lines 587 - 596, The test currently only asserts the value of step.Body["index"] when the key exists, which allows a missing key to pass; update the loop over dry.API in shortcuts/doc/doc_media_insert_test.go so that for each step where strings.Contains(step.URL, "/children") && step.Body != nil you assert that the "index" key exists (ok == true) and then assert its value equals "<locate_index>" — if the key is missing or the value differs call t.Fatalf with a clear message referencing the step and the expected "<locate_index>" placeholder; locate the check around the for _, step := range dry.API loop and replace the nested ifs to first require presence of "index" and then validate its value.
🤖 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/doc_media_insert_test.go`:
- Around line 488-492: The test currently checks both
intermediateStub.CapturedBody == nil and intermediateStub.CapturedHeaders ==
nil, but CapturedBody is always nil for a GET; change the assertion to only
check the stub's actual "was hit" signal (e.g. intermediateStub.CapturedHeaders
== nil) or the stub's explicit hit flag if available (e.g.
intermediateStub.WasHit) so the assertion clearly tests that the GET
/blocks/blk_section_child stub was invoked; update the failing t.Errorf message
accordingly to reflect that the header/hit check failed.
- Around line 587-596: The test currently only asserts the value of
step.Body["index"] when the key exists, which allows a missing key to pass;
update the loop over dry.API in shortcuts/doc/doc_media_insert_test.go so that
for each step where strings.Contains(step.URL, "/children") && step.Body != nil
you assert that the "index" key exists (ok == true) and then assert its value
equals "<locate_index>" — if the key is missing or the value differs call
t.Fatalf with a clear message referencing the step and the expected
"<locate_index>" placeholder; locate the check around the for _, step := range
dry.API loop and replace the nested ifs to first require presence of "index" and
then validate its value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4842a267-7b53-4ef5-888e-61acd257c832
📒 Files selected for processing (2)
shortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_insert_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/doc/doc_media_insert.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
==========================================
+ Coverage 60.19% 60.64% +0.44%
==========================================
Files 390 393 +3
Lines 33433 33789 +356
==========================================
+ Hits 20125 20490 +365
+ Misses 11426 11391 -35
- Partials 1882 1908 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fangshuyu-768
added a commit
that referenced
this pull request
Apr 20, 2026
Follow-up to #335 review: closes a handful of UX and robustness gaps in the new --selection-with-ellipsis flow. - Flag description rewritten to make the "insert at the top-level ancestor" semantics explicit — when the selection is inside a callout, table cell, or nested list, media lands outside that container, not inside. Also calls out the 'start...end' disambiguator. - locate-doc is now called with limit=2 so an ambiguous selection (same phrase in more than one block) surfaces a stderr warning pointing at 'start...end', instead of silently picking the first match. The first-match return behaviour is unchanged. - When the anchor is nested below the root, locateInsertIndex now logs a note to stderr naming the walk depth and the root-level ancestor's insert index. Users don't have to guess why the image landed outside the callout they were editing. - maxDepth bumped 8 → 32 with a comment explaining the invariants: `visited` is the real cycle guard, `maxDepth` is belt-and-suspenders. 32 comfortably exceeds real docx nesting depth so a deeply-nested but well-formed anchor is no longer silently rejected. - Comment added before the parent-walk loop noting why the API calls are serial (each level's parent_id is only known after the previous GET returns; can't be batched or parallelised). Tests: - TestLocateInsertIndexWarnsOnMultipleMatches: stubs two matches, asserts the stderr warning names the ambiguity and mentions 'start...end', and that the first-match insert index is unchanged. - TestLocateInsertIndexLogsNestedAnchor: anchor two levels below root, asserts stderr carries the "nested … top-level ancestor" note. - TestLocateInsertIndexCycleDetection: malformed parent chain with blk_x.parent = blk_y and blk_y.parent = blk_x, neither reachable from root. Registering a single GET /blocks/blk_y stub also bounds the call count — a regression that broke `visited` tracking would either hang or fail via httpmock's extra-call guard.
4 tasks
Follow-up to #335 review: closes a handful of UX and robustness gaps in the new --selection-with-ellipsis flow. - Flag description rewritten to make the "insert at the top-level ancestor" semantics explicit — when the selection is inside a callout, table cell, or nested list, media lands outside that container, not inside. Also calls out the 'start...end' disambiguator. - locate-doc is now called with limit=2 so an ambiguous selection (same phrase in more than one block) surfaces a stderr warning pointing at 'start...end', instead of silently picking the first match. The first-match return behaviour is unchanged. - When the anchor is nested below the root, locateInsertIndex now logs a note to stderr naming the walk depth and the root-level ancestor's insert index. Users don't have to guess why the image landed outside the callout they were editing. - maxDepth bumped 8 → 32 with a comment explaining the invariants: `visited` is the real cycle guard, `maxDepth` is belt-and-suspenders. 32 comfortably exceeds real docx nesting depth so a deeply-nested but well-formed anchor is no longer silently rejected. - Comment added before the parent-walk loop noting why the API calls are serial (each level's parent_id is only known after the previous GET returns; can't be batched or parallelised). Tests: - TestLocateInsertIndexWarnsOnMultipleMatches: stubs two matches, asserts the stderr warning names the ambiguity and mentions 'start...end', and that the first-match insert index is unchanged. - TestLocateInsertIndexLogsNestedAnchor: anchor two levels below root, asserts stderr carries the "nested … top-level ancestor" note. - TestLocateInsertIndexCycleDetection: malformed parent chain with blk_x.parent = blk_y and blk_y.parent = blk_x, neither reachable from root. Registering a single GET /blocks/blk_y stub also bounds the call count — a regression that broke `visited` tracking would either hang or fail via httpmock's extra-call guard. Co-authored-by: fangshuyu-768 <shuyufang768@outlook.com>
fangshuyu-768
approved these changes
Apr 20, 2026
4 tasks
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
3 participants
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
docs +media-insertcurrently only supports appending media to the end of a document. This PR adds two new flags —--selection-with-ellipsisand--before— that allow inserting an image or file at a position relative to a block identified by a text selection, using the MCPlocate-doctool (the same approach used bydrive +add-comment).This is more efficient than the earlier approach of fetching all document blocks:
locate-docreturns the matching block directly without paginated enumeration.Changes
shortcuts/doc/doc_media_insert.go:--after-keyword/--before-keywordwith--selection-with-ellipsis(plain text orstart...endsyntax) +--beforebool flagfetchAllBlocks,extractBlockPlainText,findInsertIndexByKeywordlocateInsertIndex(): calls MCPlocate-doc, usesparent_block_idhint to walk to the root-level ancestor (zero extra API calls in the common case; falls back to single-block GET for deeper nesting, max 8 levels)DryRunto show the MCPlocate-docstep when--selection-with-ellipsisis setMCPEndpointcall insideif hasSelectionto avoid nil-config panic in testsshortcuts/doc/doc_media_insert_test.go:extractBlockPlainText,findInsertIndexByKeyword)Execute+ httpmock: after-mode, before-mode, nested block (parent_block_id walking), no-match errorlocate-docstep appears when--selection-with-ellipsisis setshortcuts/doc/doc_media_test.go:TestFetchAllBlocksPaginationViaExecuteandTestDocMediaInsertDryRunWithAfterKeyword(deleted functions / renamed flags)Test Plan
go test ./shortcuts/doc/...— all tests passgo vet ./...— no issuesgofmt -l .— no unformatted fileslocateInsertIndexcoverage: 68.9% (above 60% threshold)Related Issues
Summary by CodeRabbit
New Features
Tests