feat(externalselect): add initialOption + option_groups (vercel/chat#410, #397)#84
feat(externalselect): add initialOption + option_groups (vercel/chat#410, #397)#84patrick-chinchill wants to merge 2 commits intomainfrom
Conversation
, #397) Ports two upstream PRs that together complete ExternalSelect support: - vercel/chat#397 introduced ExternalSelectElement and the block_suggestion / onOptionsLoad runtime; the runtime half landed here in #66 but the modal element type was deferred. This PR adds the missing ExternalSelectElement TypedDict + ExternalSelect builder and wires up _external_select_to_block in the Slack modal renderer. - vercel/chat#410 adds two new optional fields on top: initialOption (pre-selected option object) and option_groups (labeled sections, Slack max 100 groups x 100 options, label max 75 chars). The handler return type widens to OptionsLoadResult = list[options] | list[OptionsLoadGroup]; the Slack adapter detects grouped form by the presence of an "options" key on the first entry and emits Slack's option_groups response (mutually exclusive with options per Slack's spec). Hazard #1 (truthiness): min_query_length=0 is preserved (0 means "fire on every keystroke"); not silently dropped by an `or` fallback. Hazard #7 (omit vs None): unset initial_option / placeholder / min_query_length are omitted from the rendered Block Kit element, not serialized as null. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| def process_options_load( | ||
| self, event: OptionsLoadEvent, options: WebhookOptions | None = None | ||
| ) -> Awaitable[list[SelectOptionElement] | None]: ... | ||
| ) -> Awaitable[OptionsLoadResult | None]: ... |
There was a problem hiding this comment.
Code Review
This pull request adds support for ExternalSelect modal elements and grouped options (option_groups) within the Slack adapter. Key changes include the definition of new types like ExternalSelectElement and OptionsLoadGroup, the addition of an ExternalSelect builder, and logic in the Slack adapter to dynamically handle flat or grouped option results from onOptionsLoad handlers. The update also ensures compliance with Slack's specifications regarding field limits and mutual exclusivity of response keys. Extensive tests have been added to verify the builder logic, Slack conversion, and event processing. I have no feedback to provide.
patrick-chinchill
left a comment
There was a problem hiding this comment.
Review: feat(externalselect): add initialOption + option_groups
Compared the Python port at 2c00393 against upstream TS at f55378a (chat@4.27.0):
packages/chat/src/modals.tsandmodals.test.tspackages/chat/src/types.ts(ExternalSelectElement,OptionsLoadGroup,OptionsLoadResult)packages/adapter-slack/src/modals.tsandmodals.test.tspackages/adapter-slack/src/index.ts(optionsLoadResponse+handleBlockSuggestion)
🔴 Critical
None.
🟡 Medium
None — coverage is solid for both upstream-mirrored tests and adversarial Python-specific cases (empty [], 100/100/75 caps, mutual exclusivity, description round-trip, min_query_length=0).
🔵 Nit
if initial_option:truthiness in_external_select_to_block(src/chat_sdk/adapters/slack/modals.py:209). TS usesif (select.initialOption), which only filtersnull/undefined— an object literal like{}is truthy in TS. In Python,{}is falsy, so a malformedinitial_option={}would silently render as noinitial_option(parity drift). TheExternalSelectbuilder usesis not None(modals.py:235), so this only fires if a caller hand-constructs the dict withinitial_option={}. Practical risk is low, butif initial_option is not None:would match the TS semantics exactly and be consistent with themin_query_lengthhandling two lines above.- Long
ModalChildunion line (src/chat_sdk/modals.py:83) is 100+ chars and may already trip ruff line length on stricter configs. Consider wrapping with parens. - Docstring in
_external_select_to_blocksays "this just emits the placeholder element" — slight ambiguity since "placeholder" overloads the Block Kitplaceholderfield. Could read as "emits only the placeholder." Minor wording polish.
✅ Looks good
- Hazard #1 (truthiness):
min_query_length=0correctly preserved viais not Nonein both the builder and the renderer; explicit regression test included. - Hazard #2 (boundary case): snake_case internally matches Slack's snake_case Block Kit keys, so no transformation needed;
OptionsLoadResulttype alias mirrors upstreamSelectOptionElement[] | OptionsLoadGroup[]. - Hazard #7 (omit vs None): All optional fields (
placeholder,min_query_length,initial_option,description) are conditionally added — never serialized asnull. Tests assert key absence. - Hazard #15 (behavior parity): End-to-end
block_suggestiontest confirms the adapter emitsoption_groups(not bothoption_groupsANDoptions). Slack's mutual-exclusivity spec is enforced. - Empty result
[]: Falls through to{"options": []}(correct) rather than{"option_groups": []}. Thelen(result) > 0short-circuit is the right guard. - Group-shape detection: Adds defensive
isinstance(result[0].get("options"), list)beyond upstream'sArray.isArraycheck — slightly stricter, defeats malformed inputs like{"options": "string"}. Good Python-side hardening. - Slack caps: 100 groups × 100 options × 75-char label all applied correctly; 75-char truncation tested with
"x" * 200 → "x" * 75. - Shared helper extraction:
_select_option_to_slackmirrors upstream'sselectOptionToSlackOptionfactoring; description round-trips through both flat and grouped paths. - TS test fidelity: All 3 upstream
it()blocks (should create with required fields,should include optional fields,converts external select with placeholder and min query length,converts external select with initialOption,handles block_suggestion with option_groups response,should support returning option groups) have direct Python ports. - Exports:
ExternalSelect,ExternalSelectElement,external_select,OptionsLoadGroup,OptionsLoadResultall wired through__init__.py__all__.
Net: clean, faithful port. The single nit on if initial_option: is the only place where Python truthiness diverges subtly from TS — worth tightening to is not None for symmetry with min_query_length, but not blocking.
Posted by an automated reviewer agent. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code
…ct renderer Address review on PR #84 (modals.py:209). The TS expression ``if (select.initialOption)`` only filters null/undefined since ``{}`` is truthy in JS. Python ``if initial_option:`` falsely drops a hand-constructed ``initial_option={}`` because empty dicts are falsy. Switch to ``is not None`` for parity with TS and consistency with the ``min_query_length is not None`` check three lines above. Adds test_external_select_initial_option_empty_dict_renders regression test that fails before the fix. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
patrick-chinchill
left a comment
There was a problem hiding this comment.
Re-review (post-fix verification + fresh upstream-parity sweep)
Verification of the pushed fix
Confirmed 99a52a4 (adapters/slack/modals.py:208-220) replaces the truthiness check with if initial_option is not None:, matching the TS if (select.initialOption) semantic where {} is truthy in JS but falsy in Python. Verified the new regression test test_external_select_initial_option_empty_dict_renders actually catches the regression by reverting modals.py to commit 2c00393:
- Before fix:
FAILED ... assert 'initial_option' in {'action_id': 'person', 'type': 'external_select'} - After fix:
7 passedacrossTestModalExternalSelect
The test message is excellent (it names the file, function, and the exact remediation). Full affected suite: 204 passed / 2 skipped.
Fresh parity sweep against chat@4.27.0 (f55378a)
Walked the diff against packages/chat/src/{modals,types}.ts, packages/adapter-slack/src/{modals,index}.ts plus their .test.ts siblings.
- Type shape —
ExternalSelectElement: TS has{ id, initialOption, label, minQueryLength, optional, placeholder, type }; Python TypedDict has the same set with snake_case translation. ✅ OptionsLoadResult/OptionsLoadGroup: shape and union match upstream verbatim (SelectOptionElement[] | OptionsLoadGroup[]). ✅- Wire-format mutual exclusivity (
option_groupsvsoptions): TS does not enforce — it picks viaisGroups = result.length > 0 && "options" in result[0] && Array.isArray(...)and emits exactly one key. Python mirrors this branch identically (adapter.py:1117-1147), andtest_option_groups_and_options_are_mutually_exclusivepins it. ✅ - Group/option caps and label truncation: 100 groups, 100 options/group, 75-char group label — all match upstream
slicecalls; covered bytest_option_groups_limits_to_100_groups_and_100_optionsandtest_option_groups_label_truncated_to_75_chars. ✅ - Test fidelity: both upstream
it()s inchat/src/modals.test.ts > ExternalSelect(should create with required fields,should include optional fields) ported. Both upstreamit()s inadapter-slack/src/modals.test.ts(converts external select with placeholder and min query length,converts external select with initialOption) ported, with identical assertion shapes. The newchat.test.ts > should support returning option groups(vercel/chat#410) is ported intest_chat_faithful.py. Theadapter-slack/src/index.test.ts > handles block_suggestion with option_groups responseis ported intest_options_load.pywith the exact same expected JSON. ✅ fromReactModalElementExternalSelect case (modals.test.ts:268): intentionally not ported — covered by the documented "JSX Card/Modal elements" non-parity row indocs/UPSTREAM_SYNC.md:473. ✅- Hazard #1 (truthiness traps): the patch correctly uses
is not Nonefor bothmin_query_lengthandinitial_option.placeholderuses truthiness, but TS does too (if (select.placeholder)), and empty-string is falsy in both languages — match. ✅ - Backward compat:
OptionsLoadHandler's return type widens fromlist[SelectOptionElement] | NonetoOptionsLoadResult | None(a strict superset), so existing handlers stay valid. ✅
Findings
🔵 Nit (pre-existing, not a blocker): _select_to_block (modals.py:148-157) inlines the option-conversion logic that _external_select_to_block and SlackAdapter._select_option_to_slack now share. Upstream consolidates everything to selectOptionToSlackOption. Worth a follow-up to dedupe, but out of scope for this PR.
✅ Everything else looks good — no remaining parity gaps, no boundary cases missed.
Re-review verdict: PASS
Posted by an automated re-reviewer agent. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code
Final upstream-coverage audit before merging the 7 sync PRs (#84-#90) identified one undocumented N/A item: vercel/chat#415 (Teams SDK 2.0.8 + User-Agent) is a JS-only botbuilder dependency bump. The Python Teams adapter uses raw aiohttp (no botbuilder), so there is no equivalent dependency to bump. The optional User-Agent: Vercel.ChatSDK header on the ~9 outbound aiohttp call sites is a defense-in-depth nice-to-have; deferred as a follow-up rather than landed in this sync. Updates: - CHANGELOG.md: tick all completed items and link them to their PRs (#84, #85, #86, #87, #88, #89, #90, plus already-merged PR #74). Document #415 inline as N/A. - docs/UPSTREAM_SYNC.md non-parity table: add row for Teams User-Agent header divergence so future syncers don't try to "port" the JS bump. Item #6 (concurrency.maxConcurrent) is already implementation-covered in the Python port (existing divergence row at L492). The 4 new TS concurrency tests in chat.test.ts have Python-specific equivalents at test_chat_faithful.py L2969-3055 that don't name-match — leaving as deferred fidelity-baseline polish since the behavior is verified. Verdict from the coverage audit: all 18 substantive ports across PRs #84-#90 are upstream-verified. No commits in chat@4.26.0..f55378a were missed. Ready to start merging. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Summary
Ports two co-dependent upstream PRs that together complete
ExternalSelect:ExternalSelectElementand theblock_suggestion/onOptionsLoadruntime. The runtime half landed in feat(chat): port onOptionsLoad handler (#50) #66, but theExternalSelectElementmodal-element type itself was deferred. This PR closes that gap (ExternalSelectElementTypedDict,ExternalSelectbuilder,_external_select_to_blockSlack renderer,external_selectadded toVALID_MODAL_CHILD_TYPES).initialOption(pre-selected option object) andoption_groups(labeled sections of options). Slack response shape:option_groupsandoptionsare mutually exclusive per Slack's spec; max 100 groups x 100 options; group label truncated to 75 chars.The handler return type widens to
OptionsLoadResult = list[SelectOptionElement] | list[OptionsLoadGroup]. The Slack adapter detects grouped form via"options" in result[0](matching upstream) and emits{"option_groups": [...]}instead of{"options": [...]}.What landed
src/chat_sdk/modals.py,src/chat_sdk/types.py):ExternalSelectElementTypedDict +ExternalSelectbuilder +external_selectsnake-case aliasOptionsLoadGroupTypedDictOptionsLoadResulttype alias (widened fromlist[SelectOptionElement])external_selectadded toVALID_MODAL_CHILD_TYPESOptionsLoadHandler,Chat.process_options_load,ChatInstance.process_options_loadreturn typessrc/chat_sdk/adapters/slack/modals.py,src/chat_sdk/adapters/slack/adapter.py):_external_select_to_blockrendersexternal_selectelement withplaceholder,min_query_length,initial_option(full{label, value, description?}object — the loader hasn't run yet so a value string would be ambiguous)_options_load_responsenow branches on the result shape and emitsoption_groupswhen grouped, with the Slack 100/100/75 caps applied;_select_option_to_slackextracted to share between the flat and grouped pathssrc/chat_sdk/__init__.py):ExternalSelect,ExternalSelectElement,external_select,OptionsLoadGroup,OptionsLoadResultTests added (18 new)
tests/test_modals.py(TestExternalSelectBuilder, 4 tests) — ports upstreampackages/chat/src/modals.test.tsfaithfully ("should create with required fields", "should include optional fields") plus Python-specificinitial_optionand hazard-security: Fix all critical and high findings from security audit #1/fix: launch must-fix items — security, perf, docs #7 coveragetests/test_modals.py— extendedfilter_modal_childrentest to includeexternal_selecttests/test_slack_modals.py(TestModalExternalSelect, 6 tests) — ports upstreampackages/adapter-slack/src/modals.test.tsfaithfully ("converts external select with placeholder and min query length", "converts external select with initialOption") plus omit-when-unset, min_query_length=0, description round-trip, optionaltests/test_options_load.py(5 new tests inTestSlackBlockSuggestion) — ports upstreampackages/adapter-slack/src/index.test.ts"handles block_suggestion with option_groups response", plus mutual-exclusivity, 75-char label truncation, 100x100 group/option caps, empty-result rendering, description round-triptests/test_chat_faithful.py— addedtest_should_support_returning_option_groups(faithful port of upstreamchat.test.ts"should support returning option groups")Hazards considered
min_query_length=0is preserved (Slack-meaningful — fire on every keystroke). Code usesis not None, notif min_query_length. Regression test included.initial_option,min_query_length,option_groups. Slack output keys are exactly the Slack Block Kit identifiers (initial_option,min_query_length,option_groups).null. Tests assert"initial_option" not in eletc.external_selectblock and asserts the complete Block Kit JSON; adapter-level tests exercise theblock_suggestionround-trip end-to-end.Adversarial reviewer notes
is_groupsrequireslen(result) > 0so[]falls through to the flat-options path (renders as{"options": []}, which is what Slack expects for "no results"). Test included."options" in result[0]. A flat option that happens to carry an extraoptionskey would be misclassified; this matches upstream TS behavior and is acceptable sinceSelectOptionElementdoesn't define anoptionskey.initial_optiondescription round-trips through the renderer (covered).Test plan
uv run ruff check src/ tests/ scripts/uv run ruff format --check src/ tests/ scripts/uv run python scripts/audit_test_quality.py(0 hard failures)uv run pytest tests/ --tb=short -q— 3686 passed, 2 skipped, 1 pre-existing failure (tests/test_github_webhook.py::TestGitHubAdapterConstructor::test_throws_when_no_auth)TS_ROOT=/tmp/vercel-chat uv run python scripts/verify_test_fidelity.py— no new fidelity gaps; new TS test "should support returning option groups" matchedUpstream refs: vercel/chat#410, vercel/chat#397
https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code