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. 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/observability.py b/backend/app/observability.py index 5ce43919..d2171ed6 100644 --- a/backend/app/observability.py +++ b/backend/app/observability.py @@ -112,6 +112,130 @@ 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"), +) + +COLLAB_COMMENTS_TOTAL = Counter( + "qyverixai_collaboration_comments_total", + "Total number of comments successfully added across all collaboration sessions since process start.", + labelnames=("session_id",), +) + +COLLAB_COMMENT_COUNT = Gauge( + "qyverixai_collaboration_comment_count", + "Current number of comments stored in each active collaboration session.", + labelnames=("session_id",), +) + + +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 + + +def record_collab_comment_added(session_id: str) -> None: + """Increment the comment-added counter and gauge when a comment is accepted. + + This is separate from ``record_collab_message(session_id, "comment_added")`` + so that the dedicated comment gauge can be managed independently from the + generic message counter — in particular it can be decremented when a session + is destroyed via ``record_collab_session_closed``. + """ + if not metrics_enabled(): + return + try: + COLLAB_COMMENTS_TOTAL.labels(session_id=session_id).inc() + COLLAB_COMMENT_COUNT.labels(session_id=session_id).inc() + except Exception: # pragma: no cover + pass + + +def record_collab_session_closed(session_id: str, comment_count: int) -> None: + """Reset the comment-count gauge to zero when a session is destroyed. + + Called when the last client disconnects so the gauge accurately reflects + that the session's comment buffer no longer exists in memory. + + Args: + session_id: The session that was destroyed. + comment_count: The number of comments that were in the room at close + time (used to decrement the gauge by the exact amount). + """ + if not metrics_enabled(): + return + try: + COLLAB_COMMENT_COUNT.labels(session_id=session_id).dec(comment_count) + 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 04a70766..bdade956 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 @@ -10,10 +11,23 @@ from fastapi import APIRouter, Query, WebSocket, WebSocketDisconnect +from ..observability import ( + record_collab_comment_added, + record_collab_connect, + record_collab_disconnect, + record_collab_error, + record_collab_message, + record_collab_session_closed, +) + router = APIRouter() +logger = logging.getLogger(__name__) + MAX_CODE_CHARS = 50_000 MAX_COMMENT_CHARS = 1_000 +MAX_SESSION_ID_CHARS = 128 +MAX_COMMENTS_PER_SESSION = 500 COLORS = [ "#5b9cf6", @@ -24,6 +38,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: @@ -36,6 +104,9 @@ class CollaborationRoom: lock: asyncio.Lock = field(default_factory=asyncio.Lock) +# ── Manager ─────────────────────────────────────────────────────────────────── + + class CollaborationManager: def __init__(self) -> None: self.rooms: dict[str, CollaborationRoom] = {} @@ -48,6 +119,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()) @@ -57,6 +132,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, @@ -64,10 +142,22 @@ 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 ────────────────────────────────────────────────── + async def connect( self, session_id: str, @@ -93,11 +183,16 @@ async def connect( state = self._state_payload(session_id, room, client_id) users = self._users_payload(room) - await websocket.send_json(state) - await self.broadcast( + logger.info( + "client_connected session_id=%s client_id=%s active_users=%d", session_id, - {"type": "presence_update", "users": users}, + client_id, + len(users), ) + record_collab_connect(session_id) + + await websocket.send_json(state) + await self.broadcast(session_id, {"type": "presence_update", "users": users}) return client_id async def disconnect(self, session_id: str, client_id: str) -> None: @@ -110,15 +205,25 @@ async def disconnect(self, session_id: str, client_id: str) -> None: room.users.pop(client_id, None) users = self._users_payload(room) should_delete = not room.sockets + comment_count = len(room.comments) if should_delete else 0 + + logger.info( + "client_disconnected session_id=%s client_id=%s remaining_users=%d", + session_id, + client_id, + len(users), + ) + record_collab_disconnect(session_id) if should_delete: self.rooms.pop(session_id, None) + logger.info("session_closed session_id=%s reason=last_client_left", session_id) + record_collab_session_closed(session_id, comment_count) 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, @@ -135,22 +240,42 @@ 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 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, + ) + record_collab_error(session_id, "stale_socket") stale_clients.append(client_id) for client_id in stale_clients: await self.disconnect(session_id, client_id) + # ── Message dispatch ────────────────────────────────────────────────────── + async def handle_message( self, session_id: str, 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,14 +296,22 @@ async def handle_message( await self._handle_comment_added(session_id, client_id, data) return - 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}", - } - ) + 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), + 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}", + ) + + # ── Individual message handlers ─────────────────────────────────────────── async def _handle_code_update( self, @@ -186,27 +319,53 @@ 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. + 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", + ) + return if not isinstance(code, str): - if socket is not None: - await socket.send_json( - {"type": "error", "detail": "code must be a string"} - ) + 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", + ) return if len(code) > MAX_CODE_CHARS: - if socket is not None: - await socket.send_json( - { - "type": "error", - "detail": f"code exceeds {MAX_CODE_CHARS} characters", - } - ) + 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", + ) return async with room.lock: @@ -214,11 +373,21 @@ 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 - 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, @@ -226,7 +395,14 @@ 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, + client_id, + room.version, + len(code), + ) + record_collab_message(session_id, "code_update") if latest_socket is not None: await latest_socket.send_json(state) @@ -240,17 +416,38 @@ async def _handle_cursor_update( client_id: str, data: dict[str, Any], ) -> None: - room = self._get_room(session_id) - raw_cursor = data.get("cursor") + 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 + # 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", + ) + 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))), + "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: @@ -258,43 +455,154 @@ async def _handle_cursor_update( 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, + client_id, + cursor["line"], + cursor["column"], + ) + record_collab_message(session_id, "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), + ) + record_collab_message(session_id, "comment_sync") + + await socket.send_json({ + "type": "comment_sync", + "comments": comments_snapshot, + }) + async def _handle_comment_added( self, session_id: str, client_id: str, data: dict[str, Any], ) -> None: - room = self._get_room(session_id) - text = str(data.get("text", "")).strip() - line = max(1, int(data.get("line", 1))) + room = self._get_room_if_exists(session_id) + if room is None: + return 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) + if not text: - if socket is not None: - await socket.send_json( - {"type": "error", "detail": "comment text is required"} - ) + await _send_error( + socket=socket, + session_id=session_id, + error_reason="empty_comment_text", + log_message=( + f"comment_rejected session_id={session_id}" + f" client_id={client_id} reason=empty_comment_text" + ), + detail="comment text is required", + ) return if len(text) > MAX_COMMENT_CHARS: - if socket is not None: - await socket.send_json( - { - "type": "error", - "detail": f"comment exceeds {MAX_COMMENT_CHARS} characters", - } - ) + await _send_error( + socket=socket, + session_id=session_id, + error_reason="comment_too_long", + log_message=( + 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], @@ -311,6 +619,15 @@ 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, + ) + record_collab_message(session_id, "comment_added") + record_collab_comment_added(session_id) await self.broadcast(session_id, payload) @@ -318,20 +635,53 @@ async def _handle_comment_added( manager = CollaborationManager() +# ── WebSocket endpoint ──────────────────────────────────────────────────────── + + @router.websocket("/ws/{session_id}") async def collaboration_websocket( websocket: 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, + ) + record_collab_error(session_id, "malformed_json") + 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, + ) + 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_comment_observability.py b/backend/tests/test_collaboration_comment_observability.py new file mode 100644 index 00000000..6c32b104 --- /dev/null +++ b/backend/tests/test_collaboration_comment_observability.py @@ -0,0 +1,303 @@ +""" +Tests for comment-specific observability in the collaboration WebSocket (#1577). + +Covers the new metrics added for comments: +- COLLAB_COMMENTS_TOTAL counter increments on each accepted comment +- COLLAB_COMMENT_COUNT gauge increments on accepted comment +- COLLAB_COMMENT_COUNT gauge resets to zero when the session is destroyed +- COLLAB_COMMENT_COUNT gauge is accurate for multiple sequential comments +- comment_sync requests are counted in COLLAB_MESSAGES_TOTAL +- error counters fire for all comment rejection paths +- METRICS_ENABLED=false skips all comment metric updates +- new metrics appear on the /metrics scrape endpoint + +Uses unique session_id prefixes (obs-cmt-*) so prometheus_client label sets +do not collide with those created in other test files. +""" + +from app import main as app_main +from app.observability import ( + COLLAB_COMMENT_COUNT, + COLLAB_COMMENTS_TOTAL, + COLLAB_ERRORS_TOTAL, + COLLAB_MESSAGES_TOTAL, +) +from app.routers.collaboration import MAX_COMMENT_CHARS, manager +from fastapi.testclient import TestClient + +client = TestClient(app_main.app) + + +def setup_function(): + manager.reset() + + +# ── helpers ─────────────────────────────────────────────────────────────────── + +def _comment_total(session_id: str) -> float: + try: + return COLLAB_COMMENTS_TOTAL.labels(session_id=session_id)._value.get() + except KeyError: + return 0.0 + + +def _comment_count(session_id: str) -> float: + try: + return COLLAB_COMMENT_COUNT.labels(session_id=session_id)._value.get() + except KeyError: + return 0.0 + + +def _messages(session_id: str, message_type: str) -> float: + 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: + try: + return COLLAB_ERRORS_TOTAL.labels( + session_id=session_id, error_reason=error_reason + )._value.get() + except KeyError: + return 0.0 + + +# ── COLLAB_COMMENTS_TOTAL counter ───────────────────────────────────────────── + +def test_comment_total_increments_on_accepted_comment(): + session = "obs-cmt-total1" + 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": "Hello."}) + ws.receive_json() + + assert _comment_total(session) == 1.0 + + +def test_comment_total_does_not_increment_on_rejected_comment(): + session = "obs-cmt-total-rej" + 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 _comment_total(session) == 0.0 + + +def test_comment_total_increments_once_per_accepted_comment(): + session = "obs-cmt-total-seq" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + for i in range(5): + ws.send_json({"type": "comment_added", "line": i + 1, "text": f"C{i}"}) + ws.receive_json() + + assert _comment_total(session) == 5.0 + + +# ── COLLAB_COMMENT_COUNT gauge ──────────────────────────────────────────────── + +def test_comment_count_gauge_increments_on_accepted_comment(): + session = "obs-cmt-gauge1" + 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": "Gauge check."}) + ws.receive_json() + + assert _comment_count(session) == 1.0 + + # After session teardown the gauge must reach zero. + assert _comment_count(session) == 0.0 + + +def test_comment_count_gauge_is_accurate_across_multiple_adds(): + session = "obs-cmt-gauge-seq" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + for i in range(4): + ws.send_json({"type": "comment_added", "line": 1, "text": f"G{i}"}) + ws.receive_json() + assert _comment_count(session) == float(i + 1) + + assert _comment_count(session) == 0.0 + + +def test_comment_count_gauge_resets_to_zero_after_last_client_disconnects(): + """When the last client leaves, the gauge must be decremented to zero.""" + session = "obs-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": "Teardown."}) + ws.receive_json() + + assert session not in manager.rooms + assert _comment_count(session) == 0.0 + + +def test_comment_count_gauge_persists_while_session_is_alive(): + """Gauge must stay at the correct value while a second client is connected.""" + session = "obs-cmt-alive" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as alice: + alice.receive_json() + alice.receive_json() + + alice.send_json({"type": "comment_added", "line": 1, "text": "Alive."}) + 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 disconnects; Alice still connected. + alice.receive_json() # presence_update for Bob leaving + + # Room still alive — comment count must still be 1. + assert _comment_count(session) == 1.0 + + # Now Alice disconnects too — gauge drops to 0. + assert _comment_count(session) == 0.0 + + +def test_comment_count_gauge_not_decremented_on_non_last_disconnect(): + """Disconnecting a non-last client must not affect the comment count gauge.""" + session = "obs-cmt-nondec" + 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": "Still here."}) + alice.receive_json() + bob.receive_json() + + assert _comment_count(session) == 1.0 + + # Bob leaves — gauge must still be 1 (session not destroyed). + alice.receive_json() # presence_update + assert _comment_count(session) == 1.0 + + +# ── comment_sync message counter ───────────────────────────────────────────── + +def test_comment_sync_is_counted_in_messages_total(): + session = "obs-cmt-sync1" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_sync"}) + ws.receive_json() + + assert _messages(session, "comment_sync") == 1.0 + + +def test_comment_sync_count_accumulates_across_multiple_requests(): + session = "obs-cmt-sync-multi" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + for _ in range(3): + ws.send_json({"type": "comment_sync"}) + ws.receive_json() + + assert _messages(session, "comment_sync") == 3.0 + + +# ── error counters for comment rejection paths ──────────────────────────────── + +def test_invalid_comment_text_type_increments_error_counter(): + session = "obs-cmt-err-type" + 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": 99}) + ws.receive_json() + + assert _errors(session, "invalid_comment_text_type") == 1.0 + + +def test_empty_comment_text_increments_error_counter(): + session = "obs-cmt-err-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": " "}) + ws.receive_json() + + assert _errors(session, "empty_comment_text") == 1.0 + + +def test_comment_too_long_increments_error_counter(): + session = "obs-cmt-err-long" + 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)}) + ws.receive_json() + + assert _errors(session, "comment_too_long") == 1.0 + + +# ── METRICS_ENABLED=false guard ─────────────────────────────────────────────── + +def test_comment_metrics_skipped_when_metrics_disabled(monkeypatch): + """With METRICS_ENABLED=false no comment counter or gauge must be updated.""" + monkeypatch.setenv("METRICS_ENABLED", "false") + + session = "obs-cmt-disabled" + before_total = _comment_total(session) + before_gauge = _comment_count(session) + + 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": "Disabled."}) + ws.receive_json() + + assert _comment_total(session) == before_total + assert _comment_count(session) == before_gauge + + +# ── metric names on /metrics scrape endpoint ────────────────────────────────── + +def test_comment_metrics_appear_on_scrape_endpoint(): + """Both new comment metric families must be visible on /metrics.""" + session = "obs-cmt-scrape" + 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": "Scrape."}) + ws.receive_json() + + response = client.get("/metrics") + assert response.status_code == 200 + body = response.text + + assert "qyverixai_collaboration_comments_total" in body + assert "qyverixai_collaboration_comment_count" in body diff --git a/backend/tests/test_collaboration_comment_regression.py b/backend/tests/test_collaboration_comment_regression.py new file mode 100644 index 00000000..f19ed464 --- /dev/null +++ b/backend/tests/test_collaboration_comment_regression.py @@ -0,0 +1,389 @@ +""" +Regression tests for collaboration comments (#1575). + +These tests pin the specific observable contract of the comment feature so +that any future change that silently alters comment structure, ordering, +accumulation behaviour, or broadcast targeting will cause an immediate, +descriptive test failure. + +Each test is labelled with the regression scenario it guards. + +Scenarios NOT duplicated from existing test files: +- comment payload field completeness (id, createdAt, authorId, line, text, color, author) +- authorId matches the clientId issued by session_state +- comment text whitespace is stripped before storage +- null / missing text field returns a typed error +- comments list grows by exactly 1 per successful add +- cumulative comments list is correct after N sequential adds +- sender receives their own comment_added broadcast +- three-client scenario: comments from two authors visible to late-joining third client +- comment survives another client's disconnect +- comment order is preserved (FIFO) +- comments from the cumulative list in each broadcast match the full history +""" + +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() + + +# ── Regression: comment payload field completeness ─────────────────────────── + +def test_comment_payload_has_all_required_fields(): + """Every comment_added broadcast must include id, line, text, authorId, + author, color, and createdAt fields.""" + with client.websocket_connect("/collaboration/ws/reg-fields?name=Alice") as ws: + ws.receive_json() # session_state + ws.receive_json() # presence_update + + ws.send_json({"type": "comment_added", "line": 7, "text": "Field check."}) + msg = ws.receive_json() + + comment = msg["comment"] + assert "id" in comment, "comment must have an id field" + assert "line" in comment, "comment must have a line field" + assert "text" in comment, "comment must have a text field" + assert "authorId" in comment, "comment must have an authorId field" + assert "author" in comment, "comment must have an author field" + assert "color" in comment, "comment must have a color field" + assert "createdAt" in comment, "comment must have a createdAt field" + + +def test_comment_id_is_nonempty_string(): + """comment.id must be a non-empty string (hex).""" + with client.websocket_connect("/collaboration/ws/reg-id?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + ws.send_json({"type": "comment_added", "line": 1, "text": "ID check."}) + msg = ws.receive_json() + + assert isinstance(msg["comment"]["id"], str) + assert len(msg["comment"]["id"]) > 0 + + +def test_comment_created_at_is_iso8601_utc(): + """comment.createdAt must be a non-empty ISO-8601 string ending with '+00:00' or 'Z'.""" + from datetime import datetime, timezone + + with client.websocket_connect("/collaboration/ws/reg-ts?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + ws.send_json({"type": "comment_added", "line": 1, "text": "Timestamp check."}) + msg = ws.receive_json() + + created_at = msg["comment"]["createdAt"] + assert isinstance(created_at, str) and created_at, "createdAt must be a non-empty string" + + # Must be parseable as a timezone-aware datetime. + try: + dt = datetime.fromisoformat(created_at) + except ValueError: + raise AssertionError(f"createdAt is not a valid ISO-8601 string: {created_at!r}") + + assert dt.tzinfo is not None, "createdAt must include timezone info" + + +# ── Regression: authorId matches actual clientId ────────────────────────────── + +def test_comment_author_id_matches_session_client_id(): + """comment.authorId must equal the clientId that was issued in session_state.""" + with client.websocket_connect("/collaboration/ws/reg-authorid?name=Alice") as ws: + state = ws.receive_json() # session_state + ws.receive_json() # presence_update + my_client_id = state["clientId"] + + ws.send_json({"type": "comment_added", "line": 1, "text": "Who am I?"}) + msg = ws.receive_json() + + assert msg["comment"]["authorId"] == my_client_id + + +# ── Regression: text whitespace stripping ───────────────────────────────────── + +def test_comment_text_leading_trailing_whitespace_is_stripped(): + """The server must strip leading and trailing whitespace from comment text.""" + with client.websocket_connect("/collaboration/ws/reg-strip?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": " trimmed "}) + msg = ws.receive_json() + + assert msg["comment"]["text"] == "trimmed" + + +def test_comment_text_internal_whitespace_is_preserved(): + """Internal whitespace must NOT be stripped — only leading/trailing.""" + with client.websocket_connect("/collaboration/ws/reg-internal?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": " a b c "}) + msg = ws.receive_json() + + assert msg["comment"]["text"] == "a b c" + + +# ── Regression: null / missing text field ───────────────────────────────────── + +def test_null_text_returns_typed_error(): + """JSON null for text (Python None) must return an error with 'string' in detail.""" + with client.websocket_connect("/collaboration/ws/reg-null?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + # JSON null → Python None; isinstance(None, str) is False. + ws.send_json({"type": "comment_added", "line": 1, "text": None}) + msg = ws.receive_json() + + assert msg["type"] == "error" + assert "string" in msg["detail"].lower() + + +def test_missing_text_field_returns_typed_error(): + """Omitting the text field entirely must return the same error as null text.""" + with client.websocket_connect("/collaboration/ws/reg-notext?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1}) + msg = ws.receive_json() + + assert msg["type"] == "error" + assert "string" in msg["detail"].lower() + + +# ── Regression: comments list grows by exactly 1 per add ───────────────────── + +def test_comments_list_grows_by_one_per_successful_add(): + """Each accepted comment must append exactly one entry to the cumulative list.""" + session = "reg-grow" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + for i in range(1, 6): + ws.send_json({"type": "comment_added", "line": i, "text": f"Comment {i}"}) + msg = ws.receive_json() + assert len(msg["comments"]) == i, ( + f"After {i} adds, comments list must have length {i}, got {len(msg['comments'])}" + ) + + +# ── Regression: cumulative comments list is correct ────────────────────────── + +def test_cumulative_comments_list_content_is_correct(): + """The comments list in each broadcast must contain all prior comments in order.""" + session = "reg-cumulative" + texts = ["Alpha", "Beta", "Gamma"] + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + for text in texts: + ws.send_json({"type": "comment_added", "line": 1, "text": text}) + msg = ws.receive_json() + + # Final message must include all three comments in insertion order. + assert [c["text"] for c in msg["comments"]] == texts + + +# ── Regression: FIFO comment ordering ──────────────────────────────────────── + +def test_comments_are_ordered_fifo(): + """Comments must appear in the order they were accepted by the server.""" + session = "reg-fifo" + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + for i in range(5): + ws.send_json({"type": "comment_added", "line": i + 1, "text": f"Step {i}"}) + ws.receive_json() + + room = manager.rooms.get(session) + assert room is not None + texts = [c["text"] for c in room.comments] + assert texts == [f"Step {i}" for i in range(5)] + + +# ── Regression: sender receives own comment_added broadcast ────────────────── + +def test_sender_receives_own_comment_broadcast(): + """The client that posts a comment must also receive the comment_added broadcast + (broadcast does not exclude the sender for comments).""" + with client.websocket_connect("/collaboration/ws/reg-selfrecv?name=Alice") as ws: + state = ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 1, "text": "Self receive."}) + msg = ws.receive_json() + + assert msg["type"] == "comment_added" + assert msg["comment"]["authorId"] == state["clientId"] + + +# ── Regression: three-client scenario ──────────────────────────────────────── + +def test_comments_from_two_authors_visible_to_late_joining_third_client(): + """Comments posted by client A and client B must both appear in the + session_state received by a third client who joins after both posts.""" + session = "reg-three" + + 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": "From Alice."}) + alice.receive_json() + bob.receive_json() + + bob.send_json({"type": "comment_added", "line": 2, "text": "From Bob."}) + alice.receive_json() + bob.receive_json() + + # Charlie joins last. + with client.websocket_connect(f"/collaboration/ws/{session}?name=Charlie") as charlie: + charlie_state = charlie.receive_json() + alice.receive_json() + bob.receive_json() + charlie.receive_json() + + comment_texts = [c["text"] for c in charlie_state["comments"]] + assert "From Alice." in comment_texts + assert "From Bob." in comment_texts + assert charlie_state["reconnected"] is True + + +def test_comment_authors_in_three_client_scenario_are_correct(): + """Each comment in Charlie's session_state must correctly attribute its author.""" + session = "reg-three-authors" + + 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": "Alice says hi."}) + alice.receive_json() + bob.receive_json() + + bob.send_json({"type": "comment_added", "line": 3, "text": "Bob replies."}) + alice.receive_json() + bob.receive_json() + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Charlie") as charlie: + charlie_state = charlie.receive_json() + alice.receive_json() + bob.receive_json() + charlie.receive_json() + + by_text = {c["text"]: c["author"] for c in charlie_state["comments"]} + assert by_text["Alice says hi."] == "Alice" + assert by_text["Bob replies."] == "Bob" + + +# ── Regression: comment survives another client's disconnect ────────────────── + +def test_comment_persists_after_other_client_disconnects(): + """A comment added before a second client disconnects must still be in the + room's comment list after that client leaves.""" + session = "reg-persist-disc" + + 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": "Outlive Bob."}) + alice.receive_json() + bob.receive_json() + + # Bob disconnects; Alice consumes the presence_update. + alice.receive_json() + + # Comment must still be in the room. + room = manager.rooms.get(session) + assert room is not None + assert any(c["text"] == "Outlive Bob." for c in room.comments) + + +# ── Regression: comment_added.comments always includes new comment ──────────── + +def test_comment_added_comments_list_includes_the_new_comment(): + """The cumulative comments list in a comment_added payload must include + the comment that was just added.""" + with client.websocket_connect("/collaboration/ws/reg-includes?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 4, "text": "Included."}) + msg = ws.receive_json() + + ids = [c["id"] for c in msg["comments"]] + assert msg["comment"]["id"] in ids + + +# ── Regression: comment line is stored correctly ────────────────────────────── + +def test_comment_line_matches_sent_value(): + """The line number sent in the request must match the line stored in the comment.""" + with client.websocket_connect("/collaboration/ws/reg-line?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + ws.send_json({"type": "comment_added", "line": 42, "text": "Line 42."}) + msg = ws.receive_json() + + assert msg["comment"]["line"] == 42 + + +# ── Regression: comment_added payload structure under multi-comment load ────── + +def test_comments_list_in_broadcast_is_consistent_with_room_state(): + """The comments list inside each comment_added broadcast must match the + room's internal comment list at the time of the broadcast.""" + session = "reg-consistent" + received_snapshots = [] + + with client.websocket_connect(f"/collaboration/ws/{session}?name=Alice") as ws: + ws.receive_json() + ws.receive_json() + + for i in range(4): + ws.send_json({"type": "comment_added", "line": i + 1, "text": f"T{i}"}) + msg = ws.receive_json() + received_snapshots.append(list(msg["comments"])) + + # Each snapshot must be a prefix-extension of the previous one. + for idx in range(1, len(received_snapshots)): + prev = received_snapshots[idx - 1] + curr = received_snapshots[idx] + assert len(curr) == len(prev) + 1, ( + f"comments list should grow by 1 at step {idx}" + ) + # All previous comments must still be present in the same order. + for j, prev_cmt in enumerate(prev): + assert curr[j]["id"] == prev_cmt["id"], ( + f"Comment at index {j} changed between broadcast {idx-1} and {idx}" + ) 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"] == [] 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 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 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 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 diff --git a/frontend/index.html b/frontend/index.html index 9a18c400..157f1c1d 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 ═══════════════════════════════════════════════════════════════ */ @@ -2254,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; @@ -2583,7 +2680,7 @@ -
+
Open Source · AI-Powered · Free @@ -2640,15 +2738,15 @@

