@@ -144,13 +150,9 @@ function CommentEntry({
'rp-entry-focused': entry.focused,
'rp-entry-comment-resolving': animating,
})}
- style={{
- top: entry.screenPos ? `${entry.screenPos.y}px` : undefined,
- visibility: entry.visible ? 'visible' : 'hidden',
- }}
ref={entryDivRef}
>
- {!thread.submitting && (!thread || thread.messages.length === 0) && (
+ {!submitting && (!thread || thread.messages.length === 0) && (
{t('no_comments')}
)}
@@ -163,7 +165,7 @@ function CommentEntry({
/>
))}
- {thread.submitting && (
+ {submitting && (
diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/entries/comment.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/entries/comment.tsx
index b847f5614b..ac3a8538bc 100644
--- a/services/web/frontend/js/features/source-editor/components/review-panel/entries/comment.tsx
+++ b/services/web/frontend/js/features/source-editor/components/review-panel/entries/comment.tsx
@@ -2,7 +2,10 @@ import { useTranslation } from 'react-i18next'
import { useState } from 'react'
import AutoExpandingTextArea from '../../../../../shared/components/auto-expanding-text-area'
import { formatTime } from '../../../../utils/format-date'
-import { useReviewPanelValueContext } from '../../../context/review-panel/review-panel-context'
+import {
+ useReviewPanelUpdaterFnsContext,
+ useReviewPanelValueContext,
+} from '../../../context/review-panel/review-panel-context'
import { ReviewPanelCommentThread } from '../../../../../../../types/review-panel/comment-thread'
import {
ReviewPanelCommentThreadMessage,
@@ -17,8 +20,8 @@ type CommentProps = {
function Comment({ thread, threadId, comment }: CommentProps) {
const { t } = useTranslation()
- const { deleteComment, handleLayoutChange, saveEdit } =
- useReviewPanelValueContext()
+ const { deleteComment, saveEdit } = useReviewPanelValueContext()
+ const { handleLayoutChange } = useReviewPanelUpdaterFnsContext()
const [deleting, setDeleting] = useState(false)
const [editing, setEditing] = useState(false)
diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/entries/entry-container.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/entries/entry-container.tsx
index 0d5b801d76..3140302aab 100644
--- a/services/web/frontend/js/features/source-editor/components/review-panel/entries/entry-container.tsx
+++ b/services/web/frontend/js/features/source-editor/components/review-panel/entries/entry-container.tsx
@@ -1,7 +1,17 @@
import classnames from 'classnames'
-function EntryContainer({ className, ...rest }: React.ComponentProps<'div'>) {
- return
+function EntryContainer({
+ id,
+ className,
+ ...rest
+}: React.ComponentProps<'div'>) {
+ return (
+
+ )
}
export default EntryContainer
diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/nav.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/nav.tsx
index bd267dcec9..1062a1a36b 100644
--- a/services/web/frontend/js/features/source-editor/components/review-panel/nav.tsx
+++ b/services/web/frontend/js/features/source-editor/components/review-panel/nav.tsx
@@ -6,14 +6,28 @@ import {
useReviewPanelUpdaterFnsContext,
} from '../../context/review-panel/review-panel-context'
import { isCurrentFileView, isOverviewView } from '../../utils/sub-view'
+import { useCallback } from 'react'
+import { useResizeObserver } from '../../../../shared/hooks/use-resize-observer'
function Nav() {
const { t } = useTranslation()
const { subView } = useReviewPanelValueContext()
- const { handleSetSubview } = useReviewPanelUpdaterFnsContext()
+ const { handleSetSubview, setNavHeight } = useReviewPanelUpdaterFnsContext()
+ const handleResize = useCallback(
+ el => {
+ // Use requestAnimationFrame to prevent errors like "ResizeObserver loop
+ // completed with undelivered notifications" that occur if onResize does
+ // something complicated. The cost of this is that onResize lags one frame
+ // behind, but it's unlikely to matter.
+ const height = el.offsetHeight
+ window.requestAnimationFrame(() => setNavHeight(height))
+ },
+ [setNavHeight]
+ )
+ const resizeRef = useResizeObserver(handleResize)
return (
-
+
@@ -94,6 +95,7 @@ function OverviewFile({ docId, docPath }: OverviewFileProps) {
key={id}
docId={docId}
entry={entry}
+ entryId={id}
permissions={permissions}
user={users[entry.metadata.user_id]}
/>
diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx
new file mode 100644
index 0000000000..116aa51237
--- /dev/null
+++ b/services/web/frontend/js/features/source-editor/components/review-panel/positioned-entries.tsx
@@ -0,0 +1,409 @@
+import { useEffect, useRef } from 'react'
+import { useLayoutContext } from '../../../../shared/context/layout-context'
+import {
+ ReviewPanelEntry,
+ ReviewPanelEntryScreenPos,
+} from '../../../../../../types/review-panel/entry'
+import useScopeValue from '../../../../shared/hooks/use-scope-value'
+import { debugConsole } from '../../../../utils/debugging'
+import { useReviewPanelValueContext } from '../../context/review-panel/review-panel-context'
+import useEventListener from '../../../../shared/hooks/use-event-listener'
+import { ReviewPanelDocEntries } from '../../../../../../types/review-panel/review-panel'
+import { dispatchReviewPanelLayout } from '../../extensions/changes/change-manager'
+
+type EntryView = {
+ wrapper: HTMLElement
+ indicator: HTMLElement | null
+ box: HTMLElement
+ callout: HTMLElement
+ layout: HTMLElement
+ height: number
+ previousEntryTop: number | null
+ previousCalloutTop: number | null
+ entry: ReviewPanelEntry
+ visible: boolean
+ positions?: {
+ entryTop: number
+ callout: { top: number; height: number; inverted: boolean }
+ }
+}
+
+function css(el: HTMLElement, props: React.CSSProperties) {
+ Object.assign(el.style, props)
+}
+
+function applyEntryVisibility(entryView: EntryView) {
+ const visible = !!entryView.entry.screenPos
+ entryView.wrapper.classList.toggle('rp-entry-hidden', !visible)
+ return visible
+}
+
+function calculateCalloutPosition(
+ screenPos: ReviewPanelEntryScreenPos,
+ entryTop: number,
+ lineHeight: number
+) {
+ const height = screenPos.height ?? lineHeight
+ const originalTop = screenPos.y
+ const inverted = entryTop <= originalTop
+ return {
+ top: inverted ? entryTop + height : originalTop + height - 1,
+ height: Math.abs(entryTop - originalTop),
+ inverted,
+ }
+}
+
+const calculateEntryViewPositions = (
+ entryViews: EntryView[],
+ lineHeight: number,
+ calculateTop: (originalTop: number, height: number) => number
+) => {
+ for (const entryView of entryViews) {
+ const entryVisible = applyEntryVisibility(entryView)
+ if (entryVisible) {
+ const entryTop = calculateTop(
+ entryView.entry.screenPos.y,
+ entryView.height
+ )
+ const callout = calculateCalloutPosition(
+ entryView.entry.screenPos,
+ entryTop,
+ lineHeight
+ )
+ entryView.positions = {
+ entryTop,
+ callout,
+ }
+ }
+ debugConsole.log('ENTRY', { entry: entryView.entry, top })
+ }
+}
+
+type PositionedEntriesProps = {
+ entries: Array<[keyof ReviewPanelDocEntries, ReviewPanelEntry]>
+ contentHeight: number
+ children: React.ReactNode
+}
+
+function PositionedEntries({
+ entries,
+ contentHeight,
+ children,
+}: PositionedEntriesProps) {
+ const { navHeight, toolbarHeight } = useReviewPanelValueContext()
+ const containerRef = useRef
(null)
+ const { reviewPanelOpen } = useLayoutContext()
+ const [lineHeight] = useScopeValue(
+ 'reviewPanel.rendererData.lineHeight'
+ )
+ const animationTimerRef = useRef(null)
+ const previousFocusedEntryIndexRef = useRef(0)
+
+ const layout = () => {
+ const container = containerRef.current
+ if (!container) {
+ return
+ }
+
+ const padding = reviewPanelOpen ? 8 : 4
+ const toolbarPaddedHeight = reviewPanelOpen ? toolbarHeight + 6 : 0
+ const navPaddedHeight = reviewPanelOpen ? navHeight + 4 : 0
+
+ // Create a list of entry views, typing together DOM elements and model.
+ // No measuring or style change is done at this point.
+ const entryViews: EntryView[] = []
+
+ // TODO: Look into tying the entry to the DOM element without going via a DOM data attribute
+ for (const wrapper of container.querySelectorAll(
+ '.rp-entry-wrapper'
+ )) {
+ const entryId = wrapper.dataset.entryId
+ if (!entryId) {
+ throw new Error('Could not find an entry ID')
+ }
+
+ const entry = entries.find(value => value[0] === entryId)?.[1]
+ if (!entry) {
+ throw new Error(`Could not find an entry for ID ${entryId}`)
+ }
+
+ const indicator = wrapper.querySelector(
+ '.rp-entry-indicator'
+ )
+ const box = wrapper.querySelector('.rp-entry')
+ const callout = wrapper.querySelector('.rp-entry-callout')
+ const layoutElement = reviewPanelOpen ? box : indicator
+
+ if (box && callout && layoutElement) {
+ const previousEntryTopData = box.dataset.previousEntryTop
+ const previousCalloutTopData = callout.dataset.previousEntryTop
+ entryViews.push({
+ wrapper,
+ indicator,
+ box,
+ callout,
+ layout: layoutElement,
+ visible: !!entry.screenPos,
+ height: 0,
+ previousEntryTop:
+ previousEntryTopData !== undefined
+ ? parseInt(previousEntryTopData)
+ : null,
+ previousCalloutTop:
+ previousCalloutTopData !== undefined
+ ? parseInt(previousCalloutTopData)
+ : null,
+ entry,
+ })
+ } else {
+ debugConsole.log(
+ 'Entry wrapper is missing indicator, box or callout, so ignoring',
+ wrapper
+ )
+ }
+ }
+
+ if (entryViews.length === 0) {
+ return
+ }
+
+ entryViews.sort((a, b) => a.entry.offset - b.entry.offset)
+
+ // Do the DOM interaction in three phases:
+ //
+ // - Apply the `display` property to all elements whose visibility has
+ // changed. This needs to happen first in order to measure heights.
+ // - Measure the height of each entry
+ // - Move each entry without animation to their original position
+ // relative to the editor content
+ // - In the next animation frame, re-enable animation and position each
+ // entry
+ //
+ // The idea is to batch DOM reads and writes to avoid layout thrashing. In
+ // this case, the best we can do is a write phase, a read phase then a
+ // final write phase.
+ // See https://web.dev/avoid-large-complex-layouts-and-layout-thrashing/
+
+ // First, update visibility for each entry that needs it
+ for (const entryView of entryViews) {
+ entryView.wrapper.classList.toggle('rp-entry-hidden', !entryView.visible)
+ }
+
+ // Next, measure the height of each entry
+ for (const entryView of entryViews) {
+ if (entryView.visible) {
+ entryView.height = entryView.layout.offsetHeight
+ }
+ }
+
+ // Calculate positions for all visible entries
+ let focusedEntryIndex = entryViews.findIndex(view => view.entry.focused)
+ if (focusedEntryIndex === -1) {
+ focusedEntryIndex = Math.min(
+ previousFocusedEntryIndexRef.current,
+ entryViews.length - 1
+ )
+ }
+ previousFocusedEntryIndexRef.current = focusedEntryIndex
+
+ const focusedEntryView = entryViews[focusedEntryIndex]
+ if (!focusedEntryView.entry.screenPos) {
+ return
+ }
+
+ // If the focused entry has no screenPos, we can't position other
+ // entryViews relative to it, so we position all other entryViews as
+ // though the focused entry is at the top and the rest follow it
+ const entryViewsAfter = focusedEntryView.visible
+ ? entryViews.slice(focusedEntryIndex + 1)
+ : [...entryViews]
+ const entryViewsBefore = focusedEntryView.visible
+ ? entryViews.slice(0, focusedEntryIndex).reverse() // Work through backwards, starting with the one just above
+ : []
+
+ debugConsole.log('focusedEntryIndex', focusedEntryIndex)
+
+ let lastEntryBottom = 0
+ let firstEntryTop = 0
+
+ // Put the focused entry as close to where it wants to be as possible
+ if (focusedEntryView.visible) {
+ const focusedEntryScreenPos = focusedEntryView.entry.screenPos
+ const entryTop = Math.max(focusedEntryScreenPos.y, toolbarPaddedHeight)
+ const callout = calculateCalloutPosition(
+ focusedEntryScreenPos,
+ entryTop,
+ lineHeight
+ )
+ focusedEntryView.positions = {
+ entryTop,
+ callout,
+ }
+ lastEntryBottom = entryTop + focusedEntryView.height
+ firstEntryTop = entryTop
+ }
+
+ // Calculate positions for entries that are below the focused entry
+ calculateEntryViewPositions(
+ entryViewsAfter,
+ lineHeight,
+ (originalTop: number, height: number) => {
+ const top = Math.max(originalTop, lastEntryBottom + padding)
+ lastEntryBottom = top + height
+ return top
+ }
+ )
+
+ // Calculate positions for entries that are above the focused entry
+ calculateEntryViewPositions(
+ entryViewsBefore,
+ lineHeight,
+ (originalTop: number, height: number) => {
+ const originalBottom = originalTop + height
+ const bottom = Math.min(originalBottom, firstEntryTop - padding)
+ const top = bottom - height
+ firstEntryTop = top
+ return top
+ }
+ )
+
+ // Calculate the new top overflow
+ const overflowTop = Math.max(0, toolbarPaddedHeight - firstEntryTop)
+
+ // Position everything where it was before, taking into account the new top
+ // overflow
+ const moveEntriesToInitialPosition = () => {
+ // Prevent CSS animation of position for this phase
+ container.classList.add('no-animate')
+ for (const entryView of entryViews) {
+ const { callout: calloutEl, positions } = entryView
+ if (positions) {
+ const { entryTop, callout } = positions
+
+ // Position the main wrapper in its original position, if it had
+ // one, or its new position otherwise
+ const entryTopInitial =
+ entryView.previousEntryTop === null
+ ? entryTop
+ : entryView.previousEntryTop
+
+ css(entryView.box, {
+ top: entryTopInitial + overflowTop + 'px',
+ // The entry element is invisible by default, to avoid flickering
+ // when positioning for the first time. Here we make sure it becomes
+ // visible after having a "top" value.
+ visibility: 'visible',
+ })
+
+ if (entryView.indicator) {
+ entryView.indicator.style.top = entryTopInitial + overflowTop + 'px'
+ }
+
+ entryView.box.dataset.previousEntryTop = entryTopInitial + ''
+
+ // Position the callout element in its original position, if it had
+ // one, or its new position otherwise
+ calloutEl.classList.toggle(
+ 'rp-entry-callout-inverted',
+ callout.inverted
+ )
+ const calloutTopInitial =
+ entryView.previousCalloutTop === null
+ ? callout.top
+ : entryView.previousCalloutTop
+
+ css(calloutEl, {
+ top: calloutTopInitial + overflowTop + 'px',
+ height: callout.height + 'px',
+ })
+
+ entryView.box.dataset.previousCalloutTop = calloutTopInitial + ''
+ }
+ }
+ }
+
+ const moveToFinalPositions = () => {
+ // Re-enable CSS animation of position for this phase
+ container.classList.remove('no-animate')
+
+ for (const entryView of entryViews) {
+ const { callout: calloutEl, positions } = entryView
+ if (positions) {
+ const { entryTop, callout } = positions
+
+ // Position the main wrapper, if it's moved
+ if (entryView.previousEntryTop !== entryTop) {
+ entryView.box.style.top = entryTop + overflowTop + 'px'
+ }
+ entryView.box.dataset.previousEntryTop = entryTop + ''
+
+ if (entryView.indicator) {
+ entryView.indicator.style.top = entryTop + overflowTop + 'px'
+ }
+
+ // Position the callout element
+ if (entryView.previousCalloutTop !== callout.top) {
+ calloutEl.style.top = callout.top + overflowTop + 'px'
+ entryView.callout.dataset.previousCalloutTop = callout.top + ''
+ }
+ }
+ }
+
+ animationTimerRef.current = null
+ }
+
+ moveEntriesToInitialPosition()
+
+ // Inform the editor of the new top overflow
+ window.dispatchEvent(
+ new CustomEvent('review-panel:event', {
+ detail: {
+ type: 'sizes',
+ payload: {
+ overflowTop,
+ height: lastEntryBottom + navPaddedHeight,
+ },
+ },
+ })
+ )
+
+ // Schedule the final, animated move
+ animationTimerRef.current = window.setTimeout(moveToFinalPositions, 60)
+ }
+
+ useEventListener('review-panel:layout', () => {
+ if (animationTimerRef.current) {
+ window.clearTimeout(animationTimerRef.current)
+ animationTimerRef.current = null
+ }
+ layout()
+ })
+
+ // Cancel scheduled move on unmount
+ useEffect(() => {
+ return () => {
+ if (animationTimerRef.current) {
+ window.clearTimeout(animationTimerRef.current)
+ animationTimerRef.current = null
+ }
+ }
+ }, [])
+
+ // Layout on first render. This is necessary to ensure layout happens when
+ // switching from overview to current file view
+ useEffect(() => {
+ dispatchReviewPanelLayout()
+ }, [])
+
+ return (
+
+ {children}
+
+ )
+}
+
+export default PositionedEntries
diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/toolbar/resolved-comments-dropdown.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/toolbar/resolved-comments-dropdown.tsx
index 0f11e10b0a..fb23091bf4 100644
--- a/services/web/frontend/js/features/source-editor/components/review-panel/toolbar/resolved-comments-dropdown.tsx
+++ b/services/web/frontend/js/features/source-editor/components/review-panel/toolbar/resolved-comments-dropdown.tsx
@@ -11,12 +11,13 @@ import {
} from '../../../../../../../types/review-panel/review-panel'
import { ReviewPanelResolvedCommentThread } from '../../../../../../../types/review-panel/comment-thread'
import { DocId } from '../../../../../../../types/project-settings'
+import { ReviewPanelEntry } from '../../../../../../../types/review-panel/entry'
export interface FilteredResolvedComments
extends ReviewPanelResolvedCommentThread {
content: string
threadId: ThreadId
- entryId: string
+ entryId: ThreadId
docId: DocId
docName: string | null
}
@@ -56,7 +57,9 @@ function ResolvedCommentsDropdown() {
for (const [docId, docEntries] of Object.entries(resolvedComments) as Array<
[DocId, ReviewPanelDocEntries]
>) {
- for (const [entryId, entry] of Object.entries(docEntries)) {
+ for (const [entryId, entry] of Object.entries(docEntries) as Array<
+ [ThreadId, ReviewPanelEntry]
+ >) {
if (entry.type === 'comment') {
const threadId = entry.thread_id
const thread =
diff --git a/services/web/frontend/js/features/source-editor/components/review-panel/toolbar/toolbar.tsx b/services/web/frontend/js/features/source-editor/components/review-panel/toolbar/toolbar.tsx
index 13a8dd30ab..eed5f54cce 100644
--- a/services/web/frontend/js/features/source-editor/components/review-panel/toolbar/toolbar.tsx
+++ b/services/web/frontend/js/features/source-editor/components/review-panel/toolbar/toolbar.tsx
@@ -1,9 +1,26 @@
import ResolvedCommentsDropdown from './resolved-comments-dropdown'
import ToggleMenu from './toggle-menu'
+import { useReviewPanelUpdaterFnsContext } from '../../../context/review-panel/review-panel-context'
+import { useCallback } from 'react'
+import { useResizeObserver } from '../../../../../shared/hooks/use-resize-observer'
function Toolbar() {
+ const { setToolbarHeight } = useReviewPanelUpdaterFnsContext()
+ const handleResize = useCallback(
+ el => {
+ // Use requestAnimationFrame to prevent errors like "ResizeObserver loop
+ // completed with undelivered notifications" that occur if onResize does
+ // something complicated. The cost of this is that onResize lags one frame
+ // behind, but it's unlikely to matter.
+ const height = el.offsetHeight
+ window.requestAnimationFrame(() => setToolbarHeight(height))
+ },
+ [setToolbarHeight]
+ )
+ const resizeRef = useResizeObserver(handleResize)
+
return (
-
+
diff --git a/services/web/frontend/js/features/source-editor/context/review-panel/hooks/use-angular-review-panel-state.ts b/services/web/frontend/js/features/source-editor/context/review-panel/hooks/use-angular-review-panel-state.ts
index 5d4b386e33..4aec1a2b42 100644
--- a/services/web/frontend/js/features/source-editor/context/review-panel/hooks/use-angular-review-panel-state.ts
+++ b/services/web/frontend/js/features/source-editor/context/review-panel/hooks/use-angular-review-panel-state.ts
@@ -1,15 +1,13 @@
import { useState, useMemo, useCallback } from 'react'
import useScopeValue from '../../../../../shared/hooks/use-scope-value'
-import useScopeEventEmitter from '../../../../../shared/hooks/use-scope-event-emitter'
import { sendMB } from '../../../../../infrastructure/event-tracking'
import { ReviewPanelState } from '../types/review-panel-state'
import * as ReviewPanel from '../types/review-panel-state'
import { SubView } from '../../../../../../../types/review-panel/review-panel'
import { ReviewPanelCommentEntry } from '../../../../../../../types/review-panel/entry'
+import { dispatchReviewPanelLayout as handleLayoutChange } from '../../../extensions/changes/change-manager'
function useAngularReviewPanelState(): ReviewPanelState {
- const emitLayoutChange = useScopeEventEmitter('review-panel:layout', false)
-
const [subView, setSubView] = useScopeValue
>(
'reviewPanel.subView'
)
@@ -113,12 +111,6 @@ function useAngularReviewPanelState(): ReviewPanelState {
[setSubView]
)
- const handleLayoutChange = useCallback(() => {
- window.requestAnimationFrame(() => {
- emitLayoutChange()
- })
- }, [emitLayoutChange])
-
const submitReply = useCallback(
(entry: ReviewPanelCommentEntry, replyContent: string) => {
submitReplyAngular({ ...entry, replyContent })
@@ -128,6 +120,8 @@ function useAngularReviewPanelState(): ReviewPanelState {
const [entryHover, setEntryHover] = useState(false)
const [isAddingComment, setIsAddingComment] = useState(false)
+ const [navHeight, setNavHeight] = useState(0)
+ const [toolbarHeight, setToolbarHeight] = useState(0)
const values = useMemo(
() => ({
@@ -139,7 +133,6 @@ function useAngularReviewPanelState(): ReviewPanelState {
entryHover,
isAddingComment,
gotoEntry,
- handleLayoutChange,
loadingThreads,
nVisibleSelectedChanges,
permissions,
@@ -148,6 +141,8 @@ function useAngularReviewPanelState(): ReviewPanelState {
resolvedComments,
saveEdit,
shouldCollapse,
+ navHeight,
+ toolbarHeight,
submitReply,
subView,
wantTrackChanges,
@@ -180,7 +175,6 @@ function useAngularReviewPanelState(): ReviewPanelState {
entryHover,
isAddingComment,
gotoEntry,
- handleLayoutChange,
loadingThreads,
nVisibleSelectedChanges,
permissions,
@@ -189,6 +183,8 @@ function useAngularReviewPanelState(): ReviewPanelState {
resolvedComments,
saveEdit,
shouldCollapse,
+ navHeight,
+ toolbarHeight,
submitReply,
subView,
wantTrackChanges,
@@ -217,10 +213,13 @@ function useAngularReviewPanelState(): ReviewPanelState {
const updaterFns = useMemo(
() => ({
handleSetSubview,
+ handleLayoutChange,
setEntryHover,
setCollapsed,
setShouldCollapse,
setIsAddingComment,
+ setNavHeight,
+ setToolbarHeight,
}),
[
handleSetSubview,
@@ -228,6 +227,8 @@ function useAngularReviewPanelState(): ReviewPanelState {
setEntryHover,
setShouldCollapse,
setIsAddingComment,
+ setNavHeight,
+ setToolbarHeight,
]
)
diff --git a/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts b/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts
index d1bbeb5185..30edb4d02f 100644
--- a/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts
+++ b/services/web/frontend/js/features/source-editor/context/review-panel/types/review-panel-state.ts
@@ -24,7 +24,6 @@ export interface ReviewPanelState {
entryHover: boolean
isAddingComment: boolean
gotoEntry: (docId: DocId, entryOffset: number) => void
- handleLayoutChange: () => void
loadingThreads: boolean
nVisibleSelectedChanges: number
permissions: ReviewPanelPermissions
@@ -37,6 +36,8 @@ export interface ReviewPanelState {
content: string
) => void
shouldCollapse: boolean
+ navHeight: number
+ toolbarHeight: number
submitReply: (entry: ReviewPanelCommentEntry, replyContent: string) => void
subView: SubView
wantTrackChanges: boolean
@@ -68,6 +69,7 @@ export interface ReviewPanelState {
}
updaterFns: {
handleSetSubview: (subView: SubView) => void
+ handleLayoutChange: () => void
setEntryHover: React.Dispatch>>
setIsAddingComment: React.Dispatch<
React.SetStateAction>
@@ -76,6 +78,10 @@ export interface ReviewPanelState {
setShouldCollapse: React.Dispatch<
React.SetStateAction>
>
+ setNavHeight: React.Dispatch>>
+ setToolbarHeight: React.Dispatch<
+ React.SetStateAction>
+ >
}
}
/* eslint-enable no-use-before-define */
diff --git a/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts b/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts
index 85d1b52999..1652633804 100644
--- a/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts
+++ b/services/web/frontend/js/features/source-editor/extensions/changes/change-manager.ts
@@ -25,6 +25,15 @@ export const dispatchEditorEvent = (type: string, payload?: unknown) => {
}, 0)
}
+export const dispatchReviewPanelLayout = () => {
+ window.dispatchEvent(new CustomEvent('review-panel:layout'))
+}
+
+const scheduleDispatchReviewPanelLayout = debounce(
+ dispatchReviewPanelLayout,
+ 50
+)
+
export type ChangeManager = {
initialize: () => void
handleUpdate: (update: ViewUpdate) => void
@@ -66,6 +75,10 @@ export const createChangeManager = (
const y = Math.round(coords.top - contentRect.top - editorPaddingTop)
const height = Math.round(coords.bottom - coords.top)
+ if (!entry.screenPos) {
+ visibilityChanged = true
+ }
+
entry.screenPos = { y, height, editorPaddingTop }
}
@@ -284,6 +297,12 @@ export const createChangeManager = (
if (changed) {
dispatchEditorEvent('track-changes:visibility_changed')
}
+ dispatchReviewPanelLayout()
+ // Ensure the layout is updated again once the review panel entries
+ // have updated in the React review panel. The use of a timeout is bad
+ // but the timings are a bit of a mess and will be improved when the
+ // review panel state is migrated away from Angular
+ scheduleDispatchReviewPanelLayout()
break
}
@@ -348,7 +367,6 @@ export const createChangeManager = (
})
)
}
-
break
}
}
diff --git a/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js b/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js
index b0ebbdbabe..a4533dad24 100644
--- a/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js
+++ b/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js
@@ -128,17 +128,17 @@ export default App.controller(
})
$scope.$on('layout:pdf:linked', (event, state) =>
- $scope.$broadcast('review-panel:layout')
+ ide.$scope.$broadcast('review-panel:layout')
)
$scope.$on('layout:pdf:resize', (event, state) => {
ide.$scope.reviewPanel.layoutToLeft =
state.east?.size < 220 || state.east?.initClosed
- $scope.$broadcast('review-panel:layout', false)
+ ide.$scope.$broadcast('review-panel:layout', false)
})
$scope.$on('expandable-text-area:resize', event =>
- $timeout(() => $scope.$broadcast('review-panel:layout'))
+ $timeout(() => ide.$scope.$broadcast('review-panel:layout'))
)
$scope.$on('review-panel:sizes', (e, sizes) => {
@@ -206,7 +206,7 @@ export default App.controller(
delete thread.submitting
thread.messages.push(formatComment(comment))
$scope.$apply()
- return $timeout(() => $scope.$broadcast('review-panel:layout'))
+ return $timeout(() => ide.$scope.$broadcast('review-panel:layout'))
})
ide.socket.on('accept-changes', function (doc_id, change_ids) {
@@ -321,7 +321,7 @@ export default App.controller(
}
return $timeout(function () {
$scope.$broadcast('review-panel:toggle')
- return $scope.$broadcast('review-panel:layout', false)
+ return ide.$scope.$broadcast('review-panel:layout', false)
})
})
@@ -559,12 +559,12 @@ export default App.controller(
$scope.$broadcast('review-panel:recalculate-screen-positions')
dispatchReviewPanelEvent('recalculate-screen-positions', entries)
- return $scope.$broadcast('review-panel:layout')
+ return ide.$scope.$broadcast('review-panel:layout')
}
})
$scope.$on('editor:track-changes:visibility_changed', () =>
- $timeout(() => $scope.$broadcast('review-panel:layout', false))
+ $timeout(() => ide.$scope.$broadcast('review-panel:layout', false))
)
$scope.$on(
@@ -576,20 +576,41 @@ export default App.controller(
ide.$scope.reviewPanel.selectedEntryIds = []
// Count of user-visible changes, i.e. an aggregated change will count as one.
ide.$scope.reviewPanel.nVisibleSelectedChanges = 0
- delete entries['add-comment']
- delete entries['bulk-actions']
+ const offset = selection_offset_start
+ const length = selection_offset_end - selection_offset_start
+
+ // Recreate the add comment and bulk actions entries only when
+ // necessary. This is to avoid the UI thinking that these entries have
+ // changed and getting into an infinite loop.
if (selection) {
- entries['add-comment'] = {
- type: 'add-comment',
- offset: selection_offset_start,
- length: selection_offset_end - selection_offset_start,
+ const existingAddComment = entries['add-comment']
+ if (
+ !existingAddComment ||
+ existingAddComment.offset !== offset ||
+ existingAddComment.length !== length
+ ) {
+ entries['add-comment'] = {
+ type: 'add-comment',
+ offset,
+ length,
+ }
}
- entries['bulk-actions'] = {
- type: 'bulk-actions',
- offset: selection_offset_start,
- length: selection_offset_end - selection_offset_start,
+ const existingBulkActions = entries['bulk-actions']
+ if (
+ !existingBulkActions ||
+ existingBulkActions.offset !== offset ||
+ existingBulkActions.length !== length
+ ) {
+ entries['bulk-actions'] = {
+ type: 'bulk-actions',
+ offset,
+ length,
+ }
}
+ } else {
+ delete entries['add-comment']
+ delete entries['bulk-actions']
}
for (const id in entries) {
@@ -640,7 +661,11 @@ export default App.controller(
dispatchReviewPanelEvent('recalculate-screen-positions', entries)
- return $scope.$broadcast('review-panel:layout')
+ // Ensure that watchers, such as the React-based review panel component,
+ // are informed of the changes to entries
+ ide.$scope.$apply()
+
+ return ide.$scope.$broadcast('review-panel:layout')
}
)
@@ -757,7 +782,7 @@ export default App.controller(
$scope.toggleReviewPanel()
}
return $timeout(function () {
- $scope.$broadcast('review-panel:layout')
+ ide.$scope.$broadcast('review-panel:layout')
return $scope.$broadcast('comment:start_adding')
})
}
@@ -765,7 +790,7 @@ export default App.controller(
$scope.startNewComment = function () {
$scope.$broadcast('comment:select_line')
dispatchReviewPanelEvent('comment:select_line')
- return $timeout(() => $scope.$broadcast('review-panel:layout'))
+ return $timeout(() => ide.$scope.$broadcast('review-panel:layout'))
}
ide.$scope.submitNewComment = function (content) {
@@ -800,16 +825,16 @@ export default App.controller(
)
// TODO: unused?
$scope.$broadcast('editor:clearSelection')
- $timeout(() => $scope.$broadcast('review-panel:layout'))
+ $timeout(() => ide.$scope.$broadcast('review-panel:layout'))
eventTracking.sendMB('rp-new-comment', { size: content.length })
}
$scope.cancelNewComment = entry =>
- $timeout(() => $scope.$broadcast('review-panel:layout'))
+ $timeout(() => ide.$scope.$broadcast('review-panel:layout'))
$scope.startReply = function (entry) {
entry.replying = true
- return $timeout(() => $scope.$broadcast('review-panel:layout'))
+ return $timeout(() => ide.$scope.$broadcast('review-panel:layout'))
}
ide.$scope.submitReply = function (entry, entry_id) {
@@ -839,14 +864,14 @@ export default App.controller(
thread.submitting = true
entry.replyContent = ''
entry.replying = false
- $timeout(() => $scope.$broadcast('review-panel:layout'))
+ $timeout(() => ide.$scope.$broadcast('review-panel:layout'))
eventTracking.sendMB('rp-comment-reply', trackingMetadata)
}
$scope.cancelReply = function (entry) {
entry.replying = false
entry.replyContent = ''
- return $scope.$broadcast('review-panel:layout')
+ return ide.$scope.$broadcast('review-panel:layout')
}
ide.$scope.resolveComment = function (doc_id, entry_id) {
@@ -947,7 +972,7 @@ export default App.controller(
_csrf: window.csrfToken,
}
)
- return $timeout(() => $scope.$broadcast('review-panel:layout'))
+ return $timeout(() => ide.$scope.$broadcast('review-panel:layout'))
}
ide.$scope.deleteComment = function (thread_id, comment_id) {
@@ -959,7 +984,7 @@ export default App.controller(
'X-CSRF-Token': window.csrfToken,
},
})
- return $timeout(() => $scope.$broadcast('review-panel:layout'))
+ return $timeout(() => ide.$scope.$broadcast('review-panel:layout'))
}
$scope.setSubView = function (subView) {
@@ -1244,7 +1269,7 @@ export default App.controller(
}
ide.$scope.reviewPanel.commentThreads = threads
dispatchReviewPanelEvent('loaded_threads')
- return $timeout(() => $scope.$broadcast('review-panel:layout'))
+ return $timeout(() => ide.$scope.$broadcast('review-panel:layout'))
})
}
@@ -1310,7 +1335,7 @@ export default App.controller(
switch (type) {
case 'line-height': {
ide.$scope.reviewPanel.rendererData.lineHeight = payload
- $scope.$broadcast('review-panel:layout')
+ ide.$scope.$broadcast('review-panel:layout')
break
}
diff --git a/services/web/frontend/js/shared/components/auto-expanding-text-area.tsx b/services/web/frontend/js/shared/components/auto-expanding-text-area.tsx
index 995008b663..149de71663 100644
--- a/services/web/frontend/js/shared/components/auto-expanding-text-area.tsx
+++ b/services/web/frontend/js/shared/components/auto-expanding-text-area.tsx
@@ -50,7 +50,11 @@ function AutoExpandingTextArea({
if (isFirstResize) {
isFirstResize = false
} else {
- onResize()
+ // Prevent errors like "ResizeObserver loop completed with undelivered
+ // notifications" that occur if onResize does something complicated.
+ // The cost of this is that onResize lags one frame behind, but it's
+ // unlikely to matter.
+ window.requestAnimationFrame(onResize)
}
})
diff --git a/services/web/frontend/js/utils/debugging.ts b/services/web/frontend/js/utils/debugging.ts
new file mode 100644
index 0000000000..454be4f879
--- /dev/null
+++ b/services/web/frontend/js/utils/debugging.ts
@@ -0,0 +1,7 @@
+type DebugConsole = { log(...data: any[]): void }
+
+export const debugging =
+ new URLSearchParams(window.location.search).get('debug') === 'true'
+export const debugConsole: DebugConsole = debugging
+ ? console
+ : { log: () => {} }
diff --git a/services/web/frontend/stylesheets/app/editor/review-panel.less b/services/web/frontend/stylesheets/app/editor/review-panel.less
index aad64bc5f7..9976299ab4 100644
--- a/services/web/frontend/stylesheets/app/editor/review-panel.less
+++ b/services/web/frontend/stylesheets/app/editor/review-panel.less
@@ -27,6 +27,8 @@
@rp-toolbar-height: 32px;
@rp-entry-animation-speed: 0.3s;
+// Move a little faster in React to compensate for the delay before moving the vertical position
+@rp-entry-animation-speed-react: 0.2s;
.rp-button() {
display: block; // IE doesn't do flex with inline items.
@@ -65,6 +67,8 @@
}
#review-panel {
+ --rp-animation-speed: @rp-entry-animation-speed;
+
display: block;
.rp-size-expanded & {
@@ -248,10 +252,10 @@
border-radius: 3px;
color: #fff;
cursor: pointer;
- transition: top @rp-entry-animation-speed, left 0.1s, right 0.1s;
+ transition: top var(--rp-animation-speed), left 0.1s, right 0.1s;
.no-animate & {
- transition: none;
+ transition: left 0.1s, right 0.1s;
}
&-focused {
@@ -372,10 +376,10 @@
border-left: solid @rp-entry-ribbon-width transparent;
border-radius: 3px;
background-color: #fff;
- transition: top @rp-entry-animation-speed, left 0.1s, right 0.1s;
+ transition: top var(--rp-animation-speed), left 0.1s, right 0.1s;
.no-animate & {
- transition: none;
+ transition: left 0.1s, right 0.1s;
}
&-insert,
@@ -640,7 +644,7 @@
}
.rp-entry-callout {
- transition: top @rp-entry-animation-speed, height @rp-entry-animation-speed;
+ transition: top var(--rp-animation-speed), height var(--rp-animation-speed);
.rp-state-current-file & {
position: absolute;
@@ -1193,6 +1197,8 @@ button when (@is-overleaf-light = true) {
// CM6-specific review panel rules
.ol-cm-review-panel {
+ --rp-animation-speed: @rp-entry-animation-speed-react;
+
position: relative;
z-index: 6;
display: block;
@@ -1254,6 +1260,17 @@ button when (@is-overleaf-light = true) {
bottom: 0;
}
+ .rp-entry-list-react {
+ position: relative;
+
+ .rp-entry-list-react-inner {
+ position: absolute;
+ top: 0;
+ left: 0;
+ right: 0;
+ }
+ }
+
.rp-state-current-file & {
.review-panel-tools {
display: flex;
diff --git a/services/web/types/review-panel/entry.ts b/services/web/types/review-panel/entry.ts
index fb430d41a5..189d863afe 100644
--- a/services/web/types/review-panel/entry.ts
+++ b/services/web/types/review-panel/entry.ts
@@ -1,6 +1,6 @@
import { ThreadId, UserId } from './review-panel'
-interface ReviewPanelEntryScreenPos {
+export interface ReviewPanelEntryScreenPos {
y: number
height: number
editorPaddingTop: number
diff --git a/services/web/types/review-panel/review-panel.ts b/services/web/types/review-panel/review-panel.ts
index c03b48ae0c..08c84776db 100644
--- a/services/web/types/review-panel/review-panel.ts
+++ b/services/web/types/review-panel/review-panel.ts
@@ -1,6 +1,10 @@
import { Brand } from '../helpers/brand'
import { DocId } from '../project-settings'
-import { ReviewPanelEntry } from './entry'
+import {
+ ReviewPanelAddCommentEntry,
+ ReviewPanelBulkActionsEntry,
+ ReviewPanelEntry,
+} from './entry'
import { ReviewPanelCommentThread } from './comment-thread'
export type SubView = 'cur_file' | 'overview'
@@ -13,7 +17,15 @@ export interface ReviewPanelPermissions {
}
export type ThreadId = Brand
-export type ReviewPanelDocEntries = Record
+// Entries may contain `add-comment` and `bulk-actions` props along with DocIds
+// Ideally the `add-comment` and `bulk-actions` objects should not be within the entries object
+// as the doc data, but this is what currently angular returns.
+export type ReviewPanelDocEntries = Record<
+ | ThreadId
+ | ReviewPanelAddCommentEntry['type']
+ | ReviewPanelBulkActionsEntry['type'],
+ ReviewPanelEntry
+>
export type ReviewPanelEntries = Record
@@ -27,6 +39,7 @@ export interface ReviewPanelUser {
isSelf: boolean
name: string
}
+
export type ReviewPanelUsers = Record
export type CommentId = Brand