-
-
Notifications
You must be signed in to change notification settings - Fork 212
feat(dom-rules): Add 300+ dom rules #931
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
π¦ Changeset detectedLatest commit: 01b23f9 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 |
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.
1 issue found across 5 files
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="src/utils/constants/dom-rules.ts">
<violation number="1" location="src/utils/constants/dom-rules.ts:152">
P2: findMatchingSelectors returns empty results while domRules is still loading asynchronously, and call sites use it synchronously. Early DOM scans can skip domain-specific blocking rules until the JSON import finishes, changing behavior from the previous synchronous constants.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
PR Review: feat(dom-rules): Add 300+ dom rulesOverall Assessmentπ’ Good addition - Bringing in 360+ website-specific DOM rules from a fork is valuable for improving translation accuracy across popular sites. The refactor to JSON-based configuration makes rules easier to maintain. Key InsightsData structure: JSON config with two rule types ( Complexity: The glob-to-regex pattern matching is the main logic addition. Implementation is straightforward but has edge cases. Risk points:
Issues to Address1. Potential Breaking Change: YouTube rules removedThe original `.${SUBTITLES_VIEW_CLASS}`,
`.${STATE_MESSAGE_CLASS}`,
`.${TRANSLATE_BUTTON_CLASS}`,These dynamic selectors based on JS constants are NOT present in the JSON file. The YouTube entry in "www.youtube.com": [".ytp-caption-segment"]This is a regression - the extension's own subtitle-related elements won't be excluded from translation. Suggestion: Either add the actual class values to the JSON, or create a mechanism to merge runtime constants with JSON rules for YouTube. 2. Performance: Pattern matching on every element
Suggestion: Cache the matched selectors per URL at the start of page translation rather than re-computing per element. 3. Massive duplication in JSONTwitter/X rules are duplicated 10+ times for different subdomains. Same for Stack Exchange sites. Suggestion: Consider using wildcard patterns more aggressively: "*.twitter.com": [...],
"*.x.com": [...],Or introduce rule inheritance/aliases to reduce the 2200-line JSON file. 4. Missing tests for new pattern matching logic
The existing tests in 5. Edge case bug in matchUrlPatternif (!pattern.startsWith('http://') && !pattern.startsWith('https://')) {
return regex.test(url.replace(/^https?:\/\//, ''))
}If pattern doesn't start with protocol but URL does, it strips protocol from URL. But if pattern contains path like Minor Issues
if (!domRules)
// Not initialized yet
return []Comment on wrong line - should be above or inline.
"https://www.reddit.com/r/*/comments/*/*": [...]While others use just hostnames. This inconsistency works due to the pattern matching but reduces clarity. What's Good
RecommendationWorth merging with fixes:
The core value (300+ rules) is solid. The implementation needs minor hardening before merge. |
|
can I ask where are these rules originally from? |
Type of Changes
Description
dom-rules.tsfor loading rules from json (I have merged the exist rules into the json file)Related Issue
None
How Has This Been Tested?
Verified via a manually smoke test
Screenshots
None
Checklist
I think I cannot access the source code of page https://www.readfrog.app/zh/tutorial/code-contribution/custom-dom-rules
So I cannot modify the docs.
Additional Information
Summary by cubic
Added 360+ website-specific DOM exclusion and block-translation rules, loaded from a JSON config with wildcard URL matching. This improves translation accuracy across popular sites and makes rules easier to maintain.
New Features
Refactors
Written for commit 01b23f9. Summary will update on new commits.