Skip to content
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

feat(mobile): toggle unread only #2873

Merged
merged 8 commits into from
Feb 25, 2025
Merged

feat(mobile): toggle unread only #2873

merged 8 commits into from
Feb 25, 2025

Conversation

hyoban
Copy link
Member

@hyoban hyoban commented Feb 24, 2025

Description

PR Type

  • Feature
  • Bugfix
  • Hotfix
  • Other (please describe):

Screenshots (if UI change)

Demo Video (if new feature)

Linked Issues

part of FOL-1572

Additional context

Changelog

  • I have updated the changelog/next.md with my changes.

Copy link

linear bot commented Feb 24, 2025

Copy link

vercel bot commented Feb 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
follow ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 25, 2025 2:51am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
follow-external-ssr ⬜️ Ignored (Inspect) Visit Preview Feb 25, 2025 2:51am

@follow-reviewer-bot
Copy link

Suggested PR Title:

feat(icon): enhance icons and update entry handling logic

Change Summary:
Updated multiple icon components to include circles for enhanced design, modified entry service to support unread settings, and adjusted hooks and stores for handling entry states based on general settings. This improves UI consistency and user control over entry visibility.

Code Review:

Code Review

File: apps/mobile/src/icons/user_setting_cute_fi.tsx

  • Lines: 14
    The deletion of the <Path> element with fill="#fff" fillOpacity={0.01} may negatively affect the accessibility or visual appearance of the icon. This element could have been intentionally added for a specific design or interaction purpose, such as providing a transparent click target. Verify with design specifications and ensure no regressions are caused by its removal.

File: apps/mobile/src/services/entry.ts

  • Lines: 53-57
    The hydrate method is modified to pass the unreadOnly setting directly into the upsertManyInSession method. However, this change alters the responsibilities of the method. The hydrate function should focus on fetching and hydrating data, not enforcing filtering logic. Consider refactoring to decouple filtering logic from this method and handle it in the reducer or another appropriate layer.

File: apps/mobile/src/store/entry/store.ts

  • Lines: 127-127
    The upsertManyInSession method introduces direct filtering based on the unreadOnly flag. This introduces side effects that could cause unexpected behavior in other parts of the application relying on a complete dataset (e.g., debugging tools or analytics). Consider applying filtering downstream in the selectors or hooks accessing the store.

File: scripts/svg-to-rn.ts

  • Lines: 33-35
    The logic in generateElement introduces dynamic tag name resolution (<${tagName}>), but fails to validate unsupported tags robustly. Although supportedTags is defined, any parsing errors or unsupported node attributes might lead to runtime issues. Enhance validation to handle unsupported tags explicitly and provide meaningful error messages.

  • Lines: 52-57
    The filtering logic in the convertSvgToRN function assumes the structure of supported SVG nodes but lacks error handling for cases where ast.children[0] might not exist or has a different structure. Add a guard clause to ensure the AST structure meets expectations before proceeding with transformations.


These changes would ensure maintainability, adherence to responsibilities, and prevent unexpected side effects. Address the points above before merging this pull request.

@hyoban hyoban merged commit 8d76c35 into dev Feb 25, 2025
11 checks passed
@hyoban hyoban deleted the feat/toggle-unread-only branch February 25, 2025 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant