Fix Android style file selection - #1806
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds Android detection and platform-aware native file-dialog filters. Style import pickers now disable Android-specific extension filters, allowing SLD and QML files to reach content validation. Tests cover Android, iOS, desktop, override, and fallback behavior. ChangesAndroid file-dialog filter handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Cloudflare PR preview
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
| * @returns True on Android/iOS (including desktop-UA iPadOS). | ||
| */ | ||
| const MOBILE_UA_PATTERN = /Android|iPhone|iPad|iPod/i; | ||
| const ANDROID_UA_PATTERN = /Android/i; |
There was a problem hiding this comment.
Minor readability nit: the pre-existing JSDoc block just above this line documents isMobile, but the new isAndroid pattern/function/doc is now spliced in between it and the isMobile function it describes (lines 43+). A reader hits the isMobile doc comment immediately followed by unrelated isAndroid code. Consider moving the whole isAndroid block (pattern + doc + function) above the isMobile doc comment instead, so each doc stays adjacent to what it documents. Purely cosmetic — no functional impact. Confidence: low.
| androidFilters: FileDialogFilter[] | undefined, | ||
| userAgent: string = typeof navigator !== "undefined" ? navigator.userAgent : "", | ||
| ): FileDialogFilter[] { | ||
| return isAndroid(userAgent) && androidFilters !== undefined ? androidFilters : filters; |
There was a problem hiding this comment.
Passing an explicit empty array (androidFilters: []) rather than omitting the property is the crux of this fix, and it relies on the Tauri Android dialog plugin treating a zero-length filter list the same as "no filters" (i.e. same as undefined) — showing all documents rather than, say, matching nothing. That's native (Kotlin/Rust) plugin behavior I can't verify from the JS side. The PR description says the author manually tested SLD/QML import on device, so this is presumably already confirmed in practice — flagging only as a low-confidence note in case this assumption needs re-checking on a future @tauri-apps/plugin-dialog upgrade.
|
Both inline comments posted successfully. Code reviewBugs: None found. The Android-detection ( Security: None found. No new user input handling paths beyond what already existed (file content parsing is pre-existing code, untouched by this diff). Performance: None found — this is UI-thread, one-shot dialog-filter selection with no measurable cost. Quality:
CLAUDE.md: No violations — no new user-facing strings requiring |
Summary
Testing
node --import tsx --test tests/is-mobile.test.ts tests/file-dialog-filters.test.tsnpm run test:frontendnpm run buildpre-commit run --all-filesus_cities.geojsonin light modeus_cities.geojsonin dark modeFixes #1804
Summary by CodeRabbit
Bug Fixes
Tests