Support multi-tab Google Docs - #450
Conversation
Preview:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| * This reads exactly one tab and never combines tabs, so pass an ID returned by `listTabs()`. | ||
| * Omitting `tabId` is valid only when `listTabs()` returns exactly one tab. | ||
| */ | ||
| getContent(tabId?: string): Promise<string>; |
There was a problem hiding this comment.
This has been around for half a year so trying to avoid a breaking change here. My thought is tabs are probably less common (at least I've never used them) so I'm less inclined to optimize for that path.
The alternatives here could be
- adding a new
getTabbedContent()method that returns a structured object but we would still need the agent to do ahasTabsorlistTabscall. - having special "markers" in the markdown string around tabs - this is prob my least favorite
Personally I don't really mind much either way
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding; no additional unpublished findings. |
840bd28 to
59e9551
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding; no additional unpublished findings. |
59e9551 to
48917c4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding; no additional unpublished findings. |
48917c4 to
50a055c
Compare
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if (action.tabId === undefined && snapshot.tabs.length !== 1) { | ||
| throw new Error( | ||
| "Pending Google Doc edit predates tab support and the document has gained tabs since, " + | ||
| "so the tab it was approved against is unknown. Reject it and retry on a selected tab."); |
There was a problem hiding this comment.
🔴 Legacy edits target replacement tabs
When a document again has one tab, googleDocActionTab retargets a legacy edit without proving it is the original tab. Approved content can change an unrelated tab.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
LGTM! |
50a055c to
65cb75f
Compare
| async #modifiedWithoutRevision(): Promise<number> { | ||
| try { | ||
| return driveModifiedTime(await this.#driveApi.getFile(this.#documentId)).valueOf(); | ||
| } catch (error) { |
There was a problem hiding this comment.
[P2] Do not turn transient Drive failures into stale metadata
This catches more than the expected legacy missing-scope case: an exhausted 5xx/network retry, quota failure, or malformed successful Drive response also falls back to the permanently stored #observeDocRevision(undefined). For a revisionless document, getMetadata() then succeeds with an old lastModified while collaborators may have changed the document, and repeated reads keep returning that stale value for as long as Drive is unhealthy. Catch only the specific unavailable-scope/API condition that warrants the compatibility fallback and propagate operational/validation failures so callers do not treat stale metadata as current.
|
Posted 1 actionable inline finding; no additional unpublished findings. |
The gatekeeper asked Google for all tab content and then rejected any document with multiple or nested tabs, blocking both directly bound Docs and read-only Docs opened through Drive. Google's model is a recursive tab tree whose bodies have independent index spaces, so reads must traverse Document.tabs and every write Location/Range must carry the immutable tabId. The capability stays whole-document; tabId is only an operation target. No tab create, rename, move, duplicate or delete API is added. - docs-api.ts flattens the provider tree into a preorder adjacency list, deriving ancestry, sibling index and nesting level from the tree actually returned rather than trusting tabProperties, and requiring a globally unique non-empty tabId and a non-empty body. - markdown-converter.ts converts one tab at a time (DocTabSnapshot), since character indices restart per tab, and stamps the selected tabId on every insert location and delete/style/bullet/link range. - google.ts caches a per-tab snapshot, resolves a selector fail-closed (omission is legal only when the flattened list holds one tab, and an unknown ID never falls back to the first), replays the pending queue in global order but per tab, and scopes committed-marker suppression to the tab that owns the marker. - getContent/replaceText/appendText take an optional tabId; listTabs() is new on the read session shared by Drive. Agent guidance requires listing tabs before reading one. Both sessions reuse one revision for ten seconds and then recheck it, so concurrent reads share a fetch without pinning a long-lived Drive session to the revision it first saw. Google populates revisionId only for callers with edit access, so it is typed as optional and a cache is never confirmed by an absent one: a view-only Doc is refetched rather than spending a request on an answer that could not confirm it. A modification time needs the opposite default, since a document offering no change token must not look edited by every read, so the bound session reports Drive's modifiedTime for one. Only a refused grant falls back to the first observation, for an account whose grant predates the picker's metadata scope; a quota 403 (Drive rate-limits with that status too), an outage or a malformed body are raised instead, because dating a document from one would report it unchanged for as long as Drive stayed unhealthy and the stored observation would outlive the incident. Approval and observation text names a tab by ID as well as title: titles are user-authored, need not be unique, and may be empty, while the write targets the ID. An approved edit that cannot be applied now reports why, rather than being removed while the overseer records it as applied. Throwing is the documented applyAction contract: the record stays pending and the user is offered a retry or a discard. The edit is invalidated rather than removed, so later edits stop queuing behind it and a repeated approval repeats the reason instead of decaying into an unknown-action error; rejecting clears it. A pending edit stored before this change names no tab. The old code refused to read a document with more than one tab, so such an edit was approved against a document that had exactly one: it is retargeted while the document still holds a single tab, and invalidated only once tabs added since leave its target unknowable. Both the replay filter and the apply-time lookup search every tab for such an edit's marker, so a write that committed before the upgrade and lost its response is reconciled instead of being dropped with its named range orphaned in the document. A failed read or edit authorizes a generic observation before its error is thrown: the error distinguishes a live tab from a missing one, and for replaceText, present text from absent.
65cb75f to
460cd90
Compare
|
LGTM! |
1 participant
Updates the Docs session to support tabbed Docs. What's a tab in a doc, you ask?
One document can hold a tree of tabs, each with its own body and its own character indices. We were asking Google for all of it and then erroring out if there was more than one tab, which blocked the whole document, including read-only Docs opened through Drive.
Now
listTabs()returns that tree flattened depth-first, andgetContent(),replaceText()andappendText()each take an optionaltabId. Every operation reads or edits exactly one tab; we never merge them. You can leavetabIdoff only when the doc has exactly one tab, and an ID we don't recognize errors instead of quietly falling back to the first tab. Since each tab has its own index space, every write coordinate we send now carries the tab ID.This avoids being a breaking api change while also not introducing weird markers in the markdown.