Browse Source

update specs for fix off-screen indicator

Yuki Takei 1 tuần trước cách đây
mục cha
commit
61e535d7bb

+ 5 - 5
.kiro/specs/collaborative-editor-awareness/design.md

@@ -74,7 +74,7 @@ graph TB
 - `yRichCursors` is added as a separate extension alongside `yCollab`'s output; it owns all awareness-cursor interaction, including in-viewport widget rendering and off-screen indicators
 - `yRichCursors` is added as a separate extension alongside `yCollab`'s output; it owns all awareness-cursor interaction, including in-viewport widget rendering and off-screen indicators
 - `state.editors` remains the single source of truth for user identity data
 - `state.editors` remains the single source of truth for user identity data
 - `state.cursor` (anchor/head relative positions) continues to be used for cursor position broadcasting, consistent with `y-codemirror.next` convention
 - `state.cursor` (anchor/head relative positions) continues to be used for cursor position broadcasting, consistent with `y-codemirror.next` convention
-- Off-screen indicators are managed within the same `yRichCursors` ViewPlugin — it compares each remote cursor's absolute position against `view.viewport` to decide between widget decoration (in-view) and DOM overlay (off-screen)
+- Off-screen indicators are managed within the same `yRichCursors` ViewPlugin — it compares each remote cursor's absolute position against `view.visibleRanges` (the actually visible content range, excluding CodeMirror's pre-render buffer) to decide between widget decoration (in-view) and DOM overlay (off-screen)
 
 
 ### Technology Stack
 ### Technology Stack
 
 
@@ -145,10 +145,10 @@ sequenceDiagram
 | 3.10 | Full opacity during active editing (3s) | `yRichCursors` | `lastActivityMap` + `.cm-yRichCursorActive` class + `setTimeout` |
 | 3.10 | Full opacity during active editing (3s) | `yRichCursors` | `lastActivityMap` + `.cm-yRichCursorActive` class + `setTimeout` |
 | 4.1 | Off-screen indicator pinned to top edge (↑) | `yRichCursors` | `offScreenContainer` top overlay |
 | 4.1 | Off-screen indicator pinned to top edge (↑) | `yRichCursors` | `offScreenContainer` top overlay |
 | 4.2 | Off-screen indicator pinned to bottom edge (↓) | `yRichCursors` | `offScreenContainer` bottom overlay |
 | 4.2 | Off-screen indicator pinned to bottom edge (↓) | `yRichCursors` | `offScreenContainer` bottom overlay |
-| 4.3 | No indicator when cursor is in viewport | `yRichCursors` | viewport comparison in `update()` |
+| 4.3 | No indicator when cursor is in viewport | `yRichCursors` | `visibleRanges` comparison in `update()` |
 | 4.4 | Same avatar/color as in-editor widget | `yRichCursors` | shared `state.editors` data |
 | 4.4 | Same avatar/color as in-editor widget | `yRichCursors` | shared `state.editors` data |
 | 4.5 | Multiple indicators side by side | `yRichCursors` | horizontal flex layout |
 | 4.5 | Multiple indicators side by side | `yRichCursors` | horizontal flex layout |
-| 4.6 | Transition on scroll (indicator ↔ widget) | `yRichCursors` | `viewportChanged` check in `update()` |
+| 4.6 | Transition on scroll (indicator ↔ widget) | `yRichCursors` | `visibleRanges` check in `update()` |
 | 4.7 | Overlay positioning (no layout impact) | `yRichCursors` | `position: absolute` on `view.dom` |
 | 4.7 | Overlay positioning (no layout impact) | `yRichCursors` | `position: absolute` on `view.dom` |
 
 
 ## Components and Interfaces
 ## Components and Interfaces
@@ -295,7 +295,7 @@ Selection highlight: `Decoration.mark` on selected range with `background-color:
 
 
 ##### Off-Screen Cursor Indicators
 ##### Off-Screen Cursor Indicators
 
 
-When a remote cursor's absolute position falls outside `view.viewport.from`..`view.viewport.to`, the ViewPlugin renders an off-screen indicator instead of a widget decoration.
+When a remote cursor's absolute position falls outside the actually visible content range (`view.visibleRanges`), the ViewPlugin renders an off-screen indicator instead of a widget decoration. Note: `view.viewport` includes CodeMirror's pre-render buffer and must NOT be used for visibility classification — `view.visibleRanges` returns only the ranges the user can actually see.
 
 
 **DOM management**: The ViewPlugin creates two persistent container elements (`topContainer`, `bottomContainer`) and appends them to `view.dom` in the `constructor`. They are removed in `destroy()`. The containers are always present in the DOM but empty (zero height) when no off-screen cursors exist in that direction.
 **DOM management**: The ViewPlugin creates two persistent container elements (`topContainer`, `bottomContainer`) and appends them to `view.dom` in the `constructor`. They are removed in `destroy()`. The containers are always present in the DOM but empty (zero height) when no off-screen cursors exist in that direction.
 
 
@@ -313,7 +313,7 @@ view.dom (position: relative — already set by CodeMirror)
 **Indicator DOM structure**: Arrow (`↑` / `↓`) + avatar image (or initials fallback). Built by `createOffScreenIndicator()` in `off-screen-indicator.ts`. Sizing uses the same `AVATAR_SIZE` token from `theme.ts`. Opacity follows the same idle/active pattern as in-editor widgets.
 **Indicator DOM structure**: Arrow (`↑` / `↓`) + avatar image (or initials fallback). Built by `createOffScreenIndicator()` in `off-screen-indicator.ts`. Sizing uses the same `AVATAR_SIZE` token from `theme.ts`. Opacity follows the same idle/active pattern as in-editor widgets.
 
 
 **Update cycle**:
 **Update cycle**:
-1. In the `update(viewUpdate)` method, after computing absolute positions for all remote cursors, classify each into: `inViewport`, `above`, or `below` based on comparison with `view.viewport.{from, to}`
+1. In the `update(viewUpdate)` method, after computing absolute positions for all remote cursors, classify each into: `inViewport`, `above`, or `below` based on comparison with `view.visibleRanges` (first range's `from` for top boundary, last range's `to` for bottom boundary)
 2. For `inViewport` cursors: create `Decoration.widget` (same as current behavior)
 2. For `inViewport` cursors: create `Decoration.widget` (same as current behavior)
 3. For `above` / `below` cursors: rebuild `topContainer` / `bottomContainer` children via `replaceChildren()` — clear old indicator elements and append new ones
 3. For `above` / `below` cursors: rebuild `topContainer` / `bottomContainer` children via `replaceChildren()` — clear old indicator elements and append new ones
 4. Containers are rebuilt on every update where `viewportChanged` is true OR awareness has changed (same trigger as decoration rebuild)
 4. Containers are rebuilt on every update where `viewportChanged` is true OR awareness has changed (same trigger as decoration rebuild)

+ 1 - 0
.kiro/specs/collaborative-editor-awareness/research.md

@@ -102,6 +102,7 @@
 - `awareness.getStates().delete()` removal: confirm Yjs v13 awareness `update` event fires after removing the client from the internal map (verified in Yjs source: removal happens before the event).
 - `awareness.getStates().delete()` removal: confirm Yjs v13 awareness `update` event fires after removing the client from the internal map (verified in Yjs source: removal happens before the event).
 - **Recursive dispatch crash** (discovered during implementation): `setLocalStateField('cursor', ...)` inside the `update()` method fires an awareness `change` event **synchronously**. If the `change` listener calls `view.dispatch()` unconditionally, CodeMirror throws "Calls to EditorView.update are not allowed while an update is in progress". Mitigated by filtering the `change` listener to dispatch only when at least one **remote** client is in the changed set (`clients.findIndex(id => id !== awareness.doc.clientID) >= 0`). This matches the same pattern used by `y-remote-selections.js` in `y-codemirror.next`.
 - **Recursive dispatch crash** (discovered during implementation): `setLocalStateField('cursor', ...)` inside the `update()` method fires an awareness `change` event **synchronously**. If the `change` listener calls `view.dispatch()` unconditionally, CodeMirror throws "Calls to EditorView.update are not allowed while an update is in progress". Mitigated by filtering the `change` listener to dispatch only when at least one **remote** client is in the changed set (`clients.findIndex(id => id !== awareness.doc.clientID) >= 0`). This matches the same pattern used by `y-remote-selections.js` in `y-codemirror.next`.
 - **`y-protocols` not a direct dependency**: `y-protocols/awareness` exports the `Awareness` class, but neither `@growi/editor` nor `apps/app` list `y-protocols` as a direct dependency. `import type { Awareness } from 'y-protocols/awareness'` fails under strict pnpm resolution. Mitigated by deriving the type from the existing `y-websocket` dependency: `type Awareness = WebsocketProvider['awareness']`.
 - **`y-protocols` not a direct dependency**: `y-protocols/awareness` exports the `Awareness` class, but neither `@growi/editor` nor `apps/app` list `y-protocols` as a direct dependency. `import type { Awareness } from 'y-protocols/awareness'` fails under strict pnpm resolution. Mitigated by deriving the type from the existing `y-websocket` dependency: `type Awareness = WebsocketProvider['awareness']`.
+- **`view.viewport` vs `view.visibleRanges`** (discovered during validation): CodeMirror's `view.viewport` returns the **rendered** content range, which includes a pre-render buffer beyond the visible area for smooth scrolling. Using it for off-screen classification causes cursors in the buffer zone to be treated as in-viewport, resulting in invisible widget decorations instead of off-screen indicators. Must use `view.visibleRanges` (the ranges actually visible to the user) for accurate classification. Precedent: `setDataLine.ts` in the same package already uses `view.visibleRanges`.
 
 
 ## References
 ## References
 
 

+ 8 - 7
.kiro/specs/collaborative-editor-awareness/tasks.md

@@ -85,11 +85,12 @@
   - Remove both containers in the plugin's destroy method
   - Remove both containers in the plugin's destroy method
   - _Requirements: 4.7_
   - _Requirements: 4.7_
 
 
-- [x] 8.2 Classify remote cursors by viewport position and render off-screen indicators
-  - After computing absolute positions for all remote cursors in the update method, compare each position against the current viewport range
-  - For cursors above the viewport, build an indicator element (arrow up + avatar or initials fallback) and add it to the top container
-  - For cursors below the viewport, build an indicator element (arrow down + avatar or initials fallback) and add it to the bottom container
-  - For cursors within the viewport, render the in-editor widget decoration as before (no off-screen indicator)
+- [x] 8.2 Classify remote cursors by visible range position and render off-screen indicators
+  - After computing absolute positions for all remote cursors in the update method, compare each position against `view.visibleRanges` (NOT `view.viewport`, which includes CodeMirror's pre-render buffer beyond the visible area)
+  - Use `visibleRanges[0].from` as the top boundary and `visibleRanges[visibleRanges.length - 1].to` as the bottom boundary
+  - For cursors above the visible range, build an indicator element (arrow up + avatar or initials fallback) and add it to the top container
+  - For cursors below the visible range, build an indicator element (arrow down + avatar or initials fallback) and add it to the bottom container
+  - For cursors within the visible range, render the in-editor widget decoration as before (no off-screen indicator)
   - Replace container children on each relevant update cycle using a batch DOM operation
   - Replace container children on each relevant update cycle using a batch DOM operation
   - Apply the active CSS class to off-screen indicators when the corresponding client's activity state is active
   - Apply the active CSS class to off-screen indicators when the corresponding client's activity state is active
   - Rebuild containers when the viewport changes or awareness changes
   - Rebuild containers when the viewport changes or awareness changes
@@ -113,8 +114,8 @@
   - _Requirements: 4.1, 4.2, 4.4_
   - _Requirements: 4.1, 4.2, 4.4_
 
 
 - [x] 10. Integration Tests for Viewport Classification and Activity Tracking
 - [x] 10. Integration Tests for Viewport Classification and Activity Tracking
-- [x] 10.1 Test that remote cursors outside the viewport are excluded from widget decorations
-  - Simulate a remote client with a cursor position beyond the viewport range and verify that no widget decoration is created for that client
+- [x] 10.1 Test that remote cursors outside the visible range are excluded from widget decorations
+  - Simulate a remote client with a cursor position beyond `view.visibleRanges` and verify that no widget decoration is created for that client
   - _Requirements: 4.3, 4.6_
   - _Requirements: 4.3, 4.6_
 
 
 - [x] 10.2 Test activity tracking timer lifecycle with fake timers
 - [x] 10.2 Test activity tracking timer lifecycle with fake timers