-
Notifications
You must be signed in to change notification settings - Fork 3
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
chore: terms page upt #429
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request updates the terms and privacy policy page, adds a global error page, and modifies the navigation structure to reflect the new legal page. Class diagram for content types and structureclassDiagram
class ContentItem {
<<interface>>
}
class TextContent {
type: ContentTextType & ContentType
text: string
}
class ListContent {
type: 'ul'
items: string[]
}
class ImageContent {
type: 'Image'
src: string
alt: string
}
ContentItem <|-- TextContent
ContentItem <|-- ListContent
ContentItem <|-- ImageContent
note for ContentItem "Updated to combine ContentTextType & ContentType"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis pull request introduces several enhancements across the web application, focusing on error handling, content management, and development workflow improvements. The changes span multiple files, including updates to the pre-commit script, Next.js configuration, error boundaries, content rendering, and dictionary management. The modifications aim to improve code quality, security, and user experience by refining routing, content policies, and error presentation. Changes
Sequence DiagramsequenceDiagram
participant User
participant App
participant ErrorHandler
participant Middleware
User->>App: Trigger Action
App->>Middleware: Process Request
Middleware-->>App: Route/Locale Resolved
App->>App: Render Content
alt Error Occurs
App->>ErrorHandler: Capture Error
ErrorHandler->>User: Display Error Message
User->>ErrorHandler: Click Reset
ErrorHandler->>App: Attempt Recovery
end
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Hey @AndlerRL - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Avoid exposing raw error details in the UI (link)
Overall Comments:
- The Content Security Policy in next.config.js uses a loose regex pattern (^https://twitter\.com/.*) which could be more strictly defined to improve security. Consider specifying exact Twitter domains and paths needed.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
{error.message} | ||
</p> | ||
<code> | ||
{JSON.stringify(error, null, 2)} |
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.
🚨 issue (security): Avoid exposing raw error details in the UI
For security reasons, only show user-friendly error messages in production. Raw error details could expose sensitive information about your application.
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: 7
🔭 Outside diff range comments (1)
apps/web/middleware.ts (1)
Update matcher configuration to include the auth callback path
The current matcher configuration excludes the '/auth/callback' path, but the middleware code explicitly checks for this path. This creates a conflict where the middleware's auth callback handling code will never execute.
- Update the matcher in
apps/web/middleware.ts
to ensure it doesn't exclude the auth callback path🔗 Analysis chain
Line range hint
41-45
: Verify matcher configuration covers all necessary paths.The matcher configuration should be reviewed to ensure it properly handles all routes, including the auth callback path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all route definitions to ensure matcher covers them rg -g '*.tsx' -g '*.ts' "export const runtime|export async function|export default function" ./apps/web/appLength of output: 2405
Script:
#!/bin/bash # Search for auth-related routes and configurations rg -g '*.ts' -g '*.tsx' -i "auth.*callback|callback.*url|auth.*config" ./apps/webLength of output: 707
🧹 Nitpick comments (6)
.husky/pre-commit (1)
2-3
: Consider adding error handling for the formatting commands.The Prettier and format-and-lint commands should include error handling to prevent committing if formatting fails.
-prettier $(git diff --cached --name-only --diff-filter=ACMR | sed 's| |\\ |g') --write --ignore-unknown -bun format-and-lint:fix +if ! prettier $(git diff --cached --name-only --diff-filter=ACMR | sed 's| |\\ |g') --write --ignore-unknown; then + echo "❌ Prettier formatting failed" + exit 1 +fi +if ! bun format-and-lint:fix; then + echo "❌ Format and lint failed" + exit 1 +fiapps/web/app/(routes)/[lang]/global-error.tsx (2)
12-15
: Enhance error logging implementation.Consider using a proper error logging service instead of just console.error.
useEffect(() => { - // Log the error to an error reporting service - console.error(error) + // TODO: Integrate with error reporting service (e.g., Sentry) + console.error('Global error boundary caught an error:', { + message: error.message, + stack: error.stack, + digest: error.digest + }) }, [error])
17-35
: Improve error UI presentation and accessibility.The error UI lacks styling and accessibility features.
return ( - <div> - <h2>Something went wrong!</h2> + <div role="alert" className="p-4 rounded-lg bg-red-50 text-red-900"> + <h2 className="text-lg font-semibold mb-2">Something went wrong!</h2> <p> {error.message} </p> - <code> + <pre className="mt-2 p-2 bg-red-100 rounded overflow-auto"> + <code> {JSON.stringify(error, null, 2)} - </code> + </code> + </pre> <button + className="mt-4 px-4 py-2 bg-red-600 text-white rounded hover:bg-red-700" + aria-label="Attempt to recover from error" onClick={ // Attempt to recover by trying to re-render the segment () => reset() } > Try again </button> </div> )apps/web/middleware.ts (1)
16-16
: Consider adding type assertion for response initialization.The response initialization could benefit from explicit typing, and the auth callback condition could be more robust.
-let response: NextResponse = NextResponse.next() +let response: NextResponse = NextResponse.next() as NextResponse -} else if (!pathname.startsWith('/auth/callback')) { +} else if (!/^\/(?:auth\/callback|_next|api)/.test(pathname)) { const lang = getLocale(request) request.nextUrl.pathname = `/${lang}${pathname}` response = NextResponse.redirect(request.nextUrl)Also applies to: 20-23
apps/web/dictionaries/en/terms.ts (1)
65-78
: Improve content structure with TypeScript enumsThe content structure could be improved using TypeScript enums for content types.
Consider this refactor:
enum ContentType { Heading1 = 'h1', Heading2 = 'h2', Paragraph = 'p', } const content: ContentItem[] = [ { type: ContentType.Heading1, text: "Privacy Policy and Terms of Service for Bitlauncher Participants", }, // ... rest of the content ];package.json (1)
13-13
: Consider pinning the husky versionUsing
^
allows minor version updates which could introduce unexpected changes.Consider pinning the exact version:
- "husky": "^9.1.7", + "husky": "9.1.7",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockb
is excluded by!**/bun.lockb
📒 Files selected for processing (10)
.husky/pre-commit
(1 hunks)apps/apphub/my-app/next.config.js
(1 hunks)apps/web/app/(routes)/[lang]/global-error.tsx
(1 hunks)apps/web/app/(routes)/[lang]/legal/privacy/page.tsx
(1 hunks)apps/web/app/globals.css
(1 hunks)apps/web/components/layout/nav-struct.ts
(1 hunks)apps/web/components/shared/content.tsx
(2 hunks)apps/web/dictionaries/en/terms.ts
(1 hunks)apps/web/middleware.ts
(1 hunks)package.json
(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/app/(routes)/[lang]/global-error.tsx
[error] 5-5: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (4)
apps/web/app/(routes)/[lang]/legal/privacy/page.tsx (1)
10-10
: Verify the privacy content structure matches PageContent expectations.The change from
terms.content
toprivacy.content
looks correct, but let's verify the content structure.package.json (1)
26-26
: LGTM: Husky prepare scriptThe prepare script is correctly configured for husky installation.
apps/web/components/shared/content.tsx (1)
18-18
: LGTM! Consistent whitespace handling.The addition of
whitespace-pre-line
aligns with the paragraph styling in globals.css, ensuring consistent text formatting across components.apps/web/app/globals.css (1)
327-327
: LGTM! Enhanced text formatting.The addition of
whitespace-pre-line
to the paragraph class improves content formatting consistency and provides better control over text presentation.
cd ./apps/web | ||
bun scripts/generate-cms-dictionaries.ts No newline at end of file | ||
bun scripts/generate-cms-dictionaries.ts |
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.
🛠️ Refactor suggestion
Verify the working directory change is properly handled.
The script changes directory but doesn't ensure it returns to the original directory, which could affect subsequent commands.
-cd ./apps/web
-bun scripts/generate-cms-dictionaries.ts
+(cd ./apps/web && bun scripts/generate-cms-dictionaries.ts)
📝 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.
cd ./apps/web | |
bun scripts/generate-cms-dictionaries.ts | |
\ No newline at end of file | |
bun scripts/generate-cms-dictionaries.ts | |
(cd ./apps/web && bun scripts/generate-cms-dictionaries.ts) |
export default function Error({ | ||
error, | ||
reset, | ||
}: { | ||
error: Error & { digest?: string } | ||
reset: () => void | ||
}) { |
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.
🛠️ Refactor suggestion
Rename component to avoid shadowing global Error.
The component name shadows the global Error constructor. Consider a more specific name.
-export default function Error({
+export default function GlobalErrorBoundary({
📝 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.
export default function Error({ | |
error, | |
reset, | |
}: { | |
error: Error & { digest?: string } | |
reset: () => void | |
}) { | |
export default function GlobalErrorBoundary({ | |
error, | |
reset, | |
}: { | |
error: Error & { digest?: string } | |
reset: () => void | |
}) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
@@ -15,7 +15,7 @@ const nextConfig = { | |||
}, | |||
{ | |||
key: 'Content-Security-Policy', | |||
value: `object-src 'none';base-uri 'self';script-src 'self' 'report-sample' 'unsafe-inline' 'unsafe-eval' https: http:;`, | |||
value: `object-src 'self' ^https://twitter\.com/.*;base-uri 'self';script-src 'self' 'report-sample' 'unsafe-inline' 'unsafe-eval' https: http:;`, |
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.
Security: Strengthen Content Security Policy
The current CSP configuration has several security concerns:
- The Twitter URL pattern is too permissive and incorrectly formatted
- Script sources allow both HTTP and HTTPS, which is unsafe
- Usage of unsafe-inline/eval should be avoided if possible
Consider applying this more secure configuration:
- value: `object-src 'self' ^https://twitter\.com/.*;base-uri 'self';script-src 'self' 'report-sample' 'unsafe-inline' 'unsafe-eval' https: http:;`,
+ value: `object-src 'self' https://twitter.com https://*.twitter.com; base-uri 'self'; script-src 'self' 'report-sample' https:; require-trusted-types-for 'script';`,
📝 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.
value: `object-src 'self' ^https://twitter\.com/.*;base-uri 'self';script-src 'self' 'report-sample' 'unsafe-inline' 'unsafe-eval' https: http:;`, | |
value: `object-src 'self' https://twitter.com https://*.twitter.com; base-uri 'self'; script-src 'self' 'report-sample' https:; require-trusted-types-for 'script';`, |
title: "Privacy Policy", | ||
content: "Last Updated: January 17, 2025.", |
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.
Update the last modified date
The last updated date is set to January 17, 2025, which is in the future.
Update to the current date or remove the hard-coded date in favor of a dynamic date that updates when the content changes.
title: "Contact Us", | ||
content: "If you have questions about this Privacy Policy, please contact us at [email protected].", | ||
}, |
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.
🛠️ Refactor suggestion
Protect email address from scraping
Exposing email addresses directly in the code makes them vulnerable to scraping by bots.
Consider using a contact form or an encoded email address:
- content: "If you have questions about this Privacy Policy, please contact us at [email protected].",
+ content: "If you have questions about this Privacy Policy, please contact us through our secure contact form.",
📝 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.
title: "Contact Us", | |
content: "If you have questions about this Privacy Policy, please contact us at [email protected].", | |
}, | |
title: "Contact Us", | |
content: "If you have questions about this Privacy Policy, please contact us through our secure contact form.", | |
}, |
label: 'Privacy Policy', | ||
href: '/terms', | ||
href: '/legal/terms', | ||
}, |
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.
Fix incorrect Privacy Policy URL
The Privacy Policy link incorrectly points to /legal/terms
instead of /legal/privacy
.
Apply this fix:
label: 'Privacy Policy',
- href: '/legal/terms',
+ href: '/legal/privacy',
📝 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.
label: 'Privacy Policy', | |
href: '/terms', | |
href: '/legal/terms', | |
}, | |
label: 'Privacy Policy', | |
href: '/legal/privacy', | |
}, |
@@ -64,7 +64,7 @@ export type ContentType = 'h1' | 'h2' | 'p' | 'ul' | 'Image' | 'hr' | |||
export type ContentTextType = 'h1' | 'h2' | 'p' | |||
// Update the original ContentItem definition to use ContentType | |||
export type ContentItem = | |||
| { type: ContentTextType; text: string } | |||
| { type: ContentTextType & ContentType; text: string } |
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.
Fix potential type conflict in ContentItem definition.
The intersection type ContentTextType & ContentType
could lead to runtime errors since 'Image' and 'hr' exist in ContentType but not in ContentTextType. This means these types would be excluded from the union, despite being used in the switch statement.
Consider this alternative implementation:
- | { type: ContentTextType & ContentType; text: string }
+ | { type: ContentTextType; text: string }
+ | { type: 'hr'; text: string }
Or update ContentTextType to include all text-based types:
-export type ContentTextType = 'h1' | 'h2' | 'p'
+export type ContentTextType = 'h1' | 'h2' | 'p' | 'hr'
Committable suggestion skipped: line range outside the PR's diff.
Summary by Sourcery
Update the terms and conditions page with legal information, including privacy policy, terms of service, cookie policy, data protection, newsletter subscription, and children's privacy. Redirect terms and privacy policy links to
/legal/terms
. Add a global error boundary.New Features:
Tests:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores
Style