✨ Added editor presence indicator behind editorPresence labs flag#28230
Open
renatoworks wants to merge 11 commits into
Open
✨ Added editor presence indicator behind editorPresence labs flag#28230renatoworks wants to merge 11 commits into
renatoworks wants to merge 11 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request implements real-time editor presence tracking: it adds an Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
ghost/admin/app/styles/components/presence-avatars.css (1)
37-39: ⚡ Quick winConsider using CSS variables for avatar colors to support dark mode.
The hardcoded background (
#738a94) and border (#fff) colors may not adapt well to the Ghost admin dark theme. If the presence avatars should be theme-aware, consider replacing these with CSS variables.💡 Example using CSS variables
.gh-presence-avatar { position: relative; border-radius: 50%; - background: `#738a94`; + background: var(--midlightgrey); color: `#fff`; - border: 2px solid `#fff`; + border: 2px solid var(--white); display: inline-flex;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/admin/app/styles/components/presence-avatars.css` around lines 37 - 39, Replace the hardcoded avatar colors in the presence avatars rule (the background: `#738a94`; color: `#fff`; border: 2px solid `#fff`; declarations) with theme-aware CSS variables (e.g. use existing Ghost theme vars or create --presence-avatar-bg, --presence-avatar-foreground, --presence-avatar-border) so the avatars respect dark mode; update the CSS rule that sets these properties to fallback to the current hex values only as defaults (var(--presence-avatar-bg, `#738a94`), etc.) and ensure variables are defined in the root or theme-specific selectors so dark and light themes can override them.ghost/admin/app/templates/lexical-editor.hbs (1)
49-51: 💤 Low valueConsider gating presence avatars on
{{#unlessthis.post.isNew}}.The presence avatars component is rendered for new posts, but there won't be any presence data until the post is saved with a real ID. For consistency with the publish buttons section (line 53), you might want to wrap the presence avatars in
{{#unlessthis.post.isNew}}.♻️ Suggested conditional wrapper
<div class="gh-editor-header-right"> + {{`#unless` this.post.isNew}} {{`#if` (feature 'editorPresence')}} <GhPresenceAvatars `@postId`={{this.post.id}} /> {{/if}} + {{/unless}} <section class="gh-editor-publish-buttons">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/admin/app/templates/lexical-editor.hbs` around lines 49 - 51, Wrap the GhPresenceAvatars rendering so it only appears for persisted posts: keep the existing feature check (feature 'editorPresence') but also guard with {{`#unless` this.post.isNew}} so GhPresenceAvatars `@postId`={{this.post.id}} is not rendered for new posts without an ID; update the block around the GhPresenceAvatars invocation accordingly (use this.post.isNew and GhPresenceAvatars as the referenced symbols).ghost/admin/app/services/presence.js (1)
110-123: 💤 Low valueConsider checking the feature flag before sending enter requests.
When
editorPresenceis disabled,start()correctly skips EventSource initialization (line 35-37), butenterPost()still attempts to send the POST request. The server will return 404, and the client logs a warning. While benign, checkingthis.feature.get('editorPresence')here would avoid unnecessary requests and console warnings when the feature is off.♻️ Optional optimization
enterPost(postId) { if (!postId) { return; } + if (!this.feature.get('editorPresence')) { + return; + } if (this._initFailed) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/admin/app/services/presence.js` around lines 110 - 123, enterPost currently calls _sendEnter even when editorPresence is disabled; add a guard that checks this.feature.get('editorPresence') (same flag checked in start()) and return early if false so we don't make POSTs when the feature is off. Update the enterPost method (and keep existing _initFailed, _currentPostId and leavePost behavior) to check the feature flag before setting _currentPostId/_sendEnter to avoid unnecessary requests and warnings.ghost/admin/tests/unit/services/presence-test.js (1)
1-55: ⚡ Quick winConsider expanding test coverage for message handling and lifecycle.
The current tests verify the labs flag gates EventSource construction, which is the critical behavior. However, the test suite does not cover:
- Message parsing (snapshot vs post events)
enterPost/leavePostbehavior and post-switching logic- Reconnection and error handling (onopen re-enter, onerror state transitions)
usersForPostfiltering and reactivity- Pagehide cleanup
While the PR includes 44 tests total across server and client, expanding client-side unit tests would increase confidence in the service's message-handling and lifecycle logic without requiring a running backend.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/admin/tests/unit/services/presence-test.js` around lines 1 - 55, Add unit tests that exercise the presence service's message-handling and lifecycle: stub feature.get('editorPresence') true, use the EventSource spy to simulate incoming SSE events by invoking the constructed instance's onmessage with both "snapshot" and "post" payload shapes to assert that enterPost and leavePost update service._source state and usersForPost filtering correctly; simulate onopen and onerror to verify reconnection/state transitions and that onopen re-enters the current post, and call service.stop() and simulate a pagehide event to assert _beforeUnloadHandler cleanup and EventSource.close() were called. Target symbols: start, stop, _source, _beforeUnloadHandler, enterPost, leavePost, usersForPost, and the EventSource instance's onmessage/onopen/onerror handlers when writing these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ghost/core/core/server/web/api/endpoints/admin/lib/presence-enter.js`:
- Around line 36-39: The catch block around models.Post.findOne collapses all
lookup failures into a 403; update the error handling in presence-enter.js so
that you only return res.status(403) for permission-related or "not found"
errors (e.g., check error.name/message or the lookup result) and for other
errors (transient DB/network errors, unexpected exceptions from
models.Post.findOne) log the error and return res.status(500). Specifically, in
the try/catch surrounding models.Post.findOne, inspect the caught err (or the
absence of a post) to distinguish permission/not-found cases from server errors,
call processLogger.error or similar to record non-permission exceptions, and
respond with 500 for those cases while preserving the 403 response only for
genuine permission failures.
In `@ghost/core/core/server/web/api/endpoints/admin/routes.js`:
- Around line 32-34: The three new admin routes (presenceStream, presenceEnter,
presenceLeave) are unthrottled; add a lightweight per-user/session/IP
rate-limiter middleware and attach it to each route before the handlers.
Implement or reuse a limiter (e.g., createRateLimiter or rateLimit middleware)
that keys by authenticated actor (fallback to session/IP), configures low-rate
rules for SSE connections and tighter limits for POST enter/leave, and returns
429 when exceeded; then update the router lines that currently use
mw.authAdminApi to insert this limiter (e.g., mw.authAdminApi, rateLimiter,
presenceStream / presenceEnter / presenceLeave) so throttling is enforced per
actor.
---
Nitpick comments:
In `@ghost/admin/app/services/presence.js`:
- Around line 110-123: enterPost currently calls _sendEnter even when
editorPresence is disabled; add a guard that checks
this.feature.get('editorPresence') (same flag checked in start()) and return
early if false so we don't make POSTs when the feature is off. Update the
enterPost method (and keep existing _initFailed, _currentPostId and leavePost
behavior) to check the feature flag before setting _currentPostId/_sendEnter to
avoid unnecessary requests and warnings.
In `@ghost/admin/app/styles/components/presence-avatars.css`:
- Around line 37-39: Replace the hardcoded avatar colors in the presence avatars
rule (the background: `#738a94`; color: `#fff`; border: 2px solid `#fff`;
declarations) with theme-aware CSS variables (e.g. use existing Ghost theme vars
or create --presence-avatar-bg, --presence-avatar-foreground,
--presence-avatar-border) so the avatars respect dark mode; update the CSS rule
that sets these properties to fallback to the current hex values only as
defaults (var(--presence-avatar-bg, `#738a94`), etc.) and ensure variables are
defined in the root or theme-specific selectors so dark and light themes can
override them.
In `@ghost/admin/app/templates/lexical-editor.hbs`:
- Around line 49-51: Wrap the GhPresenceAvatars rendering so it only appears for
persisted posts: keep the existing feature check (feature 'editorPresence') but
also guard with {{`#unless` this.post.isNew}} so GhPresenceAvatars
`@postId`={{this.post.id}} is not rendered for new posts without an ID; update the
block around the GhPresenceAvatars invocation accordingly (use this.post.isNew
and GhPresenceAvatars as the referenced symbols).
In `@ghost/admin/tests/unit/services/presence-test.js`:
- Around line 1-55: Add unit tests that exercise the presence service's
message-handling and lifecycle: stub feature.get('editorPresence') true, use the
EventSource spy to simulate incoming SSE events by invoking the constructed
instance's onmessage with both "snapshot" and "post" payload shapes to assert
that enterPost and leavePost update service._source state and usersForPost
filtering correctly; simulate onopen and onerror to verify reconnection/state
transitions and that onopen re-enters the current post, and call service.stop()
and simulate a pagehide event to assert _beforeUnloadHandler cleanup and
EventSource.close() were called. Target symbols: start, stop, _source,
_beforeUnloadHandler, enterPost, leavePost, usersForPost, and the EventSource
instance's onmessage/onopen/onerror handlers when writing these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f9c817a-cad9-4e53-b2d0-b17892f92224
📒 Files selected for processing (26)
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxghost/admin/app/components/gh-presence-avatars.hbsghost/admin/app/components/gh-presence-avatars.jsghost/admin/app/components/posts-list/list-item-analytics.hbsghost/admin/app/routes/lexical-editor/edit.jsghost/admin/app/services/feature.jsghost/admin/app/services/presence.jsghost/admin/app/services/session.jsghost/admin/app/styles/app.cssghost/admin/app/styles/components/presence-avatars.cssghost/admin/app/styles/layouts/editor.cssghost/admin/app/templates/lexical-editor.hbsghost/admin/tests/unit/services/presence-test.jsghost/core/core/server/api/endpoints/posts.jsghost/core/core/server/services/post-presence/index.jsghost/core/core/server/services/post-presence/post-presence-service.jsghost/core/core/server/services/post-presence/presence-permissions.jsghost/core/core/server/web/api/endpoints/admin/lib/presence-enter.jsghost/core/core/server/web/api/endpoints/admin/lib/presence-leave.jsghost/core/core/server/web/api/endpoints/admin/lib/presence-stream.jsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/core/shared/labs.jsghost/core/test/unit/server/services/post-presence/labs-gate.test.jsghost/core/test/unit/server/services/post-presence/post-presence-service.test.jsghost/core/test/unit/server/services/post-presence/presence-resilience.test.jsghost/core/test/unit/server/services/post-presence/presence-security.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/web/shared/middleware/api/spam-prevention.js (1)
566-578:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing config reloads for three spam limiters.
The
reset()function reloads most spam config variables but is missing reloads forspamWebmentionsBlock,spamEmailPreviewBlock, andspamPresenceBlock. Whenreset()is called (typically in tests or during config hot-reload), these three limiters will continue using stale configuration values instead of picking up the refreshed config.🔧 Proposed fix
spamContentApiKey = spam.content_api_key || {}; spamOtcVerificationEnumeration = spam.otc_verification_enumeration || {}; spamOtcVerification = spam.otc_verification || {}; + spamWebmentionsBlock = spam.webmentions_block || {}; + spamEmailPreviewBlock = spam.email_preview_block || {}; + spamPresenceBlock = spam.presence_block || {}; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/web/shared/middleware/api/spam-prevention.js` around lines 566 - 578, The reset() function reloads many spam config variables but misses reassigning spamWebmentionsBlock, spamEmailPreviewBlock, and spamPresenceBlock, leaving them with stale values; update reset() (in spam-prevention.js) to re-read the spam config and assign these three variables just like the others (e.g., spamWebmentionsBlock = spam.webmentions_block || {}; spamEmailPreviewBlock = spam.email_preview_block || {}; spamPresenceBlock = spam.presence_block || {}) so they pick up hot-reloaded configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ghost/core/core/server/web/shared/middleware/api/spam-prevention.js`:
- Around line 566-578: The reset() function reloads many spam config variables
but misses reassigning spamWebmentionsBlock, spamEmailPreviewBlock, and
spamPresenceBlock, leaving them with stale values; update reset() (in
spam-prevention.js) to re-read the spam config and assign these three variables
just like the others (e.g., spamWebmentionsBlock = spam.webmentions_block || {};
spamEmailPreviewBlock = spam.email_preview_block || {}; spamPresenceBlock =
spam.presence_block || {}) so they pick up hot-reloaded configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a5bb208b-9701-4e2c-9b39-525cba5bd06a
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
ghost/core/core/server/web/api/endpoints/admin/lib/presence-enter.jsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/core/server/web/shared/middleware/api/spam-prevention.jsghost/core/core/server/web/shared/middleware/brute.jsghost/core/test/unit/server/services/post-presence/presence-security.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/core/server/web/api/endpoints/admin/routes.js
- ghost/core/core/server/web/api/endpoints/admin/lib/presence-enter.js
- ghost/core/test/unit/server/services/post-presence/presence-security.test.js
renatoworks
force-pushed
the
feat/presence-indicator
branch
2 times, most recently
from
9bd0670 to
ec9c1cd
Compare
Contributor
|
Actionable comments posted: 0 |
1 similar comment
Contributor
|
Actionable comments posted: 0 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #28230 +/- ##
==========================================
- Coverage 73.63% 73.57% -0.06%
==========================================
Files 1536 1544 +8
Lines 130745 131633 +888
Branches 15640 15757 +117
==========================================
+ Hits 96270 96850 +580
- Misses 33509 33781 +272
- Partials 966 1002 +36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/web/shared/middleware/api/spam-prevention.js (1)
606-618:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh presence spam config values during
reset().
reset()clearspresenceBlockInstanceandpresenceIpBlockInstance, but it never reloadsspamPresenceBlock/spamPresenceIpBlockfromconfig.get('spam'). After reset, recreated limiters still use stale settings.Suggested patch
spamMemberLogin = spam.member_login || {}; spamContentApiKey = spam.content_api_key || {}; + spamWebmentionsBlock = spam.webmentions_block || {}; + spamEmailPreviewBlock = spam.email_preview_block || {}; + spamPresenceBlock = spam.presence_block || {}; + spamPresenceIpBlock = spam.presence_ip_block || {}; spamOtcVerificationEnumeration = spam.otc_verification_enumeration || {}; spamOtcVerification = spam.otc_verification || {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/web/shared/middleware/api/spam-prevention.js` around lines 606 - 618, reset() currently clears presenceBlockInstance and presenceIpBlockInstance but doesn't reload spamPresenceBlock and spamPresenceIpBlock from config, causing recreated limiters to use stale settings; update reset() to re-read config.get('spam') and reassign spamPresenceBlock and spamPresenceIpBlock (similar to how spam, spamPrivateBlock, etc. are initialized) so the presence limiters are rebuilt with fresh config values when presenceBlockInstance/presenceIpBlockInstance are recreated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ghost/core/core/server/web/shared/middleware/api/spam-prevention.js`:
- Around line 606-618: reset() currently clears presenceBlockInstance and
presenceIpBlockInstance but doesn't reload spamPresenceBlock and
spamPresenceIpBlock from config, causing recreated limiters to use stale
settings; update reset() to re-read config.get('spam') and reassign
spamPresenceBlock and spamPresenceIpBlock (similar to how spam,
spamPrivateBlock, etc. are initialized) so the presence limiters are rebuilt
with fresh config values when presenceBlockInstance/presenceIpBlockInstance are
recreated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad1f1db2-d282-4a7e-ac5f-a91a441367db
📒 Files selected for processing (3)
ghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/core/server/web/shared/middleware/api/spam-prevention.jsghost/core/core/server/web/shared/middleware/brute.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/web/api/endpoints/admin/routes.js
Shows avatars of staff currently editing a post in the editor header and on each post-list row. SSE-based, gated by the editorPresence labs flag. State and event bus live in-process on PostPresenceService — Ghost(Pro) is single-tenant per site so no Redis or cross-process plumbing is needed. Avatars fade (idle) at 90s of API silence and are removed at 180s. Explicit enter/leave POSTs handle in-app navigation; a pagehide beacon (with fetch keepalive fallback) handles tab close. TTL is the safety net. The cleanup timer starts lazily on the first mark, so a process with no active editors does no presence work.
Follow-up on 1d05183 based on a multi-agent code review pass. No behavior change; tests come in a separate commit. Observability: - presence-stream: sendComment logs on write failure (parity with sendEvent) - presence-stream: listen for res 'close' alongside req close/error - service (server): _publish warn includes postId - service (admin): EventSource init failure sets _initFailed; enterPost skips the POST instead of writing to a server we can't render from - service (admin): onerror counts consecutive CONNECTING failures and logs once after three so reconnect storms are debuggable - service (admin): malformed-payload warn includes the caught exception Wire contract: - _toWireUser strips lastSeen from snapshot + post events (internal sweep timestamp no longer leaks) - new PresenceUserView + PresenceSnapshotEvent typedefs document the full wire shape - cross-reference comments between server PRESENCE_EVENT_TYPES and admin EVENT_TYPE_* so the contract cannot drift silently Misc: - markPostPresence param renamed model → postDto (editPost returns the JSON dto, not a Bookshelf model) - trimmed restate-the-method JSDoc on subscribe, leave, snapshot, _cleanupAll, _sweep, presence-enter/leave headers, admin service - dropped EDITOR-PRESENCE.md reference from in-file comment (doc is intentionally untracked)
Covers the algorithmically tricky paths where a regression would be silent and hard to spot in manual testing: - constructor idleMs < ttlMs invariant - mark publishes on new entry; silent on already-active heartbeat; re-publishes on idle→active transition - mark ignores invalid input without publishing - cleanup timer starts lazily on first mark - _sweep flips active→idle past idleMs and removes past ttlMs - _cleanupAll keeps iterating past a throwing subscriber - leave is a no-op for unknown post/user; emits empty users when removing the last user - snapshot drops stale entries and strips lastSeen from the wire shape (consumer-visible contract) UI surface, SSE handler, enter/leave endpoints, and the admin service are intentionally not covered — those regressions surface visually the first time the feature is exercised.
Locks the editorPresence flag-off behaviour so the feature cannot silently activate if a future refactor breaks one of the gates. Server (labs-gate.test.js): - markPostPresence does not call postPresence.mark when flag is off - markPostPresence calls postPresence.mark with the expected shape when flag is on - presence-enter handler 404s and does not mark when flag is off - presence-leave handler 404s and does not leave when flag is off - presence-stream handler 404s and never subscribes when flag is off Admin (presence-test.js): - start() does not construct EventSource and does not register the pagehide handler when feature.editorPresence is false - start() does construct EventSource when feature.editorPresence is true
Covers the two highest-severity failure modes flagged in review: markPostPresence never breaks the parent /posts API call - postPresence.mark throwing is swallowed - labs.isSet throwing is swallowed - frame.user.get throwing is swallowed presence-stream cleans up listeners on every disconnect path - req 'close' triggers unsubscribe - res 'close' triggers unsubscribe - cleanup is idempotent across req/res close+error
Addresses an adversarial review pass on the labs-flagged presence feature. No user-visible change with the flag off. Security (blockers): - SSE per-subscriber filtering. The stream now carries authorIds per post; the SSE handler captures the subscriber's role and forwards events only when the subscriber is elevated (Owner / Administrator / Super Editor / Editor) OR appears in the post's authorIds. Author and Contributor no longer receive a feed of post IDs and activity for posts they cannot read. - /presence/posts/:id/enter looks up the post through the user's context (Ghost's Post.permissible) so an unauthorized user cannot inject their avatar onto posts they have no permission for. The post's authorIds are passed through to the service. - markPostPresence skips presence when the edit context is a staff API token (Zapier/Make/scripts) so automation runs no longer trigger phantom "currently editing" avatars. Robustness: - leave() and _cleanupAll() publish the empty-users event BEFORE clearing the postContext, so non-elevated subscribers in the authorIds list receive the clear and remove the stale avatar instead of waiting for TTL. Regression tests lock this in. - Session middleware loads req.user without roles, so the SSE handler now force-loads them at connect (user.load(['roles'])) before resolving elevated access. UX: - First-name tooltip collisions (two "Alex"es) now disambiguate with the last initial — "Alex S." / "Alex J." - Admin presence service re-sends the current enter on SSE reconnect (EventSource onopen) so peers don't wait for the next autosave heartbeat to see the user reappear. Cleanup: - Dropped the dead service.PRESENCE_EVENT_TYPES assignment on the singleton — nobody read it. - Fixed dangling refs in EDITOR-PRESENCE.md to docs that live on the poc/editor-multiplayer spike branch. Tests (44 total, +18 in this commit): - presence-security.test.js: per-subscriber filter; authorIds emitted on every event and in snapshot; SSE handler filters the snapshot AND per-event forwarding; enter handler authorizes via Post.findOne; markPostPresence skips api_key contexts and passes authorIds through; publish-before-delete ordering for both leave() and _cleanupAll() (the asymmetric-leave bug).
- Updated config.test.js snapshot to include editorPresence in the labs map (e2e admin acceptance test fix) - presence-enter: distinguish permission errors (still 403) from transient lookup failures (log + 204, preserving best-effort semantics). New test covers the 204 path. - Added per-staff-user rate limiter on /presence/stream, /presence/posts/:id/enter, and /presence/posts/:id/leave via a new shared.middleware.brute.presenceLimiter. Backed by a new presenceBlock ExpressBrute instance with permissive defaults (600 requests / hour / user) so legitimate editor navigation never hits the limit, but a runaway client gets capped.
Addresses CodeQL js/missing-rate-limiting on /presence/stream, /enter, and /leave. The per-user limiter remains in place (after auth, keyed by req.user.id, 600/hr/user) and now sits behind a per-IP limiter applied before mw.authAdminApi (6000/hr/IP). Defense in depth: unauthenticated DoS attempts are now bounded before they hit the auth layer; legitimate editorial use is well below both ceilings (~60 requests/hour per active editor). Both limits are overridable via config: spam.presence_ip_block.freeRetries spam.presence_block.freeRetries
Brings E2E coverage back above the codecov 0.2% threshold by exercising the new presence routes end-to-end. Covers: - All 3 routes return 403 to unauthenticated requests - /enter returns 404 for non-existent posts - /enter returns 204 for a valid post (Owner) - /leave returns 204 regardless of state (idempotent) - /enter and /leave work for an Author on their own post These complement the existing 45 unit tests by exercising the real Express middleware chain, route registration, and the Post.findOne permission lookup that's hard to test in isolation.
Three more acceptance tests covering the labs flag gate at the HTTP layer. With editorPresence disabled via mockManager, each of the three routes returns 404 — verifies the labs.isSet branch in each handler. Brings the E2E suite for presence to 12 tests, addressing the codecov project coverage gate.
The auto-merge between this branch and main was producing a duplicated mapping key (jiti@2.7.0) because several recent dependency-bump commits on main each touched the lockfile in overlapping ways. Regenerated cleanly with pnpm install --lockfile-only.
renatoworks
force-pushed
the
feat/presence-indicator
branch
from
fe012b7 to
3d163ca
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
2 participants
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.
Describe the problem you are solving
This PR adds an editor presence indicator so staff can see at a glance who else has a post open. The two concrete pains it addresses:
/ghost#/postsrows.The feature is gated end-to-end by the
editorPresenceprivate labs flag (under Settings → Labs → Private features, visible when developer experiments is on). With the flag off, the SSE endpoint 404s, the admin never opens an EventSource, and the Glimmer component renders nothing.Transport is server-sent events. State (a
Map<postId, Map<userId, Entry>>) and the event bus (an inlineEventEmitter) live in-process on a singletonPostPresenceService— Ghost(Pro) is single-tenant per site, so no Redis or cross-process plumbing is needed for the foreseeable future.Changelog for devs
PostPresenceService(ghost/core/core/server/services/post-presence/) — in-process Map + EventEmitter, lazy cleanup timer, idle/TTL sweep, publish-on-change dedup so autosaves don't fan out eventsGET /ghost/api/admin/presence/streamwith snapshot-on-connect, comment keepalive, idempotent cleanup on req/res close/errorauthorIds; the SSE handler captures the subscriber's role at connect (req.user.load(['roles'])) and forwards events only when the subscriber is elevated (Owner / Administrator / Super Editor / Editor) OR appears in the post'sauthorIds. Author and Contributor never receive a feed of post IDs they can't read.POST /presence/posts/:id/enterandPOST /presence/posts/:id/leave(the latter is thepagehidebeacon target with afetch keepalivefallback)./enterlooks up the post through the user's context (Post.findOne({...}, {context: {user}})) so an unauthorized user can't inject their avatar onto posts they can't read.markPostPresence(frame, postDto)side-effect to theposts.editquery handler (autosave heartbeat).posts.readdeliberately does NOT mark — analytics views and search lookups don't count as "editing". Skipped when the edit context is a staff API token (Zapier/Make/scripts) so automation doesn't trigger phantom "currently editing" avatars.editorPresencetoPRIVATE_FEATURESincore/shared/labs.jsPRESENCE_EVENT_TYPESconstants on both server and admin with cross-reference comments so the wire format can't drift silentlyPresenceEntry(internal),PresenceUserView(on the wire),PresencePostEvent,PresenceSnapshotEventpresenceservice that subscribes viaEventSource, tracks a per-post usersMap, handlespagehidebeacons, re-sendsenteronEventSource.onopen(initial connect AND after auto-reconnect), logs terminal SSE closures and reconnect storms<GhPresenceAvatars>Glimmer component using Ghost's standard<GhTooltip>; rendered inlexical-editor.hbs(md, cap=2) andposts-list/list-item-analytics.hbs(sm, cap=3) with+Noverflow chip. First-name collisions disambiguate with last initial ("Alex S." / "Alex J.").apps/admin-x-settings/.../labs/private-features.tsxChangelog for customers
editorPresenceprivate labs flag.Notes
Test coverage (44 tests):
post-presence-service.test.js(13) — service invariants: sweep transitions, mark dedup, lazy timer start, leave semantics, snapshot freshness,lastSeenstrip from the wire shapelabs-gate.test.js(5) —markPostPresence+ all 3 presence handlers do nothing when the flag is offpresence-resilience.test.js(6) — worst-case failure modes:markPostPresenceswallows errors frommark/labs.isSet/frame.user.get; SSE handler unsubscribes from the bus on req/res close, idempotent across all four signal pathspresence-security.test.js(18) — per-subscriber filter unit tests; serviceauthorIdscapture/emit/snapshot; SSE snapshot filtered for Author (does NOT see editor-only post IDs); per-event forwarding to Author (yes for own post, no for others); Editor sees all events; Contributor sees empty snapshot;/enterhandler 403s on permission denial, 404s on missing post, passes authorIds through;markPostPresenceskips api_key contexts and passes authorIds; regression tests for publish-before-delete ordering in bothleave()and_cleanupAll()(asymmetric-leave bug)presence-test.js(admin, 2) —start()opens / does not open EventSource based onfeature.editorPresenceArchitectural decisions:
POST /presence/posts/:id/{enter,leave}. SSE rides standard HTTP/cookies/CSP, gets auto-reconnect for free viaEventSource, and only pushes on actual state change. Trade-off noted: browsers cap ~6 HTTP connections per origin, so 7+ admin tabs in one browser would queue — acceptable.PostPresenceService(state + bus, transport-agnostic) andpresence-stream.js(the SSE handler) is deliberately clean: a future WS layer can subscribe to the same emitter, or Yjs's built-inawarenessprotocol can replace the service outright. The "transport" code (SSE handler + enter/leave HTTP endpoints + admin EventSource block) is ~160 lines, ~17% of the PR; the service, UI, tests, and labs flag all transfer unchanged. SeeEDITOR-PRESENCE.mdfor the full audit.userIdappears in the event'sauthorIds. The filter is applied server-side per-subscriber, so cross-role post-ID leakage isn't possible via the SSE channel.EventEmitter(not an adapter). A multi-instance follow-up is documented locally as a separate refactor if deployment shape ever changes — explicitly not on the roadmap.lastSeen(server-side sweep timestamp) via_toWireUser— client doesn't need it and it would leak per-peer activity timestamps.enterPostin the admin service, which sends a leave for the previous post before entering the new one (Ember does not firedeactivateon route reuse).leave()and_cleanupAll()publish the empty-users event before dropping the post's storedauthorIds, so non-elevated subscribers in that author list receive the clear signal and remove the stale avatar instead of waiting for TTL.Deliberate omissions:
(feature 'editorPresence')is hard to unit-test in isolationfeat/presence-conflict-bannerafter this landsCodeQL
js/missing-rate-limitingalerts (dismissed):The 3 alerts on the presence routes are false positives. Rate limiting IS in place via a two-stage limiter:
shared.middleware.brute.presenceIpLimiter, 6,000/hr/IP) — defense-in-depth against unauthenticated DoSshared.middleware.brute.presenceLimiter, 600/hr/user) — catches authenticated runaway clientsThis follows Ghost's existing convention — see
routes.js:217(/automated_emails/:id/test) androutes.js:382(/email_previews/posts/:id) which useshared.middleware.brute.previewEmailLimiterin the same shape. CodeQL's static analyzer can't trace through Ghost's two-level wrapper (brute.js→spamPrevention.X().getMiddleware()); the existing routes only escape detection because they predate the CodeQL rule. The actualCodeQL / Analyzeworkflow passes (2m36s of real analysis). Alerts dismissed accordingly.Technical debt
PostPresenceService— wire to settings if product wants them tunableeditor.cssbreakpoint where.gh-editor-back-button spanis hidden was widened from 500px to 768px to give the right-cluster room — applies to all users, not just those with the flag on (intentional, callout for design)_publishwalks the SSE subscriber list synchronously inside theposts.editrequest handler. At realistic editorial-team scale (≤100 connected tabs) the cost is ~2.5ms per save — invisible. Becomes worth fixing (wrap insetImmediate) if presence ever scales to thousands of concurrent tabs OR if heavier subscribers (metrics emitter, Redis publisher) are addedPR Scope (mark all that apply)
Checklist before requesting a review (mark all that apply)
Product update phrase: "Spotted who else is editing a post — Ghost now shows live avatars in the editor header and on the post list so editorial teams know who's working on what before they start typing."