feat: render smart chips, tabs, and all paragraph elements in markdown - #649
Merged
taylorwilsdon merged 6 commits intoApr 2, 2026
Merged
Conversation
The markdown converter was silently dropping smart chips and only handling textRun elements, producing confusing output with missing content. This adds support for all 11 Google Docs ParagraphElement types: - person → mailto link - richLink → markdown link with title - dateElement → display text or ISO-formatted timestamp - inlineObjectElement → image with alt text - footnoteReference → footnote marker with definitions appended - horizontalRule → --- - autoText → [Page #] / [Page Count] placeholders - equation → [Equation] placeholder - pageBreak / columnBreak → silently skipped - Unsupported building block chips (vote, stopwatch, timer, dropdown, etc.) rendered as [Smart Chip] via PUA character replacement Also adds document tab support (includeTabsContent=True) with timeout fallback for large docs, and multi-tab rendering with heading separators. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughconvert_doc_to_markdown gained tab-aware rendering and refactored body conversion with shared footnote collection; paragraph conversion now supports smart chips, inline images, footnote references, horizontal rules, auto-text/equation placeholders, and page-break handling. get_doc_as_markdown adds a 30s fetch timeout and returns a timeout error string on timeout. Changes
Sequence DiagramssequenceDiagram
participant Client
participant DocsTools as gdocs/docs_tools.py
participant GoogleAPI as "Google Docs API"
participant Converter as gdocs/docs_markdown.py
Client->>DocsTools: get_doc_as_markdown(doc_id)
DocsTools->>GoogleAPI: documents().get(includeTabsContent=True) (30s)
alt fetch success
GoogleAPI-->>DocsTools: document (may include tabs)
DocsTools->>Converter: convert_doc_to_markdown(doc)
Converter->>Converter: detect tabs -> _collect_tabs/_convert_tabs_to_markdown or _convert_body_to_markdown
Converter-->>Client: markdown
else timeout/error
GoogleAPI-->>DocsTools: timeout/error
DocsTools-->>Client: "Timeout — document too large or network issue"
end
sequenceDiagram
participant Parser
participant ParagraphProc as _convert_paragraph_text
participant ElementHandler
participant FootnoteStore as "footnote_defs"
participant Output
Parser->>ParagraphProc: paragraph
ParagraphProc->>ElementHandler: iterate inline elements
alt smart chip
ElementHandler-->>ParagraphProc: formatted text (name/link/date)
else inline image
ElementHandler-->>ParagraphProc:  or [Image: title]
else footnoteReference
ElementHandler-->>ParagraphProc: [^id]
ElementHandler->>FootnoteStore: append definition
else horizontalRule
ElementHandler-->>ParagraphProc: ---
else autoText/equation/pageBreak
ElementHandler-->>ParagraphProc: [Page #] / [Equation] / skip
end
ParagraphProc->>Output: append paragraph
ParagraphProc->>Output: append footnote_defs at end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gdocs/docs_tools.py (1)
2073-2096:⚠️ Potential issue | 🔴 CriticalAdd
create_update_paragraph_style_requestto the imports fromgdocs.docs_helpers.Line 2073 calls
create_update_paragraph_style_request(...), but it is not imported fromgdocs.docs_helpers. Add it to the import block at line 24 to fix the undefined name error and the Ruff F821 failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 2073 - 2096, Import the missing function create_update_paragraph_style_request from gdocs.docs_helpers into the module's import block so the call in the paragraph style construction code resolves; update the existing import statement that brings in other helpers (e.g., the same import that currently imports functions from gdocs.docs_helpers) to include create_update_paragraph_style_request so the name is defined when used in the block that builds paragraph_style_request and appended to requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gdocs/docs_markdown.py`:
- Around line 92-113: _top-level metadata (footnotes_meta, inline_objects,
footnote_defs) are only passed to _convert_paragraph_text in
_convert_body_to_markdown, but _convert_table() → _extract_cell_text() still
calls _convert_paragraph_text without them, breaking
inlineObjectElement/footnoteReference in table cells; update _convert_table and
_extract_cell_text signatures to accept footnotes_meta, inline_objects, and
footnote_defs (preserve footnote_defs as the same mutable list) and thread those
arguments through when calling _convert_paragraph_text (and any other internal
paragraph conversion calls) so table cell conversion receives the same
document-scoped metadata as top-level paragraphs.
- Around line 242-250: The function _convert_person_chip currently expands
person smart chips into mailto: links, leaking email PII; change it to only
return the display name (props["name"] or a redacted placeholder) by default and
avoid embedding the email or mailto: URI; if there is a legitimate need to
expose the email make it opt-in via an explicit parameter (e.g., include_email:
bool) to _convert_person_chip or a separate helper, and update call sites to
pass that flag where safe. Ensure the function uses props.get("name", "") as the
primary output and never uses props.get("email") in the returned string unless
the explicit opt-in flag is true.
In `@gdocs/docs_tools.py`:
- Around line 2239-2274: The get_doc_as_markdown flow currently does two
sequential asyncio.wait_for(..., timeout=30) calls around
docs_service.documents().get(...).execute which can exceed the 30s request
budget and leaves the blocking thread running while silently dropping tabs
(convert_doc_to_markdown loses secondary tabs). Replace the retry-within-request
approach: perform a single await asyncio.wait_for(..., timeout=30) for the
full-request including includeTabsContent=True; if it times out, immediately
return a partial result (or a response that explicitly flags missing tabs) and
schedule a background fetch via
asyncio.create_task(asyncio.to_thread(...execute)) to retrieve tabs later (or
push to a background worker), then merge/update via convert_doc_to_markdown when
the background fetch completes. Ensure logs and the returned payload indicate
that tabs are pending so users aren’t silently missing data, and remove the
second blocking wait_for retry to avoid exceeding the 30s request context.
---
Outside diff comments:
In `@gdocs/docs_tools.py`:
- Around line 2073-2096: Import the missing function
create_update_paragraph_style_request from gdocs.docs_helpers into the module's
import block so the call in the paragraph style construction code resolves;
update the existing import statement that brings in other helpers (e.g., the
same import that currently imports functions from gdocs.docs_helpers) to include
create_update_paragraph_style_request so the name is defined when used in the
block that builds paragraph_style_request and appended to requests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46a0651c-e0b5-4a13-9ad0-c6b778d85748
📒 Files selected for processing (3)
gdocs/docs_markdown.pygdocs/docs_tools.pytests/gdocs/test_docs_markdown.py
Comment on lines
+242
to
+250
| def _convert_person_chip(person: dict[str, Any]) -> str: | ||
| """Convert a person smart chip to a mailto link.""" | ||
| props = person.get("personProperties", {}) | ||
| name = props.get("name", "") | ||
| email = props.get("email", "") | ||
| if email: | ||
| label = name or email | ||
| return f"[{label}](mailto:{email})" | ||
| return name or "" |
Contributor
There was a problem hiding this comment.
Do not expand person chips into raw email addresses by default.
This now embeds the underlying email in every person chip via mailto:, even when the document only rendered the display name. That leaks extra PII into the Markdown response and the LLM context.
🛡️ Proposed safe default
def _convert_person_chip(person: dict[str, Any]) -> str:
- """Convert a person smart chip to a mailto link."""
+ """Convert a person smart chip to display text."""
props = person.get("personProperties", {})
name = props.get("name", "")
email = props.get("email", "")
- if email:
- label = name or email
- return f"[{label}](mailto:{email})"
- return name or ""
+ return name or email or ""As per coding guidelines, "Never log or leak secrets, refresh tokens, or PII in exceptions or event streams" and "Tool response payloads should be concise; redact verbose HTML/email bodies by default and offer links or IDs to fetch full content."
📝 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
| def _convert_person_chip(person: dict[str, Any]) -> str: | |
| """Convert a person smart chip to a mailto link.""" | |
| props = person.get("personProperties", {}) | |
| name = props.get("name", "") | |
| email = props.get("email", "") | |
| if email: | |
| label = name or email | |
| return f"[{label}](mailto:{email})" | |
| return name or "" | |
| def _convert_person_chip(person: dict[str, Any]) -> str: | |
| """Convert a person smart chip to display text.""" | |
| props = person.get("personProperties", {}) | |
| name = props.get("name", "") | |
| email = props.get("email", "") | |
| return name or email or "" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gdocs/docs_markdown.py` around lines 242 - 250, The function
_convert_person_chip currently expands person smart chips into mailto: links,
leaking email PII; change it to only return the display name (props["name"] or a
redacted placeholder) by default and avoid embedding the email or mailto: URI;
if there is a legitimate need to expose the email make it opt-in via an explicit
parameter (e.g., include_email: bool) to _convert_person_chip or a separate
helper, and update call sites to pass that flag where safe. Ensure the function
uses props.get("name", "") as the primary output and never uses
props.get("email") in the returned string unless the explicit opt-in flag is
true.
Owner
There was a problem hiding this comment.
Bad feedback, it should read the contents as they are.
Contributor
There was a problem hiding this comment.
@taylorwilsdon, understood — rendering the full contents of person chips (including the email as a mailto: link) faithfully reflects what the document contains. I'll defer to your judgment here.
(´･ω･`)
🧠 Learnings used
Learnt from: hGriff0n
Repo: taylorwilsdon/google_workspace_mcp PR: 499
File: gkeep/keep_tools.py:417-425
Timestamp: 2026-02-21T18:00:37.834Z
Learning: When using asyncio.to_thread(callable, *args, **kwargs), pass a callable, not the result of a call. For methods on objects, pass the method reference (e.g., await asyncio.to_thread(service.some_method(...).execute)) rather than invoking it upfront (service.some_method(...).execute()). This ensures a callable is provided to to_thread, which will run it in a separate thread. This guideline applies broadly to all Python files in the Google Workspace MCP codebase (and similar Python projects); enforce by scanning for instances where a method is called immediately inside to_thread instead of passing the bound method/reference.
- Pass footnotes_meta, inline_objects, and footnote_defs through _convert_table → _extract_cell_text → _convert_paragraph_text so smart chips inside table cells render correctly. - Add missing create_update_paragraph_style_request import in docs_tools.py (pre-existing bug). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
A single timeout with a clear error is better than a silent fallback that drops multi-tab content without telling the user. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Addressed all the feedback, ready for re-review @taylorwilsdon |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
gdocs/docs_tools.py (1)
2240-2275:⚠️ Potential issue | 🟠 MajorAvoid the second in-request timeout fallback.
This path can now hold the tool open for ~60s, and
wait_for(asyncio.to_thread(...))will not stop the first.execute()once the worker thread has started. If the retry succeeds,convert_doc_to_markdown()also drops secondary tabs while comment fetching still runs against the full file, so the response can look complete while actually being partial. Fail fast after the first timeout, or return an explicit partial-result marker instead of issuing a second blocking fetch here.In Python asyncio, does `asyncio.wait_for(asyncio.to_thread(blocking_call), timeout=30)` cancel the underlying worker thread after the timeout expires, or does the thread continue running in the background?Based on learnings, "Avoid long-running operations (>30s) inside request context; instead stream partial results or schedule background tasks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gdocs/docs_tools.py` around lines 2240 - 2275, The retry block that does a second asyncio.wait_for(asyncio.to_thread(docs_service.documents().get(...).execute)) must be removed to avoid background threads continuing after timeout; in get_doc_as_markdown (and where convert_doc_to_markdown is used), fail fast on the first TimeoutError/asyncio.TimeoutError by returning an explicit partial-result marker or scheduling a background task, rather than issuing the second blocking fetch, and ensure logs mention document_id and that the response is partial; target the docs_service.documents().get(...).execute call and the except handler to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gdocs/docs_markdown.py`:
- Around line 213-217: The current footnote handling uses
_extract_footnote_text() which only concatenates textRuns and loses rich
content; update the footnote materialization (where "footnoteReference" is
handled) to pass document-scoped metadata (footnotes_meta, footnote_defs, and
any other context used by _convert_paragraph_text) into a new conversion path
and build each footnote body via _convert_paragraph_text() instead of
hand-stitching runs; specifically replace calls to _extract_footnote_text() with
a flow that creates a paragraph-like element for the footnote and invokes
_convert_paragraph_text(paragraph_elem, footnotes_meta, footnote_defs, ...) so
links, smart chips, inline objects, and style splits are preserved (also apply
the same change to the other occurrences around lines 314-346).
- Around line 68-74: The current loop over all_tab_docs uses tab_md =
_convert_body_to_markdown(tab_doc) and skips appending a section when
tab_md.strip() is empty, which drops empty tabs; change the logic in the loop
that builds sections (the sections list, the for title, tab_doc in all_tab_docs
block) to always append the tab heading f"# {title}\n\n{tab_md}" even if tab_md
is blank so every tab is emitted; preserve the final return behavior
("\n".join(sections).rstrip("\n") + "\n") so output trailing newline handling
remains unchanged.
---
Duplicate comments:
In `@gdocs/docs_tools.py`:
- Around line 2240-2275: The retry block that does a second
asyncio.wait_for(asyncio.to_thread(docs_service.documents().get(...).execute))
must be removed to avoid background threads continuing after timeout; in
get_doc_as_markdown (and where convert_doc_to_markdown is used), fail fast on
the first TimeoutError/asyncio.TimeoutError by returning an explicit
partial-result marker or scheduling a background task, rather than issuing the
second blocking fetch, and ensure logs mention document_id and that the response
is partial; target the docs_service.documents().get(...).execute call and the
except handler to implement this change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: eed64500-3da8-422a-96b8-61b5a05b2337
📒 Files selected for processing (2)
gdocs/docs_markdown.pygdocs/docs_tools.py
5 tasks
5 tasks
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.
Description
The markdown converter was silently dropping smart chips (person mentions, dates, rich links, etc.), producing confusing output with missing content. This adds support for all 11
ParagraphElementtypes from the Google Docs API, document tab support, and timeout handling for large docs.Smart chip rendering:
---[Page #]/[Page Count]placeholders[Equation]placeholder[Smart Chip]placeholder via PUA character replacementDocument tabs: Added
includeTabsContent=Trueto the API call. Single-tab docs render without extra headings; multi-tab docs get# Tab Titleseparators. Recursive child tab support included.Timeout handling: Wrapped the Docs API fetch in
asyncio.wait_forwith a 30s timeout, falling back to a non-tabs request for very large documents.Type of Change
Testing
42 tests total, including
TestSmartChips(17 tests covering all chip types) andTestDocumentTabs(4 tests for single/multi/child/legacy tab rendering). Manually tested against a Google Doc containing all chip types.Checklist
Additional Notes
Also fixes a pre-existing bug:
create_update_paragraph_style_requestwas called indocs_tools.pybut never imported fromdocs_helpers.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests