feat: add support for Google Docs tabs#3
Conversation
Extract tab ID from URL (?tab=t.xxx parameter) and fetch content from the specified tab instead of defaulting to the first tab. This enables fetching content from nested child tabs in multi-tab documents. - Add ExtractTabID() to parse tab parameter from URLs - Enable IncludeTabsContent(true) on all API calls - Add FindTab() to recursively search tab tree by ID - Update converter to work with tab content directly - Add comprehensive tests for new tab functionality Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds tab-aware conversion: extract optional tab ID from Google Docs URLs, locate tabs in fetched documents, and initialize the markdown converter from a specified tab or the document (preferring first tab) before generating markdown with updated frontmatter. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI
participant URL as URL Parser
participant API as Google Docs API
participant Finder as Tab Finder
participant Conv as Converter
participant MD as Markdown Builder
CLI->>URL: ExtractTabID(url)
URL-->>CLI: tabID or ""
CLI->>API: FetchDocument(docID) (include tabs)
API-->>CLI: Document with tabs
alt tabID provided
CLI->>Finder: FindTab(document, tabID)
Finder-->>CLI: tab or nil
CLI->>Conv: NewConverterFromTab(document, tab)
Note over Conv: init with tab.Body & tabName
else no tabID
CLI->>Conv: NewConverter(document)
Note over Conv: prefer first tab body or doc.Body
end
CLI->>Conv: Convert()
Conv->>Conv: generateFrontmatter()
Conv->>MD: build markdown from c.body
MD-->>Conv: markdown
Conv-->>CLI: output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cmd/gdocs-cli/main.go`:
- Around line 114-121: The code accesses tab.TabProperties.Title after
gdocs.FindTab returns a tab, which risks a nil-pointer if TabProperties is
unexpectedly nil; update the block handling the found tab (the tab variable from
gdocs.FindTab in main.go) to defensively check that tab.TabProperties is not nil
before using Title (log a clear message and return an error or fall back to a
safe/default title), and only call markdown.NewConverterFromTab(doc, tab) once
tab and tab.TabProperties are validated.
In `@internal/gdocs/client.go`:
- Around line 53-66: The function findTabRecursive should defensively handle nil
pointers: first check if the input tab is nil before accessing
tab.TabProperties, and when iterating tab.ChildTabs skip any nil child entries
before recursing; update findTabRecursive to return nil immediately if tab ==
nil, guard the TabProperties access, and only call findTabRecursive(child, ...)
for non-nil child values to avoid potential nil pointer dereferences when
traversing docs.Tab and its ChildTabs.
In `@internal/gdocs/url.go`:
- Around line 27-41: The current ExtractTabID function uses a restrictive regex
`t\.[a-zA-Z0-9]+` which assumes a specific tab ID format; change the regex in
ExtractTabID to capture the entire tab parameter value without constraining
characters (e.g., use `[?&]tab=([^&]+)`) so it returns whatever follows `tab=`
until the next `&` or end-of-string, keep using regexp.MustCompile and
FindStringSubmatch and return matches[1] or "" if missing.
🧹 Nitpick comments (4)
internal/markdown/converter.go (2)
74-91: Fragile string manipulation for frontmatter injection.The approach of trimming
---\nand appending the tab field relies on the exact output format ofGenerateFrontmatter. If that function's output format changes (e.g., trailing whitespace), this will break. Additionally, ifc.tabNamecontains YAML special characters (e.g., colons, quotes), the output may produce invalid YAML.Consider extending the
Frontmatterstruct infrontmatter.goto include an optionalTabfield and letGenerateFrontmatterhandle it, or at minimum use proper YAML escaping for the tab name.♻️ Suggested approach using YAML-safe formatting
// generateFrontmatter creates frontmatter including tab info if present. func (c *Converter) generateFrontmatter() (string, error) { - // Use the existing GenerateFrontmatter for the base, but we'll - // add tab info if we have it - frontmatter, err := GenerateFrontmatter(c.doc) - if err != nil { - return "", err - } - - // If we have a tab name that differs from the doc title, include it - if c.tabName != "" && c.tabName != c.title { - // Insert tab info before the closing --- - frontmatter = strings.TrimSuffix(frontmatter, "---\n") - frontmatter += fmt.Sprintf("tab: %s\n---\n", c.tabName) - } - - return frontmatter, nil + // Use the existing GenerateFrontmatter for the base, but we'll + // add tab info if we have it + frontmatter, err := GenerateFrontmatter(c.doc) + if err != nil { + return "", err + } + + // If we have a tab name that differs from the doc title, include it + if c.tabName != "" && c.tabName != c.title { + // Insert tab info before the closing --- + frontmatter = strings.TrimSuffix(frontmatter, "---\n") + frontmatter += fmt.Sprintf("tab: %q\n---\n", c.tabName) + } + + return frontmatter, nil }
39-51: Consider handling the case wheretabis provided but has no content.When
tab != nilbuttab.DocumentTabis nil, the converter is created with a nilbody. WhileConvert()handles this gracefully at line 66, this silent behavior might confuse users expecting content. Consider logging a warning or returning an error if a tab is explicitly requested but contains no document content.internal/gdocs/url.go (1)
30-33: Minor: Consider precompiling the regex at package level.The regex is compiled on every call to
ExtractTabID. For better performance, especially if called frequently, consider compiling it once at package initialization, similar to how production code often handles static patterns.♻️ Suggested optimization
+var tabIDPattern = regexp.MustCompile(`[?&]tab=(t\.[a-zA-Z0-9]+)`) + // ExtractTabID extracts the tab ID from a Google Docs URL if present. // Tab IDs appear in URLs as ?tab=t.{TAB_ID} or &tab=t.{TAB_ID} // Returns empty string if no tab ID is found. func ExtractTabID(url string) string { - // Regex pattern to match tab parameter (e.g., tab=t.v63b7x227gkk) - pattern := `[?&]tab=(t\.[a-zA-Z0-9]+)` - re := regexp.MustCompile(pattern) - - matches := re.FindStringSubmatch(url) + matches := tabIDPattern.FindStringSubmatch(url) if len(matches) < 2 { return "" } return matches[1] }internal/gdocs/url_test.go (1)
84-130: Good test coverage. Consider adding edge cases for malformed tab parameters.The tests cover the main scenarios well. Consider adding cases for:
- Malformed tab values like
?tab=invalid(not.prefix) - should return empty string- Tab parameter in an unexpected position like
?tab=t.abc&other=valuewith trailing params🧪 Additional test cases
{ name: "malformed tab without t. prefix", url: "https://docs.google.com/document/d/1abc123xyz/edit?tab=invalid", want: "", }, { name: "tab parameter with trailing params", url: "https://docs.google.com/document/d/1abc123xyz/edit?tab=t.abc123&other=value", want: "t.abc123", },
| // findTabRecursive recursively searches for a tab by ID. | ||
| func findTabRecursive(tab *docs.Tab, tabID string) *docs.Tab { | ||
| if tab.TabProperties != nil && tab.TabProperties.TabId == tabID { | ||
| return tab | ||
| } | ||
|
|
||
| for _, child := range tab.ChildTabs { | ||
| if found := findTabRecursive(child, tabID); found != nil { | ||
| return found | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Potential nil pointer dereference if a child tab is nil.
The function accesses tab.TabProperties without first checking if tab itself is nil. While the Google Docs API likely never returns nil entries in the ChildTabs slice, defensive programming would add a nil check.
🛡️ Suggested defensive fix
// findTabRecursive recursively searches for a tab by ID.
func findTabRecursive(tab *docs.Tab, tabID string) *docs.Tab {
+ if tab == nil {
+ return nil
+ }
if tab.TabProperties != nil && tab.TabProperties.TabId == tabID {
return tab
}
for _, child := range tab.ChildTabs {
if found := findTabRecursive(child, tabID); found != nil {
return found
}
}
return nil
}📝 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.
| // findTabRecursive recursively searches for a tab by ID. | |
| func findTabRecursive(tab *docs.Tab, tabID string) *docs.Tab { | |
| if tab.TabProperties != nil && tab.TabProperties.TabId == tabID { | |
| return tab | |
| } | |
| for _, child := range tab.ChildTabs { | |
| if found := findTabRecursive(child, tabID); found != nil { | |
| return found | |
| } | |
| } | |
| return nil | |
| } | |
| // findTabRecursive recursively searches for a tab by ID. | |
| func findTabRecursive(tab *docs.Tab, tabID string) *docs.Tab { | |
| if tab == nil { | |
| return nil | |
| } | |
| if tab.TabProperties != nil && tab.TabProperties.TabId == tabID { | |
| return tab | |
| } | |
| for _, child := range tab.ChildTabs { | |
| if found := findTabRecursive(child, tabID); found != nil { | |
| return found | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@internal/gdocs/client.go` around lines 53 - 66, The function findTabRecursive
should defensively handle nil pointers: first check if the input tab is nil
before accessing tab.TabProperties, and when iterating tab.ChildTabs skip any
nil child entries before recursing; update findTabRecursive to return nil
immediately if tab == nil, guard the TabProperties access, and only call
findTabRecursive(child, ...) for non-nil child values to avoid potential nil
pointer dereferences when traversing docs.Tab and its ChildTabs.
- Add nil check for tab.TabProperties before accessing Title (main.go) - Add nil check at start of findTabRecursive for defensive programming - Use less restrictive regex for tab ID extraction - treat as opaque identifier per Google Docs API docs - Precompile tabIDPattern regex at package level for performance - Add additional test cases for edge cases Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/gdocs-cli/main.go`:
- Around line 115-126: The code currently selects a tab via gdocs.FindTab(doc,
tabID) and proceeds to create a converter (markdown.NewConverterFromTab) even
when the tab has no DocumentTab or Body, leading to silent output with only
frontmatter; update the block after locating tab (where tab, tab.TabProperties
and tabTitle are set) to check that tab.DocumentTab != nil and
tab.DocumentTab.Body != nil (or equivalent field on the tab struct) and return a
clear error (e.g., fmt.Errorf("tab '%s' has no document content", tabID)) before
calling markdown.NewConverterFromTab so users get explicit feedback instead of
an empty body.
In `@internal/gdocs/client.go`:
- Around line 39-77: Add nil guards to avoid panics when callers pass a nil
document: at the start of FindTab and GetFirstTab check if doc == nil and return
nil immediately. No behavior change otherwise; keep findTabRecursive as-is.
Ensure the checks are added to the functions named FindTab and GetFirstTab so
both safely handle a nil *docs.Document.
🧹 Nitpick comments (1)
cmd/gdocs-cli/main.go (1)
112-129: Consider moving tab-selection logic out of the CLI entry point.The CLI is now deciding tab selection and converter construction, which is business logic better housed in
internal/gdocsorinternal/markdownsocmd/gdocs-cliremains orchestration-only.Based on learnings: “Keep the CLI entry point (cmd/gdocs-cli) free of business logic; use it only for flag parsing and orchestration.”
- Add explicit error when tab has no document content (prevents silent empty output) - Add doc == nil guard to FindTab and GetFirstTab functions - Add tests for nil document cases Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ScotterC
force-pushed
the
feature/tab-support
branch
from
3f5857f to
2f934c8
Compare
All actionable comments addressed ✅Fixed in commits
Re: architectural nitpick - For a feature this size, keeping tab-selection in the CLI entry point is appropriate. Happy to refactor if you feel strongly about it. All tests pass. Ready for review when convenient! |
2 participants
Summary
?tab=t.xxxparameter) and fetches that tab's contentChanges
internal/gdocs/url.go- AddExtractTabID()to parse tab parameter from URLsinternal/gdocs/client.go- EnableIncludeTabsContent(true)on API calls, addFindTab()to recursively search tab treeinternal/markdown/converter.go- AddNewConverterFromTab()constructor for tab-specific conversioncmd/gdocs-cli/main.go- Wire up tab extraction and selection logicTest plan
ExtractTabID(),FindTab(), andGetFirstTab()Example
Human Author's Note
I haven't coded go in more than a decade so this was entirely Claude's approach. I tested directly on my own doc and the implementation seemed straightforward enough.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests