Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

brunobergher
Copy link
Collaborator

Related GitHub Issue

Closes: #

Roo Code Task Context (Optional)

Description

Test Procedure

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Testing: New and/or updated tests have been added to cover my changes (if applicable).
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

Documentation Updates

Additional Notes

Get in Touch

Comment on lines +126 to +135
it("should handle missing API configuration", async () => {
const result = await TitleSummarizer.summarizeTitle({
text: "Some text",
apiConfiguration: undefined as any,
maxLength: 150,
})

expect(result.success).toBe(false)
expect(result.error).toBe("No API configuration available")
expect(result.summarizedTitle).toBe("Some text")
Copy link

Choose a reason for hiding this comment

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

This test has incorrect expectations. When text is "Some text" (9 characters) and maxLength is 150, the early return on line 46 of titleSummarizer.ts will execute, returning { success: true, summarizedTitle: text } without ever validating the API configuration.

The test expects success: false and an error about missing API configuration, but this validation never occurs due to the short text length.

To properly test missing API configuration handling, use text longer than maxLength to bypass the early return:

const longText = "a".repeat(151) // Exceeds maxLength of 150
const result = await TitleSummarizer.summarizeTitle({
  text: longText,
  apiConfiguration: undefined as any,
  maxLength: 150,
})

expect(result.success).toBe(false)
expect(result.error).toBe("No valid API configuration provided")

Note: The error message should also be "No valid API configuration provided" (from singleCompletionHandler.ts:14) not "No API configuration available".

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Oct 13, 2025
Comment on lines +152 to +166
it("should use custom support prompts when provided", async () => {
const customPrompts = {
SUMMARIZE_TITLE: "Custom summarization prompt: {{userInput}} (max 150 chars)",
}

// Short text doesn't need summarization
const result = await TitleSummarizer.summarizeTitle({
text: "Text to summarize",
apiConfiguration: mockApiConfiguration,
customSupportPrompts: customPrompts,
maxLength: 150,
})

expect(result.success).toBe(true)
expect(result.summarizedTitle).toBe("Text to summarize")
Copy link

Choose a reason for hiding this comment

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

This test doesn't actually verify custom support prompts functionality. The text "Text to summarize" (18 characters) is shorter than maxLength (150), triggering the early return on line 46 that bypasses prompt creation entirely.

To properly test custom support prompts:

const customPrompts = {
  SUMMARIZE_TITLE: "Custom summarization prompt: {{userInput}} (max 150 chars)",
}
const longText = "a".repeat(151)

const result = await TitleSummarizer.summarizeTitle({
  text: longText,
  apiConfiguration: mockApiConfiguration,
  customSupportPrompts: customPrompts,
  maxLength: 150,
})

// Verify the custom prompt was actually used
expect(singleCompletionHandler).toHaveBeenCalledWith(
  mockApiConfiguration,
  expect.stringContaining("Custom summarization prompt")
)

@@ -0,0 +1,125 @@
import { ProviderSettings, ClineMessage, TelemetryEventName } from "@roo-code/types"
Copy link

Choose a reason for hiding this comment

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

Unused import: ClineMessage is imported but never used in this file. Please remove it to keep imports clean.

Comment on lines +1 to +6
import { ProviderSettings, ClineMessage, TelemetryEventName } from "@roo-code/types"
import { TelemetryService } from "@roo-code/telemetry"
import { supportPrompt } from "../../shared/support-prompt"
import { singleCompletionHandler } from "../../utils/single-completion-handler"
import { ProviderSettingsManager } from "../config/ProviderSettingsManager"
import { ClineProvider } from "./ClineProvider"
Copy link

Choose a reason for hiding this comment

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

Unused imports: Both TelemetryEventName and ClineProvider are imported but never used in this file. The ClineProvider import (line 6) is particularly unnecessary since there's no reference to it anywhere.

Please remove both unused imports:

import { ProviderSettings } from "@roo-code/types"
import { TelemetryService } from "@roo-code/telemetry"
import { supportPrompt } from "../../shared/support-prompt"
import { singleCompletionHandler } from "../../utils/single-completion-handler"
import { ProviderSettingsManager } from "../config/ProviderSettingsManager"

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

Labels

Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels.

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.