feat(YfmTable): support header rows in yfm-table#1132
Merged
Conversation
Reviewer's GuideAdds configurable header row support to YFM tables across parsing, schema, serialization, node views, controls, DnD, and decoration plugins, exposes it via the YfmTable extension and demos, and introduces shared transaction-shape utilities used by both header-row decoration and folding logic, plus small accessibility and test updates unrelated to tables. File-Level Changesrow count, and serialize it back into YFM markup via a header-rows attribute line.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
|
|
|
d3m1d0v
force-pushed
the
d3m1d0v/table-header-rows
branch
from
fd5adb4 to
9b9c573
Compare
d3m1d0v
force-pushed
the
d3m1d0v/table-header-rows
branch
from
9b9c573 to
83303d2
Compare
Collaborator
|
d3m1d0v
force-pushed
the
d3m1d0v/table-header-rows
branch
from
83303d2 to
1439cf4
Compare
Member
Author
Done |
Member
Author
Done |
makhnatkin
approved these changes
May 15, 2026
d3m1d0v
force-pushed
the
d3m1d0v/table-header-rows
branch
from
12f214f to
f75f0e2
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
yfm-table-cell-view.tsx,_computeIsHeaderrecomputesTableDescvia_getCellInfo()separately from the main_getCellInfo()call used to populate_cellInfo, which both duplicates work and risks_isHeadertemporarily diverging from_cellInfo.isRowHeader; consider deriving_isHeaderdirectly from thecellInfo/tableDescyou already have inupdate()/selectNode()instead of calling_getCellInfo()twice. yfmTableFocusPluginnow accepts aheaderRowsEnabledoption but does not use it anywhere; if it’s not needed for focus/hover behavior, drop the parameter to avoid confusion, or wire it into the logic where header-specific focus should be treated differently.- The row DnD logic that computes
insertIdxand adjustsheaderRowsinYfmTableRowDnDHandleris fairly dense and embedded indrop(), making it hard to reason about; consider extracting the insert-index calculation and header-rows update into small named helpers so the drag behavior and header adjustment rules are easier to read and maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `yfm-table-cell-view.tsx`, `_computeIsHeader` recomputes `TableDesc` via `_getCellInfo()` separately from the main `_getCellInfo()` call used to populate `_cellInfo`, which both duplicates work and risks `_isHeader` temporarily diverging from `_cellInfo.isRowHeader`; consider deriving `_isHeader` directly from the `cellInfo`/`tableDesc` you already have in `update()`/`selectNode()` instead of calling `_getCellInfo()` twice.
- `yfmTableFocusPlugin` now accepts a `headerRowsEnabled` option but does not use it anywhere; if it’s not needed for focus/hover behavior, drop the parameter to avoid confusion, or wire it into the logic where header-specific focus should be treated differently.
- The row DnD logic that computes `insertIdx` and adjusts `headerRows` in `YfmTableRowDnDHandler` is fairly dense and embedded in `drop()`, making it hard to reason about; consider extracting the insert-index calculation and header-rows update into small named helpers so the drag behavior and header adjustment rules are easier to read and maintain.
## Individual Comments
### Comment 1
<location path="packages/editor/src/extensions/yfm/YfmTable/plugins/YfmTableControls/plugins/focus-plugin.ts" line_range="34" />
<code_context>
return true;
}
-export const yfmTableFocusPlugin = (opts: {dndEnabled: boolean}) => {
+export const yfmTableFocusPlugin = (opts: {dndEnabled: boolean; headerRowsEnabled: boolean}) => {
return new Plugin<PluginState>({
key: pluginKey,
</code_context>
<issue_to_address>
**nitpick:** `headerRowsEnabled` is added to the plugin options but not used.
The plugin now accepts `headerRowsEnabled`, but the option is unused and the `closest('td,th')` behavior is always applied. To avoid confusion, either integrate `headerRowsEnabled` into the focus logic (e.g., only treat `th` specially when it’s true) or remove the option until it’s needed.
</issue_to_address>
### Comment 2
<location path="packages/editor/src/table-utils/table-desc.ts" line_range="135-138" />
<code_context>
// <--- validation
- const desc = new this(rows.length, rows[0].cells.length, rows, baseOffset);
+ const rawHeaderRows = Number(table.attrs['data-header-rows']) || 0;
+ const headerRows = Math.max(0, Math.min(rawHeaderRows, rows.length));
+
+ const desc = new this(rows.length, rows[0].cells.length, rows, baseOffset, headerRows);
this.__cache.set(table, desc);
return desc;
</code_context>
<issue_to_address>
**suggestion:** Directly using the string `'data-header-rows'` risks drifting from the schema attribute definition.
Since the schema already defines `HeaderRows` via `YfmTableAttr`, consider using that constant instead of `'data-header-rows'` here to keep the implementation aligned with the schema and avoid future drift if the attribute name changes.
Suggested implementation:
```typescript
const rawHeaderRows = Number(table.attrs[YfmTableAttr.HeaderRows]) || 0;
```
1. Ensure `YfmTableAttr` is imported in this file from the module where the table schema is defined (likely something like `../schema` or `../schema/yfm-table`), e.g.:
`import {YfmTableAttr} from '../schema';`
2. If the file already imports `YfmTableAttr` under a different name or from a barrel file, reuse that existing import instead of adding a new one.
</issue_to_address>
### Comment 3
<location path="packages/editor/src/extensions/yfm/YfmTable/plugins/YfmTableControls/commands/insert-empty-row.ts" line_range="53-55" />
<code_context>
tr.insert(posToInsert, createSimpleRow(state.schema, newCellsCount));
tr.setSelection(TextSelection.near(tr.doc.resolve(posToInsert), 1));
+
+ // If the new row is inserted inside the header-rows block, shrink the block
+ // so the new row and everything below it stop being header rows.
+ if (tableDesc.base.isHeaderRow(rowIdx)) {
+ tr.setNodeAttribute(params.tablePos, YfmTableAttr.HeaderRows, rowIdx);
+ }
</code_context>
<issue_to_address>
**question:** Shrinking the header block on insert may be surprising when inserting at the very first header row.
For `rowIdx > 0`, shrinking `headerRows` to `rowIdx` matches the comment. But when `rowIdx === 0`, this sets `HeaderRows` to 0 and removes all header rows, which may not match the expected UX of inserting a body row above existing headers. Please confirm the intended behavior and consider special-casing `rowIdx === 0` if headers should be preserved.
</issue_to_address>
### Comment 4
<location path="packages/editor/src/extensions/yfm/YfmTable/plugins/YfmTableControls/nodeviews/yfm-table-cell-view.tsx" line_range="71" />
<code_context>
private readonly _dndEnabled: boolean;
+ private readonly _headerRowsEnabled: boolean;
+ private _isHeader: boolean;
private _decoRowUniqKey: number | null = null;
private _decoColumnUniqKey: number | null = null;
</code_context>
<issue_to_address>
**issue (complexity):** Consider computing header state only once in update and inlining the header toggle logic to remove redundant work and indirection.
You can simplify this change without losing functionality by:
### 1. Remove duplicated header-state computation
You currently compute header state twice via `_computeIsHeader()` and again in `update()` via `desc.isHeaderRow(...)`, both going through `_getCellInfo`.
You can keep a single source of truth by:
- Dropping `_computeIsHeader` and the `node` parameter on `_getCellInfo`.
- Computing `isHeader` once in `update()` from a single `cellInfo`.
- Using that for both DOM updates and `_cellInfo.isRowHeader`.
Example:
```ts
// remove this entirely
// private _computeIsHeader(): boolean {
// if (!this._headerRowsEnabled) return false;
// const info = this._getCellInfo();
// if (!info) return false;
// return info.tableDesc.base.isHeaderRow(info.cell.row);
// }
// keep only a narrow _getCellInfo for the current node
private _getCellInfo() {
const table = this._getParentTable();
const tableDesc = table ? TableDesc.create(table.node)?.bind(table.pos) : undefined;
const cellInfo = tableDesc?.base.getCellInfo(this._node);
return cellInfo
? {pos: this._getPos()!, table: table!, tableDesc: tableDesc!, cell: cellInfo}
: undefined;
}
```
Then in `update`:
```ts
update(node: Node, decorations: readonly Decoration[]): boolean {
const prev = this._node;
this._node = node;
const cellInfo = this._getCellInfo();
const desc = cellInfo?.tableDesc.base;
const isHeader =
this._headerRowsEnabled &&
!!desc &&
desc.isHeaderRow(cellInfo!.cell.row);
this._isHeader = isHeader; // or drop the field and pass isHeader into _updateDom
this._updateDom(prev);
if (cellInfo && (cellInfo.cell.row === 0 || cellInfo.cell.column === 0)) {
const info: YfmTableCellView['_cellInfo'] = (this._cellInfo = {
tablePos: cellInfo.table.pos,
rowIndex: cellInfo.cell.row,
columnIndex: cellInfo.cell.column,
showRowControl: false,
showColumnControl: false,
rowRange: desc!.getRowRangeByRowIdx(cellInfo.cell.row),
columnRange: desc!.getColumnRangeByColumnIdx(cellInfo.cell.column),
isRowHeader: isHeader,
canToggleRowHeader:
this._headerRowsEnabled &&
(isHeader || canMakeRowHeader(desc!, cellInfo.cell.row)),
});
// existing decorations loop...
}
return true;
}
```
And `\_updateDom` stays focused on a single boolean:
```ts
private _updateDom(prev?: Node) {
if (this._isHeader) {
if (this.dom.getAttribute('role') !== 'columnheader') {
this.dom.setAttribute('role', 'columnheader');
}
} else if (this.dom.hasAttribute('role')) {
this.dom.removeAttribute('role');
}
// existing align/colspan/rowspan logic…
}
```
This removes the second `TableDesc`/cell-info recomputation and makes `isRowHeader` the single source of truth (for both DOM and menu).
### 2. Inline `_toggleHeaderRows` to avoid one-off abstraction
`_toggleHeaderRows` is currently only used by `_onRowHeaderChange`, which makes the `event`, `source`, and `getValue` indirection unnecessary.
You can inline it into `_onRowHeaderChange` and keep all behavior:
```ts
// remove _toggleHeaderRows()
private _onRowHeaderChange = (value: boolean) => {
this._logger.event({
event: value ? 'row-set-header' : 'row-unset-header',
source: 'row-menu',
});
const info = this._getCellInfo();
if (info) {
const rowRange = info.tableDesc.base.getRowRangeByRowIdx(info.cell.row);
const newValue = value ? rowRange.endIdx + 1 : rowRange.startIdx;
toggleHeaderRows({
tablePos: info.table.pos,
value: newValue,
})(this._view.state, this._view.dispatch);
}
this._view.focus();
};
```
This keeps the logging, range computation, and command dispatch identical, but removes one level of indirection and makes the header toggle flow easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
2 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.
Summary by Sourcery
Add support for configurable header rows in YFM tables, including parsing/serialization, editor controls, and visual behavior, and update related tooling and tests.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: