fix: use plugin server URL and add question fallback#86
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughAdds explicit v2 client config derivation from the server URL, introduces pending-question tracking and reply resolution, and routes eligible text messages through the pending-question resolver before normal queue handling. ChangesV2 Client Config Derivation
Pending Question Reply Flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SessionQueue
participant PendingQuestions
participant FeishuClient
participant OpencodeClient
SessionQueue->>PendingQuestions: tryResolvePendingQuestionText(chatId, content)
PendingQuestions->>PendingQuestions: parsePendingQuestionReply
PendingQuestions->>OpencodeClient: question.reply(answers)
PendingQuestions->>FeishuClient: send confirmation or retry message
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 introduces a helper function buildV2ClientConfig to configure the OpenCode v2 client using the dynamic server URL (ctx.serverUrl) provided by the plugin host, along with regression tests. The review feedback suggests making serverUrl optional in this helper to ensure backward compatibility and prevent potential runtime crashes if the host CLI does not provide a server URL.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/index-v2-client.test.ts (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the test file to map to
src/index.ts.The helper under test lives in
src/index.ts, so this should be placed attests/index.test.tsor another path that mirrors the source path. As per coding guidelines, “Test file paths must map to corresponding source file paths.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/index-v2-client.test.ts` around lines 1 - 10, The test file location does not mirror the source module it covers, so rename this regression test to match src/index.ts (for example, tests/index.test.ts) and keep buildV2ClientConfig as the symbol under test. Update any referenced test path in the header comment or run instructions so the test path aligns with the source path mapping guideline.Source: Coding guidelines
src/index.ts (1)
88-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the exported helper input shape into an interface.
buildV2ClientConfigexposes an object-shaped parameter inline; define a small interface to align with the TypeScript style rule.Proposed refactor
+interface BuildV2ClientConfigInput { + serverUrl: URL + directory: string +} + -export function buildV2ClientConfig(input: { serverUrl: URL; directory: string }): NonNullable<Parameters<typeof createOpencodeClient>[0]> { +export function buildV2ClientConfig(input: BuildV2ClientConfigInput): NonNullable<Parameters<typeof createOpencodeClient>[0]> {As per coding guidelines, “Prefer TypeScript
interfacefor defining object shapes.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` at line 88, The exported helper buildV2ClientConfig currently uses an inline object type for its input parameter, which should be extracted to a named interface to match the TypeScript style rule. Define a small interface for the input shape used by buildV2ClientConfig, then update the function signature to reference that interface instead of the inline object type, keeping the existing serverUrl and directory fields intact.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/index.ts`:
- Line 88: The exported helper buildV2ClientConfig currently uses an inline
object type for its input parameter, which should be extracted to a named
interface to match the TypeScript style rule. Define a small interface for the
input shape used by buildV2ClientConfig, then update the function signature to
reference that interface instead of the inline object type, keeping the existing
serverUrl and directory fields intact.
In `@tests/index-v2-client.test.ts`:
- Around line 1-10: The test file location does not mirror the source module it
covers, so rename this regression test to match src/index.ts (for example,
tests/index.test.ts) and keep buildV2ClientConfig as the symbol under test.
Update any referenced test path in the header comment or run instructions so the
test path aligns with the source path mapping guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed4c4ca6-e13c-41f2-939f-f3e194ab38a2
📒 Files selected for processing (2)
src/index.tstests/index-v2-client.test.ts
chenhaipeng
changed the title
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/handler/pending-questions.ts (1)
71-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for
parsePendingQuestionReply.This function drives real user-facing answer submission (numeric selection, continue/cancel keyword matching, out-of-range handling). It's pure and easily testable but currently has no test coverage in the reviewed files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/handler/pending-questions.ts` around lines 71 - 108, Add unit tests for parsePendingQuestionReply to cover its user-facing parsing paths, including numeric selection, continue/cancel keyword matching, and out-of-range or invalid input handling. Use parsePendingQuestionReply together with normalizeOptions behavior through a minimal QuestionRequest fixture so the tests verify returned answers and displayText for valid selections and undefined for unsupported input. Include cases for empty trimmed content, a valid numeric index, a numeric value beyond options length, continue text resolving via isContinueText and findOption, and cancel text resolving via isCancelText.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/handler/interactive.ts`:
- Around line 442-477: The pending question is only being unmarked on delivery
failure, but the pending entry itself is never cleared when both the interactive
card and the fallback text fail or when the async send flow throws. Update the
error handling in interactive.ts around registerPendingQuestion,
sendInteractiveCard, sendTextMessage, and the IIFE catch so that
clearPendingQuestionByRequestId(requestId) is invoked in every full-failure
path, alongside unmarkSeen(requestId), to avoid leaving stale pending state
behind.
In `@src/handler/pending-questions.ts`:
- Around line 28-52: `registerPendingQuestion` and
`clearPendingQuestionByRequestId` can let an old requestId delete a newer
pending question for the same chat. Update the
`pendingByChatId`/`chatIdByRequestId` bookkeeping so each chat only has one
active requestId mapping at a time, and before clearing in
`clearPendingQuestionByRequestId` verify the stored `PendingQuestion.requestId`
still matches the requestId being cleared. Use the existing `PendingQuestion`,
`registerPendingQuestion`, and `clearPendingQuestionByRequestId` symbols to make
the stale-entry cleanup safe without removing a newer pending question.
In `@src/handler/session-queue.ts`:
- Around line 62-75: The pending-question fast path in enqueueMessage bypasses
the per-sessionKey FIFO serialization and can double-submit the same answer.
Update tryResolvePendingQuestionText to claim the pending entry or add a
markSeen/unmarkSeen-style guard keyed by requestId before awaiting
v2Client.question.reply, so concurrent calls for the same chat cannot both
resolve the same pending question. Keep the fix aligned with the existing
session-queue and pending-questions flow so the fast path remains safe under
races.
---
Nitpick comments:
In `@src/handler/pending-questions.ts`:
- Around line 71-108: Add unit tests for parsePendingQuestionReply to cover its
user-facing parsing paths, including numeric selection, continue/cancel keyword
matching, and out-of-range or invalid input handling. Use
parsePendingQuestionReply together with normalizeOptions behavior through a
minimal QuestionRequest fixture so the tests verify returned answers and
displayText for valid selections and undefined for unsupported input. Include
cases for empty trimmed content, a valid numeric index, a numeric value beyond
options length, continue text resolving via isContinueText and findOption, and
cancel text resolving via isCancelText.
🪄 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
Run ID: 0197362e-a364-439a-b132-5ece398bc9a7
📒 Files selected for processing (3)
src/handler/interactive.tssrc/handler/pending-questions.tssrc/handler/session-queue.ts
| void (async () => { | ||
| const res = await sender.sendInteractiveCard( | ||
| deps.feishuClient, | ||
| chatId, | ||
| buildQuestionCardDSL(request, chatId, chatType, sessionId), | ||
| deps.log, | ||
| ) | ||
| if (res.ok) return | ||
|
|
||
| deps.log("error", "发送问答卡片失败，回退纯文本问题", { | ||
| requestId, | ||
| chatId, | ||
| error: res.error ?? "unknown", | ||
| }) | ||
| const fallback = await sender.sendTextMessage( | ||
| deps.feishuClient, | ||
| chatId, | ||
| buildQuestionFallbackText(request), | ||
| deps.log, | ||
| ) | ||
| if (!fallback.ok) { | ||
| unmarkSeen(requestId) | ||
| deps.log("error", "发送问答纯文本 fallback 失败", { | ||
| requestId, | ||
| chatId, | ||
| error: fallback.error ?? "unknown", | ||
| }) | ||
| } | ||
| })().catch((err) => { | ||
| unmarkSeen(requestId) | ||
| deps.log("error", "发送问答卡片失败", { | ||
| requestId, | ||
| chatId, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pending question isn't cleared when both card and fallback text delivery fail.
registerPendingQuestion runs at Line 440 before any delivery attempt. If the interactive card fails (Line 449) and the fallback text also fails (Line 462), or the whole IIFE throws (Line 470), only unmarkSeen(requestId) is called — clearPendingQuestionByRequestId(requestId) is never invoked. The pending entry then lingers (up to the 10-minute TTL) for a question the user never actually saw, so an unrelated later text message in that chat can be misinterpreted as an answer to it.
🐛 Clear pending state on full delivery failure
if (!fallback.ok) {
unmarkSeen(requestId)
+ clearPendingQuestionByRequestId(requestId)
deps.log("error", "发送问答纯文本 fallback 失败", {
requestId,
chatId,
error: fallback.error ?? "unknown",
})
}
})().catch((err) => {
unmarkSeen(requestId)
+ clearPendingQuestionByRequestId(requestId)
deps.log("error", "发送问答卡片失败", {
requestId,
chatId,
error: err instanceof Error ? err.message : String(err),
})
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void (async () => { | |
| const res = await sender.sendInteractiveCard( | |
| deps.feishuClient, | |
| chatId, | |
| buildQuestionCardDSL(request, chatId, chatType, sessionId), | |
| deps.log, | |
| ) | |
| if (res.ok) return | |
| deps.log("error", "发送问答卡片失败，回退纯文本问题", { | |
| requestId, | |
| chatId, | |
| error: res.error ?? "unknown", | |
| }) | |
| const fallback = await sender.sendTextMessage( | |
| deps.feishuClient, | |
| chatId, | |
| buildQuestionFallbackText(request), | |
| deps.log, | |
| ) | |
| if (!fallback.ok) { | |
| unmarkSeen(requestId) | |
| deps.log("error", "发送问答纯文本 fallback 失败", { | |
| requestId, | |
| chatId, | |
| error: fallback.error ?? "unknown", | |
| }) | |
| } | |
| })().catch((err) => { | |
| unmarkSeen(requestId) | |
| deps.log("error", "发送问答卡片失败", { | |
| requestId, | |
| chatId, | |
| error: err instanceof Error ? err.message : String(err), | |
| }) | |
| }) | |
| void (async () => { | |
| const res = await sender.sendInteractiveCard( | |
| deps.feishuClient, | |
| chatId, | |
| buildQuestionCardDSL(request, chatId, chatType, sessionId), | |
| deps.log, | |
| ) | |
| if (res.ok) return | |
| deps.log("error", "发送问答卡片失败，回退纯文本问题", { | |
| requestId, | |
| chatId, | |
| error: res.error ?? "unknown", | |
| }) | |
| const fallback = await sender.sendTextMessage( | |
| deps.feishuClient, | |
| chatId, | |
| buildQuestionFallbackText(request), | |
| deps.log, | |
| ) | |
| if (!fallback.ok) { | |
| unmarkSeen(requestId) | |
| clearPendingQuestionByRequestId(requestId) | |
| deps.log("error", "发送问答纯文本 fallback 失败", { | |
| requestId, | |
| chatId, | |
| error: fallback.error ?? "unknown", | |
| }) | |
| } | |
| })().catch((err) => { | |
| unmarkSeen(requestId) | |
| clearPendingQuestionByRequestId(requestId) | |
| deps.log("error", "发送问答卡片失败", { | |
| requestId, | |
| chatId, | |
| error: err instanceof Error ? err.message : String(err), | |
| }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/handler/interactive.ts` around lines 442 - 477, The pending question is
only being unmarked on delivery failure, but the pending entry itself is never
cleared when both the interactive card and the fallback text fail or when the
async send flow throws. Update the error handling in interactive.ts around
registerPendingQuestion, sendInteractiveCard, sendTextMessage, and the IIFE
catch so that clearPendingQuestionByRequestId(requestId) is invoked in every
full-failure path, alongside unmarkSeen(requestId), to avoid leaving stale
pending state behind.
| const pendingByChatId = new TtlMap<PendingQuestion>(PENDING_QUESTION_TTL_MS) | ||
| const chatIdByRequestId = new TtlMap<string>(PENDING_QUESTION_TTL_MS) | ||
|
|
||
| export function registerPendingQuestion(params: { | ||
| request: QuestionRequest | ||
| chatId: string | ||
| sessionId: string | ||
| }): void { | ||
| const requestId = String(params.request.id ?? "") | ||
| if (!requestId) return | ||
|
|
||
| pendingByChatId.set(params.chatId, { | ||
| requestId, | ||
| sessionId: params.sessionId, | ||
| chatId: params.chatId, | ||
| request: params.request, | ||
| }) | ||
| chatIdByRequestId.set(requestId, params.chatId) | ||
| } | ||
|
|
||
| export function clearPendingQuestionByRequestId(requestId: string): void { | ||
| const chatId = chatIdByRequestId.get(requestId) | ||
| if (chatId) pendingByChatId.delete(chatId) | ||
| chatIdByRequestId.delete(requestId) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Stale requestId can wipe a newer pending question for the same chat.
registerPendingQuestion overwrites pendingByChatId.set(chatId, ...) on every call for a chat, but the previous requestId → chatId entry in chatIdByRequestId is never removed. If a second question is registered for the same chat before the first is cleared (e.g. a late card callback, a retried SSE event, or a race with tryResolvePendingQuestionText), calling clearPendingQuestionByRequestId(oldRequestId) will still resolve to the same chatId and delete the current (newer) pending entry — even though the newer question's requestId doesn't match.
This can silently cancel a live pending question and, downstream in interactive.ts (Line 606) and session-queue.ts, cause replies to be dropped or misrouted.
🐛 Verify requestId ownership before clearing
export function clearPendingQuestionByRequestId(requestId: string): void {
const chatId = chatIdByRequestId.get(requestId)
- if (chatId) pendingByChatId.delete(chatId)
+ if (chatId) {
+ const current = pendingByChatId.get(chatId)
+ // Only clear if the chat's current pending entry still belongs to this requestId;
+ // otherwise a stale/late clear would wipe out a newer pending question.
+ if (current?.requestId === requestId) pendingByChatId.delete(chatId)
+ }
chatIdByRequestId.delete(requestId)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pendingByChatId = new TtlMap<PendingQuestion>(PENDING_QUESTION_TTL_MS) | |
| const chatIdByRequestId = new TtlMap<string>(PENDING_QUESTION_TTL_MS) | |
| export function registerPendingQuestion(params: { | |
| request: QuestionRequest | |
| chatId: string | |
| sessionId: string | |
| }): void { | |
| const requestId = String(params.request.id ?? "") | |
| if (!requestId) return | |
| pendingByChatId.set(params.chatId, { | |
| requestId, | |
| sessionId: params.sessionId, | |
| chatId: params.chatId, | |
| request: params.request, | |
| }) | |
| chatIdByRequestId.set(requestId, params.chatId) | |
| } | |
| export function clearPendingQuestionByRequestId(requestId: string): void { | |
| const chatId = chatIdByRequestId.get(requestId) | |
| if (chatId) pendingByChatId.delete(chatId) | |
| chatIdByRequestId.delete(requestId) | |
| } | |
| const pendingByChatId = new TtlMap<PendingQuestion>(PENDING_QUESTION_TTL_MS) | |
| const chatIdByRequestId = new TtlMap<string>(PENDING_QUESTION_TTL_MS) | |
| export function registerPendingQuestion(params: { | |
| request: QuestionRequest | |
| chatId: string | |
| sessionId: string | |
| }): void { | |
| const requestId = String(params.request.id ?? "") | |
| if (!requestId) return | |
| pendingByChatId.set(params.chatId, { | |
| requestId, | |
| sessionId: params.sessionId, | |
| chatId: params.chatId, | |
| request: params.request, | |
| }) | |
| chatIdByRequestId.set(requestId, params.chatId) | |
| } | |
| export function clearPendingQuestionByRequestId(requestId: string): void { | |
| const chatId = chatIdByRequestId.get(requestId) | |
| if (chatId) { | |
| const current = pendingByChatId.get(chatId) | |
| // Only clear if the chat's current pending entry still belongs to this requestId; | |
| // otherwise a stale/late clear would wipe out a newer pending question. | |
| if (current?.requestId === requestId) pendingByChatId.delete(chatId) | |
| } | |
| chatIdByRequestId.delete(requestId) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/handler/pending-questions.ts` around lines 28 - 52,
`registerPendingQuestion` and `clearPendingQuestionByRequestId` can let an old
requestId delete a newer pending question for the same chat. Update the
`pendingByChatId`/`chatIdByRequestId` bookkeeping so each chat only has one
active requestId mapping at a time, and before clearing in
`clearPendingQuestionByRequestId` verify the stored `PendingQuestion.requestId`
still matches the requestId being cleared. Use the existing `PendingQuestion`,
`registerPendingQuestion`, and `clearPendingQuestionByRequestId` symbols to make
the stale-entry cleanup safe without removing a newer pending question.
| export async function enqueueMessage(ctx: FeishuMessageContext, deps: ChatDeps): Promise<void> { | ||
| if ( | ||
| ctx.shouldReply && | ||
| ctx.messageType === "text" && | ||
| deps.interactiveDeps && | ||
| await tryResolvePendingQuestionText({ | ||
| chatId: ctx.chatId, | ||
| content: ctx.content, | ||
| deps: deps.interactiveDeps, | ||
| }) | ||
| ) { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Pending-question fast path bypasses per-sessionKey serialization and can double-submit an answer.
This early-return path runs tryResolvePendingQuestionText outside the FIFO queue, before handleQueuedMessage/drainLoop ever gets involved. tryResolvePendingQuestionText (see src/handler/pending-questions.ts, Lines 110-151) reads pendingByChatId.get(chatId) synchronously and only clears the entry after awaiting v2Client.question.reply(...). If two text messages for the same chat race in close together (double send, retry, multi-device), both can read the same pending entry before either clears it, resulting in two calls to v2Client.question.reply for the same requestId — a non-idempotent external write without a dedup guard.
As per path instructions, src/handler/session-queue.ts is meant to "Implement per-sessionKey FIFO queue to prevent message race conditions and ensure serial ordering," but this new fast path sits ahead of that queue and doesn't inherit its serialization guarantee.
🔒 Guard against concurrent double-submission
Reuse the existing markSeen/unmarkSeen-style dedup guard (already used in interactive.ts) keyed by the pending requestId inside tryResolvePendingQuestionText, or claim/clear the pending entry synchronously before the await on v2Client.question.reply, so a second concurrent caller for the same chat sees no pending entry to act on.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/handler/session-queue.ts` around lines 62 - 75, The pending-question fast
path in enqueueMessage bypasses the per-sessionKey FIFO serialization and can
double-submit the same answer. Update tryResolvePendingQuestionText to claim the
pending entry or add a markSeen/unmarkSeen-style guard keyed by requestId before
awaiting v2Client.question.reply, so concurrent calls for the same chat cannot
both resolve the same pending question. Keep the fix aligned with the existing
session-queue and pending-questions flow so the fast path remains safe under
races.
Source: Path instructions
2 participants
Summary
Verification