- 
                Notifications
    
You must be signed in to change notification settings  - Fork 43
 
          [TKT-15] getSnapshot fixes for Old Agent + new Agent
          #176
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR adds support for a new snapshot format from BrowserOS that uses an items-based structure instead of the sections-based format. The implementation uses a feature flag (NEW_SNAPSHOT_FORMAT) to switch between old and new formats at runtime.
Key Changes:
- Added type definitions for 
NewSnapshotwith items array containing{type, text, url, level}objects - Modified 
BrowserOSAdapter.getSnapshot()to conditionally call different Chrome API signatures based on feature flag - Added format detection and parsing helpers in 
BrowserPageto handle both formats transparently - Refactored 
ChatAgentandExtracttool to use unified snapshot methods that abstract format differences 
Issues Found:
- Critical type mismatch in 
BrowserOSAdapter.ts:434-435where Promise and callback are typed asSnapshotbut should beSnapshotResult - Logic error in 
BrowserPage.ts:945wheregetTextWithLinksString()requests'links'type instead of'text'type - Redundant feature flag checks in multiple layers
 - Minor code style inconsistencies
 
Confidence Score: 2/5
- This PR has critical type errors that will cause TypeScript compilation issues and potential runtime failures
 - Score of 2 reflects critical type mismatches in BrowserOSAdapter (Promise vs Promise) that will cause TypeScript errors, plus a logic error in snapshot type selection. While the overall architecture is sound, these issues must be fixed before merging
 - Pay close attention to 
src/lib/browser/BrowserOSAdapter.ts(type mismatches on lines 434-435) andsrc/lib/browser/BrowserPage.ts(incorrect snapshot type on line 945) 
Important Files Changed
File Analysis
| Filename | Score | Overview | 
|---|---|---|
| src/lib/browser/BrowserOSAdapter.ts | 2/5 | Added snapshot format detection and union type support, but has critical type mismatches in Promise and callback declarations | 
| src/lib/browser/BrowserPage.ts | 3/5 | Added format detection helpers and new getTextWithLinksString method, but has logic issue with snapshot type selection and minor inconsistencies | 
| src/lib/tools/Extract.ts | 4/5 | Refactored to use helper functions and support both snapshot formats with redundant feature flag checks but otherwise solid | 
Sequence Diagram
sequenceDiagram
    participant Client as ChatAgent/Extract Tool
    participant BPage as BrowserPage
    participant Adapter as BrowserOSAdapter
    participant FF as FeatureFlags
    participant Chrome as chrome.browserOS
    Client->>BPage: getTextSnapshotString(options)
    BPage->>Adapter: getSnapshot(tabId, 'text', options)
    Adapter->>FF: isEnabled('NEW_SNAPSHOT_FORMAT')
    FF-->>Adapter: true/false
    
    alt New Format Enabled
        Adapter->>Chrome: getSnapshot(tabId, [options], callback)
        Chrome-->>Adapter: NewSnapshot (items: [{type, text, url}])
    else Old Format
        Adapter->>Chrome: getSnapshot(tabId, 'text', [options], callback)
        Chrome-->>Adapter: Snapshot (sections: [...])
    end
    
    Adapter-->>BPage: SnapshotResult (union type)
    BPage->>BPage: _isNewSnapshot(snapshot)
    
    alt New Format
        BPage->>BPage: _formatNewSnapshot(snapshot, includeLinks)
        Note over BPage: Format items as markdown<br/># headings, [text](url)
    else Old Format
        BPage->>BPage: _formatOldSnapshot(snapshot, includeLinks)
        Note over BPage: Extract from sections.textResult
    end
    
    BPage-->>Client: Formatted string content
    6 files reviewed, 3 comments
| `[BrowserOSAdapter] Getting snapshot for tab ${tabId} with type ${type}, newFormat: ${useNewFormat}, options: ${JSON.stringify(options)}`, | ||
| ); | ||
| 
               | 
          ||
| return new Promise<Snapshot>((resolve, reject) => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Return type mismatch: Promise is typed as Promise<Snapshot> but method signature returns Promise<SnapshotResult>
| return new Promise<Snapshot>((resolve, reject) => { | |
| return new Promise<SnapshotResult>((resolve, reject) => { | 
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/browser/BrowserOSAdapter.ts
Line: 434:434
Comment:
**syntax:** Return type mismatch: Promise is typed as `Promise<Snapshot>` but method signature returns `Promise<SnapshotResult>`
```suggestion
      return new Promise<SnapshotResult>((resolve, reject) => {
```
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| }, | ||
| ); | ||
| const callback = (snapshot: Snapshot) => { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Callback parameter type should be SnapshotResult not Snapshot to match union type
| const callback = (snapshot: Snapshot) => { | |
| const callback = (snapshot: SnapshotResult) => { | 
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/browser/BrowserOSAdapter.ts
Line: 435:435
Comment:
**syntax:** Callback parameter type should be `SnapshotResult` not `Snapshot` to match union type
```suggestion
        const callback = (snapshot: SnapshotResult) => {
```
How can I resolve this? If you propose a fix, please make it concise.| * @returns Formatted string with text and links | ||
| */ | ||
| async getTextWithLinksString(options?: SnapshotOptions): Promise<string> { | ||
| const snapshot = await this._browserOS.getSnapshot(this._tabId, 'links', options); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Should use 'text' type instead of 'links' for text-with-links mode - links are fetched with text type in new format
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/browser/BrowserPage.ts
Line: 945:945
Comment:
**logic:** Should use `'text'` type instead of `'links'` for text-with-links mode - links are fetched with text type in new format
How can I resolve this? If you propose a fix, please make it concise.getSnapshot fixes for Old Agent + new Agent
      
No description provided.