Skip to content

Comments

add shortcut#494

Merged
lowrt merged 13 commits intomainfrom
feat/shortcut
Jan 2, 2026
Merged

add shortcut#494
lowrt merged 13 commits intomainfrom
feat/shortcut

Conversation

@lowrt
Copy link
Contributor

@lowrt lowrt commented Dec 31, 2025

這是什麼類型的 PR?

選擇所有符合的項目

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

描述

增加捷徑功能

相關 issue

  • 相關問題 #
  • closes #

QA 指南、截圖、錄像

Android Apple
https://github.com/user-attachments/assets/d0ed212d-f45d-4eaa-8458-55e00102f44a https://github.com/user-attachments/assets/1da544d0-c04d-42ec-a625-64bdf3417568

UI 無障礙清單

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

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

@lowrt lowrt added this to the v3.1.5 milestone Dec 31, 2025
@claude
Copy link

claude bot commented Dec 31, 2025

Claude finished @lowrt's task in 1m 59s —— View job


PR Review: Add Shortcut Feature

I've completed my review of the shortcut implementation. Here's my feedback:


Summary

This PR adds Android and iOS app shortcuts/quick actions to open the earthquake monitor (強震監視器) directly. The implementation uses platform channels to communicate shortcut launches from native code to Flutter. Overall, the implementation is solid and functional, but there are several code quality and best practices issues that should be addressed.


Critical Issues

1. Platform Channel Error Handling (lib/main.dart:179-187)

The getInitialShortcut() function catches PlatformException but uses print() instead of the proper logging system:

} on PlatformException catch (e) {
  print('Failed to get initial shortcut: $e');  // ❌ Should use talker
  return null;
}

Recommendation: Use talker.error() for consistency with the rest of the codebase:

} on PlatformException catch (e, st) {
  talker.error('Failed to get initial shortcut', e, st);
  return null;
}

2. Race Condition Risk (iOS AppDelegate.swift:92-100)

The shortcut handler removes the stored shortcut immediately after reading it:

shortcutChannel.setMethodCallHandler { call, result in
  if call.method == "getInitialShortcut" {
    let shortcut = UserDefaults.standard.string(forKey: "initialShortcut")
    result(shortcut)
    UserDefaults.standard.removeObject(forKey: "initialShortcut")  // ⚠️ Race condition
  }
}

Issue: If Flutter calls getInitialShortcut() multiple times before the navigation completes (e.g., during hot restart or app state restoration), the second call will return nil instead of the intended shortcut.

Recommendation: Either:

  • Remove the shortcut value from Flutter side after successful navigation, OR
  • Add a delay/flag to ensure single consumption

3. Android SharedPreferences Leakage

The Android implementation stores the shortcut but never clears it:

private fun handleIntent(intent: Intent?) {
    val shortcut = intent?.getStringExtra("shortcut")
    if (shortcut != null) {
        getSharedPreferences("shortcut", MODE_PRIVATE)
            .edit()
            .putString("initialShortcut", shortcut)
            .apply()  // ⚠️ Never cleared
    }
}

Unlike iOS, Android doesn't clear this value. This could cause shortcuts to re-trigger on app restarts.

Recommendation: Add clearing logic in the MethodChannel handler:

if (call.method == "getInitialShortcut") {
    val shortcut = getSharedPreferences("shortcut", MODE_PRIVATE)
        .getString("initialShortcut", null)
    getSharedPreferences("shortcut", MODE_PRIVATE)
        .edit()
        .remove("initialShortcut")
        .apply()
    result.success(shortcut)
}

Code Quality Issues

4. Missing Documentation (Multiple Files)

Per CLAUDE.md (line 89): public_member_api_docs: warning is enabled, but several public members lack documentation:

  • lib/main.dart:177 - platform constant
  • lib/main.dart:179 - getInitialShortcut() function
  • ios/Runner/AppDelegate.swift:112 - setupSiriShortcut() function

Recommendation: Add dartdoc comments:

/// Platform channel for communicating app shortcut actions.
const platform = MethodChannel('com.exptech.dpip/shortcut');

/// Retrieves the initial shortcut identifier that launched the app.
///
/// Returns the shortcut ID (e.g., 'monitor') if the app was launched via
/// a shortcut, or `null` if launched normally.
Future<String?> getInitialShortcut() async { ... }

