fix: layout-engine comment/track highlight config #1800
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 264926d7c2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isInternal = el.dataset.commentInternal === 'true'; | ||
| const comments = commentIds.map((commentId) => ({ | ||
| commentId, | ||
| internal: isInternal, | ||
| trackedChange: false, |
There was a problem hiding this comment.
The new refreshCommentHighlights rebuilds the comments array from data-comment-ids using a single isInternal flag derived from data-comment-internal, which is set to true if any comment on the span is internal. If a span has mixed internal/external comments (supported by storing multiple IDs), switching the active thread to an external comment will still be treated as internal during refresh and will apply internal highlight colors/overrides. This regression only shows up on active-thread updates because the initial paint uses the real comment metadata. Consider persisting per-comment internal state (e.g., encode with the IDs) or reusing the original run metadata so refresh can choose the correct color.
Useful? React with 👍 / 👎.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
Solid PR! The comment highlight configuration system looks good - clean API, proper CSS variable fallbacks, and comprehensive tests.
Left a few inline comments:
- One issue worth addressing: default opacity mismatch between renderer and Vue layer
- A few minor items (code duplication, dead code) that can be follow-ups
| const COMMENT_EXTERNAL_COLOR = '#B1124B'; | ||
| const COMMENT_INTERNAL_COLOR = '#078383'; | ||
| const COMMENT_INACTIVE_ALPHA = '22'; | ||
| /** Default opacity for active comment highlights (0x44/0xff ≈ 0.267). */ |
There was a problem hiding this comment.
The default opacity values here differ from CommentsLayer.vue:
- Renderer: inactive=0x22 (~13%), active=0x44 (~27%)
- Vue overlay: inactive=0x33 (~20%), active=0x66 (~40%)
This creates a visual disconnect when no custom config is provided - text highlights appear more transparent than sidebar overlays. Consider synchronizing these constants.
| const baseColors = commentsConfig.highlightColors ?? {}; | ||
| const overlayOpacity = commentsConfig.overlayHighlightOpacity ?? commentsConfig.highlightOpacity ?? {}; | ||
|
|
||
| const defaultInactiveOpacity = 0x33 / 0xff; |
There was a problem hiding this comment.
These default opacity values don't match renderer.ts (0x33/0x66 here vs 0x22/0x44 there).
Maybe have them aligned for visual consistency?
| ? Math.max(0, Math.min(overlayOpacity.inactive, 1)) | ||
| : null; | ||
|
|
||
| const applyAlphaToHex = (color, opacity) => { |
There was a problem hiding this comment.
This applyAlphaToHex function is duplicated from renderer.ts
Consider extracting to a shared utility to avoid drift.
| const activeBaseColor = activeInternal | ||
| ? (highlightColors.internal ?? COMMENT_INTERNAL_COLOR) | ||
| : (highlightColors.external ?? COMMENT_EXTERNAL_COLOR); | ||
| const activeOverride = activeInternal ? highlightColors.activeInternal : highlightColors.activeExternal; |
There was a problem hiding this comment.
Nit-pick:
When activeOverride is set (e.g., activeExternal: '#ff0000'), the highlightOpacity.active config is ignored.
The override is used as-is without applying opacity. This might be intentional, but worth documenting that override colors should include their own alpha.
| const inactiveOpacity = clampOpacity(highlightOpacity.inactive) ?? DEFAULT_INACTIVE_ALPHA; | ||
| const inactiveColor = applyAlphaToHex(baseColor, inactiveOpacity); | ||
|
|
||
| const activeInternal = (activeComment ?? primary)?.internal === true; |
There was a problem hiding this comment.
Minor one:
The ?? primary fallback here is unreachable.
Since isActive = Boolean(activeComment) at line 6072, when we reach this line in an "active" context, activeComment is guaranteed to be truthy.
Could simplify to activeComment.internal === true.
| return { inactiveColor, activeColor, isActive }; | ||
| }; | ||
|
|
||
| const getCommentHighlight = (run: TextRun, config?: CommentHighlightOptions): string | undefined => { |
There was a problem hiding this comment.
This getCommentHighlight function appears to be dead code after the refactor to use computeCommentHighlightColors directly. Can be removed.
| elements.forEach((element) => { | ||
| const el = element as HTMLElement; | ||
| // Skip tracked-change comments that intentionally do not have a highlight background. | ||
| if (!el.style.backgroundColor) return; |
There was a problem hiding this comment.
Minor:
This relies on inline backgroundColor being set to identify refreshable elements. Works correctly today since the initial render always sets inline styles for highlightable comments, but could become fragile if render logic changes to use CSS classes.
A more robust check might be to look for dataset.commentIds presence instead.
2 participants
Tests:
pnpm --filter @superdoc/painter-dom test(fails in existingtable/border-utils.test.ts, 2 failures)pnpm --filter @superdoc/super-editor testpnpm --filter superdoc testVisual: