refactor: migrate vc/minutes shortcuts to FileIO#336
Conversation
- vc_notes: replace vfs.Stat + validate.SafeOutputPath + validate.AtomicWrite with FileIO.Stat/Save for transcript download - minutes_download: replace validate.SafeOutputPath + validate.AtomicWriteFromReader with FileIO.Save, use FileIO.Stat for overwrite checks - Use WrapSaveError to preserve original error messages Change-Id: I7cdeddf933b1ca76266d499ec2678eb8ce6875f3
FileIO.Save errors now distinguish path validation, mkdir, and write failures to match the original stderr messages exactly. Change-Id: Ic578a15fc72f68d893ee9717fd86a4b8c72192d0
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a new error-wrapping utility for Changes
Sequence Diagram(s)sequenceDiagram
participant Cmd as CLI Command
participant HTTP as HTTP Server
participant FileIO as runtime.FileIO
participant Out as output wrapper
Cmd->>HTTP: GET media/transcript
HTTP-->>Cmd: Response (body, Content-Type, Content-Length)
Cmd->>FileIO: Stat(target) to check existence (if !--overwrite)
FileIO-->>Cmd: exists / not found
Cmd->>FileIO: Save(path, reader, options{ContentType, Size})
alt Save succeeds
FileIO-->>Cmd: saved path/size
Cmd->>Out: return success result (resolved path, size)
else Save fails
FileIO-->>Cmd: error
Cmd->>Out: WrapSaveErrorByCategory(err, category)
Out-->>Cmd: structured output error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
domain/vc
![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/minutes/minutes_download.go (1)
284-295: Consider "io_error" category for file operations.The error category
"api_error"is semantically mismatched for file I/O failures. These are local filesystem errors, not API errors. Using a category like"io_error"would be more accurate.Additionally, the
ResolvePathcall on line 291 is redundant — perlocalfileio.go, bothSaveandResolvePathuseSafeOutputPathinternally, so they return the same canonical path. The fallback on line 292-293 handles this safely, but you could simplify by usingoutputPathdirectly (sinceSavesucceeded, the path was valid).🔧 Optional: use a more accurate category
if err != nil { - return nil, common.WrapSaveErrorByCategory(err, "api_error") + return nil, common.WrapSaveErrorByCategory(err, "io_error") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/minutes/minutes_download.go` around lines 284 - 295, The error returned from opts.fio.Save is a local filesystem/I/O failure; change common.WrapSaveErrorByCategory(err, "api_error") to use an I/O category like "io_error" and return that wrapped error; also remove the redundant ResolvePath call and its fallback — after Save succeeds use outputPath directly as the savedPath in the downloadResult (keep result.Size() for sizeBytes) so update the return to &downloadResult{savedPath: outputPath, sizeBytes: result.Size()}.
🤖 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/minutes/minutes_download.go`:
- Around line 284-295: The error returned from opts.fio.Save is a local
filesystem/I/O failure; change common.WrapSaveErrorByCategory(err, "api_error")
to use an I/O category like "io_error" and return that wrapped error; also
remove the redundant ResolvePath call and its fallback — after Save succeeds use
outputPath directly as the savedPath in the downloadResult (keep result.Size()
for sizeBytes) so update the return to &downloadResult{savedPath: outputPath,
sizeBytes: result.Size()}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e5e2f8c-1a51-47aa-874b-640c3d1db90b
📒 Files selected for processing (3)
shortcuts/common/runner.goshortcuts/minutes/minutes_download.goshortcuts/vc/vc_notes.go
Greptile SummaryThis PR migrates The Confidence Score: 5/5Safe to merge — the migration is mechanically sound, the previously flagged error-category bug is fixed, and all remaining open items are non-blocking P2. No new P0 or P1 issues found. The critical "api_error" category regression from the prior review round is resolved (now correctly "io"). Remaining open items from previous threads (relative saved_path in vc_notes, ResolvePath error swallowed) are P2 quality/consistency concerns that do not affect correctness or safety of the primary download path. All three files score 5/5 individually. No files require special attention.
|
| Filename | Overview |
|---|---|
| shortcuts/common/runner.go | Adds WrapSaveErrorByCategory helper that maps FileIO save errors to structured output errors using a caller-provided category; well-structured with clear exit-code and error-type separation. |
| shortcuts/minutes/minutes_download.go | Migrates media download path to FileIO.Stat + FileIO.Save; error category is correctly set to "io" for disk-level failures; ResolvePath fallback preserves backward-compatible saved-path output. |
| shortcuts/vc/vc_notes.go | Migrates transcript download to FileIO.Stat + FileIO.Save; structured error matching for ErrPathValidation/MkdirError is consistent with the rest of the FileIO migration series. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User invokes download command] --> B{single or batch?}
B -- single --> C[FileIO.Stat overwrite check]
B -- batch --> D[FileIO.Stat: is --output a file?]
D --> C
C -- file exists & no overwrite --> E[ErrValidation: file already exists]
C -- proceed --> F[HTTP stream download]
F --> G[FileIO.Save]
G -- ErrPathValidation --> H[WrapSaveErrorByCategory → ErrValidation exit 2]
G -- MkdirError --> I[WrapSaveErrorByCategory → Errorf ExitInternal, io]
G -- WriteError/other --> J[WrapSaveErrorByCategory default → Errorf ExitInternal, io]
G -- success --> K[FileIO.ResolvePath]
K -- ok --> L[Return absolute saved_path in JSON]
K -- error/empty --> M[Fallback: return original outputPath]
subgraph vc_notes transcript path
N[downloadTranscriptFile] --> O[FileIO.Stat overwrite check]
O -- exists & no overwrite --> P[Log warning, return relative transcriptPath]
O -- proceed --> Q[DoAPI transcript download]
Q --> R[FileIO.Save]
R -- error --> S[Log to stderr, return empty string]
R -- success --> T[Return relative transcriptPath]
end
Reviews (2): Last reviewed commit: "fix: correct error category and handle R..." | 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@6d36a82ec17271512f2b659fe55d563bd648f38a🧩 Skill updatenpx skills add larksuite/cli#feat/fileio-migrate-vc-minutes -y -g |
…ownload - WrapSaveErrorByCategory: "api_error" → "io" for local file I/O failures - ResolvePath: check returned error instead of discarding it Change-Id: I49e5a667096a81f412816c4ef88b68db8ef4a45f
2 participants
Summary
minutes_download.go:SafeOutputPath+EnsureWritableFile+MkdirAll+AtomicWriteFromReader→FileIO.Stat(overwrite check) +FileIO.Save;safePath→ResolveSavePathvc_notes.go:vfs.Stat+SafeOutputPath+MkdirAll+AtomicWrite→FileIO.Stat+FileIO.Save; error messages preserved via structured error matching (ErrPathValidation/MkdirError)common/runner.go: addWrapSaveErrorByCategory(err, category)helper for standardized save error mappingPart of the FileIO shortcut migration series:
Test plan
go test ./shortcuts/vc/...— all passgo test ./shortcuts/minutes/...— all passgo test ./shortcuts/common/...— all passgolangci-lint run— no new issueslark-cli vc +notes --minute-tokens <token>with transcript downloadlark-cli minutes +download --token <token>with overwrite checkSummary by CodeRabbit