Cleaned up tier repository initializer#26766
Conversation
no ref This change should have no user impact. It: - Creates `Tier` models in parallel, for performance - Uses `Set` instead of an object This should slightly improve boot time and makes the code a bit clearer.
WalkthroughThe tier repository's internal caching was refactored: the 🚥 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)
📝 Coding Plan for PR comments
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)
ghost/core/core/server/services/tiers/tier-repository.js (1)
33-41: Rebuild#idsand#storeatomically insideinit().
#idsis mutated during thePromise.all()mapper and never reset ininit(). If this repository instance is ever re-initialized, stale ids can survive while#storeis rebuilt, which makes Line 137 take the update branch for a tier that is no longer in#store; Line 144 then doessplice(-1, 1, toSave)and overwrites the last cached tier.Proposed change
async init() { const models = await this.#ProductModel.findAll({ withRelated: ['benefits'] }); - this.#store = await Promise.all(models.map(async (model) => { + const ids = new Set(); + const store = await Promise.all(models.map(async (model) => { const tier = await Tier.create(this.mapToTier(model)); - this.#ids.add(tier.id.toHexString()); + ids.add(tier.id.toHexString()); return tier; })); + + this.#ids = ids; + this.#store = store; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/tiers/tier-repository.js` around lines 33 - 41, Reset and rebuild the repository state atomically in init(): do not mutate this.#ids inside the Promise.all mapper; instead create local arrays (e.g., newStore and newIds) while mapping models -> Tier.create(this.mapToTier(model)), push each created tier into newStore and its id (tier.id.toHexString()) into newIds, await Promise.all, then after all creations succeed assign this.#store = newStore and this.#ids = new Set(newIds) in one step so init() never leaves a partially-updated `#ids` that diverges from `#store`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/services/tiers/tier-repository.js`:
- Around line 33-41: Reset and rebuild the repository state atomically in
init(): do not mutate this.#ids inside the Promise.all mapper; instead create
local arrays (e.g., newStore and newIds) while mapping models ->
Tier.create(this.mapToTier(model)), push each created tier into newStore and its
id (tier.id.toHexString()) into newIds, await Promise.all, then after all
creations succeed assign this.#store = newStore and this.#ids = new Set(newIds)
in one step so init() never leaves a partially-updated `#ids` that diverges from
`#store`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0595d749-b43c-426c-a280-73f5fc4173fd
📒 Files selected for processing (1)
ghost/core/core/server/services/tiers/tier-repository.js
This comment was marked as outdated.
This comment was marked as outdated.
cmraible
left a comment
cmraible
left a comment
There was a problem hiding this comment.
Approved with a non-blocking comment. Good find!
|
|
||
| async init() { | ||
| this.#store = []; | ||
| this.#ids = {}; |
There was a problem hiding this comment.
nit: if init() is called twice (as it is in test fixture utils here), this could leave stale ids in the Set since we're no longer explicitly clearing this.#ids.
Unlikely to cause any real problems I think, but a slight unintended behavioral change.
Adding a this.#ids.clear() at the beginning of init() would solve this, but I'm wouldn't consider this blocking
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/server/services/tiers/tier-repository.js`:
- Around line 37-42: init() mutates this.#ids inside the concurrent map which
can leave the repository half-updated if any Tier.create() rejects; instead,
collect results and ids into local temporaries and only swap them into the
instance fields after Promise.all resolves successfully. Specifically, stop
calling this.#ids.add(...) inside the mapped async tasks; build a local array
(e.g., tempStore) from Promise.all(models.map(...)) and a local Set (e.g.,
tempIds) from tempStore.map(t => t.id.toHexString()), then on success assign
this.#store = tempStore and this.#ids = tempIds so the repository state is
replaced atomically. Ensure you still use mapToTier(model) and Tier.create(...)
as before but avoid mutating instance state until all creates succeed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0cc63ac0-10ec-4f18-a455-283625b5f079
📒 Files selected for processing (1)
ghost/core/core/server/services/tiers/tier-repository.js
| this.#ids.clear(); | ||
| this.#store = await Promise.all(models.map(async (model) => { | ||
| const tier = await Tier.create(this.mapToTier(model)); | ||
| this.#store.push(tier); | ||
| this.#ids[tier.id.toHexString()] = true; | ||
| } | ||
| this.#ids.add(tier.id.toHexString()); | ||
| return tier; | ||
| })); |
There was a problem hiding this comment.
Populate the caches atomically after Promise.all() succeeds.
init() now mutates #ids inside the mapped tasks. If one Tier.create() rejects during a later init() call, the method exits with a partially rebuilt #ids set while #store still contains the previous snapshot. That leaves the repository in an inconsistent state and can send save() down the update path for tiers that are no longer in #store.
💡 Suggested change
async init() {
const models = await this.#ProductModel.findAll({
withRelated: ['benefits']
});
- this.#ids.clear();
- this.#store = await Promise.all(models.map(async (model) => {
- const tier = await Tier.create(this.mapToTier(model));
- this.#ids.add(tier.id.toHexString());
- return tier;
- }));
+ const store = await Promise.all(models.map((model) => {
+ return Tier.create(this.mapToTier(model));
+ }));
+
+ this.#store = store;
+ this.#ids = new Set(store.map(tier => tier.id.toHexString()));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.#ids.clear(); | |
| this.#store = await Promise.all(models.map(async (model) => { | |
| const tier = await Tier.create(this.mapToTier(model)); | |
| this.#store.push(tier); | |
| this.#ids[tier.id.toHexString()] = true; | |
| } | |
| this.#ids.add(tier.id.toHexString()); | |
| return tier; | |
| })); | |
| const store = await Promise.all(models.map((model) => { | |
| return Tier.create(this.mapToTier(model)); | |
| })); | |
| this.#store = store; | |
| this.#ids = new Set(store.map(tier => tier.id.toHexString())); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/core/server/services/tiers/tier-repository.js` around lines 37 -
42, init() mutates this.#ids inside the concurrent map which can leave the
repository half-updated if any Tier.create() rejects; instead, collect results
and ids into local temporaries and only swap them into the instance fields after
Promise.all resolves successfully. Specifically, stop calling this.#ids.add(...)
inside the mapped async tasks; build a local array (e.g., tempStore) from
Promise.all(models.map(...)) and a local Set (e.g., tempIds) from
tempStore.map(t => t.id.toHexString()), then on success assign this.#store =
tempStore and this.#ids = tempIds so the repository state is replaced
atomically. Ensure you still use mapToTier(model) and Tier.create(...) as before
but avoid mutating instance state until all creates succeed.
3 participants
no ref
This change should have no user impact. It:
Tiermodels in parallel, for performanceSetinstead of an objectThis should slightly improve boot time and makes the code a bit clearer.