Skip to content

fix: use-mobile hook crashes on older Safari without MediaQueryList.addEventListener - #235

Merged
Vect0rM merged 2 commits into
AtomicBot-ai:mainfrom
Ayush7614:fix/use-mobile-addlistener-fallback
Aug 19, 2026
Merged

fix: use-mobile hook crashes on older Safari without MediaQueryList.addEventListener#235
Vect0rM merged 2 commits into
AtomicBot-ai:mainfrom
Ayush7614:fix/use-mobile-addlistener-fallback

Conversation

@Ayush7614

@Ayush7614 Ayush7614 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

useIsMobile() called mql.addEventListener('change', onChange) directly on a MediaQueryList. Older Safari (Catalina and before) only implements the deprecated addListener/removeListener pair, causing a runtime crash.

The sibling hook useMediaQuery.ts already has an attachMediaListener helper with a try/catch fallback for this exact case. Use the same pattern here for consistency and cross-browser compatibility.

Used by Sidebar and Dropdrawer components.

…ddEventListener

useIsMobile() called mql.addEventListener('change', onChange) directly
on a MediaQueryList. Older Safari (Catalina and before) only implements
the deprecated addListener/removeListener pair, causing a runtime crash.

The sibling hook useMediaQuery.ts already has an attachMediaListener
helper with a try/catch fallback for this exact case. Use the same
pattern here for consistency and cross-browser compatibility.

Used by Sidebar and Dropdrawer components.
@Vect0rM

Vect0rM commented Aug 18, 2026

Copy link
Copy Markdown
Member

Thanks for this, @Ayush7614 — the problem is real and the instinct to reuse the pattern we already had is the right one. 🙏

I checked the claims before reading the prose:

  • The crash is genuine on our target platforms. Tauri supports macOS 10.15+, and the WKWebView that ships with Catalina implements only the deprecated addListener/removeListener pair on MediaQueryList. useIsMobile() called mql.addEventListener('change', …) unguarded.
  • The blast radius is bigger than it looksuseIsMobile feeds components/ui/sidebar.tsx:88 and components/ui/dropdrawer.tsx:48, i.e. the app shell. A throw there is not a degraded widget, it's a blank window.
  • Matching useMediaQuery.ts is the correct call — same try/catch shape, same cleanup contract, and the hook keeps returning a disposer so the useEffect contract is unchanged.

Two things before this can go in.

1. The @ts-expect-error directives break the build

lib.dom does declare addListener/removeListener (deprecated, but present), so those calls type-check fine and the directives are unused — which is itself an error under our config:

a.ts(12,5): error TS2578: Unused '@ts-expect-error' directive.
a.ts(15,7): error TS2578: Unused '@ts-expect-error' directive.

That's tsc with our lib: ["ES2021", "DOM", "DOM.Iterable"] from web-app/tsconfig.app.json, on the exact block from this PR. So yarn build (tsc -b) fails. The direct evidence is in useMediaQuery.ts itself — it makes the same two calls with no suppression at all and has compiled for as long as it's existed.

Please drop both directives. If the deprecation lint bothers you locally, // eslint-disable-next-line @typescript-eslint/no-deprecated is the tool for that, not @ts-expect-error.

Worth flagging that this went unnoticed because we don't currently run tsc/vitest on pull requests — the empty check list on this PR isn't a green one. Not your problem to fix, but it does mean the review has to catch it.

2. Third copy of the same helper

attachMediaListener (and the MediaQueryCallback type) now exists verbatim in both useMediaQuery.ts and use-mobile.ts. Please export it from useMediaQuery.ts — or lift it into a small shared module — and import it here, so the next browser quirk gets fixed in one place instead of two.

Everything else — the tests, the comment explaining why the fallback exists, keeping the file's existing quote style — is good. Once 1 is fixed I'm happy to approve; 2 is a strong preference, not a blocker if you'd rather keep the diff local. Thanks again for digging into a compatibility path most people never think about. 🧭

…expect-error

- lib.dom declares addListener/removeListener (deprecated but present), so the
  @ts-expect-error directives were unused and failed tsc (TS2578).
- Export attachMediaListener + MediaQueryCallback from useMediaQuery.ts and
  import them here, removing the duplicated helper.
@Ayush7614

Ayush7614 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review @Vect0rM

Addressed both review points on branch fix/use-mobile-addlistener-fallback (pushed c9bf47351):

  1. Dropped the @ts-expect-error directives. You're right that lib.dom declares addListener/removeListener (deprecated but present), so the directives were unused and broke tsc with TS2578. Both are gone.
  2. Reused the shared helper. attachMediaListener and MediaQueryCallback are now exported from useMediaQuery.ts and imported here — the third copy of the helper is removed, so the next browser quirk gets fixed in one place.

Verified locally: tsc -b, eslint, and the use-mobile.test.ts suite (4/4) all pass. Thanks for the careful review!

@Vect0rM

Vect0rM commented Aug 19, 2026

Copy link
Copy Markdown
Member

Both points addressed, and the second one done properly rather than minimally — thanks, @Ayush7614. 🙏

I re-verified c9bf4735 locally instead of trusting the summary: worktree off your branch, origin/main merged in (now carrying #234), full tsc -b / eslint / vitest.

  • tsc -b — exit 0. The TS2578 blocker is gone.
  • eslint — 0 errors; the 12 remaining warnings are all pre-existing in other files, none in what you touched.
  • vitest — 4/4.
  • The tests actually hold the line. I reverted use-mobile.ts to main while keeping your test file, and 2 of the 4 fail — both addListener fallback cases. So the compat path is genuinely pinned, not just described.
  • The dedupe is the right shape. use-mobile.ts is down to +3/−2 and the helper now lives in exactly one place, so the next MediaQueryList quirk is a one-file fix.

One trivial nit, deliberately not blocking the merge — noting it so it's on record rather than lost:

export type MediaQueryCallback = … pushes line 8 of useMediaQuery.ts to 85 chars, so that file now fails prettier --check where it passed on main (Prettier would break it across four lines). And the export on the type isn't actually needed — use-mobile.ts imports only the function, and MediaQueryCallback has no other consumer. Dropping export from the type alone restores the file to clean. Prettier isn't wired into CI or the pre-commit hook here, so nothing breaks either way; it's a one-liner whenever you or someone else is next in that file.

Merging. Thanks for taking a compatibility path most people never think about, and then leaving it tidier than you found it. 🧭

@Vect0rM Vect0rM left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verified locally on c9bf473 — tsc -b clean, eslint clean on the touched files, vitest 4/4, and the fallback tests fail when the fix is reverted. LGTM.

@Vect0rM
Vect0rM merged commit 267bef6 into AtomicBot-ai:main Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants