Conversation
Turn `prolific filters` into a parent command with `list` and `search` subcommands, and add `prolific filters search <query>` backed by the new GET /api/v1/filters/search/ endpoint. - Highlight matched spans in titles, questions, descriptions, filter IDs and choice labels using the API's code-point offsets - Preview up to three matching choices per filter with nested counts - Header shows query and result count on the first screen; results are ranked, separated by rules, with type and category as a subtitle - `--limit` is the number of results wanted and `--all` fetches every match; pages are requested automatically via a new generic client.FetchPages helper - Long output is piped through PROLIFIC_PAGER / PAGER (default `less -FRX`) when stdout is a terminal; `--no-pager` disables this - `--json` output for scripting The contract test entry and README coverage row for filters_SearchFilters are deferred until the API docs (prolific-oss/prolific#16270) publish, since the coverage test rejects operations missing from the live spec. BREAKING CHANGE: `prolific filters` and `prolific filters -n` no longer list filters directly; use `prolific filters list` instead. Co-Authored-By: Claude Fable 5.1 <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Co-Authored-By: Claude Fable 5.1 <[email protected]>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate issues remain in API coverage, output formats, help-text handling, and pager error handling.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (4)
What changed in this PR
Adds prolific filters search <query> with pagination, highlighted results, JSON output, and pager support while restructuring filters into parent, list, and search commands.
Changes:
- Added search models, client method, pagination helper, mocks, and tests.
- Added highlighted rendering, choice previews, JSON output, and pager handling.
- Updated command wiring, documentation, changelog, and breaking-change guidance.
Review findings:
- Moderate (2 votes): Add HTTP-level coverage for search endpoint paths and query parameters.
- Moderate (1 vote): Add standard output formats to
filters list. - Nit (2 votes): Pass the error directly to
fmt.Errorf. - Moderate (1 vote): Handle and render documented
help_textmatches. - Moderate (2 votes): Add
HelpTextto the search result model. - Moderate (3 votes): Treat benign pager
EPIPEerrors as successful exits.
| File | Change |
|---|---|
ui/pager.go |
Pager resolution and execution |
ui/pager_test.go |
Pager behavior tests |
README.md |
Updated command listing |
model/filter_search.go |
Search response models |
mock_client/mock_client.go |
Generated API mocks |
go.mod |
Added dependencies |
cmd/root.go |
Registered filters parent command |
cmd/filters/search.go |
Search command implementation |
cmd/filters/search_view.go |
Search result rendering |
cmd/filters/search_view_test.go |
Rendering and highlighting tests |
cmd/filters/search_test.go |
Command and pagination tests |
cmd/filters/list.go |
Converted list command to subcommand |
cmd/filters/list_test.go |
Updated list tests |
cmd/filters/filters.go |
Parent command wiring |
cmd/filters/filters_test.go |
Parent wiring tests |
client/responses.go |
Search response type |
client/pagination.go |
Generic page fetching |
client/pagination_test.go |
Pagination tests |
client/client.go |
Search API method |
CHANGELOG.md |
Feature and breaking-change notes |
Files not reviewed (1)
- mock_client/mock_client.go: Generated file
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| params := url.Values{} | ||
| params.Set("q", query) | ||
| params.Set("limit", strconv.Itoa(limit)) | ||
| params.Set("offset", strconv.Itoa(offset)) | ||
| if workspaceID != "" { |
There was a problem hiding this comment.
Done in d6bae3f: added TestSearchFiltersSendsExpectedRequest and TestSearchFiltersOmitsWorkspaceIDWhenEmpty in client/client_test.go, following the participant-groups httptest pattern. They assert method, path and the q, workspace_id, limit and offset parameters.
| // FilterSearchResult is a filter returned by the filter search endpoint. | ||
| type FilterSearchResult struct { | ||
| FilterID string `json:"filter_id"` | ||
| Title string `json:"title"` | ||
| Description string `json:"description"` | ||
| Question *string `json:"question"` |
There was a problem hiding this comment.
Not changed, deliberately. The spec for FilterSearchResult in prolific-oss/prolific#16270 has no help-text property; researcher_help_text appears only as a value in match.fields and as a highlight field. The API searches help text but does not return it, so there is nothing to decode or render. Showing it under "Matched on" is the most we can do without a second request. If the API later adds the property to the result it is a one-line model change.
| renderErr := render(stdin) | ||
| closeErr := stdin.Close() | ||
| waitErr := cmd.Wait() | ||
|
|
||
| if renderErr != nil { | ||
| return renderErr |
There was a problem hiding this comment.
Done in d6bae3f. RunPager now treats EPIPE, io.ErrClosedPipe and os.ErrClosed from the renderer as a normal exit while still surfacing real render errors and a non-zero pager exit. Covered by TestRunPagerTreatsEarlyQuitAsSuccess, which pipes a large render through head -n 1.
| } | ||
|
|
||
| if err := renderSearch(c, opts, w); err != nil { | ||
| return fmt.Errorf("error: %s", err.Error()) |
matt-kinton
left a comment
There was a problem hiding this comment.
Nice one, this is a genuinely useful command and the test coverage is good. The highlight clamp/merge logic is careful, and good call flagging the breaking change in the changelog.
Got a fair list though, and most of it's structural rather than nitpicks. I think the pager side needs another pass before this goes in.
Blocking, I reckon
-
Quitting the pager fails the command. If you
qout oflessbefore it's read everything, the write to the pipe returns EPIPE,Pagereturns it asrenderErr, and you getError: error: write |1: broken pipewith a non-zero exit. I checked this outside the CLI to be sure,cmd.Wait()comes back nil so it's purely the render error. Git swallows EPIPE here and I think we should too. -
The streaming pager doesn't actually stream. The comment on
ui.Pagesays the first screen appears immediately, butrenderSearchrunsFetchPagesto completion first, so with--allyou wait for every page before a single byte comes out. Two options imo: either turnFetchPagesinto a yielding version (EachPage(want, pageSize, fetch, yield)) so pages go into the pager as they arrive, or drop the claim from the comment. I'd prefer the first, it makes--allusable and keeps memory bounded, and you've gottotalfrom page 1 for the header. -
Rendering's in the wrong package.
search_view.gois 236 lines of pure presentation sitting incmd/. AGENTS.md says that lives inui/{resource}/, andui/feedback/is already doing it. I knowcmd/filters/view.goalready drifted, but I'd rather not make it worse.HighlightSpan/HighlightTextespecially, they're generic text utils andui.RenderHighlightedTextis already sat right there inui. Nothing outside the package uses any of it (search_view_test.gois in-package) so the rest can be unexported once it moves. -
A few canonical helpers got duplicated:
pluraliseoverlapsui.RenderRecordCounter, which six commands already use. If the wording genuinely needs to differ that's fine, but let's fix the shared one rather than grow a second (it's got a bug anyway,count > 1means 0 renders as "record").records == nil -> []{}is patching a gap inui.JSONRendererlocally. A nil slice encodes asnull, and study/submission/survey/collection list all have the same hole. Worth fixing it once in the renderer and deleting the branch here.--jsonis hand-rolled,shared.AddOutputFlags/shared.ResolveFormatexist. Even if csv/table don't suit a block layout,-jshould come through the shared path.dimStyleis a new lipgloss style incmd/, butuiownsDarkGreyalready, feels like it wants aui.RenderDimmed.
Other stuff worth doing in the same pass
ui/pager.gohas a few warts.exec.CommandContext(context.Background(), ...)doesn't do anything, and I saw the commit message saying it was to quietnoctx. Could we plumbcmd.Context()from cobra instead so ctrl-c actually kills the pager? Then the context earns its place. Ifcmd.Start()fails we fall back torender(w)but never closestdin, so that leaks. AndpagerEnvis parameterised for testability but nothing tests it,os.LookupEnv("LESS")would do the same in a line.LESS=FRXgets injected even when the pager isbatormore, anddefaultPager = "less -FRX"sets the same flags a second way. Two mechanisms doing one job, I'd pick one.- Cobra can delete both validation branches for us:
cmd.MarkFlagsMutuallyExclusive("all", "limit")kills the--all/--limitcheck, andArgs: cobra.MinimumNArgs(1)kills the empty-query one. Still need the blank-string check, but the errors would then come out with usage like everything else in the CLI. - The highlight path is three layers deep for one operation:
highlightField->HighlightText->highlightWith(..., identity), andHighlightTextis just an identity wrapper.spansForFieldalso rescans the whole highlights slice once per field, so six passes per result. Could we build amap[string][]HighlightSpanonce per result and hang two methods off it? That'd deletespansForField,highlightFieldandHighlightText, and the title would stop being a special path. searchPageSize = 100is an API property living in the cmd layer, probably belongs next toclient.DefaultRecordLimit.SearchOptions.Argsis assigned but never read.fmt.Errorf("error: %s", err.Error()), AGENTS.md asks forerrrather thanerr.Error().FetchPagesdoesn't trim towant, so if the API ever ignoreslimitwe'd print more rows than asked for.total = page.Totaltakes the last page's count, so a final page missingmetawould reset a known 300 down tolen(items)and quietly drop the "use --limit or --all" hint.if page.Total > totalwould cover it.- The
--allloop has no cap, so full pages plus a missing count would spin forever. Cheap guard. TestSearchFiltersNoPagerWritesDirectlydoesn't really test anything, there's no TTY in tests so both paths are already identical.
Happy to pair on the pager bits if it's easier, that one's fiddlier than it looks.
- Stream results page by page into the pager via new client.EachPage; FetchPages is now a thin collector over it - Treat the user quitting the pager (EPIPE) as a normal exit; close stdin when the pager fails to start; plumb the cobra context so interrupts terminate the pager; root now executes with a signal-aware context - Single mechanism for less flags: default pager is `less` and LESS=FRX is supplied only when the pager is less and LESS is unset - Move presentation to ui/filters; generic highlight rendering becomes ui.FieldHighlights built once per record, replacing three helper layers; add ui.RenderDimmed and ui.Pluralise - Fix ui.RenderRecordCounter pluralising zero as singular; JSONRenderer renders a nil slice as [] so callers need no local guard - Use shared output flags: --json, --table and --csv with --fields - Let cobra enforce MinimumNArgs(1) and mutually exclusive --all/--limit - FetchPages trims to the requested count, keeps the largest reported total across pages, and errors after 1000 pages rather than spinning - Move the 100-record page size to client.FilterSearchPageSize; drop the unused SearchOptions.Args; pass err rather than err.Error() - Add httptest coverage for the SearchFilters request shape and RunPager tests covering streaming, early quit, fallback and pager failure Co-Authored-By: Claude Fable 5.1 <[email protected]>
matt-kinton
left a comment
There was a problem hiding this comment.
One more pass, this time just repo conventions rather than the code itself. Sorry for the second helping haha.
The contract test one is the bit I'd actually act on
contract_test/contract_test.go has that operations table where every client method turns up either as a real call: or an explicit skip: with a reason. SearchFilters isn't in it. I went and checked the published spec:
$ curl -s https://docs.prolific.com/openapi.yaml | grep '^ /api/v1/filters'
/api/v1/filters/:
So /api/v1/filters/search/ isn't in openapi.yaml at all. Two things follow from that which I think are worth knowing:
SearchFiltersgets none of the method/path/query-param verification that every other client method gets for free, soq,limit,offsetandworkspace_idare all unchecked.TestAPICoverageonly passes right now because there's no operationId for it to miss. The day search gets published to the spec, that test goes red on whatever unrelated PR happens to run next, and someone else has to work out why.
Could we add a table entry with a skip reason so it's at least recorded? The doc comment at the top only defines OUTOFSCOPE and SPECMISMATCH, so it might want a third tag, something like NOTINSPEC.
Related: the API Coverage table in the README (Filters section, ~line 255) hasn't got the new endpoint. Same underlying problem though, it's keyed on operationId and there isn't one, so it probably wants a deliberate note rather than just being left off.
Rest of it
- AGENTS.md says list commands must support
-nfor scripting.filters list -nworks butfilters search -nerrors, which feels a bit odd inside the same resource.shared.AddOutputFlagshands you-nas a hidden--tablealias for free if we went that way. --limitdoesn't mean the same thing here as it does elsewhere. Eleven other list commands useclient.DefaultRecordLimit(200) for a single page, paired with--offset. Here it's 25 across auto-fetched pages with no offset. I actually prefer--allas a design, the bit that nags me is thatcmd/feedback/list.go:105already uses--limit 0to mean "fetch everything", and here--limit 0is a hard error. Same flag, same value, opposite meaning in one binary. Not asking you to fix the whole CLI, but maybe a line in the help text, or we pick a direction and I'll raise a ticket for the rest.--no-pagerbeing a per-command flag will get copy-pasted onto every future command that paginates. Root already owns the persistent flags (--config,--skill), feels like it belongs up there.
None of these are as urgent as the pager stuff in my other comment, but I'd like the contract table one sorted before merge if you can.
… conventions - Add a NOTINSPEC skip tag to contract_test for endpoints that are live but not yet in the published spec. Such entries keep their call, are exempt from the stale-entry check, and fail TestAPICoverage the moment the operationId appears so the skip is removed and validation begins - Record filters_SearchFilters under that tag; readme-coverage renders it with a clock marker and no method/path until the spec publishes - Promote --no-pager to a persistent root flag read via shared.NoPager - Treat --limit 0 as fetch everything, matching feedback list; --all stays as an alias Co-Authored-By: Claude Fable 5.1 <[email protected]>
…xact result counts - RunPager returns nil when the context was cancelled, so ctrl-c during paging no longer reports "pager exited with an error: signal: killed" - The search header now states only the API's match count and whether the request was truncated; a footer reports how many results were actually rendered, so the header can no longer overstate when the API returns fewer results than its own count Co-Authored-By: Claude Fable 5.1 <[email protected]>
Show the choice preview as an indented table with a dimmed "Choice ID / Label" heading, so the numeric IDs are self-describing rather than reading as counts beneath the total. The Choices line now states both the total and the number of matches. Co-Authored-By: Claude Fable 5.1 <[email protected]>
… search Track the API schema change to GET /api/v1/filters/search/ from prolific-oss/prolific#16376: - Filter and choice results carry a direct `matches` array; the `match` wrapper and its `fields` summary are removed - `num_choices` and `matched_choices` are replaced by a single `choices` group with total, matched, truncated and results, omitted for filters without enumerable choices The "Matched on" summary is now derived locally from the distinct match fields plus "choices" when any choice matched. Adds a decode test for the documented example payload. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…ass -FRX to less - Start the pager lazily on the first byte of output, so an error on the first API request (for example a missing PROLIFIC_TOKEN) is printed directly instead of appearing after quitting an empty pager - Show a dimmed "Searching filters…" status on stderr while the first page loads, cleared before output or an error; silent when stderr is not a terminal - Append -FRX on the less command line rather than via a LESS default, so short output exits immediately even when the user has set LESS themselves (for example LESS=-R) Co-Authored-By: Claude Fable 5.1 <[email protected]>


Summary
Adds
prolific filters search <query>for the new filter search API and restructuresprolific filtersinto a parent command withlistandsearchsubcommands.Warning
Breaking change:
prolific filtersandprolific filters -nno longer list filters directly. Useprolific filters list(andprolific filters list -n) instead. Noted under## nextinCHANGELOG.md.filters searchGET /api/v1/filters/search/(spec in prolific-oss/prolific#16270; endpoint is live, docs unreleased).--limitor--allwhen more exist. Results are numbered by rank, carry type and category as a dim subtitle, and are separated by a rule.--limit(default 25) is the number of results wanted and--allfetches every match. Pages of up to 100 are requested automatically through a new genericclient.FetchPageshelper.--offsetis intentionally absent.PROLIFIC_PAGER, thenPAGER, defaulting toless -FRX, so the top result stays in view and the rest is scrollable.--no-pagerprints directly. Piped or--jsonoutput never pages.--jsonemits the raw results for scripting and AI agents.Deferred
contract_testentry and README coverage row forfilters_SearchFilterscan't be added until the API docs publish, because the coverage test rejects operations missing from the live spec. The daily schema-drift check will flag it then.filters searchadoptsFetchPagesand the pager; migrating other commands is a follow-up.Test plan
make build,go vet ./...,make testpass (includingcontract_test)--limitand--allcall sequences,--no-pagerHighlightTexttable tests including code-point offsets on multi-byte text, merging and clampingFetchPagesand pager resolution unit testslessopens for long output andqreturns cleanly (no PTY available in the authoring environment)Note:
make lintfails locally on this machine with a golangci-lint / Go 1.27 export-data mismatch that also reproduces on a cleanmain; relying on CI lint here.🤖 Generated with Claude Code