REP/NIC Notifications + Push Notifications Settings + Drag to refresh for mobile#1764
REP/NIC Notifications + Push Notifications Settings + Drag to refresh for mobile#1764
Conversation
… for mobile Signed-off-by: prxt6529 <prxt@6529.io>
📝 WalkthroughWalkthroughAdds identity-rating notification rendering and a generic notification fallback; introduces push notification device/settings API and UI; adds native pull-to-refresh and a global refresh context; refactors notification types and query key/handling; and makes filter bar scrollable with identity composite filter. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AppUserConnect
participant PushNotificationSettings
participant API
participant Toast
User->>AppUserConnect: Click "Push Notifications"
AppUserConnect->>PushNotificationSettings: Open modal (isOpen=true)
rect rgba(100, 150, 200, 0.5)
Note over PushNotificationSettings: Load Phase
PushNotificationSettings->>PushNotificationSettings: Generate device ID
PushNotificationSettings->>API: GET /push-notifications/settings/{device_id}
API-->>PushNotificationSettings: Return current settings
end
rect rgba(150, 150, 100, 0.5)
Note over PushNotificationSettings: Edit Phase
User->>PushNotificationSettings: Toggle setting
PushNotificationSettings->>PushNotificationSettings: Update currentSettings
end
rect rgba(100, 200, 100, 0.5)
Note over PushNotificationSettings: Save Phase
User->>PushNotificationSettings: Click "Save Changes"
PushNotificationSettings->>API: PUT /push-notifications/settings/{device_id}
API-->>PushNotificationSettings: Settings saved
PushNotificationSettings->>Toast: Show success
end
PushNotificationSettings->>AppUserConnect: Close modal
sequenceDiagram
actor User
participant PullToRefresh
participant ContentElement
participant Router
participant Spinner
User->>PullToRefresh: Touch and drag down
PullToRefresh->>PullToRefresh: Verify at top & scrollable parent
loop On touchMove
PullToRefresh->>PullToRefresh: Calculate pullDistance
PullToRefresh->>ContentElement: Apply translateY transform
end
alt pullDistance >= PULL_THRESHOLD
rect rgba(100, 200, 100, 0.5)
PullToRefresh->>Spinner: Show spinner
PullToRefresh->>Router: Call router.refresh() / invalidateAll()
Router-->>PullToRefresh: Refresh complete
end
PullToRefresh->>PullToRefresh: Reset transform and state
else pullDistance < PULL_THRESHOLD
PullToRefresh->>ContentElement: Snap back (reset transform)
end
User->>PullToRefresh: Touch ends
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openapi.yaml (1)
7550-7571: Alignunmintednullability withedition_size.The schema marks
edition_sizeas nullable but keepsunmintedas non-nullable. Sinceunmintedis logically derived fromedition_size(remaining unminted count), it should also be nullable whenedition_sizeis null to prevent schema contract mismatches for clients.Suggested fix
unminted: type: number format: int64 + nullable: true
🤖 Fix all issues with AI agents
In `@components/header/PushNotificationSettings.tsx`:
- Around line 125-144: The save mutation (useMutation in
PushNotificationSettings handling mutationFn -> commonApiPut) lacks an onError
handler so failures give no user feedback; add an onError callback to the
useMutation config (alongside onSuccess) that calls setToast with an error
message (and optionally the error details) and ensures any UI state like
isSaving or onClose is handled appropriately so users see a failure notification
when saveSettings fails.
- Around line 88-98: The catch block in PushNotificationSettings currently
treats any error from commonApiFetch as a “not found” case and falls back to
DEFAULT_SETTINGS; change this to only use DEFAULT_SETTINGS when the API
indicates a 404/not-found and otherwise propagate or surface the error (e.g.,
rethrow or call processLogger/error handler) so real network/server errors
aren’t swallowed. Locate the call to commonApiFetch<ApiPushNotificationSettings>
and the catch that calls setOriginalSettings(DEFAULT_SETTINGS) /
setCurrentSettings(DEFAULT_SETTINGS), detect the error status (e.g., check
error.response?.status or the ApiError type/message) and handle 404 by setting
defaults but rethrow or log non-404 errors instead.
In `@components/providers/PullToRefresh.tsx`:
- Around line 110-128: The setTimeout used after triggering the refresh (when
pullDistance >= PULL_THRESHOLD) can fire after the component unmounts and cause
state updates; capture the timeout id(s) returned by setTimeout in variables
(for the block using pullDistance/PULL_THRESHOLD/isRefreshing and the other
similar block around the 147-155 range), store them in a ref (e.g.,
refreshTimeoutRef) and clear them in the cleanup of the effect/hook
(clearTimeout(refreshTimeoutRef.current)) to prevent setIsRefreshing,
setPullDistance or DOM style updates after teardown; ensure you also reset the
ref after clearing.
- Around line 83-102: When handling touch moves (the onTouchMove handler that
reads touchStartY.current and computes diff), ensure you reset pull state when
diff is <= 0: call setPullDistance(0) and clear any transform/transition on
contentRef.current so the indicator doesn't stick; also avoid calling
e.preventDefault() in that branch. Update the block that currently only handles
diff > 0 to include an else (or early-return) that resets pullDistance and
contentRef.current.style.transform/transition when the finger moves above the
start point (diff <= 0).
🧹 Nitpick comments (2)
types/feed.types.ts (1)
138-171: AddINotificationGenericto theTypedNotificationunion for type-safe fallback handling.Currently,
INotificationGenericis defined but excluded fromTypedNotification, forcingNotificationItemto use an unsafe cast in the default case (notification as unknown as INotificationGeneric). This should be part of the union to enable type-safe handling of unknown notification causes.♻️ Suggested type adjustment
export type TypedNotification = | INotificationIdentitySubscribed | INotificationIdentityMentioned | INotificationIdentityRep | INotificationIdentityNic | INotificationDropVoted | INotificationDropReacted | INotificationDropBoosted | INotificationDropQuoted | INotificationDropReplied | INotificationWaveCreated | INotificationAllDrops - | INotificationPriorityAlert; + | INotificationPriorityAlert + | INotificationGeneric;components/brain/notifications/NotificationItem.tsx (1)
107-112: Move notification type normalization to the data layer to eliminate the unsafe double cast.The
as unknown as INotificationGenericcast bypasses type safety. SinceINotificationGenericis designed as a fallback for unknown notification causes, consider mapping unrecognized causes to this type inuseNotificationsQueryrather than casting in the component. This could widen the query's return type to(TypedNotification | INotificationGeneric)[], allowingNotificationItemto receive properly typed notifications without unsafe casts.
Signed-off-by: prxt6529 <prxt@6529.io>
Signed-off-by: prxt6529 <prxt@6529.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/brain/notifications/NotificationsCauseFilter.tsx`:
- Around line 218-247: The two chevron buttons in NotificationsCauseFilter (the
left and right scroll buttons that call scrollLeft and scrollRight) are missing
type attributes and will default to type="submit" inside a form; update both
<button> elements that render the FontAwesomeIcon (the button with
onClick={scrollLeft} and the button with onClick={scrollRight}) to include
type="button" so they do not submit parent forms.
🧹 Nitpick comments (4)
components/providers/LayoutWrapper.tsx (1)
92-95: Minor redundancy:refreshKeyas bothkeyand inresetKeys.Using
refreshKeyas thekeyprop will unmount and remount the entireErrorBoundarywhen it changes, making its presence inresetKeysredundant. The current approach is harmless but slightly over-engineered.Consider keeping only one mechanism:
♻️ Option: Remove from key prop (preferred for smoother UX)
<ErrorBoundary - key={refreshKey} FallbackComponent={LayoutErrorFallback} resetKeys={[pathname, refreshKey]} >components/providers/PullToRefresh.tsx (1)
149-171: Event listeners churning on every pullDistance change.Since
handleTouchEnddepends onpullDistance, it's recreated on every state update during a pull gesture. This causes theuseEffectto remove and re-add all three event listeners repeatedly during the interaction, which can impact performance and potentially cause missed events.Consider using a ref to hold the latest callback values:
♻️ Suggested pattern using stable listeners
+ const handlersRef = useRef({ handleTouchStart, handleTouchMove, handleTouchEnd }); + useEffect(() => { + handlersRef.current = { handleTouchStart, handleTouchMove, handleTouchEnd }; + }); + useEffect(() => { if (!isNativeApp) return; + const onTouchStart = (e: TouchEvent) => handlersRef.current.handleTouchStart(e); + const onTouchMove = (e: TouchEvent) => handlersRef.current.handleTouchMove(e); + const onTouchEnd = () => handlersRef.current.handleTouchEnd(); + - document.addEventListener("touchstart", handleTouchStart, { + document.addEventListener("touchstart", onTouchStart, { passive: true, }); - document.addEventListener("touchmove", handleTouchMove, { passive: false }); - document.addEventListener("touchend", handleTouchEnd, { passive: true }); + document.addEventListener("touchmove", onTouchMove, { passive: false }); + document.addEventListener("touchend", onTouchEnd, { passive: true }); return () => { - document.removeEventListener("touchstart", handleTouchStart); - document.removeEventListener("touchmove", handleTouchMove); - document.removeEventListener("touchend", handleTouchEnd); + document.removeEventListener("touchstart", onTouchStart); + document.removeEventListener("touchmove", onTouchMove); + document.removeEventListener("touchend", onTouchEnd); // ... rest of cleanup }; - }, [isNativeApp, handleTouchStart, handleTouchMove, handleTouchEnd]); + }, [isNativeApp]);components/header/PushNotificationSettings.tsx (1)
78-117: MissingsetToastin dependency array and potential stale state on error.Two issues:
setToastis used inside the effect but not listed in the dependency array (line 117). This violates React's exhaustive-deps rule.On non-404 errors, the toast is shown but
originalSettings/currentSettingsare not reset. If the dialog was previously opened successfully, users will see stale settings instead of the error state.Suggested fix
loadSettings(); - }, [isOpen]); + }, [isOpen, setToast]);And to handle the stale state issue, reset settings before loading:
useEffect(() => { if (!isOpen) return; const loadSettings = async () => { setIsLoading(true); + setOriginalSettings(null); + setCurrentSettings(null); try {types/feed.types.ts (1)
140-166: Consider includingINotificationGenericin the union if it flows through typed APIs.
If unknown causes are meant to be handled by the generic renderer, adding it toTypedNotification(and thereforeTypedNotificationsResponse) avoids casts and keeps the type surface honest.💡 Suggested tweak
export type TypedNotification = | INotificationIdentitySubscribed | INotificationIdentityMentioned | INotificationIdentityRep | INotificationIdentityNic | INotificationDropVoted | INotificationDropReacted | INotificationDropBoosted | INotificationDropQuoted | INotificationDropReplied | INotificationWaveCreated | INotificationAllDrops - | INotificationPriorityAlert; + | INotificationPriorityAlert + | INotificationGeneric;
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/brain/notifications/NotificationsCauseFilter.tsx (1)
267-274: Missingtw-prefix onz-10class.Line 270 uses
z-10without thetw-prefix, which is inconsistent with the rest of the Tailwind classes in this codebase that use thetw-prefix.✅ Suggested fix
`tw-border-none tw-bg-transparent tw-no-underline tw-flex tw-justify-center tw-items-center tw-px-3 tw-py-2 tw-gap-2 tw-flex-1 tw-h-8 tw-rounded-lg tw-transition-colors tw-duration-300 - tw-ease-in-out tw-relative z-10 ${ + tw-ease-in-out tw-relative tw-z-10 ${ isActive ? "tw-text-iron-300" : "tw-text-iron-400 desktop-hover:hover:tw-text-iron-300" }`;
🤖 Fix all issues with AI agents
In `@components/providers/PullToRefresh.tsx`:
- Around line 50-68: handleTouchStart can start a new pull while a refresh is
active which later clears pullDistance; add an early return at the top of
handleTouchStart that checks isRefreshingRef.current and exits if true so no new
pull is initiated during refresh. Update the handleTouchStart callback
(referenced by isAtTop, getScrollableParent) to check isRefreshingRef.current
before setting isPulling.current, touchStartY.current, or contentRef.current.
- Around line 154-176: The effect registers touch event listeners but misses
touchcancel, which can leave the content transform stuck; add a touchcancel
listener (e.g., document.addEventListener("touchcancel", handleTouchEnd, {
passive: true }) or create a dedicated handleTouchCancel that mirrors
handleTouchEnd/reset logic) and remove it in the cleanup; ensure the handler
clears refreshTimeoutRef.current and resets
contentRef.current.style.transform/transition so interrupted gestures don't
leave the UI translated.
♻️ Duplicate comments (3)
components/brain/notifications/NotificationsCauseFilter.tsx (1)
218-249: Past issue resolved: chevron buttons now includetype="button".Both scroll buttons correctly specify
type="button"(lines 222 and 238), preventing unintended form submissions.components/header/PushNotificationSettings.tsx (2)
94-108: Error handling properly distinguishes 404 from other errors.The implementation now correctly treats only 404 responses as "settings not found" while surfacing other errors to the user via toast notification. This addresses the previous review feedback.
135-161: Error handling for save mutation is now in place.The
onErrorcallback properly surfaces failures to users via toast notification, addressing the previous review feedback.
🧹 Nitpick comments (3)
components/brain/notifications/NotificationsCauseFilter.tsx (2)
146-165: Inconsistent ResizeObserver instantiation without browser support check.Unlike lines 89-94 which check
typeof ResizeObserver !== "undefined", this useLayoutEffect creates a ResizeObserver directly. While modern browsers support it, this could cause issues in older environments.♻️ Suggested fix
// Handle layout shifts (resize, font load, etc.) const container = containerRef.current; if (!container) return; + if (typeof ResizeObserver === "undefined") return; const ro = new ResizeObserver(() => updateHighlightPosition(activeIndexRef.current) ); ro.observe(container); return () => ro.disconnect();
276-285: Addtype="button"to the filter button.The
NotificationCauseFilterButtoncomponent's button is missing thetype="button"attribute. Without it, the button defaults totype="submit"inside a form context, which could cause unintended behavior.✅ Suggested fix
return ( <button + type="button" className={getLinkClasses()} onClick={onClick} onMouseEnter={onMouseEnter} ref={buttonRef} >components/header/PushNotificationSettings.tsx (1)
78-117: MissingsetToastin dependency array.The effect uses
setToastbut it's not listed in the dependency array. WhilesetToastis likely stable from the auth context, ESLint'sexhaustive-depsrule would flag this.Suggested fix
loadSettings(); - }, [isOpen]); + }, [isOpen, setToast]);
Signed-off-by: prxt6529 <prxt@6529.io>
|
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.