Iris chat follows the server's conversation model - #375
Open
Predixx wants to merge 52 commits into
Open
Conversation
Tracked deliberately, against the repository's usual rule of keeping specs and plans out of the PR diff, so the work can move between machines. Drop both files before opening the PR.
A scope review found the conversation-first rewrite proportionate but PR 1 overbuilt. Seven mechanisms are cut and recorded in a new normative "Accepted simplifications" section that outranks any stale code example further down: - per-entry picker effect labels, replaced by one static hint - undo and savedPending, leaving an actionless notice - the knownInvisible eviction bound and its ordering - the revalidation loop, replaced by one probe then a fresh conversation - the dashed transcript preview line - _arrivalStamps, replaced by a monotonic union on server message id - _locallyUpdated, replaced by deriving the open row from the loaded detail The loadTicket and sendSeq counters are deliberately kept. Replacing them with exclusion rules would trade two counters in one class for six unwritten invariants across five call sites, with a silent failure mode. Four defects fixed: - topicResolution.ts stays host-only. The webview cannot import @extension/* (ESLint layer boundary), so sharing the resolver would have failed lint. - refreshOverview catches internally and settles successfully. Five callers invoke it as a discarded promise, so a failing overview was an unhandled rejection. The request moved into a helper, which also removes the temporal dead zone on the tracked promise. - SendCoordinator.send takes the full SendInput. The declared parameter type omitted sessionId while the body read it, and no test passed it. - Task 8's serviceWith harness is written out with an explicit pending option. Its reconnect test asserted a context that could never be reached. Also adds a protected contract ledger of the Artemis server facts that no future pruning may contradict, and removes six rounds of review-history tables that documented the document's ancestry rather than any requirement.
Verification of the previous commit found that the surgery itself was not clean: - _resolutionInput still passed the removed `exclude` argument to findSessionFor, which no longer accepts one. Dead leftover of the revalidation loop, and it would not compile. - refreshOverview's cleanup was rewritten to compare courseId instead of request identity. Because _overviewInFlight is a single slot, A1 settling after a switch to B and back to A would clear A2's tracking and the next refresh would duplicate a request that is still open. The record now carries its sequence number and the cleanup compares that. - The navigation notice had a contract, a store field and a component, but nothing raised it. Task 14 now posts it from the SelectTopic and NewConversation dispatcher paths, and deliberately not from OpenConversation, where the student asked for the navigation. Also: upsertMessage advances the detail's lastActivity, and setOverview takes the maximum of the detail and overview values when re-deriving the open conversation's row. Without both, an overview response could walk the history sort order backwards after a context swap. Adds the missing regressions: a genuinely overlapping A-B-A overview race (the previous test resolved the first request before starting the second, so it never exercised the sequence guard), the in-flight cleanup identity, and the 404 branch of revalidation. Clears nine stale prose references to the removed mechanisms.
The overview race tests could not work as written. `resolveCall` scanned by call name and had no notion of settlement, so with A1 and A2 both open it answered A2 twice and the test then awaited A1 forever. The 404 test also called a `rejectCall` that only Task 7's harness extension mentions. `makeApi` now tracks `settled` per deferred and exposes `outstanding(call)`, `resolveCall` (newest outstanding), `resolveOldestCall` (oldest outstanding, which is what makes an A1-B-A2 race expressible) and `rejectCall`. Both harnesses thread them through, and the two race tests assert the number of outstanding requests before answering, so a helper picking the wrong one fails loudly instead of hanging. Also replaces the last stale comment on _install, which still justified capturing the guard before the request in terms of message arrival stamps and the causal merge. Both are gone; the reasons now stated are the ones that still hold, namely request-start mutation guards and request-start ordering.
VS Code 1.131.0 no longer ships `Contents/MacOS/Electron` in its macOS bundle. 2.5.2 hardcodes that path, so `npm run test:unit` died before launching with `spawn .../Contents/MacOS/Electron ENOENT`, on a freshly downloaded archive as well as an existing one. 3.1.0 resolves the executable from the bundle instead, and its own release notes name this exact failure for 1.110+ archives. Verified: the suite now launches against 1.131.0 and runs 1772 tests. CI does not run this label, so the breakage was local only.
`tsc -p . --outDir out` never removes output for a source file that no longer exists, so `out/` accumulates compiled tests from whatever branch was checked out before. After switching from the struggle-v3 lineage to a branch off dev, 120 orphaned test files were still in out/ and mocha ran all of them. They constructed services against APIs that had changed, threw in before-each, and cascaded into 60 failures across unrelated suites, the loudest being sinon's "Attempted to wrap registerCommand which is already wrapped". The suite reported 60 failures purely because of that. With out/ cleaned first: 1420 tests, 0 failures. clean-out is a node one-liner rather than rm -rf so it also works on Windows.
eslint.config.mjs bans upward relative imports across src/, so the two '../context/contextMarkers' imports the plan showed would fail lint as written. Task 3's implementer hit exactly that and had to convert them.
Add IrisConversationService: acquisition, navigation, the single revalidation probe, overview refresh and the guard matrix that decides which async result may still install. Constructed on the provider (guarded, optional) but not routed to anything yet; the old context-first model stays live untouched until the Task 14 cut-over.
…lly land Two lines sent Task 5's implementer looking for work that is not its own. The plan asked Task 5 to create conversation/deps.ts, but sessionSyncUtils.ts survives until Task 15 and IrisServiceDeps still has four live importers, so moving it early would re-point all of them for no benefit. It moves in Task 15, in the commit that deletes its current home. reconcileCurrent was listed under Task 8 while its only caller, onSubscriptionActive, was wired in Task 5. Task 5 therefore cannot compile without the method, so its body belongs there and Task 8 adds the tests and the reconnect plumbing.
Subscribe before adopting the snapshot, mark the reconnect, and guard purely through installDetail's own tuple. Wrapping it in _navigate was wrong: a resubscribe signal can land mid-navigation, and the borrowed token would silently turn the real, user-visible navigation's install into a no-op.
The plan told Task 5 to push the conversation service onto _disposables before the session client so that it would be disposed first. _drainDisposables pops, so disposal is LIFO and pushing first disposes last: the instruction achieved the exact opposite of its own stated intent, and an in-flight install could subscribe to an already-disposed client and leak a STOMP subscription past provider disposal. The intent is unchanged. Only the mechanism that reaches it is corrected, in both the Task 5 and Task 14 statements.
… docs Dispose order was inverted: _drainDisposables pops LIFO, so the service must be pushed AFTER the session client to be disposed BEFORE it, not before. Wrap the API call in newConversation, _acquireForTarget and _createAndStage in try/catch so a server failure resolves to a rejected outcome instead of throwing, matching resolveTopicChange's documented contract. Flag the subscribeToSession adapter's unmet synchronous/latest-wins guarantee for Task 6, and fix two stale doc comments (_install's single check, _probeIn's merged JSDoc). Add a regression test pinning that reconcileCurrent must not take a navigation token.
Task 6's _handleContextSwap posts ShowChatNotice and an addMessage row with role 'contextSwap'. Neither existed, so Task 6 could not compile without adding them, even though the plan lists both under Task 10. Same shape as Task 10 specified, added additively. Noted so Task 10 does not add them a second time.
…title-change notification Task 6 fix round 1: three Important findings from the task review. - irisWebSocketMessageHandler.ts: _handleSessionTitle now calls conversation.notifyChanged() after setTitle(), so a server-side rename reaches the webview instead of waiting for an unrelated emit. - Added a regression test proving _activeConversation stays closed (not merely "does the service exist") while no session is open, which is the exact invariant the dormant period between here and Task 14 depends on. - Rewrote the feedback-edge termination test to re-enter subscribeToSession from inside the onDidResubscribe listener itself, matching the real onSubscriptionActive -> reconcileCurrent -> subscribeToSession edge; the previous sequential-calls version did not exercise the ordering it claimed to and passed even when _subscribedSessionId was set after the fire (verified: that reordering recurses to a stack overflow under the new test, and is caught). - Corrected the class doc on IrisWebSocketSessionClient (no longer rate-limits deliberate navigations) and two stale _subscribeIfConnected references in websocket.test.ts. - resubscribe.test.ts: track and dispose every client the 'onDidResubscribe' describe block creates, so the throw-and-retry test's real setTimeout chain does not keep running after the test ends.
Task 6 fix round 2: replace the em dash in the new comment with a colon. Line 30's pre-existing em dash predates this branch (commit 37a13ca) and is left alone, per project convention that already-shipped content is not this task's to clean up.
Mutation-based review found seven load-bearing guards in SendCoordinator that could be deleted without failing a single test: the origin-session refusal, the fail-bubble session/reason arguments, the workspace entityId comparison, the reconciliation guard's before-the-GET capture, the cross-session upsert check, endSend running only after reconciliation, and the try opening before beginGeneration. One test was fake-green for the exact cross-session scenario it named. Close all of them with tests that are verified to fail under the mutation they claim to catch. Also fixes a real bug: commitContext set only the committed context, never moving the cached detail/summary the way applyContextSwap does, which would leave the chip and history pointing at the old topic until the next CTXSWAP arrives, i.e. exactly while the socket is down.
Task 8. The behaviour itself landed in Task 5: `reconcileCurrent` and its trigger `onSubscriptionActive` could not compile apart, so Task 5 authored both. This commit is therefore tests only, and it closes the gap that left those two methods asserted by nothing but their own subscribe ordering. Adds a `serviceWith` harness (a service plus a SendCoordinator over one open conversation, every field of the starting state an explicit option) and five cases: the accept path of `onSubscriptionActive`, a genuine context change adopted on reconcile including the cached summary the index reads, discard on a CTXSWAP that landed while the GET was in flight, discard of a snapshot that predates an unresolved send, and non-interference with knownInvisible. Two of the planned cases already existed under their own names and were not duplicated: the "signal for a session we already left" reject path and the "subscribes before adopting the snapshot" ordering. Every case was mutation-checked: each one is the only test in this diff that turns red for the guard it names.
The Files list named chatReloadDecision.ts, handleReconnectWebSocket and a rewrite of chatWebviewProviderReconnect.test.ts, but no step in the task body said what to change in any of them. The reconnect path already reaches reconcileCurrent through the converge chain, and the legacy reconnect suite covers provider code this task does not touch, so it migrates with Task 14 like the other twenty suites in spec section 14. Task 8 is therefore test-only, and its git add line is corrected to match.
The no-em-dash rule applies to the plan document too, and the previous commit's own correction text broke it twice.
Task 10: additive wire contracts for the conversation-first Iris chat (courseId/currentSessionId/contentState/etc on updateIrisState, sessionId on the message/rejection payloads, the SelectTopic/OpenConversation/ SwitchCourse/NewConversation commands, and showChatNotice's never-dedupe classification). ChatViewStatePresenter now fills both the old and new shapes from ContextStore and IrisConversationService side by side; nothing is removed and nothing is wired into the dispatcher yet.
…tSnapshot The presenter resolves the header's course name by scanning contextSnapshot.courses, and that builder is on Task 15's delete list. The data survives the v3 migration, only the carrier goes, but courseTitle is an optional wire field so nothing would fail to compile: the course line would just go blank forever.
…e, displayMessageCount and workspaceExerciseId
The webview inferred the new model from the presence of contentState, but the presenter already mirrors that field in every logged-in session, so the new interface rendered against a host that answers none of its commands. Gate it on an explicit conversationFirst flag instead, absent until the dispatcher cuts over. Also: the notice timeout no longer restarts on every parent render, the chip's picker opener is routed so sibling popovers close, the four new dialogs have accessible names, the three copies of the focus trap are one shared hook, ChatMessageList's props are required again, and the whole interface plus the stored marker rows are in English.
…ommands Pairing each new command with its legacy equivalent could not help and could hurt: before the dispatcher cuts over the branches are unreachable, and after it the old post is either dropped or, if the flag lands ahead of the handler removal, acted on as well, turning one click into two context selections. Also name the two legacy dialogs, which are the only ones a user can open at this commit, and fix three comments that quoted German strings the interface no longer renders.
…he topic pick Re-adding a paired legacy post on the new-conversation, course-switch or open-conversation path left the suite green. Parameterise over the four (control, expected command, forbidden legacy command) tuples, driving the real controls with the flag on. The fixture targets a non-current course and conversation, since clicking the current row is defined as "just close" and would post nothing, and each case waits for the new header before clicking: the store update and the flag commit separately, so the first frame still carries the old header, whose plus button is disabled and whose course row opens the legacy picker.
The topic picker sorted exercises by due date descending, sinking the exercise due next to the bottom. The history buckets computed their boundaries by subtracting fixed 24-hour spans, landing on the wrong day across a DST transition. Also adds a last30 bucket between last7 and older.
…tory buckets Task 13's fix commit replaced the pre-existing historyBuckets test suite instead of extending it, leaving five regressions undetectable: the last30 lower boundary drifting a day, both the today and last7 lower boundaries turning exclusive, and newest-first ordering within a bucket (including a silent flip to oldest-first) all surviving with a green suite. Restores and adapts the minimal set of boundary and ordering cases needed to catch them, adds the last30 upper/lower boundary cases this task introduced but never exercised precisely, and pins render-level coverage for the Last 30 days heading. Also corrects two stale doc comments left over from the DAY_MS removal and the sentinel guard's actual scope.
The Ask-Iris commands go through IrisConversationService.resolveTopicChange with the course id travelling alongside the target, so a fresh window can still acquire a conversation instead of answering no-course. courseIdResolver is keyed on an exercise id rather than an ActiveContext. artemis.resetIrisChat keeps its id but reloads instead of clearing: nothing local owns conversations any more, so there is nothing destructive to confirm. Struggle detection now follows the workspace exercise, not the chat topic. A topic change used to retarget the detector at an exercise whose code is not open.
The dispatcher answers selectTopic, openConversation, switchCourse and newConversation and stops answering selectChatContext, switchSession, openArtemisSession, createNewSession and switchToWorkspaceContext. The presenter sets conversationFirst on the same commit, because the new interface posts only the new commands: a flag without handlers leaves every navigation control dead, handlers without the flag leave it invisible. Sends go through SendCoordinator, with the availability check still in front of it and the bubble always addressed in the conversation it was drawn in. A send that no conversation can carry now fails its bubble instead of leaving it stuck in sending. Navigation is refused host-side while a send is in flight; the webview's streaming state resets on disconnect, so UI gating is not an invariant. A topic pick that replaced the transcript, and a new conversation, raise a notice; a staged topic and an explicitly opened one do not. The store clears a notice on a real navigation only, so the overview refresh that follows the same navigation does not wipe it a round trip later.
…rkspace The course-wide history stops filtering by mode. A lecture or text-exercise conversation can never be a topic, but it can be opened by id, and hiding it made prior conversations unreachable rather than read-only. Adds a chat-side refreshCourses command so the course picker can fetch the dashboard list on a fresh installation and show a loading state while it does. WebviewCmd.ReloadCourses is not the answer: the chat provider never sees it, and its handler navigates the main panel to the course list.
Every install delivers its messages: start, reload, navigateTo, switchCourse, newConversation and a topic pick that opens another conversation all render what they loaded, and reconnect reconciliation merges instead of replacing. Delivery happens after the emit, so a transcript never overtakes the snapshot that names the conversation it belongs to. Websocket frames and the run-UI projection carry the conversation id, and the webview keys the transcript on it. Answers used to be attributed to the old model's local session, so they landed under the previous conversation's transcript. There is now exactly one acquisition and one subscription: the old session import no longer runs beside the conversation model, and registering courses no longer auto-selects a context. Cold start was telling the student there was nothing to talk about while an auto-selected context had already created a server session, subscribed the socket to it and posted its transcript. Availability follows the conversation's course rather than a selected context that no longer moves, and a bubble is addressed from the origin session argument rather than from provider state.
The reconnect marker was keyed on the old model's local session id and compared against a store entry the conversation-first host never fills, so the whole path was dead: after a websocket drop mid-answer the run stayed waiting until the next navigation, and its test suite was green over code production could not enter. Recovery now has ONE owner. A resubscribe re-reads and merges the conversation through the service, then the provider resolves the run on conclusive proof and republishes clean run UI. The baseline is keyed on the conversation like everything else, and the marker's separate fetch, its single-flight coalescing and the second onDidResubscribe subscription are gone. The reconnect suite drives that path end to end. Message feedback resolves the Artemis session from the conversation; it used to look it up in the old model's empty session list, so every thumbs click was a silent no-op while the buttons stayed rendered. A reload re-checks availability and clears the banner that sent the student to the Retry button. A reload re-installs the same conversation, so the navigation hook could never clear it and the composer stayed disabled. Also: the course picker tracks that its refresh was answered rather than that it was non-empty, so a student with no courses reaches "No courses found"; and a send that failed with no-context is retryable once a conversation is open.
…server did not
Iris availability had no proactive caller left: `_refreshAvailability` was
reachable only from the reload command, while `_onConversationChanged` HID
both banners on every navigation and never re-asked. A student in a course
where Iris is disabled saw a working chat and learned otherwise only when
their first message failed. The check now runs on every conversation change
and after `start()` (a view re-open re-installs the same conversation, so the
id guard early-returns there), and an answer that outlived its conversation is
discarded rather than published against the course the student moved to.
`_toSessionDetail` builds `context: { mode, entityId }` and never sets `name`,
so on every load path the chip read the literal word "Topic" and the history
labelled an exercise conversation "Course chat". The presenter now fills the
name from the tracked exercises, and both surfaces fall back mode-aware
through one shared helper instead of one constant.
Also spec 5.4: the history is `courseSessions` UNION `knownInvisible`. Only
the overview reached the webview, so the conversation you are in was absent
from its own history until it had a user message.
Fixtures on every path now use the nameless shape the host actually produces.
… orphaned
A rejected topic change and a rejected new conversation were dropped on the
floor: the producers document the contract ("a 500 here must become a notice"),
the consumer acted only on `opened`. Fixing that alone would still have been
silent, because the topic picker closes on the click and the header's `+` has
no popover, so `openSessionError` had no renderer on either path. Those two now
answer on the composer's notice line, which gains an error tone; the two
popover-backed navigations keep their inline banner.
`artemis.checkIrisHealth` read a course id that only the course picker ever
wrote, so on the normal path it answered "select a course or exercise context"
(an affordance this rewrite removed) about a chat that was plainly showing one.
The mirror in ContextStore is deleted rather than kept in sync: the
conversation is the course now, and the command reads it from there.
The in-product help still taught the deleted model ("each context has multiple
sessions", "switch between sessions using the context selector dropdown"). It
now describes one conversation per course, the topic chip, the history popover
and the reload escape hatch, and the side-menu item is named after what the
command actually does.
Diagnostics lost every conversation fact when the old context/session blocks
went, which was the stated justification for deleting "Debug Sessions (Raw)".
It prints the conversation snapshot again.
The course picker's Escape and focus trap were dead exactly while it is
loading, which is the fresh-install path: no focusable child existed, and the
one-shot effect never re-ran when the rows arrived. The shared popover key
hook, which had no coverage at all, is now pinned. The topic picker captures
the popover session guard like its two siblings, so a background navigation
cannot re-scope its rows to another course under the cursor.
Comments that describe code which no longer exists: the websocket handler's
"owned by the deferred reconnect-reconciliation work" (that work landed and
`_recoverOnResubscribe` owns it), `historyBuckets`' reference to the deleted
`buildCourseHistory`, and `pickerSort`'s "Choose another course..." list, which
the picker never grew.
`IrisWebSocketSessionClient.resetSession` has had no caller in src since the
conversation model took over the subscription, and its own comment described a
context switch that no longer works that way. Its one test goes with it.
`ContextPicker.conversations` was passed on every open and never destructured,
a cut-1 corpse whose JSDoc justified it with a shared prop bag that nothing
spreads. `ConversationSummary.mode` and `.entityId` stay on the contract:
`ConversationHistory` reads both for its mode-aware label.
Dead CSS in four modules, and the German strings this branch added in tests
("Unbekannter Fehler", "Woche 3"), which contradict the English-only decision.
Also deleted the "renders no preview line" test: it asserts the absence of a
testid that exists in no code path, so it can never fail.
The same shape this round set out to eliminate: a producer and a renderer with
nothing between them. The host posts `showChatNotice` with `tone: 'error'` for
a refused topic change and a failed new conversation, and the webview's message
handler rebuilt the notice as `{ text }` only. So `tone` was always undefined,
`ChatNotice`'s error branch was unreachable in production, and the one surface
those two refusals have rendered as a muted grey aside with role="status".
Pinned end to end (dispatch the host message, assert the alert), not only at
the component, because a component test cannot see a handler that drops a
field.
Four one-liners alongside it:
- the history row's `?? summary.context` was dead; `_named` answers undefined
only for an undefined input, which the signature now states outright
- the diagnostics conversation getter no longer defaults to "no conversation",
where a future wiring mistake would have shown up as a silently missing
section rather than a compile error
- `@keyframes skeleton-pulse` outlived every class that used it
- the side-menu item and the modal title now name the document they open
Picking a course whose Iris is disabled reported "Could not open that course. Please try again." Artemis answers that case with 403 and errorKey `iris.course_disabled`, but the API layer dropped the key while parsing, so every failure looked transient. The advice was also wrong: only an instructor can lift this one. ApiError now carries the server's errorKey, parsed separately from the fallback chain over human-facing fields, since that chain degrades to prose the moment Artemis stops sending `message`. Both course-scoped navigations, the course switch and a history row, name the real reason. Deliberately no persistent banner: the navigation failed, so the previous course is still the active one, and the disabled state belongs to the course you are actually in.
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.
Closes #373.
Why
Artemis changed the Iris chat session model upstream:
POST /api/iris/chat/sessionsnow takes only acourseIdand always creates a course session, and the endpoint the extension used to create context-scoped sessions is gone. Release 0.4.8 is therefore broken against production for roughly 940 installs: opening an exercise that has no conversation yet cannot start one.The fix is not a patched call. The two sides disagree about what owns what. The extension said "a context owns many local sessions"; Artemis says "a conversation has a mutable context". Three writers change a session's context: this client, another client, and Artemis itself, whose
onBuildFailureandonNewResultlisteners repoint sessions with no client involvement at all. So the client has to stop treating context as something it assigns and start treating it as something it observes.What changes
One server conversation at a time, whose context the server may change under us.
sessions/current, withsessions(create) only when there is nothing to adopt.CTXSWAP) are decoded and rendered as transcript dividers, so a repoint by another client or by Artemis itself is visible rather than silent.ActiveContext, the local session store, and their machinery are gone, and the persisted store migrates to v3 keeping tracked exercises and courses.Ordering is defended explicitly, because an unchanged local
contextRevisiondoes not imply an unchanged server context: a repoint that happens during a websocket disconnect never arrives as a frame. Four counters carry that weight (navigationGeneration,loadTicket,contextRevision,sendSeq), and websocket frames carry their source session so a frame for a conversation we have left cannot land in the one we are looking at.Scope
Deliberately cut from this PR, to be picked up in the follow-up: undo for an unexpected navigation, per-entry effect labels in the picker, and deletion semantics for messages removed on the server. Proactive hints are the next PR and are absent here by design.
An Artemis-side change is tracked separately: the overview query filters on
m.sender = USER, so a conversation Iris started after a failed build is invisible in the overview. Optional here (a client-side cache covers the process lifetime), but a dependency for the proactive-hints PR, where an unreachable hint is a failed intervention.Verification
check-types,lintandknipclean. 1374 host tests and 1203 webview tests passing, 0 failing. Both production bundles build (desktop and Open VSX).Tests were checked by mutation rather than by coverage: for each assertion, the defect it claims to catch was introduced, the test confirmed red, and the change reverted. That found several tests that asserted nothing, and two defects that a fully green suite could not see, because the fixtures supplied data the host never produces.
Manual smoke test against a live Artemis
Not automatable, and not yet run. This is the remaining gate.
+and pick a topic. Both produce a visible response, not a silent no-op.artemis.checkIrisHealthfrom the palette in a plain workspace-exercise session, without touching the course picker first.