fix: address code review issues for 1.0 release #33

leoafarias · 2026-01-29T17:49:05Z

Summary

This PR addresses 8 SHOULD_FIX issues identified during code review, focusing on resource management, security improvements, and documentation updates for the 1.0 release.

Resource Management Fixes

DeckBuilder disposal: Fixed resource leak in _runBuild - builder is now properly disposed in finally block when owned by the caller
Watch mode optimization: Manual rebuilds now reuse the watch builder instead of spawning new browser instances
MermaidGenerator timeout: Added 30-second timeout to dispose() to prevent indefinite hangs during browser initialization
Parallel disposal: Changed DeckBuilder.dispose() to use Future.wait() for parallel task disposal

Security Improvements

UriValidator traversal detection: Now handles backslash separators (\) and percent-encoded separators (%2f, %5c) case-insensitively
Path traversal tests: Added 8 new tests for DeckConfiguration traversal validation
UriValidator tests: Added 24 new tests covering edge cases (backslash, percent-encoding, HTTP/HTTPS exemption, safe filenames)

Documentation

Added SDK version requirements: Flutter 3.35.0+, Dart 3.9.0+ in docs/getting-started.mdx and packages/cli/README.md

Testing

Added DeckBuilder lifecycle tests (6 tests)
Added MermaidGenerator lifecycle management tests (3 tests)

Test plan

All existing tests pass (1,390+ tests across packages)
New tests added for all behavioral changes
Static analysis passes (dart analyze)
Manual verification of resource disposal in watch mode

docs-page · 2026-01-29T17:49:08Z

To view this pull requests documentation preview, visit the following URL:

docs.page/btwld/superdeck~33

Documentation is deployed and generated using docs.page.

Copilot

Pull request overview

This PR addresses 8 code review issues for the 1.0 release, focusing on resource management, security hardening, and documentation updates. The changes implement proper disposal patterns for browser resources, enhance path traversal protection with comprehensive encoding detection, and document SDK version requirements.

Changes:

Implemented DeckBuilder disposal mechanism with parallel task cleanup and builder reuse in watch mode to prevent resource leaks
Enhanced UriValidator security to detect backslash separators and percent-encoded path traversal attempts (case-insensitive)
Added MermaidGenerator timeout protection (30s) during disposal to prevent indefinite hangs
Updated SDK version requirements documentation to Flutter 3.35.0+ and Dart 3.9.0+

Reviewed changes

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

Show a summary per file

File	Description
pubspec.yaml	Updated SDK constraints and upgraded mix dependency from 1.7.0 to 2.0.0-rc.0
pubspec.lock	Lockfile regenerated with mix 2.0.0-rc.0 requiring Dart 3.10.0+ and Flutter 3.38.1+
packages/superdeck/lib/src/utils/uri_validator.dart	Enhanced path traversal detection with backslash and percent-encoding support
packages/superdeck/test/utils/uri_validator_test.dart	Added 24 new security tests covering edge cases
packages/core/lib/src/deck_configuration.dart	Improved path validation to allow filenames with dots while blocking traversal
packages/core/test/src/deck_configuration_test.dart	Added 8 new tests for path traversal validation
packages/cli/lib/src/commands/build_command.dart	Modified to reuse builder in watch mode and properly dispose resources
packages/builder/lib/src/deck_builder.dart	Added dispose() method with parallel task disposal using Future.wait
packages/builder/test/src/deck_builder_test.dart	Added 6 lifecycle management tests
packages/builder/lib/src/assets/mermaid_generator.dart	Enhanced dispose() with timeout protection and state tracking
packages/builder/test/src/assets/mermaid_generator_test.dart	Added 3 lifecycle tests
docs/getting-started.mdx	Documented SDK version requirements
packages/cli/README.md	Documented SDK version requirements
demo/macos/Flutter/GeneratedPluginRegistrant.swift	Auto-generated removal of unused path_provider_foundation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

docs/getting-started.mdx

packages/cli/README.md

pubspec.yaml

- T1/T2: Align SDK constraints (>=3.9.0) and fix mix version conflict (1.x vs 2.x) - T4: Fix watch task lifecycle bug - tasks were disposed after every build, causing reuse of disposed tasks in watch mode. Added DeckBuilder.dispose() - T10: Fix path validation false positives - now checks for '..' as path segment, not substring (allows '..config.png', rejects '../secret') - T12: Fix browser initialization race condition - concurrent calls to _getBrowser() now share initialization future instead of launching multiple browsers - Add code review fixes plan to .planning/ https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

Mark completed tasks (T1, T2, T4, T10, T12) and document remaining work. https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