5. Inconsistent Null Safety (lib/app.dart:72)

settings.authorizationStatus == .authorized ||  // ❌ Missing AuthorizationStatus prefix

While this compiles, it's inconsistent with Flutter best practices. Should be:

settings.authorizationStatus == AuthorizationStatus.authorized ||
settings.authorizationStatus == AuthorizationStatus.provisional;

6. Hardcoded Strings (Android strings.xml)

The shortcut labels are hardcoded in Traditional Chinese:

<string name="shortcut_monitor_short">強震監視器</string>
<string name="shortcut_monitor_long">打開強震監視器</string>

Issue: The app supports 7 locales (en, ja, ko, ru, vi, zh-Hans, zh-Hant) per CLAUDE.md, but shortcuts only show in Chinese for all users.

Recommendation: Add localized string resources (though this might be acceptable if all users are Taiwan-based).


Performance Concerns

7. Unnecessary Widget Rebuild (lib/app.dart:96-110)

The shortcut handling is embedded in _tryHandleStartupNavigation() which is called via addPostFrameCallback. This is fine, but the logic could be more defensive:

if (!_hasHandledInitialShortcut && widget.initialShortcut != null) {
  _hasHandledInitialShortcut = true;
  switch (widget.initialShortcut) {  // ⚠️ No default case
    case 'monitor':
      ctx.push(MapPage.route(...));
      return;  // ⚠️ Early return makes flow unclear
  }
}

Recommendation: Add a default case and restructure for clarity:

if (!_hasHandledInitialShortcut && widget.initialShortcut != null) {
  _hasHandledInitialShortcut = true;
  final handled = switch (widget.initialShortcut!) {
    'monitor' => _navigateToMonitor(ctx),
    _ => false,
  };
  if (handled) return;
}

Minor Issues

8. iOS Siri Shortcut Setup (AppDelegate.swift:112-122)

The Siri shortcut is set up but never used elsewhere in the code:

func setupSiriShortcut() {
    let activity = NSUserActivity(activityType: "com.exptech.dpip.monitor")
    activity.title = "強震監視器"
    activity.isEligibleForSearch = true
    activity.isEligibleForPrediction = true
    // ...
    self.userActivity = activity
    activity.becomeCurrent()
}

Question: Is this intentional for future Siri integration, or should this be utilized now?

9. Import Cleanup (lib/app.dart:22-23)

Two new imports were added but could be organized:

import 'app/map/_lib/utils.dart';
import 'app/map/page.dart';

These should use package:dpip/ prefix for consistency with other imports.


Security Considerations

No security vulnerabilities detected. The implementation:

  • Properly validates shortcut data before navigation
  • Uses platform-standard storage (UserDefaults/SharedPreferences)
  • Doesn't expose sensitive data through shortcuts

Test Coverage

⚠️ No tests added - The repository has no test suite (per CLAUDE.md line 306), so this is expected but worth noting.


Recommendations Summary

Must Fix:

  1. Replace print() with talker.error() in getInitialShortcut() (lib/main.dart:184)
  2. Fix Android SharedPreferences clearing issue (MainActivity.kt)
  3. Fix iOS race condition by moving cleanup to Flutter side

Should Fix:
4. Add documentation for public APIs
5. Fix null safety style in authorization check

Nice to Have:
6. Add localized strings for non-Chinese users
7. Improve control flow in shortcut handling
8. Clarify Siri shortcut purpose


Verification Checklist

Based on the PR template:

  • Semantic variable naming: ✅ Pass - Variables like initialShortcut, _hasHandledInitialShortcut are clear
  • AA color contrast: N/A - No UI color changes
  • Small PR: ⚠️ 11 files changed - reasonable for a cross-platform feature
  • Documentation: ⚠️ Missing public API docs
  • Screenshots: ✅ Provided for both platforms

Conclusion

The shortcut implementation is functionally correct and follows the platform-specific patterns well. The main concerns are around state management (clearing the shortcut after use) and code quality (logging, documentation). These should be addressed before merging to maintain code quality standards.

