fix: address code review feedback

1. main.dart: Add database migration from Documents to Support directory
   - Migrates existing zapstore.db to new location on first run
   - Preserves existing users' data after directory change

2. installed_packages_snapshot.dart: Use atomic file replace
   - Remove explicit delete before rename (rename atomically replaces)
   - Add try/catch to preserve temp file on rename failure

3. polls_section.dart: Fix useEffect cleanup for dynamic controllers
   - Capture controller snapshot in useEffect
   - Properly dispose controllers added via "Add Option"

4. polls_section.dart: Fix filter to include votes at exact end time
   - Changed from isBefore to !isAfter for inclusive filtering

5. polls_utils.dart: Extract shared utility functions
   - canCreatePoll, filterValidResponses, calculateVoteCounts
   - Marked @visibleForTesting for test access

6. FEAT-001-package-manager.md: Add markdown language specifiers
   - Fix MD040 lint warnings for code blocks

7. polls_section_test.dart: Use production utils instead of duplicates
   - Import and test actual canCreatePoll from polls_utils
   - Add test for votes at exact poll end time

Signed-off-by: alltheseas <alltheseas@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
alltheseas
2026-01-26 11:31:44 -06:00
co-authored by Claude Opus 4.5
parent 380264f995
commit 958841eebc
6 changed files with 239 additions and 156 deletions
+22 -1
View File
@@ -1,5 +1,5 @@
import 'dart:async';
import 'dart:io' show Platform;
import 'dart:io' show File, Platform;
import 'package:connectivity_plus/connectivity_plus.dart';
import 'package:flutter/material.dart';
@@ -204,6 +204,27 @@ final appInitializationProvider = FutureProvider<void>((ref) async {
final dir = await getApplicationSupportDirectory();
final dbPath = path.join(dir.path, 'zapstore.db');
// Migrate database from old location (Documents) to new location (Support)
// This preserves existing users' data after the directory change
final oldDir = await getApplicationDocumentsDirectory();
final oldDbPath = path.join(oldDir.path, 'zapstore.db');
final oldDbFile = File(oldDbPath);
final newDbFile = File(dbPath);
if (await oldDbFile.exists() && !await newDbFile.exists()) {
try {
// Ensure the new directory exists
await newDbFile.parent.create(recursive: true);
// Copy the old database to the new location
await oldDbFile.copy(dbPath);
// Delete the old file after successful copy
await oldDbFile.delete();
} catch (e) {
// Log but don't fail - user can still use app with fresh database
debugPrint('Database migration failed: $e');
}
}
// Clear storage if requested from a clear all operation
await maybeClearStorage(dbPath);
@@ -36,10 +36,19 @@ class InstalledPackagesSnapshot {
'installed': list,
});
await tmp.writeAsString(payload, flush: true);
if (await file.exists()) {
await file.delete();
// Atomic replace: rename() on Linux/Android atomically replaces destination
try {
await tmp.rename(file.path);
} catch (renameError) {
// Rename failed - try to preserve tmp file for recovery
if (kDebugMode) {
debugPrint(
'[InstalledPackagesSnapshot] Atomic rename failed: $renameError');
debugPrint('[InstalledPackagesSnapshot] Temp file preserved at: ${tmp.path}');
}
// Don't delete tmp - leave it for manual recovery if needed
rethrow;
}
await tmp.rename(file.path);
} catch (e) {
// Best-effort snapshot only.
if (kDebugMode) {
+11 -34
View File
@@ -9,13 +9,7 @@ import 'package:zapstore/utils/extensions.dart';
import 'package:zapstore/widgets/auth_widgets.dart';
import 'package:zapstore/widgets/common/profile_avatar.dart';
import 'package:zapstore/widgets/common/profile_name_widget.dart';
/// Zapstore team npubs authorized to create polls on any app
const _zapstoreTeamNpubs = {
'npub10r8xl2njyepcw2zwv3a6dyufj4e4ajx86hz6v4ehu4gnpupxxp7stjt2p8',
'npub1wf4pufsucer5va8g9p0rj5dnhvfeh6d8w0g6eayaep5dhps6rsgs43dgh9',
'npub1zafcms4xya5ap9zr7xxr0jlrtrattwlesytn2s42030lzu0dwlzqpd26k5',
};
import 'package:zapstore/widgets/polls_utils.dart';
/// Polls section for App detail screen (NIP-88)
class PollsSection extends HookConsumerWidget {
@@ -26,14 +20,6 @@ class PollsSection extends HookConsumerWidget {
@override
Widget build(BuildContext context, WidgetRef ref) {
// Only show polls from app developer or zapstore team
// Convert npubs to hex for the query
final zapstoreTeamHex = _zapstoreTeamNpubs.map((npub) {
try {
return Utils.decodeShareableToString(npub);
} catch (_) {
return npub;
}
}).toSet();
final authorizedPubkeys = {app.pubkey, ...zapstoreTeamHex};
// Query polls tagged to this app from authorized authors
@@ -68,21 +54,9 @@ class PollsSection extends HookConsumerWidget {
}
/// Check if a pubkey is authorized to create polls on an app
bool _canCreatePoll(String? signedInPubkey, App app) {
if (signedInPubkey == null) return false;
// App developer can create polls on their own app
if (signedInPubkey == app.pubkey) return true;
// Zapstore team can create polls on any app
// Convert npubs to hex for comparison
final zapstoreTeamHex = _zapstoreTeamNpubs.map((npub) {
try {
return Utils.decodeShareableToString(npub);
} catch (_) {
return npub;
}
}).toSet();
return zapstoreTeamHex.contains(signedInPubkey);
}
/// Delegates to shared utility function for testability
bool _canCreatePoll(String? signedInPubkey, App app) =>
canCreatePoll(signedInPubkey, app.pubkey);
/// Layout for polls section
class _PollsSectionLayout extends ConsumerWidget {
@@ -261,8 +235,9 @@ class _PollCard extends HookConsumerWidget {
};
// Filter out votes created after poll expiry (per NIP-88)
// Votes at exactly the end time are included (!isAfter = at or before)
final validResponses = poll.endsAt != null
? allResponses.where((r) => r.createdAt.isBefore(poll.endsAt!)).toList()
? allResponses.where((r) => !r.createdAt.isAfter(poll.endsAt!)).toList()
: allResponses;
// Deduplicate: one vote per pubkey (latest valid vote wins)
@@ -767,14 +742,16 @@ class _CreatePollComposer extends HookConsumerWidget {
};
}, [optionControllers.value]);
// Clean up controllers on dispose
// Clean up controllers when list changes or widget unmounts
// Capture current controllers to dispose them properly
useEffect(() {
final controllersSnapshot = List<TextEditingController>.from(optionControllers.value);
return () {
for (final controller in optionControllers.value) {
for (final controller in controllersSnapshot) {
controller.dispose();
}
};
}, []);
}, [optionControllers.value]);
final canSubmit = questionController.text.trim().isNotEmpty &&
optionControllers.value
+84
View File
@@ -0,0 +1,84 @@
import 'package:flutter/foundation.dart' show visibleForTesting;
import 'package:models/models.dart';
/// Zapstore team npubs authorized to create polls on any app
const zapstoreTeamNpubs = {
'npub10r8xl2njyepcw2zwv3a6dyufj4e4ajx86hz6v4ehu4gnpupxxp7stjt2p8',
'npub1wf4pufsucer5va8g9p0rj5dnhvfeh6d8w0g6eayaep5dhps6rsgs43dgh9',
'npub1zafcms4xya5ap9zr7xxr0jlrtrattwlesytn2s42030lzu0dwlzqpd26k5',
};
/// Convert zapstore team npubs to hex pubkeys
@visibleForTesting
Set<String> get zapstoreTeamHex => zapstoreTeamNpubs.map((npub) {
try {
return Utils.decodeShareableToString(npub);
} catch (_) {
return npub;
}
}).toSet();
/// Check if a pubkey is authorized to create polls on an app
///
/// Returns true if:
/// - signedInPubkey matches appPubkey (app developer)
/// - signedInPubkey is a zapstore team member
@visibleForTesting
bool canCreatePoll(String? signedInPubkey, String? appPubkey) {
if (signedInPubkey == null) return false;
// App developer can create polls on their own app
if (signedInPubkey == appPubkey) return true;
// Zapstore team can create polls on any app
return zapstoreTeamHex.contains(signedInPubkey);
}
/// Filter out votes created after poll expiry (per NIP-88)
/// Votes at exactly the end time are included
@visibleForTesting
List<T> filterValidResponses<T>({
required List<T> responses,
required DateTime? pollEndsAt,
required DateTime Function(T) getCreatedAt,
}) {
if (pollEndsAt == null) return responses;
// !isAfter means "at or before" - includes votes at exact end time
return responses.where((r) => !getCreatedAt(r).isAfter(pollEndsAt)).toList();
}
/// Calculate vote counts per option (NIP-88 compliant)
///
/// For singlechoice: only count first response tag
/// For multiplechoice: count all response tags
/// Returns (voteCounts, validVoterCount) - validVoterCount excludes invalid votes
@visibleForTesting
({Map<String, int> counts, int validVoterCount}) calculateVoteCounts<T>({
required Set<String> validOptionIds,
required List<T> responses,
required bool isSingleChoice,
required String? Function(T) getFirstOptionId,
required Iterable<String> Function(T) getAllOptionIds,
}) {
final voteCounts = <String, int>{};
for (final optionId in validOptionIds) {
voteCounts[optionId] = 0;
}
int validVoterCount = 0;
for (final response in responses) {
// For singlechoice: only first option counts (per NIP-88)
final optionIds = isSingleChoice
? [getFirstOptionId(response)].whereType<String>()
: getAllOptionIds(response);
bool hasValidVote = false;
for (final optionId in optionIds) {
if (validOptionIds.contains(optionId)) {
voteCounts[optionId] = (voteCounts[optionId] ?? 0) + 1;
hasValidVote = true;
}
}
if (hasValidVote) validVoterCount++;
}
return (counts: voteCounts, validVoterCount: validVoterCount);
}
+2 -2
View File
@@ -50,7 +50,7 @@ Manages the complete lifecycle: download → verify → install, with pause/resu
Operations follow this sealed class hierarchy (`install_operation.dart`):
```
```text
DownloadQueued → Downloading ↔ DownloadPaused
↓
Verifying
@@ -87,7 +87,7 @@ These are non-negotiable. Violations mean the implementation is broken.
## Integration Boundaries
```
```text
┌─────────────────────────────────────────────────────────────┐
│ PackageManager (Dart) │
│ - State machine owner │
+108 -116
View File
@@ -1,85 +1,5 @@
import 'package:flutter_test/flutter_test.dart';
import 'package:models/models.dart';
// Zapstore team npubs (same as in polls_section.dart)
const _zapstoreTeamNpubs = {
'npub10r8xl2njyepcw2zwv3a6dyufj4e4ajx86hz6v4ehu4gnpupxxp7stjt2p8',
'npub1wf4pufsucer5va8g9p0rj5dnhvfeh6d8w0g6eayaep5dhps6rsgs43dgh9',
'npub1zafcms4xya5ap9zr7xxr0jlrtrattwlesytn2s42030lzu0dwlzqpd26k5',
};
// Convert zapstore team npubs to hex (for testing)
Set<String> get _zapstoreTeamHex => _zapstoreTeamNpubs.map((npub) {
try {
return Utils.decodeShareableToString(npub);
} catch (_) {
return npub;
}
}).toSet();
/// Check if a pubkey is authorized to create polls on an app
/// (extracted from polls_section.dart for testing)
bool canCreatePoll(String? signedInPubkey, String appPubkey) {
if (signedInPubkey == null) return false;
if (signedInPubkey == appPubkey) return true;
return _zapstoreTeamHex.contains(signedInPubkey);
}
/// Filter out votes created after poll expiry (per NIP-88)
List<MockPollResponse> filterValidResponses(
List<MockPollResponse> responses,
DateTime? pollEndsAt,
) {
if (pollEndsAt == null) return responses;
return responses.where((r) => r.createdAt.isBefore(pollEndsAt)).toList();
}
/// Deduplicate responses by pubkey (latest wins)
Map<String, MockPollResponse> deduplicateResponses(
List<MockPollResponse> responses) {
final responsesByPubkey = <String, MockPollResponse>{};
for (final response in responses) {
final existing = responsesByPubkey[response.pubkey];
if (existing == null || response.createdAt.isAfter(existing.createdAt)) {
responsesByPubkey[response.pubkey] = response;
}
}
return responsesByPubkey;
}
/// Calculate vote counts per option (NIP-88 compliant)
/// For singlechoice: only count first response tag
/// For multiplechoice: count all response tags
/// Returns (voteCounts, validVoterCount) - validVoterCount excludes invalid votes
(Map<String, int>, int) calculateVoteCountsNip88(
Set<String> validOptionIds,
List<MockPollResponse> responses,
bool isSingleChoice,
) {
final voteCounts = <String, int>{};
for (final optionId in validOptionIds) {
voteCounts[optionId] = 0;
}
int validVoterCount = 0;
for (final response in responses) {
// For singlechoice: only first option counts (per NIP-88)
final optionIds = isSingleChoice
? [response.firstSelectedOptionId].whereType<String>()
: response.selectedOptionIds;
bool hasValidVote = false;
for (final optionId in optionIds) {
if (validOptionIds.contains(optionId)) {
voteCounts[optionId] = (voteCounts[optionId] ?? 0) + 1;
hasValidVote = true;
}
}
if (hasValidVote) validVoterCount++;
}
return (voteCounts, validVoterCount);
}
import 'package:zapstore/widgets/polls_utils.dart';
/// Mock poll response for testing (uses List to preserve order per NIP-88)
class MockPollResponse {
@@ -98,8 +18,21 @@ class MockPollResponse {
selectedOptionIds.isEmpty ? null : selectedOptionIds.first;
}
/// Deduplicate responses by pubkey (latest wins)
Map<String, MockPollResponse> deduplicateResponses(
List<MockPollResponse> responses) {
final responsesByPubkey = <String, MockPollResponse>{};
for (final response in responses) {
final existing = responsesByPubkey[response.pubkey];
if (existing == null || response.createdAt.isAfter(existing.createdAt)) {
responsesByPubkey[response.pubkey] = response;
}
}
return responsesByPubkey;
}
void main() {
group('canCreatePoll', () {
group('canCreatePoll (from polls_utils)', () {
const appDevPubkey =
'abcd1234567890abcd1234567890abcd1234567890abcd1234567890abcd1234';
const randomUserPubkey =
@@ -118,12 +51,12 @@ void main() {
});
test('returns true for zapstore team member on any app', () {
final zapstoreTeamMember = _zapstoreTeamHex.first;
final zapstoreTeamMember = zapstoreTeamHex.first;
expect(canCreatePoll(zapstoreTeamMember, appDevPubkey), isTrue);
});
test('npub to hex conversion works correctly', () {
for (final hex in _zapstoreTeamHex) {
for (final hex in zapstoreTeamHex) {
expect(hex.length, equals(64));
expect(RegExp(r'^[0-9a-f]+$').hasMatch(hex), isTrue);
}
@@ -151,11 +84,39 @@ void main() {
),
];
final valid = filterValidResponses(responses, pollEndsAt);
final valid = filterValidResponses(
responses: responses,
pollEndsAt: pollEndsAt,
getCreatedAt: (r) => r.createdAt,
);
expect(valid.length, equals(2));
expect(valid.map((r) => r.pubkey).toSet(), equals({'user1', 'user3'}));
});
test('includes votes at exact poll end time (NIP-88 compliance)', () {
final pollEndsAt = DateTime(2026, 1, 26, 12, 0);
final responses = [
MockPollResponse(
pubkey: 'user1',
createdAt: DateTime(2026, 1, 26, 12, 0), // Exactly at end - valid
selectedOptionIds: ['opt0'],
),
MockPollResponse(
pubkey: 'user2',
createdAt: DateTime(2026, 1, 26, 12, 0, 0, 1), // Just after - invalid
selectedOptionIds: ['opt1'],
),
];
final valid = filterValidResponses(
responses: responses,
pollEndsAt: pollEndsAt,
getCreatedAt: (r) => r.createdAt,
);
expect(valid.length, equals(1));
expect(valid.first.pubkey, equals('user1'));
});
test('returns all responses when poll has no expiry', () {
final responses = [
MockPollResponse(
@@ -170,7 +131,11 @@ void main() {
),
];
final valid = filterValidResponses(responses, null);
final valid = filterValidResponses(
responses: responses,
pollEndsAt: null,
getCreatedAt: (r) => r.createdAt,
);
expect(valid.length, equals(2));
});
});
@@ -221,7 +186,7 @@ void main() {
});
});
group('calculateVoteCountsNip88 - single choice', () {
group('calculateVoteCounts - single choice', () {
test('only counts first response tag for singlechoice polls', () {
final validOptionIds = {'opt0', 'opt1', 'opt2'};
final responses = [
@@ -238,12 +203,17 @@ void main() {
),
];
final (counts, voterCount) =
calculateVoteCountsNip88(validOptionIds, responses, true);
expect(counts['opt0'], equals(1)); // user1's first choice
expect(counts['opt1'], equals(1)); // user2's choice
expect(counts['opt2'], equals(0)); // ignored (not first)
expect(voterCount, equals(2));
final result = calculateVoteCounts(
validOptionIds: validOptionIds,
responses: responses,
isSingleChoice: true,
getFirstOptionId: (r) => r.firstSelectedOptionId,
getAllOptionIds: (r) => r.selectedOptionIds,
);
expect(result.counts['opt0'], equals(1)); // user1's first choice
expect(result.counts['opt1'], equals(1)); // user2's choice
expect(result.counts['opt2'], equals(0)); // ignored (not first)
expect(result.validVoterCount, equals(2));
});
test('first response tag order is preserved', () {
@@ -257,14 +227,19 @@ void main() {
),
];
final (counts, _) =
calculateVoteCountsNip88(validOptionIds, responses, true);
expect(counts['opt0'], equals(0));
expect(counts['opt1'], equals(1)); // Only first tag counts
final result = calculateVoteCounts(
validOptionIds: validOptionIds,
responses: responses,
isSingleChoice: true,
getFirstOptionId: (r) => r.firstSelectedOptionId,
getAllOptionIds: (r) => r.selectedOptionIds,
);
expect(result.counts['opt0'], equals(0));
expect(result.counts['opt1'], equals(1)); // Only first tag counts
});
});
group('calculateVoteCountsNip88 - multiple choice', () {
group('calculateVoteCounts - multiple choice', () {
test('counts all response tags for multiplechoice polls', () {
final validOptionIds = {'opt0', 'opt1', 'opt2'};
final responses = [
@@ -280,16 +255,21 @@ void main() {
),
];
final (counts, voterCount) =
calculateVoteCountsNip88(validOptionIds, responses, false);
expect(counts['opt0'], equals(1));
expect(counts['opt1'], equals(2));
expect(counts['opt2'], equals(1));
expect(voterCount, equals(2));
final result = calculateVoteCounts(
validOptionIds: validOptionIds,
responses: responses,
isSingleChoice: false,
getFirstOptionId: (r) => r.firstSelectedOptionId,
getAllOptionIds: (r) => r.selectedOptionIds,
);
expect(result.counts['opt0'], equals(1));
expect(result.counts['opt1'], equals(2));
expect(result.counts['opt2'], equals(1));
expect(result.validVoterCount, equals(2));
});
});
group('calculateVoteCountsNip88 - invalid options', () {
group('calculateVoteCounts - invalid options', () {
test('ignores votes for unknown option IDs', () {
final validOptionIds = {'opt0', 'opt1'};
final responses = [
@@ -305,13 +285,18 @@ void main() {
),
];
final (counts, voterCount) =
calculateVoteCountsNip88(validOptionIds, responses, false);
expect(counts['opt0'], equals(1));
expect(counts['opt1'], equals(0));
expect(counts.containsKey('invalid_option'), isFalse);
// user2 has no valid votes, so not counted in voterCount
expect(voterCount, equals(1));
final result = calculateVoteCounts(
validOptionIds: validOptionIds,
responses: responses,
isSingleChoice: false,
getFirstOptionId: (r) => r.firstSelectedOptionId,
getAllOptionIds: (r) => r.selectedOptionIds,
);
expect(result.counts['opt0'], equals(1));
expect(result.counts['opt1'], equals(0));
expect(result.counts.containsKey('invalid_option'), isFalse);
// user2 has no valid votes, so not counted in validVoterCount
expect(result.validVoterCount, equals(1));
});
test('percentage calculation excludes invalid voters', () {
@@ -329,12 +314,19 @@ void main() {
),
];
final (counts, voterCount) =
calculateVoteCountsNip88(validOptionIds, responses, true);
final result = calculateVoteCounts(
validOptionIds: validOptionIds,
responses: responses,
isSingleChoice: true,
getFirstOptionId: (r) => r.firstSelectedOptionId,
getAllOptionIds: (r) => r.selectedOptionIds,
);
// Only 1 valid voter, so opt0 should be 100%
expect(voterCount, equals(1));
final percentage = voterCount > 0 ? (counts['opt0']! / voterCount * 100) : 0.0;
expect(result.validVoterCount, equals(1));
final percentage = result.validVoterCount > 0
? (result.counts['opt0']! / result.validVoterCount * 100)
: 0.0;
expect(percentage, equals(100.0));
});
});