fix: validate serialized style entries individually instead of all-or-nothing#448
fix: validate serialized style entries individually instead of all-or-nothing#448mertcanekiz wants to merge 1 commit into
Conversation
…-nothing When Tailwind's Oxide scanner picks up false-positive class names (e.g. `[tid:%@]` from Objective-C build artifacts), `serializeJSObject` would discard the entire 255KB+ stylesheet because its single `new Function()` validation failed on the one bad entry. This changes validation to per-entry: invalid entries are skipped with a warning while all valid styles are preserved.
📝 WalkthroughWalkthroughThe serialization logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/uniwind/src/metro/utils/serialize.ts (1)
102-114: Consider logging which entries failed for easier debugging.The validation logic correctly implements per-entry filtering. However, when entries fail, developers only see a count without knowing which class names caused issues. Logging the failed entries (or at least the first few) would significantly help diagnose false positives like the
[tid:%@]case mentioned in the PR.💡 Optional: Log failed entries for debugging
- const validEntries = entries.filter(entry => { + const invalidEntries: string[] = [] + const validEntries = entries.filter(entry => { try { // eslint-disable-next-line `@typescript-eslint/no-implied-eval`, no-new-func new Function(`function v() { const o = ({ ${entry} }) }`) return true } catch { + invalidEntries.push(entry) return false } }) - if (validEntries.length < entries.length) { - Logger.warn(`Skipped ${entries.length - validEntries.length} invalid style entries during serialization`) + if (invalidEntries.length > 0) { + Logger.warn(`Skipped ${invalidEntries.length} invalid style entries during serialization: ${invalidEntries.slice(0, 3).join(', ')}${invalidEntries.length > 3 ? '...' : ''}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/uniwind/src/metro/utils/serialize.ts` around lines 102 - 114, When filtering entries in serialize.ts (the entries -> validEntries block) add capturing of the failed entries and include them in the warning: compute failedEntries = entries.filter(e => !validEntries.includes(e)) and call Logger.warn with both the count and a short sample (e.g., first 10) of failedEntries to avoid huge logs; reference the existing symbols entries, validEntries, and Logger.warn and ensure the message stays concise and safe for production (truncate or limit length).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/uniwind/src/metro/utils/serialize.ts`:
- Around line 102-114: When filtering entries in serialize.ts (the entries ->
validEntries block) add capturing of the failed entries and include them in the
warning: compute failedEntries = entries.filter(e => !validEntries.includes(e))
and call Logger.warn with both the count and a short sample (e.g., first 10) of
failedEntries to avoid huge logs; reference the existing symbols entries,
validEntries, and Logger.warn and ensure the message stays concise and safe for
production (truncate or limit length).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76977aee-285a-454d-8b47-9a324cd015e5
📒 Files selected for processing (1)
packages/uniwind/src/metro/utils/serialize.ts
| const serializedObject = pipe(obj)( | ||
| Object.entries, | ||
| entries => entries.map(([key, value]) => serializer(key, serialize(value))), | ||
| entries => entries.join(','), | ||
| result => { | ||
| if (result === '') { | ||
| return '' | ||
| } | ||
| const entries = Object.entries(obj).map(([key, value]) => serializer(key, serialize(value))) | ||
|
|
||
| const validEntries = entries.filter(entry => { | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-implied-eval, no-new-func | ||
| new Function(`function v() { const o = ({ ${entry} }) }`) | ||
| return true | ||
| } catch { | ||
| return false | ||
| } | ||
| }) |
There was a problem hiding this comment.
Let's use the pipe function like it was done before so we don't need random variable names
| }, | ||
| ) | ||
| if (validEntries.length < entries.length) { | ||
| Logger.warn(`Skipped ${entries.length - validEntries.length} invalid style entries during serialization`) |
There was a problem hiding this comment.
It should be Logger.error, warn is not logged without debug flag enabled, we want to resolve all serialization errors and this would limit number of reported errors
| const validEntries = entries.filter(entry => { | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-implied-eval, no-new-func | ||
| new Function(`function v() { const o = ({ ${entry} }) }`) |
There was a problem hiding this comment.
Let's keep the original naming
| new Function(`function v() { const o = ({ ${entry} }) }`) | |
| new Function(`function validateJS() { const obj = ({ ${entry} }) }`) |
|
Hi, thanks for the PR. It's a cool idea to move validation per entry instead of validating a full object. |
|
Implemented in #456 |
2 participants
Summary
serializeJSObjectcurrently validates the entire serialized stylesheet with a singlenew Function()call. If any single entry produces invalid JS (e.g. from a false-positive class name like[tid:%@]picked up by the Oxide scanner), the entire stylesheet is discarded, resulting instylesheet: {}and zero styles at runtime in production builds.Logger.errortoLogger.warnsince the build continues successfullyReproduction
[tid:%@](common in iOS/Objective-C logs, Sentry configs, etc.)npx expo export --platform iosstylesheet: {}, all styles missingTest plan
tsc --noEmit)Summary by CodeRabbit
Bug Fixes
Refactor