Skip to content

集成 AWS Bedrock 作为新的 LLM 服务提供商 #6430

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ac1982
Copy link

@ac1982 ac1982 commented Apr 6, 2025

💻 变更类型 | Change Type

  • feat
  • fix
  • refactor
  • perf
  • style
  • test
  • docs
  • ci
  • chore
  • build

🔀 变更说明 | Description of Change

集成 AWS Bedrock 作为新的 LLM 服务提供商。用户现在可以通过配置 AWS 凭证和区域,在 NextChat 中使用 Bedrock 托管的模型。

主要变更包括:

  • 添加 @aws-sdk/client-bedrock-runtime 依赖。
  • app/constant.ts 中添加了 Bedrock 相关的枚举和模型定义。
  • 更新了 app/config/server.ts 以读取和处理 Bedrock 的环境变量配置。
  • 创建了后端 API 处理器 app/api/bedrock/index.ts,负责:
    • 调用 AWS Bedrock Runtime API (InvokeModel 和 InvokeModelWithResponseStream)。
    • 处理流式和非流式响应。
    • 将 Bedrock 的响应格式化为 OpenAI 兼容的格式。
  • 更新了动态 API 路由 app/api/[provider]/[...path]/route.ts 以将 /api/bedrock/... 请求分发给 Bedrock 处理器。
  • 更新了认证逻辑 app/api/auth.ts 以跳过为 Bedrock 请求添加 Authorization 头。
  • 创建了前端 Bedrock API 客户端 app/client/platforms/bedrock.ts
  • 更新了前端 API 工厂 app/client/api.ts (ClientApi 构造函数和 getClientApi) 以支持 Bedrock 客户端。
  • 更新了 .env.template 添加 Bedrock 相关配置项,并提供了一个使用 CUSTOM_MODELS 重命名 Bedrock 模型名称的示例。

📝 补充信息 | Additional Information

  • 此实现目前主要测试了 Bedrock 上的 Anthropic Claude 模型(例如 us.anthropic.claude-3-5-sonnet-20241022-v2:0),因为其 API 结构与本项目中的其他 Anthropic 实现类似。理论上支持其他需要类似输入/输出格式的 Bedrock 模型,但可能需要调整 Payload 格式。
  • 用户需要在 .env 文件中提供有效的 AWS_ACCESS_KEY_IDAWS_SECRET_ACCESS_KEYAWS_REGION,并将 ENABLE_AWS_BEDROCK 设置为 true 来启用此功能。
  • 可以使用 CUSTOM_MODELS 环境变量来添加或重命名 Bedrock 模型,例如:+us.anthropic.claude-3-5-sonnet-20241022-v2:0@bedrock=sonnet

Summary by CodeRabbit

  • New Features

    • Refined custom model selection for enhanced precision.
    • Introduced AWS Bedrock support with new configuration options for credentials, region, and endpoints.
    • Added new API capabilities to process Bedrock requests, supporting both streaming and non-streaming interactions.
    • Extended client-side functionality to leverage the new Bedrock service for chat completions, usage reporting, and more.
  • Refactor

    • Streamlined authentication and provider selection with enhanced logging and error handling.
  • Chores

    • Updated configuration settings and dependencies to incorporate AWS Bedrock integration.

AC added 5 commits April 6, 2025 00:46
Adds support for using models hosted on AWS Bedrock, specifically Anthropic Claude models.

Key changes:
- Added '@aws-sdk/client-bedrock-runtime' dependency.
- Updated constants, server config, and auth logic for Bedrock.
- Implemented backend API handler () to communicate with the Bedrock API, handling streaming and non-streaming responses, and formatting output to be OpenAI compatible.
- Updated dynamic API router () to dispatch requests to the Bedrock handler.
- Created frontend client () and updated client factory ().
- Updated  with necessary Bedrock environment variables (AWS keys, region, enable flag) and an example for using  to alias Bedrock models.
- Added logging to `getClientApi` for better debugging of provider input and standardized provider names.
- Updated `ChatActions` to handle lowercase provider IDs, converting them to TitleCase for consistency with the ServiceProvider enum.
- Implemented defaulting to OpenAI when provider ID is missing and added relevant logging for session updates.
- Enhanced logging in `useChatStore` to track API call preparations and provider configurations.
- Enhanced error messages in `getAwsCredentials` to provide clearer feedback when AWS Bedrock is not configured properly.
- Added checks for missing Bedrock Access Key ID and Secret Access Key, with specific error messages for each case.
- Updated logging in the Bedrock API handler to include detailed information about the request model, stream status, and message count for improved debugging and monitoring.
Copy link

