Feat/global participant search#289
Draft
Geoshua wants to merge 16 commits into
Draft
Conversation
Geoshua
commented
Apr 24, 2026
Geoshua
commented
Contributor
Implements Phase 1 of the Tease ↔ PROMPT auto import/export workflow so course phases can be opened, edited, and saved directly against PROMPT without the CSV round-trip. - Boot-time entry flow supports ?coursePhaseId=... (launch from PROMPT), a new project picker (when PROMPT is reachable), and the existing CSV fallback (unchanged). - New WorkspaceStateService hydrates from GET /workspace, tracks a dirty flag, autosaves via PUT /workspace (2s debounce), and publishes the finalised draft + allocations atomically via POST /save. - Navigation bar gains a project-switcher dropdown, a save status pill (saving/unsaved/saved), a Save button with a last-saved tooltip, and a "Connect to PROMPT" hint when disconnected. - beforeunload listener warns on unsaved changes. - Export overlay keeps the existing CSV/legacy-export actions; adds a primary Save to PROMPT action.
… entry Removes the duplicate "Save to PROMPT" affordances so the flow is clearer: one explicit publish action in the header, autosave for the draft, and no duplicate buttons in the kebab / export / import overlays. Header: - Rename Save → "Save Teams"; always-enabled while a workspace is active. Clicking publishes workspace + allocations via POST /save. - Status pill rewords: "Saving Workspace..." / "Unsaved changes" / "Workspace Saved". The "Unsaved changes" pill is now clickable and forces an immediate draft save (PUT /workspace) via the new WorkspaceStateService.saveWorkspaceNow() helper. - "Workspace Saved" shows a hover tooltip with the last autosave time. - Save Teams tooltip shows just the last publish time. - All tooltip times use 24-hour format. - Kebab "Save to PROMPT" item removed. Overlays: - Export: drop "Prompt Export (legacy)" section and the now-unused exportPrompt() method (+ its PromptService / ToastsService / CourseIterationsService deps). Rename the remaining primary action heading to "Save Teams" for parity with the header. - Import: drop "Import Data from PROMPT" section. CSV import and example-data flow unchanged. Autosave behaviour is unchanged (2 s debounce on constraints / locks / allocations).
Swap the order in `hydrateFromCoursePhaseId` so `courseIterationsService.setCourseIteration()` runs before `workspaceStateService.hydrate()`. The constraints, locks, and allocations data services route WebSocket broadcasts through `CourseIterationsService.getCourseIteration()`; setting the phase afterwards meant hydration emissions during a project switch were attributed to the previously-selected phase id.
- Annotate every exported/public symbol touched by the workspace integration with short JSDoc blocks so the docstring-coverage CI check passes (services, components, generated API fns, models). - Remove the synchronous `promptConnectionService.isConnected()` guard in `NavigationBarComponent.loadAvailablePhases()`: on the ?coursePhaseId= launch path the workspace hydrates before the background probe resolves, so an early guard could return an empty list on the first dropdown open. `listCoursePhases(true)` already performs the probe and falls back safely when PROMPT is unreachable.
Two small holes closed: - `probe()` now drops `coursePhasesCache` in the no-JWT branch. Without this, a sign-out + re-open flow would mark the connection as disconnected but still keep the previous user's phase list. - `listCoursePhases(false)` only serves from cache when the JWT is still present, otherwise it re-probes. This avoids returning stale phase data to an unauthenticated caller.
Two races in the autosave / saveWorkspaceNow paths: 1. Snapshot-vs-await race: `buildUpsertPayload()` captured state synchronously, but an edit arriving between the snapshot and the PUT response used to be overwritten by the blanket `dirtySubject$.next(false)` — and the extra `markDirty()` that edit fired would be a no-op because the later debounce saw dirty=false. 2. Concurrent saves: two debounces firing back-to-back could both run in parallel, flicker the saving indicator, and allow out-of-order responses to rewind `lastSavedAt` with an older value. Fix: - Add a monotonic `editCounter` bumped in `markDirty()`. Save paths capture the counter before awaiting the PUT; only clear dirty if no new edit arrived. If the counter changed, keep dirty=true and re-arm the autosave debounce so the next state is persisted. - Guard autosave / saveWorkspaceNow against concurrent execution by checking `savingSubject$.getValue()` up front. If a save is already running, schedule another via `autosaveTrigger$.next()` instead of starting a second in-flight PUT. - Factor shared PUT logic into `runPutWorkspace()` to keep autosave and the explicit saveWorkspaceNow() on identical semantics.
Two CodeRabbit findings:
1. Silent autosave failures. Previously a failed PUT only emitted a
console.warn, leaving the pill stuck on "unsaved" — indistinguishable
from normal pending edits. Users could keep editing for hours while
the backend is down, unaware their changes aren't being persisted.
- Track `consecutiveFailures` on `WorkspaceStateService`; after
AUTOSAVE_FAILURE_THRESHOLD (2) consecutive fails, expose a new
`saveFailed$` observable and surface a one-time error toast.
Next successful save resets the streak and (if we'd been failing)
shows a "Save recovered" toast.
- Schedule a time-based retry (`AUTOSAVE_RETRY_BACKOFF_MS = 15s`) so
the service re-attempts saving even without user edits. Timer is
cancelled on success / destroy / reset.
- Nav-bar `saveStatusLabel$` now includes an `'error'` case with a
distinct red pill ("Save failed — retrying", clickable to retry
immediately) so the failure is visible in the UI, not just the logs.
2. `reset()` left stale data in downstream services. Clearing the
workspace without a follow-up hydrate would leave the previous
phase's constraints / locks / allocations rendered in the UI.
- `reset()` now also calls
`constraintsService.setConstraints([], false)` +
`lockedStudentsService.setLocksAsArray([], false)` +
`allocationsService.setAllocations([], false)` (no WebSocket
broadcast — the reset is local-only).
- Also resets the new failure counters and retry timer for symmetry
and doc-comments reset()'s intended use vs. hydrate().
Header changes on the workspace integration branch: - Project switcher dropdown removed. The course-phase title is now a static label — no dropdown, no "Connect to PROMPT to open other projects" hint. (TS observables/methods left in place; they're dead code in the template, easy to revive in a follow-up if we want switching back.) - "Save Teams" button + tooltip relabelled to "Save Allocations" / "Click to save allocations to PROMPT" to match the rest of the UI copy. - "Distribute Projects" CTA shortened to "Distribute".
CodeRabbit findings — both verified, both fixed.
workspace-state.service.ts:
- saveToPrompt() previously snapshotted coursePhaseId before the await
but then blanket-stamped lastSavedAtSubject$ / lastExportedAtSubject$
/ dirtySubject$=false on resolve. Two race holes:
* If the user switched workspaces while POST /save was in flight,
the response stamped state on the new (wrong) workspace.
* If the user kept editing during the in-flight save, dirty was
cleared even though the new edits weren't in the payload.
Mirrored runPutWorkspace's pattern: capture snapshotCoursePhaseId +
snapshotEditCounter, abort with autosaveTrigger$.next() if a save
is already in flight (so two POSTs can't race and flicker the
spinner), and only mutate the timestamp / dirty subjects when both
snapshots still match the current state. Edits that arrived during
the save keep the dirty flag set and re-arm autosave.
- Same workspace-switch guard added to runPutWorkspace itself; the
PUT path had the identical hazard for autosave / saveWorkspaceNow.
- Explicit save now also resets the failure-streak counter on success
so a manual recovery clears the "Save failed" pill state.
navigation-bar.component.scss:
- Replaced `currentColor` with the lowercase `currentcolor` keyword
in the .save-status-dot rule (background + box-shadow) to satisfy
Stylelint's value-keyword-case rule.
Distribute-with-constraints could leave peers with stale or missing
state. Three concrete causes; all fixed:
WebsocketService:
- `send()` now returns a boolean instead of being fire-and-forget. On a
closed STOMP socket it returns false AND flips a new `connected$`
BehaviorSubject to false so observers can react. Wrapped in try/catch
so a thrown stomp error is also surfaced as disconnect.
- `Stomp.client(...)` is configured with `reconnect_delay = 5000` so the
underlying socket retries automatically after drops.
- New `connected$ : Observable<boolean>` and synchronous `isConnected`
accessor for reactive UI bindings.
- `connect()` listens to the underlying socket's close/disconnect hooks
and updates the connection state subject.
CollaborationService:
- After the initial subscribe, watches `connected$` for transitions:
- true → false: surfaces a "Lost connection — edits may not sync"
toast so users notice instead of seeing a green check while
actually offline.
- false → true: rebinds the topic subscriptions and re-runs the
initial state push for the active course iteration. The legacy
CompatClient does not preserve subscriptions across reconnects, so
we have to re-subscribe explicitly.
- Tracks `activeCourseIterationId` for the rebind path; clears it on
explicit disconnect.
ConstraintSummaryComponent.distributeTeams:
- Re-broadcasts the constraint and lock state used by the solve before
pushing the resulting allocation. This makes the distribute payload
effectively atomic at the wire level — peers always receive
constraints + locks no later than the allocation they produced, even
if an earlier broadcast was silently dropped during a brief WS hiccup
or if STOMP delivers across topics out of order.
NavigationBarComponent template:
- Collaboration indicator now binds reactively to
`websocketService.connected$` instead of polling
`connection?.connected`, so reconnect/disconnect events update the UI
without requiring user interaction.
Audit findings on the collaboration distribute-sync branch: 1. `rebindAfterReconnect` was calling `subscribeToAllocations / Locked Students / Constraints` directly. Each of those methods broadcasts the FULL local snapshot to the server BEFORE subscribing. So if a peer made newer edits while we were offline, our auto-reconnect would silently overwrite their state on the server with our stale local state — exactly the data-loss class of bug this PR set out to fix, just shifted to the reconnect path. Fix: rebind delegates to the existing `connect()` flow, which runs the discovery handshake and shows the diff overlay if peer state differs. Same protection cold connect already has. 2. `startConnectionWatch` used `.startWith(true).pairwise()` which assumes the previous state was `true`. This produced a spurious "Lost connection" toast on cold start (true → false transition from the synthetic seed) and could mis-classify the very first real connection as a reconnect. Fix: anchor on `websocketService.isConnected` synchronously, track the previous value manually, and only fire on genuine transitions (`prev !== undefined && prev !== curr`). Both changes preserve the "latter changes win" property the relay's last-payload-per-topic semantics already provide, and stop the reconnect path from violating it.
…sync
`fix/collaboration-distribute-sync` is rebased onto `feat/automated-prompt-tease-data-exchange` (clean replay; one resolved conflict — combined the upstream `NgZone.run` wrapper with the collab branch's
re-broadcast in `distributeTeams`). Two commits on the branch:
- `f8da024a` — original collab fix (re-broadcast in distribute, multi-origin send-returns-bool, reconnect handling)
- `7cc661f1` — audit fixes from this iteration
## What the audit found
Verdict was **ship-with-fixes**. Four issues, two materially affecting "latter changes win":
| # | Finding | Status |
|---|---|---|
| 1 | `rebindAfterReconnect` sent local state before subscribing → could silently clobber a peer's newer edits made while we were offline | **Fixed** — rebind now goes through the full `connect()` flow with discovery + diff overlay |
| 2 | `startConnectionWatch`'s `startWith(true)` would fire a spurious "Lost connection" toast on cold start and mis-classify the first connect as a reconnect | **Fixed** — manual prev-value tracker anchored on `websocketService.isConnected` |
| 3 | Failed `send()` during distribute is silently dropped if the WS is down — no retry queue | Not addressed (out of scope; would need a queueing layer) |
| 4 | Three separate topic broadcasts in `distributeTeams` have no atomic ordering guarantee on the wire | Not addressed (would need a server-side bundled topic) |
## "Latter changes win" property — verified
- **Concurrent distributes:** the server's per-topic `HashMap` slot keeps the LAST frame received. Both clients converge on that. Holds.
- **Drag-drop while peer distributes:** same — last writer wins per topic. Holds.
- **WS hiccup mid-edit:** failed sends are now visible (toast + red pill), reconnect runs discovery, diff overlay surfaces if state diverged. Holds — no longer silent overwrite.
- **Edge:** if you edit offline and a peer also edits, the diff overlay asks you to choose. The "latter changes win" property bends to "user gets to decide", which is the safer default than auto-clobbering.
Adds a header-mounted "Search participants" input that finds people
across the loaded board without forcing the user to scroll teams.
Scope (intentionally narrow):
- First / last / full name (substring, case-insensitive).
- Email (substring, prefix matches on local-part rank slightly higher).
- Skill title — match anyone who has a skill whose title contains the
query (e.g. "docker", "swift").
Other attributes (gender, nationality, language, devices) are filter
concepts and stay in the constraint builder, not the search box.
UX:
- Ctrl/⌘K focuses the input from anywhere in the app.
- 120 ms debounce; minimum 2-character query.
- Up to 10 ranked results; each shows name, email, current team
(or "unallocated"), and a small badge for which field matched.
- Arrow Up/Down navigate, Enter activates, Escape closes; click
outside the component closes too.
- Activating a result scrolls the matching student card into view on
the team-allocation board and runs a 2.5s glow/pulse highlight via a
global `.global-search-highlight` class. The student-card DOM
contract (`id={studentId}`) is the same one used by drag-drop.
Architecture:
- New `GlobalSearchService` ranks matches; pulls from the existing
StudentsService / SkillsService / AllocationsService / ProjectsService
(no new state, no API changes).
- New `StudentHighlightService` owns the scroll-into-view + temporary
CSS class.
- New `GlobalSearchComponent` wraps the input + dropdown.
- Component declared in ComponentsModule and rendered inside the
navigation bar between the constraint indicator and the action
buttons; only mounts when a course iteration is active.
No backend changes. CSV-only mode also benefits — search runs against
whatever students are loaded locally.
Header layout reshuffle to free up the right-hand action cluster: - Workspace status pill (Saving / Unsaved / Saved / Save failed) and the collaboration indicator both move from the right cluster to a small sub-row under the project name on the left. - Collaboration indicator becomes a compact "Live" / "Offline" pill styled to match the workspace pill (same border-radius, font-size, padding) instead of the standalone fa-icon. Click action unchanged. - Right cluster now holds just the primary "Save Allocations" button (no stacked sub-pill), so the row stays single-line at typical widths. - All four workspace pill states harmonised to font-size 0.72rem / padding 0.1rem 0.6rem. The clickable button pills (Unsaved / Save failed) explicitly re-assert font-size + padding so the user-agent button styling no longer enlarges them via `font: inherit`. - Global search resting width trimmed from 14rem to 11rem so the placeholder hugs the input; focus-expansion to 20rem unchanged.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
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.