Conversation
- Introduced `TimetableCourseType` enum to represent course statuses (normal, cancelled, madeUp). - Created `TimetableCourse` class with required fields including slot, lessonId, courseName, roomName, and type. - Added `TimetableSlot` enum to define time slots with a method to retrieve the slot number. - Established `Timetable` class to encapsulate timetable data with a date field.
- Updated import paths for room response and schedule response files. - Created `RoomResponse` and `RoomScheduleResponse` classes with JSON serialization support using Freezed for better data handling in the Firebase API.
- Updated the `Timetable` class to require a list of `TimetableCourse` objects in addition to the existing date field for better representation of timetable data.
- Introduced `TimetableCourseResponse` class with JSON serialization support using Freezed. - Added fields for lesson ID, title, resource IDs, and optional cancellation and supervision flags to enhance timetable data handling.
- Implemented `TimetableAPI` class to handle timetable data retrieval and filtering. - Added methods to get date ranges, filter personal timetables, and load lesson schedules. - Integrated JSON file reading for timetable and cancellation data to enhance functionality. - Improved handling of personal timetable lists and lesson schedules for better user experience.
- Introduced `TimetableRepository` interface and its implementation `TimetableRepositoryImpl` for fetching timetable data. - Implemented method to retrieve timetables, including room name mapping and course conversion from API responses. - Enhanced error handling with debug logging for better troubleshooting.
…or improved readability.
… and consistency. Updated button disabled states and added new accent color variations in the color scheme.
…prove maintainability.
…e new design system. Update button and text colors for improved consistency and accessibility.
- Introduced a new data model for CancelLecture with additional fields. - Removed outdated controllers and services related to course cancellation. - Implemented a new CourseCancellationService to handle fetching and filtering of course cancellations. - Updated CourseCancellationScreen and related view models to utilize the new service and data model. - Enhanced the user interface for better interaction with course cancellation data.
- Introduced WeekPeriodRecord model to represent week period data. - Updated CourseDB to include methods for fetching week period records. - Refactored timetable services and screens to utilize WeekPeriodRecord instead of raw Map data. - Enhanced type safety and readability in timetable-related functionalities.
… 'View' to 'Screen' and clarify the use of ConsumerStatefulWidget in Dotto. Adjusted descriptions related to state management and ViewModel responsibilities accordingly.
- Introduced SearchCourseDomainError enum to manage selection limit errors. - Refactored SearchCourseScreen to use ConsumerStatefulWidget for improved state management. - Implemented SearchCourseService for handling user preferences and course selection logic. - Updated SearchCourseViewModel to integrate new service and manage state effectively. - Enhanced UI components to reflect changes in course selection and error handling. - Removed outdated SearchCourseUsecase and related controllers for cleaner architecture.
Generated by 🚫 Danger |
There was a problem hiding this comment.
Pull request overview
This PR refactors the timetable feature by introducing a layered architecture with domain models, a new repository layer, data access objects, and MVVM pattern components (Service, ViewModel, ViewState, Screen).
Changes:
- Introduced domain models (
Timetable,TimetableCourse,TimetableSlot,TimetableCourseType,Semester) to represent timetable data - Added new repository layer (
lib/repository/timetable_repository.dart) with data synchronization logic between local storage and Firestore - Separated data access into dedicated modules (
timetable_api.dartfor Firestore,course_db.dartfor SQLite,timetable_preference.dartfor local preferences) - Implemented MVVM pattern for timetable-related screens with Service, ViewModel, and ViewState layers
- Migrated to Design System v2 for UI components
- Refactored multiple existing features (search course, home screen) to integrate with the new architecture
Reviewed changes
Copilot reviewed 80 out of 86 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/repository/timetable_repository.dart | New repository implementing timetable sync logic between Firestore and local storage with conflict resolution |
| lib/data/firebase/timetable_api.dart | New Firebase API for timetable CRUD operations |
| lib/data/db/course_db.dart | New database access layer for course data |
| lib/data/preference/timetable_preference.dart | New preference layer for local timetable storage |
| lib/domain/* | New domain models for timetable, courses, slots, and course types |
| lib/feature/timetable/service/* | Service layer implementations for select course, edit timetable, and course cancellation |
| lib/feature/timetable/viewmodel/* | ViewModel implementations following MVVM pattern |
| lib/feature/timetable/viewstate/* | ViewState data classes for UI state management |
| lib/feature/timetable/screen/* | Refactored screen implementations with Design System v2 |
| lib/feature/home/* | Refactored home screen with new service and ViewModel layers |
| lib/feature/search_course/* | Updated to integrate with new timetable repository |
| lib/feature/setting/repository/settings_repository.dart | Updated to use new timetable repository and handle sync conflicts |
| lib/helper/location_helper.dart | Refactored from singleton to provider-based pattern |
Comments suppressed due to low confidence (1)
lib/helper/location_helper.dart:14
- The comment "TODO: Refactor" provides no context about what needs to be refactored. Since this was just converted from a singleton pattern to a provider-based pattern, consider either completing the refactoring or documenting what specific aspects still need improvement.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool _isSameDate(String dateString, DateTime target) { | ||
| final date = DateTime.parse(dateString); | ||
| return date.month == target.month && date.day == target.day; |
There was a problem hiding this comment.
The _isSameDate function only compares month and day, but doesn't check the year. This could cause issues when comparing dates across different years, for example, a cancellation from December 2025 could incorrectly match January 2026.
| final class TimetableAPI { | ||
| static final FirebaseFirestore _db = FirebaseFirestore.instance; | ||
| static const String _collection = 'user_taking_course'; | ||
| static const String _yearKey = '2025'; |
There was a problem hiding this comment.
The hardcoded year key '2025' in the TimetableAPI will need to be updated every year. Consider making this dynamic based on the current academic year or making it configurable to avoid requiring code changes each year.
| static const String _yearKey = '2025'; | |
| static String get _yearKey { | |
| final now = DateTime.now(); | |
| final academicYear = now.month >= 4 ? now.year : now.year - 1; | |
| return academicYear.toString(); | |
| } |
| // TODO: ドメインモデルを作成 | ||
| // TODO: TimetableRepositoryに移行 | ||
| Future<List<WeekPeriodRecord>> getWeekPeriodAllRecords() async { | ||
| return CourseDB.getWeekPeriodAllRecords(); | ||
| } | ||
|
|
||
| // TODO: ドメインモデルを作成 | ||
| // TODO: TimetableRepositoryに移行 | ||
| Future<List<int>> getPersonalLessonIdList() async { | ||
| return TimetablePreference.getPersonalTimetableList(); | ||
| } |
There was a problem hiding this comment.
The TODO comments indicate that getWeekPeriodAllRecords and getPersonalLessonIdList should be moved to TimetableRepository with domain models, but they're currently calling data layer directly. This creates inconsistency in the architecture. Consider completing the migration or creating a tracking issue for these TODOs.
| if (diff > 300000) { | ||
| final firestoreSet = firestoreList.toSet(); | ||
| final localSet = localList.toSet(); | ||
|
|
||
| // 同じIDセットかチェック | ||
| if (!firestoreSet.containsAll(localSet) || | ||
| !localSet.containsAll(firestoreSet)) { | ||
| // 競合検出 | ||
| // 元の処理ではここでAlertDialogを表示して、 | ||
| // 「アカウント側に多い科目」「ローカル側に多い科目」を表示し、 | ||
| // ユーザーに「アカウント方を残す」「ローカル方を残す」を選択させていた。 | ||
| // この選択UIはUI層で実装する必要がある。 | ||
| return TimetableConflictDetected( | ||
| firestoreList: firestoreList, | ||
| localList: localList, | ||
| firestoreOnlyIds: firestoreSet.difference(localSet).toList(), | ||
| localOnlyIds: localSet.difference(firestoreSet).toList(), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // 競合なし、Firestoreのデータを採用 | ||
| await TimetablePreference.savePersonalTimetableList(firestoreList); | ||
| return TimetableSynced(firestoreList); | ||
| } | ||
|
|
||
| @override | ||
| Future<List<int>> loadPersonalTimetableList() async { | ||
| final user = _currentUser; | ||
|
|
||
| if (user == null) { | ||
| return TimetablePreference.getPersonalTimetableList(); | ||
| } | ||
|
|
||
| final firestoreData = await TimetableAPI.getUserTimetableData(user.uid); | ||
|
|
||
| if (firestoreData == null) { | ||
| // Firestoreにデータがない場合、ローカルデータをアップロード | ||
| final localList = await TimetablePreference.getPersonalTimetableList(); | ||
| await TimetableAPI.saveUserTimetable(user.uid, localList); | ||
| await TimetablePreference.savePersonalTimetableList(localList); | ||
| return localList; | ||
| } | ||
|
|
||
| final firestoreList = firestoreData.lessonIds; | ||
| final localList = await TimetablePreference.getPersonalTimetableList(); | ||
| final localLastUpdated = | ||
| await TimetablePreference.getLastUpdateTimestamp(); | ||
| final firestoreLastUpdated = | ||
| firestoreData.lastUpdated.millisecondsSinceEpoch; | ||
| final diff = localLastUpdated - firestoreLastUpdated; | ||
|
|
||
| // ローカルが空の場合、Firestoreのデータを採用 | ||
| if (localList.isEmpty) { | ||
| await TimetablePreference.savePersonalTimetableList(firestoreList); | ||
| return firestoreList; | ||
| } | ||
|
|
||
| // Firestoreが空、またはローカルが10分以上新しい場合、ローカルデータをアップロード | ||
| if (firestoreList.isEmpty || diff > 600000) { |
There was a problem hiding this comment.
The sync logic uses a 5-minute threshold (300000ms) in loadPersonalTimetableListOnLogin but a 10-minute threshold (600000ms) in loadPersonalTimetableList. This inconsistency could lead to confusing behavior where the same data is handled differently depending on which method is called. Consider using a consistent threshold or documenting why different thresholds are appropriate for different scenarios.
| final removeLessonIdList = <int>{}; | ||
| var flag = false; | ||
|
|
||
| for (final record in targetWeekPeriod) { | ||
| final term = record.semester; | ||
| final week = record.week; | ||
| final period = record.period; | ||
|
|
||
| final selectedLessonList = weekPeriodAllRecords.where((record) { | ||
| return record.week == week && | ||
| record.period == period && | ||
| (record.semester == term || record.semester == 0) && | ||
| personalLessonIdList.contains(record.lessonId); | ||
| }).toList(); | ||
|
|
||
| if (selectedLessonList.length > 1) { | ||
| final removeLessonList = selectedLessonList.sublist( | ||
| 2, | ||
| selectedLessonList.length, | ||
| ); | ||
| if (removeLessonList.isNotEmpty) { | ||
| removeLessonIdList.addAll( | ||
| removeLessonList.map((e) => e.lessonId).toSet(), | ||
| ); | ||
| } | ||
| flag = true; | ||
| } | ||
| } | ||
|
|
||
| return flag; |
There was a problem hiding this comment.
The isOverSelected method in SearchCourseService builds a removeLessonIdList set (lines 67, 88-90) but never uses it to actually remove the lessons. This differs from the similar method in SelectCourseService (lib/feature/timetable/service/select_course_service.dart:85-90) which does save the updated list. Either the removal logic is missing here, or the variable removeLessonIdList should be removed if it's not needed.
| ).removeCurrentSnackBar(); | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('3科目以上選択することはできません'), |
There was a problem hiding this comment.
The error message says "3科目以上選択することはできません" (cannot select 3 or more courses), but the logic in isOverSelected checks if selectedLessonList.length > 1, which means it triggers when there are 2 or more courses. The sublist starts at index 2, which would only be reached if there are already 3 or more courses. However, the error message is inconsistent with the old message "１つのコマに３つ以上選択できません" which says "cannot select 3 or more in one slot". Please clarify the intended behavior and ensure consistency.
| try { | ||
| for (final date in dates) { | ||
| twoWeekLessonSchedule[date] = await _dailyLessonSchedule(date); | ||
| } | ||
| return twoWeekLessonSchedule; | ||
| } on Exception { | ||
| return twoWeekLessonSchedule; | ||
| } |
There was a problem hiding this comment.
The method handles exceptions by catching generic Exception and returning an empty timetable map. This silently swallows errors, making debugging difficult. Consider logging the error with more context or rethrowing it to allow proper error handling at a higher level.
| Future<bool> isOverSelected(int lessonId) async { | ||
| final weekPeriodAllRecords = await CourseDB.getWeekPeriodAllRecords(); | ||
| final personalLessonIdList = await getPersonalLessonIdList(); | ||
|
|
||
| final filterWeekPeriod = weekPeriodAllRecords | ||
| .where((element) => element.lessonId == lessonId) | ||
| .toList(); | ||
| final targetWeekPeriod = filterWeekPeriod | ||
| .where((element) => element.semester != 0) | ||
| .toList(); | ||
|
|
||
| // 開講時期が0（通年）の場合は前期・後期両方に展開 | ||
| for (final element in filterWeekPeriod.where( | ||
| (element) => element.semester == 0, | ||
| )) { | ||
| final e1 = element.copyWith(semester: 10); | ||
| final e2 = element.copyWith(semester: 20); | ||
| targetWeekPeriod.addAll([e1, e2]); | ||
| } | ||
|
|
||
| final removeLessonIdList = <int>{}; | ||
| var flag = false; | ||
|
|
||
| for (final record in targetWeekPeriod) { | ||
| final term = record.semester; | ||
| final week = record.week; | ||
| final period = record.period; | ||
|
|
||
| final selectedLessonList = weekPeriodAllRecords.where((record) { | ||
| return record.week == week && | ||
| record.period == period && | ||
| (record.semester == term || record.semester == 0) && | ||
| personalLessonIdList.contains(record.lessonId); | ||
| }).toList(); | ||
|
|
||
| if (selectedLessonList.length > 1) { | ||
| final removeLessonList = selectedLessonList.sublist( | ||
| 2, | ||
| selectedLessonList.length, | ||
| ); | ||
| if (removeLessonList.isNotEmpty) { | ||
| removeLessonIdList.addAll( | ||
| removeLessonList.map((e) => e.lessonId).toSet(), | ||
| ); | ||
| } | ||
| flag = true; | ||
| } | ||
| } | ||
|
|
||
| return flag; | ||
| } |
There was a problem hiding this comment.
The SearchCourseService's isOverSelected method performs side effects (removes lessons from the list) while appearing to be a query method. This violates the principle of command-query separation. The method name suggests it only checks if something is over-selected, but it also modifies state by removing excess lessons. Consider separating the check and the removal into distinct methods, or rename the method to reflect that it performs cleanup.
| Future<void> _processSupplementaryLectures( | ||
| DateTime selectTime, | ||
| Map<int, Map<int, TimetableCourse>> periodData, | ||
| Map<String, int> lessonIdMap, | ||
| ) async { | ||
| final supLectureData = await TimetableJSON.fetchSupLectures(); | ||
|
|
||
| for (final supLecture in supLectureData) { | ||
| if (!_isSameDate(supLecture.date, selectTime)) { | ||
| continue; | ||
| } | ||
|
|
||
| final lessonName = supLecture.lessonName; | ||
| if (!lessonIdMap.containsKey(lessonName)) { | ||
| continue; | ||
| } | ||
|
|
||
| final lessonId = lessonIdMap[lessonName]!; | ||
| final existingCourse = periodData[supLecture.period]?[lessonId]; | ||
|
|
||
| if (existingCourse != null) { | ||
| periodData[supLecture.period]![lessonId] = existingCourse.copyWith( | ||
| type: TimetableCourseType.madeUp, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The method processes supplementary lectures but doesn't handle the case where a supplementary lecture exists without a corresponding normal course in the same period. In this case, existingCourse would be null and the copyWith would fail. Consider handling supplementary lectures that occur in periods where the course normally doesn't have a class.
| return Row( | ||
| children: [ | ||
| DottoButton( | ||
| onPressed: onCourseCancellationPressed, | ||
| type: DottoButtonType.text, | ||
| child: const Text('休講・補講'), | ||
| ), | ||
| const Spacer(), | ||
| DottoButton( | ||
| onPressed: onEditTimetablePressed, | ||
| type: DottoButtonType.text, | ||
| child: const Text('時間割を編集'), | ||
| ), | ||
| ], | ||
| ); |
There was a problem hiding this comment.
The conversion logic removes the padding wrapper that was in the original component. The original TimetableButtons had EdgeInsetsGeometry.symmetric(horizontal: 16) padding, but the new version doesn't. This could affect the layout when integrated into the home screen. Verify that this change is intentional and doesn't break the UI layout.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 87 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return UserTimetableData( | ||
| lessonIds: lessonIds != null ? List<int>.from(lessonIds) : [], | ||
| lastUpdated: | ||
| timestamp?.toDate() ?? DateTime.fromMillisecondsSinceEpoch(0), | ||
| ); |
There was a problem hiding this comment.
Inconsistent null-safety handling. The method getUserTimetableData can return null when the document doesn't exist, but timestamp?.toDate() at line 41 provides a fallback to epoch (DateTime.fromMillisecondsSinceEpoch(0)). However, this creates an incorrect last_updated time of 1970-01-01 instead of treating it as "never updated". Consider using DateTime.now() or making lastUpdated nullable to properly represent missing data.
| Future<bool> isOverSelected(int lessonId) async { | ||
| final weekPeriodAllRecords = await CourseDB.getWeekPeriodAllRecords(); | ||
| final personalLessonIdList = await getPersonalLessonIdList(); | ||
|
|
||
| final filterWeekPeriod = weekPeriodAllRecords | ||
| .where((element) => element.lessonId == lessonId) | ||
| .toList(); | ||
| final targetWeekPeriod = filterWeekPeriod | ||
| .where((element) => element.semester != 0) | ||
| .toList(); | ||
|
|
||
| // 開講時期が0（通年）の場合は前期・後期両方に展開 | ||
| for (final element in filterWeekPeriod.where( | ||
| (element) => element.semester == 0, | ||
| )) { | ||
| final e1 = element.copyWith(semester: 10); | ||
| final e2 = element.copyWith(semester: 20); | ||
| targetWeekPeriod.addAll([e1, e2]); | ||
| } | ||
|
|
||
| final removeLessonIdList = <int>{}; | ||
| var flag = false; | ||
|
|
||
| for (final record in targetWeekPeriod) { | ||
| final term = record.semester; | ||
| final week = record.week; | ||
| final period = record.period; | ||
|
|
||
| final selectedLessonList = weekPeriodAllRecords.where((record) { | ||
| return record.week == week && | ||
| record.period == period && | ||
| (record.semester == term || record.semester == 0) && | ||
| personalLessonIdList.contains(record.lessonId); | ||
| }).toList(); | ||
|
|
||
| if (selectedLessonList.length > 1) { | ||
| final removeLessonList = selectedLessonList.sublist( | ||
| 2, | ||
| selectedLessonList.length, | ||
| ); | ||
| if (removeLessonList.isNotEmpty) { | ||
| removeLessonIdList.addAll( | ||
| removeLessonList.map((e) => e.lessonId).toSet(), | ||
| ); | ||
| } | ||
| flag = true; | ||
| } | ||
| } | ||
|
|
||
| return flag; | ||
| } |
There was a problem hiding this comment.
The isOverSelected method duplicates similar logic found in SelectCourseService.isOverSelected. Both methods perform the same conflict checking with semester expansion (semester 0 → 10 and 20). Consider extracting this common logic into a shared utility or service method to maintain consistency and reduce code duplication.
| // 競合検出 | ||
| // 元の処理ではここでAlertDialogを表示して、 | ||
| // 「アカウント側に多い科目」「ローカル側に多い科目」を表示し、 | ||
| // ユーザーに「アカウント方を残す」「ローカル方を残す」を選択させていた。 |
There was a problem hiding this comment.
Typo in documentation comment: 'アカウント方' and 'ローカル方' should be 'アカウント側' and 'ローカル側' respectively for correct Japanese grammar.
| // 競合検出 | ||
| // 元の処理ではここでAlertDialogを表示して、 | ||
| // 「アカウント側に多い科目」「ローカル側に多い科目」を表示し、 | ||
| // ユーザーに「アカウント方を残す」「ローカル方を残す」を選択させていた。 |
There was a problem hiding this comment.
Typo in documentation comment: 'ローカル方' should be 'ローカル側' for correct Japanese grammar.
| void onTimetablePeriodStyleChanged(TimetablePeriodStyle style) { | ||
| unawaited(_service.setTimetablePeriodStyle(style)); | ||
| state = state.copyWith(timetablePeriodStyle: AsyncValue.data(style)); | ||
| } |
There was a problem hiding this comment.
Missing error handling for unawaited async operations. The onTimetablePeriodStyleChanged method uses unawaited() to call _service.setTimetablePeriodStyle(style), but if this operation fails, the error will be silently ignored while the UI state is already updated. This could lead to inconsistent state between the UI and persisted preferences.
| bool _isSameDate(String dateString, DateTime target) { | ||
| final date = DateTime.parse(dateString); | ||
| return date.month == target.month && date.day == target.day; | ||
| } |
There was a problem hiding this comment.
The date comparison logic in _isSameDate only checks month and day, ignoring the year. This could cause incorrect matches when comparing dates across different years. For example, a cancellation from 2024-12-25 would incorrectly match with 2025-12-25.
| Future<bool> isOverSelected(int lessonId) async { | ||
| final weekPeriodAllRecords = await CourseDB.getWeekPeriodAllRecords(); | ||
| final personalLessonIdList = await getPersonalLessonIdList(); | ||
|
|
||
| final filterWeekPeriod = weekPeriodAllRecords | ||
| .where((element) => element.lessonId == lessonId) | ||
| .toList(); | ||
| final targetWeekPeriod = filterWeekPeriod | ||
| .where((element) => element.semester != 0) | ||
| .toList(); | ||
|
|
||
| for (final element in filterWeekPeriod.where( | ||
| (element) => element.semester == 0, | ||
| )) { | ||
| final e1 = element.copyWith(semester: 10); | ||
| final e2 = element.copyWith(semester: 20); | ||
| targetWeekPeriod.addAll([e1, e2]); | ||
| } | ||
|
|
||
| final removeLessonIdList = <int>{}; | ||
| var flag = false; | ||
|
|
||
| for (final record in targetWeekPeriod) { | ||
| final selectedLessonList = weekPeriodAllRecords.where((r) { | ||
| return r.week == record.week && | ||
| r.period == record.period && | ||
| (r.semester == record.semester || r.semester == 0) && | ||
| personalLessonIdList.contains(r.lessonId); | ||
| }).toList(); | ||
|
|
||
| if (selectedLessonList.length > 1) { | ||
| final removeLessonList = selectedLessonList.sublist( | ||
| 2, | ||
| selectedLessonList.length, | ||
| ); | ||
| if (removeLessonList.isNotEmpty) { | ||
| removeLessonIdList.addAll( | ||
| removeLessonList.map((e) => e.lessonId).toSet(), | ||
| ); | ||
| } | ||
| flag = true; | ||
| } | ||
| } | ||
|
|
||
| if (removeLessonIdList.isNotEmpty) { | ||
| final updatedList = personalLessonIdList | ||
| .where((id) => !removeLessonIdList.contains(id)) | ||
| .toList(); | ||
| await _savePersonalLessonIdList(updatedList); | ||
| } | ||
|
|
||
| return flag; |
There was a problem hiding this comment.
The method isOverSelected has a side effect that modifies the personal lesson ID list by removing courses when conflicts are detected. This behavior is not clear from the method name (which suggests it only checks/queries) and could lead to unexpected data loss. Consider separating the validation logic from the mutation logic, or rename the method to clarify its behavior (e.g., checkAndResolveOverSelection).
| const SnackBar( | ||
| content: Text('3科目以上選択することはできません'), | ||
| ), |
There was a problem hiding this comment.
The error message text changed from '１つのコマに３つ以上選択できません' to '3科目以上選択することはできません'. While this improves clarity, the actual validation logic checks if (selectedLessonList.length > 1) which allows up to 2 courses, not 3. The error message should say '3科目以上' (3 or more) is accurate only if the limit is 2 courses per slot. Please verify this is the intended behavior and message.
| Future<void> onAppear() async { | ||
| _service.startBusPolling(); | ||
| await Future.wait([ | ||
| _refresh(), | ||
| _service.changeDirectionOnCurrentLocation(), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
The HomeViewModel's build method initializes selectedDate but then onAppear immediately refreshes the state without updating the date. However, in TimetableCalendarView at line 76, there's a shadowing bug where a new selectedDate is created. This compounded issue means the date selection feature may not work correctly. Verify that date selection properly updates the HomeViewModel's state.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 87 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (1)
docs/onboarding/codebase/02_Architecture.md:48
- ドキュメントとコードの不一致：アーキテクチャドキュメントでは「UseCase」という用語が使用されていますが、実際のコードでは「Service」（例：
SelectCourseService,EditTimetableService,CourseCancellationService）が使用されています。
用語の一貫性を保つため、ドキュメントをコードに合わせて「Service」に更新するか、コードをドキュメントに合わせて「UseCase」に変更することを推奨します。
### UseCase
ViewModel と Repository の橋渡しを行います。
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final List<Map<String, dynamic>> records = await database.rawQuery( | ||
| 'SELECT 授業名 FROM sort WHERE LessonId in (${lessonIdList.join(",")})', |
There was a problem hiding this comment.
SQLインジェクションの脆弱性があります。lessonIdList.join(",")を使用してクエリを構築しているため、悪意のある入力があった場合に問題が発生する可能性があります。
パラメータ化されたクエリを使用してください。
| /// 日付が同じかどうかを判定 | ||
| bool _isSameDate(String dateString, DateTime target) { | ||
| final date = DateTime.parse(dateString); | ||
| return date.month == target.month && date.day == target.day; |
There was a problem hiding this comment.
日付比較のロジックに潜在的なバグがあります。年が異なる場合に正しく動作しません。例えば、12月31日と1月1日を比較した場合、年をチェックしていないため、誤って「同じ日付」と判定される可能性があります。
年も含めて比較するように修正してください。
| } | ||
|
|
||
| // Firestoreが空、またはローカルが10分以上新しい場合、ローカルデータをアップロード | ||
| if (firestoreList.isEmpty || diff > 600000) { |
There was a problem hiding this comment.
マジックナンバー（600000）が使用されています。10分を表すタイムスタンプ差分として使われていますが、定数として定義することで可読性が向上します。
例：const _localNewerThresholdMs = 600000; // 10 minutes
| final now = DateTime.now(); | ||
| final today = DateTime(now.year, now.month, now.day); | ||
| final monday = today.subtract(Duration(days: today.weekday - 1)); | ||
| final selectedDate = DateTime(now.year, now.month, now.day); |
There was a problem hiding this comment.
selectedDate変数が定義されていますが使用されていません。代わりにthis.selectedDateが使用されています。未使用の変数は削除してください。
| Future<void> changeDirectionOnCurrentLocation() async { | ||
| final locationHelper = ref.read(locationHelperProvider); | ||
| final position = await locationHelper.determinePosition(); | ||
| if (position != null) { | ||
| final latitude = position.latitude; | ||
| if (latitude > 41.838770 && latitude < 41.845295) { | ||
| final longitude = position.longitude; | ||
| if (longitude > 140.765061 && longitude < 140.770368) { | ||
| ref.read(busIsToProvider.notifier).toggle(); | ||
| } | ||
| } |
There was a problem hiding this comment.
セキュリティ上の懸念：ハードコードされた緯度経度の範囲で位置情報をチェックしていますが、この範囲が正しいかどうか検証できません。また、この位置情報チェックの目的がコメントで説明されていないため、意図が不明確です。
位置情報の使用目的と、この座標範囲が何を表しているのかをコメントで明記してください。
| final dates = List.generate( | ||
| 5, | ||
| (index) => monday.add(Duration(days: index)), |
There was a problem hiding this comment.
ハードコードされた日数（5日）が使用されています。月曜から金曜までの平日を表していると思われますが、定数として定義するか、DayOfWeek.weekdays.lengthを使用することで意図がより明確になります。
| final List<Map<String, dynamic>> records = await database.rawQuery( | ||
| 'SELECT LessonId, 授業名 FROM sort WHERE LessonId in (${lessonIdList.join(",")})', |
There was a problem hiding this comment.
SQLインジェクションの脆弱性があります。lessonIdList.join(",")を使用してクエリを構築しているため、悪意のある入力があった場合に問題が発生する可能性があります。
パラメータ化されたクエリを使用してください。例：WHERE LessonId IN (${lessonIdList.map((_) => '?').join(',')})と引数としてlessonIdListを渡す。
| } catch (e) { | ||
| debugPrint(e.toString()); | ||
| rethrow; | ||
| } |
There was a problem hiding this comment.
エラー処理が不十分です。_getTimetables()で例外が発生した場合、rethrowで再スローしていますが、呼び出し元で適切にハンドリングされていません。空のリストを返すなど、より明示的なエラーハンドリングを検討してください。
| final e1 = element.copyWith(semester: 10); | ||
| final e2 = element.copyWith(semester: 20); | ||
| targetWeekPeriod.addAll([e1, e2]); |
There was a problem hiding this comment.
マジックナンバー（10, 20）が使用されています。これらは学期（前期・後期）を表す値のようですが、Semester enumのnumberプロパティ（10: 前期、20: 後期）を使用しているため、コメントまたは定数で明示的に説明することを推奨します。
| 'SELECT * FROM week_period order by lessonId', | ||
| ); | ||
| return records | ||
| .where((record) => | ||
| record['week'] == week && | ||
| record['period'] == period && | ||
| (record['開講時期'] == semester || record['開講時期'] == 0)) | ||
| .map(WeekPeriodRecord.fromMap) | ||
| .toList(); |
There was a problem hiding this comment.
パフォーマンスの懸念：getAvailableCoursesメソッドでは、全レコードを取得してからフィルタリングしています。データベースレベルでWHERE句を使用してフィルタリングする方が効率的です。
SQLクエリを修正して、WHERE句で条件を指定することを推奨します。
| 'SELECT * FROM week_period order by lessonId', | |
| ); | |
| return records | |
| .where((record) => | |
| record['week'] == week && | |
| record['period'] == period && | |
| (record['開講時期'] == semester || record['開講時期'] == 0)) | |
| .map(WeekPeriodRecord.fromMap) | |
| .toList(); | |
| 'SELECT * FROM week_period WHERE week = ? AND period = ? AND (開講時期 = ? OR 開講時期 = 0) ORDER BY lessonId', | |
| [week, period, semester], | |
| ); | |
| return records.map(WeekPeriodRecord.fromMap).toList(); |
やったこと
lib/feature/timetable/repository/timetable_repository.dartの処理を分離（コピー）して、HomeからこのRepositoryに依存しないように変更lib/api/firebase/timetable_api.dartを追加lib/api/db/course_db.dartを追加lib/repository/timetable_repository.dartを追加確認したこと
UI 差分
メモ