-
Notifications
You must be signed in to change notification settings - Fork 176
fix: correct typos in SummarizerService #13
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
base: main
Are you sure you want to change the base?
Conversation
- Fix buildSystenPrompt → buildSystemPrompt (lines 47, 76) - Fix styleInstructructions → styleInstructions (line 81, 90) - Improves code readability and professionalism
WalkthroughThe pull request corrects two typos in the summarizer service: renaming Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this 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
🧹 Nitpick comments (2)
src/services/summarizer.service.ts (2)
16-17: Consider updating to Zod v4 unified error parameter.The current error message syntax uses string literals as the second parameter. Zod v4 introduces a unified
errorparameter for more flexible error customization.Apply this diff to align with Zod v4 best practices:
text: z .string() .trim() - .min(50, "Text too short to summarize (minimum 50 characters)") - .max(50000, "Text too long (maximum 50,000 characters)"), + .min(50, { + error: () => "Text too short to summarize (minimum 50 characters)" + }) + .max(50000, { + error: () => "Text too long (maximum 50,000 characters)" + }),Based on library documentation.
78-78: Consider usingSummaryStylestype for stronger type safety.The
styleparameter is currently typed asstring, requiring a type assertion at line 90. Using theSummaryStylestype would eliminate the need for the assertion and provide compile-time safety.Apply this diff to improve type safety:
private buildSystemPrompt( maxLength: number, - style: string, + style: SummaryStyles, language: string, ): string {Then simplify line 90:
- return `You are a professional text summarizer. ${styleInstructions[style as keyof typeof styleInstructions]} + return `You are a professional text summarizer. ${styleInstructions[style]}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/summarizer.service.ts(3 hunks)
🔇 Additional comments (1)
src/services/summarizer.service.ts (1)
47-47: LGTM! Typo corrections improve code quality.The renaming of
buildSystenPrompttobuildSystemPromptandstyleInstructructionstostyleInstructionsare accurate corrections that enhance code readability. All usages have been consistently updated.Also applies to: 76-76, 81-81, 90-90
Problem
Multiple typos in
src/services/summarizer.service.ts:buildSystenPromptshould bebuildSystemPromptstyleInstructructionsmisspelled asstyleInstructructionsSolution
Fix all typos to use correct spelling.
Changes
Impact
Summary by CodeRabbit