feat(web): implement macOS app feature and file logger

[cytown]

📝 Description

The commit adds macOS application features and implements file logging functionality to the web component.

🗣️ Type of Change

• 🐞 Bug fix (non-breaking change which fixes an issue)
• ✨ New feature (non-breaking change which adds functionality)
• 📖 Documentation update
• ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

• 🤖 Fully AI-generated (100% AI, 0% Human)
• 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
• 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

📚 Technical Context (Skip for Docs)

• Reference URL:
• Reasoning:

🧪 Test Environment

• Hardware:
• OS:
• Model/Provider:
• Channels:

📸 Evidence (Optional)

Click to view Logs/Screenshots

☑️ Checklist

• My code/docs follow the style of this project.
• I have performed a self-review of my own changes.
• I have updated the documentation accordingly.

[yinwm]

🔍 Code Review: PR #1723

Overview

This PR adds macOS/Windows desktop app packaging and refactors the logging system. The direction is correct, but there are several issues that need to be addressed before merging.

---

🔴 Critical Issues (Must Fix)

1. scripts/build-macos-app.sh:169-201 — Duplicate key in Info.plist

<key>NSHighResolutionCapable</key>
<true/>
...
<key>NSHighResolutionCapable</key>  <!-- Line 199, duplicate! -->
<true/>

Impact: Undefined plist parsing behavior, may cause icon display issues.

---

2. web/backend/main.go:78-82 — Error output to stderr after logger initialization

if !enableConsole {
    logger.SetConsoleLevel(logger.FATAL)
    logPath := filepath.Join(picoHome, "logs", "web.log")
    if err := logger.EnableFileLogging(logPath); err != nil {
        fmt.Fprintf(os.Stderr, "Failed to initialize logger: %v\n", err)  // ⚠️ Still outputs to stderr
        os.Exit(1)
    }
    ...
}

Problem: In non-console mode (macOS .app), stderr is not available. Error messages will be lost.

Suggestion: Use logger.Fatalf or ensure errors are written to a log file.

---

3. web/backend/main.go:88-89 — Log directory may not exist

