Skip to content

feat/callrepo-for-crudmethods#40

Closed
DevamPatel22 wants to merge 1 commit intomainfrom
feat/callrepo-for-crudmethods
Closed

feat/callrepo-for-crudmethods#40
DevamPatel22 wants to merge 1 commit intomainfrom
feat/callrepo-for-crudmethods

Conversation

@DevamPatel22
Copy link
Collaborator

No description provided.

@Pairadux
Copy link
Owner

Haven't reviewed the files yet, but I do have a note before starting. Please try to use conventional commit messages going forward. Use this reference document as necessary, ask questions if you need to: Conventional Commits Website. The same goes for PR titles.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove the changes made to this file, this falls outside of your scope.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove the changes made to this file, this falls outside of your scope.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove the changes made to this file, this falls outside of your scope.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove this file so we don't have merge conflicts later.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove the changes made to this file, this falls outside of your scope. This was already implemented by Germaine.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove this file to avoid merge conflicts later.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove this file to avoid merge conflicts later.

@Pairadux Pairadux added the enhancement New feature or request label Feb 12, 2026
@Pairadux Pairadux added this to the Sprint 2 milestone Feb 12, 2026
@Pairadux
Copy link
Owner

Also, please ref issues that your commits are tackling, or if the entire PR is to close an issue, ref that in the header please.

@Pairadux
Copy link
Owner

I have just reviewed all of the code, here are few high-level things that need to be addressed before this can be merged:

  1. You need to be using the modern Riverpod APIs. Most of the providers I saw use StateNotifier / StateNotifierProvider from flutter_riverpod/legacy.dart, which is the deprecated version of the riverpod package. Use Notifier / AsyncNotifier instead. You have this setup like this in deck_list_provider.dart, so use that as a reference to fix the rest of them.
  2. Use the existing application services. You may need to pull main for this, but in study_session_provider.dart you re-implement the card scheduling from scratch with hard-coded intervals. We already have FsrsService and StudySessionService in lib/features/study/application/ that handle this properly, plus a StudySession domain model in lib/features/study/domain/. The provider should wrap those, not replace them.
  3. Repository classes don't belong in presentation/Providers/. Interfaces, implementations, and seed data are data-layer concerns. They should live in data/, but I am taking care of most of that in my scope. The Providers/ directory should only have Riverpod provider definitions. Also, directory should be lowercase providers/ per dart convention.
  4. Please remove the out-of-scope files that I mentioned above.

Future<void> deleteCard(String cardId) async {
final index = _cards.indexWhere((c) => c.cardID == cardId);
if (index != -1) {
_cards[index].isDeleted = true;
Copy link
Owner

Choose a reason for hiding this comment

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

This will be a compile error. Flashcard fields are final. Use copyWith() instead.

Future<void> deleteDeck(String deckId) async {
final index = _decks.indexWhere((d) => d.deckID == deckId);
if (index != -1) {
_decks[index] = _decks[index].copyWith(isDeleted: true, deckName: '');
Copy link
Owner

Choose a reason for hiding this comment

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

Why is deckName being cleared on delete? Soft deleting should only mark it as isDeleted which you are doing, it shouldn't destroy data.

};
}

Flashcard _applyBasicScheduling(Flashcard card, Rating rating) {
Copy link
Owner

Choose a reason for hiding this comment

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

This reimplements scheduling that already exists in FsrsService. Use the existing service instead of hardcoding intervals.

@DevamPatel22 DevamPatel22 force-pushed the feat/callrepo-for-crudmethods branch from 2c39c9e to 65430dc Compare February 17, 2026 00:09
@DevamPatel22 DevamPatel22 deleted the feat/callrepo-for-crudmethods branch February 17, 2026 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants