Skip to content

Conversation

@lowrt
Copy link
Contributor

@lowrt lowrt commented Jan 10, 2026

這是什麼類型的 PR?

選擇所有符合的項目

  • 重構
  • 新功能
  • 錯誤修復
  • 最佳化
  • 技術文件更新

描述

更換套件,以減少Ios 大小

相關 issue

  • 相關問題 #
  • closes #

QA 指南、截圖、錄像

請將這行替換成:如何測試您的 PR 的步驟,已測試的裝置註釋,以及任何相關的 UI 更改圖片。

UI 無障礙清單

如果您的 PR 包含 UI 更改,請使用此清單:

  • 變數名稱實現語意化命名?
  • 測試通過 AA 顏色對比?

@claude
Copy link

claude bot commented Jan 10, 2026

Claude finished @lowrt's task in 2m 13s —— View job


PR Review: Package Optimization ✅

I've completed my review of this PR which aims to reduce app size by replacing packages. Here's my comprehensive feedback:


Summary of Changes

Packages Replaced:

  • ❌ Removed: flutter_icmp_ping (custom fork)
  • ❌ Removed: zstandard (^1.3.29)
  • ✅ Added: es_compression (^2.0.0)
  • ✅ Added: Custom TCP ping implementation (lib/utils/ping.dart)

Additional Changes:

  • Added native zstd libraries (~2.3MB) for Android across 3 architectures
  • Updated iOS AppDelegate for custom image saving
  • Modified analysis_options.yaml to ignore public_member_api_docs warnings
  • Updated compression handling in global.dart and exptech.dart

Code Quality Assessment

Strengths

  1. Clean Implementation: The new tcpPing function in lib/utils/ping.dart is simple and well-structured
  2. Proper Error Handling: Uses try-catch with null returns for failures
  3. Native Integration: iOS AppDelegate changes properly implement platform channels for image saving
  4. Compression Consistency: Good use of es_compression for both gzip and zstd decompression

⚠️ Areas of Concern

1. Analysis Options Change (Minor Issue)

Location: analysis_options.yaml:6

public_member_api_docs: ignore

Issue: This silences documentation warnings, which conflicts with the CLAUDE.md requirement that "All documentation must follow Effective Dart style guidelines."

Recommendation: Instead of ignoring public_member_api_docs globally:

  • Add documentation to public members that need it
  • OR exclude specific files that don't need docs
  • This ensures compliance with project documentation standards

2. App Size Concern (Major - Needs Clarification)

Issue: The PR aims to "reduce app size," but adds ~2.3MB of native libraries.

Analysis:

  • Added: android/app/src/main/jniLibs/ (~2.3MB total)
    • arm64-v8a: 612K
    • armeabi-v7a: 940K
    • x86_64: 740K
  • Removed: flutter_icmp_ping + zstandard dependencies

Question for Author:

  • What was the baseline app size before this change?
  • What is the app size after these changes?
  • Did the removal of flutter_icmp_ping and zstandard offset the addition of native libraries?

The net effect on app size is unclear without APK size comparison.

3. TCP Ping vs ICMP Ping (Functional Change)

Location: lib/utils/ping.dart

Issue: Replacing ICMP ping with TCP ping changes the behavior:

  • ICMP Ping: Tests network reachability at IP layer
  • TCP Ping: Tests port connectivity (port 443 in this case)

Impact:

  • More restrictive - only tests HTTPS port availability
  • May give different results in networks that block certain ports
  • Could affect network diagnostics accuracy in lib/core/update.dart

Recommendation: This is acceptable if the goal is specifically to test HTTPS connectivity, but should be documented.

4. iOS Image Saving Implementation (Good)

Location: ios/Runner/AppDelegate.swift, lib/route/image_viewer/image_viewer.dart

Positive: Simplified the iOS photo saving logic by:

  • Removing complex permission handling
  • Using native platform channel
  • Cleaner error handling

This is a good improvement.

5. Minor: Debug Print Statements

Location: lib/core/update.dart:33,73

print('Network info update failed: $e');
print('Network check failed: $e');

Issue: Should use TalkerManager.instance.error() for consistency with the rest of the codebase (see lib/api/exptech.dart for examples).


Performance Considerations

Positive:

  • es_compression is likely more efficient than the old zstandard package
  • TCP ping timeout (3s) is reasonable
  • Compression handling looks optimized

Concern:

  • Native libraries add to install size and may slightly increase initial load time

Security Assessment

No Major Security Concerns:

  • TCP ping implementation is safe (doesn't expose sensitive data)
  • Native library additions appear legitimate (zstd compression)
  • iOS platform channel properly scoped

Minor:

  • Ensure es_compression package is from a trusted source and actively maintained

Flutter Best Practices

Good:

  • ✅ Proper use of platform channels for native functionality
  • ✅ Async/await pattern used correctly
  • ✅ Error handling with try-catch
  • ✅ Proper null safety

Could Improve:

  • ⚠️ Replace print() statements with proper logging (TalkerManager)
  • ⚠️ Add documentation to public APIs instead of ignoring warnings

Testing Recommendations

Since there's no test suite in this project:

  1. Manual Testing Required:

    • Verify network checks work correctly on both Android/iOS
    • Test image saving on iOS with various permission states
    • Confirm compression/decompression works for all data sources
    • Check app size before/after on both platforms
  2. Network Testing:

    • Test with different network conditions (WiFi, mobile, restricted networks)
    • Verify TCP ping correctly identifies connectivity issues

Final Recommendation

Conditional Approval

This PR has good code quality overall, but needs clarification:

  1. Must Address:

    • Provide APK size comparison (before/after) to validate the "reduce app size" claim
    • Consider reverting analysis_options.yaml change and adding proper docs instead
  2. Should Consider:

    • Replace print() with TalkerManager.instance.error()
    • Document the behavioral change from ICMP to TCP ping
    • Verify es_compression package maintenance status
  3. Nice to Have:

    • Add inline documentation explaining the switch to TCP ping
    • Consider adding comment in ping.dart about why TCP is used instead of ICMP

The code changes are well-implemented, but the primary goal (app size reduction) needs validation before merging.


Note: CI builds are currently running. I'll update this review if any build issues are detected.

@lowrt lowrt added this to the v3.1.5 milestone Jan 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants