Code Review Bench PR #25189 - Updated activitypub Bluesky sharing enablement flow

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#10

Updates the ActivityPub integration for Bluesky sharing enablement.

---

Summary by Gitar

• Fixed retry logic:
  • Changed counter check from > to >= and moved increment after validation in useConfirmBlueskyHandleMutationForUser to respect MAX_CONFIRMATION_RETRIES exactly
• Updated API endpoints:
  • Migrated all Bluesky endpoints from /activitypub/v1/* to /activitypub/v2/*, bumped package to 2.0.0
• Fixed toast behavior:
  • Added guard using retryCountRef.current > 0 to prevent success toast from displaying on page revisits

_{This will update automatically on new commits.}

---

[gitar-bot-staging]

Code Review ⚠️ Changes requested 0 resolved / 4 findings

Good refactoring of the Bluesky confirmation flow with correct retry logic fixes, but error paths in handleEnable and confirmHandle leave the UI stuck in loading state or generate unhandled promise rejections.

⚠️ Bug: handleEnable stuck in loading state on mutation error

📄 apps/activitypub/src/views/Preferences/components/BlueskySharing.tsx:48

The handleEnable function sets setLoading(true) before calling enableBlueskyMutation.mutateAsync(), but the try/finally block that previously called setLoading(false) was removed. If the mutation throws (network error, server error, etc.):

1. loading stays true permanently
2. The mutation's onSuccess never fires, so blueskyEnabled stays false/undefined in the cache
3. The useEffect doesn't re-trigger since its dependencies haven't changed
4. The user is stuck seeing "Enabling Bluesky sharing..." with no way to retry (the button is disabled={loading})

The onError handler in the mutation only handles 429 rate limits, it doesn't reset the component's loading state. Either restore the try/finally or add setLoading(false) in a .catch() / finally block.

Suggested fix

    const handleEnable = async () => {
        if (!account?.avatarUrl) {
            setIsEditingProfile(true);
        } else {
            setLoading(true);
            try {
                await enableBlueskyMutation.mutateAsync();
            } catch {
                setLoading(false);
            }
        }
    };

⚠️ Bug: Unhandled promise rejection in confirmHandle callback

📄 apps/activitypub/src/views/Preferences/components/BlueskySharing.tsx:68

The confirmHandle callback calls confirmBlueskyHandleMutation.mutateAsync().then(...) without a .catch() handler. mutateAsync returns a promise that rejects on error. While the mutation's onError handles 429 status codes, the promise rejection itself is not caught, meaning:

1. Any non-429 error (network failure, 500 server error, etc.) causes an unhandled promise rejection
2. This happens repeatedly since confirmHandle is called from a setInterval — each poll that fails will generate an unhandled rejection
3. In strict mode or some environments, unhandled rejections can crash the application

Either add a .catch() to swallow/log errors (since the retry logic handles failure via exhaustion), or use mutate() instead of mutateAsync() since there's no need for the returned promise — the mutation's onSuccess/onError callbacks already handle both cases.

Suggested fix

    const confirmHandle = useCallback(() => {
        confirmBlueskyHandleMutation.mutate();
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, []); // Empty deps - mutations are stable in practice

💡 Bug: Missing accountFollows invalidation in confirm mutation

📄 apps/activitypub/src/hooks/use-activity-pub-queries.ts:2833

Both useEnableBlueskyMutationForUser and useDisableBlueskyMutationForUser invalidate the accountFollows query on success, but useConfirmBlueskyHandleMutationForUser does not. There's even a comment at line 2833 acknowledging this: // Note: Missing invalidation of accountFollows query that should happen here.

If the confirm-handle endpoint triggers any following-related changes (which is plausible given the Bridgy Fed integration), the follows list could be stale. Consider adding the invalidation or documenting why it's intentionally omitted.

Suggested fix

            updateAccountBlueskyCache(queryClient, {
                blueskyEnabled: true,
                blueskyHandleConfirmed: true,
                blueskyHandle: blueskyHandle
            });

            queryClient.invalidateQueries({
                queryKey: QUERY_KEYS.accountFollows('index', 'following')
            });

💡 Quality: Disabling TypeScript strict mode is a significant regression

📄 apps/activitypub/tsconfig.json:18

Changing "strict": true to "strict": false in tsconfig.json disables a wide range of type safety checks including strictNullChecks, strictFunctionTypes, strictBindCallApply, strictPropertyInitialization, and more. This is a heavy-handed approach that can mask real bugs across the entire codebase, not just the code being changed.

If specific strict checks are causing issues, consider disabling only those specific flags (e.g., "strictNullChecks": false) rather than disabling all of strict mode. Better yet, fix the underlying type issues — they often indicate actual logic problems.

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: PR description was minimal (~20 words), enhanced with technical summary covering retry logic fix, API version bump, and toast behavior fix

_{5 rules not applicable. Show all rules by commenting gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[gitar-bot-staging]

⚠️ Bug: Unhandled promise rejection in confirmHandle callback

The confirmHandle callback calls confirmBlueskyHandleMutation.mutateAsync().then(...) without a .catch() handler. mutateAsync returns a promise that rejects on error. While the mutation's onError handles 429 status codes, the promise rejection itself is not caught, meaning:

1. Any non-429 error (network failure, 500 server error, etc.) causes an unhandled promise rejection
2. This happens repeatedly since confirmHandle is called from a setInterval — each poll that fails will generate an unhandled rejection
3. In strict mode or some environments, unhandled rejections can crash the application

Either add a .catch() to swallow/log errors (since the retry logic handles failure via exhaustion), or use mutate() instead of mutateAsync() since there's no need for the returned promise — the mutation's onSuccess/onError callbacks already handle both cases.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	const confirmHandle = useCallback(() => {
	const confirmHandle = useCallback(() => {
	confirmBlueskyHandleMutation.mutate();
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []); // Empty deps - mutations are stable in practice

• Apply suggested fix