refactor: migrate base shortcuts to FileIO#347
Merged
Conversation
- loadJSONInput: SafeInputPath + vfs.ReadFile → fio.Open + io.ReadAll - parseJSONObject/parseJSONArray/parseJSONValue/parseObjectList/ parseStringListFlexible: add fio param, pass through to loadJSONInput - parseStringList: inline comma-split (no longer depends on fio) - record_upload_attachment: SafeInputPath + vfs.Stat → FileIO.Stat with ErrPathValidation check; vfs.Open → FileIO.Open - All ops files pass runtime.FileIO() to parse helpers Change-Id: Ie5938d8ad8600e185a7f5019c6e8c51b6b80c988
Reduce visual noise in functions that call runtime.FileIO() multiple times by caching into a local `fio` variable. Change-Id: I447215b9f48483fd43b559b5801baf0d046ec59b
Replace explicit `fio fileio.FileIO` parameter with `pc *parseCtx` across all JSON/file parsing helpers. Callers create a parseCtx once via `newParseCtx(runtime)` and pass it through. This makes the dependency injection point extensible (e.g. adding logger, context) without changing function signatures again. Change-Id: I6155c83cb3a41655c73013b2042935ac6a7b16ac
📝 WalkthroughWalkthroughIntroduces an unexported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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 |
github-actions
bot
added
domain/base
Greptile SummaryThis PR migrates the Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/consistency issues with no functional impact. Both P0/P1 concerns from previous review threads are resolved. The double loadJSONInput call pattern in workflow_create.go and parseObjectList is harmless (second call is a passthrough on already-resolved content) and does not affect correctness or user-visible behavior. shortcuts/base/workflow_create.go and shortcuts/base/base_shortcut_helpers.go have minor redundancy worth cleaning up, but neither blocks merge.
|
| Filename | Overview |
|---|---|
| shortcuts/base/base_shortcut_helpers.go | Introduces parseCtx/newParseCtx and migrates loadJSONInput to FileIO.Open; parseObjectList pre-calls loadJSONInput then passes resolved content to parseJSONArray/parseJSONObject, which each call loadJSONInput a second time (redundant but harmless). |
| shortcuts/base/workflow_create.go | Still uses the old two-step loadJSONInput + parseJSONObject(pc, raw, …) pattern in all three lifecycle functions; since parseJSONObject now calls loadJSONInput internally, the explicit pre-call is redundant and inconsistent with workflow_update.go. |
| shortcuts/base/workflow_update.go | Correctly simplified to call parseJSONObject(pc, runtime.Str("json"), "json") directly in all three lifecycle functions; redundant loadJSONInput pre-call removed cleanly. |
| shortcuts/base/record_upload_attachment.go | Migrated Stat/Open to FileIO; now correctly distinguishes ErrPathValidation from other stat errors and reports a useful 'file not accessible' message for the non-validation case. |
| shortcuts/base/helpers.go | parseJSONObject, parseJSONArray, and parseStringListFlexible now accept *parseCtx and call loadJSONInput internally, enabling @file support uniformly. |
| shortcuts/base/helpers_test.go | Tests updated to use testPC with LocalFileIO and add explicit @file path test coverage; assertions preserved. |
| shortcuts/base/base_shortcuts_test.go | Mechanical update to pass testPC as first argument to all parseObjectList call sites; no new logic. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["CLI flag value\n(--json / --file)"] --> B{starts with @?}
B -- No --> C["Return raw string"]
B -- Yes --> D["pc.fio.Open(path)"]
D -- ErrPathValidation --> E["FlagError: invalid JSON file path"]
D -- other error --> F["FlagError: cannot open JSON file"]
D -- success --> G["io.ReadAll(f)"]
G --> H{content empty?}
H -- Yes --> I["FlagError: JSON file is empty"]
H -- No --> C
C --> J["parseJSONObject / parseJSONArray\n/ parseJSONValue / parseObjectList"]
J --> K["JSON unmarshal"]
K -- error --> L["formatJSONError (with tip)"]
K -- success --> M["Return typed value"]
style E fill:#f99
style F fill:#f99
style I fill:#f99
style L fill:#f99
Reviews (3): Last reviewed commit: "fix: guard nil FileIO before @file and a..." | Re-trigger Greptile
![greptile-apps[bot]](https://waps.l3s.uni-hannover.de/live/im_/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@0684f380098f9bf6406a16f64b61d640c03df8dd🧩 Skill updatenpx skills add larksuite/cli#feat/fileio-migrate-base -y -g |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/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 (3)
shortcuts/base/workflow_update.go (3)
32-40: RedundantloadJSONInputcall - minor inefficiency.
loadJSONInputis called explicitly on line 33, then the resultingrawstring is passed toparseJSONObjecton line 37. However,parseJSONObjectinternally callsloadJSONInputagain (seehelpers.go:33).If the original input was
@path/to/file.json, the firstloadJSONInputresolves it to JSON content. The second call (insideparseJSONObject) then treats that content as literal JSON (not a file path), so it works correctly. However, this is redundant.Consider calling
parseJSONObject(pc, runtime.Str("json"), "json")directly, which handles both resolution and parsing in one step.♻️ Suggested simplification
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { if strings.TrimSpace(runtime.Str("base-token")) == "" { return common.FlagErrorf("--base-token must not be blank") } if strings.TrimSpace(runtime.Str("workflow-id")) == "" { return common.FlagErrorf("--workflow-id must not be blank") } pc := newParseCtx(runtime) - raw, err := loadJSONInput(pc, runtime.Str("json"), "json") - if err != nil { - return err - } - if _, err := parseJSONObject(pc, raw, "json"); err != nil { + if _, err := parseJSONObject(pc, runtime.Str("json"), "json"); err != nil { return err } return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/workflow_update.go` around lines 32 - 40, The code redundantly calls loadJSONInput before parseJSONObject (which itself calls loadJSONInput); update the block that currently uses newParseCtx(runtime) + loadJSONInput(...) + parseJSONObject(...) to instead call parseJSONObject(pc, runtime.Str("json"), "json") directly, removing the separate loadJSONInput invocation and its raw variable while keeping the existing error returns and using newParseCtx(runtime) and runtime.Str("json") as the inputs.
55-63: Same redundancy pattern in Execute.Consider simplifying to a single
parseJSONObjectcall for consistency with other command handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/workflow_update.go` around lines 55 - 63, In Execute, remove the redundant two-step flow that first calls loadJSONInput and then parseJSONObject; instead keep newParseCtx(runtime) and call parseJSONObject directly with the parse context and the raw JSON string from runtime.Str("json") (replace the loadJSONInput + parseJSONObject sequence with a single parseJSONObject call). Update references to parseJSONObject, loadJSONInput, newParseCtx, and runtime.Str("json") so the code obtains the parse context via newParseCtx(runtime) and parses the JSON in one step, returning any error directly.
43-47: Same redundancy pattern in DryRun.The explicit
loadJSONInputfollowed byparseJSONObjectis redundant sinceparseJSONObjecthandles resolution internally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/workflow_update.go` around lines 43 - 47, This duplicate resolution pattern uses loadJSONInput before parseJSONObject; remove the intermediate load and let parseJSONObject handle resolution directly. In the DryRun code path replace the loadJSONInput + parseJSONObject sequence (the block using newParseCtx(runtime), loadJSONInput and then parseJSONObject) with a single call that assigns body from parseJSONObject(pc, runtime.Str("json"), "json"), referencing newParseCtx, parseJSONObject and runtime.Str("json") so the JSON is resolved and parsed in one step.
🤖 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/base/base_shortcut_helpers.go`:
- Around line 41-48: The error handling for pc.fio.Open(path) incorrectly labels
all open failures as "invalid JSON file path"; update the Open error branch in
the function that reads JSON so you use errors.As to detect
fileio.PathValidationError and only return common.FlagErrorf("--%s invalid JSON
file path %q: %v", flagName, path, err) for that case, otherwise return a
different message such as common.FlagErrorf("--%s cannot open JSON file %q: %v",
flagName, path, err); add the "errors" import if missing and keep the existing
io.ReadAll error handling unchanged.
---
Nitpick comments:
In `@shortcuts/base/workflow_update.go`:
- Around line 32-40: The code redundantly calls loadJSONInput before
parseJSONObject (which itself calls loadJSONInput); update the block that
currently uses newParseCtx(runtime) + loadJSONInput(...) + parseJSONObject(...)
to instead call parseJSONObject(pc, runtime.Str("json"), "json") directly,
removing the separate loadJSONInput invocation and its raw variable while
keeping the existing error returns and using newParseCtx(runtime) and
runtime.Str("json") as the inputs.
- Around line 55-63: In Execute, remove the redundant two-step flow that first
calls loadJSONInput and then parseJSONObject; instead keep newParseCtx(runtime)
and call parseJSONObject directly with the parse context and the raw JSON string
from runtime.Str("json") (replace the loadJSONInput + parseJSONObject sequence
with a single parseJSONObject call). Update references to parseJSONObject,
loadJSONInput, newParseCtx, and runtime.Str("json") so the code obtains the
parse context via newParseCtx(runtime) and parses the JSON in one step,
returning any error directly.
- Around line 43-47: This duplicate resolution pattern uses loadJSONInput before
parseJSONObject; remove the intermediate load and let parseJSONObject handle
resolution directly. In the DryRun code path replace the loadJSONInput +
parseJSONObject sequence (the block using newParseCtx(runtime), loadJSONInput
and then parseJSONObject) with a single call that assigns body from
parseJSONObject(pc, runtime.Str("json"), "json"), referencing newParseCtx,
parseJSONObject and runtime.Str("json") so the JSON is resolved and parsed in
one step.
🪄 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: 0417dec6-e578-4e90-8dc2-b7c8384533eb
📒 Files selected for processing (14)
shortcuts/base/base_shortcut_helpers.goshortcuts/base/base_shortcuts_test.goshortcuts/base/dashboard_block_create.goshortcuts/base/dashboard_block_update.goshortcuts/base/dashboard_ops.goshortcuts/base/field_ops.goshortcuts/base/helpers.goshortcuts/base/helpers_test.goshortcuts/base/record_ops.goshortcuts/base/record_upload_attachment.goshortcuts/base/table_ops.goshortcuts/base/view_ops.goshortcuts/base/workflow_create.goshortcuts/base/workflow_update.go
- Distinguish path validation errors from file access errors in loadJSONInput - Report actual error instead of misleading "file not found" in Stat fallback - Remove redundant loadJSONInput calls in workflow_update (parseJSONObject handles it) Change-Id: I36a0e1471f8cf085737ec0e08ab9aa5113a8a40b
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/base/base_shortcut_helpers.go (1)
30-31: Add a defensive nil-guard before dereferencingpc.fio.
loadJSONInputassumespcandpc.fioare always initialized; a missed call-site setup would panic instead of returning a structured error.Proposed patch
func loadJSONInput(pc *parseCtx, raw string, flagName string) (string, error) { + if pc == nil || pc.fio == nil { + return "", common.FlagErrorf("--%s internal error: file I/O is not configured", flagName) + } raw = strings.TrimSpace(raw) if raw == "" { return "", common.FlagErrorf("--%s cannot be empty", flagName) }Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/base_shortcut_helpers.go` around lines 30 - 31, loadJSONInput currently dereferences pc.fio without checks and can panic if pc or pc.fio is nil; add defensive nil-guards at the top of loadJSONInput to validate pc != nil and pc.fio != nil and return a clear error (including flagName) instead of panicking. Locate loadJSONInput in shortcuts/base/base_shortcut_helpers.go and add these checks before using pc.fio (also apply the same guard pattern to any other spots in that file that use pc.fio, e.g., the other occurrence around line 42).
🤖 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/base/base_shortcut_helpers.go`:
- Around line 30-31: loadJSONInput currently dereferences pc.fio without checks
and can panic if pc or pc.fio is nil; add defensive nil-guards at the top of
loadJSONInput to validate pc != nil and pc.fio != nil and return a clear error
(including flagName) instead of panicking. Locate loadJSONInput in
shortcuts/base/base_shortcut_helpers.go and add these checks before using pc.fio
(also apply the same guard pattern to any other spots in that file that use
pc.fio, e.g., the other occurrence around line 42).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ae843fa-e173-47a1-9f61-0f1a3792c0a3
📒 Files selected for processing (3)
shortcuts/base/base_shortcut_helpers.goshortcuts/base/record_upload_attachment.goshortcuts/base/workflow_update.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/base/record_upload_attachment.go
- shortcuts/base/workflow_update.go
RuntimeContext.FileIO() can return nil when no provider is registered. Add nil checks before Open/Stat calls to return structured errors instead of panicking. Change-Id: Ie00799d0dea559411d9429f3a88acd02428a47bf
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/base/base_shortcut_helpers.go (1)
30-44:⚠️ Potential issue | 🟡 MinorGuard against nil
parseCtxbefore dereferenceLine 42 dereferences
pc.fiowithout checkingpc != nil. If any caller passes a nilpc, this path panics instead of returning a structured flag error.Suggested fix
func loadJSONInput(pc *parseCtx, raw string, flagName string) (string, error) { raw = strings.TrimSpace(raw) if raw == "" { return "", common.FlagErrorf("--%s cannot be empty", flagName) } @@ if path == "" { return "", common.FlagErrorf("--%s file path cannot be empty after @", flagName) } - if pc.fio == nil { + if pc == nil || pc.fio == nil { return "", common.FlagErrorf("--%s `@file` inputs require a FileIO provider", flagName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/base_shortcut_helpers.go` around lines 30 - 44, The function loadJSONInput currently dereferences pc.fio without verifying pc itself is non-nil; update loadJSONInput to first check if pc == nil and return a structured flag error (using common.FlagErrorf with the same --%s message pattern) before accessing pc.fio, so callers that pass a nil *parseCtx get a proper error instead of a panic; ensure the error message clearly indicates that a FileIO provider is required for `@file` inputs and reference the flagName in the message.
🤖 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/base/record_upload_attachment.go`:
- Around line 211-212: The function uploadAttachmentToBase currently calls
runtime.FileIO().Open() unchecked which can panic if runtime.FileIO() is nil;
change uploadAttachmentToBase signature to accept a validated FileIO (e.g., fio)
parameter instead of calling runtime.FileIO() inside, update
executeRecordUploadAttachment to pass the already-checked fio (following the
parseCtx pattern), and remove the internal call to runtime.FileIO(); ensure all
callers are updated to pass the fio so the function uses fio.Open(filePath, ...)
safely.
---
Outside diff comments:
In `@shortcuts/base/base_shortcut_helpers.go`:
- Around line 30-44: The function loadJSONInput currently dereferences pc.fio
without verifying pc itself is non-nil; update loadJSONInput to first check if
pc == nil and return a structured flag error (using common.FlagErrorf with the
same --%s message pattern) before accessing pc.fio, so callers that pass a nil
*parseCtx get a proper error instead of a panic; ensure the error message
clearly indicates that a FileIO provider is required for `@file` inputs and
reference the flagName in the message.
🪄 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: 53605218-8382-4773-967b-74605f1c010c
📒 Files selected for processing (2)
shortcuts/base/base_shortcut_helpers.goshortcuts/base/record_upload_attachment.go
kongenpei
approved these changes
Apr 9, 2026
This was referenced 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.
Summary
parseCtxstruct to carryfileio.FileIOdependency through JSON/file parsing helpers, replacing directvalidate.SafeInputPath+vfs.ReadFile/vfs.OpencallsloadJSONInput:SafeInputPath+vfs.ReadFile→pc.fio.Open+io.ReadAllparseJSONObject/parseJSONArray/parseJSONValue/parseObjectList/parseStringListFlexible: accept*parseCtxinstead offileio.FileIOparseStringList: inlined comma-split (no file I/O dependency)record_upload_attachment:SafeInputPath+vfs.Stat→FileIO.Stat(withErrPathValidationerror distinction);vfs.Open→FileIO.Openpc := newParseCtx(runtime)and pass throughPart of the FileIO shortcut migration series:
Test plan
go test ./shortcuts/base/...— all passgolangci-lint run— no new issueslark-cli base +record-create --json @file.jsonlark-cli base +record-upload-attachment --file <path>Summary by CodeRabbit