chore: opt valid font family check logic in desktop#1360
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/effects-core/src/utils/text.ts (1)
1-2: Function nameisSimulatorCellPhonemay be misleading.The imported function detects mobile devices (Android, OpenHarmony, iOS) based on user agent strings, not simulators. This naming could cause confusion when reading the logic at line 12.
Consider whether the function name accurately reflects its purpose in this context.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects-core/src/utils/text.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-09T08:44:15.153Z
Learnt from: Fryt1
Repo: galacean/effects-runtime PR: 1305
File: packages/effects-core/src/fallback/migration.ts:325-330
Timestamp: 2025-12-09T08:44:15.153Z
Learning: In `packages/effects-core/src/fallback/migration.ts`, the `textColor` 0-255 to 0-1 conversion in `version35Migration` is intentionally applied only to `TextComponent` and not to `RichTextComponent`, as confirmed by Fryt1.
Applied to files:
packages/effects-core/src/utils/text.ts
🧬 Code graph analysis (1)
packages/effects-core/src/utils/text.ts (1)
packages/effects-core/src/utils/device.ts (1)
isSimulatorCellPhone(36-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/effects-core/src/utils/text.ts (1)
12-12: This change is intentional and correct—reconsider the premise.The function validates iOS 11/12 compatibility constraints, not CSS syntax universally. The Chinese comment clarifies: "iOS 11/12 do not support custom fonts starting with numbers."
On mobile devices, strict validation applies. On desktop, the looser validation is correct because:
- CSS
font-familylegitimately supports font names with leading digits when quoted (e.g.,"123font")- Desktop browsers have no restriction against such names (unlike iOS 11/12)
- The regex pattern only enforces unquoted CSS identifier format, which is device-specific, not a universal CSS requirement
The function still logs warnings when called, so it remains informational. The device-dependent behavior is intentional, though the function documentation could clarify that it validates compatibility constraints rather than CSS syntax.
Likely an incorrect or invalid review comment.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.