Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an AI assistant feature spanning backend and frontend: Laravel AI config and dependency, aiChat API and tests, NGXS-powered Angular assistant UI/service/state, markdown pipe, siteTexts re-render support, many skill documentation files, and assorted config and CI updates. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as AiAssistantComponent
participant Store as NGXS Store
participant Service as AiAssistantService
participant API as Laravel StateController
participant Agent as AssistantAgent
User->>UI: open/send message
UI->>Store: Dispatch SendAiMessageAction
Store->>Service: chat(message, history, site, template, changeHistory)
Service->>API: POST /ai-chat
API->>Agent: forward request
Agent-->>API: structured AiChatResponse
API-->>Service: response
Service-->>Store: emit AiChatResponse
Store->>Store: Dispatch AiMessageReceivedAction, update changeHistory, apply changes (settings/sections/entries)
Store-->>UI: update messages/isLoading
UI->>User: render reply and applied updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
_api_app/app/Http/Middleware/SetupMiddleware.php (2)
26-50:⚠️ Potential issue | 🟡 MinorAdd missing return type and curly braces for control structures.
The method violates coding guidelines:
- Missing explicit return type declaration (should be
: void)- Control structures on lines 33-39 lack curly braces
As per coding guidelines: "Always use curly braces for control structures, even for single-line statements" and "Always use explicit return type declarations for methods and functions."
📝 Proposed fix
- protected function updateEnvFile() + protected function updateEnvFile(): void { $env_example = $this->parseEnvFile(base_path() . '/.env.example'); $env = $this->parseEnvFile(base_path() . '/.env'); $env_new = []; foreach ($env_example as $key => $value) { - if (isset($env[$key])) { + if (isset($env[$key])) { $env_new[$key] = $env[$key]; - } elseif ($key == 'APP_KEY' || $key == 'APP_ID') { + } elseif ($key == 'APP_KEY' || $key == 'APP_ID') { $env_new[$key] = Helpers::uuid_v4(); - } else { + } else { $env_new[$key] = $value; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@_api_app/app/Http/Middleware/SetupMiddleware.php` around lines 26 - 50, The updateEnvFile method is missing an explicit return type and uses control structures without braces; update the method signature to include a void return type (function updateEnvFile(): void) and add curly braces for the if/elseif/else blocks inside the loop that builds $env_new (the checks using isset($env[$key]), $key == 'APP_KEY' || $key == 'APP_ID', and the final else), ensuring each branch has braces, and keep the rest of the logic (parseEnvFile calls, Helpers::uuid_v4(), building $content and file_put_contents) unchanged.
52-74:⚠️ Potential issue | 🟡 MinorAdd missing type declarations and curly braces for control structures.
The method violates coding guidelines:
- Missing explicit return type declaration (should be
: array)- Missing parameter type hint for
$file(should bestring)- Control structures on lines 56-58 and 62-64 lack curly braces
As per coding guidelines: "Always use curly braces for control structures, even for single-line statements", "Always use explicit return type declarations for methods and functions", and "Use appropriate PHP type hints for method parameters."
📝 Proposed fix
- protected function parseEnvFile($file) + protected function parseEnvFile(string $file): array { $res = []; - if (! file_exists($file)) { + if (! file_exists($file)) { return $res; + } $content = trim(file_get_contents(realpath($file))); - if (empty($content)) { + if (empty($content)) { return $res; + } $rows = preg_split('/\s+/', $content); // Loop through given data foreach ((array) $rows as $key => $value) { [$key, $value] = explode('=', $value, 2); $res[$key] = $value; } return $res; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@_api_app/app/Http/Middleware/SetupMiddleware.php` around lines 52 - 74, The parseEnvFile method lacks a parameter type and explicit return type and uses single-line control structures without braces; update the function signature parseEnvFile($file) to parseEnvFile(string $file): array, add curly braces for the if (! file_exists($file)) { ... }, if (empty($content)) { ... } and the foreach loop body foreach ((array) $rows as $key => $value) { ... }, and ensure the method still returns the $res array at the end.
🧹 Nitpick comments (8)
_api_app/app/Http/Controllers/StateController.php (1)
136-147: LGTM!Adding the explicit
JsonResponsereturn type aligns with coding guidelines and improves type safety.For consistency, consider adding return type declarations to other methods in this controller (
get(),getMeta(),getSentryDSN()) in a follow-up PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@_api_app/app/Http/Controllers/StateController.php` around lines 136 - 147, The getLocaleSettings method now has an explicit JsonResponse return type; apply the same consistency to the other controller methods by adding appropriate return type declarations: update get(), getMeta(), and getSentryDSN() to declare their return types (e.g., JsonResponse or Response as appropriate for each method's actual return value) so signatures match getLocaleSettings and improve type safety and consistency across the controller._api_app/app/Http/Middleware/SetupMiddleware.php (1)
66-71: Consider using a robust .env parser library.The current parsing logic splits on whitespace and may fail for:
- Multi-line values
- Values containing spaces
- Malformed lines without
=separatorConsider using a well-tested library like
vlucas/phpdotenv(already a Laravel dependency) or Laravel's config layer instead of custom parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@_api_app/app/Http/Middleware/SetupMiddleware.php` around lines 66 - 71, Replace the fragile custom parser in SetupMiddleware (the preg_split on $content, the foreach over $rows and the explode('=',$value,2) that populates $res) with a robust .env parsing approach: use vlucas/phpdotenv (or Laravel's built-in env/config loader) to parse $content into key/value pairs and populate the $res array, handling multi-line values, spaces and malformed lines safely; update the code to call Dotenv or the appropriate loader API instead of manually splitting by whitespace and exploding on '=' and remove the fragile foreach parsing logic.editor/src/app/ai-assistant/ai-assistant.state.ts (2)
175-189: Multiple dispatch calls could be batched for efficiency.Each iteration dispatches a separate action. While functionally correct, NGXS allows dispatching an array of actions in a single call for better performance.
♻️ Proposed refactor to batch dispatches
- for (const change of action.designChanges) { - dispatch( - new UpdateSiteTemplateSettingsAction(change.group, { - [change.setting]: change.value, - }), - ); - } - - for (const change of action.settingsChanges) { - dispatch( - new UpdateSiteSettingsAction(change.group, { - [change.setting]: change.value, - }), - ); - } + const actions = [ + ...action.designChanges.map( + (change) => + new UpdateSiteTemplateSettingsAction(change.group, { + [change.setting]: change.value, + }), + ), + ...action.settingsChanges.map( + (change) => + new UpdateSiteSettingsAction(change.group, { + [change.setting]: change.value, + }), + ), + ]; + + if (actions.length > 0) { + dispatch(actions); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/src/app/ai-assistant/ai-assistant.state.ts` around lines 175 - 189, Currently each loop calls dispatch per change which is inefficient; instead map action.designChanges to an array of new UpdateSiteTemplateSettingsAction(change.group, {[change.setting]: change.value}) and map action.settingsChanges to an array of new UpdateSiteSettingsAction(change.group, {[change.setting]: change.value}), concatenate those two arrays (preserving order) and call dispatch once with the combined array; locate the loops where action.designChanges and action.settingsChanges are iterated and replace them with the single batched dispatch call using the generated action instances.
127-131: Consider providing user-facing error feedback.When the AI request fails, the error is logged to console and loading state is cleared, but the user receives no visible indication of what went wrong. Consider adding an error message to the messages array or dispatching a notification action.
💡 Proposed enhancement for error feedback
catchError((error) => { console.error('AI assistant error:', error); - patchState({ isLoading: false }); + const errorMessage: AiMessage = { + role: 'assistant', + content: 'Sorry, something went wrong. Please try again.', + }; + patchState({ + messages: [...getState().messages, errorMessage], + isLoading: false, + }); return EMPTY; }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/src/app/ai-assistant/ai-assistant.state.ts` around lines 127 - 131, The catchError block in the AI assistant observable only logs and clears isLoading, so add user-facing feedback by appending an error message to the messages state or dispatching a notification action: inside the catchError in ai-assistant.state.ts update patchState to both set isLoading: false and push a new message object into the messages array (e.g., a system/error message with a clear text like "AI request failed") or call your notification action (e.g., dispatch(NotificationActions.showError(...))). Ensure you reference the same patchState and messages state shape used elsewhere so the UI displays the error to the user.editor/src/app/ai-assistant/ai-assistant.actions.ts (1)
10-18: Consider reusingAiChangeIteminterface to reduce duplication.The inline type
{ group: string; setting: string; value: string; previous_value?: string | null }[]duplicates theAiChangeIteminterface defined inai-assistant.service.ts. Reusing the interface improves maintainability.♻️ Proposed refactor to reuse interface
+import { AiChangeItem } from './ai-assistant.service'; + export class AiMessageReceivedAction { static readonly type = 'AI_ASSISTANT:MESSAGE_RECEIVED'; constructor( public reply: string, - public designChanges: { group: string; setting: string; value: string; previous_value?: string | null }[], - public settingsChanges: { group: string; setting: string; value: string; previous_value?: string | null }[], + public designChanges: AiChangeItem[], + public settingsChanges: AiChangeItem[], public isUndo: boolean = false, ) {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/src/app/ai-assistant/ai-assistant.actions.ts` around lines 10 - 18, The inline change item type in AiMessageReceivedAction duplicates the AiChangeItem interface; update the constructor signatures for designChanges and settingsChanges to use AiChangeItem[] instead of the inline type and add an import for AiChangeItem where it’s defined (so AiMessageReceivedAction references the shared interface), ensuring any nullable previous_value semantics match AiChangeItem.editor/src/app/rerender/template-rerender.service.ts (1)
96-99: Minor inconsistency: null check not used for other cases.The
if (info.siteTexts)guard is present here but not for other cases likeBANNERS_SETTINGS,SETTINGS, orENTRY_LAYOUT(lines 84-92). SincesiteTextsis a required field inSiteSettingChildrenHandler(per context snippet 1), this check is redundant. However, if this is intentional defensive coding for a newly added field, it's acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/src/app/rerender/template-rerender.service.ts` around lines 96 - 99, The SITE_TEXTS branch currently has an extra conditional guard (if (info.siteTexts)) while other branches like BANNERS_SETTINGS, SETTINGS, and ENTRY_LAYOUT directly push their fields; to keep consistency remove the redundant null-check and change the SITE_TEXTS case to simply push info.siteTexts into compList (i.e., compList.push(info.siteTexts);) so it matches the pattern used in the other cases and relies on the type contract of SiteSettingChildrenHandler._api_app/.env.example (1)
9-10: Consider alphabetically ordering environment variables.The static analysis tool suggests placing
AI_DEFAULT_PROVIDERandANTHROPIC_API_KEYbeforeAPI_PREFIXfor alphabetical consistency. This is a minor style consideration that improves maintainability as the file grows.♻️ Suggested ordering
APP_ENV=production APP_DEBUG=false +AI_DEFAULT_PROVIDER=anthropic +ANTHROPIC_API_KEY= CACHE_STORE=file APP_KEY=[YOUR_APP_KEY] APP_ID=[YOUR_APP_ID] API_PREFIX=_api SENTRY_DSN= SENTRY_FRONTEND_DSN= -AI_DEFAULT_PROVIDER=anthropic -ANTHROPIC_API_KEY=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@_api_app/.env.example` around lines 9 - 10, Reorder the environment variable declarations so they are alphabetized: move AI_DEFAULT_PROVIDER and ANTHROPIC_API_KEY so they appear before API_PREFIX in .env.example; ensure the variables (AI_DEFAULT_PROVIDER, ANTHROPIC_API_KEY, API_PREFIX) are in ascending alphabetical order and keep the rest of the file formatting unchanged._api_app/.github/skills/ai-sdk-development/SKILL.md (1)
1-413: Consider a single-source approach for duplicated skill docs.This content is duplicated across
.github/.cursor/.agentsvariants, which will be hard to keep in sync. A template/generation step would reduce drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@_api_app/.github/skills/ai-sdk-development/SKILL.md` around lines 1 - 413, The ai-sdk-development SKILL.md is duplicated across multiple variants; consolidate into a single source (e.g., a template or canonical SKILL.md for the ai-sdk-development skill) and update the duplicated copies to be generated or included from that canonical file; modify the generator/script (or add a small build step/CI job) to produce the variant files from the canonical ai-sdk-development SKILL.md (or SKILL.md.template) and update references to the skill name "ai-sdk-development" so all variants pull from the single source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@_api_app/.codex/config.toml`:
- Around line 1-12: The config contains hardcoded absolute paths (cwd and
SITE_PATH in the mcp_servers sections: mcp_servers.laravel-boost,
mcp_servers.herd, and mcp_servers.herd.env) which break for other developers;
replace those absolute paths with relative paths or environment-variable
references (e.g., use "./_api_app" or a variable like ${SITE_PATH}) or remove
the file from VCS and add a template (e.g., config.toml.example) and add this
file to .gitignore so each developer can configure their own local cwd and
SITE_PATH values.
In `@_api_app/AGENTS.md`:
- Line 248: Replace the incorrect skill name reference "developing-with-ai-sdk"
with the defined skill name "ai-sdk-development" so activation uses the
canonical identifier; update any occurrence in the AGENTS.md text (the line that
currently reads Activate the `developing-with-ai-sdk` skill) to read Activate
the `ai-sdk-development` skill to ensure the skill activates correctly.
In `@_api_app/CLAUDE.md`:
- Around line 248-249: The skill name string in the CLAUDE.md content is
incorrect: replace the lone occurrence of "developing-with-ai-sdk" with the
canonical skill id "ai-sdk-development" to match boost.json and the SKILL.md
frontmatter; verify any other occurrences in this file use "ai-sdk-development"
so the skill activation string in the documentation matches the
boost.json/SKILL.md definition.
In `@_api_app/opencode.json`:
- Around line 20-22: The SITE_PATH in the "environment" object is a hardcoded,
user-specific absolute path; replace it with a portable solution by: change
SITE_PATH to use a relative path or an environment variable reference (e.g. read
from process env at runtime) and remove or template the absolute value in
opencode.json so it no longer contains "/Users/uldis/..."; alternatively, mark
this file as developer-specific by adding it to .gitignore or provide a sample
file (e.g. opencode.example.json) with a placeholder for SITE_PATH so other
developers/CI can set their own path.
In `@_api_app/tests/Feature/AiChatControllerTest.php`:
- Around line 10-16: The test "returns 401 when unauthenticated" uses
assertStatus(401) — change this to the more specific assertion by replacing
assertStatus(401) with assertUnauthorized() on the response from
post(route('ai_chat'), [...]) so the test uses the preferred Laravel assertion;
locate the anonymous test function (it('returns 401 when unauthenticated', ...))
and update the assertion accordingly.
In `@editor/src/app/ai-assistant/ai-assistant.component.ts`:
- Around line 40-51: The template is inserting rendered HTML via
[innerHTML]="msg.content | markdown" which relies on the markdown pipe to bypass
Angular XSS protections; update the markdown.pipe (used by
ai-assistant.component template) to treat AI-generated content as untrusted:
parse markdown to HTML, run the HTML through a robust sanitizer (e.g.,
DOMPurify) inside the MarkdownPipe, and only then, if necessary, use
DomSanitizer.bypassSecurityTrustHtml on the already-sanitized output; ensure the
pipe is named MarkdownPipe (or the exported pipe symbol used in the template)
and that ai-assistant.component continues to consume messages$ / msg.content
without changing template bindings.
In `@editor/src/app/pipes/markdown.pipe.ts`:
- Line 9: The transform method should guard against null/undefined input before
calling marked.parse; update transform(value: string): SafeHtml in the markdown
pipe to return a safe empty string or a default SafeHtml (e.g.,
sanitizer.bypassSecurityTrustHtml('')) when value is null/undefined, and only
call marked.parse(value) when value is a non-empty string; ensure the return
type SafeHtml is preserved and reference marked.parse and transform to locate
the change.
- Around line 9-17: The transform method in markdown.pipe.ts uses marked.parse
with a custom Renderer and then calls this.sanitizer.bypassSecurityTrustHtml
without sanitizing or escaping attributes, which allows raw HTML, script tags
and attribute injection (href, title, text) — fix by installing and importing
DOMPurify, run DOMPurify.sanitize on the string returned by marked.parse (pass a
safe ALLOWED_TAGS/ALLOWED_ATTRS config and allow target/rel if needed), stop
interpolating raw attribute values by ensuring marked's renderer produces safe
attributes (or escape href/title/text before constructing the <a>), and only
call this.sanitizer.bypassSecurityTrustHtml on the sanitized output; keep
references to transform, Renderer, marked.parse and
this.sanitizer.bypassSecurityTrustHtml when making the change.
---
Outside diff comments:
In `@_api_app/app/Http/Middleware/SetupMiddleware.php`:
- Around line 26-50: The updateEnvFile method is missing an explicit return type
and uses control structures without braces; update the method signature to
include a void return type (function updateEnvFile(): void) and add curly braces
for the if/elseif/else blocks inside the loop that builds $env_new (the checks
using isset($env[$key]), $key == 'APP_KEY' || $key == 'APP_ID', and the final
else), ensuring each branch has braces, and keep the rest of the logic
(parseEnvFile calls, Helpers::uuid_v4(), building $content and
file_put_contents) unchanged.
- Around line 52-74: The parseEnvFile method lacks a parameter type and explicit
return type and uses single-line control structures without braces; update the
function signature parseEnvFile($file) to parseEnvFile(string $file): array, add
curly braces for the if (! file_exists($file)) { ... }, if (empty($content)) {
... } and the foreach loop body foreach ((array) $rows as $key => $value) { ...
}, and ensure the method still returns the $res array at the end.
---
Nitpick comments:
In `@_api_app/.env.example`:
- Around line 9-10: Reorder the environment variable declarations so they are
alphabetized: move AI_DEFAULT_PROVIDER and ANTHROPIC_API_KEY so they appear
before API_PREFIX in .env.example; ensure the variables (AI_DEFAULT_PROVIDER,
ANTHROPIC_API_KEY, API_PREFIX) are in ascending alphabetical order and keep the
rest of the file formatting unchanged.
In `@_api_app/.github/skills/ai-sdk-development/SKILL.md`:
- Around line 1-413: The ai-sdk-development SKILL.md is duplicated across
multiple variants; consolidate into a single source (e.g., a template or
canonical SKILL.md for the ai-sdk-development skill) and update the duplicated
copies to be generated or included from that canonical file; modify the
generator/script (or add a small build step/CI job) to produce the variant files
from the canonical ai-sdk-development SKILL.md (or SKILL.md.template) and update
references to the skill name "ai-sdk-development" so all variants pull from the
single source.
In `@_api_app/app/Http/Controllers/StateController.php`:
- Around line 136-147: The getLocaleSettings method now has an explicit
JsonResponse return type; apply the same consistency to the other controller
methods by adding appropriate return type declarations: update get(), getMeta(),
and getSentryDSN() to declare their return types (e.g., JsonResponse or Response
as appropriate for each method's actual return value) so signatures match
getLocaleSettings and improve type safety and consistency across the controller.
In `@_api_app/app/Http/Middleware/SetupMiddleware.php`:
- Around line 66-71: Replace the fragile custom parser in SetupMiddleware (the
preg_split on $content, the foreach over $rows and the explode('=',$value,2)
that populates $res) with a robust .env parsing approach: use vlucas/phpdotenv
(or Laravel's built-in env/config loader) to parse $content into key/value pairs
and populate the $res array, handling multi-line values, spaces and malformed
lines safely; update the code to call Dotenv or the appropriate loader API
instead of manually splitting by whitespace and exploding on '=' and remove the
fragile foreach parsing logic.
In `@editor/src/app/ai-assistant/ai-assistant.actions.ts`:
- Around line 10-18: The inline change item type in AiMessageReceivedAction
duplicates the AiChangeItem interface; update the constructor signatures for
designChanges and settingsChanges to use AiChangeItem[] instead of the inline
type and add an import for AiChangeItem where it’s defined (so
AiMessageReceivedAction references the shared interface), ensuring any nullable
previous_value semantics match AiChangeItem.
In `@editor/src/app/ai-assistant/ai-assistant.state.ts`:
- Around line 175-189: Currently each loop calls dispatch per change which is
inefficient; instead map action.designChanges to an array of new
UpdateSiteTemplateSettingsAction(change.group, {[change.setting]: change.value})
and map action.settingsChanges to an array of new
UpdateSiteSettingsAction(change.group, {[change.setting]: change.value}),
concatenate those two arrays (preserving order) and call dispatch once with the
combined array; locate the loops where action.designChanges and
action.settingsChanges are iterated and replace them with the single batched
dispatch call using the generated action instances.
- Around line 127-131: The catchError block in the AI assistant observable only
logs and clears isLoading, so add user-facing feedback by appending an error
message to the messages state or dispatching a notification action: inside the
catchError in ai-assistant.state.ts update patchState to both set isLoading:
false and push a new message object into the messages array (e.g., a
system/error message with a clear text like "AI request failed") or call your
notification action (e.g., dispatch(NotificationActions.showError(...))). Ensure
you reference the same patchState and messages state shape used elsewhere so the
UI displays the error to the user.
In `@editor/src/app/rerender/template-rerender.service.ts`:
- Around line 96-99: The SITE_TEXTS branch currently has an extra conditional
guard (if (info.siteTexts)) while other branches like BANNERS_SETTINGS,
SETTINGS, and ENTRY_LAYOUT directly push their fields; to keep consistency
remove the redundant null-check and change the SITE_TEXTS case to simply push
info.siteTexts into compList (i.e., compList.push(info.siteTexts);) so it
matches the pattern used in the other cases and relies on the type contract of
SiteSettingChildrenHandler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4272ee19-9253-4285-827b-de9b8dcbc0b0
⛔ Files ignored due to path filters (2)
_api_app/composer.lockis excluded by!**/*.lockeditor/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (50)
CLAUDE.mdWARP.md_api_app/.agents/skills/ai-sdk-development/SKILL.md_api_app/.agents/skills/pest-testing/SKILL.md_api_app/.claude/skills/ai-sdk-development/SKILL.md_api_app/.claude/skills/pest-testing/SKILL.md_api_app/.codex/config.toml_api_app/.cursor/skills/ai-sdk-development/SKILL.md_api_app/.cursor/skills/pest-testing/SKILL.md_api_app/.env.example_api_app/.github/skills/ai-sdk-development/SKILL.md_api_app/.github/skills/pest-testing/SKILL.md_api_app/AGENTS.md_api_app/CLAUDE.md_api_app/app/Http/Controllers/StateController.php_api_app/app/Http/Middleware/Authenticate.php_api_app/app/Http/Middleware/SetupMiddleware.php_api_app/app/User/UserModel.php_api_app/boost.json_api_app/bootstrap/providers.php_api_app/composer.json_api_app/config/ai.php_api_app/config/auth.php_api_app/config/sanctum.php_api_app/config/twigbridge.php_api_app/database/factories/UserFactory.php_api_app/opencode.json_api_app/tests/Feature/AiChatControllerTest.php_api_app/tests/Pest.phpeditor/package.jsoneditor/src/app/ai-assistant/ai-assistant.actions.tseditor/src/app/ai-assistant/ai-assistant.component.tseditor/src/app/ai-assistant/ai-assistant.module.tseditor/src/app/ai-assistant/ai-assistant.service.tseditor/src/app/ai-assistant/ai-assistant.state.tseditor/src/app/app.component.tseditor/src/app/app.module.tseditor/src/app/header/header.component.tseditor/src/app/pipes/markdown.pipe.tseditor/src/app/pipes/pipes.module.tseditor/src/app/pipes/safe.pipe.tseditor/src/app/rerender/default-template-rerender.service.tseditor/src/app/rerender/mashup/mashup-template-rerender.service.tseditor/src/app/rerender/messy/messy-template-rerender.service.tseditor/src/app/rerender/template-rerender.service.tseditor/src/app/rerender/types/components.tseditor/src/app/rerender/white/white-template-rerender.service.tseditor/src/app/sites/sections/site-sections.module.tseditor/src/app/sites/settings/site-settings.state.tseditor/src/app/user/user.state.ts
💤 Files with no reviewable changes (2)
- WARP.md
- _api_app/app/Http/Middleware/Authenticate.php
_api_app/.codex/config.toml
Outdated
Comment on lines
+1
to
+12
| [mcp_servers.laravel-boost] | ||
| command = "php" | ||
| args = ["artisan", "boost:mcp"] | ||
| cwd = "/Users/uldis/projects/berta/berta/_api_app" | ||
|
|
||
| [mcp_servers.herd] | ||
| command = "php" | ||
| args = ["/Applications/Herd.app/Contents/Resources/herd-mcp.phar"] | ||
| cwd = "/Users/uldis/projects/berta/berta/_api_app" | ||
|
|
||
| [mcp_servers.herd.env] | ||
| SITE_PATH = "/Users/uldis/projects/berta/berta/_api_app" |
Contributor
There was a problem hiding this comment.
Hardcoded absolute paths will break for other developers.
The configuration contains machine-specific paths (/Users/uldis/projects/berta/berta/_api_app) that won't work for other team members. Consider using relative paths or removing this file from version control if it's meant for local development only.
🔧 Suggested fix using relative paths
[mcp_servers.laravel-boost]
command = "php"
args = ["artisan", "boost:mcp"]
-cwd = "/Users/uldis/projects/berta/berta/_api_app"
+cwd = "."
[mcp_servers.herd]
command = "php"
args = ["/Applications/Herd.app/Contents/Resources/herd-mcp.phar"]
-cwd = "/Users/uldis/projects/berta/berta/_api_app"
+cwd = "."
[mcp_servers.herd.env]
-SITE_PATH = "/Users/uldis/projects/berta/berta/_api_app"
+SITE_PATH = "."Alternatively, add this file to .gitignore and provide a .codex/config.toml.example template instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [mcp_servers.laravel-boost] | |
| command = "php" | |
| args = ["artisan", "boost:mcp"] | |
| cwd = "/Users/uldis/projects/berta/berta/_api_app" | |
| [mcp_servers.herd] | |
| command = "php" | |
| args = ["/Applications/Herd.app/Contents/Resources/herd-mcp.phar"] | |
| cwd = "/Users/uldis/projects/berta/berta/_api_app" | |
| [mcp_servers.herd.env] | |
| SITE_PATH = "/Users/uldis/projects/berta/berta/_api_app" | |
| [mcp_servers.laravel-boost] | |
| command = "php" | |
| args = ["artisan", "boost:mcp"] | |
| cwd = "." | |
| [mcp_servers.herd] | |
| command = "php" | |
| args = ["/Applications/Herd.app/Contents/Resources/herd-mcp.phar"] | |
| cwd = "." | |
| [mcp_servers.herd.env] | |
| SITE_PATH = "." |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@_api_app/.codex/config.toml` around lines 1 - 12, The config contains
hardcoded absolute paths (cwd and SITE_PATH in the mcp_servers sections:
mcp_servers.laravel-boost, mcp_servers.herd, and mcp_servers.herd.env) which
break for other developers; replace those absolute paths with relative paths or
environment-variable references (e.g., use "./_api_app" or a variable like
${SITE_PATH}) or remove the file from VCS and add a template (e.g.,
config.toml.example) and add this file to .gitignore so each developer can
configure their own local cwd and SITE_PATH values.
|
|
||
| ## Test Enforcement | ||
| - This application uses the Laravel AI SDK (`laravel/ai`) for all AI functionality. | ||
| - Activate the `developing-with-ai-sdk` skill when building, editing, updating, debugging, or testing AI agents, text generation, chat, streaming, structured output, tools, image generation, audio, transcription, embeddings, reranking, vector stores, files, conversation memory, or any AI provider integration (OpenAI, Anthropic, Gemini, Cohere, Groq, xAI, ElevenLabs, Jina, OpenRouter). |
Contributor
There was a problem hiding this comment.
Fix incorrect AI skill name reference.
Line 248 references developing-with-ai-sdk, but the defined skill name is ai-sdk-development (see Line 29). This mismatch can prevent correct skill activation.
🛠️ Suggested fix
-- Activate the `developing-with-ai-sdk` skill when building, editing, updating, debugging, or testing AI agents, text generation, chat, streaming, structured output, tools, image generation, audio, transcription, embeddings, reranking, vector stores, files, conversation memory, or any AI provider integration (OpenAI, Anthropic, Gemini, Cohere, Groq, xAI, ElevenLabs, Jina, OpenRouter).
+- Activate the `ai-sdk-development` skill when building, editing, updating, debugging, or testing AI agents, text generation, chat, streaming, structured output, tools, image generation, audio, transcription, embeddings, reranking, vector stores, files, conversation memory, or any AI provider integration (OpenAI, Anthropic, Gemini, Cohere, Groq, xAI, ElevenLabs, Jina, OpenRouter).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Activate the `developing-with-ai-sdk` skill when building, editing, updating, debugging, or testing AI agents, text generation, chat, streaming, structured output, tools, image generation, audio, transcription, embeddings, reranking, vector stores, files, conversation memory, or any AI provider integration (OpenAI, Anthropic, Gemini, Cohere, Groq, xAI, ElevenLabs, Jina, OpenRouter). | |
| - Activate the `ai-sdk-development` skill when building, editing, updating, debugging, or testing AI agents, text generation, chat, streaming, structured output, tools, image generation, audio, transcription, embeddings, reranking, vector stores, files, conversation memory, or any AI provider integration (OpenAI, Anthropic, Gemini, Cohere, Groq, xAI, ElevenLabs, Jina, OpenRouter). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@_api_app/AGENTS.md` at line 248, Replace the incorrect skill name reference
"developing-with-ai-sdk" with the defined skill name "ai-sdk-development" so
activation uses the canonical identifier; update any occurrence in the AGENTS.md
text (the line that currently reads Activate the `developing-with-ai-sdk` skill)
to read Activate the `ai-sdk-development` skill to ensure the skill activates
correctly.
Comment on lines
+248
to
249
| - Activate the `developing-with-ai-sdk` skill when building, editing, updating, debugging, or testing AI agents, text generation, chat, streaming, structured output, tools, image generation, audio, transcription, embeddings, reranking, vector stores, files, conversation memory, or any AI provider integration (OpenAI, Anthropic, Gemini, Cohere, Groq, xAI, ElevenLabs, Jina, OpenRouter). | ||
|
|
Contributor
There was a problem hiding this comment.
Skill name mismatch — should be ai-sdk-development.
The skill name here is developing-with-ai-sdk, but in boost.json and the SKILL.md frontmatter, it's defined as ai-sdk-development. This inconsistency may prevent proper skill activation.
Proposed fix
## Laravel AI SDK
- This application uses the Laravel AI SDK (`laravel/ai`) for all AI functionality.
-- Activate the `developing-with-ai-sdk` skill when building, editing, updating, debugging, or testing AI agents, text generation, chat, streaming, structured output, tools, image generation, audio, transcription, embeddings, reranking, vector stores, files, conversation memory, or any AI provider integration (OpenAI, Anthropic, Gemini, Cohere, Groq, xAI, ElevenLabs, Jina, OpenRouter).
+- Activate the `ai-sdk-development` skill when building, editing, updating, debugging, or testing AI agents, text generation, chat, streaming, structured output, tools, image generation, audio, transcription, embeddings, reranking, vector stores, files, conversation memory, or any AI provider integration (OpenAI, Anthropic, Gemini, Cohere, Groq, xAI, ElevenLabs, Jina, OpenRouter).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Activate the `developing-with-ai-sdk` skill when building, editing, updating, debugging, or testing AI agents, text generation, chat, streaming, structured output, tools, image generation, audio, transcription, embeddings, reranking, vector stores, files, conversation memory, or any AI provider integration (OpenAI, Anthropic, Gemini, Cohere, Groq, xAI, ElevenLabs, Jina, OpenRouter). | |
| - Activate the `ai-sdk-development` skill when building, editing, updating, debugging, or testing AI agents, text generation, chat, streaming, structured output, tools, image generation, audio, transcription, embeddings, reranking, vector stores, files, conversation memory, or any AI provider integration (OpenAI, Anthropic, Gemini, Cohere, Groq, xAI, ElevenLabs, Jina, OpenRouter). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@_api_app/CLAUDE.md` around lines 248 - 249, The skill name string in the
CLAUDE.md content is incorrect: replace the lone occurrence of
"developing-with-ai-sdk" with the canonical skill id "ai-sdk-development" to
match boost.json and the SKILL.md frontmatter; verify any other occurrences in
this file use "ai-sdk-development" so the skill activation string in the
documentation matches the boost.json/SKILL.md definition.
Comment on lines
+10
to
+16
| it('returns 401 when unauthenticated', function () { | ||
| post(route('ai_chat'), [ | ||
| 'message' => 'Make the background blue', | ||
| 'site' => '', | ||
| 'template' => 'default', | ||
| ])->assertStatus(401); | ||
| })->skip(! $pluginInstalled, 'AiAssistant plugin not installed'); |
Contributor
There was a problem hiding this comment.
Use assertUnauthorized() instead of assertStatus(401).
As per coding guidelines, prefer specific response assertion methods over generic assertStatus().
Proposed fix
it('returns 401 when unauthenticated', function () {
post(route('ai_chat'), [
'message' => 'Make the background blue',
'site' => '',
'template' => 'default',
- ])->assertStatus(401);
+ ])->assertUnauthorized();
})->skip(! $pluginInstalled, 'AiAssistant plugin not installed');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@_api_app/tests/Feature/AiChatControllerTest.php` around lines 10 - 16, The
test "returns 401 when unauthenticated" uses assertStatus(401) — change this to
the more specific assertion by replacing assertStatus(401) with
assertUnauthorized() on the response from post(route('ai_chat'), [...]) so the
test uses the preferred Laravel assertion; locate the anonymous test function
(it('returns 401 when unauthenticated', ...)) and update the assertion
accordingly.
Comment on lines
+40
to
+51
| @for (msg of messages$ | async; track $index) { | ||
| <div | ||
| class="ai-message" | ||
| [class.ai-message--user]="msg.role === 'user'" | ||
| [class.ai-message--assistant]="msg.role === 'assistant'" | ||
| > | ||
| @if (msg.role === 'assistant') { | ||
| <span [innerHTML]="msg.content | markdown"></span> | ||
| } @else { | ||
| {{ msg.content }} | ||
| } | ||
| </div> |
Contributor
There was a problem hiding this comment.
Note: This template relies on the markdown pipe which has security concerns.
The markdown pipe used on line 47 bypasses Angular's XSS protection without proper sanitization. Please address the security issues flagged in the markdown.pipe.ts review before shipping this feature. AI-generated content should be treated as untrusted input.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/src/app/ai-assistant/ai-assistant.component.ts` around lines 40 - 51,
The template is inserting rendered HTML via [innerHTML]="msg.content | markdown"
which relies on the markdown pipe to bypass Angular XSS protections; update the
markdown.pipe (used by ai-assistant.component template) to treat AI-generated
content as untrusted: parse markdown to HTML, run the HTML through a robust
sanitizer (e.g., DOMPurify) inside the MarkdownPipe, and only then, if
necessary, use DomSanitizer.bypassSecurityTrustHtml on the already-sanitized
output; ensure the pipe is named MarkdownPipe (or the exported pipe symbol used
in the template) and that ai-assistant.component continues to consume messages$
/ msg.content without changing template bindings.
| export class MarkdownPipe implements PipeTransform { | ||
| constructor(private sanitizer: DomSanitizer) {} | ||
|
|
||
| transform(value: string): SafeHtml { |
Contributor
There was a problem hiding this comment.
Add null/undefined guard for input value.
If value is null or undefined, marked.parse() may throw or produce unexpected output.
🛡️ Proposed fix
transform(value: string): SafeHtml {
+ if (!value) {
+ return '';
+ }
const renderer = new Renderer();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/src/app/pipes/markdown.pipe.ts` at line 9, The transform method should
guard against null/undefined input before calling marked.parse; update
transform(value: string): SafeHtml in the markdown pipe to return a safe empty
string or a default SafeHtml (e.g., sanitizer.bypassSecurityTrustHtml('')) when
value is null/undefined, and only call marked.parse(value) when value is a
non-empty string; ensure the return type SafeHtml is preserved and reference
marked.parse and transform to locate the change.
Comment on lines
+9
to
+17
| transform(value: string): SafeHtml { | ||
| const renderer = new Renderer(); | ||
| renderer.link = ({ href, title, text }: { href: string; title?: string | null; text: string }) => { | ||
| const titleAttr = title ? ` title="${title}"` : ''; | ||
| return `<a href="${href}"${titleAttr} target="_blank" rel="noopener noreferrer">${text}</a>`; | ||
| }; | ||
| const html = marked.parse(value, { renderer }) as string; | ||
| return this.sanitizer.bypassSecurityTrustHtml(html); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How to safely sanitize HTML with DOMPurify in Angular applications?
💡 Result:
To safely sanitize HTML with DOMPurify in Angular applications, follow these steps for XSS protection when rendering user-generated content via [innerHTML]: 1. Install DOMPurify: npm install dompurify @types/dompurify 2. For basic manual sanitization in a component (recommended for control): import { Component } from '@angular/core'; import DOMPurify from 'dompurify'; import { DomSanitizer, SafeHtml } from '@angular/platform-browser'; @Component({ template: <div [innerHTML]="safeHtml"> }) export class MyComponent { rawHtml = Safe content; safeHtml: SafeHtml; constructor(private sanitizer: DomSanitizer) { const clean = DOMPurify.sanitize(this.rawHtml); this.safeHtml = this.sanitizer.bypassSecurityTrustHtml(clean); } } DOMPurify removes malicious attributes/scripts; DomSanitizer then marks it trusted for Angular. 3. For easier integration, use
@taiga-ui/dompurify (latest v5.0.1 as of 2025, Angular 19+ compatible): npm install @taiga-ui/dompurify - As pipe (simplest): import { NgDompurifyModule } from '@taiga-ui/dompurify'; @NgModule({ imports: [NgDompurifyModule] }) export class AppModule {} <div [innerHtml]="rawHtml | dompurify"> - As service: constructor(private sanitizer: NgDompurifySanitizer) {} safeHtml = this.sanitizer.sanitize(SecurityContext.HTML, rawHtml); - Global replacement: providers: [ { provide: Sanitizer, useClass: NgDompurifySanitizer } ] 4. Configure DOMPurify (e.g., restrict tags): providers: [ { provide: DOMPURIFY_CONFIG, useValue: { ALLOWED_TAGS: ['b', 'i', 'p'] } } ] Best practices: - Always sanitize untrusted input at injection point. - Use restrictive whitelists for tags/attributes. - DOMPurify > Angular's basic sanitizer for complex HTML. - Test with XSS payloads; prefer pipes/services over manual for consistency. This prevents XSS while allowing safe HTML rendering.
Citations:
- 1: https://medium.com/@vkrntteotia/safe-html-sanitization-dompurify-2c1cbdd8714f
- 2: https://www.npmjs.com/package/@tinkoff/ng-dompurify/v/2.0.0
- 3: https://registry.npmjs.org/%40taiga-ui%2Fdompurify
- 4: https://github.com/taiga-family/ng-dompurify
- 5: https://github.com/cure53/DOMPurify/blob/main/README.md
🏁 Script executed:
# Check if marked library is installed and what version
cd /tmp && npm list marked 2>/dev/null || cat package.json | grep -A2 '"marked"' || find . -name "package.json" -type f -exec grep -l "marked" {} \;Repository: berta-cms/berta
Length of output: 138
🏁 Script executed:
# Find the markdown.pipe.ts file to verify it exists and matches the snippet
fd -t f "markdown.pipe.ts"Repository: berta-cms/berta
Length of output: 96
🏁 Script executed:
# Check if DOMPurify is already installed or referenced
cd /tmp && npm list dompurify 2>/dev/null || find . -name "package.json" -type f -exec grep -l "dompurify\|DOMPurify" {} \;Repository: berta-cms/berta
Length of output: 76
🏁 Script executed:
# Search for marked library usage and configuration in the codebase
rg "marked\|Renderer" --type ts --type js -A 3 -B 1Repository: berta-cms/berta
Length of output: 41
🏁 Script executed:
# Read the markdown.pipe.ts file
cat editor/src/app/pipes/markdown.pipe.tsRepository: berta-cms/berta
Length of output: 844
🏁 Script executed:
# Find package.json to check dependencies
find . -maxdepth 2 -name "package.json" -type f | head -5Repository: berta-cms/berta
Length of output: 119
🏁 Script executed:
# Look for marked or sanitizer usage in the entire codebase
rg "marked|DomSanitizer|sanitize" --type ts --type js | head -30Repository: berta-cms/berta
Length of output: 3184
🏁 Script executed:
# Check the main package.json for marked version
cat package.json | grep -A 2 -B 2 "marked"Repository: berta-cms/berta
Length of output: 41
🏁 Script executed:
# Check editor package.json
cat editor/package.json | grep -A 2 -B 2 "marked"Repository: berta-cms/berta
Length of output: 209
🏁 Script executed:
# Search for how this markdown pipe is being used in the codebase
rg "markdown\s*['\"]|MarkdownPipe" --type ts --type html -B 2 -A 2Repository: berta-cms/berta
Length of output: 1529
🏁 Script executed:
# Check the marked v17 documentation and behavior regarding HTML escaping
# First, let's see if there's any escape/sanitization happening
rg "escape|sanitize|xss|Security" editor/src/app/pipes/ --type tsRepository: berta-cms/berta
Length of output: 990
🏁 Script executed:
# Check the ai-assistant component to understand context better
cat editor/src/app/ai-assistant/ai-assistant.component.ts | head -100Repository: berta-cms/berta
Length of output: 2964
🏁 Script executed:
# Check what marked v17 does regarding HTML sanitization by default
# Let's look at the marked package documentation or check if there are any config options
cd editor && npm ls marked 2>/dev/null | head -10Repository: berta-cms/berta
Length of output: 109
🏁 Script executed:
# Create a test to understand marked v17 behavior with HTML input
python3 << 'EOF'
import subprocess
import json
# Check if we can inspect the marked module or its documentation
result = subprocess.run(
["npm", "info", "marked@17.0.4", "--json"],
cwd="editor",
capture_output=True,
text=True,
timeout=10
)
if result.returncode == 0:
try:
info = json.loads(result.stdout)
# Look for security-related info
if "description" in info:
print("Description:", info["description"][:500])
if "repository" in info:
print("Repository:", info.get("repository", {}).get("url", "N/A"))
except:
print(result.stdout[:1000])
else:
print("Error:", result.stderr[:500])
EOFRepository: berta-cms/berta
Length of output: 154
🏁 Script executed:
# Verify marked v17 behavior with HTML by checking its actual code or docs
# Let's look for any existing security considerations or issues
python3 << 'EOF'
# Test to understand if marked v17 sanitizes by default
# marked v17 should NOT sanitize HTML by default - this is documented behavior
# The library intentionally does not sanitize to allow users to choose their sanitizer
import subprocess
# Get information about marked's current behavior
result = subprocess.run(
["npm", "view", "marked@17.0.4", "readme"],
cwd="editor",
capture_output=True,
text=True,
timeout=10
)
# Print a snippet to verify
if result.returncode == 0 and result.stdout:
lines = result.stdout.split('\n')
# Look for security or sanitization mentions
for i, line in enumerate(lines):
if any(keyword in line.lower() for keyword in ['sanitize', 'xss', 'security', 'escape', 'html']):
start = max(0, i - 2)
end = min(len(lines), i + 3)
print('\n'.join(lines[start:end]))
print("---")
else:
print("Could not fetch readme")
EOFRepository: berta-cms/berta
Length of output: 1484
🏁 Script executed:
# Let's verify the vulnerability by checking if href/title values could contain XSS payloads
# In marked, are href/title/text values escaped or raw?
python3 << 'EOF'
# Check documentation claim: marked v17 does NOT escape HTML by default
# This is intentional - users must handle sanitization themselves
# Key facts about marked v17:
# 1. Does NOT sanitize HTML by default
# 2. Passes through raw content
# 3. Does NOT escape attribute values
# 4. Users are responsible for sanitization
print("marked v17 key facts:")
print("1. HTML sanitization: DISABLED by default")
print("2. Attribute escaping: NOT APPLIED")
print("3. Responsibility: User must sanitize")
print("\nXSS Attack Vectors in current code:")
print("- href='javascript:void(0)' -> executable")
print("- title='x\" onclick=\"alert()\"' -> attribute breakout")
print("- text with <script> tags -> raw HTML injection")
EOFRepository: berta-cms/berta
Length of output: 375
🏁 Script executed:
# Check if there's a better way to handle the security attributes in marked v17
# Look for any marked configuration options for link attributes
python3 << 'EOF'
# In marked v17, after sanitization with DOMPurify,
# we lose the target="_blank" and rel="noopener noreferrer" if we remove the renderer
# Options:
# 1. Keep custom renderer but escape the values
# 2. Use marked's renderer + DOMPurify config to allow target attribute
# 3. Post-process DOMPurify output to add attributes
# The correct approach would be to ESCAPE the values, not remove the renderer
print("Correct fix approaches:")
print("\n1. ESCAPE attribute values (simpler):")
print(" Use escapeHtml() on href, title, text before interpolation")
print("\n2. SANITIZE full output (proposed diff):")
print(" Use DOMPurify on full marked output")
print(" Then handle target/rel attributes via config or post-processing")
print("\nThe review's proposed diff is incomplete:")
print("- Removes renderer entirely (loses security attributes)")
print("- Doesn't show how to re-add target/rel")
print("- DOMPurify config shown doesn't handle target attribute setup")
EOFRepository: berta-cms/berta
Length of output: 533
Critical: XSS vulnerability – marked does not sanitize HTML by default.
The marked library (v17.0.4) explicitly does not sanitize HTML and documents this as the user's responsibility. The current code has three critical security issues:
-
No HTML sanitization:
marked.parse()passes through raw HTML, allowing<script>tags and event handlers from AI responses. -
Unescaped attribute values (lines 12–13):
href,title, andtextare interpolated directly into the HTML string without escaping, enabling:href="javascript:alert('xss')"titlebreaking out of the attribute:" onclick="malicious()"textcontaining raw HTML tags
-
bypassSecurityTrustHtmlcompletely disables Angular's built-in XSS protection.
Since the pipe renders untrusted AI assistant responses, this is exploitable.
Fix: Use DOMPurify to sanitize the marked output before bypassing security:
import DOMPurify from 'dompurify';
const html = marked.parse(value, { renderer }) as string;
const sanitized = DOMPurify.sanitize(html, {
ADD_ATTR: ['target', 'rel'],
ALLOWED_TAGS: ['p', 'br', 'strong', 'em', 'a', 'ul', 'ol', 'li', 'code', 'pre'],
});
return this.sanitizer.bypassSecurityTrustHtml(sanitized);Install DOMPurify: npm install dompurify @types/dompurify``
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/src/app/pipes/markdown.pipe.ts` around lines 9 - 17, The transform
method in markdown.pipe.ts uses marked.parse with a custom Renderer and then
calls this.sanitizer.bypassSecurityTrustHtml without sanitizing or escaping
attributes, which allows raw HTML, script tags and attribute injection (href,
title, text) — fix by installing and importing DOMPurify, run DOMPurify.sanitize
on the string returned by marked.parse (pass a safe ALLOWED_TAGS/ALLOWED_ATTRS
config and allow target/rel if needed), stop interpolating raw attribute values
by ensuring marked's renderer produces safe attributes (or escape
href/title/text before constructing the <a>), and only call
this.sanitizer.bypassSecurityTrustHtml on the sanitized output; keep references
to transform, Renderer, marked.parse and this.sanitizer.bypassSecurityTrustHtml
when making the change.
…aths Four AI tool config files contained an absolute machine-specific path. They are now gitignored and replaced with .example counterparts using /path/to/berta/_api_app. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
editor/src/app/ai-assistant/ai-assistant.state.ts (4)
1-6: Consider consolidating NGXS imports.
Storecould be imported alongside the other NGXS exports on line 2.📝 Suggested consolidation
-import { State, Action, StateContext, Selector } from '@ngxs/store'; +import { State, Action, StateContext, Selector, Store } from '@ngxs/store'; import { tap, catchError, concatMap } from 'rxjs/operators'; import { EMPTY, from, concat } from 'rxjs'; - -import { Store } from '@ngxs/store';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/src/app/ai-assistant/ai-assistant.state.ts` around lines 1 - 6, The NGXS imports are split; consolidate them by importing Store together with State, Action, StateContext, and Selector from '@ngxs/store' (replace the separate import of Store). Locate the import statement that currently lists State, Action, StateContext, Selector and the separate import of Store and merge them into a single import to keep NGXS symbols (State, Action, StateContext, Selector, Store) grouped and reduce redundant imports.
163-420: Consider decomposing this large action handler.At ~260 lines,
messageReceivedhandles multiple responsibilities:
- State updates (messages, loading, changeHistory)
- Design/settings change dispatch
- Complex section operation handling with undo fallbacks
- Sequential chain orchestration
Extracting helper methods (e.g.,
applyDesignChanges(),applySectionChanges(),buildCreateChain()) would improve readability and testability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/src/app/ai-assistant/ai-assistant.state.ts` around lines 163 - 420, The messageReceived Action handler is too large and does multiple responsibilities; refactor by extracting focused helper methods: move design/settings dispatching into applyDesignAndSettingsChanges(dispatch, action.designChanges, action.settingsChanges), move section handling (update/delete logic and undo fallbacks) into applySectionChanges(dispatch, getState, action.sectionChanges, action.isUndo), and build the observable chains into buildCreateChain(createChanges), buildCloneChain(cloneChanges) and buildReorderChain(reorderChanges, action, getState). Keep message/patchState and changeHistory assembly in messageReceived but delegate all heavy logic to these new functions (referencing messageReceived, applyDesignAndSettingsChanges, applySectionChanges, buildCreateChain, buildCloneChain, buildReorderChain) so behavior remains identical while improving readability and testability.
155-159: Silent error handling may leave users confused.When the chat request fails, the error is logged but the user only sees the loading indicator stop. Consider adding an error message to the chat or showing a toast notification so users know their message wasn't processed.
💡 Example approach
catchError((error) => { console.error('AI assistant error:', error); - patchState({ isLoading: false }); + const errorMessage: AiMessage = { + role: 'assistant', + content: 'Sorry, something went wrong. Please try again.', + }; + patchState({ + isLoading: false, + messages: [...getState().messages, errorMessage], + }); return EMPTY; }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/src/app/ai-assistant/ai-assistant.state.ts` around lines 155 - 159, The catchError in ai-assistant.state.ts currently only logs the error and clears isLoading; update the catchError handler for the chat request to surface the failure to the user by patching state with an error indicator (e.g., set patchState({ isLoading: false, errorMessage: '...' }) or push a system chat entry) and/or dispatching a toast action so the UI shows a clear error; modify the handler around catchError(...) and calls to patchState/errorMessage or whatever toast action the app uses so the chat UI displays a failure message instead of silently stopping the loader.
231-245: Fire-and-forget dispatches may cause race conditions.These
dispatch()calls return Observables that aren't awaited. If multiple design or settings changes affect overlapping state, they may execute in unpredictable order or interleave with other operations.Consider chaining these dispatches sequentially similar to the create/clone/reorder chains, or using
forkJoinif parallel execution is acceptable but completion needs to be awaited.💡 Sequential dispatch pattern
- for (const change of action.designChanges) { - dispatch( - new UpdateSiteTemplateSettingsAction(change.group, { - [change.setting]: change.value, - }), - ); - } - - for (const change of action.settingsChanges) { - dispatch( - new UpdateSiteSettingsAction(change.group, { - [change.setting]: change.value, - }), - ); - } + const designChain = from(action.designChanges).pipe( + concatMap((change) => + dispatch( + new UpdateSiteTemplateSettingsAction(change.group, { + [change.setting]: change.value, + }), + ), + ), + ); + + const settingsChain = from(action.settingsChanges).pipe( + concatMap((change) => + dispatch( + new UpdateSiteSettingsAction(change.group, { + [change.setting]: change.value, + }), + ), + ), + );Then include them in the final concat:
return concat(designChain, settingsChain, createChain, cloneChain, reorderChain);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/src/app/ai-assistant/ai-assistant.state.ts` around lines 231 - 245, The loops that call dispatch(new UpdateSiteTemplateSettingsAction(...)) and dispatch(new UpdateSiteSettingsAction(...)) are fire-and-forget and should be converted into observable chains so dispatches are awaited to avoid race conditions: transform the designChanges and settingsChanges arrays into observable sequences (e.g., from(action.designChanges) / from(action.settingsChanges)) and use concatMap to sequentially call dispatch for each change (returning the dispatch observable), producing designChain and settingsChain observables; then include those chains in the final concat alongside the existing createChain, cloneChain, and reorderChain so the function returns concat(designChain, settingsChain, createChain, cloneChain, reorderChain).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@editor/src/app/ai-assistant/ai-assistant.state.ts`:
- Line 329: The code uses an unsafe "as any" when dispatching
CreateSectionAction with an object missing required SiteSectionStateModel fields
(name set to null and missing order and site_name); fix by providing a properly
typed payload: either update CreateSectionAction to accept
Partial<SiteSectionStateModel> or introduce a new action (e.g.,
CreateAiSectionAction) that models the AI-driven creation, or construct a full
SiteSectionStateModel object before dispatch (populate name as string, add
order:number and site_name:string). Update the call site where
CreateSectionAction is dispatched so the payload matches the chosen type change
and remove the "as any" cast.
---
Nitpick comments:
In `@editor/src/app/ai-assistant/ai-assistant.state.ts`:
- Around line 1-6: The NGXS imports are split; consolidate them by importing
Store together with State, Action, StateContext, and Selector from '@ngxs/store'
(replace the separate import of Store). Locate the import statement that
currently lists State, Action, StateContext, Selector and the separate import of
Store and merge them into a single import to keep NGXS symbols (State, Action,
StateContext, Selector, Store) grouped and reduce redundant imports.
- Around line 163-420: The messageReceived Action handler is too large and does
multiple responsibilities; refactor by extracting focused helper methods: move
design/settings dispatching into applyDesignAndSettingsChanges(dispatch,
action.designChanges, action.settingsChanges), move section handling
(update/delete logic and undo fallbacks) into applySectionChanges(dispatch,
getState, action.sectionChanges, action.isUndo), and build the observable chains
into buildCreateChain(createChanges), buildCloneChain(cloneChanges) and
buildReorderChain(reorderChanges, action, getState). Keep message/patchState and
changeHistory assembly in messageReceived but delegate all heavy logic to these
new functions (referencing messageReceived, applyDesignAndSettingsChanges,
applySectionChanges, buildCreateChain, buildCloneChain, buildReorderChain) so
behavior remains identical while improving readability and testability.
- Around line 155-159: The catchError in ai-assistant.state.ts currently only
logs the error and clears isLoading; update the catchError handler for the chat
request to surface the failure to the user by patching state with an error
indicator (e.g., set patchState({ isLoading: false, errorMessage: '...' }) or
push a system chat entry) and/or dispatching a toast action so the UI shows a
clear error; modify the handler around catchError(...) and calls to
patchState/errorMessage or whatever toast action the app uses so the chat UI
displays a failure message instead of silently stopping the loader.
- Around line 231-245: The loops that call dispatch(new
UpdateSiteTemplateSettingsAction(...)) and dispatch(new
UpdateSiteSettingsAction(...)) are fire-and-forget and should be converted into
observable chains so dispatches are awaited to avoid race conditions: transform
the designChanges and settingsChanges arrays into observable sequences (e.g.,
from(action.designChanges) / from(action.settingsChanges)) and use concatMap to
sequentially call dispatch for each change (returning the dispatch observable),
producing designChain and settingsChain observables; then include those chains
in the final concat alongside the existing createChain, cloneChain, and
reorderChain so the function returns concat(designChain, settingsChain,
createChain, cloneChain, reorderChain).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57ebfa60-5a5e-4297-8cd7-1289dee529e8
⛔ Files ignored due to path filters (1)
editor/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
editor/src/app/ai-assistant/ai-assistant.actions.tseditor/src/app/ai-assistant/ai-assistant.service.tseditor/src/app/ai-assistant/ai-assistant.state.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/src/app/ai-assistant/ai-assistant.actions.ts
| const createChain = from(createChanges).pipe( | ||
| concatMap((change) => { | ||
| const sectionsBefore = this.store.selectSnapshot(SiteSectionsState.getCurrentSiteSections); | ||
| return dispatch(new CreateSectionAction({ name: null, title: change.title ?? change.name } as any)).pipe( |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the CreateSectionAction type definition to understand the expected payload
ast-grep --pattern 'class CreateSectionAction {
$$$
}'
# Also search for the action's constructor signature
rg -n "CreateSectionAction" --type ts -A 5 | head -60Repository: berta-cms/berta
Length of output: 6081
🏁 Script executed:
#!/bin/bash
# Find the SiteSectionStateModel definition
rg -n "SiteSectionStateModel" --type ts -B 2 -A 10 | head -100Repository: berta-cms/berta
Length of output: 8531
🏁 Script executed:
#!/bin/bash
# Find and read the SiteSectionStateModel definition
fd -n "site-sections-state.model" --type f | head -5Repository: berta-cms/berta
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Find the SiteSectionStateModel definition file
fd "site-sections-state.model" --type fRepository: berta-cms/berta
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Read the SiteSectionStateModel definition
cat -n editor/src/app/sites/sections/sections-state/site-sections-state.model.tsRepository: berta-cms/berta
Length of output: 1489
Avoid as any type assertion.
The object passed to CreateSectionAction does not conform to SiteSectionStateModel. It has name: null (should be string) and is missing required properties order (number) and site_name (string). The as any cast bypasses these type violations.
Either:
- Update
CreateSectionActionto accept a partial section shape - Create a dedicated action for AI-driven section creation with a more appropriate type
- Provide all required properties for
SiteSectionStateModel
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/src/app/ai-assistant/ai-assistant.state.ts` at line 329, The code uses
an unsafe "as any" when dispatching CreateSectionAction with an object missing
required SiteSectionStateModel fields (name set to null and missing order and
site_name); fix by providing a properly typed payload: either update
CreateSectionAction to accept Partial<SiteSectionStateModel> or introduce a new
action (e.g., CreateAiSectionAction) that models the AI-driven creation, or
construct a full SiteSectionStateModel object before dispatch (populate name as
string, add order:number and site_name:string). Update the call site where
CreateSectionAction is dispatched so the payload matches the chosen type change
and remove the "as any" cast.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@editor/src/app/ai-assistant/ai-assistant.state.ts`:
- Around line 122-127: The current send flow (where SendAiMessageAction creates
a user message via getState/patchState and sets isLoading) allows overlapping or
stale responses to mutate cleared chats; add a request-scoping token/ID on state
(e.g., aiRequestId) that you set/increment when starting SendAiMessageAction and
clear on ClearAiChatAction/logout, attach that ID to the outgoing request, and
before applying any async response (the handler that currently mutates
messages/history) verify the stored aiRequestId still matches the response token
and that the chat wasn’t cleared; reject or ignore responses with mismatched IDs
and ensure isLoading is updated appropriately so new sends are blocked until the
current request completes or is explicitly cancelled.
- Around line 161-167: The entry_changes mapping drops the description field
causing loss of context; update the mapping in ai-assistant.state.ts (the
entry_changes block that builds AiEntryChangeResponseItem) to include
description: c.description (or c.description as the expected type) so serialized
changeHistory preserves the original description; ensure the type
AiEntryChangeResponseItem accepts description or adjust its type accordingly and
verify messageReceived() and chat() use the same field name.
- Around line 365-371: The mapping only stores the server slug under change.name
causing title-based creates to miss the slug; inside the tap callback where you
find newSection (the dispatch(new CreateSectionAction(...)).pipe(tap(...))
block), store the newSection.name under every identifier this create path can
accept: set sectionNameMap[change.name] = newSection.name and, if change.title
is present (and different), also set sectionNameMap[change.title] =
newSection.name so createEntryChain and later fallbacks resolve the correct
server-assigned slug regardless of whether change.name or change.title was used.
- Around line 505-519: The current patchState block in the handler (guarded by
action.isUndo) updates every unresolved create in the same section, causing
multiple pending creates to receive the same newEntry.id; change the logic in
the getState().changeHistory mapping so it only backfills a single pending
create for this newEntry: iterate history entries from the end and for the first
entry whose entryChanges contains a c where c.operation === 'create' &&
c.section === change.section && !c.entryId, update only that single c.entryId to
String(newEntry.id) (leave the rest untouched) before returning the patched
state; keep using patchState, getState().changeHistory, entryChanges, and
newEntry.id to locate and update the targeted create.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75a4663f-f970-423e-9e54-0e863f4aa661
📒 Files selected for processing (4)
_api_app/tests/Feature/AiChatControllerTest.phpeditor/src/app/ai-assistant/ai-assistant.actions.tseditor/src/app/ai-assistant/ai-assistant.service.tseditor/src/app/ai-assistant/ai-assistant.state.ts
✅ Files skipped from review due to trivial changes (2)
- _api_app/tests/Feature/AiChatControllerTest.php
- editor/src/app/ai-assistant/ai-assistant.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/src/app/ai-assistant/ai-assistant.actions.ts
Comment on lines
+122
to
+127
| const state = getState(); | ||
| const userMessage: AiMessage = { role: 'user', content: action.message }; | ||
| patchState({ | ||
| messages: [...state.messages, userMessage], | ||
| isLoading: true, | ||
| }); |
Contributor
There was a problem hiding this comment.
Prevent overlapping or stale chat requests.
Nothing here blocks a second send or invalidates the current request. If another SendAiMessageAction fires, or ClearAiChatAction runs during logout, a late response will still reach Line 175; then Line 204 can attach the change history to the wrong prompt or repopulate a cleared chat.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/src/app/ai-assistant/ai-assistant.state.ts` around lines 122 - 127,
The current send flow (where SendAiMessageAction creates a user message via
getState/patchState and sets isLoading) allows overlapping or stale responses to
mutate cleared chats; add a request-scoping token/ID on state (e.g.,
aiRequestId) that you set/increment when starting SendAiMessageAction and clear
on ClearAiChatAction/logout, attach that ID to the outgoing request, and before
applying any async response (the handler that currently mutates
messages/history) verify the stored aiRequestId still matches the response token
and that the chat wasn’t cleared; reject or ignore responses with mismatched IDs
and ensure isLoading is updated appropriately so new sends are blocked until the
current request completes or is explicitly cancelled.
Comment on lines
+365
to
+371
| return dispatch(new CreateSectionAction({ name: null, title: change.title ?? change.name } as any)).pipe( | ||
| tap(() => { | ||
| const sectionsAfter = this.store.selectSnapshot(SiteSectionsState.getCurrentSiteSections); | ||
| const newSection = sectionsAfter.find((s) => !sectionsBefore.some((b) => b.name === s.name)); | ||
| if (newSection) { | ||
| // Record mapping so createEntryChain can resolve the correct section name | ||
| sectionNameMap[change.name] = newSection.name; |
Contributor
There was a problem hiding this comment.
Record the slug under every identifier this create path accepts.
Line 365 already supports title-based creates via change.title ?? change.name, but Line 371 only stores the server slug under change.name. If name is absent, Line 485 falls back to the wrong section key and entry creation for the new section can miss the server-assigned slug.
💡 Suggested fix
if (newSection) {
// Record mapping so createEntryChain can resolve the correct section name
- sectionNameMap[change.name] = newSection.name;
+ for (const key of [change.name, change.title].filter((key): key is string => Boolean(key))) {
+ sectionNameMap[key] = newSection.name;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/src/app/ai-assistant/ai-assistant.state.ts` around lines 365 - 371,
The mapping only stores the server slug under change.name causing title-based
creates to miss the slug; inside the tap callback where you find newSection (the
dispatch(new CreateSectionAction(...)).pipe(tap(...)) block), store the
newSection.name under every identifier this create path can accept: set
sectionNameMap[change.name] = newSection.name and, if change.title is present
(and different), also set sectionNameMap[change.title] = newSection.name so
createEntryChain and later fallbacks resolve the correct server-assigned slug
regardless of whether change.name or change.title was used.
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation
Updates
Tests