Repository navigation
feat(core): PowerSearch touch sheet flow behind experimental.presentation - #8001
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
b154a0f to
c07a91a
Compare
07d619c to
0efc86f
Compare
c07a91a to
36f8999
Compare
0efc86f to
7e07d10
Compare
36f8999 to
a017ec1
Compare
7e07d10 to
8b9c8d8
Compare
a017ec1 to
13206a2
Compare
8b9c8d8 to
11fb87a
Compare
There was a problem hiding this comment.
This adds an opt-in touch flow for PowerSearch, but it cannot be approved yet: the head writes DEC-3 and FR10–FR21 into the current component:PowerSearch record, and an engineering owner still has to decide those requirements, including FR17/FR21's reliance on the Tokenizer sheet requirements from #7977 that are not on main yet. The approval on #8000 covers that pull request's head, not this one; an engineering owner records the decision here by commenting /approve-spec 11fb87a50dafe3d00fb7f54d4151c577eaa3ace9 on this pull request, and the author may do so as an engineering owner. Whatever that decision is, these fixes are needed:
- Put
data-testidon the sheet trigger (today it moves from the combobox input to adiv), and drop thefamily:input-fieldsmembership and FR10 citation, since that family's record excludes PowerSearch. - Show date-only values without a time of day in the Filters rows (the forced story shows
Mar 9, 2024, 7:00 PM). - Align FR16 (focus after a value suggestion commits) and FR20 (the relative-date row labels) with the code, and declare the stable result-count fix in its own
[fix]Changeset. - Split the 4,800-line change, for example by dropping the carried spec commit once #8000 lands and moving the stable fix out, or explain why it cannot be split.
[Automated review]
| onClick={handleTriggerWrapperClick} | ||
| onFocus={handleTriggerFocusWithin} | ||
| onBlur={handleTriggerBlurWithin} | ||
| data-testid={testId} |
There was a problem hiding this comment.
In the popover presentation data-testid reaches the combobox input (through Tokenizer); here it lands on the control-box div, so a test id changes element when an adaptive field switches. Putting it on the trigger <button> keeps it on the field's primary control in both presentations.
| config, | ||
| operator.value, | ||
| filter.value, | ||
| Number.MAX_SAFE_INTEGER, |
There was a problem hiding this comment.
formatFilterValue formats every date_absolute with hour and minute, so a date-only filter (isDateOnly: true) reads Mar 9, 2024, 7:00 PM in this row while its token says Mar 9, 2024. Could the row format date-only values without the time?
| value: filterValue, | ||
| }; | ||
| pendingSheetFocusRef.current = { | ||
| type: 'manager-filter', |
There was a problem hiding this comment.
With a controlled parent this moves focus to the new filter's row after a value suggestion commits, while FR16 says a creation commit focuses Add filter. Either the code or FR16 should change so they agree.
| ] | ||
| modules: [] | ||
| families: [family:overlay-dismissal] | ||
| families: [family:input-fields, family:overlay-dismissal] |
There was a problem hiding this comment.
family:input-fields lists PowerSearch as excluded ("higher-level compositions that consume member behavior"), and its FR10 sends data-* to the primary semantic control, which this record's prop-target paragraph does not follow. Drop the membership and the FR10 citation, or amend that family first.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [experimental] PowerSearch gains a touch surface behind `experimental={{presentation: 'bottom-sheet' | 'adaptive'}}`. `bottom-sheet` makes the whole field one trigger showing display-only filter tokens, and moves filter editing into a flow of tall modal sheets: a Filters sheet listing the active filters as rows with Add filter, Done, and Clear all; an Add filter sheet with the fields grouped (searchable from eight fields), or the content search with the field typeahead's suggestions listed inline; and a value step that stages field → operator → value and confirms from its footer, with Remove while editing. Closed vocabularies are full-width rows, numbers stage as typed, and `string_list` / `entity_list` values edit through the Tokenizer's own sheet presentation titled with the field label. `adaptive` uses the sheet on compact coarse-pointer screens and the popover otherwise, resolved the same way Selector resolves it. The sheet flow loads on demand, so a PowerSearch that does not opt in does not carry it. New `@astryx.powersearch.sheet.*` catalog keys for the sheets' strings; no new theming target. The result-count announcement is now StrictMode-safe in both presentations (no announcement on mount). Anything inside PowerSearch's `experimental` prop may change or be removed in a patch release; omitting it keeps the stable behavior. |
There was a problem hiding this comment.
The StrictMode-safe result count changes the stable presentation too; spec:AST-017 FR22 keeps that out of an [experimental] entry, so it needs its own [fix] Changeset.
13206a2 to
6f02169
Compare
11fb87a to
bf9ee1b
Compare
6f02169 to
1dc081e
Compare
bf9ee1b to
3cbfa4c
Compare
3cbfa4c to
c3bbf19
Compare
1dc081e to
c1d1852
Compare
c3bbf19 to
fc16c1d
Compare
There was a problem hiding this comment.
This adds an opt-in touch flow for PowerSearch, but it cannot be approved yet: the head writes DEC-3, FR10–FR21, AR4–AR6, PR5 and AV5–AV6 into the current component:PowerSearch record, and an engineering owner has to decide those requirements, including FR17/FR21's reliance on the Tokenizer sheet requirements from #7977 that are not on main yet. An engineering owner records that decision by commenting /approve-spec followed by this head's full commit SHA on this pull request, and the author may do so as an engineering owner. Whatever that decision is, these still need changes:
- Make a date-only filter show the same day on the trigger and in the Filters row; today the token formats in the device's time zone and the row in
timezoneID. - Announce the sheets' empty-result messages as AR6 says, or drop that clause from AR6.
- Split the 5,181-line change, for example by dropping the carried record change once #8000 lands and moving the stable result-count fix out, or explain why it cannot be split.
[Automated review]
| operator.value.type === 'date_absolute' && | ||
| filter.value.type === 'date_absolute' && | ||
| operator.value.isDateOnly !== false | ||
| ? formatDateAbsoluteCompact( |
There was a problem hiding this comment.
The row formats a date-only value in timezoneID, but the trigger's token (PowerSearch.tsx:430) formats the same instant with no time zone, so it follows the device. With timezoneID America/Los_Angeles on a Tokyo device, the forced story's token says Mar 10, 2024 while this row says Mar 9, 2024. Formatting both the same way (here, or in the token as its own [fix]) keeps one filter on one day, and the boundary test could assert the token too.
| "Search filters"; every sheet action is at least 44px tall. | ||
| - **AR6 — Announcements and focus.** While a sheet is open, every announcement | ||
| the flow makes — `Added {label}` for a committed suggestion, the result count | ||
| of FR19, and the sheets' empty-result messages — is delivered by a persistent |
There was a problem hiding this comment.
Nothing announces the empty-result messages: "No filters match your search" and "No results found" render as plain Text with no live region (PowerSearchBottomSheet.tsx:1320–1334). Either deliver them through the flow's polite region or drop this clause from AR6.
fc16c1d to
5e77abd
Compare
c1d1852 to
86731bf
Compare
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| PowerSearch · Storybook | 🔴 191.7 kB (+6.1 kB) · 21.6 kB CSS (+965 B) | 6,789 (+2,811) | — · — · — |
| lab/MobilePowerSearch | 216.5 kB (+1.3 kB) · 1.2 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditor | 278.3 kB (+403 B) · 2.6 kB CSS (0) | — | — · — · — |
33 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/EmojiPicker | 70.8 kB (+399 B) · 722 B CSS (0) | — | — · — · — |
| lab/ListInput | 81.7 kB (+393 B) · 1.6 kB CSS (0) | — | — · — · — |
| lab/Comments | 233.4 kB (+391 B) · 2.5 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 223.2 kB (+391 B) · 454 B CSS (0) | — | — · — · — |
| lab/CommentComposer | 103.2 kB (+386 B) · 1.9 kB CSS (0) | — | — · — · — |
| lab/TransferList | 74.7 kB (+386 B) · 1.7 kB CSS (0) | — | — · — · — |
| lab/MobileTokenizer | 102.0 kB (+385 B) · 880 B CSS (0) | — | — · — · — |
| lab/TransferListSelector | 81.1 kB (+378 B) · 1.9 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 228.1 kB (+372 B) · 2.4 kB CSS (0) | — | — · — · — |
| DateTimeInput | 159.9 kB (+118 B) · 17.1 kB CSS (0) | — | — · — · — |
| DateInput | 103.8 kB (+112 B) · 15.3 kB CSS (0) | — | — · — · — |
| Selector | 77.7 kB (+41 B) · 15.7 kB CSS (0) | — | — · — · — |
| CheckboxList | 43.5 kB (+39 B) · 8.6 kB CSS (0) | — | — · — · — |
| NumberInput | 55.2 kB (+39 B) · 11.2 kB CSS (0) | — | — · — · — |
| ComplexSelector | 55.8 kB (+33 B) · 11.1 kB CSS (0) | — | — · — · — |
| MultiSelector | 73.3 kB (+31 B) · 15.2 kB CSS (0) | — | — · — · — |
| InputGroup | 42.1 kB (+28 B) · 8.4 kB CSS (0) | — | — · — · — |
| DateRangeInput | 70.1 kB (+15 B) · 11.9 kB CSS (0) | — | — · — · — |
| TimeInput | 90.5 kB (+13 B) · 14.3 kB CSS (0) | — | — · — · — |
| FileInput | 53.9 kB (+12 B) · 10.7 kB CSS (0) | — | — · — · — |
| Pagination | 86.6 kB (+1 B) · 16.0 kB CSS (0) | — | — · — · — |
| FieldLabel | 36.1 kB (0) · 5.9 kB CSS (0) | — | — · — · — |
| Tokenizer | 70.8 kB (0) · 17.1 kB CSS (0) | — | — · — · — |
| Typeahead | 63.1 kB (0) · 11.8 kB CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 65.5 kB (−1 B) · 1.1 kB CSS (0) | — | — · — · — |
| Markdown | 145.4 kB (−2 B) · 18.3 kB CSS (0) | — | — · — · — |
| lab/ChatEmojiPicker | 65.0 kB (−6 B) · 773 B CSS (0) | — | — · — · — |
| RadioListItem | 51.4 kB (−10 B) · 10.1 kB CSS (0) | — | — · — · — |
| TextInput · Storybook | 53.0 kB (−11 B) · 11.0 kB CSS (0) | 420 (+1) | — · — · — |
| Slider | 89.1 kB (−19 B) · 9.4 kB CSS (0) | — | — · — · — |
| TextArea | 47.2 kB (−22 B) · 9.9 kB CSS (0) | — | — · — · — |
| Field | 39.9 kB (−1.6 kB) · 7.4 kB CSS (−955 B) | — | — · — · — |
| RadioList | 41.4 kB (−1.6 kB) · 7.4 kB CSS (−959 B) | — | — · — · — |
Package size: @astryxdesign/core 6.18 MB packed (+70.2 kB); no new dependencies.
API changes: core +1 · ~0 · −0
Committed API report behind this build: core 50 rows; run pnpm api:report.
1 API change (advisory)
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | core |
. · Components · component PowerSearch |
prop experimental?: { presentation?: 'popover' | 'bottom-sheet' | 'adaptive' } @experimental |
<PowerSearch experimental="popover" /> |
Wide change (shared styles: packages/core/src/theme/derivedVarRegistry.test.ts): add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
There was a problem hiding this comment.
The touch sheet flow follows the current component:PowerSearch DEC-3 contract, but two things need changes before it can be approved. At 8,512 authored lines it needs splitting: land the formatTokenValue.ts extraction on its own, then put the sheet flow up as a stack that lands together (for example the trigger, latch and Filters sheet; Add filter with the content search; the value step), each with its tests. Separately, the Filters rows are keyed by position (PowerSearchBottomSheet.tsx:1519), so when the parent reorders the filters or removes one above the focused row, focus moves to a different filter; keying rows by filter identity, with tests for both cases, fixes it.
[Automated review]
| filter, | ||
| index, | ||
| isEditable, | ||
| key: `${index}-${filter.field}-${filter.operator}`, |
There was a problem hiding this comment.
Keying rows by ${index}-${filter.field}-${filter.operator} remounts the focused row when the parent reorders the filters or removes one above it, and the recovery effect then focuses the row at the old index: with focus on "Author is Ada" in [Status, Author, Priority], dropping Status lands focus on "Priority is P1". Keying each row by filter identity (a per-object id from a WeakMap, falling back to filterSignature for clones), with tests for a reorder and a removal above the focused row, keeps focus on the filter the person was on.
There was a problem hiding this comment.
Recommending approval: the touch sheet flow matches the current component:PowerSearch contract (DEC-3, FR10–FR21), the stable popover presentation is unchanged, and the focused tests, typecheck and a phone-sized Chromium run all hold up. The size is accepted here because the record defines the sheet presentation as one complete flow, so it cannot land in parts without shipping an option that breaks its own requirements. The two earlier blockers are resolved: the StrictMode result-count fix is already on main, and Filters rows are now keyed by filter identity, with tests. What remains is the repository's spec-owner approval for the small record update.
[Automated review]
|
/approve-spec f8c4a15 |
There was a problem hiding this comment.
Changes requested. The touch flow follows the current component:PowerSearch contract (DEC-3, FR10–FR21), and the PowerSearch and Tokenizer tests, the Core typecheck and lint all pass on this head. Three things need fixing first:
- Move the presentation-swap focus hand-off in
PowerSearch.tsx(the per-commituseLayoutEffectat line 651 and the swap Effects at lines 678 and 793) into callback refs, asTokenizer.tsxnow does withhandOffFocusRefandwrapperCallbackRef(architecture:react-component-runtimeINV2, INV6). - Replace
formatSharedListinformatFilterValue.tswith Core's shareduseListFormatter, which already owns the same narrow-conjunctionIntl.ListFormatpolicy, and delete the copy. - Importing
PowerSearchnow loads about 3.1 KB more gzip JavaScript for the touch presentation, over the 2 KB touch-only budget inarchitecture:component-style-authoringINV6, so either move that code into the lazy chunk or the shared touch modules, or justify the cost in the description.
The size is fine as one implementation of DEC-3, and the record edits are owner-approved on this head.
[Automated review]
| // commit of the closed field, so the swap below knows the input was focused | ||
| // when `adaptive` resolves to the sheet on a touch screen right after | ||
| // hydration. | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
This layout effect reads document.activeElement after every commit of every PowerSearch, and the Effect at line 678 then moves focus when the presentation flips. On main, Tokenizer runs the same hand-off from callback refs (handOffFocusRef and wrapperCallbackRef in Tokenizer.tsx). The detaching control notes focus while it is still in the document, which covers the pre-hydration autofocus case, and the attaching control takes it. Giving PowerSearch's root wrapper, trigger and Tokenizer handle that pattern removes both Effects (architecture:react-component-runtime INV2, INV6).
| * type runs Chinese values together and gives Japanese and Korean a bare | ||
| * space). | ||
| */ | ||
| export function formatSharedList( |
There was a problem hiding this comment.
packages/core/src/i18n/useListFormatter.ts already builds this exact narrow-conjunction Intl.ListFormat and keeps raw list formatting in one place (architecture:internationalization INV8). Passing useListFormatter()'s function from the sheet into formatTokenValueText lets this copy go.
| * Whether the tree root offers the group-operator picker when the field has | ||
| * several operators. The popover shows it; the sheet presentation's value | ||
| * step owns the operator choice in its own Operator group and hides it | ||
| *. @default true |
There was a problem hiding this comment.
Non-blocking: a stray *. is left here from a removed citation. @default true reads best on its own JSDoc line (the same leftover appears at powerSearchSheetUtils.ts:167 and :194).
|
/approve-spec 796dd9d |
There was a problem hiding this comment.
Changes requested. The record edits for the stacked touch flow are owner-approved on this head, so what remains is implementation and evidence work:
- Keep Core renderable from a single esbuild bundle: the lazily imported sheet graph now reaches modules that import Core barrels, so
scripts/core-single-bundle.test.mjsfails and an app that bundles Core without code splitting throws before renderingListandItem; import those modules directly instead. - Make the other two failing tests pass: document or restructure the new
--_power-search-sheet-hit-*custom properties that failderivedVarRegistry.test.ts, and rebase ontomainso the Clear all confirmation test drives a full backdrop press, since a bare click no longer dismisses an info Dialog there. - FR15a relies on
component:BottomSheetFR10's same-update replace when a direct-entry creation commits from the value step, but two sheets close on that path, which FR10 excludes, so amend FR15a (or close Add filter in an earlier update) and assert focus after the sheets settle. - Move the presentation-swap focus hand-off in
PowerSearch.tsx(the per-commituseLayoutEffectand the swap Effects) into callback refs, asTokenizer.tsxdoes withhandOffFocusRefandwrapperCallbackRef. - Announce and place focus after Clear all's confirmation in its action handler, as
TokenizerBottomSheet.tsxdoes incloseClearConfirmation, instead of an Effect that waits for the dialog to close. - Replace
formatSharedListinformatFilterValue.tswith Core's shareduseListFormatterand delete the copy. - Importing
PowerSearchnow loads about 3.1 KB more gzip JavaScript that only the sheet presentation runs, over the 2 KB touch-only budget, so move it into the lazy chunk or the shared touch modules, or justify the cost in the description. - Rewrite the description: it still says the flow has no new behavior, that DEC-3 is unchanged and that every existing test passes, while this head stacks the steps, adds a confirmed Clear all, reverses two rejected alternatives and has three failing test cases.
A few comments in PowerSearchBottomSheet.tsx and the prop's JSDoc still describe the replaced flow; worth refreshing while you are there.
[Automated review]
| // carry the media condition because StyleX allows plain values inside a | ||
| // pseudo-element. On a fine pointer the overlay is not generated, so it | ||
| // never takes hover from the action's own content. | ||
| touchHitArea: { |
There was a problem hiding this comment.
--_power-search-sheet-hit-inset and --_power-search-sheet-hit-content fail derivedVarRegistry.test.ts ("PowerSearch: all source vars are in doc file"): document them in PowerSearch.doc.mjs or reach the coarse-pointer hit area without new custom properties.
| | FR13 | Add filter — from the Filters sheet's Add filter action, or directly when an interactive field with no filter opens — is a tall modal sheet named "Add filter" opened above the Filters sheet, which stays open beneath it (FR15a), so the sheet has no Back action: dismissing it — Escape, a scrim tap, a swipe — reveals Filters again, or closes the flow when Add filter was the entry point, in which case its fixed footer holds Done. Its header — the heading and, when one shows, the search field — stays fixed while the rows scroll. Without a content-search field it lists the visible fields grouped by `group`, each group a named list: the ungrouped fields first, under the heading "General", then each group under its name in order of first appearance, each a row button with the field's icon, a trailing chevron — or, for a field whose only operator takes an `empty` value, a trailing plus, since choosing it adds the filter at once (FR15) — and, once the field count reaches eight, a "Search filters" field above the list that matches labels and `typeaheadAliases` case-insensitively, caps a non-empty query's matches at `maxSearchResults`, and shows the empty state "No filters match your search" when nothing matches. With a `contentSearchFieldKey` whose default operator takes a `string` value it shows instead a "Search" searchbox using `placeholder`, with the same suggestions the popover presentation's typeahead offers listed inline as row buttons ("Search results"): fields, field-and-operator pairs, value matches, and a leading free-text item for the content field; Enter commits the first suggestion, ArrowDown and ArrowUp move between the search and the rows — arrow movement across independently focusable controls, recorded here as this record's exception to `spec:AST-020/FR15`, which adopts arrow keys for a single composite only: the searchbox-to-list continuity is what the popover presentation's combobox gives the same suggestions, and the rows stay in the Tab order as well — an IME-composing Enter does nothing, and a query with no suggestion shows the empty state "No results found" — the searchbox's `aria-controls` names the results list, or that message while nothing lists, so it always names an element; the field list's "No filters match your search" likewise shows only for a non-empty query that matches nothing, never for a config with no field. Each of the two is a compact empty state with a search icon, that title, and the description "Try a different search.", mounted only while the Add filter sheet is the open one. Neither search field takes focus when the sheet opens: the sheet opens with focus on its panel and the software keyboard stays down until the person taps the search; a direct vertical touch drag on the rows while a search field has focus blurs it, so the keyboard leaves and the rows can be read. A suggestion that carries a value commits `onChange([...filters, filter], 'add', filters.length)`, announces `Added {label}` inside the Filters sheet (AR6), and shows Filters as FR15a states; any other suggestion or field row opens the value step with that field and operator, above Add filter. Every visible field lists, a field whose every operator is `nested` included: the value step edits every operator's value (FR14). | DEC-3; Lab touch-flow evidence; current typeahead source | Current contract; verified by focused sheet tests and real-engine touch evidence. | | ||
| | FR14 | The value step is a tall modal sheet that edits every operator's value, opened above the sheet that led to it — Filters for an edit, Add filter for a creation — which stays open beneath it (FR15a); its title is the dialog's name (AR5), and for a filter whose field or operator the config no longer has it reads as the Filters row does (FR12): the field's label when the config still has the field, the raw key for whatever is gone. Its header and footer stay fixed while the body scrolls. When the field offers more than one operator the body starts with an "Operator" radio group whose options read "{field label} {operator label}" and list the field's operators, with the operator the step opened with checked — the filter's operator while editing, and while creating the operator the suggestion named (FR13) or else the field's default operator, else its first; otherwise there is no group and the title is "{field label} {operator label}" naming that operator. When no operator label applies the title is the field label. The body below the group is the editor for the checked operator's value type: the sheet-native controls of FR17, or for a `nested` value the popover presentation's built-in nested editor — the sub-filter tree with its Add action, each sub-filter's field, operator, and value controls, and removal — rendered as the popover renders it, with one difference: the step's Operator group takes the place of the editor's root group-operator picker, which is not shown, and the tree root reads the checked operator's label; a sub-filter's controls are the popover editor's own, in their default presentation (a Selector's menu, the Tokenizer's inline field), not the sheet's choice rows (FR17) or Tokenizer sheet (FR21), which govern the step's top-level value only, because the sheet passes no `experimental` option down to them; the tree is its own width container, so below 400 CSS px its rows wrap instead of overflowing the sheet, as they wrap in a narrow popover. A group is created and edited the same way in both presentations; switching the operator swaps the body. A value chosen in the body is staged, not committed: no `onChange` fires until the footer confirms it. Changing the operator keeps a staged value only when the new operator reads the same value type; an `empty` operator stages `{type: 'empty'}`. The body's sections sit on the sheet's surface when the editor is the body's only section; when the body has more than one section — the Operator group above the editor — each section is a muted container, so the grouping reads without a frame around a lone editor. The footer holds Cancel and the confirm action — `popoverSaveButtonLabel` when given, otherwise "Add filter" while creating and "Apply" while editing — disabled until the staged value is complete; the step has no Remove action: a filter is removed from its Filters row (FR12). When `components[type].Editor` is set for the checked operator's value type it replaces the body below the Operator group and the footer — the group stays outside it, so switching operators still swaps the body — receiving the props the popover presentation hands it, with the same meaning: the caller's `config`, the partial `filter`, `mode`, `saveButtonLabel`, `isReadOnly`, `onSave`, and `onCancel`, and nothing the popover does not pass; `onSave(filter)` commits it, `onSave(null)` removes the filter while editing and returns to Filters unchanged while creating. A controlled update that removes the field or operator being edited replaces the body with "This filter is no longer available", announced from its rendered element inside the sheet while the step is the open one (AR6), and keeps Cancel as the way out, the filter's trailing remove action on its Filters row being the removal path (FR12); when focus was on a control of the replaced body it moves to Cancel, so it never falls to the page body inside the open sheet (FR16). | DEC-3; Lab touch-flow evidence | Current contract; verified by focused sheet tests and real-engine touch evidence. | | ||
| | FR15 | Confirming a created filter commits `onChange([...filters, filter], 'add', filters.length)`; choosing a field whose only operator takes an `empty` value commits that filter the same way from the Add filter row, with no value step. Tapping an editable Filters row opens the value step on that filter in edit mode; Apply commits `onChange(updated, 'edit', index)`; the row's trailing remove action, or a tap on an `empty`-valued filter's row, commits `onChange(rest, 'remove', index)`, where `index` is the filter's position at commit time: a controlled parent that reorders or clones its filters while the step is open is followed by object identity, then by a structurally equal filter in the original slot, then by a unique structural match; when the target is gone or ambiguous nothing is committed and the step closes back to Filters, focus landing as FR16 states for a row that no longer exists. Every commit shows the Filters sheet as FR15a states. While the field is read-only, or disabled, no commit happens. | DEC-3; Lab touch-flow evidence | Current contract; verified by focused sheet tests and real-engine touch evidence. | | ||
| | FR15a | The steps stack: each step opens above the sheet that led to it, which stays open — receded and blocked as a covered sheet of the stack (`component:BottomSheet` FR5, FR6) — so the stack is Filters alone; Filters under Add filter; Filters under the value step for an edit; Filters under Add filter under the value step for a creation; or, when Add filter was the entry point, Add filter alone and then Add filter under the value step. No sheet of the flow carries a Back action: a dismissal — Escape, a scrim tap, a swipe, a platform close request — reaches only the top sheet (`component:BottomSheet` FR7), closes it, and reveals the sheet beneath, which takes focus back as FR16 states; dismissing the bottom sheet of the stack closes the flow. Cancel on the value step is that dismissal. A commit closes the step that made it: an edit's commit reveals Filters beneath the value step; a creation's commit, from the value step or from an Add filter row or suggestion that commits directly, closes Add filter, and the value step above it, together and shows Filters — revealed when it is open beneath them, else opened in the same update that closes them, BottomSheet's same-update replace (`component:BottomSheet` FR10), so the person sees the filter just added. The flow therefore has one direction forward (open a step above) and one back (close the top step): no dismissal replaces a step, and a creation's commit from a direct entry is the one place a sheet opens as others close. | DEC-3; `component:BottomSheet` FR5–FR9 | Current contract; verified by focused sheet tests and real-engine touch evidence. | |
There was a problem hiding this comment.
For a creation committed from the value step after a direct-entry Add filter, two sheets close (the value step and the covered Add filter) while Filters opens. component:BottomSheet FR10 replaces only when exactly one sheet closes and one opens, and keeps the ordinary push and pop motion and focus return otherwise, so this sentence promises a replace that does not apply on that path.
| // commit of the closed field, so the swap below knows the input was focused | ||
| // when `adaptive` resolves to the sheet on a touch screen right after | ||
| // hydration. | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
This layout effect reads document.activeElement after every commit of every PowerSearch, and the swap Effects below move focus when the presentation flips. Tokenizer.tsx does the same hand-off from callback refs (handOffFocusRef, wrapperCallbackRef); the same shape here removes the per-commit read (architecture:react-component-runtime INV2, INV6).
| // Focus then moves to the sheet's first remaining control: a kept | ||
| // read-only row has none, so Add filter. The Effect synchronizes the live | ||
| // region and the document's focus, and acquires nothing. | ||
| useEffect(() => { |
There was a problem hiding this comment.
Clear all's announcement and focus are one event's work. TokenizerBottomSheet.tsx closeClearConfirmation does it in the handler: flushSync the close, then focus and announce. handleClearAll can do the same, with no Effect waiting on isClearConfirmationOpen.
| * type runs Chinese values together and gives Japanese and Korean a bare | ||
| * space). | ||
| */ | ||
| export function formatSharedList( |
There was a problem hiding this comment.
This is the narrow-conjunction Intl.ListFormat that i18n/useListFormatter.ts owns ("Raw Intl.ListFormat construction stays centralized in this hook"). Pass useListFormatter()'s function into formatTokenValueText and delete this copy.
| openSheet(); | ||
| fireEvent.click(within(sheet()).getByRole('button', {name: 'Clear all'})); | ||
| // A scrim press dismisses the confirmation. | ||
| fireEvent.click(clearConfirmation()); |
There was a problem hiding this comment.
On the merge with main this click no longer dismisses the confirmation: main dismisses an info Dialog only on a press that starts and ends on the backdrop (family:overlay-dismissal FR10), and CI's test-ui (3/4) fails at line 1620. Drive a full pointer press on the backdrop after rebasing.
|
/approve-spec fd60b07 |
There was a problem hiding this comment.
Changes requested. The stacked touch flow matches the component:PowerSearch record edits an engineering owner approved on this head, and most earlier findings are fixed; two things remain:
- CI
lintfails because Prettier reformats the dependency array atPowerSearch.tsx:1402–1407; runningpnpm exec prettier --write packages/core/src/PowerSearch/PowerSearch.tsxfixes it. - The popover reset on a presentation swap still runs from an Effect at
PowerSearch.tsx:784–790; reset the state during render asautoFocusStatedoes and hide the layer from the detaching root's callback-ref cleanup (:696–703), so the Effect can go.
Two small follow-ups are optional: DEC-3 records 333 elements with Filters open, while the head reads 313 because the Clear all confirmation mounts only when it opens, and a few new comments carry stray *. lines or a stale "offers Remove".
[Automated review]
| [ | ||
| announce, | ||
| handleFieldFocus, | ||
| isSheetOpen, | ||
| isSheetPresented, | ||
| ], |
There was a problem hiding this comment.
CI lint fails here: Prettier puts this dependency array on one line ([announce, handleFieldFocus, isSheetOpen, isSheetPresented]). Run pnpm exec prettier --write packages/core/src/PowerSearch/PowerSearch.tsx.
| useEffect(() => { | ||
| if (isSheetPresentation && wasPopoverOpenRef.current) { | ||
| setPopoverStateRaw({type: 'idle'}); | ||
| popover.hide(); | ||
| } | ||
| wasPopoverOpenRef.current = popoverState.type !== 'idle'; | ||
| }, [isSheetPresentation, popover, popoverState]); |
There was a problem hiding this comment.
This Effect resets popover state and hides the layer on the presentation swap, and copies popoverState into wasPopoverOpenRef after every commit of every PowerSearch. The swap is the popover root detaching, and its callback-ref cleanup at :696–703 already runs exactly then. Record wasPopoverOpenRef in setPopoverState, then do the reset and popover.hide() in that cleanup, or reset during render with the previous-value pattern autoFocusState uses. The Effect can then go (architecture:react-component-runtime INV2, INV6).
|
/approve-spec 63c355f |
There was a problem hiding this comment.
Changes requested: one shared-path fix remains. An engineering owner approved this head's record edits, and both earlier blockers (the Prettier failure and the popover-reset Effect) are fixed.
ChoiceRowsinPowerSearchBottomSheet.tsx(lines 599–634) hand-rolls arrow, Home and End roving focus for theenumanddate_relativeradio rows, which Core already owns inuseInternalListFocus(asSegmentedControluses it): call the shared hook, check the row that takes focus from inside the group, and delete the local handler and manualtabIndex.
Non-blocking: a stray *. before @default true in PowerSearchEditPopover.tsx, and the SheetConfirmation and useDismissSearchOnTouchDrag headers name consumers that do not use them.
[Automated review]
| const handleRadioKeyDown = ( | ||
| event: React.KeyboardEvent<HTMLButtonElement>, | ||
| index: number, | ||
| ) => { | ||
| const nextIndexFor = (key: string): number | null => { | ||
| switch (key) { | ||
| case 'ArrowDown': | ||
| case 'ArrowRight': | ||
| return (index + 1) % items.length; | ||
| case 'ArrowUp': | ||
| case 'ArrowLeft': | ||
| return (index - 1 + items.length) % items.length; | ||
| case 'Home': | ||
| return 0; | ||
| case 'End': | ||
| return items.length - 1; | ||
| default: | ||
| return null; | ||
| } | ||
| }; | ||
| const next = nextIndexFor(event.key); | ||
| if (next === null) { | ||
| return; | ||
| } | ||
| event.preventDefault(); | ||
| const rows = | ||
| groupRef.current?.querySelectorAll<HTMLButtonElement>('[role="radio"]'); | ||
| rows?.[next]?.focus(); | ||
| if (!isDisabled) { | ||
| const target = items[next]; | ||
| onToggle(target.value, selected.has(target.value)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This hand-rolls the roving radio focus (arrows, Home, End, one tab stop) that Core already owns in useInternalListFocus (hooks/useListFocus.ts); SegmentedControl uses it for its radio group with selection following focus. Calling the shared hook here, and checking the row that takes focus from inside the group, keeps one path for fixes such as its tab-stop repair and right-to-left key mapping; the local handler and the manual tabIndex can then go.
|
/approve-spec de2e652 |
There was a problem hiding this comment.
Changes requested: two implementation fixes remain. An engineering owner approved this head's record edits, and ChoiceRows now uses the shared list focus.
- The content-search searchbox and suggestion rows in
PowerSearchBottomSheet.tsx(lines 2174–2179 and 2226–2251) move focus with hand-written ArrowUp/ArrowDown handlers, a list-focus concern Core already owns inuseInternalListFocus, which this component already calls for its choice rows: drive the searchbox and the rows through oneuseInternalListFocuscall (extending the hook if it cannot take the searchbox) and delete the local handlers. - The chunk preload in
PowerSearch.tsx(lines 614–618) runs in an Effect, but it only fires when the sheet root attaches, andhandOffFocusRefalready runs on that attach: callloadPowerSearchBottomSheet()there whenisSheetRootis true and remove the Effect.
Non-blocking: the SheetConfirmation and useDismissSearchOnTouchDrag headers name consumers that do not use them, and the hasAutoFocus comment above autoFocusState could be cut to its reason.
[Automated review]
| label={item.label} | ||
| isDisabled={!isInteractive} | ||
| onClick={() => handleContentSuggestionSelect(item)} | ||
| onKeyDown={event => { |
There was a problem hiding this comment.
This hand-rolls ArrowUp/ArrowDown focus across the suggestion rows (and the searchbox's ArrowDown branch at line 2174 does the same), while this component already calls useInternalListFocus for ChoiceRows. Core owns linear list focus there (architecture:react-component-runtime INV16). Could one useInternalListFocus call cover the searchbox and the rows (orientation: 'vertical', wrap: false, no roving tab index so the rows stay in the Tab order), with Enter kept in the searchbox handler?
| isSheetOpen || isSheetPresented || resolvedPresentation === 'bottom-sheet'; | ||
| // Preload the flow's chunk while the sheet presentation is active, so the | ||
| // first press opens without waiting for the network; nothing renders. | ||
| useEffect(() => { |
There was a problem hiding this comment.
isSheetPresentation turns true only when the key="sheet" root attaches, and handOffFocusRef (line 761) already runs on that attach and computes isSheetRoot. Calling void loadPowerSearchBottomSheet() there when isSheetRoot is true starts the preload at attach and removes this Effect; the call is idempotent.
| * @input Dialog, Layout, Heading, Text, Button; caller-owned copy | ||
| * @output SheetConfirmation — the confirmation a touch sheet opens over | ||
| * itself for Clear all and for discarding a changed draft | ||
| * @position Core-internal, shared by the Tokenizer sheet and the PowerSearch |
There was a problem hiding this comment.
Non-blocking: on main the Tokenizer sheet confirms with AlertDialog (TokenizerBottomSheet.tsx:937), and nothing here discards a draft. Suggest: "Core-internal; the PowerSearch sheets use it for Clear all. Not a public export."
| * @input Caller-owned search inputs, a focus target, and touch events from a | ||
| * separate results list | ||
| * @output useDismissSearchOnTouchDrag — touch handlers for the results list | ||
| * @position Core-internal, shared by the touch sheets; not a public export |
There was a problem hiding this comment.
Non-blocking: only PowerSearch's Add filter sheet calls this. Suggest: "Core-internal; PowerSearch's Add filter sheet uses it. Not a public export."
| } | ||
| }, [isSheetPresentation]); | ||
| const triggerRef = useRef<HTMLButtonElement>(null); | ||
| // `hasAutoFocus` passes through unchanged — the popover presentation hands |
There was a problem hiding this comment.
Non-blocking (architecture:knowledge-contracts INV29): 14 lines with sentences over 25 words. Suggest: "A swap remounts the field's control, and autoFocus would take focus from elsewhere on the page. Only the first control keeps autoFocus. The roots' callback refs hand over focus the field held."
|
/approve-spec 3d1186d |
There was a problem hiding this comment.
Changes requested: two fixes remain. An engineering owner approved this head's record edits, and both findings from the previous round are fixed (the content search now uses the shared list focus, and the chunk preload runs in the sheet root's callback ref).
PowerSearchBottomSheet.tsxdecides whether Filters stays open beneath Add filter, and whether Add filter shows Done, fromfieldPickerOriginRef.currentread during render (lines 1215 and 2332) and written during render byentryStep(lines 930–938), whicharchitecture:react-component-runtimeINV4 does not allow: keep the origin in state besidestepand deriveisFieldPickerDirectEntryfrom it.- The sheet's Clear all announces "All filters removed" from a new
@astryx.powersearch.sheet.filtersClearedkey (line 1637) that duplicates the existing@astryx.powersearch.allFiltersRemovedthe popover uses, so locales and caller overrides can word one outcome two ways: useallFiltersRemovedand delete the new key.
Non-blocking: the handOffFocusRef comment in PowerSearch.tsx (lines 728–743) could be cut to its reason in a few short sentences.
[Automated review]
|
|
||
| // The steps stack: each opens above the sheet that led to it, which stays | ||
| // open beneath. Add filter entered directly has no Filters sheet beneath. | ||
| const isFieldPickerDirectEntry = fieldPickerOriginRef.current === 'trigger'; |
There was a problem hiding this comment.
This reads fieldPickerOriginRef.current during render to decide whether Filters stays open beneath Add filter (and, at line 2332, whether Add filter shows Done), and entryStep writes the ref during render. architecture:react-component-runtime INV4: "A ref mutation MUST NOT be the sole source for a value later read to produce React-rendered output." Keep the origin in state beside step (set together in openFieldList and the open adjustment) and derive this from it.
| } else { | ||
| announce( | ||
| changedCount == null | ||
| ? t('@astryx.powersearch.sheet.filtersCleared') |
There was a problem hiding this comment.
@astryx.powersearch.sheet.filtersCleared has the same English and meaning as the existing @astryx.powersearch.allFiltersRemoved, which the popover's Clear all uses; with two keys, locales and caller overrides can word this outcome differently in the two presentations. Use t('@astryx.powersearch.allFiltersRemoved') here, as the line above reuses editableFiltersRemoved, and drop the new key from en.json.
|
/approve-spec 1920c3f |
There was a problem hiding this comment.
Changes requested: one fix remains. An engineering owner approved this head's record edits, the touch sheet flow follows component:PowerSearch FR10–FR21, and both fixes the previous review asked for are in (the Add filter origin is now state, and Clear all speaks the shared "All filters removed" message).
- The new
useLayoutEffectatPowerSearchBottomSheet.tsx:997–1001clears the pending focus target when the flow closes without naming an external system (architecture:react-component-runtimeINV2, INV3); clear the target incloseFlowand in the step Effect at:1144whenstepisnull, delete the Effect, and add a test that reopens the flow with a stale target pending.
Non-blocking: several new source comments run past INV29's sentence length, and the comment at PowerSearchBottomSheet.tsx:1823–1826 still says the Operator group leaves nested operators out.
[Automated review]
|
|
||
| // A focus target left pending when the flow closed never carries into the | ||
| // next open. | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
This Effect does the flow's closing work and synchronizes no external system (architecture:react-component-runtime INV3: "A new or changed Effect MUST name the external system it synchronizes"). Every close already reaches code that can drop the target: closeFlow (:1003) for Done and the bottom sheet's dismissal, and the step Effect at :1144, which runs in the commit where any close sets step to null, the disabled close included. Suggest deleting this Effect, clearing pendingSheetFocusRef in closeFlow, and in the step Effect: if (step == null) { pendingSheetFocusRef.current = null; return; }. A test that reopens the flow with a target pending would pin it.
| draftOperator == null ? '' : resolveOperatorLabel(draftOperator, t); | ||
| // The title names the filter's own operator when the step offers no | ||
| // Operator group — one editable operator, or an operator the group does not | ||
| // list (a `nested` one with no Editor override) — so a nested filter's step |
There was a problem hiding this comment.
Non-blocking: getVisibleOperators returns every operator, so the Operator group lists nested operators too, and "a nested one with no Editor override" no longer happens. Suggest: "The title names the operator when the step shows no Operator group."
|
/approve-spec c8a9d39 |
There was a problem hiding this comment.
Changes requested: one focus fix remains. An engineering owner approved this head's record edits, the close Effect from the previous round is gone, and the new reopen test covers that case.
PowerSearchBottomSheet.tsxclearspendingRemovalFocusRefonly for a row target (lines 1069–1073), so after the last editable filter is removed, a later Apply, Cancel or direct add in the same flow focuses the row's "Remove … filter" button instead of the row, againstcomponent:PowerSearchFR16; carry the removal intent on the pending target instead of a separate ref, and add a test for that sequence.
Non-blocking: a few added comments still run past INV29's sentence limits; see the inline suggestion.
[Automated review]
| const rowButton = pendingRemovalFocusRef.current | ||
| ? (row?.querySelector<HTMLButtonElement>('[data-sheet-remove]') ?? | ||
| row?.querySelector<HTMLButtonElement>('button')) | ||
| : row?.querySelector<HTMLButtonElement>('button'); | ||
| pendingRemovalFocusRef.current = false; |
There was a problem hiding this comment.
Blocking (FR16): this is the only place pendingRemovalFocusRef is cleared. A removal whose target is manager-add (line 1553, the last editable filter removed) returns at line 1023 with the flag still set, so the next manager-filter target in the same flow (Apply at 1391, Cancel at 1701, a direct add at 1446 or 1500) focuses [data-sheet-remove] instead of the row. Reproduced: remove the only filter, add Status is Closed, open its row, Cancel → focus is on "Remove Status filter". Suggest carrying the intent on the target ({type: 'manager-filter', …, focusesRemoveAction: true} from handleRemoveFilter) and deleting the ref, plus a test for this sequence.
| // A number as the token reads it: where the | ||
| // locale's digits and decimal mark are the token's, the token's own text — | ||
| // `String(value)`, so `1e21`, `1e-7`, and `-0` read as `1e+21`, `1e-7`, and | ||
| // `0`, exactly as on the token; elsewhere the locale's digits and decimal | ||
| // mark with no grouping and no rounding — the fraction cap is Intl's maximum, | ||
| // so a value as small as `1e-21` keeps its digits — which differs from the | ||
| // token only in notation. |
There was a problem hiding this comment.
Non-blocking (INV29): this is one 78-word sentence. Suggested: "Where the locale writes Latin digits and a . decimal mark, use the token's own text, String(value). Elsewhere, use the locale's digits and decimal mark, with no grouping and no rounding."
|
/approve-spec bde719c |
There was a problem hiding this comment.
Recommending approval. The touch sheet flow behind experimental.presentation follows the current component:PowerSearch contract (FR10–FR21), the record edits on this head are owner-approved, and the earlier focus issue is fixed: after the last editable filter is removed, Apply, Cancel and a direct add now return focus to the filter's row rather than its remove button.
Before merging, rebase onto main: #9687 has since added PowerSearch FR24 and conflicts with this branch in PowerSearch.spec.md and PowerSearch.tsx, so the rebased head will need a fresh /approve-spec.
Two optional comment tidy-ups are noted inline.
[Automated review]
| // One import promise, shared by the preload and the lazy element: the sheet | ||
| // presentation loads the flow's chunk as soon as it is active, so the first | ||
| // press does not wait for the network, while nothing of the flow is rendered | ||
| // until a press. |
There was a problem hiding this comment.
Non-blocking (architecture:knowledge-contracts INV29): loadPowerSearchBottomSheet makes a new promise on every call, so "One import promise" is not quite true; only the module import is shared. Suggested:
// The preload and the lazy element share one module import. The sheet
// presentation preloads the chunk when it becomes active, so the first press
// does not wait for the network.| // Where focus was inside the Filters or Add filter sheet, read on every | ||
| // focus change there, so a controlled update that unmounts the focused | ||
| // control (a filter's row, a dropped field's row, Clear all after the last | ||
| // filter) can put focus on the nearest remaining row by that index, else | ||
| // the sheet's first control, else the sheet panel. A removed element | ||
| // fires no blur, so the position is what the effect below reads. |
There was a problem hiding this comment.
Non-blocking (INV29): the first sentence runs to 58 words. Suggested:
// The last focused row index in the Filters or Add filter sheet. A removed
// element fires no blur, so the effect below reads this index to re-home focus.|
/approve-spec 2584467 |
There was a problem hiding this comment.
Changes requested. The touch sheet flow follows the current component:PowerSearch contract, the record edits are owner-approved on this head, and the rebase onto FR24 keeps the popover token's text and accessible names unchanged. One fix remains:
- After Apply on an edited filter, or after adding a filter from Add filter opened over Filters, focus ends on the Filters sheet itself instead of the edited row or the header's Add filter that FR16 requires, because the flow focuses the sheet beneath while the closing step's modal dialog still blocks it (
PowerSearchBottomSheet.tsx:1131–1147,:2350); hand that target to the closing sheet's focus return (finalFocusRef) and cover Apply and a create commit with a real-browser test, since the jsdomshowModalstub never blocks the sheet beneath.
Two optional comment tidy-ups are noted inline.
[Automated review]
| <BottomSheet | ||
| isOpen={step === 'value'} | ||
| onOpenChange={handleValueOpenChange} | ||
| finalFocusRef={finalFocusRef} |
There was a problem hiding this comment.
Blocking (FR16). In a real browser this step's dialog is still the topmost :modal dialog during its exit, so the Effect at lines 1141–1147 cannot focus the Filters row or Add filter beneath it (focus() does not take until the exit ends). The sheet then returns focus here, to the trigger, which is outside Filters, and BottomSheet falls back to the Filters panel: after Apply and after a create commit, focus ends on div[tabindex=-1] instead of the edited row or the header's Add filter. Passing a ref the flow points at its pending target (and at the trigger only when the flow ends) lets BottomSheet's own return land there.
| // The sheet flow (three standalone sheets, the sheet editors) only | ||
| // renders for the bottom-sheet presentation. Loading it on demand keeps it out | ||
| // of pages whose PowerSearch stays a tokenizer field. | ||
| // One import promise, shared by the preload and the lazy element: the sheet |
There was a problem hiding this comment.
Non-blocking (INV29): loadPowerSearchBottomSheet makes a new promise on every call; only the module import is shared. Suggested: "The preload and the lazy element share one module import."
| // Where focus was inside the Filters or Add filter sheet, read on every | ||
| // focus change there, so a controlled update that unmounts the focused | ||
| // control (a filter's row, a dropped field's row, Clear all after the last | ||
| // filter) can put focus on the nearest remaining row by that index, else | ||
| // the sheet's first control, else the sheet panel. A removed element | ||
| // fires no blur, so the position is what the effect below reads. |
There was a problem hiding this comment.
Non-blocking (INV29): this opens with a 58-word sentence. Suggested: "The last focused row index in the Filters or Add filter sheet. A removed element fires no blur, so the effect below reads this index to re-home focus."
…tion
`experimental={{presentation: 'bottom-sheet' | 'adaptive'}}` makes the whole
field one trigger with display-only filter tokens and moves filter editing
into a stack of tall modal sheets, the touch flow the Lab prototype ships:
- Each step opens above the one that led to it, which stays open beneath;
a dismissal closes only the top sheet and reveals the step beneath, so no
sheet has a Back action.
- Filters lists the filters as rows that edit a filter, each with a trailing
remove button (a one-tap filter's row removes it), under a fixed header
with Add filter and over a fixed footer with Clear all and Done. With no
filter it is one empty state with a centered Add filter.
- Clear all asks for confirmation, removes the editable filters, keeps
read-only ones, leaves Filters open and announces there.
- Add filter lists the grouped fields, or the content search with inline
suggestions, and leaves its search unfocused; a vertical touch drag on the
rows moves focus off a focused search. A field whose only operator takes
no value adds at once.
- The value step stages operator → value and confirms from its footer. A lone
editor sits on the sheet surface; beside the Operator group each section is
a muted container.
The flow loads on demand; `adaptive` resolves through the shared
compact-touch policy and stays latched until the flow's exit has returned
focus. Value editors are Core's own in a sheet presentation; list values edit
through the Tokenizer's sheet titled with the field label; nested values use
the built-in nested editor. Announcements made while a sheet is open go
through the open dialog's own live region. The confirmation and the
search-drag handling are the internal modules the Tokenizer sheet shares.
Implements `component:PowerSearch` DEC-3 (FR10–FR21, FR15a, AR4–AR6, PR5)
and lifts the record's implementation-gap notes, except the sheet's date
handling in `timezoneID` (FR11, FR12, FR17).
|
/approve-spec 777edfe |
There was a problem hiding this comment.
Approval recommended. The touch sheet flow behind experimental.presentation follows the current component:PowerSearch contract, the record edits are owner-approved on this head, and the focus fix now lands focus where FR16 says after Apply and after adding a filter, in a real browser as well as in the new test. Omitting experimental keeps today's field and popover.
Nothing blocks. Three optional comment rewrites are noted inline.
[Automated review]
| // The control a step change named also becomes where the closing step | ||
| // sheets return focus when their exit ends: a closing sheet that held focus | ||
| // keeps it until then, and BottomSheet then returns it, so the named control | ||
| // receives it even when the first move could not land. With no control | ||
| // named, the closing steps return focus as the flow does. |
There was a problem hiding this comment.
Optional (comment style): the first sentence runs to about 45 words, and in Chromium focus sits on the page body during the closing sheet's exit rather than staying in it. A shorter version that keeps only the reason:
// A closing modal sheet blocks focus on the sheet beneath until its exit
// ends. The closing step sheets then return focus to the control the step
// named, else to the trigger.| // One import promise, shared by the preload and the lazy element: the sheet | ||
| // presentation loads the flow's chunk as soon as it is active, so the first | ||
| // press does not wait for the network, while nothing of the flow is rendered | ||
| // until a press. |
There was a problem hiding this comment.
Optional (comment accuracy): loadPowerSearchBottomSheet returns a new promise on every call; only the module import is shared. Suggested: "The preload and the lazy element share one module import."
| // Where focus was inside the Filters or Add filter sheet, read on every | ||
| // focus change there, so a controlled update that unmounts the focused | ||
| // control (a filter's row, a dropped field's row, Clear all after the last | ||
| // filter) can put focus on the nearest remaining row by that index, else | ||
| // the sheet's first control, else the sheet panel. A removed element | ||
| // fires no blur, so the position is what the effect below reads. |
There was a problem hiding this comment.
Optional (comment style): the opening sentence runs to 58 words. Suggested: "The last focused row index in the Filters or Add filter sheet. A removed element fires no blur, so the effect below reads this index to re-home focus."
User need
On a compact touch screen, the PowerSearch field fights the software keyboard. When the tokenizer takes focus, the keyboard covers the half of the screen where the field menu and the editor popover open. The editor's Selector and Tokenizer then open more anchored layers inside a small popover, and each new token makes the field taller under the thumb. Builders who ship PowerSearch on phones have no supported touch surface. Today each product keeps the popover on touch or builds the flow itself.
Problem and solution fit
experimental.presentationonPowerSearchtakes'popover','bottom-sheet'or'adaptive':'popover'is the default: the tokenizer field and the editor popover, unchanged.'bottom-sheet'shows the touch surface on every device.'adaptive'shows the sheet on compact coarse-pointer screens and the popover otherwise. It resolves through the shareduseAdaptivePresentationpolicy, so a PowerSearch and a Tokenizer on one page switch together. On the server and while hydrating, it is the popover.PowerSearch.tsx): the field's box with display-only tokens and one button, which is the only tab stop. Its name is the label plus the visible content (WCAG 2.5.3). Disabled usesaria-disabledand stays focusable.PowerSearchBottomSheet.tsx, loaded on demand). The steps stack: each opens above the one that led to it, which stays open beneath, and a dismissal (Escape, scrim, swipe) closes only the top sheet and reveals the step beneath. No sheet has a Back action.empty-valued) filter's row removes it. A fixed header holds Add filter (an icon button) and a fixed footer holds Clear all and Done, with only the rows scrolling between them. With no filter, the body is one empty state ("No filters applied") whose centered primary Add filter is the only action.onChange(kept, 'remove', firstRemovedIndex)), leaves Filters open with focus on Add filter, and announces "All filters removed" there, or "Editable filters removed" while a read-only filter remains, joined with a changed result count. This matches the popover's Clear all onmain(FR23, DEC-4). The handler closes the confirmation withflushSync, then moves focus and announces, as the Tokenizer's sheet does; Keep filters returns focus to Clear all.useInternalListFocus), without wrapping. A field whose only operator takes no value adds at once and shows a plus, not a chevron. When Add filter is the entry point, its fixed footer holds Done.useInternalListFocus), and a row that takes focus from those keys is checked.component:BottomSheetFR10). From the value step, two sheets close: the stack's ordinary closes and open (FR5–FR9). Focus lands on Add filter and is still there after every motion has settled (FR16); a test asserts it after the sheets settle.string_listandentity_listvalues edit through the publicTokenizerwithexperimental={{presentation: 'bottom-sheet'}}, whose sheet opens above the value step, saves with Save, and asks before it discards changes.nestedvalue with the popover's built-inNestedEditor;formatTokenValueTextis the single source of a token's value text (the popover token, its FR24 name, and the sheet's Filters rows all read it), so the Filters rows read as the token does; catalog keys are under@astryx.powersearch.sheet.*; theadaptiveswap andhasAutoFocushandling; every visible field lists, including a nested-only one.::afterpattern Core's clear button uses.Current specification
component:PowerSearchDEC-3, FR10–FR21, FR15a, AR4–AR6, PR5, as onmainfor the Lab flow. This pull request lifts the record's implementation-gap notes that the port makes true and records Core's element counts in DEC-3.component:PowerSearchFR23 and DEC-4 (onmain): the popover's Clear all keeps read-only filters. The sheet's Clear all has the same semantics and the same catalog key.spec:AST-017FR21 and FR27: the experimental boundary, the[experimental]changeset, and no new theming target.component:SelectorFR4: the sharedadaptiveresolution.component:TokenizerDEC-3: the list-value sheet.component:BottomSheetFR5–FR9 (the stack) andfamily:overlay-dismissal. The deprecatedBottomSheetSwitcheris not used.Public delta and caller burden
packages/core/api-report.mdgains exactly that oneprop experimental?line.experimentalgets the same render path, DOM and callbacks asmain. The evidence is below.menuWidthandtokenOverflowBehaviorhave no surface. Each warns once in development.@astryx.powersearch.sheet.*catalog keys are added. The sheet's Clear all reusesmain's@astryx.powersearch.allFiltersRemovedand@astryx.powersearch.editableFiltersRemoved. No new theming target; the touch hit layer adds two private custom properties, documented inPowerSearch.doc.mjs.fieldInputFamily.test.tsxlists PowerSearch as pendingfamily:input-fieldsFR10: its sheet trigger composes the Field box, and its stablerefand root styling props stay where they are until their own decision.Design and alternatives
DEC-3 records the chosen model and the rejected alternatives: steps that replace one another with Back actions, Remove in the value step's footer, and focusing Add filter's search on open.
What changed from this pull request's earlier flow, item by item against the Lab flow:
emptyfilters (was: Remove in the value step's footer).empty-only fields add directly from their row, with a plus.EmptyStatewith a centered primary Add filter, and the header action hides.main's date editors in their sheet presentation, which read and write the UTC day, and tokens and rows show the token's date. The record keeps an implementation-gap note on FR11, FR12 and FR17 until the date-zone fixes land.purpose="info"(wasform).Where Core differs from Lab on purpose: the empty-result title wording stays in Core's catalog ("No filters match your search"); a nested-only field lists; the icon actions meet 44 px through a hit layer. Lab's Clear all says "Editable filters removed" every time; Core says it only while a read-only filter remains.
Tokenizer: the two list-value editors render the public
Tokenizerwith itsexperimental.presentation, now onmain; nothing internal to the Tokenizer is imported.Evidence
Desktop does not regress. The popover presentation,
adaptiveon a mouse or a wide screen, and an absentexperimentalall behave as onmain:main(3615956e2, this head's base) and on this head and records every DOM snapshot, the active element, and theonChange/onFocus/onBlurlog at each step. It covers 9 prop sets, each with no filters and with 15 filters spanning every value type (including a stale and a read-only filter), plus scripted typeahead, pick, Escape, the token edit popover, remove, Backspace, Clear all, blur, content-search free text and the nested editor. The variants areexperimentalabsent,{presentation: 'popover'}, and{presentation: 'adaptive'}withmatchMediamatching nothing. Each is compared withmain's render withoutexperimental.branch vs main (absent) diffs: []), and the spoken announcements are the same messages in the same order for every variant.main's self-variance between two runs is empty.PowerSearch.test.tsxgains one popover test (hasAutoFocusstays on the combobox across re-renders).PowerSearchEditPopover.test.tsx,PowerSearchValueEditor.test.tsx,PowerSearchFilterEditor.test.tsx, and the Tokenizer and BottomSheet suites are not edited.Touch flow: real Chromium, static Storybook builds of
main(Lab) and this head (Core), a 390×844 iPhone touch profile. Each pair shows the same state, Lab on the left and Core on the right:inertandaria-hidden. Under the confirmation, Filters is blocked by the open modal.Element counts (DEC-3,
document.body.querySelectorAll('*'), coarse pointer, read when settled, on this head): 123 at rest, 313 Filters, 314 with Add filter above it, 332 with a choice-row value step above both, 349 with a list-value step, 420 with the Tokenizer's sheet above that, 373 with an edit step above Filters, 333 with the Clear all confirmation. Fine pointer: 166 at rest, 334 with the editor popover open. The record states them.Bundle:
PowerSearch.jsloadsPowerSearchBottomSheet.jsonly throughimport(), so a field that does not opt in does not carry the flow (PR5).scripts/core-single-bundle.test.mjs). The value step reuses the popover's editors (FR17). The sheet getsPowerSearchValueEditor,NestedEditorandbuildRelativeDateOptionsfromPowerSearch.tsx, whose graph already loads them for the popover, so the editors and the components they reach stay out of the lazily imported graph. The modules the sheet itself reaches import Core modules directly:TextInput,Field,RadioList,RadioListItemanduseInputStatusIconchange only their import paths.architecture:component-style-authoringINV6), flagged and justified. ImportingPowerSearchcosts about 10 KB minified (about 3 KB gzip) more than onmain. That is over the 2 KB gzip budget. Per module, minified:PowerSearch.tsx+3.2 KB, the shared English catalog +2.4 KB,PowerSearchValueEditor.tsx+1.5 KB with its catalog +0.7 KB,formatTokenValue.ts+1.5 KB with its catalog +0.3 KB, focus-ring styles +0.5 KB.bottom-sheetand the first client render on a compact touch screen foradaptive. Loading them on demand would show no field until the chunk arrives. The swap hand-off and theadaptivelatch belong with them.PowerSearchValueEditorandformatTokenValueare the popover's own modules, which the value step and the Filters rows reuse so one value reads and edits the same way in both presentations (FR11, FR12, FR17). Their sheet branches cannot move into the chunk without forking the editor.Writing rubric results
Run over this pull request's change to
docs/specs/components/PowerSearch/PowerSearch.spec.md. The record text ismain's. On top of it:The implementation-gap notes are lifted from every sheet row except the date parts of FR11, FR12 and FR17, which now say what Core does instead.
The verification row no longer points at the Lab suite.
DEC-3 records Core's element counts in place of its implementation gap.
PowerSearchBottomSheet.test.tsxandPowerSearch.ssr.test.tsxjoinverified_by, with frontmatter links for claims that source comments used to cite.R1 narration: none. The record describes the shipped behavior in the present tense.
R2 research in the record: none. Measurements and runs stay in this description.
R3 evidence naming a change: none.
R4 rejected alternatives: unchanged one-line entries in DEC-3.
R5 pull request or issue references: none.
R6 ruled questions still listing options: none.
Scope
PowerSearch.doc.mjs: the prop withstability: 'experimental', an example, and the zh and dense variants), four stories, focused tests and the[experimental]changeset ship with the feature.Testing
vitest, scoped:packages/core/src/PowerSearch,internal,RadioList,TextInput,Field,hooks,Tokenizer,BottomSheet,Dialog,Tooltip,Token, the derived-var registry test and Lab'sMobilePowerSearch: 110 files, 2,880 tests passed onmain4e495ec5e; on3615956e2(this head's base), the PowerSearch, Button, BottomSheet, Tokenizer, internal and Lab MobilePowerSearch suites: 38 files, 1,383 tests passed, and Core typecheck passes.PowerSearchBottomSheet.test.tsxhas 172 tests.scripts/core-single-bundle.test.mjs: 3 of 3 pass against a fresh Core build.pnpm buildandpnpm typecheck: exit 0.ASTRYX_STRICT_LINT=1 eslintover every changed file: 0 errors.prettier --checkover every changed file: clean.pnpm check:repo(includescheck:knowledge,check:changesets,check:i18n-runtime,check:i18n-catalog,check:sync): exit 0.node scripts/sync-exports.js --check: up to date.pnpm check:knowledge: aligned.git diff --check: clean.