feat(client): rewrite as a unified, session-scoped rssCloud/WebSub test harness#39
Merged
Merged
Conversation
…st harness Replaces the two-box, dev-only UI with one unified control (rssCloud REST/XML-RPC/WebSub), async JSON actions under /s/:sessionId/actions/* (no page navigation), feed-URL discovery, and a live combined incoming+outgoing traffic log via @andrewshell/socklog over a per-session WebSocket. Enables testing live/third-party hubs, not just a local one. Because it's now meant to run as a public utility rather than a local dev-only tool: - every outbound call goes through an SSRF-guarded fetch (reusing @rsscloud/core's createSafeFetch), refusing loopback/private targets by default - state is session-scoped (Map<sessionId, SessionState>) instead of global, so concurrent users' logs/feeds/subscriptions never cross - callback/feed routes 404 once a session's been idle past SESSION_CALLBACK_IDLE_MS, and idle sessions are fully evicted after SESSION_GC_IDLE_MS - both suspended while a live socklog connection is attached, so a page left open overnight monitoring a feed never goes dark or gets GC'd - /actions/ping and /actions/publish structurally accept only a feedName, never a feedUrl, so this harness can never be made to ping/publish a feed it doesn't itself serve - adds dotenv + Docker support (apps/client/Dockerfile, .env.example, a paired service in examples/dockge/compose.yaml) The UI reuses apps/server's stylesheet and the control box is laid out as labeled fieldsets; the socklog panel has no fixed height and grows with content, matching apps/server's own log page. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe client app is refactored into a session-scoped test harness with external config, guarded outbound fetch, feed discovery helpers, per-session state and log streaming, session-routed protocol actions, browser UI updates, and matching Docker, compose, and README changes. ChangesClient Test Harness Refactor
Estimated code review effort: 4 (Complex) | ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
apps/client/session-sockets.js (1)
39-56: 📐 Maintainability & Code Quality | 🔵 TrivialLog truncation logic lives outside the module that owns the data structure.
broadcast()directly mutatessession.requestLog(unshift/pop withMAX_LOG_ENTRIES), even though the array and its lifecycle are otherwise fully owned bysession-store.js. Any future caller that needs to append to the log without going throughbroadcast(or a change to the cap) has to know about this hidden coupling. Consider exposing anappendLog(id, entry)method on the session store that owns the truncation policy, and havebroadcastcall it.wss.on('connection', ...) is the canonical way listeners are notified after
handleUpgrade🤖 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 `@apps/client/session-sockets.js` around lines 39 - 56, Move the request log mutation and truncation policy out of broadcast() and into the session store that owns the session state. Add an appendLog(sessionId, entry) method on the session-store.js API to handle unshift/pop and MAX_LOG_ENTRIES management, then update broadcast() to call that method before sending the websocket message. Keep the existing ws.send flow in broadcast(), but remove direct session.requestLog manipulation so log lifecycle is centralized and reusable.apps/client/lib/discover.js (1)
45-48: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHardcoded
atom:linkprefix misses non-standard namespace aliases.Detection of WebSub hub links in RSS relies on the literal tag name
atom:link, which only matches feeds using exactly that namespace prefix. Feeds declaring the Atom namespace under a different prefix (e.g.<a:link rel="hub">) will silently reportwebSub: null. Low risk for a discovery/UX helper, but worth a comment noting the assumption.🤖 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 `@apps/client/lib/discover.js` around lines 45 - 48, The WebSub hub lookup in discover.js is hardcoded to the atom:link prefix, so feeds using a different namespace alias will be missed. Update the hub-link detection around channel?.['atom:link'] and findHubLink to either handle namespace aliases more generically or add an explicit comment documenting that this helper only supports the atom:link prefix assumption.apps/client/public/app.js (1)
21-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
postActionfailures are silently swallowed.None of the click handlers catch rejections from
postAction. A network error, timeout, or non-JSON error response (res.json()throwing on an HTML error page) results in an unhandled promise rejection with zero user-visible feedback — the button just appears to do nothing.♻️ Proposed fix
async function postAction(action, fields) { - const res = await fetch(`/s/${sessionId}/actions/${action}`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(fields) - }); - return res.json(); + try { + const res = await fetch(`/s/${sessionId}/actions/${action}`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(fields) + }); + return await res.json(); + } catch (err) { + console.error(`Action "${action}" failed:`, err); + return { error: err.message }; + } }Also applies to: 81-130
🤖 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 `@apps/client/public/app.js` around lines 21 - 28, The postAction flow currently lets fetch/res.json failures escape, so click handlers end up with unhandled rejections and no user feedback. Update postAction to catch network/parse errors and surface a clear failure state or message, then make the relevant click handlers that call postAction (including the ones in the affected action handlers) handle rejected promises and notify the user instead of silently doing nothing.apps/client/client.test.js (1)
7-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test that actions refresh callback liveness.
These tests only age a session from creation time. Add a case that advances time, performs an outbound action, advances less than
sessionCallbackIdleMs, and verifies/notifystill returns 200; that would catch missingtouchOutgoing()wiring.🤖 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 `@apps/client/client.test.js` around lines 7 - 25, Add a regression test in client.test.js to cover callback liveness refresh after outbound activity: after creating the session with createApp/createSessionStore, advance currentTime, perform an outbound action that should invoke touchOutgoing(), then advance time by less than sessionCallbackIdleMs and verify GET /s/:id/notify still returns 200. Use the existing helpers and route patterns around createSessionStore, createApp, and the /s/idle-session/notify flow so this catches missing touchOutgoing() wiring.
🤖 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 `@apps/client/client.js`:
- Around line 447-452: The WebSub session state update in the
subscribe/unsubscribe flow is happening too early, before the hub request
finishes successfully. In the `subscribe()` and `unsubscribe()` callback logic
inside `client.js`, move the `req.session.webSubSecrets[feedUrl]` set/delete so
it only runs after the outbound hub call succeeds, and leave the session
unchanged if the call throws. Use the existing `secret`, `feedUrl`, and
`req.session.webSubSecrets` handling in those branches to locate and adjust the
mutation points.
- Around line 357-370: Refresh the session idle clock in outbound action
handlers so `requireLiveSession` does not treat active sessions as idle. Update
the `/actions/discover` route and the other outbound request paths to call
`sessionStore.touchOutgoing()` when sending a `direction: 'outgoing', phase:
'request'` event, and prefer `broadcastOutgoingRequest(...)` for those entries
so the touch happens consistently.
- Around line 711-722: The rssCloud feed generation in renderCloudFeed is
hard-coding the cloud endpoint to localhost:5337, which causes deployments to
advertise the wrong hub. Update the cloud config passed from client.js to derive
domain, port, and path from the configured hub URL (config.hubServerUrl /
hubUrl) instead of fixed values, while keeping registerProcedure and protocol
unchanged. Use the existing feed setup near renderCloudFeed and the hubUrl value
as the source of truth so the emitted <cloud> element matches the configured
rssCloud hub.
- Around line 443-447: The WebSub subscribe request logging in logAndRespond
currently records the user-provided secret in the live traffic log and
session.requestLog. Update the logging path around websub-subscribe so the
actual hub request payload still includes secret, but the logged/redacted body
replaces that field before it is persisted or displayed. Use the existing
logAndRespond flow and the websub-subscribe call site to keep the network
request unchanged while redacting only the logged entry.
In `@apps/client/config.js`:
- Around line 8-12: The getNumericConfig helper currently returns NaN for
malformed numeric env vars, which can silently break downstream uses like
setInterval, app.listen, and timeout checks. Update getNumericConfig to validate
the parsed value and fail loudly when process.env[key] is present but not a
valid number, rather than returning NaN; keep the defaultValue behavior only
when the env var is unset. Use the existing getNumericConfig function in
config.js to centralize this validation.
In `@apps/client/Dockerfile`:
- Around line 24-38: The runtime stage in the client Dockerfile still starts as
root; update the final image setup to switch to a dedicated non-root user before
the CMD runs. Add or reuse an unprivileged user in the runtime stage, make sure
the copied app files are readable/executable by that user, and set the active
user for the final image near the runtime WORKDIR/EXPOSE/CMD block so the
container launches under that account.
In `@examples/dockge/compose.yaml`:
- Around line 56-81: The CLIENT_FETCH_ALLOW_CIDRS setting in the client Compose
service is too broad and weakens the SSRF guard by allowing the full
172.16.0.0/12 private range. Narrow the allowlist in the client service so it
only covers the specific Docker subnet needed to reach the sibling rsscloud
container, and keep the rest of the compose environment unchanged; update the
client block in compose.yaml accordingly.
---
Nitpick comments:
In `@apps/client/client.test.js`:
- Around line 7-25: Add a regression test in client.test.js to cover callback
liveness refresh after outbound activity: after creating the session with
createApp/createSessionStore, advance currentTime, perform an outbound action
that should invoke touchOutgoing(), then advance time by less than
sessionCallbackIdleMs and verify GET /s/:id/notify still returns 200. Use the
existing helpers and route patterns around createSessionStore, createApp, and
the /s/idle-session/notify flow so this catches missing touchOutgoing() wiring.
In `@apps/client/lib/discover.js`:
- Around line 45-48: The WebSub hub lookup in discover.js is hardcoded to the
atom:link prefix, so feeds using a different namespace alias will be missed.
Update the hub-link detection around channel?.['atom:link'] and findHubLink to
either handle namespace aliases more generically or add an explicit comment
documenting that this helper only supports the atom:link prefix assumption.
In `@apps/client/public/app.js`:
- Around line 21-28: The postAction flow currently lets fetch/res.json failures
escape, so click handlers end up with unhandled rejections and no user feedback.
Update postAction to catch network/parse errors and surface a clear failure
state or message, then make the relevant click handlers that call postAction
(including the ones in the affected action handlers) handle rejected promises
and notify the user instead of silently doing nothing.
In `@apps/client/session-sockets.js`:
- Around line 39-56: Move the request log mutation and truncation policy out of
broadcast() and into the session store that owns the session state. Add an
appendLog(sessionId, entry) method on the session-store.js API to handle
unshift/pop and MAX_LOG_ENTRIES management, then update broadcast() to call that
method before sending the websocket message. Keep the existing ws.send flow in
broadcast(), but remove direct session.requestLog manipulation so log lifecycle
is centralized and reusable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 020b507c-1575-4e72-95d5-9543abaf2c05
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
apps/client/.env.exampleapps/client/Dockerfileapps/client/README.mdapps/client/client.jsapps/client/client.test.jsapps/client/config.jsapps/client/index.jsapps/client/lib/discover.jsapps/client/lib/discover.test.jsapps/client/lib/guarded-fetch.jsapps/client/lib/guarded-fetch.test.jsapps/client/lib/index.jsapps/client/lib/session-store.jsapps/client/lib/session-store.test.jsapps/client/package.jsonapps/client/public/app.jsapps/client/public/css/style.cssapps/client/session-sockets.jsapps/client/session-sockets.test.jsexamples/dockge/compose.yaml
- webSubSecrets is only mutated after the hub subscribe/unsubscribe call actually succeeds, not before — a failed call no longer stores a secret for a subscription that was never accepted, or drops a still-valid one - wire sessionStore.touchOutgoing() into every outbound action via a new broadcastOutgoingRequest() helper — it was never being called, so a session's idle clock never advanced past creation regardless of activity, silently defeating the "active session stays live" intent - derive the served test feed's <cloud> domain/port from config.hubServerUrl instead of hard-coded localhost:5337, so a deployment against any other hub advertises the right one - redact the WebSub secret in the broadcast/logged copy of a subscribe request; the real value still reaches the hub unchanged - config.js's getNumericConfig now fails loudly on a malformed (but present) numeric env var instead of silently producing NaN - apps/client/Dockerfile now runs as the non-root `node` user - narrow examples/dockge/compose.yaml's CLIENT_FETCH_ALLOW_CIDRS from Docker's whole default /12 range to a dedicated /24 subnet - fix .dockerignore's `.env` pattern (found while validating the Dockerfile change) to `**/.env` — the bare form only matches at the build-context root, not at any depth like .gitignore, so a real local .env could be baked into an image built from either app's Dockerfile - lib/session-store.js: add appendLog() so session-sockets.js's broadcast() no longer reaches into session.requestLog directly - lib/discover.js: document the atom: namespace-prefix assumption in hub-link detection - public/app.js: postAction() catches network/parse failures and surfaces them instead of leaving an unhandled rejection Verified with new/updated tests (82 passing), a real docker build+run (confirmed non-root, confirmed .env no longer present in the image), and a live browser pass confirming the secret redaction end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/client/lib/session-store.js (1)
47-75: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSessions with a live socket can never be evicted — unbounded memory growth risk.
isIdleandsweepboth treat any session withsockets.size > 0as permanently active, with no fallback hard TTL. For a public-facing harness, a client that opens (and keeps open) WebSocket connections to many session IDs can pin those sessions in memory indefinitely —sweep(sessionGcIdleMs)will never reclaim them, regardless ofcreatedAt. This is an available DoS vector for memory exhaustion now that the harness is meant for public use rather than local-only dev.Consider adding a hard ceiling independent of socket state (e.g. evict regardless of live sockets once
now() - session.createdAtexceeds some absolute max age), and/or a cap on total tracked sessions.💡 Illustrative approach: hard age ceiling overriding the socket exemption
- function isIdle(id, idleMs) { + function isIdle(id, idleMs, hardMaxAgeMs) { const session = sessions.get(id); if (!session) { return true; } + if (hardMaxAgeMs && now() - session.createdAt > hardMaxAgeMs) { + return true; + } if (session.sockets.size > 0) { return false; } return now() - session.lastOutgoingAt > idleMs; } - function sweep(maxIdleMs) { + function sweep(maxIdleMs, hardMaxAgeMs) { let evicted = 0; for (const [id, session] of sessions) { - if (session.sockets.size > 0) { + const hardExpired = hardMaxAgeMs && now() - session.createdAt > hardMaxAgeMs; + if (session.sockets.size > 0 && !hardExpired) { continue; } - if (now() - session.lastOutgoingAt > maxIdleMs) { + if (hardExpired || now() - session.lastOutgoingAt > maxIdleMs) { sessions.delete(id); evicted += 1; } } return evicted; }🤖 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 `@apps/client/lib/session-store.js` around lines 47 - 75, The session eviction logic in isIdle and sweep currently exempts any session with sockets.size > 0 forever, which can pin sessions in memory indefinitely. Update the session GC path to enforce an absolute max age using session.createdAt so old sessions can be evicted even if sockets remain open, and consider adding a global session cap if needed. Keep the change localized to the isIdle/sweep logic in session-store.js and preserve the existing idle-based behavior for normally inactive sessions.
🤖 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 `@apps/client/public/app.js`:
- Line 35: The `alert` call in `app.js` is being flagged by ESLint because
browser globals are not enabled. Update the client ESLint configuration to
include browser environment globals such as `window`, `alert`, and `fetch` (or
otherwise declare the browser env) so `no-undef` no longer reports `alert` as
undefined.
---
Outside diff comments:
In `@apps/client/lib/session-store.js`:
- Around line 47-75: The session eviction logic in isIdle and sweep currently
exempts any session with sockets.size > 0 forever, which can pin sessions in
memory indefinitely. Update the session GC path to enforce an absolute max age
using session.createdAt so old sessions can be evicted even if sockets remain
open, and consider adding a global session cap if needed. Keep the change
localized to the isIdle/sweep logic in session-store.js and preserve the
existing idle-based behavior for normally inactive sessions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ac3f6cb-86d6-4273-94ee-92e4cd1cc146
📒 Files selected for processing (13)
.dockerignoreapps/client/Dockerfileapps/client/client.hub-config.test.jsapps/client/client.jsapps/client/client.test.jsapps/client/config.jsapps/client/config.test.jsapps/client/lib/discover.jsapps/client/lib/session-store.jsapps/client/lib/session-store.test.jsapps/client/public/app.jsapps/client/session-sockets.jsexamples/dockge/compose.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/dockge/compose.yaml
- apps/client/config.js
- apps/client/Dockerfile
- apps/client/lib/discover.js
- apps/client/client.js
- apps/client/client.test.js
…globals - lib/session-store.js: isIdle/sweep exempted any session with a live socket from idle/GC checks unconditionally, with no ceiling — on a public deployment, simply holding the socklog WebSocket open forever would pin that session in memory indefinitely. Added a MAX_SESSION_AGE_MS (7 days) ceiling tied to session.createdAt; past that, a held-open socket falls back to the normal lastOutgoingAt idle rules instead of exempting the session outright, and sweep() once again terminates any lingering socket on eviction. - eslint.config.js: scope browser globals (window, document, alert, etc.) to apps/client/public/**/*.js instead of leaving the shared Node-context config to flag them — an editor's live ESLint integration applies the one repo-wide config regardless of what the npm lint script's glob covers, so this was real editor noise on public/app.js even though CI wasn't affected. Verified with new/updated tests (85 passing in apps/client) and a forced (non-cached) full monorepo lint/test/typecheck run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
apps/clientwith a unified control (rssCloud REST/XML-RPC/WebSub), async JSON actions under/s/:sessionId/actions/*(no page navigation), feed-URL discovery, and a live combined incoming+outgoing traffic log via@andrewshell/socklogover a per-session WebSocket.@rsscloud/core'screateSafeFetch, idle-404 + GC eviction for abandoned sessions (suspended while a live socklog connection is attached, so an overnight-monitoring tab never goes dark), and structural enforcement that/actions/ping//actions/publishcan only ever target this harness's own feed.apps/client/Dockerfile,.env.example, a paired service inexamples/dockge/compose.yaml).apps/server's stylesheet; the control box is laid out as labeled fieldsets, and the socklog panel grows with content instead of a fixed height.Test plan
pnpm --filter @rsscloud/client-app run test— 73 passingpnpm run lint/pnpm run test:unit/pnpm run typecheck— clean across all 6 workspace packagesdocker build+docker runsmoke test ofapps/client/DockerfileCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by CodeRabbit