Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
PR Summary by QodoAdd Chat Macros authoring and output profile editors
AI Description
Diagram
High-Level Assessment
Files changed (24)
|
Code Review by Qodo
1. Profile saves revert macro toggles
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if not isinstance(title, str) or not title or len(title) > MAX_SECTION_TITLE_LENGTH: | ||
| raise MacroValidationError("invalid output profile section title") |
There was a problem hiding this comment.
5. Blank profile headings reach responses 📘 Rule violation ≡ Correctness
normalize_output_profile checks not title without trimming, so it accepts and persists
whitespace-only section titles. A direct settings API request such as {"summary": " "} remains
truthy in render_output_profile and is inserted after the Markdown heading marker instead of
falling back to the generated section title.
Agent Prompt
## Issue description
Output-profile section titles containing only whitespace pass backend validation and are persisted, causing the renderer to produce visually blank Markdown headings instead of generated section titles.
## Fix Focus Areas
- tldw_Server_API/app/core/Chat_Macros/output_profiles.py[66-76]
- tldw_Server_API/app/core/Chat_Macros/output_profiles.py[126-134]
- tldw_Server_API/tests/Chat_Macros/unit/test_macro_service.py[252-277]
## Recommended Fix
Strip each title before validating and storing it, then reject the title with `MacroValidationError` when the trimmed value is empty or exceeds the length bound. Add unit and API-level coverage proving whitespace-only headings are rejected.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @staticmethod | ||
| def _require_matching_name(resource_name: str, definition: MacroDefinition) -> None: | ||
| if definition.name != resource_name: |
There was a problem hiding this comment.
6. Seven new python functions lack docs 📘 Rule violation ✧ Quality
_require_matching_name and the six newly added Python test functions begin directly with executable statements rather than non-empty docstrings. Their name-mismatch, settings-preservation, custom-heading, and unknown-section behavior therefore has no in-code contract for later changes.
Agent Prompt
## Issue description
Seven functions introduced by this PR have no immediately following docstring, including the new name-validation helper and six regression tests.
## Fix Focus Areas
- tldw_Server_API/app/core/Chat_Macros/service.py[283-288]
- tldw_Server_API/tests/Chat_Macros/integration/test_chat_macros_api.py[190-213]
- tldw_Server_API/tests/Chat_Macros/unit/test_macro_service.py[115-138]
- tldw_Server_API/tests/Chat_Macros/unit/test_macro_service.py[156-168]
- tldw_Server_API/tests/Chat_Macros/unit/test_macro_service.py[252-277]
## Recommended Fix
Add a concise, meaningful docstring as the first statement of each new function, describing the behavior or regression it covers.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| assert not (api_client.user_base / "macros" / "daily_digest").exists() | ||
| assert not (api_client.user_base / "macros" / "other_name").exists() |
There was a problem hiding this comment.
11. Api tests depend on storage layout 📘 Rule violation ▣ Testability
test_macro_api_rejects_mismatched_definition_names_without_storage_changes asserts concrete paths under user_base/macros and reads MACRO.yaml after making HTTP requests. A storage-backend or directory-layout refactor can preserve endpoint behavior yet break this integration test because its assertions bypass the public API state.
Agent Prompt
## Issue description
The new API integration test verifies private filesystem paths and file contents instead of observable endpoint behavior.
## Fix Focus Areas
- tldw_Server_API/tests/Chat_Macros/integration/test_chat_macros_api.py[190-213]
## Recommended Fix
Replace direct `user_base` and `MACRO.yaml` assertions with GET requests through the test client that verify rejected names remain absent and the original macro definition remains unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| original_raw = _macro_yaml("daily_digest") | ||
| created = api_client.client.post(PREFIX, json={"name": "daily_digest", "raw": original_raw}) | ||
| assert created.status_code == 201, created.text |
There was a problem hiding this comment.
7. One api test mixes two endpoint flows 📘 Rule violation ▣ Testability
test_macro_api_rejects_mismatched_definition_names_without_storage_changes drives both POST rejection and PUT rejection, with separate setup and assertion groups in one function. A failure in either independent endpoint flow prevents the other scenario from being identified and maintained separately.
Agent Prompt
## Issue description
A single integration test covers two independent endpoint behaviors: rejecting a mismatched name during creation and rejecting a rename during update.
## Fix Focus Areas
- tldw_Server_API/tests/Chat_Macros/integration/test_chat_macros_api.py[190-213]
## Recommended Fix
Split the function into separate create-rejection and update-rejection tests, keeping each test's setup and assertions focused on one HTTP operation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const profiles = normalizedProfiles(drafts) | ||
| const nextSettings = outputProfilesToSettings(originalSettings, profiles) |
There was a problem hiding this comment.
1. Profile saves revert macro toggles 🐞 Bug ≡ Correctness
saveProfiles rebuilds its PUT payload from the editor's initial originalSettings snapshot, and the settings endpoint persists that complete mapping by replacing the stored JSON wholesale. If a built-in or user macro is toggled, another settings field changes, or another session updates settings after the profile draft opens, saving profiles writes those stale values back alongside the edited output_profiles.
Agent Prompt
## Issue description
Saving output profiles submits a stale complete-settings snapshot and can overwrite macro enablement or other settings changed after the editor loaded.
## Fix Focus Areas
- apps/packages/ui/src/components/Option/Settings/OutputProfileEditor.tsx[267-308]
- apps/packages/ui/src/components/Option/Settings/ChatMacrosSettings.tsx[196-215]
- tldw_Server_API/app/api/v1/endpoints/chat_macros.py[246-268]
- tldw_Server_API/app/core/Chat_Macros/repository.py[186-199]
## Recommended Fix
Add an output-profile-specific backend operation that atomically reads the current settings and replaces only `output_profiles`, preserving every other current settings field, then update the editor to submit only the normalized profiles rather than constructing saves from a cached full-settings object. Alternatively, fetch current settings immediately before saving and merge only the edited `output_profiles`, ensuring persistence receives current values for all unrelated fields.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| setServerRaw(response.data.raw) | ||
| setSourceRaw(response.data.raw) | ||
| setValidationMessage(label("validationPassed", "Server validation passed.")) | ||
| onSaved(response.data.summary.name) |
There was a problem hiding this comment.
8. Late saves discard a newer draft 🐞 Bug ☼ Reliability
saveMacro awaits validation and persistence without checking whether selectedName or the editor request generation changed before publishing the result through onSaved. If a user selects and starts editing another macro during that request, the late completion selects the previously saved macro again and replaces the newer draft state.
Agent Prompt
## Issue description
A save started for one macro can complete after the user selects another macro and force the editor back to the old macro, discarding the newer draft.
## Fix Focus Areas
- apps/packages/ui/src/components/Option/Settings/ChatMacroEditor.tsx[324-366]
- apps/packages/ui/src/components/Option/Settings/ChatMacrosSettings.tsx[252-258]
## Recommended Fix
Capture the editor request generation and selected name when saving. After each await, publish validation state, server content, and `onSaved` only if that operation still belongs to the current selection; persistence may complete without changing the newer editor state.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const response = await deleteChatMacro(selected.name) | ||
| if (!response.ok) { | ||
| setValidationError(responseError(response.status, response.error)) | ||
| return | ||
| } | ||
| onDeleted(selected.name) |
There was a problem hiding this comment.
9. Late deletes clear a newer draft 🐞 Bug ☼ Reliability
removeMacro keeps using the originally selected macro across confirmation and deletion without checking whether the editor selection changed before calling onDeleted. If another macro is selected while deletion is pending, completion unconditionally clears that newer selection and reloads the editor, removing its unsaved draft from view.
Agent Prompt
## Issue description
A deletion initiated for one macro can complete after another macro is selected and clear the newer selection and draft.
## Fix Focus Areas
- apps/packages/ui/src/components/Option/Settings/ChatMacroEditor.tsx[390-413]
- apps/packages/ui/src/components/Option/Settings/ChatMacrosSettings.tsx[260-266]
## Recommended Fix
Capture the selected name and request generation before confirmation. After confirmation and deletion, refresh the catalog but invoke selection-clearing behavior only when the editor still displays the macro that was deleted; otherwise preserve the current selection and draft.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const copyYaml = async () => { | ||
| if (loading || !hasCurrentDetail) return | ||
| const raw = serverRaw || rawForSave() | ||
| if (raw === null) return | ||
| try { | ||
| await navigator.clipboard.writeText(raw) | ||
| setValidationError(null) | ||
| setValidationMessage(label("copied", "YAML copied.")) | ||
| } catch { | ||
| setValidationError(label("copyError", "Unable to copy YAML.")) | ||
| } | ||
| } | ||
|
|
||
| const downloadYaml = () => { | ||
| if (loading || !hasCurrentDetail) return | ||
| const raw = serverRaw || rawForSave() | ||
| if (raw === null) return | ||
| const name = selected?.name || draft.name || "macro" | ||
| downloadBlob(new Blob([raw], { type: "text/yaml" }), `${name}.yaml`) | ||
| } | ||
|
|
||
| const removeMacro = async () => { | ||
| if (!selected || isBuiltin || isBusy || loading || !hasCurrentDetail) return | ||
| setBusyAction("delete") | ||
| try { | ||
| const confirmed = await confirmDanger({ |
There was a problem hiding this comment.
3. Macro exports discard unsaved edits 🐞 Bug ≡ Correctness
copyYaml and downloadYaml prefer the populated serverRaw value over rawForSave(), while guided and source edits update only draft or sourceRaw. After a previously loaded or saved macro is edited without another save, both handlers export the old server content instead of the visible draft.
Agent Prompt
## Issue description
Copying or downloading an edited existing macro exports the stale server response instead of the current unsaved editor state, causing clipboard content and downloaded files to omit visible guided or source-mode edits.
## Fix Focus Areas
- apps/packages/ui/src/components/Option/Settings/ChatMacroEditor.tsx[216-230]
- apps/packages/ui/src/components/Option/Settings/ChatMacroEditor.tsx[369-394]
- apps/packages/ui/src/components/Option/Settings/ChatMacroEditor.tsx[674-685]
## Recommended Fix
Build the export payload with `rawForSave()` first in both handlers so it reflects the current guided or source-mode draft. Reserve `serverRaw` as a fallback only when generating the current draft is not applicable; if needed, track whether the draft differs from the server version or clear `serverRaw` whenever guided fields or source YAML change so stale server content cannot override dirty editor state.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| React.useEffect(() => { | ||
| const nextDrafts = cloneProfiles(settings.output_profiles) | ||
| setOriginalSettings(settings) | ||
| setDrafts(nextDrafts) | ||
| setSelectedProfile((current) => (nextDrafts[current] ? current : firstProfileName(nextDrafts))) | ||
| setNewProfileName("") | ||
| setError(null) | ||
| setStatus(null) | ||
| }, [settings]) |
There was a problem hiding this comment.
4. Refreshing settings erases profile drafts 🐞 Bug ☼ Reliability
OutputProfileEditor resets originalSettings and all profile drafts whenever its settings prop changes. Using the new header refresh while editing profiles reloads settings and removes unsaved section, heading, or profile changes despite the editor remaining mounted across tabs.
Agent Prompt
Issue description
Refreshing the settings page replaces unsaved output-profile editor changes with the server response.
Fix Focus Areas
- apps/packages/ui/src/components/Option/Settings/OutputProfileEditor.tsx[95-103]
- apps/packages/ui/src/components/Option/Settings/ChatMacrosSettings.tsx[312-315]
Recommended Fix
Track whether profile drafts are dirty and do not replace them when refreshed settings arrive. Either retain the current draft until an explicit discard action or prompt the user before applying refreshed settings; only initialize drafts from settings when there are no local edits.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| for (const [profileName, profile] of Object.entries(profiles)) { | ||
| if (!PROFILE_KEY.test(profileName)) { | ||
| errors.push(label("validation.profileKey", "Profile names must use lowercase letters, numbers, and underscores.")) | ||
| } |
There was a problem hiding this comment.
10. Some existing profiles cannot be saved 🐞 Bug ≡ Correctness
validationErrors rejects every loaded profile name that does not match the newly introduced lowercase-and-underscore expression. The backend accepts profile map keys without this restriction, so an existing profile such as Review-Notes prevents saving even unrelated valid profile edits.
Agent Prompt
Issue description
The profile editor refuses to save backend-accepted existing profile names, preventing users from editing their profile settings.
Fix Focus Areas
- apps/packages/ui/src/components/Option/Settings/OutputProfileEditor.tsx[211-237]
- tldw_Server_API/app/core/Chat_Macros/settings.py[41-53]
- tldw_Server_API/app/core/Chat_Macros/output_profiles.py[48-84]
Recommended Fix
Align frontend validation with the backend contract. Either add the same profile-name constraint to backend normalization and provide a migration path, or validate the naming rule only when creating new profiles while allowing existing backend-accepted names to be saved unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Summary
Chat Macros settings now lets users create, edit, validate, import, export, clone, and delete custom macros. Guided fields cover the standard parallel-branch and merge workflow; advanced definitions remain editable as canonical YAML. Built-ins remain immutable, and imported or edited drafts survive catalog refreshes and tab changes.
Named output profiles can return a single response or ordered sections with custom headings and optional branch outputs. Saves preserve unrelated settings. The backend enforces matching resource and YAML names and validates bounded section headings through the existing per-user storage and validation APIs. This reuses the v1 Jobs execution model without adding a new runtime or dependencies.
Rebased onto
devatc70387f496. Post-rebase verification also corrected a hook dependency without reloading drafts and stacked editor sections to prevent cramped labels inside the settings layout.Backlog: TASK-13114. Design and implementation plan are included in
Docs/superpowers/.Validation
git diff --checkpassed.Risk & Rollback
Medium risk: this expands user-managed YAML and settings editing. Existing server-side validation, immutable built-ins, ownership boundaries, source limits, and destructive-action confirmation remain authoritative. Revert this PR to restore the v1 settings UI; no database migration is introduced.
Change Summary
Human requester summary for this v1.1 change is pending. The earlier v1 summary describes a different PR. This agent-authored description does not satisfy the repository's human-written merge gate.
Summary by cubic
Lets users create, edit, validate, import, export, clone, and delete custom Chat Macros from the settings UI, which previously only exposed a read-only macro list (TASK-13114). Adds a structured output profile editor with bounded section headings and keeps built-in macros immutable.
New Features
Backend
Written for commit 25c8b47. Summary will update on new commits.