master
4 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Romeo Van Snick
|
3d9a6743c6 |
Hash static images through the rspack build (#82726)
* Declare png and gif module imports for the bundler * Hash onboarding, upsell and example images through the build * Assert the Slack app icon link against the bundled asset * Hash the upsell card story illustration through the build * Take image sources as props instead of building paths from a base string * Add a ?url image import that the SDK bundle references instead of inlining * Hash empty state, upsell and driver logo images through the build * Hash the map skeleton sprite and the bridge illustration through the build |
||
|
Romeo Van Snick
|
ee4510a467 |
Compile jest imports lazily (#82998)
* Load embed.js per test with isolateModules * Compile jest imports lazily * Settle requests instead of sleeping in negative tests * Let async waits outlast a cold worker's first render * Hold the path loading state open longer than a waitFor poll * Run pending timers instead of all timers in the initializeQB spec * Build the Accordion chevron element per render Mantine merges a nested provider's theme into its parent's with a deep merge, and a React element keeps a reference to the fiber that rendered it. The chevron element ran at import time before, outside any render, so it had no owner and the merge stopped at its props. Now that jest compiles imports lazily the config runs during a render, so the merge walks the fiber tree until the stack overflows. A function keeps the element out of the theme. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Romeo Van Snick
|
a0845f5373 |
Serve the locale catalogues as rspack chunks, remove locale api (#81933)
* Serve the locale catalogues as rspack chunks The catalogues were fetched from `app/locales/<locale>.json`, a path outside everything rspack does for an asset. They were unhashed, so `cacheable?` never granted them far-future headers, and uncompressed, so the server gzipped each one per request. Importing them instead puts them through the normal pipeline: rspack hashes each catalogue, emits it as its own chunk under `app/dist`, writes the `.br` and `.gz` siblings, and resolves the URL at runtime. A hashed file there is already cacheable, so no new rule is needed. Production swaps `devtool` for SourceMapDevToolPlugin, which takes an exclude. A source map for a catalogue maps JSON onto itself and would add about 29 MB. * Format the specs the locale change touched * Keep the catalogues out of the SDK bundle A context module bundles its whole directory wherever it resolves, so importing the catalogues added all 36 to the SDK's reachable graph: 66 files to 102, and its total gzipped size up 275%. The SDK should not carry them regardless of size. It renders in a customer's page against whichever instance it is pointed at, so its catalogue has to come from that instance rather than from whenever the SDK was built. `locale-catalog` now names how a catalogue is loaded, the way `ee-plugins` and `sdk-ee-plugins` name their implementations. The app imports from its own bundle; the SDK bundle config aliases the fetch instead, which also keeps the context module out of it. * Let the SDK cache the catalogues it fetches A catalogue served from `app/locales/<locale>.json` carries no hash, so `cacheable?` never matched it and the SDK re-downloaded one on every load. The SDK does not need the app's chunk hash to fix that, and it cannot have it: the two bundles are separate compilations with their own chunk ids, filename templates and runtime wrappers, so the same catalogue hashes differently in each. What it needs is any token that changes when the catalogues do, and it already has one. The instance serves the SDK bundle, so whatever build produced it produced the catalogues too, and `EnvironmentPlugin` already compiles that build's version and commit into it. The SDK now names its build in the query, and a catalogue request that carries one is cacheable. Without one it is not, so a caller that omits it cannot pin a stale catalogue. * Move the locale loading out of metabase/api Nothing here calls an API endpoint. The catalogue is a static asset, loaded by import in the app and by a plain fetch in the SDK, so it sits alongside `utils/i18n`, which applies what these return. * Swap the locale loader by path, not by alias The loader was reached through a `locale-catalog` alias, which had to be declared in the rspack aliases, tsconfig and jest config before the SDK bundle could override it: four places to say one thing. The importer now uses a plain relative import, which every resolver already understands, and the SDK config keys its override on the resolved path. One config knows a swap happens, and it is the one doing the swapping. * Let the SDK load catalogues as chunks too The SDK fetched catalogues from the instance over HTTP because a context import pulled all 36 into its bundle. They were never in its initial payload: an import() context always emits async chunks. What tripped CI was the gate itself, which is `embedding-sdk-chunked/total`, the reachable set. All 36 counted, 8.5 MB gzip against a 3.1 MB bundle, so 275%. The SDK can use the same loader. `sdk-public-path.ts` points `__webpack_public_path__` at the instance, and the package injects the bundle as a script tag, so a host app's bundler never sees the import(). Chunks land under `app/embedding-sdk/chunks/`, which the backend already serves with far-future immutable headers, so the build token and its `cacheable?` clause are not needed either. Measure the catalogues out of the reachable totals instead. One loads per session and none for English, so counting all 36 overstates by 35x, and their size tracks the translation process rather than any code change. Drops the loader, its spec, the build-time alias and the backend clause. The SDK's chunkFilename takes `[name]` so the gate can recognise them. * Fold the loader into its only caller Nothing overrides `loadLocaleCatalog` through the build any more, so it does not need to be its own module. Drop the SourceMapDevToolPlugin too. It existed to keep source maps off the locale chunks, which only cost disk in a dev build, and it made `devtool` conditional to work around the two not being able to run together. Clear the comments that explained the change rather than the code: the specs said what the catalogue used to do, and two references pointed at the file this commit removes. * Point the locale test hooks at the chunk The catalogue arrives as a hashed rspack chunk, so the specs that stubbed or waited on `GET /app/locales/<locale>.json` were watching a request that no longer happens. Five e2e specs used the intercept only as a synchronisation point, so they now wait on the chunk instead. `content-translations` stubbed an empty catalogue to keep the UI in English while it checks that content is translated, and a chunk cannot carry a JSON body, so it blocks the chunk. Its Arabic block asserts English chrome and `AR-` content only, nothing that needs the catalogue applied. That leans on LocaleProvider catching a failed load, so cover it: a rejected catalogue still renders the children. Also fix the import order the module move disturbed in three consumers. * Spy on loadLocalization instead of wrapping the module * Drop a tombstone comment * Keep the locale set when its catalogue fails to load Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUuaopJ7ZtMVvdwwxF6qyx * Wait on the language landing, not on the catalogue request Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUuaopJ7ZtMVvdwwxF6qyx * Keep the locales directory when the translations step clears it Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUuaopJ7ZtMVvdwwxF6qyx * Assert a real Dutch string after the first language switch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUuaopJ7ZtMVvdwwxF6qyx * Re-render the children once the catalogue is applied Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUuaopJ7ZtMVvdwwxF6qyx --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Romeo Van Snick
|
e4cc4a61ff |
Reduce the frontend to one memoize helper, and stop top-level caches leaking (#81866)
* Add a jest project for heap retention specs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Scope the parsed x value cache to one getXValues pass Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Assert heap retention on the settled pass, not the first touch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Key the log timestamp cache on the log object Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Key the schema viewer flow graph on the response object The memo key serialized every table and field of the schema, so each distinct schema held a megabyte-scale string forever. RTK Query returns a stable reference per cache entry, so a WeakMap keeps the identity that useGraphSync depends on without retaining the schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Key pie.rows on the dataset instead of a JSON copy of it The memo key serialized every row and column, so each dataset the user viewed held roughly its own size in cache key for the life of the tab. A WeakMap on the dataset with a short settings subkey keeps the same hit rate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Ban new uses of underscore's memoize The 17 remaining call sites are allowlisted. The list may only shrink. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Narrow the memoize rule to module scope A cache built inside a function, a component render, a class field or a constructor is released with whatever owns it. Only a module scope cache lives for the life of the tab, so the rule now flags just those. This drops the allowlist from 17 files to 5, and each remaining entry carries the reason its set of keys stays small. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Remove the memoize allowlist Three of the five module scope caches are gone: - getScrollBarSize takes no arguments, so it is a lazy value, not a cache. - getScheduleExplanation returns a string that no caller compares by identity. - cronToBuilderValue moves to the one caller that needs a stable identity. Schedule takes its value as a useMemo dependency, so StrategyForm now holds that memo itself. A user can write any number of cron strings, so a module cache for them was unbounded. The remaining two carry an eslint-disable with the reason their set of keys stays small. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Follow renderer_utils into the viz-core module Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Cover metabase/utils/memoize in the module scope rule That memoize roots its WeakMap on the function it wraps, so it only releases once that function does. At module scope the function never does, and the nested Maps hold every argument ever passed. No call site does this today, but the rule could not have caught one. The rule now covers both sources, so it drops "underscore" from its name. memoizeClass stays allowed: it keys on the instance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop re-reselect from the dashboard selectors createCachedSelector keyed getQuestionByCard and getDashcardParameterMappingOptions on the card id, a primitive, in a FlatObjectCache. That cache is a plain object with no eviction, so a session kept one Question per card id it ever saw, and each Question pins the whole Metadata snapshot it was built with. reselect v5 defaults to weakMapMemoize, which keys on the card and the metadata objects themselves. Entries are released when their card leaves the store or the metadata is replaced, and identity still holds for every card in the working set, which is what connect and useSelector compare on. Measured over 300 cards, with the states dropped: 6.2 MB retained before, 0.0 MB after. Per entry, 21462 B before and 51 B after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop re-reselect from the lockfile Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Back metabase/utils/memoize with reselect's weakMapMemoize The hand-rolled implementation had two problems. It allocated two arrays on every call, including cache hits: 225 ms against 20 ms for weakMapMemoize over two million hits. And its WeakMap was keyed on the wrapped function rather than the wrapper, so a cache built around a module-level helper outlived the wrapper that owned it. A plain Map would have been safer. weakMapMemoize matches on the same terms, identity for objects and value for primitives, so no call site changes. It also weak-keys object arguments, which the old one held strongly. definition-cache passed weakMapMemoize as the memoize and argsMemoize options. reselect v5 already defaults to both, so those are dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Route all memoization through metabase/utils/memoize underscore's memoize is gone from the app. Its 16 call sites now use the house helper, which is reselect's weakMapMemoize. Three of them were in metabase-lib, which cannot import metabase/utils, and all three took no arguments, so they became plain lazy fields instead. Two rules keep it that way: - no-direct-memoize-import bans memoize from underscore and weakMapMemoize or lruMemoize from reselect, in any scope. Only metabase/utils/memoize may reach for an implementation. - no-module-level-memoize keeps its job, now that the house helper is the only one left to police. measureTextWidthSafely took a style object that was rebuilt on every render, so identity keying would never have hit. Its third argument is now the font family string, which is the only field any caller ever set. useMemoizedCallback returns the callable shape rather than casting to T, since the memoized function also carries clearCache and resultsCount. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Build memoizeClass on the house memoize It kept a module-level WeakMap and walked a nested Map trie keyed on the instance, the method, the argument count and then each argument, assembling that path into an array on every call. Keying weakMapMemoize on the instance and then the arguments gives the same per-instance caching. One cache per decorated method replaces the shared trie, so the method no longer needs to be part of the key, and neither does the argument count. The file is down to 61 lines from 128. A leak spec covers the property this rests on: an instance's results are released with the instance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Measure memoizeClass retention under load 2,000 instances per pass, 20 calls each, across a method with no arguments and one taking a primitive and an object. Nothing is retained once the instances are dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Delete memoizeClass It decorated a prototype method by rewriting the property descriptor, and put the memoization 600 lines away from the method it applied to. Its one consumer memoized a method whose body uses no `this`, so the same caching comes from assigning a memoized arrow to a class field. The field is created per instance, so the cache is released with the instance, and the memoization is now visible at the definition. metabase/utils/memoize is 14 lines, from 128. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Run the heap specs from their own jest config They were a project inside jest.config.js, and a bare `jest` runs every project, which is what CI does. So the sharded unit run picked them up, without --expose-gc, and they failed as designed. They now live in jest.memory.conf.js, the way the timezone specs live in their own config. The shared base moves to jest.base.conf.js so neither file has to duplicate it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Reduce the heap specs to one collection assertion each Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh * Follow the store mocks into test support Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |