Add commissioned ideas activity plan - #243
Draft
mrbdahlem wants to merge 8 commits into
Draft
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
Contributor
📝 WalkthroughWalkthroughChangesCommissioned Ideas Activity
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Student
participant RegistrationForm
participant CommissionedIdeasRoutes
participant WsRouter
Student->>RegistrationForm: submit display name
RegistrationForm->>CommissionedIdeasRoutes: register-participant
CommissionedIdeasRoutes-->>RegistrationForm: participantId and token
Student->>WsRouter: connect with participant identity
WsRouter-->>Student: student-safe session-state
Student->>CommissionedIdeasRoutes: create, join, or leave team
CommissionedIdeasRoutes->>WsRouter: broadcast registration update
WsRouter-->>Student: updated student snapshot
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 25
🤖 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 @.agent/plans/commissioned-ideas-activity-plan.md:
- Around line 66-86: Update the completed-phase documentation in
commissioned-ideas-activity-plan.md to match the shipped contract: replace
legacy SharkTank component names with CommissionedIdeasManager,
CommissionedIdeasStudent, RegistrationForm, and StudentRoster, add shared/id.ts,
rename the documented types to the exported CommissionedIdeas* symbols, include
instructorPasscode, and replace the REST endpoints with /settings, /create-team,
/join-team, /leave-team, /assign-participant, and /assign-random.
In `@activities/commissioned-ideas/client/hooks/useCommissionedIdeasSession.ts`:
- Around line 80-86: Extract the duplicated WebSocket protocol and base-URL
construction from buildWsUrl in useCommissionedIdeasSession and the
corresponding buildWsUrl in useStudentSession/useManagerSession into a shared
helper. Have each hook reuse that helper while preserving sessionId and optional
participantId query parameters and the existing null behavior when no sessionId
is present.
- Around line 88-101: Update handleMessage and the corresponding manager-message
handling around the referenced branches to validate msg.data at runtime using
the narrow type guards from activities/commissioned-ideas/shared/validation.ts.
Only call setSnapshot after the appropriate StudentSnapshot or ManagerSnapshot
guard succeeds, and remove the unchecked casts while preserving
malformed-message rejection.
- Line 134: Update the lifecycle handling around mountedRef in
useCommissionedIdeasSession so it is set to false when the hook unmounts, using
an effect cleanup. Preserve the existing mountedRef.current guard in
handleMessage so late messages cannot trigger updates after unmount.
- Around line 52-57: Update the WebSocket message handling around WsMessage and
handleMessage to surface commissioned-ideas:error payloads, including Invalid
instructor passcode, through the manager session state. In useManagerSession,
set shouldReconnect to false for authentication failures before the socket
closes, and update CommissionedIdeasManager.tsx to render the surfaced error
instead of remaining on “Connecting…”.
In `@activities/commissioned-ideas/client/manager/CommissionedIdeasManager.tsx`:
- Around line 9-31: Update CommissionedIdeasManager’s session bootstrap
resolution effect to use a commit/ref guard so React.StrictMode’s repeated mount
does not consume the one-time payload more than once. Resolve the passcode only
once per mount, then apply instructorPasscode and passcodeResolved state updates
from that single resolution while preserving the existing sessionId dependency.
In `@activities/commissioned-ideas/client/manager/RegistrationDashboard.tsx`:
- Around line 578-680: Update ParticipantRow so cancelling or successfully
saving an edit restores focus to that participant’s “Edit” button after edit
mode closes. Add a ref to the button and focus it when the edit state
transitions from editing to non-editing, while preserving normal focus behavior
when entering edit mode.
- Around line 229-257: Update the max-team-size increment and decrement handlers
in SettingsPanel so failed onMaxTeamSize requests restore maxInput to the
pre-click serverMax value, matching handleMaxBlur’s invalid-input behavior. Pass
serverMax as the revertTo value when invoking onMaxTeamSize, and keep
setMaxInput owned by SettingsPanel so the optimistic display can be reverted
locally.
In `@activities/commissioned-ideas/client/student/CommissionedIdeasStudent.tsx`:
- Around line 21-32: Add unit tests beside the commissioned-ideas student
components and Playwright coverage under
activities/commissioned-ideas/playwright for CommissionedIdeasStudent. Cover
registration through roster transition, participant-token identity restoration,
and rendering behavior across activity phases.
- Around line 90-99: The handleRegistered flow currently persists the
participant bearer token in localStorage; replace this with the
repository-approved session-scoped credential approach, preferably an httpOnly
per-session cookie issued by register-participant, or at minimum sessionStorage
if a server-side change is out of scope. Update the tokenKey storage and all
corresponding reads to use the same mechanism, and clear the credential when the
session ends.
- Around line 102-108: Add role="status" to both loading-state divs in the
sessionId and identityResolved branches of CommissionedIdeasStudent so assistive
technologies announce the loading messages with polite live-region behavior.
In `@activities/commissioned-ideas/client/student/RegistrationForm.tsx`:
- Around line 29-48: Update the submit flow in the RegistrationForm component to
send the locally stored participant token in the
X-Commissioned-Ideas-Participant-Token header alongside initialParticipantId.
Handle a 403 response by falling back to fresh participant registration, while
preserving the existing success and error handling for other responses.
In `@activities/commissioned-ideas/client/student/StudentRoster.tsx`:
- Around line 5-13: Update StudentRosterProps.groupingMode to use the shared
GroupingMode type imported from activities/commissioned-ideas/shared/types.ts
instead of string, preserving the existing prop name and behavior while
enforcing the valid grouping-mode union.
In `@activities/commissioned-ideas/server/routeHandlers.test.ts`:
- Around line 868-945: Extend the websocket authentication coverage around the
existing manager-auth test to include wrong or omitted instructor passcodes and
malformed commissioned-ideas:manager-auth payloads. Assert each invalid attempt
is rejected without sending a privileged session snapshot, using the existing
registeredWsHandler, messageHandlers, and sentMessages test helpers.
In `@activities/commissioned-ideas/server/routes.ts`:
- Around line 457-508: Update assignRandom to honor reshuffleOnly: when true,
retain only ungrouped participants; when false, include eligible grouped
participants, clear their existing team memberships before assignment, and
ensure the /assign-random route passes and documents the selected mode
consistently. Preserve rejection filtering and post-lock ungrouped-only
behavior.
- Around line 937-967: Update the WebSocket connection flow around the
CommissionedIdeasSocket initialization so participantId is not trusted from the
query string. Require and validate the participant token through the existing
post-connect authentication flow, mirroring manager authentication, before
assigning typedSocket.participantId; otherwise leave it null and prevent
participant-specific snapshots or connected/lastSeen updates, including in the
close handler.
- Around line 552-570: Update the register-participant handler to enforce
session.phase and allowLateRegistration before creating a new participant:
reject new registrations during voting/results when late registration is
disabled, while preserving reconnects for existing participants. Reuse the
session fields and existing response conventions, and keep normal registration
behavior unchanged when the phase permits it or allowLateRegistration is
enabled.
- Around line 572-609: Require and validate the
X-Commissioned-Ideas-Participant-Token header before the existing branch in the
participant registration flow in
activities/commissioned-ideas/server/routes.ts:572-609; return 403 on mismatch
and only issue tokens for genuinely new IDs. In
activities/commissioned-ideas/client/student/RegistrationForm.tsx:29-48, send
the locally stored token when initialParticipantId is present and fall back to
fresh registration after a 403 response.
- Around line 204-224: Update normalizeSessionData to normalize a missing
instructorPasscode to an unusable empty string instead of calling
generatePasscode, preserving existing valid passcodes. Also update the token
normalization logic in activities/commissioned-ideas/server/routes.ts lines
149-160 to use an empty string for missing tokens, ensuring verifyPasscode fails
closed; both affected sites are in the same file.
- Line 606: The participant registration logging in the relevant route must not
emit the sensitive name/student identifier. Update the registration log to use
structured fields containing only sessionId and participantId, and revise the
related line 525 message to avoid interpolated identifiers while preserving
structured logging.
- Around line 364-374: Update buildManagerSnapshot and the related snapshot
builder to use explicit allow-listed fields rather than spreading the session
data; exclude instructorPasscode and ballots while preserving only the
manager-visible roster, connection, moderation, and required session metadata
fields, plus ballotsReceived. Ensure future persisted fields are not included in
manager session-state, registration-updated, or broadcastToAll payloads.
- Around line 973-978: Add an error listener for the socket created in the
surrounding connection setup, alongside the existing socket.on('message', ...)
handler, and handle transport errors without allowing them to surface as
uncaught exceptions. Also ensure direct socket.send calls in this flow are
guarded consistently with the existing broadcast helpers.
- Around line 788-810: Serialize the read-modify-write flow in the join-team
handler around participant lookup, team capacity validation, membership
mutation, and sessions.set using the existing per-session lock mechanism, keyed
by sessionId. Ensure concurrent requests for the same session cannot interleave,
while allowing requests for different sessions to proceed independently.
In `@activities/commissioned-ideas/shared/validation.ts`:
- Around line 47-61: Update validateBallot to call the existing
isValidBallotAmount type guard instead of hard-coding 100/300/500, moving the
guard above validateBallot if needed for scope. Use the narrowed amount and
teamId types so the amounts and teamIds duplicate checks and additions no longer
require casts, while preserving the existing invalid-amount and duplicate
validation behavior.
In `@client/src/components/common/manageDashboardUtils.ts`:
- Around line 349-360: Update parseCreateSessionBootstrap so an explicit
rawCreateSessionBootstrap.allowSessionStorageFallback === false is preserved
even when sessionStorage and historyState are both empty; do not return null in
that opt-out case, and retain the existing null behavior when no payload or flag
is present. Ensure shouldPersistCreateSessionBootstrapPayloadToSessionStorage
continues to reject persistence for the explicit false value, and add coverage
in the related tests for empty storage/state with the flag disabled.
🪄 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 Plus
Run ID: 5855c2b0-782a-4db9-92eb-f4a1bf42f2e9
📒 Files selected for processing (26)
.agent/knowledge/repo_discoveries.md.agent/knowledge/security-notes.md.agent/plans/commissioned-ideas-activity-plan.mdactivities/commissioned-ideas/activity.config.tsactivities/commissioned-ideas/client/hooks/useCommissionedIdeasSession.tsactivities/commissioned-ideas/client/index.tsactivities/commissioned-ideas/client/manager/CommissionedIdeasManager.tsxactivities/commissioned-ideas/client/manager/RegistrationDashboard.tsxactivities/commissioned-ideas/client/student/CommissionedIdeasStudent.tsxactivities/commissioned-ideas/client/student/RegistrationForm.tsxactivities/commissioned-ideas/client/student/StudentRoster.tsxactivities/commissioned-ideas/server/routeHandlers.test.tsactivities/commissioned-ideas/server/routes.test.tsactivities/commissioned-ideas/server/routes.tsactivities/commissioned-ideas/shared/id.tsactivities/commissioned-ideas/shared/scoring.tsactivities/commissioned-ideas/shared/types.tsactivities/commissioned-ideas/shared/validation.tsclient/src/activities/index.test.tsclient/src/components/common/ActivityLauncher.tsxclient/src/components/common/ManageDashboard.tsxclient/src/components/common/manageDashboardUtils.test.tsclient/src/components/common/manageDashboardUtils.tsserver/activities/activityRegistry.test.tstypes/activity.tstypes/activityConfigSchema.ts
Comment on lines
+66
to
+86
| ```text | ||
| activities/commissioned-ideas/ | ||
| ├── activity.config.ts | ||
| ├── shared/ | ||
| │ ├── types.ts | ||
| │ ├── scoring.ts | ||
| │ └── validation.ts | ||
| ├── client/ | ||
| │ ├── index.ts | ||
| │ ├── manager/ | ||
| │ │ ├── SharkTankManager.tsx | ||
| │ │ ├── PodiumReveal.tsx | ||
| │ │ └── PresentationQueue.tsx | ||
| │ └── student/ | ||
| │ ├── SharkTankStudent.tsx | ||
| │ ├── TeamRegistrationForm.tsx | ||
| │ └── VotingBallot.tsx | ||
| └── server/ | ||
| ├── routes.ts | ||
| └── routes.test.ts | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Plan still documents legacy SharkTank* names and endpoint paths that the shipped code no longer uses.
Phase 0 (Line 368) records that naming switched to CommissionedIdeas..., and Phases 1-3 are checked complete, but:
- Lines 76-82 list
SharkTankManager.tsx/SharkTankStudent.tsx/TeamRegistrationForm.tsx/VotingBallot.tsx; the tree isCommissionedIdeasManager.tsx,CommissionedIdeasStudent.tsx,RegistrationForm.tsx,StudentRoster.tsx, andshared/id.tsis missing from the diagram. - Lines 91-157 declare
SharkTankPhase/SharkTankTeam/SharkTankBallot/SharkTankSessionDataand omitinstructorPasscode, whileactivities/commissioned-ideas/shared/types.tsexportsCommissionedIdeas*withinstructorPasscode. - The REST table (Lines 272-279) lists
/max-team-size,/grouping-mode,/team/create,/team/membership,/manual-assignment,/random-groups, butserver/routes.tsimplements/settings,/create-team,/join-team,/leave-team,/assign-participant,/assign-random.
Refresh these sections so the plan matches the implemented contract for the phases already marked done.
🤖 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 @.agent/plans/commissioned-ideas-activity-plan.md around lines 66 - 86,
Update the completed-phase documentation in commissioned-ideas-activity-plan.md
to match the shipped contract: replace legacy SharkTank component names with
CommissionedIdeasManager, CommissionedIdeasStudent, RegistrationForm, and
StudentRoster, add shared/id.ts, rename the documented types to the exported
CommissionedIdeas* symbols, include instructorPasscode, and replace the REST
endpoints with /settings, /create-team, /join-team, /leave-team,
/assign-participant, and /assign-random.
Comment on lines
+52
to
+57
| interface WsMessage { | ||
| type: string | ||
| sessionId?: string | ||
| data?: unknown | ||
| error?: string | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate files:"
git ls-files | rg 'activities/commissioned-ideas/(client/hooks/useCommissionedIdeasSession\.ts|client/.*/CommissionedIdeasManager\.tsx|server/routes\.ts)' || true
echo
echo "Inspect target hook outline and relevant sections:"
wc -l activities/commissioned-ideas/client/hooks/useCommissionedIdeasSession.ts
sed -n '1,220p' activities/commissioned-ideas/client/hooks/useCommissionedIdeasSession.ts
echo
echo "Search manager passcode/auth handling:"
rg -n "instructor passcode|instructorPasscode|manager-auth|Connecting|CommissionedIdeasManager|reconnect" activities/commissioned-ideas -SRepository: PerryHighCS/ActiveBits
Length of output: 18852
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Resilient WebSocket implementation(s):"
rg -n "useResilientWebSocket|shouldReconnect|onOpen|onMessage|socketRef" src -S || true
echo
echo "Manager UI relevant sections:"
sed -n '1,100p' activities/commissioned-ideas/client/manager/CommissionedIdeasManager.tsx
echo
echo "Server manager-auth handling relevant section:"
sed -n '960,1010p' activities/commissioned-ideas/server/routes.ts
echo
echo "Route tests around manager auth error:"
sed -n '930,990p' activities/commissioned-ideas/server/routeHandlers.test.tsRepository: PerryHighCS/ActiveBits
Length of output: 7830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find resilient web socket files:"
git ls-files | rg '(^|/)useResilientWebSocket\.(ts|tsx|js)$|resilient.*web|web.*resilient' || true
echo
echo "Search all tracked files for definition:"
rg -n "function useResilientWebSocket|const .*useResilientWebSocket|export .*useResilientWebSocket|shouldReconnect|onOpen|onMessage|socketRef" -S .
echo
echo "Routes test near manager auth failure if present:"
rg -n "commissioned-ideas:error|Invalid instructor passcode|manager-auth" activities/commissioned-ideas/server/routeHandlers.test.ts -A 8 -B 8 || trueRepository: PerryHighCS/ActiveBits
Length of output: 19895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "useResilientWebSocket implementation:"
sed -n '1,190p' client/src/hooks/useResilientWebSocket.ts
echo
echo "Resilient websocket error handling in nearby usages:"
rg -n "onClose|close|onError|manualClose|shouldReconnect|onMessage" client/src/hooks/useResilientWebSocket.ts activities/commissioned-ideas/client/hooks/useCommissionedIdeasSession.ts -A 8 -B 8
echo
echo "Programmatic check: manager auth auth-failure type is sent on invalid passcode and closed before session-state:"
python3 - <<'PY'
from pathlib import Path
p = Path("activities/commissioned-ideas/server/routes.ts")
text = p.read_text()
needle_start = "if (!candidatePasscode || !verifyPasscode(session.data.instructorPasscode, candidatePasscode))"
needle_end = "typedSocket.isManager = true"
block = text[text.index(needle_start):text.index(needle_end)]
print("invalid passcode block includes error message type:", "type: 'commissioned-ideas:error'" in block and "error: 'Invalid instructor passcode'" in block)
print("invalid passcode block closes socket:", "socket.close(1008, 'Invalid instructor passcode')" in block)
print("invalid passcode block contains session-state:", "'commissioned-ideas:session-state'" in block)
PYRepository: PerryHighCS/ActiveBits
Length of output: 19853
Handle manager-auth errors from the WebSocket.
The server sends commissioned-ideas:error with Invalid instructor passcode and closes the socket, but handleMessage ignores it and useManagerSession still reconnects because shouldReconnect remains true. Surface commissioned-ideas:error, stop reconnecting for auth failures, and make CommissionedIdeasManager.tsx show the error instead of leaving managers stuck on "Connecting…".
🤖 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 `@activities/commissioned-ideas/client/hooks/useCommissionedIdeasSession.ts`
around lines 52 - 57, Update the WebSocket message handling around WsMessage and
handleMessage to surface commissioned-ideas:error payloads, including Invalid
instructor passcode, through the manager session state. In useManagerSession,
set shouldReconnect to false for authentication failures before the socket
closes, and update CommissionedIdeasManager.tsx to render the surfaced error
instead of remaining on “Connecting…”.
Comment on lines
+80
to
+86
| const buildWsUrl = useCallback(() => { | ||
| if (!sessionId) return null | ||
| const proto = window.location.protocol === 'https:' ? 'wss:' : 'ws:' | ||
| const params = new URLSearchParams({ sessionId }) | ||
| if (participantId) params.set('participantId', participantId) | ||
| return `${proto}//${window.location.host}/ws/commissioned-ideas?${params.toString()}` | ||
| }, [sessionId, participantId]) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate WS URL/protocol logic between the two hooks.
buildWsUrl in useStudentSession and useManagerSession duplicate the exact same protocol-resolution and base-URL construction. Extract a small shared helper to avoid drift.
♻️ Proposed refactor
+function resolveWsProtocol(): 'wss:' | 'ws:' {
+ return window.location.protocol === 'https:' ? 'wss:' : 'ws:'
+}
+
const buildWsUrl = useCallback(() => {
if (!sessionId) return null
- const proto = window.location.protocol === 'https:' ? 'wss:' : 'ws:'
+ const proto = resolveWsProtocol()
const params = new URLSearchParams({ sessionId })
...
}, [sessionId, participantId])Also applies to: 136-141
🤖 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 `@activities/commissioned-ideas/client/hooks/useCommissionedIdeasSession.ts`
around lines 80 - 86, Extract the duplicated WebSocket protocol and base-URL
construction from buildWsUrl in useCommissionedIdeasSession and the
corresponding buildWsUrl in useStudentSession/useManagerSession into a shared
helper. Have each hook reuse that helper while preserving sessionId and optional
participantId query parameters and the existing null behavior when no sessionId
is present.
Comment on lines
+88
to
+101
| const handleMessage = useCallback((event: MessageEvent) => { | ||
| try { | ||
| const msg = JSON.parse(event.data as string) as WsMessage | ||
| if ( | ||
| msg.type === 'commissioned-ideas:session-state' || | ||
| msg.type === 'commissioned-ideas:registration-updated' || | ||
| msg.type === 'commissioned-ideas:phase-changed' | ||
| ) { | ||
| setSnapshot(msg.data as StudentSnapshot) | ||
| } | ||
| } catch { | ||
| // malformed WS frame — ignored | ||
| } | ||
| }, []) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Unvalidated cast of incoming WS payload.
msg.data as StudentSnapshot/ManagerSnapshot trusts the server payload shape with no runtime check. A malformed or partial session-state message would silently corrupt the rendered snapshot instead of failing loudly.
Since this crosses the shared client/server contract boundary, worth confirming whether activities/commissioned-ideas/shared/validation.ts already exposes narrow type guards these hooks could reuse instead of casting.
Also applies to: 150-165
🤖 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 `@activities/commissioned-ideas/client/hooks/useCommissionedIdeasSession.ts`
around lines 88 - 101, Update handleMessage and the corresponding
manager-message handling around the referenced branches to validate msg.data at
runtime using the narrow type guards from
activities/commissioned-ideas/shared/validation.ts. Only call setSnapshot after
the appropriate StudentSnapshot or ManagerSnapshot guard succeeds, and remove
the unchecked casts while preserving malformed-message rejection.
| attachSessionEndedHandler, | ||
| }: UseManagerSessionOptions): UseManagerSessionResult { | ||
| const [snapshot, setSnapshot] = useState<ManagerSnapshot | null>(null) | ||
| const mountedRef = useRef(true) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
mountedRef is never set to false — the unmount guard is a no-op.
mountedRef starts at true and nothing in the file ever flips it, so if (mountedRef.current) in handleMessage always passes. The safety check intended to prevent late updates after unmount currently does nothing.
🔧 Proposed fix
-import { useCallback, useRef, useState } from 'react'
+import { useCallback, useEffect, useRef, useState } from 'react'
@@
const [snapshot, setSnapshot] = useState<ManagerSnapshot | null>(null)
const mountedRef = useRef(true)
+
+ useEffect(() => {
+ mountedRef.current = true
+ return () => {
+ mountedRef.current = false
+ }
+ }, [])Also applies to: 158-160
🤖 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 `@activities/commissioned-ideas/client/hooks/useCommissionedIdeasSession.ts` at
line 134, Update the lifecycle handling around mountedRef in
useCommissionedIdeasSession so it is set to false when the hook unmounts, using
an effect cleanup. Preserve the existing mountedRef.current guard in
handleMessage so late messages cannot trigger updates after unmount.
Comment on lines
+788
to
+810
| const participantId = participant.id | ||
|
|
||
| const team = session.data.teams[teamId] | ||
| if (!team) { res.status(404).json({ error: 'Team not found' }); return } | ||
|
|
||
| const memberCount = team.memberIds.filter((id) => id in session.data.participantRoster).length | ||
| if (memberCount >= session.data.maxTeamSize) { | ||
| res.status(409).json({ error: 'Team is full' }) | ||
| return | ||
| } | ||
|
|
||
| // Leave current team first | ||
| if (participant.teamId) { | ||
| removeParticipantFromTeam(session.data, participant) | ||
| } | ||
|
|
||
| team.memberIds.push(participantId) | ||
| participant.teamId = teamId | ||
|
|
||
| await sessions.set(sessionId, session) | ||
| console.info('[commissioned-ideas] Participant joined team', { sessionId, teamId, participantId }) | ||
| broadcastRegistrationUpdate(ws, sessionId, session.data) | ||
| res.json({ ok: true }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does the session store expose an atomic update / locking primitive other activities use?
fd -t f 'sessions.ts' server --exec cat -n {}
rg -n -C3 --type=ts 'withSessionLock|mutateSession|updateSession\s*\(' server activities | head -50Repository: PerryHighCS/ActiveBits
Length of output: 42827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -t f 'routes.ts|sessionCache.ts|valkeyStore.ts' activities server | sed -n '1,200p'
echo
echo "== join-team route context =="
routes_file="$(fd -t f 'routes.ts' activities/commissioned-ideas/server | head -1 || true)"
if [ -n "${routes_file:-}" ]; then
sed -n '720,840p' "$routes_file" | cat -n
fi
echo
echo "== session store usages around route helpers =="
rg -n -C3 --type=ts 'sessions\.(get|set|delete|touch)|SessionStore|createSessionStore' activities/commissioned-ideas/server/routes.ts activities/commissioned-ideas/server | sed -n '1,240p'
echo
echo "== sessionCache outline/usages =="
cache_file="$(fd -t f 'sessionCache.ts' server | head -1 || true)"
if [ -n "${cache_file:-}" ]; then
wc -l "$cache_file"
ast-grep outline "$cache_file" --view flat || true
sed -n '1,260p' "$cache_file" | cat -n
fi
echo
echo "== valkeyStore relevant primitives =="
valkey_file="$(fd -t f 'valkeyStore.ts' server | head -1 || true)"
if [ -n "${valkey_file:-}" ]; then
rg -n -C2 'class ValkeySessionStore|get\(|set\(|del|delKey|get|set|watch|multi|pipeline|eval|exec|lock|LOCK|WATCH|WATCH|CAS|compare|atomic|incr|decr' "$valkey_file" | sed -n '1,260p'
fiRepository: PerryHighCS/ActiveBits
Length of output: 37850
Serialize session mutations with per-session concurrency control.
Mutating routes load the session with sessions.get(...), mutate it in memory, then persist with sessions.set(...). A concurrent join-team request can pass the capacity check while the same team is already being updated, producing over-full teams and lost membership changes. Add a per-session lock or store-side atomic update so the read-modify-write span cannot be interleaved.
🤖 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 `@activities/commissioned-ideas/server/routes.ts` around lines 788 - 810,
Serialize the read-modify-write flow in the join-team handler around participant
lookup, team capacity validation, membership mutation, and sessions.set using
the existing per-session lock mechanism, keyed by sessionId. Ensure concurrent
requests for the same session cannot interleave, while allowing requests for
different sessions to proceed independently.
Comment on lines
+937
to
+967
| const typedSocket = socket as CommissionedIdeasSocket | ||
| typedSocket.sessionId = sessionId | ||
| typedSocket.participantId = query.get('participantId') ?? null | ||
| typedSocket.wantsManager = query.get('role') === 'manager' | ||
|
|
||
| ensureBroadcastSubscription(sessionId) | ||
|
|
||
| ;(async () => { | ||
| const session = await getSession(sessions, sessionId) | ||
| if (!session) { | ||
| socket.send(JSON.stringify({ type: 'commissioned-ideas:error', error: 'Session not found' })) | ||
| socket.close(1008, 'Session not found') | ||
| return | ||
| } | ||
|
|
||
| const participantId = typedSocket.participantId | ||
| if (participantId && session.data.participantRoster[participantId]) { | ||
| const p = session.data.participantRoster[participantId] | ||
| p.connected = true | ||
| p.lastSeen = Date.now() | ||
| await sessions.set(sessionId, session) | ||
| broadcastRegistrationUpdate(ws, sessionId, session.data) | ||
| } | ||
|
|
||
| if (!typedSocket.wantsManager) { | ||
| socket.send(JSON.stringify({ | ||
| type: 'commissioned-ideas:session-state', | ||
| sessionId, | ||
| data: buildStudentSnapshot(session.data, participantId ?? null), | ||
| })) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Websocket binds participantId straight from the query string with no token check — any student can read another's ballot.
Line 939 trusts query.get('participantId'), and that value is then used to build the student snapshot (Line 965), which includes myBallot for that id. This is precisely the attack the /state route refuses to allow (Lines 544-546), and the comment there claims the server "binds participantId at socket-open time" — but no binding/verification happens. The same unverified id also lets a caller flip another participant's connected/lastSeen here and in the close handler (Lines 1010-1023).
Require the participant token (post-connect auth message, mirroring the manager flow) before setting typedSocket.participantId, and drop the id otherwise.
🤖 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 `@activities/commissioned-ideas/server/routes.ts` around lines 937 - 967,
Update the WebSocket connection flow around the CommissionedIdeasSocket
initialization so participantId is not trusted from the query string. Require
and validate the participant token through the existing post-connect
authentication flow, mirroring manager authentication, before assigning
typedSocket.participantId; otherwise leave it null and prevent
participant-specific snapshots or connected/lastSeen updates, including in the
close handler.
Comment on lines
+973
to
+978
| socket.on('message', (raw: unknown) => { | ||
| void (async () => { | ||
| if (typedSocket.isManager || !typedSocket.wantsManager) { | ||
| return | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a socket error listener.
No socket.on('error', …) is registered, so a transport error on a ws socket emits an error event with no listener and surfaces as an uncaught exception. Every socket.send here is also unguarded outside the broadcast helpers.
🛡️ Proposed fix
+ socket.on('error', (err: unknown) => {
+ console.error('[commissioned-ideas] WS socket error', { sessionId, err })
+ })
+
socket.on('message', (raw: unknown) => {📝 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
| socket.on('message', (raw: unknown) => { | |
| void (async () => { | |
| if (typedSocket.isManager || !typedSocket.wantsManager) { | |
| return | |
| } | |
| socket.on('error', (err: unknown) => { | |
| console.error('[commissioned-ideas] WS socket error', { sessionId, err }) | |
| }) | |
| socket.on('message', (raw: unknown) => { | |
| void (async () => { | |
| if (typedSocket.isManager || !typedSocket.wantsManager) { | |
| return | |
| } |
🤖 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 `@activities/commissioned-ideas/server/routes.ts` around lines 973 - 978, Add
an error listener for the socket created in the surrounding connection setup,
alongside the existing socket.on('message', ...) handler, and handle transport
errors without allowing them to surface as uncaught exceptions. Also ensure
direct socket.send calls in this flow are guarded consistently with the existing
broadcast helpers.
Comment on lines
+47
to
+61
| if (amount !== 100 && amount !== 300 && amount !== 500) { | ||
| return { valid: false, error: `Invalid amount: ${String(amount)}` } | ||
| } | ||
|
|
||
| if (amounts.has(amount as number)) { | ||
| return { valid: false, error: `Duplicate amount $${String(amount)}` } | ||
| } | ||
|
|
||
| if (teamIds.has(teamId as string)) { | ||
| return { valid: false, error: 'All three teams must be distinct' } | ||
| } | ||
|
|
||
| amounts.add(amount as number) | ||
| teamIds.add(teamId as string) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reuse isValidBallotAmount instead of re-inlining the amount literals.
100/300/500 is hard-coded here, in isValidBallotAmount (Line 77), and again in normalizeBallots (activities/commissioned-ideas/server/routes.ts Line 184), while BALLOT_AMOUNTS (Line 3) is the intended single source. Narrowing through the type guard also removes the as number / as string casts below.
♻️ Proposed refactor
- if (amount !== 100 && amount !== 300 && amount !== 500) {
+ if (!isValidBallotAmount(amount)) {
return { valid: false, error: `Invalid amount: ${String(amount)}` }
}
- if (amounts.has(amount as number)) {
+ if (amounts.has(amount)) {
return { valid: false, error: `Duplicate amount $${String(amount)}` }
}
- if (teamIds.has(teamId as string)) {
+ if (teamIds.has(teamId)) {
return { valid: false, error: 'All three teams must be distinct' }
}
- amounts.add(amount as number)
- teamIds.add(teamId as string)
+ amounts.add(amount)
+ teamIds.add(teamId)Move isValidBallotAmount above validateBallot (or keep the hoisted function declaration) so the guard is in scope.
📝 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 (amount !== 100 && amount !== 300 && amount !== 500) { | |
| return { valid: false, error: `Invalid amount: ${String(amount)}` } | |
| } | |
| if (amounts.has(amount as number)) { | |
| return { valid: false, error: `Duplicate amount $${String(amount)}` } | |
| } | |
| if (teamIds.has(teamId as string)) { | |
| return { valid: false, error: 'All three teams must be distinct' } | |
| } | |
| amounts.add(amount as number) | |
| teamIds.add(teamId as string) | |
| } | |
| if (!isValidBallotAmount(amount)) { | |
| return { valid: false, error: `Invalid amount: ${String(amount)}` } | |
| } | |
| if (amounts.has(amount)) { | |
| return { valid: false, error: `Duplicate amount $${String(amount)}` } | |
| } | |
| if (teamIds.has(teamId)) { | |
| return { valid: false, error: 'All three teams must be distinct' } | |
| } | |
| amounts.add(amount) | |
| teamIds.add(teamId) | |
| } |
🤖 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 `@activities/commissioned-ideas/shared/validation.ts` around lines 47 - 61,
Update validateBallot to call the existing isValidBallotAmount type guard
instead of hard-coding 100/300/500, moving the guard above validateBallot if
needed for scope. Use the narrowed amount and teamId types so the amounts and
teamIds duplicate checks and additions no longer require casts, while preserving
the existing invalid-amount and duplicate validation behavior.
Comment on lines
+349
to
360
| const allowSessionStorageFallback = rawCreateSessionBootstrap.allowSessionStorageFallback !== false | ||
|
|
||
| if (sessionStorage.length === 0 && historyState.length === 0) { | ||
| return null | ||
| } | ||
|
|
||
| return { | ||
| sessionStorage, | ||
| ...(historyState.length > 0 ? { historyState } : {}), | ||
| ...(allowSessionStorageFallback ? {} : { allowSessionStorageFallback: false }), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Early-return in parseCreateSessionBootstrap can silently drop an explicit allowSessionStorageFallback: false.
When both sessionStorage and historyState end up empty, the function returns null before allowSessionStorageFallback is ever applied. shouldPersistCreateSessionBootstrapPayloadToSessionStorage then treats a null config as "fallback allowed" (undefined !== false → true), silently re-enabling sessionStorage persistence for an activity that explicitly opted out. commissioned-ideas isn't affected today (its historyState is non-empty), but this is a trap for any future activity relying solely on the flag.
As per coding guidelines, "Never store instructor passcodes or manager credentials in sessionStorage... use same-tab router state, in-memory handoff... instead" — this parsing bug could defeat that guarantee for a future activity.
🔧 Proposed fix
const allowSessionStorageFallback = rawCreateSessionBootstrap.allowSessionStorageFallback !== false
- if (sessionStorage.length === 0 && historyState.length === 0) {
+ if (sessionStorage.length === 0 && historyState.length === 0 && allowSessionStorageFallback) {
return null
}This edge case also isn't covered by the tests added in client/src/components/common/manageDashboardUtils.test.ts; worth adding a case for empty sessionStorage/historyState with allowSessionStorageFallback: false.
📝 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
| const allowSessionStorageFallback = rawCreateSessionBootstrap.allowSessionStorageFallback !== false | |
| if (sessionStorage.length === 0 && historyState.length === 0) { | |
| return null | |
| } | |
| return { | |
| sessionStorage, | |
| ...(historyState.length > 0 ? { historyState } : {}), | |
| ...(allowSessionStorageFallback ? {} : { allowSessionStorageFallback: false }), | |
| } | |
| } | |
| const allowSessionStorageFallback = rawCreateSessionBootstrap.allowSessionStorageFallback !== false | |
| if (sessionStorage.length === 0 && historyState.length === 0 && allowSessionStorageFallback) { | |
| return null | |
| } | |
| return { | |
| sessionStorage, | |
| ...(historyState.length > 0 ? { historyState } : {}), | |
| ...(allowSessionStorageFallback ? {} : { allowSessionStorageFallback: false }), | |
| } | |
| } |
🤖 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 `@client/src/components/common/manageDashboardUtils.ts` around lines 349 - 360,
Update parseCreateSessionBootstrap so an explicit
rawCreateSessionBootstrap.allowSessionStorageFallback === false is preserved
even when sessionStorage and historyState are both empty; do not return null in
that opt-out case, and retain the existing null behavior when no payload or flag
is present. Ensure shouldPersistCreateSessionBootstrapPayloadToSessionStorage
continues to reject persistence for the explicit false value, and add coverage
in the related tests for empty storage/state with the flag disabled.
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.
Summary by CodeRabbit