Suggested Action: Request changes for the critical issues (#1, #2, #3), others can be addressed in follow-up PRs if needed.


@lowrt
Copy link
Contributor Author

lowrt commented Jan 1, 2026

@claude review

@claude
Copy link

claude bot commented Jan 1, 2026

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


PR Review: Add Shortcut Feature

I've completed a thorough review of the shortcut implementation. This is a second review following the earlier comprehensive feedback.

Summary

The shortcut implementation has been significantly improved since the previous review. Many critical issues have been addressed, including the Android SharedPreferences cleanup and the duplicate navigation handling. The code now properly handles shortcuts on both platforms with better state management.


✅ Improvements Since Last Review

Several critical issues from the previous review have been fixed:

  1. Android SharedPreferences Cleanup ✅ - Fixed in MainActivity.kt:22

    • Now properly clears initialShortcut after retrieval
    • Prevents shortcuts from re-triggering on app restarts
  2. Simplified Navigation Logic ✅ - Improved in lib/app.dart:98-112

    • Removed duplicate navigation code paths
    • Cleaner separation between initial and runtime shortcut handling
  3. iOS State Management ✅ - Working as intended in AppDelegate.swift:110-118

    • Properly clears stored shortcut after reading

🔴 Critical Issues

1. Using print() Instead of Talker (lib/main.dart:184)

Severity: High
Impact: Inconsistent logging, makes debugging harder

} on PlatformException catch (e) {
  print('Failed to get initial shortcut: $e');  // ❌ Wrong
  return null;
}

Fix Required:

} on PlatformException catch (e, st) {
  talker.error('Failed to get initial shortcut', e, st);
  return null;
}

Why this matters: The codebase uses talker consistently throughout (see line 26: final talker = TalkerManager.instance). Using print() bypasses the centralized logging system, making it harder to track issues in production.


2. Missing Authorization Status Prefix (lib/app.dart:74-75)

Severity: Medium
Impact: Inconsistent code style, potential confusion

settings.authorizationStatus == .authorized ||  // ❌ Missing prefix
settings.authorizationStatus == .provisional;

Fix Required:

settings.authorizationStatus == AuthorizationStatus.authorized ||
settings.authorizationStatus == AuthorizationStatus.provisional;

Why this matters: While Dart's type inference makes this work, it's inconsistent with Flutter best practices and makes the code less readable.


3. Missing Public API Documentation (lib/main.dart:177-187)

Severity: Medium
Impact: Violates analysis_options.yaml rule public_member_api_docs: warning

The following public members lack documentation:

  • Line 177: platform constant
  • Line 179: getInitialShortcut() function

Fix Required:

/// Platform channel for communicating app shortcut actions between
/// native code (Android/iOS) and Flutter.
const platform = MethodChannel('com.exptech.dpip/shortcut');

/// Retrieves the initial shortcut identifier that launched the app.
///
/// Returns the shortcut ID (e.g., 'monitor') if the app was launched via
/// a shortcut, or `null` if launched normally or on error.
///
/// This method should only be called once during app initialization.
Future<String?> getInitialShortcut() async {
  // ...
}

⚠️ Code Quality Issues

4. Hardcoded Localization (Multiple Files)

Severity: Low
Impact: Poor UX for non-Chinese users

All shortcut labels are hardcoded in Traditional Chinese:

  • android/app/src/main/res/values/strings.xml:3-4 - "強震監視器"
  • ios/Runner/Info.plist:69 - "強震監視器"

Issue: The app supports 7 locales (en, ja, ko, ru, vi, zh-Hans, zh-Hant) per lib/main.dart:118-127, but shortcuts only display in Chinese for all users.

Recommendation: Since the app is Taiwan-focused, this might be acceptable. However, consider adding localized strings for better UX:

Android: Create values-en/strings.xml, values-ja/strings.xml, etc.
iOS: Use localized InfoPlist.strings files


5. Duplicate Navigation Code (lib/app.dart:98-112 vs 128-148)

Severity: Low
Impact: Code duplication, maintenance burden

Two nearly identical functions handle shortcut navigation:

  • _tryHandleStartupNavigation() (lines 87-120)
  • _handleShortcutNavigation() (lines 128-148)

Observation: The main difference is push vs go. While this is functional, it creates maintenance overhead if shortcut types expand.

Optional Improvement: Extract common logic:

