Feat/add clerk auth and premium ux #31
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughPull request introduces RTL (Right-to-Left) language support across the application by adding language detection utilities, propagating lang and dir metadata through API routes and components, updating stylesheets for RTL layout, and migrating the build system from Node/pnpm to Bun. Additionally, URL validation is enhanced to handle malformed protocols, and a marketing feature is removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Greptile OverviewGreptile SummaryThis PR adds comprehensive RTL (Right-to-Left) language support and migrates the project from Node.js/pnpm to Bun runtime. Key Changes
Implementation QualityThe RTL implementation is well-architected with proper separation of concerns. The The URL normalization fix addresses a production issue with iterative protocol cleaning logic that handles edge cases like multiple duplicate protocols. Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant ArticleAPI as /api/article
participant JinaAPI as /api/jina
participant RTLLib as lib/rtl
participant URLValidation as lib/validation/url
participant Redis
participant Diffbot
participant JinaReader as Jina Reader
User->>Frontend: Enter URL
Frontend->>URLValidation: normalizeUrl(url)
URLValidation->>URLValidation: cleanProtocol()
URLValidation-->>Frontend: Normalized URL
alt Article Fetch (smry-fast/slow/wayback)
Frontend->>ArticleAPI: GET /api/article?url=...&source=...
ArticleAPI->>Redis: Check cache
alt Cache Hit
Redis-->>ArticleAPI: Cached article
else Cache Miss
ArticleAPI->>Diffbot: Fetch article
Diffbot-->>ArticleAPI: HTML content
ArticleAPI->>ArticleAPI: Parse with Readability
ArticleAPI->>ArticleAPI: Extract lang attribute
end
ArticleAPI->>RTLLib: getTextDirection(lang, textContent)
RTLLib->>RTLLib: isRTLLanguage(lang)
alt Language is RTL
RTLLib-->>ArticleAPI: 'rtl'
else Analyze content
RTLLib->>RTLLib: detectTextDirection(textContent)
RTLLib-->>ArticleAPI: 'rtl' or 'ltr'
end
ArticleAPI->>Redis: Cache article with dir/lang
ArticleAPI-->>Frontend: Article with dir/lang
else Jina Fetch
Frontend->>JinaAPI: GET /api/jina?url=...
JinaAPI->>Redis: Check cache
alt Cache Hit
Redis-->>JinaAPI: Cached article
else Cache Miss
JinaAPI->>JinaReader: Fetch from Jina
JinaReader-->>JinaAPI: Article content
end
JinaAPI->>RTLLib: getTextDirection(null, textContent)
RTLLib->>RTLLib: detectTextDirection(textContent)
RTLLib-->>JinaAPI: 'rtl' or 'ltr'
JinaAPI->>Redis: Cache article with dir
JinaAPI-->>Frontend: Article with dir
end
Frontend->>Frontend: Render with dir/lang attributes
Frontend->>User: Display article (RTL or LTR)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
tsconfig.json (1)
2-4: Global Bun typings are fine, but consider scoping to tests if neededAdding
"types": ["bun-types"]ensures Bun globals andbun:testare typed across the project. If you ever run into conflicts with Node/Next types, consider moving Bun typings into a dedicated test/buildtsconfiginstead of the main one.Dockerfile (1)
1-11: Bun-based build looks good; consider pinning image version and/or multi-stage buildThe Bun migration (cached
bun install+bun run build+bun run start) is clear and should work fine. For better reproducibility and smaller images, consider:
- Pinning
oven/bunto a specific version instead oflatest.- Optionally using a multi-stage build (build in one stage, run in a slimmer runtime stage) if image size becomes a concern.
app/globals.css (1)
179-221: RTL base styles are well-scoped and align with expected behaviorThe new
[dir="rtl"]rules (text alignment, blockquotes, lists, and forcing code/pre back to LTR) are appropriately scoped in the base layer and should play nicely with Tailwind utilities. This is a solid foundation for RTL rendering.lib/rtl.test.ts (1)
1-108: Strong RTL test coverage; minor describe/test mismatch for Persian caseThe test suite thoroughly exercises
isRTLLanguage,detectTextDirection, andgetTextDirectionfor RTL/LTR, mixed content, and null/empty inputs—nice coverage.One small nit: inside
describe('getTextDirection'), the"handles Persian text"test callsdetectTextDirectiondirectly. For clarity, consider either:
- Moving that test into the
detectTextDirectiondescribe block, or- Changing it to assert
getTextDirection('fa', persianText)so it matches the surrounding describe name.Functionally everything is correct; this is just a structure/readability tweak.
lib/validation/url.test.ts (1)
4-195: URL tests are thorough; consider adding coverage for uppercase HTTP(S) schemesThis suite does a great job exercising normalization (including malformed and duplicate protocols), validation, and the Zod schema across many realistic cases.
To backstop the case-insensitivity fix suggested in
lib/validation/url.ts, it would be useful to add a couple of expectations like:it("treats uppercase protocols as valid", () => { expect(normalizeUrl("HTTPS://example.com")).toBe("https://example.com"); expect(normalizeUrl("HTTP://example.com")).toBe("http://example.com"); expect(isValidUrl("HTTPS://example.com")).toBe(true); });That will ensure regressions around scheme casing are caught by tests.
Also applies to: 197-246
lib/rtl.ts (1)
72-74: Sampling strategy note.Sampling only the first 10,000 characters may not be representative for articles where RTL content appears predominantly in the body rather than the lead. Consider whether this is acceptable for your use case, or if random/distributed sampling would be more robust.
app/api/jina/route.ts (1)
173-183: Redundantdirassignment and inconsistentlanghandling.On lines 177-180:
...articleWithDiralready includesdir: articleDir, then line 179 setsdir: articleDiragain (redundant).- Line 180 hardcodes
lang: ""which overwrites thelangfromarticleWithDir.Consider cleaning up:
article: { ...articleWithDir, byline: article.byline || "", - dir: articleDir, - lang: "", + lang: articleWithDir.lang || "", publishedTime: article.publishedTime || null, htmlContent: article.content, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
Dockerfile(1 hunks)app/api/article/route.ts(11 hunks)app/api/jina/route.ts(6 hunks)app/globals.css(1 hunks)components/ai/response.tsx(4 hunks)components/article/content.tsx(2 hunks)components/features/summary-form.tsx(1 hunks)components/marketing/ad-spot.tsx(1 hunks)lib/rtl.test.ts(1 hunks)lib/rtl.ts(1 hunks)lib/validation/url.test.ts(1 hunks)lib/validation/url.ts(3 hunks)package.json(1 hunks)tsconfig.json(1 hunks)types/api.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
lib/rtl.test.ts (1)
lib/rtl.ts (3)
isRTLLanguage(45-51)detectTextDirection(66-104)getTextDirection(110-121)
components/features/summary-form.tsx (1)
components/ai/response.tsx (1)
Response(377-420)
lib/validation/url.test.ts (1)
lib/validation/url.ts (3)
normalizeUrl(49-69)isValidUrl(74-81)NormalizedUrlSchema(87-104)
app/api/jina/route.ts (2)
lib/rtl.ts (1)
getTextDirection(110-121)types/api.ts (1)
ArticleResponseSchema(49-56)
app/api/article/route.ts (1)
lib/rtl.ts (1)
getTextDirection(110-121)
🪛 ast-grep (0.40.0)
components/article/content.tsx
[warning] 300-300: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🔇 Additional comments (24)
package.json (1)
88-107: bun-types devDependency wiring looks appropriateAdding
bun-typesas a devDependency aligns with using Bun tooling andbun:testwhile keeping runtime dependencies clean. No issues from a build/tooling perspective.components/marketing/ad-spot.tsx (1)
3-6: Lucide icon imports are now minimal and consistent with usageThe
lucide-reactimport list matches actual icon usage (no unused icons likeZap), keeping the bundle clean.lib/rtl.ts (5)
6-22: RTL languages set looks comprehensive.The set covers major RTL languages. Note that Kurdish (
ku) and Hausa (ha) are bidirectional scripts - they can be written in both Arabic (RTL) and Latin (LTR) scripts. The current approach of treating them as RTL by default is reasonable for content analysis but may produce false positives for Latin-script Kurdish/Hausa content.
45-51: LGTM!The locale normalization correctly handles both hyphen and underscore separators (e.g.,
ar-SA,ar_SA), and the null-check is appropriate.
56-60: LGTM!The implementation is correct and efficient for the small number of RTL ranges.
110-121: LGTM!The function correctly prioritizes explicit RTL language codes while falling back to content analysis. The behavior of still analyzing content when a non-RTL language code is provided (e.g.,
en) is reasonable as it allows detection of mixed-content scenarios.
126-136: LGTM!Clean implementation with proper conditional property inclusion.
components/features/summary-form.tsx (1)
208-211: LGTM!The RTL props are correctly propagated to the Response component, with appropriate fallback values that align with the article schema defaults.
types/api.ts (1)
29-29: Verify intended behavior with.nullable().optional().default('ltr').With this chain,
.default('ltr')only applies when the field isundefinedor missing. Ifdir: nullis explicitly passed, it will remainnull(not default to'ltr').If null should also resolve to
'ltr', consider using.nullish().default('ltr')or applying the default after parsing. Otherwise, components consuming this schema should handle null values explicitly (as they currently do withdir || 'ltr'fallbacks).components/article/content.tsx (1)
70-74: LGTM!The header container correctly propagates article direction and language attributes for proper RTL rendering. The fallbacks align with the schema defaults.
app/api/jina/route.ts (4)
22-24: LGTM!The CachedArticleSchema correctly adds optional lang and dir fields with proper types matching the ArticleSchema constraints.
71-72: LGTM!The GET path correctly falls back to computing direction when not cached, using both the language code and text content for accurate detection.
197-198: LGTM!The existing-cache path correctly computes direction as a fallback, consistent with the GET handler.
210-220: LGTM!The error fallback path correctly computes and includes direction, ensuring RTL support even when caching fails.
components/ai/response.tsx (2)
203-206: LGTM!The type definitions for RTL support are well-documented and correctly typed. The optional nature is appropriate since language metadata may not always be available.
386-402: LGTM!The
dirandlangprops are correctly propagated to the root container div, enabling proper RTL rendering and language accessibility for the markdown content.app/api/article/route.ts (8)
12-12: LGTM!Import of
getTextDirectionaligns with the RTL support pattern used across the codebase.
26-27: LGTM!Schema addition for
langis appropriately optional and nullable for backward compatibility with existing cached data.
40-41: LGTM!The
dirfield is correctly constrained to the enum['rtl', 'ltr']and both fields are optional/nullable, ensuring backward compatibility with existing cache entries.
223-250: LGTM!The language extraction logic properly checks multiple sources (HTML
lang,xml:lang, Readability's extracted lang) with appropriate fallbacks. The text direction detection correctly uses both the language code and content analysis viagetTextDirection.
332-349: LGTM!Direction detection and field population follow the same consistent pattern as the smry-fast path. The enhanced logging with
langanddirimproves observability.
455-456: LGTM!The fallback to
getTextDirectionfor missingdirvalues ensures backward compatibility with cached articles that predate this change.
539-540: LGTM!Consistent dir/lang population pattern maintained across the fallback response path.
562-563: LGTM!Consistent dir/lang population in the main success response path.
| dir: validatedArticle.dir || getTextDirection(validatedArticle.lang, validatedArticle.textContent), | ||
| lang: validatedArticle.lang || "", | ||
| content: validatedArticle.content, | ||
| textContent: validatedArticle.textContent, | ||
| length: validatedArticle.length, | ||
| siteName: validatedArticle.siteName, | ||
| publishedTime: validatedArticle.publishedTime || null, | ||
| image: validatedArticle.image || null, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ArticleResponseSchema definition
rg -n "ArticleResponseSchema" --type=ts -A 40Repository: mrmps/SMRY
Length of output: 29677
🏁 Script executed:
#!/bin/bash
# Find ArticleSchema definition
rg -n "ArticleSchema\s*=" --type=ts -B 2 -A 20 | head -100Repository: mrmps/SMRY
Length of output: 5544
Response object inconsistency: image field presence varies across paths.
The response at line 617 includes image: validatedArticle.image || null, but similar response constructions at lines 533-549 and 556-572 omit this field. While ArticleSchema defines image as optional, this inconsistency means clients will receive different response structures depending on which code path executes (cache hit, post-fetch validation, or error handler).
Add the image field to the response blocks at lines 533-549 and 556-572 for consistency.
🤖 Prompt for AI Agents
In app/api/article/route.ts around lines 533-549, 556-572 and 610-617, the
responses are inconsistent: the block at 610-617 includes image:
validatedArticle.image || null but the earlier response blocks (533-549 and
556-572) omit image; add image: validatedArticle.image || null to the response
objects in the 533-549 and 556-572 blocks so all code paths return the same
shape (use the same null fallback as the existing block) and keep field ordering
consistent with the other response properties.
| // Detect text direction for the incoming article | ||
| const articleDir = getTextDirection(null, article.textContent); | ||
| const articleWithDir = { ...article, dir: articleDir, lang: article.lang || null }; |
There was a problem hiding this comment.
article.lang doesn't exist on the input schema.
The JinaCacheUpdateSchema (defined in types/api.ts, lines 92-104) doesn't include a lang field. The expression article.lang || null on line 166 will always evaluate to null because article.lang is undefined.
Either add lang to JinaCacheUpdateSchema if callers can provide it, or simplify:
- const articleWithDir = { ...article, dir: articleDir, lang: article.lang || null };
+ const articleWithDir = { ...article, dir: articleDir, lang: null };#!/bin/bash
# Verify JinaCacheUpdateSchema definition
echo "=== JinaCacheUpdateSchema definition ==="
rg -n -A15 'JinaCacheUpdateSchema' types/api.ts🤖 Prompt for AI Agents
In app/api/jina/route.ts around lines 164 to 166, the code reads article.lang ||
null but JinaCacheUpdateSchema does not define a lang field so article.lang will
always be undefined; either (A) add an optional lang?: string | null to
JinaCacheUpdateSchema in types/api.ts (update the type at lines ~92-104 and
ensure any callers populate it where appropriate), or (B) simplify the route
code to stop referencing article.lang and set lang: null (or omit lang entirely)
when constructing articleWithDir; pick A if callers can/should supply language,
otherwise pick B and remove the redundant check.
| ); | ||
| }, | ||
| (prevProps, nextProps) => prevProps.children === nextProps.children | ||
| (prevProps, nextProps) => prevProps.children === nextProps.children && prevProps.dir === nextProps.dir |
There was a problem hiding this comment.
Memoization predicate missing lang comparison.
The lang prop is passed to the div but not included in the memoization check. If lang changes while children and dir remain the same, the component won't re-render, leaving a stale lang attribute on the DOM element.
- (prevProps, nextProps) => prevProps.children === nextProps.children && prevProps.dir === nextProps.dir
+ (prevProps, nextProps) => prevProps.children === nextProps.children && prevProps.dir === nextProps.dir && prevProps.lang === nextProps.lang📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (prevProps, nextProps) => prevProps.children === nextProps.children && prevProps.dir === nextProps.dir | |
| (prevProps, nextProps) => prevProps.children === nextProps.children && prevProps.dir === nextProps.dir && prevProps.lang === nextProps.lang |
🤖 Prompt for AI Agents
In components/ai/response.tsx around line 419, the memoization predicate only
compares children and dir but omits lang, so when lang changes the component can
stay memoized and the DOM's lang attribute becomes stale; update the predicate
to also compare prevProps.lang === nextProps.lang (i.e., return
prevProps.children === nextProps.children && prevProps.dir === nextProps.dir &&
prevProps.lang === nextProps.lang) so the component re-renders when lang
changes.
| <div | ||
| className="mt-6 wrap-break-word prose dark:prose-invert max-w-none" | ||
| dir={data.article.dir || 'ltr'} | ||
| lang={data.article.lang || undefined} | ||
| dangerouslySetInnerHTML={{ __html: data.article.content }} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for sanitization utilities and DOMPurify usage
echo "=== Checking for DOMPurify and sanitization imports ==="
rg -n 'DOMPurify|sanitize|purify' --type=ts --type=tsx
echo ""
echo "=== Checking article-related files ==="
fd -e ts -e tsx 'article' | head -20
echo ""
echo "=== Checking for readability processing ==="
rg -n 'readability|Readability' --type=ts --type=tsxRepository: mrmps/SMRY
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Search without file type filter
echo "=== Checking for sanitization ==="
rg -n 'DOMPurify|sanitize|purify|xss|escapeHtml|strip' --glob='*.ts' --glob='*.tsx'
echo ""
echo "=== Reading use-articles.ts ==="
cat -n lib/hooks/use-articles.ts
echo ""
echo "=== Finding article API routes ==="
find . -type f \( -name '*.ts' -o -name '*.tsx' \) -path '*/routes/*' -o -path '*/api/*' | grep -i articleRepository: mrmps/SMRY
Length of output: 5161
🏁 Script executed:
#!/bin/bash
# Read jina.ts to see escapeHtml usage
echo "=== Reading lib/api/jina.ts ==="
cat -n lib/api/jina.ts
echo ""
echo "=== Reading app/api/article/route.ts ==="
cat -n app/api/article/route.tsRepository: mrmps/SMRY
Length of output: 32659
🏁 Script executed:
#!/bin/bash
# Check for Content Security Policy or other security headers
echo "=== Checking for CSP or security headers in Next.js config ==="
find . -type f \( -name 'next.config.*' -o -name 'middleware.ts' \) | head -10 | xargs cat 2>/dev/null
echo ""
echo "=== Checking package.json for marked and readability versions ==="
grep -A 2 '"marked"\|"readability"\|"dompurify"' package.jsonRepository: mrmps/SMRY
Length of output: 699
Add HTML sanitization before rendering data.article.content with dangerouslySetInnerHTML.
Article content from all sources (Jina.ai, Readability, Diffbot) is currently rendered without sanitization. The marked markdown parser, Mozilla Readability, and Diffbot all output unsanitized HTML, and the Jina fallback's escapeHtml() only encodes entities without removing dangerous tags/attributes. Without DOMPurify or similar, the application is vulnerable to XSS attacks. Integrate a sanitization library (e.g., DOMPurify) in the article processing pipeline before the content reaches this component.
The dir/lang additions are correct and improve accessibility.
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 300-300: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
[error] 301-301: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🤖 Prompt for AI Agents
In components/article/content.tsx around lines 297–302, the component currently
injects data.article.content directly via dangerouslySetInnerHTML, which is
vulnerable to XSS because content from Jina/Readability/Diffbot is unsanitized;
integrate HTML sanitization (e.g., DOMPurify) in the article processing pipeline
or just before rendering: install/import a sanitizer, run data.article.content
through the sanitizer to remove dangerous tags/attributes and then pass the
sanitized HTML to dangerouslySetInnerHTML (or store sanitized content on the
article object earlier), ensuring the sanitized string is what the component
renders while leaving dir/lang handling unchanged.
| // Valid protocol: http:// or https:// | ||
| const VALID_PROTOCOL_REGEX = /^https?:\/\//; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "url.ts" -o -name "url.test.ts" | head -20Repository: mrmps/SMRY
Length of output: 106
🏁 Script executed:
cat -n lib/validation/url.tsRepository: mrmps/SMRY
Length of output: 3593
🏁 Script executed:
cat -n lib/validation/url.test.tsRepository: mrmps/SMRY
Length of output: 10400
🏁 Script executed:
# Check if validator.js documentation or source is available in node_modules
find . -type d -name "validator" 2>/dev/null | head -5Repository: mrmps/SMRY
Length of output: 36
🏁 Script executed:
# Look for validator package info
fd -t f "package.json" | xargs grep -l "validator" | head -3Repository: mrmps/SMRY
Length of output: 66
🏁 Script executed:
cat package.json | grep -A 5 "validator"Repository: mrmps/SMRY
Length of output: 410
Add case-insensitive flag to protocol regex to handle uppercase schemes
The VALID_PROTOCOL_REGEX is case-sensitive while the cleanProtocol function uses case-insensitive regexes, creating an inconsistency. Uppercase schemes like "HTTPS://example.com" will fail the check at line 60, causing normalizeUrl to incorrectly prepend https://, resulting in "https://HTTPS://example.com" that fails validation.
Add the /i flag to match the case-insensitive handling in cleanProtocol:
-const VALID_PROTOCOL_REGEX = /^https?:\/\//;
+const VALID_PROTOCOL_REGEX = /^https?:\/\//i;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Valid protocol: http:// or https:// | |
| const VALID_PROTOCOL_REGEX = /^https?:\/\//; | |
| // Valid protocol: http:// or https:// | |
| const VALID_PROTOCOL_REGEX = /^https?:\/\//i; | |
🤖 Prompt for AI Agents
In lib/validation/url.ts around lines 4 to 6, the VALID_PROTOCOL_REGEX is
currently case-sensitive and will not match uppercase schemes (e.g.,
"HTTPS://"), causing normalizeUrl to prepend another protocol; update the regex
to be case-insensitive by adding the /i flag (e.g., /^https?:\/\//i) so it
matches "http"/"https" in any case and keep the existing comment or adjust it if
needed.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.