crop text in middle instead of decimate it

[wassname]

A few problems:

• for cropping we were discarding every N sentence, this creates garbage text
• we were calculating the max tokens wrong (or at least the logic didn't make sense to me)

So I've

• change the cropText for the whole extension to remove the excess text from the middle and replace it with a warning "this text has had X% removed"
• use \n instead of , for more natural text
• change the way we count the token limit to help us use all of it

#800

[PeterDaveHello]

So maybe the entire else block should be removed?

[Copilot]

Pull Request Overview

This PR updates the text cropping functionality to remove excess text from the middle rather than discarding every N sentence, while also adjusting token limit calculations and separator characters for a more natural output. Key changes include:

• Modifying cropText to split text by an expanded set of delimiters and crop from the middle with an appended disclaimer.
• Updating the token limit logic and default configurations for extended model contexts.
• Adjusting the YouTube site adapter to use newline as a separator and refining the summary prompt.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/utils/crop-text.mjs	Revised crop text function: new cropping algorithm, delimiter update, and token limit adjustments.
src/content-script/site-adapters/youtube/index.mjs	Updated subtitle extraction and summary prompt adjustments.
src/config/index.mjs	Increased default maxResponseTokenLength to accommodate longer texts.

[Copilot]

[nitpick] Consider explicitly converting the result of toFixed to a number (e.g., using Number(...)) to avoid any unexpected type coercion issues.

Suggested change

	(croppedTokens / totalTokens).toFixed(2) * 100
	Number((croppedTokens / totalTokens).toFixed(2)) * 100

[Copilot]

If the input text is short and the first pass never exceeds the halfTargetTokens, middleIndex will remain -1, which may lead to unexpected slicing. Consider adding a condition to bypass cropping when the text length is within limits.

[PeterDaveHello]

/review

[qodo-merge-pro]

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 800 - Partially compliant Compliant requirements: • Fix the "decimation" approach to text cropping which creates garbage text Non-compliant requirements: • Fix the issue where the extension uses subtitles instead of transcript on YouTube • Ensure the extension can handle uncommon words that are missed in subtitles Requires further human verification: • Improve the quality of text used for summarization - The PR changes the cropping method but still uses subtitles, so verification is needed to see if the summary quality has improved despite still using subtitles

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Percentage Calculation The percentage calculation for the removed tokens might be incorrect. The expression (croppedTokens / totalTokens).toFixed(2) * 100 first converts to a string with 2 decimal places, then multiplies by 100, which could lead to unexpected results. croppedText += `\n\n**Important disclaimer**, this text is incomplete! ${croppedTokens} or ${ (croppedTokens / totalTokens).toFixed(2) * 100 }% of tokens have been removed from this location in the text due to lack limited model context of ${maxLength}\n\n` Incomplete Implementation The PR addresses the cropping method but doesn't solve the core issue from the ticket - it still uses subtitles instead of transcript data. The PR improves how text is cropped but doesn't change the data source. const splits = text.split(/[,，。?？!！;；\n]/).map((s) => s.trim()) const splitsLength = splits.map((s) => (tiktoken ? encode(s).length : s.length)) const cropTargetLength = maxLength - startLength - endLength let firstHalfTokens = 0 let secondHalfTokens = 0 const halfTargetTokens = Math.floor(cropTargetLength / 2) let middleIndex = -1 let endStartIndex = splits.length let totalTokens = splitsLength.reduce((sum, length) => sum + length + 1, 0) let croppedTokens = 0 let croppedText = '' let currentLength = 0 // First pass: find the middle for (let i = 0; i < splits.length; i++) { if (firstHalfTokens < halfTargetTokens) { firstHalfTokens += splitsLength[i] + 1 } else { middleIndex = i break } } // Second pass: find the start of the end section for (let i = splits.length - 1; i >= middleIndex; i--) { secondHalfTokens += splitsLength[i] + 1 if (secondHalfTokens >= halfTargetTokens) { endStartIndex = i break } } // Calculate cropped tokens croppedTokens = totalTokens - firstHalfTokens - secondHalfTokens // Construct the cropped text croppedText = splits.slice(0, middleIndex).join('\n') if (croppedTokens > 0) { croppedText += `\n\n**Important disclaimer**, this text is incomplete! ${croppedTokens} or ${ (croppedTokens / totalTokens).toFixed(2) * 100 }% of tokens have been removed from this location in the text due to lack limited model context of ${maxLength}\n\n` } croppedText += splits.slice(endStartIndex).join('\n')

[PeterDaveHello]

/improve

[qodo-merge-pro]

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Code Suggestions ✨

Category	Suggestion	Impact
Possible issue	Fix percentage calculation and grammar The disclaimer message has a grammatical error with "due to lack limited model context". Also, the percentage calculation is incorrect as .toFixed(2) returns a string, so multiplying by 100 doesn't work as expected. src/utils/crop-text.mjs [101-103] croppedText += `\n\n**Important disclaimer**, this text is incomplete! ${croppedTokens} or ${ - (croppedTokens / totalTokens).toFixed(2) * 100 -}% of tokens have been removed from this location in the text due to lack limited model context of ${maxLength}\n\n` + ((croppedTokens / totalTokens) * 100).toFixed(2) +}% of tokens have been removed from this location in the text due to limited model context of ${maxLength}\n\n` Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies two real issues: a grammatical error ("lack limited" should be "limited") and a mathematical bug where .toFixed(2) returns a string that shouldn't be multiplied by 100. Both fixes improve code correctness and user experience.	Medium
More

[josStorer]

v2.5.9

• a new option has been added to the general settings to disable cropText, ensuring the full input tokens are always passed. This can improve summarization on sites like YouTube, but note that you should only disable cropText when using a model with a sufficiently long context.
•