fix: align floating comments by page #1813

pallavibakale · 2026-01-22T21:23:04Z

Refactored the comments layout to be page-aware: comments are aligned to their corresponding document pages, positioned relative to the page container, and rendered with independent per-page scrolling to avoid overlaps and maintain stable layouts.

Align floating comments per page using PresentationEditor page bounds and per-page containers.
Normalize comment positions on each page to avoid overlap and render with scoped scrolling.
Expand tracked-change and regular comment bodies when active; keep floating comments column full height.

- group floating comments per page using PresentationEditor bounds - normalize per-page positions and avoid overlaps in the sidebar - expand tracked-change and standard comment bodies when active - keep floating comments column full height for consistent layout

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 88c4d41523

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-01-22T21:28:03Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

+    // Update the actual top for rendering
+    comments.forEach((comment) => {
+      comment.top = comment.relativeTop;


Preserve absolute comment positions across layout updates

This block overwrites comment.top with the page‑relative value. Because processLocations is re‑invoked whenever comment positions refresh (the watchEffect depends on getFloatingComments/renderedSizes, which update on layout events), subsequent runs will treat the already‑relative comment.top as absolute and subtract pageTop again. That compounds the offset and can push comments upward/out of view after any layout update or zoom. Consider keeping an absolute top (e.g., absoluteTop) and deriving a separate render top each run, or recomputing from fresh bounds instead of mutating comment.top.

Useful? React with 👍 / 👎.

Copilot

Pull request overview

This PR refactors the floating comments layout to be page-aware, positioning comments relative to their corresponding document pages with independent per-page scrolling. This architectural change aims to prevent comment overlaps across pages and maintain stable layouts in multi-page documents.

Changes:

Refactored comment positioning from global to per-page coordinate system using PresentationEditor page bounds
Implemented independent scrollable containers for each page's comments
Added visual feedback by expanding comment bodies when active

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.

File	Description
packages/superdoc/src/components/CommentsLayer/FloatingComments.vue	Core refactoring: added page-aware positioning logic, per-page containers with independent scrolling, and updated comment positioning calculations
packages/superdoc/src/components/CommentsLayer/CommentDialog.vue	Enhanced UX by adding collapsible comment bodies that expand when active
packages/superdoc/src/SuperDoc.vue	Extended floating comments container to full height to support new per-page layout

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-22T21:31:04Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

      <div
-        v-for="comment in renderedSizes"
-        :key="comment.id"
-        :style="getCommentPosition(comment)"
-        class="floating-comment"
+        v-for="(comments, pageIndex) in commentsByPage"
+        :key="`page-${pageIndex}`"
+        class="page-comments-container"
+        :data-page-index="pageIndex"
+        :style="getPageContainerStyle(pageIndex)"
+        :ref="
+          (el) => {
+            if (el) pageContainerRefs[pageIndex] = el;
+          }
+        "
      >


The page-comments-container elements are scrollable but may not be keyboard accessible. Consider adding tabindex="0" to make these containers focusable and keyboard-navigable, allowing users to scroll through comments using keyboard arrow keys.

Copilot · 2026-01-22T21:31:04Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

-        const minTop = prev.top + prev.height + 15;
-        if (comment.top < minTop) {
-          comment.top = minTop;
+        const minTop = prev.relativeTop + prev.height + 15;


The magic number 15 represents the minimum spacing between comments but lacks explanation. Consider extracting it as a named constant for better code clarity. For example: const MIN_COMMENT_SPACING = 15;

Copilot · 2026-01-22T21:31:04Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

+  const gapOffset = pageIndex * 24;
+
+  if (!position) {
+    return {
+      position: 'absolute',
+      top: `${pageIndex * 1056 + gapOffset}px`,


The pageIndex parameter in getPageContainerStyle receives a string from the v-for loop (object keys are strings), but it's used in numeric calculations (line 82: pageIndex * 24). While JavaScript's type coercion will convert it to a number, this implicit conversion reduces code clarity. Consider explicitly converting: const gapOffset = parseInt(pageIndex) * 24; or Number(pageIndex) * 24;

Suggested change

const gapOffset = pageIndex * 24;

if (!position) {

return {

position: 'absolute',

top: `${pageIndex * 1056 + gapOffset}px`,

const numericPageIndex = Number(pageIndex);

const gapOffset = numericPageIndex * 24;

if (!position) {

return {

position: 'absolute',

top: `${numericPageIndex * 1056 + gapOffset}px`,

Copilot · 2026-01-22T21:31:04Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

          class="floating-comment"
-          :parent="parent"
-          :comment="comment.commentRef"
-        />
+        >
+          <CommentDialog
+            :key="comment.id + commentsRenderKey"
+            class="floating-comment"


The class "floating-comment" is applied to both the wrapper div (line 247) and the CommentDialog component (line 251), creating nested elements with the same class. This could lead to unintended CSS cascading effects since both elements will have position: absolute. Consider using a more specific class name for one of these elements, such as "floating-comment-wrapper" for the outer div.

Copilot · 2026-01-22T21:31:05Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

+.comments-dialog {
  position: absolute;
-  min-width: 300px;
+  min-width: 290px;
 }


The .comments-dialog class is defined here but appears to be unused in this component's template. The class is actually used in CommentDialog.vue, which has its own scoped styles defining the same class. Due to Vue's scoped styling, this definition won't affect the child component and appears to be dead code. Consider removing it to reduce confusion.

Copilot · 2026-01-22T21:31:05Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

  }

  nextTick(processLocations);
 });


The removal of the watch on activeComment means that floating comments will no longer automatically scroll into view when a comment becomes active. The old implementation called scrollIntoView on the active comment's element. With the new per-page container structure, this functionality should be re-implemented to scroll the appropriate page container to bring the active comment into view, otherwise users may lose the ability to automatically navigate to active comments.

Copilot · 2026-01-22T21:31:05Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

      <div
-        v-for="comment in renderedSizes"
-        :key="comment.id"
-        :style="getCommentPosition(comment)"
-        class="floating-comment"
+        v-for="(comments, pageIndex) in commentsByPage"
+        :key="`page-${pageIndex}`"
+        class="page-comments-container"
+        :data-page-index="pageIndex"
+        :style="getPageContainerStyle(pageIndex)"
+        :ref="
+          (el) => {
+            if (el) pageContainerRefs[pageIndex] = el;
+          }
+        "
      >


The page-comments-container elements have scrollable overflow but lack ARIA attributes for accessibility. Consider adding role="region" and aria-label or aria-labelledby attributes to help screen reader users understand that these are scrollable comment regions for specific pages. For example: role="region" :aria-label="Comments for page ${parseInt(pageIndex) + 1}"

Copilot · 2026-01-22T21:31:05Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

+      top: pageBounds.y || index * 1056, // fallback: approximate page height with gap
+      height: pageBounds.h || pageBounds.height || 1056,
+      pageIndex: index,
+    };
+  });
+
+  return positions;
+};
+
+// Get style for each page comment container
+const getPageContainerStyle = computed(() => (pageIndex) => {
+  const position = pagePositions.value[pageIndex];
+  const gapOffset = pageIndex * 24;
+
+  if (!position) {
+    return {
+      position: 'absolute',
+      top: `${pageIndex * 1056 + gapOffset}px`,


The magic numbers 1056 (page height) and 24 (gap offset) are used in multiple places without explanation. Consider extracting these as named constants at the top of the script section for better maintainability. For example: const DEFAULT_PAGE_HEIGHT = 1056; const PAGE_GAP = 24;

Copilot · 2026-01-22T21:31:06Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

+        :style="getPageContainerStyle(pageIndex)"
+        :ref="
+          (el) => {
+            if (el) pageContainerRefs[pageIndex] = el;


The ref assignment pattern pageContainerRefs[pageIndex] = el is missing .value. In Vue 3 composition API, refs created with ref() require accessing .value to update the underlying object. This should be pageContainerRefs.value[pageIndex] = el to correctly store the element references.

Suggested change

if (el) pageContainerRefs[pageIndex] = el;

if (el) pageContainerRefs.value[pageIndex] = el;

Copilot · 2026-01-22T21:31:06Z

packages/superdoc/src/components/CommentsLayer/FloatingComments.vue

+  const presentation = PresentationEditor.getInstance(props.currentDocument.id);
+  if (!presentation) return {};
+
+  const pages = presentation.getPages();


The code assumes presentation.getPages() will always return an array, but if it returns null or undefined, calling .forEach() on line 65 will throw a runtime error. Consider adding a check: const pages = presentation.getPages() || []; or const pages = presentation.getPages(); if (!pages) return {};

Suggested change

const pages = presentation.getPages();

const pages = presentation.getPages() || [];

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI review requested due to automatic review settings January 22, 2026 21:23

Copilot started reviewing on behalf of pallavibakale January 22, 2026 21:23 View session

chatgpt-codex-connector bot reviewed Jan 22, 2026

View reviewed changes

Copilot AI reviewed Jan 22, 2026

View reviewed changes

	if (el) pageContainerRefs[pageIndex] = el;
	if (el) pageContainerRefs.value[pageIndex] = el;

	const pages = presentation.getPages();
	const pages = presentation.getPages() \|\| [];

fix: align floating comments by page #1813

Are you sure you want to change the base?

fix: align floating comments by page #1813

Uh oh!

Conversation

pallavibakale commented Jan 22, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant