feat: Add download list in the example app - #3
Conversation
|
@coderabbitai review |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant DownloadExample
participant TPStreams
User->>App: Tap "Show Downloads"
App->>DownloadExample: Render component (onBack callback)
DownloadExample->>TPStreams: Fetch downloads on mount
TPStreams-->>DownloadExample: Return downloads list
DownloadExample->>User: Display downloads list
User->>DownloadExample: Tap Pause/Resume/Delete on a download
DownloadExample->>TPStreams: Call pause/resume/cancel API
TPStreams-->>DownloadExample: Return updated status
DownloadExample->>TPStreams: Refresh downloads list
TPStreams-->>DownloadExample: Return updated downloads
DownloadExample->>User: Update UI
User->>DownloadExample: Tap Back
DownloadExample->>App: Call onBack()
App->>App: Hide DownloadExample, show main UI
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
example/DownloadExample.tsx (3)
37-59: Avoid full refresh after every action
handlePauseResumeandhandleCancelre-invokeloadDownloads, triggering a network/bridge round-trip and UI flicker for every small action.
Optimistically update the affected list item locally, then fall back toloadDownloadson failure, e.g.:setDownloads((prev) => prev.map((d) => d.videoId === item.videoId ? { ...d, state: nextState } : d, ), );This keeps the UI snappy and saves bridge calls.
126-128: UseActivityIndicatorinstead of plain text for loading
<Text>Loading...</Text>gives no visual feedback on iOS dark mode and feels static.
Replace it with the platform-nativeActivityIndicator.- {loading ? ( - <Text style={styles.loading}>Loading...</Text> + {loading ? ( + <ActivityIndicator size="large" style={styles.loading} />A11y-friendly and instantly recognisable by users.
92-110: Add accessibility labels to interactive elementsThe
TouchableOpacitybuttons currently missaccessibilityLabel/accessibilityRole.
Screen-reader users will only hear “button”. Provide meaningful labels:<TouchableOpacity style={styles.button} onPress={() => handlePauseResume(item)} + accessibilityRole="button" + accessibilityLabel={isPaused ? "Resume download" : "Pause download"} >Same for the “Delete” and “Refresh” buttons.
example/src/App.tsx (1)
185-195: Consider using a navigator instead of manual boolean togglingAs the example app grows, controlling screens with booleans will quickly become unmanageable and breaks back-gesture handling on Android/iOS.
Integrating a lightweight navigator (@react-navigation/native-stack) provides:• proper history stack
• platform back-button support
• cleaner component structureThe change can be incremental: wrap the current views in two simple stack screens (
Player,Downloads) and swapsetShowDownloads(true)withnavigation.navigate('Downloads').
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
example/DownloadExample.tsx(1 hunks)example/src/App.tsx(3 hunks)
🔇 Additional comments (1)
example/src/App.tsx (1)
104-107: Great use of early return to integrate the new screenThe conditional early return keeps the main component readable and isolates the download UI.
Nice, concise solution. 👍
| const handlePauseResume = async (item: DownloadItem) => { | ||
| try { | ||
| if (item.state === 1) { | ||
| // DOWNLOADING | ||
| await TPStreamsDownload.pauseDownload(item.videoId); | ||
| } else if (item.state === 2) { | ||
| // PAUSED | ||
| await TPStreamsDownload.resumeDownload(item.videoId); | ||
| } | ||
| loadDownloads(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace magic numbers with typed enum / constants
Using raw state codes (1, 2, etc.) lowers readability and is brittle if the SDK ever changes.
Expose descriptive constants once and reuse them:
// ideally exported by SDK, otherwise create locally
enum DownloadState {
Idle = 0,
Downloading = 1,
Paused = 2,
Completed = 3,
Failed = 4,
}
if (item.state === DownloadState.Downloading) {
await TPStreamsDownload.pauseDownload(item.videoId);
} else if (item.state === DownloadState.Paused) {
await TPStreamsDownload.resumeDownload(item.videoId);
}This also simplifies getStateText.
🤖 Prompt for AI Agents
In example/DownloadExample.tsx around lines 37 to 46, replace the raw numeric
state codes (like 1 and 2) with a typed enum or constants to improve readability
and maintainability. Define a DownloadState enum with descriptive names for each
state (Idle, Downloading, Paused, Completed, Failed) either by importing from
the SDK if available or creating it locally. Then update the conditional checks
to use DownloadState.Downloading and DownloadState.Paused instead of the magic
numbers. Also update any related code like getStateText to use the enum values.
| const loadDownloads = async () => { | ||
| try { | ||
| setLoading(true); | ||
| const downloadItems = await TPStreamsDownload.getDownloads(); | ||
| setDownloads(downloadItems); | ||
| } catch (error) { | ||
| Alert.alert('Error', 'Failed to load downloads'); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Guard against state updates after unmount
loadDownloads is an async function that sets state after awaiting a promise.
If the component unmounts before the promise resolves (e.g., user quickly navigates away), the setDownloads/setLoading calls will try to update an unmounted component, triggering a memory-leak warning.
Add a simple isMounted ref or an AbortController to ignore the result once unmounted.
+ const isMounted = useRef(true);
+ useEffect(() => {
+ return () => {
+ isMounted.current = false;
+ };
+ }, []);
const loadDownloads = async () => {
try {
setLoading(true);
const downloadItems = await TPStreamsDownload.getDownloads();
- setDownloads(downloadItems);
+ if (isMounted.current) setDownloads(downloadItems);
} catch (error) {
Alert.alert('Error', 'Failed to load downloads');
} finally {
- setLoading(false);
+ if (isMounted.current) setLoading(false);
}
};📝 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 loadDownloads = async () => { | |
| try { | |
| setLoading(true); | |
| const downloadItems = await TPStreamsDownload.getDownloads(); | |
| setDownloads(downloadItems); | |
| } catch (error) { | |
| Alert.alert('Error', 'Failed to load downloads'); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| const isMounted = useRef(true); | |
| useEffect(() => { | |
| return () => { | |
| isMounted.current = false; | |
| }; | |
| }, []); | |
| const loadDownloads = async () => { | |
| try { | |
| setLoading(true); | |
| const downloadItems = await TPStreamsDownload.getDownloads(); | |
| if (isMounted.current) setDownloads(downloadItems); | |
| } catch (error) { | |
| Alert.alert('Error', 'Failed to load downloads'); | |
| } finally { | |
| if (isMounted.current) setLoading(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In example/DownloadExample.tsx around lines 25 to 35, the async function
loadDownloads updates state after awaiting a promise, which can cause
memory-leak warnings if the component unmounts before the promise resolves. To
fix this, add an isMounted ref or use an AbortController to track the
component's mounted status, and check this status before calling setDownloads
and setLoading to prevent state updates on an unmounted component.
c76c9fc to
f1c392e
Compare
Summary by CodeRabbit