Debug. Understand.
Ship faster.

suggestions — no account needed.

- - - - - - + + + +
@@ -2803,6 +2901,9 @@

Debug. Understand.
Ship faster.

/Focus editor Ctrl/⌘ + EnterAnalyze EscLeave editor + Ctrl/⌘ + KClear editor + Ctrl/⌘ + Shift + TToggle theme + Alt + 1–4Switch mode @@ -2956,7 +3057,15 @@

Debug. Understand.
Ship faster.

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

No history yet

+

Run your first analysis to see it here

+
@@ -2970,7 +3079,15 @@

Debug. Understand.
Ship faster.

-
No favorites saved yet.
+
+ +

No favorites saved

+

Star an analysis result to save it here

+
@@ -3073,6 +3190,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", @@ -3081,6 +3200,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", @@ -3181,6 +3302,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: "தர மதிப்பெண்", @@ -3189,6 +3312,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: "திறந்த மூல சமூகத்திற்காக உருவாக்கப்பட்டது", @@ -3289,6 +3414,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: "गुणवत्ता स्कोर", @@ -3297,6 +3424,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: "ओपन सोर्स समुदाय के लिए निर्मित", @@ -3397,6 +3526,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é", @@ -3405,6 +3536,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", @@ -4008,6 +4141,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() { @@ -4239,9 +4418,7 @@

Project Health Score

} function renderExplain(exp) { - document.getElementById('emptyExplain').style.display = 'none'; - const el = document.getElementById('explainResult'); - el.style.display = 'block'; + const el = showPane('explainResult', 'emptyExplain'); document.getElementById('explainBadge').textContent = exp.complexity; el.innerHTML = ` @@ -4314,9 +4491,7 @@

Project Health Score

} function renderDebug(dbg) { - document.getElementById('emptyDebug').style.display = 'none'; - const el = document.getElementById('debugResult'); - el.style.display = 'block'; + const el = showPane('debugResult', 'emptyDebug'); const badge = dbg.error_count > 0 ? dbg.error_count : dbg.issues.length; document.getElementById('debugBadge').textContent = badge || '0'; @@ -4540,9 +4715,7 @@

Project Health Score

RENDER SUGGESTIONS (with diff view) ═══════════════════════════════════════════════════════════════ */ function renderSuggest(sugg) { - document.getElementById('emptySuggest').style.display = 'none'; - const el = document.getElementById('suggestResult'); - el.style.display = 'block'; + const el = showPane('suggestResult', 'emptySuggest'); document.getElementById('suggestBadge').textContent = sugg.suggestions.length || '0'; const scoreColor = sugg.overall_score >= 80 ? 'var(--green)' @@ -4942,7 +5115,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; } @@ -5208,7 +5390,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 => { @@ -5458,6 +5649,65 @@

Project Health Score

.replace(/`(.+?)`/g, '$1'); } + /** + * Show a result pane and hide its matching empty-state placeholder. + * + * @param {string} resultId - ID of the result content div (e.g. 'explainResult'). + * @param {string} emptyId - ID of the empty-state div (e.g. 'emptyExplain'). + * @param {string} [display] - CSS display value for the result div (default 'block'). + * @returns {HTMLElement} The result element, ready for innerHTML assignment. + */ + function showPane(resultId, emptyId, display = 'block') { + document.getElementById(emptyId).style.display = 'none'; + const el = document.getElementById(resultId); + el.style.display = display; + return el; + } + + /** + * Set a CSS `display` value on a list of elements identified by ID. + * + * @param {string[]} ids - Array of element IDs. + * @param {string} display - The display value to apply. + */ + function setDisplayAll(ids, display) { + ids.forEach(id => { + const el = document.getElementById(id); + if (el) el.style.display = display; + }); + } + + /** + * Set a property (e.g. `textContent`) to a value on a list of elements by ID. + * + * @param {string[]} ids - Array of element IDs. + * @param {string} prop - Property name to set. + * @param {*} value - Value to assign. + */ + function setPropAll(ids, prop, value) { + ids.forEach(id => { + const el = document.getElementById(id); + if (el) el[prop] = value; + }); + } + + /** + * Deactivate all buttons/tabs matching `selector`, then activate `activeEl`. + * Handles both `aria-pressed` (buttons) and `aria-selected` (tabs). + * + * @param {string} selector - CSS selector for the full group. + * @param {HTMLElement} activeEl - The element to mark as active. + * @param {string} ariaAttr - ARIA attribute to toggle ('aria-pressed' | 'aria-selected'). + */ + function activateExclusive(selector, activeEl, ariaAttr) { + document.querySelectorAll(selector).forEach(el => { + el.classList.remove('active'); + el.setAttribute(ariaAttr, 'false'); + }); + activeEl.classList.add('active'); + activeEl.setAttribute(ariaAttr, 'true'); + } + /* ═══════════════════════════════════════════════════════════════ SAMPLE CODE LOADER ═══════════════════════════════════════════════════════════════ */ diff --git a/frontend/package.json b/frontend/package.json index 0bba672c..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", + "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/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.'); 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.'); 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', +);