From 099d86aa77794cf4a173cef9946bcaba1f85f190 Mon Sep 17 00:00:00 2001 From: taylorcox75 Date: Mon, 7 Sep 2026 14:02:45 -0500 Subject: [PATCH 1/3] Fix reconnect UI after a long background: skeleton instead of stale data + auth toast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After the app sits backgrounded for hours and is reopened, the torrents list kept showing stale cards and the torrent detail screen popped an "Authentication failed" toast (then "Torrent not found") for a few seconds while the app silently re-logged in. Root cause: TorrentContext's "clear recovery" effect compared dataUpdatedAt against 0, but that timestamp is the last successful sync from potentially hours ago, so it cleared isRecoveringFromBackground on the very next render instead of waiting for a genuinely new fetch. That's why the existing skeleton guard on the torrents list never engaged, and why nothing signaled a reconnect was in flight at all — checkAndReconnect() never had a visible "in progress" state anywhere. - context/TorrentContext.tsx: baseline the recovery-clear effect against the pre-recovery dataUpdatedAt instead of comparing to 0; add a 15s safety cap so a wedged recovery can't leave the skeleton up forever. - context/ServerContext.tsx: add isReconnecting, set only by the checkAndReconnect() run that owns the in-flight promise, kept separate from isLoading/isConnecting so their existing consumers are unaffected. - app/(tabs)/(torrents)/index.tsx: skeleton guard also covers isReconnecting (a session dying while already foregrounded, not just on background return). - app/(tabs)/(torrents)/torrent/[hash].tsx: classify a load failure as a dead session (same check useReactiveReconnect uses) and show a new SkeletonTorrentDetail instead of toasting the raw auth error and falling through to "Torrent not found"; retries immediately once the reconnect resolves instead of waiting on the next 2s poll. - components/SkeletonLoader.tsx: new SkeletonTorrentDetail placeholder. No preference keys, colors, or stored ServerConfig fields were touched — this is purely in-memory UI/state, so no upgrade path is needed for existing users. No qBittorrent API surface changed either. 3.8.43, changelog entry added. --- app/(tabs)/(torrents)/index.tsx | 12 +- app/(tabs)/(torrents)/torrent/[hash].tsx | 42 +++++- components/SkeletonLoader.tsx | 48 +++++++ constants/changelog.ts | 13 ++ context/ServerContext.tsx | 15 +++ context/TorrentContext.tsx | 59 ++++++--- package.json | 2 +- tests/rn/components/SkeletonLoader.test.tsx | 13 +- tests/rn/context/ServerContext.test.tsx | 61 +++++++++ tests/rn/context/TorrentContext.test.tsx | 138 ++++++++++++++++++++ 10 files changed, 379 insertions(+), 24 deletions(-) create mode 100644 tests/rn/context/TorrentContext.test.tsx diff --git a/app/(tabs)/(torrents)/index.tsx b/app/(tabs)/(torrents)/index.tsx index 9ff1a5c0..f87a0bbc 100644 --- a/app/(tabs)/(torrents)/index.tsx +++ b/app/(tabs)/(torrents)/index.tsx @@ -80,7 +80,13 @@ export default function TorrentsScreen() { initialLoadComplete, } = useTorrents(); const { graceError, isPendingError } = useGracefulError(error); - const { isConnected, isLoading: serverIsLoading, isConnecting, connectToServer } = useServer(); + const { + isConnected, + isLoading: serverIsLoading, + isConnecting, + isReconnecting, + connectToServer, + } = useServer(); const { colors, isDark } = useTheme(); const params = useLocalSearchParams<{ magnet?: string | string[]; @@ -1139,7 +1145,9 @@ export default function TorrentsScreen() { if ( (isConnecting && !showAddModal) || (!initialLoadComplete && (serverIsLoading || !isConnected || isLoading)) || - (initialLoadComplete && !showAddModal && (isRecoveringFromBackground || isPendingError)) + (initialLoadComplete && + !showAddModal && + (isRecoveringFromBackground || isReconnecting || isPendingError)) ) { return ( <> diff --git a/app/(tabs)/(torrents)/torrent/[hash].tsx b/app/(tabs)/(torrents)/torrent/[hash].tsx index c6e9ba59..bd1400b5 100644 --- a/app/(tabs)/(torrents)/torrent/[hash].tsx +++ b/app/(tabs)/(torrents)/torrent/[hash].tsx @@ -33,6 +33,7 @@ import { useServer } from '@/context/ServerContext'; import { useTheme } from '@/context/ThemeContext'; import { useToast } from '@/context/ToastContext'; import { useTorrents } from '@/context/TorrentContext'; +import { isReconnectableError } from '@/hooks/useReactiveReconnect'; import { isRealTracker, getPseudoTrackerStates, @@ -41,6 +42,7 @@ import { } from '@/utils/trackers'; import { FocusAwareStatusBar } from '@/components/FocusAwareStatusBar'; import { AnimatedProgressBar } from '@/components/AnimatedProgressBar'; +import { SkeletonTorrentDetail } from '@/components/SkeletonLoader'; import { SpeedGraph, computeSpeedGraphMax, niceGraphCeiling } from '@/components/SpeedGraph'; import { PieceMap } from '@/components/PieceMap'; import { InputModal, InputModalPreset } from '@/components/InputModal'; @@ -113,7 +115,7 @@ export default function TorrentDetail() { const { hash } = useLocalSearchParams<{ hash: string }>(); const router = useRouter(); const navigation = useNavigation(); - const { isConnected, isLoading } = useServer(); + const { isConnected, isLoading, isReconnecting } = useServer(); const { features } = useApiFeatures(); const { colors, isDark } = useTheme(); const { showToast } = useToast(); @@ -137,6 +139,11 @@ export default function TorrentDetail() { const [pieceStates, setPieceStates] = useState([]); const [loading, setLoading] = useState(true); const [refreshing, setRefreshing] = useState(false); + // True while a load failed on what looks like a dead session (an + // auto-reconnect is already in flight, per useReactiveReconnect's + // classification) — suppresses the error toast and keeps the skeleton up + // instead of falling through to "Torrent not found". See loadTorrentData. + const [sessionRecovering, setSessionRecovering] = useState(false); const [actionLoading, setActionLoading] = useState(false); const [optimisticPaused, setOptimisticPaused] = useState(null); const [lastUpdatedAt, setLastUpdatedAt] = useState(null); @@ -196,6 +203,22 @@ export default function TorrentDetail() { // eslint-disable-next-line react-hooks/exhaustive-deps }, [hash, isConnected]); + // A load that failed on a dead session (sessionRecovering) doesn't get a + // fresh attempt until the next 2s silentRefresh tick, since isConnected + // never actually changes (true the whole time). Retry immediately once + // the auto-reconnect that was already in flight resolves, rather than + // waiting on that tick. + const wasReconnectingRef = useRef(isReconnecting); + useEffect(() => { + const wasReconnecting = wasReconnectingRef.current; + wasReconnectingRef.current = isReconnecting; + if (wasReconnecting && !isReconnecting && sessionRecovering && isConnected) { + loadTorrentData(); + } + // loadTorrentData isn't memoized — only re-run on the isReconnecting transition. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [isReconnecting, sessionRecovering, isConnected]); + // Encryption is a global qBittorrent setting, not per-torrent — fetch once // per connection rather than on every poll tick. useEffect(() => { @@ -268,6 +291,7 @@ export default function TorrentDetail() { handleTorrentGone(); return null; } + setSessionRecovering(false); setTorrent(next); setProperties(props); setTrackers(trackersData); @@ -284,6 +308,15 @@ export default function TorrentDetail() { handleTorrentGone(); return null; } + // A dead session (auto-reconnect already in flight, per the same + // classification useReactiveReconnect uses) self-heals within a + // couple of seconds — show the skeleton instead of a toast that's + // stale the moment it appears, and "Torrent not found" for what's + // actually an auth problem. + if (isReconnectableError(getErrorMessage(error))) { + setSessionRecovering(true); + return null; + } showToast(getErrorMessage(error), 'error'); return null; } finally { @@ -321,6 +354,7 @@ export default function TorrentDetail() { handleTorrentGone(); return; } + setSessionRecovering(false); setTorrent(next); setProperties(props); setTrackers(trackersData); @@ -1016,13 +1050,11 @@ export default function TorrentDetail() { ); } - if (loading && !torrent) { + if ((loading || sessionRecovering || isReconnecting) && !torrent) { return ( <> - - - + ); } diff --git a/components/SkeletonLoader.tsx b/components/SkeletonLoader.tsx index e412145b..0d7ff0cb 100644 --- a/components/SkeletonLoader.tsx +++ b/components/SkeletonLoader.tsx @@ -91,6 +91,39 @@ export function SkeletonTorrentCard() { ); } +/** + * Skeleton for the torrent detail screen — shown while a dead session is + * reconnecting and there's no torrent data to render yet (see + * app/(tabs)/(torrents)/torrent/[hash].tsx). Loosely shaped after that + * screen's hero card (name + state badge, progress bar, size line) plus one + * section below — an approximate placeholder, not a pixel-accurate clone, + * same spirit as SkeletonTorrentCard above. + */ +export function SkeletonTorrentDetail() { + const { colors } = useTheme(); + + return ( + + + + + + + + + + + + + + + + + + + ); +} + const styles = StyleSheet.create({ container: { overflow: 'hidden', @@ -106,4 +139,19 @@ const styles = StyleSheet.create({ justifyContent: 'space-between', alignItems: 'center', }, + detailContainer: { + paddingHorizontal: 12, + paddingTop: 10, + }, + detailCard: { + padding: 12, + marginBottom: 12, + borderRadius: 12, + }, + heroHeaderRow: { + flexDirection: 'row', + justifyContent: 'space-between', + alignItems: 'center', + marginBottom: 10, + }, }); diff --git a/constants/changelog.ts b/constants/changelog.ts index 8d073b68..7aeda79a 100644 --- a/constants/changelog.ts +++ b/constants/changelog.ts @@ -18,6 +18,19 @@ export interface ChangelogRelease { sections?: ChangelogSection[]; } export const CHANGELOG: ChangelogRelease[] = [ + { + version: '3.8.43', + date: '2026-09-07', + sections: [ + { + title: 'Bugs Fixed', + items: [ + 'Fixed the torrent list and torrent detail screen showing old data instead of a loading state while reconnecting after a long time in the background', + 'Fixed an authentication error appearing when opening a torrent before the app finished reconnecting', + ], + }, + ], + }, { version: '3.8.42', date: '2026-09-06', diff --git a/context/ServerContext.tsx b/context/ServerContext.tsx index 48cda41f..443dfeac 100644 --- a/context/ServerContext.tsx +++ b/context/ServerContext.tsx @@ -38,6 +38,17 @@ interface ServerContextType { * Null while disconnected. */ connectedAt: Date | null; + /** + * True while checkAndReconnect() (the reactive, error-driven auto-reconnect + * path) is in flight. Deliberately separate from `isConnecting` — that + * flag also covers a *manual* reconnect() and feeds `isLoading`, both of + * which are consumed in places that shouldn't change behavior for an + * automatic background recovery. Lets UI (e.g. the torrents list, torrent + * detail) show a soft "reconnecting" placeholder instead of stale data or + * a hard auth error during the window before an automatic reconnect + * resolves. + */ + isReconnecting: boolean; connectToServer: (server: ServerConfig) => Promise; disconnect: () => Promise; /** Drop the remembered last server (e.g. after it was deleted). */ @@ -61,6 +72,7 @@ export function ServerProvider({ children }: { children: ReactNode }) { const [activeEndpoint, setActiveEndpoint] = useState(null); const [initLoading, setInitLoading] = useState(true); const [reconnecting, setReconnecting] = useState(false); + const [isReconnecting, setIsReconnecting] = useState(false); // Tracks the current connection's start time from isConnected transitions // rather than from each individual setIsConnected call site, so every @@ -272,6 +284,7 @@ export function ServerProvider({ children }: { children: ReactNode }) { } const run = async (): Promise => { + setIsReconnecting(true); if (!currentServer) { setIsConnected(false); setActiveEndpoint(null); @@ -299,6 +312,7 @@ export function ServerProvider({ children }: { children: ReactNode }) { const id = currentServer?.id; const promise = run().finally(() => { + setIsReconnecting(false); if (checkAndReconnectPromiseRef.current?.id === id) { checkAndReconnectPromiseRef.current = null; } @@ -321,6 +335,7 @@ export function ServerProvider({ children }: { children: ReactNode }) { connectedAt, isLoading, isConnecting, + isReconnecting, activeEndpoint, connectToServer, disconnect, diff --git a/context/TorrentContext.tsx b/context/TorrentContext.tsx index 8181283a..73fa7d7d 100644 --- a/context/TorrentContext.tsx +++ b/context/TorrentContext.tsx @@ -59,6 +59,14 @@ export function TorrentProvider({ children }: { children: ReactNode }) { const [isRecoveringState, setIsRecoveringState] = useState(false); const [initialLoadComplete, setInitialLoadComplete] = useState(false); + // Tracks the most recent successful sync timestamp, and the value it was + // at when a recovery window started — so the "clear recovery" effect below + // can tell a genuinely new fetch apart from the stale timestamp that was + // already sitting there when recovery began. See that effect for why this + // distinction matters. + const dataUpdatedAtRef = useRef(0); + const recoveryBaselineRef = useRef(0); + const syncQueryFn = useCallback(async (): Promise => { const version = syncVersionRef.current; const currentRid = ridRef.current; @@ -169,13 +177,35 @@ export function TorrentProvider({ children }: { children: ReactNode }) { } }, [dataUpdatedAt, initialLoadComplete]); - // Clear recovery state after successful fetch + // Keep the latest successful-sync timestamp available to the AppState + // handler (below) without adding it to that effect's own deps. + useEffect(() => { + dataUpdatedAtRef.current = dataUpdatedAt; + }, [dataUpdatedAt]); + + // Clear recovery state only once a fetch *newer than the one already + // sitting there when recovery started* actually lands. `dataUpdatedAt` + // being merely nonzero isn't enough — it holds the last successful sync + // from potentially hours ago, so comparing against 0 cleared this on the + // very next render, before the foreground re-sync had a chance to run. useEffect(() => { - if (dataUpdatedAt > 0 && isRecoveringState) { + if (isRecoveringState && dataUpdatedAt > recoveryBaselineRef.current) { setIsRecoveringState(false); } }, [dataUpdatedAt, isRecoveringState]); + // Safety cap: if a foreground recovery never resolves (e.g. the refetch + // below got deduped into an already-in-flight poll and no new data ever + // lands), don't leave the user on a skeleton forever. The normal exits are + // already bounded — a successful re-sync clears the flag above, and a + // genuine failure flips isConnected false, which routes to the + // not-connected screen regardless. + useEffect(() => { + if (!isRecoveringState) return undefined; + const timeout = setTimeout(() => setIsRecoveringState(false), 15000); + return () => clearTimeout(timeout); + }, [isRecoveringState]); + // Reset sync state when disconnected useEffect(() => { if (!isConnected) { @@ -204,6 +234,10 @@ export function TorrentProvider({ children }: { children: ReactNode }) { lastActiveTime.current = Date.now(); if (isConnected) { + // Baseline against the last successful sync *before* flipping the + // flag, so the clear effect above can tell a genuinely new fetch + // apart from the stale timestamp already sitting there. + recoveryBaselineRef.current = dataUpdatedAtRef.current; setIsRecoveringState(true); // Deliberately NOT eagerly reconnecting here. checkAndReconnect @@ -226,20 +260,15 @@ export function TorrentProvider({ children }: { children: ReactNode }) { queryClient.invalidateQueries({ queryKey: ['torrents'] }).finally(() => resolve()); }); }); - // Only clear the recovering flag once the re-sync actually - // succeeded. invalidateQueries settles regardless of whether the - // refetch itself failed (e.g. the qBittorrent session died while - // backgrounded), so clearing unconditionally here briefly exposed - // a real "Authentication failed" error before the reactive - // reconnect effect (which watches queryError, above) had a chance - // to re-login and succeed. Leaving it set on failure means the - // "clear after successful fetch" effect above is what turns it - // off, once a subsequent poll or reconnect actually goes through — - // and isConnected flipping false (genuine disconnect) still falls + // Deliberately not clearing the flag here — invalidateQueries + // settles regardless of whether the refetch itself failed (e.g. + // the qBittorrent session died while backgrounded). The baselined + // clear effect above is what turns recovery off, once a fetch + // newer than the pre-recovery baseline actually lands (either + // this re-sync succeeding outright, or a later poll succeeding + // after the reactive reconnect effect re-logs in) — and + // isConnected flipping false (genuine disconnect) still falls // through to the normal not-connected screen regardless. - if (queryClient.getQueryState(['torrents'])?.status !== 'error') { - setIsRecoveringState(false); - } } } else if (nextAppState === 'background') { lastActiveTime.current = Date.now(); diff --git a/package.json b/package.json index f217291e..9ab5e954 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "qRemote", - "version": "3.8.42", + "version": "3.8.43", "easBuild": true, "main": "index.ts", "scripts": { diff --git a/tests/rn/components/SkeletonLoader.test.tsx b/tests/rn/components/SkeletonLoader.test.tsx index c3563b0e..1d06ad18 100644 --- a/tests/rn/components/SkeletonLoader.test.tsx +++ b/tests/rn/components/SkeletonLoader.test.tsx @@ -1,6 +1,10 @@ import React from 'react'; import { render } from '@testing-library/react-native'; -import { SkeletonLoader, SkeletonTorrentCard } from '@/components/SkeletonLoader'; +import { + SkeletonLoader, + SkeletonTorrentCard, + SkeletonTorrentDetail, +} from '@/components/SkeletonLoader'; jest.mock('@/context/ThemeContext', () => ({ useTheme: () => ({ colors: require('./theme-mock').mockColors }), @@ -26,3 +30,10 @@ describe('SkeletonTorrentCard', () => { expect(toJSON()).toBeTruthy(); }); }); + +describe('SkeletonTorrentDetail', () => { + it('renders without crashing', async () => { + const { toJSON } = await render(); + expect(toJSON()).toBeTruthy(); + }); +}); diff --git a/tests/rn/context/ServerContext.test.tsx b/tests/rn/context/ServerContext.test.tsx index 92c1d1a3..1214d7fc 100644 --- a/tests/rn/context/ServerContext.test.tsx +++ b/tests/rn/context/ServerContext.test.tsx @@ -416,4 +416,65 @@ describe('ServerContext', () => { // reconnect should have only been triggered once despite two callers expect((ServerManager.reconnect as jest.Mock).mock.calls.length).toBe(1); }); + + it('isReconnecting is true while checkAndReconnect is in flight, then false on success', async () => { + (ServerManager.getCurrentServer as jest.Mock).mockResolvedValue(server1); + (ServerManager.connectToServer as jest.Mock).mockResolvedValue(true); + (apiClient.getServer as jest.Mock).mockReturnValue(server1); + + const getLatest = await renderProvider(); + await waitFor(() => expect(getLatest().isLoading).toBe(false)); + + let resolveReconnect: (value: boolean) => void; + (ServerManager.reconnect as jest.Mock).mockImplementation( + () => + new Promise((resolve) => { + resolveReconnect = resolve; + }), + ); + + expect(getLatest().isReconnecting).toBe(false); + + // Deliberately not wrapped in act() — checkAndReconnect's async run() + // suspends immediately on the still-pending ServerManager.reconnect + // promise, so the update this triggers is left for waitFor's own + // act-wrapped polling to observe rather than an outer act() scope. + const pending = getLatest().checkAndReconnect(); + await waitFor(() => expect(getLatest().isReconnecting).toBe(true)); + + await act(async () => { + resolveReconnect(true); + await pending; + }); + expect(getLatest().isReconnecting).toBe(false); + }); + + it('isReconnecting clears after checkAndReconnect fails (both reconnect and connectToServer)', async () => { + (ServerManager.getCurrentServer as jest.Mock).mockResolvedValue(server1); + (ServerManager.connectToServer as jest.Mock).mockResolvedValue(true); + (apiClient.getServer as jest.Mock).mockReturnValue(server1); + + const getLatest = await renderProvider(); + await waitFor(() => expect(getLatest().isLoading).toBe(false)); + + let rejectReconnect: (error: unknown) => void; + (ServerManager.reconnect as jest.Mock).mockImplementation( + () => + new Promise((_resolve, reject) => { + rejectReconnect = reject; + }), + ); + (ServerManager.connectToServer as jest.Mock).mockRejectedValue(new Error('dead too')); + + const pending = getLatest().checkAndReconnect(); + await waitFor(() => expect(getLatest().isReconnecting).toBe(true)); + + let result: boolean | undefined; + await act(async () => { + rejectReconnect(new Error('dead')); + result = await pending; + }); + expect(result).toBe(false); + expect(getLatest().isReconnecting).toBe(false); + }); }); diff --git a/tests/rn/context/TorrentContext.test.tsx b/tests/rn/context/TorrentContext.test.tsx new file mode 100644 index 00000000..47cc5c49 --- /dev/null +++ b/tests/rn/context/TorrentContext.test.tsx @@ -0,0 +1,138 @@ +import React from 'react'; +import { Text, AppState } from 'react-native'; +import { render, waitFor, act } from '@testing-library/react-native'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { TorrentProvider, useTorrents } from '@/context/TorrentContext'; +import { useServer } from '@/context/ServerContext'; +import { syncApi } from '@/services/api/sync'; +import { MainData } from '@/types/api'; + +jest.mock('@/context/ServerContext', () => ({ useServer: jest.fn() })); +jest.mock('@/services/api/sync', () => ({ + syncApi: { getMainData: jest.fn() }, +})); +// The reactive auto-reconnect path has its own dedicated test coverage +// (tests/rn/hooks/useReactiveReconnect.test.ts) — mock it out here so this +// suite can isolate TorrentContext's own recovery-flag bookkeeping. +jest.mock('@/hooks/useReactiveReconnect', () => ({ useReactiveReconnect: jest.fn() })); + +const emptyMainData: MainData = { + rid: 1, + full_update: true, + torrents: {}, + categories: {}, + tags: [], + server_state: {}, +}; + +function Consumer({ onRender }: { onRender: (ctx: ReturnType) => void }) { + const ctx = useTorrents(); + onRender(ctx); + return {ctx.isRecoveringFromBackground ? 'recovering' : 'idle'}; +} + +function makeQueryClient() { + return new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); +} + +async function renderProvider() { + let latest: ReturnType | undefined; + const queryClient = makeQueryClient(); + await render( + + + (latest = ctx)} /> + + , + ); + return () => latest!; +} + +beforeEach(() => { + jest.clearAllMocks(); + jest + .mocked(useServer) + .mockReturnValue({ isConnected: true } as unknown as ReturnType); +}); + +describe('TorrentContext', () => { + it('throws when useTorrents used outside provider', async () => { + const BadConsumer = () => { + useTorrents(); + return null; + }; + // Suppress React error logging noise + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}); + await expect(render()).rejects.toThrow( + 'useTorrents must be used within a TorrentProvider', + ); + spy.mockRestore(); + }); + + it('keeps isRecoveringFromBackground true across a foreground re-sync that fails', async () => { + jest.mocked(syncApi.getMainData).mockResolvedValue(emptyMainData); + + let appStateHandler: ((state: string) => void) | undefined; + jest.spyOn(AppState, 'addEventListener').mockImplementation((_event, handler) => { + appStateHandler = handler as (state: string) => void; + return { remove: jest.fn() } as unknown as ReturnType; + }); + + const getLatest = await renderProvider(); + + // Let the initial successful sync land — this is the timestamp a naive + // "dataUpdatedAt > 0" check would (wrongly) treat as proof recovery is + // already done. + await waitFor(() => expect(getLatest().initialLoadComplete).toBe(true)); + expect(getLatest().isRecoveringFromBackground).toBe(false); + + // The session died while backgrounded — the foreground re-sync fails. + jest + .mocked(syncApi.getMainData) + .mockRejectedValue(new Error('Authentication failed. Please check your credentials.')); + + await act(async () => { + appStateHandler?.('background'); + }); + await act(async () => { + appStateHandler?.('active'); + }); + + // Wait for the foreground re-sync to actually have been attempted (and + // failed) before asserting on the flag it's supposed to leave behind. + await waitFor(() => { + expect(jest.mocked(syncApi.getMainData).mock.calls.length).toBeGreaterThan(1); + }); + + expect(getLatest().isRecoveringFromBackground).toBe(true); + // The error is suppressed while recovering, so the torrents list keeps + // showing its skeleton rather than a hard error during this window. + expect(getLatest().error).toBeNull(); + }); + + it('clears isRecoveringFromBackground once a genuinely new sync succeeds', async () => { + jest.mocked(syncApi.getMainData).mockResolvedValue(emptyMainData); + + let appStateHandler: ((state: string) => void) | undefined; + jest.spyOn(AppState, 'addEventListener').mockImplementation((_event, handler) => { + appStateHandler = handler as (state: string) => void; + return { remove: jest.fn() } as unknown as ReturnType; + }); + + const getLatest = await renderProvider(); + await waitFor(() => expect(getLatest().initialLoadComplete).toBe(true)); + + await act(async () => { + appStateHandler?.('background'); + }); + await act(async () => { + appStateHandler?.('active'); + }); + + await waitFor(() => { + expect(getLatest().isRecoveringFromBackground).toBe(false); + }); + }); +}); From 8a23655f6cb9b7c5d497f409a88d9316c4618b4d Mon Sep 17 00:00:00 2001 From: taylorcox75 Date: Mon, 7 Sep 2026 14:47:53 -0500 Subject: [PATCH 2/3] docs: correct the retired develop flow, right-size the run rules, add RN test traps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md had drifted from the repo in ways that actively misled agents: - **`develop` is gone.** The doc mandated it in ten places ("Never commit to develop", "Always cut from develop — there is no hotfix exception", `gh pr create --base develop`), but no such branch exists locally or on origin, and no PR has ever targeted it. Everything since #219 goes straight to main (#218 and earlier used preview). An agent following the doc would have `git switch develop` fail outright. Rewritten around main, with an explicit note that develop/preview are retired so the next reader doesn't re-derive it, plus the release/vX.Y.ZZ branch that does get used. - **docs/RELEASING.md's changelog tree was half-dead** for the same reason — the "On develop → .TESTFLIGHT placeholder" branch can never fire (`grep -c TESTFLIGHT constants/changelog.ts` is 0). Removed; the main-branch rules it kept are unchanged. The run-cost model was off by ~25x, which was buying caution nobody needed: one suite ~18s claimed → 1.2s actual; tsc 25s warm / 85s cold → 0.8s / 3.2s full npm test 4.9s (1095 tests); lint 6.4s; whole batch ~12s The three-tier "exactly three moments to run anything" ceremony was therefore discouraging verification to save ~12 seconds. Replaced with the constraint that actually binds — context, not wall time, since npm test dumps hundreds of lines of unrelated act()/Animated noise. New rule: run the narrow check freely, pipe it through tail. Commit-time batch is unchanged. Also added two rn-project test traps that each cost about an hour to rediscover: a synchronous act(() => ...) around a promise-returning call silently swallows the state update (looks exactly like a product bug), and render() returns a promise so an outside-provider throw needs .rejects.toThrow. Smaller corrections: the Alert deviations list was missing ConfirmModal itself (nine calls across eight files, not eight); File Index gains SkeletonTorrentDetail and notes ServerContext.isReconnecting and how TorrentContext's recovery flag is cleared. --- AGENTS.md | 135 +++++++++++++++++++++++++++++++--------------- docs/RELEASING.md | 32 +++++------ 2 files changed, 107 insertions(+), 60 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ad75cb50..43d5147e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,35 +80,43 @@ version rather than the qBittorrent version: > renames are per-site: handling one does not handle the others. When you add a > version-dependent parameter, gate it *and* verify against the older wiki. -### Don't burn runs +### Run checks freely — but quietly -Every typecheck / test / lint run costs real tokens and wall time. There are -exactly **three** moments to run anything, and no others: +**The checks are fast. The output is not.** Measured on this repo: -**1. Mid-task → run nothing.** Never fire `tsc`, `jest`, or `eslint` after an -individual edit or "just to be safe." Trust the edit and keep working. +| Command | Wall time | +|---|---| +| One suite — `npm test -- tests/utils/format.test.ts` | ~1s | +| `npm test` (both projects, 1000+ tests) | ~5s | +| `npx tsc --noEmit` | ~1s warm, ~3s cold (incremental via `.tsbuildinfo`) | +| `npm run lint` | ~6s | -**2. Handing back a non-trivial change → the impacted suites only.** Never the -full run here. Pick the narrowest command that covers what you actually touched: +So wall time is not the constraint — **context is**. `npm test` prints hundreds +of lines of unrelated `act()` warnings and Animated stack traces from +`Confetti` / `PathAutocompleteInput`, and every line of that stays in the +conversation and is re-sent on every later turn. -| What you changed | What to run | -|---|---| -| One module that has a test | That one suite — `npm test -- tests/utils/format.test.ts` (~18s). Same form for `tests/services/…` and `tests/rn/…`. | -| Pure logic — `utils/`, `services/`, locales | `npm test -- --selectProjects node` (skips the slow jest-expo project) | -| Components, hooks, context | `npm test -- --selectProjects rn` | -| Types, or a change that crosses many files | `npx tsc --noEmit` | +**Therefore: run whatever you need, but pipe it.** -**Trivial edits need nothing** — a comment, a copy tweak, a doc line, a single -string. Use judgment; the point is to catch real breakage, not to perform rigor. +```bash +npm test 2>&1 | tail -5 # summary only +npx tsc --noEmit; echo "EXIT:$?" # silent when clean +npm run lint 2>&1 | tail -3 # the problem count +``` -**3. Before a commit → the full batch.** See [Commit-time checks](#commit-time-checks). +Run the narrow thing while you work — one suite after touching one module, +`tsc` after a type change. Catching a broken test one second after you write it +is far cheaper than discovering it at commit time and re-deriving what you did. +**Trivial edits still need nothing** — a comment, a copy tweak, a doc line. + +Only widen to a full unpiped run when something actually fails and you need the +detail; then read the failure, not the whole log. + +**Before a commit → the full batch.** See [Commit-time checks](#commit-time-checks). ### Read narrowly -Tool output is **permanent and recurring**: whatever a command prints stays in -the conversation and is re-sent on every later turn. A single careless dump of a -few hundred lines is a tax on the whole rest of the session, so the cost of -reading too much is much higher than it looks at the moment you do it. +Same reason as above — output is permanent and re-sent every turn: - **Never `cat` a whole file** to answer a narrow question. Use `grep -n` (with `-A`/`-B` for context) or a ranged read. Reach for the file's shape first — @@ -129,11 +137,13 @@ Only now do you run the whole thing — once, as a batch: | Command | Bar | |---|---| -| `npx tsc --noEmit` | Exit 0. Currently clean. Incremental via `.tsbuildinfo` (~25s warm, ~85s cold). | +| `npx tsc --noEmit` | Exit 0. Currently clean. Incremental via `.tsbuildinfo`. | | `npm test` | All passing, both projects — see [Testing](#7-testing). | -| `npm run lint` | **Zero errors.** Warnings are baseline noise; the count drifts, don't chase it. | +| `npm run lint` | **Zero errors.** Warnings are baseline noise (currently 37); the count drifts, don't chase it. | | `npm run format` | Prettier. Run it last, so it also formats anything you just changed. | +The whole batch is ~12s. Pipe each through `tail` unless it fails. + Then, in the same pre-commit pass: - **Update the [File Index](#5-file-index)** if you added, removed, or renamed a @@ -151,33 +161,37 @@ before. ### Branches — always work on one -**Never commit to `main`. Never commit to `develop`.** Both are protected by -convention: work reaches them only through a PR. There is no exception for a -one-line fix or a "quick" change. +**Never commit to `main`.** It's protected by convention: work reaches it only +through a PR. There is no exception for a one-line fix or a "quick" change. -Every change starts on its own branch cut from `develop`: +Every change starts on its own branch cut from `main`: ```bash -git switch develop && git pull && git switch -c bugfix/#123-short-description +git switch main && git pull && git switch -c bugfix/#123-short-description ``` Naming follows what's already in the repo — `feature/…`, `bugfix/…`, or `fix/…`, usually carrying the issue number (`bugfix/#177`, `feature/#121`). Agent-created branches use a `claude/…` prefix. -**Always cut from `develop` — there is no hotfix exception.** However urgent a -fix is, it goes branch → PR → `develop` → `main`. Never branch from `main`, and -never shortcut a fix straight into it. +**There is no hotfix exception.** However urgent a fix is, it goes branch → PR → +`main`. Never commit straight to `main`. | Branch | Role | |---|---| -| *your branch* | Where every commit goes. Cut from `develop`, merged back by PR. | -| `develop` | Integration branch. Receives work by PR only. Its changelog entry is a `.TESTFLIGHT` placeholder — see [docs/RELEASING.md](docs/RELEASING.md). | -| `main` | Release branch. Receives `develop` by PR. **Pushing it with `"easBuild": true` in `package.json` builds and submits to the App Store** — see [docs/RELEASING.md](docs/RELEASING.md). | +| *your branch* | Where every commit goes. Cut from `main`, merged back by PR. | +| `main` | The only long-lived branch. Receives work by PR. **Pushing it with `"easBuild": true` in `package.json` builds and submits to the App Store** — see [docs/RELEASING.md](docs/RELEASING.md). | +| `release/vX.Y.ZZ` | Occasionally used to gather a release's work before one PR to `main`. Only when the user sets one up — don't create one on your own. | + +> **`develop` and `preview` are retired** (as of 2026-08-14). Neither branch +> exists. Older PRs (#218 and earlier) targeted `preview`; everything since +> #219 goes straight to `main`. If you find guidance anywhere referring to +> either, it's stale. **Commit and push only when asked.** If you're asked to commit and you're sitting -on `main` or `develop`, branch first, then commit — don't ask whether the rule -applies this time. +on `main`, branch first, then commit — don't ask whether the rule applies this +time. If you're already on an unrelated branch (one cut for different work), +cut a fresh one rather than mixing the histories. ### When asked to commit, go all the way to a PR @@ -187,7 +201,7 @@ applies this time. 2. Branch if you aren't already on one. 3. Commit. **No `Co-Authored-By: Claude` trailer** on this repo. 4. `git push -u origin ` -5. `gh pr create --base develop` with a short summary and a test-plan line +5. `gh pr create --base main` with a short summary and a test-plan line covering what you ran. Stop there. **Never merge the PR** — review and merge are the user's. @@ -350,9 +364,18 @@ proxy server incl. auth, IP filtering/banned IPs, I2P (qBit 5.0+ / `connectedAt` tracks when the current connection began (derived from `isConnected` transitions, not each `setIsConnected` call site) for a client-side "Connected For" display — qBittorrent's `server_state` has no - session-uptime field of its own (#232). + session-uptime field of its own (#232). `isReconnecting` is true only while + `checkAndReconnect()` is in flight, set by the run that owns the shared + promise — deliberately *not* folded into `isLoading`/`isConnecting`, whose + consumers shouldn't change behavior for an automatic recovery. It's what + lets the torrents list and torrent detail show a skeleton instead of stale + data or a raw auth error during that window. - **`TorrentContext.tsx`** — rid-based incremental sync, plus the reactive auto-reconnect effect the other providers piggyback on. + `isRecoveringFromBackground` covers the foreground re-sync; it's cleared only + once a fetch *newer than the pre-recovery `dataUpdatedAt`* lands, never by + that timestamp merely being nonzero (which cleared it instantly, since it + holds the last success from potentially hours ago). - **`TransferContext.tsx`** — transfer-info poll; relies on TorrentContext's reconnect. - **`ToastContext.tsx`** + `components/Toast.tsx` — the global toast is a plain view. **Never wrap it in an RN ``** — a Modal captures all touches and @@ -387,7 +410,9 @@ All PascalCase function components taking a `…Props` interface. then defaults then the `avatarColor` fallback), `SearchResultRow` (+ internal ActionPill; the `+` button and the cart-toggle button are independent — see its header comment), `FilterChip`, `EmptyState`, `SkeletonLoader` - (+ `SkeletonTorrentCard`), `PieceMap`, `ServerIconBadge` (per-server tinted + (+ `SkeletonTorrentCard`, `SkeletonTorrentDetail` — the latter covers the + detail screen while a dead session reconnects), `PieceMap`, + `ServerIconBadge` (per-server tinted icon badge — `ServerConfig.icon`/`iconColor` via `utils/server.ts` `getServerIcon`/`getServerIconColor`, falling back to a default icon and `DEFAULT_AVATAR_COLOR` — never the name-derived `avatarColor`, so a badge's @@ -577,6 +602,31 @@ already carry the right imports and mocks: Import app code as `@/…` — both projects map it to the repo root. +**Two `rn`-project traps that will cost you an hour each if you meet them cold:** + +- **Don't wrap a trigger call in synchronous `act(() => …)`.** In this + React/RTL version that *silently swallows* the state update — the component + never re-renders and your assertion sees the old value, with no warning. It + looks exactly like a product bug. Call the function bare and let `waitFor` + observe the result: + + ```ts + const pending = getLatest().checkAndReconnect(); // NOT inside act() + await waitFor(() => expect(getLatest().isReconnecting).toBe(true)); + await act(async () => { resolveIt(true); await pending; }); // async act is fine + ``` + + `await act(async () => …)` around a *fully awaited* operation works normally. + It's the sync form wrapping a promise-returning call that breaks. + +- **`render()` returns a promise here.** Tests `await render(...)`, and a + component that throws during render gives you a *rejected promise*, not a + synchronous throw. The "hook used outside its provider" test therefore reads: + + ```ts + await expect(render()).rejects.toThrow('must be used within'); + ``` + --- ## 7. Testing @@ -632,10 +682,11 @@ rather than a translation gap. 5. **All user-facing strings go through i18n** — `const { t } = useTranslation()`. 6. **Prefer themed dialogs**: `InputModal` over `Alert.prompt`, `ConfirmModal` over `Alert.alert`. Native alerts ignore the app theme. **Don't add a new - one.** *Known deviations* — eight existing sites: `settings/advanced`, - `settings/torrent-defaults` ×2, `search/plugins`, `server/[id]`, `TagsModal`, - `CategoryModal`, `SuperDebugPanel`. Converting one while you're already in - that file is welcome, but it's never required. + one.** *Known deviations* — nine existing calls across eight files: + `settings/advanced`, `settings/torrent-defaults` ×2, `search/plugins`, + `server/[id]`, `TagsModal`, `CategoryModal`, `SuperDebugPanel`, and + `ConfirmModal` itself. Converting one while you're already in that file is + welcome, but it's never required. 7. **Delete superseded files in the same change.** When a component is replaced by a route-level screen or vice versa, remove the old one rather than leaving dead code. Precedent: `components/TorrentDetails.tsx` was deleted once its diff --git a/docs/RELEASING.md b/docs/RELEASING.md index 883cee13..837e95ef 100644 --- a/docs/RELEASING.md +++ b/docs/RELEASING.md @@ -23,24 +23,20 @@ speculative entries, no "while I'm here" additions, no entry per edit. When the user does ask, edit it as the **last step before the commit** — one pass covering everything in the session, not one entry per change. -### Which entry to edit depends on the branch - -The app ships OTA through EAS Update, so the two branches track versions -differently: - -- **On `develop`** — the top entry is a placeholder whose version ends in - `.TESTFLIGHT` (`X.Y.TESTFLIGHT` — the `X.Y` moves with the release train, so - match on the **suffix**, never on a specific number). **Append** your line to - the matching section in that entry. Don't create an entry, don't renumber the - placeholder, don't invent a version. If the top entry *isn't* a `.TESTFLIGHT` - placeholder, stop and ask which entry to use rather than guessing. -- **On `main`** — versions are real. Compare `package.json`'s `version` to - `CHANGELOG[0].version`: - - **They DIFFER** (changelog ahead) → an unreleased entry is already open. - **Append** to it. Don't create an entry, don't change its version. - - **They're EQUAL** → the top entry is already released. Add ONE new entry at - the top with a **patch bump only** and today's date. **Don't touch - `package.json`** — the release process owns the app version. +### Which entry to edit + +All work now branches off `main` and returns by PR (`develop` and `preview` are +retired — see AGENTS.md). Versions in the changelog are therefore always real; +there is no `.TESTFLIGHT` placeholder flow any more. + +Compare `package.json`'s `version` to `CHANGELOG[0].version`: + +- **They DIFFER** (changelog ahead) → an unreleased entry is already open. + **Append** to it. Don't create an entry, don't change its version. +- **They're EQUAL** → the top entry is already released. Add ONE new entry at + the top with a **patch bump only** and today's date. **Don't touch + `package.json`** — the release process owns the app version, unless the user + explicitly asks you to bump it. Patch numbers are always two digits (`3.8.05`, never `3.8.5`). From ceed6e20764e61d8605c2b59f56b24c6040e2356 Mon Sep 17 00:00:00 2001 From: taylorcox75 Date: Mon, 7 Sep 2026 15:04:58 -0500 Subject: [PATCH 3/3] docs: fix links broken by prior edit, correct a stale claim, add Gotchas section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review of the AGENTS.md rewrite from the previous commit: - Fixed a dead anchor (#dont-burn-runs) left over from renaming that section, and collapsed the top "How to work a task" workflow from 7 steps to 6 so it actually matches the run-checks-freely rewrite instead of still describing the old run-nothing-mid-task / verify-separately two-step split. - Wrote a script to slugify every heading and check every (#anchor) link resolves — all 19 links against 31 headings check out now. - Corrected a stale File Index claim: useTorrentActions.ts was documented as "used by both list and detail," but only the list screen imports it — the detail screen hand-rolls its own parallel handlePauseResume/handleDelete/etc. This fed the "Add a torrent action" recipe, which told an agent to edit one place when two need it; fixed both the Hooks entry and the recipe. Also, per request: framed the file as a living document (a short note near the top pointing at where to fix drift), and added a new §10 Gotchas section for surprises that don't have a natural home elsewhere — tooling quirks or misleading errors that look like product bugs. Seeded with the RN act() testing trap as the first entry, with instructions for future sessions to append rather than silently work around and move on. --- AGENTS.md | 71 +++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 56 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 43d5147e..745d83f2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -6,6 +6,15 @@ qBittorrent servers over the WebUI API v2. Read this file top to bottom once. The **File Index** is a complete map — trust it instead of re-exploring, and open only the files you're actually changing. +**This is a living document, not a snapshot.** It drifts — a rename lands and a +File Index entry doesn't follow, a branch gets retired and the branching +section still names it. [§8 Rule 8](#8-critical-rules) says not to trust it +blindly; the flip side is: when you hit something it got wrong, or something +that cost you real time because nothing here warned you, **fix it in the same +change** — correct the stale claim where it lives, or add a line to +[§10 Gotchas](#10-gotchas) if it doesn't have a natural home. A five-minute fix +now is cheaper than every future session re-learning the same thing. + ## How to work a task 1. **Read it as an API question first** — [§1](#think-in-api-terms-first): which @@ -14,13 +23,13 @@ it instead of re-exploring, and open only the files you're actually changing. 3. **Copy the nearest sibling.** Whatever you're adding — a screen, a test, a settings row, a loading or empty state — one like it already exists. Match it instead of inventing a pattern. -4. **Edit. Run nothing while you work.** -5. **Verify narrowly** — the impacted suite only, and only if the change is - non-trivial ([§1](#dont-burn-runs)). **Skip this entirely if you're heading - straight to a commit** — step 7's full batch supersedes it. Never run both. -6. **Reply in a few lines**: what changed, file links, anything surprising. -7. **Stop.** Commit only when asked — and when asked, go all the way to a PR - ([§1](#when-asked-to-commit-go-all-the-way-to-a-pr)). +4. **Edit, checking as you go.** Run the narrow thing for what you touched — + that one suite, or `tsc` after a type change — piped through `tail` + ([§1](#run-checks-freely-quietly)). Trivial edits need nothing. +5. **Reply in a few lines**: what changed, file links, anything surprising. +6. **Stop.** Commit only when asked — and when asked, go all the way to a PR + ([§1](#when-asked-to-commit-go-all-the-way-to-a-pr)), which starts with the + full [commit-time batch](#commit-time-checks). **Decide, don't ask.** When something's ambiguous, make the reasonable call and name it in your reply so it can be corrected. Stop only when genuinely blocked. @@ -80,7 +89,7 @@ version rather than the qBittorrent version: > renames are per-site: handling one does not handle the others. When you add a > version-dependent parameter, gate it *and* verify against the older wiki. -### Run checks freely — but quietly +### Run checks freely, quietly **The checks are fast. The output is not.** Measured on this repo: @@ -482,9 +491,12 @@ Thin objects over `apiClient`. - `useSearchJob.ts` — search job lifecycle: start/stop/delete, 2s status+results polling, unmount cleanup. -- `useTorrentActions.ts` — builds the per-torrent action menu used by both list - and detail. Delete exposes `deleteConfirmVisible` for a caller-mounted - `ConfirmModal`. +- `useTorrentActions.ts` — builds the per-torrent action menu for the **list + screen only**. Delete exposes `deleteConfirmVisible` for a caller-mounted + `ConfirmModal`. The **detail screen does not use this hook** — it hand-rolls + its own parallel `handlePauseResume`/`handleDelete`/etc. and its own + `deleteConfirmVisible` state. A new action (or a change to an existing one) + needs both places touched, or list and detail silently diverge. - `useReactiveReconnect.ts` — feeds query errors into ServerContext reconnect (`isReconnectableError`). - `useGracefulError.ts` — suppresses a transient error until it has persisted @@ -571,10 +583,15 @@ branch on it. Remember a misspelled param is dropped silently, not rejected — see [§1](#1-working-agreement). **Add a torrent action** -API method (above) → menu item in `hooks/useTorrentActions.ts` → strings in the -`actions` / `toast` namespaces. For a destructive confirm, expose visibility -state from the hook and mount `ConfirmModal` in the screen (see the torrents list -and detail screens). +API method (above), then **both** screens — they don't share this logic +([see the hook's note](#hooks-hooks)): +- List → menu item in `hooks/useTorrentActions.ts`. For a destructive confirm, + expose visibility state from the hook and mount `ConfirmModal` in the screen. +- Detail (`torrent/[hash].tsx`) → its own `handle*` function and, for a + destructive confirm, its own `*ConfirmVisible` state + `ConfirmModal`, + following its existing `handleDelete`/`deleteConfirmVisible` pair. + +Strings go in the `actions` / `toast` namespaces either way. **Add a settings sub-screen** Create `app/(tabs)/settings/.tsx` by copying a sibling's structure — the @@ -718,3 +735,27 @@ rather than a translation gap. - **Verify with `npx tsc --noEmit` and `npm test`** instead, batched at commit time per [§1](#1-working-agreement). The bar is exit 0, tests passing, lint 0 errors. + +--- + +## 10. Gotchas + +Surprises that don't have a natural home in a specific recipe, rule, or File +Index entry above — things that cost real time because nothing here flagged +them, usually because they look exactly like a product bug until you dig in. + +**Append here, don't just fix and move on.** If you burn more than a few +minutes on something that turned out to be a tooling quirk, an environment +default, or a misleading error rather than an actual bug, add a short entry +(2-4 lines: what it looks like, what's actually happening, the fix or +workaround) so the next session doesn't pay the same cost. If the surprise +belongs to one specific file, function, or recipe instead, put it there +instead of here — this section is for things that don't fit anywhere else. +Keep entries factual and current; if you find one that's no longer true +(fixed upstream, no longer applies), remove it rather than leaving it to rot. + +- **A synchronous `act(() => …)` wrapping a promise-returning call in an RN + test can silently swallow the resulting state update** — no warning, the + component just never re-renders, and it looks exactly like a product bug. + Full detail and the working pattern live in [§6](#6-task-recipes)'s "Add a + test" recipe, under the `rn`-project traps.