feat: table editor dialog for visual markdown table editing#5680
Open
milvasic wants to merge 12 commits intousememos:mainfrom
Open
feat: table editor dialog for visual markdown table editing#5680milvasic wants to merge 12 commits intousememos:mainfrom
milvasic wants to merge 12 commits intousememos:mainfrom
Conversation
Contributor
Author
|
Oh gosh… I ran go linter, but forget to run fe linter. @boojack How about adding a pre-commit hooks that run all linters? |
Member
Aha, I think it's better to update your local |
Contributor
Author
|
@boojack I have no local AGENTS.md or anything similar. 😄 |
milvasic
force-pushed
the
feat/table-editor
branch
from
f0701d8 to
1fb94a1
Compare
Contributor
Author
|
@boojack Can you run the tests again? |
milvasic
force-pushed
the
feat/table-editor
branch
from
1fb94a1 to
76f9098
Compare
memoclaw
reviewed
Mar 8, 2026
Contributor
memoclaw
left a comment
memoclaw
left a comment
There was a problem hiding this comment.
Code Review
Thanks for the feature — the UX looks polished and the visual table editor is a nice addition. A few issues to address before this is ready to merge:
Bugs
1. replaceNthTable off-by-one on end index
In utils/markdown-table.ts, findAllTables sets end = offset - 1 (the index of the last character), but replaceNthTable uses content.slice(table.end) which includes that last character in the output. This will produce corrupted markdown on every edit/delete. Fix: use content.slice(table.end + 1), or make end exclusive.
2. DOM-based table index resolution is fragile
Table.tsx resolves the table index by querying container.querySelectorAll("table") and matching against findAllTables() on the raw markdown. This assumes a 1:1 correspondence between rendered <table> elements and markdown tables, and depends on a specific CSS class selector ('[class*="wrap-break-word"]'). If the markdown renderer adds extra tables or the class changes, this silently breaks. Consider using the node prop from ReactMarkdownProps (which carries source position from the AST) instead.
Architecture
3. Display component directly mutates memo content
MemoContent/Table.tsx now calls updateMemo to edit/delete tables from the rendered view. This is a significant change — a display component now has write access. If the editor is also open, this could cause stale state. Consider routing edit/delete through the editor or at minimum invalidating editor state after mutation.
4. Two separate TableEditorDialog instances
InsertMenu creates its own dialog (inserting via props.onInsertText), while MemoEditor/index.tsx creates another for the slash command (inserting via editorRef). These could be unified — have InsertMenu call onOpenTableEditor from the parent instead of managing its own dialog.
Other
5. No i18n — All user-facing strings ("Add row", "Add column", "Insert column", header placeholders, etc.) are hardcoded English. The project uses useTranslate() / t() elsewhere.
6. useCallback depends on entire props object — In InsertMenu.tsx, handleTableConfirm has [props] as dependency, which defeats memoization since props is a new object every render. Should be [props.onInsertText].
7. Array index as React key for rows — When rows are inserted, deleted, or sorted, index-based keys (key={rowIdx}) can cause React to reuse inputs incorrectly. Consider stable row IDs.
8. No tests for markdown-table.ts — The parsing/serialization logic has non-trivial edge cases (alignment, empty cells, escaped pipes). This utility would really benefit from test coverage.
9. Hardcoded monospace font stack — MONO_FONT should ideally use the project's theme tokens or CSS variables.
Items 1–2 are likely to cause data corruption and should be fixed before merge. Items 3–4 are architectural concerns worth discussing. The rest are improvements that would strengthen the PR.
milvasic
force-pushed
the
feat/table-editor
branch
from
76f9098 to
b5da62c
Compare
Contributor
Author
|
@boojack All comments has been addressed. |
memoclaw
reviewed
Mar 12, 2026
Contributor
memoclaw
left a comment
memoclaw
left a comment
There was a problem hiding this comment.
Thanks for working on this — a visual table editor is a great feature for Memos!
A few thoughts on the current implementation:
Scope: This is a large changeset (~1200 lines, 15 files) that bundles several concerns together — markdown table utilities, a new dialog component, inline edit/delete on rendered tables, slash command changes, toolbar changes, and test infrastructure setup (vitest). Splitting this into smaller, focused PRs would make it easier to review and merge incrementally.
Architecture:
Table.tsxwent from a simple display component to handling edit/delete state and memo mutations. Separating the action logic from the rendering would keep things cleaner.TableEditorDialog.tsx(422 lines) could benefit from extracting the keyboard navigation and sorting logic into separate hooks.
Potential issues:
sortByColumnreadsrows/rowIdsfrom the closure instead of using functional state updaters, which can lead to stale state.- No column/row limits — users can add indefinitely, which may break the dialog layout.
- Only
en.jsonhas the new translation keys.
The markdown table utility layer (markdown-table.ts) and its tests are solid — that alone could land as a standalone first PR.
Contributor
Author
|
@memoclaw Thanks for the suggestion. At this stage I’m not planning to split this PR further—the earlier review round is addressed and splitting now would add a lot of churn and reset review context. @boojack FYI |
Contributor
Author
|
@boojack any comment? |
milvasic
force-pushed
the
feat/table-editor
branch
from
84cd51e to
b7e1291
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds end-to-end markdown table support: new parsing/serialization utilities and tests, a TableEditorDialog UI, editor integrations and slash/insert-menu wiring, rendered-table edit/delete controls, localization keys, and Vitest test scripts/dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor as MemoEditor
participant Dialog as TableEditorDialog
participant Utils as Markdown-Table-Utils
participant Store as Memo Store
User->>Editor: trigger "table" (insert menu or /table)
Editor->>Dialog: open(initialData=null)
User->>Dialog: edit cells / add/remove / sort / Tab-nav
User->>Dialog: Confirm
Dialog->>Utils: serializeMarkdownTable(tableData)
Utils-->>Dialog: markdown string
Dialog->>Editor: onConfirm(markdown)
Editor->>Editor: insertText(markdown)
Editor->>Store: save updated content
Store-->>Editor: persist result
Editor->>User: updated memo displayed
sequenceDiagram
participant Reader as Rendered Memo
participant Table as Table Component
participant Utils as Markdown-Table-Utils
participant Dialog as TableEditorDialog
participant Store as Memo Store
Reader->>Table: render with AST node
Table->>Utils: findAllTables(content)
Table->>Table: compute index for node
User->>Table: click Edit
Table->>Utils: parseMarkdownTable(tableText)
Utils-->>Table: TableData
Table->>Dialog: open(initialData=TableData)
User->>Dialog: modify and Confirm
Dialog->>Utils: serializeMarkdownTable(modified)
Utils-->>Dialog: new markdown
Table->>Utils: replaceNthTable(content, index, newMarkdown)
Utils-->>Table: updated content
Table->>Store: useUpdateMemo(updatedContent)
Store-->>Table: persist result
Table->>User: updated memo displayed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/MemoContent/Table.tsx`:
- Around line 105-123: Toolbar actions are hidden via "opacity-0" and only shown
on "group-hover/table", which makes them inaccessible on touch devices; update
the wrapper DIV that contains the edit/delete buttons (the element that
currently uses className "absolute top-1.5 right-1.5 ... opacity-0
group-hover/table:opacity-100") to provide a persistent low-opacity default
(e.g., opacity-30) and also reveal on keyboard focus by adding a
"group-focus-within" variant alongside "group-hover/table", ensuring the buttons
tied to handleDeleteClick and handleEditClick remain discoverable on touch and
keyboard navigation.
- Around line 76-97: The handlers handleConfirmEdit and handleConfirmDelete
currently call updateMemo (from useUpdateMemo()) without awaiting or handling
errors; change them to call updateMemo.mutateAsync with await (or use the
promise returned) and handle failures via try/catch or
updateMemo.mutateAsync(...).catch/onError so you only close UI (e.g., call
setDeleteDialogOpen(false)) after a successful save, and surface errors to the
user (e.g., set an error state or trigger a toast) on failure to allow retry;
keep the same payload shape ({ update: { name: memo.name, content: newContent },
updateMask: ["content"] }) and preserve optimistic rollback behavior provided by
useUpdateMemo.
In `@web/src/components/TableEditorDialog.tsx`:
- Around line 130-150: The numeric sort in sortByColumn breaks on formatted
values and blanks because compareFn uses Number(...) directly; update compareFn
in sortByColumn to first normalize cell strings (trim, strip currency symbols,
commas, whitespace and other non-numeric characters except leading - and decimal
point, e.g. via a regex) into cleaned strings, treat an empty cleaned string as
non-numeric (so blank cells are not interpreted as 0), then use parseFloat on
the cleaned string and Number.isFinite to detect numeric values; if both values
are numeric compare numerically, if only one is numeric treat the numeric one as
greater/less according to newDir, otherwise fall back to localeCompare on the
original lowercase strings; keep the rest of the logic that builds indices and
sets rows/rowIds unchanged.
- Around line 154-177: handleKeyDown currently prevents native Tab at all times
which traps keyboard users; update handleKeyDown so it only prevents default and
redirects focus when the Tab target is another cell inside the grid, but lets
the browser handle Tab at the grid boundaries. Concretely: in handleKeyDown,
compute nextCol/nextRow as now; if Tab forward from the last cell (row ===
rowCount - 1 && col === colCount - 1) or Shift+Tab from the first cell (row ===
0 && col === 0) then do not call e.preventDefault() and do not call addRow or
focusCell (allow native focus to move out); otherwise keep the existing
preventDefault + focusCell behavior (and remove the addRow-on-last-cell
auto-creation branch so Tab doesn’t trap users). Reference symbols:
handleKeyDown, focusCell, addRow, colCount, rowCount.
In `@web/src/utils/markdown-table.ts`:
- Around line 35-40: parseRow currently splits on every "|" and
serializeMarkdownTable writes cells verbatim causing cell text with "|" to
create extra columns; change parseRow to split only on unescaped "|" (i.e.,
treat "\|" as part of the cell) and change serializeMarkdownTable to escape any
"|" in cell text as "\|" when writing, adding corresponding unescape logic so
escaped pipes round-trip; update the related code paths in
serializeMarkdownTable and any helper functions used between parseRow and
serializeMarkdownTable (also the code in the block around the other
parse/serialize logic referenced in the diff) and add a regression test that
rounds trips a cell containing "foo|bar" and an already-escaped "\|" case to
ensure no extra columns are produced.
- Around line 134-175: findAllTables currently uses TABLE_LINE regex that only
matches lines with outer pipes, so GFM tables without leading/trailing pipes are
missed; replace the regex scan with a proper markdown AST lookup: use the
project's markdown parser (e.g., remark/markdown-it) to parse the content, find
table nodes (and their position/start/end offsets), extract the original text
slice and return TableMatch entries; update findAllTables to call the parser,
iterate over table nodes (table or tableRow nodes depending on parser), use
node.position.start.offset and node.position.end.offset (or compute offsets from
line/column) to populate start/end and text, and remove reliance on TABLE_LINE;
add a regression test that asserts a pipe-less table like "A | B\n--- | ---\n1 |
2" is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7008f51b-6ddd-4571-90c6-6600f4580f50
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
web/package.jsonweb/src/components/MemoContent/Table.tsxweb/src/components/MemoContent/index.tsxweb/src/components/MemoEditor/Editor/SlashCommands.tsxweb/src/components/MemoEditor/Editor/commands.tsweb/src/components/MemoEditor/Editor/index.tsxweb/src/components/MemoEditor/Toolbar/InsertMenu.tsxweb/src/components/MemoEditor/components/EditorContent.tsxweb/src/components/MemoEditor/components/EditorToolbar.tsxweb/src/components/MemoEditor/index.tsxweb/src/components/MemoEditor/types/components.tsweb/src/components/TableEditorDialog.tsxweb/src/locales/en.jsonweb/src/utils/markdown-table.test.tsweb/src/utils/markdown-table.ts
milvasic
force-pushed
the
feat/table-editor
branch
from
b7e1291 to
14fa3f1
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
web/src/utils/markdown-table.ts (2)
35-40:⚠️ Potential issue | 🟠 MajorEscape and unescape
|in cell content to prevent table corruption.Line 40 splits on every pipe and serialization writes cell text verbatim, so values like
foo|barbecome extra columns after save/reopen.Proposed fix
+const splitOnUnescapedPipe = (line: string): string[] => { + const out: string[] = []; + let current = ""; + let escaped = false; + for (const ch of line) { + if (escaped) { + current += ch; + escaped = false; + continue; + } + if (ch === "\\") { + current += ch; + escaped = true; + continue; + } + if (ch === "|") { + out.push(current); + current = ""; + continue; + } + current += ch; + } + out.push(current); + return out; +}; + +const unescapeCell = (value: string): string => value.replace(/\\\|/g, "|").replace(/\\\\/g, "\\"); +const escapeCell = (value: string): string => value.replace(/\\/g, "\\\\").replace(/\|/g, "\\|"); + const parseRow = (line: string): string[] => { let trimmed = line; if (trimmed.startsWith("|")) trimmed = trimmed.slice(1); if (trimmed.endsWith("|")) trimmed = trimmed.slice(0, -1); - return trimmed.split("|").map((cell) => cell.trim()); + return splitOnUnescapedPipe(trimmed).map((cell) => unescapeCell(cell.trim())); }; @@ const padCell = (text: string, width: number, align: ColumnAlignment): string => { - const t = text || ""; + const t = escapeCell(text || "");Also applies to: 93-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/utils/markdown-table.ts` around lines 35 - 40, The parser currently splits table rows on every "|" in parseRow, and the serializer writes cell text verbatim, so cell content containing "|" (e.g., "foo|bar") is misinterpreted as extra columns; update parseRow to split on unescaped pipes (ignore pipes preceded by a backslash) and unescape escaped pipes in returned cell strings, and update the table serialization logic (the function that builds/joins cells — e.g., serializeTable / buildRow around lines 93-127) to escape any "|" in cell content (replace "|" with "\|") before joining with " | " so round-trip preserves cell text. Ensure both escape and unescape use the same backslash convention and handle leading/trailing pipe trimming as before.
134-175:⚠️ Potential issue | 🟠 Major
findAllTablesstill misses valid pipe-less GFM tables.Line 156 only matches rows that start/end with
|, soA | Bstyle tables are not discoverable; edit/delete mapping can no-op for valid rendered tables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/utils/markdown-table.ts` around lines 134 - 175, The TABLE_LINE regex used by findAllTables only matches rows that start/end with '|' so it misses valid GFM tables like "A | B"; update the table-detection logic by replacing the TABLE_LINE pattern so it matches any line containing at least one column pipe (i.e., allow pipes anywhere in the line, not only at edges) and ensure the separator/header detection still requires a valid separator row (hyphens and pipes) when deciding tables; adjust the TABLE_LINE constant and any related separator test in findAllTables (referencing TABLE_LINE and findAllTables) so offsets/joins remain correct and pipe-less-edge tables are included.web/src/components/TableEditorDialog.tsx (2)
154-177:⚠️ Potential issue | 🟠 MajorTab handling still traps keyboard users inside the table grid.
Line 156 always prevents native Tab behavior, so users cannot tab out to footer actions.
Proposed fix
const handleKeyDown = (e: React.KeyboardEvent<HTMLInputElement>, row: number, col: number) => { if (e.key !== "Tab") return; - e.preventDefault(); + const isForwardBoundary = !e.shiftKey && row === rowCount - 1 && col === colCount - 1; + const isBackwardBoundary = e.shiftKey && row === -1 && col === 0; + if (isForwardBoundary || isBackwardBoundary) return; // let browser move focus out + + e.preventDefault(); const nextCol = e.shiftKey ? col - 1 : col + 1; let nextRow = row; if (nextCol >= colCount) { - if (row < rowCount - 1) { - nextRow = row + 1; - focusCell(nextRow, 0); - } else { - addRow(); - setTimeout(() => focusCell(rowCount, 0), 0); - } + nextRow = row + 1; + focusCell(nextRow, 0); } else if (nextCol < 0) { if (row > 0) { nextRow = row - 1; focusCell(nextRow, colCount - 1); } else { - focusCell(-1, colCount - 1); + focusCell(-1, colCount - 1); } } else { focusCell(nextRow, nextCol); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/TableEditorDialog.tsx` around lines 154 - 177, The Tab handler in handleKeyDown currently always calls e.preventDefault(), which traps keyboard users; compute the intended target cell first (using row, col, colCount, rowCount, and shiftKey) and only call e.preventDefault() when the Tab should keep focus inside the grid or when you programmatically add a row (i.e., when you will call focusCell(...) or addRow() then focusCell). If the computed target falls outside the grid bounds (e.g., Shift+Tab from first cell or Tab from last cell when you do not add a new row), do not prevent default so native tabbing moves focus out to footer actions; keep using focusCell(...), addRow(), rowCount and colCount for the internal-focus cases.
135-143:⚠️ Potential issue | 🟠 MajorNumeric sorting still breaks for blanks/currency/comma-formatted values.
Line 138/139 use
Number(...), so""becomes0and values like$100or1,000becomeNaN, leading to incorrect order.Proposed fix
+const parseSortableNumber = (raw: string): number | null => { + const cleaned = raw.trim().replace(/[^\d.-]/g, ""); + if (!cleaned || cleaned === "-" || cleaned === "." || cleaned === "-.") return null; + const n = Number.parseFloat(cleaned); + return Number.isFinite(n) ? n : null; +}; + const compareFn = (a: string[], b: string[]) => { const va = (a[col] || "").toLowerCase(); const vb = (b[col] || "").toLowerCase(); - const na = Number(va); - const nb = Number(vb); - if (!Number.isNaN(na) && !Number.isNaN(nb)) return newDir === "asc" ? na - nb : nb - na; + const na = parseSortableNumber(va); + const nb = parseSortableNumber(vb); + if (na !== null && nb !== null) return newDir === "asc" ? na - nb : nb - na; + if (na !== null) return newDir === "asc" ? 1 : -1; + if (nb !== null) return newDir === "asc" ? -1 : 1; const cmp = va.localeCompare(vb); return newDir === "asc" ? cmp : -cmp; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/TableEditorDialog.tsx` around lines 135 - 143, The compareFn is treating blank strings and currency/comma-formatted values incorrectly because it calls Number(...) on the raw cell text (va/vb); modify compareFn to first normalize strings before numeric parsing: trim whitespace, strip currency symbols and grouping commas (e.g. remove anything except digits, minus and decimal point), then if the cleaned string is empty treat it as non-numeric (NaN) so it falls back to localeCompare; parse with parseFloat and check numeric validity with Number.isFinite (or !Number.isNaN) on na/nb, and only use numeric subtraction when both are valid numbers (otherwise use localeCompare), preserving the newDir asc/desc branching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/src/components/TableEditorDialog.tsx`:
- Around line 154-177: The Tab handler in handleKeyDown currently always calls
e.preventDefault(), which traps keyboard users; compute the intended target cell
first (using row, col, colCount, rowCount, and shiftKey) and only call
e.preventDefault() when the Tab should keep focus inside the grid or when you
programmatically add a row (i.e., when you will call focusCell(...) or addRow()
then focusCell). If the computed target falls outside the grid bounds (e.g.,
Shift+Tab from first cell or Tab from last cell when you do not add a new row),
do not prevent default so native tabbing moves focus out to footer actions; keep
using focusCell(...), addRow(), rowCount and colCount for the internal-focus
cases.
- Around line 135-143: The compareFn is treating blank strings and
currency/comma-formatted values incorrectly because it calls Number(...) on the
raw cell text (va/vb); modify compareFn to first normalize strings before
numeric parsing: trim whitespace, strip currency symbols and grouping commas
(e.g. remove anything except digits, minus and decimal point), then if the
cleaned string is empty treat it as non-numeric (NaN) so it falls back to
localeCompare; parse with parseFloat and check numeric validity with
Number.isFinite (or !Number.isNaN) on na/nb, and only use numeric subtraction
when both are valid numbers (otherwise use localeCompare), preserving the newDir
asc/desc branching.
In `@web/src/utils/markdown-table.ts`:
- Around line 35-40: The parser currently splits table rows on every "|" in
parseRow, and the serializer writes cell text verbatim, so cell content
containing "|" (e.g., "foo|bar") is misinterpreted as extra columns; update
parseRow to split on unescaped pipes (ignore pipes preceded by a backslash) and
unescape escaped pipes in returned cell strings, and update the table
serialization logic (the function that builds/joins cells — e.g., serializeTable
/ buildRow around lines 93-127) to escape any "|" in cell content (replace "|"
with "\|") before joining with " | " so round-trip preserves cell text. Ensure
both escape and unescape use the same backslash convention and handle
leading/trailing pipe trimming as before.
- Around line 134-175: The TABLE_LINE regex used by findAllTables only matches
rows that start/end with '|' so it misses valid GFM tables like "A | B"; update
the table-detection logic by replacing the TABLE_LINE pattern so it matches any
line containing at least one column pipe (i.e., allow pipes anywhere in the
line, not only at edges) and ensure the separator/header detection still
requires a valid separator row (hyphens and pipes) when deciding tables; adjust
the TABLE_LINE constant and any related separator test in findAllTables
(referencing TABLE_LINE and findAllTables) so offsets/joins remain correct and
pipe-less-edge tables are included.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d145965-ad8e-4ef6-87a1-f2fdf8a0cb39
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
web/package.jsonweb/src/components/MemoContent/Table.tsxweb/src/components/MemoContent/index.tsxweb/src/components/MemoEditor/Editor/SlashCommands.tsxweb/src/components/MemoEditor/Editor/commands.tsweb/src/components/MemoEditor/Editor/index.tsxweb/src/components/MemoEditor/Toolbar/InsertMenu.tsxweb/src/components/MemoEditor/components/EditorContent.tsxweb/src/components/MemoEditor/components/EditorToolbar.tsxweb/src/components/MemoEditor/index.tsxweb/src/components/MemoEditor/types/components.tsweb/src/components/TableEditorDialog.tsxweb/src/locales/en.jsonweb/src/utils/markdown-table.test.tsweb/src/utils/markdown-table.ts
✅ Files skipped from review due to trivial changes (2)
- web/src/locales/en.json
- web/src/utils/markdown-table.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- web/package.json
- web/src/components/MemoEditor/components/EditorToolbar.tsx
- web/src/components/MemoEditor/components/EditorContent.tsx
- web/src/components/MemoEditor/Editor/commands.ts
- web/src/components/MemoEditor/Editor/SlashCommands.tsx
- web/src/components/MemoContent/index.tsx
- web/src/components/MemoEditor/index.tsx
- web/src/components/MemoContent/Table.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
web/src/components/MemoContent/Table.tsx (2)
82-89:⚠️ Potential issue | 🟠 MajorDon't turn a failed edit save into a resolved
onConfirm.This catch toasts and then resolves. If
TableEditorDialogcloses afterawait onConfirm(...), a failed save will still dismiss the editor and lose the retry path. Re-throw afterhandleError(...), or return an explicit success/failure signal that the dialog checks.Run this to verify whether
web/src/components/TableEditorDialog.tsxcloses itself afteronConfirmresolves:#!/bin/bash set -euo pipefail file="$(fd 'TableEditorDialog\.tsx$' web/src | head -n 1)" [ -n "$file" ] || { echo "TableEditorDialog.tsx not found" >&2; exit 1; } rg -n -C4 'await\s+onConfirm|onConfirm\(|onOpenChange\(false\)|setOpen\(false\)|handleConfirm' "$file"
⚠️ Minimal fix if the dialog relies on rejected promises to stay open} catch (error: unknown) { handleError(error, toast.error, { context: "Update table", fallbackMessage: "An error occurred" }); + throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/MemoContent/Table.tsx` around lines 82 - 89, The catch block in the MemoContent Table save path swallows failures (in updateMemo) by only calling handleError and thus makes onConfirm appear to succeed; update the catch in the code that calls updateMemo to either re-throw the caught error after calling handleError(error, ...) or return an explicit failure signal (e.g., false) that the caller (onConfirm/TableEditorDialog) can check; locate the try/catch around updateMemo and adjust it so failures propagate (re-throw) or a clear success/failure value is returned, and ensure TableEditorDialog's onConfirm usage reacts to the failure signal rather than always closing.
116-129:⚠️ Potential issue | 🟠 MajorMake the inline actions discoverable and labeled.
The toolbar still starts at
opacity-0, so touch users and keyboard users can land on effectively invisible controls, andtitlealone is not a reliable accessible name for icon buttons. Keep a low-opacity default, reveal ongroup-focus-within, and addaria-labels.♿ Suggested change
- <div className="absolute top-1.5 right-1.5 flex items-center gap-1 opacity-0 group-hover/table:opacity-100 transition-all"> + <div className="absolute top-1.5 right-1.5 flex items-center gap-1 opacity-40 transition-all group-hover/table:opacity-100 group-focus-within/table:opacity-100"> <button type="button" className="p-1 rounded bg-accent/80 text-muted-foreground hover:bg-destructive/20 hover:text-destructive transition-colors" onClick={handleDeleteClick} + aria-label={t("common.delete")} title={t("common.delete")} > @@ <button type="button" className="p-1 rounded bg-accent/80 text-muted-foreground hover:bg-accent hover:text-foreground transition-colors" onClick={handleEditClick} + aria-label={t("common.edit")} title={t("common.edit")} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/MemoContent/Table.tsx` around lines 116 - 129, Change the inline action toolbar so it is minimally visible by default and fully visible on group hover or when the group receives focus: replace the parent div's initial class from opacity-0 to a low-opacity (e.g., opacity-20) and add group-focus-within/table:opacity-100 alongside the existing group-hover/table:opacity-100; also add explicit accessible names to the icon buttons by adding aria-label attributes (use the same localized strings as title via t("common.delete") and t("common.edit")) on the buttons that call handleDeleteClick and handleEditClick (the button with the TrashIcon and the edit button) so keyboard and touch users can discover and identify the controls.
🧹 Nitpick comments (2)
web/src/components/MemoContent/Table.tsx (2)
13-13: Use the frontend alias for the new context import.This new relative path reintroduces import-style drift in
web/src. Please switch it to@/components/MemoView/MemoViewContext.♻️ Suggested change
-import { useMemoViewContext, useMemoViewDerived } from "../MemoView/MemoViewContext"; +import { useMemoViewContext, useMemoViewDerived } from "@/components/MemoView/MemoViewContext";As per coding guidelines, "Use
@/alias for absolute imports in frontend code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/MemoContent/Table.tsx` at line 13, The import in Table.tsx currently uses a relative path for MemoViewContext; update the import of useMemoViewContext and useMemoViewDerived to use the frontend alias by changing the module specifier to "@/components/MemoView/MemoViewContext" so the file imports useMemoViewContext and useMemoViewDerived from the aliased path instead of the relative path.
35-45: Memoize the table matches once per content string.
findAllTables()reparses the full memo here and again inhandleEditClick. Hoisting that into a memoizedtablesvalue avoids duplicate AST work and keeps the lookup/edit path in sync.♻️ Suggested change
+ const tables = useMemo(() => findAllTables(memo.content), [memo.content]); + /** Resolve which markdown table index this rendered table corresponds to using AST source positions. */ const resolveTableIndex = useMemo(() => { const nodeStart = node?.position?.start?.offset; if (nodeStart == null) return -1; - const tables = findAllTables(memo.content); for (let i = 0; i < tables.length; i++) { if (nodeStart >= tables[i].start && nodeStart < tables[i].end) return i; } return -1; - }, [memo.content, node]); + }, [node, tables]); const handleEditClick = useCallback( (e: React.MouseEvent) => { e.stopPropagation(); e.preventDefault(); - const tables = findAllTables(memo.content); if (resolveTableIndex < 0 || resolveTableIndex >= tables.length) return; const parsed = parseMarkdownTable(tables[resolveTableIndex].text); if (!parsed) return; @@ - [memo.content, resolveTableIndex], + [resolveTableIndex, tables], );Also applies to: 52-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/MemoContent/Table.tsx` around lines 35 - 45, Hoist the result of findAllTables(memo.content) into a memoized variable so the AST is parsed once per memo.content and both resolveTableIndex and handleEditClick use that same cached array; e.g., create a const tables = useMemo(() => findAllTables(memo.content), [memo.content]) and update resolveTableIndex (and any other places like handleEditClick) to reference tables instead of calling findAllTables again, ensuring the lookup/edit paths stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/src/components/MemoContent/Table.tsx`:
- Around line 82-89: The catch block in the MemoContent Table save path swallows
failures (in updateMemo) by only calling handleError and thus makes onConfirm
appear to succeed; update the catch in the code that calls updateMemo to either
re-throw the caught error after calling handleError(error, ...) or return an
explicit failure signal (e.g., false) that the caller
(onConfirm/TableEditorDialog) can check; locate the try/catch around updateMemo
and adjust it so failures propagate (re-throw) or a clear success/failure value
is returned, and ensure TableEditorDialog's onConfirm usage reacts to the
failure signal rather than always closing.
- Around line 116-129: Change the inline action toolbar so it is minimally
visible by default and fully visible on group hover or when the group receives
focus: replace the parent div's initial class from opacity-0 to a low-opacity
(e.g., opacity-20) and add group-focus-within/table:opacity-100 alongside the
existing group-hover/table:opacity-100; also add explicit accessible names to
the icon buttons by adding aria-label attributes (use the same localized strings
as title via t("common.delete") and t("common.edit")) on the buttons that call
handleDeleteClick and handleEditClick (the button with the TrashIcon and the
edit button) so keyboard and touch users can discover and identify the controls.
---
Nitpick comments:
In `@web/src/components/MemoContent/Table.tsx`:
- Line 13: The import in Table.tsx currently uses a relative path for
MemoViewContext; update the import of useMemoViewContext and useMemoViewDerived
to use the frontend alias by changing the module specifier to
"@/components/MemoView/MemoViewContext" so the file imports useMemoViewContext
and useMemoViewDerived from the aliased path instead of the relative path.
- Around line 35-45: Hoist the result of findAllTables(memo.content) into a
memoized variable so the AST is parsed once per memo.content and both
resolveTableIndex and handleEditClick use that same cached array; e.g., create a
const tables = useMemo(() => findAllTables(memo.content), [memo.content]) and
update resolveTableIndex (and any other places like handleEditClick) to
reference tables instead of calling findAllTables again, ensuring the
lookup/edit paths stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4144f527-f845-4710-a941-cf9f3f51efc4
📒 Files selected for processing (4)
web/src/components/MemoContent/Table.tsxweb/src/components/TableEditorDialog.tsxweb/src/utils/markdown-table.test.tsweb/src/utils/markdown-table.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/utils/markdown-table.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/TableEditorDialog.tsx
- web/src/utils/markdown-table.ts
milvasic
added a commit
to milvasic/memos
that referenced
this pull request
Mar 23, 2026
- escapeCell() replaces unescaped | with \| before writing each cell, applied in both width calculation and formatRow so padding is accurate - parseRow unescapes \| back to | after splitting on unescaped pipes - Adds round-trip regression test for cells containing pipe characters Addresses coderabbitai review comment on PR usememos#5680.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/utils/markdown-table.test.ts (1)
1-9: Consider using the@/import alias for consistency.The coding guidelines prefer using
@/alias for imports in frontend code. While relative imports for co-located test files are common, using@/utils/markdown-tablewould align with the project's import conventions.♻️ Suggested change
import { describe, expect, it } from "vitest"; import { createEmptyTable, findAllTables, parseMarkdownTable, replaceNthTable, serializeMarkdownTable, type TableData, -} from "./markdown-table"; +} from "@/utils/markdown-table";As per coding guidelines: "Use
@/alias for absolute imports in frontend code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/utils/markdown-table.test.ts` around lines 1 - 9, The test file imports utilities via a relative path; update the import to use the project alias so frontend imports are consistent—replace the current import from "./markdown-table" with the aliased module "@/utils/markdown-table" while keeping the same named imports (createEmptyTable, findAllTables, parseMarkdownTable, replaceNthTable, serializeMarkdownTable, TableData) so tests continue to reference the same symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/utils/markdown-table.ts`:
- Around line 39-45: The parseRow function's split regex (/((?<!\\)\|)/) treats
any backslash before a pipe as an escape and fails to distinguish an escaped
backslash before a pipe (e.g., "\\|") from an escaped pipe ("\|"), so "\\|"
won't round-trip correctly; update the parseRow implementation to either fix
parsing by splitting on pipes only when preceded by an even number of
backslashes (implement a small character-scan that counts consecutive
backslashes before each '|' and splits when count % 2 == 0) or, if you don't
want to change behavior now, add a clear comment directly above parseRow
documenting this limitation and suggesting the even-backslash scan approach as a
future fix (reference parseRow and the current split/replace logic).
---
Nitpick comments:
In `@web/src/utils/markdown-table.test.ts`:
- Around line 1-9: The test file imports utilities via a relative path; update
the import to use the project alias so frontend imports are consistent—replace
the current import from "./markdown-table" with the aliased module
"@/utils/markdown-table" while keeping the same named imports (createEmptyTable,
findAllTables, parseMarkdownTable, replaceNthTable, serializeMarkdownTable,
TableData) so tests continue to reference the same symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0979d650-0796-4eb4-b0f2-9e595bd10aac
📒 Files selected for processing (2)
web/src/utils/markdown-table.test.tsweb/src/utils/markdown-table.ts
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Add a dialog-based table editor to create and edit markdown tables via a visual grid instead of raw pipe-delimited text. Features: - Visual grid with inputs for headers and cells; add/remove rows and columns - Sort columns (asc/desc, text and numeric); tab navigation (new row at end) - Insert column/row between columns/rows via hover zones and + buttons with blue highlight lines clipped to table bounds - Sticky header with solid background; square headers; monospace cell font - Row numbers with insert zones; delete row at row end; delete column with spacing from insert button; Add row/Add column in footer and below table - Delete table button on rendered tables (with confirm); edit pencil opens dialog with parsed data; always-visible sort/delete at 40% opacity - Fixed-size dialog (56rem x 44rem); /table slash command and Table in InsertMenu open dialog; Command.action support for dialog-based commands New: TableEditorDialog.tsx, utils/markdown-table.ts. Integration in SlashCommands, EditorContent, InsertMenu, MemoContent Table. Made-with: Cursor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web/src/components/MemoContent/Table.tsx (1)
38-46: Minor naming:resolveTableIndexis a value, not a function.This
useMemoreturns a computed index but is named like an imperative action. Consider renaming tocurrentTableIndexormatchedTableIndexfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/MemoContent/Table.tsx` around lines 38 - 46, The variable resolveTableIndex is a memoized value (computed index) but named like a function; rename it (e.g., to currentTableIndex or matchedTableIndex) and update all references to that identifier in the component so the name reflects a value rather than an action; keep the useMemo logic and dependencies ([tables, node]) unchanged and ensure any JSX or other code that used resolveTableIndex now uses the new name.web/src/utils/markdown-table.ts (2)
213-219: Unnecessary type assertion.The
as ColumnAlignmentcast on line 217 is redundant since"none"is already a validColumnAlignmentliteral that TypeScript can infer.♻️ Proposed fix
- alignments: Array.from({ length: cols }, () => "none" as ColumnAlignment), + alignments: Array.from<ColumnAlignment>({ length: cols }, () => "none"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/utils/markdown-table.ts` around lines 213 - 219, The createEmptyTable function contains an unnecessary type assertion: remove the redundant `as ColumnAlignment` on the alignments Array.from call in createEmptyTable so TypeScript can infer the `"none"` literal as a ColumnAlignment; update the alignments line to simply create an array of `"none"` values (keeping the function name createEmptyTable and the headers/rows logic unchanged) to eliminate the redundant cast.
110-110: Escaped backslash followed by literal pipe (\\|) won't round-trip correctly.The
escapeCelllookbehind(?<!\\)prevents escaping when any\precedes|, but\\|(escaped backslash + literal pipe) should have its pipe escaped. The parse side correctly treats\\|as a separator (2 backslashes = even), but serialize won't escape the|in this case.Example: a cell containing literal
\\|→ serializes as\\|→ re-parses as two cells.Consider matching the parse logic by counting consecutive backslashes:
♻️ Proposed fix
- const escapeCell = (text: string): string => text.replace(/(?<!\\)\|/g, "\\|"); + const escapeCell = (text: string): string => { + // Escape pipes preceded by an even number of backslashes (0, 2, 4, …). + return text.replace(/(\|)/g, (match, _pipe, offset) => { + let backslashes = 0; + let j = offset - 1; + while (j >= 0 && text[j] === "\\") { + backslashes++; + j--; + } + return backslashes % 2 === 0 ? "\\|" : match; + }); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/utils/markdown-table.ts` at line 110, The escapeCell function's lookbehind (?<!\\) fails for sequences like "\\|" because it only checks the immediate preceding backslash; update escapeCell to mirror the parser by escaping a pipe only when it is preceded by an even number of backslashes. Replace the current regex-based approach in escapeCell with logic that locates each '|' and counts consecutive backslashes immediately before it (or use a regex that enforces an even number of backslashes before the pipe), and only insert an escape backslash for pipes with an even count of preceding backslashes so "\\|" serializes to "\\\\|" (round-trips correctly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/components/MemoContent/Table.tsx`:
- Around line 38-46: The variable resolveTableIndex is a memoized value
(computed index) but named like a function; rename it (e.g., to
currentTableIndex or matchedTableIndex) and update all references to that
identifier in the component so the name reflects a value rather than an action;
keep the useMemo logic and dependencies ([tables, node]) unchanged and ensure
any JSX or other code that used resolveTableIndex now uses the new name.
In `@web/src/utils/markdown-table.ts`:
- Around line 213-219: The createEmptyTable function contains an unnecessary
type assertion: remove the redundant `as ColumnAlignment` on the alignments
Array.from call in createEmptyTable so TypeScript can infer the `"none"` literal
as a ColumnAlignment; update the alignments line to simply create an array of
`"none"` values (keeping the function name createEmptyTable and the headers/rows
logic unchanged) to eliminate the redundant cast.
- Line 110: The escapeCell function's lookbehind (?<!\\) fails for sequences
like "\\|" because it only checks the immediate preceding backslash; update
escapeCell to mirror the parser by escaping a pipe only when it is preceded by
an even number of backslashes. Replace the current regex-based approach in
escapeCell with logic that locates each '|' and counts consecutive backslashes
immediately before it (or use a regex that enforces an even number of
backslashes before the pipe), and only insert an escape backslash for pipes with
an even count of preceding backslashes so "\\|" serializes to "\\\\|"
(round-trips correctly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f65fad56-04c3-45e2-925a-f8f067e8b66c
📒 Files selected for processing (3)
web/src/components/MemoContent/Table.tsxweb/src/utils/markdown-table.test.tsweb/src/utils/markdown-table.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/utils/markdown-table.test.ts
Bugs: - Fix replaceNthTable off-by-one: findAllTables now uses truly exclusive end index (start + text.length) so content.slice(start, end) === text - Replace fragile DOM-based table index resolution with AST-based approach using node.position.start.offset from hast ReactMarkdownProps Architecture: - Unify TableEditorDialog instances: InsertMenu no longer manages its own dialog, instead calls onOpenTableEditor from parent MemoEditor which owns the single shared dialog instance - Remove onInsertText prop chain (InsertMenu → EditorToolbar → MemoEditor) replaced by onOpenTableEditor Other improvements: - Add i18n: all hardcoded English strings now use useTranslate()/t() with new editor.table.* keys in en.json - Fix useCallback [props] dependency that defeated memoization (removed with dialog unification) - Use stable row IDs (monotonic counter) as React keys instead of array indices in TableEditorDialog - Replace hardcoded MONO_FONT constant with Tailwind font-mono class (maps to project's --font-mono CSS variable) - Add 28 vitest tests for markdown-table.ts covering parse, serialize, findAllTables, replaceNthTable, createEmptyTable with edge cases - Add vitest dev dependency with test/test:watch scripts
…ion\n\nThe table component mapping in MemoContent discarded the `node` prop\nfrom react-markdown, so `node.position.start.offset` was always\nundefined and the edit button never opened the table editor."
- Replace regex-based TABLE_LINE scan in findAllTables with a proper GFM markdown AST lookup using mdast-util-from-markdown + mdast-util-gfm + micromark-extension-gfm + unist-util-visit (all already in deps). Tables without leading/trailing pipes (e.g. 'A | B\n--- | ---\n1 | 2') are now correctly detected via node.position.start/end.offset. - Add regression test asserting pipe-less GFM table is found with correct start/end offsets. - Fix parseMarkdownTable to split on unescaped pipes only (lookbehind). - Fix import order in Table.tsx (Biome organizeImports). - Fix useless escape \- → - in TableEditorDialog.tsx sort regex. - Reformat long normalize/compareFn lines to satisfy Biome formatter.
- escapeCell() replaces unescaped | with \| before writing each cell, applied in both width calculation and formatRow so padding is accurate - parseRow unescapes \| back to | after splitting on unescaped pipes - Adds round-trip regression test for cells containing pipe characters Addresses coderabbitai review comment on PR usememos#5680.
Hoist `findAllTables(memo.content)` into a shared `useMemo` so the markdown AST is parsed exactly once per `memo.content` change. Both `resolveTableIndex` and `handleEditClick` now reference the same cached `tables` array, keeping the lookup and edit paths in sync and eliminating the duplicate parse that previously occurred on every edit click.
…s; use @/ alias in test import - Rewrite parseRow to count consecutive backslashes before each pipe so that `\\|` (escaped backslash + unescaped pipe) is correctly treated as a column separator. The previous lookbehind regex (?<!\\)| only checked the single character immediately before the pipe, causing it to fail for an even number of preceding backslashes. - Update test file import from relative `./markdown-table` to the project alias `@/utils/markdown-table` for consistency with frontend conventions.
…andling, remove redundant cast - Rename `resolveTableIndex` useMemo value to `currentTableIndex` in Table.tsx so the name reflects a computed value rather than an action; update all references (callbacks, dependency arrays, JSDoc comment) - Fix `escapeCell` in markdown-table.ts: replace the single-char lookbehind regex `(?<!\\)|` with a character loop that counts consecutive backslashes before each pipe and only inserts an escape when the count is even, mirroring the parser logic and correctly handling sequences like `\\|` - Remove redundant `as ColumnAlignment` type assertion in `createEmptyTable`; TypeScript infers `\"none\"` correctly via contextual typing from the return type - Add regression test for the `\\|` round-trip case in markdown-table.test.ts
milvasic
force-pushed
the
feat/table-editor
branch
from
88a7f46 to
0ad53f9
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/components/MemoEditor/index.tsx (1)
75-77: Make confirm flow explicit in parent (close dialog + restore editor focus).After inserting markdown on Line 76, explicitly close the dialog and focus the editor. This avoids coupling behavior to
TableEditorDialoginternals and improves typing continuity.♻️ Proposed refactor
- const handleTableConfirm = useCallback((markdown: string) => { - editorRef.current?.insertText(markdown); - }, []); + const handleTableConfirm = useCallback((markdown: string) => { + editorRef.current?.insertText(markdown); + setTableDialogOpen(false); + editorRef.current?.focus(); + }, []);Also applies to: 187-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/MemoEditor/index.tsx` around lines 75 - 77, The handleTableConfirm callback currently only inserts markdown into the editor; update it to also explicitly close the table dialog and restore editor focus by calling the parent dialog-close handler and focusing the editorRef after insertion (e.g., insertText(markdown); call the dialog close setter or onClose handler; then call editorRef.current?.focus()); apply the same change to the analogous handler at the other location referenced (line ~187) so both confirm flows close the dialog and re-focus the editor from the parent component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/components/MemoEditor/index.tsx`:
- Around line 75-77: The handleTableConfirm callback currently only inserts
markdown into the editor; update it to also explicitly close the table dialog
and restore editor focus by calling the parent dialog-close handler and focusing
the editorRef after insertion (e.g., insertText(markdown); call the dialog close
setter or onClose handler; then call editorRef.current?.focus()); apply the same
change to the analogous handler at the other location referenced (line ~187) so
both confirm flows close the dialog and re-focus the editor from the parent
component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8d05e77-282e-40de-9e1d-6150e825d289
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
AGENTS.mdweb/package.jsonweb/src/components/MemoContent/Table.tsxweb/src/components/MemoContent/index.tsxweb/src/components/MemoEditor/Editor/SlashCommands.tsxweb/src/components/MemoEditor/Editor/commands.tsweb/src/components/MemoEditor/Editor/index.tsxweb/src/components/MemoEditor/Toolbar/InsertMenu.tsxweb/src/components/MemoEditor/components/EditorContent.tsxweb/src/components/MemoEditor/components/EditorToolbar.tsxweb/src/components/MemoEditor/index.tsxweb/src/components/MemoEditor/types/components.tsweb/src/components/TableEditorDialog.tsxweb/src/locales/en.jsonweb/src/utils/markdown-table.test.tsweb/src/utils/markdown-table.ts
✅ Files skipped from review due to trivial changes (3)
- AGENTS.md
- web/src/locales/en.json
- web/src/components/TableEditorDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- web/src/components/MemoEditor/Editor/index.tsx
- web/package.json
- web/src/components/MemoEditor/Editor/SlashCommands.tsx
- web/src/components/MemoEditor/components/EditorContent.tsx
- web/src/components/MemoEditor/components/EditorToolbar.tsx
- web/src/components/MemoEditor/types/components.ts
- web/src/utils/markdown-table.test.ts
- web/src/utils/markdown-table.ts
- web/src/components/MemoEditor/Editor/commands.ts
- web/src/components/MemoContent/Table.tsx
- web/src/components/MemoEditor/Toolbar/InsertMenu.tsx
Contributor
Author
|
@boojack I addressed all the CR comments. Anything else for this PR? |
Contributor
Author
|
@johnnyjoygh Maybe you can provide some guidance here? |
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
3 participants
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.
Add a dialog-based table editor to create and edit markdown tables via a visual grid instead of raw pipe-delimited text.
Features:
New: TableEditorDialog.tsx, utils/markdown-table.ts. Integration in SlashCommands, EditorContent, InsertMenu, MemoContent Table.
Made-with: Cursor
Screenshots:
Summary by CodeRabbit
New Features
Tests
Chores
Localization
Documentation