Multi-tab group study side panel with member picker#129
Open
hendriebeats wants to merge 20 commits into
Open
Conversation
…table cells ProseMirror's default up/down navigation follows document order, causing the cursor to jump to the wrong column in the two-column study block layout. The plugin intercepts arrow keys at cell boundaries and navigates visually, preserving horizontal cursor position via coordinate probing. Also fixes a boundary-position bug where posAtCoords could return a position between block siblings, causing TextSelection to throw. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Persists each viewed document's scroll position in localStorage so switching between group members' documents restores where you left off. Also fixes a bug where StopListenToDoc was sent with the new doc ID instead of the old one. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- currentDocId typed as T.DocId | undefined for clarity - editor?.dispatchSteps() null guard against early DocUpdated events - navigateToTarget returns true (consume key) when position unchanged - Remove unnecessary TextSelection cast in navigateOutOfStudyBlocks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix boundary comparisons in resolveLinePos to use >= / <= so cursor positions exactly at block edges are not excluded - Pass navigation direction directly to resolveLinePos instead of hardcoding ±1 so up/down arrows behave symmetrically - Return false (not true) when navigation produces no cursor movement, allowing the event to fall through to the browser - Fix coordsAtPos crash when section header starts on a block boundary by snapping to the first inline position before querying coords - Guard scroll restore against a null currentDocId - Tighten parseInt fallback: treat non-numeric stored values as NaN rather than silently scrolling to 0 - Remove stale console.log - Drop unused EditorStateConfig and Selection imports from Editor.tsx
Use nullish coalescing instead of a ternary when parsing the saved scroll value, keeping the same NaN-fallback behaviour.
If the select element has no selected option, groupStudySelector.value returns an empty string. Without this guard, an invalid doc ID would be sent to the server via ListenToDoc on the first loadEditor call.
To enable a multi-tab side panel, the WebSocket layer needed to track
multiple side documents per connection instead of a single optional one.
- Add DocUpdatedMsg wrapping docId + update so the frontend can route
updates to the correct tab editor
- Replace SocketState.sideDoc (Maybe DocId) with sideDocs ([DocId])
- Handle InStopListenToDoc properly via closeSideDoc (previously fell
through to the catch-all logger)
- Disconnect cleanup now iterates all sideDocs and unsubscribes each
- Frontend DocUpdatedEvent.contents now carries { docId, update }
instead of a bare update value
- Embed groupStudy JSON and pageDocId into study.html via <script> tags
for XSS-safe access from TypeScript
The old side panel only supported one member's document at a time via a dropdown selector. This lays the HTML/CSS foundation for the new multi-tab experience. HTML (study.html): - Replace #splitPicker / p-select / #sideBySideEditor with a tab bar (#tabBar), a "+" add button, a close button, and an empty #splitEditorArea where editors are injected by JS - Move #memberPickerPopover outside #splitside so position:fixed works - Remove the inline split() function (wired via JS in GroupStudy.ts) CSS (study.css): - Rewrite #splitside as a flex column with a sticky header strip and absolutely-positioned tab editor panes (.splitTabEditor) - Add .splitTab styles (active state, close button, label ellipsis) - Add #memberPickerPopover with fixed positioning, z-index 200 - Add mobile override: at ≤768px the panel becomes a fixed full-screen overlay (z-index 300) instead of a grid column
Replaces the single-dropdown side panel with a Chrome-style tab system
where users can open any group member's document in a parallel tab.
Key behaviours:
- Toolbar button opens a member picker popover (position:fixed,
anchored to the button) listing all group members
- Each row shows a checkmark if already open, greyed if no doc yet
- openTab() creates a .splitTabEditor pane and a .splitTab button,
sends ListenToDoc, and activates the tab immediately
- Editors are created lazily on DocListenStart via a FIFO listenQueue
so concurrent opens don't cross-wire responses
- closeTab() sends StopListenToDoc, destroys the editor, and activates
an adjacent tab; closing the last tab collapses the panel
- closeSidePanel() unsubscribes all tabs in one pass
- Scroll position per tab persisted to localStorage under
split-scroll:{pageDocId}:{docId}; saved on tab switch, restored on
activate and after editor mounts
- 4-tier name disambiguation avoids duplicate tab labels for members
sharing a first name
- WS reconnect re-queues ListenToDoc for all currently open tabs
- groupStudy data read from #groupStudyData <script> tag (XSS-safe);
pageDocId from window.pageDocId injected by the template
- Use unsafeRawHtml to embed pre-encoded JSON in the groupStudyData script tag. The Ginger | safe filter is broken in 0.10.5.2 (its asText path returns empty for Text GVals), so we bypass it by setting asHtml directly. </script> sequences are replaced with <\/script> to prevent HTML-parser breakout from the script tag. - Remove the current user from the "No document yet" section of the member picker — their doc is intentionally excluded from allDocs, so they were incorrectly appearing as having no document.
- Replace empty checkmark column with avatar circles (two initials, matching the profile button style at 28×28px) - Show a "Viewing" pill on rows whose tab is already open - Toolbar splitscreen picker stays open for multi-select: click to open or close individual tabs without dismissing the dropdown - Tab-bar plus button filters out already-open members and closes after a single selection (unchanged single-select behaviour) - Fix dismiss handler ignoring clicks on DOM nodes removed during re-render (prevented spurious picker close on row click)
Tabs can now be reordered by dragging within the tab bar. The implementation uses pointer capture for reliable tracking and the FLIP animation technique to smoothly animate displaced siblings into their new positions. A .dragging class lifts the dragged tab with a shadow; dropping snaps it back with a short ease transition. Also fixes the splitscreen button to not pre-select the current user in the member picker (was passing `true` for self-exclusion). CSS: darken tab bar and inactive tab backgrounds slightly for better contrast; tighten the add-tab button to a fixed 34×34 square. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This belongs on the Fix-Table-Arrow-Key-Movement branch. Reverting to keep this branch focused on the multi-tab group study side panel. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Raw scrollTop values are resolution-dependent: text reflows at different screen widths, so a stored pixel offset restores to the wrong place on a different device or window size. - Save the ProseMirror document position nearest the top of the visible area (posAtCoords) instead of scrollTop pixels - Restore by converting the saved pos back to current coordinates (coordsAtPos) and adjusting scrollTop by the delta - Clamp the restored position to the current doc size so stale values from an edited document don't cause an error - Use a new localStorage key prefix (split-scroll-pos:) to avoid collisions with old pixel-based entries Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three correctness fixes identified during review: - Strip email fields from groupStudyJson before embedding in the page. GetUser includes email, which was serialised into the JS data blob visible in page source. A recursive stripEmails pass removes it from the JSON; the server-rendered admin HTML (which legitimately shows member emails to owners) is unaffected. GetUserPublic is added as a public-safe variant for future use. - Add docId to DocListenStart WS payload and route by ID instead of a FIFO listenQueue. If the backend silently dropped a ListenToDoc (doc not found / unauthorised), the queue would desync and subsequent tabs would receive the wrong document. Routing by docId makes each response self-describing and removes the fragile ordering assumption. - Guard against duplicate docId entries in sideDocs on reconnect, and tear down stale editors before re-subscribing so DocListenStart doesn't mount a second ProseMirror instance into the same container. Also: fix nested <button> inside <button> in tab element (invalid HTML that caused browsers to eject the close button from the DOM), switch memberPickerRow to <button> for keyboard accessibility, and add role/tabindex/keydown handling to tab divs.
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
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
Replaces the single-document side panel in the group study view with a Chrome-style multi-tab panel. Members can open any combination of peer documents simultaneously, drag tabs to reorder them, and the panel persists scroll position per document across tab switches and page reloads.
Q&A for code reviewers
WebSocket — multi-doc subscriptions
sideDoc: Maybe DocId→sideDocs: [DocId]— the old state only tracked one side subscription; the new panel supports N open tabs so this becomes a list.DocListenStartnow carriesdocId— previously the frontend matched responses to tabs via a FIFO queue. If the backend silently dropped aListenToDoc(doc not found / unauthorised), the queue would desync and the wrong document would load in each tab. The payload now includes thedocIdso the frontend routes by identity, not position.sideDocsdedup guard — reconnect callsListenToDocfor all open tabs; the guard prevents the samedocIdappearing twice insideDocsif the message races.closeSideDoccalled directly in dispatch —handleStopListenToDocwas a one-line wrapper that just calledcloseSideDoc; inlined to remove the indirection.DocUpdatednow carriesdocId— required so the frontend can route live updates to the correct tab editor rather than broadcasting to all of them.Email exposure in embedded JSON
stripEmailsinStudy.hs—GetUserincludesemail, which was being serialised into thegroupStudyJsonblob embedded in the page<script>tag and visible in page source.stripEmailsrecursively removes theemailkey from the JSON value before embedding. The server-rendered admin page (/group_study/:docId) is unaffected and still shows emails to study owners.unsafeRawHtml+</script>replacement — Ginger's| safefilter produces empty output forTextGVals in ginger-0.10.5.2, sounsafeRawHtmlis the only way to emit raw JSON into a<script>tag.Aeson.encodeescapes all user strings as JSON literals; the only remaining HTML risk is a literal</script>in a document name, which is neutralised by replacing it with<\/script>(semantically identical to a JSON parser).Frontend tab UI
Tab element is a
<div role="tab">not a<button>— a<button>containing another<button>(the close button) is invalid HTML; browsers eject the inner element from the DOM, breaking the close button. The outer element is adivwithrole="tab"and keyboard handlers instead.Member picker rows use
<button>for clickable rows — interactive rows are<button type="button">for proper keyboard and assistive technology support; non-interactive rows (owners with no document yet) remain<div>.Scroll position stored as ProseMirror doc position, not
scrollToppixels — pixel offsets change when text reflows at different viewport widths. Storing the nearest document position (posAtCoords) and restoring viacoordsAtPoskeeps the same paragraph in view regardless of screen size.computeLabelsdisambiguation — tab labels default to first name, falling back to first name + last initial, then first initial + full last name when there are collisions. This avoids showing full names in space-constrained tabs.Reconnect handling
On WS reconnect, stale editors are destroyed before re-subscribing. Without this,
DocListenStartwould mount a second ProseMirror instance into the same container while the original leaked.