Skip to content

feat(pancake): add Shopee platform support#975

Open
nguyennguyenit wants to merge 5 commits intodevfrom
feat/pancake-shopee-support
Open

feat(pancake): add Shopee platform support#975
nguyennguyenit wants to merge 5 commits intodevfrom
feat/pancake-shopee-support

Conversation

@nguyennguyenit
Copy link
Copy Markdown
Contributor

Summary

  • Fix Accept: application/json header for Pancake GET requests (Shopee returns SPA HTML without it)
  • Platform-prefix-aware pageID parsing for spo_ convID format
  • 500-char message limit for Shopee DMs (same as TikTok)
  • Rune-safe truncation with slog.Warn on truncation

Changes

File Change
api_client.go setCommonHeaders helper, wired to GetPage + newPageRequest
webhook_handler.go resolvePageIDFromConvID + platformPrefixes map (spo only)
pancake.go Added shopee to 500-char limit case
formatter.go Renamed truncateForTikToktruncateRuneSafe(content, limit)

Test plan

  • TestNewPageRequest_SetsAcceptJSONHeader — Accept header on page-API requests
  • TestGetPage_SetsAcceptJSONHeader — Accept header on user-API requests (C2)
  • TestResolvePageIDFromConvID — 6 cases: FB numeric, Shopee 3-seg, Shopee 2-seg, empty, no underscore, prefix-only
  • TestMaxMessageLength_Shopee — returns 500
  • TestTruncateRuneSafe_Shopee — Vietnamese + emoji truncation
  • All 100+ existing pancake tests pass
  • Phase 3 integration smoke (pending real Shopee credentials)

- Fix Accept header for JSON response negotiation (Shopee GETs)
- Platform-prefix-aware pageID parsing (spo_ convID format)
- 500-char message limit for Shopee DMs
- Rune-safe truncation with slog.Warn on truncation

Phase 3 (integration smoke) pending: requires real credentials.
@mrgoonie
Copy link
Copy Markdown
Contributor

🔍 Code Review — feat(pancake): add Shopee platform support

🎯 Tổng quan

Thêm Shopee platform support vào pancake channel: fix Accept header cho JSON response negotiation, platform-prefix-aware pageID parsing, 500-char message limit, và rune-safe truncation.

Scope: 8 files, +180/-18 lines


✅ Điểm Tốt

  1. Root cause rõ ràng. Shopee GET không có Accept: application/json → Pancake trả SPA HTML. Fix đơn giản, đúng chỗ: setCommonHeaders helper wired vào GetPage + newPageRequest.

  2. resolvePageIDFromConvID design cẩn thận. Platform-prefix map (spo only) với comment giải thích tại sao chưa thêm lzd/tpd — tốt, không speculative. Handle cả 2-segment (system event) và 3-segment (buyer DM) convID.

  3. truncateRuneSafe refactor đúng hướng. Rename từ truncateForTikTok → generic function với limit parameter. Dùng []rune] tránh UTF-8 corruption cho Vietnamese/emoji. Có slog.Warn] khi truncation — observability tốt.

  4. Tests coverage đầy đủ. Accept header tests (newPageRequest + GetPage), pageID parsing (6 cases), Shopee max length, rune-safe truncation với Vietnamese + emoji, regression guards cho existing platforms.

  5. Testdata fixture cho Shopee webhook — dù là assumed-shape nhưng có comment rõ ràng để verify ở Phase 3.


⚠️ Issues Cần Lưu Ý

1. [MEDIUM] — platformPrefixes map nên được inject hoặc config-driven

internal/channels/pancake/webhook_handler.go:

var platformPrefixes = map[string]struct{}{
    "spo": {}, // Shopee — verified via curl 2026-04-20
}

Hardcoded package-level variable khó test và maintain. Nếu sau thêm Lazada/Tokopedia thì phải sửa code + rebuild. Nên chuyển thành config field hoặc inject qua constructor.

Fix đề xuất: Thêm PlatformPrefixes vào pancakeInstanceConfig hoặc inject qua webhookRouter constructor.

2. [LOW] — setCommonHeaders name hơi generic

Function name setCommonHeaders] có thể conflict với future headers khác. Hiện tại chỉ set Accept] nên có thể rename thành `setAcceptJSONHeader] cho rõ intent.

3. [LOW] — testdata/shopee_inbox_webhook.json] chưa có page_id]

Fixture để trống page_id] ở cả top-level và data-level. Đây có thể là intentional để test resolvePageIDFromConvID] fallback, nhưng nên có thêm 1 fixture với `page_id] đầy đủ để verify priority logic (top-level > data-level > convID parse).


📊 Summary

Severity Count Issues
🟡 MEDIUM 1 `platformPrefixes] hardcoded, nên config-driven
🟢 LOW 2 Function name generic; testdata fixture thiếu page_id case

💡 Recommendation

🟡 APPROVE WITH CHANGES

PR clean, Shopee support implement đúng hướng. Issue #1 (platformPrefixes config) nên fix trước khi merge để avoid technical debt. #2#3 là optional.

Great work @nguyennguyenit! 🚀

- Add RegisterPlatformPrefix() for extensibility (issue #1)
- Rename setCommonHeaders → setAcceptJSONHeader (issue #2)
- Add testdata fixture with page_id for priority testing (issue #3)
Add MethodSessionsCompact to isWriteMethod to fix drift coverage test.
Method was added in #958 but missing from permissions classification.
Remove duplicate contains helper in tts_gemini_live_test.go that
conflicts with mcp_grant_revoke_test.go in same package.
@clark-cant
Copy link
Copy Markdown

🔍 Re-Review — PR #975 (after feedback fixes)

Status: ✅ All previous issues addressed

Issue Severity Status Fix
platformPrefixes hardcoded MEDIUM ✅ Fixed Added RegisterPlatformPrefix() for extensibility — Lazada/Tokopedia can register at init time
Function name generic LOW ✅ Fixed Renamed setCommonHeaderssetAcceptJSONHeader — intent rõ ràng
Testdata thiếu page_id LOW ✅ Fixed Added shopee_inbox_webhook_with_page_id.json fixture for priority testing

Additional observations

  • RegisterPlatformPrefix thread-safe (init-time only), comment giải thích rõ scope (M1: Shopee only)
  • Fixture mới có `page_id] ở top-level → verify được priority: event.page_id > data.page_id > convID parse
  • isWriteMethod bổ sung `sessions.compact] — drift coverage fix đúng
  • strings.Contains] thay local contains] helper — tránh redeclaration conflict, clean

Recommendation

🟢 APPROVE

All feedback addressed. PR sạch, Shopee support ready to merge. Phase 3 integration smoke pending real credentials — có thể merge trước, Phase 3 follow-up sau.

Great work @nguyennguyenit! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants