🐛 fix(input): revert #11755 to fix chat input unfocus#11764
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideReverts the previous input markdown crash workaround by removing contentRef-based persistence and key-driven remounting of editors, and instead initializes editor content via effects while simplifying chat input provider and editor wiring. Sequence diagram for CronJobContentEditor initialization and content changesequenceDiagram
actor User
participant CronJobContentEditor
participant useEditorHook as useEditor
participant EditorInstance as Editor
User->>CronJobContentEditor: Mount component with enableRichRender, initialValue, onChange
CronJobContentEditor->>useEditorHook: useEditor()
useEditorHook-->>CronJobContentEditor: editor (possibly undefined)
rect rgb(235, 235, 235)
CronJobContentEditor->>CronJobContentEditor: useEffect sync currentValueRef with initialValue
end
CronJobContentEditor->>CronJobContentEditor: useEffect on [editor, enableRichRender, initialValue]
CronJobContentEditor->>EditorInstance: setTimeout(100ms) setDocument(mode, initialValue)
note over EditorInstance: mode = markdown if enableRichRender else text
CronJobContentEditor-->>EditorInstance: setDocument may throw error
EditorInstance-->>CronJobContentEditor: error
CronJobContentEditor->>CronJobContentEditor: catch error, log to console
CronJobContentEditor->>EditorInstance: setTimeout(100ms) retry setDocument(mode, initialValue)
User->>EditorInstance: Type content
EditorInstance-->>CronJobContentEditor: onTextChange event
CronJobContentEditor->>CronJobContentEditor: handleContentChange(event)
CronJobContentEditor->>EditorInstance: getDocument(mode)
EditorInstance-->>CronJobContentEditor: nextContent
CronJobContentEditor->>CronJobContentEditor: compute finalContent
CronJobContentEditor->>CronJobContentEditor: compare finalContent with currentValueRef.current
alt content changed
CronJobContentEditor->>CronJobContentEditor: update currentValueRef.current
CronJobContentEditor-->>User: onChange(finalContent)
else content unchanged
CronJobContentEditor->>CronJobContentEditor: do nothing
end
Class diagram for updated editor and chat input componentsclassDiagram
class CronJobContentEditor {
+boolean enableRichRender
+string initialValue
+onChange(value string) void
-RefObject currentValueRef
-editor any
+handleContentChange(event any) void
}
class ChatInputProvider {
+string agentId
+ReactNode children
+any leftActions
+any rightActions
+boolean mobile
+any mentionItems
+boolean allowExpand
-editor any
+createStore(config any) any
}
class InputEditor {
+number defaultRows
-any editor
-any slashMenuRef
-any send
-updateMarkdownContent() void
-expand() void
-any mentionItems
-onBlur() void
-onChange() void
-onCompositionEnd() void
-onCompositionStart() void
-onFocus() void
-onInit(editor any) void
-onPressEnter(payload any) void
}
class ChatInputStoreState {
+boolean allowExpand
+any editor
+boolean isContentEmpty
+string markdownContent
+any mentionItems
+any slashMenuRef
+expand() void
+send() void
+updateMarkdownContent() void
}
ChatInputProvider --> ChatInputStoreState : creates store with
InputEditor --> ChatInputStoreState : uses state via useChatInputStore
InputEditor --> ChatInputProvider : rendered inside
CronJobContentEditor --> EditorInstance : uses via useEditor
class EditorInstance {
+setDocument(mode string, content string) void
+getDocument(mode string) string
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
TestGru AssignmentSummary
Tip You can |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
CronJobContentEditor, the initializationuseEffectschedules asetTimeouton every change toeditor,enableRichRender, orinitialValuewithout cleanup, which can lead to multiple overlappingsetDocumentcalls; consider storing the timeout id and clearing it in a cleanup function or restructuring to run only once when the editor becomes ready. - The try/catch in the
CronJobContentEditorinitialization effect schedules the samesetTimeoutlogic both in the happy path and in the catch block, making the error handling redundant; you can simplify this by moving the timeout logic outside the try/catch or by logging the error without re-running the same code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CronJobContentEditor`, the initialization `useEffect` schedules a `setTimeout` on every change to `editor`, `enableRichRender`, or `initialValue` without cleanup, which can lead to multiple overlapping `setDocument` calls; consider storing the timeout id and clearing it in a cleanup function or restructuring to run only once when the editor becomes ready.
- The try/catch in the `CronJobContentEditor` initialization effect schedules the same `setTimeout` logic both in the happy path and in the catch block, making the error handling redundant; you can simplify this by moving the timeout logic outside the try/catch or by logging the error without re-running the same code.
## Individual Comments
### Comment 1
<location> `src/app/[variants]/(main)/agent/cron/[cronId]/features/CronJobContentEditor.tsx:35-43` </location>
<code_context>
}, [initialValue]);
+ // Initialize editor content when editor is ready
+ useEffect(() => {
+ if (!editor) return;
+ try {
+ setTimeout(() => {
+ if (initialValue) {
+ editor.setDocument(enableRichRender ? 'markdown' : 'text', initialValue);
+ }
+ }, 100);
+ } catch (error) {
+ console.error('[CronJobContentEditor] Failed to initialize editor content:', error);
+ setTimeout(() => {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The try/catch around `setTimeout` will not catch errors thrown inside the callback and duplicates the initialization logic.
Because the `try/catch` only wraps the `setTimeout` call, any error thrown by `editor.setDocument` inside the callback will escape. The catch then schedules another `setTimeout` with the same logic, causing potential duplicate execution and making control flow unclear. Instead, keep a single `setTimeout` and wrap `editor.setDocument` in a `try/catch` inside its callback, optionally adding a guard (e.g., a ref or checking current content) to avoid re-initializing an already-populated editor.
Suggested implementation:
```typescript
const { t } = useTranslation('setting');
const editor = useEditor();
const currentValueRef = useRef(initialValue);
const hasInitializedRef = useRef(false);
```
```typescript
// Initialize editor content when editor is ready
useEffect(() => {
if (!editor || hasInitializedRef.current) return;
setTimeout(() => {
try {
if (!initialValue || hasInitializedRef.current) return;
editor.setDocument(enableRichRender ? 'markdown' : 'text', initialValue);
hasInitializedRef.current = true;
} catch (error) {
console.error(
'[CronJobContentEditor] Failed to initialize editor content:',
error,
);
}
}, 100);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+35
to
+43
| useEffect(() => { | ||
| if (!editor) return; | ||
| try { | ||
| setTimeout(() => { | ||
| if (initialValue) { | ||
| editor.setDocument(enableRichRender ? 'markdown' : 'text', initialValue); | ||
| } | ||
| }, 100); | ||
| } catch (error) { |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): The try/catch around setTimeout will not catch errors thrown inside the callback and duplicates the initialization logic.
Because the try/catch only wraps the setTimeout call, any error thrown by editor.setDocument inside the callback will escape. The catch then schedules another setTimeout with the same logic, causing potential duplicate execution and making control flow unclear. Instead, keep a single setTimeout and wrap editor.setDocument in a try/catch inside its callback, optionally adding a guard (e.g., a ref or checking current content) to avoid re-initializing an already-populated editor.
Suggested implementation:
const { t } = useTranslation('setting');
const editor = useEditor();
const currentValueRef = useRef(initialValue);
const hasInitializedRef = useRef(false); // Initialize editor content when editor is ready
useEffect(() => {
if (!editor || hasInitializedRef.current) return;
setTimeout(() => {
try {
if (!initialValue || hasInitializedRef.current) return;
editor.setDocument(enableRichRender ? 'markdown' : 'text', initialValue);
hasInitializedRef.current = true;
} catch (error) {
console.error(
'[CronJobContentEditor] Failed to initialize editor content:',
error,
);
}
}, 100);87e22ba to
55a3fd6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87e22ba5ae
ℹ️ 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".
Comment on lines
+35
to
+41
| useEffect(() => { | ||
| if (!editor) return; | ||
| try { | ||
| setTimeout(() => { | ||
| if (initialValue) { | ||
| editor.setDocument(enableRichRender ? 'markdown' : 'text', initialValue); | ||
| } |
There was a problem hiding this comment.
Preserve draft content when reinitializing the editor
The effect re-seeds the editor from initialValue every time enableRichRender toggles, but initialValue is sourced from cronJob?.content and doesn’t reflect in-progress edits until auto‑save completes. If a user edits a cron job and then flips the input‑markdown setting before the draft is saved, this setDocument call overwrites the unsaved text with the last persisted content, effectively discarding edits. Consider restoring from the current editor/draft state instead of initialValue on render‑mode changes.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #11764 +/- ##
=========================================
Coverage 74.43% 74.43%
=========================================
Files 1191 1191
Lines 94492 94492
Branches 10474 12940 +2466
=========================================
Hits 70335 70335
Misses 24067 24067
Partials 90 90
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
|
❤️ Great PR @arvinxx ❤️ The growth of project is inseparable from user feedback and contribution, thanks for your contribution! If you are interesting with the lobehub developer community, please join our discord and then dm @arvinxx or @canisminor1990. They will invite you to our private developer channel. We are talking about the lobe-chat development or sharing ai newsletter around the world. |
Member
|
🎉 This PR is included in version 2.0.0-next.359 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💻 Change Type
🔗 Related Issue
@Innei #11755 PR 导致输入文本后就直接失焦，我先回滚了。然后你需要针对这个场景补一个 E2E 用例确保不会有 regression
🔀 Description of Change
🧪 How to Test
📸 Screenshots / Videos
📝 Additional Information
Summary by Sourcery
Revert prior changes that persisted editor content across rich text toggle for cron job content and chat input, restoring simpler editor initialization behavior without shared content refs.
Enhancements: