Skip to content

Pdf support, fix model selector#107

Closed
tanevanwifferen wants to merge 7 commits intonanbingxyz:mainfrom
tanevanwifferen:pdf-support
Closed

Pdf support, fix model selector#107
tanevanwifferen wants to merge 7 commits intonanbingxyz:mainfrom
tanevanwifferen:pdf-support

Conversation

@tanevanwifferen
Copy link

add pdf support for anthropic (pdfSupport is a property on IChatModel, can be implemented for other models too)
fix bug where the default model was always returned, so it could never be overwritten

Bug in pdf support is that the PDF doesn't show up in electron. A base64 string can't be larger than 2mb? or it won't show up in the electron renderer. Not sure how to fix that for now. Maybe render an image of the first page, or show an icon. For now it shows an empty iframe

@nanbingxyz nanbingxyz requested a review from Copilot March 11, 2025 01:12
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.

PR Overview

This PR introduces PDF support for the Anthropic chat service and addresses a bug where the default model was always returned, preventing overrides. Key changes include updating markdown handling to allow iframes for PDF display, modifying service logic to process documents (PDFs) via base64 or URL, and extending model/provider configurations to support PDF.

Reviewed Changes

File Description
src/hooks/useMarkdown.ts Updated markdown configuration comments to clarify iframe usage for PDF display.
src/intellichat/services/AnthropicChatService.ts Added handling for 'document' type messages, including base64 processing for PDFs.
src/consts.ts Added PDF MIME type.
src/utils/util.ts Updated utility functions and regexes to support media tags (embed, iframe) for PDFs.
src/providers/Anthropic.ts Extended model definitions to include pdfSupport configurations.
src/main/main.ts Modified file selection and base64 generation to support PDFs and updated event handling.
src/hooks/useChatContext.ts Adjusted context dependencies to better reflect changes in chat and API state.
src/renderer/pages/chat/Editor/Toolbar/ImgCtrl.tsx Introduced embed support toggle for PDFs in the image control dialog.
src/intellichat/types.ts Added new 'document' message type.
src/providers/types.ts Added pdfSupport to the IChatModel type.
src/renderer/pages/chat/Editor/Toolbar/ModelCtrl.tsx Improved active model reactivity and updated model mapping handling.
src/renderer/pages/chat/Editor/index.tsx Replaced tag filtering function to preserve both image and document (PDF) media tags.

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/intellichat/services/AnthropicChatService.ts:108

  • Consider adding a check to verify that splitting the base64 string using a comma always yields a valid value. This could prevent potential issues if the input format unexpectedly changes or does not contain a comma.
data = item.data.split(',')[1]; // remove data:application/pdf;base64,

src/main/main.ts:196

  • Review whether the commented-out ingest call is intentional. If it's a temporary debugging change, consider adding a comment explaining its purpose or removing it before production.
//axiom.ingest(data);

@nanbingxyz
Copy link
Owner

Hey, bro, it's really appreciate your work. Currently, I'm about to merge a major dev branch (feat/folder) into main that contains significant changes(including the bug fix you mentioned here, also updated the model definition in the Provider configuration. The model's display now uses the label, while the value is based on the key of each mode).

I plan to release a new version this week, now merging the PR would introduce some conflicts, given that PDF support still has some issues, could we submit the PR to the feat/claude-pdf-support branch? We can release it after further improvements.

@tanevanwifferen
Copy link
Author

that's an option. otherwise I can also merge main once you're done, fix the conflicts myself. Whatever you want

@nanbingxyz
Copy link
Owner

The latest code has been merged into the main branch. Could you please resolve any merge conflicts and resubmit? Let me know if you need assistance.

@tanevanwifferen
Copy link
Author

@nanbingxyz I'm trying to merge, but there's a bunch of husky errors? Can I skip it in some way?

@nanbingxyz
Copy link
Owner

@nanbingxyz I'm trying to merge, but there's a bunch of husky errors? Can I skip it in some way?

We use Husky to keep improving our code quality, though you can skip it if your code is well-tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants