Repository navigation
Changed app installation writes to be planned before they're stored - #32083
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the main change: app-installation writes are planned before storage. |
| Description check | |
The description directly explains the write-planning changes, including approvalWrite, recheckWrite, shared store behavior, and transaction timing. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Type-Safe Boundaries | |
PASS. The PR changes internal planning and storage calls. Its database reads remain decoded with existing Zod schemas, including findDigest and whileUnchanged. LoadedManifest comes from the exis… |
| New Files Are Typescript | |
The authoritative PR diff contains four modified .ts files and no added files. It adds no .js, .jsx, .cjs, or .mjs source file, so the failure condition does not apply. |
| Tooling Ownership | |
PASS. The PR changes only four domain-service and unit-test files under ghost/core/server/services/app-installations and ghost/test/unit/server/services/app-installations. It adds no scripts, tool… |
Full details: Type-Safe Boundaries
-
Explanation
PASS. The PR changes internal planning and storage calls. Its database reads remain decoded with existing Zod schemas, including
findDigestandwhileUnchanged.LoadedManifestcomes from the existing validated manifest loader. The newAddedRowtype usesz.infer<typeof AppInstallationManifestSource>. The production diff adds noany, uncheckedas,@ts-ignore, or@ts-nocheck, and it does not duplicate a Zod-described shape.
Full details: Tooling Ownership
-
Explanation
PASS. The PR changes only four domain-service and unit-test files under
ghost/core/server/services/app-installationsandghost/test/unit/server/services/app-installations. It adds no scripts, tools, shared configuration, or moved paths. The service README permits supporting implementation inside the owning service, and the test remains beside its owner. No tooling placement or execution issue is introduced.
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 1m 47s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 1m 50s | View ↗ |
nx run ghost:test:ci:e2e |
✅ Succeeded | 3m 42s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 3m 36s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 19s | View ↗ |
nx run ghost:test:ci:unit |
✅ Succeeded | 1m 39s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 59s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 23s | View ↗ |
Additional runs (6) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-10-29 08:50:07 UTC
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #32083 +/- ##
==========================================
+ Coverage 85.53% 85.58% +0.05%
==========================================
Files 1412 1412
Lines 52858 52866 +8
Branches 9073 9073
==========================================
+ Hits 45211 45248 +37
+ Misses 6557 6537 -20
+ Partials 1090 1081 -9
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:
|
ref https://linear.app/ghost/issue/BER-4035/sanitize-app-provided-svg-icon An approval and a re-check each worked out inside their write what a decision stores, so nothing outside the transaction could tell whether a write would add a manifest row. Work that has to happen before the transaction for a new row only, such as fetching an app's icon, would have had to repeat that logic beside each write and keep it in step. - Added approvalWrite and recheckWrite, which turn a decision into the write it stores: nothing, an existing row approved, or a new row - Pure functions beside decideOnManifest, each a switch over every kind, so a new kind fails the build in both - Replaced storeApproval and storeRecheck with one store that carries out a write - A use_pending decision now carries the pending manifest it matched, so an approval no longer needs a fallback for a pending manifest it can't find
ref https://linear.app/ghost/issue/BER-4035/sanitize-app-provided-svg-icon A write plan still needed the loaded manifest passed in beside it, and an approval could plan a write that leaves its manifest pending, which the staff history would then record as approved. Work for a new row, such as fetching an app's icon, belongs on the plan itself. - Changed approvalWrite and recheckWrite to decide on the manifests themselves, so both take the same inputs and the installation is passed once - Split the add write into add_approved and add_pending, and typed an approval's write so it can't leave its manifest pending - A new row's write carries the repository's AddedManifest, which now holds the loaded manifest, so store needs only the plan - Restored the exhaustive check in store, so a new write kind fails the build at the switch
…row` ref https://linear.app/ghost/issue/BER-4035/sanitize-app-provided-svg-icon A write plan's new manifest row was called `manifest`, so reaching the app's icon read `write.manifest.loaded.manifest.icon`: three different things all called manifest. Calling it a row, as the glossary and schema already do, leaves `manifest` meaning only the manifest itself.
ref https://linear.app/ghost/issue/BER-4035/sanitize-app-provided-svg-icon Several comments described the code in roundabout phrasing ("let go of", "what storing a decision writes, decided before anything is written") that took a few reads to follow. They now say plainly what each write does and why the plan exists, so future readers don't have to trace `activate` and the transaction to work it out.
jonatansberg
force-pushed
the
ber-4035-manifest-write-plans
branch
from
October 29, 2026 08:42
0b18db3 to
aa92754
Compare
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.
ref https://linear.app/ghost/issue/BER-4035/sanitize-app-provided-svg-icon
An approval and a re-check each worked out inside their write transaction what a decision stores, so nothing outside the transaction could tell whether a write would add a manifest row. #32022 has to fetch an app's icon before the transaction, and only for a write that adds a row. Without this, it would repeat that logic beside the approval, the review and the re-check, and keep all of them in step with the writes.
The write is now a plan, decided before anything is stored.
approvalWriteandrecheckWritedecide on the loaded manifest and turn that decision into a write: nothing, an existing row approved, or a new row that becomes the approved manifest or the pending one. A new row's write carries everything the row needs, the loaded manifest included, so work for that row can be added to the plan. Onestorecarries out a write while the installation is unchanged, in place ofstoreApprovalandstoreRecheck.The planners are pure functions beside
decideOnManifest, each a switch over every decision kind, andstoreis a switch over every write kind, so a new kind of either fails the build. An approval's write can't be one that leaves its manifest pending, so the staff history can't record a manifest as approved while the installation is still suspended.A
use_pendingdecision now carries the ID of the pending manifest it matched, so an approval no longer needs a fallback for a pending manifest it can't find.Nothing changes in what's stored.
Where the write is planned
Everything up to the plan is read and decided before the transaction opens. Inside it,
storeonly checks the installation is unchanged and carries out the plan, so anything a new row needs can be prepared in between.What each decision writes
The decision kinds are the re-check's outcomes. An approval makes the reviewed manifest the approved one whatever changed, so it reuses the row that's already there when it can and otherwise adds an
approvalrow.unchangednothingnothinguse_pendingapprove_existing, the pending rownothingapplyadd_approved, anapprovalrowadd_approved, anupdaterowresumeapprove_existing, the approved row, when nothing differs from it; otherwiseadd_approved, anapprovalrowadd_approved, aresumerowsuspendadd_approved, anapprovalrow that needed approvaladd_pending, anupdaterow that needs approval