vercel bot commented Apr 6, 2025

Someone is attempting to deploy a commit to the NextChat Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

coderabbitai bot commented Apr 6, 2025

Walkthrough

This pull request integrates AWS Bedrock support into the application. Configuration changes in the .env.template add AWS-related variables and refine the custom models setup. New API routes and handlers, including a dedicated Bedrock API handler and authentication adjustments, are introduced. Client-side modifications enable instantiation of a new BedrockApi class that implements methods for chat, usage, and more. Additionally, enumerations and constants are updated to include Bedrock, and a new AWS SDK dependency is added to support runtime integration.

Changes

File(s) Change Summary
.env.template Updated CUSTOM_MODELS variable to specify explicit models; added a new "AWS Bedrock Section" with environment variables (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_REGION, ENABLE_AWS_BEDROCK, AWS_BEDROCK_ENDPOINT).
app/api/[provider]/[...path]/route.ts Added a new case for ApiPath.Bedrock in the route handler and imported bedrockHandler to support the Bedrock API endpoint.
app/api/auth.ts Removed obsolete commented code for GeminiPro/Azure; added a case for ModelProvider.Bedrock with logging and early exit using AWS credentials; enhanced log messages to include modelProvider.
app/api/bedrock/index.ts New file: Implements the Bedrock API handler with request validation, options for streaming/non-streaming responses, error handling, and formatting as Server-Sent Events (SSE).
app/client/api.ts Modified getClientApi to accept a string representation for the provider; added cases for ModelProvider.Bedrock and ServiceProvider.Bedrock to instantiate BedrockApi.
app/client/platforms/bedrock.ts New file: Introduces a BedrockApi class implementing the LLMApi interface with methods for chat (supporting streaming and non-streaming), usage, models, speech, and endpoint path construction.
app/components/chat.tsx Updated the onSelection handler to log raw selections; parsed the provider with error handling and conditionally adjusted toast messaging based on provider type.
app/config/server.ts Removed commented code for multiple OpenAI API keys; added new logic to detect AWS Bedrock enablement and incorporated new config properties (bedrockRegion, bedrockAccessKeyId, bedrockSecretAccessKey, bedrockEndpoint).
app/constant.ts Added new enum entries for Bedrock in ApiPath, ServiceProvider, and ModelProvider; defined a new constant for Bedrock with a ChatPath for completions.
app/store/chat.ts Enhanced logging for API client instantiation; ensured a valid provider name is used (defaulting to OpenAI if unspecified) during the chat session handling.
package.json Added dependency: "@aws-sdk/client-bedrock-runtime": "^3.782.0".

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant AP as API Route Handler
    participant BH as Bedrock Handler
    participant AWS as AWS Bedrock Service
    participant CL as BedrockApi (Client)

    C->>AP: Send request to Bedrock endpoint
    AP->>BH: Forward request to bedrockHandler
    BH->>AWS: Invoke model command (streaming or non-streaming)
    AWS-->>BH: Return response (or stream data)
    BH-->>AP: Format and relay response
    AP-->>C: Deliver final response
Loading

Suggested reviewers

  • Dogtiti

Poem

I'm a rabbit, hopping through the code,
Bedrock's in place on a brand new road.
AWS tunes and models in array,
Configs and handlers lead the way.
With each hop, our code sings—hip-hip-hooray!
🐰✨

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (11)
app/store/chat.ts (1)

459-478: Enhanced API call logging for debugging Bedrock integration.

