add privacy page#114
Conversation
WalkthroughAdds client-side routing (react-router-dom), nests the main UI under a wildcard Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
privacy.html (4)
7-7: Consider using a built Tailwind CSS file for production.Using the Tailwind CDN can impact performance as it includes all utility classes and requires runtime processing. For a production environment, consider generating a minimal CSS file containing only the classes used in this page.
This can be achieved by:
- Installing Tailwind CSS as a dependency
- Creating a build process that generates a minified CSS file
- Replacing the CDN script with a link to the generated CSS file
This would reduce the initial load size and improve page performance.
398-406: Dark mode script may be unnecessary.The page uses dark theme colors by default (
bg-base-900 text-base-100on line 58), but the script adds adarkclass based on system preference. Since there are no light mode color variants defined in the HTML, this script has no visual effect.Consider either:
- Remove the script if the page should always be dark-themed
- Implement proper light/dark mode toggle with appropriate color classes (e.g.,
dark:bg-base-900 bg-white)
66-78: Add accessibility attributes to SVG icons.The SVG icons throughout the page lack accessibility attributes. For better screen reader support, consider:
- Adding
aria-hidden="true"to decorative icons (like the checkmarks)- Adding
role="img"andaria-labelto meaningful icons (like the header logo)Example for decorative icon:
<svg + aria-hidden="true" class="w-5 h-5 text-green-500 mt-0.5 flex-shrink-0" fill="none" stroke="currentColor" viewBox="0 0 24 24" >Example for meaningful icon:
<svg + role="img" + aria-label="ObsidianIRC logo" class="w-6 h-6 text-white" fill="none" stroke="currentColor" viewBox="0 0 24 24" >Also applies to: 160-172
360-365: Update contact email to project's organizational domain.The contact email
obsidian@gmail.comis a personal Gmail account. For a public project, use a domain-specific email address (e.g.,contact@obsidianirc.devor similar) to appear more professional and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker/nginx.conf(1 hunks)privacy.html(1 hunks)
🔇 Additional comments (1)
privacy.html (1)
86-377: Well-structured privacy policy content.The privacy policy is comprehensive and well-organized with clear sections covering all essential aspects:
- Data collection and usage
- Data security measures
- User rights
- Legal compliance (children's privacy, international users)
- Contact information
The semantic HTML structure and responsive design using Tailwind classes are appropriate for this use case.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/App.test.tsx (1)
177-196: Duplicate test detected.This test (
"Shows error message when server connection fails") is an exact duplicate of the test at lines 156-175. Remove the duplicate or replace it with a different test case.- it("Shows error message when server connection fails", async () => { - render( - <BrowserRouter> - <App /> - </BrowserRouter>, - ); - const user = userEvent.setup(); - - // Mock failed connection - vi.mocked(ircClient.connect).mockRejectedValueOnce( - new Error("Connection failed"), - ); - - // Open modal - await user.click(screen.getByTestId("server-list-options-button")); - await user.click(screen.getByText(/Add Server/i)); - - // Check that toggleAddServerModal was called - expect(mockStoreState.toggleAddServerModal).toHaveBeenCalledWith(true); - });
🧹 Nitpick comments (3)
src/components/PrivacyPolicy.tsx (2)
32-34: Reminder: Keep the "Last updated" date current.The hardcoded date should be updated whenever the privacy policy content is modified to maintain accuracy and user trust.
18-25: Consider a placeholder icon instead of hiding on error.When the logo image fails to load, hiding it leaves an empty gradient container. A simple fallback icon or text would provide better UX.
<img src="/images/obsidian-transparent.png" alt="ObsidianIRC" className="w-10 h-10" onError={(e) => { - e.currentTarget.style.display = "none"; + e.currentTarget.outerHTML = '<span class="text-white text-2xl font-bold">O</span>'; }} />Alternatively, use a conditional render with a state variable for cleaner React patterns.
src/App.tsx (1)
173-212: Consider whether/privacyshould share layout and connection behavior with the main appThe routing setup is valid and will correctly render
PrivacyPolicyat/privacyand the main UI under/*. A couple of design points you may want to revisit:
/privacycurrently renders onlyPrivacyPolicy, withoutAppLayout, so users won’t see the usual shell/navigation on that page. If you want a consistent frame (e.g., header/back navigation), consider wrappingPrivacyPolicyin the same layout or having AppLayout host nested routes (including privacy).- App-level effects (env initialization and
connectToSavedServers) still run regardless of the current route. If/privacyis intended as a low-impact/static page (e.g., linked from outside the app before connecting anywhere), you might want to scope the connection/init logic so it only runs for the main app route (e.g., move intoAppLayoutor a route element).Not blockers, but worth aligning with desired UX and privacy expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/App.tsx(2 hunks)src/components/PrivacyPolicy.tsx(1 hunks)src/components/ui/UserSettings.tsx(5 hunks)src/main.tsx(2 hunks)tests/App.test.tsx(6 hunks)tests/setup.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/UserSettings.tsx (1)
src/components/ui/settings/SettingRenderer.tsx (1)
SettingField(240-299)
🔇 Additional comments (9)
tests/setup.ts (1)
20-32: LGTM! LocalStorage mock covers core functionality.The mock provides the essential methods (
getItem,setItem,removeItem,clear) needed for most test scenarios. If future tests requirelengthorkey()(for iteration), consider extending the mock to fully implement theStorageinterface.src/components/ui/UserSettings.tsx (3)
125-130: LGTM! Privacy category follows established patterns.The new privacy category is well-integrated with consistent structure: icon, title, id, and description matching the existing categories.
752-758: Verify the contact email address.The email
obsidian@gmail.comappears to be a generic placeholder. Ensure this is the intended contact email for privacy inquiries, as it will be user-facing.
1145-1165: LGTM! Rendering logic correctly handles the new privacy category.The conditional rendering properly routes the privacy category to its custom renderer while excluding it from the generic
SettingFieldpath, consistent with how profile and account categories are handled.src/components/PrivacyPolicy.tsx (2)
7-9: LGTM! Document title setup is correct.The
useEffectwith an empty dependency array properly sets the document title once on component mount.
533-551: Same email verification concern applies here.The
obsidian@gmail.comemail appears here as well. Ensure this is a valid, monitored mailbox for handling privacy-related inquiries.tests/App.test.tsx (1)
112-116: LGTM! Tests correctly wrap App with BrowserRouter.The routing context is properly provided for all test renders, matching the production setup in
main.tsx.src/main.tsx (1)
12-17: LGTM! Router setup is correct.
BrowserRouteris properly placed insideStrictMode, providing routing context to the entire application.Note: Ensure your web server is configured to serve
index.htmlfor all routes (e.g.,/privacy) to support client-side routing in production. Without proper server configuration, direct navigation to/privacywill result in a 404.src/App.tsx (1)
7-10: Routing imports and PrivacyPolicy wiring look correctThe new
react-router-domimports andPrivacyPolicyimport are consistent with the routing usage below and look fine, assumingAppis rendered within a<BrowserRouter>(or equivalent) at the root.
matheusfillipe
force-pushed
the
add-privacy-policy-tmp
branch
from
f1d832a to
1beab5b
Compare
matheusfillipe
force-pushed
the
add-privacy-policy-tmp
branch
from
1beab5b to
0848fb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ui/UserSettings.tsx (1)
685-775: Verify privacy panel text matches actual behavior and update contact details if neededThe privacy panel UI itself is fine, but several statements here are policy/compliance-relevant and should be double‑checked:
- “Local Storage: Your messages and settings are stored locally on your device”
- “No Central Server: We don't store your IRC communications on our servers”
- “Anonymous Analytics: Optional crash reports to improve the app”
- The contact email
obsidian@gmail.comTo avoid problems with Google Play and general privacy compliance, you’ll want to ensure:
- These bullets exactly match what the app and any backend actually do (e.g., what’s stored locally vs remotely, which analytics/crash services you use, what data they collect, whether they’re truly optional and opt‑in).
- The email address is a real, monitored contact for privacy inquiries (and ideally matches your project branding or domain).
If any of these are placeholders, consider tightening the wording and substituting real contact details before publishing.
If you’d like, I can help you draft more precise wording here based on what the app actually does (e.g., which services are used, what’s stored where).
src/App.tsx (1)
7-7: Routing structure and/privacyroute integration look soundUsing
<Routes>with a dedicated/privacyroute and a catch‑all/*route for the main app UI is a clean approach:
/privacyrendersPrivacyPolicywithout the chat layout or modals, which is ideal for a standalone policy page you can link from the Play Store.- All existing UI (AppLayout, settings/modal stack, notices popup, etc.) now lives under
/*, preserving behavior for normal usage.From a react-router-dom perspective this is correct and idiomatic.
Two things to double‑check operationally:
- Your hosting (e.g. Cloudflare Pages / any web deployment) should be configured as an SPA so that direct requests to
/privacyserve yourindex.htmland let the router renderPrivacyPolicy.- If you expect
/privacyto be opened from inside the app (e.g. via the new link in User Settings), confirm that the behavior is what you want on all targets (desktop/Tauri vs web/Android WebView) — opening in a new tab/window vs same view.Also applies to: 10-10, 173-212
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/App.tsx(2 hunks)src/components/PrivacyPolicy.tsx(1 hunks)src/components/ui/UserSettings.tsx(5 hunks)src/main.tsx(2 hunks)tests/App.test.tsx(6 hunks)tests/setup.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/App.test.tsx
- tests/setup.ts
- src/components/PrivacyPolicy.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/UserSettings.tsx (1)
src/components/ui/settings/SettingRenderer.tsx (1)
SettingField(240-299)
🔇 Additional comments (3)
src/main.tsx (1)
3-3: BrowserRouter integration at the entry point looks correctWrapping
<App />in<BrowserRouter>underReact.StrictModeis the right way to supply routing context to the whole app, and it matches the new<Routes>usage insideApp.tsx. No issues from a React/router perspective.Please just confirm that your web hosting / Tauri setup is configured for SPA-style routing so direct hits to paths like
/privacystill loadindex.htmland let the router handle the route.Also applies to: 14-16
src/components/ui/UserSettings.tsx (2)
13-16: Privacy category wiring and icon usage are consistentAdding
"privacy"toSettingsCategory, wiring it intocategories, and usingFaShieldAltfor the icon is type-safe and keeps the sidebar model consistent with existing categories. No functional issues here.If
settingsRegistry.getByCategory("privacy")isn’t backed by any definitions, just confirm that it safely returns an empty array rather than throwing, since it’s still being called even though the privacy category is custom-rendered.Also applies to: 79-86, 94-131
1145-1165: Custom rendering for the privacy category is correctly excluded from the generic settings rendererConditionally rendering
renderPrivacyFields()whenactiveCategory === "privacy"and excluding"profile","account", and"privacy"from the genericSettingFieldloop keeps behavior clean: only real registry-backed categories hit the generic renderer.Just ensure
settingsRegistry.getByCategorydoesn’t rely on"privacy"or"account"being present to avoid surprises if someone later adds registry entries for these categories.
|
merging because I need this for testing other things. |
* add privacy page * Use react router instead
1 participant
@ValwareIRC under https://add-privacy-policy-tmp.obsidianirc.pages.dev/privacy
Just a vibed template right now, help me enhance it as needed. (it has some missconceptions as of now)
Needed for google playstore android app.
Summary by CodeRabbit
New Features
UI / Navigation
Tests / Chores
✏️ Tip: You can customize this high-level summary in your review settings.