Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: TryGhost/Ghost/.coderabbit.yaml Review profile: QUIET Plan: Essentials Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (19)
🧰 Additional context used📓 Path-based instructions (6)Review whether tests prove changed behaviour, meaningful error/edge paths, and externally observable contracts without coupling to implementation details.⚙️ CodeRabbit configuration file Files:
Review lens: "where does this data become trusted?" Boundary data (HTTP input, external API/SDK responses, env/config, DB/filesystem reads, queue/webhook/event payloads) is `unknown` until validated — Zod by default.⚙️ CodeRabbit configuration file Files:
Review package boundaries and production consumption: minimal explicit exports, declared runtime dependencies, source-condition versus built-output parity, copied runtime assets, ESM/NodeNext compatibility, and consumer-facing release impac...⚙️ CodeRabbit configuration file Files:
Prioritise concrete correctness, security, data-integrity, compatibility, and regression risks.⚙️ CodeRabbit configuration file Files:
Source excerpt: Built Admin assets are copied into `ghost/core/core/built/admin/` for the Ghost release.📄 CodeRabbit inference engine (docs/codebase/monorepo-structure.md) Files:
Source excerpt: Ghost has several test suites across the monorepo.📄 CodeRabbit inference engine (docs/contributing/testing.md) Files:
🧠 Learnings (1)📚 Learning: 2026-08-03T21:09:05.797ZApplied to files:
🪛 LanguageToolpackages/memoize/README.md[locale-violation] ~43-~43: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects. (AFTERWARDS_US) 🔇 Additional comments (4)
WalkthroughThe change adds the Priority: ➖ Normal Change: Feature Merge Risk: ⚪ Minimal · up to This adds an internal package with no consumers yet, so there is no production impact. The author should reconcile the PR description with the documented behavior of keeping rejected promises. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many -t test:unit -p @tryghost/memoize,@... |
✅ Succeeded | 10m 54s | View ↗ |
nx run ghost:test:ci:integration |
✅ Succeeded | 5m 13s | View ↗ |
nx run @tryghost/admin:test:acceptance --shard=2/2 |
✅ Succeeded | 8m 10s | View ↗ |
nx run @tryghost/admin:test:acceptance --shard=1/2 |
✅ Succeeded | 8m 12s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 1s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 32s | View ↗ |
nx run ghost:test:ci:e2e |
✅ Succeeded | 4m 40s | View ↗ |
nx run-many -t lint -p @tryghost/memoize,ghost-... |
✅ Succeeded | 4m 37s | View ↗ |
Additional runs (13) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-09-30 13:56:36 UTC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #30886 +/- ##
==========================================
+ Coverage 69.19% 69.28% +0.08%
==========================================
Files 1622 1620 -2
Lines 59346 59336 -10
Branches 10253 10253
==========================================
+ Hits 41067 41112 +45
+ Misses 15990 15941 -49
+ Partials 2289 2283 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Benchmark note: this stack contributes nothing measurable to CPU, by designFlagging this so nobody infers a performance justification the data doesn't support. This PR and #30887 were measured on the Moya Pro benchmark as part of a six-PR integration branch (with #30704, #30753, #30874 and #30722). That branch came in at −11.5% CPU for a fixed 5,000-request workload — but essentially all of it is attributable to the nconf freeze and the theme translation cache. In the profile lane, neither That matches what I measured before any of this ran: a warm The case for this package is unchanged and is not about speed: one bound instead of per-site unbounded One thing the benchmark did confirm: the Generated by Claude Code |
c3da528 to
588c84d
Compare
|
@EvanHahn ccing you on this PR since you were asking about memoization helper bits last week 😄 |
|
Review findings for commit
Additional API risk: async computations are accepted by the generic signatures, but rejected promises are cached indefinitely by Validation: reviewed the diff and relevant integration code and ran focused lifecycle probes. I did not rerun the full suite. There are no consumers yet, so most runtime risks concern adoption in follow-up PRs. |
588c84d to
5051627
Compare
|
Addressed the review findings for
🤖 Generated with Claude Code |
250e50b to
80d5cda
Compare
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
It's a little hard to review this without knowing how it will be used. That said:
- I was struck by the global "is memoization enabled" state here. It seems like that will make things harder to test and less reliable. (Ghost is already riddled with global state, which I generally think causes problems.)
- The way promises are handled is nice in theory, but is a leaky abstraction in practice. If I call a memoized function twice in rapid succession, I might get two promises that will reject. IMO, it feels clearer to have it just return whatever the output was, promise or not.
- I don't like the idea of adding a new config option, because it doubles the code paths. Now, if I want to test something that uses
@tryghost/memoize, I need to test it twice: once with memoization on, and once with it off.
Again, I don't know the intended use cases, so maybe this all makes sense given that. But my hunch is that this could be a single ~100-line file with no global state.
| This is an internal workspace package. See the | ||
| [internal package golden path](../README.md) for its standing architecture and | ||
| maintenance rules. |
There was a problem hiding this comment.
nit: I don't think this is necessary.
|
|
||
| ## What this is for | ||
|
|
||
| Several places in Ghost memoise a pure derivation of an input that never |
There was a problem hiding this comment.
nit: some docs say "memoise" and others say "memoize". I'd stick with the latter, as it's more standard (e.g., Lodash).
| Clears every memo created so far. Ghost's test config helper (`configUtils.restore`) | ||
| calls it, so tests that rewrite config or swap modules in the require cache are | ||
| not served values derived from the previous state. |
There was a problem hiding this comment.
nit: in general, I'd audit this readme for internal things like this. I don't think we need to mention anything about Ghost's global test config helper.
no ref
Several profile follow-ups memoize a pure derivation of an immutable input:
compiled ICU messages by locale and text, parsed NQL filter trees, an Intl
formatter per timezone. Each of those would otherwise grow its own Map, with
its own (usually absent) bound.
One small helper gives them the same bound and the same semantics:
- memoize(compute, key, {max}) is a keyed memo backed by lru-cache, bounded at
500 entries by default. No TTL: entries are pure derivations of immutable
inputs, so an entry is never stale, only evicted.
- once(compute) computes on first call and returns the same value afterwards.
Both carry reset(), and neither memoizes a throwing compute. Promises are
stored like any other value; the helpers are meant for synchronous work.
This is a memo helper, not a cache. Nothing derived from mutable state goes
through it, because that needs real invalidation, which this deliberately does
not have.
The package holds no global state: no kill switch, no config key and no
process-wide registry. A switch would double the code paths every consumer has
to test, and a test that needs a clean memo can call its reset(). Nothing in
core uses the package yet; consumers adopt it in follow-ups.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
80d5cda to
3287437
Compare
|
Closing in favour of a plain library file in core. After review this shrank to one ~100-line file with no global state and no config option, and every planned consumer turned out to be inside The same 🤖 Generated with Claude Code |
3 participants
Why are you making it?
Several profile follow-ups memoize a pure derivation of an immutable input: compiled ICU messages by locale and text, parsed NQL filter trees, an
Intlformatter per timezone. Each of those would otherwise grow its ownMap, with its own (usually absent) bound.One small helper gives them the same bound and the same semantics.
What does it do?
Adds
@tryghost/memoizeunderpackages/memoize: a private, TypeScript-only ESM internal package, scaffolded frompackages/_template. It is one ~100-line source file with no global state.API
memoize(compute, key, {max})— keyed memo backed bylru-cache(11.5.2, added to the catalog). Carriesreset()and a readonlysize.maxdefaults to 500 and must be a positive integer. No TTL: entries are pure derivations of immutable inputs, so an entry is never stale, only evicted to stay inside the bound.once(compute)— computes on first call, returns the same value afterwards. Carriesreset().A
computethat throws is not memoized. A promise is stored like any other value; the helpers are meant for synchronous derivations.This is a memo helper, not a cache. Nothing derived from mutable state (settings, a database row) goes through it — that needs real invalidation, which this deliberately does not have.
No consumers yet, and no changes to Ghost core. Follow-ups adopt it one memo at a time.
One deviation worth a reviewer's eye:
memoizethrowserrors.IncorrectUsageErrorrather thanTypeErrorfor an invalidmax, because theno-native-errorlint rules rule out every native*Errorform. That adds@tryghost/errorsas a runtime dependency.Changes since the first review
optimization.memoizeconfig key,configure(), the process-wide registry andresetAll(), and theboot.js/configUtilswiring. The package now holds no global state; a test that needs a clean memo calls itsreset().lru-cacheoff the boot path, which no longer loads the package.require()consumers (and closed Changed repeated lazy requires to use once() #30887): a warmrequire()costs ~180 ns, so wrapping it bought nothing measurable.Verification
pnpm build/pnpm lintinpackages/memoizepnpm testinpackages/memoizepnpm lint:packagespnpm install --frozen-lockfileChecklist
🤖 Generated with Claude Code