Skip to content

Support API pagination in the CLI#242

Open
chrisghill wants to merge 5 commits into
mainfrom
support-pagination-in-SDK
Open

Support API pagination in the CLI#242
chrisghill wants to merge 5 commits into
mainfrom
support-pagination-in-SDK

Conversation

@chrisghill

Copy link
Copy Markdown
Member

No description provided.

@chrisghill chrisghill requested a review from coryodaniel as a code owner June 5, 2026 22:18
@chrisghill

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jun 5, 2026

Copy link
Copy Markdown

Claude finished @chrisghill's task in 2m 35s —— View job


Code Review

  • Gather context and read CLAUDE.md
  • Review changed files
  • Post review feedback

Summary

This PR adds lazy-loading interactive pagination (via bubbletea TUI) to all list commands, migrates them from SDK.List() to SDK.Iter(), adds --output json flags uniformly, and introduces a new resource list command. The core abstraction in internal/cli/pager.go is well-designed — the TTY detection gate, iter.Pull2 lifecycle management with defer stop(), and the prefetch-on-scroll approach are all solid.

A few issues worth fixing before merge:


Bugs

1. Unknown --sort value silently uses name in resource list

cmd/resource.go:644–650:

if sortField != "" {
    listInput.SortBy = resources.SortByName    // default even for unknown values
    if strings.EqualFold(sortField, "created_at") {
        listInput.SortBy = resources.SortByCreatedAt
    }
    ...
}

If the user passes --sort foo, it silently defaults to name-sort with no error. All other flags (--output, --origin) return an error on unknown values. buildOciReposListInput in repository.go has the same silent-default pattern — both should validate the sort field and return an error on unrecognized values.

Fix this →

2. r.Origin written directly to table as an all-caps string

cmd/resource.go:677:

return []string{r.ID, r.Name, typeName, r.Origin, r.CreatedAt.Format(...)}

If Origin is a typed enum constant (e.g. "IMPORTED", "PROVISIONED"), the table column will show uppercase values while the flag accepts lowercase (--origin imported). Consider strings.ToLower(r.Origin) for consistency with the flag input.


Design Concerns

3. Column widths sized only from the first batch

pager.go:182–184:

if !m.colsApplied && len(msg.rows) > 0 {
    m.table.SetColumns(sizeColumns(m.columns, msg.rows))
    m.colsApplied = true
}

After the first page sizes the columns, later pages with wider data will overflow the fixed column width (capped at maxWidth = 50). For IDs and names this is usually fine, but a long resource name on page 3 will appear truncated in the TUI with no indication. At minimum this should be documented as a known limitation.

4. SDK dependency is a pre-release pseudo-version

go.mod:

github.com/massdriver-cloud/massdriver-sdk-go v0.2.6-0.20260601065427-16cf4d209a22

This is a Go pseudo-version (unreleased commit hash), not a tagged release. The Iter() and types.Collect() APIs this PR depends on should be in a proper tagged release before merging to avoid pinning to an unlabeled internal state.

5. --order without --sort is silently ignored

cmd/resource.go:648–651:

listInput.SortOrder = resources.SortAsc
if strings.EqualFold(sortOrder, "desc") {
    listInput.SortOrder = resources.SortDesc
}

This block is only reached when sortField != "", so --order desc alone does nothing. The flag description should note this dependency, or the code should warn when --order is provided without --sort.


Positive Observations

  • The iter.Pull2 / defer stop() pattern in runInteractivePager is correct — the iterator goroutine is always released on exit, including on user quit or error.
  • The single-flight loading guard on loadCmd() correctly prevents concurrent calls to the underlying pull function.
  • The next == after sentinel in ListResourceTypes (internal/api/resource_type.go:1041) correctly guards against an infinite loop if the server echoes the same cursor.
  • streamAll in non-interactive mode (pipe/CI) drains cleanly without bubbletea overhead — the TTY gate is the right approach.
  • Tests in pager_test.go cover the non-interactive path and error propagation, and TestListWalksPages verifies the multi-page cursor walk with mock responses.
  • Fixing the pre-existing bug in cmd/resource_type.go where err was returned after tbl.Print() without being checked is a good catch.
  • Adding ls aliases and --output json uniformly across all list commands is a clean UX improvement.

Overall: The pagination architecture is well-thought-out and the implementation is clean. The main blockers are the SDK pseudo-version and the silent sort field default. The r.Origin casing is a polish issue. Happy to approve once those are addressed.

@chrisghill chrisghill requested a review from ChristensenJoe June 6, 2026 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants