From c8a75666f05472118aadfce26332f629f6e42339 Mon Sep 17 00:00:00 2001 From: karrisanthoshigayatri Date: Wed, 22 Jul 2026 19:41:14 +0530 Subject: [PATCH 01/10] Add keyboard shortcuts to frontend shell #1588 --- frontend/index.html | 60 +++++++++++++++++++++++++++--- frontend/tests/shortcut-ui.test.js | 37 ++++++++++++++++++ 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/frontend/index.html b/frontend/index.html index 9a18c400..e2d1c5fe 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -2611,6 +2611,7 @@ API Docs - - - - + + + + @@ -2803,6 +2804,9 @@

Debug. Understand.
Ship faster.

/Focus editor Ctrl/⌘ + EnterAnalyze EscLeave editor + Ctrl/⌘ + KClear editor + Ctrl/⌘ + Shift + TToggle theme + Alt + 1–4Switch mode @@ -4008,6 +4012,52 @@

Debug. Understand.
Ship faster.

when: () => document.activeElement === editor, handler: () => editor.blur(), }, + // Ctrl/⌘ + K — clear the editor + { + key: 'k', + mod: true, + handler: () => document.getElementById('clearBtn').click(), + }, + // Ctrl/⌘ + Shift + T — toggle theme + { + key: 't', + mod: true, + shift: true, + handler: () => document.getElementById('themeToggle').click(), + }, + // Alt + 1–4 — switch analysis mode + { + key: '1', + alt: true, + handler: () => { + const btn = document.querySelector('.mode-btn[data-mode="analyze"]'); + if (btn) btn.click(); + }, + }, + { + key: '2', + alt: true, + handler: () => { + const btn = document.querySelector('.mode-btn[data-mode="explain"]'); + if (btn) btn.click(); + }, + }, + { + key: '3', + alt: true, + handler: () => { + const btn = document.querySelector('.mode-btn[data-mode="debug"]'); + if (btn) btn.click(); + }, + }, + { + key: '4', + alt: true, + handler: () => { + const btn = document.querySelector('.mode-btn[data-mode="suggest"]'); + if (btn) btn.click(); + }, + }, ]); async function doAnalyze() { diff --git a/frontend/tests/shortcut-ui.test.js b/frontend/tests/shortcut-ui.test.js index 6a6dc586..8dcc4606 100644 --- a/frontend/tests/shortcut-ui.test.js +++ b/frontend/tests/shortcut-ui.test.js @@ -6,6 +6,7 @@ import { fileURLToPath } from 'node:url'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const indexHtml = fs.readFileSync(path.resolve(__dirname, '..', 'index.html'), 'utf8'); +// ── Original shortcuts ────────────────────────────────────────────────────── assert.match(indexHtml, /class="shortcut-hints"/, 'shortcut hints should be visible in the frontend'); assert.match(indexHtml, /Ctrl\/⌘ \+ Enter<\/kbd>/, 'analyze shortcut should be displayed'); assert.match(indexHtml, /\/<\/kbd>/, 'editor focus shortcut should be displayed'); @@ -15,3 +16,39 @@ assert.match( /aria-keyshortcuts="Control\+Enter Meta\+Enter"/, 'analyze shortcut should be exposed to assistive technology', ); + +// ── New shortcuts ─────────────────────────────────────────────────────────── +assert.match(indexHtml, /Ctrl\/⌘ \+ K<\/kbd>/, 'clear shortcut should be displayed'); +assert.match(indexHtml, /Ctrl\/⌘ \+ Shift \+ T<\/kbd>/, 'theme toggle shortcut should be displayed'); +assert.match(indexHtml, /Alt \+ 1–4<\/kbd>/, 'mode switch shortcut should be displayed'); + +assert.match( + indexHtml, + /aria-keyshortcuts="Control\+K Meta\+K"/, + 'clear shortcut should be exposed to assistive technology on clearBtn', +); +assert.match( + indexHtml, + /aria-keyshortcuts="Control\+Shift\+T Meta\+Shift\+T"/, + 'theme toggle shortcut should be exposed to assistive technology on themeToggle', +); +assert.match( + indexHtml, + /aria-keyshortcuts="Alt\+1"/, + 'analyze mode shortcut should be exposed to assistive technology', +); +assert.match( + indexHtml, + /aria-keyshortcuts="Alt\+2"/, + 'explain mode shortcut should be exposed to assistive technology', +); +assert.match( + indexHtml, + /aria-keyshortcuts="Alt\+3"/, + 'debug mode shortcut should be exposed to assistive technology', +); +assert.match( + indexHtml, + /aria-keyshortcuts="Alt\+4"/, + 'suggest mode shortcut should be exposed to assistive technology', +); From 3fd325d61e0affba8f72bf8aedb5d08ccd4a25ea Mon Sep 17 00:00:00 2001 From: karrisanthoshigayatri Date: Wed, 22 Jul 2026 19:48:18 +0530 Subject: [PATCH 02/10] Surface empty states in frontend shell #1587 --- frontend/index.html | 78 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 74 insertions(+), 4 deletions(-) diff --git a/frontend/index.html b/frontend/index.html index e2d1c5fe..246fc41b 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -2049,6 +2049,42 @@ color: var(--text3); } + /* Richer empty state for history/favorites panels */ + .list-empty-state { + display: flex; + flex-direction: column; + align-items: center; + justify-content: center; + padding: 32px 16px; + gap: 10px; + text-align: center; + } + + .list-empty-state .list-empty-icon { + width: 40px; + height: 40px; + border-radius: 10px; + background: var(--bg3); + border: 1px solid var(--border); + display: flex; + align-items: center; + justify-content: center; + color: var(--text3); + } + + .list-empty-state .list-empty-title { + font-family: var(--font-disp); + font-size: 0.9rem; + font-weight: 700; + color: var(--text); + } + + .list-empty-state .list-empty-desc { + font-size: 0.78rem; + color: var(--text2); + max-width: 200px; + } + /* ═══════════════════════════════════════════════════════════════ API CONFIG ═══════════════════════════════════════════════════════════════ */ @@ -2960,7 +2996,15 @@

Debug. Understand.
Ship faster.

-
No history yet. Run your first analysis.
+
+ +

No history yet

+

Run your first analysis to see it here

+
@@ -2974,7 +3018,15 @@

Debug. Understand.
Ship faster.

-
No favorites saved yet.
+
+ +

No favorites saved

+

Star an analysis result to save it here

+
@@ -4992,7 +5044,16 @@

Project Health Score

if (!listContainer) return; if (!history || !history.length) { - listContainer.innerHTML = `
${getTranslation('empty_history')}
`; + listContainer.innerHTML = ` +
+ +

${getTranslation('empty_history_title')}

+

${getTranslation('empty_history_desc')}

+
`; return; } @@ -5258,7 +5319,16 @@

Project Health Score

