Repository navigation
Changed the post settings sections to a narrow session port - #30755
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: 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. (6)
🧰 Additional context used📓 Path-based instructions (5)Review Admin UI for existing Shade reuse, correct component layer, semantic tokens, accessible interaction states, and whole-sentence translations.⚙️ CodeRabbit configuration file Files:
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:
Prioritise concrete correctness, security, data-integrity, compatibility, and regression risks.⚙️ CodeRabbit configuration file Files:
Type-safe boundaries: Fail only if the PR: consumes boundary data (HTTP input, external API/SDK responses, env/config, DB/filesystem reads, queue/webhook/event payloads) without validating it first — Zod by default, another format only wher...📄 CodeRabbit inference engine (Custom checks) Files:
Walkthrough
Priority: ⬇️ Low Change: Refactor Merge Risk: ⚪ Minimal · up to The editor settings render optimization has no identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 8m 28s | View ↗ |
nx run-many -t test:unit -p @tryghost/admin |
✅ Succeeded | 4m 26s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 27s | View ↗ |
nx run-many -t lint -p @tryghost/admin,ghost-mo... |
✅ Succeeded | 1m 44s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 17s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
nx run @tryghost/e2e:test:fixtures |
✅ Succeeded | 1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-09-16 17:25:44 UTC
9larsons
force-pushed
the
slars/editor-stable-session-handle
branch
from
September 14, 2026 17:10
5c6a0cb to
1226924
Compare
9larsons
force-pushed
the
slars/editor-settings-port
branch
2 times, most recently
from
September 14, 2026 17:29
57c4bd8 to
a97b8a5
Compare
no ref The sidebar handed every section the whole editing handle: about thirty members, of which the fourteen settings files use thirteen between them. The sections could reach the save dispatchers, the publish flow and the leave guard, and they were re-handed a new object whenever anything on the session moved. The panel now derives an `EditorSettingsPort` — a `Pick` of exactly what the sections read — and memoizes it over those members, so an edit the panel cannot see leaves the sections' prop alone. The binding is narrowed the same way: the sections read the title, the excerpt and the excerpt writer, so the port carries those three leaves rather than the whole `bind`, whose other members would move the port for an edit no section reads. The save engine's state stays off the port: it goes to `debouncing` on every body keystroke, so carrying it would make the port move as often as the handle does. The history pane, its only reader, takes it as its own prop. `KoenigPostEditor` is memoized to match. Every one of its props is already stable across an edit outside the body, and it mounts two composer subtrees, both of which were rebuilding on each sidebar keystroke — and on each body keystroke too, through the word count the surface holds above them.
9larsons
force-pushed
the
slars/editor-settings-port
branch
from
September 16, 2026 16:01
a97b8a5 to
a6e7f42
Compare
no ref The narrow session port only provides a useful render boundary when its consumers participate in memoization. Memoize each section and cover save-state churn so body edits no longer rebuild unrelated settings UI.
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
1 participant
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.
The post settings panel handed every section the whole
EditorSessionHandle— about thirty members, including the save dispatchers, the publish flow, the reload and the leave guard. The fourteen files undersettings/use thirteen of them between them, and because the handle moves whenever anything on the session moves, the sections were re-handed a new object for edits they cannot see.The port
settings/editor-settings-port.tsaddsEditorSettingsPort, aPick<EditorSessionHandle, …>of exactly what the sections read, plususeEditorSettingsPort()which memoizes it over those members. The panel derives it once and passes it down; every section'ssessionprop keeps its name and is retyped to the port. No behaviour changes.settingsuse-settings-fieldeditSettingsstageSettingsuse-settings-fieldcommitSettingsuse-settings-field, the panel's excerpt rowbindbind.title), the panel's excerpt row (bind.excerpt,bind.onExcerptChange)loadedRecordslugcreatedIddisposeeditSlugrestoreRevisionpublishTimeeditPublishedAtThe last six are each a single section's, and they stay in the pick rather than being threaded as extra props.
bindis narrowed the same way the handle is. The sections read three of its nine members, so the port carriesbind: Pick<EditorSessionBinding, 'title' | 'excerpt' | 'onExcerptChange'>, built in its own memo over those three values. Carrying the wholebindwould have moved the port forinitialLexicaland the body change handlers, which no section reads.stateis not on the port either. The save engine publishes{kind: 'debouncing'}on the keystroke that schedules an autosave, sostatemoves on every body keystroke; carrying it would make the port churn exactly as often as the handle and leave the sections no better off. Its one reader,PostHistorySection, which uses it to say a restore was refused because the session expired, now takes it as its ownstateprop.No section is memoized yet, so the port's payoff today is the type narrowing plus the callbacks the sections build over it —
SocialCardSection'seditImage, theonChangebehinduseImageFieldUpload, isuseCallback([network.imageKey, session])and now holds across a body keystroke instead of being rebuilt on each one. Memoizing the sections is the follow-on the port makes possible; it is not in this change.memo(KoenigPostEditor)KoenigPostEditormounts two composer subtrees and re-rendered on every parent render. That is every sidebar keystroke, and also every keystroke in the body itself:PostEditorholds the word count in its own state and passessetWordCountdown asonWordCountChange, so each word the writer types re-rendered both composer subtrees from above. The body case is the larger of the two.All twelve of its props are already referentially stable across a render of
PostEditor:cardConfiguseMemoover the live visibility and show-title values ineditor-screen.tsxinitialLexicalstring | nulloff the session bindingplaceholderpostType; a fresh but equal stringdarkModeuseFocusContext()cursorDidExitAtTopuseCallback([])inpost-editor.tsx(focusExcerpt/focusTitle)registerAPIuseCallbackinpost-editor.tsxover the screen's own (absent) callbackregisterSecondaryAPIonChangebind.onLexicalChange,useCallback([session])onSecondaryChangebind.onSecondaryChange,useCallback([session])onSecondaryErrorbind.onSecondaryError,useCallback([session])onWordCountChangeuseStatesetter inpost-editor.tsxonTkCountChangeuseStatesetter inpost-editor.tsxEvidence
koenig-post-editor.test.tsxrenders the realPostEditor— the component that builds those props — with the Koenig loader stubbed, and counts composer renders while the post title is typed into. The title isPostEditor's own prop, so a keystroke re-renders the whole surface. Two keystrokes:memoremoved fromKoenigPostEditor: 6 —expected "vi.fn()" to be called 2 times, but got 6 times, two more per keystrokeonTkCountChange={(count) => setBodyTkCount(count)}— 6, the same failureThe test then re-renders with a different
postType, which changesplaceholder, a prop the body does read, and asserts the count moves to 4, so a passing run is the memo holding renders back rather than the probe missing them. Nothing test-only was added to production code; the seam is the existing loader module.editor-settings-port.test.tsxdrives the realuseEditorSessionand asserts the port survives a body keystroke: afterbind.onLexicalChange, the handle is a different object and the session reads dirty, while the port is the same object. Staging a settings field replaces the port —settingsis on it — but notport.bind, and a title edit replaces both, since the sections read the title.Verified
Run one at a time, from
apps/admin, after committing:pnpm exec vitest run src/editor— 1178 passed, 63 files, 17spnpm --filter @tryghost/admin lint— 0 errors (41 pre-existing warnings elsewhere in the app)pnpm --filter @tryghost/admin exec tsc -b— cleanpnpm exec vitest run -c vitest.acceptance.config.ts --maxWorkers=2 src/editor— 385 passed, 25 files, 66s