Repository navigation
Conversation
…iser Core owns one FilterQuery (an AND of terms; a term is a FilterToken carrying kind, negated, values and quoted). parseFilterQuery returns it and never throws; serializeFilterQuery is its inverse, proven by a property test. An embedded quote is escaped as \" and a quoted "status:draft" stays free text. buildFilterWhere takes the structured query. The UI's queryToState and stateToQuery are removed: FilterBuilder builds on core's parser and serialiser. Implements #2248. Part of #2246. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 48e7e7e The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Coverage Report for Core Package Coverage (./packages/core)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for UI Package Coverage (./packages/ui)
File CoverageNo changed files found. |
Coverage Report for CLI Package Coverage (./packages/cli)
File CoverageNo changed files found. |
Coverage Report for Auth Package Coverage (./packages/auth)
File CoverageNo changed files found. |
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Coverage Report for RAG Package Coverage (./packages/rag)
File CoverageNo changed files found. |
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
The core design is sound. I checked parseFilterQuery and it cannot throw: every branch only advances an index or builds a string, and an unterminated quote runs to end of input. For every value the parser can currently produce (one value, no negation), serializeFilterQuery is a true inverse, including the escaping. A run of backslashes, a trailing backslash, a leading ", and an eq value starting with >, < or // all round-trip. Nothing left in the monorepo imports the removed filterBuilderState module or the queryToState/stateToQuery/FilterRow/FilterBuilderState names, including examples, docs and claude-plugins. I found no any and no casts outside one as HTMLInputElement in a test. CI is green.
Several findings are inline. These block the merge:
parse.ts:81: the claim that existing URLs keep their meaning is false. Inside quotes,\"and\\are now escapes. A URL written by the old serializer with a backslash in a quoted value now reads differently:name:"my dir\" betaused to give name=my dir\plusbeta, and now gives one unterminated value. Either change the changeset's "Every other existing?search=URL means what it meant" to say this, or keep the old reading where it is unambiguous.serialize.ts:13: the quoting test narrowed from/\s/to four ASCII whitespace characters. A row value with a leading or trailing NBSP or U+2028 is no longer quoted, andapplyQuery'strim()then strips it. This is a behaviour regression from the old builder.FilterBuilder.tsx:69: applying a hydrated query without editing it can reorder the terms. When free text comes before field terms, thesearchkey changes, so row selection resets and a history entry is pushed. That contradicts "props and behaviour are unchanged".
Fix these before #2250 makes them live:
serialize.ts:37: values joined with an unquoted,will be ambiguous oncefield:a,bmeans any-of.map.ts:58: a negated empty token narrows the results where an empty token is meant to be a no-op.serialize.ts:61: the public serializer now writes empty-valuefield:tokens and"", which it used to drop, and the changeset doesn't mention it.
Cleanups:
FilterBuilder.tsx:32re-implements emptiness and mappability rules that core owns (ADR-0083 says the UI carries no grammar), and joins with''where it should usevalues[0].parse.ts:60:query.slice(i)on every token makes parsing quadratic.types.ts:23: TSDoc saying "does not produce them yet" and "a parsed token carries exactly one" describes another file's current behaviour and will go stale without any test failing (repo comment rule).
Also, removing four public UI exports and reshaping FilterToken is a breaking change shipped as minor. That's acceptable while the package is on the 0.x alpha, but say "breaking" explicitly in the changeset.
Generated by Claude Code
Quote values containing a comma or any whitespace and free-text words starting with `-`, reading negation and comma lists in the parser so parse and serialise stay inverses. Treat a negated empty token as inert, export isEmptyFilterToken and isFieldConditionToken for the Filter builder, keep hydrated term order, and match field prefixes with a sticky regex. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES (re-review at 268b9cd; posted as a comment because GitHub does not let the PR author request changes)
Previous round. I checked all 9 earlier threads against the current code, and each fix is in place. I resolved all 9:
- Escape semantics: the changeset now documents the change.
- Values containing a comma are quoted per value, and the parser splits on commas.
- Whitespace uses
/\s/in both the parser and the serialiser. - A negated empty token is inert, because
isEmptyFilterTokenis checked first. - The changeset and TSDoc state that empty tokens are now written, and
FilterBuilderfilters them out. - Builder order is kept through slots.
rowValuereadsvalues[0], and the shared predicates are exported from core.- The prefix regex is sticky and no longer slices.
- The stale TSDoc is removed.
Parser and serialiser. I traced the quoting rules against the parser for these cases: a lone or leading -, negated "", ,a, a,, a " at the start of a non-first value, = after > or <, // after field:, a backslash before a quote or a backslash or at the end of a value, and an unterminated quote. Parse → serialise → parse was stable in every case, and the parser has no throwing path. The inverse claim holds.
Blocking: meaning changes the changeset does not cover
map.ts:89: a search made only of commas (?search=,,role:,) used to search the literal,. It now parses to all-empty values and filters nothing. That widens the result set, against #1356.FilterBuilder.tsx:48:status:draft,reviewused to hydrate as a structured row. It now goes to the free-text box, while the engine still applies it as a field condition. The PR says builder behaviour is unchanged.map.ts:92/96: negated and multi-value tokens are searched as their canonical serialisation, not as written. For example,-x,"y"now searches-x,y, anda,"b"now searchesa,b. Also,-"foo bar"used to be two words (-"fooANDbar") and is now one negated quoted word. Either fix the code, or correct the changeset's "the literal word it was written as" and list these cases.
Non-blocking
docs/content/reference/ui.md:287still says "an empty value — degrades to free text". The engine actually skips an empty token, which means it filters nothing.types.ts:35:quotedonly covers the first value. A per-value{ text, quoted }would remove the special case for the first value inserializeValues, and this matters once #2250 givesfield:a,bits meaning.map.ts:103: the secondspecForand thespec ? … : nullbranch are dead code.FilterBuilder.tsx:42:hydraterepeats thesuggestionByFieldlookup with a linearfind.- The PR description still says the parser does not yet produce negated or multi-value tokens. It does now.
Generated by Claude Code
A leading `-` and a `,` are part of a value again, so a parsed token is never negated and carries one value. The serialiser still quotes commas and leading-`-` free text, and the `\\` escape stays alongside `\"`. An empty token is one written with no characters at all. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cg5CfRyy3n2WVUNY9PWtt
| const order: QuerySlot[] = [] | ||
| for (const token of parseFilterQuery(query).terms) { | ||
| if (isEmptyFilterToken(token)) continue | ||
| if (isFieldConditionToken(token, operatorsFor)) { |
There was a problem hiding this comment.
Multi-value tokens become single-value rows and lose values on edit. isFieldConditionToken doesn't look at values.length, so hydrate turns a token like { field: 'status', values: ['draft','review'] } into a row. That row then shows only values[0] (rowValue, line 37), and changeValue replaces values with [value]. Once #2250 makes the parser emit status:draft,review as two values, the builder will show draft, and any edit to that row will silently drop review from the applied query. The row model needs to handle multi-value tokens, or hydrate should only make rows from values.length === 1 tokens and send the rest to the free-text box.
Generated by Claude Code
|
|
||
| if (token.field === null) { | ||
| freeTextWords.push(token.value) | ||
| if (token.negated) { |
There was a problem hiding this comment.
A structured negated or multi-value token searches something different from its serialised URL. For a negated token this pushes serializeFilterToken(token) as one free-text word. For { negated: true, values: ['a b'] } that is contains '-"a b"', but the URL it serialises to (-"a b") parses today as two ANDed words, -"a and b". Multi-value has the same problem: { field: 'title', values: ['a b','c'] } maps to title contains 'a b,c' (line 96), but it serialises to title:"a b",c, which re-parses as title:"a b" AND a free-text ,c. So buildFilterWhere(q) and buildFilterWhere(parse(serialize(q))) return different rows. That breaks the 'searches the literal word they serialise to' contract in the docblock.
Generated by Claude Code
| * Whether a {@link FilterToken} was written with no characters at all (`role:`, | ||
| * `""`). Such a token is incomplete input and filters nothing (#1509). | ||
| */ | ||
| export function isEmptyFilterToken(token: FilterToken): boolean { |
There was a problem hiding this comment.
values: [] is not treated as empty. isEmptyFilterToken only matches values.length === 1 && values[0] === ''. A structured token with values: [] (which the public FilterToken type allows) falls through to values.join(',') === '', and that empty string is pushed as a free-text word. On a list with no free-text fields, or where every free-text mapping rejects '', it narrows to NEVER_MATCHES, so an empty token returns zero rows instead of being a no-op. serializeFilterQuery also writes it as an empty term ({terms:[{values:[]},{values:['x']}]} becomes ' x'), which round-trips to one term. Consider values.every(v => v === ''), or normalising [].
Generated by Claude Code
| return { rows, freeText: serializeFilterQuery({ terms: freeTerms }), order } | ||
| } | ||
|
|
||
| function toQuery(state: BuilderState): string { |
There was a problem hiding this comment.
Simplification: BuilderState keeps rows plus a parallel order: QuerySlot[]. toQuery then rebuilds the ordering with a rowsById map, freeTerms.shift(), leftover appends, and stale slots from removed rows. One ordered list of terms, each either a row token or a free-text marker, would hold the same information with no reconciliation pass and no stale order entries after removeRow or addRow.
Generated by Claude Code
| } | ||
|
|
||
| function mustQuoteValue(value: string): boolean { | ||
| return value.startsWith('"') || value.includes(',') || [...value].some(isFilterWhitespace) |
There was a problem hiding this comment.
Nit: [...value].some(isFilterWhitespace) builds an array of code points and runs the regex once per character. /\s/.test(value) (the same WHITESPACE_RE, non-global) gives the same answer in one pass with no allocation. The parser's hot loop also calls isFilterWhitespace (WHITESPACE_RE.test(ch)) once per character.
Generated by Claude Code
| freeTextWords.push(token.value) | ||
| const value = token.values.join(',') | ||
| const spec = token.field === null ? undefined : specFor(token.field) | ||
| if (!spec || !isFieldConditionToken(token, () => spec.operators)) { |
There was a problem hiding this comment.
Simplification: specFor(token.field) runs, then isFieldConditionToken(token, () => spec.operators) repeats the field === null, negated and empty checks already handled above it and ignores its field argument. isFieldConditionToken(token, (f) => specFor(f)?.operators) followed by specs[token.field] would make the shared predicate the single gate. Separately, COMPARISON_PREFIXES (parse.ts) and OPERATOR_PREFIX (serialize.ts) encode the same operator-to-prefix mapping twice, so one could be derived from the other to keep them from drifting.
Generated by Claude Code
|
|
||
| Core owns one structured filter query, with a parser and serialiser that are true inverses (ADR-0083) | ||
|
|
||
| Breaking: `FilterToken` drops `value` and `raw` for `kind`, `negated`, `values` and `quoted`, and `parseFilterQuery`, `serializeFilterQuery` and `buildFilterWhere` now take or return a `FilterQuery`. `@opensaas/stack-ui` removes `queryToState`, `stateToQuery`, `FilterRow` and `FilterBuilderState`. |
There was a problem hiding this comment.
Conventions: root CLAUDE.md says "major: Breaking changes (only when user explicitly requests, include migration guide)". This changeset says "Breaking:" (it removes FilterToken.value/raw, the queryToState/stateToQuery exports, and changes the parseFilterQuery/buildFilterWhere signatures) but is versioned minor with no migration snippet for the removed UI helpers. If minor is intentional for the alpha prerelease, a short before/after for queryToState → parseFilterQuery + isFieldConditionToken would still help consumers.
Generated by Claude Code
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES (one small fix to a repo rule)
GitHub does not let this account request changes on its own PR, so I am posting this as a comment. The verdict is still REQUEST CHANGES.
Reviewed at 48e7e7e. I took the owner's two decisions as settled: the parser stays on today's grammar, and \\ stays documented in ADR-0083. Per-value quoting is left to #2250.
Earlier threads
All 16 threads from the first two rounds are resolved, and I checked each one against the head commit. The fixes are in place. ?search=, and role:, search for the literal , again. status:draft,review hydrates as a single row. The UI docs now say an empty value filters nothing. The second specFor lookup is gone, and hydrate uses suggestionByField. I found no regression against main in the parser, the serialiser or buildFilterWhere for any query the parser produces. Fifteen edge-case inputs survive a round trip, changing only the quoted flag where the serialiser added quotes.
Blocking
- The test adds type casts. Root CLAUDE.md says "do not use type casting". The PR adds three new
as HTMLInputElementcasts inpackages/ui/tests/components/FilterBuilder.test.tsx, at lines 140, 152 and 153. jest-dom is already loaded throughtests/setup.ts, andBigIntField.test.tsxin the same folder uses it. So the fix is a direct replacement:The cast at line 130 was already onexpect(screen.getByLabelText('Filter value')).toHaveValue('draft,review') expect(screen.getByLabelText('Search')).toHaveValue('')
main, so it would be good to change it in the same edit.
Non-blocking
These are the 7 inline comments from this round. None of them changes behaviour against main.
map.ts:27: avalues: []token is not counted as empty. The parser never produces one.map.ts:91: a structured negated or multi-value token searches different text than its serialised URL reads back as. The parser never produces these tokens either.FilterBuilder.tsx:51: a multi-value token becomes a single-value row. This only matters once #2250 lands, so it should be handled there.FilterBuilder.tsx:63:rowsandorderhold the same information in parallel. Simplification only.map.ts:98: the gate check is repeated, and two operator-prefix tables duplicate each other. Simplification only.serialize.ts:13: the whitespace check runs once per character. A nit..changeset/quiet-filters-invert.md:8: the changeset says "Breaking" but the bump isminor. Under the versioning rule, minor is correct unless a major is explicitly requested, so this is only a request for a short before/after example forqueryToState.
I am leaving those 7 threads open for the author.
The comma re-apply case: title:draft,review becomes title:"draft,review"
This does not matter. The meaning is identical: the parser reads both strings as the single value draft,review, and buildFilterWhere builds the same Where from each. The history entry is not new. applyQuery calls router.push on every Apply, so main already adds a history entry for an unchanged query and resets to page 1. The only new effect is that row selection resets, because the ?search= key changes.
That is also not new in kind. main's serialiser already rewrites queries on Apply. For example, it drops unneeded quotes, so title:"draft" becomes title:draft, which changes the key in the same way. This PR's quoting extends that rewriting to commas, which is what keeps these URLs meaning the same once #2250 makes , mean any-of. The effect only shows up when a user selects rows and then clicks Apply without changing anything, on a value that contains a comma. After that one Apply the URL is canonical and stays put. I don't think it needs action.
Once the three casts are replaced, I would approve this.
Generated by Claude Code
Implements #2248
Part of #2246
Summary
This is a prefactor and does not change what any query means. Core now owns one structured filter query with a parser and a serialiser. The parser stays on today's grammar, and the current Filter builder form sits on top of the new shape, so an admin sees no change.
Owner's decisions
-and a,are ordinary characters of a value. Negation and comma lists come in Filter conditions: is not, any of, is empty #2250, together with what the engine does with them. The serialiser still quotes a value containing,and a free-text word starting with-. Quoting means nothing different today, and it keeps canonical URLs the same once Filter conditions: is not, any of, is empty #2250 lands.\\escape stays alongside\"inside quotes. ADR-0083's grammar table now has a row for it, and the UI reference lists both escapes.The query tree (later tickets build on this)
parseFilterQuery(string): FilterQuerynever throws. Every token it returns hasnegated: falseand one value.-x,a,b,status:draft,reviewand,parse the same way they do onmain.serializeFilterQuery(FilterQuery): string. For everyqthe parser returns,parse(serialize(q))isq, except thatquotedis set on any value the serialiser had to quote (a,, or free text starting with-).serialize ∘ parseis the identity on serialised strings.buildFilterWhere(FilterQuery, specs)takes the structured query and searches each parsed token exactly as written. It also accepts negated or multi-value tokens, which the parser never produces. It searches the literal word they serialise to, or the comma-joined values, and never drops or negates them.isEmptyFilterTokenis true only for a token written with no characters (values.length === 1 && values[0] === ''). So?search=,androle:,search for the literal,, as they do on main.FilterTokenlosesvalueandraw.FilterTermandFilterQueryare new exports, along withisEmptyFilterTokenandisFieldConditionToken.Defects fixed
\"is a literal quote and\\a literal backslash. Any other backslash stays as it is, so a value like"C:\Users me"reads the same as before."status:draft"stays a free-text word. The serialiser also quotes any value that would otherwise read back as different syntax: a field-shaped word, a leading", aneqvalue starting with>,<or//, and agt/ltvalue starting with=./\s/) separates tokens and is quoted, so a trailing NBSP survivestrim().UI
queryToState,stateToQuery,FilterRowandFilterBuilderStateare removed, along withlib/filterBuilderState.ts.FilterBuilderkeeps its own row state overFilterTokens and calls core'sparseFilterQueryandserializeFilterQuerydirectly. Its props and slots are unchanged.status:draft,reviewhydrates as a structured row, the same as on main.Tests
Property tests (
serialize.test.ts) use a seeded PRNG with 2,000 runs each and add no new dependency. The generators produce only parser-shaped tokens (single value, not negated). They check:parse ∘ serialize ∘ parseequalsparse, withquotedraised exactly where,or a leading-forces quotesserialize ∘ parseis the identity on serialised queriesI removed the
,quoting rule to check the tests can fail. Six tests failed, including both properties.Parser table tests check that
-x,a,b,,androle:,read as literal values.buildFilterWheretests check that?search=,androle:,search for,, and that structured negated and multi-value tokens are neither dropped nor negated.engine.test.tsruns?search=through a real Test context and checks the rows that come back, including the escaped-quote cases.FilterBuildertests check thattitle:draft,reviewhydrates as one row, that unknown fields, unsupported operators and quoted field-shaped words go to the free-text box, that an unchanged query is applied as written, and that empty rows are dropped.Verification
pnpm buildpasses.tsc --noEmitpasses.vitestruns 138 files: 2,499 tests pass and 29 are skipped (the escape-only suites).pnpm typecheckpasses.vitestruns 80 files and all 1,119 tests pass.pnpm lintreports 0 errors and 1 warning that was already there before this PR.pnpm formatandpnpm manypkg fixare clean.Generated by Claude Code