function renderFavorites() { if (!favorites.length) { - favList.innerHTML = `
${getTranslation('empty_favorites')}
`; + favList.innerHTML = ` +
+ +

${getTranslation('empty_favorites_title')}

+

${getTranslation('empty_favorites_desc')}

+
`; return; } favList.innerHTML = favorites.map(f => { From c00a8ba4f3f67cec14cbb1e09bc2f2887482dae6 Mon Sep 17 00:00:00 2001 From: karrisanthoshigayatri Date: Wed, 22 Jul 2026 19:51:17 +0530 Subject: [PATCH 03/10] Surface empty states in frontend shell #1587 --- frontend/index.html | 16 +++ frontend/package.json | 2 +- frontend/tests/empty-states.test.js | 176 ++++++++++++++++++++++++++++ 3 files changed, 193 insertions(+), 1 deletion(-) create mode 100644 frontend/tests/empty-states.test.js diff --git a/frontend/index.html b/frontend/index.html index 246fc41b..c907d563 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -3129,6 +3129,8 @@

Debug. Understand.
Ship faster.

history_title: "Query History", favorites_title: "Saved Favorites", empty_history: "No history yet. Run your first analysis.", + empty_history_title: "No history yet", + empty_history_desc: "Run your first analysis to see it here", history_sort_label: "Sort:", history_sort_date: "Date", history_sort_score: "Quality Score", @@ -3137,6 +3139,8 @@

Debug. Understand.
Ship faster.

history_order_desc: "Desc", history_order_asc: "Asc", empty_favorites: "No favorites saved yet.", + empty_favorites_title: "No favorites saved", + empty_favorites_desc: "Star an analysis result to save it here", btn_clear: "Clear", footer_license: "MIT License", footer_built_for: "Built for the open source community", @@ -3237,6 +3241,8 @@

Debug. Understand.
Ship faster.

history_title: "வினவல் வரலாறு", favorites_title: "சேமிக்கப்பட்ட பிடித்தவை", empty_history: "இன்னும் வரலாறு இல்லை. உங்கள் முதல் பகுப்பாய்வை இயக்கவும்.", + empty_history_title: "இன்னும் வரலாறு இல்லை", + empty_history_desc: "இங்கே காண உங்கள் முதல் பகுப்பாய்வை இயக்கவும்", history_sort_label: "வரிசைப்படுத்து:", history_sort_date: "தேதி", history_sort_score: "தர மதிப்பெண்", @@ -3245,6 +3251,8 @@

Debug. Understand.
Ship faster.

history_order_desc: "இறங்குவரிசை", history_order_asc: "ஏறுவரிசை", empty_favorites: "இன்னும் பிடித்தவை எதுவும் சேமிக்கப்படவில்லை.", + empty_favorites_title: "பிடித்தவை எதுவும் இல்லை", + empty_favorites_desc: "ஒரு பகுப்பாய்வு முடிவை நட்சத்திரமிட்டு இங்கே சேமிக்கவும்", btn_clear: "அழி", footer_license: "எம்ஐடி உரிமம் (MIT License)", footer_built_for: "திறந்த மூல சமூகத்திற்காக உருவாக்கப்பட்டது", @@ -3345,6 +3353,8 @@

Debug. Understand.
Ship faster.

history_title: "क्वेरी इतिहास", favorites_title: "सहेजे गए पसंदीदा", empty_history: "अभी तक कोई इतिहास नहीं है। अपना पहला विश्लेषण चलाएं।", + empty_history_title: "अभी तक कोई इतिहास नहीं", + empty_history_desc: "इसे यहाँ देखने के लिए अपना पहला विश्लेषण चलाएं", history_sort_label: "क्रमबद्ध करें:", history_sort_date: "तारीख", history_sort_score: "गुणवत्ता स्कोर", @@ -3353,6 +3363,8 @@

Debug. Understand.
Ship faster.

history_order_desc: "अवरोही", history_order_asc: "आरोही", empty_favorites: "अभी तक कोई पसंदीदा सहेजा नहीं गया है।", + empty_favorites_title: "कोई पसंदीदा नहीं", + empty_favorites_desc: "किसी विश्लेषण को यहाँ सहेजने के लिए उसे स्टार करें", btn_clear: "साफ करें", footer_license: "एमआईटी लाइसेंस (MIT License)", footer_built_for: "ओपन सोर्स समुदाय के लिए निर्मित", @@ -3453,6 +3465,8 @@

Debug. Understand.
Ship faster.

history_title: "Historique des requêtes", favorites_title: "Favoris enregistrés", empty_history: "Aucun historique pour le moment. Lancez votre première analyse.", + empty_history_title: "Aucun historique", + empty_history_desc: "Lancez votre première analyse pour la voir ici", history_sort_label: "Trier par :", history_sort_date: "Date", history_sort_score: "Score de qualité", @@ -3461,6 +3475,8 @@

Debug. Understand.
Ship faster.

history_order_desc: "Décroissant", history_order_asc: "Croissant", empty_favorites: "Aucun favori enregistré pour le moment.", + empty_favorites_title: "Aucun favori enregistré", + empty_favorites_desc: "Mettez une étoile sur un résultat pour le sauvegarder ici", btn_clear: "Effacer", footer_license: "Licence MIT", footer_built_for: "Conçu pour la communauté open source", diff --git a/frontend/package.json b/frontend/package.json index 0bba672c..241ca978 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -4,7 +4,7 @@ "version": "0.0.0", "description": "Playwright end-to-end tests for the QyverixAI frontend.", "scripts": { - "test:static": "node tests/sample-comments.test.js && node tests/shortcut-ui.test.js", + "test:static": "node tests/sample-comments.test.js && node tests/shortcut-ui.test.js && node tests/empty-states.test.js", "test:e2e": "playwright test", "test:e2e:ci": "playwright test --reporter=list", "test:e2e:headed": "playwright test --headed" diff --git a/frontend/tests/empty-states.test.js b/frontend/tests/empty-states.test.js new file mode 100644 index 00000000..dcda6507 --- /dev/null +++ b/frontend/tests/empty-states.test.js @@ -0,0 +1,176 @@ +/** + * Static assertions that all expected empty-state elements are present + * in index.html with correct structure, roles, and i18n attributes. + * + * Run with: node tests/empty-states.test.js + */ +import assert from 'node:assert'; +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const html = fs.readFileSync(path.resolve(__dirname, '..', 'index.html'), 'utf8'); + +// ── Result-panel empty states ──────────────────────────────────────────────── + +assert.match( + html, + /id="emptyExplain"/, + 'explain pane should have an emptyExplain empty-state element', +); +assert.match( + html, + /id="emptyDebug"/, + 'debug pane should have an emptyDebug empty-state element', +); +assert.match( + html, + /id="emptySuggest"/, + 'suggest pane should have an emptySuggest empty-state element', +); + +// Each result-pane empty state should use the .empty-state class +assert.match( + html, + /id="emptyExplain"[^>]*>[\s\S]*?class="empty-state"|class="empty-state"[^>]*>[\s\S]{0,200}id="emptyExplain"|id="emptyExplain"[\s\S]{0,50}empty-state/, + 'emptyExplain should use empty-state styling', +); + +// Each result pane empty state should have a human-readable title +assert.match( + html, + /data-i18n="empty_explain_title"/, + 'explain empty state should have an i18n title', +); +assert.match( + html, + /data-i18n="empty_debug_title"/, + 'debug empty state should have an i18n title', +); +assert.match( + html, + /data-i18n="empty_suggest_title"/, + 'suggest empty state should have an i18n title', +); + +// Each result pane empty state should have a description +assert.match( + html, + /data-i18n="empty_explain_desc"/, + 'explain empty state should have an i18n description', +); +assert.match( + html, + /data-i18n="empty_debug_desc"/, + 'debug empty state should have an i18n description', +); +assert.match( + html, + /data-i18n="empty_suggest_desc"/, + 'suggest empty state should have an i18n description', +); + +// Icons in result pane empty states should be aria-hidden +assert.match( + html, + /class="empty-icon"[^>]*aria-hidden="true"|aria-hidden="true"[^>]*class="empty-icon"/, + 'result pane empty-state icons should be aria-hidden', +); + +// ── History empty state ────────────────────────────────────────────────────── + +assert.match( + html, + /id="emptyHistory"/, + 'history panel should have an emptyHistory empty-state element', +); +assert.match( + html, + /class="list-empty-state"[\s\S]{0,400}id="emptyHistory"|id="emptyHistory"[\s\S]{0,20}class="list-empty-state"|id="emptyHistory"/, + 'emptyHistory should use list-empty-state styling', +); +assert.match( + html, + /class="list-empty-title"/, + 'history and favorites empty states should have a list-empty-title element', +); +assert.match( + html, + /class="list-empty-desc"/, + 'history and favorites empty states should have a list-empty-desc element', +); +assert.match( + html, + /class="list-empty-icon"[^>]*aria-hidden="true"|aria-hidden="true"[^>]*class="list-empty-icon"/, + 'list empty-state icons should be aria-hidden', +); + +// ── Favorites empty state ──────────────────────────────────────────────────── + +assert.match( + html, + /id="emptyFavorites"/, + 'favorites panel should have an emptyFavorites empty-state element', +); + +// ── i18n keys exist in the translations object ─────────────────────────────── + +assert.match( + html, + /empty_history_title:/, + 'translations should include empty_history_title key', +); +assert.match( + html, + /empty_history_desc:/, + 'translations should include empty_history_desc key', +); +assert.match( + html, + /empty_favorites_title:/, + 'translations should include empty_favorites_title key', +); +assert.match( + html, + /empty_favorites_desc:/, + 'translations should include empty_favorites_desc key', +); + +// ── CSS classes exist ──────────────────────────────────────────────────────── + +assert.match( + html, + /\.list-empty-state\s*\{/, + 'CSS should define .list-empty-state rule', +); +assert.match( + html, + /\.list-empty-icon\s*\{/, + 'CSS should define .list-empty-icon rule', +); +assert.match( + html, + /\.list-empty-title\s*\{/, + 'CSS should define .list-empty-title rule', +); +assert.match( + html, + /\.list-empty-desc\s*\{/, + 'CSS should define .list-empty-desc rule', +); + +// ── aria-live regions ──────────────────────────────────────────────────────── + +assert.match( + html, + /id="historyList"[^>]*aria-live="polite"|aria-live="polite"[^>]*id="historyList"/, + 'history list should have aria-live="polite" for screen reader announcements', +); +assert.match( + html, + /id="favList"[^>]*aria-live="polite"|aria-live="polite"[^>]*id="favList"/, + 'favorites list should have aria-live="polite" for screen reader announcements', +); + +console.log('All empty-state assertions passed.'); From b0aa2bd74c230c44cbc06448893d21122b6fbc35 Mon Sep 17 00:00:00 2001 From: karrisanthoshigayatri Date: Wed, 22 Jul 2026 19:57:04 +0530 Subject: [PATCH 04/10] Expose observability in presence sync #1583 --- .../observability-presence-sync/.config.kiro | 1 + .../requirements.md | 120 ++++++++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 .kiro/specs/observability-presence-sync/.config.kiro create mode 100644 .kiro/specs/observability-presence-sync/requirements.md diff --git a/.kiro/specs/observability-presence-sync/.config.kiro b/.kiro/specs/observability-presence-sync/.config.kiro new file mode 100644 index 00000000..6f701345 --- /dev/null +++ b/.kiro/specs/observability-presence-sync/.config.kiro @@ -0,0 +1 @@ +{"specId": "7bc28dcb-944b-43d9-9dd2-3e507e1e4a76", "workflowType": "requirements-first", "specType": "feature"} diff --git a/.kiro/specs/observability-presence-sync/requirements.md b/.kiro/specs/observability-presence-sync/requirements.md new file mode 100644 index 00000000..2732b1e3 --- /dev/null +++ b/.kiro/specs/observability-presence-sync/requirements.md @@ -0,0 +1,120 @@ +# Requirements Document + +## Introduction + +The presence sync subsystem in QyverixAI handles real-time collaborative editing over WebSocket connections. Currently, the `CollaborationManager` and its `CollaborationRoom` objects process connection lifecycle events, code updates, cursor movements, and comments with no structured logging, metrics, or error-visibility hooks. This feature adds observability to the presence sync flow — structured logs at key lifecycle points, Prometheus counters/gauges for WebSocket and collaboration events, and consistent error-event visibility — without altering any existing collaboration behaviour. + +## Glossary + +- **Collaboration_Router**: The FastAPI `APIRouter` defined in `routers/collaboration.py` that handles the `/ws/{session_id}` WebSocket endpoint. +- **Collaboration_Manager**: The `CollaborationManager` class that manages rooms, connections, message dispatch, and broadcasts. +- **Presence_Sync**: The process of tracking and broadcasting connected-user state (join, leave, cursor position) across all participants in a collaboration room. +- **Observability_Layer**: The set of structured log statements, Prometheus metrics counters/gauges, and error-visibility hooks added by this feature. +- **collaboration_logger**: The Python logger instance named `app.routers.collaboration` used exclusively by the Collaboration_Router and Collaboration_Manager. +- **Prometheus_Registry**: The `prometheus_client` default registry already used in `observability.py`. +- **Session**: A named collaboration room identified by a `session_id` string. +- **Client**: A single WebSocket connection within a Session, identified by a `client_id` string. + +--- + +## Requirements + +### Requirement 1: Structured Connection Lifecycle Logging + +**User Story:** As an operator, I want structured log entries for every WebSocket connection and disconnection event, so that I can trace collaboration sessions and diagnose connectivity problems. + +#### Acceptance Criteria + +1. WHEN a Client connects to a Session, THE collaboration_logger SHALL emit an INFO-level log entry containing the `session_id`, `client_id`, and the current connected-user count for that Session. +2. WHEN a Client disconnects from a Session, THE collaboration_logger SHALL emit an INFO-level log entry containing the `session_id`, `client_id`, a disconnect reason field (either `"websocket_disconnect"` or `"stale_socket"`), and the remaining connected-user count for that Session. +3. WHEN a Session is destroyed because its last Client disconnects, THE collaboration_logger SHALL emit an INFO-level log entry containing the `session_id` and the reason `"session_closed"`. +4. THE collaboration_logger SHALL use the key-value format `key=value` consistently in all log messages to support log-aggregation queries. + +### Requirement 2: Structured Message-Event Logging + +**User Story:** As an operator, I want structured log entries for each collaboration message type handled, so that I can understand activity patterns and identify unusual message rates. + +#### Acceptance Criteria + +1. WHEN a `code_update` message is processed successfully, THE collaboration_logger SHALL emit a DEBUG-level log entry containing the `session_id`, `client_id`, the new `version`, and the code length in characters. +2. WHEN a `cursor_update` message is processed successfully, THE collaboration_logger SHALL emit a DEBUG-level log entry containing the `session_id`, `client_id`, and the cursor `line` and `column` values. +3. WHEN a `comment_added` message is processed successfully, THE collaboration_logger SHALL emit an INFO-level log entry containing the `session_id`, `client_id`, the `comment_id`, and the `line` number. +4. WHEN a message of an unsupported type is received, THE collaboration_logger SHALL emit a WARNING-level log entry containing the `session_id`, `client_id`, and the unrecognised `message_type`. + +### Requirement 3: Structured Error-Event Logging + +**User Story:** As an operator, I want structured log entries for validation failures and protocol errors within the presence sync flow, so that I can detect abuse, misconfigured clients, or bugs without relying solely on HTTP-level error signals. + +#### Acceptance Criteria + +1. WHEN a `code_update` message is rejected because the code field is not a string, THE collaboration_logger SHALL emit a WARNING-level log entry containing the `session_id`, `client_id`, and `reason="invalid_code_type"`. +2. WHEN a `code_update` message is rejected because the code length exceeds `MAX_CODE_CHARS`, THE collaboration_logger SHALL emit a WARNING-level log entry containing the `session_id`, `client_id`, `reason="code_too_long"`, and the received length. +3. WHEN a `comment_added` message is rejected because the comment text is empty, THE collaboration_logger SHALL emit a WARNING-level log entry containing the `session_id`, `client_id`, and `reason="empty_comment_text"`. +4. WHEN a `comment_added` message is rejected because the comment text exceeds `MAX_COMMENT_CHARS`, THE collaboration_logger SHALL emit a WARNING-level log entry containing the `session_id`, `client_id`, `reason="comment_too_long"`, and the received length. +5. WHEN a stale socket is detected during broadcast, THE collaboration_logger SHALL emit a WARNING-level log entry containing the `session_id`, `client_id`, and `reason="stale_socket"`. +6. IF a `sync_required` response is sent to a Client because its `incoming_version` is behind the room `version`, THEN THE collaboration_logger SHALL emit a DEBUG-level log entry containing the `session_id`, `client_id`, `incoming_version`, and the current room `version`. +7. IF a WebSocket message payload is not a JSON object, THEN THE collaboration_logger SHALL emit a WARNING-level log entry containing the `session_id`, `client_id`, and `reason="non_object_payload"`. + +### Requirement 4: Prometheus WebSocket Connection Metrics + +**User Story:** As an operator, I want Prometheus gauges and counters that track active WebSocket connections and total connection lifecycle events, so that I can alert on abnormal connection counts and visualise collaboration traffic. + +#### Acceptance Criteria + +1. THE Observability_Layer SHALL define a Prometheus Gauge named `qyverixai_collaboration_active_connections` with label `session_id` that reflects the number of Clients currently connected to each Session. +2. WHEN a Client connects to a Session, THE Observability_Layer SHALL increment the `qyverixai_collaboration_active_connections` gauge for that `session_id` by 1. +3. WHEN a Client disconnects from a Session, THE Observability_Layer SHALL decrement the `qyverixai_collaboration_active_connections` gauge for that `session_id` by 1. +4. THE Observability_Layer SHALL define a Prometheus Counter named `qyverixai_collaboration_connections_total` with label `session_id` that counts the total number of Client connections accepted since process start. +5. WHEN a Client connects to a Session, THE Observability_Layer SHALL increment the `qyverixai_collaboration_connections_total` counter for that `session_id` by 1. + +### Requirement 5: Prometheus Message-Event Metrics + +**User Story:** As an operator, I want Prometheus counters for each collaboration message type, so that I can build dashboards showing real-time edit activity, cursor movement frequency, and comment creation rates. + +#### Acceptance Criteria + +1. THE Observability_Layer SHALL define a Prometheus Counter named `qyverixai_collaboration_messages_total` with labels `session_id` and `message_type` that counts every successfully processed collaboration message. +2. WHEN a `code_update` message is processed successfully, THE Observability_Layer SHALL increment the `qyverixai_collaboration_messages_total` counter with `message_type="code_update"` for the corresponding `session_id`. +3. WHEN a `cursor_update` message is processed successfully, THE Observability_Layer SHALL increment the `qyverixai_collaboration_messages_total` counter with `message_type="cursor_update"` for the corresponding `session_id`. +4. WHEN a `comment_added` message is processed successfully, THE Observability_Layer SHALL increment the `qyverixai_collaboration_messages_total` counter with `message_type="comment_added"` for the corresponding `session_id`. + +### Requirement 6: Prometheus Error and Validation Metrics + +**User Story:** As an operator, I want a Prometheus counter for collaboration validation and protocol errors, so that I can alert on sustained error rates that may indicate a broken client or attempted abuse. + +#### Acceptance Criteria + +1. THE Observability_Layer SHALL define a Prometheus Counter named `qyverixai_collaboration_errors_total` with labels `session_id` and `error_reason` that counts every validation rejection or protocol error in the presence sync flow. +2. WHEN a `code_update` message is rejected for any validation reason, THE Observability_Layer SHALL increment the `qyverixai_collaboration_errors_total` counter with the corresponding `error_reason` label. +3. WHEN a `comment_added` message is rejected for any validation reason, THE Observability_Layer SHALL increment the `qyverixai_collaboration_errors_total` counter with the corresponding `error_reason` label. +4. WHEN a stale socket is detected during broadcast, THE Observability_Layer SHALL increment the `qyverixai_collaboration_errors_total` counter with `error_reason="stale_socket"` for the corresponding `session_id`. +5. WHEN an unsupported message type is received, THE Observability_Layer SHALL increment the `qyverixai_collaboration_errors_total` counter with `error_reason="unsupported_message_type"` for the corresponding `session_id`. + +### Requirement 7: Metrics Registration in Existing Prometheus Registry + +**User Story:** As an operator, I want the collaboration metrics to appear on the existing `/metrics` scrape endpoint, so that I can use a single Prometheus target for the entire application. + +#### Acceptance Criteria + +1. THE Observability_Layer SHALL register all collaboration Prometheus metrics (`qyverixai_collaboration_active_connections`, `qyverixai_collaboration_connections_total`, `qyverixai_collaboration_messages_total`, `qyverixai_collaboration_errors_total`) in the same `prometheus_client` default registry used by the existing metrics in `observability.py`. +2. WHILE the `METRICS_ENABLED` environment variable is set to `false`, THE Observability_Layer SHALL skip all Prometheus counter and gauge updates for collaboration events. +3. THE Observability_Layer SHALL define collaboration metrics in `observability.py` alongside the existing HTTP metrics, following the established naming and label-cardinality conventions. + +### Requirement 8: No Regression in Collaboration Behaviour + +**User Story:** As a developer, I want the observability additions to be strictly additive, so that all existing collaboration features — code sync, cursor sharing, comments, and session management — continue to work without modification. + +#### Acceptance Criteria + +1. THE Collaboration_Manager SHALL preserve all existing session state transitions (connect, broadcast, disconnect, room cleanup) after the Observability_Layer is integrated. +2. WHEN observability instrumentation raises an unexpected exception, THE Collaboration_Manager SHALL log the exception at ERROR level and continue normal operation without propagating the error to the Client. +3. THE Collaboration_Router SHALL not alter the structure or content of any WebSocket message sent to Clients as a result of adding observability. + +### Requirement 9: Logger Registration + +**User Story:** As an operator, I want the collaboration logger to participate in the existing per-component log-level override system, so that I can control its verbosity via the `LOG_LEVEL_COLLABORATION` environment variable without changing code. + +#### Acceptance Criteria + +1. THE Observability_Layer SHALL add a `"collaboration"` entry to the `COMPONENT_LOGGER_MAP` in `logging_config.py` mapped to the logger name `"app.routers.collaboration"`. +2. WHEN the `LOG_LEVEL_COLLABORATION` environment variable is set, THE Observability_Layer SHALL apply the specified log level to the `app.routers.collaboration` logger at startup. From 572af304599b2e49212bcebadbfe9d1fcc8a0b6d Mon Sep 17 00:00:00 2001 From: karrisanthoshigayatri Date: Wed, 22 Jul 2026 20:04:06 +0530 Subject: [PATCH 05/10] Harden session lifecycle handling in presence sync #1579 --- backend/app/logging_config.py | 1 + backend/app/routers/collaboration.py | 199 ++++++++++- backend/tests/test_collaboration_lifecycle.py | 327 ++++++++++++++++++ 3 files changed, 513 insertions(+), 14 deletions(-) create mode 100644 backend/tests/test_collaboration_lifecycle.py diff --git a/backend/app/logging_config.py b/backend/app/logging_config.py index ad3df660..4a424a6d 100644 --- a/backend/app/logging_config.py +++ b/backend/app/logging_config.py @@ -47,6 +47,7 @@ "upload": "app.routers.upload_file", "file_validator": "app.utils.file_validator", "main": "app.main", + "collaboration": "app.routers.collaboration", } _VALID_LEVELS = {"DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"} diff --git a/backend/app/routers/collaboration.py b/backend/app/routers/collaboration.py index 04a70766..16bff52b 100644 --- a/backend/app/routers/collaboration.py +++ b/backend/app/routers/collaboration.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import logging import uuid from dataclasses import dataclass, field from datetime import datetime, timezone @@ -12,8 +13,11 @@ router = APIRouter() +logger = logging.getLogger(__name__) + MAX_CODE_CHARS = 50_000 MAX_COMMENT_CHARS = 1_000 +MAX_SESSION_ID_CHARS = 128 COLORS = [ "#5b9cf6", @@ -48,6 +52,10 @@ def _get_room(self, session_id: str) -> CollaborationRoom: self.rooms[session_id] = CollaborationRoom() return self.rooms[session_id] + def _get_room_if_exists(self, session_id: str) -> CollaborationRoom | None: + """Return the room only if it already exists, without creating a phantom room.""" + return self.rooms.get(session_id) + def _users_payload(self, room: CollaborationRoom) -> list[dict[str, Any]]: return list(room.users.values()) @@ -93,6 +101,13 @@ async def connect( state = self._state_payload(session_id, room, client_id) users = self._users_payload(room) + logger.info( + "client_connected session_id=%s client_id=%s active_users=%d", + session_id, + client_id, + len(users), + ) + await websocket.send_json(state) await self.broadcast( session_id, @@ -111,8 +126,16 @@ async def disconnect(self, session_id: str, client_id: str) -> None: users = self._users_payload(room) should_delete = not room.sockets + logger.info( + "client_disconnected session_id=%s client_id=%s remaining_users=%d", + session_id, + client_id, + len(users), + ) + if should_delete: self.rooms.pop(session_id, None) + logger.info("session_closed session_id=%s reason=last_client_left", session_id) return await self.broadcast( @@ -138,7 +161,15 @@ async def broadcast( try: await socket.send_json(message) - except RuntimeError: + except Exception: + # Any send failure (RuntimeError from a closed socket, + # WebSocketDisconnect, etc.) marks the client as stale so it + # is cleaned up without crashing the rest of the broadcast. + logger.warning( + "stale_socket_detected session_id=%s client_id=%s", + session_id, + client_id, + ) stale_clients.append(client_id) for client_id in stale_clients: @@ -150,7 +181,17 @@ async def handle_message( client_id: str, data: dict[str, Any], ) -> None: - room = self._get_room(session_id) + # Guard: if the room was already destroyed (e.g. last client left + # concurrently), do not silently recreate a phantom room. + room = self._get_room_if_exists(session_id) + if room is None: + logger.warning( + "message_for_missing_room session_id=%s client_id=%s", + session_id, + client_id, + ) + return + message_type = data.get("type") if message_type == "ping": @@ -171,6 +212,12 @@ async def handle_message( await self._handle_comment_added(session_id, client_id, data) return + logger.warning( + "unsupported_message_type session_id=%s client_id=%s message_type=%s", + session_id, + client_id, + message_type, + ) socket = room.sockets.get(client_id) if socket is not None: await socket.send_json( @@ -186,13 +233,36 @@ async def _handle_code_update( client_id: str, data: dict[str, Any], ) -> None: - room = self._get_room(session_id) + room = self._get_room_if_exists(session_id) + if room is None: + return + socket = room.sockets.get(client_id) code = data.get("code", "") language = data.get("language") - incoming_version = int(data.get("version", 0)) + + # Safely coerce version — reject non-numeric values rather than crash. + raw_version = data.get("version", 0) + try: + incoming_version = int(raw_version) + except (TypeError, ValueError): + logger.warning( + "invalid_version_field session_id=%s client_id=%s reason=non_integer_version", + session_id, + client_id, + ) + if socket is not None: + await socket.send_json( + {"type": "error", "detail": "version must be an integer"} + ) + return if not isinstance(code, str): + logger.warning( + "code_update_rejected session_id=%s client_id=%s reason=invalid_code_type", + session_id, + client_id, + ) if socket is not None: await socket.send_json( {"type": "error", "detail": "code must be a string"} @@ -200,6 +270,12 @@ async def _handle_code_update( return if len(code) > MAX_CODE_CHARS: + logger.warning( + "code_update_rejected session_id=%s client_id=%s reason=code_too_long length=%d", + session_id, + client_id, + len(code), + ) if socket is not None: await socket.send_json( { @@ -214,6 +290,13 @@ async def _handle_code_update( state = self._state_payload(session_id, room, client_id) state["type"] = "sync_required" latest_socket = room.sockets.get(client_id) + logger.debug( + "sync_required session_id=%s client_id=%s incoming_version=%d room_version=%d", + session_id, + client_id, + incoming_version, + room.version, + ) else: room.version += 1 room.code = code @@ -227,6 +310,13 @@ async def _handle_code_update( } latest_socket = None state = payload + logger.debug( + "code_update session_id=%s client_id=%s version=%d code_length=%d", + session_id, + client_id, + room.version, + len(code), + ) if latest_socket is not None: await latest_socket.send_json(state) @@ -240,18 +330,35 @@ async def _handle_cursor_update( client_id: str, data: dict[str, Any], ) -> None: - room = self._get_room(session_id) + room = self._get_room_if_exists(session_id) + if room is None: + return + raw_cursor = data.get("cursor") if not isinstance(raw_cursor, dict): return - cursor = { - "line": max(1, int(raw_cursor.get("line", 1))), - "column": max(1, int(raw_cursor.get("column", 1))), - "selectionStart": max(0, int(raw_cursor.get("selectionStart", 0))), - "selectionEnd": max(0, int(raw_cursor.get("selectionEnd", 0))), - } + # Safely coerce cursor fields — non-integer values must not crash. + try: + cursor = { + "line": max(1, int(raw_cursor.get("line", 1))), + "column": max(1, int(raw_cursor.get("column", 1))), + "selectionStart": max(0, int(raw_cursor.get("selectionStart", 0))), + "selectionEnd": max(0, int(raw_cursor.get("selectionEnd", 0))), + } + except (TypeError, ValueError): + logger.warning( + "cursor_update_rejected session_id=%s client_id=%s reason=non_integer_cursor_field", + session_id, + client_id, + ) + socket = room.sockets.get(client_id) + if socket is not None: + await socket.send_json( + {"type": "error", "detail": "cursor fields must be integers"} + ) + return async with room.lock: user = room.users.get(client_id) @@ -262,6 +369,13 @@ async def _handle_cursor_update( "type": "cursor_update", "user": user, } + logger.debug( + "cursor_update session_id=%s client_id=%s line=%d column=%d", + session_id, + client_id, + cursor["line"], + cursor["column"], + ) await self.broadcast(session_id, payload, exclude=client_id) @@ -271,13 +385,27 @@ async def _handle_comment_added( client_id: str, data: dict[str, Any], ) -> None: - room = self._get_room(session_id) + room = self._get_room_if_exists(session_id) + if room is None: + return + text = str(data.get("text", "")).strip() - line = max(1, int(data.get("line", 1))) + + # Safely coerce line number. + raw_line = data.get("line", 1) + try: + line = max(1, int(raw_line)) + except (TypeError, ValueError): + line = 1 socket = room.sockets.get(client_id) if not text: + logger.warning( + "comment_rejected session_id=%s client_id=%s reason=empty_comment_text", + session_id, + client_id, + ) if socket is not None: await socket.send_json( {"type": "error", "detail": "comment text is required"} @@ -285,6 +413,12 @@ async def _handle_comment_added( return if len(text) > MAX_COMMENT_CHARS: + logger.warning( + "comment_rejected session_id=%s client_id=%s reason=comment_too_long length=%d", + session_id, + client_id, + len(text), + ) if socket is not None: await socket.send_json( { @@ -311,6 +445,13 @@ async def _handle_comment_added( "comment": comment, "comments": room.comments, } + logger.info( + "comment_added session_id=%s client_id=%s comment_id=%s line=%d", + session_id, + client_id, + comment["id"], + line, + ) await self.broadcast(session_id, payload) @@ -324,14 +465,44 @@ async def collaboration_websocket( session_id: str, name: str = Query(default="Anonymous", max_length=40), ) -> None: + # Validate session_id before accepting the connection. + if not session_id or not session_id.strip() or len(session_id) > MAX_SESSION_ID_CHARS: + await websocket.close(code=1008, reason="invalid session_id") + return + client_id = await manager.connect(session_id, websocket, name) try: while True: - data = await websocket.receive_json() + try: + data = await websocket.receive_json() + except ValueError: + # Malformed JSON from the client — send an error and keep the + # connection alive rather than crashing the handler loop. + logger.warning( + "non_json_message session_id=%s client_id=%s reason=malformed_json", + session_id, + client_id, + ) + try: + await websocket.send_json( + { + "type": "error", + "detail": "message must be valid JSON", + } + ) + except Exception: + pass + continue + if isinstance(data, dict): await manager.handle_message(session_id, client_id, data) else: + logger.warning( + "non_object_payload session_id=%s client_id=%s reason=non_object_payload", + session_id, + client_id, + ) await websocket.send_json( {"type": "error", "detail": "message payload must be a JSON object"} ) diff --git a/backend/tests/test_collaboration_lifecycle.py b/backend/tests/test_collaboration_lifecycle.py new file mode 100644 index 00000000..569c2ccf --- /dev/null +++ b/backend/tests/test_collaboration_lifecycle.py @@ -0,0 +1,327 @@ +""" +Tests for hardened session lifecycle handling in the presence sync router. + +Covers edge cases that were not tested before: +- session_id validation (empty, too long) +- phantom room prevention (message after room teardown) +- malformed version field in code_update +- non-integer cursor fields in cursor_update +- non-integer line field in comment_added +- empty comment text rejection +- over-length comment rejection +- unsupported message type error response +- non-object JSON payload error response +- stale-socket broadcast resilience +- session cleanup when last client disconnects +- non-dict cursor value ignored gracefully +""" + +from app import main as app_main +from app.routers.collaboration import MAX_CODE_CHARS, MAX_COMMENT_CHARS, MAX_SESSION_ID_CHARS, manager +from fastapi.testclient import TestClient + +client = TestClient(app_main.app) + + +def setup_function(): + manager.reset() + + +# ── session_id validation ───────────────────────────────────────────────────── + +def test_empty_session_id_is_rejected(): + """The router must close the WebSocket for an empty session_id.""" + # FastAPI routes /ws/{session_id} — an empty string would not match the + # path parameter at all (returns 403/404), so we verify no room is created. + assert "" not in manager.rooms + + +def test_oversized_session_id_is_rejected(): + """session_id longer than MAX_SESSION_ID_CHARS must be closed on connect.""" + long_id = "x" * (MAX_SESSION_ID_CHARS + 1) + with client.websocket_connect(f"/collaboration/ws/{long_id}?name=Alice") as ws: + # The server should close the connection; if any message arrives it + # must be a close frame, not a session_state. + try: + msg = ws.receive_json() + # If we somehow receive a message it must NOT be a valid session. + assert msg.get("type") != "session_state", ( + "Server must not accept oversized session_id" + ) + except Exception: + # Connection closed — expected behaviour. + pass + + assert long_id not in manager.rooms + + +# ── phantom room prevention ─────────────────────────────────────────────────── + +def test_handle_message_does_not_create_phantom_room(): + """handle_message on a non-existent session must not create a room.""" + import asyncio + + async def _run(): + await manager.handle_message("ghost-session", "fake-client", {"type": "ping"}) + + asyncio.get_event_loop().run_until_complete(_run()) + assert "ghost-session" not in manager.rooms + + +# ── code_update hardening ───────────────────────────────────────────────────── + +def test_code_update_with_non_integer_version_returns_error(): + """version field that cannot be cast to int must return an error, not crash.""" + with client.websocket_connect( + "/collaboration/ws/sess-ver?name=Alice" + ) as ws: + ws.receive_json() # session_state + ws.receive_json() # presence_update + + ws.send_json( + { + "type": "code_update", + "code": "x = 1", + "language": "python", + "version": "not-an-int", + } + ) + + response = ws.receive_json() + assert response["type"] == "error" + assert "version" in response["detail"].lower() + + +def test_code_update_with_non_string_code_returns_error(): + """code field that is not a string must return an error message.""" + with client.websocket_connect( + "/collaboration/ws/sess-code-type?name=Alice" + ) as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json( + { + "type": "code_update", + "code": 12345, + "language": "python", + "version": 0, + } + ) + + response = ws.receive_json() + assert response["type"] == "error" + assert "string" in response["detail"].lower() + + +def test_code_update_exceeding_max_length_returns_error(): + """code longer than MAX_CODE_CHARS must return an error.""" + with client.websocket_connect( + "/collaboration/ws/sess-code-len?name=Alice" + ) as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json( + { + "type": "code_update", + "code": "x" * (MAX_CODE_CHARS + 1), + "language": "python", + "version": 0, + } + ) + + response = ws.receive_json() + assert response["type"] == "error" + assert str(MAX_CODE_CHARS) in response["detail"] + + +# ── cursor_update hardening ─────────────────────────────────────────────────── + +def test_cursor_update_with_non_integer_fields_returns_error(): + """Cursor fields that cannot be cast to int must return an error, not crash.""" + with client.websocket_connect( + "/collaboration/ws/sess-cursor?name=Alice" + ) as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json( + { + "type": "cursor_update", + "cursor": { + "line": "bad", + "column": 1, + "selectionStart": 0, + "selectionEnd": 0, + }, + } + ) + + response = ws.receive_json() + assert response["type"] == "error" + assert "integer" in response["detail"].lower() + + +def test_cursor_update_with_non_dict_cursor_is_ignored(): + """A cursor_update where cursor is not a dict must be silently ignored.""" + with client.websocket_connect( + "/collaboration/ws/sess-cursor-null?name=Alice" + ) as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "cursor_update", "cursor": "invalid"}) + # No error frame is expected — the message is silently dropped. + # Send a ping to confirm the connection is still alive. + ws.send_json({"type": "ping"}) + pong = ws.receive_json() + assert pong["type"] == "pong" + + +# ── comment_added hardening ─────────────────────────────────────────────────── + +def test_comment_with_empty_text_returns_error(): + """An empty comment text must return an error.""" + with client.websocket_connect( + "/collaboration/ws/sess-cmt-empty?name=Alice" + ) as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": " "}) + + response = ws.receive_json() + assert response["type"] == "error" + assert "required" in response["detail"].lower() + + +def test_comment_exceeding_max_length_returns_error(): + """A comment longer than MAX_COMMENT_CHARS must return an error.""" + with client.websocket_connect( + "/collaboration/ws/sess-cmt-len?name=Alice" + ) as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json( + { + "type": "comment_added", + "line": 1, + "text": "y" * (MAX_COMMENT_CHARS + 1), + } + ) + + response = ws.receive_json() + assert response["type"] == "error" + assert str(MAX_COMMENT_CHARS) in response["detail"] + + +def test_comment_with_non_integer_line_uses_default(): + """A non-integer line field must not crash — it defaults to 1.""" + with client.websocket_connect( + "/collaboration/ws/sess-cmt-line?name=Alice" + ) as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json( + { + "type": "comment_added", + "line": "not-a-number", + "text": "Valid comment text.", + } + ) + + response = ws.receive_json() + assert response["type"] == "comment_added" + assert response["comment"]["line"] == 1 + + +# ── unsupported message type ────────────────────────────────────────────────── + +def test_unsupported_message_type_returns_error(): + """An unrecognised message type must return an error frame.""" + with client.websocket_connect( + "/collaboration/ws/sess-unknown?name=Alice" + ) as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "do_something_unknown"}) + + response = ws.receive_json() + assert response["type"] == "error" + assert "unsupported" in response["detail"].lower() + + +# ── non-object payload ──────────────────────────────────────────────────────── + +def test_non_object_json_payload_returns_error(): + """A JSON array (non-object) must return an error, not crash.""" + with client.websocket_connect( + "/collaboration/ws/sess-payload?name=Alice" + ) as ws: + ws.receive_json() + ws.receive_json() + + # TestClient encodes this as a JSON array at the wire level. + ws.send_text("[1, 2, 3]") + + response = ws.receive_json() + assert response["type"] == "error" + assert "object" in response["detail"].lower() + + +# ── session cleanup ─────────────────────────────────────────────────────────── + +def test_room_is_removed_when_last_client_disconnects(): + """After the last client leaves, the room must be removed from manager.rooms.""" + session = "sess-cleanup" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice"): + assert session in manager.rooms + + # Context manager exit triggers disconnect. + assert session not in manager.rooms + + +def test_room_persists_while_at_least_one_client_is_connected(): + """Room must not be removed while a second client is still connected.""" + session = "sess-persist" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as alice: + alice.receive_json() + alice.receive_json() + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Bob") as bob: + bob.receive_json() + alice.receive_json() + bob.receive_json() + + # Alice disconnects; Bob is still connected. + + # After Alice's context exits, room must still exist for Bob. + assert session in manager.rooms + + # After Bob's context exits too, room must be cleaned up. + assert session not in manager.rooms + + +# ── presence update after disconnect ───────────────────────────────────────── + +def test_presence_update_broadcast_on_disconnect(): + """Remaining clients must receive a presence_update when another client leaves.""" + session = "sess-presence-disc" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as alice: + alice.receive_json() # session_state + alice.receive_json() # own presence_update + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Bob"): + alice.receive_json() # presence_update for Bob joining + # Bob's context exits here — triggers disconnect. + + # Alice must receive a presence_update showing Bob has left. + update = alice.receive_json() + assert update["type"] == "presence_update" + names = [u["name"] for u in update["users"]] + assert "Bob" not in names + assert "Alice" in names From aaf072d750f251e523315985971929292127b0c7 Mon Sep 17 00:00:00 2001 From: karrisanthoshigayatri Date: Wed, 22 Jul 2026 20:12:57 +0530 Subject: [PATCH 06/10] Improve mobile layout in frontend script #1590 --- frontend/index.html | 83 ++++++++++++++++--- frontend/package.json | 2 +- frontend/tests/mobile-layout.test.js | 118 +++++++++++++++++++++++++++ 3 files changed, 191 insertions(+), 12 deletions(-) create mode 100644 frontend/tests/mobile-layout.test.js diff --git a/frontend/index.html b/frontend/index.html index c907d563..615116af 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -2290,44 +2290,105 @@ .stat-chip.info-chip .stat-chip-num{color:var(--accent)} /* responsive */ + +/* ── 600 px — tablet / large phone ─────────────────────────── */ @media(max-width:600px){ - .hero h1{font-size:2.2rem} - nav{flex-wrap:wrap;gap:8px} + /* Nav */ + nav{flex-wrap:wrap;gap:8px;padding-left:16px;padding-right:16px} .nav-links .nav-link span{display:none} + /* Hide language selector in nav on small screens — inline select is cramped */ + .nav-links .lang-select{display:none} + + /* Hero */ .hero{padding:50px 0 40px} + .hero h1{font-size:2.2rem} .hero-sub{font-size:0.98rem} + .hero-cta{gap:8px} + .btn{padding:11px 18px;font-size:0.85rem} + + /* Feature pills */ .features-row{display:flex;flex-direction:column;gap:8px;max-width:340px;margin-inline:auto;padding:16px 0 40px} .feat-pill{gap:10px;padding:10px 14px;min-width:0} .feat-pill-icon{width:32px;height:32px;flex-shrink:0} .feat-pill-info{min-width:0;overflow-wrap:break-word} .feat-pill-info .feat-pill-title{font-size:0.82rem;line-height:1.25} .feat-pill-info p{font-size:0.74rem;line-height:1.25} + + /* Workspace & panels */ .workspace{gap:14px} #workspace > .panel{grid-column:1 !important;min-width:0} - .panel-header{padding:12px 14px} + /* Panel headers that hold multiple action buttons wrap gracefully */ + .panel-header{padding:12px 14px;flex-wrap:wrap;gap:6px} + .panel-title{flex:1 1 100%} /* title takes full row so buttons go below */ + + /* Code editor: smaller fixed heights free vertical space on phones */ + #codeEditor{min-height:200px;max-height:340px} + .line-numbers{min-height:200px;max-height:340px} + .editor-footer{font-size:0.7rem;padding:8px 12px} + + /* Mode buttons */ .mode-row{gap:6px;padding:10px 14px} - .mode-btn{padding:8px 4px;font-size:0.74rem} + .mode-btn{padding:10px 6px;font-size:0.74rem;min-height:44px} /* ≥44px touch target */ + + /* Analyze row */ .analyze-row{padding:10px 14px} - .btn-analyze{padding:14px} - .result-content{min-height:280px;max-height:420px;padding:14px} + .btn-analyze{padding:14px;min-height:48px} /* comfortable touch target */ + /* Collapse shortcut hints on narrow screens — keyboard shortcuts are not + relevant on touch-only devices; hints are still present in the DOM for + screen readers via aria-keyshortcuts on the buttons themselves. */ + .shortcut-hints{display:none} + + /* Result panel */ + .result-content{min-height:240px;max-height:420px;padding:14px} + /* Result action buttons wrap instead of overflowing */ + .result-actions{flex-wrap:wrap;gap:6px;padding:8px 14px} + .result-actions .icon-btn{min-height:44px;min-width:44px} + + /* Icon buttons: ensure 44×44px touch target everywhere */ + .icon-btn{min-height:44px;min-width:44px;width:auto;padding:0 10px} + + /* History / favorites */ .bottom-grid{gap:14px} .history-list,.fav-list{max-height:200px} + /* History panel header buttons wrap on narrow screens */ + #historyList ~ * .btn-clear, + .history-controls-row{flex-wrap:wrap} + + /* API config input row wraps cleanly */ + .api-config-inner{flex-wrap:wrap} + #apiUrl{min-width:0;width:100%} + + /* Analytics */ .score-ring-wrap{flex-direction:column;text-align:center;gap:12px} .stat-bar{gap:6px} .stat-chip{padding:8px 10px} .stat-chip-num{font-size:1.2rem} + + /* Toasts */ #toastContainer{bottom:12px;right:12px;left:12px} .toast{min-width:unset;width:100%} - .hero-cta{gap:8px} - .btn{padding:11px 18px;font-size:0.85rem} } + +/* ── 400 px — small phone ───────────────────────────────────── */ @media(max-width:400px){ - .app{padding:0 14px 60px} + .app{padding:0 12px 60px} .hero h1{font-size:1.9rem} - .lang-tab{padding:7px 10px;font-size:0.72rem} - .result-tab{padding:9px 12px;font-size:0.75rem} + /* Navigation: stack logo and links vertically with full-width logo row */ + nav{padding-left:12px;padding-right:12px} + /* Tabs scroll horizontally rather than squeezing */ + .lang-tabs{overflow-x:auto;-webkit-overflow-scrolling:touch;scrollbar-width:none} + .lang-tab{padding:7px 10px;font-size:0.72rem;flex-shrink:0} + .result-tabs{overflow-x:auto;-webkit-overflow-scrolling:touch;scrollbar-width:none} + .result-tab{padding:9px 12px;font-size:0.75rem;flex-shrink:0} + /* Mode buttons: allow horizontal scroll instead of squeezing below 44px */ + .mode-row{overflow-x:auto;-webkit-overflow-scrolling:touch;flex-wrap:nowrap;scrollbar-width:none} + .mode-btn{flex:0 0 auto;min-width:68px} + /* Pagination row: ensure buttons stay tappable */ + .history-pagination-row{gap:8px} + #btnPrevHistory,#btnNextHistory{min-height:44px;min-width:44px;padding:8px 12px} } + /* ── Live Collaboration ── */ .collab-toolbar-btn { display: inline-flex; diff --git a/frontend/package.json b/frontend/package.json index 241ca978..b0b2c992 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -4,7 +4,7 @@ "version": "0.0.0", "description": "Playwright end-to-end tests for the QyverixAI frontend.", "scripts": { - "test:static": "node tests/sample-comments.test.js && node tests/shortcut-ui.test.js && node tests/empty-states.test.js", + "test:static": "node tests/sample-comments.test.js && node tests/shortcut-ui.test.js && node tests/empty-states.test.js && node tests/mobile-layout.test.js", "test:e2e": "playwright test", "test:e2e:ci": "playwright test --reporter=list", "test:e2e:headed": "playwright test --headed" diff --git a/frontend/tests/mobile-layout.test.js b/frontend/tests/mobile-layout.test.js new file mode 100644 index 00000000..ec5ac135 --- /dev/null +++ b/frontend/tests/mobile-layout.test.js @@ -0,0 +1,118 @@ +/** + * Static assertions that the mobile layout CSS improvements are present + * in index.html. + * + * Run with: node tests/mobile-layout.test.js + */ +import assert from 'node:assert'; +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const html = fs.readFileSync(path.resolve(__dirname, '..', 'index.html'), 'utf8'); + +// ── Breakpoint existence ───────────────────────────────────────────────────── + +assert.match( + html, + /@media\(max-width:600px\)/, + '600px breakpoint should be present', +); +assert.match( + html, + /@media\(max-width:400px\)/, + '400px breakpoint should be present', +); +assert.match( + html, + /@media\(max-width:900px\)/, + '900px (tablet) breakpoint for workspace grid should be present', +); + +// ── 44px touch target enforcement ──────────────────────────────────────────── + +assert.match( + html, + /min-height:44px/, + 'At least one element should enforce a 44px minimum touch target height', +); + +// ── Mode buttons: min-height touch target ──────────────────────────────────── + +assert.match( + html, + /\.mode-btn\{[^}]*min-height:44px|mode-btn[^{]*\{[^}]*min-height:44px/, + 'mode-btn should have min-height:44px in mobile breakpoint', +); + +// ── Shortcut hints hidden on small screens ─────────────────────────────────── + +assert.match( + html, + /\.shortcut-hints\{display:none\}|\.shortcut-hints\s*\{\s*display\s*:\s*none/, + 'shortcut-hints should be hidden (display:none) inside a mobile breakpoint', +); + +// ── Panel header flex-wrap ──────────────────────────────────────────────────── + +assert.match( + html, + /\.panel-header\{[^}]*flex-wrap:wrap|panel-header[^{]*\{[^}]*flex-wrap\s*:\s*wrap/, + 'panel-header should use flex-wrap:wrap in mobile breakpoint', +); + +// ── Horizontal scroll for lang-tabs and result-tabs on 400px ───────────────── + +assert.match( + html, + /\.lang-tabs\{overflow-x:auto|lang-tabs[^{]*\{[^}]*overflow-x\s*:\s*auto/, + 'lang-tabs should allow horizontal scroll on 400px breakpoint', +); +assert.match( + html, + /\.result-tabs\{overflow-x:auto|result-tabs[^{]*\{[^}]*overflow-x\s*:\s*auto/, + 'result-tabs should allow horizontal scroll on 400px breakpoint', +); + +// ── Editor height reduction on mobile ──────────────────────────────────────── + +assert.match( + html, + /#codeEditor\{[^}]*min-height:200px|#codeEditor[^{]*\{[^}]*min-height\s*:\s*200px/, + 'codeEditor should have reduced min-height on mobile', +); + +// ── API URL input full-width on mobile ──────────────────────────────────────── + +assert.match( + html, + /#apiUrl\{[^}]*width:100%|#apiUrl[^{]*\{[^}]*width\s*:\s*100%/, + 'apiUrl input should be full-width on mobile', +); + +// ── Result actions wrapping ─────────────────────────────────────────────────── + +assert.match( + html, + /\.result-actions\{[^}]*flex-wrap:wrap|result-actions[^{]*\{[^}]*flex-wrap\s*:\s*wrap/, + 'result-actions should wrap on mobile', +); + +// ── Pagination touch targets ────────────────────────────────────────────────── + +assert.match( + html, + /#btnPrevHistory,#btnNextHistory\{[^}]*min-height:44px|btnPrevHistory[^{]*\{[^}]*min-height\s*:\s*44px/, + 'history pagination buttons should have 44px touch target', +); + +// ── Viewport meta tag ───────────────────────────────────────────────────────── + +assert.match( + html, + /]+name="viewport"[^>]+content="[^"]*width=device-width/, + 'viewport meta tag with width=device-width should be present for proper mobile scaling', +); + +console.log('All mobile layout assertions passed.'); From 57b080ea1aa7c34ae3d1d230ebc031484423004f Mon Sep 17 00:00:00 2001 From: karrisanthoshigayatri Date: Wed, 22 Jul 2026 20:28:17 +0530 Subject: [PATCH 07/10] Expose observability in collaboration websocket #1571 --- backend/app/observability.py | 76 +++++ backend/app/routers/collaboration.py | 22 ++ .../tests/test_collaboration_observability.py | 309 ++++++++++++++++++ 3 files changed, 407 insertions(+) create mode 100644 backend/tests/test_collaboration_observability.py diff --git a/backend/app/observability.py b/backend/app/observability.py index 5ce43919..43f926b0 100644 --- a/backend/app/observability.py +++ b/backend/app/observability.py @@ -112,6 +112,82 @@ def initialise_app_info(version: str, ai_provider: str) -> None: APP_INFO.labels(version=version, ai_provider=ai_provider).set(1) +# ── Collaboration WebSocket metrics ────────────────────────────────────────── +# These metrics surface the presence-sync / collaboration WebSocket lifecycle +# on the same /metrics scrape endpoint as the HTTP metrics above. +# +# Label cardinality note: ``session_id`` is used as a label. In typical +# deployments the number of distinct session IDs is bounded and short-lived +# (rooms are torn down when the last client leaves), so cardinality remains +# acceptable. If deployments see unbounded session growth, operators should +# relabel or aggregate in their Prometheus recording rules. + +COLLAB_ACTIVE_CONNECTIONS = Gauge( + "qyverixai_collaboration_active_connections", + "Number of WebSocket clients currently connected to each collaboration session.", + labelnames=("session_id",), +) + +COLLAB_CONNECTIONS_TOTAL = Counter( + "qyverixai_collaboration_connections_total", + "Total number of WebSocket clients that have connected to collaboration sessions since process start.", + labelnames=("session_id",), +) + +COLLAB_MESSAGES_TOTAL = Counter( + "qyverixai_collaboration_messages_total", + "Total number of collaboration messages successfully processed, by session and message type.", + labelnames=("session_id", "message_type"), +) + +COLLAB_ERRORS_TOTAL = Counter( + "qyverixai_collaboration_errors_total", + "Total number of validation rejections and protocol errors in the collaboration WebSocket flow.", + labelnames=("session_id", "error_reason"), +) + + +def record_collab_connect(session_id: str) -> None: + """Increment connection metrics when a client joins a session.""" + if not metrics_enabled(): + return + try: + COLLAB_ACTIVE_CONNECTIONS.labels(session_id=session_id).inc() + COLLAB_CONNECTIONS_TOTAL.labels(session_id=session_id).inc() + except Exception: # pragma: no cover — metric instrumentation must not crash handlers + pass + + +def record_collab_disconnect(session_id: str) -> None: + """Decrement the active-connections gauge when a client leaves a session.""" + if not metrics_enabled(): + return + try: + COLLAB_ACTIVE_CONNECTIONS.labels(session_id=session_id).dec() + except Exception: # pragma: no cover + pass + + +def record_collab_message(session_id: str, message_type: str) -> None: + """Increment the processed-messages counter for a given message type.""" + if not metrics_enabled(): + return + try: + COLLAB_MESSAGES_TOTAL.labels(session_id=session_id, message_type=message_type).inc() + except Exception: # pragma: no cover + pass + + +def record_collab_error(session_id: str, error_reason: str) -> None: + """Increment the errors counter for a given error reason.""" + if not metrics_enabled(): + return + try: + COLLAB_ERRORS_TOTAL.labels(session_id=session_id, error_reason=error_reason).inc() + except Exception: # pragma: no cover + pass + + # ── Endpoint label resolution ──────────────────────────────────────────────── def _endpoint_label(request: Request) -> str: """Return the route template (low cardinality) rather than the raw path. diff --git a/backend/app/routers/collaboration.py b/backend/app/routers/collaboration.py index 16bff52b..df2fb7a7 100644 --- a/backend/app/routers/collaboration.py +++ b/backend/app/routers/collaboration.py @@ -11,6 +11,13 @@ from fastapi import APIRouter, Query, WebSocket, WebSocketDisconnect +from ..observability import ( + record_collab_connect, + record_collab_disconnect, + record_collab_error, + record_collab_message, +) + router = APIRouter() logger = logging.getLogger(__name__) @@ -107,6 +114,7 @@ async def connect( client_id, len(users), ) + record_collab_connect(session_id) await websocket.send_json(state) await self.broadcast( @@ -132,6 +140,7 @@ async def disconnect(self, session_id: str, client_id: str) -> None: client_id, len(users), ) + record_collab_disconnect(session_id) if should_delete: self.rooms.pop(session_id, None) @@ -170,6 +179,7 @@ async def broadcast( session_id, client_id, ) + record_collab_error(session_id, "stale_socket") stale_clients.append(client_id) for client_id in stale_clients: @@ -218,6 +228,7 @@ async def handle_message( client_id, message_type, ) + record_collab_error(session_id, "unsupported_message_type") socket = room.sockets.get(client_id) if socket is not None: await socket.send_json( @@ -251,6 +262,7 @@ async def _handle_code_update( session_id, client_id, ) + record_collab_error(session_id, "non_integer_version") if socket is not None: await socket.send_json( {"type": "error", "detail": "version must be an integer"} @@ -263,6 +275,7 @@ async def _handle_code_update( session_id, client_id, ) + record_collab_error(session_id, "invalid_code_type") if socket is not None: await socket.send_json( {"type": "error", "detail": "code must be a string"} @@ -276,6 +289,7 @@ async def _handle_code_update( client_id, len(code), ) + record_collab_error(session_id, "code_too_long") if socket is not None: await socket.send_json( { @@ -317,6 +331,7 @@ async def _handle_code_update( room.version, len(code), ) + record_collab_message(session_id, "code_update") if latest_socket is not None: await latest_socket.send_json(state) @@ -353,6 +368,7 @@ async def _handle_cursor_update( session_id, client_id, ) + record_collab_error(session_id, "non_integer_cursor_field") socket = room.sockets.get(client_id) if socket is not None: await socket.send_json( @@ -376,6 +392,7 @@ async def _handle_cursor_update( cursor["line"], cursor["column"], ) + record_collab_message(session_id, "cursor_update") await self.broadcast(session_id, payload, exclude=client_id) @@ -406,6 +423,7 @@ async def _handle_comment_added( session_id, client_id, ) + record_collab_error(session_id, "empty_comment_text") if socket is not None: await socket.send_json( {"type": "error", "detail": "comment text is required"} @@ -419,6 +437,7 @@ async def _handle_comment_added( client_id, len(text), ) + record_collab_error(session_id, "comment_too_long") if socket is not None: await socket.send_json( { @@ -452,6 +471,7 @@ async def _handle_comment_added( comment["id"], line, ) + record_collab_message(session_id, "comment_added") await self.broadcast(session_id, payload) @@ -484,6 +504,7 @@ async def collaboration_websocket( session_id, client_id, ) + record_collab_error(session_id, "malformed_json") try: await websocket.send_json( { @@ -503,6 +524,7 @@ async def collaboration_websocket( session_id, client_id, ) + record_collab_error(session_id, "non_object_payload") await websocket.send_json( {"type": "error", "detail": "message payload must be a JSON object"} ) diff --git a/backend/tests/test_collaboration_observability.py b/backend/tests/test_collaboration_observability.py new file mode 100644 index 00000000..4f80a174 --- /dev/null +++ b/backend/tests/test_collaboration_observability.py @@ -0,0 +1,309 @@ +""" +Tests for Prometheus observability metrics exposed by the collaboration +WebSocket router. + +Strategy +-------- +The prometheus_client Counter and Gauge objects are module-level singletons, +so we cannot re-create them between test runs. Instead each test uses a +unique ``session_id`` (prefixed ``obs-``) so that label combinations are +fresh and we can assert the *absolute* sample value equals the expected count +for that run. + +``METRICS_ENABLED`` is not overridden here — the default is True, so all +metrics should be collected normally. +""" + +import os + +import pytest + +from app import main as app_main +from app.observability import ( + COLLAB_ACTIVE_CONNECTIONS, + COLLAB_CONNECTIONS_TOTAL, + COLLAB_ERRORS_TOTAL, + COLLAB_MESSAGES_TOTAL, +) +from app.routers.collaboration import MAX_CODE_CHARS, MAX_COMMENT_CHARS, manager +from fastapi.testclient import TestClient + +client = TestClient(app_main.app) + + +def setup_function(): + manager.reset() + + +# ── helpers ─────────────────────────────────────────────────────────────────── + +def _active(session_id: str) -> float: + """Return the current value of the active-connections gauge for a session.""" + try: + return COLLAB_ACTIVE_CONNECTIONS.labels(session_id=session_id)._value.get() + except KeyError: + return 0.0 + + +def _total_connections(session_id: str) -> float: + """Return the cumulative connections counter for a session.""" + try: + return COLLAB_CONNECTIONS_TOTAL.labels(session_id=session_id)._value.get() + except KeyError: + return 0.0 + + +def _messages(session_id: str, message_type: str) -> float: + """Return the cumulative messages counter for a session + message_type.""" + try: + return COLLAB_MESSAGES_TOTAL.labels( + session_id=session_id, message_type=message_type + )._value.get() + except KeyError: + return 0.0 + + +def _errors(session_id: str, error_reason: str) -> float: + """Return the cumulative errors counter for a session + error_reason.""" + try: + return COLLAB_ERRORS_TOTAL.labels( + session_id=session_id, error_reason=error_reason + )._value.get() + except KeyError: + return 0.0 + + +# ── connection lifecycle metrics ────────────────────────────────────────────── + +def test_connect_increments_active_and_total_counters(): + session = "obs-connect-1" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() # session_state + ws.receive_json() # presence_update + + assert _active(session) == 1.0 + assert _total_connections(session) == 1.0 + + # After disconnect the active gauge must return to 0. + assert _active(session) == 0.0 + # Total connections is a monotonic counter — must stay at 1. + assert _total_connections(session) == 1.0 + + +def test_two_clients_increment_active_gauge_independently(): + session = "obs-connect-2" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as alice: + alice.receive_json() + alice.receive_json() + + assert _active(session) == 1.0 + assert _total_connections(session) == 1.0 + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Bob") as bob: + bob.receive_json() + alice.receive_json() + bob.receive_json() + + assert _active(session) == 2.0 + assert _total_connections(session) == 2.0 + + # Bob disconnected — gauge decrements. + assert _active(session) == 1.0 + + # Alice disconnected — gauge reaches 0. + assert _active(session) == 0.0 + assert _total_connections(session) == 2.0 + + +# ── message metrics ─────────────────────────────────────────────────────────── + +def test_code_update_increments_messages_counter(): + session = "obs-code-msg" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + state = ws.receive_json() + ws.receive_json() + + ws.send_json({ + "type": "code_update", + "code": "x = 1", + "language": "python", + "version": state["version"], + }) + ws.receive_json() # broadcast back + + assert _messages(session, "code_update") == 1.0 + + +def test_cursor_update_increments_messages_counter(): + session = "obs-cursor-msg" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as alice: + alice.receive_json() + alice.receive_json() + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Bob") as bob: + bob.receive_json() + alice.receive_json() + bob.receive_json() + + bob.send_json({ + "type": "cursor_update", + "cursor": {"line": 2, "column": 5, "selectionStart": 0, "selectionEnd": 0}, + }) + alice.receive_json() # cursor broadcast to Alice + + assert _messages(session, "cursor_update") == 1.0 + + +def test_comment_added_increments_messages_counter(): + session = "obs-comment-msg" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": "Looks good."}) + ws.receive_json() # comment broadcast + + assert _messages(session, "comment_added") == 1.0 + + +# ── error metrics ───────────────────────────────────────────────────────────── + +def test_unsupported_message_type_increments_error_counter(): + session = "obs-err-unknown" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "unknown_action"}) + ws.receive_json() # error frame + + assert _errors(session, "unsupported_message_type") == 1.0 + + +def test_invalid_code_type_increments_error_counter(): + session = "obs-err-codetype" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "code_update", "code": 999, "language": "python", "version": 0}) + ws.receive_json() # error frame + + assert _errors(session, "invalid_code_type") == 1.0 + + +def test_code_too_long_increments_error_counter(): + session = "obs-err-codelen" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({ + "type": "code_update", + "code": "x" * (MAX_CODE_CHARS + 1), + "language": "python", + "version": 0, + }) + ws.receive_json() # error frame + + assert _errors(session, "code_too_long") == 1.0 + + +def test_non_integer_version_increments_error_counter(): + session = "obs-err-version" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "code_update", "code": "x=1", "language": "python", "version": "bad"}) + ws.receive_json() # error frame + + assert _errors(session, "non_integer_version") == 1.0 + + +def test_empty_comment_increments_error_counter(): + session = "obs-err-emptycmt" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": " "}) + ws.receive_json() # error frame + + assert _errors(session, "empty_comment_text") == 1.0 + + +def test_comment_too_long_increments_error_counter(): + session = "obs-err-cmtlen" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({ + "type": "comment_added", + "line": 1, + "text": "y" * (MAX_COMMENT_CHARS + 1), + }) + ws.receive_json() # error frame + + assert _errors(session, "comment_too_long") == 1.0 + + +def test_non_object_payload_increments_error_counter(): + session = "obs-err-payload" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_text("[1, 2, 3]") + ws.receive_json() # error frame + + assert _errors(session, "non_object_payload") == 1.0 + + +# ── metrics_enabled=false guard ─────────────────────────────────────────────── + +def test_metrics_disabled_skips_all_counters(monkeypatch): + """When METRICS_ENABLED=false no counter/gauge updates should be made.""" + monkeypatch.setenv("METRICS_ENABLED", "false") + + session = "obs-disabled" + before_total = _total_connections(session) + before_active = _active(session) + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({ + "type": "code_update", + "code": "print('hi')", + "language": "python", + "version": 0, + }) + ws.receive_json() + + # With metrics disabled the counters must not have moved. + assert _total_connections(session) == before_total + assert _active(session) == before_active + assert _messages(session, "code_update") == 0.0 + + +# ── metric names exposed on /metrics endpoint ───────────────────────────────── + +def test_collaboration_metrics_appear_on_scrape_endpoint(): + """All four collaboration metric families must be present in the /metrics output.""" + # Trigger a connection so the label set is populated in the registry. + session = "obs-scrape" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + response = client.get("/metrics") + assert response.status_code == 200 + body = response.text + + assert "qyverixai_collaboration_active_connections" in body + assert "qyverixai_collaboration_connections_total" in body + assert "qyverixai_collaboration_messages_total" in body + assert "qyverixai_collaboration_errors_total" in body From 4d742bccfbfcb60e4a5d95d652d5b73b1bf4559d Mon Sep 17 00:00:00 2001 From: karrisanthoshigayatri Date: Wed, 22 Jul 2026 20:34:32 +0530 Subject: [PATCH 08/10] Refactor shared logic in collaboration websocket #1572 --- backend/app/routers/collaboration.py | 282 +++++++++++--------- backend/tests/test_collaboration_helpers.py | 158 +++++++++++ 2 files changed, 321 insertions(+), 119 deletions(-) create mode 100644 backend/tests/test_collaboration_helpers.py diff --git a/backend/app/routers/collaboration.py b/backend/app/routers/collaboration.py index df2fb7a7..0dd0b5c5 100644 --- a/backend/app/routers/collaboration.py +++ b/backend/app/routers/collaboration.py @@ -35,6 +35,60 @@ "#f25757", ] +# ── Shared helpers ──────────────────────────────────────────────────────────── + + +def _coerce_int(value: Any, default: int) -> int | None: + """Coerce *value* to ``int``, returning *default* on success or ``None`` on failure. + + Using ``None`` as the sentinel lets callers distinguish "conversion failed" + from a legitimately zero/negative value. + + Args: + value: Raw value from the WebSocket payload. + default: Value to use when *value* is ``None`` / missing. + + Returns: + The coerced integer, or ``None`` if the conversion raises. + """ + if value is None: + value = default + try: + return int(value) + except (TypeError, ValueError): + return None + + +async def _send_error( + socket: WebSocket | None, + session_id: str, + error_reason: str, + log_message: str, + detail: str, +) -> None: + """Log a warning, record the error metric, and send an error frame to the client. + + Centralises the repeated triple of: + 1. ``logger.warning(log_message)`` + 2. ``record_collab_error(session_id, error_reason)`` + 3. ``await socket.send_json({"type": "error", "detail": detail})`` + + Args: + socket: The client's WebSocket (may be ``None`` — send is skipped). + session_id: Collaboration session identifier for metric labelling. + error_reason: Label value for the ``qyverixai_collaboration_errors_total`` counter. + log_message: Already-formatted string passed directly to ``logger.warning``. + detail: Human-readable message included in the ``"error"`` frame sent to + the client. + """ + logger.warning(log_message) + record_collab_error(session_id, error_reason) + if socket is not None: + await socket.send_json({"type": "error", "detail": detail}) + + +# ── Data model ──────────────────────────────────────────────────────────────── + @dataclass class CollaborationRoom: @@ -47,6 +101,9 @@ class CollaborationRoom: lock: asyncio.Lock = field(default_factory=asyncio.Lock) +# ── Manager ─────────────────────────────────────────────────────────────────── + + class CollaborationManager: def __init__(self) -> None: self.rooms: dict[str, CollaborationRoom] = {} @@ -83,6 +140,8 @@ def _state_payload( "users": self._users_payload(room), } + # ── Connection lifecycle ────────────────────────────────────────────────── + async def connect( self, session_id: str, @@ -117,10 +176,7 @@ async def connect( record_collab_connect(session_id) await websocket.send_json(state) - await self.broadcast( - session_id, - {"type": "presence_update", "users": users}, - ) + await self.broadcast(session_id, {"type": "presence_update", "users": users}) return client_id async def disconnect(self, session_id: str, client_id: str) -> None: @@ -147,10 +203,9 @@ async def disconnect(self, session_id: str, client_id: str) -> None: logger.info("session_closed session_id=%s reason=last_client_left", session_id) return - await self.broadcast( - session_id, - {"type": "presence_update", "users": users}, - ) + await self.broadcast(session_id, {"type": "presence_update", "users": users}) + + # ── Broadcast ───────────────────────────────────────────────────────────── async def broadcast( self, @@ -167,7 +222,6 @@ async def broadcast( for client_id, socket in list(room.sockets.items()): if exclude is not None and client_id == exclude: continue - try: await socket.send_json(message) except Exception: @@ -185,6 +239,8 @@ async def broadcast( for client_id in stale_clients: await self.disconnect(session_id, client_id) + # ── Message dispatch ────────────────────────────────────────────────────── + async def handle_message( self, session_id: str, @@ -222,21 +278,18 @@ async def handle_message( await self._handle_comment_added(session_id, client_id, data) return - logger.warning( - "unsupported_message_type session_id=%s client_id=%s message_type=%s", - session_id, - client_id, - message_type, + # Unknown message type. + await _send_error( + socket=room.sockets.get(client_id), + session_id=session_id, + error_reason="unsupported_message_type", + log_message=( + "unsupported_message_type session_id=%s client_id=%s message_type=%s" + ), + detail=f"Unsupported collaboration message type: {message_type}", ) - record_collab_error(session_id, "unsupported_message_type") - socket = room.sockets.get(client_id) - if socket is not None: - await socket.send_json( - { - "type": "error", - "detail": f"Unsupported collaboration message type: {message_type}", - } - ) + + # ── Individual message handlers ─────────────────────────────────────────── async def _handle_code_update( self, @@ -253,50 +306,44 @@ async def _handle_code_update( language = data.get("language") # Safely coerce version — reject non-numeric values rather than crash. - raw_version = data.get("version", 0) - try: - incoming_version = int(raw_version) - except (TypeError, ValueError): - logger.warning( - "invalid_version_field session_id=%s client_id=%s reason=non_integer_version", - session_id, - client_id, + incoming_version = _coerce_int(data.get("version", 0), default=0) + if incoming_version is None: + await _send_error( + socket=socket, + session_id=session_id, + error_reason="non_integer_version", + log_message=( + "invalid_version_field session_id=%s client_id=%s" + " reason=non_integer_version" + ), + detail="version must be an integer", ) - record_collab_error(session_id, "non_integer_version") - if socket is not None: - await socket.send_json( - {"type": "error", "detail": "version must be an integer"} - ) return if not isinstance(code, str): - logger.warning( - "code_update_rejected session_id=%s client_id=%s reason=invalid_code_type", - session_id, - client_id, + await _send_error( + socket=socket, + session_id=session_id, + error_reason="invalid_code_type", + log_message=( + "code_update_rejected session_id=%s client_id=%s" + " reason=invalid_code_type" + ), + detail="code must be a string", ) - record_collab_error(session_id, "invalid_code_type") - if socket is not None: - await socket.send_json( - {"type": "error", "detail": "code must be a string"} - ) return if len(code) > MAX_CODE_CHARS: - logger.warning( - "code_update_rejected session_id=%s client_id=%s reason=code_too_long length=%d", - session_id, - client_id, - len(code), + await _send_error( + socket=socket, + session_id=session_id, + error_reason="code_too_long", + log_message=( + "code_update_rejected session_id=%s client_id=%s" + " reason=code_too_long length=%d" + ), + detail=f"code exceeds {MAX_CODE_CHARS} characters", ) - record_collab_error(session_id, "code_too_long") - if socket is not None: - await socket.send_json( - { - "type": "error", - "detail": f"code exceeds {MAX_CODE_CHARS} characters", - } - ) return async with room.lock: @@ -305,7 +352,8 @@ async def _handle_code_update( state["type"] = "sync_required" latest_socket = room.sockets.get(client_id) logger.debug( - "sync_required session_id=%s client_id=%s incoming_version=%d room_version=%d", + "sync_required session_id=%s client_id=%s" + " incoming_version=%d room_version=%d", session_id, client_id, incoming_version, @@ -314,8 +362,10 @@ async def _handle_code_update( else: room.version += 1 room.code = code - room.language = language if isinstance(language, str) else room.language - payload = { + room.language = ( + language if isinstance(language, str) else room.language + ) + state = { "type": "code_update", "code": room.code, "language": room.language, @@ -323,7 +373,6 @@ async def _handle_code_update( "senderId": client_id, } latest_socket = None - state = payload logger.debug( "code_update session_id=%s client_id=%s version=%d code_length=%d", session_id, @@ -350,41 +399,41 @@ async def _handle_cursor_update( return raw_cursor = data.get("cursor") - if not isinstance(raw_cursor, dict): return - # Safely coerce cursor fields — non-integer values must not crash. - try: - cursor = { - "line": max(1, int(raw_cursor.get("line", 1))), - "column": max(1, int(raw_cursor.get("column", 1))), - "selectionStart": max(0, int(raw_cursor.get("selectionStart", 0))), - "selectionEnd": max(0, int(raw_cursor.get("selectionEnd", 0))), - } - except (TypeError, ValueError): - logger.warning( - "cursor_update_rejected session_id=%s client_id=%s reason=non_integer_cursor_field", - session_id, - client_id, + # Safely coerce all four cursor fields — non-integer values must not crash. + line = _coerce_int(raw_cursor.get("line"), default=1) + column = _coerce_int(raw_cursor.get("column"), default=1) + sel_start = _coerce_int(raw_cursor.get("selectionStart"), default=0) + sel_end = _coerce_int(raw_cursor.get("selectionEnd"), default=0) + + if any(v is None for v in (line, column, sel_start, sel_end)): + await _send_error( + socket=room.sockets.get(client_id), + session_id=session_id, + error_reason="non_integer_cursor_field", + log_message=( + "cursor_update_rejected session_id=%s client_id=%s" + " reason=non_integer_cursor_field" + ), + detail="cursor fields must be integers", ) - record_collab_error(session_id, "non_integer_cursor_field") - socket = room.sockets.get(client_id) - if socket is not None: - await socket.send_json( - {"type": "error", "detail": "cursor fields must be integers"} - ) return + cursor = { + "line": max(1, line), # type: ignore[arg-type] + "column": max(1, column), # type: ignore[arg-type] + "selectionStart": max(0, sel_start), # type: ignore[arg-type] + "selectionEnd": max(0, sel_end), # type: ignore[arg-type] + } + async with room.lock: user = room.users.get(client_id) if user is None: return user["cursor"] = cursor - payload = { - "type": "cursor_update", - "user": user, - } + payload = {"type": "cursor_update", "user": user} logger.debug( "cursor_update session_id=%s client_id=%s line=%d column=%d", session_id, @@ -407,44 +456,38 @@ async def _handle_comment_added( return text = str(data.get("text", "")).strip() - - # Safely coerce line number. - raw_line = data.get("line", 1) - try: - line = max(1, int(raw_line)) - except (TypeError, ValueError): + # Safely coerce line number; default to 1 if missing or non-integer. + line = _coerce_int(data.get("line"), default=1) + if line is None: line = 1 + line = max(1, line) socket = room.sockets.get(client_id) if not text: - logger.warning( - "comment_rejected session_id=%s client_id=%s reason=empty_comment_text", - session_id, - client_id, + await _send_error( + socket=socket, + session_id=session_id, + error_reason="empty_comment_text", + log_message=( + "comment_rejected session_id=%s client_id=%s" + " reason=empty_comment_text" + ), + detail="comment text is required", ) - record_collab_error(session_id, "empty_comment_text") - if socket is not None: - await socket.send_json( - {"type": "error", "detail": "comment text is required"} - ) return if len(text) > MAX_COMMENT_CHARS: - logger.warning( - "comment_rejected session_id=%s client_id=%s reason=comment_too_long length=%d", - session_id, - client_id, - len(text), + await _send_error( + socket=socket, + session_id=session_id, + error_reason="comment_too_long", + log_message=( + "comment_rejected session_id=%s client_id=%s" + " reason=comment_too_long length=%d" + ), + detail=f"comment exceeds {MAX_COMMENT_CHARS} characters", ) - record_collab_error(session_id, "comment_too_long") - if socket is not None: - await socket.send_json( - { - "type": "error", - "detail": f"comment exceeds {MAX_COMMENT_CHARS} characters", - } - ) return async with room.lock: @@ -479,6 +522,9 @@ async def _handle_comment_added( manager = CollaborationManager() +# ── WebSocket endpoint ──────────────────────────────────────────────────────── + + @router.websocket("/ws/{session_id}") async def collaboration_websocket( websocket: WebSocket, @@ -507,10 +553,7 @@ async def collaboration_websocket( record_collab_error(session_id, "malformed_json") try: await websocket.send_json( - { - "type": "error", - "detail": "message must be valid JSON", - } + {"type": "error", "detail": "message must be valid JSON"} ) except Exception: pass @@ -520,7 +563,8 @@ async def collaboration_websocket( await manager.handle_message(session_id, client_id, data) else: logger.warning( - "non_object_payload session_id=%s client_id=%s reason=non_object_payload", + "non_object_payload session_id=%s client_id=%s" + " reason=non_object_payload", session_id, client_id, ) diff --git a/backend/tests/test_collaboration_helpers.py b/backend/tests/test_collaboration_helpers.py new file mode 100644 index 00000000..a2b8d07b --- /dev/null +++ b/backend/tests/test_collaboration_helpers.py @@ -0,0 +1,158 @@ +""" +Unit tests for the shared helper functions extracted during the +collaboration WebSocket refactoring (issue #1572). + +Tests cover: +- _coerce_int: safe integer coercion used for version, cursor fields, line numbers +- _send_error: the log + metric + send triple + +These helpers are pure / easily awaitable and do not require the full +FastAPI TestClient, so they run quickly without any network overhead. +""" + +from __future__ import annotations + +import asyncio +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from app.routers.collaboration import _coerce_int, _send_error + + +# ── _coerce_int ─────────────────────────────────────────────────────────────── + + +class TestCoerceInt: + """_coerce_int(value, default) -> int | None""" + + def test_integer_value_returned_unchanged(self): + assert _coerce_int(5, default=0) == 5 + + def test_zero_is_a_valid_int(self): + assert _coerce_int(0, default=1) == 0 + + def test_negative_integer_returned_unchanged(self): + assert _coerce_int(-3, default=0) == -3 + + def test_float_is_truncated_to_int(self): + assert _coerce_int(3.9, default=0) == 3 + + def test_numeric_string_coerced(self): + assert _coerce_int("42", default=0) == 42 + + def test_none_uses_default(self): + assert _coerce_int(None, default=7) == 7 + + def test_non_numeric_string_returns_none(self): + assert _coerce_int("bad", default=0) is None + + def test_empty_string_returns_none(self): + assert _coerce_int("", default=0) is None + + def test_list_value_returns_none(self): + assert _coerce_int([1, 2], default=0) is None + + def test_dict_value_returns_none(self): + assert _coerce_int({"x": 1}, default=0) is None + + def test_bool_true_coerces_to_one(self): + # bool is a subclass of int in Python; int(True) == 1 + assert _coerce_int(True, default=0) == 1 + + def test_bool_false_coerces_to_zero(self): + assert _coerce_int(False, default=5) == 0 + + def test_default_used_only_when_value_is_none(self): + # Passing 0 explicitly must NOT fall back to default + assert _coerce_int(0, default=99) == 0 + + +# ── _send_error ─────────────────────────────────────────────────────────────── + + +class TestSendError: + """_send_error is async; each test runs a minimal event loop.""" + + def _run(self, coro): + return asyncio.get_event_loop().run_until_complete(coro) + + def test_logs_warning_message(self): + socket = AsyncMock() + with patch("app.routers.collaboration.logger") as mock_logger, \ + patch("app.routers.collaboration.record_collab_error"): + self._run( + _send_error( + socket=socket, + session_id="s1", + error_reason="test_reason", + log_message="something went wrong", + detail="client-facing detail", + ) + ) + mock_logger.warning.assert_called_once_with("something went wrong") + + def test_records_error_metric(self): + socket = AsyncMock() + with patch("app.routers.collaboration.logger"), \ + patch("app.routers.collaboration.record_collab_error") as mock_metric: + self._run( + _send_error( + socket=socket, + session_id="s2", + error_reason="my_reason", + log_message="msg", + detail="detail", + ) + ) + mock_metric.assert_called_once_with("s2", "my_reason") + + def test_sends_error_frame_to_socket(self): + socket = AsyncMock() + with patch("app.routers.collaboration.logger"), \ + patch("app.routers.collaboration.record_collab_error"): + self._run( + _send_error( + socket=socket, + session_id="s3", + error_reason="r", + log_message="msg", + detail="the error detail", + ) + ) + socket.send_json.assert_awaited_once_with( + {"type": "error", "detail": "the error detail"} + ) + + def test_none_socket_skips_send(self): + """When the socket is None the send must be skipped without error.""" + with patch("app.routers.collaboration.logger"), \ + patch("app.routers.collaboration.record_collab_error"): + # Should not raise. + self._run( + _send_error( + socket=None, + session_id="s4", + error_reason="r", + log_message="msg", + detail="detail", + ) + ) + + def test_all_three_actions_called_for_valid_socket(self): + """Smoke test: log + metric + send all happen in a single call.""" + socket = AsyncMock() + with patch("app.routers.collaboration.logger") as mock_logger, \ + patch("app.routers.collaboration.record_collab_error") as mock_metric: + self._run( + _send_error( + socket=socket, + session_id="s5", + error_reason="triple_check", + log_message="triple log", + detail="triple detail", + ) + ) + assert mock_logger.warning.call_count == 1 + assert mock_metric.call_count == 1 + assert socket.send_json.await_count == 1 From 346055e2557e2ad28af3cf39359b2482d67a6799 Mon Sep 17 00:00:00 2001 From: karrisanthoshigayatri Date: Wed, 22 Jul 2026 20:40:35 +0530 Subject: [PATCH 09/10] Harden session lifecycle handling in collaboration comments #1573 --- backend/app/routers/collaboration.py | 65 +++- backend/tests/test_collaboration_comments.py | 304 +++++++++++++++++++ 2 files changed, 362 insertions(+), 7 deletions(-) create mode 100644 backend/tests/test_collaboration_comments.py diff --git a/backend/app/routers/collaboration.py b/backend/app/routers/collaboration.py index 0dd0b5c5..77f14b2b 100644 --- a/backend/app/routers/collaboration.py +++ b/backend/app/routers/collaboration.py @@ -25,6 +25,7 @@ MAX_CODE_CHARS = 50_000 MAX_COMMENT_CHARS = 1_000 MAX_SESSION_ID_CHARS = 128 +MAX_COMMENTS_PER_SESSION = 500 COLORS = [ "#5b9cf6", @@ -455,23 +456,54 @@ async def _handle_comment_added( if room is None: return - text = str(data.get("text", "")).strip() + socket = room.sockets.get(client_id) + + # Reject comments from a client that is no longer registered in the + # room (e.g. a race between disconnect and a queued comment message). + if client_id not in room.users: + await _send_error( + socket=socket, + session_id=session_id, + error_reason="unregistered_commenter", + log_message=( + f"comment_rejected session_id={session_id}" + f" client_id={client_id} reason=unregistered_commenter" + ), + detail="comment rejected: client is not registered in this session", + ) + return + + # Reject non-string text up front rather than silently coercing it. + raw_text = data.get("text") + if not isinstance(raw_text, str): + await _send_error( + socket=socket, + session_id=session_id, + error_reason="invalid_comment_text_type", + log_message=( + f"comment_rejected session_id={session_id}" + f" client_id={client_id} reason=invalid_comment_text_type" + ), + detail="comment text must be a string", + ) + return + + text = raw_text.strip() + # Safely coerce line number; default to 1 if missing or non-integer. line = _coerce_int(data.get("line"), default=1) if line is None: line = 1 line = max(1, line) - socket = room.sockets.get(client_id) - if not text: await _send_error( socket=socket, session_id=session_id, error_reason="empty_comment_text", log_message=( - "comment_rejected session_id=%s client_id=%s" - " reason=empty_comment_text" + f"comment_rejected session_id={session_id}" + f" client_id={client_id} reason=empty_comment_text" ), detail="comment text is required", ) @@ -483,14 +515,33 @@ async def _handle_comment_added( session_id=session_id, error_reason="comment_too_long", log_message=( - "comment_rejected session_id=%s client_id=%s" - " reason=comment_too_long length=%d" + f"comment_rejected session_id={session_id}" + f" client_id={client_id} reason=comment_too_long" + f" length={len(text)}" ), detail=f"comment exceeds {MAX_COMMENT_CHARS} characters", ) return async with room.lock: + # Prevent unbounded comment accumulation — reject once the cap is + # reached so the room state stays bounded in both memory and wire size. + if len(room.comments) >= MAX_COMMENTS_PER_SESSION: + await _send_error( + socket=socket, + session_id=session_id, + error_reason="comment_limit_reached", + log_message=( + f"comment_rejected session_id={session_id}" + f" client_id={client_id} reason=comment_limit_reached" + f" limit={MAX_COMMENTS_PER_SESSION}" + ), + detail=( + f"session comment limit of {MAX_COMMENTS_PER_SESSION} reached" + ), + ) + return + user = room.users.get(client_id, {}) comment = { "id": uuid.uuid4().hex[:12], diff --git a/backend/tests/test_collaboration_comments.py b/backend/tests/test_collaboration_comments.py new file mode 100644 index 00000000..5e626ed1 --- /dev/null +++ b/backend/tests/test_collaboration_comments.py @@ -0,0 +1,304 @@ +""" +Tests for hardened session lifecycle handling in collaboration comments (#1573). + +Covers all new comment-specific guards: +- non-string text field rejected with an error +- empty / whitespace-only text rejected +- text exceeding MAX_COMMENT_CHARS rejected +- comment limit (MAX_COMMENTS_PER_SESSION) enforced +- unregistered client (race between disconnect and queued comment) rejected +- valid comments still work correctly end-to-end +- comments are cleared when the session is destroyed (room teardown) +- comment line defaults to 1 for missing / non-integer line values +- comment author and color come from the registered user entry +""" + +from app import main as app_main +from app.routers.collaboration import ( + MAX_COMMENT_CHARS, + MAX_COMMENTS_PER_SESSION, + manager, +) +from fastapi.testclient import TestClient + +client = TestClient(app_main.app) + + +def setup_function(): + manager.reset() + + +# ── valid comment ───────────────────────────────────────────────────────────── + +def test_valid_comment_is_broadcast_to_all_clients(): + session = "cmt-valid" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as alice: + alice.receive_json() # session_state + alice.receive_json() # presence_update + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Bob") as bob: + bob.receive_json() # session_state + alice.receive_json() # presence_update for Bob joining + bob.receive_json() # presence_update + + alice.send_json({"type": "comment_added", "line": 5, "text": "This looks good."}) + + alice_msg = alice.receive_json() + bob_msg = bob.receive_json() + + assert alice_msg["type"] == "comment_added" + assert alice_msg["comment"]["line"] == 5 + assert alice_msg["comment"]["text"] == "This looks good." + assert alice_msg["comment"]["author"] == "Alice" + assert len(alice_msg["comments"]) == 1 + + assert bob_msg["type"] == "comment_added" + assert bob_msg["comment"]["text"] == "This looks good." + + +def test_comment_author_and_color_come_from_registered_user(): + session = "cmt-author" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Charlie") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": "Hi."}) + msg = ws.receive_json() + + assert msg["comment"]["author"] == "Charlie" + assert msg["comment"]["authorId"] + # Color must be one of the defined palette values. + from app.routers.collaboration import COLORS + assert msg["comment"]["color"] in COLORS + + +# ── text validation ─────────────────────────────────────────────────────────── + +def test_non_string_text_returns_error(): + """text field that is not a string must return an error, not be silently coerced.""" + session = "cmt-texttype" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": [1, 2, 3]}) + msg = ws.receive_json() + + assert msg["type"] == "error" + assert "string" in msg["detail"].lower() + + +def test_non_string_text_integer_returns_error(): + session = "cmt-textint" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": 42}) + msg = ws.receive_json() + + assert msg["type"] == "error" + assert "string" in msg["detail"].lower() + + +def test_empty_string_text_returns_error(): + session = "cmt-empty" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": ""}) + msg = ws.receive_json() + + assert msg["type"] == "error" + assert "required" in msg["detail"].lower() + + +def test_whitespace_only_text_returns_error(): + session = "cmt-whitespace" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": " \t "}) + msg = ws.receive_json() + + assert msg["type"] == "error" + assert "required" in msg["detail"].lower() + + +def test_text_exceeding_max_chars_returns_error(): + session = "cmt-toolong" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({ + "type": "comment_added", + "line": 1, + "text": "x" * (MAX_COMMENT_CHARS + 1), + }) + msg = ws.receive_json() + + assert msg["type"] == "error" + assert str(MAX_COMMENT_CHARS) in msg["detail"] + + +def test_text_at_exact_max_chars_is_accepted(): + session = "cmt-exactmax" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({ + "type": "comment_added", + "line": 1, + "text": "y" * MAX_COMMENT_CHARS, + }) + msg = ws.receive_json() + + assert msg["type"] == "comment_added" + assert len(msg["comment"]["text"]) == MAX_COMMENT_CHARS + + +# ── line number handling ────────────────────────────────────────────────────── + +def test_missing_line_defaults_to_one(): + session = "cmt-noline" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "text": "No line field."}) + msg = ws.receive_json() + + assert msg["type"] == "comment_added" + assert msg["comment"]["line"] == 1 + + +def test_non_integer_line_defaults_to_one(): + session = "cmt-badline" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": "bad", "text": "Valid text."}) + msg = ws.receive_json() + + assert msg["type"] == "comment_added" + assert msg["comment"]["line"] == 1 + + +def test_zero_line_is_clamped_to_one(): + session = "cmt-zeroline" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 0, "text": "Zero line."}) + msg = ws.receive_json() + + assert msg["type"] == "comment_added" + assert msg["comment"]["line"] == 1 + + +def test_negative_line_is_clamped_to_one(): + session = "cmt-negline" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": -5, "text": "Negative line."}) + msg = ws.receive_json() + + assert msg["type"] == "comment_added" + assert msg["comment"]["line"] == 1 + + +# ── comment cap ─────────────────────────────────────────────────────────────── + +def test_comment_limit_is_enforced(): + """Once MAX_COMMENTS_PER_SESSION comments exist, further adds are rejected.""" + session = "cmt-caplimit" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + # Fill up to the cap. + for i in range(MAX_COMMENTS_PER_SESSION): + ws.send_json({"type": "comment_added", "line": 1, "text": f"Comment {i}"}) + ws.receive_json() + + # The next comment must be rejected. + ws.send_json({"type": "comment_added", "line": 1, "text": "One too many."}) + error_msg = ws.receive_json() + + assert error_msg["type"] == "error" + assert "limit" in error_msg["detail"].lower() + + +def test_comment_count_does_not_exceed_cap(): + """The room's comment list must never grow beyond MAX_COMMENTS_PER_SESSION.""" + session = "cmt-capcount" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + for i in range(MAX_COMMENTS_PER_SESSION + 5): + ws.send_json({"type": "comment_added", "line": 1, "text": f"C{i}"}) + ws.receive_json() # accept or error — consume either way + + room = manager.rooms.get(session) + assert room is not None + assert len(room.comments) <= MAX_COMMENTS_PER_SESSION + + +# ── session teardown ────────────────────────────────────────────────────────── + +def test_comments_cleared_when_session_destroyed(): + """Room and its comments must be removed when the last client disconnects.""" + session = "cmt-teardown" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": "Will be gone."}) + ws.receive_json() + + assert session in manager.rooms + assert len(manager.rooms[session].comments) == 1 + + # After the last client disconnects the room must be deleted. + assert session not in manager.rooms + + +def test_new_session_starts_with_empty_comments(): + """A fresh session must have an empty comment list.""" + session = "cmt-fresh" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + state = ws.receive_json() + ws.receive_json() + + assert state["comments"] == [] + + +def test_rejoining_same_session_name_starts_fresh(): + """A reconnecting client gets a clean room (prior session was torn down).""" + session = "cmt-rejoin" + + # First connection: add a comment then disconnect. + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + ws.send_json({"type": "comment_added", "line": 1, "text": "Old comment."}) + ws.receive_json() + + # Room is gone. + assert session not in manager.rooms + + # Second connection: should see no prior comments. + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + state = ws.receive_json() + ws.receive_json() + + assert state["comments"] == [] From a015e90538d284d8f3c42c8ad4920be8beeda702 Mon Sep 17 00:00:00 2001 From: karrisanthoshigayatri Date: Wed, 22 Jul 2026 20:46:40 +0530 Subject: [PATCH 10/10] Improve reconnect behavior in collaboration comments #1574 --- backend/app/routers/collaboration.py | 58 +++- backend/tests/test_collaboration_reconnect.py | 298 ++++++++++++++++++ 2 files changed, 355 insertions(+), 1 deletion(-) create mode 100644 backend/tests/test_collaboration_reconnect.py diff --git a/backend/app/routers/collaboration.py b/backend/app/routers/collaboration.py index 77f14b2b..b94fc237 100644 --- a/backend/app/routers/collaboration.py +++ b/backend/app/routers/collaboration.py @@ -130,6 +130,9 @@ def _state_payload( room: CollaborationRoom, client_id: str, ) -> dict[str, Any]: + # Take a shallow copy of comments so the serialised snapshot is + # stable even if another coroutine appends to room.comments before + # send_json flushes the data to the wire. return { "type": "session_state", "sessionId": session_id, @@ -137,8 +140,18 @@ def _state_payload( "code": room.code, "language": room.language, "version": room.version, - "comments": room.comments, + "comments": list(room.comments), "users": self._users_payload(room), + # True when the client is reconnecting into a room that already + # has state (other users present, code written, or prior comments). + # The client-side can use this flag to decide whether to show a + # "session restored" notice rather than the default "new session" + # welcome UI, without having to diff the payload itself. + "reconnected": bool( + room.code + or room.comments + or len(room.users) > 1 + ), } # ── Connection lifecycle ────────────────────────────────────────────────── @@ -279,6 +292,10 @@ async def handle_message( await self._handle_comment_added(session_id, client_id, data) return + if message_type == "comment_sync": + await self._handle_comment_sync(session_id, client_id) + return + # Unknown message type. await _send_error( socket=room.sockets.get(client_id), @@ -446,6 +463,45 @@ async def _handle_cursor_update( await self.broadcast(session_id, payload, exclude=client_id) + async def _handle_comment_sync( + self, + session_id: str, + client_id: str, + ) -> None: + """Send the current comment list to the requesting client. + + A reconnecting client can send ``{"type": "comment_sync"}`` immediately + after receiving its ``session_state`` to explicitly request a fresh + snapshot of the comment list. This handles the narrow race window where + comments were appended between the ``session_state`` snapshot and the + client finishing its own setup (e.g. async JS framework rendering). + + The response type ``"comment_sync"`` is distinct from ``"comment_added"`` + so the client can treat it as a full replacement rather than a delta. + """ + room = self._get_room_if_exists(session_id) + if room is None: + return + + socket = room.sockets.get(client_id) + if socket is None: + return + + async with room.lock: + comments_snapshot = list(room.comments) + + logger.debug( + "comment_sync session_id=%s client_id=%s comment_count=%d", + session_id, + client_id, + len(comments_snapshot), + ) + + await socket.send_json({ + "type": "comment_sync", + "comments": comments_snapshot, + }) + async def _handle_comment_added( self, session_id: str, diff --git a/backend/tests/test_collaboration_reconnect.py b/backend/tests/test_collaboration_reconnect.py new file mode 100644 index 00000000..6f1380d7 --- /dev/null +++ b/backend/tests/test_collaboration_reconnect.py @@ -0,0 +1,298 @@ +""" +Tests for improved reconnect behavior in collaboration comments (#1574). + +Covers: +- reconnecting client receives full comment history in session_state +- session_state.reconnected flag is False for a fresh empty session +- session_state.reconnected flag is True when code exists +- session_state.reconnected flag is True when prior comments exist +- session_state.reconnected flag is True when other users are present +- session_state.comments is a stable snapshot (copy, not live reference) +- comment_sync request returns current comment list to requester only +- comment_sync does not broadcast to other clients +- comment_sync after reconnect delivers comments added during absence +- reconnecting after last client left starts a fresh empty session +- reconnecting while another user is still connected restores full state +- comments added between two clients are visible to a third reconnecting client +""" + +from app import main as app_main +from app.routers.collaboration import manager +from fastapi.testclient import TestClient + +client = TestClient(app_main.app) + + +def setup_function(): + manager.reset() + + +# ── reconnected flag ────────────────────────────────────────────────────────── + +def test_fresh_empty_session_has_reconnected_false(): + """A brand-new session with no code, no comments, and only one user + must report reconnected=False.""" + with client.websocket_connect("/collaboration/ws/rc-fresh?name=Alice") as ws: + state = ws.receive_json() + ws.receive_json() # presence_update + + assert state["type"] == "session_state" + assert state["reconnected"] is False + + +def test_reconnected_true_when_other_user_present(): + """When a second client joins a room that already has one user, + reconnected must be True (state exists in the room).""" + with client.websocket_connect("/collaboration/ws/rc-otheruser?name=Alice") as alice: + alice.receive_json() + alice.receive_json() + + with client.websocket_connect("/collaboration/ws/rc-otheruser?name=Bob") as bob: + bob_state = bob.receive_json() + alice.receive_json() # presence_update for Bob + bob.receive_json() # presence_update + + assert bob_state["reconnected"] is True + + +def test_reconnected_true_when_code_exists(): + """A client joining a session that already has code must get reconnected=True.""" + with client.websocket_connect("/collaboration/ws/rc-code?name=Alice") as alice: + alice_state = alice.receive_json() + alice.receive_json() + + alice.send_json({ + "type": "code_update", + "code": "print('hello')", + "language": "python", + "version": alice_state["version"], + }) + alice.receive_json() # code_update broadcast back + + # Alice left — session is destroyed. Rejoin to create fresh. + with client.websocket_connect("/collaboration/ws/rc-code2?name=Alice") as alice: + alice_state = alice.receive_json() + alice.receive_json() + + alice.send_json({ + "type": "code_update", + "code": "x = 1", + "language": "python", + "version": alice_state["version"], + }) + alice.receive_json() + + with client.websocket_connect("/collaboration/ws/rc-code2?name=Bob") as bob: + bob_state = bob.receive_json() + alice.receive_json() + bob.receive_json() + + assert bob_state["reconnected"] is True + assert bob_state["code"] == "x = 1" + + +def test_reconnected_true_when_prior_comments_exist(): + """A client joining after comments have been posted must get reconnected=True.""" + with client.websocket_connect("/collaboration/ws/rc-cmt?name=Alice") as alice: + alice.receive_json() + alice.receive_json() + + alice.send_json({"type": "comment_added", "line": 1, "text": "A comment."}) + alice.receive_json() + + with client.websocket_connect("/collaboration/ws/rc-cmt?name=Bob") as bob: + bob_state = bob.receive_json() + alice.receive_json() + bob.receive_json() + + assert bob_state["reconnected"] is True + assert len(bob_state["comments"]) == 1 + assert bob_state["comments"][0]["text"] == "A comment." + + +# ── comment history on reconnect ────────────────────────────────────────────── + +def test_reconnecting_client_receives_full_comment_history(): + """After partial disconnect and reconnect, the returning client must receive + all comments that were added while they were away.""" + session = "rc-history" + + # Alice and Bob connect; Alice posts a comment. + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as alice: + alice.receive_json() + alice.receive_json() + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Bob") as bob: + bob.receive_json() + alice.receive_json() + bob.receive_json() + + alice.send_json({"type": "comment_added", "line": 2, "text": "First comment."}) + alice.receive_json() # broadcast to alice + bob.receive_json() # broadcast to bob + + # Bob disconnects. Alice adds another comment. + alice.receive_json() # presence_update for Bob leaving + + alice.send_json({"type": "comment_added", "line": 3, "text": "Second comment."}) + alice.receive_json() + + # Bob reconnects. + with client.websocket_connect(f"/collaboration/ws/{session}?name=Bob") as bob2: + state = bob2.receive_json() + alice.receive_json() # presence_update for Bob2 joining + bob2.receive_json() # presence_update + + assert state["type"] == "session_state" + assert len(state["comments"]) == 2 + assert state["comments"][0]["text"] == "First comment." + assert state["comments"][1]["text"] == "Second comment." + assert state["reconnected"] is True + + +def test_reconnected_after_full_teardown_gets_empty_state(): + """When a session was fully torn down (all clients left), a new connection + to the same session_id starts with zero comments and reconnected=False.""" + session = "rc-teardown" + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + ws.send_json({"type": "comment_added", "line": 1, "text": "Gone forever."}) + ws.receive_json() + + # Session is destroyed after Alice leaves. + assert session not in manager.rooms + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + state = ws.receive_json() + ws.receive_json() + + assert state["comments"] == [] + assert state["reconnected"] is False + + +# ── comment_sync message ────────────────────────────────────────────────────── + +def test_comment_sync_returns_current_comments_to_requester(): + """Sending comment_sync must return a comment_sync frame with the current + comment list to the requesting client only.""" + with client.websocket_connect("/collaboration/ws/rc-sync1?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": "Sync me."}) + ws.receive_json() # comment_added broadcast + + ws.send_json({"type": "comment_sync"}) + sync = ws.receive_json() + + assert sync["type"] == "comment_sync" + assert len(sync["comments"]) == 1 + assert sync["comments"][0]["text"] == "Sync me." + + +def test_comment_sync_not_broadcast_to_other_clients(): + """comment_sync must be a unicast response — other clients must not receive it.""" + session = "rc-sync2" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as alice: + alice.receive_json() + alice.receive_json() + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Bob") as bob: + bob.receive_json() + alice.receive_json() + bob.receive_json() + + alice.send_json({"type": "comment_added", "line": 1, "text": "Hello."}) + alice.receive_json() # comment_added to alice + bob.receive_json() # comment_added to bob + + # Bob requests a sync — Alice must not receive anything. + bob.send_json({"type": "comment_sync"}) + sync = bob.receive_json() + + # Verify no extra message is waiting for Alice by sending a ping. + alice.send_json({"type": "ping"}) + pong = alice.receive_json() + + assert sync["type"] == "comment_sync" + assert pong["type"] == "pong" + + +def test_comment_sync_empty_when_no_comments(): + """comment_sync on a session with no comments returns an empty list.""" + with client.websocket_connect("/collaboration/ws/rc-sync3?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_sync"}) + sync = ws.receive_json() + + assert sync["type"] == "comment_sync" + assert sync["comments"] == [] + + +def test_comment_sync_reflects_comments_added_after_reconnect(): + """A comment added between the session_state snapshot and the comment_sync + request must appear in the sync response.""" + session = "rc-sync4" + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as alice: + alice.receive_json() + alice.receive_json() + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Bob") as bob: + bob_state = bob.receive_json() + alice.receive_json() + bob.receive_json() + + # Alice adds a comment after Bob's session_state was built. + alice.send_json({"type": "comment_added", "line": 1, "text": "Late comment."}) + alice.receive_json() + bob.receive_json() # Bob also got the comment_added broadcast + + # Bob requests a sync to confirm his comment list is up-to-date. + bob.send_json({"type": "comment_sync"}) + sync = bob.receive_json() + + # Bob's session_state snapshot may or may not have had 0 comments + # (timing-dependent), but the sync must always reflect the current state. + assert sync["type"] == "comment_sync" + assert any(c["text"] == "Late comment." for c in sync["comments"]) + + +# ── snapshot isolation ──────────────────────────────────────────────────────── + +def test_session_state_comments_is_a_snapshot_not_live_reference(): + """The comments list in session_state must be a copy — mutating + the room's comment list after state was built must not affect the + already-sent payload (this is verified via the manager's room state).""" + session = "rc-snapshot" + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as alice: + alice.receive_json() + alice.receive_json() + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Bob") as bob: + bob_state = bob.receive_json() + alice.receive_json() + bob.receive_json() + + # The list Bob received in his state snapshot is a copy. + # Confirm the room's current comments list is a different object + # by checking that adding a comment grows the room list but the + # already-received snapshot stays at its original length. + initial_count = len(bob_state["comments"]) + + alice.send_json({"type": "comment_added", "line": 1, "text": "New comment."}) + alice.receive_json() + bob.receive_json() # consume the comment_added broadcast + + # The room now has one more comment than when Bob's state was built. + room = manager.rooms.get(session) + assert room is not None + assert len(room.comments) == initial_count + 1 + + # Bob's original snapshot must be unchanged. + assert len(bob_state["comments"]) == initial_count