Skip to content

Use DataContent from Microsoft.Extensions.AI for data URI generation #7027

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.

Sign up for GitHub

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

Jump to bottom
Merged
adamsitnik merged 6 commits into from
Nov 10, 2025
Merged

Use DataContent from Microsoft.Extensions.AI for data URI generation #7027

adamsitnik merged 6 commits into from
Nov 10, 2025

Conversation

Copy link
Contributor

Copilot AI commented Nov 7, 2025 •
edited
Loading

Address remaining feedback from PR #7025:

  • Add reference to Microsoft.Extensions.AI.Abstractions package
  • Replace manual CreateDataUri method with DataContent class
  • Use MemoryStream.GetBuffer() with Length instead of ToArray() to avoid array allocations
  • Update using statements
  • Verify all tests still pass
  • Code review completed
  • All tests verified
  • Address PR feedback: remove redundant reference and optimize memory usage
  • Optimize MemoryStream allocation with pre-sizing
  • Use Math.Min for safe int casting
  • Defer data URI materialization until after MCP client creation

Latest Changes (addressing @adamsitnik feedback):

  • Changed ConvertToMarkdownAsync to accept DataContent instead of string URI
  • Materialize dataContent.Uri only after successfully creating the MCP client
  • Avoids unnecessary string allocation if the connection to the MCP server fails
  • All 4 unit tests pass across all target frameworks (net8.0, net9.0, net10.0)

Benefits:

  • Code reuse: Leveraging existing, well-tested DataContent infrastructure
  • Better performance:
    • Eliminated all unnecessary array allocations
    • Pre-sized MemoryStream buffers to avoid reallocation during stream copying
    • Deferred string allocation until after successful connection
  • Safer code: Handles edge cases with very large files gracefully
  • Cleaner code: Removed manual base64 encoding logic, pragma warnings, and redundant references
  • More concise: Consistent inline construction pattern for both overloads
Original prompt

Please submit a pr that addresses the remaining feedback on #7025 (comment)

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Microsoft Reviewers: Open in CodeFlow

@stephentoub
Use DataContent from Microsoft.Extensions.AI for data URI creation
ed3d082 
- Added reference to Microsoft.Extensions.AI.Abstractions
- Replaced manual CreateDataUri method with DataContent class
- Use MemoryStream.GetBuffer() with Length instead of ToArray() to avoid array allocations
- All existing tests pass

Co-authored-by: stephentoub <[email protected]>
Copilot AI changed the title [WIP] Address feedback on pull request 7025 Use DataContent from Microsoft.Extensions.AI for data URI generation Nov 7, 2025
Copilot AI requested a review from stephentoub November 7, 2025 21:19
stephentoub
stephentoub reviewed Nov 7, 2025
View reviewed changes
@stephentoub
Address PR feedback: remove redundant reference and simplify code
90ee687 
- Remove explicit Microsoft.Extensions.AI.Abstractions reference (available transitively)
- Change fileBytes from byte[] to ReadOnlyMemory<byte> to avoid ToArray()
- Simplify Stream reading code with inline DataContent construction

Co-authored-by: stephentoub <[email protected]>
Copilot AI requested a review from stephentoub November 7, 2025 22:08
stephentoub
stephentoub reviewed Nov 7, 2025
View reviewed changes
@stephentoub
Optimize MemoryStream allocation with pre-sizing
c8a2cfe 
- Seed MemoryStream with FileStream.Length for FileInfo overload
- Seed MemoryStream with source.Length when source.CanSeek for Stream overload
- Consolidate DataContent construction into single statement for FileInfo overload

Co-authored-by: stephentoub <[email protected]>
Copilot AI requested a review from stephentoub November 7, 2025 22:21
@stephentoub stephentoub marked this pull request as ready for review November 7, 2025 22:25
Copilot AI review requested due to automatic review settings November 7, 2025 22:25
@stephentoub stephentoub enabled auto-merge (squash) November 7, 2025 22:25
@stephentoub
Use Math.Min for safe int casting of stream lengths
c76000f 
- Replace (int)fs.Length with (int)Math.Min(int.MaxValue, fs.Length)
- Replace (int)source.Length with (int)Math.Min(int.MaxValue, source.Length)
- Prevents overflow when dealing with files/streams larger than int.MaxValue

Co-authored-by: stephentoub <[email protected]>
auto-merge was automatically disabled November 7, 2025 22:33

Head branch was pushed to by a user without write access

Copilot AI requested a review from stephentoub November 7, 2025 22:34
Copilot AI reviewed Nov 7, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the MarkItDownMcpReader class to use the DataContent class from Microsoft.Extensions.AI for creating data URIs instead of manually constructing them. This eliminates code duplication and leverages existing infrastructure.

Key changes:

  • Replaced manual base64 data URI construction with DataContent class usage
  • Changed from byte[] to ReadOnlyMemory<byte> for better memory efficiency
  • Removed the custom CreateDataUri method and associated pragma warnings

adamsitnik
adamsitnik reviewed Nov 10, 2025
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my new comment.

@adamsitnik
Defer data URI materialization until after MCP client creation
31cd994 
- Pass DataContent to ConvertToMarkdownAsync instead of string URI
- Materialize dataContent.Uri only after MCP client is successfully created
- Avoids unnecessary string allocation if connection fails

Co-authored-by: adamsitnik <[email protected]>
Copilot AI requested a review from adamsitnik November 10, 2025 13:36
adamsitnik
adamsitnik approved these changes Nov 10, 2025
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@adamsitnik adamsitnik merged commit b8ce04f into main Nov 10, 2025
6 checks passed
@adamsitnik adamsitnik deleted the copilot/address-feedback-from-pr-7025 branch November 10, 2025 15:35
jeffhandley pushed a commit to jeffhandley/extensions that referenced this pull request Nov 21, 2025
@stephentoub
@adamsitnik @jeffhandley
Use DataContent from Microsoft.Extensions.AI for data URI generation (d…
422c6dd 
…otnet#7027)

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: stephentoub <[email protected]>
Co-authored-by: adamsitnik <[email protected]>
@jeffhandley jeffhandley mentioned this pull request Nov 21, 2025
Merged
jeffhandley pushed a commit that referenced this pull request Nov 21, 2025
@stephentoub
@adamsitnik @jeffhandley
Use DataContent from Microsoft.Extensions.AI for data URI generation (#…
e3e0a6f 
…7027)

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: stephentoub <[email protected]>
Co-authored-by: adamsitnik <[email protected]>
This was referenced Dec 11, 2025
Closed
Closed
Closed
Closed
Open
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot Copilot left review comments

@adamsitnik adamsitnik adamsitnik approved these changes

@stephentoub stephentoub Awaiting requested review from stephentoub

Assignees

@stephentoub stephentoub

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stephentoub @adamsitnik