Skip to content

Conversation

neelkanth-kaushik
Copy link
Contributor

No description provided.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the directory selection strategy in the store implementation by replacing the use of getApplicationDocumentsDirectory() with getApplicationSupportDirectory() for better alignment with platform conventions.

  • Switched from documents directory to application support directory for data storage

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Future<Directory> _getDocumentDir() async {
try {
return await getApplicationDocumentsDirectory();
return await getApplicationSupportDirectory();
Copy link
Preview

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change modifies where application data is stored, which could be a breaking change for existing users. Consider implementing a migration strategy to move existing data from the documents directory to the application support directory, or provide a configuration option to maintain backward compatibility.

Suggested change
return await getApplicationSupportDirectory();
final newDir = await getApplicationSupportDirectory();
final oldDir = await getApplicationDocumentsDirectory();
// Migrate old files if they exist
final oldFiles = oldDir
.listSync()
.whereType<File>()
.where((file) => file.path.contains('analytics-flutter-') && file.path.endsWith('.json'));
for (final oldFile in oldFiles) {
final fileName = oldFile.uri.pathSegments.last;
final newFile = File('${newDir.path}/$fileName');
if (!await newFile.exists()) {
await oldFile.copy(newFile.path);
await oldFile.delete();
}
}
return newDir;

Copilot uses AI. Check for mistakes.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

.toList();

for (final oldFile in oldDirFiles) {
final fileName = oldFile.path.split('/').last;
Copy link
Preview

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using string splitting with hardcoded '/' is fragile across platforms. Use path.basename(oldFile.path) from the path package instead, which handles path separators correctly on all platforms.

Copilot uses AI. Check for mistakes.

Comment on lines +144 to +150
} catch (e) {
// The app should continue to work even if migration fails
}
}
}
} catch (e) {
// Migration failure shouldn't break the app
Copy link
Preview

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The catch block silently ignores all exceptions during file operations. Consider logging the error for debugging purposes while still allowing the app to continue.

Suggested change
} catch (e) {
// The app should continue to work even if migration fails
}
}
}
} catch (e) {
// Migration failure shouldn't break the app
} catch (e, stackTrace) {
// The app should continue to work even if migration fails
print('File migration error: \$e');
print(stackTrace);
}
}
}
} catch (e, stackTrace) {
// Migration failure shouldn't break the app
print('Migration failure: \$e');
print(stackTrace);

Copilot uses AI. Check for mistakes.

Comment on lines +145 to +150
// The app should continue to work even if migration fails
}
}
}
} catch (e) {
// Migration failure shouldn't break the app
Copy link
Preview

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The catch block silently ignores all exceptions during migration. Consider logging the error for debugging purposes while still allowing the app to continue.

Suggested change
// The app should continue to work even if migration fails
}
}
}
} catch (e) {
// Migration failure shouldn't break the app
// The app should continue to work even if migration fails
print('Error migrating file ${oldFile.path} to $newFilePath: $e');
}
}
}
} catch (e) {
// Migration failure shouldn't break the app
print('Error during migration from Documents to Support directory: $e');

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant