feat: improve doc media extension inference#364
Merged
fangshuyu-768 merged 1 commit intomainfrom Apr 9, 2026
Merged
Conversation
github-actions
Bot
added
domain/ccm
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes doc/media file-extension inference by adding an internal helper Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI Command"
participant Handler as "doc_media handler"
participant HTTP as "HTTP Response"
participant ExtUtil as "autoAppendDocMediaExtension"
participant FS as "Filesystem"
CLI->>Handler: invoke +media-download / +media-preview
Handler->>HTTP: perform request -> receive headers & body
Handler->>ExtUtil: autoAppendDocMediaExtension(outputPath, headers, fallbackExt)
ExtUtil-->>Handler: finalPath (+ resolution metadata)
Handler->>FS: validate & write file to finalPath
Handler-->>CLI: emit JSON result with data.saved_path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR centralises file-extension inference for Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/consistency suggestions that do not affect correctness. The core extension-inference logic is correct, well-tested (MIME mapping, Content-Disposition, RFC 5987, trailing-dot normalization, overwrite protection), and path safety is validated before every write. Both open findings are P2: the missing stderr log is a promised-but-absent convenience feature, and the shortcuts/doc/doc_media_ext.go — the
|
| Filename | Overview |
|---|---|
| shortcuts/doc/doc_media_ext.go | New shared helper for extension inference; centralizes MIME mapping, Content-Disposition fallback, and trailing-dot normalization — but the docMediaExtensionResolution return value (designed for logging) is never consumed by callers. |
| shortcuts/doc/doc_media_download.go | Refactored to call autoAppendDocMediaExtension; logic is correct, but the resolution return value is silently discarded with _, meaning no logging of which header was used. |
| shortcuts/doc/doc_media_preview.go | Symmetric refactor to use shared helper; also discards resolution; missing the savedPath == "" fallback that the download counterpart has (lines 108–111 of doc_media_download.go). |
| shortcuts/doc/doc_media_test.go | Good coverage: MIME mapping, Content-Disposition filename fallback, RFC 5987 filename*, trailing-dot normalization, and overwrite protection — all exercised with real HTTP stubs. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User provides --output path] --> B{Has explicit extension?}
B -- yes --> C[Return path as-is]
B -- no --> D{Trailing dot?}
D -- yes --> E[normalizedPath = strip trailing dot]
D -- no --> F[normalizedPath = path]
E & F --> G{Content-Type in MIME map?}
G -- yes --> H[Return normalizedPath + ext\nSource: Content-Type]
G -- no --> I{Content-Disposition filename\nhas extension?}
I -- yes --> J[Return normalizedPath + ext\nSource: Content-Disposition]
I -- no --> K{fallbackExt provided?}
K -- yes --> L[Return normalizedPath + fallbackExt\nSource: fallback]
K -- no --> M[Return original outputPath unchanged]
Reviews (5): Last reviewed commit: "feat: improve doc media extension infere..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@cdbdc31031fc2837e21faaa498c8fe32b359d8e8🧩 Skill updatenpx skills add larksuite/cli#feat/media-extension -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/doc/doc_media_test.go (1)
550-558: Prefer independent expected-path construction in tests.
mustDocSafeOutputPathreusesvalidate.SafeOutputPath(same validator used by production code), which can mask regressions by coupling expected values to implementation details. Consider deriving expected paths with cwd-aware path construction instead.♻️ Example refactor
-func mustDocSafeOutputPath(t *testing.T, output string) string { +func mustDocExpectedOutputPath(t *testing.T, output string) string { t.Helper() - - path, err := validate.SafeOutputPath(output) + cwd, err := os.Getwd() if err != nil { - t.Fatalf("SafeOutputPath(%q) error: %v", output, err) + t.Fatalf("Getwd() error: %v", err) } - return path + if filepath.IsAbs(output) { + return filepath.Clean(output) + } + return filepath.Clean(filepath.Join(cwd, output)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_test.go` around lines 550 - 558, The test helper mustDocSafeOutputPath currently calls validate.SafeOutputPath which couples the test expectation to production logic; instead change the test to construct the expected safe output path independently (use os.Getwd()/os.Getwd error handling and filepath.Join/filepath.Clean/filepath.Abs as needed) and compare that constructed expected string against validate.SafeOutputPath output in assertions. Update or replace mustDocSafeOutputPath to compute the expected path using cwd-aware path construction (e.g., get working dir, join with the provided output, clean/abs it) and only use validate.SafeOutputPath for the value under test so failures reveal regressions in validate.SafeOutputPath rather than mirroring them. Ensure references to mustDocSafeOutputPath and validate.SafeOutputPath are adjusted in tests accordingly.
🤖 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_test.go`:
- Around line 550-558: The test helper mustDocSafeOutputPath currently calls
validate.SafeOutputPath which couples the test expectation to production logic;
instead change the test to construct the expected safe output path independently
(use os.Getwd()/os.Getwd error handling and
filepath.Join/filepath.Clean/filepath.Abs as needed) and compare that
constructed expected string against validate.SafeOutputPath output in
assertions. Update or replace mustDocSafeOutputPath to compute the expected path
using cwd-aware path construction (e.g., get working dir, join with the provided
output, clean/abs it) and only use validate.SafeOutputPath for the value under
test so failures reveal regressions in validate.SafeOutputPath rather than
mirroring them. Ensure references to mustDocSafeOutputPath and
validate.SafeOutputPath are adjusted in tests accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d3d2024-6286-4aee-99f4-7c555051a467
📒 Files selected for processing (4)
shortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_ext.goshortcuts/doc/doc_media_preview.goshortcuts/doc/doc_media_test.go
wittam-01
force-pushed
the
feat/media-extension
branch
2 times, most recently
from
75b1e21 to
c7178fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
shortcuts/doc/doc_media_test.go (2)
531-539: Fail fast when command JSON envelope reportsok=false
decodeDocCommandOutputcurrently validates only JSON shape. A quickout.OKassertion would make failures clearer in these tests.♻️ Suggested change
func decodeDocCommandOutput(t *testing.T, stdout *bytes.Buffer) docCommandOutput { t.Helper() var out docCommandOutput if err := json.Unmarshal(stdout.Bytes(), &out); err != nil { t.Fatalf("decode command output: %v; output=%s", err, stdout.String()) } + if !out.OK { + t.Fatalf("command output reported failure: %s", stdout.String()) + } return out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_test.go` around lines 531 - 539, The helper decodeDocCommandOutput only checks JSON unmarshalling but not the command-level success flag; modify decodeDocCommandOutput to assert that the decoded docCommandOutput's OK field is true (e.g., check out.OK) and call t.Fatalf with a clear message including the full output when OK is false; keep the existing JSON error handling and return the decoded out when OK passes.
289-323: Add assertions for stderr inference-source diagnosticsThese tests verify suffix inference and file creation well, but they don’t assert the new stderr diagnostics (
Content-TypevsContent-Disposition) described by this feature. Adding those checks would lock in the behavior end-to-end.Also applies to: 411-445, 447-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_test.go` around lines 289 - 323, Update the test TestDocMediaDownloadAppendsExtensionFromContentDispositionFilename to assert the new stderr diagnostics that compare Content-Type vs Content-Disposition inference: after running mountAndRunDocs (using DocMediaDownload) and decoding the output with decodeDocCommandOutput, read the captured stderr (stdout variable from cmdutil.TestFactory provides separate streams) and assert it contains the expected diagnostic message (e.g., mentions that Content-Disposition filename was preferred over Content-Type for extension inference, or the specific phrase your implementation emits). Do the same for the other referenced tests (lines 411-445 and 447-480) by adding analogous stderr assertions so the tests verify both saved_path/file existence and the stderr inference-source diagnostics.shortcuts/doc/doc_media_ext.go (1)
54-59: Normalize fallback extension format before append
fallbackExtis appended verbatim. Normalizing to a leading-dot form makes this helper safer for future callers.♻️ Suggested change
if fallbackExt != "" { + if !strings.HasPrefix(fallbackExt, ".") { + fallbackExt = "." + fallbackExt + } return outputPath + fallbackExt, &docMediaExtensionResolution{ Ext: fallbackExt, Source: "fallback", Detail: "default fallback", } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_ext.go` around lines 54 - 59, Normalize the fallback extension before appending: when fallbackExt is non-empty, ensure it has a leading dot (e.g., if !strings.HasPrefix(fallbackExt, ".") then fallbackExt = "." + fallbackExt) and use that normalized value when constructing the returned path and the docMediaExtensionResolution.Ext and Detail fields so callers always receive a leading-dot extension; use the existing fallbackExt variable and the docMediaExtensionResolution struct to locate where to apply this change.
🤖 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_ext.go`:
- Around line 45-47: The check using filepath.Ext(outputPath) treats a lone "."
as an extension (so "download." incorrectly skips inference); update the
condition around outputPath to treat both empty string and "." as no-extension
(e.g., require ext != "" && ext != ".") so extension inference runs for
trailing-dot paths; apply the same pattern used in
docMediaExtensionByContentDisposition and adjust the branch that returns
outputPath to only do so when ext is a real extension.
---
Nitpick comments:
In `@shortcuts/doc/doc_media_ext.go`:
- Around line 54-59: Normalize the fallback extension before appending: when
fallbackExt is non-empty, ensure it has a leading dot (e.g., if
!strings.HasPrefix(fallbackExt, ".") then fallbackExt = "." + fallbackExt) and
use that normalized value when constructing the returned path and the
docMediaExtensionResolution.Ext and Detail fields so callers always receive a
leading-dot extension; use the existing fallbackExt variable and the
docMediaExtensionResolution struct to locate where to apply this change.
In `@shortcuts/doc/doc_media_test.go`:
- Around line 531-539: The helper decodeDocCommandOutput only checks JSON
unmarshalling but not the command-level success flag; modify
decodeDocCommandOutput to assert that the decoded docCommandOutput's OK field is
true (e.g., check out.OK) and call t.Fatalf with a clear message including the
full output when OK is false; keep the existing JSON error handling and return
the decoded out when OK passes.
- Around line 289-323: Update the test
TestDocMediaDownloadAppendsExtensionFromContentDispositionFilename to assert the
new stderr diagnostics that compare Content-Type vs Content-Disposition
inference: after running mountAndRunDocs (using DocMediaDownload) and decoding
the output with decodeDocCommandOutput, read the captured stderr (stdout
variable from cmdutil.TestFactory provides separate streams) and assert it
contains the expected diagnostic message (e.g., mentions that
Content-Disposition filename was preferred over Content-Type for extension
inference, or the specific phrase your implementation emits). Do the same for
the other referenced tests (lines 411-445 and 447-480) by adding analogous
stderr assertions so the tests verify both saved_path/file existence and the
stderr inference-source diagnostics.
🪄 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: 9238e74f-02eb-4562-92ba-fc10a257e324
📒 Files selected for processing (4)
shortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_ext.goshortcuts/doc/doc_media_preview.goshortcuts/doc/doc_media_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/doc_media_preview.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/doc/doc_media_download.go
wittam-01
force-pushed
the
feat/media-extension
branch
from
c7178fa to
4888ee1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/doc/doc_media_test.go (1)
289-323: Add stderr assertions for extension-source logs in at least one download and one preview test.These tests validate path inference correctly, but they don’t lock the new “source-of-inference” diagnostics behavior. Capturing
stderr(already available fromcmdutil.TestFactory) and asserting source text would prevent regressions in the new user-visible hinting.Also applies to: 446-480, 518-551
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_test.go` around lines 289 - 323, The tests currently verify saved paths but don't assert the new stderr "source-of-inference" diagnostics; update TestDocMediaDownloadAppendsExtensionFromContentDispositionFilename to capture and assert stderr (via cmdutil.TestFactory returns) includes the inference source text (e.g. contains "Content-Disposition" or the exact diagnostic string emitted by the code) after calling mountAndRunDocs/DocMediaDownload, and add a matching stderr assertion in at least one preview test (use the same pattern: get stderr from cmdutil.TestFactory, run mountAndRunDocs with the preview command, then assert stderr contains the expected source-of-inference substring). Ensure you reference the existing helpers (cmdutil.TestFactory, mountAndRunDocs, decodeDocCommandOutput) and only add the stderr presence checks so the path assertions remain unchanged.
🤖 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_download.go`:
- Around line 86-87: The call to autoAppendDocMediaExtension currently discards
its second return value, losing the inferred extension source and resolution;
change the call in doc_media_download.go to capture both returns (e.g.,
finalPath, resolutionOrSrc := autoAppendDocMediaExtension(outputPath,
resp.Header, fallbackExt)), preserve/pass along the resolution value where used,
and write an inference-source hint to stderr (not stdout) describing whether the
extension came from Content-Type, Content-Disposition, or the fallback—use the
inference value from autoAppendDocMediaExtension and emit the hint via the
existing stderr logging mechanism.
---
Nitpick comments:
In `@shortcuts/doc/doc_media_test.go`:
- Around line 289-323: The tests currently verify saved paths but don't assert
the new stderr "source-of-inference" diagnostics; update
TestDocMediaDownloadAppendsExtensionFromContentDispositionFilename to capture
and assert stderr (via cmdutil.TestFactory returns) includes the inference
source text (e.g. contains "Content-Disposition" or the exact diagnostic string
emitted by the code) after calling mountAndRunDocs/DocMediaDownload, and add a
matching stderr assertion in at least one preview test (use the same pattern:
get stderr from cmdutil.TestFactory, run mountAndRunDocs with the preview
command, then assert stderr contains the expected source-of-inference
substring). Ensure you reference the existing helpers (cmdutil.TestFactory,
mountAndRunDocs, decodeDocCommandOutput) and only add the stderr presence checks
so the path assertions remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f8bfbc8-f6c6-4a60-b70b-c645a15dbb51
📒 Files selected for processing (4)
shortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_ext.goshortcuts/doc/doc_media_preview.goshortcuts/doc/doc_media_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/doc_media_preview.go
wittam-01
force-pushed
the
feat/media-extension
branch
from
4888ee1 to
b4010c5
Compare
Change-Id: Ifc7c0e7844908b88e2d527e0933d080b140a50eb
wittam-01
force-pushed
the
feat/media-extension
branch
from
b4010c5 to
cdbdc31
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/doc/doc_media_download.go (1)
84-84:⚠️ Potential issue | 🟠 MajorDon’t discard resolution; emit inference-source hint to stderr
At Line 84, ignoring the resolution result removes the ability to log whether extension inference came from
Content-Type,Content-Disposition, or fallback.Proposed fix
- finalPath, _ := autoAppendDocMediaExtension(outputPath, resp.Header, fallbackExt) + finalPath, resolution := autoAppendDocMediaExtension(outputPath, resp.Header, fallbackExt) + if resolution != nil { + fmt.Fprintf(runtime.IO().ErrOut, "Inferred extension %q from %s (%s)\n", resolution.Ext, resolution.Source, resolution.Detail) + }As per coding guidelines,
**/*.go: Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_download.go` at line 84, The call to autoAppendDocMediaExtension currently discards the resolution result, losing whether the extension was inferred from Content-Type, Content-Disposition, or the fallback; update the invocation in the finalPath assignment to capture the returned resolution/inference value (from autoAppendDocMediaExtension) along with finalPath and then write a concise hint to stderr indicating the inference source (e.g., "inferred from Content-Type"/"Content-Disposition"/"fallback") using the captured value and the existing variables (finalPath, outputPath, resp.Header, fallbackExt); ensure you do not write this hint to stdout so JSON program output remains unchanged.
🤖 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_preview.go`:
- Line 72: The call that ignores the second return from
autoAppendDocMediaExtension should capture the inference hint (e.g.,
inferredSuffixSource) along with finalPath (replace finalPath, _ :=
autoAppendDocMediaExtension(outputPath, resp.Header, "") with a two-value
assignment), and then write a short human-readable hint to stderr stating
whether the suffix was inferred from Content-Type or Content-Disposition using
that captured value; keep program JSON output on stdout and send the hint to
stderr (use the existing preview/printing code paths so only
progress/warning/hint goes to stderr).
---
Duplicate comments:
In `@shortcuts/doc/doc_media_download.go`:
- Line 84: The call to autoAppendDocMediaExtension currently discards the
resolution result, losing whether the extension was inferred from Content-Type,
Content-Disposition, or the fallback; update the invocation in the finalPath
assignment to capture the returned resolution/inference value (from
autoAppendDocMediaExtension) along with finalPath and then write a concise hint
to stderr indicating the inference source (e.g., "inferred from
Content-Type"/"Content-Disposition"/"fallback") using the captured value and the
existing variables (finalPath, outputPath, resp.Header, fallbackExt); ensure you
do not write this hint to stdout so JSON program output remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e4c5671-bb67-4d4a-8654-9a59d9d0c08f
📒 Files selected for processing (4)
shortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_ext.goshortcuts/doc/doc_media_preview.goshortcuts/doc/doc_media_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/doc/doc_media_test.go
fangshuyu-768
approved these changes
Apr 9, 2026
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.
Change-Id: Ifc7c0e7844908b88e2d527e0933d080b140a50eb
Summary
Improve doc media extension inference for
+media-downloadand+media-previewso output files get a more accurate suffix when the user does not provide one. This change expandsContent-Typemapping coverage, addsContent- Dispositionfilename fallback, and logs which response header was used for extension inference.Changes
Content-Dispositionfilename /filename*parsing whenContent-Typecannot determine the extension.Content-TypeorContent-Disposition.Content-Dispositionfallback, and RFC 5987filename*handling.Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
New Features
Tests