fix(prompt): add null check for input to prevent runtime errors#1
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
📝 WalkthroughWalkthroughAdded presence checks and optional chaining throughout the prompt React component to prevent operations on an uninitialized input. Guards were applied to event handlers, editor integrations, paste handling, stash/open flows, extmark sync, and ref accessors without changing public APIs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (5)
203-206: Missing null check - potential runtime error.Line 205 accesses
input.focusedwithout optional chaining, which could cause the same "Cannot read properties of null" error this PR aims to fix.🐛 Proposed fix
if (autocomplete.visible) return - if (!input.focused) return + if (!input?.focused) return // TODO: this should be its own command
446-456: Missing null checks in DialogStash callback.Lines 449-452 access
inputwithout null checks, which could cause runtime errors if the component unmounts while the dialog is open.🐛 Proposed fix
dialog.replace(() => ( <DialogStash onSelect={(entry) => { + if (!input) return input.setText(entry.input) setStore("prompt", { input: entry.input, parts: entry.parts }) restoreExtmarksFromParts(entry.parts) input.gotoBufferEnd() }} /> ))
808-817: Missing null checks in keyboard handler.Lines 809-810 access
inputwithout optional chaining or null checks, which could cause runtime errors.🐛 Proposed fix
if (keybind.match("input_clear", e) && store.prompt.input !== "") { - input.clear() - input.extmarks.clear() + input?.clear() + input?.extmarks.clear() setStore("prompt", { input: "", parts: [], })
826-837: Missing null checks for input property accesses.Lines 826, 832 access
input.visualCursor.offsetwithout null checks, which could cause runtime errors if input is null.🐛 Proposed fix
- if (e.name === "!" && input.visualCursor.offset === 0) { + if (e.name === "!" && input?.visualCursor.offset === 0) { setStore("mode", "shell") e.preventDefault() return } if (store.mode === "shell") { - if ((e.name === "backspace" && input.visualCursor.offset === 0) || e.name === "escape") { + if ((e.name === "backspace" && input?.visualCursor.offset === 0) || e.name === "escape") { setStore("mode", "normal") e.preventDefault() return
839-862: Missing null checks in history navigation.Lines 841-842, 853-854, 859-861 access
input.cursorOffset,input.plainText,input.visualCursor.visualRow, andinput.heightwithout null checks.🐛 Proposed fix
if (store.mode === "normal") autocomplete.onKeyDown(e) if (!autocomplete.visible) { if ( - (keybind.match("history_previous", e) && input.cursorOffset === 0) || - (keybind.match("history_next", e) && input.cursorOffset === input.plainText.length) + (keybind.match("history_previous", e) && input?.cursorOffset === 0) || + (keybind.match("history_next", e) && input && input.cursorOffset === input.plainText.length) ) { const direction = keybind.match("history_previous", e) ? -1 : 1 - const item = history.move(direction, input.plainText) + const item = input ? history.move(direction, input.plainText) : null if (item) { - input.setText(item.input) + input?.setText(item.input) setStore("prompt", item) setStore("mode", item.mode ?? "normal") restoreExtmarksFromParts(item.parts) e.preventDefault() - if (direction === -1) input.cursorOffset = 0 - if (direction === 1) input.cursorOffset = input.plainText.length + if (input) { + if (direction === -1) input.cursorOffset = 0 + if (direction === 1) input.cursorOffset = input.plainText.length + } } return } - if (keybind.match("history_previous", e) && input.visualCursor.visualRow === 0) input.cursorOffset = 0 - if (keybind.match("history_next", e) && input.visualCursor.visualRow === input.height - 1) - input.cursorOffset = input.plainText.length + if (keybind.match("history_previous", e) && input?.visualCursor.visualRow === 0 && input) + input.cursorOffset = 0 + if (keybind.match("history_next", e) && input?.visualCursor.visualRow === input.height - 1 && input) + input.cursorOffset = input.plainText.length }
🧹 Nitpick comments (1)
.husky/pre-push (1)
2-3: LGTM! Consider documenting the assumption.The PATH export correctly prepends Bun's binary location. This assumes Bun is installed in the default location (
~/.bun/bin). If team members use custom Bun installations (e.g., via asdf, mise, or other version managers), this may not work for them.Optional: Make the path configurable
Consider checking multiple common locations or documenting this requirement:
#!/bin/sh +# Note: Assumes Bun is installed in ~/.bun/bin +# If using a version manager, ensure ~/.bun/bin exists or modify this path export PATH="$HOME/.bun/bin:$PATH"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.husky/pre-pushpackages/opencode/src/cli/cmd/tui/component/prompt/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (2)
packages/opencode/src/cli/cmd/tui/routes/session/index.tsx (1)
input(1320-1322)packages/opencode/src/session/prompt.ts (1)
prompt(150-179)
🔇 Additional comments (13)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (13)
94-101: Good defensive coding for async operations.The optional chaining prevents crashes if the component unmounts before the setTimeout callback executes.
103-108: LGTM!Properly guards the effect from running when input is not yet initialized.
163-164: LGTM!Proper null-safe access to input methods.
250-250: LGTM!Proper null checks for async editor operations.
Also applies to: 309-309
315-318: LGTM!Safely handles focus/blur when input might not be initialized.
324-326: LGTM!The early return guard is the right pattern for functions that require input to be available.
415-416: LGTM!Proper null checks for stash operations.
Also applies to: 430-435
460-493: LGTM!The ref API properly handles null input with optional chaining and appropriate fallbacks.
519-519: LGTM!Proper null-safe handling in the submit flow.
Also applies to: 601-601, 617-617
621-623: LGTM!The early return guard prevents operations on null input.
656-658: LGTM!The early return guard prevents operations on null input.
926-932: LGTM!Proper null check for the async layout update after paste.
934-943: LGTM!Proper null checks when initializing the textarea with stored prompt data.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (3)
787-869: Fix optional chaining and input validation in history navigation and mode switching.Two issues exist in this keydown handler:
Line 843:
input?.cursorOffset === input?.plainText.lengthevaluates toundefined === undefined(true) wheninputis undefined, allowing the condition to pass. The subsequentif (!input) returnon line 845 comes too late.Lines 827, 833, 861, 865:
input?.visualCursor.offsetandinput?.visualCursor.visualRowuse incomplete optional chaining. IfvisualCursoris undefined butinputis defined, accessing.offsetor.visualRowwill throw a TypeError.Proposed fix
if (!autocomplete.visible) { if ( - (keybind.match("history_previous", e) && input?.cursorOffset === 0) || - (keybind.match("history_next", e) && input?.cursorOffset === input?.plainText.length) + input && + ((keybind.match("history_previous", e) && input.cursorOffset === 0) || + (keybind.match("history_next", e) && input.cursorOffset === input.plainText.length)) ) { - if (!input) return const direction = keybind.match("history_previous", e) ? -1 : 1 const item = history.move(direction, input.plainText) ... } - if (keybind.match("history_previous", e) && input?.visualCursor.visualRow === 0 && input) + if (keybind.match("history_previous", e) && input?.visualCursor?.visualRow === 0 && input) input.cursorOffset = 0 if ( keybind.match("history_next", e) && - input?.visualCursor.visualRow === (input?.height ?? 0) - 1 && + input?.visualCursor?.visualRow === (input?.height ?? 0) - 1 && input ) input.cursorOffset = input.plainText.length }
622-699: Add optional chaining guard forvisualCursor.offsetto match pattern used elsewhere in the file.Lines 624 and 659 access
input.visualCursor.offsetdirectly after checkingif (!input), but elsewhere (lines 827, 833, 861, 865) the same property chain uses optional chaining (input?.visualCursor.offset). Ensure defensive programming by using optional chaining consistently, or guardvisualCursorexplicitly. Note: the proposed fallback toinput.cursorOffsetmay not be semantically equivalent sincecursorOffsetis a settable property whilevisualCursor.offsetappears to be read-only.
56-60: Fix optional chaining depth ongetLayoutNode():input?.getLayoutNode()?.markDirty()Line 97:
input?.getLayoutNode().markDirty()will still throw ifgetLayoutNode()returnsnullduring unmount—the?.only guardsinput, not the method's return value. Use deep optional chaining:input?.getLayoutNode()?.markDirty().The same issue appears in the paste handler around line 300+ where a safer
if (input)guard is already in place. Apply the fix consistently throughout.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx
🔇 Additional comments (9)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (9)
103-108: Cursor color guarding looks good.
155-179: Command handlers: good defensive guards for focus/input presence.Also applies to: 203-227
250-311: “Open editor” flow: guards look good; cursorOffset update is appropriately conditional.
324-367: restoreExtmarksFromParts(): early return is the right move.
403-459: Stash flows: safe input access + consistent store reset/restore.
461-494: PromptRef surface: null-safe and behavior-preserving.
496-619: submit(): safer extmark access + clears are now crash-resistant.
941-949: Textarea ref init: good restoration guard.
932-939: Verify ifgetLayoutNode()can return null/undefined.The code at line 935 calls
input.getLayoutNode().markDirty()without guarding the result ofgetLayoutNode(). The proposed fix to use optional chaining (?.markDirty()) assumes the method can return null or undefined. However, this cannot be verified without access to the@opentui/corev0.1.70 type definitions, which are not available in the repository. Note that another usage at line ~826 in the same file also chains directly ongetLayoutNode()without guarding the result, suggesting the return type may be guaranteed non-null—but this is not definitive.
|
@coderabbitai 근데 이렇게 nullabel 처리만 하면 데이터 누락생기는거 아니야? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 37015 정확히 지적하신 부분이 맞습니다. 현재 방식은 런타임 에러는 막지만, 사용자 액션이나 데이터가 조용히 무시될 수 있는 문제가 있습니다. 데이터 누락이 발생할 수 있는 주요 시나리오🏁 Script executed: Length of output: 951 좋습니다. 구체적으로 어떤 문제가 있는지 확인했습니다: 🚨 데이터 누락 위험 지점1. 이벤트 기반 텍스트 삽입 (Line 95-99)sdk.event.on(TuiEvent.PromptAppend.type, (evt) => {
input?.insertText(evt.properties.text) // input이 null이면 텍스트 손실
setTimeout(() => {
input?.getLayoutNode().markDirty()
input?.gotoBufferEnd()
renderer.requestRender()
}, 0)
})
2. Paste 작업 (Line 623, 658)function pasteText(text: string, virtualText: string) {
if (!input) return // 붙여넣기가 조용히 실패
// ... 실제 paste 로직
}
async function pasteImage(file: {...}) {
if (!input) return // 이미지 업로드 손실
// ... 실제 image paste 로직
}
3. Stash/History 복원 (Line 430, 449)onSelect: (dialog) => {
if (!input) return // stash 복원 실패
input.setText(entry.input)
setStore("prompt", { input: entry.input, parts: entry.parts })
restoreExtmarksFromParts(entry.parts)
input.gotoBufferEnd()
}
4. Submit 시점 (Line 520)const allExtmarks = input?.extmarks.getAllForTypeId(promptPartTypeId) ?? []
💡 올바른 해결 방법단순 null check 대신 다음 접근이 필요합니다: A. 초기화 완료 보장const [inputReady, setInputReady] = createSignal(false)
// ref 콜백에서
ref={(r: TextareaRenderable) => {
input = r
setInputReady(true)
// ... 기존 초기화 로직
}}B. 작업 큐잉const pendingOperations = new Queue<() => void>()
sdk.event.on(TuiEvent.PromptAppend.type, (evt) => {
const operation = () => {
input.insertText(evt.properties.text)
// ...
}
if (!input) {
pendingOperations.enqueue(operation)
} else {
operation()
}
})
// input이 준비되면 큐 처리
createEffect(() => {
if (inputReady()) {
while (!pendingOperations.isEmpty()) {
pendingOperations.dequeue()()
}
}
})C. 사용자 피드백function pasteText(text: string, virtualText: string) {
if (!input) {
toast.show({
variant: "error",
message: "Input not ready. Please try again.",
duration: 2000
})
return
}
// ... 실제 로직
}D. 상태와 DOM 분리// store에는 항상 저장
setStore("prompt", "input", value)
setStore("prompt", "parts", [...parts, newPart])
// DOM 업데이트는 input이 있을 때만
createEffect(() => {
if (input && inputReady()) {
input.setText(store.prompt.input)
restoreExtmarksFromParts(store.prompt.parts)
}
})🔍 테스트 시나리오다음 상황에서 데이터 손실 여부를 확인해야 합니다:
현재 PR은 크래시는 막지만 이런 경우들에서 데이터가 조용히 사라질 수 있습니다. 위의 개선 방법 중 최소한 작업 큐잉이나 사용자 피드백은 추가하는 게 좋을 것 같습니다. |
|
@gemini-code-assist 리뷰해줘 |
![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 effectively addresses the risk of runtime errors by adding null checks for the input ref throughout the Prompt component. The changes, primarily using optional chaining and if guards, are appropriate and well-executed. I've included a few suggestions to further simplify some of the conditional logic, which will enhance readability and maintainability. Overall, this is a solid fix that improves the component's stability.
| if (input) { | ||
| if (props.disabled) input.cursorColor = theme.backgroundElement | ||
| if (!props.disabled) input.cursorColor = theme.text | ||
| } |
There was a problem hiding this comment.
The nested if statements can be simplified into a single expression using a ternary operator. This makes the code more concise and easier to read.
if (input) {
input.cursorColor = props.disabled ? theme.backgroundElement : theme.text
}
| if (keybind.match("history_previous", e) && input?.visualCursor.visualRow === 0 && input) | ||
| input.cursorOffset = 0 | ||
| if ( | ||
| keybind.match("history_next", e) && | ||
| input?.visualCursor.visualRow === (input?.height ?? 0) - 1 && | ||
| input | ||
| ) | ||
| input.cursorOffset = input.plainText.length |
There was a problem hiding this comment.
This logic can be simplified by wrapping it in a single if (input) block. This improves readability by reducing redundant checks and making the code cleaner.
if (input) {
if (keybind.match("history_previous", e) && input.visualCursor.visualRow === 0) {
input.cursorOffset = 0
}
if (keybind.match("history_next", e) && input.visualCursor.visualRow === input.height - 1) {
input.cursorOffset = input.plainText.length
}
}
|
|
/ask 해당 pr 은 크래시를 방지하는 수준의 작업범위로 산정하는것이 올바를까? |
Ask❓해당 pr 은 크래시를 방지하는 수준의 작업범위로 산정하는것이 올바를까? Answer:결론부터 말하면, “크래시를 방지하는 수준(안정화/가드 추가)”의 작업범위로 산정하는 것이 대체로 타당합니다. 이 PR의 핵심 변경은 기능 추가/로직 변경이라기보다, 이미 존재하던 비동기 콜백·이벤트 경로에서 근거(디프 기준):
다만 산정 시 “순수 크래시 방지”로만 보기 애매한 포인트가 2가지 있습니다(범위가 커 보이진 않지만, 리스크/영향은 있음):
정리하면:
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (3)
320-322: Potential crash:inputmay not be assigned whenonMountexecutes.The
inputref is assigned in the textarea's ref callback (line 942), but the execution order between ref callbacks andonMountisn't strictly guaranteed in all scenarios. Ifinputis undefined here, this will throw.Consider adding a guard or using an effect that waits for
inputto be defined:Suggested fix
onMount(() => { - promptPartTypeId = input.extmarks.registerType("prompt-part") + if (input) { + promptPartTypeId = input.extmarks.registerType("prompt-part") + } })Alternatively, move this registration into the ref callback where
inputis guaranteed to exist.
369-371: Missing null check causes potential crash.
syncExtmarksWithPromptPartsaccessesinput.extmarksdirectly without a null guard, unlike other functions in this PR. This will throw ifinputis undefined when called fromonContentChange(line 784).Proposed fix
function syncExtmarksWithPromptParts() { + if (!input) return const allExtmarks = input.extmarks.getAllForTypeId(promptPartTypeId)
448-454: User action silently ignored wheninputis null.If the user selects a stash entry but
inputis null, the function returns early without applying the selection or providing feedback. The selected entry is effectively lost from the user's perspective.Consider showing a toast notification or keeping the dialog open with an error state when the operation cannot complete.
🤖 Fix all issues with AI agents
In @packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx:
- Around line 474-481: The setter set(prompt) writes the prompt to state via
setStore("prompt", prompt) but skips updating the UI when input is null, causing
ref.set(prompt) to appear only partially applied; change the logic so when input
is null you still persist the desired prompt and schedule/apply the UI updates
once input becomes available (e.g., add an effect or enqueue a pendingPrompt
that calls input.setText(prompt.input), restoreExtmarksFromParts(prompt.parts)
and input.gotoBufferEnd() when input is initialized), or at minimum document
that set() only updates store until input exists; reference the set(prompt)
method, setStore("prompt", prompt), input, restoreExtmarksFromParts and
input.gotoBufferEnd() when making the change.
- Around line 622-624: In pasteText, avoid silently dropping pasted content when
input is null: either enqueue the paste (store text in a pendingPaste variable
or PasteQueue and apply it when input becomes non-null) or surface user feedback
(call the existing toast/notification helper, e.g., showToast or notify, with a
message like "Paste deferred — input not ready"). Update the pasteText function
to detect input === null and perform the chosen action (enqueue pending paste
tied to the input lifecycle or show a toast) and ensure any queued paste is
applied in the input initialization code (where input is set up).
- Around line 657-659: pasteImage currently returns early and silently drops
image data when input is null (same issue as pasteText); update pasteImage to
adopt the same mitigation used for pasteText—either enqueue the incoming file
for later processing when input becomes available or surface a user-visible
notification/error indicating the paste failed. Locate pasteImage and pasteText
(and the shared input/visualCursor.offset usage) and implement the chosen
strategy consistently so no image data is lost silently when input is null.
🧹 Nitpick comments (2)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (2)
861-868: Consider simplifying the conditional chain.The multiple optional chaining operators make this condition hard to read. Consider extracting to a helper or using early guards.
Suggested simplification
- if (keybind.match("history_previous", e) && input?.visualCursor.visualRow === 0 && input) - input.cursorOffset = 0 - if ( - keybind.match("history_next", e) && - input?.visualCursor.visualRow === (input?.height ?? 0) - 1 && - input - ) + if (!input) return + if (keybind.match("history_previous", e) && input.visualCursor.visualRow === 0) { + input.cursorOffset = 0 + } + if (keybind.match("history_next", e) && input.visualCursor.visualRow === input.height - 1) { input.cursorOffset = input.plainText.length + }
941-950: Minor: redundant&& inputcheck.Since
input = ris assigned on line 942 andris always a validTextareaRenderablewhen the ref callback fires, the&& inputcheck on line 943 is redundant.Optional cleanup
ref={(r: TextareaRenderable) => { input = r - if (store.prompt.input && input) { + if (store.prompt.input) { input.setText(store.prompt.input) restoreExtmarksFromParts(store.prompt.parts) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (3)
packages/opencode/src/cli/cmd/tui/routes/session/index.tsx (1)
input(1320-1322)packages/ui/src/context/data.tsx (1)
store(44-46)packages/opencode/src/tool/webfetch.ts (2)
text(149-151)text(160-164)
🔇 Additional comments (5)
packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx (5)
103-108: LGTM!The guard correctly prevents accessing properties on a non-existent input. Since this is purely visual styling, no data loss concerns apply here.
155-178: LGTM!The null checks here are appropriate:
- For "Clear prompt", the command is already marked
disabled: trueindicating it requires additional conditions.- For "Submit prompt", the early return when input isn't focused is correct behavior.
324-326: LGTM, but callers may have inconsistent state.The early return prevents crashes, but some callers (e.g., stash operations at lines 430-435) update the store before calling this function. If
inputis null, the store reflects new state while the UI doesn't. This is part of the broader silent data loss concern flagged in PR comments.
933-939: LGTM!The guard in the
setTimeoutcallback correctly handles the case whereinputmay have been unmounted before the async callback executes. Since this is only for layout refresh after paste (not the paste itself), there's no data loss risk.
519-533: The pasted text expansion pattern is defensive but protected by thepasteTextguard.The optional chaining on
inputat line 520 correctly defaults to an empty array ifinputis null. However, this scenario cannot actually occur becausepasteText(line 623) returns early ifinputis null, preventing any text parts from being added tostore.prompt.partsin the first place. Since text parts can only be created wheninputis defined, the loop at lines 523-533 will only run when there are extmarks to process.The defensive code is good practice, but there's no practical risk of data loss here.
| sdk.event.on(TuiEvent.PromptAppend.type, (evt) => { | ||
| input.insertText(evt.properties.text) | ||
| input?.insertText(evt.properties.text) | ||
| setTimeout(() => { | ||
| input.getLayoutNode().markDirty() | ||
| input.gotoBufferEnd() | ||
| input?.getLayoutNode?.()?.markDirty() | ||
| input?.gotoBufferEnd() | ||
| renderer.requestRender() | ||
| }, 0) | ||
| }) |
There was a problem hiding this comment.
Silent data loss when input is null.
If PromptAppend fires before the input is mounted or after it unmounts, evt.properties.text is silently dropped. Consider queuing pending text when input is unavailable and flushing when it becomes ready, or notifying the user that the operation could not be applied.
There was a problem hiding this comment.
@coderabbitai
근거 = 텍스트 인풋은 단순 입력이기때문에 사용자경험에 있어 잠깐의 렉으로 느껴질뿐이지 오히려 큐로 처리하면 한번에 큐가 쏟아져 오히려 사용자 경험이 저해된다고 생각됨.
There was a problem hiding this comment.
Question:
근거 = 텍스트 인풋은 단순 입력이기때문에 사용자경험에 있어 잠깐의 렉으로 느껴질뿐이지 오히려 큐로 처리하면 한번에 큐가 쏟아져 오히려 사용자 경험이 저해된다고 생각됨.
Answer:
이 이슈( input 이 null 일 때 text가 “조용히” 유실되는 문제 )는 “큐잉”만이 해법이 아니라, UX 관점에서 더 안전한 대안(실패를 명시적으로 알리기/재시도 유도하기)이 충분히 합리적입니다.
앞으로 Qodo Merge가 유사 상황에서 “큐로 몰아서 처리” 같은 제안을 과도하게 밀지 않도록, /improve 제안 시 UX 트레이드오프(지연/버스트 입력/예상치 못한 일괄 반영)를 고려해서 실패 알림/드롭 방지(guard + user feedback) 쪽을 우선 제안하도록 가이딩할 수 있습니다.
아래처럼 pr_code_suggestions.extra_instructions에 프로젝트 선호(큐잉 지양, 명시적 실패 처리 선호)를 넣어두면 됩니다:
[pr_code_suggestions]
extra_instructions = """\
- UX 측면에서 '입력 이벤트를 큐에 쌓았다가 나중에 한 번에 flush' 하는 패턴은 지양한다(버스트 입력/예상치 못한 일괄 반영 위험).
- 대신 'silent data loss' 방지를 위해 다음 중 하나를 우선 제안한다:
1) input이 준비되지 않았을 때 명시적으로 실패를 반환/토스트 등으로 사용자에게 안내
2) 즉시 적용 불가 시 재시도 버튼/단일 최신값만 보존(last-write-wins) 같은 최소한의 보존 전략
3) 안전한 guard + telemetry/logging으로 드롭을 관측 가능하게 만들기
- 해결책 제안 시 "사용자 경험(렉/지연/버스트)" trade-off를 짧게 설명한다.
"""Relevant Sources:
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
@pinion05 I understand your reasoning. You've made a conscious trade-off: accepting that PromptAppend events might be dropped during mount/unmount transitions (perceived as brief lag) rather than implementing a queue that could flush all pending events at once and create a jarring UX.
That's a valid design decision for this use case. Thank you for clarifying the intent behind this approach.
| set(prompt) { | ||
| input.setText(prompt.input) | ||
| input?.setText(prompt.input) | ||
| setStore("prompt", prompt) | ||
| restoreExtmarksFromParts(prompt.parts) | ||
| input.gotoBufferEnd() | ||
| if (input) { | ||
| restoreExtmarksFromParts(prompt.parts) | ||
| input.gotoBufferEnd() | ||
| } | ||
| }, |
There was a problem hiding this comment.
set() updates store but may not update UI.
When input is null, setStore("prompt", prompt) executes but setText, restoreExtmarksFromParts, and gotoBufferEnd are skipped. External callers using ref.set(prompt) may expect the prompt to be fully applied, but only the store is updated.
Consider documenting this behavior or ensuring the UI syncs when input becomes available (e.g., via an effect).
🤖 Prompt for AI Agents
In @packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx around lines
474 - 481, The setter set(prompt) writes the prompt to state via
setStore("prompt", prompt) but skips updating the UI when input is null, causing
ref.set(prompt) to appear only partially applied; change the logic so when input
is null you still persist the desired prompt and schedule/apply the UI updates
once input becomes available (e.g., add an effect or enqueue a pendingPrompt
that calls input.setText(prompt.input), restoreExtmarksFromParts(prompt.parts)
and input.gotoBufferEnd() when input is initialized), or at minimum document
that set() only updates store until input exists; reference the set(prompt)
method, setStore("prompt", prompt), input, restoreExtmarksFromParts and
input.gotoBufferEnd() when making the change.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
안녕하세요, pinion05님. 질문해주셔서 감사합니다. CodeRabbit 봇의 지적은 유효하며, input이 아직 초기화되지 않았을 때 set 메서드가 호출될 경우 UI 업데이트가 완전히 이루어지지 않을 수 있다는 점에 대한 것입니다.
현재 코드에서 setStore("prompt", prompt)는 input의 상태와 관계없이 항상 실행되어 프롬프트 상태를 저장합니다. 그리고 ref 콜백(파일 packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx의 대략 943-946번째 줄)에서 input이 사용 가능해지면 store.prompt의 내용을 input.setText와 restoreExtmarksFromParts를 통해 UI에 적용하고 있습니다.
하지만 set 메서드에서 input.gotoBufferEnd() 호출도 input이 null일 때 건너뛰어지는데, ref 콜백에서는 이 부분이 처리되지 않고 있습니다. 따라서 input이 나중에 초기화될 때 커서가 텍스트의 끝으로 이동하지 않을 수 있습니다.
가장 올바른 방향은 input이 사용 가능해질 때 gotoBufferEnd()도 함께 호출하여 UI 동기화를 완료하는 것입니다. ref 콜백을 다음과 같이 수정하는 것을 고려해볼 수 있습니다:
if (store.prompt.input && input) {
input.setText(store.prompt.input)
restoreExtmarksFromParts(store.prompt.parts)
input.gotoBufferEnd() // 이 줄을 추가합니다.
}이렇게 하면 set 메서드가 input이 null일 때 호출되더라도, input이 나중에 초기화될 때 프롬프트의 내용과 커서 위치가 모두 올바르게 복원될 것입니다.
There was a problem hiding this comment.
Ask❓
@gemini-code-assist 그러면 어떤방향이 올바를까?
Answer:
결론부터 말하면 “store를 source of truth로 두고, input이 생기는 순간(store 값을) UI에 동기화” 하는 방향이 가장 안전하고 일관적입니다.
지금 PR에서 ref={(r) => { input = r; if (store.prompt.input && input) { ... }}}로 input이 초기화될 때 store → UI를 재적용하는 로직을 이미 추가하셨기 때문에, ref.set(prompt)가 input이 null인 시점에 호출되더라도 store에는 값이 남고, 나중에 input이 붙으면 UI가 따라오는 구조가 됩니다. 이 방향이 맞습니다.
다만 더 “정석”으로 정리하려면 아래 둘 중 하나를 추천합니다:
-
명시적인 동기화 effect 추가(권장)
- “ref에서 input이 잡혔는지”를 신호로 만들고,
input이 존재할 때마다 store 값이 UI에 반영되도록createEffect로 보장합니다. - 이렇게 하면 ref 콜백에 로직이 몰리지 않고,
set(),stash pop,editor open등 다양한 경로에서도 동기화 일관성이 좋아집니다.
- “ref에서 input이 잡혔는지”를 신호로 만들고,
-
현재 방식 유지 + 동작을 문서화
- “
set()은 항상 store는 업데이트하지만, input이 아직 없으면 UI 적용은 나중에(ref 연결 시점)에 일어난다”를 주석/문서로 명확히 해두는 방식입니다. - 이미 ref에서 store 값을 복원하는 코드가 들어갔으니, 최소한 이 의도를 남겨두면 오해가 줄어듭니다.
- “
요약: CodeRabbit이 지적한 “UI가 안 바뀔 수 있음”은 input이 붙을 때 store에서 복원하는 동기화 장치가 없을 때의 문제인데, 이번 PR은 그 장치를 ref에서 추가했으니 방향은 옳습니다. 다만 유지보수 관점에서는 ref 콜백 대신 effect로 동기화를 일반화하는 쪽이 더 깔끔합니다.
There was a problem hiding this comment.
/gemini
어떤방향의 개선이 이상적일까?
There was a problem hiding this comment.
pinion05님, 좋은 질문 감사합니다. qodo-code-review[bot]님의 의견처럼, store를 "source of truth"로 유지하고 input이 사용 가능해지는 시점에 UI를 동기화하는 방식이 가장 이상적이고 견고한 접근 방식입니다.
현재 set 메서드는 input이 null일 때 setStore("prompt", prompt)만 실행하고, input이 존재할 때는 UI 업데이트를 시도합니다. 하지만 input의 초기화 시점과 set 메서드 호출 시점의 비동기적인 특성 때문에, UI가 완전히 동기화되지 않거나 불일치가 발생할 가능성이 있습니다.
가장 이상적인 개선 방향은 다음과 같습니다:
set메서드는 오직store.prompt만 업데이트하도록 합니다.input에 직접 접근하는 로직은 모두 제거합니다. 이렇게 하면set메서드의 책임이 명확해지고,input의 존재 여부에 관계없이 항상 상태를 일관되게 업데이트할 수 있습니다.createEffect를 사용하여store.prompt와input의 변화에 반응하여 UI를 동기화합니다. 이effect는input객체가 유효하고store.prompt의 내용이 변경될 때마다input.setText(),restoreExtmarksFromParts(), 그리고input.gotoBufferEnd()를 호출하여 UI를store의 최신 상태로 반영하도록 합니다.
이 접근 방식은 상태 관리와 UI 렌더링 로직을 명확하게 분리하여 코드의 예측 가능성과 유지보수성을 높여줍니다. input이 나중에 초기화되더라도 store의 최신 상태가 자동으로 UI에 반영되므로, input이 null일 때 set이 호출되어도 문제가 발생하지 않습니다.
따라서, set 메서드는 다음과 같이 수정하는 것이 좋습니다:
set(prompt) {
setStore("prompt", prompt)
},그리고 Prompt 컴포넌트 내부에 다음과 같은 createEffect를 추가하여 UI 동기화를 처리합니다 (예시):
createEffect(() => {
// input이 존재하고 store.prompt가 변경될 때마다 UI를 동기화합니다.
if (input && store.prompt) {
input.setText(store.prompt.input)
restoreExtmarksFromParts(store.prompt.parts)
input.gotoBufferEnd()
}
})또한, ref 콜백 (패치 파일 packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx의 943-946번째 줄)에서 input.setText와 restoreExtmarksFromParts를 호출하는 부분은 이 새로운 createEffect가 동일한 역할을 수행하므로 제거하는 것이 좋습니다. 제 이전 코멘트에서 input.gotoBufferEnd()를 ref 콜백에 추가하는 것을 제안했지만, 이 "이상적인" 접근 방식에서는 createEffect가 모든 UI 동기화 로직을 담당하게 됩니다.
이러한 변경을 통해 Prompt 컴포넌트의 상태 관리 로직이 더욱 견고하고 명확해질 것입니다.
| set(prompt) { | |
| input.setText(prompt.input) | |
| input?.setText(prompt.input) | |
| setStore("prompt", prompt) | |
| restoreExtmarksFromParts(prompt.parts) | |
| input.gotoBufferEnd() | |
| if (input) { | |
| restoreExtmarksFromParts(prompt.parts) | |
| input.gotoBufferEnd() | |
| } | |
| }, | |
| set(prompt) { | |
| setStore("prompt", prompt) | |
| }, |
| function pasteText(text: string, virtualText: string) { | ||
| if (!input) return | ||
| const currentOffset = input.visualCursor.offset |
There was a problem hiding this comment.
Pasted text silently dropped when input is null.
The early return prevents crashes, but user-initiated paste operations are silently lost without feedback. Users may not realize their content wasn't pasted. Consider queuing the paste or showing a toast notification.
🤖 Prompt for AI Agents
In @packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx around lines
622 - 624, In pasteText, avoid silently dropping pasted content when input is
null: either enqueue the paste (store text in a pendingPaste variable or
PasteQueue and apply it when input becomes non-null) or surface user feedback
(call the existing toast/notification helper, e.g., showToast or notify, with a
message like "Paste deferred — input not ready"). Update the pasteText function
to detect input === null and perform the chosen action (enqueue pending paste
tied to the input lifecycle or show a toast) and ensure any queued paste is
applied in the input initialization code (where input is set up).
| async function pasteImage(file: { filename?: string; content: string; mime: string }) { | ||
| if (!input) return | ||
| const currentOffset = input.visualCursor.offset |
There was a problem hiding this comment.
Same silent data loss concern for image paste.
Like pasteText, image paste is silently dropped when input is null. Consider a consistent mitigation strategy (queuing or user notification) for both functions.
🤖 Prompt for AI Agents
In @packages/opencode/src/cli/cmd/tui/component/prompt/index.tsx around lines
657 - 659, pasteImage currently returns early and silently drops image data when
input is null (same issue as pasteText); update pasteImage to adopt the same
mitigation used for pasteText—either enqueue the incoming file for later
processing when input becomes available or surface a user-visible
notification/error indicating the paste failed. Locate pasteImage and pasteText
(and the shared input/visualCursor.offset usage) and implement the chosen
strategy consistently so no image data is lost silently when input is null.
1 participant
Summary
input?.) for theTextareaRenderableref inPromptcomponent.pre-pushhusky hook to explicitly include~/.bun/binin PATH to avoid "bun: command not found" errors in some environments.Issue
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.