chore: replace franc-min with franc to support more languages #839
Conversation
π¦ Changeset detectedLatest commit: 5e7b3be The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dosubot
bot
added
the
size:S
|
Related Documentation Checked 14 published document(s) in 0 knowledge base(s). No updates required. How did I do? Any feedback?Β Β |
Code Review: Replace franc-min with francSummaryThis PR replaces γCore Judgmentγβ Worth doing: Addresses a real user need (Hebrew language support) with minimal code change. γKey Insightsγ
γTaste Scoreγπ’ Good taste - Simple, focused change that solves the problem directly Review Detailsβ Positives
|
PR Review: Replace franc-min with francγCore Judgmentγβ Worth doing - This is a valid fix for issue #571. The user requested Hebrew language support which is not included in franc-min (82 languages) but is included in franc (187 languages). γCode Qualityγπ’ Good taste - The change is minimal and focused. Strengths:
γKey Observationsγ1. Bundle Size Trade-off
This is an acceptable trade-off for a browser extension where:
2. No Breaking Changes
γPotential ConcernsγNone critical. However, minor notes:
γRecommendationsγ
SummaryThis is a well-executed, minimal change that directly addresses user feedback. The approach follows the project's design principles (KISS, YAGNI, no over-engineering). The bundle size increase is justified by the expanded language support. Verdict: Approve β |
β¦ and pnpm-lock.yaml
PR Review: Replace franc-min with francSummaryThis PR replaces Code Quality β
Potential Considerations1. Bundle Size Impact
Recommendation: Consider documenting the bundle size impact in the PR. For a browser extension, bundle size can affect:
You might want to run a build comparison: # Before (on main)
pnpm build && du -sh .output/
# After (on this branch)
pnpm build && du -sh .output/2. Test Coverage
Recommendation: Consider adding a simple test to verify Hebrew detection works: import { franc } from 'franc'
test('franc detects Hebrew', () => {
const hebrewText = 'Χ©ΧΧΧ Χ’ΧΧΧ, ΧΧ ΧΧ§Χ‘Χ ΧΧ’ΧΧ¨ΧΧͺ ΧΧΧΧΧ§Χ'
expect(franc(hebrewText)).toBe('heb')
})3. Related Definition Update β
Security βNo security concerns - this is a simple dependency swap with no new attack vectors. Performance β
Verdictπ’ Approve with minor suggestions The PR correctly solves the issue with minimal changes. The only concerns are:
These are minor and shouldn't block the merge, but would be good to address for completeness. |
dosubot
bot
added
size:M
PR Review: Replace franc-min with franc for language detectionSummaryThis PR replaces γTaste Scoreγπ’ Good taste - The changes are minimal, focused, and solve a real user problem. γCore Judgmentγβ Worth doing
Code Quality βThe source code changes are clean and minimal:
Both files use Potential Concerns1. Bundle Size Impact
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid β if so, understand the root cause of each and fix them.
<file name=".changeset/quick-ties-attack.md">
<violation number="1" location=".changeset/quick-ties-attack.md:5">
P2: Changeset description doesn't match the PR's actual changes. The PR replaces `franc-min` with `franc` to support more languages, but the changeset message describes upgrading zod and ai packages for memory issues. This will generate an incorrect changelog entry.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "@read-frog/extension": patch | ||
| --- | ||
|
|
||
| fix: upgrade zod and ai packages to solve memory issue of type check |
There was a problem hiding this comment.
P2: Changeset description doesn't match the PR's actual changes. The PR replaces franc-min with franc to support more languages, but the changeset message describes upgrading zod and ai packages for memory issues. This will generate an incorrect changelog entry.
Prompt for AI agents
Check if this issue is valid β if so, understand the root cause and fix it. At .changeset/quick-ties-attack.md, line 5:
<comment>Changeset description doesn't match the PR's actual changes. The PR replaces `franc-min` with `franc` to support more languages, but the changeset message describes upgrading zod and ai packages for memory issues. This will generate an incorrect changelog entry.</comment>
<file context>
@@ -0,0 +1,5 @@
+"@read-frog/extension": patch
+---
+
+fix: upgrade zod and ai packages to solve memory issue of type check
</file context>
Type of Changes
Description
Related Issue
Closes #571
How Has This Been Tested?
Screenshots
Checklist
Additional Information
Summary by cubic
Replaced franc-min with franc to expand language detection coverage in analysis and translation flows, and upgraded zod/ai to fix a type-check memory issue. Closes #571.
Written for commit 5e7b3be. Summary will update on new commits.