- Remove mix/mix_generator from root pubspec (mix_generator has no 2.x version; melos.yaml bootstrap handles dependency overrides for packages) - Fix uri_validator path traversal check to use raw string splitting instead of parsed URI segments, since Uri.parse() resolves '..' before we can detect it (e.g., file:///../../../etc/passwd -> file:///etc/passwd) - Also catches URL-encoded traversal (%2e%2e) All tests pass: 1380 passed, 11 skipped, 0 failures Analyze: no issues across all 5 packages https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

Keep mix: ^2.0.0-rc.0 in root pubspec as intended. The earlier bootstrap failure was due to Flutter SDK not being initialized; now resolves correctly via FVM symlink. mix_generator remains removed (no 2.x version exists). https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

Resource Management: - Fix DeckBuilder not disposed in _runBuild (resource leak) - Reuse watch builder for manual rebuilds instead of spawning new instances - Add 30s timeout to MermaidGenerator dispose to prevent hangs - Add disposed state tracking to prevent usage after disposal - Change DeckBuilder.dispose() to parallel task disposal Security: - Improve UriValidator traversal detection for backslash and percent-encoded separators - Add comprehensive path traversal tests for DeckConfiguration - Add 24 new tests for UriValidator edge cases Documentation: - Add SDK version requirements (Flutter 3.35.0+, Dart 3.9.0+) Testing: - Add DeckBuilder lifecycle tests - Add MermaidGenerator lifecycle management tests

The mix 2.0.0-rc.0 dependency requires Dart >=3.10.0 and Flutter >=3.38.1, which is higher than the previously documented requirements. Updated all documentation and pubspec constraints to reflect the actual requirements. Addresses code review comments about SDK version inconsistencies.

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

AGENTS.md

docs/getting-started.mdx

packages/builder/lib/src/assets/mermaid_generator.dart

Copilot · 2026-01-29T21:43:49Z

packages/builder/test/src/deck_builder_test.dart

+final class MockTask extends Task {
+  bool wasDisposed = false;
+  final Duration? disposeDelay;
+  final bool throwOnDispose;
+
+  MockTask({this.disposeDelay, this.throwOnDispose = false})
+      : super('MockTask');
+
+  @override
+  FutureOr<void> run(SlideContext context) {
+    // No-op for testing
+  }
+
+  @override
+  FutureOr<void> dispose() async {
+    if (disposeDelay != null) {
+      await Future.delayed(disposeDelay!);
+    }
+    if (throwOnDispose) {
+      throw StateError('Simulated dispose error');
+    }
+    wasDisposed = true;
+  }
+}


Missing test coverage for error handling during disposal. The MockTask class has a throwOnDispose parameter (line 13, 28-30), but no test uses it to verify how DeckBuilder.dispose() handles exceptions thrown by tasks during disposal.

The current implementation uses Future.wait() without error handling, which means if any task throws during disposal, the entire dispose operation will fail. Consider adding a test that verifies this behavior, and potentially updating the implementation to handle partial disposal failures gracefully (e.g., using Future.wait with eagerError: false, or wrapping each task disposal in a try-catch).

Update documentation comments across modified files to use proper third-person verbs, consistent formatting, and appropriate structure.

- Update all package pubspec.yaml files to require Dart >=3.10.0 - Update superdeck package to require Flutter >=3.38.1 - Simplify timeout handling in MermaidGenerator.dispose()

leoafarias requested a review from Copilot January 29, 2026 20:15

Copilot started reviewing on behalf of leoafarias January 29, 2026 20:15 View session

Copilot AI reviewed Jan 29, 2026

View reviewed changes

docs/getting-started.mdx Outdated Show resolved Hide resolved

packages/cli/README.md Outdated Show resolved Hide resolved

pubspec.yaml Outdated Show resolved Hide resolved

leoafarias added 6 commits January 29, 2026 15:22

docs: update code review plan with implementation progress

d1987bb

Mark completed tasks (T1, T2, T4, T10, T12) and document remaining work. https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

chore: remove code review plan file

5fc4f25

https://claude.ai/code/session_017yfkWSkRfgTpmtpafCRkRq

leoafarias force-pushed the leoafarias/fix/code-review branch from 7f5afdb to 58c3a3d Compare January 29, 2026 20:23

leoafarias requested a review from Copilot January 29, 2026 21:34

Copilot started reviewing on behalf of leoafarias January 29, 2026 21:34 View session

Copilot AI reviewed Jan 29, 2026

View reviewed changes

leoafarias added 2 commits January 29, 2026 17:23

docs: improve doc comments to follow Effective Dart guidelines

0f65b4f

Update documentation comments across modified files to use proper third-person verbs, consistent formatting, and appropriate structure.

fix: update SDK version requirements to match mix dependency

77ad101

- Update all package pubspec.yaml files to require Dart >=3.10.0 - Update superdeck package to require Flutter >=3.38.1 - Simplify timeout handling in MermaidGenerator.dispose()

leoafarias merged commit 6661f98 into main Jan 30, 2026
2 checks passed

leoafarias deleted the leoafarias/fix/code-review branch January 30, 2026 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: address code review issues for 1.0 release #33

fix: address code review issues for 1.0 release #33

Uh oh!

leoafarias commented Jan 29, 2026

Uh oh!

docs-page bot commented Jan 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fix: address code review issues for 1.0 release #33

fix: address code review issues for 1.0 release #33

Uh oh!

Conversation

leoafarias commented Jan 29, 2026

Summary

Resource Management Fixes

Security Improvements

Documentation

Testing

Test plan

Uh oh!

docs-page bot commented Jan 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants