feat: add toc toolbar item#3247
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1456204222
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
VladaHarbour
force-pushed
the
sd-2751_toc-node-icon
branch
from
936efaf to
cf389fe
Compare
| #syncTableOfContentsToolbarAvailability() { | ||
| const tocItem = this.toolbarItems.find((item) => item.name.value === 'tableOfContents'); | ||
| if (!tocItem) return; | ||
|
|
||
| if (!this.activeEditor) { | ||
| tocItem.setDisabled(true); | ||
| return; | ||
| } | ||
|
|
||
| let available = false; | ||
| try { | ||
| const cap = this.activeEditor.doc.capabilities(); | ||
| available = Boolean(cap.operations['create.tableOfContents']?.available); | ||
| } catch { | ||
| available = false; | ||
| } | ||
|
|
||
| tocItem.setDisabled(!available); | ||
| } |
There was a problem hiding this comment.
every other toolbar item derives its disabled state from HEADLESS_ITEM_MAP + the registry's state() deriver, which feeds snapshot.commands[id].disabled through #applyHeadlessState. this adds a parallel one-off path that calls capabilities() directly and only refreshes when updateToolbarState() runs. could tableOfContents register through HEADLESS_ITEM_MAP instead so it stays in sync with snapshots automatically and doesn't introduce a second mechanism for the same thing?
| const XL_OVERFLOW_SAFETY_BUFFER = 20; | ||
| const XL_CUTOFF = RESPONSIVE_BREAKPOINTS.xl + XL_OVERFLOW_SAFETY_BUFFER; | ||
| const XL_ITEMS = ['linkedStyles', 'clearFormatting', 'copyFormat', 'ruler']; | ||
| const XL_ITEMS = ['linkedStyles', 'copyFormat', 'ruler']; |
There was a problem hiding this comment.
clearFormatting is still in itemsToHideXL (defaultItems.js:1079), so removing it from this expected list silently drops a boundary assertion. was that intentional? if not, adding 'clearFormatting' back keeps the coverage.
|
|
||
| const itemsToHideXL = ['linkedStyles', 'clearFormatting', 'copyFormat', 'ruler', 'formattingMarks']; | ||
| const itemsToHideSM = ['zoom', 'fontFamily', 'fontSize', 'redo']; | ||
| const itemsToHideSM = ['zoom', 'fontFamily', 'fontSize', 'redo', 'tableOfContents']; |
There was a problem hiding this comment.
tableOfContents got added to itemsToHideSM but the test file only covers XL overflow. worth a small SM case mirroring the XL one — otherwise a future reorder could quietly move tableOfContents out of overflow without a test failure.
| if (inserted && props.dispatch) { | ||
| globalThis.queueMicrotask(() => { | ||
| syncTocBookmarks(editor, prepared.sources); | ||
| }); | ||
| } |
There was a problem hiding this comment.
the non-toolbar TOC path runs syncTocBookmarks synchronously after dispatch (toc-wrappers.ts:912). here the command schedules it via queueMicrotask. was the asymmetry intentional? running the sync from the toolbar caller after the command returns would match the existing pattern and keep the command itself a pure transaction producer.
| throw error; | ||
| } | ||
| }, | ||
| insertTableOfContentsFromToolbar: () => (props) => { |
There was a problem hiding this comment.
the FromToolbar suffix bakes the caller into the API, but the behavior (insert at selection, fall back to document end) isn't toolbar-specific. if a slash menu or shortcut wants the same thing later the name will lie — drop the suffix?
luccas-harbour
left a comment
luccas-harbour
left a comment
There was a problem hiding this comment.
hey @VladaHarbour!
I was reviewing this one in the browser and seem to have found some issues.
These are the steps I took:
- I opened the
basic/advanced-text.docxdocument from our test corpus in the editor - Then I placed the cursor at the start of the first line and pressed enter to create a new line above it
- I moved back up to the empty line I created
- I clicked the new TOC button in the toolbar
Nothing happened, the TOC was not added and no javascript errors surfaced in the console.
Then, I did this:
- I placed the cursor after the document title (the
SuperDoc Advanced Text Formattingline) - I pressed enter to create a new line below the title
- I clicked the new TOC button in the toolbar
This time the TOC was added but all page numbers were set to zero. I had to right-click and select "Update table of contents" for the numbers to show up correctly.
Also, codex found this one:
- [P2] Disable the overflowed TOC item as well — packages/super-editor/src/editors/v1/components/toolbar/super-toolbar.js:729-729
On widths below the small breakpoint,tableOfContentsis moved intooverflowItems, but this sync only searchesthis.toolbarItems. In that state, ifcreate.tableOfContentsis unavailable (for example a custom editor without the TOC extension), the overflow menu item remains enabled and clicking it can still route to a missing/unsupported command. Includethis.overflowItemswhen locating the TOC item.
Claude found a few more things that I added as inline comments.
Can you check these? Thank you!
4 participants
No description provided.