The added logging helps debug provider resolution, particularly checking if the provider is Bedrock. This is valuable during integration but consider cleaning up excessive logging in production code.

Consider:

  1. Using structured logging instead of multiple concatenated values
  2. Removing or conditionalizing verbose logging in production
  3. Standardizing language (mixing Chinese comments with English logs may confuse non-Chinese speakers)
const providerNameFromConfig = modelConfig.providerName;
-console.log(
-  "[onUserInput] Preparing API call. Provider from config:",
-  providerNameFromConfig,
-  "| Type:",
-  typeof providerNameFromConfig,
-  "| Is Enum value (Bedrock)?:",
-  providerNameFromConfig === ServiceProvider.Bedrock, // 与枚举比较
-  "| Is 'Bedrock' string?:",
-  providerNameFromConfig === "Bedrock", // 与字符串比较
-  "| Model:",
-  modelConfig.model,
-);
+if (process.env.NODE_ENV !== 'production') {
+  console.log("[onUserInput] API call details:", {
+    provider: providerNameFromConfig,
+    type: typeof providerNameFromConfig,
+    isBedrockEnum: providerNameFromConfig === ServiceProvider.Bedrock,
+    isBedrockString: providerNameFromConfig === "Bedrock",
+    model: modelConfig.model,
+  });
+}
app/components/chat.tsx (1)

696-709: Consider configurable log levels.
Lines 697-709 add multiple console logs that may be too verbose for production environments. Consider using a logging framework or adjusting log verbosity levels to avoid clutter.

app/api/auth.ts (1)

117-120: Evaluate sensitive logging.
Displaying the provider name in logs may be helpful for debugging but can be unnecessary in multi-tenant or production setups. Assess whether to remove or downgrade this log to reduce potential noise or confusion.

app/api/bedrock/index.ts (5)

14-32: Add fallback or improved error messages.
Presently, getAwsCredentials() throws generic errors if Bedrock is misconfigured. For a more user-friendly experience, consider including instructions or next steps in the error messages to help resolve the missing or incorrect environment variables.


34-58: Streamline subpath authorization logic.
Currently, only one path from ALLOWED_PATH is authorized. If future expansions include multiple Bedrock endpoints, consider using a more scalable approach (e.g., partially matching known subpath patterns) to reduce manual maintenance of the ALLOWED_PATH set.


106-127: Plan for broader model support.
Currently, only Anthropic Claude-based models are supported. If you plan to integrate other Bedrock models (e.g., Amazon Titan), building a more generic payload construction mechanism here will reduce refactoring later.


207-248: Return "finish_reason" carefully.
The non-streaming path always uses "stop" for finish_reason. In some edge cases, the model may reach a max token limit or encounter an error mid-generation. Differentiate these scenarios for a more accurate representation of why the generation ended.


250-253: Log details on errors.
While the error is returned to the client, consider logging additional context (e.g., command parameters, partial responses) for troubleshooting, ensuring sensitive data (like AWS keys) is excluded.

app/config/server.ts (1)

177-177: Consider grouping Bedrock config under a single object.

Currently, you return separate properties for bedrockRegion, bedrockAccessKeyId, bedrockSecretAccessKey, etc. Grouping them into a nested object (e.g., bedrockConfig) within the returned config may improve clarity and detect misconfigurations more easily.

 export const getServerSideConfig = () => {
   ...
+  const bedrockConfig = {
+    isEnabled: isBedrock,
+    region: process.env.AWS_REGION,
+    accessKeyId: process.env.AWS_ACCESS_KEY_ID,
+    secretAccessKey: process.env.AWS_SECRET_ACCESS_KEY,
+    endpoint: process.env.AWS_BEDROCK_ENDPOINT,
+  };
   const config = {
     ...
-    isBedrock,
-    bedrockRegion: process.env.AWS_REGION,
-    bedrockAccessKeyId: process.env.AWS_ACCESS_KEY_ID,
-    bedrockSecretAccessKey: process.env.AWS_SECRET_ACCESS_KEY,
-    bedrockEndpoint: process.env.AWS_BEDROCK_ENDPOINT,
+    bedrockConfig,
     ...
   };
   return config;
 };

