-
Notifications
You must be signed in to change notification settings - Fork 197
feat: don't include asset data in bundle #11520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat_bridged_grouping
Are you sure you want to change the base?
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
f6682d7 to
a9eccb7
Compare
Moves asset data generation to outputting/serving in /public, *without* the perf changes from #8423. In effect, this means that: - The generated asset data goes from ~5MB to ~15MB *but* it is now fetched at runtime, not in the bundle - LLMs and humans alike can actually understand those files, and work better with them. It's even not unrealistic now to actually monkey patch bits there as needed TODO(gomes): Test one of two hard things in software engineering (i.e regardless of "Disable cache" in devtools, regenerated asset data cache should be invalidated on change) Changes: - Move generated files to /public/generated/ (served as static assets) - Remove all encoding/decoding (revert PR #8423 optimizations) - Output readable, pretty-printed JSON - AssetService works in both Node.js (generation) and browser (runtime) - Integrated with feat_bridged_grouping canonical asset handling - AssetLoader component initializes assets before app renders - Use existing test mocks from @/test/mocks/assets Testing: - Test *both* with dev and actual build (yarn preview:prod) and ensure no crashes, and assets look good - Try and regenerate one more time (without REGEN_ALL) and ensure it *still* looks good Risks: High Screenshots: TODO 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
a9eccb7 to
4fa03b4
Compare
Moves asset data generation to outputting/serving in /public, *without* the perf changes from #8423. In effect, this means that: - The generated asset data goes from ~5MB to ~15MB *but* it is now fetched at runtime, not in the bundle - LLMs and humans alike can actually understand those files, and work better with them. It's even not unrealistic now to actually monkey patch bits there as needed TODO(gomes): Test one of two hard things in software engineering (i.e regardless of "Disable cache" in devtools, regenerated asset data cache should be invalidated on change) Changes: - Move generated files to /public/generated/ (served as static assets) - Remove all encoding/decoding (revert PR #8423 optimizations) - Output readable, pretty-printed JSON - AssetService works in both Node.js (generation) and browser (runtime) - Integrated with feat_bridged_grouping canonical asset handling - AssetLoader component initializes assets before app renders - Use existing test mocks from @/test/mocks/assets Testing: - Test *both* with dev and actual build (yarn preview:prod) and ensure no crashes, and assets look good - Try and regenerate one more time (without REGEN_ALL) and ensure it *still* looks good Risks: High Screenshots: TODO 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Root cause: AssetLoader was running before PersistGate, so: 1. AssetLoader populated 25k assets 2. PersistGate rehydrated and migration cleared them 3. Result: 0 assets in Redux Fix: AssetLoader now runs AFTER PersistGate completes rehydration. Also: Use existing test mocks from @/test/mocks/assets 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
7d68b3e to
afdaa17
Compare
The decodeAssetData function was computing these properties, but we removed it. Now we compute them manually based on relatedAssetKey: - isPrimary = relatedAssetKey === null || relatedAssetKey === assetId - isChainSpecific = relatedAssetKey === null This fixes asset search showing 0 results (primary assets were all filtered out). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
…ex.json - encodedAssetData.json → generatedAssetData.json - encodedRelatedAssetIndex.json → relatedAssetIndex.json Files stay in /public/generated/ (just filename change, not path change) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
- Add generateManifest() function to compute SHA-256 hashes of asset files - AssetService fetches asset-manifest.json and uses hashes in query params - Fetch URLs: /generated/generatedAssetData.json?v=<hash> - When files change (regen), hashes change, cache is busted - Update all test mocks to include asset-manifest.json - Use asset-manifest.json (not manifest.json) to avoid conflict with PWA manifest Benefits: - Stable filenames (no renaming on every commit) - Automatic cache invalidation when content changes - Works with dev server and production - Tiny overhead (<1KB manifest file) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
triple-check this, there may be more cleanup needed re: asset-service
Remove networkName, explorer, explorerAddressLink, explorerTxLink from individual assets. These are chain-level properties (same for all assets on a chain). Changes: - Stripped properties from generatedAssetData.json (18MB → 14MB, 22% reduction) - AssetLoader enriches assets with chain data from getBaseAsset() when upserting to Redux - Updated asset-manifest.json with new content hash - Data in Redux store remains identical (enriched at load time) Benefits: - 4MB smaller file (faster network transfer) - Single source of truth for chain metadata (baseAssets.ts) - Easier maintenance (explorer URL changes only need chain-level updates) - Cleaner generated data (no duplication) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
- Use private properties with public getters instead of 'as any' casts - Remove unnecessary comment explanations in trade slices - Simplify comment in generateAssetData.ts 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
| return ( | ||
| <Page> | ||
| <Center flexDir='column' height='100vh' backgroundImage={colors.altBg} px={6} _after={after}> | ||
| <Center flexDir='column' height='100vh' background={colors.altBg} px={6} _after={after}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…tion - Global fetch stub in setupVitest.ts (used by all tests) - Remove duplicated fetch mocks from individual test files - portals/utils.ts: use getLocalAssetData() helper to reduce diff - Remove migration 256 (not needed) - SplashScreen: keep background (not backgroundImage) - handles gradients correctly Cleanup reduces test code duplication by ~100 lines. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
| import Polyglot from 'node-polyglot' | ||
|
|
||
| import { descriptions } from './descriptions' | ||
| import { localAssetData, relatedAssetIndex, sortedAssetIds } from './localAssetData' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole thing was basically useless (ish, is now!) - basically was a wrapper over generated encoded asset data with decoding. Since we don't have encoding anymore, no need for the additional layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be able to clean this up

Description
... and don't encode.
This PR:
networkName,explorer,explorerAddressLink,explorerTxLink- we can generate those at runtime in app as they're all the same for a givenChainId.In effect, this means that:
Issue (if applicable)
N/A
Risk
High
This PR fundamentally changes how asset data is loaded:
All asset-related functionality could be affected if asset data fails to load. The entire app depends on asset data being available.
Testing
Engineering
assetXHRsOperations
This is a foundational change that affects all asset loading. QA should verify:
Screenshots (if applicable)
TODO