chore: apply review fixes and typed error hardening
This commit is contained in:
parent
bf969d0ae6
commit
4335d3b7a2
12 changed files with 163 additions and 66 deletions
2
.gitignore
vendored
2
.gitignore
vendored
|
|
@ -55,3 +55,5 @@ app.*.map.json
|
|||
# Local testing artifacts
|
||||
/flutter_*.png
|
||||
/devtools_options.yaml
|
||||
|
||||
FINDINGS.md
|
||||
|
|
@ -256,6 +256,9 @@ class _AuthGateState extends State<AuthGate> {
|
|||
|
||||
Future<void> _ensureDefaultCollectionIfNeeded() async {
|
||||
final userId = _session?.user.id;
|
||||
// Intentional policy: attempt bootstrap once per user per app session.
|
||||
// This avoids repeated retries/toasts when backend issues are transient.
|
||||
// Users can recover on next session or via a manual retry entry point.
|
||||
if (userId == null || userId == _lastEnsuredUserId) return;
|
||||
|
||||
_lastEnsuredUserId = userId;
|
||||
|
|
|
|||
|
|
@ -95,7 +95,7 @@ class _ScannerScreenState extends State<ScannerScreen>
|
|||
|
||||
final cameras = await availableCameras();
|
||||
if (cameras.isEmpty) {
|
||||
throw Exception('No camera available');
|
||||
throw const ValidationException('No camera available');
|
||||
}
|
||||
|
||||
final backCamera = cameras.firstWhere(
|
||||
|
|
|
|||
|
|
@ -86,7 +86,9 @@ class CollectionsScreenState extends State<CollectionsScreen>
|
|||
}
|
||||
final userId = supabase.auth.currentUser?.id;
|
||||
if (userId == null) {
|
||||
throw Exception('You must be signed in to load collections.');
|
||||
throw const AuthRequiredException(
|
||||
'You must be signed in to load collections.',
|
||||
);
|
||||
}
|
||||
|
||||
final prefs = await SharedPreferences.getInstance();
|
||||
|
|
|
|||
|
|
@ -525,7 +525,9 @@ class GarageScreenState extends State<GarageScreen> {
|
|||
} else {
|
||||
final userId = supabase.auth.currentUser?.id;
|
||||
if (userId == null) {
|
||||
throw Exception('You must be signed in to copy cars.');
|
||||
throw const AuthRequiredException(
|
||||
'You must be signed in to copy cars.',
|
||||
);
|
||||
}
|
||||
|
||||
final sourceCars = _cars
|
||||
|
|
@ -1153,7 +1155,9 @@ class GarageScreenState extends State<GarageScreen> {
|
|||
} else {
|
||||
final userId = supabase.auth.currentUser?.id;
|
||||
if (userId == null) {
|
||||
throw Exception('You must be signed in to copy cars.');
|
||||
throw const AuthRequiredException(
|
||||
'You must be signed in to copy cars.',
|
||||
);
|
||||
}
|
||||
|
||||
final existing = await supabase
|
||||
|
|
|
|||
|
|
@ -180,7 +180,7 @@ class _ManageCollectionScreenState extends State<ManageCollectionScreen> {
|
|||
const SizedBox(height: 12),
|
||||
DropdownButtonFormField<String>(
|
||||
isExpanded: true,
|
||||
value: inviteRole,
|
||||
initialValue: inviteRole,
|
||||
decoration: const InputDecoration(
|
||||
labelText: 'Role',
|
||||
prefixIcon: Icon(Icons.security_outlined),
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ import '../scanner_screen.dart';
|
|||
import '../services/collection_service.dart';
|
||||
import '../services/main_collection_sync.dart';
|
||||
import '../theme/app_colors.dart';
|
||||
import '../utils/error_utils.dart';
|
||||
import '../utils/preferences_utils.dart';
|
||||
|
||||
class ScanTab extends StatefulWidget {
|
||||
|
|
@ -52,7 +53,9 @@ class ScanTabState extends State<ScanTab> {
|
|||
final list = await CollectionService.getMyCollections();
|
||||
final userId = supabase.auth.currentUser?.id;
|
||||
if (userId == null) {
|
||||
throw Exception('You must be signed in to load collections.');
|
||||
throw const AuthRequiredException(
|
||||
'You must be signed in to load collections.',
|
||||
);
|
||||
}
|
||||
|
||||
final prefs = await SharedPreferences.getInstance();
|
||||
|
|
@ -436,7 +439,7 @@ class ScanTabState extends State<ScanTab> {
|
|||
}) async {
|
||||
final userId = supabase.auth.currentUser?.id;
|
||||
if (userId == null) {
|
||||
throw Exception('You must be signed in to add cars.');
|
||||
throw const AuthRequiredException('You must be signed in to add cars.');
|
||||
}
|
||||
|
||||
await supabase.from('hotwheels').insert({
|
||||
|
|
@ -455,7 +458,9 @@ class ScanTabState extends State<ScanTab> {
|
|||
}) async {
|
||||
final userId = supabase.auth.currentUser?.id;
|
||||
if (userId == null) {
|
||||
throw Exception('You must be signed in to create catalog entries.');
|
||||
throw const AuthRequiredException(
|
||||
'You must be signed in to create catalog entries.',
|
||||
);
|
||||
}
|
||||
|
||||
final cleanedSeries = series?.trim();
|
||||
|
|
@ -477,7 +482,9 @@ class ScanTabState extends State<ScanTab> {
|
|||
Future<void> _ensureValidationVote(String hwId) async {
|
||||
final userId = supabase.auth.currentUser?.id;
|
||||
if (userId == null) {
|
||||
throw Exception('You must be signed in to validate entries.');
|
||||
throw const AuthRequiredException(
|
||||
'You must be signed in to validate entries.',
|
||||
);
|
||||
}
|
||||
final existingVote = await supabase
|
||||
.from('car_votes')
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
//import 'package:supabase_flutter/supabase_flutter.dart';
|
||||
import 'package:flutter/foundation.dart';
|
||||
import '../main.dart';
|
||||
import '../utils/error_utils.dart';
|
||||
|
||||
/// Data model for a collection.
|
||||
class Collection {
|
||||
|
|
@ -50,14 +50,30 @@ class CollectionMember {
|
|||
bool get isViewer => role == 'viewer';
|
||||
}
|
||||
|
||||
/// Typed error for collection service operations.
|
||||
class CollectionServiceException extends AppException {
|
||||
const CollectionServiceException(super.message);
|
||||
}
|
||||
|
||||
/// Service for managing collections and membership.
|
||||
class CollectionService {
|
||||
CollectionService._();
|
||||
|
||||
static Never _fail(String message) => throw CollectionServiceException(message);
|
||||
|
||||
@visibleForTesting
|
||||
static String normalizeRole(String role) => role.trim().toLowerCase();
|
||||
|
||||
@visibleForTesting
|
||||
static bool isSupportedInviteRole(String role) {
|
||||
final normalized = normalizeRole(role);
|
||||
return normalized == 'member' || normalized == 'viewer';
|
||||
}
|
||||
|
||||
static String _requireUserId() {
|
||||
final userId = supabase.auth.currentUser?.id;
|
||||
if (userId == null) {
|
||||
throw Exception('You must be signed in to perform this action.');
|
||||
_fail('You must be signed in to perform this action.');
|
||||
}
|
||||
return userId;
|
||||
}
|
||||
|
|
@ -115,31 +131,15 @@ class CollectionService {
|
|||
final itemCounts = await getCollectionItemCounts(collectionIdList);
|
||||
|
||||
final memberCounts = <String, int>{};
|
||||
await Future.wait(collectionIdList.map((collectionId) async {
|
||||
try {
|
||||
final rows = await supabase.rpc('get_collection_members', params: {
|
||||
'p_collection_id': collectionId,
|
||||
});
|
||||
memberCounts[collectionId] = (rows as List).length;
|
||||
} catch (e) {
|
||||
debugPrint('CollectionService.getMyCollections member RPC error: $e');
|
||||
}
|
||||
}));
|
||||
|
||||
final unresolvedIds = collectionIdList
|
||||
.where((id) => !memberCounts.containsKey(id))
|
||||
.toList(growable: false);
|
||||
if (unresolvedIds.isNotEmpty) {
|
||||
final members = await supabase
|
||||
.from('collection_members')
|
||||
.select('id, collection_id')
|
||||
.inFilter('collection_id', unresolvedIds);
|
||||
.select('collection_id')
|
||||
.inFilter('collection_id', collectionIdList);
|
||||
|
||||
for (final member in members) {
|
||||
final collectionId = member['collection_id'] as String;
|
||||
memberCounts[collectionId] = (memberCounts[collectionId] ?? 0) + 1;
|
||||
}
|
||||
}
|
||||
|
||||
final collections = <Collection>[];
|
||||
|
||||
|
|
@ -362,16 +362,21 @@ class CollectionService {
|
|||
.maybeSingle();
|
||||
|
||||
if (collection == null) {
|
||||
throw Exception('Collection not found.');
|
||||
_fail('Collection not found.');
|
||||
}
|
||||
if (collection['owner_id'] != userId) {
|
||||
throw Exception('Only the collection owner can perform this action.');
|
||||
_fail('Only the collection owner can perform this action.');
|
||||
}
|
||||
|
||||
final trimmedDescription = description?.trim();
|
||||
final normalizedDescription =
|
||||
(trimmedDescription != null && trimmedDescription.isNotEmpty)
|
||||
? trimmedDescription
|
||||
: null;
|
||||
|
||||
await supabase.from('collections').update({
|
||||
'name': name,
|
||||
if (description != null && description.isNotEmpty)
|
||||
'description': description,
|
||||
'description': normalizedDescription,
|
||||
}).eq('id', collectionId);
|
||||
}
|
||||
|
||||
|
|
@ -386,10 +391,10 @@ class CollectionService {
|
|||
.maybeSingle();
|
||||
|
||||
if (collection == null) {
|
||||
throw Exception('Collection not found.');
|
||||
_fail('Collection not found.');
|
||||
}
|
||||
if (collection['owner_id'] != userId) {
|
||||
throw Exception('Only the collection owner can perform this action.');
|
||||
_fail('Only the collection owner can perform this action.');
|
||||
}
|
||||
|
||||
await supabase.from('collections').delete().eq('id', collectionId);
|
||||
|
|
@ -423,9 +428,9 @@ class CollectionService {
|
|||
required String email,
|
||||
String role = 'member',
|
||||
}) async {
|
||||
final normalizedRole = role.trim().toLowerCase();
|
||||
if (normalizedRole != 'member' && normalizedRole != 'viewer') {
|
||||
throw Exception('Unsupported role "$role".');
|
||||
final normalizedRole = normalizeRole(role);
|
||||
if (!isSupportedInviteRole(role)) {
|
||||
_fail('Unsupported role "$role".');
|
||||
}
|
||||
|
||||
final currentUserId = _requireUserId();
|
||||
|
|
@ -437,10 +442,10 @@ class CollectionService {
|
|||
.maybeSingle();
|
||||
|
||||
if (collection == null) {
|
||||
throw Exception('Collection not found.');
|
||||
_fail('Collection not found.');
|
||||
}
|
||||
if (collection['owner_id'] != currentUserId) {
|
||||
throw Exception('Only the collection owner can invite members.');
|
||||
_fail('Only the collection owner can invite members.');
|
||||
}
|
||||
|
||||
// Call an RPC to look up the user ID by email.
|
||||
|
|
@ -449,14 +454,14 @@ class CollectionService {
|
|||
});
|
||||
|
||||
if (result == null || (result is List && result.isEmpty)) {
|
||||
throw Exception(
|
||||
_fail(
|
||||
'No user found with that email. They must create an account first.');
|
||||
}
|
||||
|
||||
final userId = result is List ? result.first['id'] as String : result as String;
|
||||
|
||||
if (userId == currentUserId) {
|
||||
throw Exception('You are already in this collection.');
|
||||
_fail('You are already in this collection.');
|
||||
}
|
||||
|
||||
// Check if already a member.
|
||||
|
|
@ -468,7 +473,7 @@ class CollectionService {
|
|||
.maybeSingle();
|
||||
|
||||
if (existing != null) {
|
||||
throw Exception('This user is already a member of this collection.');
|
||||
_fail('This user is already a member of this collection.');
|
||||
}
|
||||
|
||||
try {
|
||||
|
|
@ -481,7 +486,7 @@ class CollectionService {
|
|||
final message = e.toString();
|
||||
if (normalizedRole == 'viewer' &&
|
||||
message.contains('collection_members_role_check')) {
|
||||
throw Exception(
|
||||
_fail(
|
||||
'Viewer role is currently unavailable. Please contact the app administrator.',
|
||||
);
|
||||
}
|
||||
|
|
@ -503,14 +508,14 @@ class CollectionService {
|
|||
.maybeSingle();
|
||||
|
||||
if (collection == null) {
|
||||
throw Exception('Collection not found.');
|
||||
_fail('Collection not found.');
|
||||
}
|
||||
if (collection['owner_id'] != currentUserId) {
|
||||
throw Exception('Only the collection owner can remove members.');
|
||||
_fail('Only the collection owner can remove members.');
|
||||
}
|
||||
|
||||
if (memberUserId == collection['owner_id']) {
|
||||
throw Exception('Collection owner cannot be removed.');
|
||||
_fail('Collection owner cannot be removed.');
|
||||
}
|
||||
|
||||
final membersBefore = await getMembers(collectionId);
|
||||
|
|
@ -518,11 +523,11 @@ class CollectionService {
|
|||
membersBefore.where((member) => member.userId == memberUserId);
|
||||
|
||||
if (targetMembership.isEmpty) {
|
||||
throw Exception('Member not found in this collection.');
|
||||
_fail('Member not found in this collection.');
|
||||
}
|
||||
final target = targetMembership.first;
|
||||
if (target.role == 'owner') {
|
||||
throw Exception('Collection owner cannot be removed.');
|
||||
_fail('Collection owner cannot be removed.');
|
||||
}
|
||||
|
||||
var deleted = false;
|
||||
|
|
@ -552,14 +557,14 @@ class CollectionService {
|
|||
}
|
||||
|
||||
if (!deleted) {
|
||||
throw Exception('Member removal failed.');
|
||||
_fail('Member removal failed.');
|
||||
}
|
||||
|
||||
final membersAfter = await getMembers(collectionId);
|
||||
final stillExists =
|
||||
membersAfter.any((member) => member.userId == memberUserId);
|
||||
if (stillExists) {
|
||||
throw Exception(
|
||||
_fail(
|
||||
'Member could not be removed. Please try again in a moment.',
|
||||
);
|
||||
}
|
||||
|
|
@ -577,10 +582,10 @@ class CollectionService {
|
|||
.maybeSingle();
|
||||
|
||||
if (membership == null) {
|
||||
throw Exception('You are not a member of this collection.');
|
||||
_fail('You are not a member of this collection.');
|
||||
}
|
||||
if (membership['role'] == 'owner') {
|
||||
throw Exception('Owner cannot leave. Delete the collection instead.');
|
||||
_fail('Owner cannot leave. Delete the collection instead.');
|
||||
}
|
||||
|
||||
await supabase
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@ import 'package:flutter/foundation.dart';
|
|||
import 'package:image/image.dart' as img;
|
||||
import 'package:supabase_flutter/supabase_flutter.dart';
|
||||
import '../main.dart';
|
||||
import '../utils/error_utils.dart';
|
||||
|
||||
/// Handles uploading / deleting car images in Supabase Storage.
|
||||
///
|
||||
|
|
@ -29,7 +30,9 @@ class StorageService {
|
|||
}) async {
|
||||
final user = supabase.auth.currentUser;
|
||||
if (user == null) {
|
||||
throw Exception('You must be signed in to upload images.');
|
||||
throw const AuthRequiredException(
|
||||
'You must be signed in to upload images.',
|
||||
);
|
||||
}
|
||||
final userId = user.id;
|
||||
final path = '$userId/$entryId.jpg';
|
||||
|
|
@ -121,7 +124,7 @@ class StorageService {
|
|||
static Uint8List _compressImageBytes(Uint8List bytes) {
|
||||
final decoded = img.decodeImage(bytes);
|
||||
if (decoded == null) {
|
||||
throw Exception('Invalid image file.');
|
||||
throw const ValidationException('Invalid image file.');
|
||||
}
|
||||
|
||||
img.Image working = decoded.width > _maxWidth
|
||||
|
|
@ -153,7 +156,7 @@ class StorageService {
|
|||
}
|
||||
|
||||
if (out.lengthInBytes > _maxImageBytes) {
|
||||
throw Exception(
|
||||
throw ValidationException(
|
||||
'Image is too large after compression (${out.lengthInBytes} bytes).',
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,10 +1,40 @@
|
|||
import 'package:flutter/foundation.dart';
|
||||
import 'package:supabase_flutter/supabase_flutter.dart';
|
||||
|
||||
/// Base app-level exception for expected, user-facing failures.
|
||||
class AppException implements Exception {
|
||||
final String message;
|
||||
|
||||
const AppException(this.message);
|
||||
|
||||
@override
|
||||
String toString() => message;
|
||||
}
|
||||
|
||||
class AuthRequiredException extends AppException {
|
||||
const AuthRequiredException(super.message);
|
||||
}
|
||||
|
||||
class PermissionDeniedException extends AppException {
|
||||
const PermissionDeniedException(super.message);
|
||||
}
|
||||
|
||||
class NotFoundException extends AppException {
|
||||
const NotFoundException(super.message);
|
||||
}
|
||||
|
||||
class ValidationException extends AppException {
|
||||
const ValidationException(super.message);
|
||||
}
|
||||
|
||||
String userMessageForError(
|
||||
Object error, {
|
||||
String fallback = 'Something went wrong. Please try again.',
|
||||
}) {
|
||||
if (error is AppException) {
|
||||
return error.message;
|
||||
}
|
||||
|
||||
if (error is AuthException) {
|
||||
return error.message;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -588,10 +588,10 @@ packages:
|
|||
dependency: transitive
|
||||
description:
|
||||
name: matcher
|
||||
sha256: "12956d0ad8390bbcc63ca2e1469c0619946ccb52809807067a7020d57e647aa6"
|
||||
sha256: dc0b7dc7651697ea4ff3e69ef44b0407ea32c487a39fff6a4004fa585e901861
|
||||
url: "https://pub.dev"
|
||||
source: hosted
|
||||
version: "0.12.18"
|
||||
version: "0.12.19"
|
||||
material_color_utilities:
|
||||
dependency: transitive
|
||||
description:
|
||||
|
|
@ -1025,10 +1025,10 @@ packages:
|
|||
dependency: transitive
|
||||
description:
|
||||
name: test_api
|
||||
sha256: "93167629bfc610f71560ab9312acdda4959de4df6fac7492c89ff0d3886f6636"
|
||||
sha256: "8161c84903fd860b26bfdefb7963b3f0b68fee7adea0f59ef805ecca346f0c7a"
|
||||
url: "https://pub.dev"
|
||||
source: hosted
|
||||
version: "0.7.9"
|
||||
version: "0.7.10"
|
||||
typed_data:
|
||||
dependency: transitive
|
||||
description:
|
||||
|
|
|
|||
41
test/services/collection_service_test.dart
Normal file
41
test/services/collection_service_test.dart
Normal file
|
|
@ -0,0 +1,41 @@
|
|||
import 'package:flutter_test/flutter_test.dart';
|
||||
import 'package:hwhub/services/collection_service.dart';
|
||||
|
||||
void main() {
|
||||
group('Collection role permissions', () {
|
||||
Collection collectionWithRole(String role) {
|
||||
return Collection(
|
||||
id: 'c1',
|
||||
name: 'Garage',
|
||||
ownerId: 'u1',
|
||||
createdAt: DateTime(2026, 1, 1),
|
||||
role: role,
|
||||
);
|
||||
}
|
||||
|
||||
test('owner and member can modify cars, viewer cannot', () {
|
||||
expect(collectionWithRole('owner').canModifyCars, isTrue);
|
||||
expect(collectionWithRole('member').canModifyCars, isTrue);
|
||||
expect(collectionWithRole('viewer').canModifyCars, isFalse);
|
||||
});
|
||||
|
||||
test('viewer role flag is detected correctly', () {
|
||||
expect(collectionWithRole('viewer').isViewer, isTrue);
|
||||
expect(collectionWithRole('member').isViewer, isFalse);
|
||||
});
|
||||
});
|
||||
|
||||
group('CollectionService invite role validation', () {
|
||||
test('normalizes role input', () {
|
||||
expect(CollectionService.normalizeRole(' Viewer '), 'viewer');
|
||||
expect(CollectionService.normalizeRole('MEMBER'), 'member');
|
||||
});
|
||||
|
||||
test('allows only member and viewer roles', () {
|
||||
expect(CollectionService.isSupportedInviteRole('member'), isTrue);
|
||||
expect(CollectionService.isSupportedInviteRole('viewer'), isTrue);
|
||||
expect(CollectionService.isSupportedInviteRole(' owner '), isFalse);
|
||||
expect(CollectionService.isSupportedInviteRole('admin'), isFalse);
|
||||
});
|
||||
});
|
||||
}
|
||||
Loading…
Reference in a new issue