Create chapter screen fix - #806
Conversation
|
🎉 Welcome @mahmoudr80!
We appreciate your contribution! 🚀 |
📝 WalkthroughWalkthroughThis pull request fixes a memory leak in the create chapter screen by adding proper disposal of text controllers and improves null safety in file-picking methods by eliminating force-unwraps in favor of null checks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/views/screens/create_chapter_screen.dart`:
- Around line 59-62: The async file-picker handlers pickAudioFile() and
pickLyricsFile() call setState() after awaiting
FilePicker.platform.pickFiles(...) and can crash if the widget is disposed while
the picker is open; after each await result (i.e., immediately after the
FilePicker.platform.pickFiles(...) returns), add a guard "if (!mounted) return;"
before calling setState(), then proceed to setState(() => audioFile =
File(path)) and setState(() => lyricsFile = File(path)) respectively to avoid
setState() called after dispose() errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af7c2db0-242d-4947-bb33-df46ac5dcc57
📒 Files selected for processing (1)
lib/views/screens/create_chapter_screen.dart
| final path = result.files.single.path; | ||
| if (path != null) { | ||
| setState(() => audioFile = File(path)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/views/screens/create_chapter_screen.dart | head -150Repository: AOSSIE-Org/Resonate
Length of output: 5429
Add mounted check after async file picker operations to prevent post-dispose crashes.
Both pickAudioFile() and pickLyricsFile() call setState() after await FilePicker.platform.pickFiles(...) without checking the mounted property. If the screen is closed while the picker is open, the setState() call will throw setState() called after dispose() when the picker returns.
Add if (!mounted) return; immediately after the await in both methods:
Suggested fix
Future<void> pickAudioFile() async {
FilePickerResult? result = await FilePicker.platform.pickFiles(
type: FileType.custom,
allowedExtensions: [
'wav',
'aiff',
'alac',
'flac',
'mp3',
'aac',
'wma',
'ogg',
],
);
+ if (!mounted) return;
if (result != null) {
final path = result.files.single.path;
if (path != null) {
setState(() => audioFile = File(path));
}
}
}
Future<void> pickLyricsFile() async {
FilePickerResult? result = await FilePicker.platform.pickFiles(
type: FileType.custom,
allowedExtensions: ['txt'],
);
+ if (!mounted) return;
if (result != null) {
- if(result.files.single.path!=null){
+ if (result.files.single.path != null) {
setState(() {
lyricsFile = File(result.files.single.path!);
});
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/views/screens/create_chapter_screen.dart` around lines 59 - 62, The async
file-picker handlers pickAudioFile() and pickLyricsFile() call setState() after
awaiting FilePicker.platform.pickFiles(...) and can crash if the widget is
disposed while the picker is open; after each await result (i.e., immediately
after the FilePicker.platform.pickFiles(...) returns), add a guard "if
(!mounted) return;" before calling setState(), then proceed to setState(() =>
audioFile = File(path)) and setState(() => lyricsFile = File(path)) respectively
to avoid setState() called after dispose() errors.
There was a problem hiding this comment.
@mahmoudr80 please add the mounted check as code rabbit suggested
There was a problem hiding this comment.
@4555jan, sure! Here's the plan to add the mounted guard in both async file-picker methods:
- Add
if (!mounted) return;inpickAudioFile()after theawait - Add
if (!mounted) return;inpickLyricsFile()after theawait - Fix minor spacing in the
pickLyricsFile()null check
Failed to handle agent chat message. Please try again.
|
@mahmoudr80 also please open the PR to the dev branch only |
2 participants
Description
In the create chapter screen I solved leak of memory cause of controller does not disposed.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Please include screenshots below if applicable.
Checklist:
Maintainer Checklist
Summary by CodeRabbit