mirror of
https://github.com/kovetskiy/mark.git
synced 2026-04-18 04:01:12 +00:00
ac264210b5
This is based on guoweis-work PR https://github.com/kovetskiy/mark/pull/145 * feat(confluence): add support for fetching page body and inline comments * feat(cmd): add --preserve-comments flag to preserve inline comments * feat(mark): implement context-aware inline comment preservation * test(mark): add tests for context-aware MergeComments logic * fix: remove empty else branch in MergeComments to fix SA9003 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * perf: compile markerRegex once as package-level variable Avoids recompiling the inline comment marker regex on every call to MergeComments, which matters for pages with many comment markers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: guard against nil comments pointer in MergeComments Prevents a panic when GetInlineComments returns nil (e.g. on pages where the inline comments feature is not enabled). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: add edge-case tests for MergeComments; fix overlapping replacement Four new test cases: - SelectionMissing: comment dropped gracefully when text is gone from new body - OverlappingSelections: overlapping comments no longer corrupt the body; the later match (by position) wins and the earlier overlapping one is dropped - NilComments: nil pointer returns new body unchanged - HTMLEntities: <, >, ' selections match correctly Also fixes the overlapping replacement bug: apply back-to-front and skip any replacement whose end exceeds the start of an already-applied one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: escape ref attribute value in inline comment marker XML Use html.EscapeString on r.ref before interpolating it into the ac:ref attribute to prevent malformed XML if the value ever contains quotes or other special characters. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use first occurrence when no context is available in MergeComments Without context the old code left distance=0 for every match and updated bestStart on each iteration, so the final result depended on whichever occurrence was visited last (non-deterministic with respect to the search order). Restructure the loop to break immediately on the first match when hasCtx is false, making the behaviour explicit and deterministic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: log warning when overlapping inline comment marker is dropped Previously the overlap was silently skipped. Now a zerolog Warn message is emitted with the ref, the conflicting byte offsets, and the ref of the already-placed marker, so users can see which comment was lost rather than silently getting incomplete output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: warn when inline comments are silently dropped in MergeComments Three cases now emit a zerolog Warn instead of silently discarding: 1. Comment location != "inline": logs ref and actual location. 2. Selected text not found in new body: logs ref and selection text. 3. Overlapping replacement (existing): adds selection text to the already-present overlap warning for easier diagnosis. Also adds a selection field to the replacement struct so the overlap warning can report the dropped text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: update markerRegex to match markers with nested tags Replace ([^<]*) with (?s)(.*?) so the pattern: - Matches marker content that contains nested inline tags (e.g. <strong>) - Matches across newlines ((?s) / DOTALL mode) The old character class [^<]* stopped at the first < inside the marker body, causing the context-extraction step to miss any comment whose original selection spanned formatted text. Add TestMergeComments_NestedTags to cover this path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: guard against empty OriginalSelection in MergeComments strings.Index(s, "") always returns 0, so an empty escapedSelection would spin the search loop indefinitely (or panic when currentPos advances past len(newBody)). Skip comments with an empty selection early, emit a Warn log, and add TestMergeComments_EmptySelection to cover the path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: paginate GetInlineComments to avoid silently truncating results The Confluence child/comment endpoint is paginated. The previous single-request implementation silently dropped any comments beyond the server's default page size. Changes: - Add Links (context, next) to InlineComments struct so the _links field from each page response is decoded. - Rewrite GetInlineComments to loop with limit/start parameters (pageSize=100), accumulating all results, following the same pattern used by GetAttachments and label fetching. - Add TestMergeComments_DuplicateMarkerRef to cover the deduplication guard added in the previous commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix UTF-8 safety, API compat, log verbosity - levenshteinDistance: convert to []rune before empty-string checks so rune counts (not byte counts) are returned for strings with multi-byte characters - Add contextBefore/contextAfter helpers that use utf8.RuneStart to avoid slicing in the middle of a multi-byte UTF-8 sequence when extracting 100-char context windows from oldBody and newBody - Add truncateSelection helper (50 runes + ellipsis) and apply it in all Warn log messages that include the selected text, preventing large or sensitive page content from appearing in logs - Downgrade non-inline comment log from Warn to Debug with message 'comment ignored during inline marker merge: not an inline comment'; page-level comments are not inline markers and are not 'lost' - Restore original one-argument GetPageByID (expand='ancestors,version') and add GetPageByIDExpanded for the one caller that needs a custom expand value, preserving backward compatibility for API consumers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address new PR review comments - Remove custom min() function: shadows the Go 1.21+ built-in min for the entire package; the built-in handles the 3-arg call in levenshteinDistance identically - Validate rune boundaries on strings.Index candidates: skip any match where start or end falls in the middle of a multi-byte UTF-8 rune to prevent corrupt UTF-8 output - Defer preserve-comments API calls until after shouldUpdatePage is determined: avoids unnecessary GetPageByIDExpanded + GetInlineComments round-trips on no-op --changes-only runs - Capitalize Usage string for --preserve-comments flag (util/flags.go) and matching README.md entry to match sentence case of surrounding flags - Run gofmt on util/cli.go to fix struct literal field alignment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: document --preserve-comments feature in README Add a dedicated 'Preserving Inline Comments' section under Tricks with: - Usage examples (CLI flag and env var) - Step-by-step explanation of the Levenshtein-based relocation algorithm - Limitations (deleted text, overlapping selections, new pages, changes-only interaction) Also add a cross-reference NOTE near the --preserve-comments flag entry in the Usage section. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: fix markdownlint errors in README - Change unordered list markers from dashes to asterisks (MD004) - Remove extra blank line before Issues section (MD012) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Extract named types for InlineComments; optimize Levenshtein search - Introduce InlineCommentProperties, InlineCommentExtensions, and InlineCommentResult named types in confluence/api.go, replacing the anonymous nested struct in InlineComments.Results. Callers and tests can now construct/inspect comment objects without repeating the JSON shape. - Simplify makeComments helper in mark_test.go to use the new named types directly, eliminating the verbose anonymous struct literal. - Add two Levenshtein candidate-search optimisations in MergeComments: * Exact-context fast path: if both the before and after windows match exactly, take that occurrence immediately without computing distance. * Lower-bound pruning: skip the full O(m*n) Levenshtein computation for a candidate when the absolute difference in window lengths alone already meets or exceeds the current best distance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Use stable sort with ref tie-breaker; fix README overlap description - Replace slices.SortFunc with slices.SortStableFunc for the replacements slice, adding ref as a lexicographic tie-breaker when two markers resolve to the same start offset. This makes overlap resolution fully deterministic across runs. - Correct the README limitation note: the *earlier* overlapping match (lower byte offset) is what gets dropped; the later one (higher byte offset, applied first in the back-to-front pass) is kept. The previous wording said 'the second one is dropped' which was ambiguous and inaccurate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix rune-based lower-bound pruning; clarify test comment - Use utf8.RuneCountInString instead of len() for the Levenshtein lower-bound pruning computation. The levenshteinDistance function operates on rune slices, so byte-length differences can exceed the true rune-length difference for multibyte UTF-8 content, causing valid candidates to be incorrectly skipped. - Update TestMergeComments_SelectionMissing comment to say the comment is 'dropped with a warning' rather than 'silently dropped', matching the actual behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add missing unit tests for helpers and MergeComments scenarios Helper function tests: - TestTruncateSelection: short/exact/long strings and multibyte runes - TestLevenshteinDistance: empty strings, identical, insertions, deletions, substitutions, 'kitten/sitting', and a multibyte UTF-8 case to exercise rune-based counting - TestContextBefore / TestContextAfter: basic windowing, window larger than string, and a case where the raw byte offset lands mid-rune (é) to verify the rune-boundary correction logic MergeComments scenario tests: - TestMergeComments_MultipleComments: two non-overlapping comments both correctly applied via back-to-front replacement - TestMergeComments_EmptyResults: non-nil InlineComments with zero results returns body unchanged - TestMergeComments_NonInlineLocation: page-level comments (location != 'inline') are skipped; body unchanged - TestMergeComments_NoContext: when a ref has no marker in oldBody the first occurrence of the selection in newBody is used - TestMergeComments_UTF8: multibyte (Japanese) characters in both body and selection are handled correctly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix three correctness issues in MergeComments - Fix html import shadowing: alias the 'html' import as 'stdhtml' to avoid shadowing by the local 'html' variable used throughout ProcessFile. Both callers updated: stdhtml.EscapeString for the ref attribute, htmlEscapeText for the selection search. - Fix selection search with quotes/apostrophes: replace html.EscapeString for the selection with a new htmlEscapeText helper that only escapes &, <, > — not ' or ". Confluence storage HTML often leaves quotes and apostrophes unescaped in text nodes, so fully-escaped selections would fail to match and inline comments would be silently dropped. Add TestMergeComments_SelectionWithQuotes. - Fix duplicate-ref warnings: move seenRefs[ref]=true to immediately after the duplicate-check, before the search loop. Previously seenRefs was only set on a successful match, so multiple results for the same MarkerRef with no match in the new body would each emit a 'dropped' warning. Add TestMergeComments_DuplicateMarkerRefDropped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Optimize levenshteinDistance to use two rolling rows instead of full matrix Reduces memory allocation from O(m×n) to O(n) by keeping only the previous and current rows. Also swaps r1/r2 so the shorter string is used for column width, minimizing row allocation size. This matters in MergeComments where levenshteinDistance is called for every candidate match of every comment's selection in newBody — on pages with many comments or short/common selections the number of calls can be high. Addresses thread [40] from PR review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix test description and README algorithm doc mark_test.go (thread [43]): - TestMergeComments_HTMLEntities: the description incorrectly claimed ' (apostrophe) was tested; the selection '<world>' contains no apostrophe. Updated comment to accurately describe what is covered (</> entity matching) and note the ' limitation. - Add TestMergeComments_ApostropheSelection: verifies a selection with a literal apostrophe is found when the new body also has a literal apostrophe (the common case from mark's renderer). This exercises the htmlEscapeText path which intentionally does not encode ' or ". README.md (thread [42]): - Step 2 of the algorithm description said context was recorded 'immediately before and after the commented selection' which is ambiguous. Clarified that context windows are taken around the <ac:inline-comment-marker> tag boundaries in the old body (not around the raw selection text), so the context is stable even when the marker wraps additional inline markup such as <strong>. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Unexport mergeComments and cap candidate evaluation Thread [44]: MergeComments was exported but is internal-only — only called within the mark package and tested from the same package. Unexport it to mergeComments to avoid expanding the public API surface unnecessarily. Add a Go doc comment describing the function contract, HTML expectations, and the candidate cap. Thread [45]: The candidate-scoring loop had no upper bound. For short or common selections (e.g. 'a', 'the') on large pages the loop could invoke levenshteinDistance thousands of times, each allocating rune and int slices. Add a maxCandidates=100 constant and break once that many on-rune-boundary occurrences have been evaluated. The exact-context fast-path and lower-bound pruning already skip many candidates before Levenshtein is called, so in practice the cap is only reached for very common selections where the 100th candidate is unlikely to be meaningfully better than an earlier one anyway. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: fix HTMLEntities description and add ApostropheEncoded limitation test Thread #43: TestMergeComments_HTMLEntities had a misleading note claiming it covered the ' apostrophe case, but the selection under test ('<world>') did not include an apostrophe. Remove that note and add a dedicated TestMergeComments_ApostropheEncoded test that explicitly documents the known limitation: when a Confluence body stores an apostrophe as the numeric entity ', mergeComments cannot locate the selection (htmlEscapeText does not encode ' to '), so the comment is dropped with a warning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix CDATA selection fallback and extract contextWindowBytes constant Thread #46: mergeComments only searched for htmlEscapeText(selection) and would fail for selections inside CDATA-backed macro bodies (e.g. ac:code), where < and > are stored as raw characters rather than HTML entities. Restructure the search loop to build a searchForms slice: the escaped form is tried first (covers normal XML text nodes), and the raw unescaped form is appended as a fallback when they differ. A stopSearch flag exits early on an exact context match or when maxCandidates is reached, preserving the same performance guarantees as before. Add TestMergeComments_CDATASelection to cover this path. Thread #47: The context-window size 100 was repeated in four places across mergeComments (two in the context-extraction loop and two in the scoring loop). Extract it to const contextWindowBytes = 100 so it is easy to tune and stays consistent everywhere. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>