refactor: use resolver token list limits#91
Conversation
feralfile-bot
left a comment
There was a problem hiding this comment.
Reviewers: 3 (approve: 0, comment: 3, request-changes: 0).
Critical issues
- None validated.
Medium issues
src/commands/find.ts:303still resolves Objkt collections withDP1_MAX_ITEMSbefore--limitis parsed inrunResolvedTarget. This was raised by all three reviewers and is supported by the current flow:find <objkt collection> --limit 1will still callresolveObjktCollection(parsed.slug, 1024), causing the Objkt GraphQL token query to request1025rows, then slice locally later. It is bounded by the DP-1 cap, so this is not a correctness blocker, but it contradicts the PR’s stated “honor the CLI/DP-1 cap” behavior at the resolver/API boundary and keeps unnecessary latency/API load for small-limit runs. Consider parsing the effective limit beforeresolveTargetor otherwise passing it into the Objkt collection branch.
Missing tests
- The new
token-listcommand execution paths are not covered at CLI/flow level. The added parser fixtures and directresolveObjktCollectiontest are useful, but they would not catch the current caller issue above, nor regressions in prompt wording,hasMore, title propagation, empty token handling, or resolver-backed Feral File show / Verse series / SuperRare collection fallback behavior. This gap was also raised by all three reviewers. A focused mock-backedfindcommand test for at least Objkt collection--limitbehavior, and ideally one resolver-backedtoken-listfallback path, would close the main risk.
No PR discussion comments indicated any of these were already deferred or tracked elsewhere.
feralfile-bot
left a comment
There was a problem hiding this comment.
Reviewers: 3 (approve: 0, comment: 2, request-changes: 1).
Critical issues
None.
Medium issues
-
package.json:60andpackage-lock.jsonpin@feralfile/source-resolverto54be943014c8cf7860114510257cdd713bbf4e79, while the PR description says this should use1706377e563e01743311127389b1b6a2fa52417d. Sincesrc/commands/find.tsnow depends onresolveTokenInfos(input, { limit }), please either pin the intended accepted resolver SHA or make the dependency choice explicit. As written, the PR appears to ship against a different resolver point than advertised. -
All three reviewers flagged the same bounded-resolution issue:
resolveTokenListInput()always callsresolveTokenInfos(input, { limit: DP1_MAX_ITEMS })beforerunResolvedTarget()parses the user’s--limit. For collection-like resolver paths,ff-cli find URL --limit 1can still ask the resolver for up to 1024 tokens and only slice locally afterward. That weakens the stated goal of bounded collection resolution on large catalogs. Please parse the effective CLI limit before resolver-backed resolution and pass it intoresolveTokenInfos.
Missing tests
- The new resolver-backed
token-listcommand path lacks command-level coverage. The added tests mainly validateparseFindInputshapes, but do not assert thatresolveTokenInfosreceives the user limit, that{ kind: 'tokens', hasMore }reaches the “first N tokens” prompt wording, or that token-list results proceed through the playlist flow instead of falling back to older single-token paths. A focused mocked command/helper test for an Objkt collection, Feral File show, or Verse series would cover the behavior this PR adds.
No PR discussion items were marked as deferred or already tracked elsewhere, so nothing was skipped on that basis.
Problem
ff-cli findhad started carrying resolver-specific workaround code for bounded Objkt collection reads and SuperRare collection fallback. That duplicated behavior that belongs in@feralfile/source-resolver.Why It Matters
The CLI should ask the resolver library for supported source token lists directly. Keeping token resolution semantics in the library avoids site-specific pagination and URL-shape hacks in the command layer.
Changes
@feralfile/source-resolverto54be943014c8cf7860114510257cdd713bbf4e79, from resolver PR feat: support bounded token info resolution ff-source-resolver#9.findtoken-list resolution to callresolveTokenInfos(input, { limit: DP1_MAX_ITEMS })and preserve resolverhasMore.Acceptance Checks
npm run verifyresolveTokenInfos(..., { limit })is covered in the resolver package for DOM and API token-list paths.Remaining Risk
This PR depends on feral-file/ff-source-resolver#9 being accepted or the dependency SHA being replaced with the final merged resolver commit before release.