feat(chat): port onOptionsLoad handler (#50)#66
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds external-select options-load support: new Changes
Sequence Diagram(s)sequenceDiagram
participant Slack as Slack Client
participant Adapter as SlackAdapter
participant Chat as Chat Instance
participant Handler as Options Handler
Slack->>Adapter: POST interactive (block_suggestion) (action_id, value, user)
Adapter->>Adapter: Parse payload → OptionsLoadEvent
Adapter->>Chat: process_options_load(event) [await wait_for(2.5s) + shield]
Chat->>Chat: Match handlers (action-specific → catch-all)
Chat->>Handler: Invoke handler(s) until non-None result
Handler-->>Chat: list<SelectOptionElement> or None
Chat-->>Adapter: options list or None
Adapter->>Adapter: Serialize up to 100 items → {"options":[...]}
Adapter-->>Slack: HTTP 200 {"options":[...]} (or {"options":[]} on timeout/error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| ) -> Awaitable[ModalResponse | None]: ... | ||
| def process_options_load( | ||
| self, event: Any, options: WebhookOptions | None = None | ||
| ) -> Awaitable[list[SelectOptionElement] | None]: ... |
There was a problem hiding this comment.
False positive — standard typing.Protocol method body per PEP 544. Consistent with every other Protocol method in this file.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for handling dynamic option loading for external selects, specifically porting Slack's block_suggestion dispatch logic. It introduces Chat.on_options_load and Chat.process_options_load to manage these events, allowing for both specific action ID handlers and catch-all handlers. The Slack adapter implementation includes a 2.5-second timeout budget and uses asyncio.shield to ensure that orphaned tasks can still log errors. Feedback was provided regarding the logic in process_options_load, suggesting that checking for is not None instead of truthiness would allow handlers to explicitly return an empty list and stop further processing.
| if result: | ||
| return result |
There was a problem hiding this comment.
The check if result: will skip handlers that return an empty list []. If the intention is to allow a handler to explicitly return an empty list of options and stop the search (meaning "I handled this ID and there are no matches"), this should be if result is not None:. However, if the goal is to find the first handler that actually provides options, the current implementation is correct. Given the description "first non-empty result wins", this seems intended, but it's worth noting that a specific handler returning [] will fall through to catch-all handlers.
There was a problem hiding this comment.
Addressed in 3fedbbc — if result is not None: now correctly handles handlers returning [] as an explicit "handled, no options" short-circuit, matching upstream TS if (options) where [] is truthy. See thread.py:951 docstring.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 186148aa94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
|
|
||
| load_task.add_done_callback(_late_error) | ||
| _pin_task(load_task) |
There was a problem hiding this comment.
When block_suggestion times out, the handler task is only pinned in-process and immediately returned, but it is never passed to WebhookOptions.wait_until. In runtimes that end request work unless tasks are explicitly registered (the same pattern already used elsewhere in this adapter), the timed-out process_options_load task can be terminated before completion, so the intended late-error logging path may never run. This breaks the commit’s stated timeout behavior specifically in webhook environments that rely on wait_until for background work.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/chat_sdk/chat.py (1)
87-87: NarrowOptionsLoadHandlerreturn type instead ofAny.A stricter alias improves static checks and prevents accidental non-option return values from propagating.
♻️ Proposed type narrowing
-OptionsLoadHandler = Callable[[OptionsLoadEvent], Any] +OptionsLoadHandler = Callable[ + [OptionsLoadEvent], + Awaitable[list[SelectOptionElement] | None] | list[SelectOptionElement] | None, +]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chat_sdk/chat.py` at line 87, The alias OptionsLoadHandler currently returns Any which is too permissive; change it to a stricter return type (e.g., None | OptionsType | Coroutine[Any, Any, OptionsType] depending on async usage) so handlers can only return option objects or None. Update the OptionsLoadHandler declaration to use the concrete OptionsType (or Optional[OptionsType] / Awaitable[OptionsType]) and adjust any handler implementations and call-sites expecting Any to match the new signature; reference the OptionsLoadHandler and OptionsLoadEvent types when making this change so static checkers validate handler returns correctly.src/chat_sdk/types.py (1)
1368-1370: Use a typed event inprocess_options_loadprotocol signature.
event: Anyweakens the contract for thisprocess_*method and makes incorrect payload shapes easier to pass undetected.♻️ Proposed typing fix
def process_options_load( - self, event: Any, options: WebhookOptions | None = None + self, event: OptionsLoadEvent, options: WebhookOptions | None = None ) -> Awaitable[list[SelectOptionElement] | None]: ...As per coding guidelines "Use typed dataclasses instead of raw dicts when passing data to
process_*methods."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chat_sdk/types.py` around lines 1368 - 1370, The protocol method process_options_load currently accepts event: Any which weakens type safety; replace Any with a specific typed dataclass (e.g., OptionsLoadEvent or OptionsLoadPayload) and update the protocol signature to def process_options_load(self, event: OptionsLoadEvent, options: WebhookOptions | None = None) -> Awaitable[list[SelectOptionElement] | None]:, add/import that dataclass in the module, and update any callers/tests to construct and pass the typed dataclass instead of raw dicts so payload shapes are enforced at compile-time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chat_sdk/adapters/slack/adapter.py`:
- Around line 1031-1037: Replace truthy `or` fallbacks with explicit None
checks: for user_name and full_name, retrieve username/name into temporaries and
assign using the `x if x is not None else y` pattern so falsy-but-valid values
(e.g. empty string or 0) are preserved; likewise replace payload.get("value") or
"" passed into OptionsLoadEvent.query with `val if val is not None else ""`
where val = payload.get("value"); apply the same change to the other occurrence
noted (the variable named result around the referenced location) and any similar
uses in this adapter so all fallbacks use explicit `is not None` coalescing
rather than `or`.
In `@tests/test_options_load.py`:
- Around line 87-88: The test helper currently builds the request body as raw
JSON with body = f"payload={json.dumps(payload)}", which can break when the JSON
contains & or =; update the helper so the JSON payload is URL-encoded before
building the body (use urllib.parse.quote_plus or urllib.parse.urlencode on the
payload JSON) and then call _make_signed_request(body) as before; reference the
body construction and the _make_signed_request call to locate and change the
code.
---
Nitpick comments:
In `@src/chat_sdk/chat.py`:
- Line 87: The alias OptionsLoadHandler currently returns Any which is too
permissive; change it to a stricter return type (e.g., None | OptionsType |
Coroutine[Any, Any, OptionsType] depending on async usage) so handlers can only
return option objects or None. Update the OptionsLoadHandler declaration to use
the concrete OptionsType (or Optional[OptionsType] / Awaitable[OptionsType]) and
adjust any handler implementations and call-sites expecting Any to match the new
signature; reference the OptionsLoadHandler and OptionsLoadEvent types when
making this change so static checkers validate handler returns correctly.
In `@src/chat_sdk/types.py`:
- Around line 1368-1370: The protocol method process_options_load currently
accepts event: Any which weakens type safety; replace Any with a specific typed
dataclass (e.g., OptionsLoadEvent or OptionsLoadPayload) and update the protocol
signature to def process_options_load(self, event: OptionsLoadEvent, options:
WebhookOptions | None = None) -> Awaitable[list[SelectOptionElement] | None]:,
add/import that dataclass in the module, and update any callers/tests to
construct and pass the typed dataclass instead of raw dicts so payload shapes
are enforced at compile-time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac0d3d01-1422-4d74-94bf-48b827af46a0
📒 Files selected for processing (7)
CHANGELOG.mdsrc/chat_sdk/__init__.pysrc/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/chat.pysrc/chat_sdk/types.pytests/test_chat_faithful.pytests/test_options_load.py
- process_options_load: switch to `is not None` check so handlers returning [] short-circuit correctly (Port Rule #1 truthiness trap) - Slack adapter: replace `or ""` fallbacks in OptionsLoadEvent construction with explicit None checks - test helper: URL-encode payload JSON in signed-request body - Narrow OptionsLoadHandler return type and typed Protocol signature for process_options_load (replace Any with OptionsLoadEvent) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_until Addresses Codex P2 on PR #66. When the 2.5s options-load budget fires, the shielded handler task kept running in-process (pinned against GC and wired to a late-error logging callback) but was never handed off to `WebhookOptions.wait_until`. On serverless runtimes that end request work unless tasks are registered — the same pattern other handlers in this adapter already use — the orphan could be terminated before its late-error path ran, silently dropping the commit's stated late-error logging guarantee. Registers the pinned task with `options.wait_until` when the timeout path is taken and adds a regression test that asserts the task handed to `wait_until` is still live at return time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chat_sdk/adapters/slack/adapter.py`:
- Around line 1081-1083: The options.wait_until callback invocation is unguarded
and can throw, causing the timeout path to fail and not return the empty-options
response; wrap the call to options.wait_until(load_task) in a try/except that
catches any exception from the user-provided callback (preferably logging it)
and then proceed to return self._options_load_response([]) regardless, ensuring
failures in options.wait_until (referenced symbol: options.wait_until, parameter
load_task) cannot prevent the fallback response from _options_load_response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5458c149-b9af-40eb-914d-88661452fb98
📒 Files selected for processing (4)
src/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/chat.pysrc/chat_sdk/types.pytests/test_options_load.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/chat_sdk/types.py
- src/chat_sdk/chat.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_options_load.py`:
- Around line 149-159: The test leaves a 5s background task running; shorten the
sleep inside _slow_handler (e.g., to ~0.1s) so it doesn't tie up teardown and
ensure the test waits for completion by awaiting slow_done.wait() in the test's
finally block; apply the same change to the other slow handler instance
referenced around lines 191-193 so both slow_done and _slow_handler are cleaned
up deterministically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0d53048-0a32-434c-8fed-df8120f49142
📒 Files selected for processing (3)
src/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/chat.pytests/test_options_load.py
|
|
||
| # Patch the module-level timeout so the test runs quickly (real | ||
| # 2.5s budget is exercised in fidelity-level production code). | ||
| import chat_sdk.adapters.slack.adapter as slack_adapter_mod |
There was a problem hiding this comment.
Duplicate of earlier import-style nit — deferred. Style opinion, not enforced by ruff/pyrefly; the suggested sys.modules workaround is strictly uglier.
|
|
||
| options = WebhookOptions(wait_until=_wait_until) | ||
|
|
||
| import chat_sdk.adapters.slack.adapter as slack_adapter_mod |
There was a problem hiding this comment.
Duplicate of earlier import-style nit — deferred. Style opinion, not enforced by ruff/pyrefly; the suggested sys.modules workaround is strictly uglier.
|
|
||
| options = WebhookOptions(wait_until=_raising_wait_until) | ||
|
|
||
| import chat_sdk.adapters.slack.adapter as slack_adapter_mod |
There was a problem hiding this comment.
Duplicate of earlier import-style nit — deferred. Style opinion, not enforced by ruff/pyrefly; the suggested sys.modules workaround is strictly uglier.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_options_load.py (1)
221-223:⚠️ Potential issue | 🟡 MinorAwait cancelled timeout tasks for deterministic teardown.
Line 222 and Line 282 still use 5s sleeps, and cleanup at Line 264-267 and Line 327-329 only calls
.cancel()without awaiting completion. That can leak pending tasks into adjacent async tests.Proposed patch
@@ async def _slow_handler(event: Any, options: Any = None): - await asyncio.sleep(5.0) + await asyncio.sleep(0.2) return [] @@ finally: slack_adapter_mod.OPTIONS_LOAD_TIMEOUT_MS = original_timeout # Cancel the still-running slow task so it doesn't leak. if registered and not registered[0].done(): registered[0].cancel() + await asyncio.gather(registered[0], return_exceptions=True) @@ async def _slow_handler(event: Any, options: Any = None): - await asyncio.sleep(5.0) + await asyncio.sleep(0.2) return [] @@ finally: slack_adapter_mod.OPTIONS_LOAD_TIMEOUT_MS = original_timeout if slow_task_ref and not slow_task_ref[0].done(): slow_task_ref[0].cancel() + await asyncio.gather(slow_task_ref[0], return_exceptions=True)As per coding guidelines,
**/*.py: “Every async call must be awaited.”Also applies to: 264-267, 281-283, 327-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_options_load.py` around lines 221 - 223, The test defines a long-running async helper _slow_handler (await asyncio.sleep(5.0)) and several teardown blocks simply call .cancel() on running tasks which can leak pending tasks; replace the long 5.0s sleeps with a much shorter sleep (e.g. 0.1s) for speed and, in every place that calls task.cancel() (the cleanup code referenced around the handlers), immediately await the cancelled task inside a try/except catching asyncio.CancelledError (or use asyncio.wait_for) to ensure the cancellation completes before test exit so no pending tasks leak into adjacent async tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_options_load.py`:
- Around line 221-223: The test defines a long-running async helper
_slow_handler (await asyncio.sleep(5.0)) and several teardown blocks simply call
.cancel() on running tasks which can leak pending tasks; replace the long 5.0s
sleeps with a much shorter sleep (e.g. 0.1s) for speed and, in every place that
calls task.cancel() (the cleanup code referenced around the handlers),
immediately await the cancelled task inside a try/except catching
asyncio.CancelledError (or use asyncio.wait_for) to ensure the cancellation
completes before test exit so no pending tasks leak into adjacent async tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c7a022fb-d1d6-45db-9537-6e1fca4b5206
📒 Files selected for processing (1)
tests/test_options_load.py
|
Review verdict: needs follow-up. Refreshed and reviewed latest head 4cfa184 against main. The options-load behavior now matches upstream parity, including explicit empty-list results short-circuiting catch-all handlers. Remaining issue: src/chat_sdk/adapters/slack/adapter.py line 1058 introduces asyncio.ensure_future inside the async block_suggestion path, which violates this repo async rule. Use asyncio.get_running_loop().create_task(self._chat.process_options_load(event, options)) instead. Verification: tests/test_options_load.py plus tests/test_chat_faithful.py passed (105 passed, 2 skipped), and ruff passed. |
Adds `Chat.on_options_load(...)` + `Chat.process_options_load(...)` mirroring upstream's external-select suggestion lookup, and wires the Slack adapter to route `block_suggestion` payloads through it — returning a Slack-shaped options JSON response and falling back to empty options (with a shielded background task that still logs late errors) after the 2.5s `OPTIONS_LOAD_TIMEOUT_MS` budget. Closes 4 [Options Load] fidelity gaps in chat.test.ts. Refs #50. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- process_options_load: switch to `is not None` check so handlers returning [] short-circuit correctly (Port Rule #1 truthiness trap) - Slack adapter: replace `or ""` fallbacks in OptionsLoadEvent construction with explicit None checks - test helper: URL-encode payload JSON in signed-request body - Narrow OptionsLoadHandler return type and typed Protocol signature for process_options_load (replace Any with OptionsLoadEvent) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_until Addresses Codex P2 on PR #66. When the 2.5s options-load budget fires, the shielded handler task kept running in-process (pinned against GC and wired to a late-error logging callback) but was never handed off to `WebhookOptions.wait_until`. On serverless runtimes that end request work unless tasks are registered — the same pattern other handlers in this adapter already use — the orphan could be terminated before its late-error path ran, silently dropping the commit's stated late-error logging guarantee. Registers the pinned task with `options.wait_until` when the timeout path is taken and adds a regression test that asserts the task handed to `wait_until` is still live at return time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…returns fallback
wait_until is user/runtime-provided; if it raises, the timeout branch
would fail the whole webhook instead of returning the empty-options
HTTP 200 fallback. Wrap the call in try/except with a warn log matching
the adjacent late-error callback's logger pattern, and add a regression
test that supplies a raising wait_until and asserts the handler still
returns status=200 with parsed body == {"options": []}.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… parity) Docstring said "first non-empty result wins" but the code (and upstream TS) actually short-circuits on any non-None result — including []. A handler returning [] is an explicit "I handled this action, show no options", and blocks catch-all handlers by design. Matches upstream chat.ts:920-923 where if(options) is truthy for []. No behavior change; docstring + PR body wording only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Shortens the mock slow handler and awaits its task in finally so the 5s sleep doesn't linger and flake adjacent async tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-2 fixup over-applied Port Rule #1. Upstream JS uses || chain intentionally — empty-string username falls through to name/user_id. The is-not-None chain preserved "" and diverged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
patrick-chinchill
force-pushed
the
feat/options-load-50
branch
from
4cfa184 to
1776b7d
Compare
|
Noted from comment 4311953884 — |
Per gemini-code-assist review on PR #83. Without the repo prefix, GitHub auto-links the upstream PR numbers to local PRs in chat-sdk-python, which collides with the local refs (#64, #66, #67, #74, #82) elsewhere in the file. Use vercel/chat#NNN so the upstream refs link correctly. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
1 participant
Summary
onOptionsLoad+processOptionsLoadfrom upstream Vercel Chat: specific action IDs match before catch-all handlers, errors log-and-skip so later handlers still run, first handler result that isn'tNonewins (including[], which short-circuits — matches upstream TSif (options)where arrays are truthy).block_suggestioninteractive payload toprocess_options_loadand serializes results to{options: [...]}JSON.OPTIONS_LOAD_TIMEOUT_MS = 2500verbatim usingasyncio.wait_for+asyncio.shieldso the orphaned handler task still logs late errors when the budget is exceeded.OptionsLoadEvent,OptionsLoadHandler.Refs #50. Queued under
## Unreleasedfor bundled0.4.26.2release.Fidelity impact
Closed 4
[Options Load]gaps inchat.test.ts.Test plan
Notes
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation