Handle markdown when not recognized - #52
Merged
Merged
Conversation
Deploying everfreenote with
|
| Latest commit: |
c135fc4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9ebfb1d3.everfreenote.pages.dev |
| Branch Preview URL: | https://features-markdown-imperative.everfreenote.pages.dev |
📝 WalkthroughWalkthroughAdds a "Force Paste Format" flow that lets users convert a selected editor range to Markdown. Changes extend SmartPasteService.resolvePaste to accept an optional forcedType, propagate selection state between web and native layers, add an apply-as-Markdown action in web/mobile editors, and include tests and docs. Changes
Sequence DiagramsequenceDiagram
actor User
participant WebEditor as RichTextEditor<br/>(WebView)
participant Page as app/editor-webview/<br/>page.tsx
participant Native as Native UI<br/>(Mobile)
participant SmartPaste as SmartPasteService
User->>WebEditor: Select text
WebEditor->>Page: onSelectionChange(true)
Page->>Native: postMessage(SELECTION_CHANGE, true)
Native->>Native: update hasSelection state
User->>Native: Tap MD button
Native->>WebEditor: runCommand('applySelectionAsMarkdown')
WebEditor->>WebEditor: extract selected text
WebEditor->>SmartPaste: resolvePaste(payload, options, 'markdown')
SmartPaste->>SmartPaste: use synthetic detection (forced-by-user)
SmartPaste-->>WebEditor: return { html, type: 'markdown' }
WebEditor->>WebEditor: replace selection with HTML
WebEditor->>Native: notify onContentChange
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
core/services/smartPaste.ts (2)
146-151: Catch-path fallback skipssanitizePasteHtml— add a comment explaining why it's safeThe normal plain-text path at line 145 runs output through
sanitizePasteHtml, but the catch fallback at line 149–150 usesplainTextToHtmldirectly without the extra sanitization layer. This is safe becauseplainTextToHtmlcallsescapeHtmlinternally andsafeStripHtmlalready passes output through DOMPurify — but the asymmetry is easy to miss during future maintenance. A brief comment would prevent accidental removal of the intentional design.💬 Suggested comment
} catch { warnings.push('plain:parse-failed') const fallbackText = payload.text ?? safeStripHtml(payload.html ?? '') + // plainTextToHtml escapes all text via escapeHtml; no additional sanitizePasteHtml needed here const fallbackHtml = plainTextToHtml(fallbackText) return { html: fallbackHtml, type: 'plain', warnings, detection } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/services/smartPaste.ts` around lines 146 - 151, Add a brief comment above the catch-path fallback in smartPaste.ts explaining why we intentionally skip calling sanitizePasteHtml there: note that the fallback builds fallbackText via safeStripHtml (which already runs DOMPurify) and then converts to HTML with plainTextToHtml (which calls escapeHtml), so additional sanitizePasteHtml is redundant; reference the functions safeStripHtml, plainTextToHtml, escapeHtml and sanitizePasteHtml in the comment so future maintainers understand the rationale and won't remove the intentional asymmetry.
125-141:result.typediverges fromdetection.typewhen forced markdown falls back to plainWhen
forcedType === 'markdown'but the content is oversized (line 129) or contains unsupported constructs (line 136), the returnedPasteResulthastype: 'plain'whiledetection.typeremains'markdown'(withreasons: ['forced-by-user']). Callers relying onresult.typeto reflect what was actually rendered will get the correct value, but callers also inspectingresult.detection.typewill see a mismatch. A comment at these branch points would clarify the intentional divergence.💬 Suggested comment
if (payload.text.length > config.maxLength) { warnings.push('plain:oversized-text') + // Falls back to plain even when forcedType='markdown'; result.type intentionally differs from detection.type const html = plainTextToHtml(payload.text) return { html: sanitizePasteHtml(html), type: 'plain', warnings, detection } } if (containsUnsupportedMarkdown(payload.text)) { warnings.push('plain:unsupported-markdown') + // Falls back to plain even when forcedType='markdown' const html = plainTextToHtml(payload.text) return { html: sanitizePasteHtml(html), type: 'plain', warnings, detection } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/services/smartPaste.ts` around lines 125 - 141, In the branches where detection.type === 'markdown' but you return type: 'plain' (the oversized-text check payload.text.length > config.maxLength and the containsUnsupportedMarkdown(payload.text) check), add a concise inline comment explaining the intentional divergence: that detection remains 'markdown' (often with reasons like 'forced-by-user') to preserve origin/intent, while the returned PasteResult.type is 'plain' to reflect what was actually rendered (plainTextToHtml -> sanitizePasteHtml) so callers know the rendered format; reference the checks and functions markdown.render, plainTextToHtml, sanitizePasteHtml and the warnings array to make the intent clear to future readers.ui/web/lib/editor.ts (1)
4-12: Add a brief comment explaining thefocus()in the chain
focus()beforedeleteRangeis non-obvious — without it, TipTap may ignore the mutation if the editor is not currently active. A one-liner improves future maintainability.💬 Suggested comment
const result = SmartPasteService.resolvePaste(payload, undefined, 'markdown') if (!result.html) return + // focus() ensures the editor is active before we mutate the selection range editor.chain().focus().deleteRange({ from, to }).insertContent(result.html).run()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/web/lib/editor.ts` around lines 4 - 12, The chain call in applySelectionAsMarkdown uses focus() before deleteRange to ensure the tiptap editor is active and will accept the mutation; add a one-line comment above editor.chain().focus() explaining that focus() is required so mutations (deleteRange/insertContent) are applied when the editor is not currently active, preventing no-op updates.ui/mobile/tests/unit/core-services-smartPaste.test.ts (1)
535-543: Strengthen the empty-text test with a result assertionThe test only asserts no exception is thrown. With
text: '', the implementation falls through to the plain-text branch and returns{ html: '<p></p>', type: 'plain' }. Adding a concrete assertion on the shape of the result makes the test more valuable and guards against regressions.✅ Suggested addition
expect(() => SmartPasteService.resolvePaste(payload, undefined, 'markdown')).not.toThrow() + const result = SmartPasteService.resolvePaste(payload, undefined, 'markdown') + expect(result.html).toBeDefined() + expect(result.html).not.toContain('<script>')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mobile/tests/unit/core-services-smartPaste.test.ts` around lines 535 - 543, The test currently only checks that SmartPasteService.resolvePaste doesn't throw for payload with text: '' but should also assert the returned value shape; call SmartPasteService.resolvePaste(payload, undefined, 'markdown') and assert it equals the expected plain-text result (e.g. { html: '<p></p>', type: 'plain' }) so the test verifies behavior rather than just absence of exceptions.docs/ai/planning/feature-force-paste-format.md (1)
11-13: Milestones and tasks are all unchecked despite being implemented in this PR.Consider marking completed tasks as
[x]to keep the planning doc accurate as a record of what was done.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai/planning/feature-force-paste-format.md` around lines 11 - 13, Update the milestone checklist to reflect completed work by changing the unchecked boxes to checked for the implemented items: mark "Milestone 1: Service layer — SmartPasteService accepts forced type" as [x], "Milestone 2: UI — 'Apply as Markdown' button in web and mobile toolbars" as [x], and "Milestone 3: Tests — unit + integration coverage, manual QA sign-off" as [x], so the planning doc accurately records the PR's completed tasks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/editor-webview/page.tsx`:
- Around line 258-262: handleSelectionChange currently posts a SELECTION_CHANGE
on every invocation causing noisy native messages; add a ref (e.g.,
prevSelectionRef using React.useRef<boolean | null>) to cache the previous
hasSelection value and only call window.ReactNativeWebView.postMessage when
prevSelectionRef.current !== hasSelection, updating prevSelectionRef.current
after posting; this change should be applied around the existing
handleSelectionChange function and used with the
onSelectionUpdate/onSelectionChange flow so duplicate boolean payloads are
suppressed.
In `@docs/ai/design/feature-force-paste-format.md`:
- Around line 152-153: Remove the duplicate component-table row for
EditorToolbar and consolidate into a single row that mentions the new
hasSelection prop and the MD button wired to a disabled state; specifically
merge the two entries so the row reads that EditorToolbar (component name) adds
a hasSelection prop and a markdown (MD) button whose disabled prop is driven by
hasSelection from the native screen, ensuring there is only one row describing
both changes.
- Line 65: The doc text incorrectly says applySelectionAsMarkdown is "registered
as a custom TipTap command"; update the sentence to state that inside
RichTextEditorWebView the runCommand function performs an early-return guard and
directly dispatches applySelectionAsMarkdown (i.e., runCommand checks for that
action and invokes applySelectionAsMarkdown) rather than it being registered as
a TipTap extension command; reference the runCommand function and the
applySelectionAsMarkdown action so readers can locate the implementation.
- Around line 41-44: Add a language specifier (e.g., "text") to the two fenced
code blocks that currently lack one: the block containing "Input: editor
selection (from, to) + selected plain text / Output: parsed HTML → inserted back
into editor at same position" and the block starting with "RichTextEditorWebView
(web, onSelectionUpdate) → onSelectionChange(bool) prop ..."; update the
opening ``` to ```text for both code fences so markdownlint MD040 is satisfied.
In `@docs/ai/implementation/feature-force-paste-format.md`:
- Line 19: The fenced code block in the markdown lacks a language specifier
causing MD040; update the opening fence to include a language (for example
change "```" to "```text") so the file-tree block is declared as text (e.g., use
"```text" before the tree and keep the closing "```" after).
- Line 176: Update the sentence "markdown-it is configured with `html: false` —
raw HTML in selected markdown text is not rendered." to capitalize the proper
noun by changing "markdown" to "Markdown" so it reads "markdown-it is configured
with `html: false` — raw HTML in selected Markdown text is not rendered.";
locate this exact sentence in the
docs/ai/implementation/feature-force-paste-format.md content (end of the
paragraph referencing markdown-it) and make the single-word capitalization
change.
- Around line 48-75: The docs incorrectly show a class with static resolvePaste
and private static _resolve; update the example to match the real implementation
by using the object-literal export SmartPasteService and the actual internal
helper name resolvePasteInternal (and its real signature) instead of _resolve,
and ensure the example uses the object method form
(SmartPasteService.resolvePaste) rather than class static syntax so readers see
the correct symbols (SmartPasteService, resolvePaste, resolvePasteInternal).
In `@docs/ai/planning/feature-force-paste-format.md`:
- Around line 34-38: Update the planning doc to mark Task 2.2 as removed/stale
and reflect that the markdown-apply button was inlined into MenuBar within
RichTextEditor.tsx instead of being created as a new component
(ApplyMarkdownButton.tsx); edit Task 2.2 to state that the button is implemented
inline in MenuBar, and adjust Task 2.3 to describe wiring inside the MenuBar JSX
in RichTextEditor.tsx rather than composing a separate ApplyMarkdownButton
component.
In `@docs/ai/requirements/feature-force-paste-format.md`:
- Around line 12-64: In docs/ai/requirements/feature-force-paste-format.md
ensure all instances of the format name "markdown" are capitalized to "Markdown"
(including inline text and headings) so references to the format are consistent;
update occurrences that mention SmartPasteService, the toolbar/button
descriptions, User Stories and Goals to use "Markdown", but do not change
package or code identifiers where lowercase is correct (e.g., `markdown-it`) and
keep inline code spans like `markdown` as-is only if they refer to package
names.
In `@docs/ai/testing/feature-force-paste-format.md`:
- Around line 47-101: Update inconsistent capitalization of the term "Markdown"
in the documentation: change all occurrences of lowercase "markdown" to
"Markdown" (e.g., in test steps and headings referencing the new fixture
`force-markdown.txt`, the Web editor checklist item `"Apply as Markdown"`, and
mentions near
`SmartPasteService.resolvePaste()`/`SmartPasteService._resolve()`); ensure
"Markdown" is used consistently everywhere (including lines describing the
fixture content and checklist entries) and run a quick grep for " markdown" to
catch any remaining lowercase instances.
- Around line 26-30: The doc references a stale standalone ApplyMarkdownButton
component; update the tests/docs to exercise the inlined button through
RichTextEditor instead: remove references to ApplyMarkdownButton.tsx and replace
with instructions to render RichTextEditor (or mount MenuBar within
RichTextEditor), assert the markdown-apply control in MenuBar is disabled when
there is no selection, assert it becomes enabled when selection exists, and
assert clicking it invokes the RichTextEditor handler (onApplyMarkdown) once;
use identifiers like MenuBar, RichTextEditor, and the onApplyMarkdown prop to
locate the code to test.
In `@ui/mobile/tests/unit/core-services-smartPaste.test.ts`:
- Around line 545-556: Wrap the spy usage in a try/finally to guarantee cleanup:
create the spy via jest.spyOn(SmartPasteService, 'detectPasteType'), then call
SmartPasteService.resolvePaste(...) and assert
expect(spy).not.toHaveBeenCalled() inside a try block, and call
spy.mockRestore() in the finally block so the spy is always restored even if the
assertion fails; reference the test that uses SmartPasteService.resolvePaste and
the spy on detectPasteType.
---
Nitpick comments:
In `@core/services/smartPaste.ts`:
- Around line 146-151: Add a brief comment above the catch-path fallback in
smartPaste.ts explaining why we intentionally skip calling sanitizePasteHtml
there: note that the fallback builds fallbackText via safeStripHtml (which
already runs DOMPurify) and then converts to HTML with plainTextToHtml (which
calls escapeHtml), so additional sanitizePasteHtml is redundant; reference the
functions safeStripHtml, plainTextToHtml, escapeHtml and sanitizePasteHtml in
the comment so future maintainers understand the rationale and won't remove the
intentional asymmetry.
- Around line 125-141: In the branches where detection.type === 'markdown' but
you return type: 'plain' (the oversized-text check payload.text.length >
config.maxLength and the containsUnsupportedMarkdown(payload.text) check), add a
concise inline comment explaining the intentional divergence: that detection
remains 'markdown' (often with reasons like 'forced-by-user') to preserve
origin/intent, while the returned PasteResult.type is 'plain' to reflect what
was actually rendered (plainTextToHtml -> sanitizePasteHtml) so callers know the
rendered format; reference the checks and functions markdown.render,
plainTextToHtml, sanitizePasteHtml and the warnings array to make the intent
clear to future readers.
In `@docs/ai/planning/feature-force-paste-format.md`:
- Around line 11-13: Update the milestone checklist to reflect completed work by
changing the unchecked boxes to checked for the implemented items: mark
"Milestone 1: Service layer — SmartPasteService accepts forced type" as [x],
"Milestone 2: UI — 'Apply as Markdown' button in web and mobile toolbars" as
[x], and "Milestone 3: Tests — unit + integration coverage, manual QA sign-off"
as [x], so the planning doc accurately records the PR's completed tasks.
In `@ui/mobile/tests/unit/core-services-smartPaste.test.ts`:
- Around line 535-543: The test currently only checks that
SmartPasteService.resolvePaste doesn't throw for payload with text: '' but
should also assert the returned value shape; call
SmartPasteService.resolvePaste(payload, undefined, 'markdown') and assert it
equals the expected plain-text result (e.g. { html: '<p></p>', type: 'plain' })
so the test verifies behavior rather than just absence of exceptions.
In `@ui/web/lib/editor.ts`:
- Around line 4-12: The chain call in applySelectionAsMarkdown uses focus()
before deleteRange to ensure the tiptap editor is active and will accept the
mutation; add a one-line comment above editor.chain().focus() explaining that
focus() is required so mutations (deleteRange/insertContent) are applied when
the editor is not currently active, preventing no-op updates.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/editor-webview/page.tsxcore/services/smartPaste.tscore/tests/fixtures/clipboard/force-markdown.txtdocs/ai/design/feature-force-paste-format.mddocs/ai/implementation/feature-force-paste-format.mddocs/ai/planning/feature-force-paste-format.mddocs/ai/requirements/feature-force-paste-format.mddocs/ai/testing/feature-force-paste-format.mdui/mobile/app/note/[id].tsxui/mobile/components/EditorToolbar.tsxui/mobile/components/EditorWebView.tsxui/mobile/tests/integration/smartPaste.integration.test.tsui/mobile/tests/unit/core-services-smartPaste.test.tsui/web/components/RichTextEditor.tsxui/web/components/RichTextEditorWebView.tsxui/web/lib/editor.ts
Comment on lines
+258
to
+262
| const handleSelectionChange = (hasSelection: boolean) => { | ||
| if (window.ReactNativeWebView) { | ||
| window.ReactNativeWebView.postMessage(JSON.stringify({ type: 'SELECTION_CHANGE', payload: hasSelection })) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "onSelectionChange" --type ts --type tsx -A 5Repository: koreyba/EverFreeNote
Length of output: 91
🏁 Script executed:
rg -n "onSelectionChange" -A 5Repository: koreyba/EverFreeNote
Length of output: 6949
🏁 Script executed:
sed -n '140,160p' ui/web/components/RichTextEditorWebView.tsxRepository: koreyba/EverFreeNote
Length of output: 565
🏁 Script executed:
sed -n '250,280p' app/editor-webview/page.tsxRepository: koreyba/EverFreeNote
Length of output: 773
Cache the previous selection state to avoid high-frequency postMessage calls
handleSelectionChange posts a SELECTION_CHANGE message on every invocation. However, TipTap's onSelectionUpdate event fires on every cursor movement or selection change, and onSelectionChange?.(from !== to) is called for each event. This results in postMessage calls even when the boolean value hasn't changed (e.g., moving the cursor while maintaining a selection will trigger many messages with identical payloads).
Store the previous hasSelection value in a ref and only post when the boolean state actually transitions to avoid unnecessary native layer communication:
Suggested pattern
const prevSelectionRef = useRef<boolean | null>(null)
const handleSelectionChange = (hasSelection: boolean) => {
if (prevSelectionRef.current !== hasSelection) {
prevSelectionRef.current = hasSelection
if (window.ReactNativeWebView) {
window.ReactNativeWebView.postMessage(JSON.stringify({ type: 'SELECTION_CHANGE', payload: hasSelection }))
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/editor-webview/page.tsx` around lines 258 - 262, handleSelectionChange
currently posts a SELECTION_CHANGE on every invocation causing noisy native
messages; add a ref (e.g., prevSelectionRef using React.useRef<boolean | null>)
to cache the previous hasSelection value and only call
window.ReactNativeWebView.postMessage when prevSelectionRef.current !==
hasSelection, updating prevSelectionRef.current after posting; this change
should be applied around the existing handleSelectionChange function and used
with the onSelectionUpdate/onSelectionChange flow so duplicate boolean payloads
are suppressed.
Comment on lines
+41
to
+44
| ``` | ||
| Input: editor selection (from, to) + selected plain text | ||
| Output: parsed HTML → inserted back into editor at same position | ||
| ``` |
There was a problem hiding this comment.
Add language specifiers to fenced code blocks (markdownlint MD040).
The blocks at lines 41–44 and 69–75 are flagged by markdownlint as missing language identifiers.
📝 Suggested fix
-```
+```text
Input: editor selection (from, to) + selected plain text
Output: parsed HTML → inserted back into editor at same position
-```
+```-```
+```text
RichTextEditorWebView (web, onSelectionUpdate)
→ onSelectionChange(bool) prop
...
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ai/design/feature-force-paste-format.md` around lines 41 - 44, Add a
language specifier (e.g., "text") to the two fenced code blocks that currently
lack one: the block containing "Input: editor selection (from, to) + selected
plain text / Output: parsed HTML → inserted back into editor at same position"
and the block starting with "RichTextEditorWebView (web, onSelectionUpdate) →
onSelectionChange(bool) prop ..."; update the opening ``` to ```text for both
code fences so markdownlint MD040 is satisfied.
| ref.current.runCommand('applySelectionAsMarkdown') | ||
| ``` | ||
|
|
||
| Inside `RichTextEditorWebView`, `runCommand` dispatches to TipTap's `editor.chain().focus()[command](...args).run()`. The `applySelectionAsMarkdown` action is registered as a custom TipTap command so it is reachable via `runCommand`. |
There was a problem hiding this comment.
Inaccurate description of how applySelectionAsMarkdown is dispatched.
The text states the action is "registered as a custom TipTap command", but the actual implementation in RichTextEditorWebView.tsx (lines 181–184) uses an early-return guard at the top of runCommand — it is not a registered TipTap extension command. Update to accurately describe the dispatch mechanism.
📝 Suggested revision
-Inside `RichTextEditorWebView`, `runCommand` dispatches to TipTap's `editor.chain().focus()[command](...args).run()`. The `applySelectionAsMarkdown` action is registered as a custom TipTap command so it is reachable via `runCommand`.
+Inside `RichTextEditorWebView`, `runCommand` intercepts `'applySelectionAsMarkdown'` with an early-return before the generic TipTap dispatch, invoking the `handleApplySelectionAsMarkdown` handler directly.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Inside `RichTextEditorWebView`, `runCommand` dispatches to TipTap's `editor.chain().focus()[command](...args).run()`. The `applySelectionAsMarkdown` action is registered as a custom TipTap command so it is reachable via `runCommand`. | |
| Inside `RichTextEditorWebView`, `runCommand` intercepts `'applySelectionAsMarkdown'` with an early-return before the generic TipTap dispatch, invoking the `handleApplySelectionAsMarkdown` handler directly. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ai/design/feature-force-paste-format.md` at line 65, The doc text
incorrectly says applySelectionAsMarkdown is "registered as a custom TipTap
command"; update the sentence to state that inside RichTextEditorWebView the
runCommand function performs an early-return guard and directly dispatches
applySelectionAsMarkdown (i.e., runCommand checks for that action and invokes
applySelectionAsMarkdown) rather than it being registered as a TipTap extension
command; reference the runCommand function and the applySelectionAsMarkdown
action so readers can locate the implementation.
Comment on lines
+152
to
+153
| | `EditorToolbar` | `ui/mobile/components/EditorToolbar.tsx` | Add `hasSelection` prop + MD button with disabled state | | ||
| | `EditorToolbar` | `ui/mobile/components/EditorToolbar.tsx` | Add button with `disabled` prop wired to `hasSelection` from native screen | |
There was a problem hiding this comment.
Duplicate row for EditorToolbar in component breakdown table.
EditorToolbar appears twice (lines 152 and 153) with slightly different descriptions. Consolidate into a single row.
📝 Suggested fix
| `EditorToolbar` | `ui/mobile/components/EditorToolbar.tsx` | Add `hasSelection` prop + MD button with disabled state |
-| `EditorToolbar` | `ui/mobile/components/EditorToolbar.tsx` | Add button with `disabled` prop wired to `hasSelection` from native screen |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `EditorToolbar` | `ui/mobile/components/EditorToolbar.tsx` | Add `hasSelection` prop + MD button with disabled state | | |
| | `EditorToolbar` | `ui/mobile/components/EditorToolbar.tsx` | Add button with `disabled` prop wired to `hasSelection` from native screen | | |
| | `EditorToolbar` | `ui/mobile/components/EditorToolbar.tsx` | Add `hasSelection` prop + MD button with disabled state | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ai/design/feature-force-paste-format.md` around lines 152 - 153, Remove
the duplicate component-table row for EditorToolbar and consolidate into a
single row that mentions the new hasSelection prop and the MD button wired to a
disabled state; specifically merge the two entries so the row reads that
EditorToolbar (component name) adds a hasSelection prop and a markdown (MD)
button whose disabled prop is driven by hasSelection from the native screen,
ensuring there is only one row describing both changes.
| ## Code Structure | ||
| **How is the code organized?** | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Fenced code block is missing a language specifier (MD040)
The file-tree block should declare a language (e.g., text) to satisfy the markdownlint MD040 rule flagged by static analysis.
-```
+```text
core/
services/
smartPaste.ts ...🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ai/implementation/feature-force-paste-format.md` at line 19, The fenced
code block in the markdown lacks a language specifier causing MD040; update the
opening fence to include a language (for example change "```" to "```text") so
the file-tree block is declared as text (e.g., use "```text" before the tree and
keep the closing "```" after).
Comment on lines
+34
to
+38
| - [ ] **Task 2.2 — Create `ApplyMarkdownButton` web component** | ||
| - Props: `disabled: boolean`, `onClick: () => void`. | ||
| - `aria-label="Apply as Markdown"`, respects `disabled`. | ||
| - Reuse existing toolbar button styling. | ||
| - File: `ui/web/components/ApplyMarkdownButton.tsx` (new) |
There was a problem hiding this comment.
Task 2.2 is stale — ApplyMarkdownButton.tsx was not created.
The design was revised to inline the MD button directly in MenuBar inside RichTextEditor.tsx. Task 2.2 should be updated to reflect that decision, and Task 2.3 adjusted accordingly (the wiring is now part of the MenuBar JSX, not a separate component composition step). Based on learnings, "Update phase docs when significant changes or decisions are made."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ai/planning/feature-force-paste-format.md` around lines 34 - 38, Update
the planning doc to mark Task 2.2 as removed/stale and reflect that the
markdown-apply button was inlined into MenuBar within RichTextEditor.tsx instead
of being created as a new component (ApplyMarkdownButton.tsx); edit Task 2.2 to
state that the button is implemented inline in MenuBar, and adjust Task 2.3 to
describe wiring inside the MenuBar JSX in RichTextEditor.tsx rather than
composing a separate ApplyMarkdownButton component.
Comment on lines
+12
to
+64
| - The existing `SmartPasteService` auto-detects clipboard content as `html`, `markdown`, or `plain` using a scoring heuristic. When the score for markdown falls below the threshold (default: 3), the content is silently downgraded to plain text. | ||
| - Users who copy markdown from terminals, AI chat outputs, GitHub previews, or documentation sites may receive garbled plain text in the editor — losing all formatting. | ||
| - There is currently no way to fix an incorrect detection after the fact. The user must manually reformat the content — which is tedious and error-prone. | ||
|
|
||
| **Who is affected?** | ||
| - Developers and technical users who frequently paste from AI assistants, terminals, GitHub, and documentation sites. | ||
| - Power users who know the source format and want a quick way to correct a failed auto-detection. | ||
|
|
||
| **Current workaround:** Manually re-apply heading/bold/list formatting after an incorrect plain-text paste. | ||
|
|
||
| ## Goals & Objectives | ||
| **What do we want to achieve?** | ||
|
|
||
| **Primary goals:** | ||
| - Allow the user to select already-pasted text and explicitly re-render it as markdown via a toolbar button. | ||
| - The action is one-shot: select → click → formatted. No persistent state or mode involved. | ||
|
|
||
| **Secondary goals:** | ||
| - The solution should be extensible to other formats (`html`, `plain`) in the future. | ||
|
|
||
| **Non-goals (Phase 1):** | ||
| - Forcing `html` or `plain` paste formats. | ||
| - Persisting any format preference across sessions. | ||
| - Changing the underlying markdown parsing or sanitization pipeline. | ||
| - Handling edge cases where the user selects already-formatted TipTap content — the user is responsible for what they select. | ||
|
|
||
| ## User Stories & Use Cases | ||
| **How will users interact with the solution?** | ||
|
|
||
| 1. **As a developer,** I want to select plain text that contains markdown syntax and click "Apply as Markdown", so that headings, code blocks, and lists are rendered correctly after a failed auto-detection. | ||
| 2. **As a mobile user,** I want the same button available in the mobile toolbar. | ||
|
|
||
| **Key workflow:** | ||
| 1. User pastes content → auto-detection misses → content appears as plain text with raw markdown syntax. | ||
| 2. User selects the pasted text → clicks "Apply as Markdown" button in toolbar → selection is replaced with properly rendered markdown. | ||
|
|
||
| **Known limitation:** | ||
| If the selection contains text that was already formatted by TipTap (not raw markdown characters), `textBetween` will strip that formatting before parsing. The user is responsible for selecting only the relevant plain text. This is not a bug — it is expected behaviour for a corrective tool. | ||
|
|
||
| **Edge cases:** | ||
| - Selection contains plain text with no markdown syntax → markdown-it parses it as-is (paragraph), result looks the same as before. No crash. | ||
| - Selection is empty → button is disabled (no action). | ||
| - Undo works the same as any other editor content replacement. | ||
|
|
||
| ## Success Criteria | ||
| **How will we know when we're done?** | ||
|
|
||
| - [ ] An "Apply as Markdown" button is present in the web toolbar (`RichTextEditor.tsx`) and mobile toolbar (`EditorToolbar.tsx`). | ||
| - [ ] The button is disabled when there is no text selection. | ||
| - [ ] Clicking the button with a selection: extracts plain text of the selection, parses as markdown, replaces selection with rendered HTML. | ||
| - [ ] Existing auto-detection behaviour on paste is 100% unchanged. | ||
| - [ ] Unit tests cover the selection-to-markdown conversion path in `SmartPasteService`. | ||
| - [ ] Integration tests verify end-to-end: select plain markdown text → button → correct HTML output. |
There was a problem hiding this comment.
Capitalize "Markdown" consistently (proper noun).
"Markdown" is a proper noun (the name of the format) and should be capitalized throughout the document. It appears lowercase at lines 12, 26, 41, 45, 52, 61, and 64.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~12-~12: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...a scoring heuristic. When the score for markdown falls below the threshold (default: 3),...
(MARKDOWN_NNP)
[uncategorized] ~26-~26: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...ted text and explicitly re-render it as markdown via a toolbar button. - The action is o...
(MARKDOWN_NNP)
[uncategorized] ~41-~41: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...want to select plain text that contains markdown syntax and click "Apply as Markdown", s...
(MARKDOWN_NNP)
[uncategorized] ~45-~45: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ... content appears as plain text with raw markdown syntax. 2. User selects the pasted text...
(MARKDOWN_NNP)
[uncategorized] ~52-~52: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...- Selection contains plain text with no markdown syntax → markdown-it parses it as-is (p...
(MARKDOWN_NNP)
[uncategorized] ~61-~61: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ... plain text of the selection, parses as markdown, replaces selection with rendered HTML....
(MARKDOWN_NNP)
[uncategorized] ~64-~64: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...n tests verify end-to-end: select plain markdown text → button → correct HTML output. - ...
(MARKDOWN_NNP)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ai/requirements/feature-force-paste-format.md` around lines 12 - 64, In
docs/ai/requirements/feature-force-paste-format.md ensure all instances of the
format name "markdown" are capitalized to "Markdown" (including inline text and
headings) so references to the format are consistent; update occurrences that
mention SmartPasteService, the toolbar/button descriptions, User Stories and
Goals to use "Markdown", but do not change package or code identifiers where
lowercase is correct (e.g., `markdown-it`) and keep inline code spans like
`markdown` as-is only if they refer to package names.
Comment on lines
+47
to
+101
| - [ ] **Web flow:** select plain markdown text in editor → click "Apply as Markdown" → selection replaced with rendered markdown (heading/list visible). | ||
| - [ ] **Button disabled with no selection:** click elsewhere to deselect → button becomes disabled. | ||
| - [ ] **Mobile flow:** same action via mobile toolbar button. | ||
| - [ ] **Undo:** `Ctrl+Z` after applying → selection content restored to plain text. | ||
| - [ ] **Regression — normal paste:** pasting content without using the button still uses auto-detection. | ||
|
|
||
| ## Test Data | ||
|
|
||
| ### New fixture: `force-markdown.txt` | ||
|
|
||
| Content that scores below auto-detection threshold (< 3 pts) but is valid markdown: | ||
|
|
||
| ```markdown | ||
| Project notes | ||
|
|
||
| - Buy milk | ||
| - Call dentist | ||
| - Review PR | ||
| ``` | ||
|
|
||
| Scores ~2 pts (bullet list only) → auto-detects as plain, but renders correctly when forced. | ||
|
|
||
| ### Existing fixtures (unchanged): | ||
| - `ui/mobile/tests/integration/fixtures/ai-chat-markdown.md` | ||
| - `ui/mobile/tests/integration/fixtures/google-docs.html` | ||
| - `ui/mobile/tests/integration/fixtures/web-article.html` | ||
| - `ui/mobile/tests/integration/fixtures/plain.txt` | ||
|
|
||
| ## Test Reporting & Coverage | ||
|
|
||
| - Run: `npm run test -- --coverage` | ||
| - New paths to verify at 100%: | ||
| - `SmartPasteService.resolvePaste()` forced-type branch | ||
| - `SmartPasteService._resolve()` private helper | ||
| - Coverage gaps allowed: visual/accessibility attributes (covered by manual QA) | ||
|
|
||
| ## Manual Testing | ||
|
|
||
| ### Web editor checklist | ||
| - [ ] "Apply as Markdown" button visible in toolbar | ||
| - [ ] Button is greyed out / disabled when no text is selected | ||
| - [ ] Select low-score markdown text → click button → correct rendering (headings, lists, code blocks) | ||
| - [ ] Button has no persistent state — clicking again on a new selection works identically | ||
| - [ ] `aria-label="Apply as Markdown"` announced by screen reader; `disabled` properly communicated | ||
| - [ ] No visual regression in existing toolbar buttons | ||
|
|
||
| ### Mobile toolbar checklist | ||
| - [ ] Button appears in scrollable toolbar | ||
| - [ ] Disabled appearance when no selection | ||
| - [ ] Tap with selection → markdown rendered correctly | ||
| - [ ] Undo works via mobile undo gesture | ||
|
|
||
| ### Edge cases | ||
| - [ ] Empty selection → button disabled, no action on click | ||
| - [ ] Selection with no markdown syntax → result looks same as before (plain paragraph), no crash |
There was a problem hiding this comment.
Capitalize "Markdown" consistently (proper noun).
"markdown" appears lowercase at lines 47, 88, and 101.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~47-~47: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...on?** - [ ] Web flow: select plain markdown text in editor → click "Apply as Markdo...
(MARKDOWN_NNP)
[uncategorized] ~88-~88: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...text is selected - [ ] Select low-score markdown text → click button → correct rendering...
(MARKDOWN_NNP)
[uncategorized] ~101-~101: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...action on click - [ ] Selection with no markdown syntax → result looks same as before (p...
(MARKDOWN_NNP)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ai/testing/feature-force-paste-format.md` around lines 47 - 101, Update
inconsistent capitalization of the term "Markdown" in the documentation: change
all occurrences of lowercase "markdown" to "Markdown" (e.g., in test steps and
headings referencing the new fixture `force-markdown.txt`, the Web editor
checklist item `"Apply as Markdown"`, and mentions near
`SmartPasteService.resolvePaste()`/`SmartPasteService._resolve()`); ensure
"Markdown" is used consistently everywhere (including lines describing the
fixture content and checklist entries) and run a quick grep for " markdown" to
catch any remaining lowercase instances.
Comment on lines
+545
to
+556
| it('does not call detectPasteType when forcedType is provided', () => { | ||
| const spy = jest.spyOn(SmartPasteService, 'detectPasteType') | ||
| const payload = { | ||
| html: null, | ||
| text: '# Heading\n\nContent', | ||
| types: ['text/plain'], | ||
| } | ||
|
|
||
| SmartPasteService.resolvePaste(payload, undefined, 'markdown') | ||
| expect(spy).not.toHaveBeenCalled() | ||
| spy.mockRestore() | ||
| }) |
There was a problem hiding this comment.
Wrap spy assertion in try/finally to guarantee mockRestore() on failure
If the expect(spy).not.toHaveBeenCalled() assertion at line 554 fails, spy.mockRestore() at line 555 is never reached, leaking the spy into subsequent tests in the same file.
🛡️ Proposed fix
it('does not call detectPasteType when forcedType is provided', () => {
const spy = jest.spyOn(SmartPasteService, 'detectPasteType')
const payload = {
html: null,
text: '# Heading\n\nContent',
types: ['text/plain'],
}
- SmartPasteService.resolvePaste(payload, undefined, 'markdown')
- expect(spy).not.toHaveBeenCalled()
- spy.mockRestore()
+ try {
+ SmartPasteService.resolvePaste(payload, undefined, 'markdown')
+ expect(spy).not.toHaveBeenCalled()
+ } finally {
+ spy.mockRestore()
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('does not call detectPasteType when forcedType is provided', () => { | |
| const spy = jest.spyOn(SmartPasteService, 'detectPasteType') | |
| const payload = { | |
| html: null, | |
| text: '# Heading\n\nContent', | |
| types: ['text/plain'], | |
| } | |
| SmartPasteService.resolvePaste(payload, undefined, 'markdown') | |
| expect(spy).not.toHaveBeenCalled() | |
| spy.mockRestore() | |
| }) | |
| it('does not call detectPasteType when forcedType is provided', () => { | |
| const spy = jest.spyOn(SmartPasteService, 'detectPasteType') | |
| const payload = { | |
| html: null, | |
| text: '# Heading\n\nContent', | |
| types: ['text/plain'], | |
| } | |
| try { | |
| SmartPasteService.resolvePaste(payload, undefined, 'markdown') | |
| expect(spy).not.toHaveBeenCalled() | |
| } finally { | |
| spy.mockRestore() | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/mobile/tests/unit/core-services-smartPaste.test.ts` around lines 545 -
556, Wrap the spy usage in a try/finally to guarantee cleanup: create the spy
via jest.spyOn(SmartPasteService, 'detectPasteType'), then call
SmartPasteService.resolvePaste(...) and assert
expect(spy).not.toHaveBeenCalled() inside a try block, and call
spy.mockRestore() in the finally block so the spy is always restored even if the
assertion fails; reference the test that uses SmartPasteService.resolvePaste and
the spy on detectPasteType.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/mobile/tests/component/editorWebViewMessages.test.tsx (1)
1-4:⚠️ Potential issue | 🟡 MinorStale file-level comment — update to reflect SELECTION_CHANGE coverage.
Lines 1–4 state the file "Specifically tests CONTENT_ON_BLUR safety net feature," which no longer reflects the full scope now that SELECTION_CHANGE tests are included.
📝 Proposed fix
/** * Tests for EditorWebView message handling - * Specifically tests CONTENT_ON_BLUR safety net feature + * Covers CONTENT_ON_BLUR safety net, chunked transfer, SELECTION_CHANGE, and EDITOR_BLUR message handling */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mobile/tests/component/editorWebViewMessages.test.tsx` around lines 1 - 4, Update the stale file-level comment at the top of editorWebViewMessages.test.tsx (lines referencing the file purpose) to reflect that the tests now cover both the CONTENT_ON_BLUR safety-net feature and SELECTION_CHANGE message handling; change the phrase "Specifically tests CONTENT_ON_BLUR safety net feature" to mention both CONTENT_ON_BLUR and SELECTION_CHANGE so the file header accurately describes the current test coverage.
♻️ Duplicate comments (1)
docs/ai/testing/feature-force-paste-format.md (1)
61-61:⚠️ Potential issue | 🟡 MinorCapitalize "Markdown" consistently as a proper noun (lines 61, 75, 116, 129).
"markdown" (lowercase) appears at these four locations. It should be "Markdown" to match its usage as a proper noun throughout the rest of the document.
✏️ Proposed fix
-**Plain text no crash:** text without markdown syntax → no crash, content preserved. +**Plain text no crash:** text without Markdown syntax → no crash, content preserved.-- [x] **Web flow:** select plain markdown text in editor → click "Apply as Markdown" → ... +- [x] **Web flow:** select plain Markdown text in editor → click "Apply as Markdown" → ...-- [ ] Select low-score markdown text → click button → correct rendering (headings, lists, code blocks) +- [ ] Select low-score Markdown text → click button → correct rendering (headings, lists, code blocks)-- [ ] Selection with no markdown syntax → result looks same as before (plain paragraph), no crash +- [ ] Selection with no Markdown syntax → result looks same as before (plain paragraph), no crashAlso applies to: 75-75, 116-116, 129-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai/testing/feature-force-paste-format.md` at line 61, Update the four occurrences of the lowercase word "markdown" to the proper noun "Markdown" in this document: change the instance in the checklist item "- [x] **Plain text no crash:** text without markdown syntax → no crash, content preserved." and the other three occurrences currently at the same locations referenced in the review (lines 75, 116, 129) so all four read "Markdown"; ensure only the word casing is changed and surrounding punctuation/formatting remains unchanged.
🧹 Nitpick comments (5)
ui/mobile/tests/component/editorWebViewMessages.test.tsx (1)
213-261: Consider adding a null/undefined payload edge-case test forSELECTION_CHANGE.The
CONTENT_ON_BLURsuite has explicit null/empty payload tests (lines 112–129) to guard against unexpected coercions.SELECTION_CHANGEcurrently lacks equivalent coverage — if the web layer ever sendsnullorundefinedas the payload, it's unclear whether the component silently ignores it or forwards a falsy value toonSelectionChange. A single extra test case would close that gap.🧪 Suggested additional test
+ it('does not call onSelectionChange when payload is null', async () => { + const onSelectionChange = jest.fn() + + render( + <EditorWebView + initialContent="" + onSelectionChange={onSelectionChange} + /> + ) + + await waitFor(() => { + expect(capturedOnMessage).not.toBeNull() + }) + + sendMessage('SELECTION_CHANGE', null) + + // Adjust the expectation to match the component's actual null-handling contract + expect(onSelectionChange).not.toHaveBeenCalled() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mobile/tests/component/editorWebViewMessages.test.tsx` around lines 213 - 261, Add a test in the 'SELECTION_CHANGE handling' suite that sends a null (and/or undefined) payload via sendMessage('SELECTION_CHANGE', null) (and sendMessage('SELECTION_CHANGE', undefined)) and assert it does not throw and does not call the provided onSelectionChange handler; locate this near the existing SELECTION_CHANGE tests using EditorWebView, capturedOnMessage, sendMessage and onSelectionChange to mirror the CONTENT_ON_BLUR null/empty payload coverage.cypress/component/editor/RichTextEditorApplyMarkdown.cy.tsx (2)
49-96: Consider extracting the repeated “select all + apply” flow.This sequence is repeated across multiple tests and can be centralized for readability and easier updates.
♻️ Suggested extraction
+function selectAllAndApplyMarkdown() { + cy.get('[data-cy="editor-content"]').click() + cy.get('.ProseMirror').type('{selectall}') + cy.get('[data-cy="apply-markdown-button"]').click() +}- cy.get('[data-cy="editor-content"]').click() - cy.get('.ProseMirror').type('{selectall}') - cy.get('[data-cy="apply-markdown-button"]').click() + selectAllAndApplyMarkdown()As per coding guidelines: "Follow the project's established code style and conventions with clear, self-documenting code using meaningful variable names".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/component/editor/RichTextEditorApplyMarkdown.cy.tsx` around lines 49 - 96, Tests repeat the "select all + apply markdown" steps; extract them into a helper to improve readability and DRY. Create a utility function (e.g., selectAllAndApply or applyMarkdownToMountedEditor) and use it in tests that call mountEditor and then perform cy.get('.ProseMirror').type('{selectall}') and cy.get('[data-cy="apply-markdown-button"]').click(); replace the repeated three-line sequence in each spec with the new helper and update any tests referencing the DOM selectors ('.ProseMirror' and '[data-cy="apply-markdown-button"]') to use that helper.
80-88: StrengthenonContentChangeassertion to avoid false positives.The current check only verifies it was called at some point. Add a precondition before the action so the test proves the click path triggers it.
✅ Proposed refinement
it('calls onContentChange after applying markdown', () => { mountEditor('<p>- Item one\n- Item two</p>') + cy.get('@onChange').should('not.have.been.called') cy.get('[data-cy="editor-content"]').click() cy.get('.ProseMirror').type('{selectall}') cy.get('[data-cy="apply-markdown-button"]').click() cy.get('@onChange').should('have.been.called') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/component/editor/RichTextEditorApplyMarkdown.cy.tsx` around lines 80 - 88, The test currently only asserts that the onContentChange spy was called at some point; update the test in RichTextEditorApplyMarkdown.cy.tsx to set a clear precondition (e.g., alias the onContentChange spy via mountEditor or stub it and assert it has not been called yet) before triggering the markdown button, then perform the click on '[data-cy="apply-markdown-button"]' and assert that onContentChange was called as a result (use .should('have.been.calledOnce') or compare call counts before/after). Ensure you reference the existing mountEditor helper and the onContentChange alias used in the test so the assertion verifies the click path triggers onContentChange.docs/ai/testing/feature-force-paste-format.md (2)
108-108: Avoid targetingSmartPasteService._resolve()as a named test surface.Listing a private helper (
_resolve) in the coverage-path checklist encourages tests to reach into private implementation details. Prefer expressing the coverage goal in terms of the public API (resolvePastewithforcedType) and rely on coverage tooling to confirm the private branch is exercised indirectly.✏️ Proposed revision
- - `SmartPasteService._resolve()` private helper + - `SmartPasteService.resolvePaste()` forced-type branch (covers internal helpers transitively)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai/testing/feature-force-paste-format.md` at line 108, Replace the checklist entry that targets the private helper SmartPasteService._resolve() with a public-API focused item: test the behavior of SmartPasteService.resolvePaste(...) when invoked with a forcedType and verify expected outputs/side-effects; rely on coverage tooling to confirm that internal branches in _resolve are exercised rather than naming the private method directly. Ensure the checklist text and any examples reference resolvePaste and forcedType (and not _resolve), and keep assertions at the public method level.
77-79: Three E2E test cases are still unchecked — track as known gaps.Mobile flow, undo, and regression-paste checks remain
[ ]. These are valid open items, but the document's coverage section (line 109) should explicitly list them as outstanding gaps rather than leaving them as silent to-dos in the checklist.Would you like me to draft the gap entries for the "Test Reporting & Coverage" section, or open a new issue to track these scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai/testing/feature-force-paste-format.md` around lines 77 - 79, The three unchecked E2E test checklist items ("Mobile flow" mobile toolbar button, "Undo" Ctrl+Z restoring plain text, and "Regression — normal paste" auto-detection) must be moved from the inline checklist into the "Test Reporting & Coverage" section as explicit outstanding gaps; update the "Test Reporting & Coverage" section to list each gap by name, expected behavior, and a short note that they are known untested scenarios, and add a TODO or issue reference for tracking; ensure the section clearly marks these three items as open gaps rather than leaving them as silent checklist items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypress/component/editor/RichTextEditorApplyMarkdown.cy.tsx`:
- Around line 33-39: Test title and keystroke are inconsistent: update the spec
inside the it block to make them match by either changing the test name in the
it(...) call to reflect pressing End, or (preferably) send the Escape key with
cy.get('.ProseMirror').type('{esc}') so the test title "becomes disabled again
after deselecting (pressing Escape)" aligns with the action; adjust the .type
call that currently sends '{end}' to '{esc}' and keep existing helpers like
mountEditor, '.ProseMirror', and '[data-cy="apply-markdown-button"]' unchanged.
---
Outside diff comments:
In `@ui/mobile/tests/component/editorWebViewMessages.test.tsx`:
- Around line 1-4: Update the stale file-level comment at the top of
editorWebViewMessages.test.tsx (lines referencing the file purpose) to reflect
that the tests now cover both the CONTENT_ON_BLUR safety-net feature and
SELECTION_CHANGE message handling; change the phrase "Specifically tests
CONTENT_ON_BLUR safety net feature" to mention both CONTENT_ON_BLUR and
SELECTION_CHANGE so the file header accurately describes the current test
coverage.
---
Duplicate comments:
In `@docs/ai/testing/feature-force-paste-format.md`:
- Line 61: Update the four occurrences of the lowercase word "markdown" to the
proper noun "Markdown" in this document: change the instance in the checklist
item "- [x] **Plain text no crash:** text without markdown syntax → no crash,
content preserved." and the other three occurrences currently at the same
locations referenced in the review (lines 75, 116, 129) so all four read
"Markdown"; ensure only the word casing is changed and surrounding
punctuation/formatting remains unchanged.
---
Nitpick comments:
In `@cypress/component/editor/RichTextEditorApplyMarkdown.cy.tsx`:
- Around line 49-96: Tests repeat the "select all + apply markdown" steps;
extract them into a helper to improve readability and DRY. Create a utility
function (e.g., selectAllAndApply or applyMarkdownToMountedEditor) and use it in
tests that call mountEditor and then perform
cy.get('.ProseMirror').type('{selectall}') and
cy.get('[data-cy="apply-markdown-button"]').click(); replace the repeated
three-line sequence in each spec with the new helper and update any tests
referencing the DOM selectors ('.ProseMirror' and
'[data-cy="apply-markdown-button"]') to use that helper.
- Around line 80-88: The test currently only asserts that the onContentChange
spy was called at some point; update the test in
RichTextEditorApplyMarkdown.cy.tsx to set a clear precondition (e.g., alias the
onContentChange spy via mountEditor or stub it and assert it has not been called
yet) before triggering the markdown button, then perform the click on
'[data-cy="apply-markdown-button"]' and assert that onContentChange was called
as a result (use .should('have.been.calledOnce') or compare call counts
before/after). Ensure you reference the existing mountEditor helper and the
onContentChange alias used in the test so the assertion verifies the click path
triggers onContentChange.
In `@docs/ai/testing/feature-force-paste-format.md`:
- Line 108: Replace the checklist entry that targets the private helper
SmartPasteService._resolve() with a public-API focused item: test the behavior
of SmartPasteService.resolvePaste(...) when invoked with a forcedType and verify
expected outputs/side-effects; rely on coverage tooling to confirm that internal
branches in _resolve are exercised rather than naming the private method
directly. Ensure the checklist text and any examples reference resolvePaste and
forcedType (and not _resolve), and keep assertions at the public method level.
- Around line 77-79: The three unchecked E2E test checklist items ("Mobile flow"
mobile toolbar button, "Undo" Ctrl+Z restoring plain text, and "Regression —
normal paste" auto-detection) must be moved from the inline checklist into the
"Test Reporting & Coverage" section as explicit outstanding gaps; update the
"Test Reporting & Coverage" section to list each gap by name, expected behavior,
and a short note that they are known untested scenarios, and add a TODO or issue
reference for tracking; ensure the section clearly marks these three items as
open gaps rather than leaving them as silent checklist items.
In `@ui/mobile/tests/component/editorWebViewMessages.test.tsx`:
- Around line 213-261: Add a test in the 'SELECTION_CHANGE handling' suite that
sends a null (and/or undefined) payload via sendMessage('SELECTION_CHANGE',
null) (and sendMessage('SELECTION_CHANGE', undefined)) and assert it does not
throw and does not call the provided onSelectionChange handler; locate this near
the existing SELECTION_CHANGE tests using EditorWebView, capturedOnMessage,
sendMessage and onSelectionChange to mirror the CONTENT_ON_BLUR null/empty
payload coverage.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cypress/component/editor/RichTextEditorApplyMarkdown.cy.tsxdocs/ai/testing/feature-force-paste-format.mdui/mobile/tests/component/editorToolbar.test.tsxui/mobile/tests/component/editorWebViewMessages.test.tsx
Comment on lines
+33
to
+39
| it('becomes disabled again after deselecting (pressing Escape)', () => { | ||
| mountEditor('<p>Some text</p>') | ||
| cy.get('[data-cy="editor-content"]').click() | ||
| cy.get('.ProseMirror').type('{selectall}') | ||
| cy.get('[data-cy="apply-markdown-button"]').should('not.be.disabled') | ||
| cy.get('.ProseMirror').type('{end}') | ||
| cy.get('[data-cy="apply-markdown-button"]').should('be.disabled') |
There was a problem hiding this comment.
Align test title with the actual key used.
The test name says “pressing Escape,” but Line 38 sends {end}. This makes failures harder to interpret.
✏️ Proposed fix
- it('becomes disabled again after deselecting (pressing Escape)', () => {
+ it('becomes disabled again after deselecting (pressing End)', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('becomes disabled again after deselecting (pressing Escape)', () => { | |
| mountEditor('<p>Some text</p>') | |
| cy.get('[data-cy="editor-content"]').click() | |
| cy.get('.ProseMirror').type('{selectall}') | |
| cy.get('[data-cy="apply-markdown-button"]').should('not.be.disabled') | |
| cy.get('.ProseMirror').type('{end}') | |
| cy.get('[data-cy="apply-markdown-button"]').should('be.disabled') | |
| it('becomes disabled again after deselecting (pressing End)', () => { | |
| mountEditor('<p>Some text</p>') | |
| cy.get('[data-cy="editor-content"]').click() | |
| cy.get('.ProseMirror').type('{selectall}') | |
| cy.get('[data-cy="apply-markdown-button"]').should('not.be.disabled') | |
| cy.get('.ProseMirror').type('{end}') | |
| cy.get('[data-cy="apply-markdown-button"]').should('be.disabled') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/component/editor/RichTextEditorApplyMarkdown.cy.tsx` around lines 33
- 39, Test title and keystroke are inconsistent: update the spec inside the it
block to make them match by either changing the test name in the it(...) call to
reflect pressing End, or (preferably) send the Escape key with
cy.get('.ProseMirror').type('{esc}') so the test title "becomes disabled again
after deselecting (pressing Escape)" aligns with the action; adjust the .type
call that currently sends '{end}' to '{esc}' and keep existing helpers like
mountEditor, '.ProseMirror', and '[data-cy="apply-markdown-button"]' unchanged.
This was referenced Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests