fix(doc): harden +media-insert selection UX on top of #335#577
Merged
fangshuyu-768 merged 1 commit intofeat/media-insert-keyword-positionfrom Apr 20, 2026
Merged
Conversation
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.
github-actions
Bot
added
domain/ccm
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
fangshuyu-768 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
fangshuyu-768
merged commit 1e5de85
into
feat/media-insert-keyword-position
2 of 3 checks passed
fangshuyu-768
added a commit
that referenced
this pull request
Apr 20, 2026
…rt (#335) * feat(doc): add --after-keyword/--before-keyword flags to +media-insert 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 * test(doc): add fetchAllBlocks pagination and keyword dry-run coverage - 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% * refactor(doc): use MCP locate-doc for keyword-based block positioning Replace fetchAllBlocks + keyword scan with MCP locate-doc tool, consistent with DriveAddComment. Flags changed from --after-keyword / --before-keyword to --selection-with-ellipsis + --before. * fix(doc): show <locate_index> in dry-run create-block when selection 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. * fix(doc): address CodeRabbit review on +media-insert selection feature - 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. * fix(doc): harden +media-insert selection UX on top of #335 (#577) 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>
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
2 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
Follow-up to #335, targeting
feat/media-insert-keyword-positionso the changes land together with that feature. Addresses a handful of UX and robustness gaps I spotted while reviewing the final state of that PR. No functional changes to the happy path.Changes
shortcuts/doc/doc_media_insert.go--selection-with-ellipsisflag description rewritten. Now explicit about the "insert at the top-level ancestor" semantics — when the selection is inside a callout, table cell, or nested list, media lands outside that container, not inside it. Also calls out the'start...end'disambiguator.locateInsertIndexnow calls locate-doc withlimit=2. When more than one block matches, a warning is written to stderr pointing at thestart...endnarrowing syntax. The first-match return behaviour is unchanged — users whose selection is unique see no difference.maxDepthbumped 8 → 32 with a comment explaining the invariants.visitedis the real cycle guard;maxDepthis belt-and-suspenders. 32 comfortably exceeds real docx nesting depth so a deeply-nested but well-formed anchor is no longer silently rejected.parent_idis only known after the previousGET /blocks/{id}returns — can't be batched or parallelised).shortcuts/doc/doc_media_insert_test.goThree new tests:
TestLocateInsertIndexWarnsOnMultipleMatchesstart...end; asserts first-match insert index is unchangedTestLocateInsertIndexLogsNestedAnchorTestLocateInsertIndexCycleDetectionblk_x ↔ blk_y, neither reachable from root); assertsblock_not_reachableerror and thatGET /blocks/blk_yis hit exactly once. A regression that brokevisitedtracking would either hang or fail via httpmock's extra-call guardTest plan
go test ./shortcuts/doc/...— all tests pass (existing + 3 new)gofmtcleango vet ./shortcuts/doc/...clean6e667ee)Why not in #335 directly
maintainer_can_modifyis disabled on #335 (internal branch), so this is submitted as a stacked PR. Targeting #335's branch means the changes merge in the same commit as the feature.