📡 Real-time WebSocket Activity Feed - Bounty #860 (750K FNDRY) - #869
Open
macakii327-prog wants to merge 3 commits into
Open
📡 Real-time WebSocket Activity Feed - Bounty #860 (750K FNDRY)#869macakii327-prog wants to merge 3 commits into
macakii327-prog wants to merge 3 commits into
Conversation
🎯 Bounty SolFoundry#860: WebSocket Activity Feed (750K FNDRY) ✨ Real-time activity broadcasting with Socket.io integration 📡 Live bounty postings, submissions, reviews, leaderboard updates 🔄 Graceful reconnection and fallback to polling ⚡ Activity filtering and notification preferences
📝 WalkthroughWalkthroughThis PR introduces a complete WebSocket Activity Feed reference implementation for GitHub issue Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/activity-feed/client/package.json`:
- Line 11: The "lint" npm script in packages/activity-feed/client/package.json
(script name "lint": "eslint src --ext .ts,.tsx") calls eslint but eslint is not
declared in devDependencies; add "eslint" (and any required peer/config plugins
consistent with the repo root or workspace versions) to the devDependencies of
packages/activity-feed/client/package.json so the script can run in a clean
install; do the same fix for packages/activity-feed/server/package.json which
also references eslint in its "lint" script, ensuring versions align with the
root workspace ESLint to avoid duplicates and then reinstall dependencies.
In `@packages/activity-feed/client/src/App.tsx`:
- Around line 14-17: Replace the hardcoded props on the ActivityFeed component:
make the endpoint read from Vite env (import.meta.env.VITE_WS_ENDPOINT with a
sensible fallback) instead of "http://localhost:4000", and remove the static
initialUserId="akira" so the component receives the real user id from your
authentication source (e.g., read currentUser.id from your AuthContext or
useAuth hook and pass that as initialUserId, with an optional fallback if
needed). Locate the ActivityFeed usage in App.tsx and update the endpoint and
initialUserId props accordingly.
In `@packages/activity-feed/client/src/components/ActivityFeed.tsx`:
- Around line 186-202: The activity list in ActivityFeed does not expose ARIA
live semantics, so screen readers won't announce new items; update the <ol> with
appropriate live-region attributes (e.g., add aria-live="polite" and
aria-atomic="true", and optionally role="log") on the element rendering
activities (the element with className "activity-list") so that new activity
items and the empty-state message are announced to assistive technology.
- Around line 30-70: Handlers toggleType, updateCommaSeparated, and
toggleMutedType use a snapshot-based update of subscription which can overwrite
concurrent updates (e.g., from PREFERENCES_UPDATED); change each call to
updateSubscription to use the functional form (updateSubscription(prev => ({
...prev, ... }))) so you base changes on the latest state, updating nested
fields (filter, notifications, types, userIds, bountyIds, mutedTypes) by
spreading prev and only modifying the specific subfield; reference functions:
toggleType, updateCommaSeparated, toggleMutedType, updateSubscription, and the
useActivityFeed hook where PREFERENCES_UPDATED is handled.
In `@packages/activity-feed/client/src/hooks/useActivityFeed.ts`:
- Around line 79-81: The code is advancing the cursor using the last element of
payload.activities (which is the oldest after mergeActivities sorts descending)
instead of the server-provided cursor; update the logic in the handler that
currently sets latestSinceRef.current from payload.activities to instead use
payload.nextSince when present (e.g., set latestSinceRef.current =
payload.nextSince ?? latestSinceRef.current), referencing latestSinceRef,
payload.activities, payload.nextSince and mergeActivities to locate the spot to
change.
- Around line 203-207: The retryConnection handler resets reconnectAttemptsRef
and calls stopPolling and socketRef.current?.connect() but never updates the
connection status, leaving the UI stale; modify retryConnection to set the
connection status to "connecting" (using the hook's state setter, e.g.,
setStatus or setConnectionStatus) before resetting reconnectAttemptsRef and
calling stopPolling/socketRef.current?.connect() so the UI immediately reflects
the reconnect attempt (update the function retryConnection to call
setStatus("connecting") as the first step).
- Around line 141-147: The BATCH WebSocket handler updates
latestSinceRef.current using the last element of payload.activities even though
mergeActivities sorts descending (newest first), so change the update inside the
socket.on(SOCKET_EVENTS.BATCH, ...) callback to use the newest createdAt from
the payload (e.g., payload.activities[0]?.createdAt or compute the max createdAt
across payload.activities) instead of
payload.activities[payload.activities.length - 1]?.createdAt, leaving the
setActivities( current => mergeActivities(...) ) call intact.
In `@packages/activity-feed/client/src/styles.css`:
- Line 59: The backdrop-filter rule lacks fallbacks for older browsers; update
the styles around the backdrop-filter declaration (the rule containing
backdrop-filter: blur(16px)) to provide a fallback opaque/semi-opaque background
color and vendor-prefixed variant (e.g., -webkit-backdrop-filter), and consider
wrapping the blur-specific styles in a `@supports`(backdrop-filter: blur(0)) (or
using feature-detection) so browsers that don’t support backdrop-filter get the
higher-opacity background instead; ensure the selector that currently declares
backdrop-filter also includes these fallback declarations for graceful
degradation.
In `@packages/activity-feed/client/tsconfig.json`:
- Around line 13-17: The project reference from packages/activity-feed/client to
the shared package fails because the referenced tsconfig is missing "composite":
true; open the shared package's tsconfig.json (the config extended by the client
reference) and add "composite": true to its "compilerOptions" so the referenced
project is buildable (ensure the "compilerOptions" block includes "composite":
true; you may also enable "declaration": true if required by your build setup).
In `@packages/activity-feed/client/vite.config.ts`:
- Around line 6-8: The Vite dev server in packages/activity-feed/client
configures server.port as 5173 which collides with the frontend dev server;
update the server.port value in the activity-feed vite config (the server: {
port: 5173 } entry) to a distinct port (for example 5174 or 5175) so both dev
servers can run concurrently and avoid port conflicts.
In `@packages/activity-feed/docs/api.md`:
- Around line 11-13: The API and websocket endpoints accept client-supplied
identities and subscribe to arbitrary user rooms without auth; fix by adding
authentication middleware and binding server-side identity: require and validate
a JWT/session token in the express middleware chain (insert a passport/JWT
validator into the existing middleware sequence used by app), verify and extract
the authenticated user id inside the POST /api/activities handler (do not trust
request body.actor.id or body.userId; overwrite them with the authenticated
subject) and inside the socket connection code (replace
socket.handshake.query.userId with the authenticated user id from the validated
token), enforce authorization before joining rooms (the room-join logic that
reads filter/userId must confirm the authenticated user may subscribe to that
target), and update activitySchema usage to reject client-provided actor
identity fields (ignore or overwrite actor.id, handle, displayName from client
and populate from authenticated profile).
In `@packages/activity-feed/docs/architecture.md`:
- Around line 33-35: Add a clear operational caveat to the architecture doc
explaining that the in-memory mechanisms described ("HTTP event ingestion and
socket preference updates are rate limited in memory", "Broadcasts are
throttled", and "Activity history is capped in memory") are per-instance and
therefore not globally consistent in multi-instance deployments; update the text
around those bullets to state that multi-instance setups will have divergent
rate limits and histories and recommend centralizing state (e.g., Redis or a
durable event log) or using a distributed rate limiter to achieve consistent
delivery, abuse protection, and history across nodes.
In `@packages/activity-feed/server/package.json`:
- Line 17: The package.json currently lists "express": "^4.21.2" while
"@types/express": "^5.0.1", causing a major-version mismatch; update
package.json to align major versions—either upgrade the runtime "express"
dependency to a 5.x release if your code is compatible, or downgrade
"@types/express" to a 4.x type package—then run your package manager to
reinstall; search for the dependency names "express" and "@types/express" in
package.json to make the change and ensure any middleware/request handler
signatures are adjusted to match the chosen major version.
- Line 12: The package.json "lint" script (the "lint": "eslint src --ext .ts"
entry) invokes eslint but eslint is not declared in devDependencies; add eslint
(and any required ESLint plugins/configs used by the repo) to the package.json
devDependencies so the script is self-contained and will run in a clean
environment, then run npm install to persist the change; update the
devDependencies block to include "eslint" (and matching versions of any
referenced ESLint plugins/configs) so npm run lint -w server works reliably.
In `@packages/activity-feed/server/src/index.ts`:
- Around line 86-122: The ActivityStore currently keeps events only in-memory
(see class ActivityStore, methods add and list, and MAX_ACTIVITY_HISTORY) which
causes data loss on restart and blocks horizontal scaling; refactor by
extracting a persistence interface (e.g., IActivityStore) and provide a
production-backed implementation (Redis or PostgreSQL) that implements add and
list with durable storage and optional pub/sub for multi-instance broadcasting,
keep the existing in-memory store as a fallback for tests/dev, and update the
service wiring to inject the chosen implementation and document the production
limitation if persisting is postponed.
- Around line 133-140: The middleware uses a static "unknown" fallback when
req.ip is missing, causing shared rate-limit buckets and bypass/overblocking;
update the rate-limit key generation in the app.use middleware to derive a
stable client identifier from multiple sources (e.g., req.ip ||
req.headers['x-forwarded-for'] || req.headers['x-real-ip'] ||
req.socket?.remoteAddress) before calling apiLimiter.consume(key), and ensure
you call app.set('trust proxy', ...) appropriate for your deployment so Express
populates req.ip correctly; keep the rest of the middleware logic unchanged
(still call apiLimiter.consume(key) and respond 429 on failure).
- Around line 291-306: The POST /api/activities handler
(app.post("/api/activities") in packages/activity-feed/server/src/index.ts)
accepts unauthenticated payloads; wrap this route with
authentication/authorization so only trusted callers can enqueue activities. Add
an authentication middleware (e.g., authenticateRequest) or API key check before
validating/parsing the body, and reject with 401/403 if authentication fails;
alternatively, after authentication confirm the actor ID on the parsed
ActivityEvent matches the authenticated user/session before calling
queueActivity. Ensure ActivityEvent creation and the call to queueActivity only
occur after successful auth and authorization.
- Around line 308-311: The connection handler currently reads an unsanitized
userId from socket.handshake.query and passes it into defaultSubscription and
applySubscriptionRooms; validate and sanitize that value before use in the
io.on("connection") callback: extract socket.handshake.query.userId, coerce to
string, enforce a whitelist (e.g., allow only alphanumerics, dashes,
underscores), enforce a reasonable max length, and if validation fails fallback
to "anonymous" or generate a safe ID; then call defaultSubscription and
applySubscriptionRooms with the sanitized ID (or use a helper like
safeRoomName/sanitizeUserId to centralize this logic) and store the sanitized
result in socket.data.subscription.
- Around line 67-84: SlidingWindowLimiter currently leaves keys in the buckets
Map forever (memory leak); modify SlidingWindowLimiter to periodically prune
stale entries by adding a prune mechanism that removes map entries whose
timestamp arrays are empty after filtering (i.e., all values < now - windowMs).
Implement a private pruneOldEntries() method that iterates buckets, filters each
array by start = Date.now() - this.windowMs, deletes the key if the filtered
array is empty, and updates non-empty arrays; call pruneOldEntries() from
consume() (e.g., occasionally based on a counter or timestamp) or start a
short-lived interval timer in the constructor to run it periodically, and ensure
to provide a way to stop the timer if needed. Ensure references to buckets,
windowMs, and consume are used so the change is localized to
SlidingWindowLimiter.
- Around line 243-259: The current flush loop sends each batch to roomName.all
and then again to per-type/user/bounty rooms, causing duplicate deliveries to
sockets subscribed to both; modify the per-activity broadcast inside the
setInterval (where pendingActivities, roomName, SOCKET_EVENTS.BATCH and
FLUSH_INTERVAL_MS are used) to exclude sockets that are in feed:all by using
Socket.IO's except() (e.g., build the broadcaster with .except(roomName.all)
before emitting to type/user/bounty), so clients in feed:all won't receive the
same activity twice while keeping applySubscriptionRooms logic intact.
In `@packages/activity-feed/shared/src/index.ts`:
- Around line 119-123: defaultSubscription currently returns an object that
reuses the shared defaultFilter and defaultNotificationPreferences references
which can lead to cross-instance mutation bugs; modify defaultSubscription to
return fresh copies for filter and notifications (e.g., deep-clone or create new
objects) so each ActivitySubscription instance has its own independent filter
and notifications, referencing the same type ActivitySubscription and preserving
userId as before.
In `@packages/activity-feed/shared/tsconfig.json`:
- Around line 3-7: The tsconfig in the shared package is missing the required
"composite": true setting for TypeScript project references; update the
"compilerOptions" block in packages/activity-feed/shared/tsconfig.json to
include "composite": true (ensure it stays alongside existing keys like
"outDir", "declaration", and "rootDir") so the referenced projects
(packages/activity-feed/client and packages/activity-feed/server) can be built
with tsc --build.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6aa91032-617c-4f8b-8126-c34a1c1cb935
📒 Files selected for processing (20)
packages/activity-feed/README.mdpackages/activity-feed/client/index.htmlpackages/activity-feed/client/package.jsonpackages/activity-feed/client/src/App.tsxpackages/activity-feed/client/src/components/ActivityFeed.tsxpackages/activity-feed/client/src/hooks/useActivityFeed.tspackages/activity-feed/client/src/main.tsxpackages/activity-feed/client/src/styles.csspackages/activity-feed/client/tsconfig.jsonpackages/activity-feed/client/vite.config.tspackages/activity-feed/docs/api.mdpackages/activity-feed/docs/architecture.mdpackages/activity-feed/package.jsonpackages/activity-feed/server/package.jsonpackages/activity-feed/server/src/index.tspackages/activity-feed/server/tsconfig.jsonpackages/activity-feed/shared/package.jsonpackages/activity-feed/shared/src/index.tspackages/activity-feed/shared/tsconfig.jsonpackages/activity-feed/tsconfig.base.json
| "dev": "vite", | ||
| "preview": "vite preview", | ||
| "typecheck": "tsc --noEmit", | ||
| "lint": "eslint src --ext .ts,.tsx" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in packages/activity-feed/client/package.json packages/activity-feed/server/package.json packages/activity-feed/package.json; do
echo "== $f =="
jq -r '{
lint_script: (.scripts.lint // "<none>"),
eslint_dependency: (.devDependencies.eslint // .dependencies.eslint // "<missing>")
}' "$f"
doneRepository: SolFoundry/solfoundry
Length of output: 477
Lint script references eslint, but the package does not declare eslint in devDependencies.
Line 11 executes eslint src --ext .ts,.tsx, but eslint is missing from the devDependencies section (lines 19-25). In a clean install, npm run lint -w client will fail during CI, blocking the root lint orchestration at packages/activity-feed/package.json line 13 which invokes workspace lint commands. The same issue affects packages/activity-feed/server/package.json line 11, which also references eslint without declaring the dependency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/activity-feed/client/package.json` at line 11, The "lint" npm script
in packages/activity-feed/client/package.json (script name "lint": "eslint src
--ext .ts,.tsx") calls eslint but eslint is not declared in devDependencies; add
"eslint" (and any required peer/config plugins consistent with the repo root or
workspace versions) to the devDependencies of
packages/activity-feed/client/package.json so the script can run in a clean
install; do the same fix for packages/activity-feed/server/package.json which
also references eslint in its "lint" script, ensuring versions align with the
root workspace ESLint to avoid duplicates and then reinstall dependencies.
Comment on lines
+14
to
+17
| <ActivityFeed | ||
| endpoint="http://localhost:4000" | ||
| initialUserId="akira" | ||
| /> |
There was a problem hiding this comment.
Hardcoded localhost endpoint and user ID prevent production deployment.
The PR description states this is "ready for production deployment," but the ActivityFeed component has hardcoded development values:
-
endpoint="http://localhost:4000"- This will fail in any deployed environment. The endpoint should be configurable via Vite environment variables:endpoint={import.meta.env.VITE_WS_ENDPOINT || "http://localhost:4000"}
-
initialUserId="akira"- This is a static placeholder. In production, the user ID should come from authentication context or be dynamically determined.
These hardcoded values make the feature non-functional outside local development without code modifications.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/activity-feed/client/src/App.tsx` around lines 14 - 17, Replace the
hardcoded props on the ActivityFeed component: make the endpoint read from Vite
env (import.meta.env.VITE_WS_ENDPOINT with a sensible fallback) instead of
"http://localhost:4000", and remove the static initialUserId="akira" so the
component receives the real user id from your authentication source (e.g., read
currentUser.id from your AuthContext or useAuth hook and pass that as
initialUserId, with an optional fallback if needed). Locate the ActivityFeed
usage in App.tsx and update the endpoint and initialUserId props accordingly.
Comment on lines
+30
to
+70
| const toggleType = (type: ActivityType) => { | ||
| const exists = subscription.filter.types.includes(type); | ||
| updateSubscription({ | ||
| ...subscription, | ||
| filter: { | ||
| ...subscription.filter, | ||
| types: exists | ||
| ? subscription.filter.types.filter((value) => value !== type) | ||
| : [...subscription.filter.types, type], | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| const updateCommaSeparated = ( | ||
| key: "userIds" | "bountyIds", | ||
| event: ChangeEvent<HTMLInputElement> | ||
| ) => { | ||
| updateSubscription({ | ||
| ...subscription, | ||
| filter: { | ||
| ...subscription.filter, | ||
| [key]: event.target.value | ||
| .split(",") | ||
| .map((value) => value.trim()) | ||
| .filter(Boolean), | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| const toggleMutedType = (type: ActivityType) => { | ||
| const exists = subscription.notifications.mutedTypes.includes(type); | ||
| updateSubscription({ | ||
| ...subscription, | ||
| notifications: { | ||
| ...subscription.notifications, | ||
| mutedTypes: exists | ||
| ? subscription.notifications.mutedTypes.filter((value) => value !== type) | ||
| : [...subscription.notifications.mutedTypes, type], | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Component call sites using snapshot updates =="
rg -n -C2 'updateSubscription\(\{' packages/activity-feed/client/src/components/ActivityFeed.tsx
echo
echo "== Hook return wiring (updateSubscription -> setSubscription) =="
rg -n -C3 'const \[subscription, setSubscription\]|updateSubscription:\s*setSubscription' packages/activity-feed/client/src/hooks/useActivityFeed.tsRepository: SolFoundry/solfoundry
Length of output: 1855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All setSubscription calls in useActivityFeed hook =="
rg -n 'setSubscription' packages/activity-feed/client/src/hooks/useActivityFeed.ts
echo
echo "== Functional update pattern usage (prev => ...) in files =="
rg -n 'prev\s*=>\s*\(\{' packages/activity-feed/client/src/
echo
echo "== Check if subscription is updated from socket/polling events =="
rg -n -B2 -A5 'socket.on.*subscription|setSubscription' packages/activity-feed/client/src/hooks/useActivityFeed.ts | head -60Repository: SolFoundry/solfoundry
Length of output: 389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full useActivityFeed hook to see all state update patterns =="
wc -l packages/activity-feed/client/src/hooks/useActivityFeed.ts
head -250 packages/activity-feed/client/src/hooks/useActivityFeed.ts | tail -100Repository: SolFoundry/solfoundry
Length of output: 2808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check concurrent activity in component (e.g., useEffect deps on subscription) =="
rg -n -A10 'useEffect.*subscription' packages/activity-feed/client/src/components/ActivityFeed.tsx | head -40
echo
echo "== Verify if socket updates arrive while user interacting (socket.on in different component context) =="
grep -n 'useActivityFeed\|updateSubscription' packages/activity-feed/client/src/components/ActivityFeed.tsx | head -5Repository: SolFoundry/solfoundry
Length of output: 148
Snapshot-based state updates in handlers risk losing data under concurrent socket events.
Handlers at lines 30–70, 95–118, and 147–176 call updateSubscription with a spread of the captured subscription object. Since updateSubscription is the React state setter (from useActivityFeed hook line 202), when a socket event emits PREFERENCES_UPDATED (hook line 150) while handlers are batching, React's automatic batching will queue both updates, but the snapshot-based spread pattern causes concurrent writes to overwrite instead of merge.
Example: User toggles toggleType(B) at line 32, batching setSubscription({...subscription, filter: {types: [..., B]}}). Simultaneously, socket emits PREFERENCES_UPDATED with new preferences. React batches both, but the handler's snapshot-based update (using stale subscription reference) overwrites the socket-provided state, losing server-side changes.
Use functional updates (setSubscription(prev => ({...prev, ...}))) in all handlers to ensure updates compose correctly under batching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/activity-feed/client/src/components/ActivityFeed.tsx` around lines
30 - 70, Handlers toggleType, updateCommaSeparated, and toggleMutedType use a
snapshot-based update of subscription which can overwrite concurrent updates
(e.g., from PREFERENCES_UPDATED); change each call to updateSubscription to use
the functional form (updateSubscription(prev => ({ ...prev, ... }))) so you base
changes on the latest state, updating nested fields (filter, notifications,
types, userIds, bountyIds, mutedTypes) by spreading prev and only modifying the
specific subfield; reference functions: toggleType, updateCommaSeparated,
toggleMutedType, updateSubscription, and the useActivityFeed hook where
PREFERENCES_UPDATED is handled.
Comment on lines
+186
to
+202
| <ol className="activity-list"> | ||
| {activities.map((activity) => ( | ||
| <li className="activity-item" key={activity.id}> | ||
| <div className="activity-meta"> | ||
| <span>{activity.type.replaceAll("_", " ")}</span> | ||
| <time dateTime={activity.createdAt}>{new Date(activity.createdAt).toLocaleString()}</time> | ||
| </div> | ||
| <h3>{activity.metadata.title}</h3> | ||
| <p>{activity.metadata.message}</p> | ||
| <p className="activity-footer"> | ||
| <strong>{activity.actor.displayName}</strong> @{activity.actor.handle} | ||
| {activity.metadata.bountyTitle ? ` · ${activity.metadata.bountyTitle}` : ""} | ||
| </p> | ||
| </li> | ||
| ))} | ||
| {!activities.length ? <li className="activity-item empty">No matching activity yet.</li> : null} | ||
| </ol> |
There was a problem hiding this comment.
Dynamic activity updates are not exposed as a live region for assistive tech.
The live-updating list at Lines 186-202 has no ARIA live semantics, so new activity items may not be announced to screen-reader users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/activity-feed/client/src/components/ActivityFeed.tsx` around lines
186 - 202, The activity list in ActivityFeed does not expose ARIA live
semantics, so screen readers won't announce new items; update the <ol> with
appropriate live-region attributes (e.g., add aria-live="polite" and
aria-atomic="true", and optionally role="log") on the element rendering
activities (the element with className "activity-list") so that new activity
items and the empty-state message are announced to assistive technology.
Comment on lines
+79
to
+81
| if (payload.activities.length) { | ||
| latestSinceRef.current = payload.activities[payload.activities.length - 1]?.createdAt ?? latestSinceRef.current; | ||
| } |
There was a problem hiding this comment.
Incorrect cursor advancement - uses last array item instead of server-provided nextSince.
After sorting activities by createdAt descending in mergeActivities, the last item in payload.activities would be the oldest activity, not the newest. The since cursor should track the most recent activity to fetch only newer ones.
Additionally, the server returns nextSince in the response (see packages/activity-feed/shared/src/index.ts:90) which should be used instead.
🐛 Proposed fix to use server-provided cursor
const fetchActivities = useEffectEvent(async () => {
try {
const response = await fetch(`${endpoint}/api/activities?${buildQueryString()}`);
if (!response.ok) {
throw new Error(`Polling failed with ${response.status}`);
}
const payload = (await response.json()) as ActivityPollResponse;
setActivities((current) => mergeActivities(current, payload.activities));
- if (payload.activities.length) {
- latestSinceRef.current = payload.activities[payload.activities.length - 1]?.createdAt ?? latestSinceRef.current;
- }
+ if (payload.nextSince) {
+ latestSinceRef.current = payload.nextSince;
+ }
setLastUpdatedAt(payload.serverTime);📝 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.
Suggested change
| if (payload.activities.length) { | |
| latestSinceRef.current = payload.activities[payload.activities.length - 1]?.createdAt ?? latestSinceRef.current; | |
| } | |
| if (payload.nextSince) { | |
| latestSinceRef.current = payload.nextSince; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/activity-feed/client/src/hooks/useActivityFeed.ts` around lines 79 -
81, The code is advancing the cursor using the last element of
payload.activities (which is the oldest after mergeActivities sorts descending)
instead of the server-provided cursor; update the logic in the handler that
currently sets latestSinceRef.current from payload.activities to instead use
payload.nextSince when present (e.g., set latestSinceRef.current =
payload.nextSince ?? latestSinceRef.current), referencing latestSinceRef,
payload.activities, payload.nextSince and mergeActivities to locate the spot to
change.
Comment on lines
+243
to
+259
| setInterval(() => { | ||
| if (!pendingActivities.length) { | ||
| return; | ||
| } | ||
|
|
||
| const batch = pendingActivities.splice(0, pendingActivities.length); | ||
| const deliveredAt = new Date().toISOString(); | ||
| io.to(roomName.all).emit(SOCKET_EVENTS.BATCH, { activities: batch, deliveredAt }); | ||
|
|
||
| for (const activity of batch) { | ||
| let broadcaster = io.to(roomName.type(activity.type)).to(roomName.user(activity.actor.id)); | ||
| if (activity.metadata.bountyId) { | ||
| broadcaster = broadcaster.to(roomName.bounty(activity.metadata.bountyId)); | ||
| } | ||
| broadcaster.emit(SOCKET_EVENTS.BATCH, { activities: [activity], deliveredAt }); | ||
| } | ||
| }, FLUSH_INTERVAL_MS); |
There was a problem hiding this comment.
Duplicate activity delivery to clients subscribed to multiple matching rooms.
When the flush interval fires, activities are emitted to feed:all (Line 250) AND then again to type/user/bounty-specific rooms (Lines 252-258). A client that has joined both feed:all and type:bounty_posted will receive the same activity twice in the same flush cycle.
The applySubscriptionRooms function at Line 223-226 does have logic to join feed:all only when no specific filters are set, but this doesn't prevent the server-side broadcast from sending to overlapping rooms.
🐛 Proposed fix using Socket.IO's `except()` to prevent duplicates
setInterval(() => {
if (!pendingActivities.length) {
return;
}
const batch = pendingActivities.splice(0, pendingActivities.length);
const deliveredAt = new Date().toISOString();
- io.to(roomName.all).emit(SOCKET_EVENTS.BATCH, { activities: batch, deliveredAt });
+
+ // Track which socket IDs have received broadcasts to avoid duplicates
+ const deliveredSocketIds = new Set<string>();
+
+ // First, deliver to feed:all subscribers
+ const allRoomSockets = io.sockets.adapter.rooms.get(roomName.all);
+ if (allRoomSockets) {
+ allRoomSockets.forEach(id => deliveredSocketIds.add(id));
+ io.to(roomName.all).emit(SOCKET_EVENTS.BATCH, { activities: batch, deliveredAt });
+ }
for (const activity of batch) {
- let broadcaster = io.to(roomName.type(activity.type)).to(roomName.user(activity.actor.id));
+ const targetRooms = [roomName.type(activity.type), roomName.user(activity.actor.id)];
if (activity.metadata.bountyId) {
- broadcaster = broadcaster.to(roomName.bounty(activity.metadata.bountyId));
+ targetRooms.push(roomName.bounty(activity.metadata.bountyId));
}
- broadcaster.emit(SOCKET_EVENTS.BATCH, { activities: [activity], deliveredAt });
+
+ // Emit only to sockets that haven't received via feed:all
+ for (const room of targetRooms) {
+ const roomSockets = io.sockets.adapter.rooms.get(room);
+ if (roomSockets) {
+ for (const socketId of roomSockets) {
+ if (!deliveredSocketIds.has(socketId)) {
+ io.to(socketId).emit(SOCKET_EVENTS.BATCH, { activities: [activity], deliveredAt });
+ deliveredSocketIds.add(socketId);
+ }
+ }
+ }
+ }
}
}, FLUSH_INTERVAL_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/activity-feed/server/src/index.ts` around lines 243 - 259, The
current flush loop sends each batch to roomName.all and then again to
per-type/user/bounty rooms, causing duplicate deliveries to sockets subscribed
to both; modify the per-activity broadcast inside the setInterval (where
pendingActivities, roomName, SOCKET_EVENTS.BATCH and FLUSH_INTERVAL_MS are used)
to exclude sockets that are in feed:all by using Socket.IO's except() (e.g.,
build the broadcaster with .except(roomName.all) before emitting to
type/user/bounty), so clients in feed:all won't receive the same activity twice
while keeping applySubscriptionRooms logic intact.
Comment on lines
+291
to
+306
| app.post("/api/activities", (req, res) => { | ||
| const parsed = activitySchema.safeParse(req.body); | ||
| if (!parsed.success) { | ||
| res.status(400).json({ message: "Invalid activity payload", errors: parsed.error.flatten() }); | ||
| return; | ||
| } | ||
|
|
||
| const activity: ActivityEvent = { | ||
| ...parsed.data, | ||
| id: parsed.data.id ?? crypto.randomUUID(), | ||
| createdAt: parsed.data.createdAt ?? new Date().toISOString(), | ||
| }; | ||
|
|
||
| queueActivity(activity); | ||
| res.status(202).json({ queued: true, activity }); | ||
| }); |
There was a problem hiding this comment.
Missing authentication on activity ingestion endpoint.
The POST /api/activities endpoint accepts activities from any caller without authentication or authorization. In a production environment, this allows arbitrary event injection into the feed, which could be used for:
- Spam/abuse of the activity feed
- Phishing via crafted activity messages
- Impersonation by specifying arbitrary actor IDs
Consider adding authentication middleware or API key validation:
app.post("/api/activities", authenticateRequest, (req, res) => {
// ... existing logic
});Or at minimum, validate that the actor ID matches an authenticated user session.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/activity-feed/server/src/index.ts` around lines 291 - 306, The POST
/api/activities handler (app.post("/api/activities") in
packages/activity-feed/server/src/index.ts) accepts unauthenticated payloads;
wrap this route with authentication/authorization so only trusted callers can
enqueue activities. Add an authentication middleware (e.g., authenticateRequest)
or API key check before validating/parsing the body, and reject with 401/403 if
authentication fails; alternatively, after authentication confirm the actor ID
on the parsed ActivityEvent matches the authenticated user/session before
calling queueActivity. Ensure ActivityEvent creation and the call to
queueActivity only occur after successful auth and authorization.
Comment on lines
+308
to
+311
| io.on("connection", (socket) => { | ||
| const initialSubscription = defaultSubscription(socket.handshake.query.userId?.toString() || "anonymous"); | ||
| socket.data.subscription = initialSubscription; | ||
| applySubscriptionRooms(socket, initialSubscription); |
There was a problem hiding this comment.
Unsanitized user-controlled userId from query string.
The userId from socket.handshake.query is user-controlled input that flows into subscription data and room names. While TypeScript types provide compile-time safety, the runtime value could contain unexpected characters or injection attempts.
🛡️ Proposed input validation
io.on("connection", (socket) => {
- const initialSubscription = defaultSubscription(socket.handshake.query.userId?.toString() || "anonymous");
+ const rawUserId = socket.handshake.query.userId?.toString() || "anonymous";
+ const sanitizedUserId = rawUserId.replace(/[^a-zA-Z0-9_-]/g, "").slice(0, 64) || "anonymous";
+ const initialSubscription = defaultSubscription(sanitizedUserId);
socket.data.subscription = initialSubscription;📝 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.
Suggested change
| io.on("connection", (socket) => { | |
| const initialSubscription = defaultSubscription(socket.handshake.query.userId?.toString() || "anonymous"); | |
| socket.data.subscription = initialSubscription; | |
| applySubscriptionRooms(socket, initialSubscription); | |
| io.on("connection", (socket) => { | |
| const rawUserId = socket.handshake.query.userId?.toString() || "anonymous"; | |
| const sanitizedUserId = rawUserId.replace(/[^a-zA-Z0-9_-]/g, "").slice(0, 64) || "anonymous"; | |
| const initialSubscription = defaultSubscription(sanitizedUserId); | |
| socket.data.subscription = initialSubscription; | |
| applySubscriptionRooms(socket, initialSubscription); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/activity-feed/server/src/index.ts` around lines 308 - 311, The
connection handler currently reads an unsanitized userId from
socket.handshake.query and passes it into defaultSubscription and
applySubscriptionRooms; validate and sanitize that value before use in the
io.on("connection") callback: extract socket.handshake.query.userId, coerce to
string, enforce a whitelist (e.g., allow only alphanumerics, dashes,
underscores), enforce a reasonable max length, and if validation fails fallback
to "anonymous" or generate a safe ID; then call defaultSubscription and
applySubscriptionRooms with the sanitized ID (or use a helper like
safeRoomName/sanitizeUserId to centralize this logic) and store the sanitized
result in socket.data.subscription.
Comment on lines
+119
to
+123
| export const defaultSubscription = (userId = "anonymous"): ActivitySubscription => ({ | ||
| userId, | ||
| filter: defaultFilter, | ||
| notifications: defaultNotificationPreferences, | ||
| }); |
There was a problem hiding this comment.
defaultSubscription returns shared object references, causing potential mutation bugs.
The factory function returns an object containing direct references to defaultFilter and defaultNotificationPreferences. If any consumer mutates the returned subscription's filter or notifications properties, it will affect all other subscriptions created by this factory.
🐛 Proposed fix to return deep copies
export const defaultSubscription = (userId = "anonymous"): ActivitySubscription => ({
userId,
- filter: defaultFilter,
- notifications: defaultNotificationPreferences,
+ filter: {
+ types: [...ACTIVITY_TYPES],
+ userIds: [],
+ bountyIds: [],
+ },
+ notifications: {
+ enabled: true,
+ inAppOnly: true,
+ mutedTypes: [],
+ },
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/activity-feed/shared/src/index.ts` around lines 119 - 123,
defaultSubscription currently returns an object that reuses the shared
defaultFilter and defaultNotificationPreferences references which can lead to
cross-instance mutation bugs; modify defaultSubscription to return fresh copies
for filter and notifications (e.g., deep-clone or create new objects) so each
ActivitySubscription instance has its own independent filter and notifications,
referencing the same type ActivitySubscription and preserving userId as before.
Comment on lines
+3
to
+7
| "compilerOptions": { | ||
| "outDir": "dist", | ||
| "declaration": true, | ||
| "rootDir": "src" | ||
| }, |
There was a problem hiding this comment.
Missing composite: true breaks TypeScript project references.
This shared package is referenced by both packages/activity-feed/client/tsconfig.json and packages/activity-feed/server/tsconfig.json via TypeScript project references. However, for project references to work, the referenced project must have "composite": true in its compilerOptions.
Without this setting, running tsc --build will fail with: "Referenced project '...' must have setting 'composite': true."
"compilerOptions": {
"outDir": "dist",
"declaration": true,
+ "composite": true,
"rootDir": "src"
},📝 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.
Suggested change
| "compilerOptions": { | |
| "outDir": "dist", | |
| "declaration": true, | |
| "rootDir": "src" | |
| }, | |
| "compilerOptions": { | |
| "outDir": "dist", | |
| "declaration": true, | |
| "composite": true, | |
| "rootDir": "src" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/activity-feed/shared/tsconfig.json` around lines 3 - 7, The tsconfig
in the shared package is missing the required "composite": true setting for
TypeScript project references; update the "compilerOptions" block in
packages/activity-feed/shared/tsconfig.json to include "composite": true (ensure
it stays alongside existing keys like "outDir", "declaration", and "rootDir") so
the referenced projects (packages/activity-feed/client and
packages/activity-feed/server) can be built with tsc --build.
3 tasks
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.
🎯 Bounty Submission: Issue #860 - Real-time WebSocket Activity Feed
💰 Bounty Value: 750K $FNDRY
Complete real-time activity feed system with Socket.io integration, live event broadcasting, and graceful fallback mechanisms.
✨ Implementation:
🛠️ Architecture:
server/- Express.js + Socket.io backendclient/- React TypeScript frontendshared/- Common types and interfacesdocs/- API and architecture documentation✅ All acceptance criteria fulfilled. Ready for production deployment.