Updated activitypub Bluesky sharing enablement flow#48
Conversation
ref TryGhost/ActivityPub#1377 We updated the backend implementation of the Bluesky sharing enablement process to include a confirmation step in which the client would prompt the backend to check what handle has been assigned to the Bluesky account generated by Bridgy Fed - This circumvents the need to for us to compute the handle manually and potentially getting it wrong if we compute it in a different way to Bridgy Fed
|
Caution Review failedFailed to post review comments WalkthroughThe pull request refactors the Bluesky integration from a single-step enable process to a two-step flow: enable followed by handle confirmation. API method signatures for Changes
Sequence DiagramsequenceDiagram
actor User
participant BlueskySharing as BlueskySharing Component
participant useEnableMutation as useEnableBlueskyMutation
participant useConfirmMutation as useConfirmBlueskyHandleMutation
participant API as ActivityPubAPI
participant Backend as Backend (v2 endpoints)
User->>BlueskySharing: Click Enable Bluesky
BlueskySharing->>useEnableMutation: trigger enableBluesky()
useEnableMutation->>API: enableBluesky()
API->>Backend: POST /v2/enable
Backend-->>API: 200 OK
API-->>useEnableMutation: onSuccess (void)
useEnableMutation-->>BlueskySharing: update cache (enabled: true, confirmed: false)
BlueskySharing->>BlueskySharing: start polling effect
loop Polling (interval, max retries)
BlueskySharing->>useConfirmMutation: trigger confirmBlueskyHandle()
useConfirmMutation->>API: confirmBlueskyHandle()
API->>Backend: POST /v2/confirm-handle
Backend-->>API: return handle string
API-->>useConfirmMutation: onSuccess with handle
useConfirmMutation-->>BlueskySharing: update cache (confirmed: true, handle: string)
BlueskySharing->>BlueskySharing: stop polling, show badge + profile link
end
User->>BlueskySharing: Click Disable Bluesky
BlueskySharing->>useEnableMutation: trigger disableBluesky()
useEnableMutation->>API: disableBluesky()
API->>Backend: POST /v2/disable
Backend-->>API: 200 OK
API-->>useEnableMutation: onSuccess (void)
useEnableMutation-->>BlueskySharing: update cache (enabled: false, confirmed: false)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@apps/activitypub/src/hooks/use-activity-pub-queries.ts`:
- Around line 2731-2743: The cached profile update in updateAccountBlueskyCache
incorrectly types the query callback param as BlueskyDetails; change the
callback parameter type to the full Account | undefined (or a union that matches
what's stored in the cache) so currentProfile reflects the actual cached shape,
and update the function signature/any related local types to use Account where
appropriate (keep using QUERY_KEYS.account('index') and the same
QueryClient.setQueryData call). Ensure the runtime guard (if (!currentProfile)
return currentProfile) remains valid for undefined cache entries.
- Around line 2827-2834: The updateAccountBlueskyCache call currently updates
the Bluesky flags but skips invalidating the accountFollows cache; add the same
invalidation used in useEnableBlueskyMutationForUser and
useDisableBlueskyMutationForUser so the UI refreshes—after calling
updateAccountBlueskyCache in the success handler, call the
queryClient.invalidateQueries for the accountFollows query key (the same
key/name those two hooks use) to force refetch/update of follow state.
In `@apps/activitypub/src/views/Preferences/components/BlueskySharing.tsx`:
- Around line 103-117: The polling interval clears when retries exceed
MAX_CONFIRMATION_RETRIES but the async call disableBlueskyMutation.mutateAsync()
is not inside a try/catch, so failures are unhandled; update the
confirmHandleInterval callback (where confirmHandle() is invoked and
retryCountRef is incremented) to await disableBlueskyMutation.mutateAsync()
inside a try/catch, handle errors by logging and/or showing toast.error, and
ensure setLoading(false) runs in a finally block (or after the catch) so loading
state is always cleared.
In `@apps/activitypub/tsconfig.json`:
- Line 18: The tsconfig currently turns off all strict type checking by setting
"strict": false; change this to enable stricter checks—either set "strict": true
in the tsconfig or selectively enable sub-options (e.g., "strictNullChecks",
"strictFunctionTypes", "strictPropertyInitialization") instead of disabling
everything; update the tsconfig entry that currently reads "strict": false and
then fix resulting type errors in code (use type guards, explicit types, or
non-null assertions) in functions/classes that fail under the stricter settings
such as any files flagged by the compiler until the project compiles cleanly.
🧹 Nitpick comments (3)
apps/activitypub/src/api/activitypub.ts (1)
723-733: Minor: redundant String() conversion.Line 728 already confirms
typeof json.handle === 'string', so theString()wrapper on line 732 is unnecessary.♻️ Suggested simplification
- return String(json.handle); + return json.handle;apps/activitypub/src/views/Preferences/components/BlueskySharing.tsx (2)
32-32: Initial state may become stale if account data changes.The state initializer reads from
accountduring the first render, but ifaccountchanges (e.g., from undefined to loaded data), the initial state won't update. This could lead to incorrect initial loading state.♻️ Suggested fix
- const [loading, setLoading] = useState(() => account?.blueskyEnabled && !account?.blueskyHandleConfirmed); + const [loading, setLoading] = useState(false);The
useEffecton lines 77-121 already handles synchronizing the loading state based on account changes, so initializing tofalseis sufficient.
68-75: Consider adding error handling to confirmHandle.The
confirmBlueskyHandleMutation.mutateAsync()call uses.then()without a.catch(). While the mutation hook handles errors internally (showing rate limit toasts), network errors or unexpected failures could go unhandled.♻️ Suggested improvement
const confirmHandle = useCallback(() => { confirmBlueskyHandleMutation.mutateAsync().then((handle) => { if (handle) { setHandleConfirmed(true); } - }); + }).catch(() => { + // Errors are handled by the mutation hook + }); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); // Empty deps - mutations are stable in practice
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/activitypub/package.jsonapps/activitypub/src/api/activitypub.test.tsapps/activitypub/src/api/activitypub.tsapps/activitypub/src/hooks/use-activity-pub-queries.tsapps/activitypub/src/views/Preferences/components/BlueskySharing.tsxapps/activitypub/tsconfig.json
🧰 Additional context used
🧬 Code graph analysis (2)
apps/activitypub/src/views/Preferences/components/BlueskySharing.tsx (1)
apps/activitypub/src/hooks/use-activity-pub-queries.ts (4)
useAccountForUser(1616-1628)useEnableBlueskyMutationForUser(2746-2776)useDisableBlueskyMutationForUser(2778-2808)useConfirmBlueskyHandleMutationForUser(2810-2841)
apps/activitypub/src/hooks/use-activity-pub-queries.ts (1)
apps/admin-x-framework/src/index.ts (1)
useQueryClient(44-44)
🔇 Additional comments (9)
apps/activitypub/package.json (1)
3-3: LGTM!The major version bump to 2.0.0 is appropriate given the breaking API changes where
enableBluesky()no longer returns a handle and the new two-step enable/confirm flow is introduced.apps/activitypub/src/hooks/use-activity-pub-queries.ts (1)
2725-2729: LGTM!The
BlueskyDetailstype cleanly encapsulates the Bluesky state properties needed for cache updates.apps/activitypub/src/api/activitypub.ts (2)
29-30: LGTM!The Account interface updates properly reflect the new Bluesky state model with
blueskyHandleConfirmedas an optional boolean andblueskyHandleacceptingstring | null | undefined.
711-721: LGTM!The
enableBlueskyanddisableBlueskymethods are cleanly implemented with the v2 endpoints. The void return type correctly reflects that enable no longer returns a handle.apps/activitypub/src/api/activitypub.test.ts (3)
1600-1626: LGTM!The test correctly verifies that
enableBlueskysends a POST request to the v2 endpoint and no longer expects a return value.
1629-1656: LGTM!The
disableBlueskytest correctly verifies the POST method and v2 endpoint.
1658-1767: LGTM!Excellent test coverage for
confirmBlueskyHandlecovering:
- Successful handle confirmation
- Null response handling
- Missing handle property
- Invalid handle type (array instead of string)
apps/activitypub/src/views/Preferences/components/BlueskySharing.tsx (2)
27-28: LGTM!The polling constants are reasonable: 5 second intervals with 12 retries gives approximately 60 seconds for handle confirmation, which should accommodate network delays.
190-228: LGTM!The confirmed state UI is well-structured with the avatar, Bluesky badge overlay, handle display with copy functionality, and the "Open profile" link. The profile URL construction correctly strips the leading
@from the handle.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| function updateAccountBlueskyCache(queryClient: QueryClient, blueskyDetails: BlueskyDetails) { | ||
| const profileQueryKey = QUERY_KEYS.account('index'); | ||
|
|
||
| queryClient.setQueryData(profileQueryKey, (currentProfile?: {blueskyEnabled: boolean, blueskyHandle: string | null}) => { | ||
| queryClient.setQueryData(profileQueryKey, (currentProfile?: BlueskyDetails) => { | ||
| if (!currentProfile) { | ||
| return currentProfile; | ||
| } | ||
|
|
||
| return { | ||
| ...currentProfile, | ||
| blueskyEnabled: blueskyHandle !== null, | ||
| blueskyHandle | ||
| ...blueskyDetails | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Type annotation may be too narrow.
The currentProfile parameter is typed as BlueskyDetails but the cached data is a full Account object. This could cause type confusion or silent failures if the cache is empty/undefined.
♻️ Suggested fix
- queryClient.setQueryData(profileQueryKey, (currentProfile?: BlueskyDetails) => {
+ queryClient.setQueryData(profileQueryKey, (currentProfile?: Account) => {🤖 Prompt for AI Agents
In `@apps/activitypub/src/hooks/use-activity-pub-queries.ts` around lines 2731 -
2743, The cached profile update in updateAccountBlueskyCache incorrectly types
the query callback param as BlueskyDetails; change the callback parameter type
to the full Account | undefined (or a union that matches what's stored in the
cache) so currentProfile reflects the actual cached shape, and update the
function signature/any related local types to use Account where appropriate
(keep using QUERY_KEYS.account('index') and the same QueryClient.setQueryData
call). Ensure the runtime guard (if (!currentProfile) return currentProfile)
remains valid for undefined cache entries.
| updateAccountBlueskyCache(queryClient, { | ||
| blueskyEnabled: true, | ||
| blueskyHandleConfirmed: true, | ||
| blueskyHandle: blueskyHandle | ||
| }); | ||
|
|
||
| // Note: Missing invalidation of accountFollows query that should happen here | ||
| }, |
There was a problem hiding this comment.
Address the TODO: missing cache invalidation.
The comment on line 2833 notes that accountFollows invalidation is missing. Both useEnableBlueskyMutationForUser and useDisableBlueskyMutationForUser invalidate this query on success (lines 2766-2768, 2797-2799). For consistency and to ensure the UI reflects the latest state, this invalidation should be added here.
♻️ Suggested fix
updateAccountBlueskyCache(queryClient, {
blueskyEnabled: true,
blueskyHandleConfirmed: true,
blueskyHandle: blueskyHandle
});
- // Note: Missing invalidation of accountFollows query that should happen here
+ queryClient.invalidateQueries({
+ queryKey: QUERY_KEYS.accountFollows('index', 'following')
+ });
},Do you want me to open an issue to track this task?
📝 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.
| updateAccountBlueskyCache(queryClient, { | |
| blueskyEnabled: true, | |
| blueskyHandleConfirmed: true, | |
| blueskyHandle: blueskyHandle | |
| }); | |
| // Note: Missing invalidation of accountFollows query that should happen here | |
| }, | |
| updateAccountBlueskyCache(queryClient, { | |
| blueskyEnabled: true, | |
| blueskyHandleConfirmed: true, | |
| blueskyHandle: blueskyHandle | |
| }); | |
| queryClient.invalidateQueries({ | |
| queryKey: QUERY_KEYS.accountFollows('index', 'following') | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In `@apps/activitypub/src/hooks/use-activity-pub-queries.ts` around lines 2827 -
2834, The updateAccountBlueskyCache call currently updates the Bluesky flags but
skips invalidating the accountFollows cache; add the same invalidation used in
useEnableBlueskyMutationForUser and useDisableBlueskyMutationForUser so the UI
refreshes—after calling updateAccountBlueskyCache in the success handler, call
the queryClient.invalidateQueries for the accountFollows query key (the same
key/name those two hooks use) to force refetch/update of follow state.
| const confirmHandleInterval = setInterval(async () => { | ||
| if (retryCountRef.current >= MAX_CONFIRMATION_RETRIES) { | ||
| clearInterval(confirmHandleInterval); | ||
|
|
||
| toast.error('Something went wrong, please try again.'); | ||
|
|
||
| await disableBlueskyMutation.mutateAsync(); | ||
| setLoading(false); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| retryCountRef.current += 1; | ||
| confirmHandle(); | ||
| }, CONFIRMATION_INTERVAL); |
There was a problem hiding this comment.
Missing error handling for async operations in polling.
The disableBlueskyMutation.mutateAsync() call on line 109 is not wrapped in try/catch. If this mutation fails, the interval would already be cleared but the error would be unhandled, potentially causing silent failures.
♻️ Suggested fix
const confirmHandleInterval = setInterval(async () => {
if (retryCountRef.current >= MAX_CONFIRMATION_RETRIES) {
clearInterval(confirmHandleInterval);
toast.error('Something went wrong, please try again.');
- await disableBlueskyMutation.mutateAsync();
- setLoading(false);
+ try {
+ await disableBlueskyMutation.mutateAsync();
+ } catch {
+ // Error toast already shown by mutation
+ } finally {
+ setLoading(false);
+ }
return;
}
retryCountRef.current += 1;
confirmHandle();
}, CONFIRMATION_INTERVAL);📝 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.
| const confirmHandleInterval = setInterval(async () => { | |
| if (retryCountRef.current >= MAX_CONFIRMATION_RETRIES) { | |
| clearInterval(confirmHandleInterval); | |
| toast.error('Something went wrong, please try again.'); | |
| await disableBlueskyMutation.mutateAsync(); | |
| setLoading(false); | |
| return; | |
| } | |
| retryCountRef.current += 1; | |
| confirmHandle(); | |
| }, CONFIRMATION_INTERVAL); | |
| const confirmHandleInterval = setInterval(async () => { | |
| if (retryCountRef.current >= MAX_CONFIRMATION_RETRIES) { | |
| clearInterval(confirmHandleInterval); | |
| toast.error('Something went wrong, please try again.'); | |
| try { | |
| await disableBlueskyMutation.mutateAsync(); | |
| } catch { | |
| // Error toast already shown by mutation | |
| } finally { | |
| setLoading(false); | |
| } | |
| return; | |
| } | |
| retryCountRef.current += 1; | |
| confirmHandle(); | |
| }, CONFIRMATION_INTERVAL); |
🤖 Prompt for AI Agents
In `@apps/activitypub/src/views/Preferences/components/BlueskySharing.tsx` around
lines 103 - 117, The polling interval clears when retries exceed
MAX_CONFIRMATION_RETRIES but the async call disableBlueskyMutation.mutateAsync()
is not inside a try/catch, so failures are unhandled; update the
confirmHandleInterval callback (where confirmHandle() is invoked and
retryCountRef is incremented) to await disableBlueskyMutation.mutateAsync()
inside a try/catch, handle errors by logging and/or showing toast.error, and
ensure setLoading(false) runs in a finally block (or after the catch) so loading
state is always cleared.
|
|
||
| /* Linting */ | ||
| "strict": true, | ||
| "strict": false, |
There was a problem hiding this comment.
Disabling strict mode weakens type safety for the entire app.
Setting "strict": false disables important type checks including strictNullChecks, strictFunctionTypes, and strictPropertyInitialization. This is a broad change that could mask type errors throughout the codebase.
Consider enabling specific strict sub-options while disabling only the ones causing issues:
♻️ Suggested approach
- "strict": false,
+ "strict": true,
+ "strictNullChecks": false,Or better yet, keep strict mode and fix the type issues with proper type guards/assertions where needed.
📝 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.
| "strict": false, | |
| "strict": true, | |
| "strictNullChecks": false, |
🤖 Prompt for AI Agents
In `@apps/activitypub/tsconfig.json` at line 18, The tsconfig currently turns off
all strict type checking by setting "strict": false; change this to enable
stricter checks—either set "strict": true in the tsconfig or selectively enable
sub-options (e.g., "strictNullChecks", "strictFunctionTypes",
"strictPropertyInitialization") instead of disabling everything; update the
tsconfig entry that currently reads "strict": false and then fix resulting type
errors in code (use type guards, explicit types, or non-null assertions) in
functions/classes that fail under the stricter settings such as any files
flagged by the compiler until the project compiles cleanly.
3 participants
Benchmark PR from agentic-review-benchmarks#10
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.