PoC v2: layered primitives (batchify + memoize + reactive), migrate VariantCountCache#5565
Draft
alisman wants to merge 1 commit into
Draft
PoC v2: layered primitives (batchify + memoize + reactive), migrate VariantCountCache#5565alisman wants to merge 1 commit into
alisman wants to merge 1 commit into
Conversation
…he (PoC v2)
Three composable primitives replace the LazyMobXCache hierarchy. Each
addresses one concern, each is independently usable. Cache classes
become 5–10 lines of composition; no inheritance.
batchify : turns a bulk endpoint into a single-item function that
auto-coalesces concurrent calls within one microtask tick
(DataLoader pattern). No caching, no MobX.
memoize : promise-level dedup. Same key in flight or resolved →
same Promise. Rejections evict so callers can retry.
reactive : wrap a Promise-returning fn so MobX observers can read it
synchronously during render. Returns null while fetching;
observable.box flips when it resolves.
VariantCountCache is migrated as a worked example. The pipeline reads:
reactive(memoize(batchify(bulkFetch, keyOf, keyOf), keyOf), keyOf)
Compared to the v1 PoC (#5563, closed): same call-site shape, but the
internals are three small, generic, recognizable primitives instead of
one fused custom helper. Each layer can be swapped or omitted at
use-site for caches that don't need all three concerns.
LazyMobXCache, the 18 other subclasses, SampleGeneCache, and SimpleCache
are untouched. This PR proposes the building blocks; follow-up PRs
migrate caches one or two at a time.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for cbioportalfrontend ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces three small composable primitives (batchify, memoize, reactive) to replace parts of the LazyMobXCache hierarchy, and migrates VariantCountCache to the new layered approach. The goal is to keep batching/dedup/reactivity opt-in per cache rather than inherited via a single fused helper or base class.
Changes:
- Added
src/shared/lib/batchedFetch.tsimplementingbatchify(microtask batching),memoize(promise dedup), andreactive(MobX observable read-through wrapper). - Added unit tests for each primitive and their composition in
src/shared/lib/batchedFetch.spec.ts. - Migrated
VariantCountCachefrom aLazyMobXCachesubclass to a factory that composes the primitives, and updated call sites/types accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/shared/lib/batchedFetch.ts | Adds the new composable batching/memoization/reactive primitives. |
| src/shared/lib/batchedFetch.spec.ts | Adds unit coverage for each primitive and a composition test suite. |
| src/shared/components/mutationTable/MutationTable.tsx | Updates VariantCountCache import to the migrated cache’s exported type. |
| src/shared/components/mutationTable/column/CohortColumnFormatter.tsx | Switches from CacheData to ReactiveEntry for the cohort frequency column. |
| src/shared/cache/VariantCountCache.ts | Migrates VariantCountCache to reactive(memoize(batchify(...))) and exports a factory + type alias. |
| src/pages/patientView/clinicalInformation/PatientViewPageStore.ts | Uses createVariantCountCache(...) instead of instantiating the old class. |
Comment on lines
+92
to
+102
| const state = observable.box<{ [key: string]: ReactiveEntry<V> }>( | ||
| {}, | ||
| { deep: false }, | ||
| ); | ||
| const inflight = new Set<string>(); | ||
|
|
||
| const settle = (key: string, entry: ReactiveEntry<V>) => | ||
| runInAction(() => { | ||
| state.set({ ...state.get(), [key]: entry }); | ||
| inflight.delete(key); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #5563
Same goal — propose a replacement for the `LazyMobXCache` hierarchy — but factored differently per review feedback. The previous PoC bundled batching, dedup, and MobX reactivity into one fused helper. This version splits them into three independent primitives so each cache only opts into the concerns it needs.
Three primitives, ~120 lines total
```ts
// src/shared/lib/batchedFetch.ts
batchify(bulkFetch, queryKey, resultKey)
// Turns a bulk endpoint into a single-item function. Concurrent calls
// within one microtask tick collapse into one bulkFetch call.
// No caching, no MobX. (DataLoader pattern.)
memoize(fn, keyOf)
// Promise-level dedup. Same key in flight or already resolved → same
// Promise. Rejections evict so retries work.
reactive(fn, keyOf)
// Wraps a Promise-returning fn in a MobX observable.box so callers can
// read it synchronously during render. `get(q)` returns null while
// fetching, then flips to {status, data} reactively when it resolves.
```
Each cache then composes the layers it needs:
```ts
// src/shared/cache/VariantCountCache.ts (post-migration)
const fetchOne = memoize(
batchify(
qs => client.fetchVariantCountsUsingPOST({ ... }),
keyOf, keyOf,
),
keyOf,
);
return reactive(fetchOne, keyOf);
```
A cache that doesn't need batching skips `batchify`. A cache that doesn't need MobX skips `reactive` and uses the bare Promise-returning function. Etc.
Why this is better than the v1 fused helper
Diff
+378 / -38 across 6 files. The bulk of the new lines are the three primitives plus 15 tests.
Test plan
What's intentionally NOT in this PR
Review questions
If approved, follow-ups
Each migrating one or two caches:
Once all subclasses are migrated, the LazyMobXCache hierarchy and ~20 wrapper files can be deleted in a final cleanup PR.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need.