Also applies to: 248-253

app/client/platforms/bedrock.ts (2)

16-120: Check streaming chunk parsing reliability.

  1. Splitting lines by “\n” could fail if partial JSON lines span multiple chunks. This might lead to JSON parse errors or data loss.
  2. Consider buffering partial lines across chunks to handle SSE streaming more robustly.

Everything else, including comfort calls for onUpdate, onFinish, and onError, looks consistent.

+ // Potential partial line buffer
 let leftover = "";
 while (true) {
   const { done, value } = await reader.read();
   if (done) break;

-  const text = decoder.decode(value, { stream: true });
-  const lines = text.split("\n");
+  const chunk = leftover + decoder.decode(value, { stream: true });
+  const lines = chunk.split("\n");
+  leftover = lines.pop() ?? ""; // store incomplete line

   for (const line of lines) {
     ...
   }
 }

136-140: No direct TTS support.

Throwing an error in speech is reasonable if AWS Bedrock TTS is not yet implemented. Consider returning a user-friendly error message or a specialized exception if you decide to add partial TTS handling later.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 48469bd and a5caf98.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (11)
  • .env.template (2 hunks)
  • app/api/[provider]/[...path]/route.ts (2 hunks)
  • app/api/auth.ts (2 hunks)
  • app/api/bedrock/index.ts (1 hunks)
  • app/client/api.ts (4 hunks)
  • app/client/platforms/bedrock.ts (1 hunks)
  • app/components/chat.tsx (1 hunks)
  • app/config/server.ts (3 hunks)
  • app/constant.ts (4 hunks)
  • app/store/chat.ts (1 hunks)
  • package.json (1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
app/api/bedrock/index.ts (2)
app/config/server.ts (1)
  • getServerSideConfig (128-275)
app/api/auth.ts (1)
  • auth (27-129)
app/store/chat.ts (1)
app/client/api.ts (2)
  • ClientApi (136-229)
  • getClientApi (363-436)
app/client/api.ts (1)
app/client/platforms/bedrock.ts (1)
  • BedrockApi (8-140)
app/client/platforms/bedrock.ts (2)
app/client/api.ts (5)
  • ChatOptions (76-86)
  • getHeaders (244-361)
  • LLMUsage (88-91)
  • LLMModel (93-99)
  • SpeechOptions (67-74)
app/constant.ts (1)
  • Bedrock (272-274)
🔇 Additional comments (18)
package.json (1)

24-24: Added AWS Bedrock SDK dependency.

The addition of @aws-sdk/client-bedrock-runtime is necessary for integrating AWS Bedrock as a new LLM provider. This aligns with the PR objective of supporting AWS Bedrock-hosted models.

app/api/[provider]/[...path]/route.ts (2)

17-17: Added Bedrock handler import.

The import statement for the Bedrock handler follows the same pattern as other provider handlers. This is a clean implementation.


54-55: Added Bedrock API path routing.

The case statement for ApiPath.Bedrock correctly routes requests to the Bedrock handler, maintaining consistency with the existing API routing pattern.

app/constant.ts (4)

75-75: Added Bedrock to ApiPath enum.

The addition follows the consistent naming pattern used for other providers.


134-134: Added Bedrock to ServiceProvider enum.

This is necessary for client-side provider identification and follows the established pattern.


161-161: Added Bedrock to ModelProvider enum.

Properly integrates Bedrock into the model provider ecosystem for use in model selection logic.


272-274: Added Bedrock constant with ChatPath.

The Bedrock constant correctly defines the API path for chat completions. Note that unlike some other providers (e.g., OpenAI), this only defines ChatPath without other capabilities like speech or image generation.

Is this intentional or are you planning to add additional capabilities for Bedrock in future updates? Some AWS Bedrock models support image generation and other multimodal capabilities that could be added later.

app/store/chat.ts (1)

475-478: Improved API client instantiation with fallback.

The modification correctly handles cases where providerNameFromConfig might be undefined by providing a default value of ServiceProvider.OpenAI. This increases robustness.

app/client/api.ts (3)

27-27: Use consistent import grouping.

While this import correctly references the new BedrockApi, ensure consistent import grouping or ordering if your coding guidelines require alphabetical or categorized ordering. Otherwise, this is fine.


177-179: Good extension for Bedrock provider.

Adding a ModelProvider.Bedrock case is straightforward and consistent with the existing pattern. No issues detected.


363-434: Check for fallback correctness and future expansions.

  1. The conversion logic for string-based providers to ServiceProvider enum is well-structured. Logging the standardized provider aids debugging.
  2. The final default case falling back to GPT is appropriate, but ensure that your team is aligned on defaulting to GPT if an unknown provider string is received. This behavior might hide configuration errors.

No immediate issues, but consider adding additional known lowercase mappings (e.g., "anthropic", "baidu") to unify your usage across the codebase.

Would you like me to scan for other occurrences of string-based providers in the repository to suggest more mappings?

app/config/server.ts (2)

167-171: Validate AWS credentials for Bedrock.

The isBedrock logic correctly checks environment variables for AWS credentials. Ensure that you handle scenarios where credentials are invalid or missing at runtime, especially if ENABLE_AWS_BEDROCK is set to "true" but credentials are incorrect.

Would you like a script to search for usage of these environment variables and confirm they are validated properly?


274-274: Clear separation of concerns.

Wrapping the configuration in a config object prior to returning is a neat approach — it decouples environment variable loading from usage. Code looks good.

.env.template (2)

60-60: Verify custom models correctness.

You have removed the -all option and added new models, including a bedrock-based model named sonnet. Ensure these custom tokens are recognized by the back end. Missing or mismatched model references could lead to unexpected errors.

Please confirm that “us.anthropic.claude-3-5-sonnet-20241022-v2:0@bedrock=sonnet” is a valid internal or Bedrock model reference. If needed, I can provide a script to locate all references to “sonnet” in the codebase.


85-103: Follow best practices for secrets in templates.

Defining placeholders for AWS credentials indicates good security hygiene. Be sure to remove or comment these lines out if you do not want new developers to accidentally commit real credentials.

app/client/platforms/bedrock.ts (3)

1-14: Ensure bedrock path constants are validated.

The path method concatenates ApiPath.Bedrock with a subpath. The final endpoint depends on your backend route definitions. Validate that ApiPath.Bedrock is correct and that backend routes match this prefix.


122-128: Usage reporting placeholder.

Returning static usage data is acceptable as a placeholder. Ensure you revisit this method if future usage/billing metrics for Bedrock become relevant.


130-134: Model fetch is intentionally unimplemented.

Acknowledging that dynamic model retrieval from Bedrock is complex, the empty array fallback is fine for now. No issues.

Comment on lines +99 to 104
case ModelProvider.Bedrock:
console.log(
"[Auth] Using AWS credentials for Bedrock, no API key override.",
);
return { error: false };
case ModelProvider.GPT:
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Verify AWS credentials before returning.
This block immediately returns { error: false } for Bedrock, skipping an explicit credential validation step. Consider ensuring that AWS credential checks (e.g., presence of required environment variables) have definitively passed before returning success.

🧰 Tools
🪛 Biome (1.9.4)

[error] 104-104: Useless case clause.

because the default clause is present:

Unsafe fix: Remove the useless case.

(lint/complexity/noUselessSwitchCase)

Comment on lines +128 to +206
if (stream) {
const command = new InvokeModelWithResponseStreamCommand({
modelId: model,
contentType: "application/json",
accept: "application/json",
body: JSON.stringify(payload),
});
const response = await client.send(command);

if (!response.body) {
throw new Error("Empty response stream from Bedrock");
}
const responseBody = response.body;

const encoder = new TextEncoder();
const decoder = new TextDecoder();
const readableStream = new ReadableStream({
async start(controller) {
try {
for await (const event of responseBody) {
if (event.chunk?.bytes) {
const chunkData = JSON.parse(decoder.decode(event.chunk.bytes));
let responseText = "";
let finishReason: string | null = null;

if (
chunkData.type === "content_block_delta" &&
chunkData.delta.type === "text_delta"
) {
responseText = chunkData.delta.text || "";
} else if (chunkData.type === "message_stop") {
finishReason =
chunkData["amazon-bedrock-invocationMetrics"]
?.outputTokenCount > 0
? "stop"
: "length"; // Example logic
}

// Format as OpenAI SSE chunk
const sseData = {
id: `chatcmpl-${nanoid()}`,
object: "chat.completion.chunk",
created: Math.floor(Date.now() / 1000),
model: model,
choices: [
{
index: 0,
delta: { content: responseText },
finish_reason: finishReason,
},
],
};
controller.enqueue(
encoder.encode(`data: ${JSON.stringify(sseData)}\n\n`),
);

if (finishReason) {
controller.enqueue(encoder.encode("data: [DONE]\n\n"));
break; // Exit loop after stop message
}
}
}
} catch (error) {
console.error("[Bedrock] Streaming error:", error);
controller.error(error);
} finally {
controller.close();
}
},
});

return new NextResponse(readableStream, {
headers: {
"Content-Type": "text/event-stream",
"Cache-Control": "no-cache",
Connection: "keep-alive",
},
});
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle partial chunk scenarios.
While streaming data, a partial chunk may be returned from Bedrock if the chunk boundaries don't align with JSON structures. Consider buffering incomplete segments before parsing to prevent JSON parse errors.

- Modified the assignment of `providerName` in `ChatActions` to assert non-null, ensuring type safety and preventing potential runtime errors.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (1)
app/components/chat.tsx (1)

710-733: 🛠️ Refactor suggestion

Improved provider ID handling with case normalization.

The code now properly handles the conversion from lowercase provider IDs (e.g., "bedrock") to TitleCase format required by the ServiceProvider enum. This is a good improvement that will help maintain consistency when new providers are added.

However, when an unknown provider ID is encountered, the code only logs an error without setting a fallback provider, which could lead to undefined behavior.

Consider adding a fallback to OpenAI for unknown providers, similar to how you handle missing providers:

 } else {
   console.error(
     `[ChatActions] Unknown provider ID: ${providerId}`,
   );
-  // Handle error or fallback if needed
+  // Fallback to OpenAI for unknown providers
+  targetProvider = ServiceProvider.OpenAI;
+  console.warn(
+    `[ChatActions] Unknown provider ID: ${providerId}, defaulting to OpenAI.`,
+  );
 }
🧹 Nitpick comments (1)
app/components/chat.tsx (1)

740-750: Non-null assertion on targetProvider could be improved.

Line 744 uses a non-null assertion (targetProvider!) which may cause runtime errors if targetProvider is somehow null despite the enclosing if check.

Consider a safer approach:

 if (targetProvider) {
   // Only update if we found a valid provider
   chatStore.updateTargetSession(session, (session) => {
     session.mask.modelConfig.model = model as ModelType;
-    session.mask.modelConfig.providerName = targetProvider!; // Use the Enum value (Assert non-null)
+    session.mask.modelConfig.providerName = targetProvider; // Already checked in if condition
     session.mask.syncGlobalConfig = false;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a5caf98 and 9a865fd.

📒 Files selected for processing (1)
  • app/components/chat.tsx (1 hunks)
🔇 Additional comments (2)
app/components/chat.tsx (2)

697-708: Good addition of detailed logging for debugging provider selection.

Adding logging for both raw and parsed values will help with troubleshooting model selection issues that may arise when implementing a new provider like Bedrock.


751-760: Improved provider-specific toast message display.

The code now correctly displays different toast messages based on the provider type, which improves the user experience by showing more relevant information.

- Removed redundant Bedrock configuration check in the API handler for cleaner code.
- Enhanced logging in `ChatActions` to provide clearer feedback when an unknown provider ID is encountered, defaulting to OpenAI when necessary.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (2)
app/api/bedrock/index.ts (2)

60-70: 🛠️ Refactor suggestion

Consolidate configuration checks.

The server-side config is retrieved twice - once at line 61 and again inside getAwsCredentials() called at line 68. This creates redundant code paths and potential inconsistencies.

 try {
-    const config = getServerSideConfig();
+    const credentials = getAwsCredentials();
+    const config = getServerSideConfig();
 
     const bedrockRegion = config.bedrockRegion as string;
     const bedrockEndpoint = config.bedrockEndpoint;
 
     const client = new BedrockRuntimeClient({
       region: bedrockRegion,
-      credentials: getAwsCredentials(),
+      credentials: credentials,
       endpoint: bedrockEndpoint || undefined,
     });

Alternatively, modify getAwsCredentials() to return both credentials and the config:

 function getAwsCredentials() {
   const config = getServerSideConfig();
   if (!config.isBedrock) {
     throw new Error(
       "AWS Bedrock is not configured properly (ENABLE_AWS_BEDROCK is not true)",
     );
   }
   if (!config.bedrockAccessKeyId) {
     throw new Error("AWS Bedrock Access Key ID is missing or empty.");
   }
   if (!config.bedrockSecretAccessKey) {
     throw new Error("AWS Bedrock Secret Access Key is missing or empty.");
   }
   return {
+    config,
     accessKeyId: config.bedrockAccessKeyId as string,
     secretAccessKey: config.bedrockSecretAccessKey as string,
   };
 }

140-142: 🛠️ Refactor suggestion

Handle partial chunk scenarios.

When streaming data from Bedrock, a partial chunk may be returned if the chunk boundaries don't align with complete JSON structures. Direct parsing could lead to JSON parse errors.

Implement a buffer for incomplete JSON:

try {
+  let buffer = '';
   for await (const event of responseBody) {
     if (event.chunk?.bytes) {
-      const chunkData = JSON.parse(decoder.decode(event.chunk.bytes));
+      buffer += decoder.decode(event.chunk.bytes);
+      
+      // Try to parse complete JSON objects from the buffer
+      try {
+        const chunkData = JSON.parse(buffer);
+        // Reset buffer after successful parse
+        buffer = '';
🧹 Nitpick comments (5)
app/api/bedrock/index.ts (5)

248-249: Move import statement to the top of the file.

The import for nanoid should be moved to the top of the file with other imports to follow standard import organization practices.

-// Need nanoid for unique IDs
-import { nanoid } from "nanoid";

Add this to the imports section at the top:

import {
  BedrockRuntimeClient,
  InvokeModelWithResponseStreamCommand,
  InvokeModelCommand,
} from "@aws-sdk/client-bedrock-runtime";
+import { nanoid } from "nanoid";

152-157: Add documentation for finish reason logic.

The determination of finish reason is complex and would benefit from clearer documentation explaining why different finish reasons are assigned based on token counts.

 } else if (chunkData.type === "message_stop") {
+  // Determine finish reason based on output token count:
+  // - "stop" when tokens were generated (normal completion)
+  // - "length" when no tokens were generated (likely hit max token limit)
   finishReason =
     chunkData["amazon-bedrock-invocationMetrics"]
       ?.outputTokenCount > 0
       ? "stop"
       : "length"; // Example logic
 }

90-97: Add support for non-Claude Bedrock models.

The implementation currently only supports Claude models from Anthropic on Bedrock, rejecting other models. This limits the flexibility of the Bedrock integration.

Consider implementing adapters for other Bedrock models like Llama, Titan, or Cohere to expand the available options:

 // --- Payload formatting for Claude on Bedrock ---
 const isClaudeModel = model.includes("anthropic.claude");
-if (!isClaudeModel) {
-  return NextResponse.json(
-    { error: true, msg: "Unsupported Bedrock model: " + model },
-    { status: 400 },
-  );
-}
+
+let payload;
+if (isClaudeModel) {
+  // Claude payload formatting
+  const systemPrompts = messages.filter((msg: any) => msg.role === "system");
+  const userAssistantMessages = messages.filter(
+    (msg: any) => msg.role !== "system",
+  );
+
+  payload = {
+    anthropic_version: "bedrock-2023-05-31",
+    max_tokens: max_tokens || 4096,
+    temperature: temperature,
+    messages: userAssistantMessages.map((msg: any) => ({
+      role: msg.role, // 'user' or 'assistant'
+      content:
+        typeof msg.content === "string"
+          ? [{ type: "text", text: msg.content }]
+          : msg.content,
+    })),
+    ...(systemPrompts.length > 0 && {
+      system: systemPrompts.map((msg: any) => msg.content).join("\n"),
+    }),
+  };
+} else if (model.includes("llama")) {
+  // Add Llama model support
+  // Format payload according to Llama requirements
+} else if (model.includes("amazon.titan")) {
+  // Add Titan model support
+  // Format payload according to Titan requirements 
+} else {
+  return NextResponse.json(
+    { error: true, msg: "Unsupported Bedrock model: " + model },
+    { status: 400 },
+  );
+}

108-118: Improve content type validation for messages.

The current implementation assumes the message content is either a string or already in the expected format, but doesn't validate the structure for non-string content.

Add explicit validation for non-string content to ensure it matches Claude's expected format:

 messages: userAssistantMessages.map((msg: any) => ({
   role: msg.role, // 'user' or 'assistant'
   content:
     typeof msg.content === "string"
       ? [{ type: "text", text: msg.content }]
-      : msg.content, // Assuming MultimodalContent format is compatible
+      : validateMessageContent(msg.content), // Validate non-string content
 })),

+// Add this validation function elsewhere in the file
+function validateMessageContent(content: any) {
+  // Validate that content is an array
+  if (!Array.isArray(content)) {
+    throw new Error("Non-string message content must be an array");
+  }
+  
+  // Validate each content item has required properties
+  for (const item of content) {
+    if (!item.type) {
+      throw new Error("Each content item must have a 'type' property");
+    }
+    
+    if (item.type === "text" && typeof item.text !== "string") {
+      throw new Error("Text content items must have a valid 'text' property");
+    }
+    // Add additional validation for other content types as needed
+  }
+  
+  return content;
+}

227-238: Simplify token usage calculation.

The current token usage calculation logic is unnecessarily complex with multiple nullish coalescing operators and conditional expressions.

 usage: {
-  prompt_tokens:
-    responseBody["amazon-bedrock-invocationMetrics"]?.inputTokenCount ??
-    -1,
-  completion_tokens:
-    responseBody["amazon-bedrock-invocationMetrics"]
-      ?.outputTokenCount ?? -1,
-  total_tokens:
-    (responseBody["amazon-bedrock-invocationMetrics"]
-      ?.inputTokenCount ?? 0) +
-      (responseBody["amazon-bedrock-invocationMetrics"]
-        ?.outputTokenCount ?? 0) || -1,
+  prompt_tokens: responseBody["amazon-bedrock-invocationMetrics"]?.inputTokenCount ?? -1,
+  completion_tokens: responseBody["amazon-bedrock-invocationMetrics"]?.outputTokenCount ?? -1,
+  total_tokens: calculateTotalTokens(responseBody),
 },

+// Add this helper function
+function calculateTotalTokens(responseBody: any) {
+  const metrics = responseBody["amazon-bedrock-invocationMetrics"];
+  if (!metrics) return -1;
+  
+  const inputTokens = metrics.inputTokenCount ?? 0;
+  const outputTokens = metrics.outputTokenCount ?? 0;
+  
+  return inputTokens + outputTokens || -1;
+}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9a865fd and 3aae552.

📒 Files selected for processing (2)
  • app/api/bedrock/index.ts (1 hunks)
  • app/components/chat.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/components/chat.tsx
🧰 Additional context used
🧬 Code Definitions (1)
app/api/bedrock/index.ts (2)
app/config/server.ts (1)
  • getServerSideConfig (128-275)
app/api/auth.ts (1)
  • auth (27-129)

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.

1 participant