void _navigateToShortcut(BuildContext ctx, String shortcut, {bool replace = false}) {
  final route = switch (shortcut) {
    'monitor' => MapPage.route(
      options: MapPageOptions(initialLayers: {MapLayer.monitor}),
    ),
    _ => null,
  };
  
  if (route != null) {
    replace ? ctx.go(route) : ctx.push(route);
  }
}

6. Unused Method Call (lib/app.dart:164)

Severity: Low
Impact: Misleading code

_onEnterMonitorPage();  // Called on every app start

This is called in initState(), which means it donates the Siri shortcut every time the app starts, not when the user actually enters the monitor page.

Questions:

  1. Is this intentional to ensure the shortcut is always available?
  2. Should this be called when the user actually navigates to the monitor page?

Recommendation: Either rename to _setupSiriShortcut() or move to actual monitor page entry.


7. Redundant Shortcut Handling (lib/app.dart:175-191)

Severity: Low
Impact: Duplicate logic

The _handleInitialShortcut() method (lines 180-191) duplicates the logic already in _tryHandleStartupNavigation() (lines 87-120).

Observation: Both methods check _hasHandledInitialShortcut and call _handleShortcutNavigation(). The flow is:

  1. initState() calls both _onEnterMonitorPage() and adds post-frame callback for _handleInitialShortcut()
  2. _handleInitialShortcut() calls _handleShortcutNavigation()
  3. But _tryHandleStartupNavigation() already does this

Recommendation: Simplify by consolidating into one entry point.


✅ What's Working Well

  1. Platform Integration - Proper use of MethodChannel for bidirectional communication
  2. State Management - Correct use of flags to prevent duplicate handling
  3. Error Handling - Try-catch blocks in appropriate places (except the print issue)
  4. Android Implementation - Clean Kotlin code with proper intent handling
  5. iOS Implementation - Handles both Quick Actions and Siri Shortcuts
  6. Navigation - Proper use of go_router with type-safe routes

🔒 Security Check

No security vulnerabilities detected

  • Shortcut data is validated before navigation
  • Platform channels use standard Flutter APIs
  • No sensitive data exposed through shortcuts
  • Proper use of UserDefaults/SharedPreferences

📋 Testing Recommendations

Since the repository has no test suite (per CLAUDE.md), manual testing should cover:

  1. Cold Start with Shortcut

    • Android: Long-press app icon → tap shortcut
    • iOS: 3D Touch → tap shortcut
  2. Runtime Shortcut Activation

    • App already running → use shortcut → should navigate correctly
  3. Siri Integration (iOS)

    • "Hey Siri, 強震監視器" → should open monitor page
  4. Edge Cases

    • Multiple rapid shortcut taps
    • Shortcut while app is backgrounded
    • First launch via shortcut

📝 Recommendations Summary

Must Fix (Blocks Merge):

  1. ✅ Replace print() with talker.error() (lib/main.dart:184)
  2. ✅ Fix authorization status syntax (lib/app.dart:74)
  3. ⚠️ Add public API documentation (lib/main.dart:177, 179)

Should Fix (Can be Follow-up):
4. Consider localized shortcut labels for international users
5. Refactor duplicate navigation code
6. Clarify _onEnterMonitorPage() purpose/timing

Nice to Have:
7. Simplify shortcut handling flow


🎯 Verdict

Status: ✅ Approve with Minor Changes

The implementation is functionally correct and the critical issues from the previous review have been addressed. The remaining issues are mostly code quality improvements that don't affect functionality.

The shortcut feature works as expected on both platforms. The main blocking issues are:

  1. Logging consistency (print → talker)
  2. Code style compliance (authorization status)
  3. Documentation requirements (public API docs)

These are quick fixes that maintain code quality standards without requiring architectural changes.


Changed Files Analysis


@lowrt
Copy link
Contributor Author

lowrt commented Jan 1, 2026

@claude review

@claude
Copy link

claude bot commented Jan 1, 2026

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


PR Review: Add Shortcut Feature

I've reviewed the shortcut implementation. Excellent work addressing previous feedback! The localization improvements are particularly noteworthy.


✅ Improvements Since Last Review

Major fixes implemented:

  1. Logging Fixed (lib/main.dart:184) - Now uses talker.error() instead of print()
  2. Localization Added - Android strings now support en, ja, ko, zh-Hans, zh-Hant
  3. iOS Localization - Added en, ja, ko .lproj folders with proper strings

🔴 Critical Issue

Authorization Status Syntax (lib/app.dart:72-73)

Missing AuthorizationStatus prefix for enums:

settings.authorizationStatus == .authorized ||  // ❌
settings.authorizationStatus == .provisional;

Fix required:

settings.authorizationStatus == AuthorizationStatus.authorized ||
settings.authorizationStatus == AuthorizationStatus.provisional;

While Dart's type inference makes this compile, it violates Flutter style guidelines and reduces code readability.


⚠️ Code Quality Issues

1. Missing Documentation (lib/main.dart:177, 179)

Per analysis_options.yaml rule public_member_api_docs: warning, these need docs:

/// Platform channel for app shortcut communication.
const platform = MethodChannel('com.exptech.dpip/shortcut');

/// Retrieves shortcut ID that launched the app, if any.
///
/// Returns shortcut identifier (e.g., 'monitor') or null.
Future<String?> getInitialShortcut() async { ... }

2. Android Shortcut Intent Handling (MainActivity.kt:42-43)

Two different ways to retrieve shortcut ID:

val shortcutId = intent.getStringExtra("android.intent.extra.shortcut.ID")
    ?: intent.getStringExtra("shortcut")

Question: Is the fallback to "shortcut" needed? Android shortcuts should use the standard android.intent.extra.shortcut.ID.

3. iOS Localization Keys (en.lproj/Monitor.strings:1, 3)

Uses generated keys instead of semantic names:

"l2uH53" = "This shortcut opens the Earthquake Monitor screen directly";
"qno9IY" = "Open Earthquake Monitor";

These appear to be auto-generated by Xcode's Intent system. While functional, they're harder to maintain than semantic keys.


💡 Architecture Observations

Shortcut Flow Simplified

The implementation now has a cleaner flow:

  1. Cold start: getInitialShortcut() → stores in widget.initialShortcut → handled in _tryHandleStartupNavigation()
  2. Runtime: iOS notifyFlutterShortcut() calls method channel → handled in Flutter

Good: Removed duplicate navigation code from previous iteration.

State Management

Both platforms properly clear stored shortcuts after retrieval:

  • Android: MainActivity.kt:22 - sets initialShortcut = null
  • iOS: AppDelegate.swift:114 - removes from UserDefaults

Good: Prevents re-triggering on app restart.


🔍 Minor Issues

4. Inconsistent Error Messages (lib/main.dart:184)

Error message is in English while most logs use Chinese (see line 170: '✅ LocationServiceManager 完成').

Suggestion: Use Chinese for consistency, or standardize on English - but be consistent across the codebase.

5. iOS Siri Shortcut Activity (AppDelegate.swift:63-66)

Handles both:

  • com.exptech.dpip.monitor (manual NSUserActivity)
  • OpenMonitorIntentIntent (Intent Definition)

Question: Why two activity types? The Intent Definition should be sufficient for Siri.


✅ What's Working Well

  1. Platform Integration - Proper MethodChannel usage on both platforms
  2. Localization - Now supports 5 locales (en, ja, ko, zh-Hans, zh-Hant)
  3. State Management - Correct cleanup prevents duplicate triggers
  4. Error Handling - Uses talker consistently
  5. Code Quality - Clean Kotlin/Swift implementation

🔒 Security

✅ No security issues detected


📋 Recommendations Summary

Must Fix:

  1. Fix authorization status enum prefix (lib/app.dart:72-73)
  2. Add public API documentation (lib/main.dart:177, 179)

Should Consider:
3. Verify if "shortcut" fallback is needed (MainActivity.kt:42)
4. Standardize error message language
5. Clarify why two iOS activity types

Nice to Have:
6. Use semantic keys instead of generated IDs in iOS strings


🎯 Verdict

Status: ✅ Approve with Minor Fix

Excellent implementation! The localization additions address a major concern from the previous review. Only one critical issue remains (authorization status syntax), which is a quick fix.

The shortcut functionality works correctly on both platforms and follows platform conventions. Once the enum syntax is fixed and documentation is added, this is ready to merge.

Blocking Issue: Authorization status enum prefix (lib/app.dart:72-73)


@lowrt lowrt merged commit c9ae44c into main Jan 2, 2026
7 checks passed
@lowrt lowrt deleted the feat/shortcut branch January 2, 2026 08:33
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