fix(slack_webhook): address PR review feedback

- Add 30s timeout to HTTP client to prevent hanging connections
- Fix splitText to work with runes (not bytes) for proper UTF-8 handling
- Chunk large tables to respect Slack's 3000 char limit per block
- Fix data races in tests using atomic counters and channels
- Add proper error classification assertions (ErrTemporary vs ErrSendFailed)
- Use targetName instead of msg.ChatID in log messages

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Author: loafoe

pkg/channels/slack_webhook/slack_webhook.go

@@ -9,6 +9,7 @@ import (
 	"net/url"
 	"sort"
 	"strings"
+	"time"
 
 	"github.com/sipeed/picoclaw/pkg/bus"
 	"github.com/sipeed/picoclaw/pkg/channels"
@@ -67,7 +68,9 @@ func NewSlackWebhookChannel(
 		BaseChannel: base,
 		bc:          bc,
 		config:      cfg,
-		client:      &http.Client{},
+		client: &http.Client{
+			Timeout: 30 * time.Second,
+		},
 	}, nil
 }
 
@@ -134,22 +137,22 @@ func (c *SlackWebhookChannel) Send(ctx context.Context, msg bus.OutboundMessage)
 	resp, err := c.client.Do(req)
 	if err != nil {
 		logger.ErrorCF("slack_webhook", "Failed to send message", map[string]any{
-			"target": msg.ChatID,
+			"target": targetName,
 		})
 		return nil, fmt.Errorf("slack_webhook: send failed: %w", channels.ClassifyNetError(err))
 	}
 	defer resp.Body.Close()
 
 	if resp.StatusCode >= 400 {
 		logger.ErrorCF("slack_webhook", "Slack API error", map[string]any{
-			"target": msg.ChatID,
+			"target": targetName,
 			"status": resp.StatusCode,
 		})
 		return nil, fmt.Errorf("slack_webhook: send failed: %w", channels.ClassifySendError(resp.StatusCode, fmt.Errorf("status %d", resp.StatusCode)))
 	}
 
 	logger.DebugCF("slack_webhook", "Message sent successfully", map[string]any{
-		"target": msg.ChatID,
+		"target": targetName,
 	})
 
 	return nil, nil
@@ -184,7 +187,9 @@ func (c *SlackWebhookChannel) buildBlocks(content string) []map[string]any {
 	for _, seg := range segments {
 		if seg.isTable {
 			tableText := renderTable(seg.content)
-			blocks = append(blocks, c.textSection(tableText))
+			for _, chunk := range splitText(tableText, maxTextBlockLength) {
+				blocks = append(blocks, c.textSection(chunk))
+			}
 		} else {
 			text := strings.TrimSpace(seg.content)
 			if text == "" {
@@ -215,23 +220,25 @@ func (c *SlackWebhookChannel) textSection(text string) map[string]any {
 }
 
 func splitText(text string, maxLen int) []string {
-	if len(text) <= maxLen {
+	runes := []rune(text)
+	if len(runes) <= maxLen {
 		return []string{text}
 	}
 
 	var chunks []string
-	for len(text) > maxLen {
+	for len(runes) > maxLen {
 		splitAt := maxLen
-		if idx := strings.LastIndex(text[:maxLen], "\n"); idx > 0 {
-			splitAt = idx + 1
-		} else if idx := strings.LastIndex(text[:maxLen], " "); idx > 0 {
-			splitAt = idx + 1
+		window := string(runes[:maxLen])
+		if idx := strings.LastIndex(window, "\n"); idx > 0 {
+			splitAt = len([]rune(window[:idx])) + 1
+		} else if idx := strings.LastIndex(window, " "); idx > 0 {
+			splitAt = len([]rune(window[:idx])) + 1
 		}
-		chunks = append(chunks, text[:splitAt])
-		text = text[splitAt:]
+		chunks = append(chunks, string(runes[:splitAt]))
+		runes = runes[splitAt:]
 	}
-	if len(text) > 0 {
-		chunks = append(chunks, text)
+	if len(runes) > 0 {
+		chunks = append(chunks, string(runes))
 	}
 	return chunks
 }

pkg/channels/slack_webhook/slack_webhook_test.go

@@ -3,15 +3,18 @@ package slackwebhook
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"sync/atomic"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/sipeed/picoclaw/pkg/bus"
+	"github.com/sipeed/picoclaw/pkg/channels"
 	"github.com/sipeed/picoclaw/pkg/config"
 )
 
@@ -79,10 +82,12 @@ func TestNewSlackWebhookChannel_Validation(t *testing.T) {
 }
 
 func TestSlackWebhookChannel_Send(t *testing.T) {
-	var receivedPayload map[string]any
+	payloadCh := make(chan map[string]any, 1)
 	server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		body, _ := io.ReadAll(r.Body)
-		json.Unmarshal(body, &receivedPayload)
+		var payload map[string]any
+		json.Unmarshal(body, &payload)
+		payloadCh <- payload
 		w.WriteHeader(http.StatusOK)
 	}))
 	defer server.Close()
@@ -115,6 +120,7 @@ func TestSlackWebhookChannel_Send(t *testing.T) {
 	require.NoError(t, err)
 
 	// Verify payload structure
+	receivedPayload := <-payloadCh
 	assert.Equal(t, "TestBot", receivedPayload["username"])
 	assert.Equal(t, ":test:", receivedPayload["icon_emoji"])
 	blocks, ok := receivedPayload["blocks"].([]any)
@@ -123,9 +129,9 @@ func TestSlackWebhookChannel_Send(t *testing.T) {
 }
 
 func TestSlackWebhookChannel_FallbackToDefault(t *testing.T) {
-	var requestCount int
+	var requestCount atomic.Int32
 	server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		requestCount++
+		requestCount.Add(1)
 		w.WriteHeader(http.StatusOK)
 	}))
 	defer server.Close()
@@ -149,7 +155,7 @@ func TestSlackWebhookChannel_FallbackToDefault(t *testing.T) {
 		ChatID:  "unknown_target",
 	})
 	require.NoError(t, err)
-	assert.Equal(t, 1, requestCount)
+	assert.Equal(t, int32(1), requestCount.Load())
 }
 
 func TestSlackWebhookChannel_ErrorClassification(t *testing.T) {
@@ -188,7 +194,12 @@ func TestSlackWebhookChannel_ErrorClassification(t *testing.T) {
 
 			_, err := ch.Send(context.Background(), bus.OutboundMessage{Content: "Test"})
 			require.Error(t, err)
-			// Error classification is internal; just verify we got an error
+
+			if tt.expectTemp {
+				assert.True(t, errors.Is(err, channels.ErrTemporary), "expected temporary error for %d", tt.statusCode)
+			} else {
+				assert.True(t, errors.Is(err, channels.ErrSendFailed), "expected permanent error for %d", tt.statusCode)
+			}
 		})
 	}
 }