Conversation
…d context handling ✨ feat(rewrite): enhance rewrite prompt template with improved formatting ✨ feat(cli): add line range and context options to CLI command builder ✨ style: add styles for rewrite confirmation modal and text display
… and context extraction
There was a problem hiding this comment.
Pull request overview
This PR adds a "rewrite lines" feature that allows users to select text in their Obsidian vault and have it rewritten by an AI model with specific goals (e.g., removing AI-like phrases). The feature integrates with both the Obsidian plugin frontend and the Go CLI backend.
Key changes:
- Adds a new rewrite command accessible from the Obsidian editor that prompts for rewrite goals, sends the selected text to the AI with surrounding context, and displays a confirmation modal with before/after comparison
- Implements Go CLI backend support including line extraction with context, template-based prompt generation, and both JSON (for plugin use) and direct file modification modes
- Includes validation tests, CSS styling for the confirmation modal, and updates to the config check command
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| obsidian-novelmaker/styles.css | Adds CSS styling for the rewrite confirmation modal, including text boxes, section headers, and usage info display |
| obsidian-novelmaker/src/utils/cli.js | Extends CLI command builder to support new rewrite-specific options (line ranges, context windows, prompt goal) |
| obsidian-novelmaker/src/modals/rewrite-prompt.js | New modal for collecting the rewrite goal/instruction from the user |
| obsidian-novelmaker/src/modals/rewrite-confirm.js | New modal displaying before/after comparison and applying the rewrite to the editor |
| obsidian-novelmaker/src/commands/rewrite.js | Command registration and execution logic for the rewrite feature, handling text selection and modal flow |
| obsidian-novelmaker/main.js | Registers the rewrite command with the plugin |
| novelmaker/struct.go | Adds RewritePrompt struct to hold template configuration |
| novelmaker/render.go | Implements RenderRewrite function to execute AI rewriting with context and goal |
| internal/obsidian/vault.go | Adds LoadRewritePrompt method to read and parse the rewrite prompt template |
| internal/obsidian/init_template/Config/rewrite_prompt.md | Default template for rewrite prompts with system instructions and context structure |
| cmd/novelmaker-obs/main.go | Integrates rewrite command into the CLI root command |
| cmd/novelmaker-obs/cmd_rewrite_test.go | Unit tests for validation logic and file reading functionality |
| cmd/novelmaker-obs/cmd_rewrite.go | Full CLI implementation including argument parsing, line extraction, context building, and file modification |
| cmd/novelmaker-obs/cmd_config_check.go | Updates config check to validate the new rewrite prompt template |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (options.promptGoal && options.promptGoal.trim()) { | ||
| args.push('--prompt', options.promptGoal); | ||
| } |
There was a problem hiding this comment.
Both options.prompt (line 78-80) and options.promptGoal (line 82-84) are being mapped to the same CLI flag --prompt. If both options are provided simultaneously, this will result in duplicate --prompt arguments being passed to the CLI command. While the rewrite command uses promptGoal and other commands use prompt, this creates potential for confusion and bugs if the function is called with both options set.
Consider either:
- Renaming the flag in the Go CLI to match the JavaScript property name (e.g., use
--prompt-goalfor rewrite commands) - Making these conditions mutually exclusive with an early check
- Using different flag names for different command types
| // Get current document content | ||
| const doc = this.editor.getDoc(); | ||
|
|
There was a problem hiding this comment.
The variable doc is declared but never used. The getDoc() call appears to be unnecessary since replaceRange() is called directly on this.editor without using the doc variable. This line can be safely removed.
| // Get current document content | |
| const doc = this.editor.getDoc(); | |
| try { | ||
| // Default context lines | ||
| const contextPrev = 3; | ||
| const contextNext = 3; | ||
|
|
||
| const loadingModal = new LoadingModal(plugin.app, '正在改寫文字...請稍候'); | ||
| loadingModal.open(); | ||
|
|
||
| try { | ||
| const vaultPath = plugin.app.vault.adapter.basePath; | ||
|
|
||
| const args = buildCLICommand('rewrite', { | ||
| json: true, | ||
| filepath: file.path, | ||
| lineStart, | ||
| lineEnd, | ||
| contextPrev, | ||
| contextNext, | ||
| promptGoal, | ||
| backend: plugin.settings.backend, | ||
| timeout: plugin.settings.timeout, | ||
| }); | ||
|
|
||
| const { stdout } = await executeCLI(plugin, plugin.settings.cliPath, args, vaultPath); | ||
| const output = parseJSONOutput(stdout); | ||
|
|
||
| // Show confirmation modal with before/after comparison | ||
| new RewriteConfirmModal( | ||
| plugin.app, | ||
| editor, | ||
| file, | ||
| selectedText, | ||
| output.rewritten_content, | ||
| lineStart, | ||
| lineEnd, | ||
| output | ||
| ).open(); | ||
|
|
||
| } catch (error) { | ||
| new Notice(`❌ 錯誤: ${error.message}`); | ||
| console.error(error); | ||
| } finally { | ||
| loadingModal.forceCloseNow(); | ||
| } | ||
|
|
||
| } catch (error) { | ||
| new Notice(`❌ 錯誤: ${error.message}`); | ||
| console.error(error); |
There was a problem hiding this comment.
There are nested try-catch blocks with identical error handling logic. The outer try-catch (lines 54-102) wraps an inner try-catch (lines 62-97), but both handle errors the same way by showing a Notice and logging to console. The outer try-catch is redundant and should be removed to simplify the error handling flow.
| try { | |
| // Default context lines | |
| const contextPrev = 3; | |
| const contextNext = 3; | |
| const loadingModal = new LoadingModal(plugin.app, '正在改寫文字...請稍候'); | |
| loadingModal.open(); | |
| try { | |
| const vaultPath = plugin.app.vault.adapter.basePath; | |
| const args = buildCLICommand('rewrite', { | |
| json: true, | |
| filepath: file.path, | |
| lineStart, | |
| lineEnd, | |
| contextPrev, | |
| contextNext, | |
| promptGoal, | |
| backend: plugin.settings.backend, | |
| timeout: plugin.settings.timeout, | |
| }); | |
| const { stdout } = await executeCLI(plugin, plugin.settings.cliPath, args, vaultPath); | |
| const output = parseJSONOutput(stdout); | |
| // Show confirmation modal with before/after comparison | |
| new RewriteConfirmModal( | |
| plugin.app, | |
| editor, | |
| file, | |
| selectedText, | |
| output.rewritten_content, | |
| lineStart, | |
| lineEnd, | |
| output | |
| ).open(); | |
| } catch (error) { | |
| new Notice(`❌ 錯誤: ${error.message}`); | |
| console.error(error); | |
| } finally { | |
| loadingModal.forceCloseNow(); | |
| } | |
| } catch (error) { | |
| new Notice(`❌ 錯誤: ${error.message}`); | |
| console.error(error); | |
| // Default context lines | |
| const contextPrev = 3; | |
| const contextNext = 3; | |
| const loadingModal = new LoadingModal(plugin.app, '正在改寫文字...請稍候'); | |
| loadingModal.open(); | |
| try { | |
| const vaultPath = plugin.app.vault.adapter.basePath; | |
| const args = buildCLICommand('rewrite', { | |
| json: true, | |
| filepath: file.path, | |
| lineStart, | |
| lineEnd, | |
| contextPrev, | |
| contextNext, | |
| promptGoal, | |
| backend: plugin.settings.backend, | |
| timeout: plugin.settings.timeout, | |
| }); | |
| const { stdout } = await executeCLI(plugin, plugin.settings.cliPath, args, vaultPath); | |
| const output = parseJSONOutput(stdout); | |
| // Show confirmation modal with before/after comparison | |
| new RewriteConfirmModal( | |
| plugin.app, | |
| editor, | |
| file, | |
| selectedText, | |
| output.rewritten_content, | |
| lineStart, | |
| lineEnd, | |
| output | |
| ).open(); | |
| } catch (error) { | |
| new Notice(`❌ 錯誤: ${error.message}`); | |
| console.error(error); | |
| } finally { | |
| loadingModal.forceCloseNow(); |
| .rewrite-before h3 { | ||
| color: var(--text-muted); | ||
| } | ||
|
|
||
| .rewrite-after h3 { |
There was a problem hiding this comment.
The CSS selectors .rewrite-before h3 (line 212) and .rewrite-after h3 (line 216) assume the h3 elements are children of the div elements. However, in the JavaScript code (rewrite-confirm.js lines 30-35, 38-43), the h3 elements are created as siblings directly under contentEl, not as children of the beforeDiv and afterDiv. This means these CSS rules will never match and the h3 color styling will not be applied.
To fix this, either:
- Move the h3 creation inside the respective divs in the JavaScript
- Change the CSS selectors to target the h3 elements differently (e.g., using adjacent sibling selectors or different class names)
| .rewrite-before h3 { | |
| color: var(--text-muted); | |
| } | |
| .rewrite-after h3 { | |
| .rewrite-before { | |
| color: var(--text-muted); | |
| } | |
| .rewrite-after { |
No description provided.