fix(chunk-context): address PR #767 review — extract bbox helper, fix page_number overwrite

Resolves both 🟡 important issues from the latest review:

1. `page_number` was unconditionally overwritten in `viz_routes.py:696` even
   when Qdrant's payload lacked the field, clobbering the value resolved
   from `chunk_context.page_number`. The new helper returns each field
   independently and both call sites only overwrite via `is not None`
   guards, matching the existing logic in `visualization.py`.

2. The ~60-line `if chunk_index is not None: ... else: ...` Qdrant scroll
   block was duplicated between `api/visualization.py` and
   `auth/viz_routes.py`. Extracted into `get_chunk_bbox_and_page_from_qdrant`
   in `search/context.py` alongside the existing private `_get_chunk_*_from_qdrant`
   helpers; both routes now share ~12 lines of caller code.

New unit tests at `tests/unit/test_chunk_bbox_helper.py` cover the indexed
and offset paths, the `(bbox, None)` regression case, and graceful
degradation on Qdrant strict-mode 400 (which also closes nit #4).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

nextcloud_mcp_server/api/visualization.py

@@ -14,7 +14,6 @@ import logging
 from typing import Any
 
 import pymupdf
-from qdrant_client.models import FieldCondition, Filter, MatchValue
 from starlette.requests import Request
 from starlette.responses import JSONResponse
 
@@ -31,13 +30,14 @@ from nextcloud_mcp_server.search import (
     BM25HybridSearchAlgorithm,
     SemanticSearchAlgorithm,
 )
-from nextcloud_mcp_server.search.context import get_chunk_with_context
+from nextcloud_mcp_server.search.context import (
+    get_chunk_bbox_and_page_from_qdrant,
+    get_chunk_with_context,
+)
 from nextcloud_mcp_server.vector.oauth_sync import (
     NotProvisionedError,
     get_user_client_basic_auth,
 )
-from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter
-from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client
 from nextcloud_mcp_server.vector.visualization import compute_pca_coordinates
 
 logger = logging.getLogger(__name__)
@@ -567,82 +567,23 @@ async def get_chunk_context(request: Request) -> JSONResponse:
 
         # For PDF files, also fetch the chunk's bounding box from Qdrant if
         # available so the client can overlay a highlight on top of a
-        # render-on-demand page image (Deck #76).
+        # render-on-demand page image (Deck #76). Qdrant's page_number is
+        # trusted over the context-expansion fallback when present.
         chunk_bbox = None
         page_number = chunk_context.page_number
 
         if doc_type == "file":
-            try:
+            qdrant_bbox, qdrant_page = await get_chunk_bbox_and_page_from_qdrant(
-                settings = get_settings()
+                user_id=user_id,
-                qdrant_client = await get_qdrant_client()
+                doc_id=doc_id_val,
-
+                chunk_index=chunk_index,
-                # Prefer chunk_index for the chunk-bbox lookup (always indexed);
+                chunk_start=start,
-                # fall back to (chunk_start_offset, chunk_end_offset) when not provided.
+                chunk_end=end,
-                if chunk_index is not None:
+            )
-                    points_response = await qdrant_client.scroll(
+            if qdrant_bbox is not None:
-                        collection_name=settings.get_collection_name(),
+                chunk_bbox = qdrant_bbox
-                        scroll_filter=Filter(
+            if qdrant_page is not None:
-                            must=[
+                page_number = qdrant_page
-                                get_placeholder_filter(),
-                                FieldCondition(
-                                    key="doc_id", match=MatchValue(value=doc_id_val)
-                                ),
-                                FieldCondition(
-                                    key="user_id", match=MatchValue(value=user_id)
-                                ),
-                                FieldCondition(
-                                    key="chunk_index",
-                                    match=MatchValue(value=chunk_index),
-                                ),
-                            ]
-                        ),
-                        limit=1,
-                        with_vectors=False,
-                        with_payload=["chunk_bbox", "page_number"],
-                    )
-                else:
-                    # Legacy fallback for clients that don't send chunk_index
-                    # (pre-cbcoutinho/astrolabe#75). chunk_start/end_offset
-                    # aren't indexed in Qdrant Cloud strict mode, so this
-                    # call may fail with HTTP 400 there; the outer except
-                    # logs a warning and the response degrades gracefully
-                    # (no chunk_bbox).
-                    points_response = await qdrant_client.scroll(
-                        collection_name=settings.get_collection_name(),
-                        scroll_filter=Filter(
-                            must=[
-                                get_placeholder_filter(),
-                                FieldCondition(
-                                    key="doc_id", match=MatchValue(value=doc_id_val)
-                                ),
-                                FieldCondition(
-                                    key="user_id", match=MatchValue(value=user_id)
-                                ),
-                                FieldCondition(
-                                    key="chunk_start_offset",
-                                    match=MatchValue(value=start),
-                                ),
-                                FieldCondition(
-                                    key="chunk_end_offset",
-                                    match=MatchValue(value=end),
-                                ),
-                            ]
-                        ),
-                        limit=1,
-                        with_vectors=False,
-                        with_payload=["chunk_bbox", "page_number"],
-                    )
-
-                if points_response[0]:
-                    payload = points_response[0][0].payload
-                    if payload:
-                        chunk_bbox = payload.get("chunk_bbox")
-                        # Trust Qdrant page number if available (might be more accurate than context expansion logic)
-                        if payload.get("page_number") is not None:
-                            page_number = payload.get("page_number")
-
-            except Exception as e:
-                logger.warning(f"Failed to fetch chunk bbox: {e}")
 
         # Build response
         response_data = {

nextcloud_mcp_server/auth/viz_routes.py

@@ -18,7 +18,6 @@ from pathlib import Path
 import anyio
 import numpy as np
 from jinja2 import Environment, FileSystemLoader
-from qdrant_client.models import FieldCondition, Filter, MatchValue
 from starlette.authentication import requires
 from starlette.requests import Request
 from starlette.responses import HTMLResponse, JSONResponse
@@ -34,13 +33,15 @@ from nextcloud_mcp_server.search import (
     BM25HybridSearchAlgorithm,
     SemanticSearchAlgorithm,
 )
-from nextcloud_mcp_server.search.context import get_chunk_with_context
+from nextcloud_mcp_server.search.context import (
+    get_chunk_bbox_and_page_from_qdrant,
+    get_chunk_with_context,
+)
 from nextcloud_mcp_server.vector.oauth_sync import (
     NotProvisionedError,
     get_user_client_basic_auth,
 )
 from nextcloud_mcp_server.vector.pca import PCA
-from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter
 from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client
 
 logger = logging.getLogger(__name__)
@@ -625,82 +626,22 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse:
 
         # For PDF files, also fetch the chunk bbox from Qdrant so the client
         # can overlay a highlight on top of a render-on-demand page image
-        # (Deck #76).
+        # (Deck #76). Qdrant's page_number is trusted over the
+        # context-expansion fallback when present.
         chunk_bbox = None
         page_number = chunk_context.page_number
         if doc_type == "file":
-            try:
+            qdrant_bbox, qdrant_page = await get_chunk_bbox_and_page_from_qdrant(
-                settings = get_settings()
+                user_id=user_id,
-                qdrant_client = await get_qdrant_client()
+                doc_id=doc_id_int,
-
+                chunk_index=chunk_index,
-                # Prefer chunk_index for the chunk-bbox lookup (always indexed);
+                chunk_start=start,
-                # fall back to (chunk_start_offset, chunk_end_offset) when not provided.
+                chunk_end=end,
-                if chunk_index is not None:
+            )
-                    points_response = await qdrant_client.scroll(
+            if qdrant_bbox is not None:
-                        collection_name=settings.get_collection_name(),
+                chunk_bbox = qdrant_bbox
-                        scroll_filter=Filter(
+            if qdrant_page is not None:
-                            must=[
+                page_number = qdrant_page
-                                get_placeholder_filter(),
-                                FieldCondition(
-                                    key="doc_id", match=MatchValue(value=doc_id_int)
-                                ),
-                                FieldCondition(
-                                    key="user_id", match=MatchValue(value=user_id)
-                                ),
-                                FieldCondition(
-                                    key="chunk_index",
-                                    match=MatchValue(value=chunk_index),
-                                ),
-                            ]
-                        ),
-                        limit=1,
-                        with_vectors=False,
-                        with_payload=["chunk_bbox", "page_number"],
-                    )
-                else:
-                    # Legacy fallback for clients that don't send chunk_index
-                    # (pre-cbcoutinho/astrolabe#75). chunk_start/end_offset
-                    # aren't indexed in Qdrant Cloud strict mode, so this
-                    # call may fail with HTTP 400 there; the outer except
-                    # logs a warning and the response degrades gracefully
-                    # (no chunk_bbox).
-                    points_response = await qdrant_client.scroll(
-                        collection_name=settings.get_collection_name(),
-                        scroll_filter=Filter(
-                            must=[
-                                get_placeholder_filter(),
-                                FieldCondition(
-                                    key="doc_id", match=MatchValue(value=doc_id_int)
-                                ),
-                                FieldCondition(
-                                    key="user_id", match=MatchValue(value=user_id)
-                                ),
-                                FieldCondition(
-                                    key="chunk_start_offset",
-                                    match=MatchValue(value=start),
-                                ),
-                                FieldCondition(
-                                    key="chunk_end_offset",
-                                    match=MatchValue(value=end),
-                                ),
-                            ]
-                        ),
-                        limit=1,
-                        with_vectors=False,
-                        with_payload=["chunk_bbox", "page_number"],
-                    )
-
-                points = points_response[0]
-                if points and points[0].payload:
-                    chunk_bbox = points[0].payload.get("chunk_bbox")
-                    page_number = points[0].payload.get("page_number")
-                    if chunk_bbox:
-                        logger.info(
-                            f"Found chunk bbox: page={page_number}, "
-                            f"rects={len(chunk_bbox)}"
-                        )
-            except Exception as e:
-                logger.warning(f"Failed to fetch chunk bbox: {e}")
 
         # Return response compatible with frontend expectations
         response_data: dict = {

nextcloud_mcp_server/search/context.py

@@ -12,6 +12,7 @@ from qdrant_client.models import FieldCondition, Filter, MatchValue
 from nextcloud_mcp_server.client import NextcloudClient
 from nextcloud_mcp_server.config import get_settings
 from nextcloud_mcp_server.vector.html_processor import html_to_markdown
+from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter
 from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client
 
 logger = logging.getLogger(__name__)
@@ -195,6 +196,98 @@ async def _get_deck_metadata_from_qdrant(
         return None
 
 
+async def get_chunk_bbox_and_page_from_qdrant(
+    user_id: str,
+    doc_id: int | str,
+    chunk_index: int | None,
+    chunk_start: int,
+    chunk_end: int,
+) -> tuple[list | None, int | None]:
+    """Fetch chunk_bbox and page_number for a chunk from Qdrant payload.
+
+    Prefers chunk_index for the lookup (always indexed); falls back to
+    (chunk_start_offset, chunk_end_offset) when chunk_index is not provided
+    — this is the legacy path for clients pre-cbcoutinho/astrolabe#75. The
+    fallback may 400 in Qdrant Cloud strict mode because those offset fields
+    aren't indexed there; that's logged as a warning and (None, None) is
+    returned so callers degrade gracefully.
+
+    Args:
+        user_id: User ID who owns the document
+        doc_id: Document ID (int for file/note, str for some doc types)
+        chunk_index: Zero-based chunk index, or None to use offset fallback
+        chunk_start: Character offset where chunk starts (used when
+            chunk_index is None)
+        chunk_end: Character offset where chunk ends (used when chunk_index
+            is None)
+
+    Returns:
+        Tuple of (chunk_bbox, page_number); either field may be None
+        independently if absent from the payload, or both may be None on
+        miss/error.
+    """
+    try:
+        settings = get_settings()
+        qdrant_client = await get_qdrant_client()
+
+        if chunk_index is not None:
+            points_response = await qdrant_client.scroll(
+                collection_name=settings.get_collection_name(),
+                scroll_filter=Filter(
+                    must=[
+                        get_placeholder_filter(),
+                        FieldCondition(key="doc_id", match=MatchValue(value=doc_id)),
+                        FieldCondition(key="user_id", match=MatchValue(value=user_id)),
+                        FieldCondition(
+                            key="chunk_index", match=MatchValue(value=chunk_index)
+                        ),
+                    ]
+                ),
+                limit=1,
+                with_vectors=False,
+                with_payload=["chunk_bbox", "page_number"],
+            )
+        else:
+            points_response = await qdrant_client.scroll(
+                collection_name=settings.get_collection_name(),
+                scroll_filter=Filter(
+                    must=[
+                        get_placeholder_filter(),
+                        FieldCondition(key="doc_id", match=MatchValue(value=doc_id)),
+                        FieldCondition(key="user_id", match=MatchValue(value=user_id)),
+                        FieldCondition(
+                            key="chunk_start_offset",
+                            match=MatchValue(value=chunk_start),
+                        ),
+                        FieldCondition(
+                            key="chunk_end_offset",
+                            match=MatchValue(value=chunk_end),
+                        ),
+                    ]
+                ),
+                limit=1,
+                with_vectors=False,
+                with_payload=["chunk_bbox", "page_number"],
+            )
+
+        points = points_response[0]
+        if not points or not points[0].payload:
+            return None, None
+
+        payload = points[0].payload
+        chunk_bbox = payload.get("chunk_bbox")
+        page_number = payload.get("page_number")
+        if chunk_bbox:
+            logger.info(
+                "Found chunk bbox: page=%s, rects=%d", page_number, len(chunk_bbox)
+            )
+        return chunk_bbox, page_number
+
+    except Exception as e:
+        logger.warning("Failed to fetch chunk bbox: %s", e)
+        return None, None
+
+
 @dataclass
 class ChunkContext:
     """Expanded chunk with surrounding context and position markers.

tests/unit/test_chunk_bbox_helper.py

@@ -0,0 +1,215 @@
+"""Unit tests for
+`nextcloud_mcp_server.search.context.get_chunk_bbox_and_page_from_qdrant`.
+
+Covers the two paths the helper handles:
+- Indexed lookup via `chunk_index` (the preferred path post
+  cbcoutinho/astrolabe#75)
+- Legacy offset fallback via `(chunk_start_offset, chunk_end_offset)`, which
+  may 400 in Qdrant Cloud strict mode
+
+Plus the regression case from PR #767 review: when the payload has
+`chunk_bbox` but no `page_number`, the helper must surface that as
+`(bbox, None)` so callers can preserve their context-derived page_number
+fallback.
+"""
+
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import pytest
+
+# Import via the auth surface first to side-step the known
+# `nextcloud_mcp_server.search.__init__` circular-init issue (same workaround
+# used in test_chunk_context_offset_gate.py).
+import nextcloud_mcp_server.auth.viz_routes  # noqa: F401
+from nextcloud_mcp_server.search import context as context_module
+from nextcloud_mcp_server.search.context import get_chunk_bbox_and_page_from_qdrant
+
+pytestmark = pytest.mark.unit
+
+
+def _make_point(payload: dict) -> MagicMock:
+    point = MagicMock()
+    point.payload = payload
+    return point
+
+
+def _patch_qdrant(scroll_return=None, scroll_side_effect=None):
+    qdrant_client = MagicMock()
+    if scroll_side_effect is not None:
+        qdrant_client.scroll = AsyncMock(side_effect=scroll_side_effect)
+    else:
+        qdrant_client.scroll = AsyncMock(return_value=scroll_return)
+    return patch.object(
+        context_module,
+        "get_qdrant_client",
+        new_callable=AsyncMock,
+        return_value=qdrant_client,
+    ), qdrant_client
+
+
+class TestIndexedPath:
+    """When `chunk_index` is supplied, the helper must use the indexed
+    `chunk_index` filter (not the offset fallback)."""
+
+    async def test_returns_bbox_and_page_when_payload_complete(self):
+        bbox = [[0, 0, 100, 50]]
+        point = _make_point({"chunk_bbox": bbox, "page_number": 7})
+        ctx, qdrant_client = _patch_qdrant(scroll_return=([point], None))
+        with ctx:
+            result = await get_chunk_bbox_and_page_from_qdrant(
+                user_id="alice",
+                doc_id=42,
+                chunk_index=3,
+                chunk_start=0,
+                chunk_end=100,
+            )
+
+        assert result == (bbox, 7)
+        # One scroll call, and the filter must include chunk_index (not offsets)
+        qdrant_client.scroll.assert_awaited_once()
+        scroll_kwargs = qdrant_client.scroll.await_args.kwargs
+        filter_keys = [c.key for c in scroll_kwargs["scroll_filter"].must]
+        assert "chunk_index" in filter_keys
+        assert "chunk_start_offset" not in filter_keys
+        assert "chunk_end_offset" not in filter_keys
+
+
+class TestOffsetFallbackPath:
+    """When `chunk_index` is None, the helper must use the offset filter."""
+
+    async def test_returns_bbox_and_page_when_payload_complete(self):
+        bbox = [[10, 20, 110, 70]]
+        point = _make_point({"chunk_bbox": bbox, "page_number": 2})
+        ctx, qdrant_client = _patch_qdrant(scroll_return=([point], None))
+        with ctx:
+            result = await get_chunk_bbox_and_page_from_qdrant(
+                user_id="bob",
+                doc_id=99,
+                chunk_index=None,
+                chunk_start=500,
+                chunk_end=600,
+            )
+
+        assert result == (bbox, 2)
+        scroll_kwargs = qdrant_client.scroll.await_args.kwargs
+        filter_keys = [c.key for c in scroll_kwargs["scroll_filter"].must]
+        assert "chunk_start_offset" in filter_keys
+        assert "chunk_end_offset" in filter_keys
+        assert "chunk_index" not in filter_keys
+
+    async def test_strict_mode_400_returns_none_pair_and_warns(self, caplog):
+        """Qdrant Cloud strict mode 400s on unindexed offset filters; the
+        helper must swallow the exception, log a warning, and degrade
+        gracefully so the route can still return chunk text."""
+        ctx, _ = _patch_qdrant(scroll_side_effect=Exception("strict mode: 400"))
+        with ctx, caplog.at_level("WARNING"):
+            result = await get_chunk_bbox_and_page_from_qdrant(
+                user_id="bob",
+                doc_id=99,
+                chunk_index=None,
+                chunk_start=0,
+                chunk_end=100,
+            )
+
+        assert result == (None, None)
+        assert any("Failed to fetch chunk bbox" in r.message for r in caplog.records)
+
+
+class TestPayloadShape:
+    """Each payload field can be missing independently — callers rely on
+    that to decide whether to overwrite their fallback values."""
+
+    async def test_empty_points_returns_none_pair(self):
+        ctx, _ = _patch_qdrant(scroll_return=([], None))
+        with ctx:
+            result = await get_chunk_bbox_and_page_from_qdrant(
+                user_id="alice",
+                doc_id=1,
+                chunk_index=0,
+                chunk_start=0,
+                chunk_end=10,
+            )
+
+        assert result == (None, None)
+
+    async def test_missing_page_returns_bbox_only(self):
+        """Regression for PR #767 review issue #1: when Qdrant returns a
+        point whose payload lacks `page_number`, the helper must return
+        `(bbox, None)` so callers preserve their `chunk_context.page_number`
+        fallback rather than clobbering it to None."""
+        bbox = [[0, 0, 100, 50]]
+        point = _make_point({"chunk_bbox": bbox})  # no page_number
+        ctx, _ = _patch_qdrant(scroll_return=([point], None))
+        with ctx:
+            result = await get_chunk_bbox_and_page_from_qdrant(
+                user_id="alice",
+                doc_id=42,
+                chunk_index=3,
+                chunk_start=0,
+                chunk_end=100,
+            )
+
+        assert result == (bbox, None)
+
+    async def test_missing_bbox_returns_page_only(self):
+        point = _make_point({"page_number": 5})  # no chunk_bbox
+        ctx, _ = _patch_qdrant(scroll_return=([point], None))
+        with ctx:
+            result = await get_chunk_bbox_and_page_from_qdrant(
+                user_id="alice",
+                doc_id=42,
+                chunk_index=3,
+                chunk_start=0,
+                chunk_end=100,
+            )
+
+        assert result == (None, 5)
+
+    async def test_empty_payload_returns_none_pair(self):
+        point = _make_point({})
+        ctx, _ = _patch_qdrant(scroll_return=([point], None))
+        with ctx:
+            result = await get_chunk_bbox_and_page_from_qdrant(
+                user_id="alice",
+                doc_id=42,
+                chunk_index=3,
+                chunk_start=0,
+                chunk_end=100,
+            )
+
+        assert result == (None, None)
+
+    async def test_falsy_payload_treated_as_no_point(self):
+        """`if not points[0].payload` short-circuits when payload is None or
+        an empty dict, mirroring the original guards in the route handlers."""
+        point = MagicMock()
+        point.payload = None
+        ctx, _ = _patch_qdrant(scroll_return=([point], None))
+        with ctx:
+            result = await get_chunk_bbox_and_page_from_qdrant(
+                user_id="alice",
+                doc_id=42,
+                chunk_index=3,
+                chunk_start=0,
+                chunk_end=100,
+            )
+
+        assert result == (None, None)
+
+
+class TestExceptionHandling:
+    """Any error from Qdrant must produce `(None, None)` — never propagate."""
+
+    async def test_indexed_path_exception_returns_none_pair(self, caplog):
+        ctx, _ = _patch_qdrant(scroll_side_effect=RuntimeError("qdrant unavailable"))
+        with ctx, caplog.at_level("WARNING"):
+            result = await get_chunk_bbox_and_page_from_qdrant(
+                user_id="alice",
+                doc_id=42,
+                chunk_index=3,
+                chunk_start=0,
+                chunk_end=100,
+            )
+
+        assert result == (None, None)
+        assert any("Failed to fetch chunk bbox" in r.message for r in caplog.records)