feat(editor): Table cell merging and row text alignment features - #14885
Open
001-mak wants to merge 10 commits into
Open
feat(editor): Table cell merging and row text alignment features#14885001-mak wants to merge 10 commits into
001-mak wants to merge 10 commits into
Conversation
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTable cells now support merging, unmerging, hidden covered cells, horizontal and vertical alignment, span-aware rendering, and preservation of alignment and span metadata in HTML and Markdown exports. ChangesTable layout editing
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant TableCell
participant TableDataManager
participant Store
participant table-block
TableCell->>TableDataManager: mergeCells(selection)
TableDataManager->>Store: transact span and hidden-cell updates
Store-->>table-block: updated table data
table-block->>TableCell: render visible cells with colspan and rowspan
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (1)
blocksuite/affine/blocks/table/src/adapters/html.ts (1)
79-84: Consider extracting the inline style building logic.The style string concatenation could be simplified.
♻️ Optional simplification
- const divStyle = [ - `min-height: 22px;min-width:${DefaultColumnWidth}px;padding: 8px 12px;`, - textAlign ? `text-align: ${textAlign};` : '', - ] - .filter(Boolean) - .join(''); + const divStyle = `min-height: 22px;min-width:${DefaultColumnWidth}px;padding: 8px 12px;${textAlign ? `text-align: ${textAlign};` : ''}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/affine/blocks/table/src/adapters/html.ts` around lines 79 - 84, The inline style construction around divStyle (using DefaultColumnWidth and optional textAlign) should be extracted into a small helper (e.g., buildCellStyle or createStyleString) that accepts parameters like width and textAlign and returns the final style string; replace the inline array/filter/join in adapters/html.ts with a call to that helper to simplify concatenation and make testing reusable, keeping the same behavior for `min-height: 22px;min-width:${DefaultColumnWidth}px;padding: 8px 12px;` and optional `text-align` when textAlign is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocksuite/affine/blocks/table/src/table-data-manager.ts`:
- Around line 436-444: The concatenation for the merged top-left cell can
produce a leading newline when topLeft.text is empty: change the logic around
topLeft, extraTexts and combined so you only prepend '\n' when topLeft.text has
non-empty content; build combined by either using topLeft.text.toString() + '\n'
+ extraTexts.join('\n') when topLeft.text.length > 0, or using
extraTexts.join('\n') alone when topLeft.text is empty, then call
topLeft.text.replace(0, topLeft.text.length, combined) and still set
topLeft.colSpan and topLeft.rowSpan as before.
---
Nitpick comments:
In `@blocksuite/affine/blocks/table/src/adapters/html.ts`:
- Around line 79-84: The inline style construction around divStyle (using
DefaultColumnWidth and optional textAlign) should be extracted into a small
helper (e.g., buildCellStyle or createStyleString) that accepts parameters like
width and textAlign and returns the final style string; replace the inline
array/filter/join in adapters/html.ts with a call to that helper to simplify
concatenation and make testing reusable, keeping the same behavior for
`min-height: 22px;min-width:${DefaultColumnWidth}px;padding: 8px 12px;` and
optional `text-align` when textAlign is present.
🪄 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: b57153d7-da13-4174-846c-870aa162e901
📒 Files selected for processing (8)
blocksuite/affine/blocks/table/src/adapters/html.tsblocksuite/affine/blocks/table/src/adapters/markdown.tsblocksuite/affine/blocks/table/src/adapters/utils.tsblocksuite/affine/blocks/table/src/table-block.tsblocksuite/affine/blocks/table/src/table-cell-css.tsblocksuite/affine/blocks/table/src/table-cell.tsblocksuite/affine/blocks/table/src/table-data-manager.tsblocksuite/affine/model/src/blocks/table/table-model.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocksuite/affine/blocks/table/src/table-data-manager.ts`:
- Around line 425-430: The merge routine currently copies covered cell text into
the top-left cell but only marks covered cells hidden (cell.hidden = true),
leaving their Y.Text content intact so it reappears after unmerge; update the
merge handling (the block that builds extraTexts and sets cell.hidden=true, and
the similar block around lines 474-485) to also clear/normalize the covered
cell's text content after copying it (i.e., remove or reset the Y.Text payload
for those covered cells) and ensure the unmerge/unhide path does not restore
that cleared text.
🪄 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: 753fa2fd-8880-4c1a-a335-2e9e1d7ca412
📒 Files selected for processing (1)
blocksuite/affine/blocks/table/src/table-data-manager.ts
…ppearance after unmerge
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
blocksuite/affine/blocks/table/src/adapters/utils.ts (1)
88-91: Consider preserving thehiddenmarker for consistency.The
Celltype now declareshidden?: boolean, but the hidden-cell branch doesn't populate it when pushing the empty cell. While current adapters (plain-text.tsandmarkdown.ts) only read the cell content and ignore the field, preserving it maintains consistency for future adapters that may reference the Cell shape.Suggested fix
if (cell?.hidden) { - row.cells.push({ value: { delta: [] } }); + row.cells.push({ value: { delta: [] }, hidden: true }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/affine/blocks/table/src/adapters/utils.ts` around lines 88 - 91, When handling hidden cells in adapters/utils.ts (in the branch checking cell?.hidden), preserve the hidden marker on the pushed empty cell by setting hidden: true on the object pushed to row.cells (so the shape matches the Cell type with hidden?: boolean); update the row.cells.push call in that branch accordingly so future adapters (e.g., plain-text.ts and markdown.ts) can rely on the hidden flag being present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocksuite/affine/blocks/table/src/table-data-manager.ts`:
- Around line 421-430: When re-hiding covered cells you must also clear any
stale merge metadata so a previously merged top-left cell doesn't retain
colSpan/rowSpan after being hidden; update the hide logic that operates on
this.model.props.cells[key] (the block that touches cell.text and cell.hidden)
to normalize merge fields by resetting cell.colSpan and cell.rowSpan back to
their defaults (e.g., 1) or deleting those properties before setting cell.hidden
= true; apply the same change to the analogous block covering lines 437-445 so
both hide paths clear prior merge metadata consistently.
---
Nitpick comments:
In `@blocksuite/affine/blocks/table/src/adapters/utils.ts`:
- Around line 88-91: When handling hidden cells in adapters/utils.ts (in the
branch checking cell?.hidden), preserve the hidden marker on the pushed empty
cell by setting hidden: true on the object pushed to row.cells (so the shape
matches the Cell type with hidden?: boolean); update the row.cells.push call in
that branch accordingly so future adapters (e.g., plain-text.ts and markdown.ts)
can rely on the hidden flag being present.
🪄 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: 2c4861fc-4a01-46d7-8f27-f79435fd308e
📒 Files selected for processing (2)
blocksuite/affine/blocks/table/src/adapters/utils.tsblocksuite/affine/blocks/table/src/table-data-manager.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocksuite/affine/blocks/table/src/table-data-manager.ts`:
- Around line 414-431: The merge logic currently mutates covered cells in-place
without guarding against intersecting existing merged regions; update the
mergeCells flow to detect any covered cell that is an existing merge top-left
(has cell.colSpan or cell.rowSpan) whose span area extends outside the new
selection and reject the merge (or require prior normalization) instead of
silently masking descendants. Concretely: inside the loop that iterates
rows/columns and inspects this.model.props.cells[key], if cell.colSpan ||
cell.rowSpan is present compute the merged span rectangle (using
row.rowId/col.columnId plus cell.rowSpan/colSpan) and check whether that
rectangle is fully contained within the new selection bounds
(rowStartIndex..rowEndIndex and columnStartIndex..columnEndIndex); if it is not,
abort/return an error from mergeCells (or surface a clear rejection) so callers
must normalize/unmerge first, preventing orphaned hidden cells and preserving
table-state integrity.
🪄 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: 2d86329c-4486-45ea-ae01-3b5896ff7f62
📒 Files selected for processing (1)
blocksuite/affine/blocks/table/src/table-data-manager.ts
001-mak
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## canary #14885 +/- ##
==========================================
+ Coverage 55.11% 56.63% +1.52%
==========================================
Files 3006 3144 +138
Lines 168964 171204 +2240
Branches 24860 25265 +405
==========================================
+ Hits 93128 96966 +3838
+ Misses 72761 71086 -1675
- Partials 3075 3152 +77
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
FYI Notion has implemented this feature on May 26, 2026: It would be great if this feature got some love too. |
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.
This PR implements #14443 and #12786
Table cell merging and row text alignment features
Summary by CodeRabbit
New Features
Bug Fixes