logPath := filepath.Join(picoHome, "logs", "web.log")
if err := logger.EnableFileLogging(logPath); err != nil {

Problem: The logs directory may not exist. Does EnableFileLogging create parent directories? If not, this will fail.

---

4. web/backend/main.go:217-218 — Defer order affects shutdown logging

defer logger.DisableFileLogging()  // Line 100
...
defer shutdownApp()                 // Line 217

Problem: Defer executes LIFO, so:

1. shutdownApp() runs first
2. logger.DisableFileLogging() runs second

This means logs during shutdown won't be written to the file. Shutdown logs are critical for debugging.

---

🟡 Medium Issues (Should Fix)

5. web/backend/api/gateway.go — Inconsistent owned check between StopGateway and handleGatewayStop

// StopGateway - has owned check
func (h *Handler) StopGateway() {
    if !gateway.owned || gateway.cmd == nil || gateway.cmd.Process == nil {
        return
    }
    ...
}

// handleGatewayStop - no owned check, calls stopGatewayLocked() directly
func (h *Handler) handleGatewayStop(...) {
    ...
    pid, err := stopGatewayLocked()  // No owned check
    ...
}

Problem: API endpoint can stop any gateway process (including attached ones), but Shutdown only stops self-started processes. This may be intentional, but should be documented with a comment.

---

6. scripts/setup.iss:286 — Windows icon path may not exist

Source: "..\web\backend\icon.ico"; DestDir: "{app}"; Flags: ignoreversion

Problem: The code references web/backend/icon.ico, but this PR only adds scripts/icon.icns. Where is the .ico file?

---

7. web/backend/utils/runtime.go:14-19 — GetPicoclawHome ignores error

func GetPicoclawHome() string {
    ...
    home, _ := os.UserHomeDir()  // ⚠️ Error ignored
    return filepath.Join(home, ".picoclaw")
}

Problem: If os.UserHomeDir() fails, home is empty, returning .picoclaw (relative path).

Suggestion: Handle the error or fallback to current directory.

---

8. web/Makefile:90 — clean target doesn't remove hidden files

clean:
    rm -rf frontend/dist backend/dist $(BUILD_DIR)/*

Problem: $(BUILD_DIR)/* won't remove hidden files (starting with .). Should use rm -rf $(BUILD_DIR) then recreate.

---

🟢 Suggestions (Nice to Have)

9. Consider adding error handling for icon file in build script

if [ ! -f "$ICON_SOURCE" ]; then
    echo "Error: Icon file not found: $ICON_SOURCE"
    exit 1
fi

---

📊 Summary

Level	Count
🔴 Critical	4
🟡 Medium	4
🟢 Suggestion	1

---

✅ What's Done Well

1. Process ownership tracking: The gateway.owned design is excellent for distinguishing self-started vs attached processes
2. Complete logger migration: Thorough migration from log package to custom logger
3. Platform compatibility: Both macOS and Windows are considered
4. Shutdown flow improvements: SSE connection closing, keep-alive disabling
5. Code organization: stopGatewayLocked extracted as a reusable function

Please address the critical issues before merging. Happy to discuss any of these points!

[cytown]

🔍 Code Review: PR #1723

Overview

This PR adds macOS/Windows desktop app packaging and refactors the logging system. The direction is correct, but there are several issues that need to be addressed before merging.

🔴 Critical Issues (Must Fix)

1. scripts/build-macos-app.sh:169-201 — Duplicate key in Info.plist

<key>NSHighResolutionCapable</key>
<true/>
...
<key>NSHighResolutionCapable</key>  <!-- Line 199, duplicate! -->
<true/>

Impact: Undefined plist parsing behavior, may cause icon display issues.

done

2. web/backend/main.go:78-82 — Error output to stderr after logger initialization

if !enableConsole {
    logger.SetConsoleLevel(logger.FATAL)
    logPath := filepath.Join(picoHome, "logs", "web.log")
    if err := logger.EnableFileLogging(logPath); err != nil {
        fmt.Fprintf(os.Stderr, "Failed to initialize logger: %v\n", err)  // ⚠️ Still outputs to stderr
        os.Exit(1)
    }
    ...
}

Problem: In non-console mode (macOS .app), stderr is not available. Error messages will be lost.

Suggestion: Use logger.Fatalf or ensure errors are written to a log file.

#1734

3. web/backend/main.go:88-89 — Log directory may not exist

logPath := filepath.Join(picoHome, "logs", "web.log")
if err := logger.EnableFileLogging(logPath); err != nil {

Problem: The logs directory may not exist. Does EnableFileLogging create parent directories? If not, this will fail.

not a issue

4. web/backend/main.go:217-218 — Defer order affects shutdown logging

defer logger.DisableFileLogging()  // Line 100
...
defer shutdownApp()                 // Line 217

Problem: Defer executes LIFO, so:

1. shutdownApp() runs first
2. logger.DisableFileLogging() runs second

This means logs during shutdown won't be written to the file. Shutdown logs are critical for debugging.

not a issue

🟡 Medium Issues (Should Fix)

5. web/backend/api/gateway.go — Inconsistent owned check between StopGateway and handleGatewayStop

// StopGateway - has owned check
func (h *Handler) StopGateway() {
    if !gateway.owned || gateway.cmd == nil || gateway.cmd.Process == nil {
        return
    }
    ...
}

// handleGatewayStop - no owned check, calls stopGatewayLocked() directly
func (h *Handler) handleGatewayStop(...) {
    ...
    pid, err := stopGatewayLocked()  // No owned check
    ...
}

Problem: API endpoint can stop any gateway process (including attached ones), but Shutdown only stops self-started processes. This may be intentional, but should be documented with a comment.

done

6. scripts/setup.iss:286 — Windows icon path may not exist

Source: "..\web\backend\icon.ico"; DestDir: "{app}"; Flags: ignoreversion

Problem: The code references web/backend/icon.ico, but this PR only adds scripts/icon.icns. Where is the .ico file?

not a issue

7. web/backend/utils/runtime.go:14-19 — GetPicoclawHome ignores error

func GetPicoclawHome() string {
    ...
    home, _ := os.UserHomeDir()  // ⚠️ Error ignored
    return filepath.Join(home, ".picoclaw")
}

Problem: If os.UserHomeDir() fails, home is empty, returning .picoclaw (relative path).

Suggestion: Handle the error or fallback to current directory.

not a issue

8. web/Makefile:90 — clean target doesn't remove hidden files

clean:
    rm -rf frontend/dist backend/dist $(BUILD_DIR)/*

Problem: $(BUILD_DIR)/* won't remove hidden files (starting with .). Should use rm -rf $(BUILD_DIR) then recreate.

done

🟢 Suggestions (Nice to Have)

9. Consider adding error handling for icon file in build script

if [ ! -f "$ICON_SOURCE" ]; then
    echo "Error: Icon file not found: $ICON_SOURCE"
    exit 1
fi

not a issue

📊 Summary

Level Count
🔴 Critical 4
🟡 Medium 4
🟢 Suggestion 1

✅ What's Done Well

1. Process ownership tracking: The gateway.owned design is excellent for distinguishing self-started vs attached processes
2. Complete logger migration: Thorough migration from log package to custom logger
3. Platform compatibility: Both macOS and Windows are considered
4. Shutdown flow improvements: SSE connection closing, keep-alive disabling
5. Code organization: stopGatewayLocked extracted as a reusable function

Please address the critical issues before merging. Happy to discuss any of these points!