fix(base): resolve record-list --view-id by view name#258
fix(base): resolve record-list --view-id by view name#258wwenrr wants to merge 3 commits intolarksuite:mainfrom
Conversation
|
|
github-actions
bot
added
domain/base
📝 WalkthroughWalkthroughAdded view-name-to-ID resolution for record-list: inputs to Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Shortcut
participant Runtime as Runtime
participant ViewsAPI as Views Service
participant RecordsAPI as Records Service
CLI->>Runtime: run record-list with --view-id="Main"
Runtime->>ViewsAPI: list views (paged) / resolve view name -> view_id="vew_x"
ViewsAPI-->>Runtime: return view_id="vew_x"
Runtime->>RecordsAPI: list records with view_id="vew_x", offset/limit
RecordsAPI-->>Runtime: return records (rec_1,...)
Runtime-->>CLI: render records output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes Key changes:
Both prior review comments (silent failure for 200+ views, and Confidence Score: 5/5Safe to merge — both prior review concerns are addressed, new logic is well-tested, and no P0/P1 issues remain. The pagination loop correctly handles all cases (view on first page, view on later page, empty pages with reliable total, missing view, and view with no canonical ID). The two previously raised concerns — silent failure for 200+ views and the strings.HasPrefix idiom — are both fully resolved. Three new execute tests and one dry-run test provide solid coverage. No new correctness or security issues were introduced. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "fix(base): guard missing canonical id in..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/base/record_ops.go`:
- Line 36: The call to listAllViews(runtime, runtime.Str("base-token"),
baseTableID(runtime), 0, 200) only fetches the first page (limit 200), so views
beyond that are omitted; update the code around that call to implement
pagination: repeatedly call listAllViews using an increasing offset (e.g.,
offset += limit) and collect/append returned views until the returned slice
length is less than the limit (or no more items), then use the aggregated views
for name resolution; alternatively, if the API supports it, replace the
hardcoded limit with a sufficiently large value, but prefer the looped
pagination approach to ensure correctness when using listAllViews,
runtime.Str("base-token"), and baseTableID(runtime).
- Around line 40-44: resolveViewRef may return a view without a canonical ID so
change the success path after resolveViewRef(viewRef) to call viewID(view) and
if it returns empty, fail fast by returning an error (e.g., fmt.Errorf("view %q
has no canonical id", viewRef or view.Name)) instead of returning an empty
string; update the function that currently returns viewID(view) to validate
non-empty, import the fmt package, and ensure callers such as executeRecordList
receive an error rather than an empty view_id.
🪄 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: b080c0f7-526c-4989-ab45-9f751390f9e2
📒 Files selected for processing (2)
shortcuts/base/base_execute_test.goshortcuts/base/record_ops.go
|
Addressed review feedback ✅ Thanks for the detailed comments. I updated the PR with the following fixes:
Tests added/updated
Verificationdocker run --rm -v /tmp/larksuite-cli:/src -w /src golang:1.24 bash -lc "export PATH=/usr/local/go/bin:$PATH; gofmt -w shortcuts/base/record_ops.go shortcuts/base/base_dryrun_ops_test.go shortcuts/base/base_execute_test.go && go test ./shortcuts/base -run 'TestBaseRecordExecuteListWithViewNameResolvesToViewID|TestBaseRecordResolveViewIDAcrossPages|TestDryRunRecordListWithViewName'"Result: pass ( |
|
Addressed CodeRabbit feedback ✅ I verified and handled both actionable points:
Also kept the prior dry-run consistency + prefix cleanup improvements. Added/updated tests
Verificationdocker run --rm -v /tmp/larksuite-cli:/src -w /src golang:1.24 bash -lc "export PATH=/usr/local/go/bin:$PATH; gofmt -w shortcuts/base/record_ops.go shortcuts/base/base_execute_test.go && go test ./shortcuts/base -run 'TestBaseRecordExecuteListWithViewNameResolvesToViewID|TestBaseRecordResolveViewIDAcrossPages|TestDryRunRecordListWithViewName|TestBaseRecordResolveViewIDMissingCanonicalID'"Result: pass ( |
2 participants
Summary
base +record-list --view-idrobust when users pass a view name instead of a rawvew_...IDview_idorview_name) to canonicalview_idbefore records query--view-idis already avew_.../viw_...idWhy
Issue #255 reports that after
+view-set-filtersucceeds,+record-list --view-idcan still appear to return full data in some usage flows.A practical root cause addressed by this PR: users pass a view name, while record-list previously forwarded raw input as
view_idquery param.Changes
shortcuts/base/record_ops.goresolveRecordListViewID(runtime, viewRef)vew_,viw_) directlyview_id/view_nameshortcuts/base/base_execute_test.goTestBaseRecordExecuteListWithViewNameResolvesToViewIDTestBaseRecordResolveViewIDAcrossPagesTestBaseRecordResolveViewIDMissingCanonicalIDshortcuts/base/base_dryrun_ops_test.goTestDryRunRecordListWithViewNameVerification
docker run --rm -v /tmp/larksuite-cli:/src -w /src golang:1.24 bash -lc "export PATH=/usr/local/go/bin:$PATH; gofmt -w shortcuts/base/record_ops.go shortcuts/base/base_dryrun_ops_test.go shortcuts/base/base_execute_test.go && go test ./shortcuts/base -run 'TestBaseRecordExecuteListWithViewNameResolvesToViewID|TestBaseRecordResolveViewIDAcrossPages|TestDryRunRecordListWithViewName|TestBaseRecordResolveViewIDMissingCanonicalID'"Result: pass (
ok github.com/larksuite/cli/shortcuts/base)Additional evidence (real CLI terminal run)
To make behavior explicit, I captured real CLI dry-run outputs:
Observed:
+record-listwithout--view-id=> request has onlyoffset/limit--view-id Main=> request includes resolved view placeholder in dry-run--view-id vew_x=> request includesview_id=vew_xCloses #255