Repository navigation
feat(core): PowerSearch touch sheet flow behind experimental.presentation - #8001
imdreamrunner wants to merge 1 commit into
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
There was a problem hiding this comment.
This pull request is on hold for a spec decision: the proposed component:PowerSearch record (DEC-3, FR10–FR21, AR4–AR6, PR5 and AV5–AV6, with FR13's keyboard exception and DEC-3's surface-budget exception) and the sheet trigger's departure from family:input-fields FR10 need an engineering owner to approve them or name the requirements to change. An engineering owner records approval by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; FR17 and FR21 also depend on the Tokenizer record in #7977. Two changes are needed either way:
hasAutoFocusnow applies on the first mount only (PowerSearch.tsx:611-616), which removesdata-autofocusfrom the stable field after its first re-render, so a PowerSearch inside a Dialog that opens later no longer takes focus; keep it as passed until a presentation swap actually happens, and add a test with a Dialog that opens after mount.- The change is 8,668 authored lines; split out the separable pieces (the carried spec commit, the stable StrictMode fix, the formatter extraction) or land the size exception under current authority.
[Automated review]
| const hasMountedRef = useRef(false); | ||
| const isFirstMount = !hasMountedRef.current; | ||
| useEffect(() => { | ||
| hasMountedRef.current = true; | ||
| }, []); | ||
| const autoFocusOnMount = hasAutoFocus && isFirstMount; |
There was a problem hiding this comment.
autoFocusOnMount comes from a ref an Effect flips after the first commit, so it is false on every later render — including in the stable popover presentation, where it reaches the Tokenizer's hasAutoFocus and removes data-autofocus from the combobox. An Astryx Dialog keeps its children mounted while closed and focuses only [data-autofocus] after showModal(), so a <PowerSearch hasAutoFocus> in a Dialog that opens later no longer takes focus over the controls before it (in Chromium, with a button before the field: base focuses the combobox, head leaves focus on the button). Track the swap in state adjusted during render, as wasDisabled is, and keep hasAutoFocus constant until a swap actually happens.
There was a problem hiding this comment.
This adds an opt-in touch presentation to PowerSearch, and approval waits on a spec decision: the head writes DEC-3, FR10–FR21, AR4–AR6, PR5 and AV5–AV6 into the current component:PowerSearch record and keeps a recorded departure from family:input-fields FR10, so an engineering owner needs to approve or amend those requirements by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner) and decide whether FR10 is amended for this trigger or the trigger stops composing Field. FR17 and FR21 also depend on the Tokenizer requirements in #7977, which are not on main yet. Independently of that decision:
- Derive the swap-render
hasAutoFocusrule from state adjusted during render instead of the Effect-writtenrenderedPresentationRef(PowerSearch.tsx:617-622, react-component-runtime INV4 and INV3). - Show "No filters match your search" only for a non-empty query, as FR18 says (
PowerSearchBottomSheet.tsx:1852). - Split the 8,697-line change: land the record through its own spec pull request and move the StrictMode fix and the
formatTokenValue.tsextraction into their own pull requests.
[Automated review]
| const renderedPresentationRef = useRef(isSheetPresentation); | ||
| const isSwapRender = renderedPresentationRef.current !== isSheetPresentation; | ||
| useEffect(() => { | ||
| renderedPresentationRef.current = isSheetPresentation; | ||
| }, [isSheetPresentation]); | ||
| const autoFocusOnMount = hasAutoFocus && !isSwapRender; |
There was a problem hiding this comment.
This reads renderedPresentationRef, written by an Effect, during render to decide the trigger's autoFocus and the Tokenizer's hasAutoFocus. architecture:react-component-runtime INV4 says a ref mutation must not be the sole source for a value later read to produce rendered output, and INV3 asks a new Effect to name the external system it synchronizes. Deriving the swap from state adjusted during render (as wasDisabled is), for example a hasSwapped flag, keeps the intent without the ref.
There was a problem hiding this comment.
This adds an opt-in touch flow for PowerSearch, and approval waits on an engineering owner: the head writes DEC-3, FR10–FR21, AR4–AR6, PR5 and AV5–AV6 (with two INV7 owner exceptions) into the current component:PowerSearch record, keeps a recorded departure from family:input-fields FR10, and hosts the steps in BottomSheetSwitcher, which main now deprecates, so an engineering owner needs to approve or amend those requirements by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner) and decide the FR10 and switcher questions. Independently of that decision, these need the author:
- Split the 8,720-line change: let the record land with #8000, move the StrictMode fix, the
formatTokenValue.tsextraction and the story decorator out, and stage the flow. - Describe the stacked list-value sheet in DEC-3 and AV5 as an owner exception beyond the 5-element open-layer allowance, and drop "as it displays it" from AR6.
- Show "No filters match your search" only for a non-empty query, and keep a target for the content search's
aria-controlswhen nothing matches. - Add the writing-rubric results to the description and update the passages that still describe removed behavior.
[Automated review]
| list value (its Tokenizer trigger), and 401 with the Tokenizer's list-value | ||
| sheet open above the step (FR21) — every single-sheet state stays under the | ||
| open popover's 334; the stacked state does not, and is recorded under the | ||
| open-layer allowance (`spec:AST-017/FR60`, `DEC-29`) as this record's INV7 |
There was a problem hiding this comment.
spec:AST-017 FR60 and DEC-29 allow an open layer at most 5 extra elements under touch ("Anything beyond this needs an owner exception with before and after counts"). By this record's own counts the stacked state is 67 over the open popover (401 against 334), so it is an owner exception beyond the allowance rather than something recorded under it; could DEC-3 and AV5 say so?
| for a filter removed from the value step (the label is the Filters row's | ||
| text — the same "{label} {value}" catalog message the row renders, its | ||
| summaries and relative-date values resolved from the catalog as FR12 | ||
| states; each presentation announces a removed filter as it displays it: the |
There was a problem hiding this comment.
The popover presentation announces the Tokenizer item's label, which formatFilterValue builds (PowerSearch.tsx:841: a date with its time of day in timezoneID, a number with its units), not the token's visible text, and the first known gap below says the two differ in four places. "As it displays it" holds for the sheet only; could this say the popover announces its item label?
| // root styling props is a stable API change for existing callers, so it waits | ||
| // for its own decision rather than riding the experimental option | ||
| // (`component:PowerSearch` Open questions). | ||
| const PENDING_FR10: ReadonlySet<string> = new Set<string>(['PowerSearch']); |
There was a problem hiding this comment.
This keeps the family coverage test green while the trigger composes <Field> with ref on the root and root xstyle / className / style in the props type, which current family:input-fields FR10 does not allow for any component that composes Field. The exemption should follow an engineering owner's decision on FR10 rather than precede it.
There was a problem hiding this comment.
This adds an opt-in touch flow for PowerSearch, and approval waits on an engineering owner: the head writes DEC-3, FR10–FR21, AR4–AR6, PR5 and AV5–AV6 (with two INV7 owner exceptions) into the current component:PowerSearch record and keeps a recorded departure from family:input-fields FR10, so an engineering owner needs to approve or amend those requirements by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner) and decide the FR10 question. Independently of that decision, these need the author:
- Split the 8,800-line change: let the record land with #8000, move the StrictMode fix and the
formatTokenValue.tsextraction out, and stage the flow. - Update the description and the test file header where they still say nested-only fields are left out and nested rows are plain, and where the i18n list names the removed
editFilterandselectedChoicekeys.
[Automated review]
| * sheet, Add filter, the operator step, each value type (list values | ||
| * through the Tokenizer's sheet), content search, focus after each step, | ||
| * disabled while open, the StrictMode-safe result count, onChange metadata, | ||
| * nested exclusion |
There was a problem hiding this comment.
This header still lists "nested exclusion", which this head removed: nested-only fields now list in Add filter and their steps use the built-in nested editor. The description has the same outdated passages.
There was a problem hiding this comment.
This adds an opt-in touch flow for PowerSearch, and approval waits on an engineering owner. The head writes DEC-3, FR10–FR21, AR4–AR6, PR5 and AV5–AV6, with two INV7 owner exceptions, into the current component:PowerSearch record, and keeps recorded departures from family:input-fields FR10 and from the deprecated BottomSheetSwitcher. An engineering owner needs to approve or amend those requirements by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Independently of that decision, these need the author:
- Order Add filter's field groups with the ungrouped fields first, as FR13 says and the popover's field list already does (
PowerSearchBottomSheet.tsx:1012-1023), and add a test whose first field is grouped. - Split the 8,800-line change: let the record land with #8000, move the StrictMode fix and the
formatTokenValue.tsextraction into their own pull requests, and stage the flow. - Update the description where its Evidence list still names a nested-only exclusion test and where its catalog-key counts disagree with the 24 keys
en.jsonadds.
[Automated review]
| const groupedFields = useMemo(() => { | ||
| const groups = new Map<string, PowerSearchField[]>(); | ||
| for (const field of matchingFields) { | ||
| const key = field.group ?? ''; | ||
| const existing = groups.get(key); | ||
| if (existing) { | ||
| existing.push(field); | ||
| } else { | ||
| groups.set(key, [field]); | ||
| } | ||
| } | ||
| return [...groups.entries()]; |
There was a problem hiding this comment.
Add filter's groups follow insertion order here, so a config whose first field is grouped lists that group above the ungrouped fields. FR13 at this head says "ungrouped first", and the popover's field list already orders them that way through groupItems(items, {ungroupedFirst: true}) in usePowerSearchSource.ts. Reusing that ordering (or putting the '' group first) keeps the two presentations of one config consistent; a test with a grouped first field would cover it.
There was a problem hiding this comment.
This adds an opt-in touch flow for PowerSearch, and approval waits on an engineering owner: the head writes DEC-3, FR10–FR21, AR4–AR6, PR5 and AV5–AV6 into the current component:PowerSearch record, with two INV7 owner exceptions and a recorded departure from family:input-fields FR10, so an engineering owner needs to approve or amend them by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner) and decide whether FR10 is amended for this trigger or the trigger stops composing Field. The previous review's Add filter group-order finding is fixed, and FR10 now records the layer runtime's missing presentation hold. Independently of that decision, the 8,856-line change still needs splitting: let the record land through #8000, and move the StrictMode fix and the formatTokenValue.ts extraction into their own pull requests.
[Automated review]
There was a problem hiding this comment.
The touch sheet flow now matches the component:PowerSearch DEC-3 contract that merged on main, and its tests, typecheck and browser checks hold up; the remaining blocker is size. At 8,480 authored lines the change needs to be broken up, and parts of it are separable: please land the StrictMode result-count [fix] (with its test and Changeset) and the formatTokenValue.ts extraction ahead of the flow, then put the sheet presentation up as a stack that lands together, for example the trigger, latch and Filters sheet, then Add filter with the content search, then the value step with list and nested values.
[Automated review]
There was a problem hiding this comment.
The touch sheet flow now matches the component:PowerSearch DEC-3 contract on main, with standalone step sheets, and its tests, typecheck and browser checks hold up; the remaining blocker is size. At 8,521 authored lines the change needs to be broken up: please land the StrictMode result-count [fix] (with its test and Changeset) and the formatTokenValue.ts extraction ahead of the flow, then put the sheet presentation up as a stack that lands together. While the record is being edited, please also refresh the AV5 and DEC-3 element counts, which were measured with the switcher host.
[Automated review]
| @@ -218,8 +216,7 @@ FR10–FR21 govern. | |||
| standalone BottomSheets driven by one step state — a step that closes in the | |||
| update another opens is replaced by it in place (`component:BottomSheet` | |||
| FR10), the pattern `component:BottomSheetSwitcher` DEP-0015 names as the | |||
| deprecated switcher's replacement — or, until that replace is shipped, in a | |||
| BottomSheetSwitcher; it may compose List, RadioList, TextInput, Button, Heading, and Text in any | |||
| deprecated switcher's replacement; it may compose List, RadioList, TextInput, Button, Heading, and Text in any | |||
There was a problem hiding this comment.
With the switcher allowance gone, the counts a few lines below (296 Filters, 296 Add filter, 318, 328 and 401 stacked) and DEC-3's "adds 73 above it" still describe the switcher host. On this head Chromium measures 290, 290, 307, 322 and 393 (+71) on the Experimental Adaptive story at 390 × 844, so every relation holds; please re-measure and update the figures in this upkeep commit, as DEC-3 asks ("The counts are re-measured when either surface changes").
There was a problem hiding this comment.
The touch sheet flow follows the current component:PowerSearch record and holds up in the tests, the typecheck and a phone-sized Chromium run, but two things need changes before it can be approved. At 8,555 authored lines it needs splitting: please land the StrictMode result-count [fix] (with its test and Changeset) and the formatTokenValue.ts extraction first, then put the sheet presentation up as a stack that lands together. Separately, the Filters rows are keyed by position (PowerSearchBottomSheet.tsx:1519), so when the parent removes a filter above the focused row, focus jumps to the next filter or the focused row silently shows a different one; keying rows by filter identity, with a test, fixes it.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] PowerSearch no longer announces its initial `resultCount` on mount under React StrictMode. The result-count live-region effect guarded its first run with a mounted flag that StrictMode's effect replay reset, so the count present on mount was spoken unprompted; the guard now compares against the last announced text, so only a change after mount is announced in every render mode. |
There was a problem hiding this comment.
This [fix] and its popover StrictMode test change the stable presentation and stand on their own (the record says it "ships as a [fix] patch release of its own"); landing them ahead of the sheet flow is the first step of the split this review asks for.
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| PowerSearch · Storybook | 🔴 190.9 kB (+6.1 kB) · 21.5 kB CSS (+964 B) | 6,730 (+2,724) | — · — · — |
| lab/MobilePowerSearch | 215.9 kB (+1.2 kB) · 1.2 kB CSS (0) | — | — · — · — |
| lab/TransferList | 74.8 kB (+411 B) · 1.7 kB CSS (0) | — | — · — · — |
33 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| richtext/RichTextEditor | 278.1 kB (+407 B) · 2.6 kB CSS (0) | — | — · — · — |
| lab/ListInput | 81.9 kB (+402 B) · 1.6 kB CSS (0) | — | — · — · — |
| lab/EmojiPicker | 70.6 kB (+401 B) · 722 B CSS (0) | — | — · — · — |
| lab/Comments | 233.3 kB (+400 B) · 2.5 kB CSS (0) | — | — · — · — |
| lab/MobileTokenizer | 101.6 kB (+397 B) · 880 B CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 223.0 kB (+392 B) · 454 B CSS (0) | — | — · — · — |
| lab/TransferListSelector | 81.2 kB (+385 B) · 1.9 kB CSS (0) | — | — · — · — |
| lab/CommentComposer | 103.1 kB (+384 B) · 1.9 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 227.9 kB (+367 B) · 2.4 kB CSS (0) | — | — · — · — |
| DateTimeInput | 159.2 kB (+122 B) · 17.0 kB CSS (0) | — | — · — · — |
| DateInput | 103.4 kB (+109 B) · 15.3 kB CSS (0) | — | — · — · — |
| lab/ChatEmojiPicker | 65.2 kB (+46 B) · 773 B CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 65.7 kB (+44 B) · 1.1 kB CSS (0) | — | — · — · — |
| MultiSelector | 73.1 kB (+40 B) · 15.2 kB CSS (0) | — | — · — · — |
| CheckboxList | 43.4 kB (+39 B) · 8.6 kB CSS (0) | — | — · — · — |
| ComplexSelector | 55.6 kB (+33 B) · 11.1 kB CSS (0) | — | — · — · — |
| InputGroup | 42.1 kB (+25 B) · 8.4 kB CSS (0) | — | — · — · — |
| DateRangeInput | 69.9 kB (+24 B) · 11.8 kB CSS (0) | — | — · — · — |
| NumberInput | 55.0 kB (+24 B) · 11.2 kB CSS (0) | — | — · — · — |
| Selector | 77.5 kB (+17 B) · 15.6 kB CSS (0) | — | — · — · — |
| FileInput | 53.7 kB (+14 B) · 10.7 kB CSS (0) | — | — · — · — |
| TimeInput | 90.1 kB (+8 B) · 14.3 kB CSS (0) | — | — · — · — |
| Markdown | 145.3 kB (+1 B) · 18.3 kB CSS (0) | — | — · — · — |
| FieldLabel | 36.0 kB (0) · 5.9 kB CSS (0) | — | — · — · — |
| Tokenizer | 70.6 kB (0) · 17.1 kB CSS (0) | — | — · — · — |
| Typeahead | 63.1 kB (0) · 11.8 kB CSS (0) | — | — · — · — |
| Pagination | 86.6 kB (−1 B) · 16.0 kB CSS (0) | — | — · — · — |
| RadioListItem | 51.3 kB (−9 B) · 10.1 kB CSS (0) | — | — · — · — |
| TextInput · Storybook | 52.8 kB (−17 B) · 10.9 kB CSS (0) | 420 (+1) | — · — · — |
| Slider | 88.9 kB (−21 B) · 9.4 kB CSS (0) | — | — · — · — |
| TextArea | 47.2 kB (−21 B) · 9.8 kB CSS (0) | — | — · — · — |
| Field | 39.9 kB (−1.6 kB) · 7.4 kB CSS (−955 B) | — | — · — · — |
| RadioList | 41.3 kB (−1.6 kB) · 7.4 kB CSS (−956 B) | — | — · — · — |
Package size: @astryxdesign/core 6.17 MB packed (+70.9 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.
…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 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."
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, 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(692aa2647, 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, the derived-var registry test and Lab'sMobilePowerSearch: 107 files, 2,788 tests passed onmain4f452f927; on692aa2647(this head's base), the PowerSearch, Dialog, internal, hooks, Tokenizer and BottomSheet suites: 84 files, 2,028 tests passed, and Core typecheck passes.PowerSearchBottomSheet.test.tsxhas 169 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.