Skip to content

fix(Tokenizer): forward cancel to the search source - #8777

Merged
cixzhang merged 6 commits into
meta:mainfrom
hurrymaplelad:tokenizer-cancel-forwarding
Nov 8, 2026
Merged

cixzhang merged 6 commits into
meta:mainfrom
hurrymaplelad:tokenizer-cancel-forwarding

Conversation

@hurrymaplelad

@hurrymaplelad hurrymaplelad commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Tokenizer filters the supplied search source but does not forward its optional cancel method. Cancellation should reach that source with the source as its receiver.

Forward cancel in one line; no new API. Sources without cancel continue to work as before. This forwarding alone does not prevent every late popup from a source that ignores cancellation.

#6360 includes the same forwarding line in a broader change; this PR isolates it.

Testing

  • Tokenizer and Typeahead component tests: 293 passed. Five regression tests fail without the forwarding line.
  • Core typecheck, eslint (no errors; one pre-existing warning), prettier, changeset checks and coverage passed.
  • These recorded checks do not establish green CI.

Complementary but independent of #8778: this PR forwards an abort attempt where the source supports cancellation; #8778 keeps a dismissed menu closed when pending search or bootstrap answers arrive, regardless of source cooperation with cancellation. Either proposal can land without the other; sequencing is deferred.

@vercel

vercel Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
astryx Ready Ready Preview Nov 8, 2026 5:16am UTC

Request Review

@astracat-bot

astracat-bot Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

AI review status for this pull request.

Review status Updated
🟢 Reviewed (for maintainers only) Nov 8, 2026, 5:30 AM UTC

@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge labels Oct 28, 2026
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

PR Analysis Report

🔎 Preview · 7af0472

Storybook · Sandbox · built from this PR's head

Modified components

Component Size LoC Visual · A11y · RTL
Tokenizer · Storybook 90.1 kB (+8 B) · 18.3 kB CSS (0) 2,224 (+1) ✓ · ✓ · ✓
PowerSearch 281.7 kB (+4 B) · 23.5 kB CSS (0) — — · — · —

Package size: @astryxdesign/core 6.97 MB packed (+133 B); no new dependencies.

API changes: none in this pull request

Committed API report behind this build: cli 29 rows, core 40 rows; run pnpm api:report.

Touched components checked; add ci:run-visual or ci:run-rtl to run the full suites.

Accessibility: 1 violation(s)

Accessibility Audit

Status: 1 accessibility violation(s) found — 1 serious.

Tokenizer - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 2/31 stories · Learn more
    • WCAG: 1.4.3 (Level AA)

Generated by PR Enrichment workflow | View full report

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 28, 2026

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks ready to merge. Forwarding cancel restores the documented SearchSource.cancel behaviour for Tokenizer: the new cancellation tests fail without the change and pass with it, and a popup dismissed with Escape no longer reopens when a source that aborts on cancel would have answered. CI's only failure is a Breadcrumbs focus test that this change doesn't touch. One optional follow-up, outside this PR: because Tokenizer rebuilds its filtered source whenever value changes, BaseTypeahead's render-time cancel of the replaced source now reaches the caller's cancel after each selection, so a cancel that sets React state logs React's "Cannot update a component while rendering a different component" error.

[Automated review]

# Conflicts:
#	packages/core/src/Tokenizer/Tokenizer.test.tsx

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for one wording fix: the forwarding itself looks right, and five of the new cancellation tests fail without the line and pass with it.

  • Regression: the Changeset says Tokenizer forwards cancel so that a newer search, closing the menu, focus leaving or unmounting cancels the consumer's search, but now that presentation defaults to adaptive, compact touch screens get the bottom-sheet presentation, which never calls cancel (TokenizerBottomSheet.tsx), so please scope the Changeset to the inline field (suggested wording inline) or make the sheet call cancel too, with a test.

A non-blocking wording suggestion for the new source comment is inline. CI has not run on this head: the pull request is a draft from a fork, so it runs once it is marked ready and a maintainer adds the ci:run label.

[Automated review]

@cixzhang
cixzhang marked this pull request as ready for review November 7, 2026 17:24
@cixzhang cixzhang added the ci:run A maintainer lets this outside contributor's pull request run CI (AST-030 FR17) label Nov 7, 2026

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for one wording fix: forwarding cancel looks right for the inline field, and the new cancellation tests fail without the line and pass with it.

  • Regression: the Changeset says Tokenizer forwards cancel so that a newer search, closing the menu, focus leaving or unmounting cancels the consumer's search, but with presentation defaulting to adaptive, compact touch screens get the bottom-sheet presentation, which never calls cancel (TokenizerBottomSheet.tsx), so please scope the Changeset to the inline field (suggested wording in the earlier inline comment on the Changeset) or make the sheet call cancel too, with a test.

The earlier non-blocking suggestion on the new source comment still applies.

[Automated review]

@vercel

vercel Bot commented Nov 7, 2026

Copy link
Copy Markdown

Deployment failed for project astryx with the following error:

The provided GitHub repository does not contain the requested branch or commit reference. Please ensure the repository is not empty.

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved pending CI: CI is still running on this head.

Forwarding cancel restores the documented SearchSource.cancel behaviour for Tokenizer's inline field: five of the new cancellation tests fail without the line and pass with it, and a popup dismissed with Escape stays closed when a source that aborts on cancel would have answered late. The Changeset now names the inline field and says the bottom sheet does not call cancel yet, which resolves the earlier request. Two finished CI jobs failed in code this change does not touch (a NavigationStack frame-timing interaction test and a CLI build-ranking test), so a re-run of those jobs is the next step.

  • Optional, not blocking: one pick now calls the source's cancel three times, once from BaseTypeahead's insertion effect when the rebuilt filtered source replaces the old one, so a cancel should be idempotent, and one that sets React state logs a development warning.

[Automated review]

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

Forwarding cancel restores the documented SearchSource.cancel behaviour for Tokenizer's inline field: five of the new cancellation tests fail without the line and pass with it, and a popup dismissed with Escape stays closed when a source that aborts on cancel would have answered late. Merging main left the change itself as it was. CI finished on this head with one failure, the Table focus-ring accessibility contract (and the test gate that reports it): it looks for tr[aria-expanded] rows that main already removed from TableTree, so it is not caused by this change and needs a fix on main rather than a re-run here.

  • Optional, not blocking: one pick now calls the source's cancel three times, once from BaseTypeahead's insertion effect when the rebuilt filtered source replaces the old one, so a cancel should be idempotent, and one that sets React state logs a development warning.

[Automated review]

@github-actions github-actions Bot removed the needs:code-review High-risk change (new package/component/API) — needs human code review before merge label Nov 8, 2026
@cixzhang cixzhang added the ci:run-a11y Run the accessibility suite on this PR; write access, next push (AST-030 FR12) label Nov 8, 2026
@cixzhang
cixzhang merged commit 462048e into meta:main Nov 8, 2026
80 checks passed

This branch was successfully deployed

1 active deployment
Preview — 7af04727 Deployed Nov 8, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:run A maintainer lets this outside contributor's pull request run CI (AST-030 FR17) ci:run-a11y Run the accessibility suite on this PR; write access, next push (AST-030 FR12) CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants