refactor(data): migrate QuickPhrase to Prompt with version management#13430
refactor(data): migrate QuickPhrase to Prompt with version management#13430zhangjiadi225 wants to merge 5 commits intoCherryHQ:v2from
Conversation
- Replace QuickPhrase with Prompt entity backed by SQLite via Drizzle ORM - Add prompt and prompt_version tables with automatic versioning - Implement PromptService with CRUD, version history, and rollback - Add API handlers and schemas for /prompts endpoints - Create PromptMigrator for Dexie quick_phrases → SQLite migration - Refactor QuickPhrasesButton with version sub-menu selection - Replace QuickPhraseSettings with PromptSettings page - Remove unused template variables system and templateEngine utility - Update routing from /settings/quickphrase to /settings/prompts Signed-off-by: zhangjiadi225 <625013594@qq.com>
|
Note This comment was translated by Claude. Add unit tests for the key migrator Original Content关键的migrator加一下单元测试 |
- Test prepare phase: table existence, valid/invalid filtering, empty table, error handling - Test execute phase: insertion, title defaults, progress reporting, transaction errors - Test validate phase: count matching, mismatch detection, db failure handling, skip tracking Signed-off-by: zhangjiadi225 <625013594@qq.com>
|
Nice work on the migration and version management! A few things before detailed review: Question: Variable designThe old QuickPhrase had a Have you considered the variable substitution story? How should Want to make sure the data model accounts for this before we merge. Initial code observations
|
|
[!NOTE]
This comment was translated by Claude.
Thanks for the detailed reply!
**Variable substitution**: Agreed — variable management should be designed as an independent global module, not tied to Prompts. The current `` syntax is just a simple design for this stage. Going forward, the variable system should serve as shared infrastructure across the entire application — Prompts, Agent system prompts, MCP configs, and other areas could all reference the same global variables with type constraints and usage history.
**Code fixes**: Great catches. Will fix all three issues and push shortly.
— Di Ge
…---
<details>
<summary>Original Content</summary>
Thanks for the detailed reply!
**Variable substitution**: Agreed — variable management should be designed as
an independent global module, not tied to Prompts. The current `` syntax
is just a simple design for this stage. Going forward, the variable system
should serve as shared infrastructure across the entire application — Prompts,
Agent system prompts, MCP configs, and other areas could all reference the
same global variables with type constraints and usage history.
**Code fixes**: Great catches. Will fix all three issues and push shortly.
...迪哥
***@***.***
</details>
|
…aints - Wrap create/update/rollback/reorder in db.transaction() - Move reads inside transactions to prevent race conditions - Add notNull constraints to currentVersion and sortOrder - Change prompt_version composite index to uniqueIndex - Remove redundant null fallbacks in rowToPrompt Signed-off-by: zhangjiadi225 <625013594@qq.com>
EurFelux
left a comment
There was a problem hiding this comment.
Not fully reviewed yet.
- Merge 2 migrations into 1 clean migration file - Add Zod schemas for Prompt types and DTO validation - Validate request body with Zod .parse() in handlers - Remove redundant await and return promises directly Signed-off-by: zhangjiadi225 <625013594@qq.com>
…ttings Remove all styled-components definitions from PromptSettings page and replace with Tailwind CSS utility classes to align with v2 UI refactoring requirements.
| createdAt: z.string(), | ||
| updatedAt: z.string() |
There was a problem hiding this comment.
use z.iso namespace and see what you want.
| // ============================================================================ | ||
|
|
||
| export const PromptSchema = z.object({ | ||
| id: z.string(), |
There was a problem hiding this comment.
consider more strict z.uuid. Also other id fields if you want them to be uuid
| /** Whether this item comes from the assistant's regularPhrases */ | ||
| isAssistantPhrase: boolean |
There was a problem hiding this comment.
better to use a type field for extensibility.
is it really necessary to discriminate them?
| sources: { | ||
| electronStore: { get: vi.fn() }, | ||
| reduxState: { | ||
| getCategory: vi.fn(), | ||
| getAllCategories: vi.fn() | ||
| } as unknown as MigrationContext['sources']['reduxState'], | ||
| dexieExport: { | ||
| tableExists: vi.fn().mockResolvedValue(tableExists), | ||
| readTable: vi.fn().mockResolvedValue(tableData), | ||
| getExportPath: vi.fn().mockReturnValue('/tmp/export'), | ||
| createStreamReader: vi.fn(), | ||
| getTableFileSize: vi.fn() | ||
| } as unknown as MigrationContext['sources']['dexieExport'] | ||
| }, |
There was a problem hiding this comment.
CI Blocker: createMockContext is missing dexieSettings and localStorage from sources, causing the typecheck to fail:
error TS2739: Type '{ electronStore: ...; reduxState: ...; dexieExport: ... }'
is missing the following properties: dexieSettings, localStorage
Need to add mock stubs for both:
dexieSettings: {
get: vi.fn(),
getAll: vi.fn()
} as unknown as MigrationContext['sources']['dexieSettings'],
localStorage: {
get: vi.fn(),
getAll: vi.fn()
} as unknown as MigrationContext['sources']['localStorage']| ) | ||
| } | ||
|
|
||
| const Label = styled.div` | ||
| const VarLabel = styled.div` |
There was a problem hiding this comment.
styled-components is being deprecated in v2 per kangfenmao's review. This VarLabel should be replaced with a TailwindCSS class like in PromptSettings.tsx:
// Replace VarLabel usage with:
<div className="mb-1 text-(--color-text) text-sm">...</div>| await loadPrompts() | ||
| } | ||
|
|
||
| const reversedPrompts = [...promptsList].reverse() |
There was a problem hiding this comment.
Nit: reversedPrompts creates a new reversed array on every render. Consider memoizing it:
const reversedPrompts = useMemo(() => [...promptsList].reverse(), [promptsList])| content: p.content, | ||
| isAssistantPhrase: true, | ||
| currentVersion: 1 | ||
| })) | ||
|
|
||
| const globalPrompts: UnifiedPromptItem[] = prompts.map((p) => ({ | ||
| id: p.id, | ||
| title: p.title, | ||
| content: p.content, | ||
| isAssistantPhrase: false, | ||
| currentVersion: p.currentVersion | ||
| })) | ||
|
|
||
| setPromptItems([...assistantPhrases, ...globalPrompts]) |
There was a problem hiding this comment.
assistant.regularPhrases is still read from / written to Redux (not migrated to SQLite). This means:
- Assistant-level phrases remain in the old Redux store while global phrases move to SQLite — two different storage backends for conceptually the same data type
- The "Add to assistant" flow (line ~165) still calls
updateAssistant({ ...assistant, regularPhrases: updatedPhrases })which writes directly to Redux
Is this intentional? If so, it should be documented as a known limitation / follow-up. If not, consider unifying storage by adding a relation table, e.g.:
CREATE TABLE assistant_prompt (
assistant_id TEXT NOT NULL REFERENCES assistant(id) ON DELETE CASCADE,
prompt_id TEXT NOT NULL REFERENCES prompt(id) ON DELETE CASCADE,
sort_order INTEGER NOT NULL DEFAULT 0,
PRIMARY KEY (assistant_id, prompt_id)
);This way all prompts live in the prompt table (single source of truth), and the relation table maps which prompts belong to which assistant. Global prompts are simply those with no relation entry.
| useEffect(() => { | ||
| loadQuickListPhrases() | ||
| }, [loadQuickListPhrases]) | ||
| loadPromptItems() | ||
| }, [loadPromptItems]) |
There was a problem hiding this comment.
We have useDataApi hook. Don't use useEffect to fetch data.
| const loadPrompts = useCallback(async () => { | ||
| const data = await dataApiService.get('/prompts') | ||
| setPromptsList(data) | ||
| }, []) | ||
|
|
||
| useEffect(() => { | ||
| loadPrompts() | ||
| }, [loadPrompts]) |
What this PR does
Before this PR:
After this PR:
${var}syntax in contentWhy we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Breaking changes
Data migration from Dexie
quick_phrasesto SQLiteprompttable runs automatically via PromptMigrator.Checklist
Release note