Skip to content

feat: support comment mode - #16

Open
renjie-run wants to merge 2 commits into
mainfrom
ai-page-comment
Open

feat: support comment mode#16
renjie-run wants to merge 2 commits into
mainfrom
ai-page-comment

Conversation

@renjie-run

Copy link
Copy Markdown
Collaborator

No description provided.

@seafile-dev seafile-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Comment

Warnings:

  • 新增 comment mode 没有对应自动化测试,关键的启停、事件拦截、元素数据及滚动位置更新路径未受回归保护。见内联评论。
    Suggested fix: 为 CommentModeAdapter 添加 JSDOM 测试,并覆盖 enable/disable、click/hover 的 postMessage payload、scroll RAF 合并和 destroy 清理。

Comment thread src/comment-mode.js

@seafile-dev seafile-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Request changes

Blocking:

  • Comment mode 的 postMessage 信任边界被绕过:任意嵌入方可启用模式,随后获得预览 DOM 的 htmlHint、文本、样式和坐标。见内联评论。

Warnings:

  • 新增模式缺少关键交互回归测试;见此前评论。

Comment thread src/iframe-adapter.js

@seafile-dev seafile-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Request changes

Blocking:

  • Comment-mode control messages are accepted from an untrusted source, then selected-DOM data is posted back with *; see inline comment.

Warnings:

  • Hover state is never cleared after the pointer leaves the iframe; see inline comment.
  • The earlier comment review also covers the missing regression tests.

Comment thread src/iframe-adapter.js
Comment thread src/comment-mode.js Outdated

@seafile-dev seafile-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Request changes

Blocking:

  • Comment-mode control messages are accepted from an untrusted source, then selected-DOM data is posted back with *; see inline comment.

Warnings:

  • Hover state is never cleared after the pointer leaves the iframe; see inline comment.
  • The earlier comment review also covers the missing regression tests.

Comment thread src/iframe-adapter.js
Comment thread src/comment-mode.js Outdated
Comment thread src/iframe-adapter.js Outdated

@seafile-dev seafile-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Comment

Warnings:

  • IframeAdapter.destroy() 对普通对象调用 forEach,在 comment mode 已启用的销毁路径会提前抛错,导致监听器与 crosshair 样式残留;见新增内联评论。
    Suggested fix: 使用 Object.values(this.pendingRequests) 迭代后再执行 CommentModeAdapter 清理,并增加该销毁路径的回归测试。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants