BuzzbeeSCD/openchamber
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Consolidate multi-line comment inputs in Plan and File views* (#371)

Browse Source 
* refactor: adapt BlockWidget to use shared container map

Enable BlockWidget to reuse DOM containers via a shared map
Pass container map into widget decorations to preserve portals across updates

* feat: enable drag-to-select and guard new comments in FilesView

Guard new comment input from opening during drag-to-select actions
Enable drag-based line selection with initial drag state and mobile support
Reset dragging state on mouseup to end the selection

* fix(PlanView): gate new comment input during dragging

Add drag state to disable new comment input during selection drag
Indicate drag mode during mobile tap-extend to avoid unintended actions
Pass drag state to child UI to reflect ongoing drag
This commit is contained in:
Nelson Pires
2026-02-09 16:52:19 +02:00
committed by GitHub
parent f65388cf13
commit 8d8abdb54b
3 changed files with 74 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/ui/src/components/ui/CodeMirrorEditor.tsx
+24 -32
View File
@@ -39,22 +39,19 @@ const highlightLinesCompartment = new Compartment();
const blockWidgetsCompartment = new Compartment(); const blockWidgetsCompartment = new Compartment();
const searchCompartment = new Compartment(); const searchCompartment = new Compartment();
// Map to store widget container elements by ID // BlockWidget class definition moved inside helper or adapted to take map
// This allows us to render portals into them even if they are created by CM
const widgetContainers = new Map<string, HTMLElement>();
class BlockWidget extends WidgetType { class BlockWidget extends WidgetType {
constructor(readonly id: string) { constructor(readonly id: string, readonly containerMap: Map<string, HTMLElement>) {
super(); super();
} }
toDOM() { toDOM() {
let div = widgetContainers.get(this.id); let div = this.containerMap.get(this.id);
if (!div) { if (!div) {
div = document.createElement('div'); div = document.createElement('div');
div.className = 'oc-block-widget'; div.className = 'oc-block-widget';
div.dataset.widgetId = this.id; div.dataset.widgetId = this.id;
widgetContainers.set(this.id, div); this.containerMap.set(this.id, div);
} }
return div; return div;
} }
@@ -64,17 +61,13 @@ class BlockWidget extends WidgetType {
} }
destroy() { destroy() {
// Optional: cleanup if needed, but we might want to keep the element for React to unmount gracefully? // We do NOT remove from map here because CM might destroy the widget
// Actually, if CM destroys the DOM, React portal might complain if we don't unmount. // when it scrolls out of view, but we want to reuse the same container (and Portal)
// But since we render portals based on the 'blockWidgets' prop, if the widget is removed from prop, // when it scrolls back in.
// the portal will be removed by React.
// If CM removes it because it's out of viewport, we still want the container to exist in our map?
// No, if CM removes it, we should probably let it go.
// But for now let's keep it simple.
} }
} }
const createBlockWidgetsExtension = (widgets?: BlockWidgetDef[]) => { const createBlockWidgetsExtension = (widgets: BlockWidgetDef[] | undefined, containerMap: Map<string, HTMLElement>) => {
if (!widgets || widgets.length === 0) return []; if (!widgets || widgets.length === 0) return [];
return StateField.define<DecorationSet>({ return StateField.define<DecorationSet>({
@@ -90,7 +83,7 @@ const createBlockWidgetsExtension = (widgets?: BlockWidgetDef[]) => {
const line = state.doc.line(w.afterLine); const line = state.doc.line(w.afterLine);
// Add widget decoration // Add widget decoration
builder.add(line.to, line.to, Decoration.widget({ builder.add(line.to, line.to, Decoration.widget({
widget: new BlockWidget(w.id), widget: new BlockWidget(w.id, containerMap),
block: true, block: true,
side: 1, side: 1,
})); }));
@@ -98,16 +91,8 @@ const createBlockWidgetsExtension = (widgets?: BlockWidgetDef[]) => {
return builder.finish(); return builder.finish();
}, },
update(deco, tr) { update(deco, tr) {
// Always rebuild decorations when doc changes or widgets config changes // If the doc changed, map the decorations.
// But here we only see transaction. // If the widgets prop changed, the compartment reconfigure will handle it (create() will run).
// Since we reconfigure the compartment when props change, this update might mostly handle doc changes.
// For simplicity, we can map existing decorations or rebuild.
// Let's rebuild to ensure correct line placement.
// Wait, we can't access 'widgets' prop here easily unless we use a closure or effect.
// The `create` method runs when state is created.
// When we reconfigure the compartment, `create` might run again or we need `provide`.
// Actually, standard pattern is to map decorations.
return deco.map(tr.changes); return deco.map(tr.changes);
}, },
provide: f => EditorView.decorations.from(f) provide: f => EditorView.decorations.from(f)
@@ -169,6 +154,9 @@ export function CodeMirrorEditor({
const onViewDestroyRef = React.useRef(onViewDestroy); const onViewDestroyRef = React.useRef(onViewDestroy);
const [, forceUpdate] = React.useReducer((x) => x + 1, 0); const [, forceUpdate] = React.useReducer((x) => x + 1, 0);
// Scoped map for widget containers to avoid global collisions and memory leaks
const widgetContainersRef = React.useRef(new Map<string, HTMLElement>());
React.useEffect(() => { React.useEffect(() => {
valueRef.current = value; valueRef.current = value;
}, [value]); }, [value]);
@@ -209,7 +197,7 @@ export function CodeMirrorEditor({
editableCompartment.of(EditorView.editable.of(!readOnly)), editableCompartment.of(EditorView.editable.of(!readOnly)),
externalExtensionsCompartment.of(extensions ?? []), externalExtensionsCompartment.of(extensions ?? []),
highlightLinesCompartment.of(createHighlightLinesExtension(highlightLines)), highlightLinesCompartment.of(createHighlightLinesExtension(highlightLines)),
blockWidgetsCompartment.of(createBlockWidgetsExtension(blockWidgets)), blockWidgetsCompartment.of(createBlockWidgetsExtension(blockWidgets, widgetContainersRef.current)),
searchCompartment.of(enableSearch ? [search({ top: true }), keymap.of(searchKeymap)] : []), searchCompartment.of(enableSearch ? [search({ top: true }), keymap.of(searchKeymap)] : []),
], ],
}); });
@@ -243,10 +231,14 @@ export function CodeMirrorEditor({
editableCompartment.reconfigure(EditorView.editable.of(!readOnly)), editableCompartment.reconfigure(EditorView.editable.of(!readOnly)),
externalExtensionsCompartment.reconfigure(extensions ?? []), externalExtensionsCompartment.reconfigure(extensions ?? []),
highlightLinesCompartment.reconfigure(createHighlightLinesExtension(highlightLines)), highlightLinesCompartment.reconfigure(createHighlightLinesExtension(highlightLines)),
blockWidgetsCompartment.reconfigure(createBlockWidgetsExtension(blockWidgets)), blockWidgetsCompartment.reconfigure(createBlockWidgetsExtension(blockWidgets, widgetContainersRef.current)),
searchCompartment.reconfigure(enableSearch ? [search({ top: true }), keymap.of(searchKeymap)] : []), searchCompartment.reconfigure(enableSearch ? [search({ top: true }), keymap.of(searchKeymap)] : []),
], ],
}); });
// Force a re-render to ensure Portals can find the new widget containers in the DOM
// The containers are created synchronously by CodeMirror during dispatch -> toDOM
forceUpdate();
}, [extensions, highlightLines, lineNumbersConfig, readOnly, blockWidgets, enableSearch]); }, [extensions, highlightLines, lineNumbersConfig, readOnly, blockWidgets, enableSearch]);
React.useEffect(() => { React.useEffect(() => {
@@ -288,10 +280,10 @@ export function CodeMirrorEditor({
)} )}
/> />
{blockWidgets?.map((w) => { {blockWidgets?.map((w) => {
// Look for the widget container in the editor DOM // Look for the widget container in our scoped map
// Since we store them in a map too (as backup/optimization), we could check there, // We prefer the map over querySelector because the container might be created but not yet attached,
// but querySelector is safer to ensure it's actually in the DOM // or detached temporarily by CM (virtual scrolling). Keeping the portal mounted preserves state.
const container = viewRef.current?.dom.querySelector(`[data-widget-id="${w.id}"]`); const container = widgetContainersRef.current.get(w.id);
if (!container) return null; if (!container) return null;
return createPortal(w.content, container, w.id); return createPortal(w.content, container, w.id);
})} })}
packages/ui/src/components/views/FilesView.tsx
+30 -24
View File
@@ -647,6 +647,7 @@ export const FilesView: React.FC = () => {
const [lineSelection, setLineSelection] = React.useState<SelectedLineRange | null>(null); const [lineSelection, setLineSelection] = React.useState<SelectedLineRange | null>(null);
const isSelectingRef = React.useRef(false); const isSelectingRef = React.useRef(false);
const selectionStartRef = React.useRef<number | null>(null); const selectionStartRef = React.useRef<number | null>(null);
const [isDragging, setIsDragging] = React.useState(false);
// Session/config for sending comments // Session/config for sending comments
const currentSessionId = useSessionStore((state) => state.currentSessionId); const currentSessionId = useSessionStore((state) => state.currentSessionId);
@@ -663,6 +664,7 @@ export const FilesView: React.FC = () => {
const handleGlobalMouseUp = () => { const handleGlobalMouseUp = () => {
isSelectingRef.current = false; isSelectingRef.current = false;
selectionStartRef.current = null; selectionStartRef.current = null;
setIsDragging(false);
}; };
document.addEventListener('mouseup', handleGlobalMouseUp); document.addEventListener('mouseup', handleGlobalMouseUp);
return () => document.removeEventListener('mouseup', handleGlobalMouseUp); return () => document.removeEventListener('mouseup', handleGlobalMouseUp);
@@ -1805,10 +1807,10 @@ export const FilesView: React.FC = () => {
}); });
// Add input for new comment // Add input for new comment
if (lineSelection && !editingDraftId) { if (lineSelection && !editingDraftId && !isDragging) {
widgets.push({ widgets.push({
afterLine: lineSelection.end, afterLine: lineSelection.end,
id: 'new-comment-input', id: 'files-new-comment-input',
content: ( content: (
<InlineCommentInput <InlineCommentInput
lineRange={lineSelection} lineRange={lineSelection}
@@ -1820,7 +1822,7 @@ export const FilesView: React.FC = () => {
} }
return widgets; return widgets;
}, [selectedFile, currentSessionId, allDrafts, editingDraftId, lineSelection, handleSaveComment, removeDraft]); }, [selectedFile, currentSessionId, allDrafts, editingDraftId, lineSelection, handleSaveComment, removeDraft, isDragging]);
const fileViewer = ( const fileViewer = (
<div <div
@@ -2205,19 +2207,21 @@ export const FilesView: React.FC = () => {
const lineNumber = view.state.doc.lineAt(line.from).number; const lineNumber = view.state.doc.lineAt(line.from).number;
// Mobile: tap-to-extend selection // Mobile: tap-to-extend selection
if (isMobile && lineSelection && !event.shiftKey) { if (isMobile && lineSelection && !event.shiftKey) {
const start = Math.min(lineSelection.start, lineSelection.end, lineNumber); const start = Math.min(lineSelection.start, lineSelection.end, lineNumber);
const end = Math.max(lineSelection.start, lineSelection.end, lineNumber); const end = Math.max(lineSelection.start, lineSelection.end, lineNumber);
setLineSelection({ start, end }); setLineSelection({ start, end });
isSelectingRef.current = false; isSelectingRef.current = false;
selectionStartRef.current = null; selectionStartRef.current = null;
return true; setIsDragging(false);
} return true;
}
isSelectingRef.current = true; isSelectingRef.current = true;
selectionStartRef.current = lineNumber; selectionStartRef.current = lineNumber;
setIsDragging(true);
if (lineSelection && event.shiftKey) { if (lineSelection && event.shiftKey) {
const start = Math.min(lineSelection.start, lineNumber); const start = Math.min(lineSelection.start, lineNumber);
const end = Math.max(lineSelection.end, lineNumber); const end = Math.max(lineSelection.end, lineNumber);
setLineSelection({ start, end }); setLineSelection({ start, end });
@@ -2239,17 +2243,19 @@ export const FilesView: React.FC = () => {
} }
const lineNumber = view.state.doc.lineAt(line.from).number; const lineNumber = view.state.doc.lineAt(line.from).number;
const start = Math.min(selectionStartRef.current, lineNumber); const start = Math.min(selectionStartRef.current, lineNumber);
const end = Math.max(selectionStartRef.current, lineNumber); const end = Math.max(selectionStartRef.current, lineNumber);
setLineSelection({ start, end }); setLineSelection({ start, end });
return false; setIsDragging(true);
return false;
},
mouseup: () => {
isSelectingRef.current = false;
selectionStartRef.current = null;
setIsDragging(false);
return false;
},
}, },
mouseup: () => {
isSelectingRef.current = false;
selectionStartRef.current = null;
return false;
},
},
}} }}
/> />
</div> </div>
packages/ui/src/components/views/PlanView.tsx
+20 -12
View File
@@ -156,11 +156,13 @@ export const PlanView: React.FC = () => {
}, []); }, []);
const isSelectingRef = React.useRef(false); const isSelectingRef = React.useRef(false);
const selectionStartRef = React.useRef<number | null>(null); const selectionStartRef = React.useRef<number | null>(null);
const [isDragging, setIsDragging] = React.useState(false);
React.useEffect(() => { React.useEffect(() => {
const handleGlobalMouseUp = () => { const handleGlobalMouseUp = () => {
isSelectingRef.current = false; isSelectingRef.current = false;
selectionStartRef.current = null; selectionStartRef.current = null;
setIsDragging(false);
}; };
document.addEventListener('mouseup', handleGlobalMouseUp); document.addEventListener('mouseup', handleGlobalMouseUp);
return () => document.removeEventListener('mouseup', handleGlobalMouseUp); return () => document.removeEventListener('mouseup', handleGlobalMouseUp);
@@ -428,10 +430,10 @@ export const PlanView: React.FC = () => {
}); });
// Add new comment input if selecting AND not editing an existing draft // Add new comment input if selecting AND not editing an existing draft
if (lineSelection && !editingDraftId) { if (lineSelection && !editingDraftId && !isDragging) {
widgets.push({ widgets.push({
afterLine: lineSelection.end, afterLine: lineSelection.end,
id: 'new-comment-input', id: 'plan-new-comment-input',
content: ( content: (
<InlineCommentInput <InlineCommentInput
fileLabel={fileLabel} fileLabel={fileLabel}
@@ -457,6 +459,7 @@ export const PlanView: React.FC = () => {
handleSaveComment, handleSaveComment,
handleCancelComment, handleCancelComment,
removeDraft, removeDraft,
isDragging,
]); ]);
return ( return (
@@ -593,11 +596,14 @@ export const PlanView: React.FC = () => {
setLineSelection({ start, end }); setLineSelection({ start, end });
isSelectingRef.current = false; isSelectingRef.current = false;
selectionStartRef.current = null; selectionStartRef.current = null;
// Mobile tap-extend is atomic, so we don't start drag
setIsDragging(false);
return true; return true;
} }
isSelectingRef.current = true; isSelectingRef.current = true;
selectionStartRef.current = lineNumber; selectionStartRef.current = lineNumber;
setIsDragging(true);
if (lineSelection && event.shiftKey) { if (lineSelection && event.shiftKey) {
const start = Math.min(lineSelection.start, lineNumber); const start = Math.min(lineSelection.start, lineNumber);
@@ -614,17 +620,19 @@ export const PlanView: React.FC = () => {
if (event.buttons !== 1) return false; if (event.buttons !== 1) return false;
if (!isSelectingRef.current || selectionStartRef.current === null) return false; if (!isSelectingRef.current || selectionStartRef.current === null) return false;
const lineNumber = view.state.doc.lineAt(line.from).number; const lineNumber = view.state.doc.lineAt(line.from).number;
const start = Math.min(selectionStartRef.current, lineNumber); const start = Math.min(selectionStartRef.current, lineNumber);
const end = Math.max(selectionStartRef.current, lineNumber); const end = Math.max(selectionStartRef.current, lineNumber);
setLineSelection({ start, end }); setLineSelection({ start, end });
return false; setIsDragging(true);
return false;
},
mouseup: () => {
isSelectingRef.current = false;
selectionStartRef.current = null;
setIsDragging(false);
return false;
},
}, },
mouseup: () => {
isSelectingRef.current = false;
selectionStartRef.current = null;
return false;
},
},
}} }}
/> />
</div> </div>